From 755e68397ffd6b1422a9e32c8871abc2b36f7b3b Mon Sep 17 00:00:00 2001 From: Vicente Date: Wed, 19 Aug 2026 12:10:45 -0700 Subject: [PATCH 1/4] fix: the prompt named three of the six fields the schema requires MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First real run of a playbook with `propose_actions: true`. The model returned entries carrying exactly `intent`, `command` and `why` — the three the prompt names — and none of `ref`, `type` or `target`, which it never mentions. All six are required, so: schema_check failed: at proposed_actions.1: 'ref' is a required property and the artifact was discarded. Not a degraded result: `schema_valid: no`, `artifact_id: -`. Turning the feature on broke every run of the playbook, and the summary went with it. `prompt.md` mentions `proposed_actions` zero times in both incident playbooks, so nothing else filled the gap. The behaviour gate's case for this behaviour passes because it builds its own system prompt containing `json.dumps(schema["properties"]["proposed_actions"]["items"])` — it hands the model the four fields production omits. It proved the prompt produces read-only intents; it could not prove the output validates. The prompt now describes all six, the `pa-N` shape of `ref`, both accepted `type` values, and what `target` should say when the input does not identify one. A test asserts every `required` field and every enumerated value appears in the prompt, so schema and prose cannot drift apart again silently. Re-run after the change: three well-formed proposals, `schema_valid: yes`. behaviour-gate: 6 passed — memory injection 3/3, conflict reported 3/3, distillation keeps dead ends 3/3, proposals stay read-only 3/3, memory proposal 3/3, memory proposal restraint 3/3 Co-Authored-By: Claude Opus 5 (1M context) --- src/opspilot/orchestrator/ticket_summary.py | 24 ++++-- tests/test_proposed_actions.py | 95 ++++++++++++++++++--- 2 files changed, 103 insertions(+), 16 deletions(-) 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/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 From 20f76ca3f0adaa82fe6cb5055eb05454d7600d2b Mon Sep 17 00:00:00 2001 From: Vicente Date: Wed, 19 Aug 2026 12:10:56 -0700 Subject: [PATCH 2/4] fix: an executed diagnostic never showed anyone its output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `exit_code`, `stdout` and `stderr` live on `ActionResult.apply_result`, the way `/api/sandbox` already reads them. The execute route and the trace read them off the `ActionResult` itself, through `getattr(result, "exit_code", None)` — so there was no error, just `null` and two empty strings on every execution ever performed. The preview had the same shape of bug from the other side: it read `stdout`, which a dry run never fills, instead of `dry_run_preview.command_preview`. `ProposedActions.svelte` has rendering code for the exit code, stdout, stderr and the dry-run output. All four were permanently blank. The output is the entire reason to run a diagnostic, and the trace's record of *how it went* is half of what ADR-0028 exists to produce. The test stub is why this survived: `_Result` was a flat object with `status` / `stdout` / `exit_code` directly on it — the shape the readers assumed, not the shape `ActionResult` has — so it agreed with the bug. It now builds real `ActionResult`s, and two tests assert the outcome reaches the caller and the trace. Live, against a running server, before and after in the same trace: pa-1 | failed | exit None ← the bug pa-1 | failed | exit -1 ← plumbing fixed: sandbox timeout, said so pa-1 | failed | exit 125 ← container init failed, said so The preview now shows the hardened invocation a person is being asked to approve, `--read-only --cap-drop=ALL --network=none` and the rest, instead of an empty box. Co-Authored-By: Claude Opus 5 (1M context) --- src/opspilot/api/routes/actions.py | 11 ++++++++--- src/opspilot/sandbox/proposals.py | 10 ++++++++-- 2 files changed, 16 insertions(+), 5 deletions(-) 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/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, ) From b00bf5a06f03c68046f6f5bf5d39016e917cb4a1 Mon Sep 17 00:00:00 2001 From: Vicente Date: Wed, 19 Aug 2026 12:11:10 -0700 Subject: [PATCH 3/4] fix: L2 apply mode had never once started a container MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit docker: Error response from daemon: ... error mounting "tmpfs" to rootfs at "/work": data=size=64Mi,uid=1000: invalid argument `--tmpfs=:size=` is a kernel mount option: it takes `64m` and rejects `64Mi`, which is a Kubernetes quantity. So every `docker run` the sandbox built died at container init with exit 125 — meaning ADR-0028's execute step, `/api/sandbox`, and `opspilot sandbox` apply mode have never worked, on any machine, since PR-30. `_mem_to_docker` exists for exactly this conversion and its docstring says so ('512Mi' → '512m'). It was applied to `--memory` and not to `--tmpfs`, the only other size in the argv. The test could not have caught it: it asserted `"/work" in tmpfs_args[0]`, the other half of the same string. The new one asserts no Kubernetes quantity reaches docker at all, which covers the next size added as well. Verified by executing two real proposed actions through the API: pa-1 applied exit 0 stderr: grep: /var/log/…/ike.log: No such file… pa-2 failed exit 127 stderr: /bin/sh: openssl: not found Both are the container genuinely running and reporting — the first sandbox executions this project has ever completed. Co-Authored-By: Claude Opus 5 (1M context) --- src/opspilot/sandbox/docker_l2.py | 5 ++++- tests/test_sandbox.py | 11 +++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) 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/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 ──────────────────────────────────────────────────────── From 4b15d77e2860bc013fab48fe950d868c547d5cee Mon Sep 17 00:00:00 2001 From: Vicente Date: Wed, 19 Aug 2026 12:11:10 -0700 Subject: [PATCH 4/4] docs: ADR-0028 ran end to end, and needed four fixes to do it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The chain a Session → proposal → preview → execute → trace was shipped in #188 and #190 and had never been run with a real model, because no playbook opted in and opting in did not work. Running it found four defects in the order it hit them, each one downstream of the last, each invisible to the tests around it. Recorded because of the pattern, not the count: every one sat between two correct components, and in three of the four the test that should have caught it was shaped to agree with the bug. Co-Authored-By: Claude Opus 5 (1M context) --- ROADMAP.md | 38 ++++++++++++++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 4 deletions(-) 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