Audit the isolates the validators already cover (#350, #521) - #520
Open
realmarcin wants to merge 2 commits into
Open
Audit the isolates the validators already cover (#350, #521)#520realmarcin wants to merge 2 commits into
realmarcin wants to merge 2 commits into
Conversation
Most of #350 had been fixed since it was filed: validate_strict's roots, validate-all, validate-terms-all and validate-references-all all cover data/isolates today, and the four term-validator failures it measured no longer reproduce. What remained is the network audit, which kept its own `Path("kb/communities")` default and so never looked at the 12 interactions in data/isolates. They are clean, and that is the point — the gap produced no findings either way and would have stayed invisible until an isolate gained a dangling reference. The workflow did not even trigger on data/isolates, so both halves were missing: a gate that did not fire, in front of a job that would not have covered it. `default_record_roots()` in communitymech.paths is now the one list; the auditor and validate_strict source it rather than restating it. A single directory is still accepted, since callers and tests pass one deliberately. The canary earned its keep. After the auditor was fixed the audit still reported 312 records, because click passed "kb/communities" explicitly on every invocation — the fix looked wired and was not, and only running it and reading the count showed that. `test_the_cli_does_not_override_the_default` exists for exactly that. It is scoped to audit_network's decorators: generate-umap takes the same option name, but whether isolates belong in a visualisation is a modelling question, filed as #519 rather than settled by widening a test. Mutation-checked: restoring the click default reddens the CLI test, removing the trigger path reddens the workflow test, and injecting a bogus participant into an isolate is now reported by the audit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. |
Review of this PR found BatchReporter doing exactly what the click option did:
`communities_dir: Path = Path("kb/communities")`, passed to the auditor
explicitly, so widening the auditor's default did nothing for it. cli.py builds
it as bare `BatchReporter()` in four places, so the literal is what ran every
time — meaning the LLM repair path could not see records the audit now reports
on. Two halves of one workflow disagreeing about which records exist.
It now takes the roots back off the auditor rather than recomputing them, so
they cannot disagree by construction.
The test that caught it is written against the shape rather than the two known
names: any `communities_dir` default that hardcodes a record directory fails.
A check naming only the click option is how the second site survived in the
first place. Visualisation paths are exempt by name with the reason attached —
whether isolates belong in a UMAP or the browser UI is #519, and widening a
coverage test would answer it silently.
Mutation-checked: restoring the literal reddens it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
realmarcin
changed the title
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
1 participant
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.
Most of #350 was already fixed
Reconciled before starting. The issue overstates what remains:
validate_strict.DEFAULT_ROOTSiskb/communitiesonlydata/isolatesvalidate-allis communities-onlydata/isolates/*.yamlvalidate-terms-allis communities-onlydata/isolates/*.yamlvalidate-references-allis communities-onlydata/isolates/*.yamllinkml-term-validator --labelsWhat was actually left
NetworkIntegrityAuditor.__init__keptcommunities_dir: Path = Path("kb/communities"), so the 12 interactions across 3 isolate records were never audited. And the workflow didn't listdata/isolatesin its trigger paths — both halves missing: a gate that never fired, in front of a job that wouldn't have covered it anyway.All 12 are clean, which is why this went unnoticed. The gap produced no findings either way and would have stayed invisible until an isolate gained a dangling reference.
The fix
default_record_roots()lives incommunitymech.paths; the auditor andvalidate_strictsource it rather than restating it. A single directory is still accepted —NetworkIntegrityAuditor(tmp_path)keeps meaning what it did.The canary earned its keep, twice
First: after the auditor was fixed,
audit-networkstill reported 312 records. Click was passing"kb/communities"explicitly on every invocation. The fix looked wired and wasn't — the exit code never changed, only the count did.Second (#521): review then found
BatchReporterdoing the identical thing — same literal default, passed explicitly to the auditor, constructed as bareBatchReporter()in four places incli.py. So the LLM repair path could not see records the audit now reports on: two halves of one workflow disagreeing about which records exist. It now takes the roots off the auditor rather than recomputing them, so they cannot disagree by construction.That second find is why
test_no_constructor_hardcodes_the_old_rootis written against the shape — anycommunities_dirdefault hardcoding a record directory — rather than naming the known offenders. A test checking one of two identical sites is exactly how the second one survives.Visualisation paths (
browser_export,render,umap_generator) are exempt by name with the reason attached: whether isolates belong in a UMAP or the browser UI is a modelling question (#519), and widening a coverage test would answer it silently.Checks
uv run communitymech audit-network— 316 records (312 + 4), was 312just validate-strict— 316 files, exit 0just lint— exit 0uv run pytest tests/— 2382 passed, 16 skippedjust check-network-quality— exit 1, unchanged: that is the pre-existing "warnings" classification and is 1 with communities alone too; the workflow step maps 1 →warningsand always exits 0BatchReporter's literal reddens the constructor test; removing the trigger path reddens the workflow test; injecting a bogus participant into an isolate is now reported by the audit and was silently ignored beforeCloses #350, closes #521. Found #519.
🤖 Generated with Claude Code