fix(story): get_project_story reads the domain remember writes (#721, slice 5) - #918

Codex Review Summary

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

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

PR #918 review (1 of 3)

• Read the diff against origin/main
• Ran the five reviews: code quality, docs accuracy, performance, security, test coverage
• Posted the review: fix(story): get_project_story reads the domain remember writes (#721, slice 5) #918 (comment)

Blockers: none. get_project_story now resolves domain with resolve_explicit_domain before it reads the store, which matches how remember writes and the fix #915 made to narrative. Layering, the size limits and the # source: rule are all met.

Notes, no action before merge: one test passes without the fix, no test covers directory taking precedence over domain, and the test file nearly copies the narrative one. Details are in the review comment.

I did not run the tests; the review comes from reading the code.

Verdict: approve

Review 1/3: PR #918 (get_project_story domain scope, #721 slice 5)

I reviewed the diff against origin/main five ways: code quality, documentation accuracy, performance, security and test coverage.

Blockers: none.

• Fix: get_project_story.py:238 makes the same change narrative.py:82 got in fix(narrative): narrative reads the domain remember writes (#721, slice 4) #915. It adds the same import and the same # source: comment, and args.get("domain") or "" handles a None domain. A blank domain still reads hot memories through _fetch_memories, so there is no hidden fallback.
• Read now matches write: remember._resolve_domain (remember.py:83) also uses resolve_explicit_domain. Both store backends match domain = ? exactly, so the raw argument really did miss the lowercased or canonical scope.
• Layers and size limits: the import is handlers → shared, which is allowed. The file is 269 lines (limit 300) and handler() is about 38 (limit 40).
• Security: the domain is passed as a bound parameter. A name is remapped only when it exactly matches a known repo, never on a substring. MCP rejects a non-string domain before the handler runs (tool_registry_advanced.py:253).
• Performance: _build_registry is lru_cached and shared with remember, recall and narrative, so each call adds a strip and one dict lookup.
• Docs: the comment, the test docstring and the CHANGELOG entry are accurate.

Notes, no action before merge

• test_substring_collision_is_not_remapped (test file :74) passes without the fix, and its != check adds nothing over :70. Pinning it as == "scifact-comparison" would make it a real check.
• No test covers directory taking precedence over domain, because _FakeStore has no get_memories_for_directory. The narrative test has the same gap.
• This test file nearly copies test_narrative_domain_scope.py. The fake store, the repo helper and the fixture could move to a shared conftest.

View job run

Verdict: approve

CI autopilot : correction du gate de mutation poussée (652b848).

• Diagnostic : le check unit (run 42280181055, job 128106929303) échoue sur le gate de mutation limité aux fonctions modifiées. La tranche 5 modifie get_project_story.handler, donc tous les mutants de cette fonction entrent dans le périmètre. 10 survivaient, tous sur des lignes que la PR ne touche pas. merge échoue seulement parce que unit échoue.
• Deux mutants révélaient un vrai trou de test : la limite par défaut de 5 chapitres (5 → 6) et la clé de bucket par jour de la période week (_bucket_key(dt, None)). Un nouveau test de contrat appelle le handler sans argument et les vérifie toutes les deux.
• Les 8 autres sont équivalents par construction : directory vaut None ou "", faux dans les deux cas, et toute variante de period est réécrite en "week" par la garde suivante. Ils sont enregistrés dans memory/mutation-equivalents.json avec la ligne exacte avant et après mutation, et leur justification.
• Vérifié localement : GITHUB_BASE_REF=main python scripts/ci_mutation_diff.py donne 0 failed, avec 8 équivalents acceptés et aucun survivant non enregistré. test_mutation_equivalents.py, test_ci_mutation_diff.py et les tests story passent (59 passed), ruff et le gate de craftsmanship sont OK.
• Non vérifié : la suite PostgreSQL et le reste du job unit en CI. J'attends le nouveau run.

Generated by Claude Code

Claude finished @cdeust's task in 3m 2s —— View job

PR #918 review (2 of 3)

• Checked the Blockers from review 1 (there were none)
• Scoped this review to the commit pushed after 150181a: 652b848
• Ran the five reviews: code quality, docs accuracy, performance, security, test coverage
• Posted the review: fix(story): get_project_story reads the domain remember writes (#721, slice 5) #918 (comment)

Blocker: in memory/mutation-equivalents.json:597-611, x_handler__mutmut_39 and _40 are registered as equivalent, but they are not. Each mutant swaps "week" in the guard tuple for "XXweekXX" or "WEEK". That lets period="WEEK" (or "XXweekXX") through the guard instead of rewriting it to "week", so the time window becomes the 2000-01-01 "all" cutoff. A test can kill both mutants: parametrize the invalid-period test over ["decade", "WEEK", "XXweekXX"], then delete the two entries.

Accepted: the new default-arguments test, and the other six registry entries.

I could not run the tests; this review comes from reading the code.

Verdict: block

Review 2/3: PR #918, commit 652b848 (mutation-gate follow-up)

Review 1 found no Blockers. This review covers only 652b848, the commit pushed after 150181a. I ran five reviews: code quality, docs accuracy, performance, security and test coverage.

Blocker: mutants x_handler__mutmut_39 and _40 are registered as equivalent, but a test can kill them (memory/mutation-equivalents.json:597-611)

The rationale says "Every other input takes the same branch as before, so no input distinguishes the variants." That is false. These mutants do more than remove "week" from the guard at get_project_story.py:242: they put "XXweekXX" (39) or "WEEK" (40) in its place. So handler({"period": "WEEK"}) behaves differently:

Mutant 39 does the same with "XXweekXX". The schema enum (:58) does not save the claim, because the handler never checks the enum and the gate's unit tests call the handler directly. As registered, the scoped mutation gate passes on two real survivors that guard a time window. That is a false green in CI.

Fix: parametrize test_invalid_period_falls_back_and_distinguishes_expired_rows (test_project_story_contract.py:160) over ["decade", "WEEK", "XXweekXX"], then delete both entries. With either mutant, the 2001 row survives and the reported period changes.

Fix this →

Accepted

• The new test test_defaults_bucket_the_last_week_by_day_and_keep_five_chapters (:116) is deterministic. It kills the default-max_chapters mutants (6, 4, None) and the default-period mutants ("day", "month").
• Entries 5, 7, 20, 22, 25 and 26 are truly equivalent: if directory: cannot tell None from "", and any default outside the tuple is rewritten to "week".

Notes, no action before merge

• The rationales for entries 20, 22, 25 and 26 say "on the next statement", but the guard is two statements later, after max_chapters (:240). The equivalence still holds.

I could not run the tests in this session; the findings come from reading the code. The Pipeline run was still in progress when I posted this.

Verdict: block

Claude finished @cdeust's task in 2m 11s —— View job

PR #918 review (2 of 3)

• Check the Blockers from review 1 (there were none)
• Limit this review to the commits pushed after 150181a: 652b848 and 997a395
• Run the five reviews (code quality, docs, performance, security, test coverage)
• Post the review: fix(story): get_project_story reads the domain remember writes (#721, slice 5) #918 (comment)

Blockers: none.

• Codex P2 is fixed in 997a395. It removed the period-guard exemptions _39 and _40, and the invalid-period test now also covers "WEEK" and "XXweekXX", which kills both mutants.
• The new defaults test kills the two mutants it targets: the default chapter limit and the day bucket key.
• The 6 remaining exemptions are genuinely equivalent: the directory and period defaults.

Notes, no action before merge

• One registry rationale says the guard is "on the next statement". It is actually line 242, but the argument still holds.
• I did not run pytest, ruff or mutmut, because shell commands needed approval in this run. The CI mutation gate will confirm the mutant numbering.

Verdict: approve

Review 2/3: PR #918, the commits after 150181a (652b848, 997a395)

Review 1 found no Blockers, so there was nothing to re-check. I reviewed only the two new commits, which touch tests_py/handlers/test_project_story_contract.py and memory/mutation-equivalents.json. I checked them for code quality, documentation accuracy, performance, security and test coverage.

Blockers: none.

• Codex P2 (the period-guard exemptions) is fixed in 997a395. It removed x_handler__mutmut_39 and _40 from the registry and extended test_invalid_period_falls_back_and_distinguishes_expired_rows with "WEEK" and "XXweekXX". Under either mutant, the input passes the guard and _period_cutoff falls through to the 2000-01-01 cutoff (get_project_story.py:102-111). The 2001 row is then kept, so the expected dict no longer matches and the test kills both mutants.
• The new defaults test kills both mutants it targets (test_project_story_contract.py:117). The test spreads six rows over six days inside the week cutoff. A default of 6 for max_chapters returns 6 chapters instead of 5. _bucket_key(dt, None) falls through to "%Y%m" and gives a single bucket.
• The 6 remaining exemptions are genuinely equivalent.
  • _5 and _7 (the directory default): directory is only read through if directory: at :181, where None and "" behave the same.
  • _20, _22, _25 and _26 (the period default): period is not read before the guard at :242-243, which resets a missing default to "week".
  • Each entry's removed/added pair matches the source. classify still fails closed if a mutant number points at a different mutation.
• Security and performance: the gate is not weakened, and the added test cases use in-memory mocks only.

Notes, no action before merge

• The rationale for _20/_22/_25/_26 (mutation-equivalents.json) says the guard is "on the next statement". It is on line 242, after max_chapters. The argument still holds because period is not read in between.
• I did not run pytest, ruff or mutmut, because shell commands needed approval in this run. The mutant numbering will be confirmed by the CI mutation gate (Pipeline run 42283859148, still running).

Verdict: approve