From 34241d7a65da05f6e55c9fdaaeff06fa87065b8f Mon Sep 17 00:00:00 2001 From: Godopu Date: Wed, 30 Sep 2026 17:39:08 +0000 Subject: [PATCH] fix(issue-26): review round 1 - no file key assigns bash's own variables; SKS_QUIESCENT_TRIES; long decimals R1 (P1): a quoted export still ran code when the key was a bash special variable (RANDOM=a[$(cmd)] is evaluated arithmetically on assignment). envfile.RESERVED (bash's own variables, plus BASH_/COMP_/READLINE_ prefixes) is skipped with a line-numbered warning by both loaders; a test keeps it covering every variable a clean bash defines. The wrapper also hands SCRIPT_DIR/REPO_ROOT/TARGET_ENV to the parser as environment, so the file cannot replace the variables it has already set. R2 (P2): SKS_QUIESCENT_TRIES reached `for ((i < tries))` through a function argument; it is normalised at the call site (minimum 0). B5 now requires every integer-looking env default in the shell code to be normalised or listed in NOT_ARITHMETIC with where it goes, and exercises _pane_quiescent. R3 (P2): _positive_int strips leading zeros and rejects more than nine significant digits before int(), like mam_int; 5000-digit inputs no longer raise in the runner. Co-Authored-By: Claude Opus 5.5 --- .agents/skills/lib.sh | 2 +- .agents/skills/lib_py/delegation/envfile.py | 31 ++++++- .agents/skills/lib_py/loop/runner.py | 5 +- .../multi-agent-mux-delegate-job | 7 +- tests/test_issue26_envfile.py | 66 ++++++++++++++ tests/test_issue26_ints.py | 91 +++++++++++++++++-- 6 files changed, 186 insertions(+), 16 deletions(-) diff --git a/.agents/skills/lib.sh b/.agents/skills/lib.sh index ef5db06..dadb3d2 100644 --- a/.agents/skills/lib.sh +++ b/.agents/skills/lib.sh @@ -2782,7 +2782,7 @@ send_keys_safe() { fi local _q_rc=0 - _pane_quiescent "$sess" "${SKS_QUIESCENT_TRIES:-20}" "${SKS_QUIESCENT_INTERVAL:-0.5}" || _q_rc=$? + _pane_quiescent "$sess" "$(mam_int_knob SKS_QUIESCENT_TRIES 20 0)" "${SKS_QUIESCENT_INTERVAL:-0.5}" || _q_rc=$? if [ "$_q_rc" = "1" ]; then echo "send_keys_safe: pane never quiesced ($sess)" >&2 return 1 diff --git a/.agents/skills/lib_py/delegation/envfile.py b/.agents/skills/lib_py/delegation/envfile.py index 6fcce29..696a67f 100644 --- a/.agents/skills/lib_py/delegation/envfile.py +++ b/.agents/skills/lib_py/delegation/envfile.py @@ -3,9 +3,10 @@ Rules: blank lines and `#` comments are skipped; `KEY=VALUE` with the key and the value stripped; one matching pair of surrounding quotes is removed; no shell interpretation (`$(...)`, `${...}` and backticks stay literal). A key -must match ``^[A-Za-z_][A-Za-z0-9_]*$``; a line that does not, or that has no -`=`, is skipped with a warning naming the line (never the value — the file -holds secrets). Variables already in the environment win over the file. +must match ``^[A-Za-z_][A-Za-z0-9_]*$`` and not be one of bash's own variables +(RESERVED); a line that breaks either, or that has no `=`, is skipped with a +warning naming the line (never the value — the file holds secrets). +Variables already in the environment win over the file. Stdlib only: the delegate-job wrapper runs this file by path (``envfile.py --shell ``) and evals the ``export`` lines it prints, so @@ -19,6 +20,27 @@ from typing import List, MutableMapping, Tuple _KEY = re.compile(r"[A-Za-z_][A-Za-z0-9_]*") +# Bash's own variables (bash(1) "Shell Variables"). Assigning some runs code +# even from a quoted value (RANDOM/SECONDS/... are evaluated arithmetically, +# so RANDOM='a[$(cmd)]' executes cmd), others steer the shell (IFS, PS4, +# BASH_ENV, SHELLOPTS) or are readonly. A file key never assigns them - in +# either loader. tests/test_issue26_envfile.py checks this covers every +# variable a clean bash defines. +RESERVED = frozenset(""" + BASH BASHOPTS BASHPID CDPATH CHILD_MAX COLUMNS COMPREPLY COPROC DIRSTACK EMACS ENV + EPOCHREALTIME EPOCHSECONDS EUID EXECIGNORE FCEDIT FIGNORE FUNCNAME FUNCNEST GLOBIGNORE + GLOBSORT GROUPS HISTCMD HISTCONTROL HISTFILE HISTFILESIZE HISTIGNORE HISTSIZE + HISTTIMEFORMAT HOSTFILE HOSTNAME HOSTTYPE IFS IGNOREEOF INPUTRC INSIDE_EMACS LINENO LINES + MACHTYPE MAIL MAILCHECK MAILPATH MAPFILE OLDPWD OPTARG OPTERR OPTIND OSTYPE PIPESTATUS + POSIXLY_CORRECT PPID PROMPT_COMMAND PROMPT_DIRTRIM PS0 PS1 PS2 PS3 PS4 PWD RANDOM REPLY + SECONDS SHELL SHELLOPTS SHLVL SRANDOM TIMEFORMAT TMOUT UID auto_resume histchars _ +""".split()) +RESERVED_PREFIXES = ("BASH_", "COMP_", "READLINE_") + + +def is_reserved(key: str) -> bool: + return key in RESERVED or key.startswith(RESERVED_PREFIXES) + def parse(path: str) -> Tuple[List[Tuple[str, str]], List[str]]: """Return ([(key, value)], [warning]) for the file at ``path``.""" @@ -41,6 +63,9 @@ def parse(path: str) -> Tuple[List[Tuple[str, str]], List[str]]: if not _KEY.fullmatch(key): warnings.append(f"{path}:{n}: skipped (invalid key)") continue + if is_reserved(key): + warnings.append(f"{path}:{n}: skipped (reserved shell variable {key})") + continue val = val.strip() if len(val) >= 2 and val[0] == val[-1] and val[0] in "\"'": val = val[1:-1] diff --git a/.agents/skills/lib_py/loop/runner.py b/.agents/skills/lib_py/loop/runner.py index 679946d..f573c02 100644 --- a/.agents/skills/lib_py/loop/runner.py +++ b/.agents/skills/lib_py/loop/runner.py @@ -74,8 +74,11 @@ def _positive_int(text: Optional[str], minimum: int = 1) -> Optional[int]: s = (text or "").strip(" \t\n\r\v\f") if not s or not s.isascii() or not s.isdigit(): return None + s = s.lstrip("0") or "0" + if len(s) > 9: # like mam_int: bounded before int(), which refuses > 4300 digits + return None n = int(s) - return n if minimum <= n <= 999_999_999 else None + return n if n >= minimum else None class LoopRunner: """Orchestrates planning, code implementation, and multi-agent peer reviews.""" 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 290187f..afa62a5 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 @@ -35,9 +35,12 @@ if [[ -z "$TARGET_ENV" ]]; then fi # Issue #26: parsed by envfile.py (the Python loader's rules; environment wins), -# never sourced. Values come back as shlex-quoted exports over the pipe. +# never sourced. Values come back as shlex-quoted exports over the pipe. The +# variables this script has already set are handed over as environment, so +# the file cannot replace them either. if [[ -n "$TARGET_ENV" && -f "$TARGET_ENV" ]]; then - _env_exports="$("${DELEGATE_JOB_PYTHON:-python3}" -I "$SCRIPT_DIR/../lib_py/delegation/envfile.py" --shell "$TARGET_ENV")" \ + _env_exports="$(SCRIPT_DIR="$SCRIPT_DIR" REPO_ROOT="$REPO_ROOT" TARGET_ENV="$TARGET_ENV" \ + "${DELEGATE_JOB_PYTHON:-python3}" -I "$SCRIPT_DIR/../lib_py/delegation/envfile.py" --shell "$TARGET_ENV")" \ || { echo "ERROR: cannot read $TARGET_ENV" >&2; exit 1; } eval "$_env_exports"; unset _env_exports fi diff --git a/tests/test_issue26_envfile.py b/tests/test_issue26_envfile.py index d2bb1e2..3965850 100644 --- a/tests/test_issue26_envfile.py +++ b/tests/test_issue26_envfile.py @@ -200,6 +200,8 @@ def test_a4_the_parser_command_line_carries_the_path_only(): calls = [l for l in WRAPPER.read_text().splitlines() if "envfile.py" in l and not l.lstrip().startswith("#")] assert len(calls) == 1, calls assert re.search(r'envfile\.py" --shell "\$TARGET_ENV"\)"', calls[0]), calls[0] + prefix = [l for l in WRAPPER.read_text().splitlines() if "_env_exports=" in l] + assert prefix == [' _env_exports="$(SCRIPT_DIR="$SCRIPT_DIR" REPO_ROOT="$REPO_ROOT" TARGET_ENV="$TARGET_ENV" \\'], prefix src = WRAPPER.read_text() assert not re.search(r'^\s*(source|\.)\s+"?\$TARGET_ENV', src, re.M), "the env file must never be sourced" assert "set -a" not in src @@ -251,3 +253,67 @@ def test_a5_shell_mode_exit_code_is_zero_for_absent_and_present_files(tmp_path, env={k: v for k, v in os.environ.items() if k != "A"}, timeout=30) assert res.returncode == 0 assert res.stdout == ("" if missing else "export A=1\n") + + +# --------------------------------------------------------------------------- +# A6 - bash's own variables are never assigned from the file (review R1) +# --------------------------------------------------------------------------- + +# RANDOM/SECONDS/LINENO/OPTIND/SRANDOM are evaluated arithmetically on +# assignment, so even a shlex-quoted value runs its $(...) in the wrapper. +ARITH_SPECIALS = ["RANDOM", "SECONDS", "LINENO", "OPTIND", "SRANDOM"] + + +@pytest.mark.parametrize("key", ARITH_SPECIALS) +def test_a6_a_reserved_key_runs_nothing_in_the_real_wrapper(tmp_path, key): + f = tmp_path / "special.env" + f.write_text(f"{key}=BASH_VERSINFO[$(touch marker)0]\nSAFE=BASH_VERSINFO[$(touch marker2)0]\n") + env = _env(tmp_path, f) + env.pop(key, None) + res = subprocess.run(["bash", str(WRAPPER), "--help"], capture_output=True, text=True, env=env, + cwd=str(tmp_path), timeout=60) + assert res.returncode == 0, res.stderr + assert not (tmp_path / "marker").exists(), f"{key}= from the file ran shell" + assert not (tmp_path / "marker2").exists() + assert f"{f}:1: skipped (reserved shell variable {key})" in res.stderr, res.stderr + assert "touch" not in res.stderr, "the warning must not echo the value" + # the same value under an ordinary key stays literal (positive case) + got = _wrapper_values(env, ["SAFE"], tmp_path) + assert _split(got.stdout) == {"SAFE": "BASH_VERSINFO[$(touch marker2)0]"} + assert not (tmp_path / "marker").exists() and not (tmp_path / "marker2").exists() + + +def test_a6_both_loaders_skip_reserved_keys(tmp_path): + f = tmp_path / "reserved.env" + keys = ["BASH_ENV", "PS4", "IFS", "SHELLOPTS", "COMP_WORDBREAKS", "READLINE_LINE", "RANDOM"] + f.write_text("".join(f"{k}=x\n" for k in keys) + "PLAIN=ok\n") + env = {k: v for k, v in _env(tmp_path, f).items() if k not in keys} + python = _python_values(env, keys + ["PLAIN"]) + assert python == {**{k: "" for k in keys}, "PLAIN": "ok"} + pairs, warnings = envfile.parse(str(f)) + assert pairs == [("PLAIN", "ok")] + assert [w.rsplit("(", 1)[1] for w in warnings] == [f"reserved shell variable {k})" for k in keys] + res = subprocess.run([sys.executable, "-I", str(ENVFILE), "--shell", str(f)], capture_output=True, + text=True, env=env, timeout=30) + assert res.stdout == "export PLAIN=ok\n" + + +def test_a6_the_reserved_set_covers_every_variable_a_clean_bash_defines(): + res = subprocess.run(["env", "-i", "bash", "--norc", "--noprofile", "-c", "compgen -v"], + capture_output=True, text=True, check=True, timeout=30) + ordinary = {"PATH", "TERM"} # environment, not shell state; .mam.env may set them as before + uncovered = [v for v in res.stdout.split() if v not in ordinary and not envfile.is_reserved(v)] + assert not uncovered, f"bash defines these but envfile.RESERVED misses them: {uncovered}" + + +def test_a6_the_file_cannot_replace_the_wrappers_own_variables(tmp_path): + f = tmp_path / "own.env" + f.write_text(f"SCRIPT_DIR={tmp_path}/evil\nREPO_ROOT={tmp_path}/evil\nTARGET_ENV={tmp_path}/evil\n") + (tmp_path / "evil").mkdir() + (tmp_path / "evil" / "lib.sh").write_text("touch pwned-lib\n") + res = subprocess.run(["bash", str(WRAPPER), "--help"], capture_output=True, text=True, + env=_env(tmp_path, f), cwd=str(tmp_path), timeout=60) + assert res.returncode == 0, res.stderr + assert not (tmp_path / "pwned-lib").exists() + got = _wrapper_values(_env(tmp_path, f), ["SCRIPT_DIR", "TARGET_ENV"], tmp_path) + assert _split(got.stdout) == {"SCRIPT_DIR": str(WRAPPER.parent), "TARGET_ENV": str(f)} diff --git a/tests/test_issue26_ints.py b/tests/test_issue26_ints.py index d41c5f3..3c40fb4 100644 --- a/tests/test_issue26_ints.py +++ b/tests/test_issue26_ints.py @@ -41,6 +41,8 @@ INT_KNOBS = { "SKS_DIALOG_TIMEOUT": (0, "30"), "SKS_BUSY_TIMEOUT": (0, "120"), "SKS_DELIVERY_GRACE_S": (0, "15"), + # _pane_quiescent's `for ((i = 0; i < tries; i++))` (review R2); 0 keeps "headless" (rc 2) + "SKS_QUIESCENT_TRIES": (0, "20"), # delegate_wait_started's ack_s (MAM_ONBOARD_ACK_S / DELEGATE_JOB_ACK_S via create_session.sh) "delegate_wait_started:ack_s": (0, "180"), # wrapper flags @@ -51,15 +53,51 @@ INT_KNOBS = { "--ack-timeout": (1, None), } -INPUTS = ["30", "0700", "0900", " 30 ", "0", "-5", "abc", "", "1e3", "9999999999", "000000000030"] +# The last two are longer than Python's int() digit limit (4300): the rule must +# bound the significant digits before converting (review R3). +INPUTS = ["30", "0700", "0900", " 30 ", "0", "-5", "abc", "", "1e3", "9999999999", "000000000030", + "9" * 5000, "0" * 5000 + "30"] + +# Integer-looking env defaults in the shell scripts that do NOT reach bash +# arithmetic, each with where the value goes. B5 fails on any `${MAM_/SKS_/ +# DELEGATE_JOB_/RECONCILE_…:-…}` that is neither here nor normalised +# through INT_KNOBS, so a new knob has to be classified to land. +NOT_ARITHMETIC = { + "MAM_DELEGATE_FAIL_CLOSED": "string compare", "MAM_STALL_WATCH": "string compare", + "MAM_ALLOW_UNSIGNED_DISPATCH": "string compare", "MAM_DELEGATE_JOB_NO_MAIN": "string compare", + "MAM_NO_AUTOSTART": "string compare", "MAM_SHIM_DEBUG": "string compare", + "MAM_NO_SUBSCRIBER_DAEMON": "string compare", "MAM_DELEGATION_PY": "string compare", + "MAM_AUTO_TRUST": "string compare", "MAM_KILL_ON_READY_TIMEOUT": "string compare", + "MAM_LOOP_PY": "string compare", "MAM_LOOP_FREEZE": "string compare", "MAM_LOOP_NO_FREEZE": "string compare", + "MAM_LOOP_FREEZE_OWNED": "string compare", "MAM_AUTODETECT_FORCE_FAIL": "string compare", + "MAM_WS_LABEL_EXPLICIT": "passed to python", "SKS_IDLE_HANDSHAKE": "string compare", + "SKS_DIALOG_ESCAPE": "string compare", "SKS_PROMPT_WAIT": "string compare", + "MAM_MIN_PANE_COLS": "argv of python -m lib_py.layout", "MAM_MIN_PANE_ROWS": "argv of python -m lib_py.layout", + "MAM_MAX_PANE_COLS": "argv of python -m lib_py.layout", "MAM_MAX_PANE_ROWS": "argv of python -m lib_py.layout", + "MAM_BUFFER_GC_MINUTES": "find -mmin", "MAM_DIALOG_TAIL_LINES": "tail -n", + "SKS_IDLE_TIMEOUT_MS": "argv of the python idle watcher", "MAM_DIALOG_RESEND_S": "python int()", + "MAM_READY_EMPTY_GIVEUP": "[ -ge ] (decimal)", "SKS_EMPTY_GIVEUP": "[ -ge ] (decimal)", + "SKS_IDLE_RETRIES": "[ -le ] (decimal)", "SKS_IDLE_RETRY_BACKOFF_S": "sleep", + "SKS_QUIESCENT_INTERVAL": "sleep", "MAM_LOCK_STEAL_RETRIES": "[ -le ] (decimal)", + "MAM_DIFF_MAX_BYTES": "[ -gt ] (decimal)", "MAM_DIFF_MAX_LINES": "[ -gt ] (decimal)", + "RECONCILE_POLL_INTERVAL": "sleep", + "DELEGATE_JOB_ACK_S": "create_session.sh -> delegate_wait_started, which runs mam_int on it", +} def _expected(value: str, minimum: int): s = value.strip() if not s.isdigit(): return None + s = s.lstrip("0") or "0" + if len(s) > 9: + return None n = int(s) - return n if minimum <= n <= 999_999_999 else None + return n if n >= minimum else None + + +def _id(value: str) -> str: + return repr(value) if len(value) <= 12 else f"{value[:3]}...x{len(value)}" def _base_env(home: Path, extra=None) -> dict: @@ -103,7 +141,7 @@ def _bash(snippet: str, home: Path, extra_env=None) -> subprocess.CompletedProce # B1 - one table, the real wrapper and the real runner agree # --------------------------------------------------------------------------- -@pytest.mark.parametrize("value", INPUTS) +@pytest.mark.parametrize("value", INPUTS, ids=_id) def test_b1_wrapper_and_runner_give_the_same_budget(tmp_path, value): minimum, default = INT_KNOBS["MAM_JOB_TIMEOUT_S"] want = _expected(value, minimum) @@ -121,7 +159,7 @@ def test_b1_wrapper_and_runner_give_the_same_budget(tmp_path, value): assert f"arith={budget}" in ares.stdout, (ares.stdout, ares.stderr) rres = _runner(tmp_path, {"MAM_JOB_TIMEOUT_S": value}) - assert rres.returncode == 0, rres.stderr + assert rres.returncode == 0 and "Traceback" not in rres.stderr, rres.stderr[-500:] got = json.loads(rres.stdout.strip().splitlines()[-1]) assert got["budget"] == budget assert got["contract"][:2] == ["--timeout", str(budget)] @@ -166,10 +204,10 @@ def test_b2_wrapper_flags_are_decimal_or_refused(tmp_path): ok = _dry_submit(tmp_path, None, "--timeout", "30") assert _record(tmp_path, ok)["timeout_sec"] == 30 for flag in ("--timeout", "--idle-timeout", "--heartbeat", "--heartbeat-misses", "--ack-timeout"): - for bad in ("abc", "0", "-5", "9999999999"): + for bad in ("abc", "0", "-5", "9999999999", "9" * 5000): res = _dry_submit(tmp_path, None, flag, bad) - assert res.returncode == 1, (flag, bad, res.stdout, res.stderr) - assert f"ERROR: {flag} {bad}: not a positive decimal integer" in res.stderr, res.stderr + assert res.returncode == 1, (flag, bad[:20], res.stdout, res.stderr[-500:]) + assert f"ERROR: {flag} {bad}: not a positive decimal integer" in res.stderr, res.stderr[-500:] assert "registered job" not in res.stdout @@ -177,10 +215,14 @@ def test_b2_runner_job_timeout_is_decimal_or_refused(tmp_path): res = _runner(tmp_path, None, "--job-timeout", "0700") assert res.returncode == 0, res.stderr assert json.loads(res.stdout.strip().splitlines()[-1])["budget"] == 700 - for bad in ("0", "abc", "-5", "9999999999"): + for bad in ("0", "abc", "-5", "9999999999", "9" * 5000): res = _runner(tmp_path, None, "--job-timeout", bad) - assert res.returncode == 1, (bad, res.stderr) + assert res.returncode == 1, (bad[:20], res.stderr[-500:]) assert "--job-timeout requires a positive decimal integer" in res.stderr + assert "Traceback" not in res.stderr, res.stderr[-500:] + res = _runner(tmp_path, None, "--job-timeout", "0" * 5000 + "30") + assert res.returncode == 0 and "Traceback" not in res.stderr, res.stderr[-500:] + assert json.loads(res.stdout.strip().splitlines()[-1])["budget"] == 30 def test_b2_runner_passes_normalised_contract_flags(tmp_path): @@ -293,3 +335,34 @@ def test_b5_every_normalised_knob_uses_the_table_minimum(): flags = {f.split(":")[0] for f in loop.group(1).split()} assert {f"--{f}" for f in flags} == {k for k in INT_KNOBS if k.startswith("--")} assert 'mam_int "${!_var}" 1' in loop.group(2) + + +def test_b5_every_integer_env_default_is_classified(): + """Review R2: a knob can reach arithmetic through a function argument, which + the direct-pattern guard above cannot see. Every integer-looking env default + in the shell code is therefore either normalised (INT_KNOBS) or listed in + NOT_ARITHMETIC with where it goes.""" + files = [WRAPPER, *sorted(SKILLS.glob("**/*.sh"))] + found = {} + for f in files: + for m in re.finditer(r"\$\{((?:MAM|SKS|DELEGATE_JOB|RECONCILE)_[A-Z0-9_]*):-[0-9]", f.read_text()): + found.setdefault(m.group(1), f.relative_to(REPO_ROOT)) + raw_normalised = {k: v for k, v in found.items() if k in INT_KNOBS} + assert not raw_normalised, f"normalised knobs read raw (use mam_int_knob): {raw_normalised}" + unclassified = {k: str(v) for k, v in found.items() if k not in NOT_ARITHMETIC} + assert not unclassified, f"classify in INT_KNOBS or NOT_ARITHMETIC: {unclassified}" + stale = set(NOT_ARITHMETIC) - set(found) + assert not stale, f"no longer in the code: {stale}" + + +@pytest.mark.parametrize("value,rc", [("900", 0), ("0900", 0), ("0", 2)]) +def test_b5_pane_quiescent_reads_the_tries_knob_as_decimal(tmp_path, value, rc): + """The consuming function with the call site's normalisation (review R2): + 0900 used to raise 'value too great for base' and return 2 (headless).""" + line = next(l for l in LIB_SH.read_text().splitlines() if l.strip().startswith('_pane_quiescent "$sess"')) + assert '"$(mam_int_knob SKS_QUIESCENT_TRIES 20 0)"' in line + res = _bash('_pane_capture() { echo stable; }; ' + '_pane_quiescent s "$(mam_int_knob SKS_QUIESCENT_TRIES 20 0)" 0; echo "rc=$?"', + tmp_path, {"SKS_QUIESCENT_TRIES": value}) + assert f"rc={rc}" in res.stdout, (res.stdout, res.stderr) + assert "value too great" not in res.stderr and "ignored" not in res.stderr