dev/cockpit → live: first dev-route feature (plain words, honest freshness, hardened reader) #857

Merged

pi-landing merged 14 commits from dev/cockpit into live 2026-11-05 22:38:43 +00:00

Conversation 4 Commits 14 Files changed 36 +4191 -628

pi-landing commented 2026-11-05 21:43:30 +00:00

Collaborator

Copy link

Promotion of the first dev-route feature to live (INTENT:2). What the operator sees after this lands: plain fail-closed wording in the Waiting block; a plain sign-in failure page with a sign-in link (no more bare JSON), with login starts logged without values; a header that shows a recognizable identity and never the numeric account id; a version chip that never says dev; Fleet/Gates/Chat tabs and the STOP button off the page until they can act (one-line reverts); the Programs reader that degrades per field instead of blanking the page, carries long texts whole, never silently drops a Sendrast status, and treats a future published_at as stale; an all-clear only when the list state is nothing; snapshot-check CLI for the publisher's pre-flight.

Sandbox proof on the exact head by the dev-route gate (from the base's own copy): sandbox-proof success on 2c736a40 — go vet, go test -race, svelte-check, vitest via the gate's own JSON report, fresh build, e2e in a real browser with the Programs and sign-in-failure specs required. One reviewer seat found a real regression (stale empty list shown with a false cause), fixed and verified by a fresh seat on this head. CI runs here on live by the operator's order.

Promotion of the first dev-route feature to live (INTENT:2). What the operator sees after this lands: plain fail-closed wording in the Waiting block; a plain sign-in failure page with a sign-in link (no more bare JSON), with login starts logged without values; a header that shows a recognizable identity and never the numeric account id; a version chip that never says dev; Fleet/Gates/Chat tabs and the STOP button off the page until they can act (one-line reverts); the Programs reader that degrades per field instead of blanking the page, carries long texts whole, never silently drops a Sendrast status, and treats a future published_at as stale; an all-clear only when the list state is nothing; snapshot-check CLI for the publisher's pre-flight. Sandbox proof on the exact head by the dev-route gate (from the base's own copy): sandbox-proof success on 2c736a40 — go vet, go test -race, svelte-check, vitest via the gate's own JSON report, fresh build, e2e in a real browser with the Programs and sign-in-failure specs required. One reviewer seat found a real regression (stale empty list shown with a false cause), fixed and verified by a fresh seat on this head. CI runs here on live by the operator's order.

pi-landing added 14 commits 2026-11-05 21:43:30 +00:00

T-D2: página llana de fallo de login + log de inicios sin valores bb4b040695

T-D1: tabs Fleet/Gates/Chat fuera hasta cableados (PANE_ORDER reversible) 05d701bc2c

T-D3a: header reconocible; el chip de versión nunca dice dev 7139bc8f28

T-D7: STOP fuera del header hasta que pueda actuar (reversible) 6d80f59a27

T-D6: reader degrada por campo; test de forma v1.1 con texto largo 1b2caa8f88

T-D45: wording llano en Waiting, items below, frescura honesta 82608cab95

T-D8: snapshot-check — parser real por stdin, falla nombrando el campo sin valores 1ad58c7e0a

T-D6b: causas de degradación al logger de la app b8c5cd0b9c

T-D3b: userinfo en login; el header nunca muestra el id numérico b5fd95cf39

T-D3c: name_source incluye userinfo en el tipo web 80362860e7

T-F2: e2e de la página de fallo de sign-in en browser real d862b81b23

T-F1: all-clear solo con state nothing; published_at futuro = stale; status de Sendrast nunca se pierde; dashboard sin subject; userinfo con timeout; Waiting sin causa falsa fa43bfbccf

T-F3: stale + lista vacía pinta stale, no not-connected (resuelve F1 del asiento R1) 2c736a406d

