run: signal every concurrent child on Ctrl-C, and make --color take effect

[colinhacks]

Fixes #685 plus four adjacent defects found while verifying it. Measured against pnpm 10.15.1. Recursive concurrency defaults to min(4, cpus), so this hits plain nub run -r, not only --parallel.

• Ctrl-C signalled one child: a single AtomicI32 target each spawn overwrote, zeroed by the first exit. Siblings were orphaned and nub hung. Now a set; group_on_spawn untouched, so SIGINT emitted twice on Ctrl+C #26/Does not work with Nx TUI #27/nub run orphans its workload on SIGKILL (Playwright webServer default teardown) — proposal: PR_SET_PDEATHSIG on the script child #463 hold.
• --color=always was inert. Now exports FORCE_COLOR=1 to scripts and --no-color exports 0, matching pnpm; --no-color also parses after the verb, as pnpm allows.
• NO_COLOR was ignored, and FORCE_COLOR now follows Node's value table ('' and false were both wrong).
• --reporter-hide-prefix stripped nub's own Done label; pnpm keeps it.

Reverting the signal fix turns three tests red. 12 new tests.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
nub	Ready	Preview	Aug 16, 2026 12:56am

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[pullfrog]

Important

One of the two commands in the new --color docs section fails to parse. nub run -r --no-color build exits 2 with error: unexpected argument '--no-color' found — --no-color is only recognized by the pre-verb argv scan, never by clap.

Reviewed changes — the full diff at 15234cb, plus the surrounding signal-forwarding, color-predicate, and workspace-run code, and an empirical check of the documented flag positions against a published nub binary.

• Concurrent Ctrl-C fan-out — ctrl_c's single AtomicI32 target becomes a Mutex<Vec<i32>> multiset, so each of the min(4, cpus) concurrent -r members registers and removes its own group instead of evicting its siblings.
• untrack_child takes a pid — every call site now passes the pid it tracked, so the first member to finish no longer disarms forwarding for the rest.
• --color becomes load-bearing — a new COLOR_MODE global is recorded from both the pre-verb scan and the parsed clap Cli, and build_script_command exports FORCE_COLOR=1/0 to script children for always/never.
• One color predicate — color_enabled replaces the open-coded is_terminal() || FORCE_COLOR present checks in the stream prefix, help_bold, and pm_engine::scope_warning_uses_dim, adding the missing NO_COLOR consultation and reading FORCE_COLOR=0 as off.
• --reporter-hide-prefix narrowed — Nub's own Done / exit N / error: status lines route through a new format_status_prefix that keeps the <dir> <script>: label.
• Tests and docs — three signal tests (fan-out, sibling survival, the rewritten evict-vs-add test), five color/reporter integration tests with a color-probe fixture script, and new --reporter-hide-prefix / --color sections in run.mdx.

ℹ️ Three doc comments still describe the predicate this PR replaced

color_enabled is now the single answer for every ANSI decision, but three comments around its callers still describe the old two-term check and omit the explicit-flag layer that now outranks the environment. The one in pm_engine/mod.rs sits two lines above the function this PR changed.

Technical details

# Stale color-predicate doc comments

## Affected sites
- `crates/nub-cli/src/pm_engine/mod.rs:1492` — `emit_scope_warnings`'s doc says dim is chosen when "stderr is a terminal (or `FORCE_COLOR` set) and `NO_COLOR` is unset". It now also depends on `--color`/`--no-color`, and `FORCE_COLOR=0` now reads as off. Directly above `scope_warning_uses_dim`, whose body this PR rewrote.
- `crates/nub-cli/src/cli.rs:10360` — `emit_shim_version_line` describes itself as "reusing the PM engine's TTY/`FORCE_COLOR`/`NO_COLOR` predicate"; the engine now delegates to `cli::color_enabled`.
- `crates/nub-cli/src/pm_engine/use_nub.rs:1044` — `warn_phantom_dependencies` says "dim-styled when stderr is a terminal and `NO_COLOR` is unset (same gate as the rest of pm_engine)".

## Required outcome
- Each comment describes the four-tier precedence `color_enabled` actually implements, or defers to it by name instead of restating it.

ℹ️ Nitpicks

• crates/nub-cli/src/cli.rs:6369 and crates/nub-core/src/node/spawn.rs:1390 both ? out of child.wait() before reaching untrack_child. That shape predates this PR, but its consequence changed: the old single slot healed itself on the next track(), whereas the Vec keeps the stale -pid for the life of the process and a later signal would kill whatever group inherits it. Child::wait() failing is close to impossible, so this is only worth the cost if an RAII guard over the track/untrack pair falls out cheaply.
• The FORCE_COLOR=1 / FORCE_COLOR=0 mapping is attributed to a local measurement of pnpm 10.15.1. pnpm's CLI docs don't list --color at all and its code search is sign-in gated, so nothing external corroborates it — worth a source link in the comment at cli.rs:5704 if one exists.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏}

[pullfrog]

ℹ️ No blocking issues in the new commit — both prior findings are properly fixed, with one observation and two nits inline.

Reviewed changes — the delta since the prior pullfrog review at 15234cb, plus the surrounding color-resolution code, the clap positional-splitting helpers, and Node's lib/internal/tty.js as the authority for the new value table. The signal-forwarding work from the first commit is unchanged and was not re-reviewed.

• Made --no-color a real clap arg — declared as a global, conflicts_with = "color" flag on Cli, so the post-verb spelling nub run -r --no-color build parses instead of exiting 2, and the docs example is no longer broken. Being a boolean, it needs no value_consuming_flags entry, so split_subcommand_argv still binds the script positional correctly.
• Routed it through dispatch_subcommand — if cli.no_color { Never } else if cli.color != Auto { cli.color }, so the later spelling wins while clap's auto default still can't clobber a pre-verb --color.
• Aligned the FORCE_COLOR arm with Node's table — '', '1', 'true', '2' and '3' enable color; everything else, '0' and 'false' included, disables it. I checked this against nodejs/node's lib/internal/tty.js on main: the value table matches exactly, and Node's NO_COLOR check is likewise "defined and non-empty", so the new comments are accurate.
• Documented the one deliberate divergence — when both FORCE_COLOR and NO_COLOR are set, Node lets FORCE_COLOR win and warns; Nub lets NO_COLOR win.
• Added two integration tests — --no-color in both flag positions, and the five-value FORCE_COLOR table.
• Listed --no-color in the hand-rolled --help.

ℹ️ Nitpicks

• Post-verb --color=<when> still has no end-to-end coverage. color_always_exports_force_color_to_the_script uses the pre-verb spelling, so the else if cli.color != ColorWhen::Auto branch at crates/nub-cli/src/cli.rs:2799 — restructured by this commit — is only exercised through clap's own parse test. That's the same shape as the defect this PR fixes (a flag that parsed into a field nothing read), and it costs one extra row in the existing --no-color position loop.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏}

[pullfrog]

ℹ️ No blocking issues in the new commit — both prior findings are properly fixed, with one scoping observation inline.

Reviewed changes — the delta since the prior pullfrog review at 5966656, plus every call site of build_script_command and the StreamMode each one passes. The signal-forwarding work and the --reporter-hide-prefix change are unchanged and were not re-reviewed.

• Extracted the FORCE_COLOR value table into a shared helper — force_color_enables(&OsStr) is now called by both color_enabled and the script launcher, so the value Nub reads and the value it hands a child can't drift. The table itself is unchanged.
• Pinned the child's FORCE_COLOR for the one contradictory pair — in auto, when FORCE_COLOR is set to an enabling value and NO_COLOR is set, build_script_command exports FORCE_COLOR=0 so Nub's plain label and the child's lines agree. The predicate reduces to exactly that pair: FORCE_COLOR alone can never trigger it, because color_enabled reads the same value and agrees.
• Named the consequence in the divergence note — color_enabled's doc comment now says what the NO_COLOR-wins order would otherwise do to a run, and points at the arm that closes it.
• Added the test and its control — contradictory_color_env_does_not_split_nub_from_its_child asserts the child receives FC=0, and auto_still_forces_nothing_when_only_no_color_is_set asserts FC=unset with only one variable set. Reverting the arm turns the first red, so it isn't passing for the wrong reason.
• Tightened the --no-color position test — .env_remove("NO_COLOR") added, so an ambient NO_COLOR can no longer satisfy the assertion on the flag's behalf.

ℹ️ Nitpicks

• site/content/docs/runner/run.mdx:382 still says "Without the flag Nub forces nothing either way, and the usual environment signals decide". That is now false for the one case this commit added — with both NO_COLOR and FORCE_COLOR set, Nub decides for the child rather than letting the environment signals reach it. Half a sentence covers it.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏}

[pullfrog]

✅ No new issues found.

