Guard duplicate preferred_term, and stop it manufacturing a spurious ID_MISMATCH (#328) - #332
Merged
Merged
Conversation
…ID_MISMATCH (#328) Two taxonomy entries sharing a `preferred_term` produced an error-severity finding out of a perfectly valid id: taxonomy: X/NCBITaxon:100, X/NCBITaxon:200 ; source 'Y'/NCBITaxon:100 → [warning] NAME_MISMATCH: resolved to 'X' by source_id NCBITaxon:100 → [error] ID_MISMATCH: Source 'Y' has ID NCBITaxon:100, expected NCBITaxon:200 Both halves are wrong, and they have different causes. **The ID_MISMATCH check ran on an id-resolved participant.** It asks whether an id agrees with the entry its *name* picked out. When the id is what resolved the participant, they agree by construction, so the check has nothing to say — and running it anyway read the expected id off `taxonomy_by_term`, which is last-write-wins, so it compared against whichever duplicate happened to win. Now guarded to name matches only. The check still fires for the copy-paste error it exists to catch, on both sides. **The duplicate name is itself a defect, and was silent.** Because that dict is last-write-wins, the earlier entry disappears from every name lookup: no interaction can connect it, and the record quietly has one fewer reachable member than it lists. That is now DUPLICATE_TAXON_NAME at error severity, safe to gate on because no record in the KB has one — pinned by a test over kb/communities so the assumption cannot rot. Mutation-tested. Six wrong implementations, five caught immediately; the sixth exposed a pre-existing gap — deleting the *target*-side ID_MISMATCH check passed all 52 tests, because only the source side was ever covered. Half that detector could have been removed unnoticed. Now covered. KB unchanged: 55 findings, 0 error. 939 passed, 9 skipped.
Network integrity findingsWarnings only — a member with no interaction yet, or a participant matched by ontology id rather than by name, or one on a community-level interaction that resolves to no member. Reported, but does not fail the build. The full report is attached to the workflow run as an artifact. |
There was a problem hiding this comment.
🟡 Not ready to approve
ID_MISMATCH can still be spuriously emitted on name-matched participants when the matched taxonomy name is duplicated (last-write-wins), which can mislead curators and contradict the PR’s intent to avoid manufactured mismatches.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR updates the network integrity auditor to (1) detect duplicate preferred_term values in a community’s taxonomy and (2) prevent duplicate names from manufacturing a spurious error-level ID_MISMATCH. It fits the codebase’s CI gating model by ensuring only truly contradictory findings block merges while also surfacing an otherwise-silent data integrity defect.
Changes:
- Add a new error-severity issue type
DUPLICATE_TAXON_NAMEto flag duplicate taxonomy display names within a record. - Guard
ID_MISMATCHso it only runs for name-matched participants (not id-resolved fallbacks), avoiding false error findings. - Add focused regression tests, including coverage for target-side
ID_MISMATCHand a KB-wide invariant that no duplicate taxon names exist.
File summaries
| File | Description |
|---|---|
src/communitymech/network/auditor.py |
Adds DUPLICATE_TAXON_NAME, reports duplicates during taxonomy indexing, and narrows ID_MISMATCH to name matches. |
tests/test_network_auditor.py |
Adds regression tests for duplicate-name behavior, KB invariant test, and target-side ID_MISMATCH coverage. |
Review details
Suppressed comments (3)
src/communitymech/network/auditor.py:253
- When detecting a duplicate preferred_term, also record the name as duplicated so later ID_MISMATCH checks can skip comparing IDs against an arbitrary last-write-wins taxonomy_by_term entry.
if preferred in taxonomy_by_term:
issues.append(
{
src/communitymech/network/auditor.py:392
- Even with the new
not source_by_idguard, ID_MISMATCH can still be manufactured ifsource_keyis a duplicated taxonomy name (name match selects an arbitrary last-write-wins entry). Skip ID_MISMATCH when the matched taxonomy key is known-duplicated.
# Only meaningful for a *name* match: it asks whether
# the id agrees with the entry the name picked out. When the
# id is what resolved the participant, it agrees by
# construction — and comparing anyway read the id off a
# last-write-wins entry, so a duplicate name produced a
# spurious error-severity finding (#328).
expected_id = taxonomy_by_term[source_key]["id"]
if not source_by_id and source_id != expected_id:
issues.append(
src/communitymech/network/auditor.py:467
- Same as source side: ID_MISMATCH can still be manufactured on a name match if
target_keyis duplicated in taxonomy_by_term. Skip the comparison for duplicated keys (the record is already flagged as DUPLICATE_TAXON_NAME).
# Only meaningful for a *name* match: it asks whether
# the id agrees with the entry the name picked out. When the
# id is what resolved the participant, it agrees by
# construction — and comparing anyway read the id off a
# last-write-wins entry, so a duplicate name produced a
# spurious error-severity finding (#328).
expected_id = taxonomy_by_term[target_key]["id"]
if not target_by_id and target_id != expected_id:
issues.append(
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Comment on lines
+236
to
237
| taxonomy_by_term: dict[str, dict] = {} | ||
| taxonomy_keys_by_id: dict[str, list[str]] = defaultdict(list) |
This was referenced Aug 3, 2026
Closed
Closed
Six issues from the review. Two matter. **#333 — I made the exact mistake this PR was celebrating catching.** The PR's own mutation table claimed "revert the ID_MISMATCH guard ✓", but the fixture built only a `source_taxon`, so the target-side guard was never exercised: reverting it passed all 939 tests. That is structurally the same one-sided coverage defect this PR found one level down, where disarming the target-side ID_MISMATCH *detector* passed 52 tests — reintroduced one level up, for the guard. The fixture now takes a role parameter and both sides are pinned. **#334 — the test carrying the safety claim could audit nothing.** The KB guard used a relative `Path("kb/communities")`, and `Path.glob` on a missing directory yields nothing without raising, so from any working directory but the repo root it audited zero records and asserted `[] == []`. It is the sole support for gating on DUPLICATE_TAXON_NAME at error severity. Now resolved against `__file__`, following the convention the three other KB-wide test modules already use, and it asserts the sweep was non-empty — verified to fail loudly when pointed at a missing directory. The rest are small: the finding rendered as `• [N/A] ...` in the console because it is record-scoped and hit the generic interaction branch (#335); its message said "Two taxonomy entries" when three or more can collide (#335); the SEVERITY table's stated rationale described only "names something that is not there" and so did not cover naming one thing twice, in the module and in the workflow's curator-facing note (#336); the `term.label` fallback in the duplicate key was unpinned (#337); and a null `taxon_term` raised AttributeError and surfaced as an error-severity UNREADABLE — the per-entry twin of the whole-file case fixed in #329 (#338). Both previously surviving mutations now fail. KB unchanged: 55 findings, 0 error. 944 passed, 9 skipped.
realmarcin
added a commit
that referenced
this pull request
Aug 3, 2026
…ended (#340-#344) The review read the prompt as executable instructions rather than prose, which is the right test, and found the loop unsafe in three ways. **#340 — an unbounded autonomous merge loop.** Step 9 stated squash-merge as an unconditional per-iteration action, with no merge clause in the pause list and no acknowledgement that CLAUDE.md reserves merging to the user. Worse, the loop could not terminate: step 8 files an issue for every review finding, which step 1 then re-ranks and feeds back — borne out by this very session, where reviewing #332 produced #333 and #334, and reviewing #316 produced #328. Now scoped explicitly ("running this prompt authorizes merges *inside* this loop only"), with a do-not-merge list (red CI, branch conflicts, unresolved findings, anything that would redden main), a stop condition (only won't-fix and upstream-blocked left, or 5 merges), and a rule that issues filed in step 9 never feed the same pass. Also adds the missing failure modes: check main's CI is green before branching so a pre-existing failure isn't blamed on the PR, and close the PR unmerged when the review shows the premise was wrong — which is what #315 did to #273, the case the prompt itself cites. **#341 — no cost guardrail, and nothing scoped it to this repo.** The loop could pick a backlog item that fans out a billed deep-research sweep over 311 records with no human in it; the canary rule now covers paid sweeps, not just CI gates, and money is a pause condition. `NEXT_TASKS.md` instructs cross-Mech sync with three sibling repos, which the loop would have inherited implicitly — now forbidden outright. **#342 — the rescue command was wrong.** `gh api repos/OWNER/REPO/...` 404s; gh substitutes `{owner}`/`{repo}`, not uppercase placeholders. Verified the corrected form resolves PR #339. The linkml-validate gotcha now says what *does* catch duplicates rather than implying nothing does, and the gotchas section carries an instruction to fix itself when it goes stale — #290 is open and is plausibly the loop's own first pick, which would have invalidated its own advice. **#343 — internal contradictions.** Step 1 told the agent to update NEXT_TASKS.md before step 3 said to branch; the backlog update now explicitly rides on the issue branch. Step 1 restated the next-tasks skill instead of invoking it, and dropped two of its rules. "One issue at a time" was undercut by a much narrower ban on parallel PRs; now one branch and one PR, start to merge. Upstream-blocked items are dispositioned, and the canary is pinned to the PR branch with a confirm-the-revert-landed step. **#344 — discoverability.** CLAUDE.md's architecture block now lists `prompts/`; without it the next agent would not know the directory exists. 3984 characters, inside the 4000-char limit.
realmarcin
added a commit
that referenced
this pull request
Aug 3, 2026
* Add a /goal prompt for working the backlog end to end A reusable loop: reconcile and prioritize the open issues, then take the top one through branch -> measure -> verify -> PR -> adversarial review -> file issues -> address -> squash-merge -> delete branch, and go again. Two things it encodes that are not obvious from the repo: **Dependencies between PRs.** Several recent pairs had to be done together or in order — #273 could not restore the network gate over a checker blind to dangling edges (#313), and #315 had to correct #273's premise first. The loop asks what a fix touches and whether an open PR already touches it, before starting. **Re-review after review fixes.** The last three real defects all came from commits that landed *after* a review — the fixes themselves were unreviewed code. Two of them were one-sided coverage: a guard tested on the source side but not the target, twice in a row, one level apart. The gotchas section is the accumulated tax of this repo, each verified still true today: `gh pr edit --body` is broken, "Not fixed: #N" silently closes #N because GitHub parses `fixed: #N`, `git add -A` has swept unrelated work into a PR, `just install` fails (#290), and linkml-validate is blind to both duplicate YAML keys and duplicate preferred_terms. 3380 characters, inside the 4000-char limit, so it pastes whole. * Address the review of #339: bound the loop, and stop it merging unattended (#340-#344) The review read the prompt as executable instructions rather than prose, which is the right test, and found the loop unsafe in three ways. **#340 — an unbounded autonomous merge loop.** Step 9 stated squash-merge as an unconditional per-iteration action, with no merge clause in the pause list and no acknowledgement that CLAUDE.md reserves merging to the user. Worse, the loop could not terminate: step 8 files an issue for every review finding, which step 1 then re-ranks and feeds back — borne out by this very session, where reviewing #332 produced #333 and #334, and reviewing #316 produced #328. Now scoped explicitly ("running this prompt authorizes merges *inside* this loop only"), with a do-not-merge list (red CI, branch conflicts, unresolved findings, anything that would redden main), a stop condition (only won't-fix and upstream-blocked left, or 5 merges), and a rule that issues filed in step 9 never feed the same pass. Also adds the missing failure modes: check main's CI is green before branching so a pre-existing failure isn't blamed on the PR, and close the PR unmerged when the review shows the premise was wrong — which is what #315 did to #273, the case the prompt itself cites. **#341 — no cost guardrail, and nothing scoped it to this repo.** The loop could pick a backlog item that fans out a billed deep-research sweep over 311 records with no human in it; the canary rule now covers paid sweeps, not just CI gates, and money is a pause condition. `NEXT_TASKS.md` instructs cross-Mech sync with three sibling repos, which the loop would have inherited implicitly — now forbidden outright. **#342 — the rescue command was wrong.** `gh api repos/OWNER/REPO/...` 404s; gh substitutes `{owner}`/`{repo}`, not uppercase placeholders. Verified the corrected form resolves PR #339. The linkml-validate gotcha now says what *does* catch duplicates rather than implying nothing does, and the gotchas section carries an instruction to fix itself when it goes stale — #290 is open and is plausibly the loop's own first pick, which would have invalidated its own advice. **#343 — internal contradictions.** Step 1 told the agent to update NEXT_TASKS.md before step 3 said to branch; the backlog update now explicitly rides on the issue branch. Step 1 restated the next-tasks skill instead of invoking it, and dropped two of its rules. "One issue at a time" was undercut by a much narrower ban on parallel PRs; now one branch and one PR, start to merge. Upstream-blocked items are dispositioned, and the canary is pinned to the PR branch with a confirm-the-revert-landed step. **#344 — discoverability.** CLAUDE.md's architecture block now lists `prompts/`; without it the next agent would not know the directory exists. 3984 characters, inside the 4000-char limit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #328. Closes #333. Closes #334. Closes #335. Closes #336. Closes #337. Closes #338.
Deferred out of the second review of #316 as wanting its own guard.
The defect
Two taxonomy entries sharing a
preferred_termmanufactured an error-severity finding — one that fails the build — out of a perfectly valid id:NCBITaxon:100is the id of a taxonomy entry named X. Nothing is inconsistent.Two causes, two fixes
1.
ID_MISMATCHran on an id-resolved participant. The check asks whether an id agrees with the entry its name picked out. When the id is what resolved the participant they agree by construction, so the check has nothing to say — and running it anyway read the expected id offtaxonomy_by_term, which is last-write-wins, comparing against whichever duplicate won the race. Now guarded to name matches only; it still fires for the copy-paste error it exists to catch, on both sides.2. The duplicate name was itself an unreported defect. Because that dict is last-write-wins, the earlier entry disappears from every name lookup: no interaction can ever connect it, and the record quietly has one fewer reachable member than it lists. Now
DUPLICATE_TAXON_NAMEat error severity.Error severity is safe because no record in the KB has one — verified by an independent parse of all 311 records, not only via the auditor, and pinned by a test.
Distinct from
tests/test_no_duplicate_yaml_keys.py, which guards duplicate mapping keys; this is duplicatepreferred_termvalues across sibling entries — valid YAML, and invisible tolinkml-validate.Review findings, and two that matter (#333-#338)
#333 — I made the exact mistake this PR was celebrating catching. The original mutation table claimed "revert the ID_MISMATCH guard ✓", but the fixture built only a
source_taxon, so the target-side guard was never exercised: reverting it passed all 939 tests. That is structurally the same one-sided-coverage defect this PR found one level down — where disarming the target-side ID_MISMATCH detector passed 52 tests — reintroduced one level up, for the guard. The fixture now takes a role parameter; both sides are pinned.#334 — the test carrying this PR's safety claim could audit nothing. The KB guard used a relative
Path("kb/communities"), andPath.globon a missing directory yields nothing without raising, so from any working directory but the repo root it audited zero records and asserted[] == []— passing in 1s instead of 5s. It is the sole support for gating on this finding at error severity. Now resolved against__file__(the convention the three other KB-wide test modules already use) and asserting the sweep was non-empty. Verified to fail loudly when pointed at a missing directory.The rest are small:
• [N/A] Two taxonomy entries share...in the console, because it is record-scoped and fell through to the generic interaction branch; and its message said "Two taxonomy entries" when three or more can collide (3 entries → 2 findings, chained pairwise).term.labelfallback in the duplicate key was unpinned. Unreachable for schema-valid records (preferred_termis required), but the branch exists, so it is now tested.taxon_termraisedAttributeErrorand surfaced as an error-severityUNREADABLEnaming a Python type. The per-entry twin of the whole-file case fixed in None-safety fix missed get_taxonomy_lookup, and an empty YAML file still gates with an AttributeError #329.Verification
Fifteen mutations across two rounds. Round one caught 11 of 13; the two survivors — reverting the target-side guard, and restricting the duplicate key to an explicit
preferred_term— both now fail.The review also ran a differential fuzz of this branch against
main: 19,683 exhaustive cases plus 6,000 randomized ones, finding zero inputs wheremainreports something this branch drops without also emittingDUPLICATE_TAXON_NAME. The guard is inert except under exactly the condition that now errors.mainjust lint,mypy: clean