[Dhirenderchoudhary]

Description

Refs #793. That issue tracks five stages, so it stays open. This only finishes the read stub part of stage 1, which landed in #1359 (compaction landed in #1066). This fixes the gaps left on main where read_file could answer "already in context" for a file the current history never saw.

• ACP: stubs were global to the process, so one editor chat could get a stub for a read made in another chat. Each ACP chat now has its own read scope (acp:<sessionId>), using the same scope mechanism subagents use.
• ACP: /clear, retry, timeline revert, loadSession from disk, and resumeSession cut or replace the history without touching the stubs. They now clear that chat's scope.
• TUI: /resume swapped the history but kept the stubs. applySession now bumps the read generation, the same as compact does.
• TUI: /tune and finishing the config wizard clear the history the same way, so they bump the read generation too.

ACP deleteSession also drops that chat's scope, so deleted chats don't keep stubs in memory. The edit guard (seenFiles) is unchanged.

Every new test failed on main and passes with the fix.

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update

Changeset

• Added a changeset (pnpm changeset) describing this change for the changelog

Docs-only or internal chores need no changeset (or run pnpm changeset --empty to note that intentionally).

Testing

Automated Tests

• New features include passing tests in .spec.ts/tsx files
• All existing tests pass (pnpm test:all completes successfully)
• Tests cover both success and error scenarios

Manual Testing

• Tested with Ollama
• Tested with OpenRouter
• Tested with OpenAI-compatible API
• Tested MCP integration (if applicable)

Checklist

• If this was for an open issue, I was assigned to it
• Code follows project style guidelines
• Self-review completed
• Documentation updated (if needed)
• No breaking changes (or clearly documented)
• Appropriate logging added using structured logging (see CONTRIBUTING.md)

[github-actions]

nc-review: comments — 1 important, 1 nit

@Dhirenderchoudhary — a few things worth a look, none blocking.

Small, well-scoped bug fix: read_file stubs now reset (per ACP session or via read generation bump) when the history is swapped by /clear, retry, timeline revert, load from disk, resume, or TUI /resume. The ACP scope-per-session change mirrors the subagent pattern and the test coverage of the affected code paths is reasonable. The one substantive concern is that the PR is framed as 'Closes #793' even though #793 is the much larger 'Semantic Context Compiler' umbrella issue (5 stages) and this only patches a sub-bug of Stage 1; merging would auto-close the umbrella and silently lose the remaining stages.

🟠 important · completeness · source/acp/acp-agent.ts

The PR description says Closes #793, but issue #793 is the umbrella 'Semantic Context Compiler' tracking five stages (semantic dedup, LSP-aware trimming, tool result compression, budget enforcement, visible meter). This PR only fixes a sub-bug of Stage 1 (read-stub reset on history swap). The other four stages plus the remaining Stage 1 work are unaddressed. Per the rubric, a partial fix that auto-closes a tracking issue silently loses the rest — at minimum, drop the 'Closes #793' wording (or change it to 'Refs #793') and let the umbrella stay open until the remaining stages land. The implementation itself is correct and complete for the sub-bug it targets.

⚪ nit · design · source/acp/acp-agent.ts

In the /clear branch the new call uses session.sessionId:

session.messages = [];
clearAcpReadStubs(session.sessionId);

The sibling sites (loadSession, resumeSession, retryTurn, timeline/revert) use params.sessionId / sessionId from the method parameters. Both resolve to the same id in this code path, so it is not a bug — but picking one identifier style consistently across the five new call sites would make the grep footprint of 'every place that clears ACP read stubs' uniform. The acpReadScope() helper already centralises the format, so a one-line clearAcpReadStubs(session) or clearAcpReadStubs(params.sessionId) everywhere would be a trivial cleanup.

---

_{🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional}

_{Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.}

[github-actions]

nc-review: comments — 2 nits

@Dhirenderchoudhary — a few things worth a look, none blocking.

Scoped, well-tested fix: read_file stubs now reset on every code path that swaps the chat history (TUI /resume via applySession bumping the read generation, ACP /clear / retryTurn / timeline-revert / resumeSession / loadSession clearing the chat's acp:<sessionId> scope). Per-session ACP scoping mirrors the subagent pattern. Five new AVA specs cover each affected path and exercise both the failure path and a complementary negative case (two ACP sessions do not share stubs; same-session repeat read still stubs). The previous review's 'Closes #793' framing concern is fixed: the description now reads 'Refs #793' and the changeset matches.

Addressed since the last review

• ✅ Previous 'important — Closes [Feature] Semantic Context Compiler (Intelligent Context Budget Management) #793' wording concern: PR description and changeset now both say 'Refs [Feature] Semantic Context Compiler (Intelligent Context Budget Management) #793' and the body explicitly states the umbrella stays open for its remaining stages.
• ✅ Previous 'nit — identifier style inconsistency': the five new clearAcpReadStubs call sites now use params.sessionId for protocol methods (clear, loadSession, resumeSession) and the locally-destructured sessionId for extMethod handlers (timeline/revert, retryTurn), which matches each enclosing method's existing pattern.

⚪ nit · completeness · source/acp/acp-agent.ts:599

deleteSession is deliberately left without a clearAcpReadStubs call. The author's justification (a later loadSession would clear them) is sound for correctness, but a long-running editor process that creates and deletes many chats will accumulate acp:<sessionId> entries in readContentScopes until either a future loadSession for the same id or a clearReadTracker() call reclaims them. Tiny in practice (one small map entry per id) and deleteSession is not in the listed regression paths, so this is a 'wants a sentence in the code or PR description' rather than a defect — flagging it so the deliberate omission is not silently lost when the next reader asks why every other site clears.

⚪ nit · tests · source/acp/acp-agent.spec.ts:1488

The new loadSession from disk and resumeSession tests use (agent as any).sessions.delete(sessionId) (and rely on the prior newSession having persisted the session) to force the cold-load branches that exercise the new clearAcpReadStubs(params.sessionId) calls. The assertion is on lastRead().includes('kept = 1') after the reload, which is enough to catch a regression where either branch forgot to clear stubs — but if loadSession / resumeSession ever started returning early without doing the clear-and-reregister dance, this test would still observe a stub return from the previous in-memory scope and the assertion would pass for the wrong reason. A small in-test confirmation that this.sessions.has(sessionId) is false immediately before loadSession and true immediately after would close that gap cheaply. Strictly optional.

---

_{🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional}

_{Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.}

[addyCooks]

Thanks @Dhirenderchoudhary, this fixes the ACP and /resume cases cleanly.
I ran the specs: the 7 new stub tests fail on main and pass here, and types, lint, format and knip pass.

One gap in the same bug class: two TUI paths clear the whole history without resetting the read stubs.

• useModeHandlers.tsx:360 (handleTuneSelect, "Clear conversation when toggling")
• useModeHandlers.tsx:236 (handleConfigWizardComplete, after the new client is created)
  Both call setMessages([]), and client.clearContext() doesn't touch the stubs, so after /tune or the config wizard read_file can still reply "already in context" for a file the empty history never saw. Could you add bumpReadContentGeneration() at those two sites, with a test each, and add /tune and the config wizard to the list in docs/features/context-compression.md?

Tiny, optional: deleteSession isn't cleared, so a deleted ACP chat's scope stays in memory until exit; a one-line comment saying that's accepted would help.

Everything else looks good, and "Refs #793" is right for the umbrella.

[Dhirenderchoudhary]

@addyCooks all things addressed please have a look

[addyCooks]

Good Work @Dhirenderchoudhary,

Approving 🎉