Reviewed changes — the delta since the prior pullfrog review at d8dfd79, plus StreamMode's full variant set, both build_script_command call sites, and the workspace-detection walk that decides which one the new test reaches.

• Scoped the auto FORCE_COLOR=0 export to the prefixed path — the arm is now ColorWhen::Auto if matches!(stream, StreamMode::Prefixed) with a bare ColorWhen::Auto => {} fall-through. StreamMode has exactly two variants, so every piped run still gets the contradictory pair repaired and every inherited-stdio run hands the script the environment it was given, matching pnpm, npm and a bare shell.
• Rewrote the rationale in place — the comment now says why the exception is confined to the path where Nub actually wraps the child's lines, rather than describing a repair it was applying where nothing needed repairing.
• Pinned the Inherit path with a test — an_unprefixed_run_passes_the_color_env_through_untouched runs nub run color-probe from packages/core with both variables set and asserts the child sees FC=1. I traced the path it exercises: detect_project_walk roots the project at the member directory, nothing infers an implicit -r, so it lands in run_single_script → spawn_script, the only StreamMode::Inherit call site. Deleting the guard makes the assertion read FC=0, so the test is load-bearing rather than decorative.
• Closed the docs gap — run.mdx now states what happens when both variables are set, and that an unprefixed run passes them through, which is the qualification the earlier "Nub forces nothing either way" sentence was missing.

  ^{｜ View workflow run ｜ Using Claude Opus ｜ 𝕏}

[pullfrog]

ℹ️ No new issues — the fix is correct and minimal. One observation on the constraint it works around.

Reviewed changes — the delta since the prior pullfrog review at 14c164e, plus cli_grammar_parity's rejection detector and the --help surface it reads. The signal fan-out, --color resolution and --reporter-hide-prefix work is unchanged and was not re-reviewed.

• Demoted the --no-color rationale to a plain comment — the clap-versus-argv-scan explanation on Cli::no_color is now // rather than ///, so clap stops rendering it as long_help. The user-facing one-liner stays a doc comment, so --help still describes the flag.
• Recorded the coupling in place — the new comment states why the text cannot be a doc comment, which is the part a future edit would otherwise re-break.

I traced the mechanism rather than taking the commit message for it. clap_rejected spawns nub <form> --help and greps the combined stdout and stderr for unexpected argument / unrecognized (crates/nub-cli/tests/cli_grammar_parity.rs:78-84); clap derive turns a multi-paragraph doc comment into long_help, and a global = true arg's long_help lands in every subcommand's help. So the removed paragraph did make every row in that table report a rejection. That failure was loud rather than silent — rejected == true pushes a row into failures — so nothing was passing vacuously in the meantime. I also checked that no other global-arg doc comment in cli.rs carries either marker.

ℹ️ The parity detector still reads help text as if it were parser output

clap_rejected decides "the parser refused this form" from a substring of stdout and stderr, but clap prints help to stdout and parse errors to stderr. Nothing in --help prose is parser output, so scanning it can only produce false rejections — which is exactly what happened here, and the fix landed on the prose rather than the detector. The workaround holds today, but it leaves a standing rule that no --help text may contain two ordinary English phrases.

Technical details

# `cli_grammar_parity`'s reject markers are matched against help output

## Affected sites
- `crates/nub-cli/tests/cli_grammar_parity.rs:78-84` — `combined` concatenates stdout and
  stderr, then `rejected` is `combined.contains("unexpected argument") ||
  combined.contains("unrecognized")`. Help text printed to stdout is scanned as though it
  were a clap error.
- `crates/nub-cli/src/cli.rs:749-753` — the new comment exists only to keep help prose out
  of the detector's way. Any future `global` arg doc comment describing a rejection
  reintroduces the same red.

## Required outcome
- A row fails only when clap actually refused the grammar, so help prose cannot influence
  the verdict — and `cli.rs` no longer has to avoid two English phrases.

## Suggested approach (optional)
- Match the markers against `out.stderr` alone. Clap writes `--help` output to stdout and
  `Error::print` to stderr, so the two streams already separate parser verdicts from prose;
  `combined` can stay in the returned tuple for the failure dump.
- Worth confirming empirically on one known-rejecting form before relying on it — the same
  suite is the natural place to pin it, since a row that stops detecting rejections would
  otherwise fail open.

## Open questions for the human
- Is this worth touching in this PR at all, given the comment now documents the constraint?
  It is orthogonal to #685 and would be a clean standalone change.

  ^{｜ Fix it ➔ ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏}