diff --git a/docs/ecosystem.md b/docs/ecosystem.md index cd74778c..88087bac 100644 --- a/docs/ecosystem.md +++ b/docs/ecosystem.md @@ -81,6 +81,7 @@ again. | `frustration-watchdog` | hook | | `named-verb-guard` | hook | | `plan-discipline` | hook (not always installed) | +| `prove-it-ship-gate` | hook (Stop; blocks a done/shipped claim about a live surface -- an external service, or the user's own machine, session, or screen -- when the message shows no receipt the run itself emitted) | | `pr-schema-gate` | hook (advisory; PreToolUse on shell tools; checks direct PR text writes with the repo's own `scripts/validate-pr-body.mjs` and reminds about the stack follow-up; never blocks) | | `reflect-on-thrash` | hook (off unless `CATSTACK_REFLECT_ENFORCEMENT=1`) | | `scope-lock` | hook (off unless `CATSTACK_REFLECT_ENFORCEMENT=1`; stops every tool after a second scope correction) | diff --git a/engine/hooks/llm-judge/README.md b/engine/hooks/llm-judge/README.md index 88a6910e..fe93a2e7 100644 --- a/engine/hooks/llm-judge/README.md +++ b/engine/hooks/llm-judge/README.md @@ -90,6 +90,14 @@ A runner fails, and the next one is tried, when its binary is not on `PATH` parses as a JSON object. Each try is recorded in `attempts` with a reason of at most 300 characters, taken from the end of stderr or the error text. +A runner that is not installed or exits non-zero is left out of the table for +the next 6 hours, so a runner this account cannot use (a usage limit, a login +it does not have) stops costing every later verdict a failed try. The marker +lives in `unavailable/` under the state folder, and `judge.log` gets a line +saying which runner was left out and why. A timeout or a reply with no JSON +does not leave a runner out. If every runner is left out, the whole table is +tried anyway, and a runner that answers is put back at once. + `CATSTACK_LLM_JUDGE_RUNNERS` replaces the three runners. It is a JSON list of `[name, argv]` pairs, and any argv item equal to `{prompt}` becomes the prompt. Tests use it to plug in small fake runners. If it is set but not that shape, diff --git a/engine/hooks/llm-judge/judge.py b/engine/hooks/llm-judge/judge.py index 507d2d6d..6475c5c1 100644 --- a/engine/hooks/llm-judge/judge.py +++ b/engine/hooks/llm-judge/judge.py @@ -23,6 +23,8 @@ TIMEOUT_SECONDS = 60 INVESTIGATE_TIMEOUT_CAP = 600 KILL_GRACE_SECONDS = 5 +UNAVAILABLE_SECONDS = 6 * 3600 +NOT_INSTALLED = "not installed" REASON_LIMIT = 300 PROMPT_SLOT = "{prompt}" CHILD_ENV = "CATSTACK_LLM_JUDGE_CHILD" @@ -130,7 +132,7 @@ def bounded_timeout(timeout_seconds: object) -> int | float: def run_runner(name: str, argv: list[str], prompt: str, timeout_seconds: object = TIMEOUT_SECONDS, cwd: object = None) -> tuple[dict, dict | None]: if shutil.which(argv[0]) is None: - return failed(name, "not installed"), None + return failed(name, NOT_INSTALLED), None command = [prompt if item == PROMPT_SLOT else item for item in argv] env = dict(os.environ) env[CHILD_ENV] = "1" @@ -167,15 +169,69 @@ def run_runner(name: str, argv: list[str], prompt: str, timeout_seconds: object return {"runner": name, "ok": True, "reason": "answered"}, answer +def unavailable_path(name: str) -> str: + digest = hashlib.sha1(name.encode("utf-8")).hexdigest()[:16] + return os.path.join(state_root(), "unavailable", f"{digest}.json") + + +def unavailable_until(name: str) -> float: + path = unavailable_path(name) + try: + with open(path, encoding="utf-8") as handle: + data = json.load(handle) + except FileNotFoundError: + return 0.0 + except (OSError, ValueError) as exc: + log(f"runner {name}: unreadable unavailable marker {path}, keeping the runner: {exc}") + return 0.0 + until = data.get("until") if isinstance(data, dict) else None + if isinstance(until, bool) or not isinstance(until, (int, float)): + return 0.0 + return float(until) + + +def shows_unavailable(attempt: dict) -> bool: + reason = attempt.get("reason") or "" + return not attempt.get("ok") and (reason == NOT_INSTALLED or reason.startswith("exit ")) + + +def mark_unavailable(name: str, reason: str) -> None: + try: + write_json_atomic(unavailable_path(name), {"runner": name, "until": time.time() + UNAVAILABLE_SECONDS, "reason": reason}) + log(f"runner {name}: left out of the judge table for {UNAVAILABLE_SECONDS}s after: {reason}") + except OSError as exc: + print(f"catstack-hook-error llm-judge: could not mark runner {name} unavailable: {exc}", file=sys.stderr) + + +def mark_available(name: str) -> None: + path = unavailable_path(name) + if not os.path.exists(path): + return + try: + os.remove(path) + except OSError as exc: + print(f"catstack-hook-error llm-judge: could not clear unavailable marker for {name}: {exc}", file=sys.stderr) + + +def available_runners(mode: object = None) -> list[tuple[str, list[str]]]: + table = runners(mode) + now = time.time() + kept = [(name, argv) for name, argv in table if unavailable_until(name) <= now] + return kept or table + + def ask(prompt: str, mode: object = None, timeout_seconds: object = None, cwd: object = None) -> dict: if timeout_seconds is None: timeout_seconds = TIMEOUT_SECONDS attempts = [] - for name, argv in runners(mode): + for name, argv in available_runners(mode): attempt, answer = run_runner(name, argv, prompt, timeout_seconds=timeout_seconds, cwd=cwd) attempts.append(attempt) if answer is not None: + mark_available(name) return {"outcome": "answered", "runner": name, "answer": answer, "attempts": attempts} + if shows_unavailable(attempt): + mark_unavailable(name, attempt["reason"]) return {"outcome": "unchecked", "runner": None, "answer": None, "attempts": attempts} diff --git a/engine/hooks/llm-judge/tests/test_judge.py b/engine/hooks/llm-judge/tests/test_judge.py index b9dc3c80..47f7b019 100644 --- a/engine/hooks/llm-judge/tests/test_judge.py +++ b/engine/hooks/llm-judge/tests/test_judge.py @@ -127,6 +127,42 @@ def test_long_stderr_reason_is_capped_at_300_characters(self): self.assertLessEqual(len(reason), 300) self.assertTrue(reason.startswith("exit 1: eee")) + def test_runner_that_exits_non_zero_is_left_out_of_the_next_ask(self): + self.use_runners(EXIT_NONZERO, ANSWER_MATCH) + judge.ask("first") + result = judge.ask("second") + self.assertEqual([a["runner"] for a in result["attempts"]], ["answers"]) + self.assertEqual(result["runner"], "answers") + + def test_missing_binary_is_left_out_of_the_next_ask(self): + self.use_runners(MISSING_BINARY, ANSWER_MATCH) + judge.ask("first") + self.assertEqual([a["runner"] for a in judge.ask("second")["attempts"]], ["answers"]) + + def test_left_out_runner_comes_back_after_the_window(self): + self.use_runners(EXIT_NONZERO, ANSWER_MATCH) + judge.ask("first") + with patch.object(judge.time, "time", return_value=time.time() + judge.UNAVAILABLE_SECONDS + 1): + result = judge.ask("later") + self.assertEqual([a["runner"] for a in result["attempts"]], ["crashes", "answers"]) + + def test_timeout_and_prose_do_not_leave_a_runner_out(self): + self.use_runners(runner("hangs", "import time; time.sleep(30)"), PROSE_ONLY, ANSWER_MATCH) + with patch.object(judge, "TIMEOUT_SECONDS", 1): + judge.ask("first") + result = judge.ask("second") + self.assertEqual([a["runner"] for a in result["attempts"]], ["hangs", "rambles", "answers"]) + + def test_when_every_runner_is_left_out_the_whole_table_is_tried_and_an_answer_clears_it(self): + flaky = os.path.join(self.state.name, "flaky-ok") + script = f"import json, os, sys; sys.exit(3) if not os.path.exists({flaky!r}) else print(json.dumps({{'match': True}}))" + self.use_runners(runner("flaky", script)) + self.assertEqual(judge.ask("first")["outcome"], "unchecked") + open(flaky, "w").close() + result = judge.ask("second") + self.assertEqual(result["runner"], "flaky") + self.assertFalse(os.path.exists(judge.unavailable_path("flaky"))) + def test_malformed_runners_env_refuses_instead_of_running_defaults(self): os.environ[judge.RUNNERS_ENV] = "not json" with self.assertRaises(ValueError): diff --git a/engine/hooks/pr-schema-gate/README.md b/engine/hooks/pr-schema-gate/README.md index 730a0c79..f6a233df 100644 --- a/engine/hooks/pr-schema-gate/README.md +++ b/engine/hooks/pr-schema-gate/README.md @@ -1,10 +1,23 @@ # pr-schema-gate -Keeps PR text in the repo's own style without blocking anything. In any repo -that has `scripts/create-pr.mjs`, when a shell tool call writes PR text -directly, the hook checks that text with the repo's own -`scripts/validate-pr-body.mjs` and tells the agent the result. The command -always runs. +Keeps PR text in the repo's own style without blocking anything. When a +shell tool call writes PR text directly, the hook checks that text with the +repo's own validator and tells the agent the result. The command always runs. + +## Which repos are in scope + +The repo is the directory holding `.git` (a directory, or a worktree's +`.git` file), found by walking up from the command's working directory. Its +validator is the first of these that exists: + +1. `scripts/validate-pr-body.mjs` (Invoker) +2. `engine/skills/draft-pr/scripts/validate-pr-body.mjs` (catstack) + +A repo with either is fully in scope. A PR-publishing command (`gh pr create`, +`gh pr edit` with a body, `gh api` on `pulls` with a body, `mergify stack +push`) in a git repo with neither reports UNCHECKED, naming both paths it +looked for. It is never silent. Outside any git repo the hook says nothing. +`scripts/create-pr.mjs` plays no part in scope. ## What counts as a direct PR text write @@ -31,11 +44,18 @@ Three outcomes, never two: |---|---|---| | clean | the validator exits 0 | nothing | | failed | the validator exits 1 | `pr-schema-gate: the PR text in does not follow this repo's PR style ... The command is not blocked.` plus the validator's error lines (up to 20) | -| unchecked | inline or piped text, a missing or unreadable file, no validator, `node` missing, a crash (any other exit code), a timeout (3s), or a command the parser cannot read | `pr-schema-gate: could not check this PR text against the repo's PR style: . The command is not blocked.` | +| unchecked | inline or piped text, a missing or unreadable file, no validator at either path, `node` missing, a crash (any other exit code), a timeout (3s), an exit 0 that states no verdict and prints UNCHECKED/SKIPPED/not-installed, or a command the parser cannot read | `pr-schema-gate: could not check this PR text against the repo's PR style: . The command is not blocked.` | An unchecked write is never reported as clean. The rules live only in the repo's validator, so this hook carries no copy of them to drift. +A run that prints `PR body validation passed.` has judged the body, so it is +clean even when the same run names a sub-check it skipped. The catstack +validator says `Summary reading grade unchecked: ...` for a Summary too short +to grade and still accepts the body; reading that note as a vacuous pass told +the agent an accepted body was unchecked and left an owed stack follow-up +armed. + Claude Code gets the message as `additionalContext` on its `Bash` tool. Every harness also gets it on stderr. Whether Cursor and Codex show a stderr line from an exit-0 `preToolUse` hook to the agent is unverified. @@ -45,6 +65,8 @@ stderr line from an exit-0 `preToolUse` hook to the agent is unverified. `mergify stack push` publishes PRs with a bare body. The push is told which follow-up is owed (`node scripts/create-pr.mjs ... --update-existing`, or a direct body write whose file passes the validator) and arms a pending flag. +In a repo with no validator the push reports UNCHECKED instead and arms +nothing. A later push while the flag is armed repeats the reminder. Either follow-up clears it; an unchecked or failing direct write does not. @@ -68,7 +90,9 @@ The hook never blocks, so every failure fails open, and says so: with no local checkout under `PR_SCHEMA_GATE_CHECKOUTS_ROOT` (default `~/Documents/GitHub`): out of scope; -- a repo with no `scripts/create-pr.mjs`: out of scope. +- a directory outside any git repo: out of scope; +- an unparseable command in a repo with no validator: silent, since it may + not be a PR write at all. There is no escape hatch because there is nothing to escape. diff --git a/engine/hooks/pr-schema-gate/claude_pretooluse.py b/engine/hooks/pr-schema-gate/claude_pretooluse.py index 33bb86b4..1cb0e7cb 100644 --- a/engine/hooks/pr-schema-gate/claude_pretooluse.py +++ b/engine/hooks/pr-schema-gate/claude_pretooluse.py @@ -16,15 +16,18 @@ from detect import ( # noqa: E402 UNPARSEABLE_MESSAGE, + VALIDATOR_RELATIVE_PATHS, check_body_file, classify_pr_text_write, clear_pending, + find_validator, followup_message, is_create_pr_followup, is_stack_push, mark_pending, read_pending, scope_root, + stack_push_unchecked_message, style_message, ) from shell_model import parse_commands, shell_call_from_tool_input # noqa: E402 @@ -66,7 +69,8 @@ def evaluate(payload: dict) -> list[str]: commands = parse_commands(call, session_cwd) if commands is None: base = call.workdir or session_cwd - return [UNPARSEABLE_MESSAGE] if scope_root(base, None) else [] + root = scope_root(base, None) + return [UNPARSEABLE_MESSAGE] if root and find_validator(root) else [] messages: list[str] = [] for command in commands: @@ -83,7 +87,8 @@ def evaluate(payload: dict) -> list[str]: outcome, detail = check_body_file(root, write.body_file, write.cwd) if outcome == "clean": clear_pending(root) - message = style_message(outcome, detail, write.body_file) + message = style_message(outcome, detail, write.body_file, + find_validator(root) or VALIDATOR_RELATIVE_PATHS[0]) if message: messages.append(message) continue @@ -93,7 +98,11 @@ def evaluate(payload: dict) -> list[str]: if is_create_pr_followup(command): clear_pending(root) elif is_stack_push(command): - messages.append(followup_message(read_pending(root) is not None)) + validator = find_validator(root) + if validator is None: + messages.append(stack_push_unchecked_message()) + continue + messages.append(followup_message(read_pending(root) is not None, validator)) mark_pending(root) return messages diff --git a/engine/hooks/pr-schema-gate/detect.py b/engine/hooks/pr-schema-gate/detect.py index c9425f4e..709e855b 100644 --- a/engine/hooks/pr-schema-gate/detect.py +++ b/engine/hooks/pr-schema-gate/detect.py @@ -4,7 +4,7 @@ command the shell will run), not on the raw payload text. A direct write is `gh pr create`, `gh pr edit` with a body flag, or `gh api` on a `pulls` endpoint with a `body=` field. When the text comes from a file, the hook -runs the repo's own `scripts/validate-pr-body.mjs` on that file and hands the +runs the repo's own validator on that file and hands the result to the agent: silent when the text passes, the validator's error lines when it fails, and an explicit "could not check" with the reason when the check cannot run (inline text, a missing or unreadable file, no @@ -16,8 +16,14 @@ lands when a follow-up writes it. The push arms a bounded pending flag and reminds the agent which follow-up is owed; a later push while the flag is armed repeats the reminder. `scripts/create-pr.mjs`, or a direct body write -whose file passes the validator, clears it. A repo with no -scripts/create-pr.mjs is out of scope entirely. +whose file passes the validator, clears it. + +The repo is the one the `.git` boundary marks. Its validator is the first of +VALIDATOR_RELATIVE_PATHS that exists: `scripts/validate-pr-body.mjs` +(Invoker), then `engine/skills/draft-pr/scripts/validate-pr-body.mjs` +(catstack). A PR-publishing command in a repo with neither is reported as +unchecked, never passed over in silence. Outside any git repo the hook says +nothing. PreToolUse fires before the command, so the hook cannot see the push's exit status; pending is recorded when the push is let through. A push that then @@ -47,11 +53,15 @@ from shell_model import Command -VALIDATOR_RELATIVE_PATH = os.path.join("scripts", "validate-pr-body.mjs") +VALIDATOR_RELATIVE_PATHS = ( + "scripts/validate-pr-body.mjs", + "engine/skills/draft-pr/scripts/validate-pr-body.mjs", +) VALIDATOR_TIMEOUT_SECONDS = 3.0 VALIDATOR_OUTPUT_MAX_LINES = 20 VACUOUS_PASS_RE = re.compile( r"\bUNCHECKED\b|\bSKIPPED?\b|\bnot installed\b|\bno rules loaded\b", re.IGNORECASE) +VALIDATOR_PASS_VERDICT_RE = re.compile(r"\bPR body validation passed\b", re.IGNORECASE) PENDING_TTL_SECONDS = 2 * 60 * 60 STATE_DIR_ENV = "PR_SCHEMA_GATE_STATE_DIR" @@ -61,13 +71,13 @@ STYLE_FAILED_MESSAGE = ( "pr-schema-gate: the PR text in {path} does not follow this repo's PR style " - "(scripts/validate-pr-body.mjs exited 1). The command is not blocked. Fix the " + "({validator} exited 1). The command is not blocked. Fix the " "file and write it to the PR again so the live PR matches:\n{details}" ) STYLE_UNCHECKED_MESSAGE = ( "pr-schema-gate: could not check this PR text against the repo's PR style: " "{reason}. The command is not blocked. Check it yourself with " - "`node scripts/validate-pr-body.mjs --body-file `, or write it with " + "`node {validator} --body-file `, or write it with " "`node scripts/create-pr.mjs`, which checks before writing." ) UNPARSEABLE_MESSAGE = ( @@ -78,13 +88,13 @@ "pr-schema-gate: '{cmd}' publishes PRs with a bare body. Follow up on each " "PR with `node scripts/create-pr.mjs --title \"...\" --base " "--body-file --update-existing`, or a direct body write whose file " - "passes scripts/validate-pr-body.mjs. Either one clears this reminder." + "passes {validator}. Either one clears this reminder." ) FOLLOWUP_STILL_OWED_MESSAGE = ( "pr-schema-gate: the follow-up for the last '{cmd}' in this repository has " "not run yet, so those PRs may still have a bare body. The command is not " "blocked. Run `node scripts/create-pr.mjs ... --update-existing`, or a " - "direct body write whose file passes scripts/validate-pr-body.mjs." + "direct body write whose file passes {validator}." ) GH_BODY_FILE_FLAGS = frozenset({"--body-file", "-F"}) @@ -238,34 +248,40 @@ def sibling_repo_dir(repo_spec: str) -> str | None: return candidate if os.path.isdir(candidate) else None -def repo_root_with_create_pr_tool(start_dir: str) -> str | None: - """Walk up from start_dir; return the dir containing scripts/create-pr.mjs, or None. - - Stops at a .git boundary (repo root) or filesystem root, whichever comes first. - """ +def git_root(start_dir: str) -> str | None: + """Walk up from start_dir to the dir holding `.git` (a directory, or a worktree's file), or None.""" cur = os.path.abspath(start_dir) if start_dir else os.getcwd() - for _ in range(12): - if os.path.isfile(os.path.join(cur, "scripts", "create-pr.mjs")): + while True: + if os.path.exists(os.path.join(cur, ".git")): return cur - if os.path.isdir(os.path.join(cur, ".git")): - return None parent = os.path.dirname(cur) if parent == cur: return None cur = parent + + +def find_validator(repo_root: str) -> str | None: + """The first of VALIDATOR_RELATIVE_PATHS present in repo_root, as that relative path, or None.""" + for relative in VALIDATOR_RELATIVE_PATHS: + if os.path.isfile(os.path.join(repo_root, relative)): + return relative return None +def no_validator_reason() -> str: + return "this repo has no PR validator (looked for " + " and ".join(VALIDATOR_RELATIVE_PATHS) + ")" + + def scope_root(cwd: str, repo_spec: str | None) -> str | None: - """The repo this command acts on, when that repo has scripts/create-pr.mjs. + """The git repo this command acts on. A `--repo` naming a repo with no local checkout resolves to None: out of scope, never a guess. """ if repo_spec is not None: sibling = sibling_repo_dir(repo_spec) - return repo_root_with_create_pr_tool(sibling) if sibling else None - return repo_root_with_create_pr_tool(cwd) + return git_root(sibling) if sibling else None + return git_root(cwd) def check_body_file(repo_root: str, body_path: str | None, start_dir: str) -> tuple[str, str]: @@ -274,6 +290,19 @@ def check_body_file(repo_root: str, body_path: str | None, start_dir: str) -> tu Three outcomes: "clean" (validator exit 0), "failed" (exit 1, detail is its error lines) and "unchecked" (the check could not run, detail is why). "unchecked" is never reported as clean. + + Exit 0 is vacuous, and so unchecked, when the run states no verdict on the + body and prints an UNCHECKED/SKIPPED/not-installed line: the validator + never judged the text. Exit 0 that states VALIDATOR_PASS_VERDICT_RE is a + real pass even when the same run names a sub-check it skipped, which the + catstack validator does for a Summary too short to grade. Only a pass + verdict lifts the vacuous reading, so a "failed" banner beside exit 0 + stays unchecked. Reading a skipped sub-check as a vacuous pass told the + agent a body the validator had accepted was unchecked, and left an owed + stack follow-up armed. + + The whole output is scanned, not the first VALIDATOR_OUTPUT_MAX_LINES: + truncation shortens what the agent is shown, never what is judged. """ if body_path is None: return "unchecked", "the PR text is inline or piped, not in a file the hook can read" @@ -285,9 +314,10 @@ def check_body_file(repo_root: str, body_path: str | None, start_dir: str) -> tu fh.read(1) except (OSError, UnicodeDecodeError) as exc: return "unchecked", f"body file unreadable: {path}: {exc}" - validator = os.path.join(repo_root, VALIDATOR_RELATIVE_PATH) - if not os.path.isfile(validator): - return "unchecked", f"this repo has no {VALIDATOR_RELATIVE_PATH}" + relative = find_validator(repo_root) + if relative is None: + return "unchecked", no_validator_reason() + validator = os.path.join(repo_root, relative) try: proc = subprocess.run( ["node", validator, "--body-file", path], @@ -300,23 +330,25 @@ def check_body_file(repo_root: str, body_path: str | None, start_dir: str) -> tu return "unchecked", "node is not on PATH, so the validator could not run" except subprocess.TimeoutExpired: return "unchecked", f"the validator timed out after {VALIDATOR_TIMEOUT_SECONDS:g}s" - lines = [line for line in (proc.stdout + "\n" + proc.stderr).splitlines() if line.strip()] - lines = lines[:VALIDATOR_OUTPUT_MAX_LINES] + output = [line for line in (proc.stdout + "\n" + proc.stderr).splitlines() if line.strip()] + lines = output[:VALIDATOR_OUTPUT_MAX_LINES] if proc.returncode == 0: - for line in lines: - if VACUOUS_PASS_RE.search(line): - return "unchecked", f"the validator exited 0 without checking: {line.strip()}" + if not any(VALIDATOR_PASS_VERDICT_RE.search(line) for line in output): + for line in output: + if VACUOUS_PASS_RE.search(line): + return "unchecked", f"the validator exited 0 without checking: {line.strip()}" return "clean", "" if proc.returncode == 1: return "failed", "\n".join(lines) return "unchecked", f"the validator crashed (exit {proc.returncode}): " + " | ".join(lines[:3]) -def style_message(outcome: str, detail: str, body_path: str | None) -> str | None: +def style_message(outcome: str, detail: str, body_path: str | None, + validator: str = VALIDATOR_RELATIVE_PATHS[0]) -> str | None: if outcome == "failed": - return STYLE_FAILED_MESSAGE.format(path=body_path, details=detail) + return STYLE_FAILED_MESSAGE.format(path=body_path, details=detail, validator=validator) if outcome == "unchecked": - return STYLE_UNCHECKED_MESSAGE.format(reason=detail) + return STYLE_UNCHECKED_MESSAGE.format(reason=detail, validator=validator) return None @@ -385,6 +417,13 @@ def clear_pending(repo_root: str) -> None: sys.stderr.write(f"pr-schema-gate: could not clear pending state at {path}: {exc}\n") -def followup_message(already_owed: bool) -> str: +def followup_message(already_owed: bool, validator: str = VALIDATOR_RELATIVE_PATHS[0]) -> str: template = FOLLOWUP_STILL_OWED_MESSAGE if already_owed else FOLLOWUP_OWED_MESSAGE - return template.format(cmd=STACK_PUSH_LABEL) + return template.format(cmd=STACK_PUSH_LABEL, validator=validator) + + +def stack_push_unchecked_message() -> str: + return STYLE_UNCHECKED_MESSAGE.format( + reason=f"'{STACK_PUSH_LABEL}' publishes PRs and {no_validator_reason()}", + validator=VALIDATOR_RELATIVE_PATHS[0], + ) diff --git a/engine/hooks/pr-schema-gate/tests/test_advisory.py b/engine/hooks/pr-schema-gate/tests/test_advisory.py index d20eb364..72cdd250 100644 --- a/engine/hooks/pr-schema-gate/tests/test_advisory.py +++ b/engine/hooks/pr-schema-gate/tests/test_advisory.py @@ -34,6 +34,11 @@ VALIDATOR_EXITS_ZERO_UNCHECKED = ( 'console.log("UNCHECKED: PR body rules not checked (drafter-core not installed)");\n' ) +VALIDATOR_PASSES_WITH_A_SKIPPED_SUBCHECK = ( + 'console.error("Summary reading grade unchecked: Summary has 9 words; ' + 'under 30 the score is too noisy to trust.");\n' + 'console.log("PR body validation passed.");\n' +) VALIDATOR_HANGS = "setTimeout(() => {}, 60000);\n" @@ -176,6 +181,22 @@ def test_validator_exit_zero_with_an_unchecked_line_is_not_clean(self): self.assertIn("could not check", context) self.assertIn("exited 0 without checking", context) + def test_pass_with_a_skipped_subcheck_is_clean_not_unchecked(self): + """A sub-check the validator skipped is not a vacuous pass. + + VALIDATOR_PASSES_WITH_A_SKIPPED_SUBCHECK is what the catstack + validator really prints for a body it accepts whose Summary is too + short to grade: the skipped sub-check on stderr, the verdict on + stdout, exit 0 (engine/skills/draft-pr/scripts/validate-pr-body.mjs + lines 56 and 78). The verdict says the body was judged and accepted. + """ + with _repo(VALIDATOR_PASSES_WITH_A_SKIPPED_SUBCHECK) as repo: + body = _body_file(repo) + code, _, context = _run(GH_PR + "edit 7 --body-file " + body, repo) + self.assertEqual(code, 0) + self.assertNotIn("could not check", context) + self.assertNotIn("exited 0 without checking", context) + def test_validator_real_pass_stays_clean(self): with _repo(VALIDATOR_PASSES) as repo: body = _body_file(repo) @@ -256,6 +277,14 @@ def test_clean_direct_body_write_clears_the_owed_follow_up(self): self.assertEqual(code, 0) self.assertIsNone(detect.read_pending(repo)) + def test_pass_with_a_skipped_subcheck_clears_the_owed_follow_up(self): + with _repo(VALIDATOR_PASSES_WITH_A_SKIPPED_SUBCHECK) as repo: + _run(STACK_PUSH_CMD, repo) + body = _body_file(repo) + code, _, _ = _run(GH_PR + "edit 7 --body-file " + body, repo) + self.assertEqual(code, 0) + self.assertIsNone(detect.read_pending(repo)) + def test_failing_direct_body_write_keeps_the_owed_follow_up(self): with _repo(VALIDATOR_FAILS) as repo: _run(STACK_PUSH_CMD, repo) @@ -284,10 +313,16 @@ def test_create_pr_mjs_subprocess_shape_is_silent(self): with _repo(VALIDATOR_FAILS) as repo: self.assertEqual(_run("gh api repos/o/r/pulls --method POST --input -", repo), (0, "", "")) - def test_repo_without_create_pr_tool_is_silent(self): + def test_git_repo_without_a_validator_reports_unchecked(self): with tempfile.TemporaryDirectory() as repo: os.makedirs(os.path.join(repo, ".git")) - self.assertEqual(_run(GH_PR + "edit 7 --body 'x'", repo), (0, "", "")) + code, _, context = _run(GH_PR + "edit 7 --body 'x'", repo) + self.assertEqual(code, 0) + self.assertIn("could not check", context) + + def test_directory_outside_any_git_repo_is_silent(self): + with tempfile.TemporaryDirectory() as plain: + self.assertEqual(_run(GH_PR + "edit 7 --body 'x'", plain), (0, "", "")) def test_heredoc_that_only_writes_the_text_is_silent(self): with _repo(VALIDATOR_FAILS) as repo: diff --git a/engine/hooks/pr-schema-gate/tests/test_api_repo_scope.py b/engine/hooks/pr-schema-gate/tests/test_api_repo_scope.py index ae8eba8e..89e1a216 100644 --- a/engine/hooks/pr-schema-gate/tests/test_api_repo_scope.py +++ b/engine/hooks/pr-schema-gate/tests/test_api_repo_scope.py @@ -74,9 +74,12 @@ def setUp(self): self.session = os.path.join(self.root.name, "Invoker") _repo(self.session, with_tool=True) - def test_real_catstack_edit_from_an_invoker_checkout_is_out_of_scope(self): + def test_real_catstack_edit_from_an_invoker_checkout_is_checked_in_catstack(self): + open(os.path.join(self.session, "scripts", "validate-pr-body.mjs"), "w").close() _repo(os.path.join(self.root.name, "catstack"), with_tool=False) - self.assertEqual(_run(REAL_COMMAND, self.session), "") + err = _run(REAL_COMMAND, self.session) + self.assertIn("could not check", err) + self.assertIn("restack-pr2.md", err) def test_edit_of_a_repo_with_no_local_checkout_is_out_of_scope(self): self.assertEqual(_run(REAL_COMMAND, self.session), "") diff --git a/engine/hooks/pr-schema-gate/tests/test_hooks.py b/engine/hooks/pr-schema-gate/tests/test_hooks.py index f61d07e0..42dcb2b3 100644 --- a/engine/hooks/pr-schema-gate/tests/test_hooks.py +++ b/engine/hooks/pr-schema-gate/tests/test_hooks.py @@ -8,6 +8,7 @@ import io import json import os +import shutil import sys import tempfile import time @@ -53,6 +54,18 @@ def _repo_with_tool() -> tempfile.TemporaryDirectory: os.makedirs(os.path.join(tmp.name, ".git")) with open(os.path.join(tmp.name, "scripts", "create-pr.mjs"), "w") as f: f.write("// stub\n") + with open(os.path.join(tmp.name, "scripts", "validate-pr-body.mjs"), "w") as f: + f.write("// stub\n") + return tmp + + +def _catstack_repo(validator_source: str) -> tempfile.TemporaryDirectory: + tmp = tempfile.TemporaryDirectory() + os.makedirs(os.path.join(tmp.name, ".git")) + scripts = os.path.join(tmp.name, "engine", "skills", "draft-pr", "scripts") + os.makedirs(scripts) + with open(os.path.join(scripts, "validate-pr-body.mjs"), "w") as f: + f.write(validator_source) return tmp @@ -173,19 +186,25 @@ def test_create_pr_followup_forms(self): self.assertTrue(detect.is_create_pr_followup(_cmd("./scripts/create-pr.mjs"))) self.assertFalse(detect.is_create_pr_followup(_cmd("cat", "scripts/create-pr.mjs"))) - def test_repo_root_found_when_tool_present(self): - with _repo_with_tool() as repo: - self.assertEqual(detect.repo_root_with_create_pr_tool(repo), repo) - - def test_repo_root_none_when_tool_absent(self): + def test_git_root_found_at_repo(self): with _repo_without_tool() as repo: - self.assertIsNone(detect.repo_root_with_create_pr_tool(repo)) + self.assertEqual(detect.git_root(repo), repo) - def test_repo_root_found_from_subdirectory(self): + def test_git_root_none_outside_any_repo(self): + with tempfile.TemporaryDirectory() as plain: + self.assertIsNone(detect.git_root(plain)) + + def test_git_root_found_from_subdirectory(self): with _repo_with_tool() as repo: sub = os.path.join(repo, "packages", "app") os.makedirs(sub) - self.assertEqual(detect.repo_root_with_create_pr_tool(sub), repo) + self.assertEqual(detect.git_root(sub), repo) + + def test_find_validator_prefers_scripts_then_draft_pr_skill(self): + with _repo_with_tool() as invoker, _catstack_repo("") as catstack, _repo_without_tool() as bare: + self.assertEqual(detect.find_validator(invoker), "scripts/validate-pr-body.mjs") + self.assertEqual(detect.find_validator(catstack), "engine/skills/draft-pr/scripts/validate-pr-body.mjs") + self.assertIsNone(detect.find_validator(bare)) def test_sibling_repo_dir_missing_returns_none(self): with tempfile.TemporaryDirectory() as root: @@ -214,20 +233,22 @@ def test_never_exits_nonzero_for_any_pr_write_in_scope(self): advised, _ = _run(command, repo) self.assertTrue(advised) - def test_repo_without_tool_is_silent(self): + def test_repo_without_validator_reports_unchecked(self): with _repo_without_tool() as repo: - self.assertEqual(_run(GH_PR_CREATE_CMD, repo), (False, "")) + advised, err = _run(GH_PR_CREATE_CMD, repo) + self.assertTrue(advised) + self.assertIn("could not check", err) def test_cd_into_repo_with_tool_is_in_scope(self): with _repo_with_tool() as repo, _repo_without_tool() as session_cwd: self.assertTrue(_run(f"cd {repo} && {GH_PR_CREATE_CMD}", session_cwd)[0]) - def test_cd_into_repo_without_tool_is_out_of_scope(self): - with _repo_with_tool() as session_cwd, _repo_without_tool() as repo: - self.assertFalse(_run(f"cd {repo} && {GH_PR_CREATE_CMD}", session_cwd)[0]) + def test_cd_out_of_any_git_repo_is_out_of_scope(self): + with _repo_with_tool() as session_cwd, tempfile.TemporaryDirectory() as plain: + self.assertFalse(_run(f"cd {plain} && {GH_PR_CREATE_CMD}", session_cwd)[0]) def test_codex_nested_workdir_outranks_session_cwd(self): - with _repo_with_tool() as session_cwd, _repo_without_tool() as target: + with _repo_with_tool() as session_cwd, tempfile.TemporaryDirectory() as target: source = ('const r = await tools.exec_command({' f'"cmd":"{GH_PR_EDIT_BODY_CMD}","workdir":"{target}"' '});') payload = {"tool_name": "exec_command", "tool_input": {"input": source}, "cwd": session_cwd} @@ -255,14 +276,16 @@ def test_repo_flag_into_sibling_repo_with_tool_is_in_scope(self): finally: os.environ.pop(detect.GITHUB_CHECKOUTS_ROOT_ENV, None) - def test_repo_flag_into_sibling_repo_without_tool_is_out_of_scope(self): + def test_repo_flag_into_sibling_repo_without_validator_reports_unchecked(self): with tempfile.TemporaryDirectory() as checkouts_root: os.environ[detect.GITHUB_CHECKOUTS_ROOT_ENV] = checkouts_root try: os.makedirs(os.path.join(checkouts_root, "catstack", ".git")) with _repo_with_tool() as session_cwd: command = GH + " pr edit 209 --repo EdbertChan/catstack --body-file /tmp/pr209-body-new.md" - self.assertFalse(_run(command, session_cwd)[0]) + advised, err = _run(command, session_cwd) + self.assertTrue(advised) + self.assertIn("could not check", err) finally: os.environ.pop(detect.GITHUB_CHECKOUTS_ROOT_ENV, None) @@ -441,5 +464,89 @@ def test_unchecked_direct_writer_does_not_clear_pending(self): self.assertIsNotNone(detect.read_pending(repo)) +CATSTACK_VALIDATOR_PASSES = 'console.log("PR body validation passed.");\n' +CATSTACK_VALIDATOR_FAILS = ( + 'console.error("PR body validation failed:");\n' + 'console.error("- Missing required section: ## Revert Plan");\n' + "process.exit(1);\n" +) + + +@unittest.skipUnless(shutil.which("node"), "node is required to run the validator stub") +class TestCatstackShapedRepo(StackFollowUpBase): + def _body(self, repo: str) -> str: + path = os.path.join(repo, "pr-body.md") + with open(path, "w") as f: + f.write("## Summary\n\nhi\n") + return path + + def test_stack_push_is_never_silent(self): + with _catstack_repo(CATSTACK_VALIDATOR_PASSES) as repo: + advised, err = _run(STACK_PUSH_CMD, repo) + self.assertTrue(advised) + self.assertIn("engine/skills/draft-pr/scripts/validate-pr-body.mjs", err) + self.assertIsNotNone(detect.read_pending(repo)) + + def test_failing_body_file_reports_the_validator_errors(self): + with _catstack_repo(CATSTACK_VALIDATOR_FAILS) as repo: + advised, err = _run(f"{GH_PR_CREATE_CMD} --body-file {self._body(repo)}", repo) + self.assertTrue(advised) + self.assertIn("does not follow this repo's PR style", err) + self.assertIn("## Revert Plan", err) + + def test_passing_body_file_is_clean(self): + with _catstack_repo(CATSTACK_VALIDATOR_PASSES) as repo: + self.assertEqual(_run(f"{GH_PR_CREATE_CMD} --body-file {self._body(repo)}", repo), (False, "")) + + def test_validator_found_from_a_subdirectory(self): + with _catstack_repo(CATSTACK_VALIDATOR_FAILS) as repo: + sub = os.path.join(repo, "engine", "hooks") + os.makedirs(sub) + self.assertTrue(_run(f"{GH_PR_CREATE_CMD} --body-file {self._body(repo)}", sub)[0]) + + def test_git_file_marks_a_worktree_root(self): + with tempfile.TemporaryDirectory() as tmp: + repo = os.path.join(tmp, "wt") + scripts = os.path.join(repo, "engine", "skills", "draft-pr", "scripts") + os.makedirs(scripts) + with open(os.path.join(repo, ".git"), "w") as f: + f.write("gitdir: /elsewhere\n") + with open(os.path.join(scripts, "validate-pr-body.mjs"), "w") as f: + f.write(CATSTACK_VALIDATOR_FAILS) + self.assertEqual(detect.git_root(repo), repo) + self.assertTrue(_run(STACK_PUSH_CMD, repo)[0]) + + +class TestRepoWithNoValidator(StackFollowUpBase): + def test_pr_publishing_commands_report_unchecked(self): + with _repo_without_tool() as repo: + for command in ( + STACK_PUSH_CMD, + GH_PR_CREATE_CMD, + GH_PR_EDIT_BODY_CMD, + GH + " api -X PATCH repos/{owner}/{repo}/pulls/7 -F body=@b.md", + ): + with self.subTest(command=command): + advised, err = _run(command, repo) + self.assertTrue(advised) + self.assertIn("could not check", err) + self.assertIsNone(detect.read_pending(repo)) + + def test_stack_push_names_the_missing_validator(self): + with _repo_without_tool() as repo: + _, err = _run(STACK_PUSH_CMD, repo) + self.assertIn("no PR validator", err) + + def test_non_publishing_commands_stay_silent(self): + with _repo_without_tool() as repo: + for command in ("git status", GH + " pr view 7", "mergify stack push --dry-run", "echo 'unbalanced"): + with self.subTest(command=command): + self.assertEqual(_run(command, repo), (False, "")) + + def test_directory_outside_any_git_repo_is_silent(self): + with tempfile.TemporaryDirectory() as plain: + self.assertEqual(_run(STACK_PUSH_CMD, plain), (False, "")) + + if __name__ == "__main__": unittest.main() diff --git a/engine/hooks/prove-it-ship-gate/README.md b/engine/hooks/prove-it-ship-gate/README.md index f0067907..e69b4a35 100644 --- a/engine/hooks/prove-it-ship-gate/README.md +++ b/engine/hooks/prove-it-ship-gate/README.md @@ -1,12 +1,48 @@ # prove-it-ship-gate Stop hook: when the outgoing message claims done / shipped / deployed / live -about work with a live side effect (Linear, deploy, production host, webhook, -Slack, external API), the same message must carry evidence a reviewer can -chase (URL, sha, ticket or PR id, fenced output, exit code, PID, timestamp), -or a live command must have run this turn (`ssh`, `curl`, `gh api`, -`gh pr view`, `systemctl`, ...), or the claim must carry the literal prefix -`{{CAT-UNVERIFIED: -- cannot verify: }}`. Otherwise the turn is blocked (exit 2). +about work with a live side effect, the same message must carry evidence a +reviewer can chase (URL, sha, ticket id, fenced output, exit code, PID, +timestamp), or a live command must have run this turn (`ssh`, `curl`, +`gh api`, `gh pr view`, `systemctl`, ...), or the claim must carry the literal +prefix `{{CAT-UNVERIFIED: -- cannot verify: }}`. Otherwise the +turn is blocked (exit 2). + +## What counts as a live surface + +Two families, and the second is not optional: + +- **External services** -- Linear, a deploy or production host, DO1, a + droplet, a webhook, Slack, an external API, a live mine, the nightly + pipeline, the merge queue, a real tick. +- **The user's own machine, session, or screen** -- `your machine`, `your + Mac`, `your laptop`, `your desktop`, `your screen`, `your session`, and + the same nouns spelled as the user's or theirs, plus `playwright`, + `electron`, and `popup`. One qualifier may sit between the two -- `your + own machine`, `the user's own screen`, `your real laptop` all count, since + `own` is the wording the block message, this README, and the skill all + use. A window opened on the user's desktop is as real a side effect as a + ticket write, and no fixture in the repo can stand in for it. + +Bare `end-to-end` and `e2e` are not on that list. They say how a check ran, +not where, and `working end-to-end` / `confirmed end-to-end` are already +claim phrases -- so counting the idiom as a surface would park every such +claim on top of a live noun and collapse the two parts into one, blocking +"the parser now works end-to-end" for showing no live proof it never needed. +Say the surface: `end-to-end on your Mac` still fires. + +Silent on a mention without a ship claim ("I'm about to run the Playwright +suite on your machine"), because the gate needs a claim and a live noun +within 240 characters of each other. + +## What does not count as evidence + +A link to the pull request that carries this change. The block message +already said the PR *number* of this change does not prove the live path +ran; a link to that same PR is the same claim in another spelling, so the +evidence scan blanks any `.../pull/` URL span before it looks. Only that +span is blanked -- a sha, an exit code, or a fenced block sitting beside the +link still counts, and an Actions-run URL is still a live receipt. Fixture tests and UI registration do not count. That is the whole point: Invoker PRs #10553-#10558 shipped cross-repo-research after unit + fixture + @@ -14,7 +50,16 @@ UI only, and the user had to force a live Linear e2e. Mechanical half of `corpus/skills/prove-it-ship-gate`. Judgment (is this work really live-side-effect work) stays with the model; the hook only matches -shapes. Fail-open on parse/read errors; `stop_hook_active` skips. +shapes. + +## Fail direction + +Open, on every read. A payload that will not parse, a transcript that cannot +be opened, and a message with no claim all allow the turn: a bug in this hook +can only under-block, never hold a correct message hostage. The escape hatch +is the tag, not a flag -- `{{CAT-UNVERIFIED: -- cannot verify: }}` +ends the turn, and `stop_hook_active` does not, so a retry that still claims +without proof is blocked again. ## Files diff --git a/engine/hooks/prove-it-ship-gate/detect.py b/engine/hooks/prove-it-ship-gate/detect.py index 45fc0f3b..427d7101 100644 --- a/engine/hooks/prove-it-ship-gate/detect.py +++ b/engine/hooks/prove-it-ship-gate/detect.py @@ -11,6 +11,20 @@ Incident: Invoker PRs #10553-#10558 published cross-repo-research after unit + fixture + UI only; the user forced a live Linear e2e and a reflect. + +No claim phrase is also a live noun. Bare "end-to-end" and "e2e" say how a +check ran, not where, and the claim list already spells "working end-to-end" +and "confirmed end-to-end" as claims, so counting the idiom as a surface would +put every such claim permanently on top of a live noun and collapse the +two-part check into one part. "Done, the parser works end-to-end" is a unit +suite. The surface in the incident was the desktop the windows opened on, and +"your machine", playwright, electron and popup already name it. + +The possessive is not always glued to the noun. "your own machine" and "the +user's own screen" are the wording this repo uses everywhere -- the block +message below, the README, and the skill -- so the surface nouns accept one +qualifier ("own", "real", "actual", "personal", "local") after the possessive. +Without it the gate stays silent on exactly the phrasing it tells you to use. """ from __future__ import annotations @@ -51,7 +65,10 @@ r"\b(?:linear|deploy(?:ed|ment|s)?|production|prod|do-?1|droplet|digital\s*ocean|" r"webhook|slack|external api|live mine|posthog|stripe|sentry|live path|" r"nightly|pipeline|merge queue|(?:real|scheduled|next)\s+tick|" - r"live (?:worker|owner|host|server|tick))\b", + r"live (?:worker|owner|host|server|tick)|" + r"(?:your|their|the user'?s)\s+(?:(?:own|real|actual|personal|local)\s+)?" + r"(?:machine|mac|macbook|laptop|desktop|screen|session|computer|keyboard)|" + r"playwright|electron|pop-?ups?)\b", re.IGNORECASE, ) PROXIMITY_WINDOW = 240 # chars between a claim word and a live noun @@ -66,6 +83,11 @@ re.IGNORECASE, ) +PR_LINK_RE = re.compile( + r"https?://\S*?/(?:pull|pull-requests|merge_requests)/\d+\S*", + re.IGNORECASE, +) + LIVE_RECEIPT_RE = re.compile( r"\bwf-\d{10,}-\d+\b|actions/runs/\d+|\bdaily-\d{8}\b|" r"\bDelegated to live owner\b", @@ -98,8 +120,12 @@ def has_live_receipt(message: str) -> bool: return bool(LIVE_RECEIPT_RE.search(message or "")) +def without_pr_links(message: str) -> str: + return PR_LINK_RE.sub(" ", message or "") + + def has_evidence(message: str) -> bool: - return bool(EVIDENCE_RE.search(message or "")) or has_live_receipt(message) + return bool(EVIDENCE_RE.search(without_pr_links(message))) or has_live_receipt(message) def _is_user_line(data: dict) -> bool: @@ -185,10 +211,11 @@ def decide(payload: dict) -> str | None: return ( "prove-it-ship-gate: this message claims done/shipped/live/proven for work " "with a live side effect (Linear, deploy, production host, webhook, external " - "API) but shows no live evidence -- no URL, sha, ticket id, fenced output, " - "exit code, live-output receipt, or live command this turn. Fixture tests, UI " - "registration, a dry run, and the PR number of this change do not prove the " - "live path ran; only an id the pipeline itself emitted does (a workflow id, " + "API, or the user's own machine, session, or screen) but shows no live " + "evidence -- no URL, sha, ticket id, fenced output, exit code, live-output " + "receipt, or live command this turn. Fixture tests, UI registration, a dry " + "run, and the PR number or PR link of this change do not prove the live path " + "ran; only an id the pipeline itself emitted does (a workflow id, " "an Actions run URL, a release tag, a live-owner dispatch). Paste that " "evidence in this message, or tag the claim: " f"`{markers.TAG_TEMPLATE}`." diff --git a/engine/hooks/prove-it-ship-gate/tests/test_hooks.py b/engine/hooks/prove-it-ship-gate/tests/test_hooks.py index 587c6483..2c748d07 100644 --- a/engine/hooks/prove-it-ship-gate/tests/test_hooks.py +++ b/engine/hooks/prove-it-ship-gate/tests/test_hooks.py @@ -234,6 +234,219 @@ def test_fires_when_the_only_id_is_the_changes_own_pr_number(self): self.assertIsNotNone(detect.decide({"last_assistant_message": text})) +INCIDENT_SHIP_MESSAGE = ( + 'Shipped. Both PRs are live with full bodies: [#12927](https://github.com/Neko-Catpital-Labs/Invoker/pull/12927) (base `master`) and [#12928](https://github.com/Neko-Catpital-Labs/Invoker/pull/12928) (stacked on it).\n' + '\n' + 'The popups are stopped on your machine, and 586 stale test checkouts were patched so a retry refuses instead of opening windows.\n' + '\n' + "**Waiting on:** the reflect pass. It's checking whether this same complaint happened before. If it did, the rule says the fix belongs in your always-on working-style rules — the `automate-me` skill, which rewrites the habit so you don't have to catch it again — rather than another one-off patch. Estimate: back by **00:41 PDT**; a fallback wake is armed, and it may land sooner.\n" + '\n' + '**Still open:**\n' + '1. `packages/app/e2e/fixtures/electron-app.ts` has no hidden-window path on macOS.\n' + "2. Invoker's default pool still includes your Mac, so these jobs still get sent here — they just refuse now.\n" + "3. Your catstack hooks are stale (checkout sits on `install-hook-open-probe`), so merged hook fixes aren't live here.\n") + +INCIDENT_STACK_MESSAGE = ( + 'Stack is live and correctly formed:\n' + '\n' + '| PR | Title | Base |\n' + '|---|---|---|\n' + '| [#12927](https://github.com/Neko-Catpital-Labs/Invoker/pull/12927) | `[Headless E2E Guard](1) Refuse Electron e2e on an interactive desktop` | `master` |\n' + "| [#12928](https://github.com/Neko-Catpital-Labs/Invoker/pull/12928) | `[Headless E2E Guard](2) Gate the e2e shell launchers on a virtual display` | PR #12927's branch |\n" + '\n' + 'Both carry the full section schema; both bodies passed `validate-pr-body-local.mjs`.\n' + '\n' + '**Done on your machine:** popups stopped, 586 stale worktree copies patched so a retry refuses too.\n' + '\n' + '**Still open, carried forward:**\n' + '1. `/reflect` pass still running — checking whether this complaint recurred before. If yes, it routes to `automate-me`, not another one-off.\n' + '2. `packages/app/e2e/fixtures/electron-app.ts` still has no hidden-window path on macOS.\n' + "3. Invoker's default pool still includes your Mac, so these jobs still get *dispatched* here — they just refuse now.\n" + "4. Your catstack hooks are stale (checkout sits on `install-hook-open-probe`), so merged hook fixes aren't live.\n") + +INCIDENT_COMPLIANT_REWRITE = ( + "I ran the e2e I'd skipped. Exact incident command on the fixed branch:\n" + '\n' + '```\n' + '40-playwright-app.sh: refusing to run Electron e2e without a virtual display.\n' + 'SUITE_EXIT=1\n' + '```\n' + 'A watcher sampling every 0.5s alongside it recorded **zero Electron windows**.\n' + '\n' + "**I was wrong about the cause.** The guard didn't drift — a codex agent **deleted it**. The launcher called `xvfb-run` unconditionally and hard-failed; the agent rewrote it to run headed instead, then ran 9.3 minutes of windows on your desktop.\n" + '\n' + 'So I wired `pnpm run check:all` to run the gate test — dropping the gate from any launcher now fails CI.\n' + '\n' + "Also wrong earlier: I told you your catstack hooks were stale. They aren't (`git rev-list --count HEAD..origin/main` = 0). Ignore that.\n" + '\n' + '#12927 merged; #12928 open and refreshed.\n' + '\n' + 'Back at **01:05 PDT** with the second reflect and `automate-me`.\n') + +LOCAL_SURFACE_FIRE = [ + INCIDENT_SHIP_MESSAGE, + INCIDENT_STACK_MESSAGE, + "Shipped — the popups are stopped on your Mac.", + "Done. The Electron windows no longer open on your desktop.", + "The Playwright shard is working end to end now on your laptop.", + "All of it is done: the e2e guard is live in your session.", + "The guard is shipped and nothing opens on the user's screen any more.", + "Shipped -- the windows are gone from your own machine.", + "Done. Nothing opens on the user's own screen any more.", + "All of it is done: the guard is live in their own session.", + "Shipped. The suite ran on your real laptop and nothing popped.", +] + +LOCAL_SURFACE_SILENT = [ + INCIDENT_COMPLIANT_REWRITE, + "I'm about to run the Playwright suite on your machine; nothing has run yet.", + "Shipped the headless guard on your machine (`8262dc40a6` on master).", + "The e2e shard is working end to end on your Mac -- " + "https://github.com/EdbertChan/catstack/actions/runs/34259072426 is green.", +] + +IDIOM_ONLY_SILENT = [ + "Done. The parser now works end-to-end.", + "All of it is done: the e2e suite is green.", + "The retry path is working end to end.", + "Confirmed end-to-end: the date formatter handles leap years.", + "Shipped the e2e coverage for the tokenizer.", +] + + +class TestLocalSurfaceIsALiveSurface(unittest.TestCase): + """The 2026 incident: an agent ran a CI Playwright shard on the user's own + Mac, opened Electron windows on their desktop for minutes, then said + "Shipped ... the popups are stopped on your machine" with only two links to + its own PRs. The gate ran on that turn and stayed silent, because its noun + list held only external services and a PR link counted as live evidence. + Both halves have to change; either alone leaves the message silent.""" + + def test_fires_on_each_local_surface_claim(self): + for text in LOCAL_SURFACE_FIRE: + with self.subTest(text=text[:60]): + self.assertTrue(detect.claims_live_ship(text)) + self.assertIsNotNone(detect.decide({"last_assistant_message": text})) + + def test_the_incident_message_names_a_live_noun(self): + self.assertTrue(detect.LIVE_NOUN_RE.search(INCIDENT_SHIP_MESSAGE)) + self.assertTrue(detect.LIVE_NOUN_RE.search(INCIDENT_STACK_MESSAGE)) + + def test_silent_on_each_local_surface_near_neighbour(self): + for text in LOCAL_SURFACE_SILENT: + with self.subTest(text=text[:60]): + self.assertIsNone(detect.decide({"last_assistant_message": text})) + + def test_the_compliant_rewrite_carries_its_own_receipt(self): + """The real follow-up message: same work, same local surface, but the + suite's own output pasted. It makes no bare ship claim and it shows a + receipt, so neither half of the gate has anything to say.""" + self.assertFalse(detect.claims_live_ship(INCIDENT_COMPLIANT_REWRITE)) + self.assertTrue(detect.has_evidence(INCIDENT_COMPLIANT_REWRITE)) + for retry in (False, True): + self.assertIsNone(detect.decide({ + "last_assistant_message": INCIDENT_COMPLIANT_REWRITE, + "stop_hook_active": retry, + })) + + def test_the_blocked_message_plus_its_receipt_stops_blocking(self): + """Fix/re-trigger pair: the message the gate blocks, and the same + message once the real run's output is pasted into it, which must not + trip this or any other check in the hook.""" + blocked = INCIDENT_SHIP_MESSAGE + fixed = INCIDENT_SHIP_MESSAGE + ( + "\n\n```\n40-playwright-app.sh: refusing to run Electron e2e " + "without a virtual display.\nSUITE_EXIT=1\n```\n" + ) + self.assertIsNotNone(detect.decide({"last_assistant_message": blocked})) + for retry in (False, True): + self.assertIsNone(detect.decide({ + "last_assistant_message": fixed, "stop_hook_active": retry, + })) + + def test_silent_when_only_the_idiom_names_the_surface(self): + """`working end-to-end` and `confirmed end-to-end` are already claim + phrases. If the same words also counted as the live noun, every one of + those claims would sit permanently on top of a surface and the + two-part check would collapse to one part -- a unit-suite done-claim + would be blocked for showing no live evidence it never needed.""" + for text in IDIOM_ONLY_SILENT: + with self.subTest(text=text[:60]): + self.assertIsNone(detect.LIVE_NOUN_RE.search(text)) + self.assertIsNone(detect.decide({"last_assistant_message": text})) + + def test_a_claim_phrase_never_doubles_as_its_own_live_noun(self): + """The general shape of the bug above: no claim phrase may consist + entirely of live nouns, or matching it proves both halves at once.""" + for phrase in ("working end-to-end", "confirmed end-to-end", + "now works", "fully fixed", "done and shipped"): + with self.subTest(phrase=phrase): + self.assertTrue(detect.CLAIM_RE.search(phrase)) + stripped = detect.LIVE_NOUN_RE.sub(" ", phrase).strip() + self.assertTrue(stripped, f"{phrase!r} is entirely live nouns") + + def test_a_qualifier_between_the_possessive_and_the_noun_still_fires(self): + """`own` is the word the block message, the README, and the skill all + use for this surface. If the noun had to sit immediately after the + possessive, the gate would stay silent on its own recommended + phrasing: same sentence, one extra word, opposite verdict.""" + for qualifier in ("own", "real", "actual", "personal", "local"): + bare = "Shipped -- the windows are gone from your machine." + with_qualifier = bare.replace("your machine", + f"your {qualifier} machine") + with self.subTest(qualifier=qualifier): + self.assertIsNotNone(detect.LIVE_NOUN_RE.search(bare)) + self.assertIsNotNone(detect.LIVE_NOUN_RE.search(with_qualifier)) + self.assertIsNotNone( + detect.decide({"last_assistant_message": with_qualifier})) + + def test_the_block_messages_own_phrasing_trips_the_noun_scan(self): + """The gate must not describe a surface it cannot detect.""" + feedback = detect.decide({"last_assistant_message": INCIDENT_SHIP_MESSAGE}) + self.assertIn("the user's own machine, session, or screen", feedback) + self.assertIsNotNone(detect.LIVE_NOUN_RE.search( + "the user's own machine, session, or screen")) + + def test_block_message_names_the_users_own_surface(self): + feedback = detect.decide({"last_assistant_message": INCIDENT_SHIP_MESSAGE}) + self.assertIn("own machine, session, or screen", feedback) + + +class TestOwnPrLinkIsNotLiveEvidence(unittest.TestCase): + """The block message already said the PR number of this change does not + prove the live path ran. A link to that same PR is the same claim in + another spelling, so the evidence scan has to decline it too.""" + + def test_a_pr_link_alone_is_not_evidence(self): + self.assertFalse(detect.has_evidence(INCIDENT_SHIP_MESSAGE)) + self.assertFalse(detect.has_evidence(INCIDENT_STACK_MESSAGE)) + + def test_the_strip_covers_only_the_link_span(self): + text = ( + "Shipped on your machine. PR: " + "https://github.com/Neko-Catpital-Labs/Invoker/pull/12927 " + "and the live run ended `EXIT_CODE=0`." + ) + self.assertTrue(detect.has_evidence(text)) + self.assertIsNone(detect.decide({"last_assistant_message": text})) + + def test_a_non_pr_url_is_still_evidence(self): + text = ( + "The e2e guard is live on your Mac -- " + "https://github.com/EdbertChan/catstack/actions/runs/34259072426" + ) + self.assertTrue(detect.has_live_receipt(text)) + self.assertTrue(detect.has_evidence(text)) + self.assertIsNone(detect.decide({"last_assistant_message": text})) + + def test_tagging_the_blocker_still_ends_the_turn(self): + tagged = INCIDENT_SHIP_MESSAGE + ( + "\n\n{{CAT-UNVERIFIED: the popups are stopped -- cannot verify: " + "the suite cannot run headless on this host}}" + ) + self.assertIsNone(detect.decide({"last_assistant_message": tagged})) + + if __name__ == "__main__": unittest.main() diff --git a/engine/hooks/wrong-check-reflect/README.md b/engine/hooks/wrong-check-reflect/README.md index 02db976a..1b2a32d9 100644 --- a/engine/hooks/wrong-check-reflect/README.md +++ b/engine/hooks/wrong-check-reflect/README.md @@ -12,7 +12,8 @@ up. If the judge result was unchecked, the inbox reports "could not judge" instead of treating the reply as clean. Finish the live correction first. Fail-open. -Once per transcript. Skip if the user already said `/reflect`. +Once per reply. Skip if the user asked for `/reflect` in the same turn that +produced that reply. Not word-count (`diu-stop`). Not token_audit thrash (`reflect-on-thrash`). Assistant text only - user messages and fenced code stay silent. @@ -24,9 +25,26 @@ builds a phrase-dictionary job, and sends it to `llm-judge`. The dictionary defines the meaning with `match` and `not_match` examples and supplies the static `on_hit` follow-up text. -No job is sent when `stop_hook_active` is set, when this transcript or reply -was already prompted, when the reply is empty, or when the user already asked -for `/reflect`. Inside a judge run (`CATSTACK_LLM_JUDGE_CHILD=1`) `llm-judge` +No job is sent when `stop_hook_active` is set, when this exact reply was +already prompted, when the reply is empty, or when the user asked for +`/reflect` in the turn that produced this reply. Only the person counts: the +harness files its own injections as `type: "user"` rows carrying `isMeta`, so +a Stop hook's own feedback and a skill's injected body are read as harness +text, not as the user asking. A typed `/reflect` is not harness text: the +harness writes it as `` inside the person's own row, so it +still counts as the person asking. Before that, `diu-stop`'s block text and the +reflect skill's own body both said "reflect" and switched this hook off. + +The one-shot key is the +transcript path plus a hash of the reply text: keyed on the transcript alone, +the Stop of the reply *before* a correction spent the key, and the correction +a minute later found itself already prompted. The `/reflect` scan is scoped to +the current turn for the same reason -- scanning the whole transcript let one +`/reflect` switch the hook off for the rest of the session. That turn runs +from the person's own last message to the end of the file, never from the +last assistant row: a Stop carries the reply before its row is written, so +the last assistant row is the turn before's, and a turn writes several +assistant rows anyway (narration, a subagent's sidechain). Inside a judge run (`CATSTACK_LLM_JUDGE_CHILD=1`) `llm-judge` refuses the job. The model call runs in a detached background process, so the reply is never @@ -50,7 +68,7 @@ pattern to this hook; the prose meaning belongs in the phrase dictionary. ## Files -- `detect.py` - judge enqueue + once-per-transcript state +- `detect.py` - judge enqueue + once-per-reply state - `claude_stop_check.py` — Claude `Stop` (stderr + exit 2) - `cursor_session.py` — Cursor `stop` / `sessionEnd` (`followup_message`) - `codex_notify.py` — Codex `notify` (advisory print + chain) diff --git a/engine/hooks/wrong-check-reflect/detect.py b/engine/hooks/wrong-check-reflect/detect.py index e49ab3b1..8bf94422 100644 --- a/engine/hooks/wrong-check-reflect/detect.py +++ b/engine/hooks/wrong-check-reflect/detect.py @@ -24,8 +24,12 @@ os.path.join(os.path.expanduser("~"), ".cache", "catstack-wrong-check-reflect"), ) -ALREADY_REFLECT_RE = re.compile(r"(?i)\b/?reflect\b|\b/?automate-me\b|\bautomate me\b") -META_USER_PREFIXES = ("\s*/?(?:reflect|automate-me)\b" + r"|\s*(?:reflect|automate-me)\s*" +) +META_USER_PREFIXES = ( + " str: - key = transcript_path or "no-transcript" - digest = hashlib.sha1(os.path.abspath(key).encode()).hexdigest()[:16] +def reply_key(transcript_path: str, text: str) -> str: + """One-shot key for a single reply, not for a whole session. + + Keying on the transcript alone made the hook fire at most once per + session, and the Stop that spent the key was the Stop of the reply + BEFORE the correction -- so the correction itself, a minute later, was + already marked as prompted. A reply is the thing being judged, so the + reply's text is what the key is made of. The transcript stays in the key + so the same sentence in two sessions is two chances, not one. + """ + base = os.path.abspath(transcript_path) if transcript_path else "no-transcript" + return base + "\n" + hashlib.sha256((text or "").encode("utf-8")).hexdigest() + + +def _state_file(key: str) -> str: + digest = hashlib.sha1((key or "no-transcript").encode()).hexdigest()[:16] return os.path.join(STATE_DIR, f"{digest}.prompted") -def already_prompted(transcript_path: str) -> bool: - return os.path.isfile(_state_file(transcript_path or "no-transcript")) +def already_prompted(key: str) -> bool: + return os.path.isfile(_state_file(key or "no-transcript")) -def mark_prompted(transcript_path: str) -> None: - path = _state_file(transcript_path or "no-transcript") +def mark_prompted(key: str) -> None: + path = _state_file(key or "no-transcript") os.makedirs(os.path.dirname(path), exist_ok=True) with open(path, "w", encoding="utf-8") as handle: - handle.write((transcript_path or "") + "\n") + handle.write((key or "") + "\n") def _is_user_line(data: dict) -> bool: @@ -60,6 +77,43 @@ def _is_user_line(data: dict) -> bool: return isinstance(message, dict) and message.get("role") == "user" +def _is_tool_result_line(data: dict) -> bool: + """True for a user-shaped row that is only a tool's output. + + Claude Code files every tool result as `type: "user"` and, unlike its + other injections, marks it with neither `isMeta` nor `isSidechain` -- the + record it does carry is a `tool_result` content block, plus a + `toolUseResult` field alongside the message. Reading that is the same + move `engine/hooks/agent-relay-attribution/detect.py:76` makes. + """ + if data.get("toolUseResult") is not None: + return True + message = data.get("message") + content = message.get("content") if isinstance(message, dict) else data.get("content") + return isinstance(content, list) and any( + isinstance(block, dict) and block.get("type") == "tool_result" for block in content + ) + + +def _is_meta_line(data: dict) -> bool: + """True for a user-shaped row that is not the person speaking. + + The harness files its own injections as `type: "user"`: a Stop hook's + feedback, a skill's body, a subagent's transcript, a tool's result. Each + carries a record saying so -- `isMeta`, which is what + `engine/skills/reflect/scripts/token_audit.py:313` keys off, or the + `tool_result` shape above -- so this reads the record instead of the + prose. A prose prefix could only ever catch the wordings someone had + already seen -- and it missed both the hook feedback and the reflect + skill's own body, which is how running `/reflect` disarmed this hook. + """ + if data.get("isMeta") or data.get("agentId") or data.get("isSidechain"): + return True + if _is_tool_result_line(data): + return True + return _message_text(data).lstrip().startswith(META_USER_PREFIXES) + + def _message_text(data: dict) -> str: message = data.get("message") content = message.get("content") if isinstance(message, dict) else data.get("content") @@ -76,9 +130,9 @@ def _message_text(data: dict) -> str: return "" -def user_already_asked_reflect(path: str) -> bool: - if not path or not os.path.isfile(path): - return False +def _transcript_roles(path: str) -> list[tuple[str, str]] | None: + """(role, text) per transcript line, or None when the file cannot be read.""" + rows: list[tuple[str, str]] = [] try: with open(path, encoding="utf-8") as handle: for line in handle: @@ -86,15 +140,68 @@ def user_already_asked_reflect(path: str) -> bool: data = json.loads(line) except json.JSONDecodeError: continue - if not isinstance(data, dict) or not _is_user_line(data): + if not isinstance(data, dict): continue - text = _message_text(data) - if not text or text.lstrip().startswith(META_USER_PREFIXES): - continue - if ALREADY_REFLECT_RE.search(text): - return True - except OSError: + if _is_user_line(data): + rows.append(("meta" if _is_meta_line(data) else "user", _message_text(data))) + elif _is_assistant_line(data): + rows.append(("assistant", _message_text(data))) + except OSError as exc: + print( + f"catstack-hook-error wrong-check-reflect: cannot read {path}, " + f"the user's own reflect request is unchecked: {exc}", + file=sys.stderr, + ) + return None + return rows + + +def user_already_asked_reflect(path: str) -> bool: + """True when the user invoked reflect in the turn that produced this reply. + + Only a real invocation counts, which the harness records as a + `/reflect` envelope. Prose that merely says + the word does not: `"Claim I made was wrong" is a trigger for /reflect` + describes the rule, it does not ask for anything, and suppressing on it + let a sentence about the hook switch the hook off. Erring toward asking + is the safe direction for a detector that spoke 0 times in 1,682 runs. + + Scoped to that one turn on purpose. Scanning the whole transcript meant a + single `/reflect` typed at the start of a session switched the detector + off for every reply after it, however many hours later. + + The turn is the stretch from the person's own last message to the end of + the file. Anchoring it on assistant rows instead was wrong twice over. A + Stop payload carries the reply before its row is written, so the last + assistant row was then the PREVIOUS turn's reply: that turn's `/reflect` + suppressed this one -- the session lockout back, just one turn wide -- + and the `/reflect` on the current message sat after the window and was + ignored. And a turn writes more than one assistant row: mid-turn + narration and a subagent's sidechain rows each pushed the window's start + past the message that opened the turn. The person's message is the row + that actually starts a turn, so it is the anchor; rows after it are this + turn's whether or not the reply has landed yet. + + A tool result is filed as a `type: "user"` row too, so it only counts as + the person speaking if nothing checks -- and then the first tool call of + the turn became the anchor and the `/reflect` that opened the turn fell + outside the window. `_is_meta_line` rules those rows out. + """ + if not path or not os.path.isfile(path): + return False + rows = _transcript_roles(path) + if rows is None: return False + start = 0 + for index in range(len(rows) - 1, -1, -1): + if rows[index][0] == "user": + start = index + break + for role, text in rows[start:]: + if role == "assistant" or not text: + continue + if ALREADY_REFLECT_RE.search(text): + return True return False @@ -192,7 +299,7 @@ def enqueue_judge(payload: dict) -> str | None: return None path = resolve_transcript(payload) text = last_assistant_text(payload, path) - key = path or text[:200] + key = reply_key(path, text) if not text.strip() or already_prompted(key): return None if path and user_already_asked_reflect(path): diff --git a/engine/hooks/wrong-check-reflect/tests/test_hooks.py b/engine/hooks/wrong-check-reflect/tests/test_hooks.py index e3d44630..bf7a5373 100644 --- a/engine/hooks/wrong-check-reflect/tests/test_hooks.py +++ b/engine/hooks/wrong-check-reflect/tests/test_hooks.py @@ -31,6 +31,7 @@ PY = sys.executable +REFLECT_COMMAND = "reflect/reflect" HIT_TEXT = "Correction: the file I pointed you to earlier is not the one in use; the real one is src/b.py." OPTION_TEXT = "You're right. Let's go with option B." COUNT_TEXT = "I double-checked my earlier count and it holds; nothing in it was wrong." @@ -71,7 +72,35 @@ def run_codex_notify(argv: list[str]) -> str: def transcript_line(role: str, text: str) -> str: - return json.dumps({"type": role, "message": {"role": role, "content": [{"type": "text", "text": text}]}}) + """One transcript row. A role of "meta" is the harness talking, not the user. + + The shape of a meta row is taken from a real Claude Code transcript: the + harness files its Stop-hook feedback and a skill's injected body as + `type: "user"` rows carrying `isMeta: true`. A `sidechain-` prefix files + the row under a subagent, which a real transcript marks with + `isSidechain: true`. + + A role of "tool_result" is the other user-shaped row the harness writes, + and the one it flags with none of those keys: a real transcript gives it + a `tool_result` content block and a `toolUseResult` field, and no + `isMeta`. + """ + if role == "tool_result": + return json.dumps({ + "type": "user", + "message": {"role": "user", "content": [ + {"type": "tool_result", "tool_use_id": "toolu_1", "content": text}]}, + "toolUseResult": {"stdout": text}, + }) + sidechain = role.startswith("sidechain-") + role = role[len("sidechain-"):] if sidechain else role + meta = role == "meta" + kind = "user" if meta else role + row = {"type": kind, "message": {"role": kind, "content": [{"type": "text", "text": text}]}} + if meta or sidechain: + row["isMeta"] = meta + row["isSidechain"] = sidechain + return json.dumps(row) class TestWrongCheckReflect(JudgeTestCase): @@ -198,15 +227,220 @@ def test_judge_not_enqueued_when_stop_hook_active(self): def test_judge_not_enqueued_when_already_prompted(self): path = self.write_transcript(("assistant", HIT_TEXT)) - detect.mark_prompted(path) + detect.mark_prompted(detect.reply_key(path, HIT_TEXT)) self.assertIsNone(detect.enqueue_judge({"transcript_path": path})) self.assertEqual(self.jobs(), []) def test_judge_not_enqueued_when_user_already_asked_reflect(self): - path = self.write_transcript(("user", "please /reflect"), ("assistant", HIT_TEXT)) + path = self.write_transcript(("user", REFLECT_COMMAND), ("assistant", HIT_TEXT)) + self.assertIsNone(detect.enqueue_judge({"transcript_path": path})) + self.assertEqual(self.jobs(), []) + + def test_prose_about_reflect_does_not_count_as_asking_for_one(self): + """A sentence naming the command is not an invocation of it. + + `"Claim I made was wrong" is a trigger for /reflect` describes when + the hook fires. Treating that as a request let a sentence about the + hook switch the hook off for the rest of the turn. + """ + path = self.write_transcript( + ("user", '"Claim I made was wrong" is a trigger for /reflect'), + ("assistant", HIT_TEXT), + name="prose-mention.jsonl", + ) + self.assertIsNotNone(detect.enqueue_judge({"transcript_path": path})) + + def test_the_later_correction_still_fires_after_the_reply_before_it(self): + """Lockout one: the Stop of the pre-correction reply spent the key.""" + self.use_runners(SLOW_CLEAN) + turn_one = (("user", "check the path"), ("assistant", "The live file is src/a.py.")) + path = self.write_transcript(*turn_one, name="turn.jsonl") + self.assertIsNotNone(detect.enqueue_judge({"transcript_path": path})) + self.write_transcript( + *turn_one, + ("user", "are you sure?"), + ("assistant", HIT_TEXT), + name="turn.jsonl", + ) + self.assertIsNotNone(detect.enqueue_judge({"transcript_path": path})) + + def test_reflect_asked_earlier_in_the_session_does_not_silence_a_later_reply(self): + """Lockout two: one /reflect used to switch the hook off for good.""" + self.use_runners(SLOW_CLEAN) + path = self.write_transcript( + ("user", "please /reflect on the last hour"), + ("assistant", "Here is the reflect write-up."), + ("user", "now fix the import"), + ("assistant", HIT_TEXT), + name="long.jsonl", + ) + self.assertIsNotNone(detect.enqueue_judge({"transcript_path": path})) + + def test_reflect_asked_in_this_same_turn_is_still_not_doubled_up(self): + path = self.write_transcript( + ("user", "fix the import"), + ("assistant", "Done."), + ("user", "that was wrong. " + REFLECT_COMMAND), + ("assistant", HIT_TEXT), + name="same-turn.jsonl", + ) + self.assertIsNone(detect.enqueue_judge({"transcript_path": path})) + self.assertEqual(self.jobs(), []) + + def test_the_same_reply_is_never_queued_twice(self): + self.use_runners(SLOW_CLEAN) + path = self.write_transcript(("assistant", HIT_TEXT), name="dedup.jsonl") + self.assertIsNotNone(detect.enqueue_judge({"transcript_path": path})) + self.assertIsNone(detect.enqueue_judge({"transcript_path": path})) + + def test_the_same_reply_is_judged_under_both_spellings_of_the_stop_event(self): + """Harness parity: no branch may turn on the case of the event name.""" + self.use_runners(SLOW_CLEAN) + queued = {} + for spelling in ("Stop", "stop"): + path = self.write_transcript( + ("assistant", HIT_TEXT), name=f"parity-{spelling}.jsonl") + queued[spelling] = detect.enqueue_judge( + {"transcript_path": path, "hook_event_name": spelling}) + self.assertIsNotNone(queued["Stop"]) + self.assertIsNotNone(queued["stop"]) + + def test_a_stop_hook_feedback_line_does_not_suppress_the_hook(self): + """The ecosystem used to silence itself: diu-stop's own block says reflect.""" + self.use_runners(SLOW_CLEAN) + path = self.write_transcript( + ("user", "fix the import"), + ("meta", "Stop hook feedback: [diu-stop/claude_stop_check.py]: read the " + "reflect skill and say why"), + ("assistant", HIT_TEXT), + name="hook-feedback.jsonl", + ) + self.assertIsNotNone(detect.enqueue_judge({"transcript_path": path})) + + def test_the_reflect_skills_own_body_does_not_suppress_the_hook(self): + """Running /reflect used to disarm the detector that asks for it.""" + self.use_runners(SLOW_CLEAN) + path = self.write_transcript( + ("user", "fix the import"), + ("meta", "Base directory for this skill: ~/.claude/skills/reflect\n# Reflect"), + ("assistant", HIT_TEXT), + name="skill-body.jsonl", + ) + self.assertIsNotNone(detect.enqueue_judge({"transcript_path": path})) + + def test_a_real_user_reflect_request_in_the_same_turn_still_suppresses(self): + path = self.write_transcript( + ("user", "fix the import"), + ("assistant", "Done."), + ("user", "that was wrong. " + REFLECT_COMMAND), + ("meta", "Stop hook feedback: unrelated"), + ("assistant", HIT_TEXT), + name="real-request.jsonl", + ) + self.assertIsNone(detect.enqueue_judge({"transcript_path": path})) + self.assertEqual(self.jobs(), []) + + def test_a_reflect_this_turn_counts_before_the_reply_row_is_written(self): + """The Stop payload carries the reply; its transcript row is not there yet. + + Anchoring the window on the last assistant row made that row the + PREVIOUS turn's reply, so the `/reflect` the person typed a moment + ago sat past the window and the hook nagged for a reflect already + in flight. + """ + self.use_runners(SLOW_CLEAN) + path = self.write_transcript( + ("user", "fix the import"), + ("assistant", "Done."), + ("user", "that was wrong. " + REFLECT_COMMAND), + name="reply-row-not-written.jsonl", + ) + self.assertIsNone(detect.enqueue_judge( + {"transcript_path": path, "last_assistant_message": HIT_TEXT})) + self.assertEqual(self.jobs(), []) + + def test_last_turns_reflect_does_not_silence_a_reply_still_being_written(self): + """The same stale window, pointing the other way: a one-turn lockout.""" + self.use_runners(SLOW_CLEAN) + path = self.write_transcript( + ("user", "that was wrong. " + REFLECT_COMMAND), + ("assistant", "Here is the reflect write-up."), + ("user", "now fix the import"), + name="stale-window.jsonl", + ) + self.assertIsNotNone(detect.enqueue_judge( + {"transcript_path": path, "last_assistant_message": HIT_TEXT})) + + def test_mid_turn_narration_does_not_push_the_window_past_the_request(self): + """A turn writes many assistant rows: narration, then the reply. + + Starting the window after the previous assistant row cut the turn's + own opening message out of it, so the `/reflect` in that message was + never seen. + """ + path = self.write_transcript( + ("user", "that was wrong. " + REFLECT_COMMAND), + ("assistant", "Let me open the file first."), + ("assistant", HIT_TEXT), + name="mid-turn-narration.jsonl", + ) self.assertIsNone(detect.enqueue_judge({"transcript_path": path})) self.assertEqual(self.jobs(), []) + def test_a_subagents_reply_row_does_not_hide_the_turns_request(self): + """Sidechain rows land in the same file and used to move the window.""" + path = self.write_transcript( + ("user", "that was wrong. " + REFLECT_COMMAND), + ("assistant", "Spawning a subagent."), + ("sidechain-user", "go read the file"), + ("sidechain-assistant", "The live file is src/b.py."), + ("assistant", HIT_TEXT), + name="sidechain.jsonl", + ) + self.assertIsNone(detect.enqueue_judge({"transcript_path": path})) + self.assertEqual(self.jobs(), []) + + def test_a_tool_result_row_does_not_push_the_window_past_the_request(self): + """Claude Code files a tool result as a `type: "user"` row with no `isMeta`. + + Treating it as the person speaking made the turn's first tool call + the start of the window, so the `/reflect` typed at the top of the + turn sat before it and the hook nagged for a reflect already asked + for. + """ + path = self.write_transcript( + ("user", "that was wrong. " + REFLECT_COMMAND), + ("assistant", "Let me open the file first."), + ("tool_result", "def parse_args(argv):\n return argv[1]\n"), + ("assistant", HIT_TEXT), + name="tool-result.jsonl", + ) + self.assertIsNone(detect.enqueue_judge({"transcript_path": path})) + self.assertEqual(self.jobs(), []) + + def test_a_tool_result_quoting_reflect_is_not_a_request_for_one(self): + """Grepping the hook's own source prints the word; that is not an ask.""" + self.use_runners(SLOW_CLEAN) + path = self.write_transcript( + ("user", "grep the hook"), + ("assistant", "Running grep."), + ("tool_result", "detect.py:31:ALREADY_REFLECT_RE = re.compile(r\"/reflect\")"), + ("assistant", HIT_TEXT), + name="tool-result-prose.jsonl", + ) + self.assertIsNotNone(detect.enqueue_judge({"transcript_path": path})) + + def test_unreadable_transcript_reports_unchecked_instead_of_going_quiet(self): + missing = os.path.join(self.reflect_state.name, "does-not-exist.jsonl") + self.assertFalse(detect.user_already_asked_reflect(missing)) + directory = os.path.join(self.reflect_state.name, "a-directory.jsonl") + os.makedirs(directory, exist_ok=True) + err = io.StringIO() + with redirect_stderr(err): + rows = detect._transcript_roles(directory) + self.assertIsNone(rows) + self.assertIn("unchecked", err.getvalue()) + def test_claude_malformed_stdin_fail_open(self): err = io.StringIO() with patch.object(sys, "stdin", io.StringIO("not-json")): diff --git a/engine/skills/draft-pr/tests/test_draft_pr_scripts.py b/engine/skills/draft-pr/tests/test_draft_pr_scripts.py index 22b5b8a8..9b924d01 100644 --- a/engine/skills/draft-pr/tests/test_draft_pr_scripts.py +++ b/engine/skills/draft-pr/tests/test_draft_pr_scripts.py @@ -116,6 +116,16 @@ def _with_summary(summary: str) -> str: "engine/hooks/prove-it-ship-gate/detect.py", ] +FILES_WHOSE_STEM_IS_AN_ORDINARY_ENGLISH_WORD = [ + "engine/hooks/llm-judge/judge.py", + "engine/hooks/llm-judge/tests/test_judge.py", +] + +FILES_UNDER_A_SKILL_FOLDER = [ + "engine/skills/draft-pr/tests/test_draft_pr_scripts.py", + "docs/ecosystem.md", +] + CODE_NAME_ERROR = "Summary and Review Claim must not use code names" @@ -212,12 +222,98 @@ def test_hyphenated_english_words_are_not_code_names(self): self.assertEqual(result.returncode, 0, result.stderr + result.stdout) self.assertNotIn(CODE_NAME_ERROR, result.stderr) + def test_changed_file_stem_is_a_code_name_even_when_it_reads_like_english(self): + files = ["engine/hooks/wrong-check-reflect/detect.py", "tests/scenarios/self-correction.json"] + summary = ( + "The nudge for a review after a self-correction has never once spoken: " + "1,682 runs, zero. Four separate things silenced it, and each one alone " + "was enough to keep it quiet." + ) + result = _run_validator(self._engine_body(summary), files) + self.assertEqual(result.returncode, 1, result.stdout) + self.assertIn(CODE_NAME_ERROR, result.stderr) + self.assertIn('"self-correction" (changed file name)', result.stderr) + + def test_rewording_the_changed_file_stem_clears_the_failure(self): + files = ["engine/hooks/wrong-check-reflect/detect.py", "tests/scenarios/self-correction.json"] + summary = ( + "The nudge for a review after the assistant takes back a wrong claim has " + "never once spoken: 1,682 runs, zero. Four separate things silenced it, " + "and each one alone was enough to keep it quiet." + ) + result = _run_validator(self._engine_body(summary), files) + self.assertEqual(result.returncode, 0, result.stderr + result.stdout) + self.assertNotIn(CODE_NAME_ERROR, result.stderr) + def test_missing_summary_is_reported_unchecked_not_clean(self): body = VALID_BODY.replace(f"## Summary\n\n{VALID_SUMMARY}\n\n", "") self.assertNotIn("## Summary", body) result = _run_validator(body) self.assertIn("Code-name check unchecked: no ## Summary section to read", result.stderr) + def _engine_claim(self, claim: str) -> str: + return ENGINE_BODY.replace("Approve the null-check fix for the widget renderer.", claim) + + def test_review_claim_is_checked_too_not_just_summary(self): + """A changed file's stem fails even when it reads as plain English. + + PR #799 shipped "A judge runner that cannot answer ..." as its Review + Claim while changing judge.py. Every other code-name test here edits + the Summary, so nothing pinned the Review Claim half of + CODE_NAME_SECTIONS. Keep the word banned: loosening the rule for + words that look like English would let real file names through. + """ + result = _run_validator( + self._engine_claim("A judge runner that cannot answer is left out for six hours."), + FILES_WHOSE_STEM_IS_AN_ORDINARY_ENGLISH_WORD, + ) + self.assertEqual(result.returncode, 1, result.stdout) + self.assertIn(CODE_NAME_ERROR, result.stderr) + self.assertIn('Review Claim: "judge" (changed file name)', result.stderr) + + def test_review_claim_passes_once_the_changed_file_stem_is_gone(self): + result = _run_validator( + self._engine_claim( + "A wording-reviewer runner that cannot answer is left out for six hours." + ), + FILES_WHOSE_STEM_IS_AN_ORDINARY_ENGLISH_WORD, + ) + self.assertEqual(result.returncode, 0, result.stderr + result.stdout) + self.assertNotIn(CODE_NAME_ERROR, result.stderr) + + def test_review_claim_names_a_changed_folder_not_just_a_changed_file(self): + """A changed directory's own name is a code name too. + + PR #840 failed this gate with "keeps its checker under the draft-pr + skill" while changing a file under engine/skills/draft-pr/. Every + other code-name test here uses a changed *file* stem, so the folder + half of changedFileNames had no test at all. + """ + result = _run_validator( + self._engine_claim( + "The PR description guard checks PR text in a repo that keeps its " + "checker under the draft-pr skill, and says UNCHECKED instead of " + "staying silent when a PR is published from a repo with no checker." + ), + FILES_UNDER_A_SKILL_FOLDER, + ) + self.assertEqual(result.returncode, 1, result.stdout) + self.assertIn(CODE_NAME_ERROR, result.stderr) + self.assertIn('Review Claim: "draft-pr" (changed folder name)', result.stderr) + + def test_review_claim_passes_once_the_changed_folder_name_is_gone(self): + result = _run_validator( + self._engine_claim( + "The PR description guard checks PR text in a repo that keeps its " + "checker under the PR-drafting skill folder, and says UNCHECKED " + "instead of staying silent when a PR is published from a repo with " + "no checker." + ), + FILES_UNDER_A_SKILL_FOLDER, + ) + self.assertEqual(result.returncode, 0, result.stderr + result.stdout) + self.assertNotIn(CODE_NAME_ERROR, result.stderr) + def test_code_names_in_later_sections_do_not_fail(self): body = self._engine_body(AFTER_SUMMARY).replace( "- [x] `pytest tests/test_widget_renderer.py`", diff --git a/scripts/ci/check_hook_test_coverage.py b/scripts/ci/check_hook_test_coverage.py index 107d07a7..1accd201 100755 --- a/scripts/ci/check_hook_test_coverage.py +++ b/scripts/ci/check_hook_test_coverage.py @@ -21,7 +21,7 @@ import sys import tempfile -REPO_DIR = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) +REPO_DIR = os.path.dirname(os.path.dirname(os.path.dirname(os.path.realpath(__file__)))) HOOKS_DIR = os.path.join(REPO_DIR, "engine", "hooks") # Checked in this order -- "no_hit" must classify as negative before the diff --git a/scripts/ci/check_no_new_comments.py b/scripts/ci/check_no_new_comments.py index 97210b61..010231f9 100644 --- a/scripts/ci/check_no_new_comments.py +++ b/scripts/ci/check_no_new_comments.py @@ -16,7 +16,7 @@ import subprocess import sys -SCRIPTS_DIR = os.path.dirname(os.path.abspath(__file__)) +SCRIPTS_DIR = os.path.dirname(os.path.realpath(__file__)) REPO_ROOT = os.path.dirname(os.path.dirname(SCRIPTS_DIR)) sys.path.insert(0, os.path.join(REPO_ROOT, "engine", "hooks", "no-comments")) sys.path.insert(0, SCRIPTS_DIR) diff --git a/tests/scenarios/self-correction.json b/tests/scenarios/self-correction.json index 8d41bd49..d5886be4 100644 --- a/tests/scenarios/self-correction.json +++ b/tests/scenarios/self-correction.json @@ -19,11 +19,20 @@ }, { "name": "admission-skipped-when-user-already-said-reflect", - "situation": "Pins the documented skip: ALREADY_REFLECT_RE suppresses the follow-up when the user's own message already asked for /reflect, so the hook does not nag for something already in flight. Caught by writing the scenario above with the user's literal message, which contained /reflect.", - "user": "\"Claim I made was wrong\" is a trigger for /reflect", + "situation": "Pins the documented skip: the hook does not nag for a reflect the user has already asked for. The user's message must be an actual request. An earlier version of this scenario used a message that only mentioned /reflect while describing the rule, so it pinned the suppression against a sentence that never asked for anything; the mention case is now its own scenario below.", + "user": "reflect/reflecton this", "reply": "Also: a claim I made earlier was wrong. The coverage run compared against origin/main rather than the slice, so the pass was vacuous.", "expect_no_enqueue": [ "wrong-check-reflect" ] + }, + { + "name": "admission-still-caught-when-reflect-is-only-mentioned", + "situation": "The mention-is-not-a-use half of the same rule. Quoting the word /reflect while describing when it fires is not a request for one, so an admission in the same turn must still reach the judge. This is the exact text the previous scenario used to suppress on.", + "user": "\"Claim I made was wrong\" is a trigger for /reflect", + "reply": "Also: a claim I made earlier was wrong. The coverage run compared against origin/main rather than the slice, so the pass was vacuous.", + "expect_enqueue": [ + "wrong-check-reflect" + ] } ]