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.
This commit is contained in:
@@ -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"]
|
||||
|
||||
+19
-6
@@ -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(
|
||||
|
||||
Reference in New Issue
Block a user