fix(memory,ccr): close review-pass gaps from PRs #500 + #501

[chopratejas]

Summary

Follow-up to PRs #500 (CCR workspace scoping) and #501 (memory READ-ONLY framing + fail-closed router). The post-merge review surfaced 5 gaps; this PR closes all of them.

#	Gap	Severity	Files
1	5 tool-call methods (save/search/update/delete/list) still ran on the sentinel scope	🔴 real save-side leak surface	memory_handler.py
2	_resolve_ccr_workspace hardcoded project_root_override=None → CCR ≠ memory when CLI override set	🟠 docstring claim was false	anthropic.py
3	ProjectResolver docstring still said "today: GLOBAL"	doc rot	storage_router.py
4	unresolved_project_fallback: str lets typos through to runtime	type safety	storage_router.py
5	track_compression with empty workspace_key recorded un-matchable entries	hygiene	context_tracker.py

Fix 1 — Fail-close all 5 tool-call methods

PR #501 only gated the inject path (search_and_format_context). The 5 tool-call methods still wrote into / read from global_db_path via the sentinel scope. So memory_save in an unresolved-project session would land in the global pool — unreadable from the same-mode session (search short-circuits), but visible the moment anyone opted into unresolved_project_fallback=\"global\" or ran --memory-storage=global.

Two new private helpers on MemoryHandler:

@staticmethod
def _scope_is_unresolved_project(scope) -> bool: ...

@staticmethod
def _unresolved_project_skip_response(op, user, scope) -> str:
    # Returns {\"status\": \"skipped\", \"reason\": \"...\"} JSON.
    # Tool calls must return SOMETHING — silent no-op would mislead
    # the model. Inject path keeps returning None (no tool response).

Each tool method gates right after _resolve_for_request. The inject path's gate from PR #501 is refactored to use the same helper for DRY.

Fix 2 — Thread project_root_override through CCR

Mirrors memory's RequestContext construction. CCR helper now accepts an optional override and the Anthropic call site passes self.memory_handler.config.project_root_override or None. Safety impact zero (both paths were already project-scoped), but the docstring claim "CCR and memory always agree" is now actually true.

Fixes 3-5 — Polish

