fix(coverage): assess_coverage scores the domain remember writes (#721, slice 7) by cdeust · Pull Request #928 · cdeust/Cortex

cdeust · 2026-11-07T04:28:50Z

Summary

assess_coverage handed an explicit domain unresolved to get_memories_for_domain, while remember writes under resolve_explicit_domain(domain). assess_coverage(domain="cortex-cowork") therefore scored a scope no write used (remember stores it under cortex), and domain="SciFact" missed scifact. The handler now resolves the domain with resolve_explicit_domain, as remember, recall, recall_hierarchical, narrative, get_project_story and drill_down already do. A blank domain counts as no domain (global read); the returned domain is the scope actually read.

Refs #721 (slice 7; does not close the issue).

Type of change

Bug fix (non-breaking change which fixes an issue)

Test plan

New tests_py/handlers/test_assess_coverage_domain_scope.py (7 tests, same pattern as slices 4–6): registry rebuilt from real repo directories under tmp_path; asserts the scope the public handler hands to the store — family member → canonical, explicit name kept lowercased, no substring remap, read scope == remember._resolve_domain write scope, blank → global read. 6 of the 7 fail without the fix (checked by stashing the handler change).
pytest tests_py/handlers: 1909 passed, 81 skipped.
ruff check / ruff format --check on the changed files: clean. (ruff check on the whole tree reports 8 pre-existing E501 in scripts/ and tests_py/scripts/ files this PR does not touch; identical on main.)
python scripts/check_craftsmanship.py: OK.
Not verified locally: PostgreSQL suite, full suite, pyright, scoped mutation gate, benchmarks (torch cannot be downloaded in this session; the env was synced without it). CI is the reference.

Audit notes

Root cause, not symptom: an explicit domain must have the same identity on every read path as on the write path; this aligns the coverage scorer with the others.
Outstanding (listed on fix: avoid silently remapping explicit domain identities through substring collisions #721): directory > domain priority in remember (owner decision); validate_memory, detect_gaps, query_methodology still pass a raw domain; alias/migration documentation.

Coding-standards compliance

§2.2 Layer direction preserved (handler → shared).
§4.1 / §4.2 file and function sizes unchanged in order of magnitude.
§9 No dead code.

Breaking changes

None for callers that pass a canonical domain. A caller passing a family member or a mixed-case name now gets the coverage of the scope remember wrote for the same argument, instead of an empty score.

Reviewer checklist

CHANGELOG.md updated.
No secrets / credentials / PII in the diff.
CI passes on the latest commit (pending).

https://claude.ai/code/session_018kyZYs8eJC3rQpPDcS6G6L

Generated by Claude Code

… slice 7) assess_coverage passed an explicit `domain` unresolved to get_memories_for_domain, so assess_coverage(domain="cortex-cowork") scored a scope no write used (remember stores it under `cortex`) and domain="SciFact" missed `scifact`. Resolve it with resolve_explicit_domain, like remember, recall, recall_hierarchical, narrative, get_project_story and drill_down; a blank domain counts as no domain. Refs #721 Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018kyZYs8eJC3rQpPDcS6G6L

chatgpt-codex-connector · 2026-11-07T04:29:00Z

Codex Review Summary: ✅ Completed

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

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-07T04:31:59.099164Z	`0b8f0a9`	New commits
🔒 Security Review	✅ Completed 2026-11-07T04:33:30.292116Z	`0b8f0a9`	New commits

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

claude · 2026-11-07T04:29:22Z

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

PR #928 review (1 of 3)

Read the diff, CLAUDE.md and the review contract
Ran five reviews: code quality, performance, test coverage, documentation, security
Posted the review summary: fix(coverage): assess_coverage scores the domain remember writes (#721, slice 7) #928 (comment)

No Blockers. There are three Notes, none needing action before merge: one CHANGELOG sentence claims more than the code does, one test passes with or without the fix, and the tests never check the returned domain field.

Tests, ruff and the craftsmanship gate were not run here because the commands were not approved. CI is the reference.

Verdict: approve

claude · 2026-11-07T04:30:46Z

Review 1 of 3 — PR #928

All five reviews ran: code quality, performance, test coverage, documentation and security. None found a Blocker.

assess_coverage.py:307 now resolves the domain the same way remember.py:83 does when it writes. The other handlers that already did this (recall, recall_hierarchical, narrative, get_project_story, drill_down) were checked too.
The new import goes from handlers to shared, which the layer rules allow. The # source: comment is there and accurate.
The domain is still passed to the store as a bound parameter in both backends. The lookup only matches exact names, so the scope a call can read stays the same or gets narrower.
assess_coverage.py is 333 lines, over the 300-line limit, but .craftsmanship-baseline.json already lists that file.

Notes, no action before merge

CHANGELOG.md:179-180 says "the returned domain is the scope read". That only holds when no directory other than the cwd is given. If one is, the handler reads by directory but still returns domain (assess_coverage.py:252-253, :332). This behaviour is older than this PR.
test_substring_collision_is_not_remapped (test_assess_coverage_domain_scope.py:77-78) passes with or without the fix. Line 74 already covers the same case with a stronger ==.
The tests never check the returned domain field: _FakeStore returns [], so the handler always takes the early return.

I could not run tests, ruff or the craftsmanship gate in this session (the commands were not approved). CI is the reference.

Verdict: approve

…es (#721, slice 7) fixup! fix(coverage): assess_coverage scores the domain remember writes (#721, slice 7) The returned domain is the resolved name, not always the scope read: a directory other than the cwd is read by directory (review note on #928). Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018kyZYs8eJC3rQpPDcS6G6L

cdeust · 2026-11-07T04:31:48Z

Suite aux notes de la revue 1 :

CHANGELOG : corrigé dans 0b8f0a9 (fixup!). Le domain renvoyé est le nom résolu ; quand un directory autre que le cwd est donné, la lecture se fait par répertoire, comportement antérieur à cette PR et inchangé ici.
test_substring_collision_is_not_remapped : conservé. Il est redondant avec le test == de la ligne 74, mais il nomme le cas de l'issue (fix: avoid silently remapping explicit domain identities through substring collisions #721) et reprend le patron des tranches 4 à 6.
Champ domain renvoyé non testé : laissé tel quel. Le scope passé au store est la propriété que l'issue exige ; le champ renvoyé n'est qu'un écho de l'argument résolu.

Generated by Claude Code

claude · 2026-11-07T04:32:04Z

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

PR #928 review (2 of 3)

Checked Review 1's Blockers: there were none
Reviewed only the commit pushed after fd16b57: 0b8f0a9, a one-line CHANGELOG.md change
Ran five reviews: code quality, performance, test coverage, documentation, security
Posted the review summary: fix(coverage): assess_coverage scores the domain remember writes (#721, slice 7) #928 (comment)

No Blockers. The new CHANGELOG wording ("the returned domain is the resolved name") is accurate and fixes Review 1's Note. There are two Notes, neither needing action before merge: the empty-result return has no domain key, and the tests never check the returned domain.

Tests, ruff and the craftsmanship gate were not run. This commit changes only docs, so they would give the same result. CI is the reference.

Verdict: approve

claude · 2026-11-07T04:33:13Z

Review 2 of 3 — PR #928

Review 1 (on fd16b57) found no Blockers, so there was nothing to check off. This review covers only the commit pushed after it, 0b8f0a9, which changes one line in CHANGELOG.md. All five reviews ran: code quality, performance, test coverage, documentation and security. None found a Blocker.

The new wording, "the returned domain is the resolved name", is accurate. assess_coverage.py:307 resolves the domain and :332 returns that same value. This fixes the Review 1 Note: when a directory other than the cwd is given, the handler reads by directory (:252-253), so the old wording "the scope read" was wrong in that case.
The commit changes no code, so nothing changes for performance or security.

Notes, no action before merge

The early return for an empty result (assess_coverage.py:314-318) has no domain key, so the CHANGELOG sentence only holds when memories are found. That code is unchanged by this commit.
The tests still never assert result["domain"]. If a regression returned the raw input instead, the suite would still pass. This was already noted in Review 1.

I did not run tests, ruff or the craftsmanship gate; a docs-only commit does not change them. CI is the reference.

View job run

Verdict: approve

cdeust mentioned this pull request Nov 7, 2026

fix: avoid silently remapping explicit domain identities through substring collisions #721

Open

4 tasks

github-actions Bot merged commit fb6cc93 into main Nov 7, 2026
18 checks passed

cdeust mentioned this pull request Nov 7, 2026

fix(validate): validate_memory reads the domain remember writes (#721, slice 8) #929

Merged

4 tasks done

Navigation Menu

Repository navigation

fix(coverage): assess_coverage scores the domain remember writes (#721, slice 7) - #928

fix(coverage): assess_coverage scores the domain remember writes (#721, slice 7)#928
github-actions[bot] merged 2 commits into
mainfrom
fix/721-assess-coverage-domain

cdeust commented Nov 7, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot commented Nov 7, 2026 •

edited

Loading

Uh oh!

claude Bot commented Nov 7, 2026 •

edited

Loading

Uh oh!

claude Bot commented Nov 7, 2026

Uh oh!

cdeust commented Nov 7, 2026

Uh oh!

claude Bot commented Nov 7, 2026 •

edited

Loading

Uh oh!

claude Bot commented Nov 7, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

cdeust commented Nov 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Type of change

Test plan

Audit notes

Coding-standards compliance

Breaking changes

Reviewer checklist

Uh oh!

chatgpt-codex-connector Bot commented Nov 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude Bot commented Nov 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR #928 review (1 of 3)

Uh oh!

claude Bot commented Nov 7, 2026

Review 1 of 3 — PR #928

Uh oh!

cdeust commented Nov 7, 2026

Uh oh!

claude Bot commented Nov 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR #928 review (2 of 3)

Uh oh!

claude Bot commented Nov 7, 2026

Review 2 of 3 — PR #928

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cdeust commented Nov 7, 2026 •

edited

Loading

chatgpt-codex-connector Bot commented Nov 7, 2026 •

edited

Loading

claude Bot commented Nov 7, 2026 •

edited

Loading

claude Bot commented Nov 7, 2026 •

edited

Loading