Skip to content

TUI: create and interact with shared terminal tabs - #72

Merged
nicodes merged 2 commits into
mainfrom
feature/505-shared-terminal-tabs
Aug 25, 2026
Merged

TUI: create and interact with shared terminal tabs#72
nicodes merged 2 commits into
mainfrom
feature/505-shared-terminal-tabs

Conversation

@nicodes

@nicodes nicodes commented Aug 25, 2026

Copy link
Copy Markdown
Owner

Closes nicodes/ormos-be#505

Risk: high. This adds raw-terminal ownership, local shell input/output and resize forwarding, shared-session attachment, mouse routing, and pairing-token persistence. A cleanup or identity defect could leave the operator TTY raw, consume dashboard input, or attach to the wrong shared shell.

Summary

  • list persisted terminal tabs through the deployed agent control plane and render them under their projects in backend created,id order
  • preserve row selection by durable project/port/session identity across independently arriving project and terminal refreshes
  • create a 144-bit base64url session id, retry only explicit HTTP 409 collisions up to 8 total attempts, persist before attach, and use projectID:sessionID as the shared PTY key
  • add keyboard open/create actions plus primary-button row selection with top-clipped viewport hit testing
  • refresh tabs on Init, realtime nudges, mutations, terminal completion, and every twentieth 500 ms tick (approximately 10 seconds)
  • run interactive attachments through Bubble Tea v1.3.10 tea.Exec, then explicitly restore mouse cell-motion reporting
  • make Ctrl-G the documented local detach chord; it is intercepted and never forwarded to the shared shell

Terminal Runner

  • attach only inside ExecCommand.Run, after Bubble Tea releases its reader and restores cooked mode
  • duplicate process stdin with F_DUPFD_CLOEXEC; poll it every 10 ms so cancellation does not close or strand a reader on process stdin
  • enter raw mode locally and defer restoration plus non-destructive TerminalClient.Detach across normal return, context cancellation, client completion, I/O failure, resize failure, and panic
  • forward initial and SIGWINCH dimensions when both are in the relay-valid 1..1000 range
  • retry unchanged input on ErrTerminalInputBackpressure every 10 ms for at most 2 seconds while draining live output
  • reject short writes and propagate input, output, attach, resize, raw-mode, and cleanup failures

Verification

Pinned toolchain: go1.25.13; Bubble Tea: v1.3.10.

  • focused new system tests: named terminal HTTP/TUI/Exec tests, -count=20
  • focused race tests: the same named tests, -count=5
  • mise exec -- go test ./... -count=1
  • mise exec -- go test -race -count=1 ./...
  • mise exec -- go vet ./...
  • mise exec -- govulncheck ./... (0 reachable vulnerabilities; one required-module vulnerability is not called)
  • test -z "$(gofmt -l .)"
  • git diff --check
  • static -trimpath builds with CGO_ENABLED=0: linux/amd64, linux/arm64, darwin/amd64, darwin/arm64
  • unsupported target selection gate: windows, freebsd, openbsd, netbsd, solaris, dragonfly, illumos, aix, plan9, js/wasm, wasip1/wasm, android/arm64, and ios/arm64 each selected only github.com/nicodes/ormos/relay

Manual PTY Walkthrough

A one-off shared-tag test file, removed before this commit, used a real pseudo-terminal and the provisioned agent control plane without starting a second tunnel. It created one temporary persisted tab, attached through a real Bubble Tea program at 80x24, typed ORMOS505-TYPE, resized the pseudo-terminal to 100x32 and observed stty size report 32 100, detached with Ctrl-G, observed ORMOS505-DASHBOARD-RESTORED, reattached the same projectID:sessionID, observed preserved shell state as ORMOS505-REATTACH:shared, detached again, and passed in 1.650s with a 90-second test timeout. The exact temporary record was deleted; a before/after record-ID set comparison reported zero created and zero removed residue.

An earlier script(1) attempt did not deliver navigation keys as discrete Bubble Tea events and entered the system-name editor instead. The unintended value was corrected back to the exact captured pre-test value, verified through /system/info; before/after terminal-record sets were unchanged. No value or credential is included here.

Deliberate Gate Failures

