Skip to content

fix(agent): a background command that dies at once is reported as ended, not started - #720

Merged
ZuyiZhou merged 2 commits into
mainfrom
fix/background_exec_reports_early_exit
Sep 23, 2026
Merged

ZuyiZhou merged 2 commits into
mainfrom
fix/background_exec_reports_early_exit

Conversation

@ZuyiZhou

@ZuyiZhou ZuyiZhou commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Summary

A background exec launch (run_in_background=True) returned as soon as Popen did and always told the model the command was running. A command that fails at once - a port already bound, a missing binary - was handed back as a live task. In session 692e5c (2026-09-22) python -m http.server 8765 lost the port and exited, and the agent went on debugging its client against a server that never bound.

  • start_note now waits up to 0.9s (_CONFIRM_S) for the process to contradict the launch. A command that ended in that window gets an "ended immediately" note with its exit code (or the signal that killed it) and the last 2000 bytes of its log, and is named as not running and not reaped later. A command still alive after the window gets the existing running note, which claims no more than the window shows.
  • The wait runs in a worker thread (asyncio.to_thread in ExecTool.execute), so a launch that keeps running does not stall the event loop, and every other turn in the gateway with it, for that second.
  • The note carries the exit verdict: start_note returns a ToolOutput with ok=False for a nonzero exit seen in the window, so the registry, the tool event and every success-sensitive consumer record the call as failed, as the synchronous lane does. An exit 0 inside the window and a task still running stay ok=True.
  • The env-hygiene test now reads the child's output from the launch note, since an env that finishes inside the window is reported as ended with its log attached.

Type

  • Fix
  • Feature
  • Docs
  • CI / tooling
  • Refactor
  • Other

Verification

Head e6183de, rebased on origin/main 8ee5ae0; git range-diff shows both commits replayed unchanged, and tests/test_shell_background.py reran at this head: 23 passed. The other numbers below are from ea9d6c2 on afdc04f.

uv run --frozen --python 3.12 --all-extras pytest -q tests/test_shell_background.py tests/test_tool_registry*.py tests/test_shell*.py
  615 passed

COLORTERM=truecolor uv run --frozen --python 3.12 --all-extras pytest -q -p no:cacheprovider
  25910 passed, 109 skipped, 1 failed: tests/test_rpc_files.py::test_a_host_without_libreoffice_says_so
  (local only and pre-existing: it fails the same way on origin/main afdc04f89 on this machine, which has
  LibreOffice installed; this branch does not touch rpc/files)

uv run --frozen --python 3.12 --extra dev ruff check raven tests                  -> All checks passed
uv run --frozen --python 3.12 --extra dev ruff format --check raven/agent/tools tests/test_shell_background.py -> already formatted
make lint-types                                                                    -> exit 0
git diff --check origin/main...HEAD                                                -> clean
PYTHONPATH=. uv run --frozen --python 3.12 --extra dev python scripts/check_source_language.py origin/main...HEAD -> exit 0
make check-commits; make check-pr-title; COMMIT_RANGE=origin/main...HEAD make check-large-files -> exit 0
git merge-tree --write-tree HEAD origin/main                                       -> clean

Revert-to-red, run with tests/test_shell_background.py:

  • start_note reverted to the unconditional running note: 7 failed (the six ended-note tests plus the env-hygiene test).
  • ExecTool calling start_note on the loop thread instead of asyncio.to_thread: 1 failed, test_the_launch_check_waits_off_the_event_loop (it measures the largest gap between 50 ms ticks of a concurrent task, about 0.9s when blocked).
  • start_note returning the ended note without ok=: 1 failed, test_the_registry_reads_an_early_exit_as_the_call_s_verdict[exit 7-True] (it goes through ToolRegistry.execute and call_failed).

Not run: make coverage-diff, and the integration suite (-m integration); this change does not touch an integration path.

  • Relevant tests pass locally
  • Relevant lint / type checks pass locally
  • User-facing docs or screenshots are updated when needed (no doc describes the launch note)

Risk

  • Every successful background launch now returns about 0.9s later than before; the turn still does not wait for the command itself.

  • A command that legitimately finishes within 0.9s (for example echo) is now reported as ended, with its output, instead of as a background task to poll. That is the accurate answer, and nothing is left to reap.

  • Rollback: revert the commit; no state or config format changes.

  • Security impact considered

  • Backward compatibility considered

  • Rollback path is clear for risky changes

