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 <noreply@anthropic.com>
This commit is contained in:
GodopuandClaude Opus 5.5 committed
1 parent
2826e709ae
commit
34241d7a65
6 files changed
+186
-16
No files matched your search
@@ -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_…:-<digit>…}` 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
|
||||
Reference in new issue
Block a user