Skip to content

fix(tui-gateway): preserve skill install review - #38688

Open
coygeek wants to merge 2 commits into
NousResearch:mainfrom
coygeek:fix/tui-gateway-skill-install-skips-required-review-surface
Open

fix(tui-gateway): preserve skill install review#38688
coygeek wants to merge 2 commits into
NousResearch:mainfrom
coygeek:fix/tui-gateway-skill-install-skips-required-review-surface

Conversation

@coygeek

@coygeek coygeek commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Summary

The TUI gateway's non-interactive skills.manage install action is now inspect-only. It no longer invokes do_install(..., skip_confirm=True) with a silent console, so a TUI or local RPC client cannot move third-party skill code into the live skills directory without the operator-visible scan, review, and confirmation performed by the interactive CLI.

Instead, the gateway returns a truthful result:

  • installed: false
  • status: review_required
  • review_required: true
  • inspect metadata and a SKILL.md preview when available
  • an explicit direction to finish through the interactive CLI

Both TUI callers preserve that contract. The slash command shows a review panel and no longer says installation is underway; the Skills Hub keeps the overlay open, renders the preview and review notice, and does not label the expected review requirement as an error.

Root cause

The TUI route used the CLI installer as a non-interactive shortcut, hardcoding skip_confirm=True and discarding all console output. That suppressed the scan report, disclaimer, and confirmation prompt while still allowing quarantine content to be installed.

Validation

  • scripts/run_tests.sh tests/tui_gateway/test_protocol.py -q — 84 passed
  • npm run check --workspace ui-tui — build, typecheck, and full Vitest suite passed (1,208 passed, 1 skipped)
  • focused slash-handler Vitest suite — 75 passed
  • ESLint on all changed TUI files — passed with no warnings
  • Ruff on changed Python files — passed
  • git diff --check origin/main...HEAD — passed

Rebased onto main at 477c08b44766ace8b890faa72bf82ecbcf2b3ba8.
Head: 7df2e41a0366bb0efde32996c2a5b7646545abde.

Closes #38687

@alt-glitch alt-glitch added type/security Security vulnerability or hardening P2 Medium — degraded but workaround exists comp/tui Terminal UI (ui-tui/ + tui_gateway/) comp/gateway Gateway runner, session dispatch, delivery tool/skills Skills system (list, view, manage) labels Jun 4, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for identifying this TUI-specific trust-boundary gap. The premise still holds on current main: tui_gateway/server.py:14264-14272 invokes do_install(..., skip_confirm=True, console=_Q()), while hermes_cli/skills_hub.py:687-722 only shows the third-party disclaimer and asks for confirmation when that flag is false.

The proposed inspect-only response removes the gateway-side installer call rather than attempting to prompt through the TUI transport. That fits the stated browse/inspect-only remediation and covers both current TUI callers: /skills install in ui-tui/src/app/slash/commands/ops.ts:602-615 and the SkillsHub overlay in ui-tui/src/components/skillsHub.tsx:75-82. The PR also adds focused protocol and slash-handler coverage for the review-required result.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/install-update Installer, updater, packaging, wheels, doctor labels Jul 14, 2026
Make the non-interactive TUI gateway install route inspect-only so it cannot call the installer with review and confirmation disabled. Return an explicit review-required, installed-false result with skill metadata, and render that result truthfully in both TUI callers without claiming installation is underway or treating the required review as an error.
@coygeek
coygeek force-pushed the fix/tui-gateway-skill-install-skips-required-review-surface branch from f5e42bf to 7df2e41 Compare July 21, 2026 03:57

coygeek commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Rebased and refreshed onto current main (477c08b44766ace8b890faa72bf82ecbcf2b3ba8) at head 7df2e41a0366bb0efde32996c2a5b7646545abde. The TUI gateway route is now strictly inspect-only: it returns installed: false and review_required, never calls the installer, and directs the operator to the interactive CLI where scan output, review, and confirmation remain visible. Both TUI callers now report that result truthfully rather than claiming installation is underway or treating review as an error. Validation: 84/84 gateway protocol tests; full TUI check with 1,208 passed/1 skipped; focused slash tests 75/75; ESLint, Ruff, and diff-check pass. No review threads are outstanding.

@coygeek

coygeek commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

CI retry receipt

  • Original root failure: JS & TS checks / Typecheck & Test (apps/desktop) failed in the unrelated apps/desktop/src/app/settings/toolset-config-panel.test.tsx test while locating Actions for ELEVENLABS_API_KEY.
  • All required checks pass was derivative of that single workspace failure. The PR-changed ui-tui workspace passed.
  • Exact desktop test passed 4/4 consecutive local reproductions, so I made no unrelated source-code change.
  • GitHub rejected the direct rerun API with Must have admin rights to Repository; I pushed empty fast-forward commit caffe5fc90eb7728df6b28f96e2b0642925afd60 to trigger a permission-safe retry.
  • Retry run 29800403320: formerly failing desktop check passed, changed ui-tui check passed, all eight Python slices passed, and All required checks pass passed.
  • The expected Python tests / save-durations skipped state and osv-scanner neutral state are non-fail terminal states.

Source behavior is unchanged by the retry commit; the PR remains open and mergeable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/gateway Gateway runner, session dispatch, delivery comp/tui Terminal UI (ui-tui/ + tui_gateway/) P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data tool/skills Skills system (list, view, manage) type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(tui-gateway): preserve skill install review

3 participants