ld-1 dev-route 1: plain words, honest freshness, hardened reader #856

Merged
pi-landing merged 13 commits from ld-1/dev-route-1 into dev/cockpit 2026-11-05 21:40:58 +00:00
Collaborator

First dev-route feature (INTENT:2), from what Aaron saw on the live page at 13:22Z.

What changes: plain fail-closed wording in Waiting (no internal language); the sign-in failure is a plain page with a sign-in link instead of bare JSON, and login starts are logged without values; the header shows a recognizable identity (userinfo read at login when the token has no name) and never the numeric account id; the version chip never says dev; Fleet/Gates/Chat tabs and the STOP button are off the page, reversibly, until they can act; the snapshot reader degrades per field instead of blanking the page on one bad field; long texts pass untruncated; snapshot-check CLI for the publisher; freshness: no green badge over stale data, one last-updated per row; Sendrast status text is never silently dropped; published_at in the future reads as stale; all-clear only when the list state is nothing.

Proof: full suite green on this exact head in the sandbox (go vet, go test -race, svelte-check 0/0, vitest 432+11skip, build, e2e 33 passed / 52 skipped in a real browser — 52 skips are the unwired tabs/STOP/attach blocks, ceiling written in the gate). Gate proof posts separately from the base tip.

Pre-reviewed by two independent seats (no blockers; findings fixed in T-F1). Reviewer verdicts land on this head next.

First dev-route feature (INTENT:2), from what Aaron saw on the live page at 13:22Z. What changes: plain fail-closed wording in Waiting (no internal language); the sign-in failure is a plain page with a sign-in link instead of bare JSON, and login starts are logged without values; the header shows a recognizable identity (userinfo read at login when the token has no name) and never the numeric account id; the version chip never says dev; Fleet/Gates/Chat tabs and the STOP button are off the page, reversibly, until they can act; the snapshot reader degrades per field instead of blanking the page on one bad field; long texts pass untruncated; snapshot-check CLI for the publisher; freshness: no green badge over stale data, one last-updated per row; Sendrast status text is never silently dropped; published_at in the future reads as stale; all-clear only when the list state is nothing. Proof: full suite green on this exact head in the sandbox (go vet, go test -race, svelte-check 0/0, vitest 432+11skip, build, e2e 33 passed / 52 skipped in a real browser — 52 skips are the unwired tabs/STOP/attach blocks, ceiling written in the gate). Gate proof posts separately from the base tip. Pre-reviewed by two independent seats (no blockers; findings fixed in T-F1). Reviewer verdicts land on this head next.
Collaborator

Verdict of record — ld-reviewer — head fa43bfbccf: REQUEST_CHANGES

model: omniroute/deepseek-flash
session: 01a1ad7f-53a7-7383-8335-5a5cadcde178

What I reviewed

The whole diff origin/dev/cockpit..origin/ld-1/dev-route-1 at the exact head, read independently of the two earlier seats: auth/loginpage.go, auth/oidc.go, auth/session.go, cmd/cockpit/main.go, cmd/snapshot-check/main.go, programs/snapshot.go, programs/snapshot_file.go, server/{attach,gates,server}.go (callers), and the web side: plain-language.ts, format.ts, programs-pane, dashboard-pane, identity-chip, app-header, panes.svelte.ts, +layout.svelte, plus the Go/vitest/e2e tests and fixtures. I ran, in a detached worktree at this head: go test ./auth/... ./programs/... ./cmd/snapshot-check/... → all ok. I did not re-run vitest/e2e (no node_modules in my worktree); the PR's gate numbers are taken as reported.

Findings

F1 — blocker — lab-director/cockpit/web/src/lib/programs/plain-language.ts:502 (with the branch it makes unreachable at :513)

