Harden Patrol readiness streaming transport (#1640)

Follow-up to 8d0d74e35. The keepalive mechanism was right, the edges
were not.

1. The evaluation ran on a bare goroutine with no recover, so a panic in
   provider streaming or validation took the whole Pulse process down.
   Before that commit the same panic was on the request goroutine and the
   recovery middleware turned it into a logged 500. The goroutine now
   recovers, logs the panic with its stack, and answers with an ordinary
   readiness result carrying the new internal_error cause and every
   dimension reported as not assessed. A Pulse defect is not a model
   verdict.

2. Headers were only Set, never committed, despite the comment, the
   commit message, and api-contracts.md all claiming otherwise. The
   status line went out with the first keepalive at +10s, so a proxy
   with a sub-10s time-to-first-byte budget still severed the request.
   The transport now writes and flushes WriteHeader(200) before the
   ticker starts, matching the pattern the file already uses for SSE.

3. The flusher was resolved with a discarded ok, so a writer that is not
   an http.Flusher silently buffered the keepalives and degraded back to
   the original bug. It is now checked and logged; the response still
   completes, so a warning is the right level here rather than the hard
   failure the SSE handlers use.

4. TestIssue1640HandlerUsesKeepaliveTransport grepped the handler source
   for substrings, which proves nothing about behaviour. Replaced with a
   real httptest.NewServer test that runs a 300ms evaluation and asserts
   the client sees the 200 and a body byte before the evaluation
   completes, and that the padded body still parses as the expected JSON.
   Added coverage for the panic path and the non-flushable writer, and
   fixed the eager body[:1] that would panic when a transport regression
   left the body empty.

5. The settings readiness banner had no not_assessed branch, so an
   interrupted run still rendered the red "Patrol model not verified"
   headline: the exact blame-the-model presentation the backend fix
   removed. Tone and headline are now exported pure functions with a
   neutral treatment for not_assessed and interrupted results, and an
   interrupted run cannot claim verification from a max_verified_mode
   recorded before the cancellation.

6. createAPIErrorFromResponse let a short plain-text body override an
   explicit caller fallbackMessage. A caller passing a fallback knows
   which operation it was performing; an intermediary writing the body
   does not. Precedence is now canonical JSON, then caller fallback,
   then body, with the HTML and oversize suppression unchanged.

7. patrolRunCancelled classified on the raw "context canceled" substring
   as its first switch case. Ollama embeds that phrase in its own error
   body when it aborts an upstream request, so a genuine provider
   failure on a healthy run was classified interrupted and finish()
   persisted it as not_assessed. Cancellation is now established from
   the run itself (errors.Is(err, context.Canceled), or a cancelled run
   context), never from error wording, and the readiness paths classify
   through a context-aware entry point. context.DeadlineExceeded keeps
   its provider-path timeout classification.

The readiness gate in HandlePatrolModelReadiness keying off ToolProtocol
alone is untouched, as agreed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
courtmanr@gmail.com 2026-07-28 12:16:25 +01:00
parent b45bd66b94
commit c3fb35c8f8
23 changed files with 704 additions and 114 deletions

View file

@ -2154,9 +2154,12 @@ Agent` secondary handoff against the live setup wizard instead of relying
it must not request `agent:exec`, dispatch an agent command, inspect agent
inventory, or turn a successful model probe into command authority. The
advisor's keepalive-streaming response transport changes request pacing
only: keepalive bytes, interrupted-run classification, and preserved
only: keepalive bytes, the up-front status commit, interrupted-run
classification, panic recovery on the evaluation goroutine, and preserved
partial probe evidence grant no agent capability and must not be read as
agent lifecycle signals.
agent lifecycle signals. A readiness result carrying the `interrupted` or
`internal_error` cause is an absence of evidence, so it must never be
treated as an agent-readiness verdict in either direction.
20. Keep Docker container-update proof on the production recreate path. Unit
coverage must include standalone and Compose-shaped host, shared-service,
shared-container, bridge, and custom-network plans, generated and explicit

View file

@ -3940,18 +3940,33 @@ verified independently, while Safe auto-fix and Autopilot remain explicitly
unassessed until deterministic remediation evals exist.
A cancelled run is not provider evidence. The runtime failure classifier
(`internal/ai/patrol_runtime_failure.go`) must classify mid-run cancellation
(`context.Canceled`, whether wrapped or surfaced as provider error text) as
(`internal/ai/patrol_runtime_failure.go`) must classify mid-run cancellation as
`interrupted`, never as a provider connection or analysis fault;
`context.DeadlineExceeded` keeps its provider-path timeout classification. An
interrupted readiness evaluation reports the overall status and every
unfinished dimension and autonomy mode as not assessed while preserving the
per-scenario evidence completed before the interruption, and it must not
overwrite the last completed evaluation in the readiness cache. The readiness
handler streams insignificant JSON-whitespace keepalives while the evaluation
runs so intermediaries with short read timeouts do not sever slow local-model
runs; severed or operator-cancelled runs surface as interrupted, not as model
verdicts (#1640).
`context.DeadlineExceeded` keeps its provider-path timeout classification.
Cancellation is established from the run itself, never from error wording: an
error that wraps `context.Canceled`, or a run whose own context is cancelled.
A raw `context canceled` substring in a provider error body is not evidence of
anything, because providers embed that phrase when they abort an upstream
request of their own while the Pulse run is healthy, and treating it as
cancellation would persist a genuine provider failure as not assessed. Callers
holding the run context must classify through the context-aware entry point so
a torn-down request is not blamed on the provider, and the detail rewrite
follows the same rule as the cause. An interrupted readiness evaluation reports
the overall status and every unfinished dimension and autonomy mode as not
assessed while preserving the per-scenario evidence completed before the
interruption, and it must not overwrite the last completed evaluation in the
readiness cache. The readiness handler streams insignificant JSON-whitespace
keepalives while the evaluation runs so intermediaries with short read timeouts
do not sever slow local-model runs; severed or operator-cancelled runs surface
as interrupted, not as model verdicts (#1640).
A Pulse-side defect is not a model verdict either. A panic recovered on the
readiness evaluation path produces a readiness result with the
`internal_error` cause and every dimension and autonomy mode reported as not
assessed, because nothing about the model was measured. `internal_error`
carries the same non-evidence status as `interrupted`: neither may be
presented, cached, or reasoned about as a statement about the provider or the
selected model.
## Current State

View file

@ -3667,13 +3667,26 @@ can diagnose a failure from the snapshot alone.
The readiness response is a keepalive-padded JSON document. Because a full
advisor run makes multiple sequential provider calls and can legitimately run
for minutes on slow local hardware, the handler commits a 200 status with
`Content-Type: application/json` up front and writes flushed newline
keepalives while the evaluation runs, then appends the ordinary JSON payload.
Leading newlines are insignificant JSON whitespace, so any standard JSON
parser consumes the response unchanged; clients must not depend on the first
response byte being `{`, and evaluation failures stay in-band in the payload
rather than becoming non-200 statuses after the first keepalive (#1640).
for minutes on slow local hardware, the handler writes and flushes the 200
status line with `Content-Type: application/json`, `Cache-Control: no-store`,
and `X-Accel-Buffering: no` before the evaluation starts, not merely before
the first keepalive: an intermediary whose time-to-first-byte budget is
shorter than the keepalive interval must still see the response begin
immediately. It then writes flushed newline keepalives while the evaluation
runs and appends the ordinary JSON payload. Leading newlines are
insignificant JSON whitespace, so any standard JSON parser consumes the
response unchanged; clients must not depend on the first response byte being
`{`, and evaluation failures stay in-band in the payload rather than becoming
non-200 statuses after the first keepalive (#1640).
The evaluation runs on its own goroutine, outside the reach of the server's
per-request panic recovery, so the readiness transport must recover panics
itself and answer with the ordinary 200 readiness payload carrying an
`internal_error` cause. A defect on the evaluation path must never take the
Pulse process down, and it must never be reported as a model or provider
verdict. A response writer that does not implement `http.Flusher` degrades the
keepalives rather than failing the request: the handler logs the degradation
and still completes the response.
Every mobile-facing contract change must update the canonical manifest,
regenerate both repositories, keep the mobile consumer minimum compatible, and

View file

@ -2826,7 +2826,12 @@ When a response body is not canonical JSON at all — a hosted proxy, gateway,
or load-balancer HTML error page — the shared client must not surface the raw
body as the thrown error message. Only short plain-text bodies without markup
may pass through; anything else collapses to a generic status-derived message
so hosted intermediary HTML never reaches tenant-facing UI (#1640).
so hosted intermediary HTML never reaches tenant-facing UI (#1640). Message
precedence on that path is fixed: canonical JSON `error` / `message` first, a
caller-supplied fallback next, and a non-JSON body last. A caller that passes
a fallback knows which tenant-scoped operation it was performing, while a
non-JSON body may have been written by an intermediary that has no idea, so a
body-derived message must never displace an explicit fallback.
That same boundary now also owns stable structured error metadata on the shared
browser client. When backend routes return canonical JSON `code` plus
string-valued `details`, `frontend-modern/src/utils/apiClient.ts` must preserve

View file

@ -1883,7 +1883,16 @@ default` instead of fusing provider and badge text such as
operator's unsaved dropdown selection rather than whatever was
previously saved, and must surface a stale-cache warning when the
form's selection differs from the cached result's model so the
green badge cannot silently mislead)
green badge cannot silently mislead). The readiness banner's tone and
headline are pure functions exported from
`AIModelSelectionSection.tsx` so this presentation is provable without
mounting the settings shell, and a result that was never assessed —
`status: not_assessed`, or the `interrupted` cause left by an operator
cancel or a severed request — must render in a neutral "check did not
complete" treatment. It must not use the failure treatment or the
"model not verified" headline, because a run that measured nothing is
not a verdict on the model, and it must not claim verification from a
`max_verified_mode` recorded before the interruption (#1640)
2. Keep top-level settings surfaces routed through the canonical settings shell
and maintain both `frontend-modern/src/components/Settings/__tests__/settingsArchitecture.test.ts`
plus `tests/integration/tests/15-settings-shell-consistency.spec.ts`

View file

@ -4798,6 +4798,7 @@
"test_prefixes": [],
"exact_files": [
"frontend-modern/src/components/Settings/__tests__/dataHandlingPanelModel.test.ts",
"frontend-modern/src/components/Settings/__tests__/patrolReadinessBanner.issue1640.test.ts",
"frontend-modern/src/components/Settings/__tests__/settingsArchitecture.test.ts",
"frontend-modern/src/components/Settings/__tests__/useAuditLogPanelState.test.tsx"
]

View file

@ -1972,8 +1972,10 @@ pressure to assess context selection, but it must not read live backup records,
invoke recovery APIs, mutate storage, or treat a synthetic pass as evidence
that Safe auto-fix or Autopilot remediation is verified. An interrupted
readiness run keeps the same isolation: preserved partial scenario evidence is
synthetic probe output only, and the keepalive response transport introduces no
storage or recovery side channel.
synthetic probe output only, and the keepalive response transport, its up-front
status commit, and its panic recovery introduce no storage or recovery side
channel. A result recovered from a panic reports every dimension as not
assessed and carries no storage or backup state at all.
26. Storage row presentation resolves its topology label from
`storage.vdevLayout` first and falls back to `storage.topology`, so a