Merge pull request 'ld-1 dev-route 1: plain words, honest freshness, hardened reader' (#856) from ld-1/dev-route-1 into dev/cockpit 4c4892b968

ld-reviewer commented 2026-11-05 22:30:09 +00:00

Collaborator

Copy link

Verdict of record — ld-reviewer — head 4c4892b968: APPROVE

model: omniroute/deepseek-flash
session: 01a1adce-4c18-73e0-bb86-00d2177332df

What I reviewed

The whole cockpit change going to live, at the exact head: git diff fb8598c9..4c4892b9 (merge-base three-dot; the only non-cockpit files in origin/live..head are live's own newer gits-sbx gate work, and a local dry-run merge of this head into origin/live keeps scripts/gits-sbx-epic-test.sh untouched and merges clean — this PR does not revert the gate script). I read auth/{loginpage.go,oidc.go,session.go,roles.go,middleware.go}, programs/{snapshot.go,snapshot_file.go,source.go,types.go}, cmd/snapshot-check, cmd/cockpit/main.go, and the shell/programs web changes plus their tests and e2e.

I also re-ran the proof myself on the head in a clean worktree, not on trust: go vet ./... clean; go test ./auth/... ./programs/... ./cmd/... ok; npm ci + vitest run 432 passed / 11 skipped (37 files); svelte-check 0 errors 0 warnings.

Security

• IdP error_description is escaped. loginFailureTemplate is an html/template, {{.}} renders as text, no <script>, no external resource (the inline <style> is authorized by the production CSP's style-src 'self' 'unsafe-inline'; verified in server/middleware.go). TestCallbackFailurePageEscapesTheIdPDescription is a real test: it asserts the payload is absent and that <script>… is visible to the reader.
• Login-start log carries no values. handleLogin logs only dev + request_id; the test binds a JSON logger and asserts the sealed state cookie's own value, nonce, sealed, subject, roles and next do not appear.
• The userinfo read cannot grant privileges. identityFromClaims runs first and the sealed Identity (Subject, Roles) is built from the verified ID token only; nameFromUserInfo adopts only Name/Email/NamedByUserInfo, refuses a sub that differs from the token's, and fails open to an unnamed header (never to a failed login). Email is consumed nowhere except display and principalOf's fallback after Subject. Timeout is bounded (userInfoTimeout, 5s) and tested against a hanging endpoint.
• RBAC stays fail-closed on the subject claim. Unchanged: sub required at callback and at decodeIdentity; OwnerForSubject compares exactly; unmapped/no-identity yields decisions: null and waiting.state: not-connected. The display chain's changes are display-only.

Correctness and wire-shape cross-check

I walked the shapes programs/snapshot.go/source.go can actually emit against every branch of plain-language.ts; no mismatch found:

emitted by Go	UI branch	result
not-connected + decisions:null	ownListRead === false	"not connected yet", no all-clear
mapped+collected, empty own list, fresh	state:"nothing"	all-clear (only this path)
mapped+collected, empty own list, stale	state:"listed", stale:true → stale branch before the nothing test	amber "may be out of date" (the R1 F1 regression, fixed at both ends)
item dropped (decisionsComplete=false)	CollectedOwners emptied → not-connected	no all-clear
unmapped / uncollected viewer with a real list	state:"not-connected", decisions filtered	no all-clear; sentence claims no cause
future published_at (>5m)	StaleAt true on every catalog row + waiting.stale	chip amber, no green; cause recorded once per document

Server side has no false all-clear path: the verdict requires len(own)==0 AND mapped AND collected AND !stale, and the parser empties CollectedOwners whenever it had to leave an item out. The client independently requires state === "nothing", so neither end can grant it alone. Long text is carried verbatim (requiredText never trims into emptiness or truncates; TestSnapshotLongTextIsCarriedWhole), a Sendrast status with no epic is kept and named as a Problem, and the chip cannot stay green when the same file's waiting answer dates it old.

Findings (none blocking)

• F1 — minor, copy on the failure page. auth/loginpage.go:73 hardcodes "Reloading this page does not retry the sign-in." The template is also used by loginStartFailure (auth/oidc.go:315), which is served at /auth/login — there, reloading does start a new login, and the "Sign in again" link points at that same URL. Only reachable on an entropy/seal failure, but the sentence is the wrong one on that path.
• F2 — nit, doc drift. web/src/lib/programs/plain-language.ts:29 still promises a missing file says "a written status file that hasn't arrived" — no string says that any more, and the new tests explicitly forbid "hasn't arrived"/"didn't arrive" because it claims a cause. The module header now contradicts its own behaviour.
• F3 — nit, copy. identity-chip.svelte:54 tooltip says "signed in via the identity provider" for every named session, including a dev-mode one whose identity comes from X-Cockpit-Dev-User (the dev badge beside it says exactly the opposite). Cannot ship on live (dev is false there), but it is the component whose rule is "never say something untrue".
• F4 — nit, surprise at distance. programs/snapshot_file.go:78 makes the constructor assign the package-global SnapshotProblemSink. Documented, and the CLI resets it, but a constructor with a process-wide side effect means a second reader silently wins; a sink field on the reader would be local.
• F5 — nit, formatting. identity-chip.test.ts:35 has two fixture fields on one line (name_source: 'claim-name', then tab then roles:), which looks like a botched edit. No prettier gate, so it survives; harmless.
• F6 — observation, not this change. web/src/routes/unauthorized/+page.svelte:47-48 still paints the raw numeric subject. It is not the header and predates this PR, but it now sits oddly beside the new "no numeric account id" rule — worth a follow-up seat.
• F7 — observation. programs/snapshot.go:612: a sendrast block with state:"ok" but no usable needed_aaron collapses the whole block to "not connected yet", dropping an epic/status the document did write. It is documented as a required measurement and the cause is recorded (never silent), so I am not asking for a change — just noting that "not connected yet" then describes a source the document says is connected.
• F8 — observation, pre-existing. formatAgo renders a future instant as "just now", so a milestone at ahead of the clock reads "last updated just now"; document-level freshness now treats a future published_at as stale, per-row instants do not. Cannot produce a green-over-stale chip (the chip follows the document verdict), but the row sentence is softer than the rule.

On the skipped e2e blocks

The fleet/gates/chat/attach/telemetry/STOP blocks are describe.skipped, not deleted, each with the reason and the one-line reactivation, and the panels stay mounted (pane count 4 asserted). That is defensible for an unreachable tab, and the removals are genuinely one-line reversible (PANE_ORDER, StopControl import+mount, StopBanner import+mount). The honest consequence: while those tabs are off, a browser regression in those panes is untested — so re-wiring a pane must un-skip its block in the same change.

Tests

They prove behaviour rather than restate it: the escape test asserts both absence of the marker and visibility of the escaped text; TestLoginStartLogsNoValues extracts the actual state cookie value and greps for it; plainWaiting has an iterated regression test across listed/error/unknown states for the all-clear and for the stale-empty fix; TestSnapshotEmptyDecisionsIsNothingWaitingOnlyWhenFresh proves the server half of the same rule; the chip test opens the portal tooltip before asserting the subject is nowhere (so the number cannot hide one hover away); the dashboard/chip tests cover name-repeated-as-subject; snapshot-check fixtures carry a sentinel word in an untouched field to prove no value is ever printed.

No blocking findings. Approving.

**Verdict of record — ld-reviewer — head 4c4892b968eba624901247b8caf2c2e5327bee51: APPROVE** model: omniroute/deepseek-flash session: 01a1adce-4c18-73e0-bb86-00d2177332df ## What I reviewed The whole cockpit change going to live, at the exact head: `git diff fb8598c9..4c4892b9` (merge-base three-dot; the only non-cockpit files in `origin/live..head` are live's own newer gits-sbx gate work, and a local dry-run merge of this head into `origin/live` keeps `scripts/gits-sbx-epic-test.sh` untouched and merges clean — this PR does not revert the gate script). I read `auth/{loginpage.go,oidc.go,session.go,roles.go,middleware.go}`, `programs/{snapshot.go,snapshot_file.go,source.go,types.go}`, `cmd/snapshot-check`, `cmd/cockpit/main.go`, and the shell/programs web changes plus their tests and e2e. I also re-ran the proof myself on the head in a clean worktree, not on trust: `go vet ./...` clean; `go test ./auth/... ./programs/... ./cmd/...` ok; `npm ci` + `vitest run` 432 passed / 11 skipped (37 files); `svelte-check` 0 errors 0 warnings. ## Security - **IdP `error_description` is escaped.** `loginFailureTemplate` is an `html/template`, `{{.}}` renders as text, no `<script>`, no external resource (the inline `<style>` is authorized by the production CSP's `style-src 'self' 'unsafe-inline'`; verified in `server/middleware.go`). `TestCallbackFailurePageEscapesTheIdPDescription` is a real test: it asserts the payload is absent *and* that `&lt;script&gt;…` is visible to the reader. - **Login-start log carries no values.** `handleLogin` logs only `dev` + `request_id`; the test binds a JSON logger and asserts the sealed state cookie's own value, `nonce`, `sealed`, `subject`, `roles` and `next` do not appear. - **The userinfo read cannot grant privileges.** `identityFromClaims` runs first and the sealed `Identity` (Subject, Roles) is built from the verified ID token only; `nameFromUserInfo` adopts *only* `Name`/`Email`/`NamedByUserInfo`, refuses a `sub` that differs from the token's, and fails open to an unnamed header (never to a failed login). Email is consumed nowhere except display and `principalOf`'s fallback *after* `Subject`. Timeout is bounded (`userInfoTimeout`, 5s) and tested against a hanging endpoint. - **RBAC stays fail-closed on the subject claim.** Unchanged: `sub` required at callback and at `decodeIdentity`; `OwnerForSubject` compares exactly; unmapped/no-identity yields `decisions: null` and `waiting.state: not-connected`. The display chain's changes are display-only. ## Correctness and wire-shape cross-check I walked the shapes `programs/snapshot.go`/`source.go` can actually emit against every branch of `plain-language.ts`; no mismatch found: | emitted by Go | UI branch | result | |---|---|---| | `not-connected` + `decisions:null` | `ownListRead === false` | "not connected yet", no all-clear | | mapped+collected, empty own list, fresh | `state:"nothing"` | all-clear (only this path) | | mapped+collected, empty own list, **stale** | `state:"listed"`, `stale:true` → stale branch *before* the `nothing` test | amber "may be out of date" (the R1 F1 regression, fixed at both ends) | | item dropped (`decisionsComplete=false`) | `CollectedOwners` emptied → `not-connected` | no all-clear | | unmapped / uncollected viewer with a real list | `state:"not-connected"`, `decisions` filtered | no all-clear; sentence claims no cause | | future `published_at` (>5m) | `StaleAt` true on every catalog row + `waiting.stale` | chip amber, no green; cause recorded once per document | Server side has no false all-clear path: the verdict requires `len(own)==0 AND mapped AND collected AND !stale`, and the parser empties `CollectedOwners` whenever it had to leave an item out. The client independently requires `state === "nothing"`, so neither end can grant it alone. Long text is carried verbatim (`requiredText` never trims into emptiness or truncates; `TestSnapshotLongTextIsCarriedWhole`), a Sendrast status with no epic is kept and *named* as a Problem, and the chip cannot stay green when the same file's waiting answer dates it old. ## Findings (none blocking) - **F1 — minor, copy on the failure page.** `auth/loginpage.go:73` hardcodes "Reloading this page does not retry the sign-in." The template is also used by `loginStartFailure` (`auth/oidc.go:315`), which is served at `/auth/login` — there, reloading *does* start a new login, and the "Sign in again" link points at that same URL. Only reachable on an entropy/seal failure, but the sentence is the wrong one on that path. - **F2 — nit, doc drift.** `web/src/lib/programs/plain-language.ts:29` still promises a missing file says "a written status file that hasn't arrived" — no string says that any more, and the new tests explicitly forbid "hasn't arrived"/"didn't arrive" because it claims a cause. The module header now contradicts its own behaviour. - **F3 — nit, copy.** `identity-chip.svelte:54` tooltip says "signed in via the identity provider" for every named session, including a dev-mode one whose identity comes from `X-Cockpit-Dev-User` (the dev badge beside it says exactly the opposite). Cannot ship on live (`dev` is false there), but it is the component whose rule is "never say something untrue". - **F4 — nit, surprise at distance.** `programs/snapshot_file.go:78` makes the constructor assign the package-global `SnapshotProblemSink`. Documented, and the CLI resets it, but a constructor with a process-wide side effect means a second reader silently wins; a sink field on the reader would be local. - **F5 — nit, formatting.** `identity-chip.test.ts:35` has two fixture fields on one line (`name_source: 'claim-name',` then tab then `roles:`), which looks like a botched edit. No prettier gate, so it survives; harmless. - **F6 — observation, not this change.** `web/src/routes/unauthorized/+page.svelte:47-48` still paints the raw numeric `subject`. It is not the header and predates this PR, but it now sits oddly beside the new "no numeric account id" rule — worth a follow-up seat. - **F7 — observation.** `programs/snapshot.go:612`: a `sendrast` block with `state:"ok"` but no usable `needed_aaron` collapses the whole block to "not connected yet", dropping an epic/status the document did write. It is documented as a required measurement and the cause is recorded (never silent), so I am not asking for a change — just noting that "not connected yet" then describes a source the document says *is* connected. - **F8 — observation, pre-existing.** `formatAgo` renders a future instant as "just now", so a milestone `at` ahead of the clock reads "last updated just now"; document-level freshness now treats a future `published_at` as stale, per-row instants do not. Cannot produce a green-over-stale chip (the chip follows the document verdict), but the row sentence is softer than the rule. ## On the skipped e2e blocks The fleet/gates/chat/attach/telemetry/STOP blocks are `describe.skip`ped, not deleted, each with the reason and the one-line reactivation, and the panels stay mounted (`pane` count 4 asserted). That is defensible for an unreachable tab, and the removals are genuinely one-line reversible (`PANE_ORDER`, `StopControl` import+mount, `StopBanner` import+mount). The honest consequence: while those tabs are off, a browser regression in those panes is untested — so re-wiring a pane must un-skip its block in the same change. ## Tests They prove behaviour rather than restate it: the escape test asserts both absence of the marker and visibility of the escaped text; `TestLoginStartLogsNoValues` extracts the actual state cookie value and greps for it; `plainWaiting` has an iterated regression test across `listed`/`error`/unknown states for the all-clear and for the stale-empty fix; `TestSnapshotEmptyDecisionsIsNothingWaitingOnlyWhenFresh` proves the server half of the same rule; the chip test opens the portal tooltip before asserting the subject is nowhere (so the number cannot hide one hover away); the dashboard/chip tests cover name-repeated-as-subject; `snapshot-check` fixtures carry a sentinel word in an untouched field to prove no value is ever printed. No blocking findings. Approving.

ld-reviewer commented 2026-11-05 22:30:45 +00:00

Collaborator

Copy link

Verdict of record — ld-reviewer — head 4c4892b968: APPROVE

model: omniroute/deepseek-flash
session: 01a1adce-4c25-77d0-a478-0c6dc8744fb0

What I reviewed

The full change going to live: git diff origin/live..4c4892b9, read as a promotion, not as a PR diff. I checked the merge effect separately, because the two-way diff shows the sbx-gits work as a deletion: git merge-tree origin/live 4c4892b9 reports no conflicts, and dev/cockpit touches only lab-director/** since the merge-base fb8598c9; the gits-sbx/examples scripts therefore stay at live's versions (PR #853) and are not reverted by this merge.

Local verification on the exact head (worktree at 4c4892b9, not the shared checkout):

• go vet ./... clean; go test ./... all packages ok; go test -race ./auth/... ./programs/... ./cmd/snapshot-check/... ok (auth 7.1s).
• npm ci && npx vitest run: 432 passed | 11 skipped (443) — exactly the gate's GATE_VITEST_MIN_TOTAL=443, and the fixture↔wire parity guard ran.
• npm run check (svelte-check): 0 errors, 0 warnings.
• npx playwright test --list --reporter=json: 85 tests, 50 skipped, 35 expected → above the gate floor (>=33) and under its ceiling (<=52), and the four required titles (three Programs + sign-in failure) are all passed, not skipped.

Security (checked, holds)

• Escaping: auth/loginpage.go is an html/template with the detail only in an element context; TestCallbackFailurePageEscapesTheIdPDescription asserts the <script> payload appears as <script>…, and the page carries no script and no external resource. auth/loginpage.go:44-96.
• CSP fit: the inline <style> is authorised by the deployed policy (server/middleware.go:165, style-src 'self' 'unsafe-inline', hashed script-src), so the page is not a blank document in production.
• No values in the login-start log: auth/oidc.go:98-104 logs only dev + request_id; TestLoginStartLogsNoValues greps for the state-cookie value, nonce, sealed cookie, roles, subject and next in the log.
• Userinfo grants nothing: auth/oidc.go:333-380 (nameFromUserInfo) only writes Name/Email/NamedByUserInfo; roles come from the verified ID token's project-roles claim (auth/roles.go:62-72), and a userinfo sub that differs from the token's is refused. The tests drive a real discovery+JWKS+token+userinfo provider (fakeIdP), not a stub, and assert userinfoHits == 1 and "not asked when the token named somebody".
• RBAC unchanged and fail-closed: RequireOperator → resolveIdentity/HasRole untouched (auth/middleware.go:163-180), decodeIdentity still refuses a session without a subject (auth/roles.go:102-104). DisplayName losing the subject as a display value is presentation only; Subject survives in the sealed identity (asserted).
• No credential or token touched by the CLI: snapshot-check reads stdin, prints field paths only, and a no-value contract is enforced with SENTINEL fixtures.

Correctness (checked against the wire shapes, not just the tests)

I re-derived the states from programs/snapshot.go + programs/source.go and compared with the branches in web/src/lib/programs/plain-language.ts, and probed the reader directly:

• false all-clear is unreachable: empty+fresh list earns nothing only with collected_owners naming the viewer and a complete list; a dropped item empties CollectedOwners → not-connected; a fresh empty list nobody vouches for → not-connected; an old empty list → listed+stale: true → the UI's stale branch (which correctly runs before the state !== 'nothing' refusal, the F1 fix). I confirmed the emitted shapes: nothing/stale=false, not-connected for absent/incomplete/uncollected, listed/stale=true for both published_at 45m old and 3h in the future.
• freshness cannot be green over stale data: StaleAt treats > MaxClockSkew in the future as stale, Response marks every row stale (so the chip is amber), and snapshotChip additionally refuses green when the waiting answer dates the same file old.
• per-field degradation never paints wrong: bad milestone → only its row loses standing (row falls back to "no status was read for it", never to blank/absent); bad Sendrast block → only that row not-connected; a status with no epic is kept verbatim and recorded as a Problem; long text is carried verbatim (no truncation anywhere) — TestSnapshotLongTextIsCarriedWhole.
• "not connected yet" vs stale vs all-clear in the UI matches every shape the API can emit, including the two viewer-policy cases (unmapped/uncollected with items on screen → listed + WAITING_OWN_LIST_UNCOLLECTED; unmapped with others' items → listed + WAITING_NO_OWNER).

Findings

No blocking findings.

Nits (none of these should stop the merge):

• F1 — nit (pre-existing, and the one I would fix first): a repeated decision id kills the whole pane. web/src/lib/components/programs/programs-pane.svelte:270 keys the list {#each wait.items as item (item.id)}, while neither readDecisions nor readDecision (programs/snapshot.go:433-540) checks id uniqueness. I rendered the pane with two decisions sharing id: "P0" on this head: Svelte throws each_key_duplicate and the pane does not render at all — the one page-level failure the per-item degradation story claims to have closed. Same on live, so not introduced here; the fix is a per-item Problem (fail-closed, which then also withholds nothing) or keying by index.
• F2 — nit: the dev branch still logs next, contradicting the new comment. auth/oidc.go:100-108 says the line never carries "the next target", and the dev-mode Warn four lines below logs "next", next. TestLoginStartLogsNoValues exercises only the non-dev path. Harmless in dev, but the comment overstates the guarantee.
• F3 — nit: formatVersion still prints dev for a suffixed tag. web/src/lib/format.ts:115 only special-cases the literal dev; a stamped 0.1.0-dev renders as v0.1.0-dev, so "the version chip never says dev" holds for the exact word only.
• F4 — nit: snapshot-check cannot pre-flight the future-published_at rule. cmd/snapshot-check/main.go:56-90 calls only ParseSnapshot, while the future verdict lives in Response/StaleAt (needs a clock), so a document stamped an hour ahead exits 0 and the cockpit will paint it stale forever. Defensible (the publisher's own clock is the wrong reference), but worth knowing when the pre-flight is used to explain a stale tablet.
• F5 — nit: a constructor mutates a package global. programs/snapshot_file.go:74-78 sets SnapshotProblemSink from FileSnapshotOptions.Problems; a second reader built anywhere silently redirects the first one's causes. Documented, and correct for one-reader-per-process, but it is hidden state.
• F6 — nit: cosmetic + a skipped assertion. web/src/lib/components/shell/identity-chip.test.ts:35 has name_source: 'claim-name',\t\troles: ['operator'], collapsed onto one line; and the updated dashboard identity expectations (web/e2e/panes.spec.ts:365-386) now sit inside test.describe.skip('the dashboard pane'), so those two not.toContainText('dev:e2e-operator') assertions no longer run — the behaviour is covered by dashboard-pane.test.ts, but the e2e text is now dead.

Intent fit

Plain words on screen (the Waiting block, the Sendrast headline/status split, "unversioned build", "operator account"), no internal identifiers painted (numeric subject gone from chip, tooltip and dashboard strip; program labels only), removals reversible in one line each (PANE_ORDER, the two STOP mounts, the commented imports) with the gate's skip ceiling rather than silent deletion, and no silent data loss in the reader (long texts whole, Sendrast status never dropped, every degraded field named in a Problem and logged once per published document). Coverage is genuinely withdrawn for the fleet/gates/chat surfaces while those panes stay live — the gate records that as an explicit ceiling, so it is a known trade-off, not a hidden one.

**Verdict of record — ld-reviewer — head 4c4892b968eba624901247b8caf2c2e5327bee51: APPROVE** model: omniroute/deepseek-flash session: 01a1adce-4c25-77d0-a478-0c6dc8744fb0 ## What I reviewed The full change going to live: `git diff origin/live..4c4892b9`, read as a promotion, not as a PR diff. I checked the merge effect separately, because the two-way diff shows the sbx-gits work as a deletion: `git merge-tree origin/live 4c4892b9` reports **no conflicts**, and `dev/cockpit` touches only `lab-director/**` since the merge-base `fb8598c9`; the gits-sbx/examples scripts therefore stay at live's versions (PR #853) and are not reverted by this merge. Local verification on the exact head (worktree at 4c4892b9, not the shared checkout): - `go vet ./...` clean; `go test ./...` all packages ok; `go test -race ./auth/... ./programs/... ./cmd/snapshot-check/...` ok (auth 7.1s). - `npm ci && npx vitest run`: **432 passed | 11 skipped (443)** — exactly the gate's `GATE_VITEST_MIN_TOTAL=443`, and the fixture↔wire parity guard ran. - `npm run check` (svelte-check): **0 errors, 0 warnings**. - `npx playwright test --list --reporter=json`: 85 tests, **50 skipped, 35 expected** → above the gate floor (`>=33`) and under its ceiling (`<=52`), and the four required titles (three Programs + `sign-in failure`) are all `passed`, not skipped. ## Security (checked, holds) - **Escaping**: `auth/loginpage.go` is an `html/template` with the detail only in an element context; `TestCallbackFailurePageEscapesTheIdPDescription` asserts the `<script>` payload appears as `&lt;script&gt;…`, and the page carries no script and no external resource. `auth/loginpage.go:44-96`. - **CSP fit**: the inline `<style>` is authorised by the deployed policy (`server/middleware.go:165`, `style-src 'self' 'unsafe-inline'`, hashed `script-src`), so the page is not a blank document in production. - **No values in the login-start log**: `auth/oidc.go:98-104` logs only `dev` + `request_id`; `TestLoginStartLogsNoValues` greps for the state-cookie value, nonce, sealed cookie, roles, subject and `next` in the log. - **Userinfo grants nothing**: `auth/oidc.go:333-380` (`nameFromUserInfo`) only writes `Name`/`Email`/`NamedByUserInfo`; roles come from the verified ID token's project-roles claim (`auth/roles.go:62-72`), and a userinfo `sub` that differs from the token's is refused. The tests drive a **real** discovery+JWKS+token+userinfo provider (`fakeIdP`), not a stub, and assert `userinfoHits == 1` and "not asked when the token named somebody". - **RBAC unchanged and fail-closed**: `RequireOperator` → `resolveIdentity`/`HasRole` untouched (`auth/middleware.go:163-180`), `decodeIdentity` still refuses a session without a subject (`auth/roles.go:102-104`). `DisplayName` losing the subject as a display value is presentation only; `Subject` survives in the sealed identity (asserted). - **No credential or token touched by the CLI**: `snapshot-check` reads stdin, prints field paths only, and a no-value contract is enforced with SENTINEL fixtures. ## Correctness (checked against the wire shapes, not just the tests) I re-derived the states from `programs/snapshot.go` + `programs/source.go` and compared with the branches in `web/src/lib/programs/plain-language.ts`, and probed the reader directly: - false all-clear is unreachable: empty+fresh list earns `nothing` **only** with `collected_owners` naming the viewer and a complete list; a dropped item empties `CollectedOwners` → `not-connected`; a fresh empty list nobody vouches for → `not-connected`; an old empty list → `listed`+`stale: true` → the UI's `stale` branch (which correctly runs **before** the `state !== 'nothing'` refusal, the F1 fix). I confirmed the emitted shapes: `nothing/stale=false`, `not-connected` for absent/incomplete/uncollected, `listed/stale=true` for both `published_at` 45m old and 3h in the future. - freshness cannot be green over stale data: `StaleAt` treats `> MaxClockSkew` in the future as stale, `Response` marks every row `stale` (so the chip is amber), and `snapshotChip` additionally refuses green when the waiting answer dates the same file old. - per-field degradation never paints wrong: bad milestone → only its row loses standing (row falls back to "no status was read for it", never to blank/absent); bad Sendrast block → only that row `not-connected`; a status with no epic is **kept** verbatim and recorded as a Problem; long text is carried verbatim (no truncation anywhere) — `TestSnapshotLongTextIsCarriedWhole`. - "not connected yet" vs stale vs all-clear in the UI matches every shape the API can emit, including the two viewer-policy cases (unmapped/uncollected with items on screen → `listed` + `WAITING_OWN_LIST_UNCOLLECTED`; unmapped with others' items → `listed` + `WAITING_NO_OWNER`). ## Findings **No blocking findings.** Nits (none of these should stop the merge): - **F1 — nit (pre-existing, and the one I would fix first): a repeated decision `id` kills the whole pane.** `web/src/lib/components/programs/programs-pane.svelte:270` keys the list `{#each wait.items as item (item.id)}`, while neither `readDecisions` nor `readDecision` (`programs/snapshot.go:433-540`) checks id uniqueness. I rendered the pane with two decisions sharing `id: "P0"` on this head: Svelte throws `each_key_duplicate` and the pane does not render at all — the one page-level failure the per-item degradation story claims to have closed. Same on live, so not introduced here; the fix is a per-item Problem (fail-closed, which then also withholds `nothing`) or keying by index. - **F2 — nit: the dev branch still logs `next`, contradicting the new comment.** `auth/oidc.go:100-108` says the line never carries "the `next` target", and the dev-mode `Warn` four lines below logs `"next", next`. `TestLoginStartLogsNoValues` exercises only the non-dev path. Harmless in dev, but the comment overstates the guarantee. - **F3 — nit: `formatVersion` still prints `dev` for a suffixed tag.** `web/src/lib/format.ts:115` only special-cases the literal `dev`; a stamped `0.1.0-dev` renders as `v0.1.0-dev`, so "the version chip never says dev" holds for the exact word only. - **F4 — nit: `snapshot-check` cannot pre-flight the future-`published_at` rule.** `cmd/snapshot-check/main.go:56-90` calls only `ParseSnapshot`, while the future verdict lives in `Response`/`StaleAt` (needs a clock), so a document stamped an hour ahead exits 0 and the cockpit will paint it stale forever. Defensible (the publisher's own clock is the wrong reference), but worth knowing when the pre-flight is used to explain a stale tablet. - **F5 — nit: a constructor mutates a package global.** `programs/snapshot_file.go:74-78` sets `SnapshotProblemSink` from `FileSnapshotOptions.Problems`; a second reader built anywhere silently redirects the first one's causes. Documented, and correct for one-reader-per-process, but it is hidden state. - **F6 — nit: cosmetic + a skipped assertion.** `web/src/lib/components/shell/identity-chip.test.ts:35` has `name_source: 'claim-name',\t\troles: ['operator'],` collapsed onto one line; and the updated dashboard identity expectations (`web/e2e/panes.spec.ts:365-386`) now sit inside `test.describe.skip('the dashboard pane')`, so those two `not.toContainText('dev:e2e-operator')` assertions no longer run — the behaviour is covered by `dashboard-pane.test.ts`, but the e2e text is now dead. ## Intent fit Plain words on screen (the Waiting block, the Sendrast headline/status split, "unversioned build", "operator account"), no internal identifiers painted (numeric subject gone from chip, tooltip and dashboard strip; program labels only), removals reversible in one line each (`PANE_ORDER`, the two STOP mounts, the commented imports) with the gate's skip ceiling rather than silent deletion, and no silent data loss in the reader (long texts whole, Sendrast status never dropped, every degraded field named in a Problem and logged once per published document). Coverage is genuinely withdrawn for the fleet/gates/chat surfaces while those panes stay live — the gate records that as an explicit ceiling, so it is a known trade-off, not a hidden one.

pi-landing merged commit fb099824dd into live 2026-11-05 22:38:42 +00:00

atlantis commented 2026-11-05 22:38:43 +00:00

Collaborator

Copy link

Plan Error

cannot run "plan": the default workspace at path . is currently locked for this pull request by "plan".
Wait until the previous command is complete and try again

**Plan Error** ``` cannot run "plan": the default workspace at path . is currently locked for this pull request by "plan". Wait until the previous command is complete and try again ```

pi-landing referenced this pull request from a commit 2026-11-05 22:38:45 +00:00

Merge pull request 'dev/cockpit → live: first dev-route feature (plain words, honest freshness, hardened reader)' (#857) from dev/cockpit into live

pi-landing commented 2026-11-05 22:39:29 +00:00

Author

Collaborator

Copy link

Merge note, per El Director: Aaron's yes covers this one fix ("3 - yes."). The first image build failure on this head is still unexplained: logs unreadable, the rerun passed. The two seats shared one brief. Sandbox lock: forgejo/landing.ts sha256 cce961523a6f83922537846888709543b00ef378b284284f708d5b1f079e19fd.

Merge note, per El Director: Aaron's yes covers this one fix ("3 - yes."). The first image build failure on this head is still unexplained: logs unreadable, the rerun passed. The two seats shared one brief. Sandbox lock: forgejo/landing.ts sha256 cce961523a6f83922537846888709543b00ef378b284284f708d5b1f079e19fd.

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

operator-review

Requires human operator review before merge — dangerous class (RBAC, secrets, network policy, Terraform) or explicitly gated. Never auto-merge.

No labels

operator-review

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

3 participants

Notifications

Subscribe

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!857

No description provided.