A stale document whose decision list is empty — i.e. "nothing was waiting the last time the publisher wrote" — is now shown as "not connected yet" with the sentence "This screen can't read your list right now — an empty list here would mean something is missing, not that nothing waits on you." The file was read; it is only old. That is a false cause on the operator's main screen, and it contradicts the amber chip and the rows directly above it ("last updated 39m ago, may be out of date").

The wire really does carry that case as waiting.state = "listed", waiting.stale = true:

  • programs/snapshot.go:849-857 — a stale empty list is deliberately kept as WaitingListed (its own comment: "Stale and empty … the list is shown as it is (empty) and marked");
  • programs/source.go:304-307 — ViewerPolicy.For keeps {State: listed, Stale: true} for the viewer's own list;
  • so the new early return at plain-language.ts:502 fires before the staleness branch at :513, and that branch — the one that says "was read but is more than half an hour old" — has become unreachable for any payload the API can produce: the API never emits {nothing, stale:true} (snapshot.go:850 requires !stale for nothing; source.go:304 forces listed when stale).

So this is a regression from the base (origin/dev/cockpit reached the stale branch for exactly this shape) and it breaks the pane's own documented rule 4 and the reader's rule at snapshot.go:853-857. The direction is still fail-closed — no all-clear is painted — but the stated cause is wrong, which is the class of dishonesty this PR exists to remove: the most common freshness failure (publisher stopped while nothing was waiting) sends the operator to look for a missing/unreadable status file instead of a stale one.

Why the tests missed it: the fixtures model the impossible shape — fixtures.ts:230 waiting('nothing', true) (same in programs-pane.test.ts:500), and plain-language.test.ts:236 asserts 'listed' + empty ⇒ not-connected, which is right for an incomplete list and wrong for a stale one.

Fix (small): decide waiting?.stale with an empty own list before the state !== 'nothing' check (empty + stale → the existing 'stale' branch), leaving the not-connected branch to the incomplete/unknown states; and make programsStale() carry waiting('listed', true) so the fixture is the shape the API actually sends.

Nits (not blocking)

  • N1 (nit) — auth/loginpage.go:74: "Sign in again" goes to bare /auth/login, dropping the next the operator was on; the 401 path keeps it (middleware.go:274). One url.QueryEscape away.
  • N2 (nit) — cmd/snapshot-check/main.go:60-69: the checker runs only ParseSnapshot, so a published_at more than MaxClockSkew in the future exits 0 — a document the reader then marks stale (noteFutureStamp). The one writer-clock bug the reader now distrusts is not caught by the publisher's pre-flight.
  • N3 (nit) — server/attach.go:91: DisplayName no longer falls back to the subject, so attach.Caller.Display is empty for a subject-only session (the Zitadel default this PR is about); the attach banner/ledger would print a blank display. Latent (attach is off the page) and the ledger keys on Subject, so no correctness or security impact.
  • N4 (nit) — web/src/routes/unauthorized/+page.svelte:22: the 403 page still paints the numeric subject. Defensible (it is the key an operator needs to grant the role), but it is the last number on screen if "never the numeric account id" is meant globally — worth recording as the deliberate exception.
  • N5 (nit) — auth/oidc.go:315-323: a login that could not start renders err.Error() (entropy/sealing internals) in the page detail. Same content as the old JSON and an exceptional path, but it is internal wording on a human page.
  • N6 (nit) — plain-language.ts:589-600: a stale chip now carries the whole sentence ("last updated 39m ago, may be out of date") while the waiting block prints the same sentence; the chip used to say only "may be out of date". Cosmetic duplication.
  • N7 (nit) — programs/snapshot.go:591-595: a status carried with state not-connected is dropped with no Problem, while every other dropped field is recorded — the one silent loss left.
  • N8 (nit) — programs/snapshot_file.go:78: NewFileSnapshot overwrites the package-level SnapshotProblemSink; fine with one entrypoint, but it turns a package var into a per-call side effect.

Checked and clean

  • Security. The IdP's error_description reaches the page only through html/template's contextual escaping (loginpage.go:36-96), proven by auth_test.go:928 and the real-browser e2e (e2e/auth.spec.ts:377); /api/ and explicit JSON Accept still get Problem JSON (wantsJSON, middleware.go:324); the login-start line carries only dev + request_id — no nonce/state/sealed cookie/next (TestLoginStartLogsNoValues).
  • Userinfo cannot grant anything. nameFromUserInfo writes only Name, Email, NamedByUserInfo; roles still come from the verified token's project-roles claim (identityFromClaims untouched), HasRole is exact, no roles ⇒ no operator; a userinfo answer about another subject is refused.
  • RBAC stays fail-closed on subject. /api asks on Subject; ViewerPolicy.For still keys on the mapped subject; no identity / unmapped / uncollected ⇒ decisions: null and "not connected yet".
  • Per-field degradation cannot paint a false all-clear. A dropped decision empties CollectedOwners (snapshot.go:432-441), so For answers not-connected; nothing is earned only with !stale && decisionsComplete; decisions absent/null/non-list is refused as form; collected_owners holes fail closed with a Problem.
  • Freshness. Future published_at beyond MaxClockSkew reads stale and is named once per published document (TestSnapshotFuturePublishedAtIsStaleAndNamed); the chip cannot stay green beside a stale waiting answer; one "last updated" per row (the warning line stops repeating it).
  • Intent fit. The version chip never says "dev" (format.ts:100-118); header and dashboard never paint the subject; STOP + banner + the three tabs are off with exact one-line restores documented (app-header.svelte, +layout.svelte, panes.svelte.ts); long texts pass verbatim (TestSnapshotLongTextIsCarriedWhole). The 52 skipped e2e specs are not a finding: each skip names its reason and its one-line reactivation.

Only F1 needs a change before this lands.

**Verdict of record — ld-reviewer — head fa43bfbccf133a78916588b722358467ab78223a: REQUEST_CHANGES** model: omniroute/deepseek-flash session: 01a1ad7f-53a7-7383-8335-5a5cadcde178 ## What I reviewed The whole diff `origin/dev/cockpit..origin/ld-1/dev-route-1` at the exact head, read independently of the two earlier seats: `auth/loginpage.go`, `auth/oidc.go`, `auth/session.go`, `cmd/cockpit/main.go`, `cmd/snapshot-check/main.go`, `programs/snapshot.go`, `programs/snapshot_file.go`, `server/{attach,gates,server}.go` (callers), and the web side: `plain-language.ts`, `format.ts`, `programs-pane`, `dashboard-pane`, `identity-chip`, `app-header`, `panes.svelte.ts`, `+layout.svelte`, plus the Go/vitest/e2e tests and fixtures. I ran, in a detached worktree at this head: `go test ./auth/... ./programs/... ./cmd/snapshot-check/...` → all ok. I did not re-run vitest/e2e (no node_modules in my worktree); the PR's gate numbers are taken as reported. ## Findings **F1 — blocker — lab-director/cockpit/web/src/lib/programs/plain-language.ts:502 (with the branch it makes unreachable at :513)** A stale document whose decision list is empty — i.e. "nothing was waiting the last time the publisher wrote" — is now shown as **"not connected yet"** with the sentence *"This screen can't read your list right now — an empty list here would mean something is missing, not that nothing waits on you."* The file *was* read; it is only old. That is a false cause on the operator's main screen, and it contradicts the amber chip and the rows directly above it ("last updated 39m ago, may be out of date"). The wire really does carry that case as `waiting.state = "listed", waiting.stale = true`: - `programs/snapshot.go:849-857` — a stale empty list is deliberately kept as `WaitingListed` (its own comment: "Stale and empty … the list is shown as it is (empty) and marked"); - `programs/source.go:304-307` — `ViewerPolicy.For` keeps `{State: listed, Stale: true}` for the viewer's own list; - so the new early return at `plain-language.ts:502` fires **before** the staleness branch at `:513`, and that branch — the one that says "was read but is more than half an hour old" — has become unreachable for any payload the API can produce: the API never emits `{nothing, stale:true}` (`snapshot.go:850` requires `!stale` for `nothing`; `source.go:304` forces `listed` when stale). So this is a regression from the base (`origin/dev/cockpit` reached the stale branch for exactly this shape) and it breaks the pane's own documented rule 4 and the reader's rule at `snapshot.go:853-857`. The direction is still fail-closed — no all-clear is painted — but the stated cause is wrong, which is the class of dishonesty this PR exists to remove: the most common freshness failure (publisher stopped while nothing was waiting) sends the operator to look for a missing/unreadable status file instead of a stale one. Why the tests missed it: the fixtures model the impossible shape — `fixtures.ts:230` `waiting('nothing', true)` (same in `programs-pane.test.ts:500`), and `plain-language.test.ts:236` asserts `'listed'` + empty ⇒ not-connected, which is right for an *incomplete* list and wrong for a *stale* one. Fix (small): decide `waiting?.stale` with an empty own list **before** the `state !== 'nothing'` check (empty + stale → the existing `'stale'` branch), leaving the not-connected branch to the incomplete/unknown states; and make `programsStale()` carry `waiting('listed', true)` so the fixture is the shape the API actually sends. ## Nits (not blocking) - **N1 (nit) — auth/loginpage.go:74**: "Sign in again" goes to bare `/auth/login`, dropping the `next` the operator was on; the 401 path keeps it (`middleware.go:274`). One `url.QueryEscape` away. - **N2 (nit) — cmd/snapshot-check/main.go:60-69**: the checker runs only `ParseSnapshot`, so a `published_at` more than `MaxClockSkew` in the future exits 0 — a document the reader then marks stale (`noteFutureStamp`). The one writer-clock bug the reader now distrusts is not caught by the publisher's pre-flight. - **N3 (nit) — server/attach.go:91**: `DisplayName` no longer falls back to the subject, so `attach.Caller.Display` is empty for a subject-only session (the Zitadel default this PR is about); the attach banner/ledger would print a blank display. Latent (attach is off the page) and the ledger keys on `Subject`, so no correctness or security impact. - **N4 (nit) — web/src/routes/unauthorized/+page.svelte:22**: the 403 page still paints the numeric subject. Defensible (it is the key an operator needs to grant the role), but it is the last number on screen if "never the numeric account id" is meant globally — worth recording as the deliberate exception. - **N5 (nit) — auth/oidc.go:315-323**: a login that could not start renders `err.Error()` (entropy/sealing internals) in the page detail. Same content as the old JSON and an exceptional path, but it is internal wording on a human page. - **N6 (nit) — plain-language.ts:589-600**: a stale chip now carries the whole sentence ("last updated 39m ago, may be out of date") while the waiting block prints the same sentence; the chip used to say only "may be out of date". Cosmetic duplication. - **N7 (nit) — programs/snapshot.go:591-595**: a `status` carried with state `not-connected` is dropped with no Problem, while every other dropped field is recorded — the one silent loss left. - **N8 (nit) — programs/snapshot_file.go:78**: `NewFileSnapshot` overwrites the package-level `SnapshotProblemSink`; fine with one entrypoint, but it turns a package var into a per-call side effect. ## Checked and clean - **Security.** The IdP's `error_description` reaches the page only through `html/template`'s contextual escaping (`loginpage.go:36-96`), proven by `auth_test.go:928` and the real-browser e2e (`e2e/auth.spec.ts:377`); `/api/` and explicit JSON Accept still get `Problem` JSON (`wantsJSON`, `middleware.go:324`); the login-start line carries only `dev` + `request_id` — no nonce/state/sealed cookie/`next` (TestLoginStartLogsNoValues). - **Userinfo cannot grant anything.** `nameFromUserInfo` writes only `Name`, `Email`, `NamedByUserInfo`; roles still come from the verified token's project-roles claim (`identityFromClaims` untouched), `HasRole` is exact, no roles ⇒ no operator; a userinfo answer about another subject is refused. - **RBAC stays fail-closed on subject.** `/api` asks on `Subject`; `ViewerPolicy.For` still keys on the mapped subject; no identity / unmapped / uncollected ⇒ `decisions: null` and "not connected yet". - **Per-field degradation cannot paint a false all-clear.** A dropped decision empties `CollectedOwners` (`snapshot.go:432-441`), so `For` answers not-connected; `nothing` is earned only with `!stale && decisionsComplete`; `decisions` absent/null/non-list is refused as form; `collected_owners` holes fail closed with a Problem. - **Freshness.** Future `published_at` beyond `MaxClockSkew` reads stale and is named once per published document (TestSnapshotFuturePublishedAtIsStaleAndNamed); the chip cannot stay green beside a stale waiting answer; one "last updated" per row (the warning line stops repeating it). - **Intent fit.** The version chip never says "dev" (`format.ts:100-118`); header and dashboard never paint the subject; STOP + banner + the three tabs are off with exact one-line restores documented (`app-header.svelte`, `+layout.svelte`, `panes.svelte.ts`); long texts pass verbatim (TestSnapshotLongTextIsCarriedWhole). The 52 skipped e2e specs are not a finding: each skip names its reason and its one-line reactivation. Only F1 needs a change before this lands.
T-F3: stale + lista vacía pinta stale, no not-connected (resuelve F1 del asiento R1)
All checks were successful
sandbox-proof sandbox gate ok head=2c736a40; checks: vintage,verify,scope,go-vet,go-test,web-sync,web-check,web-test,web-build,e2e,proof.
2c736a406d
Collaborator

