fix(triage): fail safe on code/SAST findings — never a speculative real + cut 0.2.2#277
Merged
Merged
Conversation
…eal` (ADR-0051/0052)
The Quick-read synthesizer shipped fake CRITICAL `real` verdicts on SAST/code
findings. Root cause (Mealie round-3 audit): the exposure analyzer reasons about
a code finding from the file PATH, not the file's code, and hedges ("Likely
reachable … needs verification to confirm") — but `_classify_reachable` matched
the affirmative keywords ("likely"/"reachable") → "yes", and with
`internet_facing=true` the dependency-shaped projection emitted `real` 0.82. The
Deep dive that would open the file:line never ran (no repo profile/clone), so the
speculation shipped as the final verdict.
Four fail-safes, all upholding ADR-0051 §10 (never false-clear, never
false-confirm):
- Hedge-aware reachability: "likely / suggests / appears / needs verification"
classify as undetermined, never a confident reachable. `likely` is a
probability, not a confirmation — removed from the affirmative set.
- Code/SAST findings defer: `synthesize_triage` gains `finding_type`; a `code`
finding can't be confidently cleared OR confirmed from the Quick read (it
never opens the file), so it returns `needs_review`, which auto-escalates to
the file-reading Deep dive. Dependency/posture keep the existing projection.
- Triage never strands: a failed prerequisite (e.g. the exposure agent timing
out on a deploy-time migration file) degrades to `needs_review` instead of
aborting with no verdict — the prior behavior left the CLI to poll-timeout and
exit 1. Triage always lands a verdict; never a silent clear, never a crash.
- Deep-dive grounded-path guard: a `trace_path` `reached=yes` with no file:line
hop is ungrounded speculation → `needs_review`, not a confident `real`
(symmetric to the disproof challenge on the clearing side).
Verified: all 5 Mealie noise findings now triage to `needs_review` (zero `real`),
both deterministically against the recorded exposure inputs and live end-to-end
through the daemon. The two prior `real` false positives and the migration
crash are gone.
Also cuts the 0.2.2 release (VERSION + backend + cliffsec CLI + CHANGELOG) so the
triage-first `cliffsec fix` flow (#276) reaches installed users.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThree triage fail-safes are added for v0.2.2: hedged reachability language (e.g., "likely", "may need verification") in exposure analysis is now classified as ChangesTriage Hardening Logic and Tests
Version Bump to 0.2.2
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CHANGELOG.md (1)
755-760:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUpdate CHANGELOG.md reference links for v0.2.2 release.
The bottom-of-file reference links are stale and incomplete. The
[Unreleased]link must point from v0.2.2 (not v0.2.1) to HEAD; a new[0.2.2]link is missing.🔗 Proposed fix to update reference links
-[Unreleased]: https://github.com/cliff-security/cliff/compare/v0.2.1...HEAD +[Unreleased]: https://github.com/cliff-security/cliff/compare/v0.2.2...HEAD +[0.2.2]: https://github.com/cliff-security/cliff/releases/tag/v0.2.2 [0.2.1]: https://github.com/cliff-security/cliff/releases/tag/v0.2.1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CHANGELOG.md` around lines 755 - 760, Update the reference links at the bottom of CHANGELOG.md to reflect the v0.2.2 release. Modify the [Unreleased] link to compare v0.2.2 to HEAD instead of v0.2.1 to HEAD. Additionally, add a new [0.2.2] reference link that points to the v0.2.2 release tag URL following the same pattern as the other version links below it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/cliff/agents/triage_deep/runner.py`:
- Around line 92-97: The `_has_grounded_path` function currently requires only a
`file` to exist for a hop to be considered grounded, but the grounding rule
requires BOTH `file` and `line` to be present. Update the condition in the
return statement to verify that each node in the path has both a non-None `file`
and a non-None `line` value, not just `file` alone. This prevents traces like
`{"file": "a.py", "line": None}` from being incorrectly treated as grounded
reachability.
In `@backend/cliff/agents/triage_runner.py`:
- Around line 189-201: When a prerequisite fails (prereq_failed is True), the
code currently still retrieves stale enrichment and exposure data from previous
successful triage runs in the workspace via list_latest_runs_by_workspace_ids
and passes them to synthesize_triage, which can result in a confident verdict
instead of the intended degraded needs_review. To fix this, conditionally
extract the enrichment and exposure variables (using _structured_output on the
runs) only when prereq_failed is False; when prereq_failed is True, pass empty
or None values for enrichment and exposure to synthesize_triage so that the
degraded verdict is computed without influence from stale prior data.
---
Outside diff comments:
In `@CHANGELOG.md`:
- Around line 755-760: Update the reference links at the bottom of CHANGELOG.md
to reflect the v0.2.2 release. Modify the [Unreleased] link to compare v0.2.2 to
HEAD instead of v0.2.1 to HEAD. Additionally, add a new [0.2.2] reference link
that points to the v0.2.2 release tag URL following the same pattern as the
other version links below it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a6a52e5c-ffcf-4130-a12c-777a255a8aea
⛔ Files ignored due to path filters (2)
backend/uv.lockis excluded by!**/*.lockcli/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
CHANGELOG.mdVERSIONbackend/cliff/agents/runtime/triage_synthesizer.pybackend/cliff/agents/triage_deep/runner.pybackend/cliff/agents/triage_runner.pybackend/pyproject.tomlbackend/tests/agents/test_deep_dive_runner.pybackend/tests/agents/test_triage_runner.pybackend/tests/agents/test_triage_synthesizer.pycli/cliff_cli/__init__.pycli/pyproject.toml
…erral, prereq robustness
Code-review pass (CodeRabbit + a multi-angle finder sweep) surfaced four more
defects in the same false-verdict class; all fixed with tests:
- **Hedged negatives were confidently cleared.** `_classify_reachable` checked
NEGATIVE before HEDGE, so "cannot confirm it is not reachable" / "appears
unreachable but unverified" matched "not reachable" → `unexploitable` 0.88 —
a false-clear of a possibly-real finding. Hedging/uncertainty is now checked
FIRST and dominates any negative/affirmative keyword; a clean "no path found"
still clears. (Combined `_REACH_UNCERTAIN`+`_REACH_HEDGE` into one pass.)
- **`secret` findings fell through to the dependency projection.** A leaked
secret has no CVE → the enricher abstains → the projection cleared it as
`false_positive`, false-clearing a real secret. The deferral now covers
`code` AND `secret` (no-advisory-model types); `dependency`/`posture` keep
the projection. `_code_finding_deferral` → `_deferred_quick_verdict`, type-aware.
- **Deep-dive grounded-path guard required `file` only.** Now requires a real
`file:line` (int line > 0), matching the trace prompt's "no hop without a
file:line you actually read" — a `{file, line: null}` hop no longer counts.
- **Prereq-failure degrade hardened.** Degrade on ANY non-`completed` status
(not just failed/rate_limited — e.g. awaiting_permission), and synthesize the
degraded verdict from THIS run's inputs only (drop a prior attempt's stale
enricher/exposure output) so a degraded verdict can't be a confident
projection from stale data.
Full agents suite: 249 passed. ADR-0051 amendment + CHANGELOG updated.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CHANGELOG.md (1)
768-773: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUpdate changelog reference links to match the new 0.2.2 section.
The [0.2.2] heading on line 10 lacks a corresponding reference link at the bottom, and the [Unreleased] link still points from v0.2.1 instead of v0.2.2. Keep a Changelog format requires these to stay synchronized.
Update the links section to:
-[Unreleased]: https://github.com/cliff-security/cliff/compare/v0.2.1...HEAD +[Unreleased]: https://github.com/cliff-security/cliff/compare/v0.2.2...HEAD +[0.2.2]: https://github.com/cliff-security/cliff/releases/tag/v0.2.2 [0.2.1]: https://github.com/cliff-security/cliff/releases/tag/v0.2.1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CHANGELOG.md` around lines 768 - 773, The reference links section at the bottom of CHANGELOG.md needs to be updated to match the new [0.2.2] section. Update the [Unreleased] reference link to point from v0.2.2 instead of v0.2.1 by changing the version in the compare URL from v0.2.1 to v0.2.2. Additionally, add a new reference link for [0.2.2] that points to the corresponding release tag, and ensure all version links are properly ordered from newest to oldest to maintain consistency with Keep a Changelog format.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/cliff/agents/runtime/triage_synthesizer.py`:
- Around line 155-160: The affirmative branch using substring matching on
_REACH_AFFIRMATIVE can incorrectly classify hedged text like "unconfirmed
reachable" as "yes" because it matches "confirmed" as a substring. To fix this,
ensure that uncertain or negated variations of affirmative keywords are caught
by the unknown check before reaching the affirmative check. Either add hedging
modifiers like "unconfirmed" to the _REACH_UNKNOWN set so they are caught
earlier in the evaluation, or modify the affirmative substring matching to use
word boundary checks instead of raw substring matching to avoid matching
affirmative keywords that are part of negated compound words.
---
Outside diff comments:
In `@CHANGELOG.md`:
- Around line 768-773: The reference links section at the bottom of CHANGELOG.md
needs to be updated to match the new [0.2.2] section. Update the [Unreleased]
reference link to point from v0.2.2 instead of v0.2.1 by changing the version in
the compare URL from v0.2.1 to v0.2.2. Additionally, add a new reference link
for [0.2.2] that points to the corresponding release tag, and ensure all version
links are properly ordered from newest to oldest to maintain consistency with
Keep a Changelog format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f0d4b56a-8146-4735-81a7-951871eebb0c
📒 Files selected for processing (7)
CHANGELOG.mdbackend/cliff/agents/runtime/triage_synthesizer.pybackend/cliff/agents/triage_deep/runner.pybackend/cliff/agents/triage_runner.pybackend/tests/agents/test_deep_dive_runner.pybackend/tests/agents/test_triage_runner.pybackend/tests/agents/test_triage_synthesizer.py
Comment on lines
+155
to
160
| if any(k in s for k in _REACH_UNKNOWN): | ||
| return "unknown" | ||
| if any(k in s for k in _REACH_NEGATIVE) or s in ("no", "false", "none"): | ||
| return "no" | ||
| if any(k in s for k in _REACH_UNCERTAIN): | ||
| return "unknown" | ||
| if any(k in s for k in _REACH_AFFIRMATIVE): | ||
| return "yes" |
There was a problem hiding this comment.
Hedge-safe logic can still be bypassed by affirmative substring collisions.
The affirmative branch does raw substring matching, so uncertain text like unconfirmed reachable can match confirmed and be classified as yes. That can reintroduce confident real projections from non-confirmed reachability language.
Suggested hardening patch
+import re
from typing import Any
@@
_REACH_NEGATIVE = (
@@
"no reachable path",
+ "non-exploitable",
+ "non exploitable",
)
@@
_REACH_AFFIRMATIVE = (
@@
"exploitable",
)
+
+_REACH_AFFIRMATIVE_RE = re.compile(
+ r"\b(?:reachable|yes|true|confirmed|direct|exploitable)\b"
+)
@@
- if any(k in s for k in _REACH_AFFIRMATIVE):
+ if _REACH_AFFIRMATIVE_RE.search(s):
return "yes"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/cliff/agents/runtime/triage_synthesizer.py` around lines 155 - 160,
The affirmative branch using substring matching on _REACH_AFFIRMATIVE can
incorrectly classify hedged text like "unconfirmed reachable" as "yes" because
it matches "confirmed" as a substring. To fix this, ensure that uncertain or
negated variations of affirmative keywords are caught by the unknown check
before reaching the affirmative check. Either add hedging modifiers like
"unconfirmed" to the _REACH_UNKNOWN set so they are caught earlier in the
evaluation, or modify the affirmative substring matching to use word boundary
checks instead of raw substring matching to avoid matching affirmative keywords
that are part of negated compound words.
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.
Problem
Re-auditing the fixed Cliff (PR #276) on a SAST-heavy repo (Mealie) found the code/SAST triage path shipping fake CRITICAL
realverdicts. Two of five noise findings came backreal0.82, exploitable: yes with an empty reachability path.Root cause (corrected from the round-3 hand-off): the bug is not in the Deep dive — that never ran here (
provenance: null, emptyrepo_urlon every workspace). It's in the Quick-read synthesizer:exposure_analyzerreasons about a code finding from the file path, never opening the file, and hedged:"Likely reachable … the exact call chain needs verification to confirm"+internet_facing=1._classify_reachablematched the affirmative keywords (likely/reachable) →"yes".reached=yes+ internet-facing →real,_CONF_REAL=0.82) fired. That projection assumes a CVE/advisory model that does not hold for code findings (the enricher always abstains — there is no CVE), so it never should have been applied to them.Fix (4 fail-safes, all upholding ADR-0051 §10 "never false-clear, never false-confirm")
likely / suggests / appears / needs verification / may / couldclassify as undetermined, never a confident reachable.likelyremoved from the affirmative set (a probability ≠ a confirmation). Protects dependency findings from the same promotion.synthesize_triagegainsfinding_type; acodefinding can't be confidently cleared or confirmed from the Quick read (it never opens thefile:line), so it returnsneeds_review, which auto-escalates to the file-reading Deep dive (decide_escalation). When no Deep dive can run, the honest "needs your review" stands rather than a fabricated verdict.dependency/posturekeep the existing projection.exposure_analyzertiming out on the deploy-time alembic migration) degrades toneeds_reviewinstead of aborting with no verdict. The prior behavior left the CLI to poll-timeout and exit 1 (the migration "crash"). Triage always lands a verdict.trace_pathreached=yeswith nofile:linehop is ungrounded speculation →needs_review, not a confidentreal(symmetric to the disproof challenge on the clearing side). Defense-in-depth for when the Deep dive does run.Plus the 0.2.2 release (VERSION + backend +
cliffsecCLI + CHANGELOG) so the triage-firstcliffsec fixflow from #276 reaches installed users. (Taggingv0.2.2+ the release-environment approval is the maintainer step.)Verification — zero
realon all 5 Mealie noise findingsreal0.82 ❌needs_review✅real0.82 ❌needs_review✅false_positiveneeds_review✅needs_review✅needs_reviewneeds_review✅real.needs_review.Test plan
cd backend && uv run ruff check cliff/ tests/→ cleancd backend && uv run pytest -m 'not e2e'→ 1675 passed, 14 skipped (the 1 failure on a re-run is a flaky live-LLM eval on the untouched report path — skips keyless)cd cli && uv run --extra dev pytest→ 99 passedfinding.typethreading; deep-dive ungrounded-reached=yesguard.Docs (private cliff-os)
ADR-0051 §3 amended (speculation never confirms; code findings defer; prerequisite-failure degrade) and ADR-0052 §6 (grounded-path failure mode).
Follow-up (out of scope)
cliffsec fixpolls the sidebar for any verdict after POSTing/triage, so re-running on an already-triaged finding can read the stale verdict before the background re-triage overwrites it. Fresh-scan findings (the report tour) are unaffected. Worth a separate fix.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests