fix(remember): keep an explicit domain instead of remapping it by substring (#721, slice 1) by cdeust · Pull Request #877 · cdeust/Cortex

cdeust · 2026-11-05T04:30:37Z

Summary

remember(domain=...) passed the caller's domain to resolve_domain, which treats its input as a hint: when no exact name matches, it returns the canonical of the longest known repository fragment found as a substring of the input. With a harness-comparison repository in the dev root, domain="scifact-comparison" was persisted as harness-comparison through the shared fragment comparison — the collision reported in the SciFact campaign (mcp_server/shared/domain_mapping.py, step 4 of resolve_domain).

Root cause: no distinction between an explicit domain identity and an inferred hint. This PR adds resolve_explicit_domain (mcp_server/shared/domain_mapping.py): only an exact known repository name (remote or directory name, or its ADR-0647 family) maps to its canonical; any other name is kept as given, stripped and lowercased. remember uses it for the caller's domain; inferred hints (profile detection, slugs, paths) keep using resolve_domain.

Refs #721 (slice 1 — the remaining acceptance items are listed below, so this PR does not close the issue).

Type of change

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

Test plan

New tests_py/shared/test_explicit_domain_identity.py (6 tests), built from real repository directories under tmp_path (controlled registry):
- the hint path still reproduces the collision (resolve_domain("scifact-comparison") == "harness-comparison");
- resolve_explicit_domain("scifact-comparison") stays scifact-comparison;
- adding an unrelated repository to the registry does not change an explicit domain;
- an exact repository name still maps to its family (cortex-cowork → cortex);
- through the public remember handler (SQLite), the persisted row's domain is scifact-comparison. Without the remember.py change this test fails with 'harness-comparison' == 'scifact-comparison' (checked locally).
pytest tests_py/shared tests_py/handlers tests_py/invariants: 2580 passed, 105 skipped (PostgreSQL not reachable here → PG-gated tests skipped).
python scripts/check_craftsmanship.py: OK. ruff check / ruff format --check (0.17.3, from uv.lock) clean on the three touched files.

Audit notes

Behaviour change, deliberate: an explicit domain that is only a fragment of a repository name (e.g. domain="architect" with an ai-architect repo) is no longer remapped; it is kept as architect. Exact repository names and their ADR-0647 families still map.
Not verified: the full suite on PostgreSQL; the environment has no PostgreSQL and torch could not be installed (proxy), so embedding-dependent tests ran on whatever the suite does without it.

Remaining slices for #721

remember resolves directory before domain, so a caller-supplied domain does not override a known repository cwd although the schema documents it as an "override" — needs an owner decision on precedence.
recall/fetch pass domain to the store unresolved, so remember(domain="cortex-cowork") (stored as cortex) and recall(domain="cortex-cowork") do not agree on canonical scope.
Document the alias/migration behaviour (which names map to a family, and how rows already written under a collided domain are moved).

Coding-standards compliance

Layer direction preserved (shared → shared; handler imports shared).
No new file or function over the caps; no new constants.

Generated by Claude Code

…string (#721, slice 1) remember(domain=...) went through resolve_domain, which treats its input as a hint and returns the canonical of the longest known repository fragment found inside it: with a harness-comparison repository in the dev root, domain="scifact-comparison" was persisted as harness-comparison through the shared fragment "comparison". resolve_explicit_domain maps only an exact known repository name (or its ADR-0647 family) to its canonical and keeps any other name as given, stripped and lowercased. remember uses it for the caller's domain; inferred hints (profile detection, slugs, paths) still use resolve_domain. Refs #721 Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018kyZYumzRRnGTwTkCk6tdj

chatgpt-codex-connector · 2026-11-05T21:04:58Z

Codex Review Summary

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

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-06T02:13:09.606745Z	`d31dd3c`	New commits
🔒 Security Review	✅ Completed 2026-11-06T02:16:42.777878Z	`d31dd3c`	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-05T21:05:21Z

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

PR #877 review: done

Read the diff and the repo rules
Ran five agent reviews: code quality, security, test coverage, docs accuracy, performance
Checked the craftsmanship baseline and CI status
Posted the review: fix(remember): keep an explicit domain instead of remapping it by substring (#721, slice 1) #877 (comment)

The review found no Blockers and posts 3 Notes. Its verdict is approve.

I couldn't run pytest, ruff or the craftsmanship gate in this job because the Bash commands weren't permitted. The gate result comes from reading .craftsmanship-baseline.json, and the test result from the Pipeline run on this commit, which passed.

claude · 2026-11-05T21:08:18Z

Review of PR #877 (review 1 of 3) — commit `5c6cc80`

I found no Blockers. I read the diff myself and five agents reviewed it (code quality, security, test coverage, docs accuracy, performance). I traced these points:

The resolver is correct. resolve_explicit_domain (mcp_server/shared/domain_mapping.py:476-491) does the same exact-name and family lookup as step 3 of resolve_domain. Any other name is returned stripped and lowercased.
No new security exposure. The old fallback already returned arbitrary lowercased input, so callers can't store anything they couldn't before. Domain filters use bound parameters. Wiki and profile paths sanitise the domain.
The new handler test is sound on both backends. It writes to the isolated per-test store (tests_py/conftest.py). %s and the dict rows work on SQLite through sqlite_compat.PsycopgCompatConnection and on PostgreSQL. The _build_registry cache and the store singletons are reset between tests.
The craftsmanship gate should pass. domain_mapping.py was already over the 300-line cap, and .craftsmanship-baseline.json exempts it by file and violation type, not by line count. The new function is 16 lines. The Pipeline run on this head succeeded.
No performance regression. The new path is one dict lookup on the cached registry, where the old one scanned every fragment.

I couldn't run pytest, ruff or the gate myself because the commands were not permitted in this job, so the last two lines above rely on the baseline file and CI.

Notes, no action before merge

remember.py:78-83: a directory that resolves to a known repository still overrides an explicit domain, so remember(domain="scifact-comparison", directory=<inside harness-comparison>) is still stored under harness-comparison. This is your remaining slice 1, and no test sends both arguments yet.
test_explicit_domain_identity.py:47-49: this test asserts the current substring collision in resolve_domain, so it will fail on purpose if hint resolution is fixed later. It would help to name it as documenting current behaviour.
remember.py:82: domain=" " counts as true, so it returns "" and skips the profile-detection fallback. This behaviour predates this PR.

Verdict: approve

…e (ADR-1122) (#898) * ci: run only the mutants of the changed functions in the mutation gate (ADR-1122) mutation_check.sh ran `mutmut run` with no name, so mutmut ran every mutant of each changed file and the gate judged the changed functions afterwards. #865 (backend_resolution.py, 262 mutants) was cancelled at the six-hour job limit after 141; #877 ran past two hours. The script now passes one `<prefix>*` name per changed function (scripts/ci_mutation_scope.py --globs). mutmut 3.8.1 asserts when no mutant matches; after a failed run the gate passes only when mutmut's stats file exists and its .meta files hold no mutant in scope, any other failure fails the job. Without MUTATION_ONLY_PREFIXES, every mutant runs as before. Measured on #877's diff (local macOS arm64, PostgreSQL 18.6): 36 mutants instead of 1,164, verdict in 21 min 15 s. No mutant hit mutmut's CPU timeout, so no wall-clock cap is added. * test(ci): keep the mutation scope test helper within four parameters * ci: fail loudly when the glob helper fails, and require each target's meta for an empty scope Review 1 of #898 notes: - A failure of `ci_mutation_scope.py --globs` inside `< <(...)` did not stop the script: NAMES stayed empty and every mutant ran. The names are now read from an assignment, so `set -e` stops the gate. - `--empty-scope` passed when the mutated file's `.meta` was missing. It now takes the mutated files and concludes only when each `.meta` exists; an empty file list is a usage error. - The shell tests used rc=1 for the fake failures; they now use 3, so the script must forward mutmut's own exit code.

…in-identity

claude · 2026-11-06T00:28:48Z

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

PR #877 review 2 of 3: done

Checked the earlier Blockers. Review 1 had none.
Reviewed the only commit since 5c6cc80: e9b0077, a merge of origin/main.
Confirmed the merge is clean. The PR's diff against main is still the same three files at +120/−2, and nothing from main touches domain resolution.
Posted the review: fix(remember): keep an explicit domain instead of remapping it by substring (#721, slice 1) #877 (comment)

The review found no Blockers and adds 1 Note. Its verdict is approve.

Not done:

I didn't run the five agent reviews again. The new commit adds no code from this PR, and the review contract limits review 2 to commits pushed after review 1.
I didn't run pytest, ruff or the craftsmanship gate locally. The unit CI job was still queued when I posted the review.
The review-pr skill failed to load, so I did the review by hand.

claude · 2026-11-06T00:29:36Z

Review of PR #877 (review 2 of 3): commit `e9b0077`

I found no Blockers.

Earlier Blockers: review 1 (commit 5c6cc80) had none, so there was nothing to check.

What changed since review 1: one commit, e9b0077, which merges origin/main (f9f8074) into the branch. I checked it:

git diff 5c6cc80 e9b0077 shows no change to the PR's three files (remember.py, domain_mapping.py, test_explicit_domain_identity.py).
git diff f9f8074 e9b0077 contains only those three files, at +120/−2, the same as the original PR diff. The merge had no conflict edits and adds no code beyond main.
What main brought in near this code changes only text: the agent_topic and include_low_signal descriptions in remember_schema.py and recall.py. Nothing in it touches domain resolution or resolve_explicit_domain.

The merge adds no new code from this PR, so I didn't re-run the five agent reviews on code review 1 already checked. That follows the review contract: review only the commits pushed after the last review.

I didn't run pytest, ruff or the craftsmanship gate locally. On this head, CodeQL and contract pass, and unit was still queued when I posted this.

Notes, no action before merge

remember.py:78-83: a directory that resolves to a known repository still overrides an explicit domain. This is your remaining slice 1 and is unchanged since review 1.

Verdict: approve

) The mutation gate (ADR-1122) of check `unit`, run 42137023299, failed on PR #877. mcp_server.shared.domain_mapping.x_resolve_explicit_domain__mutmut_4 survived because `(name or "")` guards an input the contract never sends (`domain` is a str and the caller checks `if domain:`). No test reached the 26 mutants of remember._resolve_domain quickly enough, so each one ran the whole suite and hit its timeout. Run locally with only the explicit-domain and remember tests, 24 of the 26 survived. Drop the dead guard, drop the redundant default in `detection.get("domain", "") or ""`, which produced two equivalent mutants, and test the resolution order of _resolve_domain with fakes that only answer for the arguments the contract passes. Local mutmut 3.8.1 (SQLite, root): resolve_explicit_domain 8/8 killed; remember._resolve_domain gate on the CI's 36-file selection exits 0. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018kyZYumzRRnGTwTkCk6tdj

cdeust · 2026-11-06T02:12:06Z

CI autopilot : unit (run 42137023299) échouait sur le gate de mutation ADR-1122, pas sur les tests (11763 passed).

resolve_explicit_domain : le mutant 4 (name or "XXXX") survivait parce que (name or "") gardait une entrée que le contrat n'envoie jamais ; garde supprimée.
remember._resolve_domain (fonction préexistante, dans le périmètre parce que la PR en modifie une ligne) : aucun test rapide ne tuait ses 26 mutants, chacun exécutait les 36 fichiers et atteignait son timeout. Ajout de tests_py/handlers/test_remember_resolve_domain.py (ordre de résolution, fakes qui ne répondent qu'aux arguments du contrat) ; suppression du défaut redondant .get("domain", ""), source de deux mutants équivalents.
Commit d31dd3c (fix, pas de réécriture de la branche, ta fusion de main est conservée). Local, mutmut 3.8.1 sur SQLite : 8/8 mutants tués pour resolve_explicit_domain, gate remember.py à rc 0 sur la sélection de 36 fichiers de la CI.

Non vérifié : le gate sur PostgreSQL (absent ici), et le gate domain_mapping.py sur sa sélection complète (test_wiki_coverage_dashboard échoue localement parce que l'environnement tourne en root, sans lien avec cette PR). Aucun reçu de check vert avant la fin de la CI.

Generated by Claude Code

claude · 2026-11-06T02:12:30Z

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

Review of PR #877 (review 3 of 3): commit `d31dd3c`

I found no Blockers.

Earlier Blockers: reviews 1 (5c6cc80) and 2 (e9b0077) had none, so there was nothing to check.

What changed since review 2: one commit, d31dd3c ("kill the mutants of the domain resolution functions"). I read its diff and ran the five agent reviews on it (code quality, security, test coverage, docs accuracy, performance).

domain_mapping.py:488 now calls name.strip() where it used to call (name or "").strip(). That is safe. The function has one caller, remember.py:83, and that call sits behind if domain:, so None and "" never reach it. The tool wrappers also turn None into "" first (tool_registry_memory.py:64, :105). A non-string value would have crashed on .strip() before this commit too.
remember.py:88 now reads detection.get("domain") or "". It returns the same result as the old code when the key is missing, empty or None. The parametrized test covers all three cases.
test_remember_resolve_domain.py runs on any machine because tests_py/conftest.py:237 calls isolate_dev_root_scan(). That stubs _candidate_dev_roots to return nothing and clears the _build_registry cache, so "scifact" can't map to a local repository. Each fake answers only for the exact arguments it expects. Working the mutants out by hand, the tests kill the branch flips, the key changes, the or "" change and the changes to the return values. I didn't run mutmut.
Performance: no change. Both edits are single expressions.

Not run: I couldn't run pytest, ruff or the craftsmanship gate because those Bash commands weren't permitted in this job. The new test file is 63 lines and _resolve_domain is about 17. The Pipeline run on this head was still running when I posted this.

What is accepted: the whole PR, meaning all three commits and all four files (remember.py, domain_mapping.py, test_explicit_domain_identity.py, test_remember_resolve_domain.py). No file or commit has an open Blocker. Slices 1–3 listed in the PR body stay open for #721.

Notes, no action before merge

test_remember_resolve_domain.py:4: the docstring says "kept as given", but resolve_explicit_domain strips and lowercases the domain and maps exact repository names to their family. The file's own test at line 44 expects SciFact → scifact.
test_remember_resolve_domain.py:28-36: the fixture relies on the global registry isolation. A dm._build_registry.cache_clear() in the fixture, or a fake for resolve_explicit_domain, would protect lines 44 and 48 against a later test that fills the cache and doesn't clear it.
remember.py:78-83: a directory that resolves to a known repository still overrides an explicit domain. This is your remaining slice 1 and is now pinned by test_known_repository_directory_wins.

Verdict: approve

…ess (ADR-1122) (#906) mutmut's default fork isolation forks every mutant from the process that already ran the suite. On the Linux runner the child then hangs in torch autograd (titans_memory.py loss.backward) until mutmut times it out, and a timeout fails the gate: #877's 26 _resolve_domain mutants all timed out, and #878 was cancelled at the six-hour job limit. Reproduced in an Ubuntu 26.04 container set up as ci.json "unit" on 11 mutants of #878: fork, 5 of the first 7 timed out (py-spy: child idle in backward()); forkserver, 0 of 11 timed out in 71 s. mutmut 3.8.1 documents forkserver for test setups that are not fork-safe (gevent, grpc, torch).

cdeust mentioned this pull request Nov 5, 2026

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

Open

4 tasks

cdeust marked this pull request as ready for review November 5, 2026 21:04

cdeust mentioned this pull request Nov 5, 2026

ci: run only the mutants of the changed functions in the mutation gate (ADR-1122) #898

Merged

10 tasks done

Merge remote-tracking branch 'origin/main' into fix/721-explicit-doma…

e9b0077

…in-identity

github-actions Bot merged commit 79bd6c3 into main Nov 6, 2026
18 checks passed

This was referenced Nov 6, 2026

fix(recall): scope by the domain identity remember writes (#721, slice 2) #901

Merged

ci: fork mutmut mutants from a fork server, not from the suite's process (ADR-1122) #906

Merged

cdeust mentioned this pull request Nov 6, 2026

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

Merged

8 of 10 tasks

Navigation Menu

Repository navigation

fix(remember): keep an explicit domain instead of remapping it by substring (#721, slice 1) - #877

fix(remember): keep an explicit domain instead of remapping it by substring (#721, slice 1)#877
github-actions[bot] merged 3 commits into
mainfrom
fix/721-explicit-domain-identity

cdeust commented Nov 5, 2026

Uh oh!

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

edited

Loading

Uh oh!

claude Bot commented Nov 5, 2026 •

edited

Loading

Uh oh!

claude Bot commented Nov 5, 2026

Uh oh!

claude Bot commented Nov 6, 2026 •

edited

Loading

Uh oh!

claude Bot commented Nov 6, 2026

Uh oh!

cdeust commented Nov 6, 2026

Uh oh!

claude Bot commented Nov 6, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

cdeust commented Nov 5, 2026

Summary

Type of change

Test plan

Audit notes

Remaining slices for #721

Coding-standards compliance

Uh oh!

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

Uh oh!

Codex Review Summary

Uh oh!

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

Uh oh!

PR #877 review: done

Uh oh!

claude Bot commented Nov 5, 2026

Review of PR #877 (review 1 of 3) — commit 5c6cc80

Uh oh!

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

Uh oh!

PR #877 review 2 of 3: done

Uh oh!

claude Bot commented Nov 6, 2026

Review of PR #877 (review 2 of 3): commit e9b0077

Uh oh!

cdeust commented Nov 6, 2026

Uh oh!

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

Uh oh!

Review of PR #877 (review 3 of 3): commit d31dd3c

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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

edited

Loading

claude Bot commented Nov 5, 2026 •

edited

Loading

Review of PR #877 (review 1 of 3) — commit `5c6cc80`

claude Bot commented Nov 6, 2026 •

edited

Loading

Review of PR #877 (review 2 of 3): commit `e9b0077`

claude Bot commented Nov 6, 2026 •

edited

Loading

Review of PR #877 (review 3 of 3): commit `d31dd3c`