HOMELAB-1683: feat(cockpit-web): LD3.3 frontend half — sandbox attach (xterm.js pane, refusal postures, audited honesty) #584
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!584
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "plane/HOMELAB-1683-attach-ui"
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?
What
LD3.3 frontend half (
LD3.3-ui): the sandbox attach surface. 5 commits,lab-director/cockpit/web/**only — no Go, noci.yaml, no manifests, no flags.GET /api/v1/attach/{sandbox}/info, rendering the backend's three postures: enabled, disabled with the reason, or disabled because the cockpit cannot look (no registry read). The column is second, not last — measured: at 1440px the Fleet Overview's middle column is ~520px while the table wants ~640px, so a trailing column put the operator's one action off the right edge on the screen it was built for.<body>with the LD3.2b viewport guard re-applied.session.svelte.tsprobesinfobefore it opens a socket, which is the contract rather than an optimisation: a browser'sWebSocket.onerrorcarries no status and no body by spec, so a client that opened first could not name the reason for the failure it hits most often (flag off → 503). A refusal therefore renders with no socket, no upgrade, no ledger open — and the dialog draws no terminal element at all until a session has actually opened.P-ATT-00Npolicy line + "refused and recorded in the attach ledger", with the backend's own sentence verbatim (never a client paraphrase).The one scope judgement to review
The rows carry the affordance, but the cockpit has no sandbox-list read at all —
infois a lookup BY ID, and the read-model census names four sources, none of which enumerates sandboxes (the wave table says so in its own words). Rows-only would have shipped an unreachable feature: zero rows, zero ids, and the ticket's own acceptance ("operator attaches from the app into a live sandbox") impossible.The alternatives were (a) fabricate census rows — the single worst lie this application could print, and the one its own table refuses to print — or (b) make the capability reachable by naming a target. I chose (b): an attach-by-id launcher under the table, labelled with the missing registry read (LD3.4), reusing the same info probe and the same dialog. When the census lands it becomes the escape hatch an operator wants mid-incident anyway. If you'd rather defer it, say so and I'll gate it behind the census — the row path is complete without it.
Dependency
Merges
origin/plane/HOMELAB-1683-attach-bridge(PR #579) as its base, because this slice wires against that wire contract and its fixture parity guard readsattach/attach.go,attach/bridge.goandattach/protocol.gofrom the Go source. #579 must land first (it is mergeable; #579's diff is unchanged here). If #579 changes before it lands, the guard fails loudly rather than drifting.Test plan
Counts before → after (asserted, not inferred — measured by stashing this branch's changes and re-running):
New coverage, by layer:
Infojson tag set, theTermstruct's absence of tags (the PascalCase trap), theRefusalKindvocabulary and status map, the no-TTY 200 override,registryWiring,DefaultTerm, the close-reason vocabulary, and the two refusal sentences character for character.disabled/audit_recorded), the three postures from fulfilled payloads, the dialog landing on the honest refusal with no terminal in the DOM, the portalled-to-<body>structural check, and the viewport guard at 1440 + 390.Measured at the tip: at 390px the open dialog sits 675..836 inside an 844px viewport with its backdrop at 0..844, and
documentElement.scrollWidthis 390 against a 390px clientWidth (no sideways scroll). At 1440px: dialog 369..531 inside 900, backdrop 0..900, scrollWidth 1440 == clientWidth 1440.Screenshots
/home/aaron/repos/homelab/.rpiv/artifacts/build/ld3-1683b-screenshots/01-sandbox-row-attach-cell-no-id-1440.png02-attach-button-disabled-live-1440.pngP-ATT-001, the server's own reason03-attach-button-no-tty-1440.png04-attach-button-attachable-1440.png05-attach-dialog-refused-1440.png06-attach-dialog-refused-390.pngOpen items for the landing reviewer
attach. PR #579 deliberately left it out ("the census row lands with the xterm.js pane slice, which owns that guard") — but that row needsreadmodel.ExtraSourcesincmd/cockpit/main.go, i.e. Go, and this slice's bound isweb/**only. So the attach seam declares itself where it can without Go: the info answers, the boot log and--check. The census row + thee2e/shell.spec.ts"unnamed fifth row" guard extension is a follow-up ticket, not a gap I can close from here.sr-onlycontaining-block trap is documented in the cell (relativeis load-bearing). Worth knowing if another cell ever hides a sentence.Adds the attach bridge behind LD_ATTACH_ENABLED (default false): the cockpit/** mechanism, the server wiring and the refusal matrix, with no manifests, no RBAC, no CNP, no dispatcher change and no web/**. attach/ (new): - Registry seam (read-only interface, recording fixture, and the refusing unwired registry that names the unratified RBAC) — the live CRD reads wait for the operator's ratification package §4/§5. - Executor seam + client-go impl: remotecommand.NewSPDYExecutor over pods/exec with Tty:true and a TerminalSizeQueue; construction reads a kubeconfig (LD_ATTACH_KUBECONFIG, empty = in-cluster) and contacts nothing. - Refusal matrix with a P-ATT-001..008 policy book, the class harness allowlist (spawn's own class constants; unknown class refuses), and the ld.audit.attach/v1 writer (audit.Entry gains attach kinds, outcomes and fields; Validate enforces the schema/kind/outcome pairing both ways and keeps sentinel entries free of attach fields). - Wire() is the one place a kube client can be constructed: disabled builds none (asserted by a construction counter), enabled builds one and refuses per attempt while the registry is unwired. server/: - GET /api/v1/attach/{sandbox}/info and WebSocket /ws/attach/{sandbox}, registered outside RequireOperator so that a refused handshake is still a ledger entry; identity resolution, refusals and the WS protocol all audit (open/close/refused), statusLogger gains the Hijack passthrough a websocket upgrade needs through the chain. - One attach_open is answered by exactly one attach_close (a failed dial writes the aborted close), and an attach never opens when its open entry cannot be written. config: LD_ATTACH_ENABLED (default false) + LD_ATTACH_KUBECONFIG; a kubeconfig configured while the flag is off is refused as ambiguous. main: attach declared in the read-model census, in --check's OK lines and in the boot log ("no kube client constructed"). Tests: 192 pass under -race (attach 32, audit 9, server 47, config 13 new or extended); full refusal matrix, WS protocol over a real socket (upgrade/resize/EOF both ways/abrupt disconnect/protocol error/slow consumer), ledger pairing and entry JSON shape.The frontend half of LD3.3 starts at the contract. Every shape in types/attach.ts was read off the Go it mirrors (attach.Info, attach.Server*Frame, attach.RefusalKind, server.attachRefusal) and then VERIFIED by marshalling those structs and diffing the JSON — which is how two things the Go doc comments get wrong were found: - attach.Term carries NO json tags, so the info payload's terminal object is {"Columns":120,"Rows":32} (PascalCase), NOT the cols/rows the comment above the type claims. normaliseTerm accepts both spellings so a future tag addition is a no-op rather than a silent 0x0 geometry. - class_without_tty is answered 200, not the refusal kind's own 409: the read succeeded, so a 4xx would tell the frontend its request was wrong. attachApi.info therefore does NOT throw on non-2xx. The backend answers a decidable refusal as HTTP so a browser's onerror and an operator's curl see the same JSON — but a browser cannot read a failed WebSocket handshake's status at all, which is why every 400/401/403/404/409/502/503 is a POSTURE the button renders rather than a transport failure it retries. xterm 6.0.0 and @xterm/addon-fit 0.11.0 are pinned exactly (both are lazily imported, so they stay out of the home screen's first-paint chunk). formatBytes joins the format helpers for the session status strip.The UI half: a per-row attach cell, a by-id launcher, an xterm terminal and the Sandbox Environment View as a modal (design Part 2 §4.1 screen 4 — "what is this agent doing, and let me type into it"). Three decisions worth a reviewer's attention: - The rows carry the affordance, and the LAUNCHER exists because the cockpit has no sandbox-list read at all: info is a lookup BY ID, and the read model's census names four sources, none of which enumerates sandboxes. Rows-only would have shipped an unreachable feature. The launcher is the same posture as every other seam here — declare what is missing, make what IS possible possible — and the empty row renders a disabled button WITH the reason rather than no button, because "the registry is not read" and "this sandbox cannot be attached" are different facts. - The attach column is SECOND, not last, and that was decided by measurement: at 1440px the Fleet Overview's middle column is ~520px while the table wants ~640px, so a trailing column put the operator's one action off the right edge on the screen it was built for. - The dialog draws NO terminal until a session has actually opened. A refusal renders the status alone; an empty xterm below it would read as a shell that has not printed yet, which is the dead pane R6 forbids. It is portalled to <body> (the LD3.2b containing-block lesson), and its height is the terminal's height — with no terminal it shrinks to its five lines of status instead of wasting the screen. One measurement worth recording, because nothing in review would have found it: the visually-hidden reason sentence is position:absolute with no positioned ancestor, so it escaped the table's overflow-x-auto scroll container and grew the DOCUMENT's scrollWidth to 575px at a 390px viewport — the exact sideways-scrolling page the 3.1a responsive net exists to catch, and it caught it. Anchoring the cell (position: relative) keeps the sentence inside the column the scroller clips.14 new Playwright specs (48 -> 62), against the real cockpit binary in dev mode serving the real frontend build. The split is forced by what this build can produce. LD_ATTACH_ENABLED is false by design (the RBAC/CNP/arming delta is the operator's ratification package), so the LIVE server answers 503 "attach disabled" on both transports and records every attempt — the disabled posture is therefore asserted completely end to end, including reading the response the page actually received to check `disabled` and `audit_recorded` are true. An attachable answer is impossible live, so the other two postures run with the info payload fulfilled in the browser, from the same fixtures the Go tests assert. The most valuable spec is the one that follows the real refusal through the frontend: a fulfilled "attachable" answer opens the dialog, the live server then refuses the WebSocket upgrade, and the dialog has to land on an honest end state — the disabled refusal, its policy line and "refused and recorded" — with NO terminal element in the DOM. It is the R6 rule ("never a dead black box") as an assertion rather than a promise, and it is also the path a real operator hits whenever the registry moves under them. The viewport guard from LD3.2b is parameterised rather than copied, so the attach dialog — a second portal-to-body surface, mounted from inside a pane nested under the sticky blurred header — gets the same measured containing-block assertions at 1440 and 390, plus a structural check that its parent really is <body>. Measured at the end of this branch: at 390px the open dialog sits 675..836 inside an 844px viewport with its backdrop at 0..844, and the document scrollWidth is 390 against a 390px clientWidth.Builder report — implementation complete, CI green
Head:
126af06113· CI: run #1730 (cockpit-test✔ 2m39s,cockpit-e2e✔ 2m51s,dispatcher-test✔,helm-validate✔) and run #1731 (build-and-push✔ 2m3s) — all pass.Local verification (at the tip, full paste)
Counts asserted, not inferred
Measured by
git stash -uon this branch and re-running the untouched tree:Every pre-existing spec is green:
shell.spec.ts(including the "unnamed fifth row" census guard, still 4 rows),panes.spec.ts,gates-configured.spec.ts,responsive.spec.ts(all three routes at 390 and 1440),auth.spec.ts.390px, measured directly with the dialog open
scrollWidth == clientWidthat both widths — no sideways scroll — and zero terminal elements are drawn for a refused session.Three bugs this slice found in itself (all fixed, all now tested)
sr-onlycontaining-block escape. The visually-hidden reason sentence isposition: absolutewith no positioned ancestor, so it left the table'soverflow-x-autoscroll container and grew the document'sscrollWidthto 575px at 390px.responsive.spec.tsandpanes.spec.tswent red naming no offender (the element is invisible). Fixed withrelativeon the cell; documented in the component.formatDurationfed milliseconds — the status strip rendered a 61-second session as "17h 05m".instanceof ArrayBufferis realm-local. Under jsdom aUint8ArrayfromTextEncoderfailed the check, so terminal output was misclassified as a control frame. NowArrayBuffer.isView+ a tag read.Two fixtures that were wrong about the wire (found by the parity guard + the live binary)
wiringarray on the disabled answer: the flag-off refusal is raised with an empty wiring argument, so the key is named in the reason and the payload carries no list. Onlyregistry_unwiredpopulates it.Evidence
/home/aaron/repos/homelab/.rpiv/artifacts/build/ld3-1683b-builder.md(full record) andld3-1683b-review.html(all six screenshots on one dark page)./home/aaron/repos/homelab/.rpiv/artifacts/build/ld3-1683b-screenshots/—01row cell /02live disabled /03no-tty /04attachable /05dialog @1440 /06dialog @390.For the landing reviewer, in priority order
attach— needs Go (cmd/cockpit/main.goExtraSources), outside this slice's bound. Follow-up ticket.Review 1 — contract/security lens (fresh context,
deepseek/deepseek-flash, head126af0611)Independent reviewer, no author context. VERDICT: BLOCK
Blockers:
The session's displayed byte accounting is wrong: every output byte is counted as BOTH
bytesInandbytesOut, and no input byte is counted at all.web/src/lib/attach/session.svelte.ts:204-207—emit()doesbytesOut += bytes.byteLength; its only caller is:332-333, which already didbytesIn += message.bytes.byteLengthfor the same server→client data frame.sendInput()(:540-543) increments nothing.web/src/lib/components/attach/attach-status.svelte:70renders{formatBytes(session.bytesIn)} in · {formatBytes(session.bytesOut)} out, i.e. two identical numbers that can never reflect the operator's keystrokes. The backend's direction is the opposite (cockpit/server/attach.go:bytesIn= client→sandbox keystrokes,bytesOut= sandbox→client output, the same pair the ledger andServerCloseFramecarry).attach-status.svelte:70reads the store counters, whileend.bytesIn/bytesOut(session.svelte.ts:402-403) are only reachable throughend, which the accounting line does not use.web/src/lib/components/attach/attach-status.test.ts:137). The store test codifies the bug:session.svelte.test.ts:393assertsbytesOut === 15after 15 received bytes, whileattach-status.test.ts's stub feedsend.bytesIn/outintosession.bytesIn/out(:43-44), so the component test cannot catch it.after 13 bytes of OUTPUT: in = 13 out = 13/after 3 bytes of INPUT: in = 13 out = 13.The cockpit's 401 (and 403) attach refusals are converted into a transport failure, so the UI prints "unreachable · API error 401" for a refusal the backend decided, reasoned about and audited.
web/src/lib/api/client.ts:274+:279+ the predicate at:308-317:isAttachAnswerrequiresreason|attachable|disabled|has_tty.server.writeAttachRefusalanswers an unauthenticated/api/.../infowiths.opts.Auth.DenyUnauthenticated(cockpit/server/attach.go:249-253), which for an/api/path (wantsJSON,auth/middleware.go:288-293) writes the Problem envelope{code,detail}(auth/middleware.go:236-237,299-311) — noreason, noattachable. SoattachApi.infotakes thethrow new ApiError(res.status, body)branch.unreachablePosture(error.message):attach-cell.svelte:76-79,attach-launcher.svelte:73-76,session.svelte.ts:~250.ApiError.messageisAPI error 401, andApiError.detail— which does hold the server's sentence — is not consulted. The operator sees chip "unreachable", tonebad, reason "API error 401", andexplainUnexplainedClose/startlabel the end statekind: 'failed'/code: 'probe_unreachable'("the cockpit did not decide anything").posture.ts:179-190(401),:192-203(403) and the entirepostureFromStatus/STATUS_REFUSAL_KINDpath (:113,:282-330) are dead code on the real wire. The server's verbatim sentence — "Sign in at /auth/login to use the cockpit." — never reaches the operator, and the distinction this lens asks about ("refused by policy" vs "cockpit cannot look") is inverted: the cockpit did look, and refused.unauthorizedtick (web/src/routes/+layout.svelte:45,49,77), andsession.me.operator === falseat boot. A session that lapses after boot leaves the SPA up, so the next attach probe is a 401 → "unreachable". The e2e cannot see this (dev identity always carries the role,auth/middleware.go:200-213), and no unit test importsattachApiat all (grep oversrc/**/*.test.ts).Concerns:
start()never tears down a previous socket, and the dialog has no unmount teardown.session.svelte.ts:213-230resets every field exceptsocket;:307overwrites the reference. The orphan is then unreachable by design — theonclose/onmessageguards comparesocket !== source(:320-323,:327) and return — so nothing ever closes it: the backend keeps the PTY ofserveAttachrunning and leaves anattach_openledger entry with no matching close (P-ATT-005: "every open is answered by exactly one close"). Confirmed empirically: after a secondstart(),sockets built: 2 first.closedWith = null. Reachability is narrow (the modal backdrop atattach-dialog.svelte:120blocks a second attach click, so the concrete trigger is an effect-driven re-startwith a changedsandbox), butsession.svelte.test.ts"ignores a late frame from a socket the operator already replaced" exercises exactly this double-start and asserts only that frames are ignored — the leak is untested rather than absent. Separately,attach-dialog.sveltehas noonDestroy/effect cleanup;session.close()runs only fromdismiss()(:94), so unmounting the pane (browser back, route change) with a live session orphans the socket and its ledger-open shell.recorded: trueis asserted for everyerrorframe without evidence.session.svelte.ts:414; the rendered claim isattach-status.svelte:104-110("refused and recorded in the attach ledger").attach.ServerErrorFrame(cockpit/attach/protocol.go:110-116) carries no audit flag, yetRefusal.AuditRecordedis false whenever the sink refused the entry — andRefusalAuditUnavailableis raised precisely becauseauditor.Openedfailed (bridge.go,Open). So the one case where the ledger is provably broken is the case where the UI says the refusal is in it. The honest rendering is "the backend does not report whether this entry landed". (Contrast the info path, which does carryaudit_recordedand is rendered honestly.)exists === falsemislabels two named refusals.posture.ts:240mapsexists === falsetokind: 'unknown_sandbox'.RefusalMalformedTargetis returned withSandbox{}(bridge.go,resolve), soInfo.Existsis false and a 409 renders chip "unknown sandbox" beside the server's own sentence "the registry resolved a sandbox the bridge cannot dial." — the label contradicts the reason. Same for status 409 generally: it has no branch inpostureFromInfoat all, despiteattach.gogiving it three kinds. Andsandbox_without_ticket(200) falls through to the genericrefusedbucket (:265-274): honest reason, but the chip drops the named refusal the backend sent. Probe output:409 malformed -> unknown_sandbox | unknown sandbox | the registry resolved a sandbox the bridge cannot dial.No connect budget, so a black-holed upgrade is an empty terminal forever.
showTerminal(attach-dialog.svelte:86-88) is true whilestate === 'connecting', andconnectinghas no timeout — nothing insession.svelte.tsbounds the handshake, andonerrormerely re-assertsconnecting(:~313-317). A proxy that accepts and never completes leaves a blank xterm under "opening" indefinitely, which is exactly the ambiguity R6 exists to remove (the operator can dismiss, which does close the socket, but nothing says the open is stuck).Suggestions:
attach-terminal.svelte:2statically imports@xterm/xterm/css/xterm.csswhile:66-67lazily imports the JS. I built the PR (npm install && npm run build): the JS claim holds — xterm is its own chunk (build/_app/immutable/chunks/B-qIQCd3.js, 329 KB) imported only from the page node and nevermodulepreloaded inindex.html— but the CSS is a page-level asset (assets/2.DDGTF8rc.css, 3.6 KB) pulled in byattach-dialog.svelte's static import, so it is fetched on first paint of the home route, not "when the component mounts" as the comment at:18-22states. Additionally the (pre-existing) service worker precaches[...build, ...files](web/src/service-worker.ts), which includes the xterm chunk, so on an installed SW the lazy import defers parsing, not download. Fix the comment or move the CSS import into the dynamic path.attachInfoPath/attachSocketUrl(client.ts:287,299-302) percent-encode the sandbox id, but Go decodes%2Fintor.URL.Pathbefore theGET /api/v1/attach/{sandbox}/infopattern is matched, so an id containing/fails at the mux (plain-text 404) instead of reaching the handler as an unknown sandbox — i.e. the opposite of the reasoning in the comment at:283-286. Harmless for real ids; the justification is wrong.normaliseTerm(types/attach.ts:42-47) accepts any finite positive number, including non-integers; onlyopenTerm→validateTerm(protocol.ts:284,:78-89) enforces integrality. AddingNumber.isIntegerto the normaliser would make?cols=120.5unrepresentable at the boundary rather than relying on the second check.attach-cell.svelte:56-57states "the sandbox registry is not read yet (LD3.4)" as a fact about the cockpit, but the cell only knows itssandboxprop is null. Once LD3.4 supplies rows, that sentence becomes a client invention about backend state (and the(LD3.4)ticket reference will be stale).No unit coverage for
attachApi.info,attachInfoPathorattachSocketUrl. The socket URL construction (Q2's whole subject) is asserted only indirectly by the e2e; a three-line test forhttp→ws/https→wss, the escaped path and the absent query when the terminal is null would make the contract checkable without a browser. (protocol.test.tsdoes this well for the frame grammar; the URL builder got none of it.)Evidence:
git show origin/epic/LD3:lab-director/cockpit/{attach/wire.go,attach/attach.go,attach/bridge.go,attach/protocol.go,attach/registry.go,server/attach.go}; pluscockpit/server/server.go:189-190(routes),cockpit/auth/middleware.go:185-193,234-263,288-311(DenyUnauthenticated/DenyForbidden/wantsJSON/Problem).web/src/lib/{api/client.ts,attach/posture.ts,attach/protocol.ts,attach/session.svelte.ts,types/attach.ts,components/attach/*.svelte,tests/fixtures.ts,e2e/attach.spec.ts}.npm test→ 25 files, 221 tests, all pass (including the 13-test fixture↔Go parity guard, which confirms theattach.TermPascalCase trap is genuinely handled).package-lock.jsonrestored withgit checkout --):attachApi.infowith the cockpit's real 401 body →THREW API error 401(posture never derived); 403 →THREW API error 403.postureOfwith anInfobody formalformed_target@409 →unknown_sandbox; forsandbox_without_ticket@200 →refused.in = 13 out = 13; then 3 input bytes →in = 13 out = 13; secondstart()→first.closedWith = null.node_moduleswas absent; rannpm install(2 packages) thennpm run buildto inspect chunking:build/index.htmlmodulepreloads only the layout/entry chunks;grep -rl B-qIQCd3 build/_app→ onlynodes/2.CGcRWatI.js(confirms the lazy JS import);grep -o 'href="[^"]*\.css"' build/index.html→ only0.DYSU5LqX.csswhile2.DDGTF8rc.css(the xterm stylesheet) is referenced from the client manifest.@xterm/xterm@6.0.0/@xterm/addon-fit@0.11.0are exact pins (package.json:29-30), self-hosted (no CDN), and thatcss/xterm.cssexists in the tarball with noexportsmap blocking the subpath (npm pack+ unpack).What I verified as correct, for the record: the button's enabled state is derived only from
info.attachable(posture.ts:146-153,attach-cell.svelte:90), and the disabled reason is the server's own sentence where the server gave one; no credential/token is placed in the socket URL and the scheme/host come from the page origin (client.ts:299-302); binary-vs-text is split before parsing (protocol.ts:180-200) and the realm-independentArrayBuffer.isView/toStringchecks are the right call; resize is bounded by the same 1000-cell limit as the backend and withheld rather than sent (protocol.ts:57,110-124);attach.Term's PascalCase geometry is normalised, so the{"Columns":120,"Rows":32}trap does not silently become 0×0; 503-flag-off and 200-class_without_ttyare interpreted correctly (200 +has_tty:false→no_tty, andResizeObserver/window listeners/unsubscribe are all torn down inattach-terminal.svelte:127-140).Review 2 — test-adequacy lens (fresh context,
deepseek/deepseek-flash, head126af0611)Independent reviewer, no author context. VERDICT: BLOCK
Blockers:
The terminal — the thing this PR exists to render — is never mounted by any test, and its mount gate can be pinned to
falsewhile 221 unit + 14 e2e tests stay green.lab-director/cockpit/web/src/lib/components/attach/attach-dialog.svelte:86(showTerminal), consumed at:126(h-[92vh]) and:177({#if showTerminal}).attach-dialog.sveltehas no unit test file (src/lib/components/attach/contains tests for cell/launcher/status only), andgrep -rn attach-terminaloversrc/+e2e/finds exactly one assertion in the whole suite:e2e/attach.spec.ts:177toHaveCount(0).fulfilInfoThenDisabled(e2e/attach.spec.ts:58-72) returns attachable on call #1 and disabled thereafter, but the launcher consumes call #1, so the dialog's own probe is call #2 = disabled and the session refuses before ever reachingstate = 'connecting'(session.svelte.ts:285). Instrumented observation (my temporary spec, MutationObserver withattributeOldValueon[data-state],sawcounter on[data-testid=attach-terminal]):PROBE2-EVENTS ["state:idle->probing","state:idle->probing","state:probing->refused","state:probing->refused"]and the terminal node count{"saw":0}— it is never inserted, not even transiently. So the e2e test's own name ("an attachable answer enables the terminal") and the spec comment ("so the frontend opens a terminal") describe something the spec cannot reach, and line 177 passes for the wrong reason (it does retain exactly one discriminating power: a gate that is true too early, verified — forcingshowTerminal = trueturns it red).showTerminal = $derived(false)→ unit 221/221 GREEN, e2e 14/14 GREEN (nothing detects it);$derived(true)→ e2e RED (test 5). Thereforeattach-terminal.svelte(178 lines: xterm mount + lazy chunk load,term.onData → session.sendInput,session.onOutput → term.write, ResizeObserverfit() → session.resize, theme token resolution, teardown) is 100% unverified, as isshowTerminal'sopened !== nullandlivelegs. Why it matters: the ticket's acceptance ("operator attaches from the app into a live sandbox") is exactly the path with no assertion, and a one-token regression ships green.The fixture↔wire parity guard is silently blind to the two numeric wire bounds it claims to mirror.
src/lib/attach/protocol.ts:57MAX_TERM_DIMENSION = 1000and:62MAX_CONTROL_FRAME_BYTES = 4096are hard-coded restatements of Goattach.MaxTermDimension(../attach/attach.go:109) andattach.MaxControlFrameBytes(../attach/protocol.go:49), and both doc-comments name the Go symbols.fixtures.test.tsreadsattach/attach.go,attach/bridge.go,attach/protocol.goand pins tags, refusal kinds,Status(), both reason sentences,registryWiring,DefaultTermand close reasons — but never these two constants;protocol.test.ts:165-169,180-196andsession.svelte.test.ts:360only compare the constants against themselves.MaxTermDimension = 1000 → 500→ 221/221 GREEN; GoMaxControlFrameBytes = 4<<10 → 1<<10→ 221/221 GREEN (full suite, both times). A lowered Go bound is the direction that bites: the client would then send a resize the backend answers by closing the session withprotocol_error— the precise failureencodeResizeexists to prevent. Blast radius today is small (real panes ≲300 cols), but this is structurally the #578 pattern (a guard that doesn't guard) inside the guard that was written to close it, and the fix is ~6 lines in the same parser.Concerns:
e2e/dialog-viewport.ts:112-123asserts all four dialog edges plusbackdropTop === 0/backdropBottom === viewportHeight, collected into onetoEqual([])— a genuine geometric assertion, not a no-op. But instrumented ate2e/attach.spec.ts:240(1440) and:280(390), the terminal is absent (toHaveCount(1, {timeout:5000})failed at both), so the measured dialog is the small status-only one; theh-[92vh] sm:h-[80vh](attach-dialog.svelte:126) terminal-bearing dialog — the real 390px overflow risk — is measured by nothing, at any viewport. The containing-block canary still catches the #578 class (a header-sized backdrop fails regardless of dialog size), butmin-h-0/flex-1terminal geometry is uncovered.e2e/attach.spec.ts:290computesscrollsInsideSelf: backdrop.scrollHeight >= backdrop.clientHeightand never asserts it (alsocoverat:285is measured and unused); the test name's first half — "the dialog scrolls inside itself" — is unasserted.:297toBeGreaterThan(300)at a 390px viewport is a weak floor for aw-fullbox.postureFromStatus/ theinfo === nullbranch ofpostureOf(posture.ts:129-131,:246-...), andSTATUS_REFUSAL_KIND(posture.ts:113). All four production call sites pass a non-nullinfo; no test callspostureOfat all. The map is load-bearing only forfixtures.test.ts:307-327.attachApi.info,isAttachAnswerandnormaliseAttachAnswer(api/client.ts) were added without touchingapi/client.test.ts. The e2e does route stubbed payloads through the real client, which covers the posture-relevant fields, but thedetail → reasonfallback and thewiringlist path are exercised only through injected probes that bypass the client entirely (e.g.session.svelte.test.ts"pre-bridge refusal body" constructs the merged object itself).formatBytes(format.ts:74, new export) has no unit test —format.test.tsdoes not import it; only two happy values (1024→1.0 kB,2048→2.0 kB) are covered indirectly viaattach-status.test.ts. Boundaries (0, negative, 999, 1000, 999_999, 1e6, non-finite) are untested.AttachRefusalKind(the TS union) is not actually pinned.fixtures.test.ts:286-305compares Go'sRefusalKindconstants against a hand-copied literal list inside the test; a new Go kind plus an updated literal list leaves the union intypes/attach.tsstale and green (the comment at:289claims a compile error, but the union is only used asAttachRefusalKind | string, so it isn't load-bearing).attach-cell.test.tscoversno_id,attachable,no_tty,disabled,unknown_sandbox,unreachable; the row-level rendering ofunwired,unauthenticated,not_operator,invalid_terminal,dial_failedand genericrefusedis never asserted per row.Suggestions:
fulfilInfoThenDisabledso the dialog's own probe also gets an attachable payload (e.g. stay attachable until a flag is flipped, or key on call count ≥ 2), then assertattach-terminalis visible and that a keystroke reaches the wire — that single change converts blocker 1 into the test the spec already claims to be.attach-dialog.svelte.test.tswith the existingFakeSocketseam (session.svelte.tsalready takes an injectedsocket), asserting: terminal mounts on an open frame, keystrokes →sendInput(binary), a resize frame carriesfit()'s geometry, and the terminal stays mounted throughended.MaxTermDimension, andMaxControlFrameBytes— note Go spells it4 << 10, so compare against a computed value or ask the Go side for a literal).session.svelte.test.ts:360assert the refusal viaString(MAX_TERM_DIMENSION)rather than the literal'1000'(the literal survives a change to10000, since"10000-cell"contains"1000").postureFromStatus/STATUS_REFUSAL_KINDor unit-test the null-info branch; today it is untested scaffolding whose only consumer is the parity guard.measured.scrollsInsideSelfine2e/attach.spec.ts(and either rename the "terminal is usable at 390px" test or make it actually mount a terminal).e2e/run-server.shdefaults its binary to a shared${TMPDIR:-/tmp}/ld-cockpit-e2e, which two concurrent worktree runs can clobber (pre-existing, not this PR — worth a follow-up).Parity-guard falsification: Hard-fails in every shape I could construct; no silent skip. Two blind spots proven separately.
Isolated harness:
git worktree add --detach /tmp/rev-1683 126af06(node_modules symlinked from the author's worktree; temp worktrees removed afterwards), mutations applied tolab-director/cockpit/, run vianpx vitest run src/lib/tests/fixtures.test.ts, restored withgit checkout --:attach/attach.gorenamed awayFAIL src/lib/tests/fixtures.test.ts [ ... ] Error: attach fixture parity guard cannot read attach/attach.go — tried … (ENOENT), `Test Files 1 failed (1)attach/dir moved awayattach.gohiddenattach.gogutted topackage attachTermgains`json:"cols"`disabledReasonreworded one wordCloseReasonStreamEOFvalue →stream_endedregistryWiringchip editedRefusalUnknownSandbox404 → 410DefaultTerm120x32 → 100x40DefaultTerm→defaultTermregistryWiring→registryGrantsRefusalNoTTYremoved from the 200-overridecaseNegative control included so I did not over-claim: my first "pattern break" attempt (reformat
DefaultTermacross lines) stayed green — the regex still matched and parsed the same120/32, so green was correct, not a silent skip. Also verified the aggregate guard:npm testprintsfixture parity guard: 13 tests collected (221 tests total)exit 0; withsrc/lib/tests/fixtures.test.tsrenamed away,npm testexit 1 withError: the fixture parity guard (…) collected 0 tests, expected at least 4. Notenpx vitest run(the command I was asked to use) does not have that count floor — it relies on the collection error alone, which it does raise.Blind spots: Go
MaxTermDimension 1000→500→ 221/221 GREEN, exit 0; GoMaxControlFrameBytes 4<<10 → 1<<10→ 221/221 GREEN, exit 0.Evidence:
git diff --name-status origin/epic/LD3..origin/plane/HOMELAB-1683-attach-ui→ 26 files,--diff-filter=Dempty (no test deleted); only pre-existing test file touched issrc/lib/tests/fixtures.test.ts(additive only), plus the e2e helpere2e/dialog-viewport.ts(parameterised,CONFIRM_DIALOGdefault preserves both existing callers).grep -nE '^\+.*(\.skip|\.only|fixme|xit\(|xdescribe\(|waitForTimeout|setTimeout)'over the whole diff → no matches.origin/epic/LD3at the same checkout = 20 files / 145 tests passed; PR head = 25 files / 221 tests passed; delta +76 = 19 (protocol) + 27 (session) + 6 (cell) + 8 (launcher) + 7 (status) + 9 (new guard tests) — exact, so no pre-existing test was added, removed or weakened. Per-file JSON report confirms all 20 pre-existing files at their baseline counts.E2E_PORToff the shared 8080 to avoid reusing another session's server): full suite 62 passed (1.3m);e2e/attach.spec.tsalone 14 passed, zero skipped,forbidOnlyhonoured./tmp/rev-1683):attachSocketUrlwss-on-https broken → RED (2 failed,session.svelte.test.tshandshake tests); disabled-posture reason replaced → RED (3 files, 3 failed, incl.attach-celltitle⊇LD_ATTACH_ENABLED);no_ttybranch removed → RED (3 files, 4 failed);openTermfallback removed → RED (protocol.test.ts"falls back to the default");showTerminal = false→ GREEN everywhere (blocker 1);showTerminal = true→ e2e RED (1 failed).page.route(...).fulfillis howauth/gates-configured/panesalready stub (grep page.route e2e/*.spec.ts); the flag-off 503 path is asserted end-to-end against the live binary (attach.spec.ts:105-151, includingexpect(answers[0]?.status).toBe(503)andbody.audit_recorded === trueread off the real response);attach-cell.test.tswaits on text/tone rather than tone alone (:118-121comment is accurate); protocol tests assert concrete values, not shapes — I found no tautology among the binary/text split, encode/decode, resize bounds or close-reason vocabulary./home/aaron/repos/homelab-wt/LD3/infra-core— that worktree is dirty with another session's HOMELAB-1684 changes. All runs were in the clean-1683bworktree at126af06or in throwaway detached worktrees, which have been removed (git worktree list | grep -c rev-→ 0);-1683bis clean at126af06.Fix worker — all 4 landing blockers addressed (
d581628a66)Not a review verdict; the landing worker still owns the fresh-context re-review on this head.
1. Byte accounting.
emit()counted every output frame intobytesOutwhile its caller counted the same bytes intobytesIn, andsendInputcounted nothing. Output is now counted once (out), input once (in) and only when actually sent. The status strip prints the close frame's authoritativebytes_in/bytes_outwhen there is one (data-accounting-source="close-frame") and labels a tab-local count as such. Tests: a mixed probe — 13 output bytes then 3 input bytes — now assertsin=0/out=13, thenin=3/out=13, then[in,out]=[4,18]after a second frame each, and again from the end state a client-ended session writes.session.svelte.test.ts3 failed / 30 passed,AssertionError: output is not input: expected 13 to be +0.2. 401/403 refused ≠ unreachable.
attachApi.infonow accepts the auth helper's Problem envelope ({code, detail}) as an answer at 401/403 only — a 5xx carrying the same shape still throws, so a failing cockpit is not dressed as a refusal. The server'sdetailsentence is rendered verbatim; the WS-handshake path reaches the same first-class refused state through the re-probe. Because the envelope carries noaudit_recorded, that fact is now three-valued ('unknown') instead of rendering "this refusal was NOT recorded" about a refusal the backend audited. Tests: envelope 401/403 through the real client, a thrownApiError401/403 vs 502 vsTypeError, and the refused-upgrade re-probe (200 attachable → 401 envelope).3. Terminal mount coverage. New e2e:
page.routeWebSocketserves a real session (open frame → dialoglive→ terminal mounts → xterm's.xterm-rowsrenders the bytes the fixture sent), plus the terminal-bearing dialog is geometry-checked inside a 390px viewport.showTerminal = $derived(false)→ 2 failed / 14 passed (both new specs fail onattach-terminalcount 0). The reviewed blind spot was 14/14 green.4. Parity guard bounds.
fixtures.test.tsnow parsesMaxTermDimensionandMaxControlFrameBytes(expression evaluator, so4 << 10is read rather than restated) out of the Go and compares them to the client constants; an unreadable/renamed bound throws.MaxTermDimension 1000→500→ guard red; GoMaxControlFrameBytes 4<<10 → 1<<10→ guard red (expected 4096 to be 1024). Both were green before.Suites on this head: svelte-check 0 errors / 0 warnings · vitest 26 files, 249 tests passed (guard 17 collected; was 25/221) · build ok · Playwright 64 passed (attach.spec 14 → 16). Scope:
lab-director/cockpit/web/**only, no Go changes.Observed gap, NOT fixed here (reported, not silently touched): the terminal subscribes to
session.onOutputonly after its lazily-imported xterm chunks resolve, and the session fans output out to whatever sinks exist at that instant — so output the backend sends immediately after the open frame can be dropped before the terminal finishes loading. My first e2e run reproduced it (sessionlive, terminal mounted, banner frame sent at connect time → empty.xterm-rows); the test now sends after mount and says so in a comment. Worth its own ticket (bounded pre-subscription buffer), out of this fix set.Independent verification of head
d581628(fresh-context fix worker)A second fix worker (dispatched for the same four landing blockers) checked out this head in a separate worktree and re-ran the mutation proofs against the LANDED tree. Result: all four blockers are genuinely covered; CI runs #1738 / #1739 are green on
d581628.Blocker 3 — the terminal is now mounted by a test (mutation-verified).
e2e/attach.spec.ts:358(the terminal itself › a live session MOUNTS the terminal and renders the bytes the sandbox sent) withshowTerminal = $derived(false):Restored:
✓ 1 … (1.2s). The test asserts a real.xtermsubtree and.xterm-rowstext, so an empty host div cannot satisfy it.Blocker 4 — the parity guard is no longer blind to the Go bounds (mutation-verified).
const MaxTermDimension = 1000 → 500inattach/attach.go:const MaxControlFrameBytes = 4 << 10 → 1 << 10inattach/protocol.go:Both hard-fail (no skip), restored green.
Blockers 1 and 2 (read-only review of this head, not re-mutated here).
session.svelte.tscounts output only in the emit path, counts input only insendInputonce the frame is on the wire, andendOfClosecarries the close frame's totals with an explicitaccountingSource: 'close-frame';client.ts:274accepts the 401/403 Problem envelope as an answer viaisIdentityRefusal, withposture.test.tscovering the refused-vs-unreachable split.Process note for the landing worker: this PR's branch received two independent implementations of the same four fixes (only one,
d581628, is pushed; the second exists as a local patch and was deliberately not stacked). Please reviewd581628on its own merits — the commit message appears once, so no history rewrite is needed.Note for any re-run of these checks: ports 8080 and 8099 were occupied by other sessions' servers (and a reviewer binary serving the pre-fix build). A
webServerwithreuseExistingServerwill silently test a stale build — use an isolatedE2E_PORT.Landing verdict — delta review of head
d581628(fresh context) + class-D visualLanding worker, fresh reviewer process (
deepseek/deepseek-flash, no author context, no priorreview context), brief built from primary sources only: the delta
126af0611..d581628, theticket acceptance, the repo's frozen contracts. Reviewer ran the delta's own suite in a
throwaway worktree at
d581628: 249 unit / 26 files · svelte-check 0/0 · 16 e2e specspassed. It ran 9 mutations of its own, and the landing worker independently re-ran the
four closure mutations (below).
VERDICT: PASS-WITH-CONCERNS — no blocker findings.
Fix closure (both reviewer and landing worker verified, independently)
emit()is the only writer ofbytesOut(session.svelte.ts:231);the data branch no longer touches
bytesIn;bytesInmoves after the wire insendInput(
:594-599); a close frame sets the frame's ownbytes_in/bytes_outwithaccountingSource: 'close-frame'(:449-455); the strip prefers the end state's pair andlabels the source (
attach-status.svelte:60-73,87-105). Landing mutation: restoringbytesIn += message.bytes.byteLengthin the data branch → 2 tests RED (expected 15 to be +0,output is not input: expected 13 to be +0); restored GREEN.isIdentityRefusalaccepts{code, detail}at exactly 401/403(
client.ts:341) and is wired into the answer test (:274);audit_recordedis genuinelyoptional (
:389,types/attach.ts:104);auditRecordOf(undefined) === 'unknown'(
posture.ts:57-61);probeFailureis the single classifier (posture.ts:218) and allfour old
unreachablePosturecall sites use it (session.svelte.ts:305,556,attach-cell.svelte:79,attach-launcher.svelte:74). 5xx and a thrownTypeErrorstill landin
failed/unreachable. Landing mutation:isIdentityRefusal → false→ 2 tests RED(client + session); restored GREEN.
e2e/attach.spec.ts:358asserts.xterminside
attach-terminalplus.xterm-rowscarrying the fixture's bytes;:394geometry-checksthe terminal-bearing dialog at 390px. Landing mutation:
showTerminal = $derived(false)→exactly specs 13/14 RED (2 failed, 14 passed), restored GREEN (2 passed).
goIntConst/evaluateGoInt(fixtures.test.ts:257-272) parse thedeclarations; the pin test (
:514) readsMaxTermDimension(1000) andMaxControlFrameBytes = 4 << 10.Landing mutations:
1000 → 500→ RED (attach.MaxTermDimension: expected 1000 to be 500);4 << 10 → 1 << 10→ RED (attach.MaxControlFrameBytes: expected 4096 to be 1024); both restored GREEN.Findings accepted as concerns (non-blocking, carried — not silently dropped)
accountingSourcehas a false green from its producer. DeletingaccountingSource: 'close-frame'from
endOfCloseleaves all 249 unit + 16 e2e specs green (the flag is only asserted against ahand-built
end). The landing visual closes this seam — a fixture close frame drives the strip todata-accounting-source="close-frame",data-bytes-in="5",data-bytes-out="40",5 B in · 40 B out · 4swith the provenance tooltip — but that spec is uncommitted. Recommend a one-line assertion in
session.svelte.test.ts(expect(session.end?.accountingSource).toBe('close-frame'), andtoBeUndefined()for
client_close).probeFailureclassifies 401/403 by status alone, so a non-envelope 401/403 (an intermediary infront of the cockpit) renders a client-authored sentence and the ledger claim "The attempt is recorded."
The reviewer could not verify the cockpit's ingress from this repo and escalated conditionally.
Landing worker resolved the condition: the cockpit has no exposure definition anywhere in
prod(no
manifests/gateway/routes/*entry, no Pangolin resource, no Deployment — the namespace is the onlycockpit-adjacent manifest), so no intermediary can answer 401/403 for
/api/v1/attach/*/infotoday.This becomes live when the cockpit gets a gateway/Pangolin route; the reviewer's hardening (gate the
classification on
error.code/detail, or stop inventing the sentence and drop the "The attempt isrecorded." claim) should land with that exposure.
endOfErrorstill hardcodesrecorded: truefor anerrorframe whose Go struct carries no audit flag(
session.svelte.ts:465);start()still never tears down a replaced socket (no unmount teardown);unreachablePosturehardcodesauditRecorded: falseunder the new tri-state doctrine (inert/unrendered);two stale doc comments (
probeFailure's "the cockpit ANSWERING",attach-cell's two-outcome list);the
accountingSourceexplanation lives only in atitleattribute (touch/keyboard operators get thenumbers without the "which").
attach-terminal's lazy import resolves aredropped (no buffering in
emit/sinks).Class-D visual (built SPA + real Go binary at
d581628, isolated port 8137)10/10 assertions passed; screenshots in
.rpiv/artifacts/build/ld3-1683b-landing2-screens/, measurements inevidence.json.attachable+ "open terminal" (01); no-TTY → chipno tty, disabled, backendreason "the "browser" class has no TTY harness…" (
02); disabled (LIVE server) → chipattach disabled,P-ATT-001, the real 503 sentence verbatim (03). Wave-table cell renders disabled with the reason as thetitletooltip on both button and chip (10).server's own sentence + "P-ATT-001 refused and recorded in the attach ledger", no terminal element drawn
(
04). Identity 401 → "attach refused · unauthenticated", "Sign in at /auth/login to use the cockpit."(server's sentence), and the tri-state honesty row "the reply does not say whether this refusal was recorded
in the attach ledger" (
05) — the exact defect the first review found, now unrenderable..xterm+.xterm-rowstext"pi v0.4 ready — sandbox ld-light-1" at 1440 (
06, dialog 896×720 inside 1440×900, top 90) and at 390(
07, 374×776.5 inside 390×844, top 59.5);expectDialogFullyInViewportclean at both, backdrop exactlyviewport-height,
scrollWidth === clientWidth.strip prints
5 B in · 40 B out · 4sattributed toclose-frame(08) — separated directions, authoritativetotals, provenance labelled. (A
client_closenever shows athis-tabrow on screen:dismiss()ends thesession and unmounts the dialog in one action — recorded as an observation, not a defect.)
Merging on green CI (#1738/#1739 on
d581628) + this verdict.Blocker closure on
d581628— all four verified fixedresolved 1 — byte accounting (
session.svelte.ts:231emit is the onlybytesOutwriter;:594-599sendInputcounts input after the wire;:449-455close frame carries the frame's own totals withaccountingSource: 'close-frame';attach-status.svelte:60-73,87-105labels the source). Closure mutation: restoring the pre-fix double count → 2 tests RED, restored GREEN.resolved 2 — 401/403 rendered as REFUSED with the backend's verbatim sentence (
client.ts:274,341accepts the{code,detail}envelope at exactly 401/403;posture.ts:57-61auditRecordOfgives a real third value'unknown';posture.ts:218probeFailureis the single classifier, all four oldunreachablePosturecall sites converted). Closure mutation:isIdentityRefusal → false→ 2 tests RED, restored GREEN.resolved 3 — the terminal is mounted by a test and the test discriminates (
e2e/attach.spec.ts:358asserts real.xterm+.xterm-rowstext;:394re-checks the 390px geometry with the terminal present). Closure mutation:showTerminal = $derived(false)→ exactly 2 specs RED, restored GREEN.resolved 4 — the parity guard pins the Go bounds (
fixtures.test.ts:257-272parses the declarations,:514pins both). Closure mutations: GoMaxTermDimension 1000 → 500→ RED;MaxControlFrameBytes 4<<10 → 1<<10→ RED; both restored GREEN.All four blockers are fixed and independently mutation-verified on this head, and the landing delta review on
d581628found none open. The verdict is posted above; the accepted non-blocking concerns and the resolved ingress-escalation question are recorded there.