From a670ef53eec1e7eab8785e0991788cfb078a6a0f Mon Sep 17 00:00:00 2001 From: Godopu Date: Thu, 8 Oct 2026 08:12:16 +0900 Subject: [PATCH] fix(issue-17): a decision the wrapper cannot make ends it with 1 (N2) `exit "$(_decide exit-code ...)"` became `exit ""` when wrapper.py could not answer (its module or $PY broken): rc 255 on bash 3.2. The other printed decisions failed open in their own ways: a failed wait-rc returned "" (255), a failed finalize-begin left finalize-end with an empty --final (argparse rc 2), and a failed sub-status or finalize-end ended under set -e with the interpreter's own code (1 for a traceback, 127 for one that cannot run). Printed decisions now go through _decided , and the exits through _exit_for : a failed call, an empty answer or a non-numeric exit code / wait rc prints one ERROR line and exits 1. finalize- end may legitimately answer nothing (its wrapper gone), so only its rc is checked. The normal path is unchanged: same answers, exit codes and output. Left as they are: subscriber-up and stall-watch are answered by rc, so a failure reads as "no" - the subscriber is then treated as dead (the error path, whose sub-status/exit-code are now fail-closed) and the notify-only stall watcher is not started. Tests (F): the real submit (human job, unreachable broker, 3 s budget) with a $PY that fails one decision - exit-code, sub-status, finalize-begin, finalize-end; traceback, rc 127 or no answer - ends with 1 and the ERROR line; the same run without a failure still ends with 3. cmd_wait with a failing wait-rc ends with 1. A static guard keeps printed decisions off raw $(...). At 56d0bfd: exit-code 255/255, finalize-begin 2/2, wait-rc 255/255, sub-status and finalize-end 127 for "gone"; the traceback and no-answer cases there already ended with 1 (by set -e or by the empty status mapping to 1) and fail only on the missing ERROR line. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01HCGpJXhSZasJLU38ksDghe --- .../multi-agent-mux-delegate-job | 57 ++++++++++---- tests/test_issue17_item4_decisions.py | 75 +++++++++++++++++++ 2 files changed, 118 insertions(+), 14 deletions(-) diff --git a/.agents/skills/multi-agent-mux-delegate-job/multi-agent-mux-delegate-job b/.agents/skills/multi-agent-mux-delegate-job/multi-agent-mux-delegate-job index 93bff1a..4e6bf08 100755 --- a/.agents/skills/multi-agent-mux-delegate-job/multi-agent-mux-delegate-job +++ b/.agents/skills/multi-agent-mux-delegate-job/multi-agent-mux-delegate-job @@ -100,6 +100,32 @@ _decide() { PYTHONPATH="$SCRIPT_DIR/..${PYTHONPATH:+:$PYTHONPATH}" "$PY" -m lib_py.delegation.wrapper "$@" } +# A decision wrapper.py could not make (its module or $PY broken) ends the +# wrapper with 1 (fail-closed): never with its own code, 0, or the empty answer +# (`exit ""` is 255 on bash 3.2). +_decision_failed() { + echo "ERROR: the wrapper could not decide '$1' (lib_py.delegation.wrapper: $2); exiting 1 (fail-closed)" >&2 + exit 1 +} + +# _decided : a printed decision into $var. +_decided() { + local __var="$1" __out __rc=0 + shift + __out="$(_decide "$@")" || __rc=$? + [ "$__rc" -eq 0 ] || _decision_failed "$1" "rc $__rc" + [ -n "$__out" ] || _decision_failed "$1" "no answer" + printf -v "$__var" '%s' "$__out" +} + +# _exit_for : exit with the wrapper exit code for a job status. +_exit_for() { + local code + _decided code exit-code "$1" + [[ "$code" =~ ^[0-9]+$ ]] || _decision_failed exit-code "answer '$code'" + exit "$code" +} + # _pub_cmd : the publish command as one shell-quoted string. _pub_cmd() { local s @@ -361,12 +387,12 @@ EOF else echo "ERROR: subscriber died early (pid=$sub_pid, exit=$sub_exit, check $logf)" >&2 local mapped_st - mapped_st="$(_decide sub-status "$sub_exit" --job "$JOB_ID" --registry-dir "$REGISTRY_DIR")" + _decided mapped_st sub-status "$sub_exit" --job "$JOB_ID" --registry-dir "$REGISTRY_DIR" finalize_unfinished "$JOB_ID" "$mapped_st" "$sub_exit" "subscriber" mapped_st="${FINAL_JOB_STATUS:-$mapped_st}" local logs_root="${DELEGATE_JOB_LOGS_DIR:-$WORKDIR/delegate_job_logs}" echo "$logs_root/$JOB_ID" - exit "$(_decide exit-code "$mapped_st")" + _exit_for "$mapped_st" fi fi sleep 0.2 @@ -402,7 +428,7 @@ EOF echo "subscriber output:"; cat "$logf" || true local job_status - job_status="$(_decide sub-status "$sub_rc" --job "$JOB_ID" --registry-dir "$REGISTRY_DIR")" + _decided job_status sub-status "$sub_rc" --job "$JOB_ID" --registry-dir "$REGISTRY_DIR" finalize_unfinished "$JOB_ID" "$job_status" "$sub_rc" "direct" job_status="${FINAL_JOB_STATUS:-$job_status}" @@ -411,7 +437,7 @@ EOF # "Audit Logs"). Callers can scrape `tail -n1` to find it. local logs_root="${DELEGATE_JOB_LOGS_DIR:-$WORKDIR/delegate_job_logs}" echo "$logs_root/$JOB_ID" - exit "$(_decide exit-code "$job_status")" + _exit_for "$job_status" else # Implement loop/discuss orchestrator local iteration=1 @@ -533,12 +559,12 @@ EOF else echo "ERROR: subscriber died early (pid=$sub_pid, exit=$sub_exit, check $logf)" >&2 local mapped_st - mapped_st="$(_decide sub-status "$sub_exit" --job "$JOB_ID" --registry-dir "$REGISTRY_DIR")" + _decided mapped_st sub-status "$sub_exit" --job "$JOB_ID" --registry-dir "$REGISTRY_DIR" finalize_unfinished "$JOB_ID" "$mapped_st" "$sub_exit" "subscriber" mapped_st="${FINAL_JOB_STATUS:-$mapped_st}" local logs_root="${DELEGATE_JOB_LOGS_DIR:-$WORKDIR/delegate_job_logs}" echo "$logs_root/$JOB_ID" - exit "$(_decide exit-code "$mapped_st")" + _exit_for "$mapped_st" fi fi sleep 0.2 @@ -571,7 +597,7 @@ EOF echo "subscriber output:"; cat "$logf" || true local job_status - job_status="$(_decide sub-status "$sub_rc" --job "$JOB_ID" --registry-dir "$REGISTRY_DIR")" + _decided job_status sub-status "$sub_rc" --job "$JOB_ID" --registry-dir "$REGISTRY_DIR" echo "Job role $display_role finished with status: $job_status" @@ -590,7 +616,7 @@ EOF else local logs_root="${DELEGATE_JOB_LOGS_DIR:-$WORKDIR/delegate_job_logs}" echo "$logs_root/$JOB_ID" - exit "$(_decide exit-code "$job_status")" + _exit_for "$job_status" fi fi @@ -615,7 +641,7 @@ EOF else local logs_root="${DELEGATE_JOB_LOGS_DIR:-$WORKDIR/delegate_job_logs}" echo "$logs_root/$JOB_ID" - exit "$(_decide exit-code "$job_status")" + _exit_for "$job_status" fi fi @@ -805,10 +831,11 @@ _within_deadline() { finalize_unfinished() { local job_id="$1" job_status="$2" sub_rc="$3" phase="${4:-}" local target_sess="${5:-${current_session:-${target_session:-$AGENT_SESSION}}}" - local step final evidence_until deadline out - read -r step final evidence_until deadline <<<"$(_decide finalize-begin --job "$job_id" \ + local step final evidence_until deadline out begin end_rc=0 + _decided begin finalize-begin --job "$job_id" \ --registry-dir "$REGISTRY_DIR" --status "$job_status" ${START_TIME:+--start-time "$START_TIME"} \ - --timeout "$TIMEOUT" --idle-timeout "$IDLE_TIMEOUT" --agent "$AGENT" --session "$target_sess")" + --timeout "$TIMEOUT" --idle-timeout "$IDLE_TIMEOUT" --agent "$AGENT" --session "$target_sess" + read -r step final evidence_until deadline <<<"$begin" case "$step" in keep) return 0 ;; done) FINAL_JOB_STATUS="$final"; return 0 ;; @@ -826,7 +853,8 @@ finalize_unfinished() { ${START_TIME:+--start-time "$START_TIME"} --timeout "$TIMEOUT" --idle-timeout "$IDLE_TIMEOUT" \ --sub-rc "$sub_rc" --phase "$phase" --type "$TYPE" --final "$final" --evidence-until "$evidence_until" \ --dispatch-token "${_disp_tok:-}" --preflight-reason "${PREFLIGHT_REASON:-}" \ - --python "$PY" --scripts-dir "$SCRIPT_DIR/scripts" --watch-pid "$$" 3>&1 1>&4 4>&-)"; } 4>&1 + --python "$PY" --scripts-dir "$SCRIPT_DIR/scripts" --watch-pid "$$" 3>&1 1>&4 4>&-)" || end_rc=$?; } 4>&1 + [ "$end_rc" -eq 0 ] || _decision_failed finalize-end "rc $end_rc" FINAL_JOB_STATUS="$out" } @@ -1039,7 +1067,8 @@ cmd_wait() { fi # Issue #33: either clock is a timeout (rc 2) to callers of `wait`, as before; # the subscriber's own log says which clock it was. - rc="$(_decide wait-rc "$rc")" + _decided rc wait-rc "$rc" + [[ "$rc" =~ ^[0-9]+$ ]] || _decision_failed wait-rc "answer '$rc'" return "$rc" } diff --git a/tests/test_issue17_item4_decisions.py b/tests/test_issue17_item4_decisions.py index 5580f99..a3428c7 100644 --- a/tests/test_issue17_item4_decisions.py +++ b/tests/test_issue17_item4_decisions.py @@ -628,3 +628,78 @@ def test_u_a_wait_is_not_stretched_by_a_wall_clock_step_back(monkeypatch, second monkeypatch.setattr(decide, "time", clock) decide._sleep(argparse.Namespace(watch_pid=None), seconds) assert seconds <= clock.slept <= seconds + 0.5, clock.slept + + +# --------------------------------------------------------------------------- +# F - a decision wrapper.py cannot make ends the wrapper with 1 (N2) +# --------------------------------------------------------------------------- + +def _failing_py(tmp_path: Path, decision: str, mode: str) -> Path: + """The wrapper's $PY: the real interpreter, except that `decision` fails - + with a traceback and rc 1 (a broken module), with rc 127 (an interpreter + that cannot run), or with rc 0 and no answer. + job_subscriber.py is a stand-in exiting $SUB_RC when SUB_STANDIN=1.""" + shim = tmp_path / "py-fail" + fail = {"error": 'echo "ModuleNotFoundError: No module named lib_py.delegation.wrapper" >&2; exit 1', + "gone": 'echo "py: not found" >&2; exit 127', "empty": "exit 0"}[mode] + shim.write_text( + "#!/bin/sh\n" + 'if [ "${SUB_STANDIN:-0}" = 1 ]; then case "$1" in */scripts/job_subscriber.py) exit "${SUB_RC:-0}";; esac; fi\n' + f'case "$*" in *"lib_py.delegation.wrapper {decision} "*|*"lib_py.delegation.wrapper {decision}") {fail};; esac\n' + f'exec "{sys.executable}" "$@"\n') + shim.chmod(0o755) + return shim + + +def _human_submit(tmp_path: Path, py: str) -> subprocess.CompletedProcess: + """The real wrapper, one human job, an unreachable broker: the subscriber + ends with rc 3, the job is closed at its 3 s wall clock and the wrapper + exits through sub-status, finalize-begin/-end and exit-code (rc 3).""" + env = {k: v for k, v in os.environ.items() + if not k.startswith(("HERDR_", "MAM_", "SKS_", "MQTT_")) and k not in ("PYTHONPATH", "WORKSPACE_ROOT")} + env.update(HOME=str(tmp_path), WORKSPACE_ROOT=str(tmp_path), DELEGATE_JOB_PYTHON=py, + AGENT_PYTHON_BIN=sys.executable, PYTHONPATH=str(SKILLS), DELEGATE_JOB_LOGS_DIR=str(tmp_path / "logs"), + MAM_ENV_FILE=str(tmp_path / ".mam.env.absent"), MQTT_BROKER="127.0.0.1", MQTT_PORT="9", MQTT_TLS="0") + return subprocess.run(["bash", str(WRAPPER), "submit", "--agent", "human", "--registry-dir", str(tmp_path / "reg"), + "--workdir", str(tmp_path), "--prompt", "n2", "--timeout", "3", "--idle-timeout", "2"], + capture_output=True, text=True, env=env, cwd=str(tmp_path), timeout=120) + + +def test_f_the_normal_path_is_unchanged(tmp_path): + res = _human_submit(tmp_path, sys.executable) + assert res.returncode == 3, res.stdout[-800:] + res.stderr[-800:] + assert res.stdout.strip().splitlines()[-1] == str(tmp_path / "logs" / next((tmp_path / "reg").glob("*.json")).stem) + assert "could not decide" not in res.stderr + + +@pytest.mark.parametrize("decision,mode", [ + ("exit-code", "error"), ("exit-code", "empty"), + ("sub-status", "error"), ("sub-status", "gone"), ("sub-status", "empty"), + ("finalize-begin", "error"), ("finalize-begin", "empty"), + ("finalize-end", "error"), ("finalize-end", "gone"), # it may answer nothing (its wrapper gone): rc only +]) +def test_f_a_failed_decision_ends_the_submit_with_1(tmp_path, decision, mode): + res = _human_submit(tmp_path, str(_failing_py(tmp_path, decision, mode))) + assert res.returncode == 1, (res.returncode, res.stderr[-800:]) + assert f"ERROR: the wrapper could not decide '{decision}'" in res.stderr, res.stderr[-800:] + + +@pytest.mark.parametrize("mode", ["error", "empty"]) +def test_f_a_failed_wait_rc_ends_wait_with_1(tmp_path, mode): + py = _failing_py(tmp_path, "wait-rc", mode) + script = f'source "{WRAPPER}"; PY="{py}"; set +e; (cmd_wait --job j1 --timeout 5); echo "rc=$?"' + res = subprocess.run(["bash", "-c", script], capture_output=True, text=True, cwd=str(tmp_path), timeout=60, + env=_env(tmp_path, SUB_STANDIN=1, SUB_RC=4, DELEGATE_JOB_PYTHON=py)) + assert "rc=1" in res.stdout, (res.stdout, res.stderr[-800:]) + assert "ERROR: the wrapper could not decide 'wait-rc'" in res.stderr, res.stderr[-800:] + + +def test_s_no_printed_decision_is_read_raw(): + """Every printed answer goes through _decided/_exit_for; the boolean ones + (subscriber-up, stall-watch) are read by their rc, and finalize-end is + checked by its rc.""" + text = "\n".join(l for l in WRAPPER.read_text().splitlines() if not l.lstrip().startswith("#")) + raw = [l.strip() for l in text.splitlines() if "$(_decide" in l] + assert raw == ['__out="$(_decide "$@")" || __rc=$?', + '{ out="$(_decide finalize-end --job "$job_id" --registry-dir "$REGISTRY_DIR" --status "$job_status" \\'], raw + assert 'exit "$(' not in text