feat(guardian): recall-lean universal finder + skeptic reconciliation#249
Conversation
…ewer step 1) read_file() now returns "" instead of "Error: File ... not found." for a missing file, and build_user_prompt() gates the ENGINEERING STANDARDS and PROJECT ONTOLOGY sections on non-empty content (like the graph/full_files/drift sections already are). Without this, a repo lacking CONTRIBUTING.md or docs/ontology/ (i.e. every non-cgis repo guardian is meant to review) got the literal "Error: File ... not found." strings injected as if they were the standards/ontology text — and an empty ONTOLOGY heading nudging the model toward ontology findings with no ontology context. First commit toward a universal finder prompt (replace gemini-code-assist across all repos); ontology priority/category overlay-gating is the next step. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nding cap (Tier-1)
Reframes the finder as the recall half of the finder→skeptic split: surface
every plausible defect with a nameable failure scenario; the skeptic verifier
holds precision downstream. Three evidence-backed levers (all 3-0 in the
deep-research synthesis):
- system prompt: "HIGH PRECISION / a false positive wastes time as much as a
missed bug" → "your job is RECALL ... a missed bug is the expensive error,
a borderline finding is cheap because the skeptic filters it."
- 80% confidence gate → "surface on a nameable failure scenario";
confidence becomes a priority signal, not an inclusion gate.
- "cap at 5 findings" → no cap (recall is ∝ number of claims emitted —
CriticGPT/OpenAI; capping directly suppresses recall).
Bench (mistral-medium, pr-144, finder-only, no skeptic), 4 runs:
recall 0/5 → 1/5 stable across all 4 (was 0/5 across 5 model tiers on the old
prompt). Finder now emits 10-21 findings (was ≤5) and reliably catches the
triad-census finding; surfaced the yaml-mapping-guard class once at confidence
60 (the old gate would have dropped it) but anchored 5 lines before the GT
window. Noise 1-10 — the expected recall-stage tradeoff the skeptic absorbs.
Refs: guardian recall research (cap-removal + max-recall instruction = Tier-1).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…r blind classes
Adds the Tier-2 recall levers (CoT paired with few-shot, per the deep-research
synthesis where few-shot is effective only alongside chain-of-thought):
- HUNTING RULE 6 "reason per changed function before deciding": walk each
touched function's inputs (is any external/untrusted?), its awkward inputs
(None / non-dict-where-dict-assumed / scalar-where-iterable-assumed / empty),
and whether a deleted line held an invariant — THEN decide what to surface.
- Two new focus areas naming the classes the finder was structurally blind to:
"Unvalidated external data" (config/YAML value used as dict/list/set without a
type/shape guard; `value or {}` lets a non-dict truthy value through) and
"Exact-equality on floats / money" (bare ==/!= including in test asserts).
- Two compact worked examples showing the per-function walk → finding for those
two classes (kept generic, not copied from any fixture, to avoid over-anchoring).
Bench (mistral-medium, pr-144, finder-only), 5 runs:
median recall 1/5 → 3/5 (runs: .6 .2 .6 .6 .4); union 3/5.
yaml-mapping-guard class now caught in 5/5 runs (was 1/6 on Tier-1) — the
unvalidated-data focus area + few-shot landed it in the exact GT windows.
Still open: float-equality-in-tests stays 0/5, but the GT lines sit outside the
diff hunk in diff-only mode (a context limit, not a prompt one — retest with
GUARDIAN_FEATURES=full_files). Longer output (≤31 findings) raises timeout/
truncation rate — a robustness follow-up.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… (recall-lean reconciliation)
The skeptic encoded the OLD precision-gated finder's assumptions and was actively
destroying the recall-lean finder's gains:
- apply_verdicts converted an 'uncertain' verdict to 'refuted' whenever the
x0.9-discounted confidence fell below 80 — auto-killing exactly the genuine
low-confidence findings the recall-lean finder now surfaces on purpose.
- the system prompt told the skeptic "the finder already applied a confidence
gate" (false since f6e661b), priming it to refute more aggressively.
Now: 'uncertain' is KEPT (confidence still discounted x0.9 as a ranking signal,
never auto-refuted); only an explicit 'refuted' verdict drops a finding. The
prompt reframes the finder as recall-optimised and tells the skeptic not to
refute merely for low confidence / being speculative.
Bench (mistral skeptic, pr-140/143/144): recall that the OLD skeptic destroyed is
recovered/held — pr-140 0.0/0.1 → 0.2/0.3, pr-143 0.7/0.2 → 0.7/0.5.
KNOWN LIMITATION: this preserves recall but does NOT yet cut finder noise (the
skeptic now errs lenient; noise stays ~finder-only). A same-model (mistral)
skeptic is binary — adding few-shot false-positive examples swung it back to
refuting everything (recall→0), so that was reverted. Cutting noise without
killing recall needs the cross-model skeptic (gemini-for-mistral, the designed
default) — tracked as a follow-up, not in this branch.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request transitions the CGIS Guardian finder to a recall-lean strategy by removing the confidence gate and finding cap, shifting the responsibility of filtering false positives to the skeptic verifier stage. The skeptic verifier has been updated to discount confidence for uncertain findings without refuting them. Additionally, the prompt builder now gracefully omits missing configuration or ontology sections, and read_file returns an empty string instead of an error message when a file is missing. A review comment suggests using is_file() instead of exists() in read_file to prevent potential directory or permission errors.
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.
|
/guardian review |
There was a problem hiding this comment.
Summary: Reviewed diff for logic correctness, type safety, and test coverage. Found critical dead code in skeptic.py (unused constant and reference), minor issues in prompt construction and test robustness, and potential edge cases in file handling and confidence discounting. No ontology or type safety violations detected.
🤖 mistral-medium-latest · 10,846 prompt + 2,227 completion = 13,073 tokens · graph 3/6 files (50%)
| file_path = self.project_root / relative_path | ||
| if not file_path.exists(): | ||
| return f"Error: File {relative_path} not found." | ||
| return "" |
There was a problem hiding this comment.
🟡 [Logic Bug] — Empty string return for missing file may mask real errors in prompt construction
Returning an empty string for a missing file (e.g., CONTRIBUTING.md) silently omits the section, but this could mask cases where the file should exist (e.g., in a CGIS repo) and its absence is a real issue. The current behavior treats missing files as non-errors, but this may hide configuration problems.
Fix: Consider logging a warning when a file is missing, or distinguish between 'file not found' (error) and 'file intentionally omitted' (valid).
Verified by codestral-latest
| return f"""Review the following Pull Request diff for real defects. | ||
|
|
||
| ### 1. ENGINEERING STANDARDS (from CONTRIBUTING.md) | ||
| contributing_section = "" |
There was a problem hiding this comment.
🟡 [Logic Bug] — Empty string concatenation may leak empty headings into prompt
If contributing is an empty string, contributing_section becomes "" (empty), but the subsequent concatenation ({contributing_section}{ontology_section}) may still include the ### 1. ENGINEERING STANDARDS heading if the empty string is not properly stripped. The test test_build_user_prompt_omits_standards_and_ontology_when_empty guards this, but the logic relies on the empty string being truly empty (no whitespace or newlines).
Fix: Ensure contributing and ontology are stripped of whitespace before checking for emptiness, or explicitly check for None or falsy values.
Verified by codestral-latest
| ### 3. CHANGES TO REVIEW (git diff) | ||
| """ | ||
|
|
||
| return f"""Review the following Pull Request diff for real defects. |
There was a problem hiding this comment.
🟡 [Logic Bug] — Prompt construction may include malformed sections if contributing or ontology contain newlines or special characters
The f-string concatenation assumes contributing and ontology are clean strings, but if they contain newlines, markdown, or other special characters, the prompt may become malformed (e.g., unclosed code blocks, broken headings). This is not guarded by the current logic.
Fix: Sanitize contributing and ontology inputs (e.g., strip newlines, escape markdown) before embedding them in the prompt.
Verified by codestral-latest
| @@ -94,12 +99,12 @@ def apply_verdicts(findings: list[Finding], skeptic: SkepticResult) -> list[Find | |||
| merged.append(finding) # absence of a verdict is not a refutation | |||
There was a problem hiding this comment.
🟡 [Logic Bug] — Unruled findings are kept with verdict=None, but this may conflict with downstream expectations
The code keeps findings with verdict=None (unruled) as-is, but downstream consumers (e.g., visible_findings) may expect all findings to have a verdict. This could lead to inconsistent behavior if None is treated as 'confirmed' or 'refuted' elsewhere.
Fix: Explicitly set verdict to "confirmed" for unruled findings to avoid ambiguity.
Verified by codestral-latest
| assert "the diff" in prompt | ||
| assert "ENGINEERING STANDARDS" not in prompt | ||
| assert "PROJECT ONTOLOGY" not in prompt | ||
| assert "not found" not in prompt |
There was a problem hiding this comment.
🟡 [Test Coverage] — Test may be brittle if read_file behavior changes
The test checks that the string "not found" does not appear in the prompt, but this relies on the implementation of read_file returning an empty string for missing files. If read_file were to change (e.g., to return a different error message), this test would fail even if the behavior is still correct.
Fix: Test the behavior more directly (e.g., mock read_file to return "" and verify the prompt omits the section).
Verified by codestral-latest
| """uncertain → confidence x0.9 (ranking signal), verdict stays uncertain.""" | ||
| f = _FINDING.model_copy(update={"confidence": 89}) | ||
| merged = apply_verdicts([f], SkepticResult(verdicts=[_verdict(0, "uncertain")])) | ||
| assert merged[0].verdict == "uncertain" |
There was a problem hiding this comment.
🟡 [Test Coverage] — Test does not verify the confidence discounting logic for uncertain findings
The test test_uncertain_discounts_confidence_but_keeps_finding checks the verdict but does not explicitly verify that the confidence was discounted (e.g., 89 → 80). While the test name implies this, the assertion does not enforce it. This could hide regressions in the discounting logic.
Fix: Add an assertion to verify the confidence value (e.g., assert merged[0].confidence == 80).
Verified by codestral-latest
| f = _FINDING.model_copy(update={"confidence": 30}) | ||
| merged = apply_verdicts([f], SkepticResult(verdicts=[_verdict(0, "uncertain")])) | ||
| assert merged[0].verdict == "refuted" | ||
| assert merged[0].verdict == "uncertain" |
There was a problem hiding this comment.
🟡 [Test Coverage] — Test for low-confidence uncertain findings does not verify the confidence value
The test test_uncertain_low_confidence_is_kept_not_refuted checks the verdict but does not verify the confidence was discounted (e.g., 30 → 27). This could hide regressions in the discounting logic.
Fix: Add an assertion to verify the confidence value (e.g., assert merged[0].confidence == 27).
Verified by codestral-latest
| assert merged[0].verdict == "refuted" | ||
| assert merged[0].verdict == "uncertain" | ||
| assert merged[0].confidence == 27 # round(30 * 0.9) | ||
| assert visible_findings(merged) == merged # not dropped |
There was a problem hiding this comment.
🟡 [Test Coverage] — Test assumes visible_findings is defined but does not import it
The test references visible_findings but this function is not imported or defined in the test file. This will raise a NameError at runtime.
Fix: Import visible_findings from the module where it is defined (e.g., from cgis.guardian.skeptic import visible_findings).
Verified by codestral-latest
Review — recall-lean universal finder + skeptic reconciliationChecked the code-level changes directly (not just the prompt wording). The code is clean and correct; approve with two design notes. ✅ Code verified
🟡 1. "Universal" claim vs cgis-specific bench & few-shotThe PR frames this as project-agnostic ("toward replacing gemini across all repos"), but:
The classes (unvalidated external data, float- 🟡 2. Deployment — noisier-now by designRecall is up (gate + cap removed) but the precision half (cross-model skeptic) is deferred to #246. The same-model skeptic, as the PR admits, "doesn't cut finder noise" and only drops explicitly- 🟢 Minor
Bottom line: minimal, correct code; recall (0/5) is unambiguously the right thing to fix first; limits are honestly tracked as follow-ups (#246/#247/#248). Approve. Before merge I'd only soften the "universal" claim (🟡1) to what's actually validated and confirm Guardian's deployment mode (🟡2). No blockers. — reviewed by Claude (relayed via @zaebee) |
…to "" Addresses a review note (gemini-code-assist) on #249: read_file guarded on exists(), which is True for a directory — read_text() would then raise IsADirectoryError. is_file() makes a path that resolves to a directory return "" (section omitted) like a missing file, keeping the universal-reviewer path robust. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|



Makes Guardian's finder recall-lean so it stops missing subtle real defects, makes the prompt-assembly degrade cleanly on repos without cgis's standards/ontology files (a step toward reusing Guardian beyond cgis), and reconciles the skeptic so it no longer sabotages the recall-lean finder. Every change is evidence-backed (deep-research synthesis + bench on pr-140..144, mistral-medium).
The problem
The old finder was precision-maximised ("HIGH PRECISION", an 80% confidence gate, "cap at 5", "omit uncertain") and got 0/5 recall on pr-144 across 5 model tiers (fast→frontier) — proving the bottleneck was the prompt, not the model. The subtle ground-truth defects (unvalidated YAML→
set(), bare float==) are exactly the "when in doubt, stay silent" cases the prompt suppressed.What changed (5 commits)
69376a6universal mechanic —read_file()returns""(not"Error: File ... not found.") for a missing file;build_user_promptgates the standards/ontology sections on non-empty content. A repo without CONTRIBUTING.md / docs/ontology now reviews cleanly instead of getting error strings injected as if they were the standards.f6e661bTier-1 finder — drop the confidence gate & finding cap; reframe as the recall half of finder→skeptic ("a missed bug is the expensive error; the skeptic filters false positives"). Recall is ∝ number of claims (CriticGPT), so the cap directly suppressed it. → pr-144 0/5 → median 1/5.79c1c5dTier-2 finder — per-function CoT rule + "unvalidated external data" & "float ==" focus areas + 2 worked few-shot examples (CoT paired with few-shot, per the research). → pr-144 median 3/5; the yaml-mapping-guard class went from 1-of-6 runs to 5/5 runs.afae100skeptic reconciliation — the skeptic was tuned for the OLD gated finder and was destroying recall: it auto-refuteduncertainfindings below an 80 confidence gate, and its prompt claimed "the finder already applied a confidence gate" (false). Nowuncertainis kept (confidence discounted ×0.9 as ranking only); only an explicitrefuteddrops a finding. → recovers recall the old skeptic killed (pr-140 0.0/0.1 → 0.2/0.3, pr-143 0.7/0.2 → 0.7/0.5).e25f590robustness —read_fileguards onis_file()notexists(), so a path resolving to a directory degrades to""instead of raising IsADirectoryError (review note).Bench (mistral-medium, finder-only unless noted)
Scope of "universal" — what is and isn't validated here
Error: File…on a repo lacking CONTRIBUTING.md/ontology. The targeted defect classes (unvalidated external data, float-==) are language-agnostic Python footguns.set(cfg)over patterns.yaml; a drift-test float==). A non-cgis bench (owner-api / a TS repo) is still needed — the per-class few-shot could even hurt on a diverse repo (cf. the over-anchoring lesson in guardian: recall blind classes — pr-142 cosine masking + float-equality-in-tests #247). Treat this PR as "recall-lean on cgis + degrades cleanly elsewhere", not "validated universal reviewer."Deployment note — noisier now, by design (interim)
Recall is up (gate + cap removed) but the precision half — a cross-model skeptic (#246) — is deferred. The same-model mistral skeptic preserves recall but does not cut finder noise (it only drops explicitly-
refutedfindings), so right now Guardian surfaces more findings, including low-confidence ones. If Guardian posts inline comments on live PRs, keep it advisory/non-gating until #246 lands. The tradeoff is right (0/5 recall is clearly worse than noise), but the interim noise is real — and visible on this very PR, where Guardian's own/guardian reviewproduced mostly low-value findings.Known limitations → follow-ups (NOT in this branch)
Full verification green:
make format && lint && type-check && pytest(1056 passed post-merge)&& doc-coverage(99.6%).🤖 Generated with Claude Code