Verdict of record — ld-reviewer — head 2c736a406d: APPROVE

model: omniroute/deepseek-flash
session: 01a1ad98-c58e-71e0-a816-6fd2a1183b80

What I reviewed

The whole diff origin/dev/cockpit..origin/ld-1/dev-route-1 at the exact head (git rev-parse origin/ld-1/dev-route-1 = 2c736a40…, 0 commits of drift), read in a detached worktree of the existing checkout (no credentialled clone): auth/{loginpage,oidc,session}.go, cmd/cockpit/main.go, cmd/snapshot-check/, programs/{snapshot,snapshot_file}.go and their callers, and the web side (plain-language.ts, format.ts, programs-pane, dashboard-pane, identity-chip, app-header, panes.svelte.ts, +layout.svelte, fixtures) plus the T-F1/T-F2/T-F3 deltas and the tests.

Execution on this head:

  • go test ./programs/ ./auth/ ./server/ ./cmd/snapshot-check/ → all ok (the package suites include TestCallbackFailurePageEscapesTheIdPDescription, TestLoginStartLogsNoValues, TestSnapshotFuturePublishedAtIsStaleAndNamed, TestSnapshotEmptyDecisionsIsNothingWaitingOnlyWhenFresh, TestSnapshotBadDecisionIsLeftOutWithItsCause, TestSnapshotDegradationCauseIsLoggedOncePerPublishedDocument).
  • vitest full suite: 438 passed / 11 skipped (the deliberately skipped unwired-tab suites); svelte-check 0 errors / 0 warnings.
  • My own scratch tests (deleted afterwards) for R1's F1, run against the built code:
    • waiting {listed, stale:true} + empty list → state:'stale', headline "may be out of date", detail "…was read…", never NOT_CONNECTED / NOTHING_WAITING; same for an unknown future state with stale; component render of that payload → data-waiting-state="stale" and the amber stale line, plus "was read", minus "not connected yet";
    • {nothing, stale:true} → stale; {nothing, fresh} → the all-clear; {listed|not-connected|unknown, fresh} + empty → not-connected; no waiting answer → not-connected; unidentified viewer → not-connected in every variant. So the all-clear still requires state === 'nothing' and freshness.
  • I also read the producer side rather than trusting the fixture: programs/snapshot.go Response emits listed + Stale:true for an old empty list (nothing only when !stale && decisionsComplete), and ViewerPolicy.For keeps {listed, stale:true} for a collected owner. The fixture change to waiting('listed', true) is therefore the shape the API really sends, and the fix is coherent end-to-end (wire → plainWaiting → pane), not just a reordered branch.

