From 03544b43a3dd494ee1509006b61f92e9a07aa78f Mon Sep 17 00:00:00 2001 From: Matthew Maclay Date: Tue, 10 Nov 2026 19:37:29 -0700 Subject: [PATCH 1/2] fix(controlled-gate): ask only on a controlled file the check finds - Let a call through when the check cannot finish: a directory past the walk limit, a glob past its limit, an unreadable payload, a script failure or garbled output, or no python3 - Check the first 5000 files under a directory and the first 500 paths of a glob, still asking on a held file found among them - Flip the four tests that pinned the asks to pin the pass - Say so in the README's "Permissions", SECURITY.md and design.md Co-Authored-By: Claude Opus 5.5 --- README.md | 6 ++++-- SECURITY.md | 4 ++-- docs/design.md | 2 ++ hooks/controlled-gate.sh | 29 +++++++------------------- scripts/controlled_gate.py | 39 ++++++++++++----------------------- tests/test_controlled_gate.py | 35 ++++++++++++++++--------------- 6 files changed, 46 insertions(+), 69 deletions(-) diff --git a/README.md b/README.md index 34ba8e3..8e8fbff 100644 --- a/README.md +++ b/README.md @@ -386,8 +386,10 @@ the first line alone passes, and so do a Write that keeps that line, parts such `rm`, `find` without `-exec`, `git status` or `git commit -m`, and every command that does not name the file. It is there to catch a mistake, not to stop someone determined: a path built from a variable, a program that -opens a path the command does not name, and a recursive read from above `.review/` are not seen, -and the record is a file the session could write (`scripts/controlled_gate.py`). +opens a path the command does not name, a recursive read from above `.review/`, and files past +the first 5000 under one directory or 500 from one glob are not seen, and the record is a file the +session could write. It asks only on a file it found: a call it cannot read or check passes +(`scripts/controlled_gate.py`). ### Your responsibility diff --git a/SECURITY.md b/SECURITY.md index 1cc7551..04f9719 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -8,8 +8,8 @@ Kinds of report that matter most here: - a way past a gate: a push, a post, a commit or a Jira or Confluence write that happens without the person's yes, or past `hooks/atlassian-gate.sh`, `hooks/secret-gate.sh` or - `hooks/controlled-gate.sh` (a call the secret scan cannot read passing is by design, not a - vulnerability: the README's "Permissions" says what it reads); + `hooks/controlled-gate.sh` (a call the secret scan or the controlled-file check cannot read + passing is by design, not a vulnerability: the README's "Permissions" says what each reads); - a secret, credential or internal link that reaches a pull request, an issue or a page despite the redaction in `scripts/transcript_excerpt.py`; - a script that runs or writes outside the checkout it was given. diff --git a/docs/design.md b/docs/design.md index 290b2c6..d6063a4 100644 --- a/docs/design.md +++ b/docs/design.md @@ -131,6 +131,8 @@ Libera's standard. **The secret scan stops only what it finds.** `hooks/secret-gate.sh` once asked whenever it could not read a call: a command with an unclosed quote, a repository named by a variable, a message file it could not name, a scan past its time, a server whose kind it could not tell, or a machine with no `python3`. Skills run such commands in their ordinary steps, so the prompt came often, mostly over commands that carried nothing, and a skill-feedback issue (#57) called it a nuisance. A prompt people approve unread protects nothing, and the person stays responsible for what they commit and post, as with any other tool. So what the scan cannot read passes, and the rest of the call is still read: a credential found in the command's text, another commit or another file it sends is denied, naming where and what kind, even when part of the call could not be read. A command it cannot split is judged on its text when it mentions a commit or a `gh` write, since the shell may still run it, and a tool of a server it cannot place as GitHub's or Atlassian's is not scanned. It is a backstop for the commit or post that carries a key by mistake, not a review of every command. (`scripts/secret_scan.py`; the README, "Permissions".) +**The controlled-file check asks only on a file it finds.** `hooks/controlled-gate.sh` asked, for the same reason, whenever it could not finish: a `.review/` tree past its walk limit, a glob past its limit, a payload or script output it could not read, or a machine with no `python3`. Those asks named no item, so the person had nothing to judge and approved them unread. It now asks only when it has found a `.review/` file opening with `Controlled:`, naming the item and its warning; what it cannot reach passes, like the secret scan. (`scripts/controlled_gate.py`; the README, "Permissions".) + **The first merge changes nothing for a teammate who does not use the plugin.** The long pull request template, a version check requiring a bump, the measurement narrative and American spelling in libera_utils' pre-commit were kept out of it. The measurement narrative was dropped with the record store. **The corpus is private, beside public repositories.** `libera_utils` is public, and R-014 keeps internal links and internal document content out of it, while an archive entry says what went wrong, on which mission, and what the team decided. So the archive, the context links and the records live in the private `dev-tools`, and the code repository names its context entries rather than copying them. (The corpus README and the archive README in Libera's standard; R-014.) diff --git a/hooks/controlled-gate.sh b/hooks/controlled-gate.sh index db0db7c..e0b0646 100755 --- a/hooks/controlled-gate.sh +++ b/hooks/controlled-gate.sh @@ -3,32 +3,17 @@ # argument says which, `pre` or `post`. scripts/controlled_gate.py decides: before a call that reads # past the first line of a `.review/` file opening with `Controlled:`, it asks, naming the item and # its warning, unless the person cleared that item earlier in the session; after such a call ran, -# it records the item as cleared. When `pre` fails, or prints what is not a decision, the call is -# asked about, since nothing checked it. With no python3, a call whose input names `.review` is -# asked about. +# it records the item as cleared. It asks only on a held file it found: when the check cannot run, +# fails, or prints what is not a decision, the call passes. set -u -ask='{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"ask","permissionDecisionReason":"%s"}}\n' mode=${1:-pre} -input=$(cat) -command -v python3 >/dev/null 2>&1 || { - if [ "$mode" = pre ]; then - case "$input" in - *.review*) printf "$ask" "python3 is not installed, so the controlled-file check could not run on this call, which names .review. Allow it only if no file it reads opens with Controlled:." ;; - esac - fi - exit 0 -} +command -v python3 >/dev/null 2>&1 || exit 0 if [ "$mode" = post ]; then - printf '%s' "$input" | python3 "$(dirname "$0")/../scripts/controlled_gate.py" post >/dev/null 2>&1 + python3 "$(dirname "$0")/../scripts/controlled_gate.py" post >/dev/null 2>&1 exit 0 fi -out=$(printf '%s' "$input" | python3 "$(dirname "$0")/../scripts/controlled_gate.py" pre 2>/dev/null) -status=$? -if [ "$status" -ne 0 ]; then - printf "$ask" "The controlled-file check failed (exit $status), so it could not tell whether this call reads a .review file that opens with Controlled:. Allow it only if it reads no such file." -elif [ -n "$out" ] && ! printf '%s' "$out" | python3 -c 'import json,sys; d=json.load(sys.stdin); assert d["hookSpecificOutput"]["permissionDecision"]' 2>/dev/null; then - printf "$ask" "The controlled-file check printed something it could not read, so this call is asked about." -else - [ -n "$out" ] && printf '%s\n' "$out" +out=$(python3 "$(dirname "$0")/../scripts/controlled_gate.py" pre 2>/dev/null) || exit 0 +if [ -n "$out" ] && printf '%s' "$out" | python3 -c 'import json,sys; d=json.load(sys.stdin); assert d["hookSpecificOutput"]["permissionDecision"]' 2>/dev/null; then + printf '%s\n' "$out" fi exit 0 diff --git a/scripts/controlled_gate.py b/scripts/controlled_gate.py index db7863d..4de35e5 100755 --- a/scripts/controlled_gate.py +++ b/scripts/controlled_gate.py @@ -33,13 +33,13 @@ session's id replaces the record, so a new session asks again. It is a stop for a mistake, not a lock: a path built from a variable, a program that opens a path -the command does not name, and a recursive read from above `.review/` are not seen, and the record -is a file the session could write. +the command does not name, a recursive read from above `.review/`, files past the first WALK_LIMIT +under one directory and paths past the first GLOB_LIMIT of one glob are not seen, and the record +is a file the session could write. It asks only on a held file it found: a call it cannot read +passes. `pre` and `post` always exit 0. `pre` prints `ask` naming the held file and its first line, which -holds only the key and the warning, or `ask` when the payload cannot be read; otherwise nothing, -which lets the call through. hooks/controlled-gate.sh asks when `pre` fails or prints something it -cannot read. +holds only the key and the warning; otherwise nothing, which lets the call through. """ import glob import itertools @@ -51,14 +51,13 @@ MARK = "Controlled:" RECORD = "controlled-cleared.json" -WALK_LIMIT = 5000 # files looked at under one `.review/` before the check counts it as holding one -GLOB_LIMIT = 500 # paths one glob may expand to before the check asks +WALK_LIMIT = 5000 # files looked at under one directory +GLOB_LIMIT = 500 # paths looked at from one glob FIRST_LINE = (["-1"], ["-n", "1"], ["-n1"], ["--lines=1"]) QUIET = {"ls", "stat", "test", "du", "mkdir", "rmdir", "rm", "touch", "mv", "realpath", "echo", "printf", "pwd", "true", "false", "which", "basename", "dirname"} # show no file's text OPERATORS = {"&&", "||", ";", ";;", "|", "|&", "&", "(", ")", "\n"} REVIEW_PATH = re.compile(r"(?:[\w.~/+-]*/)?\.review(?:/[^\s'\"`;|&<>()]*)?(?![\w.-])", re.IGNORECASE) -TOO_MANY = "too many to check" def decide(reason): @@ -101,7 +100,7 @@ def held_under(directory): for name in names: seen += 1 if seen > WALK_LIMIT: - return found + [(directory, TOO_MANY)] + return found path = os.path.join(root, name) line = first_line(path) if line: @@ -192,17 +191,14 @@ def named(path): def paths_named(word, cwd): - """The paths a word may name, a glob expanded, or None when a glob expands past GLOB_LIMIT.""" + """The paths a word may name, a glob expanded to its first GLOB_LIMIT paths.""" found = [] for value in candidates(word) | {m.group(0) for m in REVIEW_PATH.finditer(word)}: if not value or "$" in value: continue path = os.path.join(cwd, os.path.expanduser(value)) if re.search(r"[*?[]", value): - paths = list(itertools.islice(glob.iglob(path, recursive=True), GLOB_LIMIT + 1)) - if len(paths) > GLOB_LIMIT: - return None - found += paths + found += itertools.islice(glob.iglob(path, recursive=True), GLOB_LIMIT) else: found.append(os.path.normpath(path)) return found @@ -216,10 +212,7 @@ def bash_reads(command, cwd): words = [w for ws, rs, xargs in parts if xargs or not quiet(ws, rs) for w in ws + [t for _, t in rs]] found = {} for word in words: - paths = paths_named(word, cwd) - if paths is None: - return [(os.path.join(cwd, word), TOO_MANY)] - for path in paths: + for path in paths_named(word, cwd): for held, line in named(path): found.setdefault(os.path.realpath(held), (held, line)) return list(found.values()) @@ -264,7 +257,7 @@ def cleared(path, session): def record(found, session): by_file = {} for path, line in found: - if line != TOO_MANY and record_path(path): + if record_path(path): by_file.setdefault(record_path(path), set()).add(item_of(line)) for target, items in by_file.items(): keys = sorted(cleared(target, session) | items) @@ -276,11 +269,6 @@ def record(found, session): def reason(found, cwd): - for path, line in found: - if line == TOO_MANY: - return (f"{path} holds more files than the controlled-file check can look at, so it " - "cannot tell whether one opens with Controlled:. Allow this only if it reads no " - "such file.") lines = sorted({line for _, line in found}) items = ", ".join(item_of(line) for line in lines) shown = "; ".join(f"{os.path.relpath(path, cwd) if cwd else path} opens with \"{line}\"" @@ -308,10 +296,9 @@ def pre(text): try: tool, inp, cwd, session = payload(text) except (ValueError, AttributeError): - decide("The tool call could not be read, so the controlled-file check could not run.") return found = [(path, line) for path, line in reads(tool, inp, cwd) - if line == TOO_MANY or item_of(line) not in cleared(path, session)] + if item_of(line) not in cleared(path, session)] if found: decide(reason(found, cwd)) diff --git a/tests/test_controlled_gate.py b/tests/test_controlled_gate.py index f984375..57b079e 100644 --- a/tests/test_controlled_gate.py +++ b/tests/test_controlled_gate.py @@ -206,26 +206,27 @@ def test_a_held_file_named_in_another_case_is_held_where_the_file_system_ignores self.asks("Read", {"file_path": upper}) self.asks("Bash", {"command": f"cat {upper}"}) - def test_too_many_files_to_check_asks(self): + def test_past_the_walk_limit_only_held_files_found_ask(self): sys.path.insert(0, str(ROOT / "scripts")) import controlled_gate as gate many = make_repo(self.tmp / "many", held=False) for i in range(6): (many / ".review" / f"f{i}.md").write_text("x\n") + payload = json.dumps({"tool_name": "Bash", "tool_input": {"command": "grep -r x .review"}, + "cwd": str(many), "session_id": "s1"}) old = gate.WALK_LIMIT gate.WALK_LIMIT = 3 try: - [(_, line)] = gate.reads("Bash", {"command": "grep -r x .review"}, str(many)) - self.assertEqual(line, gate.TOO_MANY) - payload = json.dumps({"tool_name": "Bash", "tool_input": {"command": "grep -r x .review"}, - "cwd": str(many), "session_id": "s1"}) + self.assertEqual(gate.reads("Bash", {"command": "grep -r x .review"}, str(many)), []) out = io.StringIO() with contextlib.redirect_stdout(out): gate.pre(payload) - self.assertIn("more files than the controlled-file check can look at", - json.loads(out.getvalue())["hookSpecificOutput"]["permissionDecisionReason"]) - gate.post(payload) - self.assertFalse((many / ".review" / gate.RECORD).exists()) + self.assertEqual(out.getvalue(), "") + gate.WALK_LIMIT = 100 + (many / ".review" / "held.md").write_text("Controlled: PROJ-1 — CUI\nx\n") + with contextlib.redirect_stdout(out): + gate.pre(payload) + self.assertEqual(json.loads(out.getvalue())["hookSpecificOutput"]["permissionDecision"], "ask") finally: gate.WALK_LIMIT = old @@ -293,20 +294,20 @@ def test_a_record_that_cannot_be_read_clears_nothing(self): record.write_text(text) self.asks("Read", {"file_path": HELD}) - # Failing closed + # A call the check cannot read passes - def test_an_unreadable_payload_asks(self): + def test_an_unreadable_payload_passes(self): for payload in ("not json", json.dumps({"tool_name": "Read", "tool_input": {"file_path": 7}, "cwd": str(self.repo)}), json.dumps({"tool_name": "Bash", "tool_input": {"command": ["cat", HELD]}, "cwd": str(self.repo)})): with self.subTest(payload=payload): for cmd in ([sys.executable, str(SCRIPT), "pre"], [str(GATE), "pre"]): out = subprocess.run(cmd, input=payload, capture_output=True, text=True, check=True).stdout - self.assertEqual(json.loads(out)["hookSpecificOutput"]["permissionDecision"], "ask") + self.assertEqual(out, "") out = subprocess.run([str(GATE), "post"], input=payload, capture_output=True, text=True, check=True).stdout self.assertEqual(out, "") - def test_a_crash_or_a_garbled_answer_in_the_script_asks(self): + def test_a_crash_or_a_garbled_answer_in_the_script_passes(self): for body in ("raise RuntimeError('boom')", "print('not a decision')"): with self.subTest(body=body): copy = Path(tempfile.mkdtemp(dir=self.tmp)) @@ -317,7 +318,7 @@ def test_a_crash_or_a_garbled_answer_in_the_script_asks(self): payload = json.dumps({"tool_name": "Read", "tool_input": {"file_path": "x"}, "cwd": str(self.repo)}) out = subprocess.run([str(copy / "hooks" / "controlled-gate.sh"), "pre"], input=payload, capture_output=True, text=True, check=True).stdout - self.assertEqual(json.loads(out)["hookSpecificOutput"]["permissionDecision"], "ask") + self.assertEqual(out, "") def test_the_script_runs_alone_with_no_other_script_beside_it(self): alone = Path(tempfile.mkdtemp(dir=self.tmp)) @@ -340,17 +341,17 @@ def test_the_hook_runs_the_script_both_ways(self): self.ran("Read", {"file_path": HELD}, script=False) self.passes("Read", {"file_path": HELD}, script=False) - def test_with_no_python3_a_call_naming_review_asks_and_the_rest_pass(self): + def test_with_no_python3_every_call_passes(self): bin_dir = self.tmp / "bin" bin_dir.mkdir() - for tool in ("bash", "cat", "dirname"): + for tool in ("bash", "dirname"): (bin_dir / tool).symlink_to(shutil.which(tool)) def run(inp, event="pre"): payload = json.dumps({"tool_name": "Read", "tool_input": inp, "cwd": str(self.repo)}) out = subprocess.run([str(bin_dir / "bash"), str(GATE), event], input=payload, capture_output=True, text=True, env={"PATH": str(bin_dir)}, check=True).stdout return json.loads(out)["hookSpecificOutput"]["permissionDecision"] if out.strip() else None - self.assertEqual(run({"file_path": ".review/plain.md"}), "ask") + self.assertIsNone(run({"file_path": HELD})) self.assertIsNone(run({"file_path": "src/outside.md"})) self.assertIsNone(run({"file_path": ".review/plain.md"}, event="post")) From 792ee8a8d65577dfd3076034dc264515a33886ff Mon Sep 17 00:00:00 2001 From: Matthew Maclay Date: Tue, 10 Nov 2026 19:43:15 -0700 Subject: [PATCH 2/2] test(controlled-gate): pin held files found past the walk and glob limits - Assert a held file reached before WALK_LIMIT still asks, and a glob past GLOB_LIMIT over held files still asks - Say in the docstring that the hook passes when pre fails and records nothing when post fails - Word design.md's entry as a fact about the code Co-Authored-By: Claude Opus 5.5 --- docs/design.md | 2 +- scripts/controlled_gate.py | 5 +++-- tests/test_controlled_gate.py | 39 +++++++++++++++++++++-------------- 3 files changed, 27 insertions(+), 19 deletions(-) diff --git a/docs/design.md b/docs/design.md index d6063a4..1b62a3f 100644 --- a/docs/design.md +++ b/docs/design.md @@ -131,7 +131,7 @@ Libera's standard. **The secret scan stops only what it finds.** `hooks/secret-gate.sh` once asked whenever it could not read a call: a command with an unclosed quote, a repository named by a variable, a message file it could not name, a scan past its time, a server whose kind it could not tell, or a machine with no `python3`. Skills run such commands in their ordinary steps, so the prompt came often, mostly over commands that carried nothing, and a skill-feedback issue (#57) called it a nuisance. A prompt people approve unread protects nothing, and the person stays responsible for what they commit and post, as with any other tool. So what the scan cannot read passes, and the rest of the call is still read: a credential found in the command's text, another commit or another file it sends is denied, naming where and what kind, even when part of the call could not be read. A command it cannot split is judged on its text when it mentions a commit or a `gh` write, since the shell may still run it, and a tool of a server it cannot place as GitHub's or Atlassian's is not scanned. It is a backstop for the commit or post that carries a key by mistake, not a review of every command. (`scripts/secret_scan.py`; the README, "Permissions".) -**The controlled-file check asks only on a file it finds.** `hooks/controlled-gate.sh` asked, for the same reason, whenever it could not finish: a `.review/` tree past its walk limit, a glob past its limit, a payload or script output it could not read, or a machine with no `python3`. Those asks named no item, so the person had nothing to judge and approved them unread. It now asks only when it has found a `.review/` file opening with `Controlled:`, naming the item and its warning; what it cannot reach passes, like the secret scan. (`scripts/controlled_gate.py`; the README, "Permissions".) +**The controlled-file check asks only on a file it finds.** `hooks/controlled-gate.sh` asked, as the secret scan once did, whenever it could not finish: a `.review/` tree past its walk limit, a glob past its limit, a payload or script output it could not read, or a machine with no `python3`. Those asks named no item, so the person had nothing to judge and approved them unread. So it asks only when it has found a `.review/` file opening with `Controlled:`, naming the item and its warning, and what it cannot reach passes. (`scripts/controlled_gate.py`; the README, "Permissions".) **The first merge changes nothing for a teammate who does not use the plugin.** The long pull request template, a version check requiring a bump, the measurement narrative and American spelling in libera_utils' pre-commit were kept out of it. The measurement narrative was dropped with the record store. diff --git a/scripts/controlled_gate.py b/scripts/controlled_gate.py index 4de35e5..8e8926c 100755 --- a/scripts/controlled_gate.py +++ b/scripts/controlled_gate.py @@ -38,8 +38,9 @@ is a file the session could write. It asks only on a held file it found: a call it cannot read passes. -`pre` and `post` always exit 0. `pre` prints `ask` naming the held file and its first line, which -holds only the key and the warning; otherwise nothing, which lets the call through. +`pre` prints `ask` naming the held file and its first line, which holds only the key and the +warning; otherwise nothing, which lets the call through. hooks/controlled-gate.sh lets a call +through when `pre` fails; when `post` fails, nothing is recorded. """ import glob import itertools diff --git a/tests/test_controlled_gate.py b/tests/test_controlled_gate.py index 57b079e..2c5a157 100644 --- a/tests/test_controlled_gate.py +++ b/tests/test_controlled_gate.py @@ -206,29 +206,36 @@ def test_a_held_file_named_in_another_case_is_held_where_the_file_system_ignores self.asks("Read", {"file_path": upper}) self.asks("Bash", {"command": f"cat {upper}"}) - def test_past_the_walk_limit_only_held_files_found_ask(self): + def test_past_the_walk_and_glob_limits_only_held_files_found_ask(self): sys.path.insert(0, str(ROOT / "scripts")) import controlled_gate as gate many = make_repo(self.tmp / "many", held=False) + top = many / ".review" / "many" + (top / "sub").mkdir(parents=True) for i in range(6): - (many / ".review" / f"f{i}.md").write_text("x\n") - payload = json.dumps({"tool_name": "Bash", "tool_input": {"command": "grep -r x .review"}, - "cwd": str(many), "session_id": "s1"}) - old = gate.WALK_LIMIT - gate.WALK_LIMIT = 3 - try: - self.assertEqual(gate.reads("Bash", {"command": "grep -r x .review"}, str(many)), []) + (top / "sub" / f"f{i}.md").write_text("x\n") + + def decision(command): out = io.StringIO() with contextlib.redirect_stdout(out): - gate.pre(payload) - self.assertEqual(out.getvalue(), "") - gate.WALK_LIMIT = 100 - (many / ".review" / "held.md").write_text("Controlled: PROJ-1 — CUI\nx\n") - with contextlib.redirect_stdout(out): - gate.pre(payload) - self.assertEqual(json.loads(out.getvalue())["hookSpecificOutput"]["permissionDecision"], "ask") + gate.pre(json.dumps({"tool_name": "Bash", "tool_input": {"command": command}, + "cwd": str(many), "session_id": "s1"})) + return json.loads(out.getvalue())["hookSpecificOutput"]["permissionDecision"] if out.getvalue() else None + + old = gate.WALK_LIMIT, gate.GLOB_LIMIT + gate.WALK_LIMIT, gate.GLOB_LIMIT = 3, 2 + try: + self.assertIsNone(decision("grep -r x .review/many")) + self.assertIsNone(decision("cat .review/many/sub/*.md")) + (top / "held.md").write_text("Controlled: PROJ-1 — CUI\nx\n") + self.assertEqual(decision("grep -r x .review/many"), "ask") + self.assertEqual(decision("cat .review/many/*.md"), "ask") + (top / "held").mkdir() + for i in range(4): + (top / "held" / f"h{i}.md").write_text(f"Controlled: PROJ-{i} — CUI\nx\n") + self.assertEqual(decision("cat .review/many/held/*.md"), "ask") finally: - gate.WALK_LIMIT = old + gate.WALK_LIMIT, gate.GLOB_LIMIT = old # Clearance, per item and per session