feat(guardian): context upgrades, cross-model skeptic, inline comments#156
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…odel - GuardianReviewer.__init__ now accepts optional skeptic_provider; finder logic extracted into _finder_pass(); run_review() orchestrates the two-pass flow (skip when no findings/parse_failed; degrade to single-pass on any exception) - build_skeptic_provider() added to runner: defaults to the opposite provider, GUARDIAN_SKEPTIC overrides (gemini|mistral|off), GUARDIAN_SKEPTIC_MODEL allows same-provider/different-model pairs; missing API key degrades to None silently - record_review() extended with skeptic_model/skeptic_status kwargs (defaults preserve backward compat; Task 10 will wire them through run_guardian) - 7 new tests (3 core skeptic scenarios + 4 provider-selection cases); 605 total Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds render_inline_comment and render_review_body helpers to render.py (spec §6.3), and a new github_poster.py module with build_review (pure splitter) and post_inline_review (subprocess gh api, spec §6.4). Tests: 15 render + 3 poster, all green; full gate clean. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Review-backlog minors: - explicit encoding="utf-8" in collector file reads - per-file line cap off-by-one: a file of exactly 1200 lines is now included - get_git_diff() cached (needed twice per run: LLM context + inline index) - post_inline_review moved off the event loop via asyncio.to_thread - drift section returns "" instead of a header-only table when no domains - documented lgtm semantics under all-refuted findings Real bug found in bench telemetry: ReviewResult doubles as the finder's structured-output schema, so the model sometimes fills skeptic_status / verdict / skeptic_note itself. A hallucinated verdict="refuted" would make visible_findings() silently drop a finder finding. _sanitize_finder_result now wipes skeptic-owned fields after the finder pass. Verified: no scoring impact on the 18 committed re-baseline rows (verdicts were all null). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Mean recall ~0.21 (vs 0.22 on gemini-2.5-flash), but noise drops from 0.94/PR to ~0.19/PR with near-perfect precision. PRs 142/143/144 score zero recall — the large-diff targets for the full_files ablation arm. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lash N=1 No arm lifts recall (baseline 0.191 → best combo 0.208, within run variance); large PRs 142-144 stay at zero even with 94K-token context. full_files zeroes noise; drift doubles it. Decision: GUARDIAN_FEATURES stays empty by default — features ship behind flags, off. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… results Arm matrix (6 PRs, N=1, features off): refute-by-default 3.5-flash skeptic killed 7/7 finder findings — 3 noise (good), 2 ambiguous (neutral), and 2 ground-truth matches on PR 122 (file-container-regression, container-zorder), failing the spec gate of at most 1 lost match. Mistral skeptic fails on large PRs (per-minute token cap) and degrades to single-pass as designed. Per plan: flip SKEPTIC_SYSTEM_PROMPT to confirm-by-default (refute only on concrete contrary evidence) and re-run the best arm once. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ate PASSED finder gemini-2.5-flash + skeptic gemini-3.5-flash, 6 PRs N=1: noise 0.83/PR vs 0.94 no-skeptic baseline (strictly down), lost GT matches = 0 (gate allows 1). All 3 skeptic kills were noise/ambiguous. Repo vars set: GUARDIAN_SKEPTIC=gemini, GUARDIAN_SKEPTIC_MODEL=gemini-3.5-flash. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
/guardian review |
![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 implements the Guardian Context, Skeptic, and Inline Comments plan. It introduces feature flags to ContextCollector to gate new context sections (full file contents, flow-fallback for new files, and architectural drift), adds a cross-model skeptic pass to verify findings, updates report rendering to hide refuted findings, and adds a unified-diff line indexer and GitHub review poster to support inline comments. The review feedback is highly constructive, suggesting a robust improvement to the diff line indexer in src/cgis/guardian/diff_index.py to prevent lines starting with +++ inside a hunk from being misparsed as new file headers.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
LGTM — no defects found in this diff. The Pull Request introduces significant enhancements to the CGIS Guardian, including dynamic context collection (full file contents, flow graphs, architectural drift), a robust cross-model skeptic pass, and inline GitHub review comment posting. The implementation meticulously adheres to the stated engineering standards, prioritizing correctness and type safety through Pydantic models, comprehensive type annotations, and graceful degradation for external API interactions. Critical areas like the
|
diff_line_index: a `+++ ` content line inside a hunk (diff quoted in markdown, string literal) was misparsed as a file header, dropping the rest of the file and shifting line numbers. Real headers only appear between hunks — check _NEW_FILE_RE only when not in_hunk. Resolves the known accepted edge from the plan; suggested by review on PR #156. Sonar duplication (4.3% > 3% gate): _StubProvider and the canned finding JSON were copy-pasted across test_guardian_core/runner — extracted to tests/unit/guardian_stubs.py. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Merge the identical added-line and context-line branches into one fall- through, collapse the skip guards into a single condition, and extract the /dev/null ternary into _new_file_path. Cognitive complexity 22 -> ~12 (limit 15). Behavior unchanged - covered by existing 7 diff_index tests incl. the +++-inside-hunk regression. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
1 participant
Implements spec §§4–6 of
docs/specs/2026-06-10-guardian-sprint-design.md(Plan 2:docs/specs/plans/2026-06-10-guardian-context-skeptic-inline.md). Every feature benchmark-gated; raw data inbenchmarks/guardian/results.jsonl.What's new
GUARDIAN_FEATURES=full_files,flow,drift): full HEAD file contents (1200-line/120K-char budgets, smallest-first, explicit omission notes), flow-graph fallback for new files (feat(guardian): inject flow graph (downstream) for new files #94), architectural drift section (first consumer of drift v2 + quotient outside tests).GUARDIAN_SKEPTIC=gemini|mistral|off,GUARDIAN_SKEPTIC_MODEL): second model rules on each finding (confirmed/refuted/uncertain); refuted findings stay in metrics but disappear from the report; any skeptic failure degrades to single-pass (skeptic_status=failed, never breaks a review).gh api); the rest go in the review body; any failure falls back to the classic big comment (peter-evans step is now conditional onposted_inline).Benchmark results (6 curated PRs, ground truth from real review history)
Re-baseline: gemini-3.5-flash vs committed gemini-2.5-flash
3.5-flash: near-zero noise but mute on large diffs; high run-to-run variance in both models.
Task 12 — context ablation (gemini-3.5-flash, N=1, skeptic off)
Decision: no feature ships on by default. None lifts recall (large PRs stay at 0 even at 94K prompt tokens — the failure mode is attention dilution, not missing context → #154). Features land behind flags, off.
Task 13 — skeptic arms (features off, N=1)
Decision: repo vars set to the winner —
GUARDIAN_MODEL=gemini-2.5-flash,GUARDIAN_SKEPTIC=gemini,GUARDIAN_SKEPTIC_MODEL=gemini-3.5-flash. The skeptic stance flip (refute→confirm-by-default) is the single highest-leverage prompt change measured this sprint.Bugs found by the bench itself
skeptic_status,verdict) — a hallucinatedverdict=refutedwould silently hide a finder finding. Fixed with_sanitize_finder_result; verified zero scoring impact on committed rows.Notes for review
parse_featuresraising on unknown names is intentional fail-loud — a typo silently disabling an ablation arm corrupts benchmark comparisons. Everything else degrades gracefully.lgtm=Falsewhen all findings are refuted is by design (finder flagged something; the skeptic killed it).+++content line inside a hunk misparses indiff_line_index→ that finding falls back to the review body.Follow-ups
🤖 Generated with Claude Code
Live smoke note
/guardian reviewran green on this PR (LGTM, finder gemini-2.5-flash from the updated repo vars). Caveat:issue_commentworkflows execute the YAML from the default branch, so the new step wiring (--inline, skeptic env, conditional fallback) only goes live after merge — this run used the new code but the old workflow wiring. First post-merge PR review exercises the full inline path.