fix(recall): keep cross-paragraph AND hits and forward synonyms to the chat fallback (L2, #142, #143) - #156
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Target branch corrected This pull request now targets The |
github-actions
Bot
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 995a8edbb9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // file-span hit so split tokens still surface. Do not merge paragraphs | ||
| // and do not run this when a paragraph matched but cwd-only dropped it. | ||
| if (paragraphMatches === 0) { | ||
| const scoped = scopeAdjust(scope, fileCwd, lowerFile, fileRepoKey); |
There was a problem hiding this comment.
When --cwd-only searches an aggregate file such as MEMORY.md, passing the entire lowerFile to scopeAdjust lets any mention of the requested path admit the hit. For example, query terms in two unrelated paragraphs are returned as in-scope if a third paragraph contains applies_to: cwd=<requested path>, even though the existing paragraph path checks scope against only the matching chunk. This defeats the CLI's hard project filter; the fallback should require structured file/repository scope or evaluate the prose-path signal only within the region contributing the match.
Useful? React with 👍 / 👎.
The PRD survived revalidation with nothing invalidated by L1, but two of its context blocks are collages rather than quotes and must not be diffed against, the header SHA is stale, and the gate demanded npm test exit 0 on a host where it has never exited 0. Amendment A also pins the exact hit shape the file-span fallback must emit and names the one-character mistake that would let the fallback fix itself into a no-op: copying chat's opts.synonyms === true instead of memory's opts.synonyms ?? true.
NEAR-PASS. The reviewer traced the matchedThreadIds hole through scopeAdjust and the stage1 skip and confirmed the plan closes it. Folded: Test B is a lock and must not be counted as red-green proof, dist is build output rather than a hand edit, Test C needs the cwd-scope sqlite setup with the handles closed before searchMemory or it fails for the wrong reason on Windows, firstMatchStartLine must be tested off line 1 or dropped, and the file-span scoring magnitude difference is accepted rather than special-cased.
…e chat fallback (#142, #143) A memory file whose AND tokens straddle a blank line satisfied the file-level match and was then thrown away by the per-paragraph match, returning nothing with no warning. It now falls back to one file-span hit, emitted through scopeAdjust with the same MemoryHit shape as a paragraph hit, and only when no paragraph matched. matchedThreadIds recorded the thread as soon as the file-level AND passed, so stage1 skipped the chat rows for a thread whose file hits had all been dropped - the query returned nothing at all. It now records only after a hit is actually kept. The chat fallback passed neither synonyms nor any, and chat defaults synonyms off while memory defaults it on, so the fallback re-zeroed exactly the mixed-language query it was there to rescue.
The ubuntu CI lane measures the suite and inventory.mjs --check --tests fails when the README badge disagrees. Every layer in this stack adds tests, so every layer publishes its own cumulative total; this one is 3081.
dc56907 to
9fa8a5e
Compare
995a8ed to
45b8555
Compare
github-actions
Bot
changed the title
2 participants
A file whose AND tokens straddle a blank line now keeps one file-span hit, emitted through
scopeAdjustwith the same nine-field shape as a paragraph hit and only when no paragraph matched.matchedThreadIdsrecords a thread only after a hit is actually kept — recording it on the file-level AND made stage1 skip that thread's chat rows, so the query returned nothing at all. The chat fallback forwardssynonymsandany; memory defaults synonyms on and chat defaults them off, so omitting them re-zeroed the mixed-language query the fallback existed to rescue.Closes #142
Closes #143
Stack (merge bottom-up)
codex/memory-recall-roadmapcodex/fix-recall-cwd-normalizationcodex/fix-memory-search-semanticscodex/fix-recall-cli-arg-hygienecodex/fix-chat-index-freshnesscodex/fix-recall-intent-regexcodex/fix-memory-write-gateYou are here: L2. Base is
codex/fix-recall-cwd-normalization. Review this PR's diff only — it is already scoped to this layer.Review focus
the ordering of
matchedThreadIds.addrelative toscopeAdjust, and?? truevs=== truein the fallback.Evidence
Every layer was planned to diff level before any code, audited by an independent
xai/grok-4.6reviewer, and implemented only after the audit's blockers were folded. Each new test was observed failing on the parent tip before it was shown passing; the per-layer receipt indevlog/_plan/260911_memory_recall_sweep/records the exact red output.Local gate:
npm run buildexit 0, andnpm testshowing exactly the two pre-existing environmental failures recorded in002_host_verification_baseline.md(hook-benchhardcodescwd: "/tmp", andcxc map --helpneedspy) and no third.Note on the target-branch check
Enforce PR target branchrequiresdev. Layers L1-L6 legitimately target the layer below, so that workflow will flag them. Do not retarget them todev— that would dissolve the stack. Merge bottom-up; each merge retargets the next child.