Security:

  • The failure page is an html/template with {{.}}; the IdP's error_description is rendered as text (&lt;script&gt;…), no <script>/<link>/<img>, text/html; charset=utf-8, Cache-Control: no-store, body rendered into a buffer before any byte goes out, and the JSON Problem is kept only for wantsJSON clients. Inline <style> is authorised by the deployed CSP (style-src 'self' 'unsafe-inline').
  • login start logs only dev + request_id; the test asserts the state-cookie value, "nonce", "sealed", roles, subject and /gates are absent from the line.
  • userinfo is display-only: nameFromUserInfo writes Name/Email/NamedByUserInfo and nothing else — Roles and OperatorRole are untouched, a userinfo sub that differs from the verified token is refused, the call is 5s-bounded and never fails the login. RBAC still keys on Identity.Subject (server/gates.go principalOf starts at Subject; programs.OwnerForSubject compares subjects only), and the numeric subject is no longer painted (identity chip + dashboard strip, with the name === subject guard for a backend that repeats a number in name).
  • Per-field degradation stays fail-closed: an item left out sets decisionsComplete=false, empties collected_owners and blocks WaitingNothing — asserted through ViewerPolicy.For for the owner whose item was dropped.
  • Freshness: a published_at beyond MaxClockSkew in the future reads stale and is named as a Problem; no green chip over stale rows (snapshotChip(rows, {waiting}) plus the row-derived state); one "last updated" per row.

Findings

No blocking findings. F1 from the R1 seat is fixed at this head, and I verified the fix by execution (stale + empty → stale warning; all-clear still only from state:'nothing'; incomplete/unknown fresh lists still not-connected).

