fix(recall): recall_hierarchical reads the domain remember writes (#721, slice 3) - #914

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Claude finished @cdeust's task in 1m 31s —— View job

PR #914 review (1 of 3)

• Read the diff against origin/main and the resolver it calls
• Ran the five reviews: code quality, performance, tests, documentation accuracy, security
• Posted the review: fix(recall): recall_hierarchical reads the domain remember writes (#721, slice 3) #914 (comment)

None of the five reviews found a Blocker. The review comment lists three notes that need no action before merge: one weak test assertion, three paths with no test, and a CHANGELOG wording that is only true when remember's directory doesn't resolve to a known repo.

Not run: I couldn't run pytest or scripts/check_craftsmanship.py here, because running Python needed approval this session didn't have. The review comes from reading the code. The craftsmanship gate passes because the file's existing size violation is already in the baseline.

Verdict: approve

Review 1/3 — PR #914 (recall_hierarchical domain scope, #721 slice 3)

I ran five reviews: code quality, performance, tests, documentation accuracy and security. None found a Blocker. I traced the following by hand:

• _scoped_by_explicit_domain (mcp_server/handlers/recall_hierarchical.py:318-335) copies the arguments and resolves domain with resolve_explicit_domain. Only then does get_memories_for_domain run. This matches recall.py:693-711.
• A blank domain resolves to "". With no memory_ids, that raises the existing ADR-0045 R3 ValidationError, so the call never falls through to an unbounded query.
• The small-set fallback passes the resolved domain to flat_recall, which resolves it again. That is harmless because resolution is idempotent: a canonical name maps to itself.
• Layering is respected (handlers → shared). The file is over the 300-line cap, but it is already listed in .craftsmanship-baseline.json:3666, so the gate does not flag it.
• Security: the lookup is an exact registry match, so it cannot widen read scope beyond what recall already allows. The domain reaches SQL as a bound parameter.
• Not run: I could not execute pytest or the craftsmanship script in this session, so all of the above comes from reading the code.

Notes, no action before merge

• test_substring_collision_is_not_remapped (tests_py/handlers/test_recall_hierarchical_domain_scope.py:72) only checks != "harness-comparison", so it would also pass on "" or on the raw input. An == check would make it a real test.
• These paths have no test: the flat-recall fallback (recall_hierarchical.py:278) receiving the resolved domain, a blank domain sent with memory_ids (the call should go down the ids path and not raise), and a missing or None domain left as given.
• In CHANGELOG.md:91, "remember stores it under cortex" is only true when directory doesn't resolve to a known repo, because remember.py:78-83 gives the cwd priority. That precedence is slice 1 of the issue.

Verdict: approve

Réponse à la revue 1/3 (verdict : approve, aucun bloquant). Conformément à ADR-1119, qui réserve les pushs aux bloquants, je ne pousse rien pour ces trois notes. Elles sont exactes et partiront avec le prochain push de code sur cette branche, ou avec la tranche suivante de #721 :

• test_substring_collision_is_not_remapped passera à une égalité : == "scifact-comparison".
• Tests à ajouter : la bascule vers flat_recall reçoit le domaine résolu ; un domaine blanc accompagné de memory_ids passe par le chemin des ids sans lever d'erreur ; un domaine absent ou None est laissé tel quel.
• L'entrée CHANGELOG sera précisée : « stocké sous cortex » n'est vrai que si directory ne désigne pas un dépôt connu, puisque directory est prioritaire (tranche 1 de fix: avoid silently remapping explicit domain identities through substring collisions #721, décision du propriétaire).

Generated by Claude Code