From 7df9a6781b5bf5bd4baeb6aac3ec87b69601bd3c Mon Sep 17 00:00:00 2001 From: Guillermo Montero Date: Thu, 17 Sep 2026 16:35:35 +0200 Subject: [PATCH] fix(hooks): validate event_name against the canonical event set --- CHANGELOG.md | 2 ++ src/bmad_loop/data/bmad_loop_hook.py | 19 ++++++++++++++++++ src/bmad_loop/events.py | 20 +++++++++++++++++++ tests/test_events.py | 30 ++++++++++++++++++++++++++-- tests/test_hook_script.py | 20 +++++++++++++++++++ 5 files changed, 89 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5259aee07..39c13067e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ breaking changes may land in a minor release. ### Added +- The hook script now warns (non-blocking) if it receives an event name outside the canonical set, without ever dropping the signal. + - Prove real-tmux teardown reaps the exact detached child after identity publication fails (DW-149). - **The accepted-park arm's `_harvest_gate_exclude` join is now graded engine-side** diff --git a/src/bmad_loop/data/bmad_loop_hook.py b/src/bmad_loop/data/bmad_loop_hook.py index 7b22db521..8f6f463d4 100644 --- a/src/bmad_loop/data/bmad_loop_hook.py +++ b/src/bmad_loop/data/bmad_loop_hook.py @@ -35,6 +35,15 @@ import sys import time +# The whitelist adapters/profile.py already enforces at hook-registration time +# (SessionStart/Stop/SessionEnd/PreCompact), duplicated here because this file +# is stdlib-only and cannot import bmad_loop.adapters.profile -- same twin +# constraint as events.py below. Not a gate: an event_name outside this set +# still gets written exactly as before, only with an added drift warning, +# since a hand-edited hook config or a profile that skipped that validation is +# the only way one reaches this script at all. +CANONICAL_EVENTS = {"SessionStart", "Stop", "SessionEnd", "PreCompact"} + # Windows reparse tags that make a directory entry REDIRECT somewhere else, # compared against os.lstat().st_reparse_tag (Windows, 3.8+). Deliberately not # os.path.isjunction(), which is 3.12+ — this relay runs under whatever @@ -184,6 +193,16 @@ def main() -> int: if not run_dir or not task_id: return 0 event_name = sys.argv[1] if len(sys.argv) > 1 else "Unknown" + if event_name not in CANONICAL_EVENTS: + # Drift, not a gate: profile.py already enforces this whitelist before a + # hook is ever registered, so reaching here means a hand-edited hook + # config or a profile that skipped that validation. The event is still + # written exactly as below -- never withhold the signal the orchestrator + # waits on -- same never-fail treatment as the hooks.relay-stale check. + print( + f"bmad_loop_hook: event_name {event_name!r} is outside the canonical set {sorted(CANONICAL_EVENTS)}", + file=sys.stderr, + ) try: payload = json.load(sys.stdin) except (json.JSONDecodeError, ValueError): diff --git a/src/bmad_loop/events.py b/src/bmad_loop/events.py index 273439f9f..bb2fe5834 100644 --- a/src/bmad_loop/events.py +++ b/src/bmad_loop/events.py @@ -29,9 +29,19 @@ import json import os import stat +import sys import time from typing import IO, Any +# The whitelist adapters/profile.py already enforces at hook-registration time +# (SessionStart/Stop/SessionEnd/PreCompact), duplicated here because this file +# is stdlib-only and cannot import bmad_loop.adapters.profile -- same twin +# constraint as the copied hook script above. Not a gate: an event_name outside +# this set still gets written exactly as before, only with an added drift +# warning, since a hand-edited hook config or a profile that skipped that +# validation is the only way one reaches this relay at all. +CANONICAL_EVENTS = {"SessionStart", "Stop", "SessionEnd", "PreCompact"} + # Windows reparse tags that make a directory entry REDIRECT somewhere else, # compared against os.lstat().st_reparse_tag (Windows, 3.8+). Deliberately not # os.path.isjunction(), which is 3.12+ — this relay runs under whatever @@ -246,6 +256,16 @@ def relay(event_name: str, stdin: IO[str]) -> int: task_id = os.environ.get("BMAD_LOOP_TASK_ID") if not run_dir or not task_id: return 0 + if event_name not in CANONICAL_EVENTS: + # Drift, not a gate: profile.py already enforces this whitelist before a + # hook is ever registered, so reaching here means a hand-edited hook + # config or a profile that skipped that validation. The event is still + # written exactly as below -- never withhold the signal the orchestrator + # waits on -- same never-fail treatment as the hooks.relay-stale check. + print( + f"bmad_loop relay: event_name {event_name!r} is outside the canonical set {sorted(CANONICAL_EVENTS)}", + file=sys.stderr, + ) ts = time.time_ns() event = shape_event(ts, event_name, task_id, _read_payload(stdin)) # $BMAD_LOOP_EVENTS_DIR when the orchestrator names one (#494 moved the diff --git a/tests/test_events.py b/tests/test_events.py index 83fee6d15..25ce43c23 100644 --- a/tests/test_events.py +++ b/tests/test_events.py @@ -441,6 +441,26 @@ def test_relay_writes_the_event_and_says_nothing(tmp_path, monkeypatch, capsys): assert not list((tmp_path / "events").glob("*.tmp")) +def test_relay_warns_but_still_writes_for_a_noncanonical_event_name(tmp_path, monkeypatch, capsys): + """The relay's own half of the drift-detection warning: profile.py already + whitelists event names before a hook is ever registered, so reaching this + relay with something else means a hand-edited hook config or a profile + that skipped that validation -- defense in depth, not a gate. The warning + goes to stderr, never stdout (the hosts parse hook stdout), and the event + is written exactly as any other. + + Ablation guard: dropping the `event_name not in CANONICAL_EVENTS` check in + `events.relay` makes the stderr assertion fail while the rest stays green.""" + assert _relay("Weird", {"session_id": "s1"}, monkeypatch, tmp_path) == 0 + out, err = capsys.readouterr() + assert out == "" + assert "Weird" in err and "canonical" in err + + files = list((tmp_path / "events").glob("*.json")) + assert len(files) == 1 + assert json.loads(files[0].read_text())["event"] == "Weird" + + def test_relay_is_a_silent_noop_outside_a_driven_session(tmp_path, monkeypatch, capsys): """An operator (or a stray hook config) can invoke `bmad-loop relay` in a session bmad-loop never spawned. The session-protocol env is the detector, and @@ -598,9 +618,15 @@ def test_relay_defaults_the_event_name_like_the_hook(tmp_path, monkeypatch, caps """The hook script reads `sys.argv[1] if len(sys.argv) > 1 else "Unknown"`, so a misconfigured registration that forgets the event name still produces a file the operator can see. argparse would otherwise turn that into a usage error at - rc 2, before any handler runs — nothing `cmd_relay` does could take it back.""" + rc 2, before any handler runs — nothing `cmd_relay` does could take it back. + + "Unknown" is itself outside CANONICAL_EVENTS, so this is exactly the drift the + canonical-set warning exists to flag — the file still lands, but now with a + stderr warning an operator can act on instead of a silent "Unknown" file.""" assert _relay_no_event({"session_id": "s1"}, monkeypatch, tmp_path) == 0 - assert capsys.readouterr() == ("", "") + out, err = capsys.readouterr() + assert out == "" + assert "Unknown" in err and "canonical" in err assert "Unknown" in next((tmp_path / "events").glob("*.json")).name diff --git a/tests/test_hook_script.py b/tests/test_hook_script.py index 3c81351a3..83b48c096 100644 --- a/tests/test_hook_script.py +++ b/tests/test_hook_script.py @@ -113,6 +113,26 @@ def test_camelcase_payload(tmp_path): assert event["transcript_path"].endswith("events.jsonl") +def test_event_name_outside_canonical_set_warns_but_still_writes(tmp_path): + """profile.py already whitelists event names at hook-registration time + (SessionStart/Stop/SessionEnd/PreCompact), so reaching here with something + else means a hand-edited hook config or a profile that skipped that + validation -- defense in depth, not a gate. The event must still be + written exactly as any other, with a warning on stderr flagging the drift. + + Ablation guard: dropping the `event_name not in CANONICAL_EVENTS` check + makes the stderr assertion fail while the rest of this test stays green.""" + env = {"BMAD_LOOP_RUN_DIR": str(tmp_path), "BMAD_LOOP_TASK_ID": "t1"} + proc = run_hook("Weird", env, {"session_id": "s1"}) + + assert proc.returncode == 0 + assert "Weird" in proc.stderr and "canonical" in proc.stderr + + files = list((tmp_path / "events").glob("*.json")) + assert len(files) == 1 + assert json.loads(files[0].read_text())["event"] == "Weird" + + def test_tolerates_garbage_stdin(tmp_path): env = {"BMAD_LOOP_RUN_DIR": str(tmp_path), "BMAD_LOOP_TASK_ID": "t1"} proc = run_hook("SessionEnd", env, None) # empty stdin