• Stale docstring updated to reference BackendRouterConfig.unresolved_project_fallback and the \"empty\" | \"global\" choice.
• unresolved_project_fallback: Literal[\"empty\", \"global\"] — mypy catches typos at compile time. Runtime ValueError kept as defence-in-depth for dynamic / non-typed config loaders. The negative test uses # type: ignore[arg-type].
• track_compression(workspace_key=\"\") now early-returns. The handler already gates this upstream, but defending inside the tracker keeps the invariant local — a future caller bypassing the upstream gate can't poison the LRU with un-matchable entries.

Tests

Suite	New	Coverage
test_memory_handler_project_isolation.py	5	One per tool method (save/search/update/delete/list) — assert status=skipped + (for save) no backend received the leaked content
test_proxy_handler_helpers.py	2	Override drives resolution; explicit header still beats override (tier precedence)
test_ccr_context_tracker.py	2	Empty workspace_key noops; subsequent valid call is not poisoned
test_memory_storage_router.py	+1 type:ignore	Literal + runtime ValueError coexist

make ci-precheck green: cargo fmt + clippy + workspace + 176 python tests + commitlint + mypy + ruff.

Behaviour after this PR

Default unresolved_project_fallback=\"empty\" config:

Operation	Pre-PR #501	After #501 only	After this PR
Memory inject	pooled into GLOBAL	✅ skip	✅ skip
memory_search tool	pooled into GLOBAL	❌ pooled into GLOBAL	✅ status=skipped
memory_save tool	wrote to GLOBAL	❌ wrote to GLOBAL	✅ status=skipped
memory_update tool	mutated GLOBAL	❌ mutated GLOBAL	✅ status=skipped
memory_delete tool	deleted from GLOBAL	❌ deleted from GLOBAL	✅ status=skipped
memory_list tool	listed GLOBAL	❌ listed GLOBAL	✅ status=skipped
CCR tracker w/ override	scoped via CWD	scoped via CWD	✅ scoped via override

No public-API changes. The breaking-ish behavioural shift landed in PR #501; this PR just closes the remaining surface and the polish gaps.

Still open (separate PR)

• compression_store workspace hardening (defense-in-depth) — task #44.
• Multi-user-per-proxy CCR scoping (project-only key today; would leak across users sharing one project on a shared proxy). Different bug class than TAM-550; low risk in solo-dev OSS, real in team SaaS deployments. Filed for design discussion.

Description

This PR prepares fix(memory,ccr): close review-pass gaps from PRs #500 + #501 for review by documenting the intended change, validation evidence, and remaining merge-readiness context.

Linked issues: None declared.

Type of Change

• [x] Bug fix
• [ ] New feature
• [ ] Documentation
• [ ] Refactor
• [ ] Tests only

Changes Made

• Commit: fix(memory,ccr): close review-pass gaps from PRs #500 + #501
• Touches headroom/ccr/context_tracker.py
• Touches headroom/memory/storage_router.py
• Touches headroom/proxy/handlers/anthropic.py
• Touches headroom/proxy/memory_handler.py
• Touches tests/test_ccr_context_tracker.py
• Touches tests/test_memory_handler_project_isolation.py

Testing

• [x] GitHub checks reviewed
• [x] Metadata/template validation
• [ ] Local functional testing

Test Output

gh pr view 502 --repo chopratejas/headroom --json statusCheckRollup
- CI / test (3.10): SUCCESS
- CodeQL / Analyze (actions): SUCCESS
- Init E2E / docker-init-e2e: SUCCESS
- Wrap E2E / docker-wrap-e2e: SUCCESS
- CI / test (3.11): SUCCESS
- CodeQL / Analyze (c-cpp): SUCCESS
- CI / test (3.12): SUCCESS
- CodeQL / Analyze (javascript-typescript): SUCCESS
- CI / test (3.13): SUCCESS
- CodeQL / Analyze (python): SUCCESS
- CodeQL / Analyze (rust): SUCCESS
- CI / test-extras: SUCCESS

Real Behavior Proof

• Environment: GitHub PR metadata and checks for chopratejas/headroom PR #502.
• Exact command / steps: Reviewed PR title, commits, changed files, linked issues, labels, and check rollup; appended this maintainer template completion block without replacing the author's original description.
• Observed result: PR body now contains all required governance sections, checked readiness fields, and a non-placeholder validation evidence block.
• Not tested: This pass updated PR metadata only; code validation remains represented by the linked GitHub checks and any author-provided evidence above.

Review Readiness

• [x] I have performed a self-review
• [x] This PR is ready for human review

[jocel1]

10D Parallel Code Review

Diff: ~454 lines, 7 files

---

Findings

🟡 Warnings (6)

[Security — D9] headroom/proxy/memory_handler.py:~1073 _execute_native_memory_tool has no _scope_is_unresolved_project gate. The five custom methods (save/search/update/delete/list) are correctly gated, but the native Anthropic memory tool path (create/str_replace/insert/delete/rename commands) bypasses the check entirely. A create command via the native tool under an unresolved scope would still write to global_db_path. → Propagate request_context into _execute_native_memory_tool and add the gate at the top, or document explicitly that the native tool path is intentionally out of scope and open a follow-up issue.

[RFC Compliance — D10] headroom/proxy/memory_handler.py:~1072 The PR description states it extends the fail-closed gate to "all 5 tool-call methods" — confirmed. However _execute_native_memory_tool is a 6th write path not covered. The stated goal is not fully met, and test_memory_handler_project_isolation.py has no test for this path either. → Either gate it or document the gap and open a tracking issue.

[Breaking Change — D4] headroom/memory/storage_router.py:134 unresolved_project_fallback: str → Literal["empty", "global"] is a public type contract change. Any consumer building BackendRouterConfig from a dynamic source (dict/YAML parsed as plain str) will get a mypy error at their call site. → Either document in CHANGELOG/UPGRADE notes, or keep the field typed as str and validate in __post_init__ to stay permissive externally.

[Correctness — D1] headroom/proxy/handlers/anthropic.py:1309 getattr(self.memory_handler.config, "project_root_override", "") or None: a genuinely empty string in the attribute is indistinguishable from an absent attribute — both resolve to None. Silently masks misconfiguration. → Use getattr(self.memory_handler.config, "project_root_override", None) directly.

[Test Coverage — D2] tests/test_memory_handler_project_isolation.py The test bodies for update/delete/list appear as ... in the diff. If these are live Python ellipsis literals, the tests pass without asserting anything — only save and search have visible assertions. → Ensure each test contains at minimum assert result["status"] == "skipped" and a side-effect absence check.

[Test Realism — D3] headroom/proxy/handlers/anthropic.py:1309 No integration test for the full project_root_override → _finalize_pre_upstream → _resolve_ccr_workspace → track_compression wiring. The unit test exercises the helper in isolation, not the handler wiring. → Add a test with a real AnthropicHandler (lightweight backend) asserting that the workspace_key in the CCR tracker matches the override.

---

🟢 Informational (4)

[Maintainability — D7] headroom/proxy/memory_handler.py:~648 The op parameter of _unresolved_project_skip_response is a free str. Given the PR already uses Literal for unresolved_project_fallback, typing op: Literal["save", "search", "update", "delete", "list"] would catch typos at compile time consistently.

[Design — D5] headroom/proxy/memory_handler.py:~624 _scope_is_unresolved_project operates exclusively on ResolvedScope but lives on MemoryHandler. Conceptually this predicate belongs on ResolvedScope (or BackendRouter) — an is_unresolved_project property would remove the coupling and make the predicate reusable without importing MemoryHandler.

[KISS — D6] headroom/proxy/memory_handler.py:~762 The search_and_format_context (inject) path keeps its own inline logger.info instead of calling the shared helper. Minor duplication and slight log inconsistency: event=memory_inject_skipped vs event=memory_{op}_skipped. → Unify toward a single log call.

[RFC Compliance — D10] headroom/proxy/memory_handler.py:668 In _unresolved_project_skip_response, scope.display_name if scope else "<none>" — the else branch is dead code: this helper is only ever called after _scope_is_unresolved_project, which already guarantees scope is not None. → Replace with a direct scope.display_name access to make the invariant explicit.

---

Verdict: 🟡 0 critical · 6 warnings · 4 informational

The most notable point is the _execute_native_memory_tool gap (D9 + D10): if the native tool path is a real write surface, the security gate this PR introduces is not complete. The other warnings are non-blocking refinements.

[JerrettDavis]

This is mergeable and CI was green, but I do not think it should merge until the review warning about _execute_native_memory_tool is resolved. The PR closes the five explicit tool methods, but native memory tool commands appear to be another read/write path. Please either gate that path under the same unresolved-project sentinel or document why it is not reachable/in-scope and add a tracking issue. A small regression test for the native tool path would make this much easier to accept.