Repository navigation
feat(guardian): give the finder diff-check's dependants as leads (flagged) - #736
Conversation
…gged) A new GUARDIAN_FEATURES value, `diff_check` (#733), off by default. The collector runs diff-check from the PR's merge base to HEAD on the review's graph and adds a prompt section, "code outside this diff that depends on it": callers, subclasses and other readers or writers of changed data keys, capped at 15 in diff-check's own ranking. The finder is told these are leads, to be raised only with a nameable breakage anchored on a diff line. - Any failure (no graph, no merge base, a range git cannot read) costs the section, never the review. Without the flag the prompt is unchanged. - It goes through check_database, so guardian gains no storage edge. - guardian.yml ingests with --data-keys exactly when the feature is set; a test runs the step's own shell with a stand-in `uv` for five values. The pre-registered evaluation in #733 decides whether it is switched on. Chunked mode does not carry the section yet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'diff_check' feature to the guardian service, which identifies and reports code outside the current diff that depends on the modified data keys. The changes include updates to the collector and prompt builder to support this feature, as well as comprehensive unit tests. My review highlights two areas for improvement: ensuring robustness against missing system dependencies by catching OSError in subprocess calls, and improving environment variable handling in tests to avoid potential failures on different platforms.
Sonar's new-code coverage was 77.4% (gate 80): DiffError, an unexpected error and an empty report each now have a test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ator hints Review of #736: - The section is rendered here, not by diff-check's terminal text: paths are repo paths as in the diff (`src/report.py`, not `report.py`), and the MCP and re-ingest hints, which the finder cannot act on, are gone. It is bound to 6,000 characters. - diff-check gets the ingest prefix explicitly (the graph collector's source_root) instead of inferring it from a recorded ingest root. - A CI-shaped test: `pr/` cloned from an origin whose main moves on, sources under src/, no base_ref. It fails with the range taken from the base tip instead of the merge base. - The workflow reads GUARDIAN_FEATURES as parse_features does, ignoring any whitespace, so a tab cannot switch the feature on without --data-keys. - Chunked mode not carrying the section is now said in the docstring. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
From Gemini's review of #736: merge_base also catches OSError, with a test; the workflow shell test extends os.environ instead of replacing it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/guardian review |
![codegraph-brain[bot]](https://avatars.githubusercontent.com/in/4450645?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary: Checked the workflow shell feature-flag parsing against the tr/case logic and its parametrized test, the render_dependants truncation bound, the merge-base range choice (base sha to HEAD, avoiding moved-base symbols), and the best-effort exception handling; these are correct as written. The concerns raised are a test that does not reach the path it names and a silent degradation when merge-base fails.
🤖 claude-haiku-5-5 · 17,499 prompt + 3,520 completion = 21,019 tokens · graph 2/5 files (40%)
From guardian's own review of #736 (posted as codegraph-brain[bot]): - the dependants block sits in a text fence introduced as data extracted from the code under review, never instructions; - a failed merge-base logs a warning, so an inert feature shows in CI; - the test that covered the missing-merge-base path no longer claims to cover DiffError (which has its own test). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Implements the change in #733. The flag stays off: the pre-registered evaluation in #733 decides whether production turns it on. This PR does not close #733.
Change
GUARDIAN_FEATURES=diff_check.GraphContextCollector.collect_dependantsrunscheck_databasefrom the PR's merge base (SourceCollector.merge_base) to HEAD.--data-keysexactly whendiff_checkis invars.GUARDIAN_FEATURES. A test runs the step's real shell with a stand-inuvfor 5 values, and it fails with the case removed.surface-storagegate caught the first draft).Deviation from the issue text
#733 said callers before key sites. I kept diff-check's existing ranking instead (score, then the fixed kind order). That ranking is the one measured in the data-key lab note, and re-ranking it here would change two things at once.
Not covered yet
collect_for_chunk) does not carry the section.guardian_martianstill ingests without data keys. The Martian regression guard will rebuild those graphs with--data-keysas part of the experiment.Checks
tests/unit/test_guardian_dependants.py: 7 tests. The caller of a changed function is listed, and the flag-off, no-graph and bad-range cases add nothing.tests/self_parsing: 1151 passed.🤖 Generated with Claude Code