Each production mutation was applied alone and reverted before b864476.

  • changed POST session_id to session; TestTerminalSessionHTTPWireShape failed with POST body ... want exact snake-case fields
  • changed the 8-attempt collision cap to 7; the corrected literal-threshold gate failed with conflict exhaustion requests=7 random=7, want 8
  • retried ambiguous transport and non-conflict failures; both subtests failed with calls = 8, ambiguous/non-conflict failure was retried
  • sorted tabs by random session id; TestTerminalRowsAndSanitizedLabels failed with terminal order = "a-tab", "z-tab"; want relay created,id order
  • removed + terminal; the row-construction gate failed with the exact flattened row list
  • disabled durable selection restoration; project refresh failed by selecting another projects action row
  • disabled deleted-tab fallback; it failed by selecting zz-after instead of the projects + terminal row
  • accepted every mouse action/button; motion selected row 2
  • removed the top-clipping offset; an 8-row viewport mapped y=3 to the SYSTEM name while rendering :4000
  • removed terminalsCmd; Init, event, mutation, twentieth tick, and terminal-completion subtests each reported terminal refresh calls = 0, want 1
  • omitted post-Exec mouse restoration; it failed with terminal completion did not re-enable mouse cell motion
  • opened an existing tab with its database record id; it failed with key record-a instead of project-a:a-tab
  • attached after failed persistence; it failed with failed create attached=true while still proving the error string was sanitized
  • forwarded Ctrl-G; it failed with writes = ["ab\a"]; Ctrl-G or trailing bytes were forwarded
  • stopped retrying backpressure; it failed immediately with terminal input backpressure
  • stopped draining output while backpressured; it failed after the exact 2-second cap with terminal input remained busy for 2s
  • replaced F_DUPFD_CLOEXEC with F_DUPFD; it failed with polling stdin duplicate can leak into a spawned PTY shell
  • read directly from process stdin and closed it on return; it failed with process stdin was closed: ... broken pipe
  • removed raw restoration; cancellation, client completion, input failure, output failure, resize failure, and attach failure each reported restores=0
  • removed detach; all post-attach exit paths reported detach=0
  • removed both cleanup defers for panic; it failed with panic cleanup restored=false detach=0
  • ignored SIGWINCH; the second-resize condition was not reached within 500 ms
  • skipped the initial resize; it failed with initial resizes = nil
  • painted the raw session id; the sanitizer gate observed the escape-bearing label
  • renamed the narrow action; the 14-column view failed with narrow view hid the terminal action
  • ignored entropy failure; it failed with error = <nil>
  • ignored short writes; it failed with Run = <nil> detach=1
  • let project refresh clear a terminal load error; the independent-error gate reported that the terminal failure was hidden

Two initially proposed assertions were false greens and were corrected before the evidence above: collision exhaustion had compared against the production constant, and deleted-tab fallback happened to land on + terminal by unchanged index. The final tests use a literal threshold of 8 and retain a later tab so each mutation changes the verdict.

Remediation\n\nInitial review head: b864476913ce876e36908d5aaa8a7ff244571b82. GPT-5.6 Luna identified one user-visible FOLLOW-UP: a successful terminal return left a prior terminal error on the restored dashboard. Commit ca4a85c clears that error only on terminalFinishedMsg{err:nil} and adds a failure-then-success regression.\n\nVerification after remediation:\n\n- focused regression: -count=20\n- focused race regression: -count=5\n- full tests and full race tests: -count=1\n- go vet ./..., formatting, and git diff --check\n- deliberate omission of the success branch failed with successful terminal return left stale error \"attach failed\"\n

Review

Two independent reviewers must inspect the same unchanged green initial head. The repository has only the PR author as a collaborator, so GitHub cannot record self-approval; per the repository-owner decision on the preceding stack layers, published independent APPROVE statuses are the approval record.

@nicodes

nicodes commented Aug 25, 2026

Copy link
Copy Markdown
Owner Author

Independent initial review: GPT-5.6 Luna

Pinned and verified unchanged head b864476913ce876e36908d5aaa8a7ff244571b82; complete main...HEAD diff inspected. git diff --check and go test ./... passed.

Risk: high.

FOLLOW-UP - stale error remains after successful terminal return

internal/system/tui.go:405-410

When an attach/resize/output attempt sets m.err, a later successful terminalFinishedMsg{err:nil} does not clear it. The dashboard therefore continues displaying an obsolete terminal failure after the user successfully detaches or reattaches. Clear the terminal action error on successful completion, with a regression test covering failure followed by successful completion.

Residual risk: no live Darwin PTY/Bubble Tea tea.Exec integration test was available in the reviewer environment; exact CI and the recorded real-PTY walkthrough reduce but do not eliminate that coverage gap. Bubble Tea v1.3.10 Exec and Linux/Darwin terminal semantics were checked against versioned sources.

NEEDS ATTENTION

@nicodes

nicodes commented Aug 25, 2026

Copy link
Copy Markdown
Owner Author

Independent initial review: Grok 4.6

Risk: high. Confirmed exact head b864476913ce876e36908d5aaa8a7ff244571b82; reviewed the complete diff, shared-terminal implementation, project/port/browser controls, Bubble Tea v1.3.10, x/term v0.2.2, and Linux/Darwin descriptor semantics.

No BLOCKING, BLOCKING TEST GAP, or material FOLLOW-UP findings. Required behavior is implemented and reachable: 144-bit IDs, exact agent wire shape, eight-attempt 409-only collision retry, persist-before-attach, projectID:sessionID, durable cursor fallback, left-press mouse geometry, all refresh paths, post-Exec mouse restoration, CLOEXEC stdin duplication, 10 ms cancellable polling, raw restore/non-destructive detach, 2-second bounded backpressure handling, SIGWINCH resize, Ctrl-G interception, and sanitized labels/errors.

Residual risks, not blockers: no permanent real-TTY integration test remains; selected-project deletion can move the cursor by index but cannot silently attach; mouse offsets are coupled to the header and pinned by rendered-geometry tests; unread bytes after Ctrl-G can become dashboard keys.

APPROVE

@nicodes

nicodes commented Aug 25, 2026

Copy link
Copy Markdown
Owner Author

Initial-wave disposition

Accepted the GPT-5.6 Luna FOLLOW-UP. The error is reachable after a failed attachment followed by a successful one and is user-visible, though it does not cross a security boundary. The remediation will clear only the terminal action error on a successful terminalFinishedMsg and add a failure-then-success regression test. No other initial-wave findings require code or issues.

@nicodes

nicodes commented Aug 25, 2026

Copy link
Copy Markdown
Owner Author

Independent remediation review: Grok 4.6

Confirmed exact head ca4a85ca962d0a50de8f2c0601e636546b44466e. The complete remediation delta is only tui.go +2 and tui_test.go +13; the initial review remains disposed with no new evidence on unchanged code.

The inherited finding is fixed. A failed finish assigns the sanitized terminal error; a later nil-error finish clears m.err. Omitting the else fails with successful terminal return left stale error "attach failed". Failure sanitization, mouse restoration, terminal refresh, and independent terminal/project load errors remain intact.

No BLOCKING, BLOCKING TEST GAP, or remaining FOLLOW-UP items; no remediation-caused defects.

APPROVE

@nicodes

nicodes commented Aug 25, 2026

Copy link
Copy Markdown
Owner Author

Independent remediation review: GPT-5.6 Luna

Pinned and verified unchanged at ca4a85ca962d0a50de8f2c0601e636546b44466e.

The prior finding is fixed at internal/system/tui.go:405-410; successful terminalFinishedMsg now clears m.err. The added regression test is reachable and fails when that branch is omitted.

Inspected the complete remediation delta and current main...ca4a85c diff. Focused tests (-count=20), focused race tests (-count=5), full tests, go vet ./..., and diff checks pass. No new evidenced, reachable, material defects were found.

Residual risk: no additional live interactive Darwin walkthrough was performed in this follow-up; native Darwin CI is green.

APPROVE

@nicodes
nicodes merged commit a5152bb into main Aug 25, 2026
4 checks passed
@nicodes
nicodes deleted the feature/505-shared-terminal-tabs branch August 25, 2026 04:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant