From 8581b57f4671a67c640d377f02101203e8baf626 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sat, 5 Sep 2026 06:30:33 -0700 Subject: [PATCH] fix(terminal): promotion keeps the shell-detachment refusal; the note only promises a notification the session can receive Independent review: an over-cap timeout on `cmd &` was promoted, so the tracked shell exited at once while the payload ran untracked (the exact thing the '&' guidance exists to prevent); and the note promised a notification even on finite sessions where async delivery is disabled and the spawn had already cleared notify_on_complete. The detachment guidance now runs before the promotion decision regardless of timeout, and the note reads the spawn's actual notify_on_complete: notification wording when kept, poll-only wording when the session cannot receive one. Tests (2 new): '&' and nohup with an over-cap timeout still refuse; the note matches the delivery capability. --- .../test_terminal_foreground_timeout_cap.py | 23 +++++++++++++++++ tools/terminal_tool.py | 25 ++++++++++++++----- 2 files changed, 42 insertions(+), 6 deletions(-) diff --git a/tests/tools/test_terminal_foreground_timeout_cap.py b/tests/tools/test_terminal_foreground_timeout_cap.py index 7d437c304b..b43c4941b9 100644 --- a/tests/tools/test_terminal_foreground_timeout_cap.py +++ b/tests/tools/test_terminal_foreground_timeout_cap.py @@ -170,3 +170,26 @@ class TestForegroundMaxTimeoutConstant: timeout_desc = TERMINAL_SCHEMA["parameters"]["properties"]["timeout"]["description"] assert str(FOREGROUND_MAX_TIMEOUT) in timeout_desc assert "background process" in timeout_desc + + +class TestPromotionKeepsTheDetachmentGuard: + def test_over_cap_timeout_with_shell_backgrounding_is_still_refused(self): + """Independent-review witness: a promoted `cmd &` started a tracked shell that exited at once + while the payload ran untracked, defeating the guidance the refusal exists for.""" + from tools.terminal_tool import terminal_tool + + with patch("tools.terminal_tool._get_env_config", return_value=_make_env_config()), \ + patch("tools.terminal_tool._start_cleanup_thread"): + result = json.loads(terminal_tool(command="sleep 5 &", timeout=9999)) + result2 = json.loads(terminal_tool(command="nohup make test", timeout=9999)) + assert "'&' backgrounding" in result["error"] + assert "nohup" in result2["error"] + + def test_note_does_not_promise_a_notification_the_session_cannot_receive(self): + from tools.terminal_tool import _with_promoted_note + + kept = json.loads(_with_promoted_note(json.dumps({"session_id": "proc_x", "error": None, "notify_on_complete": True}), 900)) + assert "arrives as a notification" in kept["promoted_from_foreground"] + dropped = json.loads(_with_promoted_note(json.dumps({"session_id": "proc_x", "error": None, "notify_on_complete": False}), 900)) + assert "cannot receive completion notifications" in dropped["promoted_from_foreground"] + assert "poll" in dropped["promoted_from_foreground"] diff --git a/tools/terminal_tool.py b/tools/terminal_tool.py index 6815ad542a..d199c255fd 100644 --- a/tools/terminal_tool.py +++ b/tools/terminal_tool.py @@ -954,12 +954,13 @@ def _plan_execution( # re-sent lower/split/background. Promote to a tracked background process instead; the # caller is told in the result. The `&`/nohup/server guidance below stays a refusal: those # need the command itself rewritten, which the tool cannot do safely. + # The detachment guidance applies whether or not the call is promoted: a promoted `cmd &` + # would start a tracked shell that exits at once while its payload runs untracked. + guidance = _foreground_background_guidance(command) + if guidance: + raise _Rejected(_error_json(guidance, status="error")) if timeout and timeout > FOREGROUND_MAX_TIMEOUT: promoted = timeout - else: - guidance = _foreground_background_guidance(command) - if guidance: - raise _Rejected(_error_json(guidance, status="error")) return _ExecPlan( config=config, env_type=env_type, effective_task_id=effective_task_id, @@ -968,15 +969,25 @@ def _plan_execution( ) +_PROMOTED_NOTE_POLL_ONLY = ( + "Requested foreground timeout {requested}s exceeds the {cap}s cap, so this command was started as a " + "tracked background process instead of being refused. Do NOT re-run it. This session cannot receive " + "completion notifications, so poll it with process(action=\"poll\", session_id=...) until it exits." +) + + def _with_promoted_note(result_json: str, requested_timeout: int) -> str: - """Attach the foreground->background promotion note to a spawn result (unchanged on error).""" + """Attach the foreground->background promotion note to a spawn result (unchanged on error). The + note only promises a notification when the spawn actually kept notify_on_complete (finite sessions + such as one-shot runners cannot route one back; the spawn already said so and cleared the flag).""" try: data = json.loads(result_json) except (TypeError, ValueError): return result_json if not isinstance(data, dict) or data.get("error"): return result_json - data["promoted_from_foreground"] = _PROMOTED_NOTE.format(requested=requested_timeout, cap=FOREGROUND_MAX_TIMEOUT) + template = _PROMOTED_NOTE if data.get("notify_on_complete") else _PROMOTED_NOTE_POLL_ONLY + data["promoted_from_foreground"] = template.format(requested=requested_timeout, cap=FOREGROUND_MAX_TIMEOUT) return json.dumps(data, ensure_ascii=False) @@ -1181,6 +1192,8 @@ def terminal_tool( pty_disabled = pty and _command_requires_pipe_stdin(command) if plan.promoted_from_foreground_timeout is not None: + # Promotion implies notify_on_complete; watch_patterns is a background-only flag the + # caller could not have meant for a foreground call, and the two are exclusive anyway. background, notify_on_complete, watch_patterns = True, True, None if background: result = spawn_background_process(