[Tool Mode] wire runtime-signal sidecar in tool-mode curate + query paths

[RyanNg1403]

Summary

• Adds bumpSidecarOnCurateWrite / bumpSidecarOnQueryRead helpers under src/server/infra/context-tree/ with full unit-test coverage (best-effort, never throws, no-op on missing store).
• Wires them into the four tool-mode touchpoints that previously left runtime-signal sidecar files stale:
  • Daemon case 'curate-html-direct' (MCP brv-curate write path) — bumps updateCount + recency.
  • CLI curate-session continueSession (CLI brv curate write path) — bumps updateCount + recency.
  • SearchExecutor — accepts optional runtimeSignalStore, bumps accessCount per matched path on read.
  • QueryExecutor.executeToolMode — same, wraps the existing tool-mode envelope with a post-bump step.
• Updates the daemon construction site to pass daemonRuntimeSignalStore into both executors.

Why

Tool-mode write/read paths are the only routes signal data flows through on tool-mode projects (the legacy daemon-LLM curate path is unreachable). Without this wiring, every tool-mode topic looks 'freshly created with no access history' regardless of real activity — blocking any signal-driven ranking (prune candidate generation, importance decay, etc).

The store API and schema were already on this branch — this PR only adds the call sites.

What this does NOT do

• No changes to the existing legacy curate path in src/agent/infra/tools/implementations/curate-tool.ts (it already calls these helpers itself).
• No new dream operations, candidate generators, or CLI surface — those land in follow-up PRs.
• No new types/interfaces on the sidecar store; uses existing update / delete / list.

Test plan

• tool-mode-sidecar-updaters.test.ts — 9 new tests covering both helpers (seed/update/no-op/error-swallow paths).
• search-executor.test.ts — +2 tests confirming bump-on-read and graceful behavior when store is absent.
• Existing query-executor.test.ts — 30/30 tests still pass after the executeToolMode wrapper.
• tsc --noEmit clean.
• npm run lint --quiet clean on all changed files.

[github-actions]

Claude finished @RyanNg1403's task in 5m 45s —— View job

---

PR Review: Tool Mode Runtime-Signal Sidecar Wiring

• Fetch base branch and inspect the diff
• Read new helper tool-mode-sidecar-updaters.ts + tests
• Audit four call-sites (curate-html-direct, curate-session, SearchExecutor, QueryExecutor)
• Check daemon wiring in agent-process.ts
• Verify TDD compliance (test changes match implementation)
• Post inline comments for specific findings
• Post summary with score

---

Summary

Overall: 3 / 5 — Right direction and the helper itself is clean, but two functional bugs (Windows path normalization, shared-origin leak) and a TDD gap need to land before merge.

What's good (praise)

• tool-mode-sidecar-updaters.ts is a clean factoring of mirrorCurateAdd + mirrorCurateUpdate from curate-tool.ts:55-104 into reusable, well-documented helpers.
• Helper test coverage is solid — seed / update / undefined-store / throwing-store / empty / partial-failure all exercised in tool-mode-sidecar-updaters.test.ts.
• QueryExecutor.executeToolMode refactor (extract → executeToolModeInternal, wrap with post-bump) keeps the cache + retrieval logic intact and isolates the new behavior to a 6-line tail.
• Best-effort semantics consistently applied: every call site is wrapped so a sidecar failure never breaks the write/query that already succeeded.

Blocking issues

1. 🐛 [issue] curate-session.ts:375 — relative(contextTreeRoot, writeResult.filePath) is not separator-normalized. On Windows, FileKeyStorage.validateKey (file-key-storage.ts:500) throws on \ in a segment, and the outer catch {} swallows it silently → CLI tool-mode signals never persist on Windows. Daemon (agent-process.ts:713) and legacy curate-tool (curate-tool.ts:41) both normalize; CLI must too.
2. 🐛 [issue] search-executor.ts:50-55 — Bumps shared-source paths into the local sidecar. SearchKnowledgeResult.results[].path is relative to the origin tree, not the local one. Filter to local-only (mirror query-executor.ts:554) before passing into bumpSidecarOnQueryRead. Otherwise orphan entries accumulate and collisions can corrupt local rankings.
3. ❓ [issue] query-executor.ts:155-170 — New wrapper has zero test coverage (grep -c "runtimeSignalStore\|bumpSidecarOnQueryRead\|accessCount" query-executor.test.ts → 0). Per CLAUDE.md strict-TDD policy, this needs the same two tests SearchExecutor got.

Worth discussing

