fix(exec): forward signals to child and exit silently on failure by jdx · Pull Request #339 · jdx/fnox

jdx · 2026-03-08T22:12:18Z

Summary

Replace exec(2) with spawn + wait so Rust destructors run and as_file temp files are properly cleaned up
Forward SIGINT/SIGTERM to the child process via signal-hook + nix so Ctrl-C and kill reach it
Exit silently with the child's exit code (128+signal for signal deaths) instead of printing a noisy fnox error message
Adds signal-hook and nix (signal, process features) as dependencies

Test plan

test/file_secrets.bats passes — temp files are cleaned up after exec
CI passes on both ubuntu and macos
Ctrl-C during fnox exec -- sleep 60 kills the child

🤖 Generated with Claude Code

Note

Medium Risk
Changes process lifecycle and Unix signal handling for fnox exec, which can affect termination semantics and exit codes across platforms.

Overview
fnox exec now spawns the subprocess and, on Unix, forwards SIGINT/SIGTERM to the child so Ctrl-C/kill behave as expected.

On subprocess failure, exec no longer returns a FnoxError; it exits silently with the child’s exit status (including 128+signal when terminated by a signal), and the CommandExitFailed error variant is removed.

Adds signal-hook and nix (signal/process features) dependencies and updates Cargo.lock accordingly.

^{Written by Cursor Bugbot for commit faea52c. This will update automatically on new commits. Configure here.}

gemini-code-assist · 2026-03-08T22:12:29Z

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refines the error handling for subprocess execution within the fnox exec command. The primary goal is to streamline the user experience by avoiding duplicate error messages when a child process terminates with a non-zero exit code. Instead of wrapping the subprocess's failure in a fnox specific error, the application now directly propagates the subprocess's exit status, ensuring that only the original error message from the child process is displayed.

Highlights

Silent Exit on Subprocess Failure: Modified the exec command to exit silently with the subprocess's exit code when it fails, preventing fnox from printing a redundant error message.
Error Variant Removal: Removed the CommandExitFailed error variant from FnoxError as it is no longer used after the change in subprocess error handling.

Changelog

src/commands/exec.rs
- Replaced FnoxError::CommandExitFailed with std::process::exit(code) for subprocess failures.
- Added a comment explaining the rationale for silent exit.
src/error.rs
- Removed the CommandExitFailed error variant.

Activity

The author provided a test plan to verify the new behavior: fnox exec -- false exits with code 1 silently, and fnox exec -- ls continues to work normally.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

On Unix, replace the fnox process with the child via exec(2) so there's no wrapper process. This ensures signals are delivered directly to the child and removes the noisy "Command exited with status" error. On non-Unix, fall back to spawning and silently exiting with the child's exit code. Also removes the now-unused `CommandExitFailed` error variant. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

gemini-code-assist

Code Review

This pull request aims to silence the error message when a subprocess executed via fnox exec fails, and instead exit with the subprocess's exit code. However, the current implementation uses std::process::exit directly, which bypasses Rust's cleanup mechanisms (destructors). This introduces a critical security vulnerability where temporary files containing sensitive secrets are not deleted from the disk upon failure. A detailed review comment provides an explanation of this issue and recommends propagating the exit code to the main function for a clean shutdown, ensuring proper resource cleanup.

gemini-code-assist · 2026-03-08T22:13:43Z

