fix(narrative): narrative reads the domain remember writes (#721, slice 4) - #915

Codex Review Summary

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

Claude finished @cdeust's task in 55s —— View job

I'll analyze this and get back to you.

Review 1 of 3 — PR #915

Five reviews were run: code quality, performance, test coverage, documentation accuracy and security. None found a Blocker.

The fix in mcp_server/handlers/narrative.py:82 now reads the same domain remember writes. remember._resolve_domain (remember.py:83) and recall_hierarchical (recall_hierarchical.py:334) both use resolve_explicit_domain too. A blank domain resolves to "" and falls through to get_hot_memories, which is what a call with no domain already did. The domain still reaches the store as a bound parameter (? or %s), so nothing new is injectable. The import from shared is allowed by the layer rules. The CHANGELOG entry, the # source: comment and the test docstring match the code.

I could not run pytest, ruff or the craftsmanship gate in this job because the commands needed approval. CI is the check for those.

Notes, no action before merge

• Every new test passes brief: True, so the resolved domain that the full-narrative path (narrative.py:102-105) uses as its directory= label is untested.
• A non-string domain that is truthy (e.g. 123) raises AttributeError at domain_mapping.py:488. The schema declares the field as a string, so this only fails that one call.
• test_substring_collision_is_not_remapped passes without the fix and adds nothing: the test before it already asserts scifact-comparison.

Verdict: approve

Merci pour la revue. Les trois notes non bloquantes restent en l'état dans cette PR :

• chemin non-brief : le domaine résolu y sert seulement d'étiquette directory= ; un test dédié pourra accompagner la prochaine poussée de code si une autre raison en déclenche une ;
• domain non textuel : le schéma déclare une chaîne, et recall_hierarchical (fix(recall): recall_hierarchical reads the domain remember writes (#721, slice 3) #914) a le même comportement ; durcir la validation d'entrée serait une modification transverse, hors du périmètre de cette tranche ;
• test_substring_collision_is_not_remapped : redondant avec le test précédent, gardé comme énoncé explicite du symptôme de fix: avoid silently remapping explicit domain identities through substring collisions #721, comme dans test_recall_hierarchical_domain_scope.py.

Generated by Claude Code

CI autopilot : unit (et l'agrégat merge) en échec sur 538082e, run 42261842595.

Les 11929 tests passent ; c'est l'étape mutation qui échoue avec 44 mutants survivants de narrative.handler. Le gate juge toute fonction modifiée et aucun test ne fixait ce que lit le handler (min_heat, limit, la priorité directory > domain > hot) ni ce qu'il renvoie (clés du résumé, étiquette du récit). Le correctif est dans f8e077e (fixup! du commit 538082e) :

• tests_py/handlers/test_narrative_handler_contract.py : 9 tests du contrat du handler, ramenant 44 survivants à 4 ;
• les 4 derniers étaient des équivalents par construction (args.get("directory", "") → None, args.get("brief", False) → None, tous faux). Les défauts redondants sont retirés (args.get("directory") or "", bool(args.get("brief"))) plutôt qu'enregistrés comme équivalents.

Reçu local : GITHUB_BASE_REF=main python scripts/ci_mutation_diff.py → 0 surviving mutants, 1 file(s), 0 failed, 0 untested ; 147 tests liés passent, ruff et le gate craftsmanship OK. Non vérifié : la CI de f8e077e, en cours.

Remarque hors périmètre : ci_mutation_diff.run_job passe -- à mutation_check.sh, qui le transmet comme source ; d'où l'avertissement mutmut Following patterns are likely invalid: ['--'] sur chaque run. Sans effet sur le verdict ici.

Generated by Claude Code

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

Review 2 of 3 — PR #915

Review 1 (at 538082e) found no Blockers, so nothing needed fixing. This review covers only f8e077e, the one commit pushed since. Five reviews were run: code quality, performance, test coverage, documentation accuracy and security. None found a Blocker.

In mcp_server/handlers/narrative.py:78, args.get("directory") or "" turns a falsy non-string into "". That takes the same if directory: branch and gives the same directory or domain label as before. In narrative.py:83, bool(args.get("brief")) changes nothing, because if brief: already tested truthiness. directory and domain still reach the store as bound parameters. The new test_narrative_handler_contract.py is 115 lines, and checked by reading, each of its nine assertions matches the handler. Its fixture clears the _build_registry cache before and after each test and swaps in a fake store and fake renderers, so nothing leaks between tests. It also covers the full-narrative label (directory="cortex"), which review 1 noted was untested.

I could not run pytest or ruff in this job because the commands needed approval. When this was posted, unit was still running. This review is posted here because gh pr comment could not be run from this job: writing the comment body to a file needed approval.

Notes, no action before merge

• No test passes directory=None, a blank domain (" ") or brief=None, and the new tests would pass on the old versions of lines 78 and 83.
• The name test_directory_reads_every_memory_of_the_directory says "every", but the store returns only the latest version of each memory, plus the global memories.

Verdict: approve

Merci pour la revue 2. Notes non bloquantes, sans poussée dédiée :

• domaine blanc : couvert par test_blank_domain_is_no_scope dans test_narrative_domain_scope.py ; directory=None / brief=None ne sont pas des comportements nouveaux (mêmes branches qu'avant), le gate de mutation confirme qu'aucun mutant de ces lignes ne survit ;
• le nom test_directory_reads_every_memory_of_the_directory est imprécis : renommage repris à la prochaine poussée de code s'il y en a une.

Generated by Claude Code

Claude finished @cdeust's task in 37s —— View job

Review 3 of 3 — PR #915

• List the PR commits
• Check that 12b9902 has the same content review 2 approved
• Review whatever differs
• Post the verdict

Reviews 1 and 2 found no Blockers, so there was nothing to re-check. Since review 2, the branch was squashed: it is now one commit, 12b9902, and f8e077e is no longer on the branch. That commit was not in this checkout, and fetching it or calling the compare API needed approval. So instead of diffing the two commits, I read the whole diff of 12b9902 against origin/main. It contains what reviews 1 and 2 approved, with nothing new:

• mcp_server/handlers/narrative.py:78-83: args.get("directory") or "", resolve_explicit_domain(args.get("domain") or "") with its # source: comment, and bool(args.get("brief")).
• The CHANGELOG entry for fix: avoid silently remapping explicit domain identities through substring collisions #721 slice 4, unchanged.
• tests_py/handlers/test_narrative_domain_scope.py and tests_py/handlers/test_narrative_handler_contract.py, the nine contract tests review 2 read. I found no added assertions or logic.

The squash brought no new code, so I did not run the five-agent pass again. When this was posted, the Pipeline run on 12b9902 (unit tests and the mutation gate) was still running, so its result is not part of this review. This review is posted by updating this comment, because gh pr comment needed approval in this job.

Accepted: the whole PR, all four files in 12b9902. No file or commit carries an open Blocker.

Notes, no action before merge

• test_directory_reads_every_memory_of_the_directory still says "every"; you planned to rename it in a later push.

Verdict: approve