fix(#725): gate tui/ with its own typecheck + test job, and handle shell_approval - #855
Merged
Merged
Conversation
…ell_approval tui/ had no automated gate: the only 'tui' string in the Makefile was the check-message-types diff, and .github/workflows/ grepped to zero matches. Its typecheck had been failing on main as a result. The dispatcher's assertNever() exhaustiveness check had caught a real drift that no gate ever ran. #725 was filed when the missing variants were SrvStickySet/SrvStickyClear; those have since been handled, and today it is SrvShellApproval, added by #850. Same bug, second instance, because the gate still did not exist. - install-tui / check-tui / test-tui mirror install-js / check-js / test-js, including install-js's npm ci lockfile guard and its #706/#716 rationale. Deliberately NOT part of 'make check': tui/ has its own package-lock and node_modules, and nothing in the Python gate needs them. - A separate check-tui CI job runs both goals in one make invocation, so install-tui's npm ci happens once rather than per target. Node version comes from .nvmrc, consistent with the other two jobs after #854. - shell_approval is now surfaced as a system transcript line rather than added to the spike-deferred no-op group. Auto-approved shell commands are security-relevant and the web UI already shows them, so silently dropping them in the TUI is the worse failure. It needs no new rendering capability, unlike the sticky/canvas widgets (#507). assertNever stays in the default branch, no @ts-ignore was added, tsconfig strictness is unchanged, and types.generated.ts was not hand-edited - the fix belongs in dispatcher.ts, not in generated output. Tests written first and observed failing (transcript undefined, since shell_approval fell through to the no-op default). Covers ALLOWED, DECLINED, and empty risk, which is reachable because the server publishes data.get('risk', '') and must not render a dangling '(risk: )'. Verified: tui 41 -> 44 tests, none skipped or lost; tui typecheck clean; make check, make test-js (178 tests) and make test (3894 passed, 2 skipped) all unaffected; tui/package-lock.json unchanged by npm ci on npm 11. Gate teeth-checked: a deliberate TS2345 under tui/src/ makes make check-tui exit 2.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
check-tui does not run tests, and archived shell approval history is not restored.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds dedicated TUI typecheck/test gates and shell approval transcript handling.
Changes:
- Adds TUI Make targets and a dedicated CI job.
- Renders and tests
shell_approvalevents.
File summaries
| File | Summary |
|---|---|
tui/src/dispatcher.ts |
Handles live shell approval transcript events. |
tui/src/dispatcher.test.ts |
Tests shell approval rendering cases. |
Makefile |
Adds TUI dependency, typecheck, and test targets. |
.github/workflows/ci.yml |
Adds the TUI CI gate. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| fi | ||
|
|
||
| # Type check the TUI (mirrors check-js) | ||
| check-tui: install-tui |
Comment on lines
+163
to
+169
| case "shell_approval": { | ||
| const verdict = m.approved ? "ALLOWED" : "DECLINED"; | ||
| const risk = m.risk ? ` (risk: ${m.risk})` : ""; | ||
| return appendTranscript(s, { | ||
| kind: "system", | ||
| text: `[shell auto-approval: ${verdict}] ${m.command}${risk}`, | ||
| }); |
This was referenced Sep 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #725.
tui/had no automated gate. The onlytuistring in the Makefile was thecheck-message-typesdiff, andgrep -rn tui .github/workflows/returned zeromatches. Its typecheck had been failing on
mainas a result.The bug this is really about
dispatcher.ts'sassertNever()exhaustiveness check caught a real drift thatno gate ever ran. #725 was filed when the unhandled variants were
SrvStickySet/SrvStickyClear; those have since been handled, and todayit is
SrvShellApproval, added by #850. Same bug, second instance, becausethe gate still didn't exist. Since
assertNeverhas an empty body and doesn'tthrow,
shell_approvalmessages fell todefault:and silently no-opped.Changes
install-tui/check-tui/test-tuimirrorinstall-js/check-js/test-js, includinginstall-js'snpm cilockfile guard and its #706/#716rationale. Deliberately not part of
make check(issue decision #2):tui/has its ownpackage-lock.jsonandnode_modules, and nothing in thePython gate needs them.
A separate
check-tuiCI job (decision #3) runs both goals in onemakeinvocation, so
install-tui'snpm cihappens once rather than per target(verified: exactly one
npm ciin the combined run). Node version comes from.nvmrc, consistent with the other two jobs after #854 — that satisfies theissue's "pin node 22" requirement with one source of truth instead of a third
hardcoded
'22'.shell_approvalis surfaced as a system transcript line, not added to thespike-deferred no-op group. Auto-approved shell commands are security-relevant
and the web UI already shows them, so silently dropping them in the TUI is the
worse failure — and it needs no new rendering capability, unlike the
sticky/canvas widgets. Rendered as
[shell auto-approval: ALLOWED] <command> (risk: <risk>), matching theserver's own
shell auto-approval: ALLOWED/DECLINEDphrasing.Guards from the issue, all verified
assertNever(m)remains indefault:dispatcher.ts:190)@ts-expect-error/@ts-ignoreindispatcher.tstui/tsconfig.jsonstrictness not reducedtypes.generated.tsnot hand-editednpm ci, lockfile clean after a full runmake test-jsunaffectedmake check-message-typesgreenmake checkVerification
transcriptcame backundefinedbecauseshell_approvalfell through to the no-op default.Three cases: ALLOWED, DECLINED, and empty
risk. That last one is reachable(the server publishes
data.get("risk", "")) and must not render a dangling(risk: ).TS2345undertui/src/makes
make check-tuiexit 2 and name the error. The probe was a newlycreated file, deleted afterwards, so no existing file was touched.
make check,make test-js,make check-tui test-tuiall exit 0;make testis 3894 passed / 2 skipped.Not included
Per the issue's explicit scope: no Ink render-test harness (this gates the
tests that already exist), no sticky widget implementation, no lint/format for
tui/.One thing I noticed but left alone: the spike-deferred case group in
dispatcher.tshas no pointer to where that work lives. #507 (TUI: widgetinputs + canvas mirror) is the natural home for the sticky/canvas entries, and
adding that reference would help the next reader — but it's outside this
issue's diff.