Show spinners during persona session install and exit sync#59
Show spinners during persona session install and exit sync#59willwashburn merged 2 commits intomainfrom
Conversation
Two visible UX fixes for the CLI persona launch lifecycle: - Skill install now runs behind an "Installing skills…" ora spinner with stdio captured. On success it collapses to a single line; on failure the buffered output is dumped after spinner.fail so the user still sees what broke. Replaces the prior raw `npx prpm install` / `npx skills add` output that looked broken to the eye. - Exiting a persona session via /exit no longer hangs silently while relayfile syncs back. The mount lifecycle (createMount → onBeforeLaunch → startAutoSync → spawn → autoSync.stop → syncBack → cleanup) is now inlined so we own the gap between child-exit and finalize, and the sync spinner now starts the moment the child closes — not just on Ctrl-C. Reduced abort to 2 presses (was 3): 1st press during sync = abort immediately; 2nd press = force-quit and rm session dir. Also bumps @relayfile/local-mount from ^0.6.14 to ^0.6.15, and isolates AGENT_WORKFORCE_HOME in the --version test so the load-time persona scan can't surface unrelated warnings from the developer's own ~/.agentworkforce. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe CLI inlines the ChangesMount-mode execution and install UI refactor
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant createMount
participant Installer
participant Harness
participant AutoSync
participant SyncBack
User->>CLI: run CLI in mount-mode
CLI->>createMount: create mount
createMount-->>CLI: mount handle
alt deferred installs
CLI->>Installer: run installs inside mount
Installer-->>CLI: install results
end
CLI->>Harness: spawn child inside mount
Harness-->>CLI: child exits
CLI->>AutoSync: stop autosync (if present)
CLI->>SyncBack: run syncBack with spinner
SyncBack-->>CLI: sync results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b651cc321
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (activeChild && !activeChild.killed && activeChild.exitCode === null) { | ||
| try { | ||
| activeChild.kill('SIGINT'); | ||
| } catch { |
There was a problem hiding this comment.
In the non-syncing path this handler always calls activeChild.kill('SIGINT'), but with spawn(..., { stdio: 'inherit' }) the child is already in the foreground process group and receives the terminal Ctrl-C directly. That means each Ctrl-C can deliver two SIGINTs to the harness, which can skip graceful first-interrupt behavior and force premature termination for CLIs that escalate on repeated SIGINT.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in a2bcee3. The handler is now a true no-op while the child runs (its only purpose is suppressing Node's default exit-on-SIGINT). Removed the activeChild.kill('SIGINT') forward and the activeChild ref. Updated the surrounding comment to call out why the no-op is required.
| const res = spawnSync(bin, args, { | ||
| stdio: ['ignore', 'pipe', 'pipe'], | ||
| shell: false, | ||
| encoding: 'utf8', | ||
| ...(cwd ? { cwd } : {}) |
There was a problem hiding this comment.
This install path now captures stdout/stderr via spawnSync pipes but does not set maxBuffer, so Node applies its default per-stream buffer limit (about 1 MiB). Verbose install commands can exceed that limit, causing spawnSync to terminate the subprocess (ENOBUFS/SIGTERM) and report a non-zero exit even when the install would otherwise complete, which is a regression from the previous inherited-stdio behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a2bcee3 — spawnSync now sets maxBuffer: 100 * 1024 * 1024 (100 MiB), well past anything these installers print in practice. Comment in-place explains why.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Duplicate failure message on install error in runInstallOrThrow path
Same pattern as runInstall: runInstallWithSpinner already outputs spinner.fail(...) with the failure message at packages/cli/src/cli.ts:430. When runInstallOrThrow throws InstallCommandError, the catch block at line 1379 writes ${err.message}. Aborting.\n — and err.message is "${label} failed (exit ${exitCode})" (set at packages/cli/src/cli.ts:454), producing a second nearly identical error line on stderr.
(Refers to lines 1378-1380)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in a2bcee3 — the InstallCommandError catch no longer re-prints err.message. The failure line is shown once via spinner.fail inside runInstallWithSpinner.
| if (code !== 0) { | ||
| process.stderr.write(`${label} failed (exit ${code}). Aborting.\n`); | ||
| process.exit(code); |
There was a problem hiding this comment.
🟡 Duplicate failure message on install error in runInstall
runInstallWithSpinner already writes a failure message via spinner.fail(${label} failed (exit ${code})) at packages/cli/src/cli.ts:430, and dumps captured output at line 431. Then runInstall at line 441 writes a nearly identical message "${label} failed (exit ${code}). Aborting.\n" to stderr before exiting. On a failed install the user sees the same failure reported twice:
Example duplicated output
✗ Installing skills: foo → /path failed (exit 1)
<captured subprocess output>
Installing skills: foo → /path failed (exit 1). Aborting.
| if (code !== 0) { | |
| process.stderr.write(`${label} failed (exit ${code}). Aborting.\n`); | |
| process.exit(code); | |
| if (code !== 0) { | |
| process.exit(code); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in a2bcee3 — runInstall no longer writes the duplicate "Aborting" line; the failure message already comes from spinner.fail inside runInstallWithSpinner.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/cli.ts (1)
1246-1283:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
SIGTERMhandler to the inline mount path for consistent signal cleanup.The inline mount lifecycle registers only
SIGINT, while the other execution path handles bothSIGINTandSIGTERM. Without aSIGTERMlistener, Node uses its default behavior (immediate exit), bypassing the sync and cleanup logic entirely. ASIGTERMsignal during a session would skipsyncBack()and leave the session directory behind.Suggested refactor
- const sigintHandler = () => { + const handleShutdownSignal = (signal: NodeJS.Signals) => { if (!isSyncing) { // Defensive: forward to child if it somehow didn't get the TTY signal. // No spinner side effects while the harness owns the screen. if (activeChild && !activeChild.killed && activeChild.exitCode === null) { try { - activeChild.kill('SIGINT'); + activeChild.kill(signal); } catch { /* ignore — child likely exited between checks */ } } return; } abortPresses += 1; if (abortPresses === 1) { if (syncSpinner) { syncSpinner.text = 'Aborting sync — partial changes will be propagated. (Ctrl-C again to force quit)'; } shutdownController.abort(); return; } ... }; - process.on('SIGINT', sigintHandler); + const sigintHandler = () => handleShutdownSignal('SIGINT'); + const sigtermHandler = () => handleShutdownSignal('SIGTERM'); + process.on('SIGINT', sigintHandler); + process.on('SIGTERM', sigtermHandler); ... process.removeListener('SIGINT', sigintHandler); + process.removeListener('SIGTERM', sigtermHandler);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/cli.ts` around lines 1246 - 1283, The inline mount path only registers sigintHandler for SIGINT so SIGTERM bypasses cleanup; add a SIGTERM listener that invokes the same sigintHandler to ensure syncSpinner, shutdownController.abort(), rmSync(sessionRoot, ...) and process.exit handling run for both signals. Locate the sigintHandler function and replace or augment the process.on('SIGINT', sigintHandler) registration to also register process.on('SIGTERM', sigintHandler) (or otherwise wire SIGTERM to call sigintHandler) so both signals trigger the same teardown logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/cli.ts`:
- Around line 411-433: In runInstallWithSpinner, increase spawnSync's
stdout/stderr buffer by adding maxBuffer: 16 * 1024 * 1024 to the options and
include any spawn error text by appending res.error?.message (if present) to the
computed output so buffer exhaustion or spawn errors are visible; keep using
subprocessExitCode(res) but ensure the final output string includes
res.error?.message (and is written to stderr on failure) so ENOBUFS or other
spawn errors are distinguishable from normal command exit codes.
---
Outside diff comments:
In `@packages/cli/src/cli.ts`:
- Around line 1246-1283: The inline mount path only registers sigintHandler for
SIGINT so SIGTERM bypasses cleanup; add a SIGTERM listener that invokes the same
sigintHandler to ensure syncSpinner, shutdownController.abort(),
rmSync(sessionRoot, ...) and process.exit handling run for both signals. Locate
the sigintHandler function and replace or augment the process.on('SIGINT',
sigintHandler) registration to also register process.on('SIGTERM',
sigintHandler) (or otherwise wire SIGTERM to call sigintHandler) so both signals
trigger the same teardown logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 22410145-6940-4768-9210-d880b3f5908b
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/cli/package.jsonpackages/cli/src/cli.test.tspackages/cli/src/cli.ts
- Drop duplicate failure messages on install error: spinner.fail in
runInstallWithSpinner already prints the failure line, so both runInstall
and the InstallCommandError catch were re-printing nearly the same text.
- Stop double-delivering SIGINT to the harness: with stdio: 'inherit' the
child shares the parent's process group, so Ctrl-C at the TTY already
reaches it. Forwarding via child.kill('SIGINT') from our handler
delivered a second SIGINT, which can break harnesses that escalate on
repeated interrupts (e.g. claude treats 1st = cancel, 2nd = quit). The
handler now stays a true no-op while the child runs, only there to
suppress Node's default exit-on-SIGINT.
- Raise spawnSync maxBuffer to 100 MiB for the captured installer output.
Default is 1 MiB; verbose `npx prpm install` / `npx skills add` runs
can blow past it and have spawnSync kill the child with ENOBUFS.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
npx prpm install/npx skills add) in an "Installing skills…" ora spinner with stdio captured. Successful runs collapse to a single ✓ line; failures dump the buffered output afterspinner.fail. Replaces the prior raw output that looked broken at session start.launchOnMount) so the sync-back spinner appears the moment the persona child exits — not just when the user presses Ctrl-C. Exiting via/exitno longer looks like a silent hang. Abort reduced from 3 presses to 2 (1st press during sync aborts immediately; 2nd force-quits and rms the session dir).@relayfile/local-mountfrom ^0.6.14 to ^0.6.15.AGENT_WORKFORCE_HOMEin the--versiontest so the load-time local-persona scan can't surface warnings from developer-local state.Test plan
pnpm typecheck(after a fresh workspacepnpm build— workload-router's generated types must be up to date)pnpm testfrompackages/cli— 126/126 pass/exita persona session, confirm the "Syncing session changes back…" spinner appears immediately and Ctrl-C aborts on first press🤖 Generated with Claude Code