Skip to content

fix(tools): interpret lethal signal exits in background-process notifications - #88379

Open
pierrenode wants to merge 1 commit into
NousResearch:mainfrom
pierrenode:fix/background-process-signal-interpretation
Open

fix(tools): interpret lethal signal exits in background-process notifications#88379
pierrenode wants to merge 1 commit into
NousResearch:mainfrom
pierrenode:fix/background-process-signal-interpretation

Conversation

@pierrenode

Copy link
Copy Markdown
Contributor

Summary

204302b added terminal_tool._interpret_signal_exit() (11 curated signals: SIGKILL/SIGSEGV/SIGBUS/SIGFPE/etc.) and wired it into the foreground terminal_tool exit-code report via _interpret_exit_code(). It never touched format_process_notification() — the shared formatter for background/detached process completions, called from process_registry.py itself, tui_gateway/server.py (x2), and gateway/run.py's async-delegation path. That formatter kept its own, much older check that recognizes only SIGTERM:

if _exit in {-15, 143, "-15", "143"}:
    _signal = ", SIGTERM"

A background process killed by the OOM killer (exit 137/-9 — "the big one" per 204302b's own commit message) or crashing with SIGSEGV (139/-11) reached the model as a bare (exit code 137) with zero explanation — exactly the failure mode 204302b set out to fix, just on a different surface.

Changes

  • tools/process_registry.py: new _background_exit_signal_note() resolves exit_code (int or the numeric-string shape some producers use) through terminal_tool._interpret_signal_exit() via a lazy import — mirroring the existing established pattern in this pair of modules (process_registry.py already lazy-imports terminal_tool._rewrite_compound_background for the same reason: a circular top-level import). The note is surfaced as its own line rather than crammed into the exit-code parenthetical, matching how the foreground path surfaces it as its own exit_code_meaning field rather than a compact suffix.
  • tests/tools/test_process_registry.py: 7 new tests (TestBackgroundExitSignalNote).

No test previously asserted on the exact SIGTERM-suffix string, so the format change (moving the note out of the parenthetical) does not touch any covered contract — grepped test_watch_patterns.py, test_async_delegation.py, and test_context_compressor_zero_user_provenance.py's format_process_notification usages: all exercise other branches (watch_overflow, async_delegation, or only the "[IMPORTANT: Background process" prefix), none reach this code path.

Test plan

  • New tests reproduce the gap against pre-fix code and pass after the fix
  • Mutation-verified: reverting the fix makes 3 of the 7 new tests fail against pre-fix code (SIGKILL as int, SIGKILL via the 128+signum shell convention, SIGSEGV as a numeric string); the other 4 (SIGTERM still recognized, clean exit, ordinary non-zero exit, unknown "?" placeholder) correctly pass either way since they were never exposed to the bug
  • Broader neighbor suite (160 tests: test_process_registry.py, test_watch_patterns.py, test_async_delegation.py, test_context_compressor_zero_user_provenance.py, test_terminal_tool.py) and the dedicated test_terminal_signal_exit.py (27 tests) pass unchanged
  • ruff check clean on both changed files

Competing PR check

  • My own open fix(gateway): close 3 watch-event gaps left by the completion-notification hardening #86261 touches format_process_notification's neighborhood (redaction for watch_match/watch_disabled/watch_overflow_*) but in gateway/run.py's separate _format_gateway_process_notification — confirmed that function has no completion/exit-code branch of its own (evt_type == "completion" falls through to return None; the actual exit-code text is generated exclusively by process_registry.py::format_process_notification, which all 5 call sites — including the 2 in tui_gateway/server.py — delegate to directly). No overlap.
  • feat(desktop): let the agent reap background processes + neutral signal-kill rendering #49809 ("neutral signal-kill rendering") touches tools/process_registry.py and tests/tools/test_process_registry.py, but its diff adds remove()/clear_finished() registry methods and extends the process tool's action schema/dispatch — it never touches format_process_notification()'s body. No overlap.

…ications

204302b added terminal_tool._interpret_signal_exit() (11 curated
signals: SIGKILL/SIGSEGV/SIGBUS/SIGFPE/etc.) and wired it into the
foreground terminal_tool exit-code report via _interpret_exit_code(). It
never touched format_process_notification() -- the shared formatter for
background/detached process completions, called from process_registry.py
itself, tui_gateway/server.py (x2), and gateway/run.py's async-delegation
path. That formatter kept its own, much older check that recognizes only
SIGTERM:

    if _exit in {-15, 143, "-15", "143"}:
        _signal = ", SIGTERM"

A background process killed by the OOM killer (exit 137/-9 -- "the big
one" per 204302b's own commit message) or crashing with SIGSEGV
(139/-11) reached the model as a bare "(exit code 137)" with zero
explanation, exactly the failure mode 204302b set out to fix, just on
a different surface.

Fix: format_process_notification() now resolves exit_code (int or the
numeric-string shape some producers use) through terminal_tool's
_interpret_signal_exit() via a lazy import (existing established pattern
in this pair of modules -- process_registry.py already lazy-imports
terminal_tool._rewrite_compound_background for the same reason: a
circular top-level import). The note is surfaced as its own line rather
than crammed into the exit-code parenthetical, matching how the
foreground path surfaces it as its own `exit_code_meaning` field rather
than a compact suffix.

No test previously asserted on the exact SIGTERM-suffix string, so the
format change (moving the note out of the parenthetical) does not touch
any covered contract; grepped tests/tools/test_watch_patterns.py,
test_async_delegation.py, and test_context_compressor_zero_user_
provenance.py's format_process_notification usages -- all exercise other
branches (watch_overflow, async_delegation, or only the "[IMPORTANT:
Background process" prefix), none reach this code path.

Mutation-verified: reverting the fix makes 3 of the 7 new tests fail
against pre-fix code (SIGKILL as int, SIGKILL via the 128+signum shell
convention, SIGSEGV as a numeric string); the other 4 (SIGTERM still
recognized, clean exit, ordinary non-zero exit, unknown "?" placeholder)
correctly pass either way since they were never exposed to the bug.
Broader neighbor suite (160 tests across test_process_registry.py,
test_watch_patterns.py, test_async_delegation.py,
test_context_compressor_zero_user_provenance.py, test_terminal_tool.py)
and the dedicated test_terminal_signal_exit.py (27 tests) pass unchanged.
ruff clean.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/tools Tool registry, model_tools, toolsets tool/terminal Terminal execution and process management labels Aug 17, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference; please use your judgment.

Right unification: background-process notifications kept a stale SIGTERM-only check while the foreground path had gained the full _interpret_signal_exit table, so OOM-killed or segfaulting background jobs reached the model with an unexplained bare exit code. Delegating to the shared table (with the lazy import documented against the real circular dependency), handling both int and numeric-string producers, and the test matrix — OOM via -9 and shell-convention 137, string-form -11, SIGTERM regression guard, clean/ordinary/placeholder negatives — are all correct. Points:

  1. tools/process_registry.py:_background_exit_signal_note (~2878) — exit_code.lstrip("-").isdigit() admits malformed inputs like "--9": lstrip removes all leading dashes so the digit check passes, then int("--9") raises ValueError that this function does not catch — crashing notification formatting for that event. Tighten to a full-match regex (^-?\d+$) or wrap the int conversion; one-line fix.
  2. The note line is inserted inside the [IMPORTANT: …] block before "Command:" — good placement so the explanation travels with the payload. (positive)
  3. Consider asserting once that _interpret_signal_exit's table covers the shell-convention 128+N range generally, since producers may report either convention per signal — the tests sample two signals but the contract is "any lethal signal". (nit)

No blocking issues found beyond item 1's tiny parse gap.

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

Labels

comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists tool/terminal Terminal execution and process management type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants