fix(triage): gate the remediation planner on the triage verdict (ADR-0051 §6)#276
Conversation
…0051 §6)
`cliffsec fix` and `/pipeline/run-all` drove the remediation planner directly,
skipping triage entirely — so reachability never resolved (the exposure
analyzer is a no-tools agent that stops at "Unknown") and noise findings got
heavy "fix today + WAF + egress-deny" plans. ADR-0051 §6 already mandates the
Plan gate ("an issue cannot enter the plan without a recorded `real` verdict");
the fix path just wasn't enforcing it. The agentic triage Deep dive (ADR-0052/
0054) that actually reads the repo, traces sinks, and rules out dev/test/
non-ship code was wired only to `POST /findings/{id}/triage` (web UI), never to
the CLI fix path.
This wires the existing, eval-validated triage into the fix flow and gates the
planner on its verdict:
- `cliffsec fix` now triages first (runs the Deep dive), then branches:
- real -> remediation plan, exit 2 (awaiting approval) [unchanged]
- unexploitable /
false_positive -> cleared as noise WITH reasoning, no plan, exit 0
- needs_review -> flagged for human judgment, exit 2
Triage creates a non-status-advancing workspace (ADR-0051 §6), so the finding
stays `new` until a real verdict is human-confirmed.
- `/pipeline/run-all` gains a server-side Plan gate: if a finding was triaged
non-real (and no human-approved plan overrides it), it returns `gated` and
runs no planner — so no caller (web/API/CLI) can emit an alarmist plan on
noise. Back-compat: a finding that was never triaged behaves as before.
- secure-repo skill docs document the new cleared/needs_review/real outcomes.
Verified live against the karakeep ground-truth set (5/5, gpt-5 Deep dive):
shell-quote/happy-dom/drizzle/minimatch -> unexploitable (file-grounded, no
plan); nodemailer address-parser DoS -> needs_review, reachable with a traced
exploit path (kept, not dismissed). Cardinal test flips 5/5 FAIL -> 5/5 PASS.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughIntroduces a triage verdict gate across three layers: the backend ChangesTriage-Gated Fix Flow
Sequence Diagram(s)sequenceDiagram
participant User
participant fix_command as cliffsec fix
participant TriageEndpoint as Triage API
participant SidebarPoller as Sidebar Poll
participant RunAllEndpoint as POST /pipeline/run-all
User->>fix_command: cliffsec fix <finding>
fix_command->>TriageEndpoint: create/reuse workspace → workspace_id
loop poll until verdict set
fix_command->>SidebarPoller: GET workspace sidebar
SidebarPoller-->>fix_command: triage verdict
end
alt verdict == "unexploitable" or "false_positive"
fix_command-->>User: cleared=true + reason, exit 0
else verdict != "real"
fix_command-->>User: awaiting=human_review + reason, exit 2
else verdict == "real"
fix_command->>RunAllEndpoint: POST pipeline/run-all
RunAllEndpoint-->>fix_command: plan steps
fix_command-->>User: verdict=real + plan, awaiting=plan_approval, exit 2
end
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: 3
🤖 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/tests/test_routes_agent_execution.py`:
- Around line 318-342: The test currently only asserts that executor.execute was
not called, but this does not catch a potential regression where a background
task could be scheduled via asyncio.create_task before execute() is invoked. Add
asyncio.create_task to the patch context manager alongside the existing patches
for get_workspace and get_sidebar, mock it appropriately, and add an assertion
after the request to verify that asyncio.create_task was never called (or check
its call count to ensure no background task was created for the gated branch).
In `@cli/cliff_cli/cli.py`:
- Around line 336-346: The is_done lambda function in the poll call is only
checking for the existence of a triage object but not verifying that the verdict
field is populated within it. Update the is_done lambda to check both that the
triage object exists AND that the verdict field within the triage object is
non-empty. This will ensure polling continues until the verdict is actually
ready, preventing premature completion that could cause incorrect routing.
In `@cli/tests/test_cli.py`:
- Line 483: The JSON string containing the "reason" field at line 483 exceeds
the configured maximum line length, causing the ruff E501 check to fail. Break
this long line into multiple lines by splitting the string across multiple lines
while preserving the JSON structure, such as by moving the string value to a new
line or using implicit string concatenation. Ensure the resulting code is valid
JSON and adheres to the project's max line length configuration.
🪄 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: 6cc3ddaa-f266-4ba4-b9f7-154104f1895f
📒 Files selected for processing (6)
backend/cliff/api/routes/agent_execution.pybackend/tests/test_routes_agent_execution.pycli/cliff_cli/cli.pycli/tests/test_cli.pyplugins/cliff-security/skills/cliff-security/SKILL.mdplugins/cliff-security/skills/cliff-security/knowledge/secure-repo.md
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| # The Plan gate (ADR-0051 §6 / ADR-0054): a finding triaged as *not real* | ||
| # never gets a remediation plan — clearing noise with reasoning is the | ||
| # product's value, and an alarmist plan on a non-reachable finding is a | ||
| # false-positive-shaped result. The CLI (`cliffsec fix`) gates on the | ||
| # verdict before it ever calls run-all; this is the server-side backstop so | ||
| # NO caller (web, API) can drive the planner on noise. Defence-in-depth, so | ||
| # it is intentionally narrow: it engages only when triage has run AND | ||
| # returned a non-real verdict AND there is no human-approved plan overriding | ||
| # it. A finding that was never triaged behaves exactly as before. | ||
| sidebar = await get_sidebar(db, workspace_id) | ||
| triage = sidebar.triage if sidebar else None | ||
| if triage is not None: |
There was a problem hiding this comment.
The status="gated" path returns 2xx, so the frontend's optimistic in_progress/planning update never rolls back — should we either raise a 4xx or use a started: false flag? Also, the gate triggers on any non-real verdict including empty ones from partial writes like {"triage": {}} — can we require a truthy verdict (or reuse _has_triage_verdict) so only concrete non-real verdicts block the pipeline?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
In backend/cliff/api/routes/agent_execution.py around lines 337-351 inside
`run_all_pipeline`: 1. The "gated" branch returns RunAllResponse(status="gated") with a
2xx, causing the frontend to stay stuck in optimistic planning/in_progress state.
Refactor so a gated/no-plan outcome is unambiguously "not started" — either raise an
HTTPException with a 4xx status or add a `started: false` flag that the frontend
rollback logic keys off. 2. The plan-gating condition fires even when
`triage.get("verdict")` is empty (e.g., partial sidebar write `{"triage": {}}`). Require
a truthy/valid verdict before gating — reuse `_has_triage_verdict` or add an explicit
guard — so partial/empty verdicts fall through to normal pipeline execution while
still respecting the no-human-approved-plan-override condition.
Heads up!
Your free trial ends tomorrow.
To keep getting your PRs reviewed by Baz, update your team's subscription
There was a problem hiding this comment.
Commit 7f639b8 addressed this comment by requiring a concrete, truthy triage verdict before entering the gated path, so partial writes like {"triage": {}} now fall through to normal execution. It did not change the 2xx status="gated" response, but it did fix one of the specific gating bugs raised.
There was a problem hiding this comment.
Empty-verdict point: fixed in 7f639b8 — the gate now triggers only on a concrete (truthy) non-real verdict; a partial {"triage": {}}/no-verdict write falls through to the normal pipeline. Added a regression test (test_run_all_not_gated_on_empty_triage_verdict).
On the 2xx/frontend-rollback point — I don't think it applies here: useRunAllPipeline (frontend/src/api/hooks.ts) does no optimistic state update; onSuccess only invalidateQueries (agent-runs, sidebar, findings), so the UI re-fetches server truth and reflects the gated outcome — there's no optimistic state to leave stuck. Its only caller is the post-failure Retry button (IssueSidePanel.tsx), which fires on a workspace that already has a real verdict + plan, so the gate passes there. 202 is also accurate: the request was validly handled and gated is a handled outcome, not a client error (a 4xx would be incorrect). Left as-is.
There was a problem hiding this comment.
Thanks for the clarification — that makes sense. I'll save this to memory once the PR is merged.
| def _triage_reason(triage: dict[str, Any]) -> str | None: | ||
| """One-line reasoning behind a triage verdict (mirrors the chat-card line). | ||
|
|
||
| Prefers the exploitability reason, then the reachability summary, then the | ||
| first proof-check detail — whichever the synthesizer/Deep dive populated. | ||
| """ | ||
| exploit = triage.get("exploitability") or {} | ||
| if exploit.get("reason"): | ||
| return exploit["reason"] | ||
| reach = triage.get("reachability") or {} | ||
| if reach.get("summary"): | ||
| return reach["summary"] |
There was a problem hiding this comment.
_triage_reason duplicates triage_runner._verdict_summary’s reason selection, should we surface a single triage.reason from the response instead of re-implementing the fallback order in the CLI?
Want Baz to fix this for you? Activate Fixer
Heads up!
Your free trial ends tomorrow.
To keep getting your PRs reviewed by Baz, update your team's subscription
There was a problem hiding this comment.
_triage_reason lives in the cliffsec CLI — a separate package that talks to the daemon over HTTP, so it can't import the backend's _verdict_summary. True dedup would mean adding a computed reason field to the TriageOutput schema server-side and having the CLI read it; that's a schema change beyond this PR's scope. The CLI extracts from the same canonical fields (exploitability.reason → reachability.summary → first check detail). Leaving as a reasonable follow-up rather than expanding scope here.
There was a problem hiding this comment.
Thanks, that makes sense — the package boundary and schema-change scope are good constraints to keep in mind here. I’ll save this to memory once the PR is merged.
- Fix Ruff E501 in test_cli.py (CI lint-and-test was red).
- run-all Plan gate now triggers only on a CONCRETE non-real verdict; a
partial/empty triage write ({} / no verdict) falls through to the normal
pipeline instead of blocking it (Baz). Adds a regression test.
- `cliffsec fix` polls until triage.verdict is populated, not just until a
triage object exists, so an in-flight write can't route to the non-real
branch early (CodeRabbit).
- Harden the gated route test: assert the gated branch schedules no
background pipeline (asyncio.create_task), not just that execute() didn't run.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ssion
Two real bugs surfaced by a pre-merge multi-agent code review:
1. (high) The server-side Plan gate broke the web "Looks real — remediate"
flow. That action confirms a non-real finding by advancing its status to
`triaged` (ADR-0051 §6) and THEN fires run-all — but the gate keyed only on
`sidebar.triage.verdict` (still non-real), so it returned `gated`, scheduled
no pipeline, and the optimistic UI hung on "Thinking" with no plan and no
error. The gate now bypasses on human confirmation: it blocks only an
unconfirmed finding (status `new`) with a non-real verdict and no approved
plan. The CLI `real` path passes the verdict check and never reaches here;
confirmRealAndRemediate runs onStart in the status-mutation's onSuccess, so
`triaged` is persisted before run-all reads it (no race). New test.
2. (moderate) Re-running `fix` on an already-shipped finding wrongly reported
`awaiting=plan_approval` (exit 2): the prior `if validation and not
plan.get("plan_steps")` fell through to the plan-gate emit when both a plan
and a validation result were present. Reverted to `if validation:` —
validation present means resolved, not awaiting approval. Regression test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pre-merge self-review (multi-agent
|
| triage = sidebar.triage if sidebar and isinstance(sidebar.triage, dict) else None | ||
| # Gate only on a CONCRETE non-real verdict. A missing/partial triage write | ||
| # ({} or no ``verdict``) leaves ``verdict`` empty and falls through to the | ||
| # normal pipeline — only a real dismissal/needs_review verdict blocks the | ||
| # planner, so an in-flight or never-triaged finding behaves as before. | ||
| verdict = (triage.get("verdict") or "").lower() if triage else "" |
There was a problem hiding this comment.
triage.get("verdict").lower() is called unconditionally, but sidebar.triage can be arbitrary JSON from PUT /workspaces/{workspace_id}/sidebar, so a non-string or missing verdict raises AttributeError and 500s POST /pipeline/run-all — should we guard with isinstance(verdict, str) and fall back to '' when not?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
In `backend/cliff/api/routes/agent_execution.py` around lines 347-352 inside
`run_all_pipeline`, add a type guard before calling `.lower()` on
`triage.get("verdict")`. If the value is missing or not a `str`, treat it as unapproved
(e.g., normalize to `''` or `None`) so malformed or legacy sidebar JSON can't raise
`AttributeError`. Ensure the endpoint continues down the normal gated response path
instead of failing the request.
Heads up!
Your free trial ends tomorrow.
To keep getting your PRs reviewed by Baz, update your team's subscription
| finding = await get_finding(db, workspace.finding_id) | ||
| human_confirmed = finding is not None and finding.status != "new" | ||
| plan_approved = bool((sidebar.plan or {}).get("approved")) if sidebar else False | ||
| if not human_confirmed and not plan_approved: |
There was a problem hiding this comment.
human_confirmed treats any status other than "new" as confirmed-real, so mark_started_on_workspace_create() + run_triage_endpoint() can leave a non-real triage eligible for /pipeline/run-all; should we narrow this to finding.status == "triaged" plus the approved-plan override instead of status != "new"?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
backend/cliff/api/routes/agent_execution.py around lines 318-367 inside
`run_all_pipeline`, the gate computes `human_confirmed = finding is not None and
finding.status != "new"`, which incorrectly treats any non-new Finding.status as a
confirmed-real verdict and can bypass the auto-planning/noise block. Refactor this logic
so the bypass only happens when the finding is actually in the confirmed-real state
(e.g., `finding.status == "triaged"`) and still allow the approved-plan override to
bypass as intended. Ensure `finding` is fetched before the gate, and update the log
message/variable naming to reflect the narrower condition so non-real triage updates
can’t slip through.
Heads up!
Your free trial ends tomorrow.
To keep getting your PRs reviewed by Baz, update your team's subscription
…eal` + cut 0.2.2 (#277) * fix(triage): fail safe on code/SAST findings — never a speculative `real` (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> * fix(triage): address review — hedged-negative false-clear, secret deferral, 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> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
User description
What & why
cliffsec fixandPOST /pipeline/run-alldrove the remediation planner directly, skipping triage. The exposure analyzer is a no-tools agent that stops at"Unknown", so reachability never resolved — and noise findings got heavy "fix today + WAF + egress-deny" plans. That's the false-positive-shaped output the product exists to kill.ADR-0051 §6 already mandates the Plan gate — "an issue cannot enter the plan without a recorded
realverdict; triage is a gate, not a suggestion." The agentic triage Deep dive (ADR-0052/0054) that actually reads the repo, traces sinks, and rules out dev/test/non-ship code — backed by a 44-case eval with zero false-clears on a strong tier — was wired only toPOST /findings/{id}/triage(web UI), never to the CLI fix path. So the capability existed; the fix path just bypassed the gate.This PR wires the existing, eval-validated triage into the fix flow and enforces the gate.
Changes
cliffsec fixtriages first (runs the Deep dive), then branches on the verdict:real→ remediation plan, exit 2 (awaiting approval) — unchanged contractunexploitable/false_positive→ cleared as noise with the reasoning on record, no plan, exit 0needs_review→ flagged for human judgment, exit 2Triage creates a non-status-advancing workspace (ADR-0051 §6), so the finding stays
newuntil arealverdict is human-confirmed./pipeline/run-allgains a server-side Plan gate: a finding triaged non-real (without a human-approved plan overriding it) returnsgatedand runs no planner — so no caller (web/API/CLI) can emit an alarmist plan on noise. Back-compat: a finding never triaged behaves exactly as before.secure-reposkill docs updated for the new cleared / needs_review / real outcomes.Tests
realverdict proceeds.Live verification — karakeep ground-truth set (gpt-5 Deep dive)
Cardinal test flips 5/5 FAIL → 5/5 PASS:
/**/**/asset.bin, not user input"users.ts:236 → email.ts:190 sendMail; adversarial panel downgraded real→needs_review (SMTP-config-gated); kept, no premature planNoise cleared with file-grounded reasoning; the real one kept and surfaced. Zero false positives.
🤖 Generated with Claude Code
Generated description
Below is a concise technical summary of the changes proposed in this PR:
Introduce an ADR-0051 §6 Plan gate for
fixby triaging viaPOST /findings/{id}/triage, inspecting the resultingsidebarverdict, and emitting noise-cleared or human-review signals before invoking the planner. Reinforce the gate inrun_all_pipelineby checkingsidebar.triageverdict/status, skipping planning when a finding is stillnewwith a non-real verdict, and extend docs/tests for the cleared/needs_review/real outcomes.run_all_pipelineby inspectingsidebar.triageverdict/status, blocking planner execution for non-real verdicts unless a human confirmation or approved plan exists, and cover the gate with dedicated backend route tests.Modified files (2)
Latest Contributors(0)
cliffsec fixon the Deep dive triage verdict before planning, emit cleared/noise or awaiting-human-review payloads, and document the cleared/needs_review/real outcomes while covering the CLI and doc flows with the new tests.Modified files (4)
Latest Contributors(0)