Skip to content

fix: harden cli signal and control-fd handling#236

Open
covercrust wants to merge 1 commit into
anthropic-experimental:mainfrom
covercrust:fix-cli-signal-and-control-fd-handling
Open

fix: harden cli signal and control-fd handling#236
covercrust wants to merge 1 commit into
anthropic-experimental:mainfrom
covercrust:fix-cli-signal-and-control-fd-handling

Conversation

@covercrust

@covercrust covercrust commented May 5, 2026

Copy link
Copy Markdown

Why

This tightens a few shutdown and control-fd edge cases in src/cli.ts that make the CLI harder to reason about during failure/interruption paths:

  • --control-fd abc currently parses to NaN and still enters the control-fd path
  • SIGINT / SIGTERM currently exit with 0, which hides interruption from callers
  • one-shot shutdown handlers currently use process.on(...) instead of process.once(...)
  • the child error path does not currently call cleanupAfterCommand()
  • the control-fd stream is not explicitly destroyed during exit cleanup

What changed

  • parse --control-fd with an explicit radix: parseInt(val, 10)
  • ignore invalid control-fd values by requiring the fd to be both defined and non-NaN
  • hoist the control stream so it can be destroyed during exit cleanup
  • switch exit / signal cleanup handlers from process.on(...) to process.once(...)
  • preserve conventional signal exit codes with 128 + signal
  • run SandboxManager.cleanupAfterCommand() in the child error handler too

Reviewer guide

This PR is intentionally narrow:

  • 1 file changed: src/cli.ts
  • No public API changes
  • No behavior changes on the happy path
  • only affects invalid control-fd input, interrupt handling, and cleanup/error paths

Concrete repro

1. Invalid control-fd input

Run:
node dist/cli.js --control-fd abc -c 'echo hello'

Before:

  • parseInt('abc') becomes NaN
  • the code still enters the control-fd branch
  • it attempts to open a stream with an invalid fd

After:

  • invalid numeric values are ignored cleanly

2. SIGINT exit code

Run:
node dist/cli.js -c 'sleep 30'

Then press Ctrl+C.

Before:

  • wrapper exits with 0

After:

  • wrapper exits with 130 (128 + SIGINT)

Validation

  • npx tsc --noEmit
  • npm run build

Risk

Low. The patch is isolated to CLI argument validation and shutdown/error cleanup behavior.

Copilot AI review requested due to automatic review settings May 5, 2026 17:00

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the srt CLI’s lifecycle handling around control-fd config streaming and child-process termination, aiming to make cleanup more reliable and exit statuses more conventional.

Changes:

  • Parses --control-fd with an explicit base-10 radix and skips setup when the parsed value is invalid.
  • Switches to process.once for exit/signal handlers and ensures the control-fd stream is destroyed during exit cleanup.
  • Preserves conventional signal exit codes (128 + signal) and runs sandbox cleanup on child process error as well as exit.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/cli.ts
Comment on lines 60 to 64
.option(
'--control-fd <fd>',
'read config updates from file descriptor (JSON lines protocol)',
parseInt,
(val: string) => parseInt(val, 10),
)
Comment thread src/cli.ts
Comment on lines +187 to +193
const sigNum: Record<string, number> = {
SIGINT: 2,
SIGTERM: 15,
SIGHUP: 1,
SIGQUIT: 3,
}
const exitCode = 128 + (sigNum[signal] ?? 1)
@covercrust

Copy link
Copy Markdown
Author

Concrete repro scenarios for review:

  1. Invalid --control-fd value

    • Run: node dist/cli.js --control-fd abc -c 'echo hello'
    • Before this patch: parseInt('abc') becomes NaN, the code still enters the control-fd path, and attempts to open a stream with an invalid fd.
    • With this patch: invalid numeric values are ignored cleanly because the fd must be both defined and non-NaN.
  2. SIGINT exit code handling

    • Run: node dist/cli.js -c 'sleep 30'
    • Press Ctrl+C
    • Before this patch: the wrapper exits with code 0, which hides that the process was interrupted.
    • With this patch: the wrapper exits with the conventional signal code 130 (128 + SIGINT).
  3. Repeated listener safety / cleanup behavior

    • Start a sandboxed command and interrupt it repeatedly.
    • Before this patch: cleanup/signal handlers used process.on(...), which can accumulate if reused and are less precise for one-shot shutdown paths.
    • With this patch: one-shot shutdown behavior uses process.once(...), and the control-fd stream is explicitly destroyed during exit cleanup.
  4. Child process spawn failure cleanup

    • Force a child-process error path (for example by provoking a failed execution path in the wrapped command).
    • Before this patch: cleanupAfterCommand() was not called from the child error handler.
    • With this patch: cleanup also runs on that error path.

Validation used for this PR:

  • npx tsc --noEmit
  • npm run build

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.

2 participants