4. 🤔 [question] tool-mode-sidecar-updaters.ts:84-87 — bumpSidecarOnQueryRead bumps only accessCount; legacy mirrorHitsToSignalStore (search-knowledge-service.ts:1345) also bumps importance (via recordAccessHits) and recomputes maturity (via determineTier). Intentional divergence (then add a comment) or accidental drop (then mirror it)?

Nits

5. 🧹 tool-mode-sidecar-updaters.ts:28-34 — logger is declared but never wired by any of the four call sites. warnSidecarFailure becomes a no-op → production failures are invisible. Wire daemonLogger / ConsoleLogger at call sites or drop the param.
6. 🧹 curate-session.ts:378-381 — Bare catch {} silently eats store-init errors. At least pass a ConsoleLogger into the helper for breadcrumbs.

Inline comments posted on each finding.
· Branch: feat/ENG-2858-sidecar

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6a9b256684

ℹ️ 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".

[github-actions]

Claude finished @RyanNg1403's task in 2m 18s —— View job

---

Re-Review after aeb5801 — All Prior Findings Addressed

• Diff aeb5801 vs 6a9b256 (the originally-reviewed commit)
• Verify Windows path normalization fix in curate-session.ts
• Verify shared-source filter in search-executor.ts
• Verify TDD gap closed for query-executor.executeToolMode
• Verify access-hit scoring parity (importance + maturity)
• Verify logger wiring / observability change

---

Verdict: 5 / 5 — Ready to merge

The follow-up commit cleanly resolves every blocking item, the worth-discussing question, and both nits from the prior review. Test coverage now matches CLAUDE.md's strict-TDD requirement, and the helper's read-path semantics are at parity with the legacy markdown mirrorHitsToSignalStore.

Verification of each prior finding

1. Windows path normalization — src/oclif/lib/curate-session.ts:381 now normalizes via .replaceAll(sep, '/'), mirroring agent-process.ts:713. CLI tool-mode signals will persist on Windows.

2. Shared-source leak — src/server/infra/executor/search-executor.ts:55-65 filters r.origin === 'local' || !r.origin before the bump (matches query-executor.ts:554). Two new tests cover mixed and all-shared result sets.

   • Sanity check on QueryExecutor: executeToolMode doesn't need the same filter because buildToolModeMatches already strips shared results before they reach matchedDocs — verified at query-executor.ts:554.

3. TDD gap on QueryExecutor.executeToolMode — closed at test/unit/infra/executor/query-executor.test.ts:957-1031 with three tests: bumps accessCount on matched paths, returns envelope unchanged without a store, skips bump when there are no matches.

4. Access-hit scoring parity — bumpSidecarOnQueryRead in tool-mode-sidecar-updaters.ts:101-109 now goes through recordAccessHits + determineTier, mirroring SearchKnowledgeService.mirrorHitsToSignalStore. Two new tests confirm importance rises and maturity is reclassified. The accompanying comment (L73-78) is exactly the kind of intent-anchor that prevents future drift.

5. Logger wiring — write sites now pass loggers:

   • Daemon: agent-process.ts:726-731 wraps agentLog in an ILogger-shaped adapter so swallowed sidecar failures land in the session log.
   • CLI: curate-session.ts:371-388 uses new ConsoleLogger() and turns the previously bare catch {} into a warn with full operator-readable context.
   • Executor read sites intentionally omit the logger; the doc comment at tool-mode-sidecar-updaters.ts:85-88 explains why (avoid stderr spam on bulk reads — the store's mandatory logger still surfaces storage-layer failures).

6. Bare catch {} — replaced with sidecarLogger.warn(...) carrying projectRoot and the underlying error message (curate-session.ts:384-387).

Optional polish (non-blocking)

• 🧹 agent-process.ts:726-731 declares an inline 4-line ILogger adapter around agentLog. If a second daemon site needs the same shim later, consider extracting it into a small helper (e.g. createAgentLogLogger(agentLog) near daemon/utils). Single use today, so leaving it inline is fine.

I couldn't run npm run typecheck / unit tests locally in this environment (permission-restricted), so I'm relying on the PR description's tsc --noEmit / npm run lint --quiet clean assertions and the visible test additions. CI should be the source of truth there.

---

Nice turnaround — the divergence rationale you added at tool-mode-sidecar-updaters.ts:73-78 is especially good. That's the comment that keeps future agents from "simplifying" the helper back to accessCount++ six months from now.

· Branch: feat/ENG-2858-sidecar ·

[github-actions]

Claude finished @RyanNg1403's task in 2m 42s —— View job

---

Re-Review for 6c5f451 — read-side sidecar wraps dropped

• Diff 6c5f451 vs aeb5801 (one commit; -357/+24)
• Verify the double-counting claim against SearchKnowledgeService
• Audit SearchExecutor + QueryExecutor.executeToolMode after wrap removal
• Confirm write-side wiring (curate-html-direct + curate-session) is still intact
• Confirm test deletions are coherent with the implementation change
• Confirm no stale callers reference the dropped helper or removed constructor arg

---

Verdict: 5 / 5 — Good catch, ready to merge

This commit is a correction to my own prior 5/5 verdict on aeb5801. That review explicitly noted the previous fix "mirrored mirrorHitsToSignalStore" and treated parity-with-legacy as a positive — but exact parity is precisely what was wrong: the canonical mechanism is already running on every search, so adding a second, identical pass at the executor layer double-counted importance and pushed topics into higher maturity tiers prematurely. Dropping the wraps fixes that without losing anything.

Verification of the claim

The commit's claim ("SearchKnowledgeService already mirrors via flushAccessHits → mirrorHitsToSignalStore inside acquireIndex") holds at the source level:

• search-knowledge-service.ts:1102-1109 — every search() call goes through acquireIndex with flushAccessHits as the post-rebuild callback.
• search-knowledge-service.ts:1620-1626, 1679, 1750-1752 — accumulateAccessHits() runs inside the result-assembly paths (runTextSearch, trySymbolicSearch exact-match, trySymbolicSearch subtree expansion). The accumulation is unconditional on each search() call regardless of cache hit/miss.
• search-knowledge-service.ts:1338-1357 — mirrorHitsToSignalStore then applies recordAccessHits (bumps accessCount + importance) and determineTier (recomputes maturity). This is exactly the logic the deleted helper had after aeb5801's scoring-parity fix — confirming they were duplicates.
• QueryExecutor.executeToolModeInternal (query-executor.ts:746) calls this.searchService.search(...), so tool-mode queries route through the canonical mechanism with no detour.

What changed cleanly

• Helper file (tool-mode-sidecar-updaters.ts) — bumpSidecarOnQueryRead and its recordAccessHits/determineTier imports deleted. JSDoc at L23-31 now anchors the rationale, naming the canonical mechanism explicitly. That's the kind of intent-anchor that prevents the next agent from "fixing" this back in.
• QueryExecutor — drops the runtimeSignalStore dep entirely; executeToolMode is back to a single return this.executeToolModeInternal(...). No stale IRuntimeSignalStore import left behind.
• SearchExecutor — constructor back to single-arg; sidecar import + field removed; file-level JSDoc at L8-15 explains why (the same anchor pattern, good).
• agent-process.ts:455-461 — no longer passes daemonRuntimeSignalStore into either executor.
• Tests — read-side specs removed coherently: 7 tests dropped from tool-mode-sidecar-updaters.test.ts, 3 from query-executor.test.ts, 3 from search-executor.test.ts. The remaining helper tests cover only bumpSidecarOnCurateWrite, which matches the surviving API.
• Write side untouched — agent-process.ts:723-733 (curate-html-direct) and curate-session.ts:370-389 (CLI continueSession) still call bumpSidecarOnCurateWrite with logger + Windows-normalized path. That path doesn't have a canonical mirror, so it stays.

One subtle behavioral note (not blocking, worth knowing)

The canonical mechanism accumulates hits in memory and flushes via the next acquireIndex cache-refresh callback (TTL expiry or invalidation), not synchronously per-search. The deleted wraps used store.update directly, so they persisted immediately. After this commit, a prune that runs immediately after a query may not yet see the latest counts in the sidecar — they'll be sitting in pendingAccessHits.

This matches main's behavior (it's the original legacy contract), so no regression. Just worth knowing if you ever wire a "prune right after read" path — you'd want to either trigger flushAccessHits explicitly or accept the delivery delay.

Nits / optional polish

• 🧹 agent-process.ts:725-730 — still has the inline 4-line agentLog → ILogger adapter from aeb5801. Single-use, fine to leave; only worth extracting if a second daemon site needs it later. (Same note as the previous review.)
• 🧹 tool-mode-sidecar-updaters.ts:5 — the ILogger import is now only consumed by bumpSidecarOnCurateWrite's logger? param. Still needed, just narrower than before; no action.

I couldn't run npm run typecheck / unit tests in this environment, so I'm relying on the visible call-site cleanup (no stale runtimeSignalStore references in src/test executor files) and the prior CI's confirmation. CI on this push should confirm — the constructor surface narrowed cleanly and all callers in src/ + test/ have been updated.

---

Solid correction. The decision to delete the helper outright rather than gate it behind a flag is the right call — keeping a dead "double-bump" code path around just invites someone to re-enable it.
· Branch: feat/ENG-2858-sidecar