HOMELAB-1681: feat(cockpit): LD3.1a observer shell — Go backend, SvelteKit frontend, PWA, e2e #570
No reviewers
Labels
No labels
operator-review
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Homelab/infra-core!570
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "plane/HOMELAB-1681-cockpit-shell"
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?
Slice LD3.1a (HOMELAB-1681) of epic LD3 (Lab Director Cockpit): the credentialless observer shell. Class B/C, auto-land after review.
Design authority:
.rpiv/artifacts/designs/ld3/cockpit-design-brief.md(R1–R12),.rpiv/artifacts/research/ld3/legacy-ui-scout.md,.rpiv/artifacts/research/ld3/dispatcher-surfaces-scout.md§6, contract §4/§7/§9.What
New tree
lab-director/cockpit/: its own Go module (replace … => ../dispatcher), a SvelteKit 2 / Svelte 5 runes / Tailwind 4 frontend built to a static SPA the Go binary embeds, and a Playwright suite that is committed and wired into CI.Backend —
config/(env-only schema, collected-faults refusal;OIDC_ISSUERempty is dev mode, and any other OIDC key set while the issuer is empty is a hard refusal),auth/(go-oidc v3 authorization-code flow, stateless AES-GCM session cookie with a rotatable keyring,RequireRole(operator)overurn:zitadel:iam:org:project:roles— the piece the legacy app never had),readmodel/(one provider, one snapshot, a wiring census withconfigured|unwired|error|unknownand a sentence per source),sse/(stdlib SSE: hub-level tickers so N clients cost one render, slow consumers dropped not buffered, no replay),server/(Go 1.22 ServeMux, local middleware Chain, embedded SPA with fallback + precompressed-variant negotiation),cmd/cockpit/(--version, hermetic--check).Frontend — the three-pane shell (dashboard with the real wiring census / gates / chat placeholders, each naming the ticket that fills it),
/login,/unauthorized, PWA manifest + offline-shell service worker, dark-first token spine with one accent and a hatch texture for "not backed by an observation".Realtime —
GET /api/v1/events:helloon connect,readmodelticks,heartbeats. The SSE tick andGET /api/v1/readmodelare the same provider and the same struct; the shell bridges them into one store.Why
The cockpit is the contract's observer surface (§7): adds reach, never power. It stores nothing — the session is a cookie, the read model is computed at read time, and anything it cannot read it declares as
unwiredwith the keys or ticket that would change it (R4). 3.1a is credentialless by design, so it lands without an operator ruling; the deploy slice (3.1b) and its two credentials are separately gated.Test plan (all run, evidence below)
go vet ./... && go test -race -count=1 ./...npm ci && npx svelte-kit sync && npx svelte-check --tsconfig ./tsconfig.json && npm run test && npm run buildbuild/npm run e2e(real binary + real build, dev mode)docker build/ runManual server evidence (dev mode,
COCKPIT_STATIC_DIR=web/build, read-model tick 2s / heartbeat 3s):Two defects the e2e suite caught and this PR fixes (worth the reviewer's attention because they are the exact class of bug unit tests miss):
authApi.me()went through the/api-prefixed client and called/api/auth/me→ 404, which the shell rendered as "not signed in". The client now hasrequest(API) andrequestRoot(session surface), with a regression test.events.on('readmodel', …) → readmodel.apply(...)explicitly, and the e2e asserts the uptime actually ticks.Slice boundary (hard)
In:
lab-director/cockpit/**,.forgejo/workflows/cockpit-image-build.yaml,.forgejo/workflows/ci.yaml(additive: three new jobs, existing jobs byte-identical). One territory-adjacent file:lab-director/.dockerignore, required because R1 fixes the image's build context atlab-director/. It cannot affect the dispatcher image (that workflow's context islab-director/dispatcher, so Docker reads the.dockerignorethere).Out (not touched): no prod manifests, no HTTPRoute, no ESO/secrets, no CNP — that is 3.1b, gated on the operator ruling (
.rpiv/artifacts/designs/ld3/ratification-package-3.1b.md). No Plane read wiring (the client/transport seam is not built;planerendersunwired, orconfiguredwith an honest note). No NATS, no attach, no xterm, no STOP button.lab-director/dispatcher/**is unmodified — the cockpit importsdispatcher/eventsfor the fleet-class vocabulary and nothing else.Deviations from the brief, with reasons
golang.org/x/cryptois not a direct dependency. It appears only transitively (via go-oidc's go-jose). Cookie encryption is AES-GCM, which is stdlibcrypto/aes+crypto/cipher; adding an unused direct dep would be a lie ingo.mod.web/.npmrcsetslegacy-peer-deps=true. npm 10's resolver crashes (TypeError: … reading 'edgesOut') on vitest's optional-peer graph, and vitest must be >= 4.1.11 (below that is a critical advisory in@vitest/mocker). Result: 2 low-severity advisories, both transitive throughbits-uiwith no fix that does not regress the pinned stack. Stated in the file itself, with the cost ("an unmet peer stops being a hard error") written down.node:22-alpinein CI is pinned to the same digest as the Dockerfile's web stage so CI's node and the image's node cannot drift.@internationalized/dateis an explicit devDependency (as in the legacy app): withlegacy-peer-deps, npm 10 does not hoist bits-ui's own dependency, and the build fails without it. Found by a cleannpm ciafter a passing run — an npm-11-installed tree had masked it.Open items for the landing reviewer
cockpit-image-buildjob is the gate; if the runner's BuildKit handles the two-stage context as expected, it goes green, and the digest it prints is what 3.1b pins.urn:zitadel:iam:org:project:rolesclaim never reaches the ID token and every operator gets a 403. That is a Zitadel-side setting, recorded here and in the 3.1b package — the code is correct either way and fails closed.e2e/auth.spec.tsrather than implied.ci.yamlplus a new image workflow have never executed on this runner.cockpit-e2eis the riskiest (Playwright image +xvfb-run+ a mounted prebuilt binary); if it fails for environment reasons rather than assertion reasons, the fallback is to dropE2E_HEADEDand the Xvfb hop (kept for the brief's "headed chromium").Progress log:
/home/aaron/repos/homelab/.rpiv/artifacts/build/ld3-1681a-builder.md.CI green — and what the pipeline caught (builder note, 2026-10-02)
Head
b688ecf399— all jobs pass:cockpit-testcockpit-e2exvfb-runin the digest-pinned Playwright image)helm-validate,receiver-test,dispatcher-test,alert-receiver-testbuild-and-push(cockpit-image-build)The image builds. That was the one thing I could not verify locally (no docker socket), so it is worth stating plainly:
node:22-alpine→golang:1.24.1-alpinewith the two-module context works, thewebstage'snpm ciworks inside the container,--versionand--checkpass against the built image, and the serve-and-stream smoke passes (health, read model, the embedded SPA being the app rather than the placeholder, and a live SSE handshake carryinghello/heartbeat/readmodel).Finding the image workflow took three iterations, and each one was a real defect in the smoke rather than in the product — recorded here because a reviewer should not have to reconstruct it from failing runs:
msg="listening");TextHandleronly quotes a value that needs it, so a single word ismsg=listeningand the loop never matched. Failed after 30s against a container that had been healthy the whole time.http://cockpit:8080while the container is namedld-cockpit-smoke-server. The traceback read as DNS failure (Errno -5 Name has no usable address) and was a typo. The host is now passed as argv, so the two cannot drift, and the probe prints its base URL first.dev: true,subjectprefixeddev:) and never renders as a real login.Two earlier defects were caught before CI, by the committed e2e suite — and they are the reason this PR insists on a real one:
authApi.me()was calling/api/auth/me(404 → the shell showed "not signed in" for an authenticated operator), and the SSE ticks were never bridged into the read-model store (the connection dot said "live" while the uptime stayed frozen). Neither is visible to a unit test.Nothing else outstanding from my side.
lab-director/dispatcher/**is unmodified, and the territory is as declared in the PR body.Review verdict 1/2 — correctness & security (fresh context)
Verdict:
BLOCKERS: 1 | CONCERNS: 5 | SUGGESTIONS: 6— for headb688ecf3990f5486412eeecd9c193e190a4dd0ff.Independent fresh-context reviewer (not the implementer), primary sources only: the PR diff, the branch tree read-only, the design brief R1–R12, and acceptance criteria HOMELAB-1681. The reviewer additionally built the SPA and ran an independent headless-Chromium reproduction of B1.
Blockers
B1. The production (non-dev) CSP blocks the SPA's inline bootstrap script, so the authenticated cockpit renders a blank page.
lab-director/cockpit/server/middleware.go:110-135builds the CSP. The non-dev branch (lines 111-122) emits"script-src 'self'"(line 120) — no'unsafe-inline', no nonce, no hash. The dev branch (lines 124-134) emits"script-src 'self' 'unsafe-inline' 'unsafe-eval'"(line 132). The mode is chosen atserver/server.go:140(SecurityHeaders(s.opts.Dev)), fed fromcmd/cockpit/main.go:166(Dev: cfg.Dev()), which is true only whenOIDC_ISSUERis empty (config/config.go:213). So the strict CSP applies exactly in the deployed/authenticated configuration.<script>. The committed source isweb/src/app.html:18(%sveltekit.body%) with nokit.cspconfigured (web/svelte.config.js:17-38), so SvelteKit injects its classic inline bootstrap + service-worker registration intoindex.html.Dockerfile:53builds it andDockerfile:97-99stages that build as the embedded SPA. A policy ofscript-src 'self'with no hash/nonce blocks that inline script; the app never callskit.start(...).npm run buildfrom the branch source): serving the builtindex.htmlwith the exact production CSP string and loading it in headless Chromium yields[error] Executing inline script violates the following Content Security Policy directive 'script-src 'self''. Either the 'unsafe-inline' keyword, a hash ('sha256-…'), or a nonce ('nonce-…') is required…with
appMounted: false,bodyText: "". With the dev CSP the same bundle mounts (appMounted: true, header/panes render). So the code is fine; the document is unreachable under the deployed policy.web/playwright.config.ts:6-15andweb/e2e/run-server.sh:5-11,37-44(dev mode is "implied by leaving OIDC_ISSUER unset"), the image smoke.forgejo/workflows/cockpit-image-build.yaml:288-298(-e COCKPIT_LOG_LEVEL=info, no issuer → dev), andserver/server_test.go:5-15(newTestServeralwaysDev: true).TestSecurityHeaders(server/server_test.go:359-384) asserts the dev CSP — it even requiresws:to be present (lines 379-381).index.htmlat startup (server/static.go:68already holds the bytes;writeIndexat 257-275 is the one write site) and append'sha256-…'toscript-src, or (b)kit.cspnonce mode inweb/svelte.config.js+ a per-response nonce injected into both the header andindex.html, or (c) fall back to'unsafe-inline'onscript-src(works, but gives up the XSS hardening). Whichever is chosen, add a check that the real build boots under the non-dev CSP; a Go test that asserts the header's hashes cover the embeddedindex.html's inline scripts is enough and needs no browser.Concerns
C1. Dev mode is reachable by absence, and its one warning can be silenced by configuration.
config/load.go:140-160refuses the ambiguous case (any ofOIDC_CLIENT_ID/OIDC_CLIENT_SECRET/OIDC_REDIRECT_URLset while the issuer is empty →FaultIncompleteOIDC/ErrUnprotected). But an entirely empty OIDC environment is legal dev mode: every endpoint unprotected, and any caller picks its own identity viaX-Cockpit-Dev-User, always granted the operator role (auth/middleware.go:167-182,Roles: []string{h.cfg.OperatorRole}). The only guard is the WARN atcmd/cockpit/main.go:82-88— andnewLogger(cmd/cockpit/main.go:302-311) sets the level fromCOCKPIT_LOG_LEVEL, soCOCKPIT_LOG_LEVEL=errorsuppresses it. A Deployment whose four OIDC env keys are typo'd (or whose optional Secrets never populate) therefore starts an unprotected operator surface whose page celebrates being unprotected (web/src/lib/components/shell/app-header.svelte:50-57,web/src/routes/login/+page.svelte:51-59), possibly with no startup warning. R3 rules "issuer empty → dev mode", so this is not a blocker, but it is the weakest link in the credential-free posture 3.1b inherits. Fix: require a positive opt-in for dev mode (e.g.COCKPIT_DEV=1) and/or emit the dev warning to stderr unconditionally rather than through the levelled logger.C2. The ID-token nonce is verified only when the IdP happens to return one.
auth/oidc.go:167-170:if idToken.Nonce != "" && stateEqual(idToken.Nonce, state.Nonce) != 1 { … }. The server always sends a nonce (auth/oidc.go:78,:93-96), and go-oidc only surfaces the claim — it never verifies it (pinnedgo-oidc v3.13.0,oidc/verify.go:253), so this conditional is the entire nonce check. A token response without a nonce is accepted, defeating the replay/injection binding the nonce exists for. The sealed state cookie (:137-145) still blocks CSRF and code-swapping, so this is defence-in-depth rather than a direct bypass. Fix: fail whenidToken.Nonce == "".C3. No PKCE on the authorization-code flow.
auth/oidc.go:93-96buildsAuthCodeURL(nonce, oidc.Nonce(nonce), oauth2.AccessTypeOnline)and:150exchanges with no verifier (oauth2.S256ChallengeOption/VerifierOptionare absent). For a confidential client whose secret never leaves the server the exposure is bounded, but current OAuth security guidance (RFC 9700 §2.1.1, OAuth 2.1) requires PKCE for browser-based code flows, and it is a few lines given the state cookie already exists. Not a blocker.C4. Session role state is frozen at login; the 403 page tells the operator otherwise.
web/src/routes/unauthorized/+page.svelte:64-67says "Ask an operator to grant … Revoking the role closes access; it deletes nothing." Actually the roles are copied into the sealed cookie at login (auth/roles.go:62-71,auth/session.go:74-93) and re-read from the cookie only (auth/middleware.go:157), withSessionDuration = 24 * time.Hour(config/config.go:109-111). So revocation takes up to 24h to bite, and a fresh grant does not take effect until the operator signs in again — neither of which the page (or the README) states. This is inherent to R3's stateless-cookie ruling, so the defect is the claim, not the design. Fix: correct the copy and record the revocation latency in the 3.1b verification plan.C5. Server shutdown does not end SSE streams, so every restart with an open tab burns the full shutdown budget.
cmd/cockpit/main.go:176gives the hub the signal context, but each stream's subscription is bound to the request context (sse/sse.go:350-351,:370-373), andcmd/cockpit/main.go:234-238callshttpServer.Shutdown, which does not cancel active request contexts. There is noHub-level shutdown andStreamselects on nothing process-wide. Measured on the built binary: SIGTERM with onecurl -N /api/v1/eventsattached →shutdown requested; drainingthen, 15.13 s later,graceful shutdown did not finish: context deadline exceeded(ShutdownTimeout = 15s,config/load.go:53); the same binary with no stream attached shut down in 0.003 s. Against a 30 sterminationGracePeriodthat is half the budget spent waiting on connections that will never go idle, on every restart. The other two unsubscribe paths are correct (client disconnect viar.Context(); write error viadefer sub.Cancel()). Fix: give the hub a process context thatStreamalso selects on (or have shutdown cancel the hub ctx), so streams close immediately and drain is bounded by real work.Suggestions
S1. The
cockpit-e2ejob writes root-owned files into the mounted checkout, which the sibling job deliberately avoids..forgejo/workflows/ci.yaml:164mounts the workspace read-write (-v "$PWD/lab-director/cockpit/web:/web") and runsnpm ci+npm run buildin it, and:167-168writes the Go binary into the repo tree (-o /src/cockpit/web/ld-cockpit-e2e) — the latter is not covered byweb/.gitignore. Compare the cockpit-test job's own stated rationale at:127("READ-ONLY and copied inside, so node_modules never lands root-owned in the workspace") and:133, and the e2e job's step 3, which does mount:ro(:184). On the shared:hostrunner this can leave artifacts that break the next job. Fix: copy-then-build as step 3 does, and emit the binary outside the checkout.S2. Two exported auth helpers have no production caller, and one's doc comment is wrong.
auth/oidc.go:285-295(SetSessionCookie, "how the e2e/dev tooling mints a session") and:300-310(SessionFrom, "The SSE handler uses it at connect time") — the SSE handler actually usesauth.IdentityFrom(server/server.go:173). An unused exported session-minting primitive is a footgun for LD3.2/3.3; delete both or fix the comment.S3. Dangling doc reference.
config/config.go:62points at "Secrets.Resolved"; noSecretstype exists anywhere in the module (grepped.gofiles). Either implement it or drop the pointer before 3.1b copies the schema.S4.
COCKPIT_COOKIE_SECURE=falseis accepted outside dev mode with only a WARN.config/load.go:85-92applies it unconditionally andcmd/cockpit/main.go:93-96warns. Since the deployment is HTTPS-only, refusing it whenOIDC_ISSUERis set (where the cookie is a real credential) matches the "no unprotected operator surface by accident" posture the rest of the config enforces.S5. The image digest is printed but never published.
.forgejo/workflows/cockpit-image-build.yaml:375-381reads the digest back withcraneandechos it to the log; the comment calls it "the input to the 3.1b deploy pin", but nothing durable carries it (no tag, no artifact, no PR comment) — a human must copy it from a log. This faithfully mirrorsdispatcher-image-build.yaml, so it is house style rather than a defect; still, publishing it (e.g. av-<sha>tag or a step summary line) would make the 3.1b pin mechanical.S6. CI Go patch version drifts from the shipped image.
.forgejo/workflows/ci.yaml:118,167use floatinggolang:1.24whileDockerfile:62pinsgolang:1.24.1-alpine@sha256:…. The comment argues the floor cannot drop belowgo 1.24.1; using the same pinned tag in both would remove the class of "green in CI, different toolchain in the image".Territory / slice-boundary check
Passes, mechanically. From the diff the 94 changed paths are exactly
lab-director/cockpit/**(91) +.forgejo/workflows/ci.yaml+.forgejo/workflows/cockpit-image-build.yaml+lab-director/.dockerignore; a filter for anything outside the declared IN list returned nothing.git diff --stat origin/epic/LD3 origin/plane/HOMELAB-1681-cockpit-shell -- lab-director/→ 92 files, zero underlab-director/dispatcher/. Noprod/**, no HTTPRoute, no ExternalSecret, no CNP. The dispatcher image is unaffected by the new.dockerignore(its context islab-director/dispatcher; only the cockpit workflow usesCONTEXT: lab-director). ci.yaml is additive-only, proven mechanically: the single hunk is@@ -90,3 +90,104 @@, 0 removed lines, first 92 lines byte-identical.What the reviewer verified and found CORRECT
[]string{}→ 403 (auth/roles.go:39-57,auth/middleware.go:157-163); a claim of the wrong JSON shape aborts the login (auth/oidc.go:171-175). No empty-role config can match "everything" (config/load.go:49,197-202). No "unknown ⇒ allow" path.Sealcrypto/randnonce (auth/session.go:176-179); 32-byte key enforced; AAD binds version+key id; every failure returns a wrapped sentinel and no decrypt path returns success on garbage. Rotation correct (Keys[0]seals, all keys open). CookieHttpOnly; Secure; SameSite=Lax; Path=/, Secure defaults true.statecompared constant-time against the sealed, single-use cookie; no open redirect; signature/issuer/audience/expiry validated by go-oidc v3.13.0.GET /api/v1/health,/api/v1/version, and the four/auth/*routes are public;/api/v1/readmodel,/api/v1/eventsand the SPA catch-all sit behindRequireOperator; no unauthenticated reachable route found that should not be.sync.Onceclose makes publish-vs-cancel/drop safe; no goroutine leak; no replay; one render per tick shared by all clients. The concurrency test does exercise the race under-race.Loadnever returns a partial config. No secret value reaches a message or log.go.modpins all four direct deps;replace … => ../dispatchercorrect;go mod verifyclean andgo mod tidya no-op.package-lock.jsonlockfileVersion 3, 247 packages, zero resolved entries lackingintegrity, no^/~;.npmrconly the documentedlegacy-peer-deps=true. OSV query on the pinned frontend stack returned no advisories (point-in-time).auth/auth_test.go:945,config/config_test.go:3314) and the${{ secrets.HARBOR_PUSH_* }}references scoped to the non-PR push step;latestmoves only onrefs/heads/live. Docker base images digest-pinned; runtime distroless nonroot.go vet ./...clean and all Go packages' tests pass; 9 vitest files / 62 tests pass; a freshly built binary in dev mode served health, streamedhello/readmodel/heartbeatwith monotonic ids. No external origins, noconsole.log.Review verdict 2/2 — test adequacy (fresh context)
Verdict:
BLOCKERS: 0 | CONCERNS: 4 | SUGGESTIONS: 5— for headb688ecf3990f5486412eeecd9c193e190a4dd0ff.Independent fresh-context reviewer (not the implementer). Findings below are mutation-verified where they claim load-bearingness: the reviewer rebuilt the binary + a fresh
npm run buildand re-ran the suites, and injected deliberate regressions to confirm tests fail.Claims checked
web/package.json:18("e2e": "playwright test"),:25;.forgejo/workflows/ci.yaml:153-193jobcockpit-e2e; executed CI run 1671 job 5cockpit-e2e success 3m39son headb688ecf. Independently reproduced: binary from the branch + fresh build → 17 passed (9.5s)playwright.config.ts:26-27(testDir: 'e2e',testMatch: '**/*.spec.ts') matches the two committed specs;--list→ "Total: 17 tests in 2 files";:30forbidOnlyunder CI;webServerstarts the real binary (:57-71,e2e/run-server.sh:26-44); no--passWithNoTestsanywheretsconfig.json:18-19includese2e/**/*.ts+playwright.config.ts;ci.yaml:140runssvelte-check. Injectedconst deliberateTypeError: number = "not a number"intoe2e/shell.spec.ts→ 0 errors → 1 error (checked, not skipped)src/lib/api/client.ts:89(request),:94(requestRoot),:103(requestRoot<MeResponse>('/auth/me')). Regression:client.test.ts:65-72. Mutation (restoring pre-fixrequest<MeResponse>('/auth/me')) → that test fails:expected '/api/auth/me' to be '/auth/me'4229adealready containsrequestRoot+ the bridge, so the pre-fix state is not in the branch history. Provenance unprovable; the regression artefacts' load-bearingness is proven by mutationsrc/routes/+layout.svelte:62-64(events.on('readmodel', …) → readmodel.apply(...)); assertion:e2e/shell.spec.ts:53-69(non-—and text changes within 10s) against a 1s tick. Mutation (deleting the bridge, rebuilding) → exactly that spec fails (10.6s) while "the connection dot says so" (shell.spec.ts:43-51) still passes — i.e. it asserts a rendered value advancing, not a store write and not a mock callshell.spec.ts:43-51+:53-69; Gosse/sse_test.go:184-245,server/server_test.go:170-225; image smoke assertshello+heartbeat+readmodelon the wire (CI run 1672 green)auth/auth_test.go:314-346(401),:348-419(403 + JSON + document),:421-443(admit). Mutation (if !id.HasRole(...)→if false) →TestRequireOperatorForbiddenfails. But no test drives a non-operator through the assembled route table — see C1e2e/auth.spec.ts:5-19states what is real, what is not, and where the server side is asserted. Caveat: the Go suite never exercises the callback success path either — see C2go test -race -count=1 ./...→ 5 pkgs ok); vitest 62 passed / 9 files; playwright 17 passed + CI greenBlockers
None. Both claimed defect fixes carry regression coverage the reviewer proved load-bearing by mutation; the e2e suite executes for real in CI (run 1671 job 5) and locally; the AC's denial semantics are covered by a test that fails when the check is removed.
Concerns
server/server_test.go,server/server.go:128-134). The only test server is dev mode (server_test.go:53-98), whereRequireOperatoradmits everything (auth/middleware.go:167-182gives every dev identity the operator role), andserver_test.gohas no 401/403 assertion at all. The e2e cannot close this (dev mode only; no role-less session spec). Mitigations verified: removingRequireOperatorfromserver.go:134is caught (server_test.go:156/:216fail), and the role check is caught in the auth package. Residual gap: a role bypass between the route and the middleware, and zero coverage of the 401/403 wire contract for/api/v1/readmodel,/api/v1/eventsand the SPA catch-all. Structural cause:auth.NewHandleralways performs discovery when the issuer is set (auth/middleware.go:90-101), so the server package cannot build a non-dev handler in a test today. Fix: an exported non-discovery constructor (or injectable provider) + two assertions throughServer.Handler().auth/oidc.go:73-98,:149-203). Grep across all_test.go:identityFromClaims,sessionCookie,SetSessionCookie,randomToken,ztClaimsare referenced by no test;auth_test.gocovers only callback refusals (:522-554) and dev-mode login (:512-520). The headline capability — code exchange → ID-token verification → role from the roles claim → sealed cookie → 302 — is proven only at its two ends. A wrong cookie attribute (oidc.go:261-271untested), a wrong claim struct tag, or a broken post-verify branch would surface only in the operator-gated 3.1b deploy. The e2e gap is disclosed honestly (auth.spec.ts:11-18); the Go-side gap is not. Anhttptestfake IdP would cover most of it.slow_consumerwire contract is asserted on the client only. Server emitsevent: error {code:"slow_consumer"}atsse/sse.go:379-382, but no Go test drives that branch; the client's test fabricates the payload (events.svelte.test.ts:157-162). That test passes even if the server stopped emitting it — a cross-boundary contract pinned on one side.config_test.go:118-121+:127-142pin the refusal and all 6 faults at the loader. Butcmd/cockpithas no test file, somain.go:69-76(runreturns 2 on a faulted env) is never exercised, and the image smoke only asserts--checkexits 0 on a good env — nothing asserts a non-zero exit on a bad one. Cheap fix: amain-package test callingrunwith a faulted env.Suggestions
sse/sse_test.go:318-347has no assertion — a race-detector fixture, honestly labelled, but a plaingo testpasses it trivially; it drivesHub.Subscribe/Publishdirectly, so the handler's drop-frame path (sse.go:374-383) is neither raced nor asserted.1via a test-only hook.DefaultBuffer = 3(sse.go:62-65) is asserted nowhere.readmodelcensus:configured/unwiredhonestly asserted (readmodel_test.go:57-103), buterrorhas no producer in non-test code andunknownis an unasserted fixture. Honest for 3.1a (nothing reads yet); thereadmodel.go:14-20doc advertises a closed four-state contract nothing pins.events.svelte.test.ts:76asserts an argument passed to a mock (weakest kind); the URL contract is already pinned atclient.test.ts:74-77.+layout.svelte:52-55(authenticated-but-not-operator →/unauthorized) has no test and is unreachable in dev mode. Low risk (the server 403s the document first), but an untested branch in the file that owns the AC's client half.e2e wired-into-CI: yes
.forgejo/workflows/ci.yaml:153-193—cockpit-e2e, noif:, noneeds:gating; workflow triggers includepull_request: branches: [live, 'epic/**'](ci.yaml:3-7), which baseepic/LD3satisfies.ci.yaml:192—xvfb-run -a npx playwright test(CI=true,E2E_HEADED=1,E2E_PREBUILT=1,E2E_COCKPIT_BIN=/tmp/work/ld-cockpit-e2e).npm run e2eexists (package.json:18) and@playwright/testis in devDependencies (:25) — neither was true of the legacy app's dead specs.b688ecf→cockpit-e2e success — 3m39s(runs/1671/jobs/5), alongsidecockpit-test success 1m34s.--list→ 17 tests in 2 files; no--passWithNoTests, notest.skip/.only/xit/fit/t.Skipin the PR tree;forbidOnlyunder CI; specs andrun-server.share tracked; the prebuilt binary keeps mode 755 through the CIcp -r, andrun-server.sh:26-30exits non-zero rather than silently continuing if it is not executable.Landing worker — LANDING BLOCKED on B1 (no merge)
Head
b688ecf3990f5486412eeecd9c193e190a4dd0ff. Both independent fresh-context reviews are posted above (correctness/security: 1 blocker / 5 concerns / 6 suggestions; test adequacy: 0 blockers / 4 concerns / 5 suggestions). CI on this head is green (run #1671 all jobs incl.cockpit-e2e3m39s; run #1672build-and-pushpass).Why no merge: B1 is a real, reproduced defect, independently confirmed by the landing worker.
server/middleware.gonon-dev branch emitsscript-src 'self'(no'unsafe-inline', no nonce, no hash);web/svelte.config.jssets nokit.csp;web/src/app.htmlhas no nonce placeholder.npm ci+npm run buildfrom the branch atb688ecf, scratch worktree): the producedbuild/index.htmlcontains an inline<script>(the SvelteKit bootstrap:kit.start(app, element)+ service-worker registration), with nosrcand no nonce.kit.startnever runs, and the authenticated cockpit renders a blank page — the ticket's acceptance criteria cannot be met in the deployed configuration.playwright.config.ts,e2e/run-server.sh, the image smoke, andserver_test.go'snewTestServerall set dev/unsetOIDC_ISSUER), andTestSecurityHeadersasserts only the dev CSP.Class-D visual check was deliberately not run — the landing pipeline halts at the blocker per the landing contract. The R11 visual-verifier pass remains owed and should be run on the fix commit (the fix is CSP-only and does not change the rendered UI, so dev-mode visual evidence gathered after the fix stays valid).
Path to land: a fix worker (fresh context) addresses B1 — make the shipped document and the shipped policy agree (hash-in-CSP computed from the embedded
index.html, akit.cspnonce, or an explicit documented'unsafe-inline'fallback) and add a test that boots the real build under the non-dev CSP. Concerns C1–C5 (dev-mode opt-in + unconditional warning; nonce fail-closed; PKCE; revocation-latency copy; SSE shutdown) are posted for the fix worker's judgement; only B1 gates the merge. Then re-review the delta + run the class-D visual check + merge on green.No CI run was triggered by this comment; the head is unchanged.
Review verdict 3/3 — DELTA review of the B1 fix (fresh context)
Verdict:
BLOCKERS: 0 | CONCERNS: 2 | SUGGESTIONS: 3B1 closed: yes— for head2e0d9b8aad90eb6828979c27b5c2fe86d329e0b6(deltab688ecf399→2e0d9b8, 5 files underlab-director/cockpit/server/**).Strongest single piece of evidence: in headless Chromium, the real served document + the real emitted production CSP executes the inline SvelteKit bootstrap and the app mounts (
__sveltekit_…= object,body.innerText= "LD Cockpit … DASHBOARD", 8452 body bytes), while the defect policyscript-src 'self'yieldsundefined/ empty body (834 bytes) and Chromium asks for exactly the hash the Go extractor emits (sha256-Hj9EQp+tH5RiA1YN1/29IPBse3J/oZZ+FMk235CGftE=).Independent fresh-context reviewer (not the implementer, not the earlier reviewer). It verified against the bytes the image actually ships:
docker buildstagesweb/buildovercmd/cockpit/static/(Dockerfile:91-92), the binary embeds it (cmd/cockpit/embed.go//go:embed all:static),NewSPAHandlerhashes thatindex.html(static.go:79-81), and the same instance serves it (server.go:133,static.go:466).Blockers
None. Every reintroduction of B1 the reviewer could construct is caught (see Mutation check).
Concerns
C1. The only witness that the REAL built document is covered is skipped in CI, so the B1 guarantee rests on a hand-written fixture.
TestProductionCSPCoversTheRealBuild(csp_test.go:176-179)t.Skips when../web/build/index.htmlis absent. That file is gitignored (web/.gitignore:3), and the requiredcockpit-testjob runsgo test -race -count=1 ./...(.forgejo/workflows/ci.yaml:119) on a freshactions/checkout@v4(ci.yaml:101) before it builds the frontend (ci.yaml:142). The reviewer reproduced the exact CI condition locally (hidweb/build) and observed--- SKIP: TestProductionCSPCoversTheRealBuild. So in CI the onlyDev:falsedocument witness is the literalsvelteKitShapedShell(csp_test.go:84-113) plus the placeholder. Why it matters: the defect class is "the served document contains an inline script the policy does not cover"; a parser blind spot the fixture happens not to exercise would go green in CI while the deployed shell goes blank — precisely the failure mode this commit fixes. The commit's headless-Chromium claim was reproduced, but no artifact of it is committed, so it is not continuous. Fix: makecockpit-testbuild the frontend beforego test, or commit a realbuild/index.htmlasserver/testdata/and point the test at it.C2. The extractor diverges from the HTML tokenizer in fail-closed, but production-coloured, ways. Pinned by direct probes of
inlineScriptHashes:<!--occurring inside a non-script attribute value aborts the whole scan:<p title="<!--">then</p><script>real();</script>→ 0 hashes (static.go:146-152matches the<!--inside the attribute, thenbreaks when no-->follows, so the real script is never reached) → that script would be blocked = a B1-shaped blank page. The shape is one edit of a hand-maintainedweb/src/app.htmlaway.</scriptxtruncates the body (static.go:174indexFold(doc[contentStart:], "</script")accepts any suffix), whereas the tokenizer does not end the element there → hash mismatch.<script>\n<!--<script>alert(1)</script>-->\n</script>) hashes a prefix.Why it is not a blocker: every one of these drops or mismatches a hash, i.e. the script is blocked (visible), never silently authorized; and none is reachable from the current build — the reviewer re-ran
npm run buildwithsrc/app.htmlconverted to CRLF and the real builtindex.htmlstill carries no<in any attribute and no CR in the bootstrap body, and Chromium accepted the emitted hash for it. The test's independent regex (csp_test.go:25-53) does catch the first shape if a fixture contains it; it cannot see the real build in CI (C1).Suggestions
S1. Mirror the tokenizer's newline preprocessing, or assert its precondition.
static.go:178hashes raw bytes; the HTML tokenizer replaces CR and CRLF with LF before a script's child text exists, so a CR inside a script body makes the emitted hash unusable. Proven in Chromium: for a bodya();\r\nb();Chromium demandssha256-wrEMctykYIJXhAyfCKKphjLeSVuUKhNQ4uVZWiS2cTQ=(normalized) whilestatic.go:178producessha256-GkkofoCWtVa7bC5kRRvGFNdkKxD7K5NvuBTRjvkYI3Y=(raw) → blocked. Empirically unreachable today (adapter-static writes the injected bootstrap with LF;git grep -Il $'\r' 2e0d9b8 -- lab-director/cockpitis empty). Cheap guard: reject a\rinside an extracted body at startup rather than ship a hash that cannot match.S2.
InlineScriptHashes()hands out the internal slice (static.go:102). Nothing mutates it today (middleware.go:154appends into a fresh[]string{"'self'"}), but a future caller could corrupt the policy for every response; return a copy or state the immutability contract.S3. The test's "is this inline?" check is coarser than production in a way that can spuriously fail.
csp_test.go:60skips any script whose attribute run merely containssrc, while production parses attribute names (static.go:217hasAttribute). On<script data-src="x">v();</script>production correctly hashes it and the test expects no hash → a false failure for a correct implementation. Tighten it to an attribute-name match, and add the<!---in-attribute fixture from C2.Mutation check (required)
Setup in
/tmponly (the read-only worktree was never written to):git archive 2e0d9b8 lab-director | tar -x -C /tmp/delta570, copy inweb/build, baselinego test ./...all ok. Each mutation applied on a pristine checkout, thengo test ./server/:server.go:140SecurityHeaders(s.opts.Dev, s.spa.InlineScriptHashes())→SecurityHeaders(s.opts.Dev, nil)— the exact B1 revertTestProductionCSPCoversInlineScripts,TestProductionCSPCoversTheRealBuildweb/buildhidden (the CI condition)TestProductionCSPCoversInlineScripts/sveltekit-shaped_shellstill caught itstatic.go:79inlineScriptHashes: inlineScriptHashes(index)→nilTestNewSPAHandlerDerivesHashesFromTheServedBytesmiddleware.go:119productionCSP(scriptHashes)→productionCSP(nil)static.go:178hashbytes.TrimSpace(content)(right script, wrong bytes)static.go:174case-sensitivebytes.Index(..., "</script>")TestInlineScriptHashExtraction/upper_case_tagsstatic.go:169drop thehasAttribute(..., "src")skipstatic.go:146disable comment skipping…/a_comment_mentioning_a_script_is_not_a_scriptAll seven mutations failed; the pristine tree passes
go vet ./...clean andgo test -race -count=1 ./...green (CI's exact command). Independent non-Go browser control: with the emitted policy the cockpit mounts; withscript-src 'self'it is blank and Chromium logs the violation naming the byte-identical hash. The precompressed path serves the same policy (Accept-Encoding: br), and the committed Playwright suite passes 17/17 in dev mode against the real binary.Territory check
git show --stat 2e0d9b8→ 5 files, 638 insertions, 18 deletions, alllab-director/cockpit/server/*; the diff under review is byte-identical to the real commit delta.SecurityHeadershas exactly one caller (server.go:140); the dev CSP block is byte-identical tob688ecf; no'unsafe-inline'/'unsafe-eval'/nonce-reaches the deployed policy; hashes are computed once inNewSPAHandler(static.go:79), no per-request cost. Robustness: 1.78M randomised documents through the extractor produced no panic and no hang.Not verified
The image smoke and a real
docker build(no docker socket) — the embed path is covered by readingDockerfile:91-92+cmd/cockpit/embed.goand bynpm run e2epassing against a binary from this tree. CI itself (the skip claim comes from readingci.yaml:101/119/142+ reproducing the condition locally). Future SvelteKit/adapter versions. Service-worker registration under the production CSP is a probe artifact (identical with and without the fix);e2e/shell.spec.tsspec 15 passes in dev mode.Class-D visual check (R11) — head
2e0d9b8Run against the real binary built from
2e0d9b8(dev mode,COCKPIT_STATIC_DIR=web/build), plus a production-CSP positive control. Viewports 1440×900 and 390×844; routes/,/login,/unauthorized. Screenshots captured for all six combinations.Result: B1 is verifiably FIXED, but the check found one open blocker (the PWA service worker never installs) plus responsive/accessibility defects. Landing withheld — see the landing note.
B1 re-verified as fixed (positive + negative control)
Static server over the real
build/, real SvelteKit document:script-src 'self' 'sha256-jT5G81Y9O3oya+N28K/IVhaAaBXCxx36y9fLXVaTDbc='(computed independently from the builtindex.html): app mounts (wordmark + shell rendered, bodyLen 805), zero CSP violations.script-src 'self'(the old defect policy, negative control): blank page (bodyLen 0, no wordmark), console names the identical hash the Go extractor emits.So the document and the policy now agree. Good.
BLOCKER — the PWA service worker never installs (silent, on every page load)
TypeError: Failed to register a ServiceWorker for scope ('…/') with script ('…/service-worker.js'): ServiceWorker script evaluation failed.navigator.serviceWorker.getRegistrations()is 0 at t+1.5s and t+7.5s;navigator.serviceWorker.readynever resolves (times out);controlleris null. No offline shell ever activates.web/src/service-worker.tsuses the SvelteKit service-worker globalsbuildandfileswithout importing them. The built artefact is literallyconst n="ld-cockpit-shell-v1",o=[...build,...files,"/index.html"];…—build/filesare undefined identifiers at evaluation, so the worker throws aReferenceErrorbefore it installs.grepconfirms noimport … from '$service-worker'and no ambientdeclare const build|filesanywhere undersrc/. (new Function(src)parses fine — it is a runtime, not syntax, failure.)import { build, files } from '$service-worker';insrc/service-worker.ts(a one-line change; the comment there already says "buildandfilescome from SvelteKit's own manifest").The e2e that claims to cover it is vacuous (R11 concern)
e2e/shell.spec.ts:95-114asserts onlynavigator.serviceWorker.getRegistrations().length > 0. Reproduced against the real binary: that assertion passes in 194 ms on a transient registration object that dies immediately — after waitForFunction resolves,countis back to 0,readytimes out, and the pageerror fires. The test never assertsactive/activated/ready, so a greencockpit-e2e(3m39s, run #1679) says nothing about whether the worker works. This is the same class of failure the brief calls out ("the legacy repo's e2e was dead code — do not repeat that"). Suggest assertingawait navigator.serviceWorker.ready(orregistration.active.state === 'activated') after a real fetch-through.HIGH — mobile horizontal overflow (all three routes)
At 390×844 the document is 522 px wide (
scrollWidth522 vsinnerWidth390) → a 132 px horizontal scroll on/,/loginand/unauthorized. Offenders are in the always-present app header: adiv.flex.items-center.gap-2at x 315–522, the connection-dot button (356–436), the identity chip and the "sign out" button (444–522). The header does not wrap or shrink below ~520 px. Desktop 1440 is clean (scrollWidth == innerWidth).MEDIUM — pointer targets below the 24 px minimum (WCAG 2.5.8 AA)
Header controls are 21 px tall (and 16 px for the identity chip):
devbadge 33×21, connection-dot button 104×21,dev-anonymous107×16, the threeunwiredbuttons 48×21. None meets 24×24; none approaches the 44 px mobile guidance. (Desktop/: 7 elements < 24 px; mobile: 1–4.)MEDIUM — small-text contrast below AA
Canvas-resolved ratios (my first probe mis-parsed
oklch, so these are re-measured properly): muted/faint tokens at 10–12 px land at 3.91–4.19 : 1 against a 4.5 : 1 requirement. Counts: desktop/29/48 checked,/login8/21,/unauthorized12/26; mobile similar. Representative:403 · not authorized(3.91), thedtlabelsrequired role/your roles/subject(3.91),devbadge (4.19), the P-COCKPIT invariant lines (4.19). Body text is fine; it is the muted tokens that miss.LOW — 10 px type
Many labels render at 10–11 px (26–27 elements on the dashboard: invariant ids,
uptime/server time/sourcesmeta-labels). Dense operational UI justifies small type, but 10 px is below the usual floor.What is correct
All routes 200 and mounted; no CSP violations under the real production policy; connection dot reports
data-state="live"; the uptime actually ticks (29s → 31s), so the SSE→read-model bridge works end-to-end; the wiring census renders 3 rows, allunwired, each with a real sentence naming the missing key/ticket; 3 panes; no console errors other than the service-worker one; no overflow at 1440.Not measured
Real device/touch behaviour; Safari/Firefox (Chromium only); a11y tree (axe) — out of scope for this pass. The SW and overflow findings were independently reproduced more than once.
Landing worker — MERGE WITHHELD on a class-D blocker (head
2e0d9b8)All three reviews are posted after the current head:
b688ecf): 1 blocker (B1) · 5 concerns · 6 suggestionsb688ecf): 0 blockers · 4 concerns · 5 suggestions2e0d9b8: 0 blockers · 2 concerns · 3 suggestions —B1 closed: yes(7/7 mutation regressions fail the suite)CI is green on
2e0d9b8(run #1679:cockpit-test,cockpit-e2e3m39s,helm-validate,receiver-test,dispatcher-test,alert-receiver-test; run #1680:build-and-push2m28s).The merge is withheld because the class-D visual check (R11) found an open blocker that no test currently catches: the PWA service worker never installs.
web/src/service-worker.tsuses SvelteKit'sbuild/filesglobals without importing them from$service-worker, so the builtservice-worker.jscontains[...build,...files,"/index.html"]and throws aReferenceErroron evaluation — reproduced:getRegistrations()= 0,navigator.serviceWorker.readytimes out, and a console error on every page load. "SvelteKit shell + PWA" is a named 3.1a deliverable (design brief lines 17, 127).Compounding it, the e2e that claims to cover the PWA (
e2e/shell.spec.ts:95-114) asserts onlyregistrations.length > 0, which passes in 194 ms on a transient registration that dies immediately — precisely the "dead e2e" failure R11 warns against.Full evidence, plus the HIGH mobile-overflow (522 px content in a 390 px viewport) and the MEDIUM pointer-target/contrast findings, are in the class-D comment above. B1 itself is confirmed fixed (positive + negative control).
Recommended next step (small, one fix commit): add
import { build, files } from '$service-worker';, strengthenshell.spec.tsto awaitnavigator.serviceWorker.ready(not just a registration object), fix the header's mobile overflow, and bump the muted tokens to ≥4.5:1. Then re-run the delta review + visual check.No merge was performed and no CI run was triggered by this comment; the head is unchanged.
Review verdict 4/4 — DELTA review of the PWA/responsive fix (fresh context)
Verdict:
BLOCKERS: 0 | CONCERNS: 4 | SUGGESTIONS: 3—BLOCKER-CLOSED: yes— for heada6fd65feb42c1efdd75b91ef10ad092f20a00e84(delta2e0d9b8→a6fd65f, 7 files, alllab-director/cockpit/web/**).Independent fresh-context reviewer (not the implementer, not any earlier reviewer; it was explicitly barred from reading prior verdicts). It verified against the bytes the image ships and proved closure by mutation in both directions — a pre-fix worker fails the strengthened spec for the right reason, and a pre-fix UI fails the new responsive spec 9/9. The landing worker reproduced the same conclusions independently (positive + negative control, built artefact, headless Chromium).
Posted by the landing worker. One concern is a flake in the new test (C1) — recorded below; it is a false-red risk, not a false-green, and is not a merge blocker.
VERDICT: BLOCKERS: 0 | CONCERNS: 4 | SUGGESTIONS: 3BLOCKER-CLOSED: yes— strongest evidence: I reverted onlysrc/service-worker.tsto2e0d9b8, rebuilt, and ran the spec:and the built fixed artefact resolves the import (no bare specifier left):
Blockers
None. The shipped artefact is correct: the PWA installs and activates, the e2e is load-bearing in both directions, the 390 px overflow is gone, and the contrast numbers are real (independently reproduced below). The defects I found are in the test and in cache hygiene, not in the deliverable — so per the brief's blocker definition (artefact wrong / acceptance criteria unmeetable / a test that proves something it doesn't) they are concerns, with the first one called out as the one I'd fix before trusting CI.
Concerns
shell.spec.ts:172-175is racy against the correct worker — it flaked on me. Immediately afternavigator.serviceWorker.readyresolves, Chromium'sregistration.active.statecan still beactivating. My first (cold-server) run of the fixed tree: The next 4 runs of the same command passed (14.7s, 2.5s, 2.4s, 2.3s) — so 1 failure in 5 runs, ~20% in my sample, on the cold path that CI always takes. This directly weakens the commit's "mutation proof … passes against the fix" and will flake the landing gate that merges on green CI. Fix:await page.waitForFunction(() => navigator.serviceWorker.controller?.state === 'activated')(or acceptactivating|activatedat line 175 and keep the separatecontroller !== nullwait, which is already there).context.setOffline(true)+page.reload()(shell.spec.ts:~200) can be satisfied by Chromium's HTTP disk cache — the SW'sfetch()passes through and a cached 200 is indistinguishable from the SW cache's 200;reloaded?.status()cannot tell them apart. And because my mutation failed at the first assertion (ready), the offline assertions never executed under mutation, so the commit's "mutation proof" covers only the install claim, not the offline claim. Alsoreadmodel-erroris never asserted hidden before going offline (it was genuinely absent in dev-mode before that point, but nothing pins it). Fix: assertawait page.evaluate(() => caches.match('/index.html').then(r => !!r))(orcaches.keys()naming the versioned cache) inside the offline block, andawait expect(page.getByTestId('readmodel-error')).toBeHidden()beforesetOffline(true).activatedeletes every cache on the origin, not just its own (src/service-worker.ts:56-60:keys.filter((key) => key !== CACHE)). On a shared origin this nukes unrelated apps' caches; and with a per-buildversion(concern 4) every deploy wipes the cache a still-open tab depends on, so a lazily-imported hashed chunk for that tab fails offline. Fix: filter on the prefix —keys.filter(k => k.startsWith('ld-cockpit-shell-') && k !== CACHE).versionis1790911337390— kit 2.70.3's defaultconfig.kit.version.nameisDate.now()(node_modules/@sveltejs/kit/src/exports/vite/index.js:1717,src/types/ambient.d.ts:78-80). It changes every build (including dev-loop rebuilds), so it is a new cache name per rebuild, not per release; combined with concern 3 this deletes old caches constantly. Character set is digits only (safe), and it is never empty. Real but low-impact; a kitversion.nameset to the git SHA would be honest and stable within a build.Suggestions
source-row.svelte:53—min-h-[var(--target-min)]on a<p>is cosmetic: a<p>is not a control and the spec's selector list (a,button,[role="button"],input,select,summary, responsive.spec.ts:125) never measures it. The real target-size fix there is the bits-uiTooltip.Trigger(renders a<button>) growing via the inner span'smin-h(source-row.svelte:47-53) — worth a comment saying so, and the<p>min-h only adds vertical space.sm:wrap point are untested.hidden … sm:blockon both separators (app-header.svelte:82, 88) means the firstsm:-visible separator appears at 640 — untested.waitForTimeout(300)in all 9 responsive tests (responsive.spec.ts:169, 181, 189) is a fixed sleep before measurement; apage.waitForLoadState('networkidle')or adata-testidreadiness wait would be deterministic and faster.Claim-by-claim
import … from '$service-worker'is the documented idiom andversiona real exportbuild/files/versionwith no bare specifier (commands above);versionis ambientnode_modules/@sveltejs/kit/src/types/ambient.d.ts:80and emitted atsrc/exports/vite/index.js:1717;resolveIdfor$service-workeratsrc/exports/vite/build/build_service_worker.js:84-90ServiceWorker script evaluation failed; fixed worker → passes 4/5 runs (1 flake, C1)9 failed, each scroll testscrollWidth 503 > clientWidth 390, offendersdiv[identity-chip],button "e2e-operator",button "sign out"atleft 314 → right 503color-mix(in oklch, edge 70%, transparent)1px over the 7px gap, app.css:108-118): ink-muted 6.80, ink-faint 4.68; over ground/surface/raised 8.78/8.20/7.49 and 6.04/5.64/5.15. Old values reproduce the previous review's numbers: ink-faint 0.56 → 3.92 surface, 4.20 ground (the spec's own canvas measured 3.91/4.19)/,/login,/unauthorized(the earlier 522/132 was a different measurement; the commit's number is the one I reproduce)9 failed; fixed tree: the 9 responsive tests pass (they were among the 17 passed in my first run)shell.spec.ts -g "PWA shell"on the fixed tree failed withExpected: "activated", Received: "activating"(C1)min-hgrows; the<p>in source-row.svelte:53 is not a target (S1). Header controls at 36 px, not 44 px — stated and justified in the commit, and 24 px (2.5.8 AA) is what the spec enforcesgit diff --stat 2e0d9b8..a6fd65f= 7 files, alllab-director/cockpit/web/**playwright.config.tstestDir: 'e2e',testMatch: '**/*.spec.ts';.forgejo/workflows/ci.yaml:192runsxvfb-run -a npx playwright testwithE2E_PREBUILT=1Mutation check
src/service-worker.tsfrom2e0d9b8,npm run build, ranE2E_PORT=18212 npx playwright test e2e/shell.spec.ts -g "PWA shell"→ 1 failed, 0 passed, reason stated in the diagnostic:navigator.serviceWorker.ready did not resolve within 20s (… ServiceWorker script evaluation failed). Right reason, not a timeout artefact.npx playwright test e2e/shell.spec.ts e2e/responsive.spec.ts→ 17 passed, 1 failed (the failure was C1'sactivatingrace, not the fix). Then 4×-g "PWA shell"→ 1 passed each.app.css,app-header.svelte,identity-chip.svelte,source-row.svelteto2e0d9b8(kept the fixed SW), rebuilt, ran the full responsive spec → 9 failed, 0 passed, withscrollWidth 503 > clientWidth 390on all three routes, the target-floor failures, and the contrast dump showingoklch(0.56 0.012 272)atratio 3.91/4.19.Not verified
min-h-12+py-1.5with the tallest child atmin-h-[var(--target-comfortable)](36 px) = 48 px, but I did not measure it, and any taller child (e.g. thesm:version span withmin-h-36siblings) would grow the row.class="min-h-[var(--target-comfortable)]"on<ConnectionDot>is actually applied (I confirmed the component declaresclass?: stringatconnection-dot.svelte:19but not that the root consumes it) — the e2e target measurement passing is consistent with either.git diff --statshowing nopackage.jsonedit./home/aaron/repos/homelab/.rpiv/artifacts/build/ld3-1681a-landing/**(no prior verdicts consumed).Commands run
Working copies:
/tmp/review-a6fd65f/**(scratch only). The LD3 worktree at/home/aaron/repos/homelab-wt/LD3/infra-corewas never modified (itssrc/reverts were performed on the scratch copy;go buildoutput and the Playwright run wrote outside the worktree).Landing worker — class-D visual re-check on
a6fd65f(R11)Rebuilt from scratch (worktree
.worktrees/final-570@a6fd65f, read/build only):npm run build(18.75s, +.br/.gzvariants) andgo build ./cmd/cockpit; served the real binary in dev mode; headless Chromium (channel: chromium), viewports 1440x900 / 390x844, routes/,/login,/unauthorized.Result: clean. No blockers. The blocker from visual check #1 is verifiably fixed, and the desktop shell is unchanged.
Fixed — the PWA service worker installs, activates, and answers offline
build/service-worker.jsnow inlines SvelteKit's manifest (const a=location.pathname.split("/")...,o="1790911380147",n=\ld-cockpit-shell-${o}`) —grep -c '$service-worker'= **0**, no bare...build/...files`, declaration header parses.navigator.serviceWorker.readyresolved in 2004 ms;registration.active.state=activated; scope = origin root;navigator.serviceWorker.controller=/service-worker.js(the page is controlled). Zero service-worker evaluation errors.context.setOffline(true)+ reload → HTTP 200, wordmark rendered, 3 panes;/api/v1/readmodelthrew (never served from a cache) and[data-testid=readmodel-error]was visible..br+.gzall replaced byconst SHELL = [...build, ...files, '/index.html']— on a fresh origin:readydid not resolve (15s),getRegistrations()= 0, pageerrorServiceWorker script evaluation failed. So the strengthened spec's assertions watch exactly the failure that existed.Fixed — no horizontal overflow at 390 px (this was HIGH)
/,/login,/unauthorizedat both 390x844 and 1440x900:scrollWidth == clientWidth(390 and 1440),offenders: []in all six cases. Prior head: 503-522 px. Extra probe at 320 px: also clean, identity chip visible and the sign-out button visible/clickable.No desktop regression
1440x900: header is the single 48 px row, 3 panes, identity chip + connection dot visible, 2 vertical separators visible, layout identical to before (screenshot inspected). Phone screenshot inspected too: wordmark row, status row, hatched wiring census, all three panes render.
Contrast — the commit's numbers are real (independently recomputed)
oklch -> sRGB -> WCAG:
--color-ink-muted(0.75) 8.78 / 8.20 / 7.49 over ground / surface / raised, and 6.80 over the hatch stripe;--color-ink-faint(0.65) 6.04 / 5.64 / 5.15 and 4.68 over the hatch stripe. The token comment's "worst case ink-muted 6.8:1 and ink-faint 4.7:1" is accurate.--color-status-idlemeasures 4.22:1 onraised, but it is used only as a dot background (connection-dot,source-row), never as small text, so it is out of scope.Observations (no action required to land)
unprotected/ connection dot / dev badge + identity + sign out), where the component comment and the commit message say "wordmark row + right-aligned status row". Functionally correct with no overflow; worth a comment tidy-up, and ~15% of a 390 px viewport is chrome if a later slice revisits the frame.e2e/responsive.spec.tsresolves text backgrounds fromgetComputedStyle(...).backgroundColoronly, so the hatch stripe (background-image) is never composited — the spec's contrast floor is the plain-surface case (5.15:1), while the token comment's documented worst case (4.52-4.68:1) is the stripe. The tokens are correct today; the test just cannot see the case the comment says defines the floor.active === 'activated'assertion can racereadyresolving while the worker is stillactivating: I observedactive: "activating"atready-resolution on two independent runs, and the reviewer's cold-path run failedExpected: "activated" | Received: "activating"(1 in 5). CI ona6fd65fpassed (cockpit-e2e, 3m47s) andretries: 1in CI masks most of it, so this is a false-red risk in the test, not a false-green about the product — it does not gate this merge, but it should be fixed before the suite is relied on as a gate for 3.1b.