Carry the Permission Rules the Review Loop Needs in the Installer

host-setup/agent-safety/README.md

@@ -10,6 +10,7 @@ Into `~/.claude/` (or `%USERPROFILE%\.claude\` on Windows):
 - **Two marker-delimited blocks of host-wide content in `CLAUDE.md`**, loaded into every session on the machine (including ad-hoc work outside any project), installed and updated independently so one can change without rewriting the other.
   - The `agent-safety` block carries restrictions alone, so nothing in it can widen a permission. `Repository Boundaries and Write Safety` mirrors the committed `GOVERNANCE.md` section of the same name, carrying its write rules and its rule that a task works in its own checkout as behavioral guidance. `Data in Agent-Authored Text` mirrors `GOVERNANCE.md` "Representative Data in Agent-Authored Text", so an agent illustrates with constructed data rather than data observed on the machine. `Authorization Scope and Memory Hygiene` bounds how a granted permission is recorded and read. Each mirrors a rule that otherwise reaches fleet repos only.
   - The `fleet-bootstrap` block carries `Fleet Governance Entry Point`, which names the template repository and routes by the state a repository is actually in. It is separate precisely because it enables rather than restricts, and the safety block's own text says nothing in it widens a permission, so merging the two would contradict that. It is host-wide rather than per repository because the repositories that most need it are the ones carrying no instruction set to point the way, and it mirrors the byte-locked `AGENTS.md` "Fleet Bootstrap" section that a conformant repository carries.
+- **The permission rules this kit owns, merged into `settings.json`** beside the hook registration. Each is declared as a prefix and a rule, and a re-run drops every rule the prefix owns before adding the current one, so a rule whose spelling changes updates in place rather than accumulating beside the version it replaced. Ownership requires a rule-syntax delimiter after the prefix, since the prefix ends at the script name and a bare prefix test would also claim a longer path such as `pr_review.py-custom`, so a rule written by hand for a different script is never touched. These widen rather than restrict, which is why they are their own component for the same reason the `fleet-bootstrap` block is separate from the `agent-safety` one. Today the list holds one rule, for `scripts/pr_review.py`, the review loop's reply and resolve. Driving that loop by hand needs a raw GraphQL mutation carrying a node id, which is the shape that reached a stranger's repository, where the script queries the id itself and takes no argument an id fits in. What the rule decides is which command runs without a prompt, and it matches the command text rather than the directory the command runs in, so it reaches a `scripts/pr_review.py` in any checkout that carries one. An absolute path would not narrow that, since the hub is reached as a checkout of the caller's own and its location differs per task, so pinning one path would name a checkout the next task does not use. What bounds it is the rule that an agent reaches the hub as a checkout of its own, fetched immediately before it is read, rather than a copy it happens to find on disk, which the `fleet-bootstrap` block beside this carries and [`GOVERNANCE.md`][governance] "Hub-Hosted Tooling" states in full.
 
 The hook is the mechanical backstop. The CLAUDE.md rules and the carried GOVERNANCE.md rules are the behavioral layer. Prose alone is not enough, since the incident happened under prose rules, so both ship. The GitHub write rules have a hook behind them, as do the git operations that bypass a repository protection, while which checkout a command belongs in, the data a line of text quotes, and how an authorization is recorded are prose only, since no hook can see any of the three.
 
@@ -25,7 +26,7 @@ host-setup/agent-safety/install.sh
 .\host-setup\agent-safety\install.ps1
 ```
 
-Both are thin wrappers around `install.py`, so every OS runs one tested code path. The installer self-tests the hook before registering it, merges the settings.json entry without clobbering other keys, and updates each CLAUDE.md block in place by its own markers rather than duplicating it, so the two blocks move independently.
+Both are thin wrappers around `install.py`, so every OS runs one tested code path. The installer self-tests the hook before registering it, merges the settings.json hook entry and the permission rules without clobbering other keys, and updates each CLAUDE.md block in place by its own markers rather than duplicating it, so the two blocks move independently. The settings file is read once and written once, so the hook and the permission rules land together or not at all.
 
 **Restart Claude Code sessions on the machine afterward** so the new hook and CLAUDE.md load.
 
@@ -39,6 +40,7 @@ The deployed copy on each machine is a snapshot, so when the guard changes upstr
 python3 ~/.claude/hooks/gh-write-guard.py --selftest    # decision matrix: all cases pass
 grep -c 'agent-safety v' ~/.claude/CLAUDE.md            # expect 2 (start + end marker)
 grep -c 'fleet-bootstrap v' ~/.claude/CLAUDE.md         # expect 2 (start + end marker)
+grep -cF 'Bash(python3 scripts/pr_review.py:*)' ~/.claude/settings.json   # expect 1 (never duplicated)
 ```
 
 On Windows PowerShell:
@@ -47,6 +49,7 @@ On Windows PowerShell:
 py -3 "$env:USERPROFILE\.claude\hooks\gh-write-guard.py" --selftest    # all cases pass
 (Select-String 'agent-safety v' "$env:USERPROFILE\.claude\CLAUDE.md").Count      # expect 2
 (Select-String 'fleet-bootstrap v' "$env:USERPROFILE\.claude\CLAUDE.md").Count   # expect 2
+(Select-String -SimpleMatch 'Bash(python3 scripts/pr_review.py:*)' "$env:USERPROFILE\.claude\settings.json").Count   # expect 1
 ```
 
 Live end-to-end (in any repo): attempt a discarded-output write and confirm the Bash tool is blocked:
@@ -65,10 +68,15 @@ The installer writes this. It is here so you can inspect or hand-place it:
     "PreToolUse": [
       { "matcher": "Bash", "hooks": [ { "type": "command", "command": "\"python3\" \"<home>/.claude/hooks/gh-write-guard.py\"" } ] }
     ]
+  },
+  "permissions": {
+    "allow": [ "Bash(python3 scripts/pr_review.py:*)" ]
   }
 }
 ```
 
+Every other key in the file is left as it stands, `permissions.allow` included, apart from the rules whose prefix this kit owns.
+
 ## Scope and Limits
 
 - **Per-machine.** `~/.claude/` does not travel, so run the installer on each box. This is the rollout that [#365][issue-365] tracks.
@@ -80,4 +88,5 @@ The installer writes this. It is here so you can inspect or hand-place it:
 - **Not a credential control.** A fine-grained PAT limited to owned repositories is a separate, stronger structural guard (a hard `403` on any non-owned repo) and is left to per-machine credential setup, out of this kit.
 
 <!-- Repo -->
+[governance]: ../../GOVERNANCE.md
 [issue-365]: https://github.com/ptr727/ProjectTemplate/issues/365

host-setup/agent-safety/install.py

@@ -1,8 +1,9 @@
 #!/usr/bin/env python3
 """Install the agent write-safety kit for the current user account. Cross-platform, idempotent.
 
-Deploys the PreToolUse hook, registers it in the user settings.json, adds the safety rules to the user
-CLAUDE.md (marker-delimited so re-runs update in place), and self-tests the hook before registering it.
+Deploys the PreToolUse hook, registers it in the user settings.json, merges the permission rules this
+kit owns into the same file, adds the safety rules to the user CLAUDE.md (marker-delimited so re-runs
+update in place), and self-tests the hook before registering it.
 The bash and PowerShell wrappers both call this, so every OS runs one tested code path.
 
 Usage: python3 install.py            (installs to ~/.claude)
@@ -18,6 +19,42 @@
 
 HERE = pathlib.Path(__file__).resolve().parent
 
+# Distinguishes an absent key from one holding an explicit null, which `dict.get` reports alike.
+# The two need different answers, since a gap is filled and a null is a settings error.
+MISSING = object()
+
+
+def owns(entry, prefix):
+    """Whether an allow rule names the script the prefix identifies, rather than a longer path.
+
+    The prefix ends at the script name, so a bare `startswith` also claims `pr_review.py-custom`,
+    and dropping that would delete a hand-written rule for a different script. What separates the
+    two is the character after the name: a rule that invokes this script continues with a rule-syntax
+    delimiter, where a different script continues with more of its own path.
+    """
+    return entry.startswith(prefix) and entry[len(prefix):len(prefix) + 1] in (":", " ", ")")
+
+
+def at(data, path):
+    """The value at a slash-separated key path, or MISSING where any step of it is absent."""
+    node = data
+    for part in path.split("/"):
+        if not isinstance(node, dict) or part not in node:
+            return MISSING
+        node = node[part]
+    return node
+
+# Permission rules this kit installs, each as (owned prefix, rule).
+# A re-run drops every rule the prefix owns before adding the current one, so a changed rule updates in place.
+# Ownership needs a delimiter after the prefix, so a longer path such as `pr_review.py-custom` is not claimed.
+# These widen rather than restrict, so they stay their own step for the reason the two CLAUDE.md blocks stay separate.
+MANAGED_PERMISSIONS = [
+    # The review loop's reply and resolve, the one write in that loop an agent performs.
+    # Driving it by hand needs a raw GraphQL mutation carrying a node id, which is the shape to avoid.
+    # The rule decides which command skips a prompt, and it bounds no checkout, since it matches the text.
+    ("Bash(python3 scripts/pr_review.py", "Bash(python3 scripts/pr_review.py:*)"),
+]
+
 
 def hook_launcher():
     """A python invocation for the settings.json command. Prefer a bare `python3` (portable and
@@ -61,15 +98,63 @@ def main():
     launcher = hook_launcher()
     # Quote the launcher too: the sys.executable fallback can contain spaces (e.g. C:\Program Files\...).
     hook_cmd = f'"{launcher}" "{hook_dst}"'
+    # Read into a variable rather than twice off disk, once to test for content and once to parse.
+    # Two reads can also disagree, since another process may write between them.
     data = {}
-    if settings.exists() and settings.read_text(encoding="utf-8").strip():
+    raw = settings.read_text(encoding="utf-8") if settings.exists() else ""
+    if raw.strip():
         try:
-            data = json.loads(settings.read_text(encoding="utf-8"))
+            data = json.loads(raw)
         except json.JSONDecodeError as e:
             sys.stderr.write(
                 f"{settings} exists but is not valid JSON ({e}). Fix or remove it, then re-run.\n"
             )
             return 1
+    # A settings file is an object, and any other JSON value parses cleanly and breaks every lookup below.
+    # The root is therefore checked before the keys under it are.
+    if not isinstance(data, dict):
+        sys.stderr.write(
+            f"{settings} is valid JSON but holds {type(data).__name__} at its root where an object "
+            "is required. Fix or remove it, then re-run. This file is unchanged, so the hook is "
+            "deployed but not registered.\n"
+        )
+        return 1
+    # Every container this installer descends into is checked before it is used.
+    # A key holding an unexpected type would otherwise raise a traceback mid-edit.
+    # That reads as a crash rather than as the settings problem it is.
+    # The invalid-JSON refusal above is the shape this file already answers a malformed file with.
+    def reject(where, held, want):
+        sys.stderr.write(
+            f"{settings} has `{where}` as {type(held).__name__} where {want.__name__} is required. "
+            "Fix or remove that key, then re-run. This file is unchanged, so the hook is deployed "
+            "but not registered.\n"
+        )
+
+    for path, want in (("hooks", dict), ("hooks/PreToolUse", list),
+                       ("permissions", dict), ("permissions/allow", list)):
+        held = at(data, path)
+        # An explicit null is present rather than absent, and `setdefault` hands back the null it found.
+        # It is therefore rejected here rather than read as a gap the default fills.
+        if held is not MISSING and not isinstance(held, want):
+            reject(path.replace("/", "."), held, want)
+            return 1
+
+    # A list of the right type can still hold the wrong elements.
+    # The registration below reads each group as an object, and each group's `hooks` as a list it appends to.
+    groups = at(data, "hooks/PreToolUse")
+    if groups is not MISSING:
+        for i, g in enumerate(groups):
+            if not isinstance(g, dict):
+                reject(f"hooks.PreToolUse[{i}]", g, dict)
+                return 1
+            if "hooks" in g and not isinstance(g["hooks"], list):
+                reject(f"hooks.PreToolUse[{i}].hooks", g["hooks"], list)
+                return 1
+            for j, h in enumerate(g.get("hooks") or []):
+                if not isinstance(h, dict):
+                    reject(f"hooks.PreToolUse[{i}].hooks[{j}]", h, dict)
+                    return 1
+
     pre = data.setdefault("hooks", {}).setdefault("PreToolUse", [])
     # Strip the hook from every existing group first, so a re-run leaves no duplicate behind.
     # That matters when settings.json already carries more than one Bash group.
@@ -83,10 +168,40 @@ def main():
         group = {"matcher": "Bash", "hooks": []}
         pre.append(group)
     group.setdefault("hooks", []).append({"type": "command", "command": hook_cmd})
+    done = ["PreToolUse/Bash hook registered"]
+
+    # 3. Permission rules, merged under the prefixes this installer owns.
+    # The strip-then-register shape is the hook registration's above, applied to a flat list.
+    # Written in the same pass as the hook, so the file is read once and written once.
+    allow = data.setdefault("permissions", {}).setdefault("allow", [])
+    for prefix, rule in MANAGED_PERMISSIONS:
+        matched = [a for a in allow if isinstance(a, str) and owns(a, prefix)]
+        allow[:] = [a for a in allow if a not in matched] + [rule]
+        # Counted over what the write removes rather than over what the prefix matched.
+        # The current rule matches its own prefix, so a match-set count reports it as superseded.
+        # A duplicate of it is removed too, and both can happen at once, so both are named.
+        older = [a for a in matched if a != rule]
+        duplicates = max(0, len(matched) - len(older) - 1)
+        changes = []
+        if older:
+            changes.append(f"superseding {len(older)}")
+        if duplicates:
+            changes.append(f"removing {duplicates} duplicate" + ("s" if duplicates > 1 else ""))
+        if not matched:
+            action = "added"
+        elif changes:
+            action = "updated, " + " and ".join(changes)
+        else:
+            action = "already current"
+        done.append(f"permission {rule}: {action}")
+
+    # Reported after the write rather than as each edit is made, since both edits share one write.
+    # A line printed before it claims a change that a later failure would leave unmade.
     settings.write_text(json.dumps(data, indent=2) + "\n", encoding="utf-8")
-    print(f"  settings -> {settings} (PreToolUse/Bash hook registered)")
+    for line in done:
+        print(f"  settings -> {settings} ({line})")
 
-    # 3. CLAUDE.md carries one marker block per snippet, replaced where present and appended where not.
+    # 4. CLAUDE.md carries one marker block per snippet, replaced where present and appended where not.
     # The two blocks install and update independently, so one can change without rewriting the other.
     # The safety block states restrictions only.
     # The fleet block enables, so it stays separate from a block whose own text says nothing in it widens a permission.
@@ -113,6 +228,10 @@ def main():
     print(f"  {launcher} \"{hook_dst}\" --selftest")
     print(f"  grep -c 'agent-safety v' \"{claude_md}\"      # expect 2")
     print(f"  grep -c 'fleet-bootstrap v' \"{claude_md}\"   # expect 2")
+    # One line per rule, matching the rule itself rather than a word inside it.
+    # A hint naming a fixed word would stop matching the moment a rule that lacks it is added.
+    for _, rule in MANAGED_PERMISSIONS:
+        print(f"  grep -cF '{rule}' \"{settings}\"   # expect 1")
     print("Restart Claude Code sessions on this machine so the hook and CLAUDE.md load.")
     return 0