Nits

  • N1 — programs/snapshot.go:482,490,494: a decision whose optional field is unusable ("area": 7, recommendation: 12) is dropped from the list whole, while the tier-2 doc at :57-60 lists only id/owner/program/question/asked_at as item-costing, and readSendrast treats a bad optional field as omitted-not-fatal. The direction is safe (list becomes incomplete → nobody collected → no all-clear), but a real decision disappears from the tray for a cosmetic writer bug. Either omit the field with a Problem, or state in the doc that any unusable field costs the item.
  • N2 — programs/snapshot.go:591: state: "not-connected" carrying epic/status drops them with no Problem — the one degraded field not named, while every other one is. Given the intent's "Sendrast status never silently dropped", that contradiction deserves a cause.
  • N3 — web/src/lib/components/programs/programs-pane.svelte:171,214: a stale row with no measurement for it renders only "may be out of date" in the standing slot (program-standing-none) and loses the amber line, because dated and the trailing {#if (view || sendrastView) && line} key off a view that does not exist (confirmed by rendering that payload). Not false; "no status was read for it, may be out of date" would be plainer.
  • N4 — web/src/lib/format.ts:115: only the exact word "dev" is rejected, so a stamp like dev-1a2b still renders as vdev-1a2b; /^dev\b/i (or a whitelist of tag shapes) closes it.
  • N5 (inherited from R1, still open) — cmd/snapshot-check/main.go: it runs only ParseSnapshot, so a published_at far in the future prints nothing and exits 0 although the reader will show the document stale. Calling Response(now) on the parsed snapshot would make the checker answer the page's question.
  • N6 — programs/snapshot_file.go NewFileSnapshot assigns the package-global SnapshotProblemSink from a constructor. Documented and fine for one reader per process, but it makes two readers share one sink and tests order-dependent; pass the sink per reader when that seam next moves.

Reversibility holds: tabs (PANE_ORDER), STOP (import + mount commented with the restore line) and their suites (describe.skip) each carry the one-line note, and the panels stay mounted so nothing else has to change with them. Intent fit: no numeric account id, no ticket/hash/pod names, "written status file" instead of "snapshot this build does not read yet", long texts pass whole.

I did not re-run the Playwright e2e (no browser in my worktree); the gate's e2e numbers are taken as reported.

APPROVE at 2c736a406d.

**Verdict of record — ld-reviewer — head 2c736a406d3bd8b78907446d232f6c53d3da66a5: APPROVE** model: omniroute/deepseek-flash session: 01a1ad98-c58e-71e0-a816-6fd2a1183b80 ## What I reviewed The whole diff `origin/dev/cockpit..origin/ld-1/dev-route-1` at the exact head (`git rev-parse origin/ld-1/dev-route-1` = 2c736a40…, 0 commits of drift), read in a detached worktree of the existing checkout (no credentialled clone): `auth/{loginpage,oidc,session}.go`, `cmd/cockpit/main.go`, `cmd/snapshot-check/`, `programs/{snapshot,snapshot_file}.go` and their callers, and the web side (`plain-language.ts`, `format.ts`, `programs-pane`, `dashboard-pane`, `identity-chip`, `app-header`, `panes.svelte.ts`, `+layout.svelte`, fixtures) plus the T-F1/T-F2/T-F3 deltas and the tests. Execution on this head: - `go test ./programs/ ./auth/ ./server/ ./cmd/snapshot-check/` → all ok (the package suites include TestCallbackFailurePageEscapesTheIdPDescription, TestLoginStartLogsNoValues, TestSnapshotFuturePublishedAtIsStaleAndNamed, TestSnapshotEmptyDecisionsIsNothingWaitingOnlyWhenFresh, TestSnapshotBadDecisionIsLeftOutWithItsCause, TestSnapshotDegradationCauseIsLoggedOncePerPublishedDocument). - vitest full suite: 438 passed / 11 skipped (the deliberately skipped unwired-tab suites); `svelte-check` 0 errors / 0 warnings. - My own scratch tests (deleted afterwards) for R1's F1, run against the built code: - `waiting {listed, stale:true}` + empty list → `state:'stale'`, headline "may be out of date", detail "…was read…", never NOT_CONNECTED / NOTHING_WAITING; same for an unknown future state with `stale`; component render of that payload → `data-waiting-state="stale"` and the amber stale line, plus "was read", minus "not connected yet"; - `{nothing, stale:true}` → stale; `{nothing, fresh}` → the all-clear; `{listed|not-connected|unknown, fresh}` + empty → not-connected; no waiting answer → not-connected; unidentified viewer → not-connected in every variant. So the all-clear still requires `state === 'nothing'` and freshness. - I also read the producer side rather than trusting the fixture: `programs/snapshot.go` Response emits `listed` + `Stale:true` for an old empty list (`nothing` only when `!stale && decisionsComplete`), and `ViewerPolicy.For` keeps `{listed, stale:true}` for a collected owner. The fixture change to `waiting('listed', true)` is therefore the shape the API really sends, and the fix is coherent end-to-end (wire → `plainWaiting` → pane), not just a reordered branch. Security: - The failure page is an `html/template` with `{{.}}`; the IdP's `error_description` is rendered as text (`&lt;script&gt;…`), no `<script>/<link>/<img>`, `text/html; charset=utf-8`, `Cache-Control: no-store`, body rendered into a buffer before any byte goes out, and the JSON Problem is kept only for `wantsJSON` clients. Inline `<style>` is authorised by the deployed CSP (`style-src 'self' 'unsafe-inline'`). - `login start` logs only `dev` + `request_id`; the test asserts the state-cookie value, "nonce", "sealed", roles, subject and `/gates` are absent from the line. - userinfo is display-only: `nameFromUserInfo` writes Name/Email/NamedByUserInfo and nothing else — Roles and OperatorRole are untouched, a userinfo `sub` that differs from the verified token is refused, the call is 5s-bounded and never fails the login. RBAC still keys on `Identity.Subject` (`server/gates.go` principalOf starts at Subject; `programs.OwnerForSubject` compares subjects only), and the numeric subject is no longer painted (identity chip + dashboard strip, with the `name === subject` guard for a backend that repeats a number in `name`). - Per-field degradation stays fail-closed: an item left out sets `decisionsComplete=false`, empties `collected_owners` and blocks `WaitingNothing` — asserted through `ViewerPolicy.For` for the owner whose item was dropped. - Freshness: a `published_at` beyond `MaxClockSkew` in the future reads stale and is named as a Problem; no green chip over stale rows (`snapshotChip(rows, {waiting})` plus the row-derived state); one "last updated" per row. ## Findings No blocking findings. F1 from the R1 seat is fixed at this head, and I verified the fix by execution (stale + empty → stale warning; all-clear still only from `state:'nothing'`; incomplete/unknown fresh lists still not-connected). ## Nits - N1 — `programs/snapshot.go:482,490,494`: a decision whose *optional* field is unusable (`"area": 7`, `recommendation: 12`) is dropped from the list whole, while the tier-2 doc at :57-60 lists only id/owner/program/question/asked_at as item-costing, and `readSendrast` treats a bad optional field as omitted-not-fatal. The direction is safe (list becomes incomplete → nobody collected → no all-clear), but a real decision disappears from the tray for a cosmetic writer bug. Either omit the field with a Problem, or state in the doc that any unusable field costs the item. - N2 — `programs/snapshot.go:591`: `state: "not-connected"` carrying `epic`/`status` drops them with no Problem — the one degraded field not named, while every other one is. Given the intent's "Sendrast status never silently dropped", that contradiction deserves a cause. - N3 — `web/src/lib/components/programs/programs-pane.svelte:171,214`: a stale row with no measurement for it renders only "may be out of date" in the standing slot (`program-standing-none`) and loses the amber line, because `dated` and the trailing `{#if (view || sendrastView) && line}` key off a view that does not exist (confirmed by rendering that payload). Not false; "no status was read for it, may be out of date" would be plainer. - N4 — `web/src/lib/format.ts:115`: only the exact word "dev" is rejected, so a stamp like `dev-1a2b` still renders as `vdev-1a2b`; `/^dev\b/i` (or a whitelist of tag shapes) closes it. - N5 (inherited from R1, still open) — `cmd/snapshot-check/main.go`: it runs only `ParseSnapshot`, so a `published_at` far in the future prints nothing and exits 0 although the reader will show the document stale. Calling `Response(now)` on the parsed snapshot would make the checker answer the page's question. - N6 — `programs/snapshot_file.go` `NewFileSnapshot` assigns the package-global `SnapshotProblemSink` from a constructor. Documented and fine for one reader per process, but it makes two readers share one sink and tests order-dependent; pass the sink per reader when that seam next moves. Reversibility holds: tabs (`PANE_ORDER`), STOP (import + mount commented with the restore line) and their suites (`describe.skip`) each carry the one-line note, and the panels stay mounted so nothing else has to change with them. Intent fit: no numeric account id, no ticket/hash/pod names, "written status file" instead of "snapshot this build does not read yet", long texts pass whole. I did not re-run the Playwright e2e (no browser in my worktree); the gate's e2e numbers are taken as reported. **APPROVE** at 2c736a406d3bd8b78907446d232f6c53d3da66a5.
pi-landing merged commit 4c4892b968 into dev/cockpit 2026-11-05 21:40:58 +00:00
Sign in to join this conversation.
No reviewers
No labels
operator-review
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
Homelab/infra-core!856
No description provided.