Related Issues

N/A

@ZuyiZhou
ZuyiZhou requested a review from LivXue as a code owner September 23, 2026 09:25

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: preserve a nonzero early-exit verdict in the tool result.

I found one functional blocker. I reviewed the full diff and the surrounding process lifecycle, the sole production caller, relevant history, backward compatibility, and the runtime/tool-result architecture. I also checked the repository rules and canonical runtime terminology, and verified that the adjusted tests were not weakened to manufacture a pass.

Verification: uv run pytest tests/test_shell_background.py -q passed (20 tests), and git diff --check github/main...HEAD passed. I additionally reproduced the finding through ToolRegistry.execute: exit 7 is described as exited but returns ok=True and call_failed=False.

Comment thread raven/agent/tools/shell.py
@ZuyiZhou
ZuyiZhou force-pushed the fix/background_exec_reports_early_exit branch from b4c5c95 to ea9d6c2 Compare September 23, 2026 11:51

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No blockers; this can merge as far as I am concerned.

The prior verdict issue is fixed: early nonzero exits now carry ok=False through ExecTool and the registry, while exit 0 and still-running launches retain ok=True. I reviewed the full branch diff and the new revision delta, callers, relevant history, backward compatibility, tool-result architecture, repository rules and runtime terminology; I also checked that the tests preserve the earlier behavioral coverage and add a genuine registry-level regression case.

Verification: uv run pytest tests/test_shell_background.py tests/test_tool_registry_execute.py -q passed (50 tests); direct registry reproduction produced the expected verdicts for exit 7, exit 0, and sleep 30; git diff --check github/main...HEAD passed. The earlier thread is resolved.

@ZuyiZhou
ZuyiZhou force-pushed the fix/background_exec_reports_early_exit branch from ea9d6c2 to af81da8 Compare September 23, 2026 14:09

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No blockers; this can merge as far as I am concerned.

This head is a rebase onto the newer main; git range-diff shows both PR commits are patch-identical to the previously reviewed clean revision. I rechecked the complete branch diff, the sole production caller, relevant history, backward compatibility, tool-result architecture, repository rules and runtime terminology, and confirmed the tests were not weakened. The earlier finding remains fixed and its thread remains resolved.

Verification: uv run pytest tests/test_shell_background.py tests/test_tool_registry_execute.py -q passed (50 tests), and git diff --check github/main...HEAD passed.

ZuyiZhou and others added 2 commits September 23, 2026 22:34
…ed, not started

A background exec launch returned as soon as Popen did and always told the
model the command was running. A command that fails immediately (a port
already bound, a missing binary) was handed back as a live task, and the
model went looking for a fault in its client against a server that never
bound.

The launch note now waits up to 0.9s for the process to contradict it. A
command that ended in that window is reported with its exit code or signal
and the tail of its log, and is named as not running and not reaped later.
A command still alive after the window gets the running note, which claims
no more than the window shows. The wait runs in a worker thread, so a launch
that keeps running does not stall the event loop for the other turns.

Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
start_note knew the command had exited and with which code, but handed
back a plain string, so the registry recorded the call as ok=True: the
tool event and every success-sensitive consumer read `exit 7` as a good
call, where the synchronous lane reports it failed. start_note now returns
a ToolOutput whose ok is the exit verdict: nonzero fails, while an exit 0
inside the window and a task still running both stay successful.

Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
@ZuyiZhou
ZuyiZhou force-pushed the fix/background_exec_reports_early_exit branch 2 times, most recently from af81da8 to e6183de Compare September 23, 2026 14:38

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No blockers; this can merge as far as I am concerned.

This head is another rebase onto current main; git range-diff shows both PR commits are patch-identical to the previously reviewed clean revision. I rechecked the complete three-file diff, caller and compatibility boundaries, relevant history, tool-result architecture, repository rules and runtime terminology, and confirmed the tests remain substantive rather than weakened. The earlier finding remains fixed and its thread remains resolved.

Verification: uv run pytest tests/test_shell_background.py tests/test_tool_registry_execute.py -q passed (50 tests), and git diff --check github/main...HEAD passed.

@ZuyiZhou
ZuyiZhou merged commit c415890 into main Sep 23, 2026
45 of 70 checks passed
@ZuyiZhou
ZuyiZhou deleted the fix/background_exec_reports_early_exit branch September 23, 2026 14:57
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.

3 participants