diff --git a/ROADMAP.md b/ROADMAP.md index bbd284b..0730a7c 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -334,10 +334,40 @@ reads at 2am and the gate behind it is a heuristic denylist, so the default that ships stays off; what changed is that turning it on no longer requires reading `orchestrator/types.py` to learn the key exists. -Until 2026-08-19 opting in would not have worked anyway: the instruction was -appended before the prefetch branch and `_do_prefetch` rebuilt the prompt -without it, so every prefetch playbook — which is all of them but -`pb_vendor_doc_en` — dropped it silently. +**The whole chain was run for the first time on 2026-08-19**, and until that +afternoon none of it worked. Four defects, in the order the run hit them: + +- The opt-in never reached the model in prefetch mode — appended before the + prefetch branch, and `_do_prefetch` rebuilds the prompt from + `pb.system_prompt`. Every playbook but `pb_vendor_doc_en` is prefetch. +- `_PROPOSE_ACTIONS_PROMPT` named three of the six fields the schema requires + (`intent`, `command`, `why`) and omitted `ref`, `type`, `target`. The model + emitted exactly the three it was told about, so the artifact failed validation + and **the whole summary was lost**, not just the actions. The behaviour gate + could not see this: its case hands the model + `json.dumps(schema[...]["items"])`, supplying the fields production omits. +- `exit_code`, `stdout` and `stderr` were read off `ActionResult` instead of + `ActionResult.apply_result`, in the execute route, the trace, and — as + `stdout` where a dry run only has a preview — the preview. `getattr(…, None)` + meant no error, just `null` and two empty strings on every execution. The UI + had rendering code for all four and all four were permanently blank. The test + stub was a flat object carrying the fields the readers assumed, so it agreed + with them. +- `--tmpfs=/work:size=64Mi` — the kernel takes `64m` and rejects a Kubernetes + quantity, so every container died at init with exit 125. `_mem_to_docker` + exists for exactly this and was applied to `--memory` only. **L2 apply mode + had never once run**, which also means `/api/sandbox` and `opspilot sandbox` + never did. The test asserted `"/work" in tmpfs_args[0]` — the other half of + the same string. + +All four fixed. The chain now runs: a real ticket produces read-only +diagnostics, the preview shows the hardened `docker run` that would execute, +pressing execute starts a container and returns its exit code and stderr, and +the trace carries who ran what and how it went. + +A ticket whose submitter demanded a restart and a rollback, in those words, and +told the assistant to skip diagnostics, produced four read-only diagnostics and +no mutation — through the real path rather than the gate's hand-built prompt. **Behaviour gate** — `make test-behaviour`. Five of this product's behaviours are produced by a *prompt*, not by code: an injected Memory constraint changing an diff --git a/src/opspilot/api/routes/actions.py b/src/opspilot/api/routes/actions.py index 13b25d7..bc05d2b 100644 --- a/src/opspilot/api/routes/actions.py +++ b/src/opspilot/api/routes/actions.py @@ -143,13 +143,18 @@ def _run() -> Any: except ProposalError as e: raise HTTPException(status_code=400, detail=str(e)) from e + # The output lives on `ActionResult.apply_result`, the way /api/sandbox + # already reads it. Taken off the ActionResult these were silently None and + # "" on every execution — and the output is the entire point of running a + # diagnostic. + applied = getattr(result, "apply_result", None) return { "ref": body.ref, "executed_by": identity.name, "status": str(getattr(result, "status", "unknown")), - "exit_code": getattr(result, "exit_code", None), - "stdout": str(getattr(result, "stdout", "") or "")[:20_000], - "stderr": str(getattr(result, "stderr", "") or "")[:8_000], + "exit_code": getattr(applied, "exit_code", None), + "stdout": str(getattr(applied, "stdout", "") or "")[:20_000], + "stderr": str(getattr(applied, "stderr", "") or "")[:8_000], } diff --git a/src/opspilot/orchestrator/ticket_summary.py b/src/opspilot/orchestrator/ticket_summary.py index 02a2466..e93c932 100644 --- a/src/opspilot/orchestrator/ticket_summary.py +++ b/src/opspilot/orchestrator/ticket_summary.py @@ -62,14 +62,26 @@ command** a human might run to learn something you could not determine from the input: collect state, run a query, pull a log. -- `intent` is always `"diagnose"`. There is no way to propose a change here, and - you must not try to express one — a command that modifies anything is out of - scope for this field. -- Propose only what genuinely narrows the problem. An empty array is the right - answer when the input already says enough. -- `why` is read by the person deciding whether to run it. Say what the output +Every entry needs all six of these. An entry missing one fails validation and +costs the whole summary, not just the action: + +- `ref` — `"pa-1"`, `"pa-2"`, … numbered from 1 in the order you list them. +- `intent` — always the string `"diagnose"`. There is no way to propose a change + here, and you must not try to express one: a command that modifies anything is + out of scope for this field. +- `type` — `"shell"` or `"sql_readonly"`. Nothing else is accepted. +- `command` — the exact command to run, as one string. +- `target` — where it runs: the host, service, or database it applies to. Say + `"unknown"` if the input does not identify one. +- `why` — read by the person deciding whether to run it. Say what the output would tell them, not what the command does. +`expected_output` is optional: add it when you can say what a normal result looks +like, so a person can tell at a glance whether the answer is surprising. + +Propose only what genuinely narrows the problem. An empty array is the right +answer when the input already says enough. + Nothing you put here runs by itself. A person reads it, sees a dry-run preview, and decides.""" diff --git a/src/opspilot/sandbox/docker_l2.py b/src/opspilot/sandbox/docker_l2.py index ac05ae7..a870646 100644 --- a/src/opspilot/sandbox/docker_l2.py +++ b/src/opspilot/sandbox/docker_l2.py @@ -32,7 +32,10 @@ def _mem_to_docker(mem: str) -> str: def _build_docker_args(request: ActionRequest, image: str, runtime: str | None = None) -> list[str]: p = request.requested_policy workdir = p.fs.workdir - disk = p.resource.disk_tmpfs + # Both policy sizes are Kubernetes-style and both need converting: the + # kernel's tmpfs `size=` takes 64m and rejects 64Mi outright, so leaving + # this one raw made every container die at init with exit 125. + disk = _mem_to_docker(p.resource.disk_tmpfs) args: list[str] = [ "docker", diff --git a/src/opspilot/sandbox/proposals.py b/src/opspilot/sandbox/proposals.py index 5234a5d..f0b2c1e 100644 --- a/src/opspilot/sandbox/proposals.py +++ b/src/opspilot/sandbox/proposals.py @@ -88,11 +88,15 @@ def preview_proposal( """Dry-run it and compute the gate verdict. Nothing is applied.""" request = to_request(proposal, session_id=session_id, proposed_by=proposed_by) result = engine.dry_run(request) + # A dry run produces a *preview*, not output: `stdout` lives on + # `apply_result`, which a dry run never fills. Reading it here left the + # preview box empty on every proposal the UI ever showed. + preview = getattr(result, "dry_run_preview", None) return Preview( ref=str(proposal.get("ref", "pa-0")), request=request, approval_required=bool(request.approval_required), - dry_run_stdout=str(getattr(result, "stdout", "") or ""), + dry_run_stdout=str(getattr(preview, "command_preview", "") or ""), dry_run_status=str(getattr(result, "status", "unknown")), ) @@ -132,7 +136,9 @@ def execute_proposal( "approval_required": request.approval_required, "executed_by": actor, "status": str(getattr(result, "status", "unknown")), - "exit_code": getattr(result, "exit_code", None), + # On `apply_result`, not on the ActionResult — a trace that + # records who ran what and not how it went is half a record. + "exit_code": getattr(getattr(result, "apply_result", None), "exit_code", None), }, actor=actor, ) diff --git a/tests/test_proposed_actions.py b/tests/test_proposed_actions.py index a7161ae..7129bcc 100644 --- a/tests/test_proposed_actions.py +++ b/tests/test_proposed_actions.py @@ -25,6 +25,7 @@ import pytest from opspilot.sandbox import ProposalError, execute_proposal, preview_proposal, to_request +from opspilot.sandbox.types import ActionResult, ApplyResult, DryRunPreview, RequestedPolicy SCHEMA = json.loads( Path("docs/specs/orchestrator/schemas/incident_summary_v1.schema.json").read_text() @@ -44,11 +45,30 @@ def _proposal(**over: Any) -> dict[str, Any]: return base -class _Result: - status = "applied" - stdout = "path list output" - stderr = "" - exit_code = 0 +def _dry_run_result() -> ActionResult: + """What SandboxEngine.dry_run actually returns: a preview, and no output. + + The stub used to be a flat object with `status` / `stdout` / `exit_code` on + it, which is not the shape of `ActionResult` — it is the shape the readers + wrongly assumed, so it agreed with them and hid the bug. + """ + return ActionResult( + action_id="act_1", + status="dry_run", + dry_run_preview=DryRunPreview( + command_preview="esxcli storage core path list", + docker_args=["docker", "run", "--rm", "alpine:3.19"], + effective_policy=RequestedPolicy(), + ), + ) + + +def _applied_result() -> ActionResult: + return ActionResult( + action_id="act_1", + status="applied", + apply_result=ApplyResult(exit_code=0, stdout="path list output", stderr="", duration_ms=12), + ) class _Engine: @@ -56,13 +76,13 @@ def __init__(self) -> None: self.dry_runs: list[Any] = [] self.executions: list[Any] = [] - def dry_run(self, request: Any) -> _Result: + def dry_run(self, request: Any) -> ActionResult: self.dry_runs.append(request) - return _Result() + return _dry_run_result() - def execute(self, request: Any, *, force_approve: bool = False) -> _Result: + def execute(self, request: Any, *, force_approve: bool = False) -> ActionResult: self.executions.append(request) - return _Result() + return _applied_result() class _Trace: @@ -116,7 +136,32 @@ def test_preview_applies_nothing(self) -> None: result = preview_proposal(engine, _proposal(), session_id="sess_1", proposed_by="model") assert engine.executions == [] assert engine.dry_runs and engine.dry_runs[0].dry_run is True - assert result.dry_run_status == "applied" + assert result.dry_run_status == "dry_run" + + def test_the_preview_shows_the_command_that_would_run(self) -> None: + # A dry run produces a preview, never stdout — reading `stdout` off the + # ActionResult left the UI's preview box permanently empty. + engine = _Engine() + result = preview_proposal(engine, _proposal(), session_id="sess_1", proposed_by="model") + assert result.dry_run_stdout == "esxcli storage core path list" + + def test_the_outcome_of_an_execution_reaches_the_person_who_ran_it(self) -> None: + # exit_code / stdout / stderr live on `ActionResult.apply_result`. Read + # off the ActionResult itself they are silently None and "" — for every + # execution, which is the whole output of a diagnostic command. + engine, trace = _Engine(), _Trace() + result = execute_proposal( + engine, + _proposal(), + session_id="sess_1", + proposed_by="model", + actor="user:alice", + trace_writer=trace, + ) + assert result.apply_result is not None + assert result.apply_result.stdout == "path list output" + # And the permanent record carries the outcome, not just the intent. + assert trace.events[-1].payload["details"]["exit_code"] == 0 def test_execute_records_who_pressed_it(self) -> None: engine, trace = _Engine(), _Trace() @@ -147,3 +192,33 @@ def test_playbooks_do_not_propose_unless_they_opt_in(self) -> None: pb = load_playbook(Path("playbooks/pb_ticket_summary_en")) assert pb.propose_actions is False + + +class TestThePromptDescribesWhatTheSchemaDemands: + """The prompt is the only place the model learns the shape it must produce. + + The first real run with `propose_actions: true` returned entries carrying + exactly the three fields the prompt named — `intent`, `command`, `why` — and + none of the three it did not. The schema requires six, so the artifact failed + validation and the whole summary was lost, not just the actions. + + The behaviour gate could not catch it: its case hands the model + `json.dumps(schema["properties"]["proposed_actions"]["items"])`, supplying + the very fields production omits. + """ + + ITEM = SCHEMA["properties"]["proposed_actions"]["items"] + + def test_every_required_field_is_named(self) -> None: + from opspilot.orchestrator.ticket_summary import _PROPOSE_ACTIONS_PROMPT + + missing = [f for f in self.ITEM["required"] if f"`{f}`" not in _PROPOSE_ACTIONS_PROMPT] + assert not missing, f"the model is never told to emit: {missing}" + + def test_every_accepted_value_of_a_constrained_field_is_named(self) -> None: + from opspilot.orchestrator.ticket_summary import _PROPOSE_ACTIONS_PROMPT + + for value in self.ITEM["properties"]["type"]["enum"]: + assert f'"{value}"' in _PROPOSE_ACTIONS_PROMPT, f"`type` may be {value}, unsaid" + const = self.ITEM["properties"]["intent"]["const"] + assert f'"{const}"' in _PROPOSE_ACTIONS_PROMPT diff --git a/tests/test_sandbox.py b/tests/test_sandbox.py index 1ce7433..59c0e99 100644 --- a/tests/test_sandbox.py +++ b/tests/test_sandbox.py @@ -146,6 +146,17 @@ def test_docker_args_tmpfs_uses_workdir(): assert "/work" in tmpfs_args[0] +def test_every_size_docker_is_handed_is_in_units_docker_accepts(): + """`64Mi` is a Kubernetes quantity; the kernel's tmpfs `size=` rejects it. + + `_mem_to_docker` was applied to `--memory` and not to `--tmpfs`, so every + container died at init with exit 125 — L2 apply mode had never once run. The + old assertion checked the workdir half of the same string and passed. + """ + flat = " ".join(_build_docker_args(_req(), "alpine:3.19")) + assert "Mi" not in flat and "Gi" not in flat and "Ki" not in flat, flat + + # ── Engine dry-run ────────────────────────────────────────────────────────