+            // Persist temp files so they survive exec (drop would delete them)
+            for tf in _temp_files {
+                tf.into_temp_path().keep().ok();
+            }
+            let err = cmd.exec();
+            return Err(FnoxError::CommandExecutionFailed {


The use of std::process::exit here prevents the execution of Rust's Drop implementation for local variables, specifically the _temp_files vector. This means temporary files containing sensitive secrets will not be deleted from the disk, leading to a potential security vulnerability where sensitive information could be leaked. A safer approach is to propagate the exit code to main for a clean shutdown, ensuring proper resource cleanup. Alternatively, ensure _temp_files is explicitly dropped before exiting.

Suggested change

// Persist temp files so they survive exec (drop would delete them)

for tf in _temp_files {

tf.into_temp_path().keep().ok();

}

let err = cmd.exec();

return Err(FnoxError::CommandExecutionFailed {

if !status.success() {

// Exit silently with the subprocess's exit code — the child

// already printed its own error, so an additional fnox error

// message would just be noise.

let code = status.code().unwrap_or(1);

drop(_temp_files);

std::process::exit(code);

}

greptile-apps · 2026-03-08T22:14:46Z

Greptile Summary

This PR changes fnox exec from a simple cmd.status() call to a spawn() + wait() model with manual Unix signal forwarding, and removes the now-unused CommandExitFailed error variant. On non-zero child exit the process now silently calls std::process::exit() (with _temp_files explicitly dropped first to avoid as_file leaks), matching the child's exit code.

Note: Despite the PR title and description claiming the use of exec(2) on Unix, the actual implementation uses spawn() + wait() with signal forwarding via signal-hook/nix — not os::unix::process::CommandExt::exec(). The PR description should be updated to reflect the real approach.

Key issues found:

Double signal delivery (logic bug): Because the child inherits the parent's process group, Ctrl-C sends SIGINT to both processes simultaneously via the terminal. The parent's handler then forwards it a second time, so the child receives two SIGINTs. Interactive or signal-sensitive programs may misbehave. Fix: call cmd.process_group(0) on Unix to isolate the child before spawning.
Signal handler registration race (logic bug): Handlers are registered after spawn() returns, leaving a window where a SIGINT/SIGTERM would trigger the parent's default action (exit) instead of being forwarded, orphaning the child.

Confidence Score: 2/5

The signal forwarding logic has two correctness bugs (double delivery and a registration race) that will affect interactive subprocesses; needs fixes before merging.
The overall shape of the change is sound — silent exit propagation and temp-file cleanup are handled correctly. However, the Unix signal-forwarding block has a double-delivery bug (child in same process group as parent) and a TOCTOU race between spawn and handler registration. Both can cause observable misbehavior with signal-sensitive subprocesses, and neither is guarded by a test. The PR description also incorrectly states that exec(2) is used.
src/commands/exec.rs — signal handler registration block (lines 166–182)

Important Files Changed

Filename	Overview
src/commands/exec.rs	Core change: replaces `cmd.status()` with `spawn()`+`wait()`, adds Unix signal forwarding via `signal-hook`/`nix`, and calls `process::exit` on non-zero child exit. Signal forwarding logic has two issues: double delivery (child is in same process group as parent) and a TOCTOU race between spawn and handler registration.
src/error.rs	Removes the unused `CommandExitFailed` variant; straightforward cleanup with no issues.
Cargo.toml	Adds `signal-hook = "0.4.3"` and `nix = { version = "0.31.2", features = ["signal", "process"] }` as direct dependencies; both are pulled in for the new Unix signal forwarding code.
Cargo.lock	Lock file updated to record `nix 0.31.2` and `signal-hook 0.4.3`; no concerns.

Sequence Diagram

sequenceDiagram
    participant User
    participant fnox as fnox (parent)
    participant OS as Terminal / OS
    participant Child as Child Process

    fnox->>Child: spawn()
    fnox->>fnox: register SIGINT/SIGTERM handlers (Unix)
    User->>OS: Ctrl-C
    OS->>fnox: SIGINT (process group broadcast)
    OS->>Child: SIGINT (process group broadcast) ← first delivery
    fnox->>Child: kill(child_pid, SIGINT) ← second delivery (bug)
    Child-->>fnox: exits (wait() returns)
    fnox->>fnox: drop(_temp_files)
    alt child succeeded
        fnox-->>User: Ok(())
    else child failed
        fnox-->>User: process::exit(code)
    end

_{Last reviewed commit: faea52c}

jdx · 2026-03-08T22:33:28Z

bugbot run

cursor

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Replace exec(2) with spawn+wait so destructors run and as_file temp files are cleaned up. Forward SIGINT/SIGTERM to the child process so Ctrl-C reaches it. Exit silently with the child's exit code (128+signal for signal deaths) to avoid noisy fnox error messages. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

socket-security · 2026-03-08T22:53:14Z

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	cargo/nix@0.31.2

View full report

greptile-apps · 2026-03-08T22:55:33Z

+        // Forward SIGINT/SIGTERM to the child so Ctrl-C and `kill` reach it.
+        #[cfg(unix)]
+        {
+            let child_pid = nix::unistd::Pid::from_raw(child.id() as i32);
+            unsafe {
+                // Ignore signals in the parent — the child handles them.
+                // When the child exits we propagate its exit code below.
+                signal_hook::low_level::register(signal_hook::consts::SIGINT, move || {
+                    nix::sys::signal::kill(child_pid, nix::sys::signal::SIGINT).ok();
+                })
+                .ok();
+                signal_hook::low_level::register(signal_hook::consts::SIGTERM, move || {
+                    nix::sys::signal::kill(child_pid, nix::sys::signal::SIGTERM).ok();
+                })
+                .ok();
+            }
+        }


Double signal delivery to child process

By default, std::process::Command::spawn() leaves the child in the same process group as the parent. When the user presses Ctrl-C, the terminal sends SIGINT to the entire foreground process group — so both the parent and the child receive it simultaneously. The parent's signal handler then forwards the signal to the child a second time, causing the child to receive the signal twice.

For many programs this is harmless, but programs that trap SIGINT for graceful shutdown (e.g. Python REPLs, long-running daemons with custom handlers, or tools that treat a second SIGINT as a hard kill) may behave incorrectly.

The standard fix is to move the child into its own process group before the signal handlers are registered, so the terminal signal only reaches the parent, which then forwards it exactly once:

use std::os::unix::process::CommandExt; // Inside the cfg(unix) block, before spawn: cmd.process_group(0); // child gets its own process group (PGID = child PID)

With process_group(0) set on cmd, the child is isolated from the terminal's broadcast, and the parent is the sole forwarder.

greptile-apps · 2026-03-08T22:55:34Z

+        let mut child = cmd.spawn().map_err(|e| FnoxError::CommandExecutionFailed {
+            command: self.command.join(" "),
+            source: e,
+        })?;
+
+        // Forward SIGINT/SIGTERM to the child so Ctrl-C and `kill` reach it.
+        #[cfg(unix)]
+        {
+            let child_pid = nix::unistd::Pid::from_raw(child.id() as i32);
+            unsafe {
+                // Ignore signals in the parent — the child handles them.
+                // When the child exits we propagate its exit code below.
+                signal_hook::low_level::register(signal_hook::consts::SIGINT, move || {
+                    nix::sys::signal::kill(child_pid, nix::sys::signal::SIGINT).ok();
+                })
+                .ok();
+                signal_hook::low_level::register(signal_hook::consts::SIGTERM, move || {
+                    nix::sys::signal::kill(child_pid, nix::sys::signal::SIGTERM).ok();
+                })
+                .ok();
+            }
+        }


Race condition between spawn and signal handler registration

Signal handlers are registered after cmd.spawn() returns. There is a small but real window between the moment the child process starts and the moment the handlers are armed. If SIGINT or SIGTERM arrives during that window (e.g., rapid Ctrl-C), the default parent action (terminate the parent) fires before the forwarding handler is in place — the child is left running without its expected signal, and fnox exits without waiting for it.

Moving handler registration to before cmd.spawn() (using a AtomicBool / OnceLock to store the PID after spawn) or switching to signal_hook's process-group-based approach would close this window.

@jdx

### 🚀 Features - **(cloudflare)** add Cloudflare API token lease backend by [@jdx](https://github.com/jdx) in [#335](#335) - **(fido2)** bump demand to v2, mask PIN during typing by [@jdx](https://github.com/jdx) in [#334](#334) - **(init)** add -f as short alias for --force by [@jdx](https://github.com/jdx) in [#329](#329) - **(lease)** add --all flag, default to creating all leases by [@jdx](https://github.com/jdx) in [#337](#337) - **(lease)** add GitHub App installation token lease backend by [@jdx](https://github.com/jdx) in [#342](#342) ### 🐛 Bug Fixes - **(config)** fix directory locations to follow XDG spec by [@jdx](https://github.com/jdx) in [#336](#336) - **(exec)** use unix exec and exit silently on subprocess failure by [@jdx](https://github.com/jdx) in [#339](#339) - **(fido2)** remove duplicate touch prompt by [@jdx](https://github.com/jdx) in [#332](#332) - **(set)** write to lowest-priority existing config file by [@jdx](https://github.com/jdx) in [#331](#331) - **(tui)** skip providers requiring interactive auth by [@jdx](https://github.com/jdx) in [#333](#333) ### 🛡️ Security - **(ci)** retry lint step to handle transient pkl fetch failures by [@jdx](https://github.com/jdx) in [#341](#341) - **(mcp)** add MCP server for secret-gated AI agent access by [@jdx](https://github.com/jdx) in [#343](#343) - add guide for fnox sync by [@jdx](https://github.com/jdx) in [#328](#328) ### 🔍 Other Changes - share Rust cache across CI jobs by [@jdx](https://github.com/jdx) in [#340](#340)

jdx force-pushed the fix/exec-silent-exit branch from 2e4cdbe to 26a1f1d Compare March 8, 2026 22:13

jdx changed the title ~~fix(exec): exit silently when subprocess fails~~ fix(exec): use unix exec and exit silently on subprocess failure Mar 8, 2026

gemini-code-assist Bot reviewed Mar 8, 2026

View reviewed changes

greptile-apps Bot reviewed Mar 8, 2026

View reviewed changes

Comment thread src/commands/exec.rs Outdated

cursor Bot reviewed Mar 8, 2026

View reviewed changes

Comment thread src/commands/exec.rs

Comment thread src/commands/exec.rs Outdated

greptile-apps Bot reviewed Mar 8, 2026

View reviewed changes

Comment thread src/commands/exec.rs Outdated

cursor Bot reviewed Mar 8, 2026

View reviewed changes

jdx enabled auto-merge (squash) March 8, 2026 22:39

jdx force-pushed the fix/exec-silent-exit branch from 26f32c3 to faea52c Compare March 8, 2026 22:52

jdx changed the title ~~fix(exec): use unix exec and exit silently on subprocess failure~~ fix(exec): forward signals to child and exit silently on failure Mar 8, 2026

greptile-apps Bot reviewed Mar 8, 2026

View reviewed changes

jdx merged commit 89dc437 into main Mar 8, 2026
17 checks passed

jdx deleted the fix/exec-silent-exit branch March 8, 2026 23:06

mise-en-dev mentioned this pull request Mar 8, 2026

chore: release v1.17.0 #330

Merged

BrewTestBot mentioned this pull request Mar 9, 2026

fnox 1.17.0 Homebrew/homebrew-core#271314

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(exec): forward signals to child and exit silently on failure#339

fix(exec): forward signals to child and exit silently on failure#339
jdx merged 2 commits intomainfrom
fix/exec-silent-exit

jdx commented Mar 8, 2026 •

edited by cursor Bot

Loading

Uh oh!

gemini-code-assist Bot commented Mar 8, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot Mar 8, 2026

Uh oh!

greptile-apps Bot commented Mar 8, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jdx commented Mar 8, 2026

Uh oh!

cursor Bot left a comment

Uh oh!

socket-security Bot commented Mar 8, 2026

Uh oh!

greptile-apps Bot Mar 8, 2026

Uh oh!

greptile-apps Bot Mar 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

-            // Persist temp files so they survive exec (drop would delete them)
-            for tf in _temp_files {
-                tf.into_temp_path().keep().ok();
-            }
-            let err = cmd.exec();
-            return Err(FnoxError::CommandExecutionFailed {
+        if !status.success() {
+            // Exit silently with the subprocess's exit code — the child
+            // already printed its own error, so an additional fnox error
+            // message would just be noise.
+            let code = status.code().unwrap_or(1);
+            drop(_temp_files);
+            std::process::exit(code);
+        }

Uh oh!

Conversation

jdx commented Mar 8, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

gemini-code-assist Bot commented Mar 8, 2026

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Mar 8, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot commented Mar 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 2/5

Important Files Changed

Sequence Diagram

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jdx commented Mar 8, 2026

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

socket-security Bot commented Mar 8, 2026

Uh oh!

greptile-apps Bot Mar 8, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Mar 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

jdx commented Mar 8, 2026 •

edited by cursor Bot

Loading

greptile-apps Bot commented Mar 8, 2026 •

edited

Loading