ld-1 dev-route 1: plain words, honest freshness, hardened reader #856
No reviewers
Labels
No labels
operator-review
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Homelab/infra-core!856
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "ld-1/dev-route-1"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.
Verdict of record — ld-reviewer — head
fa43bfbccf: REQUEST_CHANGESmodel: omniroute/deepseek-flash
session: 01a1ad7f-53a7-7383-8335-5a5cadcde178
What I reviewed
The whole diff
origin/dev/cockpit..origin/ld-1/dev-route-1at 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 asWaitingListed(its own comment: "Stale and empty … the list is shown as it is (empty) and marked");programs/source.go:304-307—ViewerPolicy.Forkeeps{State: listed, Stale: true}for the viewer's own list;plain-language.ts:502fires 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:850requires!stalefornothing;source.go:304forceslistedwhen stale).So this is a regression from the base (
origin/dev/cockpitreached the stale branch for exactly this shape) and it breaks the pane's own documented rule 4 and the reader's rule atsnapshot.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:230waiting('nothing', true)(same inprograms-pane.test.ts:500), andplain-language.test.ts:236asserts'listed'+ empty ⇒ not-connected, which is right for an incomplete list and wrong for a stale one.Fix (small): decide
waiting?.stalewith an empty own list before thestate !== 'nothing'check (empty + stale → the existing'stale'branch), leaving the not-connected branch to the incomplete/unknown states; and makeprogramsStale()carrywaiting('listed', true)so the fixture is the shape the API actually sends.Nits (not blocking)
/auth/login, dropping thenextthe operator was on; the 401 path keeps it (middleware.go:274). Oneurl.QueryEscapeaway.ParseSnapshot, so apublished_atmore thanMaxClockSkewin 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.DisplayNameno longer falls back to the subject, soattach.Caller.Displayis 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 onSubject, so no correctness or security impact.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.statuscarried with statenot-connectedis dropped with no Problem, while every other dropped field is recorded — the one silent loss left.NewFileSnapshotoverwrites the package-levelSnapshotProblemSink; fine with one entrypoint, but it turns a package var into a per-call side effect.Checked and clean
error_descriptionreaches the page only throughhtml/template's contextual escaping (loginpage.go:36-96), proven byauth_test.go:928and the real-browser e2e (e2e/auth.spec.ts:377);/api/and explicit JSON Accept still getProblemJSON (wantsJSON,middleware.go:324); the login-start line carries onlydev+request_id— no nonce/state/sealed cookie/next(TestLoginStartLogsNoValues).nameFromUserInfowrites onlyName,Email,NamedByUserInfo; roles still come from the verified token's project-roles claim (identityFromClaimsuntouched),HasRoleis exact, no roles ⇒ no operator; a userinfo answer about another subject is refused./apiasks onSubject;ViewerPolicy.Forstill keys on the mapped subject; no identity / unmapped / uncollected ⇒decisions: nulland "not connected yet".CollectedOwners(snapshot.go:432-441), soForanswers not-connected;nothingis earned only with!stale && decisionsComplete;decisionsabsent/null/non-list is refused as form;collected_ownersholes fail closed with a Problem.published_atbeyondMaxClockSkewreads 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).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
2c736a406d: APPROVEmodel: omniroute/deepseek-flash
session: 01a1ad98-c58e-71e0-a816-6fd2a1183b80
What I reviewed
The whole diff
origin/dev/cockpit..origin/ld-1/dev-route-1at 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}.goand 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).svelte-check0 errors / 0 warnings.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 withstale; 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 requiresstate === 'nothing'and freshness.programs/snapshot.goResponse emitslisted+Stale:truefor an old empty list (nothingonly when!stale && decisionsComplete), andViewerPolicy.Forkeeps{listed, stale:true}for a collected owner. The fixture change towaiting('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:
html/templatewith{{.}}; the IdP'serror_descriptionis rendered as text (<script>…), 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 forwantsJSONclients. Inline<style>is authorised by the deployed CSP (style-src 'self' 'unsafe-inline').login startlogs onlydev+request_id; the test asserts the state-cookie value, "nonce", "sealed", roles, subject and/gatesare absent from the line.nameFromUserInfowrites Name/Email/NamedByUserInfo and nothing else — Roles and OperatorRole are untouched, a userinfosubthat differs from the verified token is refused, the call is 5s-bounded and never fails the login. RBAC still keys onIdentity.Subject(server/gates.goprincipalOf starts at Subject;programs.OwnerForSubjectcompares subjects only), and the numeric subject is no longer painted (identity chip + dashboard strip, with thename === subjectguard for a backend that repeats a number inname).decisionsComplete=false, emptiescollected_ownersand blocksWaitingNothing— asserted throughViewerPolicy.Forfor the owner whose item was dropped.published_atbeyondMaxClockSkewin 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
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, andreadSendrasttreats 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.programs/snapshot.go:591:state: "not-connected"carryingepic/statusdrops 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.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, becausedatedand 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.web/src/lib/format.ts:115: only the exact word "dev" is rejected, so a stamp likedev-1a2bstill renders asvdev-1a2b;/^dev\b/i(or a whitelist of tag shapes) closes it.cmd/snapshot-check/main.go: it runs onlyParseSnapshot, so apublished_atfar in the future prints nothing and exits 0 although the reader will show the document stale. CallingResponse(now)on the parsed snapshot would make the checker answer the page's question.programs/snapshot_file.goNewFileSnapshotassigns the package-globalSnapshotProblemSinkfrom 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.