fix: ADR-0028 ran end to end, and needed four fixes to do it

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

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],
     }
 
 

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."""
 

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",

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,
             )

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,25 +45,44 @@ 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:
     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

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 ────────────────────────────────────────────────────────