diff --git a/backend/cliff/api/routes/agent_execution.py b/backend/cliff/api/routes/agent_execution.py index 02863357..b3105230 100644 --- a/backend/cliff/api/routes/agent_execution.py +++ b/backend/cliff/api/routes/agent_execution.py @@ -334,6 +334,48 @@ async def run_all_pipeline( executor = request.app.state.agent_executor context_builder = request.app.state.context_builder + # 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 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 "" + if verdict and verdict != "real": + # ...but a HUMAN can override a non-real verdict. The web "Looks real — + # remediate" action confirms the finding by advancing its status to + # ``triaged`` (ADR-0051 §6 — ``triaged`` means "confirmed real") BEFORE + # firing run-all; an already-approved plan is the same signal. Only an + # unconfirmed finding still sitting at ``new`` with a non-real verdict is + # blocked, so the gate stops auto-planning on noise without breaking the + # human confirm-real path. (The CLI `real` path passes the verdict check + # above and never reaches here.) + 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: + logger.info( + "run-all gated for workspace %s: triaged %r, finding still new — no plan", + workspace_id, + verdict, + ) + return RunAllResponse( + status="gated", + message=( + f"Triaged as {verdict} — no remediation plan needed. " + f"Confirm it's real to plan a fix." + ), + ) + try: await executor.check_not_busy(db, workspace_id) except AgentBusyError as exc: diff --git a/backend/tests/test_routes_agent_execution.py b/backend/tests/test_routes_agent_execution.py index e3af0e81..48263c74 100644 --- a/backend/tests/test_routes_agent_execution.py +++ b/backend/tests/test_routes_agent_execution.py @@ -4,6 +4,7 @@ import asyncio from datetime import UTC, datetime +from types import SimpleNamespace from typing import TYPE_CHECKING, Any from unittest.mock import AsyncMock, patch @@ -212,6 +213,11 @@ def _capturing_create_task( "cliff.api.routes.agent_execution.get_workspace", return_value=_make_workspace(), ), + # No prior triage → the Plan gate is dormant; behaviour unchanged. + patch( + "cliff.api.routes.agent_execution.get_sidebar", + AsyncMock(return_value=None), + ), patch( "cliff.api.routes.agent_execution._resolve_repo_env_vars", AsyncMock(return_value={}), @@ -278,6 +284,11 @@ def _capturing_create_task( "cliff.api.routes.agent_execution.get_workspace", return_value=_make_workspace(), ), + # No prior triage → the Plan gate is dormant; behaviour unchanged. + patch( + "cliff.api.routes.agent_execution.get_sidebar", + AsyncMock(return_value=None), + ), patch( "cliff.api.routes.agent_execution._resolve_repo_env_vars", AsyncMock(return_value={}), @@ -296,6 +307,262 @@ def _capturing_create_task( assert executor.execute.await_count == 1 + @pytest.mark.asyncio + async def test_run_all_gated_when_triaged_not_real( + self, app: FastAPI, client: AsyncClient + ) -> None: + """The Plan gate (ADR-0051 §6 / ADR-0054): a finding triaged as *not + real* never drives the planner pipeline. run-all returns ``gated`` and + executes no agent — the report-tour bright line, enforced server-side so + no caller (web, API) can emit an alarmist plan on noise.""" + executor = app.state.agent_executor + executor.check_not_busy = AsyncMock() + executor.execute = AsyncMock(return_value=_make_execution_result()) + + # Capture create_task so we can assert the gated branch schedules NO + # background pipeline at all (a regression could create the task and + # fail before execute() — checking execute alone would miss it). + captured_tasks: list[asyncio.Task[Any]] = [] + real_create_task = asyncio.create_task + + def _capturing_create_task( + coro: Coroutine[Any, Any, Any], + ) -> asyncio.Task[Any]: + task = real_create_task(coro) + captured_tasks.append(task) + return task + + sidebar = SimpleNamespace( + triage={"verdict": "unexploitable", "confidence": 0.88}, + plan=None, + ) + with ( + patch( + "cliff.api.routes.agent_execution.get_workspace", + return_value=_make_workspace(), + ), + patch( + "cliff.api.routes.agent_execution.get_sidebar", + AsyncMock(return_value=sidebar), + ), + # Finding still `new` (un-confirmed) → the non-real verdict gates. + patch( + "cliff.api.routes.agent_execution.get_finding", + AsyncMock(return_value=SimpleNamespace(status="new")), + ), + patch( + "cliff.api.routes.agent_execution.asyncio.create_task", + _capturing_create_task, + ), + ): + resp = await client.post("/api/workspaces/ws-1/pipeline/run-all") + + assert resp.status_code == 202 + assert resp.json()["status"] == "gated" + assert not captured_tasks, "the gated branch must schedule no background pipeline" + assert executor.execute.await_count == 0, ( + "the planner pipeline must never run for a non-real triage verdict" + ) + + @pytest.mark.asyncio + async def test_run_all_not_gated_when_human_confirmed_real( + self, app: FastAPI, client: AsyncClient + ) -> None: + """A human can override a non-real verdict: the web "Looks real — + remediate" action advances the finding to ``triaged`` (ADR-0051 §6) + before firing run-all. The gate must let that through even though + ``sidebar.triage.verdict`` is still non-real — otherwise the confirm-real + path is silently dead-ended (no plan, no error).""" + executor = app.state.agent_executor + executor.check_not_busy = AsyncMock() + executor.execute = AsyncMock(return_value=_make_execution_result()) + executor.push_permission_event = lambda ws_id, evt: None + + # All sections present so the loop breaks immediately (no execute call) — + # we only need to prove the gate let the run through (status running). + app.state.context_builder.get_context_snapshot = AsyncMock( + return_value={ + "finding": {"id": "f-1"}, + "enrichment": {"normalized_title": "x"}, + "ownership": None, + "exposure": {"recommended_urgency": "high"}, + "evidence": {"affected_files": [], "fix_safety": "safe_bump"}, + "plan": {"plan_steps": ["s"]}, + "remediation": {"status": "pr_created"}, + "agent_run_history": [], + } + ) + + # Non-real verdict, NO approved plan — but the human confirmed it real + # (finding advanced to ``triaged``). + sidebar = SimpleNamespace( + triage={"verdict": "needs_review", "confidence": 0.55}, plan=None + ) + captured_tasks: list[asyncio.Task[Any]] = [] + real_create_task = asyncio.create_task + + def _capturing_create_task( + coro: Coroutine[Any, Any, Any], + ) -> asyncio.Task[Any]: + task = real_create_task(coro) + captured_tasks.append(task) + return task + + with ( + patch( + "cliff.api.routes.agent_execution.get_workspace", + return_value=_make_workspace(), + ), + patch( + "cliff.api.routes.agent_execution.get_sidebar", + AsyncMock(return_value=sidebar), + ), + patch( + "cliff.api.routes.agent_execution.get_finding", + AsyncMock(return_value=SimpleNamespace(status="triaged")), + ), + patch( + "cliff.api.routes.agent_execution._resolve_repo_env_vars", + AsyncMock(return_value={}), + ), + patch( + "cliff.api.routes.agent_execution.asyncio.create_task", + _capturing_create_task, + ), + ): + resp = await client.post("/api/workspaces/ws-1/pipeline/run-all") + assert resp.status_code == 202 + assert resp.json()["status"] == "running", ( + "a human-confirmed (triaged) finding must NOT be gated" + ) + assert captured_tasks, "the confirm-real path must reach the pipeline" + await captured_tasks[0] + + @pytest.mark.asyncio + async def test_run_all_not_gated_on_empty_triage_verdict( + self, app: FastAPI, client: AsyncClient + ) -> None: + """A partial/empty triage write ({} or no verdict) must NOT gate — only + a concrete non-real verdict blocks the planner. An in-flight or + never-triaged finding behaves exactly as before (proceeds).""" + executor = app.state.agent_executor + executor.check_not_busy = AsyncMock() + executor.execute = AsyncMock(return_value=_make_execution_result()) + executor.push_permission_event = lambda ws_id, evt: None + + app.state.context_builder.get_context_snapshot = AsyncMock( + return_value={ + "finding": {"id": "f-1"}, + "enrichment": {"normalized_title": "x"}, + "ownership": None, + "exposure": {"recommended_urgency": "high"}, + "evidence": {"affected_files": [], "fix_safety": "safe_bump"}, + "plan": {"plan_steps": ["s"], "approved": True}, + "remediation": {"status": "pr_created"}, + "agent_run_history": [], + } + ) + + sidebar = SimpleNamespace(triage={}, plan=None) # partial write, no verdict + captured_tasks: list[asyncio.Task[Any]] = [] + real_create_task = asyncio.create_task + + def _capturing_create_task( + coro: Coroutine[Any, Any, Any], + ) -> asyncio.Task[Any]: + task = real_create_task(coro) + captured_tasks.append(task) + return task + + with ( + patch( + "cliff.api.routes.agent_execution.get_workspace", + return_value=_make_workspace(), + ), + patch( + "cliff.api.routes.agent_execution.get_sidebar", + AsyncMock(return_value=sidebar), + ), + patch( + "cliff.api.routes.agent_execution._resolve_repo_env_vars", + AsyncMock(return_value={}), + ), + patch( + "cliff.api.routes.agent_execution.asyncio.create_task", + _capturing_create_task, + ), + ): + resp = await client.post("/api/workspaces/ws-1/pipeline/run-all") + assert resp.status_code == 202 + assert resp.json()["status"] == "running" + assert captured_tasks, "an empty triage verdict must not gate the pipeline" + await captured_tasks[0] + + @pytest.mark.asyncio + async def test_run_all_proceeds_when_triaged_real( + self, app: FastAPI, client: AsyncClient + ) -> None: + """A ``real`` verdict releases the Plan gate — run-all proceeds normally + (status ``running``, background pipeline created).""" + executor = app.state.agent_executor + executor.check_not_busy = AsyncMock() + executor.execute = AsyncMock(return_value=_make_execution_result()) + executor.push_permission_event = lambda ws_id, evt: None + + # All sections present + plan approved → the loop breaks immediately + # (no execute call), but the task is still created — proving the gate + # let the run through rather than short-circuiting to ``gated``. + app.state.context_builder.get_context_snapshot = AsyncMock( + return_value={ + "finding": {"id": "f-1"}, + "enrichment": {"normalized_title": "x"}, + "ownership": None, + "exposure": {"recommended_urgency": "high"}, + "evidence": {"affected_files": [], "fix_safety": "safe_bump"}, + "plan": {"plan_steps": ["s"], "approved": True}, + "remediation": {"status": "pr_created"}, + "agent_run_history": [], + } + ) + + sidebar = SimpleNamespace( + triage={"verdict": "real", "confidence": 0.82}, + plan={"plan_steps": ["s"], "approved": True}, + ) + captured_tasks: list[asyncio.Task[Any]] = [] + real_create_task = asyncio.create_task + + def _capturing_create_task( + coro: Coroutine[Any, Any, Any], + ) -> asyncio.Task[Any]: + task = real_create_task(coro) + captured_tasks.append(task) + return task + + with ( + patch( + "cliff.api.routes.agent_execution.get_workspace", + return_value=_make_workspace(), + ), + patch( + "cliff.api.routes.agent_execution.get_sidebar", + AsyncMock(return_value=sidebar), + ), + patch( + "cliff.api.routes.agent_execution._resolve_repo_env_vars", + AsyncMock(return_value={}), + ), + patch( + "cliff.api.routes.agent_execution.asyncio.create_task", + _capturing_create_task, + ), + ): + resp = await client.post("/api/workspaces/ws-1/pipeline/run-all") + assert resp.status_code == 202 + assert resp.json()["status"] == "running" + assert captured_tasks, "the gate must let a real verdict proceed" + await captured_tasks[0] + # --------------------------------------------------------------------------- # Suggest-next endpoint diff --git a/cli/cliff_cli/cli.py b/cli/cliff_cli/cli.py index 39317f04..e61e9b3e 100644 --- a/cli/cliff_cli/cli.py +++ b/cli/cliff_cli/cli.py @@ -286,40 +286,111 @@ def issues(client: Client, severity: str, status_filter: str, limit: int) -> Non # ---- 4. fix --------------------------------------------------------------- +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"] + checks = triage.get("checks") or [] + if checks and isinstance(checks[0], dict) and checks[0].get("detail"): + return checks[0]["detail"] + return None + + @main.command() @click.argument("issue_id") -@click.option("--timeout", default=900.0, help="Max seconds to wait for the planner.") +@click.option("--timeout", default=900.0, help="Max seconds to wait for triage + planner.") @_with_client def fix(client: Client, issue_id: str, timeout: float) -> None: - """Open a workspace, run the pipeline through the planner, stop at the plan gate. + """Triage a finding (resolve reachability), then plan only if it's real. - Exits 2 (awaiting human) when the plan is ready for review. Run - ``cliffsec approve `` after the user confirms. + Runs the agentic triage Deep dive to decide whether the flagged code is + actually reachable in your repo, then gates on the verdict (ADR-0051 §6 — + the Plan gate; the planner never fires until reachability is established): + + \b + * real -> builds a remediation plan, exits 2 (awaiting approval). + * unexploitable / + false_positive -> cleared as noise with the reasoning on record, exits 0. + * needs_review -> flagged for your judgment, exits 2. + + After a `real` plan is ready, run ``cliffsec approve ``. """ client.version_handshake() - finding = client.get(f"/api/findings/{issue_id}") + # 1. Triage first. The endpoint creates/reuses a workspace WITHOUT advancing + # the finding's status (it stays ``new`` until a `real` verdict is + # confirmed — ADR-0051 §6) and runs the agentic triage in the background. + started = client.post(f"/api/findings/{issue_id}/triage") + workspace_id = started["workspace_id"] - existing_ws = (finding.get("derived") or {}).get("workspace_id") - if existing_ws: - workspace_id = existing_ws - else: - ws = client.post( - "/api/workspaces", - json={"finding_id": issue_id, "current_focus": "remediation"}, + # 2. Poll for the triage verdict. Tolerate 404: the sidebar row is created + # lazily by the first triage write. + sidebar = poll( + client, + f"/api/workspaces/{workspace_id}/sidebar", + # Wait for the verdict itself, not just a (possibly partial) triage + # object — otherwise an in-flight write could route us to the non-real + # branch before the verdict lands. + is_done=lambda s: bool(((s.get("triage") or {}).get("verdict") or "").strip()), + interval=2.0, + timeout=timeout, + tolerate_status=(404,), + ) + triage = sidebar.get("triage") or {} + verdict = (triage.get("verdict") or "").lower() + reason = _triage_reason(triage) + confidence = triage.get("confidence") + + # 3a. Cleared as noise — the dismiss-with-reasoning verdict IS the output. + # No remediation plan is produced (the report-tour bright line). + if verdict in ("unexploitable", "false_positive"): + emit( + { + "workspace_id": workspace_id, + "finding_id": issue_id, + "verdict": verdict, + "cleared": True, + "reason": reason, + "confidence": confidence, + "awaiting": None, + "next": f"close {workspace_id}", + } + ) + return + + # 3b. Low-signal — flag for human judgment. Never an alarmist plan. + if verdict != "real": + emit( + { + "workspace_id": workspace_id, + "finding_id": issue_id, + "verdict": verdict or "needs_review", + "cleared": False, + "reason": reason, + "confidence": confidence, + "awaiting": "human_review", + "next": None, + }, + exit_code=EXIT_AWAITING_HUMAN, ) - workspace_id = ws["id"] + return - # The pipeline runs its agents in-process via Pydantic AI — no OpenCode - # session to pre-create (the old POST /sessions step was a no-op since - # the substrate migration; ADR-0047). + # 4. Real — build the remediation plan, stop at the approval gate. The + # pipeline runs its agents in-process via Pydantic AI (ADR-0047); the + # triaged workspace already has enricher + exposure, so run-all advances + # straight to evidence + planner. client.post(f"/api/workspaces/{workspace_id}/pipeline/run-all") - # Poll the sidebar until either: - # * a plan exists (awaiting approval), - # * or a validation result already exists (auto-resolved short-circuit). - # Tolerate 404: the sidebar row is created lazily by the first agent - # write, so a 404 right after run-all is normal. + # Poll until a plan exists (awaiting approval) or a validation result is + # already present (auto-resolved short-circuit). def _done(s: dict[str, Any]) -> bool: plan = s.get("plan") or {} validation = s.get("validation") or {} @@ -339,11 +410,14 @@ def _done(s: dict[str, Any]) -> bool: validation = sidebar.get("validation") or {} if validation: - # Pipeline ran end-to-end without a plan gate (e.g. the planner - # decided no work was needed). Treat as auto-resolved. + # A validation result is present (the planner decided no work was + # needed, or — on a re-run of an already-shipped finding — the pipeline + # already ran end to end). Either way it's resolved, not awaiting a + # plan-approval gate. emit( { "workspace_id": workspace_id, + "verdict": "real", "plan": plan, "validation": validation, "awaiting": None, @@ -356,6 +430,7 @@ def _done(s: dict[str, Any]) -> bool: { "workspace_id": workspace_id, "finding_id": issue_id, + "verdict": "real", "plan": { "steps": plan.get("plan_steps") or [], "interim_mitigation": plan.get("interim_mitigation"), diff --git a/cli/tests/test_cli.py b/cli/tests/test_cli.py index c7c22fbb..72011b38 100644 --- a/cli/tests/test_cli.py +++ b/cli/tests/test_cli.py @@ -390,48 +390,55 @@ def test_scan_counts_by_severity(cli, httpx_mock): # --------------------------------------------------------------------------- -# fix — exits 2 awaiting plan approval +# fix — triage-gated: real -> plan (exit 2); noise -> cleared (exit 0); +# needs_review -> human review (exit 2). ADR-0051 §6 Plan gate. # --------------------------------------------------------------------------- -def test_fix_creates_workspace_and_pauses_at_plan(cli, httpx_mock): - _stub_version(httpx_mock) +def _stub_triage_start(httpx_mock, workspace_id="ws-1"): + """``POST /api/findings/{id}/triage`` returns 202 with the workspace id.""" httpx_mock.add_response( - url="http://test-server/api/findings/f1", - json={ - "id": "f1", - "source_type": "scanner", - "source_id": "s1", - "title": "log4j", - "status": "new", - "type": "dependency", - "created_at": "2026-01-01T00:00:00Z", - "updated_at": "2026-01-01T00:00:00Z", - "derived": {"workspace_id": None}, - }, + url="http://test-server/api/findings/f1/triage", + method="POST", + status_code=202, + json={"workspace_id": workspace_id, "status": "running"}, ) + + +def test_fix_real_verdict_pauses_at_plan(cli, httpx_mock): + """A `real` triage verdict proceeds to a remediation plan and stops at the + approval gate (exit 2) — the existing CLI contract for a true positive.""" + _stub_version(httpx_mock) + _stub_triage_start(httpx_mock) + # First poll: triage verdict = real (reachable). httpx_mock.add_response( - url="http://test-server/api/workspaces", - method="POST", - status_code=201, + url="http://test-server/api/workspaces/ws-1/sidebar", json={ - "id": "ws-1", - "finding_id": "f1", - "state": "open", - "created_at": "x", + "workspace_id": "ws-1", + "triage": { + "verdict": "real", + "confidence": 0.82, + "exploitability": { + "exploitable": "yes", + "reason": "Reachable from an internet-facing entrypoint.", + }, + }, "updated_at": "x", }, ) + # The real branch runs the planner pipeline. httpx_mock.add_response( url="http://test-server/api/workspaces/ws-1/pipeline/run-all", method="POST", status_code=202, json={"status": "running", "message": "started"}, ) + # Second poll: plan ready. httpx_mock.add_response( url="http://test-server/api/workspaces/ws-1/sidebar", json={ "workspace_id": "ws-1", + "triage": {"verdict": "real", "confidence": 0.82}, "plan": { "plan_steps": ["update pom.xml", "rebuild"], "interim_mitigation": "Block log4j on WAF", @@ -445,6 +452,7 @@ def test_fix_creates_workspace_and_pauses_at_plan(cli, httpx_mock): assert res.exit_code == 2, res.stderr payload = _last_json(res.stdout) assert payload["workspace_id"] == "ws-1" + assert payload["verdict"] == "real" assert payload["awaiting"] == "plan_approval" assert payload["plan"]["steps"] == ["update pom.xml", "rebuild"] assert payload["plan"]["interim_mitigation"] == "Block log4j on WAF" @@ -456,33 +464,18 @@ def test_fix_creates_workspace_and_pauses_at_plan(cli, httpx_mock): assert payload["next"] == "approve ws-1" -def test_fix_tolerates_initial_404(cli, httpx_mock): - """The sidebar row is created lazily by the first agent write — a 404 - on the very first poll is normal and must not crash the CLI.""" +def test_fix_real_auto_resolves_when_validation_present(cli, httpx_mock): + """A `real` finding whose pipeline already reached a validation result (the + planner decided no work was needed, or a re-run of an already-shipped + finding) is auto-resolved — exit 0, awaiting None — NOT re-sent through the + plan-approval gate (regression guard for the validation branch).""" _stub_version(httpx_mock) + _stub_triage_start(httpx_mock) httpx_mock.add_response( - url="http://test-server/api/findings/f1", - json={ - "id": "f1", - "source_type": "scanner", - "source_id": "s1", - "title": "log4j", - "status": "new", - "type": "dependency", - "created_at": "2026-01-01T00:00:00Z", - "updated_at": "2026-01-01T00:00:00Z", - "derived": {"workspace_id": None}, - }, - ) - httpx_mock.add_response( - url="http://test-server/api/workspaces", - method="POST", - status_code=201, + url="http://test-server/api/workspaces/ws-1/sidebar", json={ - "id": "ws-1", - "finding_id": "f1", - "state": "open", - "created_at": "x", + "workspace_id": "ws-1", + "triage": {"verdict": "real", "confidence": 0.82}, "updated_at": "x", }, ) @@ -492,52 +485,135 @@ def test_fix_tolerates_initial_404(cli, httpx_mock): status_code=202, json={"status": "running"}, ) - # First sidebar poll: 404 (worker hasn't seeded yet) httpx_mock.add_response( url="http://test-server/api/workspaces/ws-1/sidebar", - status_code=404, - json={"detail": "Sidebar state not found"}, + json={ + "workspace_id": "ws-1", + "triage": {"verdict": "real", "confidence": 0.82}, + "validation": {"verdict": "fixed", "recommendation": "close"}, + "updated_at": "x", + }, ) - # Second poll: plan ready + res = cli.invoke(main, ["fix", "f1"]) + assert res.exit_code == 0, res.stderr + payload = _last_json(res.stdout) + assert payload["awaiting"] is None + assert payload["next"] == "close ws-1" + + +@pytest.mark.parametrize("verdict", ["unexploitable", "false_positive"]) +def test_fix_clears_noise_with_reasoning(cli, httpx_mock, verdict): + """A non-real verdict clears the finding as noise WITH the reasoning on + record and NEVER produces a remediation plan (the report-tour bright line: + zero false-positive-shaped alarms). Exits 0 — nothing to fix.""" + _stub_version(httpx_mock) + _stub_triage_start(httpx_mock) httpx_mock.add_response( url="http://test-server/api/workspaces/ws-1/sidebar", json={ "workspace_id": "ws-1", - "plan": {"plan_steps": ["bump"], "approved": False}, - "definition_of_done": {"items": ["tests pass"]}, + "triage": { + "verdict": verdict, + "confidence": 0.88, + "exploitability": { + "exploitable": "no", + "reason": ( + "shell-quote is a dev-only dependency, " + "never imported by shipped code." + ), + }, + "reachability": {"reached": False, "path": [], "summary": "No path found."}, + }, "updated_at": "x", }, ) res = cli.invoke(main, ["fix", "f1"]) - assert res.exit_code == 2, res.stderr + assert res.exit_code == 0, res.stderr payload = _last_json(res.stdout) - assert payload["plan"]["steps"] == ["bump"] + assert payload["workspace_id"] == "ws-1" + assert payload["verdict"] == verdict + assert payload["cleared"] is True + assert "dev-only" in payload["reason"] + assert payload["confidence"] == 0.88 + assert payload["next"] == "close ws-1" + assert "plan" not in payload + # The planner pipeline must never have been invoked for noise. + assert not any( + r.url.path.endswith("/pipeline/run-all") + for r in httpx_mock.get_requests() + ) -def test_fix_timeout_emits_json_error(cli, httpx_mock): - """A polling timeout must surface as a JSON error, not a Python traceback.""" +def test_fix_needs_review_flags_for_human(cli, httpx_mock): + """A `needs_review` verdict is flagged for human judgment (exit 2) — not an + alarmist plan, not a silent clear.""" _stub_version(httpx_mock) + _stub_triage_start(httpx_mock) httpx_mock.add_response( - url="http://test-server/api/findings/f1", + url="http://test-server/api/workspaces/ws-1/sidebar", json={ - "id": "f1", - "source_type": "scanner", - "source_id": "s1", - "title": "x", - "status": "new", - "type": "dependency", - "created_at": "2026-01-01T00:00:00Z", - "updated_at": "2026-01-01T00:00:00Z", - "derived": {"workspace_id": "ws-1"}, + "workspace_id": "ws-1", + "triage": { + "verdict": "needs_review", + "confidence": 0.55, + "exploitability": { + "exploitable": "unknown", + "reason": "A real advisory, but reachability of the sink is undetermined.", + }, + }, + "updated_at": "x", }, ) + res = cli.invoke(main, ["fix", "f1"]) + assert res.exit_code == 2, res.stderr + payload = _last_json(res.stdout) + assert payload["verdict"] == "needs_review" + assert payload["cleared"] is False + assert payload["awaiting"] == "human_review" + assert payload["next"] is None + assert "undetermined" in payload["reason"] + assert not any( + r.url.path.endswith("/pipeline/run-all") + for r in httpx_mock.get_requests() + ) + + +def test_fix_tolerates_initial_404(cli, httpx_mock): + """The sidebar row is created lazily by the first triage write — a 404 on + the very first poll is normal and must not crash the CLI.""" + _stub_version(httpx_mock) + _stub_triage_start(httpx_mock) + # First sidebar poll: 404 (worker hasn't seeded yet) httpx_mock.add_response( - url="http://test-server/api/workspaces/ws-1/pipeline/run-all", - method="POST", - status_code=202, - json={"status": "running"}, + url="http://test-server/api/workspaces/ws-1/sidebar", + status_code=404, + json={"detail": "Sidebar state not found"}, + ) + # Second poll: verdict ready (cleared as noise). + httpx_mock.add_response( + url="http://test-server/api/workspaces/ws-1/sidebar", + json={ + "workspace_id": "ws-1", + "triage": { + "verdict": "unexploitable", + "confidence": 0.88, + "exploitability": {"exploitable": "no", "reason": "Not reachable."}, + }, + "updated_at": "x", + }, ) - # Sidebar with no plan — _done() never returns true, poll() times out. + res = cli.invoke(main, ["fix", "f1"]) + assert res.exit_code == 0, res.stderr + payload = _last_json(res.stdout) + assert payload["verdict"] == "unexploitable" + assert payload["cleared"] is True + + +def test_fix_timeout_emits_json_error(cli, httpx_mock): + """A polling timeout must surface as a JSON error, not a Python traceback.""" + _stub_version(httpx_mock) + _stub_triage_start(httpx_mock) + # Sidebar never gets a triage verdict — is_done never true, poll() times out. httpx_mock.add_response( url="http://test-server/api/workspaces/ws-1/sidebar", is_reusable=True, diff --git a/plugins/cliff-security/skills/cliff-security/SKILL.md b/plugins/cliff-security/skills/cliff-security/SKILL.md index d54a4cfe..d2c653e6 100644 --- a/plugins/cliff-security/skills/cliff-security/SKILL.md +++ b/plugins/cliff-security/skills/cliff-security/SKILL.md @@ -52,8 +52,8 @@ Every `cliffsec` command emits one JSON object on stdout (or stderr on error) an | Code | Meaning | What you do | |---|---|---| -| 0 | Success, no human gate needed | Read `next` field, continue | -| 2 | Awaiting human gate (plan / validation) | Surface details, wait for user | +| 0 | Success, no human gate needed (incl. `fix` clearing a finding as **noise** — `cleared: true`) | Read `next` (+ `verdict`/`reason` for `fix`), continue | +| 2 | Awaiting human gate (plan to approve, `needs_review` triage verdict, or validation) | Surface details, wait for user | | 3 | Daemon unreachable | Run `knowledge/install.md` | | 4 | Version mismatch | Stop, ask user to upgrade | | 5 | Scan completed with zero findings | Tell user the repo is clean, stop | diff --git a/plugins/cliff-security/skills/cliff-security/knowledge/secure-repo.md b/plugins/cliff-security/skills/cliff-security/knowledge/secure-repo.md index 6a86584b..881c0344 100644 --- a/plugins/cliff-security/skills/cliff-security/knowledge/secure-repo.md +++ b/plugins/cliff-security/skills/cliff-security/knowledge/secure-repo.md @@ -71,9 +71,13 @@ If `total > 5`, ask the user which issues to tackle this session (or "all"). Oth cliffsec fix ``` -Exit 2 means the planner is done and the plan is awaiting approval. The JSON contains `plan.steps`, `plan.interim_mitigation`, and `plan.definition_of_done`. Render that to the user as a **short bullet list** and ask for approval. Wait for an explicit "yes". +`fix` **triages first** — it resolves whether the flagged code is actually reachable in the repo — then gates on the verdict (ADR-0051 §6: an issue cannot enter the plan without a `real` verdict). Read the `verdict` field and branch: -Once approved: +- **Exit 0, `cleared: true`** (`verdict: unexploitable` / `false_positive`) — triaged as **noise**: the vulnerable code isn't reachable here. Report the one-line `reason` to the user and move on — there is nothing to fix. (Optionally `cliffsec close ` to dismiss it.) **Do not** invent a fix for a cleared finding. +- **Exit 2, `awaiting: "human_review"`** (`verdict: needs_review`) — reachability couldn't be settled automatically. Surface the `reason` and let the user decide whether it's real; do **not** auto-plan. +- **Exit 2, `awaiting: "plan_approval"`** (`verdict: real`) — the planner ran. The JSON contains `plan.steps`, `plan.interim_mitigation`, and `plan.definition_of_done`. Render that to the user as a **short bullet list** and ask for approval. Wait for an explicit "yes". + +This is the core value: noise is cleared with reasoning, only a `real` finding gets a plan. Once a `real` plan is approved: ```bash cliffsec approve