From d28a2ff05a64d6cbd9a9f58d90773b9ebafecfd2 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 20 Jul 2026 09:21:50 -0700 Subject: [PATCH 01/17] Add Repository Boundaries and Write Safety rules; fix the runbook footgun (#364/#366) An autonomous PR-review loop hand-fabricated a GraphQL thread node id and fired a reply mutation with output suppressed (>/dev/null 2>&1 || true); because node ids resolve globally, the guessed id landed on a real thread in a stranger's repo and the write succeeded under the maintainer's identity, while the suppressed output read as a harmless failure. New AGENTS.md section "Repository Boundaries and Write Safety" (after Foundational Principles, before Git and Commit Rules), three rules: write only to the project's own origin, never fabricate/guess/reuse an id passed to a write, a write is never a probe and its output is never suppressed. Added to the AGENTS.md carried-sections allowlist in spec/files.json so it travels with the fleet. copilot-instructions.md carried the footgun: the reply/resolve example used a disembodied -F threadId="PRRT_..." literal disconnected from the reviewThreads(first:100) query above it. Rewritten to capture $TID from that live query with an empty-result guard, use -F threadId="$TID", show the mutation output, and confirm isResolved before closing. Added an intra-doc cross-reference to the new AGENTS.md section. The requestReviews example already captured its ids from live queries and needed no change. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/copilot-instructions.md | 27 +++++++++++++++++++++++---- AGENTS.md | 8 ++++++++ spec/files.json | 2 +- 3 files changed, 32 insertions(+), 5 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index ec55c76b..500a9a7d 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -119,6 +119,8 @@ If a review did not run on the current head, retry: ### Reply and Thread Resolution Workflow +Every id below is captured from a live query into a variable and passed from there - never hand-typed, guessed, or pasted as a `PRRT_...` literal. A node id resolves globally, so a fabricated or stale id does not fail, it writes to a real thread on an unrelated repository. This runbook implements [AGENTS.md "Repository Boundaries and Write Safety"](../AGENTS.md#repository-boundaries-and-write-safety): write only to this repo, capture every id from a live query, and never suppress a mutation's output. + List unresolved threads. Use `first: 100` with cursor-based pagination; if `hasNextPage` is true, re-run with `after: ""` to retrieve the next page: ```sh @@ -142,20 +144,37 @@ gh api graphql -f query=' ' ``` -Reply on a thread, then resolve it: +Reply on a thread, then resolve it. Capture the target thread's id into `$TID` from the listing query above - filter to the thread being answered by its `path` (and, when a file carries more than one thread, its first-comment body), and guard for an empty result so a mutation never runs on a guessed id: ```sh +TID=$(gh api graphql -f query=' +{ + repository(owner: "", name: "") { + pullRequest(number: ) { + reviewThreads(first: 100) { + nodes { id isResolved path comments(first: 1) { nodes { body } } } + } + } + } +}' --jq '.data.repository.pullRequest.reviewThreads.nodes[] + | select(.isResolved == false and .path == "") + | .id' | head -n 1) +[ -n "$TID" ] || { echo "no matching unresolved thread on - do not guess an id" >&2; return 1 2>/dev/null || exit 1; } + +# Show the mutation's output; never append >/dev/null, 2>&1, or || true to a write. gh api graphql -f query=' mutation($threadId: ID!, $body: String!) { addPullRequestReviewThreadReply(input: { pullRequestReviewThreadId: $threadId, body: $body }) { - comment { id } + comment { id url } } -}' -F threadId="PRRT_..." -F body="Fixed in : ." +}' -F threadId="$TID" -F body="Fixed in : ." +# Confirm isResolved: true in this response before treating the thread as closed - a write that +# appears to fail may have taken on the server. gh api graphql -f query=' mutation($threadId: ID!) { resolveReviewThread(input: { threadId: $threadId }) { thread { id isResolved } } -}' -F threadId="PRRT_..." +}' -F threadId="$TID" ``` Issue-level Copilot comments (those in `issues//comments`) have no resolution action - GitHub provides no API or UI to resolve them. Reply if the finding warrants it; no resolution step is needed or possible. diff --git a/AGENTS.md b/AGENTS.md index 253b97e9..ba9893b6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -15,6 +15,14 @@ The specific rules in this file implement a few governing principles. Read these - **Two version numbers, two jobs.** The 2-digit `major.minor` in `version.json` carries human meaning - the maintainer raises it only for a functional change (feature, behavior or API change, breaking change), at their discretion - while NBGV owns the patch position and always increments with git height, so every build is uniquely versioned with no edit. Human-facing docs name the 2-digit line; the toolchain guarantees monotonic builds. See "Release Model". - **Contracts state what, not how, and favor reuse.** [`WORKFLOW.md`](./WORKFLOW.md) fixes required outcomes, not a required implementation - two repos may satisfy a guarantee with different YAML. Within that freedom, apply good engineering practice: minimize duplication and maximize reuse, which is why the pipeline splits a carried, generic orchestration layer from a repo-owned build layer. +## Repository Boundaries and Write Safety + +A state-changing GitHub call is the highest-blast-radius thing an agent does here: it runs under the maintainer's identity, so one wrong target writes to another owner's repository as the maintainer - an outward-facing, hard-to-reverse act. These rules bound every write - a git push, an API mutation, a comment, a label, a merge - on any platform. Reads are unrestricted. The bounds below are on writes. + +- **Write only to the current project's own repository.** Every state-changing call targets this project's `origin` and nothing else. A broad or logged-in identity is capability, not permission - a token that *can* reach another repository does not authorize writing to it. Writing to any other repository needs explicit, per-session human permission for that specific repository, and a "harmless test" write is still a write, so there is no probe exception. Reads from anywhere are fine. +- **Never fabricate, guess, or reuse an identifier passed to a write.** Every id a state-changing call consumes - a node id, a numeric id, a thread or comment id - is captured from a live query in the **same** session into a variable and passed from there. Do not hand-type an id, guess it, recall it from memory or an earlier session, or copy it from documentation or an example. Ids commonly resolve **globally**, so a wrong-but-valid id does not fail - it writes to the wrong target, in someone else's repository. If a query returns no id, stop rather than invent one to proceed. +- **A write is never a probe, and a write's output is never suppressed.** Never fire a state-changing call to see whether it works: decide it should happen, make it happen, and read the result. Never append output-discarding redirection or a force-success tail to a mutation (`>/dev/null`, `2>&1`, `|| true`, `|| echo`) - the write's output is exactly what must be read. A write that appears to fail is **verified, not assumed harmless** - the operation may have succeeded on the server while the client reported an error - so confirm the actual state before retrying or moving on. + ## Git and Commit Rules - **Default to staging, not committing.** Stage changes with `git add` and leave `git commit` to the developer unless the developer has explicitly authorized the agent to commit for the current ask ("commit this", "open a PR", etc.). Authorization is scope-bound - it covers the commits needed for that specific task, not a blanket commit license for the rest of the session. diff --git a/spec/files.json b/spec/files.json index b622105d..fb9760c3 100644 --- a/spec/files.json +++ b/spec/files.json @@ -2,7 +2,7 @@ "$schema": "./files.schema.json", "note": "The standardization baseline: files and sections a fleet repo is expected to carry, and their intent authority. The audit checks presence (letter) and equivalence (intent); a section for an absent language or target is N/A.", "baseline": [ - { "path": "AGENTS.md", "sections": ["Git and Commit Rules", "Branching Model", "Release Model", "Pull Request Title and Commit Message Conventions", "Documentation Style Conventions", "PR Review Etiquette", "Workflow YAML Conventions"], "intentRef": "AGENTS.md", "appliesTo": "*" }, + { "path": "AGENTS.md", "sections": ["Repository Boundaries and Write Safety", "Git and Commit Rules", "Branching Model", "Release Model", "Pull Request Title and Commit Message Conventions", "Documentation Style Conventions", "PR Review Etiquette", "Workflow YAML Conventions"], "intentRef": "AGENTS.md", "appliesTo": "*" }, { "path": "CODESTYLE.md", "whole": true, "placeholders": ["InternalsVisibleTo project names"], "intentRef": "CODESTYLE.md", "appliesTo": "*" }, { "path": "WORKFLOW.md", "whole": true, "intentRef": "WORKFLOW.md", "appliesTo": "*" }, { "path": "README.md", "appliesTo": "*" }, From e0c1ecc2cf0297f6f9f91fd009ca5806a6b6f03f Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 20 Jul 2026 09:34:20 -0700 Subject: [PATCH 02/17] Add portable per-machine agent write-safety kit (#365) The behavioral rules in AGENTS.md and CLAUDE.md are necessary but not sufficient - the incident happened under prose rules. This kit adds the mechanical backstop and makes both deployable on every machine in one idempotent step, so a new system is protected as its first setup action. host-setup/agent-safety/ (a coordinator asset, not carried to fleet repos): - gh-write-guard.py: a PreToolUse hook that DENIES three GitHub-write footguns - a state-changing gh call whose output is suppressed, a GraphQL mutation passing a literal node id instead of a $variable, and a gh write whose explicit target is outside the checkout's origin. Reads and non-writes pass through. It fires even in autonomous / bypass-permissions sessions, which is how the incident happened. Precision over recall: it denies the specific dangerous shapes with a clear reason, never gating legitimate work. --selftest runs an 11-case decision matrix. - claude-md-safety.md: the same three rules as host-scope guidance, marker-delimited, appended into ~/.claude/CLAUDE.md so every session on a machine inherits them (including ad-hoc work outside any project). - install.py + thin install.sh / install.ps1 wrappers: one tested cross-platform installer. Deploys and self-tests the hook before registering it, merges the PreToolUse entry into settings.json without clobbering other keys, and updates the CLAUDE.md block in place. Fully idempotent (re-run to update; same-version re-run and version-upgrade replace-in-place both verified). Pins the kit's shebang-executable .py to LF in .editorconfig and .gitattributes (same rule as spec/audit.py); a nested markdownlint config exempts the appended snippet from MD041 (it opens at H2 by design). Verified: hook self-test passes; the real stdin deny path emits the documented hookSpecificOutput/permissionDecision:deny for the incident command and passes reads through; installer idempotency and replace-in-place tested against throwaway homes; editorconfig-checker and markdownlint (whole-repo) clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- .editorconfig | 6 + .gitattributes | 2 + .../agent-safety/.markdownlint-cli2.jsonc | 8 + host-setup/agent-safety/README.md | 78 +++++++ host-setup/agent-safety/claude-md-safety.md | 23 ++ host-setup/agent-safety/gh-write-guard.py | 196 ++++++++++++++++++ host-setup/agent-safety/install.ps1 | 18 ++ host-setup/agent-safety/install.py | 91 ++++++++ host-setup/agent-safety/install.sh | 10 + 9 files changed, 432 insertions(+) create mode 100644 host-setup/agent-safety/.markdownlint-cli2.jsonc create mode 100644 host-setup/agent-safety/README.md create mode 100644 host-setup/agent-safety/claude-md-safety.md create mode 100644 host-setup/agent-safety/gh-write-guard.py create mode 100644 host-setup/agent-safety/install.ps1 create mode 100644 host-setup/agent-safety/install.py create mode 100755 host-setup/agent-safety/install.sh diff --git a/.editorconfig b/.editorconfig index 2038d6fa..464d489c 100644 --- a/.editorconfig +++ b/.editorconfig @@ -63,6 +63,12 @@ end_of_line = lf [spec/{validate,audit}.py] end_of_line = lf +# The agent-safety kit's Python is shebang-executable tooling run by path (the PreToolUse hook and its +# installer), so pin LF for the same reason as the entry points above - a CRLF shebang breaks direct +# execution on a Unix host. +[host-setup/agent-safety/*.py] +end_of_line = lf + # uv regenerates uv.lock with LF on every platform, so pin it or an EOL check (editorconfig-checker/CI) # reds on every `uv lock`/`uv sync` until the file is manually reconverted - same rationale as the # shebang/Dockerfile pins (a tool owns the ending). A Python repo on the CRLF default carries this; a repo diff --git a/.gitattributes b/.gitattributes index 126849d2..2a14e795 100644 --- a/.gitattributes +++ b/.gitattributes @@ -18,6 +18,8 @@ catalog/snippets/husky/pre-commit text eol=lf # here the CI validation entry point; do not re-add a blanket `*.py text eol=lf`. spec/validate.py text eol=lf spec/audit.py text eol=lf +host-setup/agent-safety/gh-write-guard.py text eol=lf +host-setup/agent-safety/install.py text eol=lf # uv regenerates uv.lock with LF on every platform; pin it so git enforces LF on checkout/renormalize and a # CRLF-default repo does not fight the tool on every `uv lock`/`uv sync`. A repo with no lockfile is unaffected. diff --git a/host-setup/agent-safety/.markdownlint-cli2.jsonc b/host-setup/agent-safety/.markdownlint-cli2.jsonc new file mode 100644 index 00000000..8eb5a70a --- /dev/null +++ b/host-setup/agent-safety/.markdownlint-cli2.jsonc @@ -0,0 +1,8 @@ +{ + // claude-md-safety.md is a fragment the installer appends into ~/.claude/CLAUDE.md (which already + // has its own H1), so it intentionally opens at H2. MD041 (first line must be a top-level heading) + // does not apply to an appended snippet. This nested config affects only this directory. + "config": { + "MD041": false + } +} diff --git a/host-setup/agent-safety/README.md b/host-setup/agent-safety/README.md new file mode 100644 index 00000000..cbb1ef04 --- /dev/null +++ b/host-setup/agent-safety/README.md @@ -0,0 +1,78 @@ +# Agent write-safety kit + +Per-machine, user-account-scoped guards against an agent making a mis-targeted GitHub **write** under the +maintainer's identity. Deploy it as the **first thing on any new system** where Claude Code runs with the +`gh` credentials logged in (WSL, Linux, macOS, Proxmox, Windows). + +## What it installs + +Into `~/.claude/` (or `%USERPROFILE%\.claude\` on Windows): + +- **`hooks/gh-write-guard.py`** - a PreToolUse hook that denies the three write footguns behind the + cross-repo comment incident: a state-changing `gh` call whose output is suppressed, a GraphQL mutation + passing a **literal** node id instead of a `$variable`, and a `gh` write whose explicit target is + outside the checkout's `origin`. Reads and everything else pass through. It fires even in autonomous / + bypass-permissions sessions - which is how the incident happened. +- **A `## GitHub write safety` section in `CLAUDE.md`** - the same three rules as behavioral guidance, + loaded into every session on the machine (including ad-hoc work outside any project). It mirrors the + committed `AGENTS.md` "Repository Boundaries and Write Safety" rules, which only reach fleet repos. + +The hook is the mechanical backstop; the CLAUDE.md rules and the carried AGENTS.md rules are the +behavioral layer. Prose alone is not enough - the incident happened under prose rules - so both ship. + +## Install (idempotent - safe to re-run to update) + +```sh +# Linux / WSL / macOS / Proxmox +host-setup/agent-safety/install.sh +``` + +```powershell +# Windows +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 the CLAUDE.md block in place (marker-delimited) rather than duplicating it. + +**Restart Claude Code sessions on the machine afterward** so the new hook and CLAUDE.md load. + +## Verify + +```sh +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) +``` + +Live end-to-end (in any repo): attempt a suppressed-output write and confirm the Bash tool is blocked: + +```sh +gh api graphql -f query='mutation{noop}' -F t="PRRT_x" >/dev/null 2>&1 || true # blocked by the hook +``` + +## Manual settings.json shape (for reference) + +The installer writes this; it is here so you can inspect or hand-place it: + +```json +{ + "hooks": { + "PreToolUse": [ + { "matcher": "Bash", "hooks": [ { "type": "command", "command": "python3 \"/.claude/hooks/gh-write-guard.py\"" } ] } + ] + } +} +``` + +## Scope and limits + +- **Per-machine.** `~/.claude/` does not travel; run the installer on each box. This is the rollout that + ptr727/ProjectTemplate#365 tracks. +- **Precision over recall.** The hook denies the specific dangerous shapes with high confidence rather + than gating every write, so it never blocks legitimate work. A shape it does not catch still falls + under the behavioral rules. It cannot see the target behind an opaque GraphQL node id (that is why + rule 2 blocks a *literal* id at all - a captured `$variable` is trusted). +- **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. diff --git a/host-setup/agent-safety/claude-md-safety.md b/host-setup/agent-safety/claude-md-safety.md new file mode 100644 index 00000000..4e027eba --- /dev/null +++ b/host-setup/agent-safety/claude-md-safety.md @@ -0,0 +1,23 @@ + +## GitHub write safety (any project, every session) + +A `gh` / GitHub API write runs under the logged-in identity, so a mis-targeted write acts publicly as +that account on someone else's repository - outward-facing and hard to reverse. These rules bound every +write (a git push, an API mutation, a comment, a label, a merge) in every session on this machine, +including ad-hoc work outside any project. Reads are unrestricted. A committed repo's `AGENTS.md` +"Repository Boundaries and Write Safety" states the same rules for its fleet; the two are kept in sync +deliberately, because this file also covers sessions that `AGENTS.md` never reaches. The +`gh-write-guard` PreToolUse hook enforces the mechanical half. + +- **Write only to the current project's own repository.** Every state-changing call targets this + checkout's `origin` and nothing else. A broad or logged-in identity is capability, not permission. + Another repository needs explicit, per-session human permission for that specific repository, and a + "harmless test" write is still a write. +- **Never fabricate, guess, or reuse an identifier passed to a write.** Every id a write consumes (a + node id, a numeric id, a thread or comment id) is captured from a live query in the same session into + a variable and passed from there. Ids resolve globally, so a wrong-but-valid id does not fail - it + writes to the wrong target in another repository. If a query returns no id, stop rather than invent one. +- **A write is never a probe, and a write's output is never suppressed.** Never fire a state-changing + call to see whether it works, and never append `>/dev/null`, `2>&1`, or `|| true` to a mutation. A + write that appears to fail is verified, not assumed harmless - it may have succeeded on the server. + diff --git a/host-setup/agent-safety/gh-write-guard.py b/host-setup/agent-safety/gh-write-guard.py new file mode 100644 index 00000000..c5487aef --- /dev/null +++ b/host-setup/agent-safety/gh-write-guard.py @@ -0,0 +1,196 @@ +#!/usr/bin/env python3 +"""PreToolUse guard: deny the GitHub-write footguns behind the cross-repo comment incident. + +Registered as a Claude Code PreToolUse hook on the Bash tool. It reads the tool-input JSON on stdin, +classifies the command, and DENIES (with a reason shown to the agent) when a command is a GitHub *write* +matching a known-dangerous pattern. Reads and everything that is not a clear write pass through. + +Precision over recall by design: it denies the specific shapes that caused the incident, not everything +it cannot parse. A false deny would break the agent; a missed case still falls under the AGENTS.md +"Repository Boundaries and Write Safety" prose rules. The three denied shapes: + + 1. a state-changing gh call whose output is suppressed (>/dev/null, 2>&1, || true, || echo) + 2. a GraphQL mutation passing a literal GitHub node id (PRRT_/PR_/BOT_/...) instead of a $variable + 3. a gh write with an explicit -R/--repo/repos// target outside the checkout's origin + +Run `gh-write-guard.py --selftest` to verify the decision matrix without Claude Code. +""" +import json +import os +import re +import subprocess +import sys + +# --- What counts as a GitHub write ------------------------------------------------------------------- +# gh subcommands that mutate. `gh api` is handled separately (it needs field/method inspection). +_GH_WRITE_SUB = re.compile( + r"""\bgh\s+(?: + pr\s+(?:create|comment|close|merge|edit|review|reopen|ready|lock|unlock) + | issue\s+(?:create|comment|close|edit|reopen|delete|lock|unlock|pin|unpin|transfer) + | release\s+(?:create|edit|delete|upload) + | repo\s+(?:create|delete|edit|rename|archive) + | (?:label|secret|variable|ruleset)\s+(?:create|delete|edit|set) + | gist\s+(?:create|edit|delete) + )\b""", + re.X, +) +_GH_API = re.compile(r"\bgh\s+api\b") +_EXPLICIT_WRITE_METHOD = re.compile(r"(?:--method|-X)\s+(?:POST|PUT|PATCH|DELETE)\b", re.I) +# gh api with a field flag defaults to POST even without -X, so it is a write. +_API_FIELD_FLAG = re.compile(r"(?:^|\s)(?:-f|-F|--field|--raw-field|--input)\b") +_GRAPHQL = re.compile(r"\bgh\s+api\b.*\bgraphql\b", re.S) +_MUTATION = re.compile(r"\bmutation\b") +_GIT_PUSH = re.compile(r"\bgit\s+push\b") + +# --- Risk-pattern detectors -------------------------------------------------------------------------- +_SUPPRESS = re.compile(r">\s*/dev/null|&>\s*/dev/null|2>&1|\|\|\s*(?:true|:|echo)\b") +# A GitHub global node id literal: an uppercase-ish prefix + underscore + base64url body, or legacy MDxx. +_NODE_ID_LITERAL = re.compile(r'^(?:[A-Za-z]{1,6}_[A-Za-z0-9_\-]{6,}|MD[A-Za-z0-9]{6,})$') +# -F/-f name=VALUE (captures the value; handles "quoted" and bare) +_FIELD_ASSIGN = re.compile(r"""(?:-F|-f|--field|--raw-field)\s+[A-Za-z_][\w]*=(?P'[^']*'|"[^"]*"|\S+)""") +_EXPLICIT_REPO = re.compile(r"(?:-R|--repo)\s+(?P[^\s'\"]+)") +_API_REPO_PATH = re.compile(r"\bgh\s+api\b[^\n|]*?\brepos/(?P[A-Za-z0-9_.\-]+)/(?P[A-Za-z0-9_.\-]+)") + + +def _is_gh_write(cmd): + if _GH_WRITE_SUB.search(cmd) or _GIT_PUSH.search(cmd): + return True + if _GH_API.search(cmd): + if _EXPLICIT_WRITE_METHOD.search(cmd): + return True + if _GRAPHQL.search(cmd) and _MUTATION.search(cmd): + return True + if _API_FIELD_FLAG.search(cmd) and not _GRAPHQL.search(cmd): + return True # gh api -f k=v => POST + return False + + +def _origin_owner_repo(cwd): + try: + url = subprocess.run( + ["git", "-C", cwd or ".", "remote", "get-url", "origin"], + capture_output=True, text=True, timeout=5, + ).stdout.strip() + except Exception: + return None + m = re.search(r"[:/]([A-Za-z0-9_.\-]+)/([A-Za-z0-9_.\-]+?)(?:\.git)?/?$", url) + return (m.group(1).lower(), m.group(2).lower()) if m else None + + +def classify(cmd, cwd=None, origin=None): + """Return (decision, reason). decision is 'allow' or 'deny'. + + origin, when given, is a (owner, repo) tuple used instead of resolving from cwd - the self-test + passes it for a deterministic, offline run. + """ + if not _is_gh_write(cmd): + return "allow", "" + + # 1. suppressed output on a write + if _SUPPRESS.search(cmd): + return "deny", ( + "This is a GitHub write with its output discarded (>/dev/null, 2>&1, || true). " + "A write's result is exactly what must be read: a mutation can succeed on the server " + "while the client reports an error. Run it without the output-discarding tail and read " + "the response. See AGENTS.md 'Repository Boundaries and Write Safety'." + ) + + # 2. literal node id in a mutation + if _GRAPHQL.search(cmd) and _MUTATION.search(cmd): + for m in _FIELD_ASSIGN.finditer(cmd): + val = m.group("v").strip("'\"") + if val.startswith("$") or val.startswith("${"): + continue + if _NODE_ID_LITERAL.match(val): + return "deny", ( + f"This mutation passes a literal GitHub node id ({val[:16]}...) instead of a " + "variable captured from a live query. Node ids resolve globally, so a fabricated " + "or stale id writes to a real object in another repository. Capture the id from a " + "query in this session into a variable and pass -F ...=\"$VAR\". See AGENTS.md " + "'Repository Boundaries and Write Safety'." + ) + + # 3. explicit target outside origin + if origin is None: + origin = _origin_owner_repo(cwd) + targets = [] + mr = _EXPLICIT_REPO.search(cmd) + if mr and "/" in mr.group("r") and "<" not in mr.group("r"): + o, r = mr.group("r").split("/", 1) + targets.append((o.lower(), r.lower())) + for m in _API_REPO_PATH.finditer(cmd): + if "<" not in m.group("owner"): + targets.append((m.group("owner").lower(), m.group("repo").lower())) + if origin: + for t in targets: + if t != origin: + return "deny", ( + f"This write targets {t[0]}/{t[1]}, which is not this checkout's origin " + f"({origin[0]}/{origin[1]}). Write only to the current project's own repository. " + "Another repository needs explicit per-session permission. See AGENTS.md " + "'Repository Boundaries and Write Safety'." + ) + + return "allow", "" + + +# --- Self-test --------------------------------------------------------------------------------------- +_CASES = [ + # (command, expected_decision, label) + ("gh api graphql -f query='mutation($t:ID!){addPullRequestReviewThreadReply(input:{pullRequestReviewThreadId:$t,body:\"x\"}){comment{id}}}' -F t=\"PRRT_kwDODvuuzM6SFvx0\" >/dev/null 2>&1 || true", "deny", "the incident: suppressed + literal id"), + ("gh api graphql -f query='mutation($t:ID!){resolveReviewThread(input:{threadId:$t}){thread{isResolved}}}' -F t=\"PRRT_kwDOabc123def\"", "deny", "literal node id in a mutation"), + ("gh api graphql -f query='mutation($t:ID!){resolveReviewThread(input:{threadId:$t}){thread{isResolved}}}' -F t=\"$TID\"", "allow", "mutation with captured $TID"), + ("gh issue comment 5 -R mankatcheung/job-finder --body \"hi\"", "deny", "cross-origin explicit -R"), + ("gh pr create --title x --body y >/dev/null 2>&1", "deny", "suppressed gh pr create"), + ("gh api repos/ptr727/PlexCleaner/issues/1/comments -f body=\"ok\"", "allow", "gh api POST to origin"), + ("gh api graphql -f query='{repository(owner:\"o\",name:\"r\"){pullRequest(number:1){reviewThreads(first:100){nodes{id}}}}}'", "allow", "graphql READ query"), + ("gh pr view 5 --json reviews", "allow", "gh pr view (read)"), + ("return 1 2>/dev/null || exit 1", "allow", "shell guard, not a gh write"), + ("git push origin develop", "allow", "normal push (no suppression, no cross-repo)"), + ("git commit -m 'x' && git push >/dev/null 2>&1", "deny", "push with suppressed output"), +] + + +def _selftest(): + # Deterministic offline run: pin origin to ptr727/PlexCleaner (the incident repo) so the + # cross-origin case resolves without touching a real checkout. + origin = ("ptr727", "plexcleaner") + ok = True + for cmd, want, label in _CASES: + got, _ = classify(cmd, origin=origin) + mark = "ok " if got == want else "FAIL" + if got != want: + ok = False + print(f" {mark} [{got:5}] want={want:5} {label}") + print("SELFTEST PASS" if ok else "SELFTEST FAIL") + return 0 if ok else 1 + + +# --- Hook entrypoint (PreToolUse) -------------------------------------------------------------------- +def _main(): + try: + data = json.load(sys.stdin) + except Exception: + sys.exit(0) # not our event shape; do not interfere + if data.get("tool_name") != "Bash": + sys.exit(0) + cmd = (data.get("tool_input") or {}).get("command", "") + cwd = data.get("cwd") or os.getcwd() + decision, reason = classify(cmd, cwd) + if decision == "deny": + # Documented PreToolUse deny contract (confirm field names against current docs before shipping). + print(json.dumps({ + "hookSpecificOutput": { + "hookEventName": "PreToolUse", + "permissionDecision": "deny", + "permissionDecisionReason": reason, + } + })) + sys.exit(0) + sys.exit(0) + + +if __name__ == "__main__": + if "--selftest" in sys.argv: + sys.exit(_selftest()) + _main() diff --git a/host-setup/agent-safety/install.ps1 b/host-setup/agent-safety/install.ps1 new file mode 100644 index 00000000..ac0f3fe6 --- /dev/null +++ b/host-setup/agent-safety/install.ps1 @@ -0,0 +1,18 @@ +# Thin wrapper: run the cross-platform installer with the available python (Windows). +# All logic lives in install.py so every OS runs one tested code path. Idempotent; safe to re-run. +# .\install.ps1 +# $env:CLAUDE_HOME = "C:\path"; .\install.ps1 # override the target (testing) +$ErrorActionPreference = "Stop" +$here = Split-Path -Parent $MyInvocation.MyCommand.Path +$script = Join-Path $here "install.py" + +if (Get-Command "python" -ErrorAction SilentlyContinue) { + & python $script @args +} elseif (Get-Command "python3" -ErrorAction SilentlyContinue) { + & python3 $script @args +} elseif (Get-Command "py" -ErrorAction SilentlyContinue) { + & py -3 $script @args +} else { + Write-Error "Python is required and was not found on PATH (tried python, python3, py)." + exit 1 +} diff --git a/host-setup/agent-safety/install.py b/host-setup/agent-safety/install.py new file mode 100644 index 00000000..ce84610c --- /dev/null +++ b/host-setup/agent-safety/install.py @@ -0,0 +1,91 @@ +#!/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. +The bash and PowerShell wrappers both call this, so every OS runs one tested code path. + +Usage: python3 install.py (installs to ~/.claude) + CLAUDE_HOME=/x python3 install.py (override target, for testing) +""" +import json +import os +import pathlib +import re +import shutil +import subprocess +import sys + +HERE = pathlib.Path(__file__).resolve().parent + + +def hook_launcher(): + """A python invocation for the settings.json command. Prefer a bare name on PATH (portable across + machines), else this interpreter's absolute path.""" + for name in ("python3", "python"): + if shutil.which(name): + return name + return sys.executable + + +def main(): + claude_home = pathlib.Path(os.environ.get("CLAUDE_HOME", pathlib.Path.home() / ".claude")) + hooks_dir = claude_home / "hooks" + hook_dst = hooks_dir / "gh-write-guard.py" + settings = claude_home / "settings.json" + claude_md = claude_home / "CLAUDE.md" + + print(f"Installing agent write-safety kit into: {claude_home}") + hooks_dir.mkdir(parents=True, exist_ok=True) + + # 1. Deploy the hook and self-test it BEFORE wiring anything up. + shutil.copyfile(HERE / "gh-write-guard.py", hook_dst) + try: + os.chmod(hook_dst, 0o755) + except OSError: + pass + print(f" hook -> {hook_dst}") + r = subprocess.run([sys.executable, str(hook_dst), "--selftest"], capture_output=True, text=True) + if r.returncode != 0: + sys.stderr.write("Hook self-test FAILED; aborting before registration.\n" + r.stdout + r.stderr) + return 1 + print(" hook self-test: PASS") + + # 2. Register in settings.json: exactly one PreToolUse/Bash group carrying our hook command. + launcher = hook_launcher() + hook_cmd = f'{launcher} "{hook_dst}"' + data = {} + if settings.exists() and settings.read_text(encoding="utf-8").strip(): + data = json.loads(settings.read_text(encoding="utf-8")) + pre = data.setdefault("hooks", {}).setdefault("PreToolUse", []) + group = next((g for g in pre if g.get("matcher") == "Bash"), None) + if group is None: + group = {"matcher": "Bash", "hooks": []} + pre.append(group) + entries = group.setdefault("hooks", []) + entries[:] = [h for h in entries if "gh-write-guard" not in str(h.get("command", ""))] + entries.append({"type": "command", "command": hook_cmd}) + settings.write_text(json.dumps(data, indent=2) + "\n", encoding="utf-8") + print(f" settings -> {settings} (PreToolUse/Bash hook registered)") + + # 3. CLAUDE.md: replace the agent-safety marker block if present, else append it. + snippet = (HERE / "claude-md-safety.md").read_text(encoding="utf-8").strip() + existing = claude_md.read_text(encoding="utf-8") if claude_md.exists() else "" + block_re = re.compile(r".*?", re.S) + if block_re.search(existing): + updated, action = block_re.sub(lambda _: snippet, existing), "updated" + else: + sep = "" if existing == "" or existing.endswith("\n\n") else ("\n" if existing.endswith("\n") else "\n\n") + updated, action = existing + sep + snippet + "\n", "appended" + claude_md.write_text(updated, encoding="utf-8") + print(f" CLAUDE.md -> {claude_md} (safety block {action})") + + print("\nDone. Verify:") + print(f" {launcher} \"{hook_dst}\" --selftest") + print(f" grep -c 'agent-safety v' \"{claude_md}\" # expect 2") + print("Restart Claude Code sessions on this machine so the hook and CLAUDE.md load.") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/host-setup/agent-safety/install.sh b/host-setup/agent-safety/install.sh new file mode 100755 index 00000000..c60e3ad5 --- /dev/null +++ b/host-setup/agent-safety/install.sh @@ -0,0 +1,10 @@ +#!/usr/bin/env bash +# Thin wrapper: run the cross-platform installer with the available python (Linux / WSL / macOS / Proxmox). +# All logic lives in install.py so every OS runs one tested code path. Idempotent; safe to re-run. +# ./install.sh installs to ~/.claude +# CLAUDE_HOME=/x ./install.sh overrides the target (testing) +set -Eeuo pipefail +here="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +py="$(command -v python3 || command -v python || true)" +[ -n "$py" ] || { echo "python3 is required and was not found on PATH." >&2; exit 1; } +exec "$py" "$here/install.py" "$@" From 3c5fe714560040c612ffefdee35b46808962adb5 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 20 Jul 2026 09:45:18 -0700 Subject: [PATCH 03/17] Fix 2>&1 suppression detection, quote the launcher, house-style the kit docs (Copilot #367) Correctness: - Bare 2>&1 merges stderr into stdout and leaves output visible, so it is not suppression - denying it would break `cmd 2>&1 | tee log`. The _SUPPRESS regex now matches only real discards (>/dev/null, &>/dev/null, 2>/dev/null, || true/:/echo); the incident's >/dev/null 2>&1 still denies via >/dev/null. Added three cases (bare 2>&1 -> allow x2, stderr-discard -> deny); 14/14 pass. Wording corrected in AGENTS.md, claude-md-safety.md, and the hook docstring/reason. - install.py quotes the launcher in the settings command, so a sys.executable fallback path with spaces does not break invocation. Docs: rewrote README.md and claude-md-safety.md in house style (one logical paragraph per line, no clause-joining semicolons). Documented that the cross-origin check only runs when origin resolves - a non-git directory or a node-id target is covered by rules 1-2 - in both the README limits and a code comment. Co-Authored-By: Claude Opus 4.8 (1M context) --- AGENTS.md | 2 +- host-setup/agent-safety/README.md | 38 ++++++--------------- host-setup/agent-safety/claude-md-safety.md | 22 +++--------- host-setup/agent-safety/gh-write-guard.py | 16 ++++++--- host-setup/agent-safety/install.py | 3 +- 5 files changed, 30 insertions(+), 51 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ba9893b6..f4dd74e2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -21,7 +21,7 @@ A state-changing GitHub call is the highest-blast-radius thing an agent does her - **Write only to the current project's own repository.** Every state-changing call targets this project's `origin` and nothing else. A broad or logged-in identity is capability, not permission - a token that *can* reach another repository does not authorize writing to it. Writing to any other repository needs explicit, per-session human permission for that specific repository, and a "harmless test" write is still a write, so there is no probe exception. Reads from anywhere are fine. - **Never fabricate, guess, or reuse an identifier passed to a write.** Every id a state-changing call consumes - a node id, a numeric id, a thread or comment id - is captured from a live query in the **same** session into a variable and passed from there. Do not hand-type an id, guess it, recall it from memory or an earlier session, or copy it from documentation or an example. Ids commonly resolve **globally**, so a wrong-but-valid id does not fail - it writes to the wrong target, in someone else's repository. If a query returns no id, stop rather than invent one to proceed. -- **A write is never a probe, and a write's output is never suppressed.** Never fire a state-changing call to see whether it works: decide it should happen, make it happen, and read the result. Never append output-discarding redirection or a force-success tail to a mutation (`>/dev/null`, `2>&1`, `|| true`, `|| echo`) - the write's output is exactly what must be read. A write that appears to fail is **verified, not assumed harmless** - the operation may have succeeded on the server while the client reported an error - so confirm the actual state before retrying or moving on. +- **A write is never a probe, and a write's output is never suppressed.** Never fire a state-changing call to see whether it works: decide it should happen, make it happen, and read the result. Never append output-discarding redirection or a force-success tail to a mutation (`>/dev/null`, `&>/dev/null`, `|| true`, `|| echo`) - the write's output is exactly what must be read. A write that appears to fail is **verified, not assumed harmless** - the operation may have succeeded on the server while the client reported an error - so confirm the actual state before retrying or moving on. ## Git and Commit Rules diff --git a/host-setup/agent-safety/README.md b/host-setup/agent-safety/README.md index cbb1ef04..41689f10 100644 --- a/host-setup/agent-safety/README.md +++ b/host-setup/agent-safety/README.md @@ -1,24 +1,15 @@ # Agent write-safety kit -Per-machine, user-account-scoped guards against an agent making a mis-targeted GitHub **write** under the -maintainer's identity. Deploy it as the **first thing on any new system** where Claude Code runs with the -`gh` credentials logged in (WSL, Linux, macOS, Proxmox, Windows). +Per-machine, user-account-scoped guards against an agent making a mis-targeted GitHub **write** under the maintainer's identity. Deploy it as the **first thing on any new system** where Claude Code runs with the `gh` credentials logged in (WSL, Linux, macOS, Proxmox, Windows). ## What it installs Into `~/.claude/` (or `%USERPROFILE%\.claude\` on Windows): -- **`hooks/gh-write-guard.py`** - a PreToolUse hook that denies the three write footguns behind the - cross-repo comment incident: a state-changing `gh` call whose output is suppressed, a GraphQL mutation - passing a **literal** node id instead of a `$variable`, and a `gh` write whose explicit target is - outside the checkout's `origin`. Reads and everything else pass through. It fires even in autonomous / - bypass-permissions sessions - which is how the incident happened. -- **A `## GitHub write safety` section in `CLAUDE.md`** - the same three rules as behavioral guidance, - loaded into every session on the machine (including ad-hoc work outside any project). It mirrors the - committed `AGENTS.md` "Repository Boundaries and Write Safety" rules, which only reach fleet repos. +- **`hooks/gh-write-guard.py`** - a PreToolUse hook that denies the three write footguns behind the cross-repo comment incident: a state-changing `gh` call whose output is discarded, a GraphQL mutation passing a **literal** node id instead of a `$variable`, and a `gh` write whose explicit target is outside the checkout's `origin`. Reads and everything else pass through. It fires even in autonomous / bypass-permissions sessions, which is how the incident happened. +- **A `## GitHub write safety` section in `CLAUDE.md`** - the same three rules as behavioral guidance, loaded into every session on the machine (including ad-hoc work outside any project). It mirrors the committed `AGENTS.md` "Repository Boundaries and Write Safety" rules, which only reach fleet repos. -The hook is the mechanical backstop; the CLAUDE.md rules and the carried AGENTS.md rules are the -behavioral layer. Prose alone is not enough - the incident happened under prose rules - so both ship. +The hook is the mechanical backstop. The CLAUDE.md rules and the carried AGENTS.md rules are the behavioral layer. Prose alone is not enough - the incident happened under prose rules - so both ship. ## Install (idempotent - safe to re-run to update) @@ -32,9 +23,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 the CLAUDE.md block in place (marker-delimited) rather than duplicating it. +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 the CLAUDE.md block in place (marker-delimited) rather than duplicating it. **Restart Claude Code sessions on the machine afterward** so the new hook and CLAUDE.md load. @@ -45,7 +34,7 @@ python3 ~/.claude/hooks/gh-write-guard.py --selftest # decision matrix: all c grep -c 'agent-safety v' ~/.claude/CLAUDE.md # expect 2 (start + end marker) ``` -Live end-to-end (in any repo): attempt a suppressed-output write and confirm the Bash tool is blocked: +Live end-to-end (in any repo): attempt a discarded-output write and confirm the Bash tool is blocked: ```sh gh api graphql -f query='mutation{noop}' -F t="PRRT_x" >/dev/null 2>&1 || true # blocked by the hook @@ -53,7 +42,7 @@ gh api graphql -f query='mutation{noop}' -F t="PRRT_x" >/dev/null 2>&1 || true ## Manual settings.json shape (for reference) -The installer writes this; it is here so you can inspect or hand-place it: +The installer writes this. It is here so you can inspect or hand-place it: ```json { @@ -67,12 +56,7 @@ The installer writes this; it is here so you can inspect or hand-place it: ## Scope and limits -- **Per-machine.** `~/.claude/` does not travel; run the installer on each box. This is the rollout that - ptr727/ProjectTemplate#365 tracks. -- **Precision over recall.** The hook denies the specific dangerous shapes with high confidence rather - than gating every write, so it never blocks legitimate work. A shape it does not catch still falls - under the behavioral rules. It cannot see the target behind an opaque GraphQL node id (that is why - rule 2 blocks a *literal* id at all - a captured `$variable` is trusted). -- **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. +- **Per-machine.** `~/.claude/` does not travel, so run the installer on each box. This is the rollout that ptr727/ProjectTemplate#365 tracks. +- **Precision over recall.** The hook denies the specific dangerous shapes with high confidence rather than gating every write, so it never blocks legitimate work. A shape it does not catch still falls under the behavioral rules. +- **Opaque targets are unseen.** The hook cannot see the repository behind a GraphQL node id, which is exactly why rule 2 blocks a *literal* id at all - a captured `$variable` is trusted. Likewise, the cross-origin check only runs when an `origin` can be resolved and the write names an explicit `-R`/`repos//` target. A write from a non-git directory, or one whose target is only a node id, is evaluated by rules 1 and 2 alone. +- **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. diff --git a/host-setup/agent-safety/claude-md-safety.md b/host-setup/agent-safety/claude-md-safety.md index 4e027eba..52cb1520 100644 --- a/host-setup/agent-safety/claude-md-safety.md +++ b/host-setup/agent-safety/claude-md-safety.md @@ -1,23 +1,9 @@ ## GitHub write safety (any project, every session) -A `gh` / GitHub API write runs under the logged-in identity, so a mis-targeted write acts publicly as -that account on someone else's repository - outward-facing and hard to reverse. These rules bound every -write (a git push, an API mutation, a comment, a label, a merge) in every session on this machine, -including ad-hoc work outside any project. Reads are unrestricted. A committed repo's `AGENTS.md` -"Repository Boundaries and Write Safety" states the same rules for its fleet; the two are kept in sync -deliberately, because this file also covers sessions that `AGENTS.md` never reaches. The -`gh-write-guard` PreToolUse hook enforces the mechanical half. +A `gh` / GitHub API write runs under the logged-in identity, so a mis-targeted write acts publicly as that account on someone else's repository - outward-facing and hard to reverse. These rules bound every write (a git push, an API mutation, a comment, a label, a merge) in every session on this machine, including ad-hoc work outside any project. Reads are unrestricted. A committed repo's `AGENTS.md` "Repository Boundaries and Write Safety" states the same rules for its fleet, and the two are kept in sync deliberately, because this file also covers sessions that `AGENTS.md` never reaches. The `gh-write-guard` PreToolUse hook enforces the mechanical half. -- **Write only to the current project's own repository.** Every state-changing call targets this - checkout's `origin` and nothing else. A broad or logged-in identity is capability, not permission. - Another repository needs explicit, per-session human permission for that specific repository, and a - "harmless test" write is still a write. -- **Never fabricate, guess, or reuse an identifier passed to a write.** Every id a write consumes (a - node id, a numeric id, a thread or comment id) is captured from a live query in the same session into - a variable and passed from there. Ids resolve globally, so a wrong-but-valid id does not fail - it - writes to the wrong target in another repository. If a query returns no id, stop rather than invent one. -- **A write is never a probe, and a write's output is never suppressed.** Never fire a state-changing - call to see whether it works, and never append `>/dev/null`, `2>&1`, or `|| true` to a mutation. A - write that appears to fail is verified, not assumed harmless - it may have succeeded on the server. +- **Write only to the current project's own repository.** Every state-changing call targets this checkout's `origin` and nothing else. A broad or logged-in identity is capability, not permission. Another repository needs explicit, per-session human permission for that specific repository, and a "harmless test" write is still a write. +- **Never fabricate, guess, or reuse an identifier passed to a write.** Every id a write consumes (a node id, a numeric id, a thread or comment id) is captured from a live query in the same session into a variable and passed from there. Ids resolve globally, so a wrong-but-valid id does not fail - it writes to the wrong target in another repository. If a query returns no id, stop rather than invent one. +- **A write is never a probe, and a write's output is never suppressed.** Never fire a state-changing call to see whether it works, and never append `>/dev/null`, `&>/dev/null`, or `|| true` to a mutation. A write that appears to fail is verified, not assumed harmless - it may have succeeded on the server. diff --git a/host-setup/agent-safety/gh-write-guard.py b/host-setup/agent-safety/gh-write-guard.py index c5487aef..7478cabd 100644 --- a/host-setup/agent-safety/gh-write-guard.py +++ b/host-setup/agent-safety/gh-write-guard.py @@ -9,7 +9,7 @@ it cannot parse. A false deny would break the agent; a missed case still falls under the AGENTS.md "Repository Boundaries and Write Safety" prose rules. The three denied shapes: - 1. a state-changing gh call whose output is suppressed (>/dev/null, 2>&1, || true, || echo) + 1. a state-changing gh call whose output is discarded (>/dev/null, &>/dev/null, 2>/dev/null, || true) 2. a GraphQL mutation passing a literal GitHub node id (PRRT_/PR_/BOT_/...) instead of a $variable 3. a gh write with an explicit -R/--repo/repos// target outside the checkout's origin @@ -43,7 +43,9 @@ _GIT_PUSH = re.compile(r"\bgit\s+push\b") # --- Risk-pattern detectors -------------------------------------------------------------------------- -_SUPPRESS = re.compile(r">\s*/dev/null|&>\s*/dev/null|2>&1|\|\|\s*(?:true|:|echo)\b") +# Output-discard / force-success tails. Bare `2>&1` is NOT here: it merges stderr into stdout, leaving +# the output visible, so it is not suppression (and denying it would break `... 2>&1 | tee log`). +_SUPPRESS = re.compile(r">\s*/dev/null|&>\s*/dev/null|2>\s*/dev/null|\|\|\s*(?:true|:|echo)\b") # A GitHub global node id literal: an uppercase-ish prefix + underscore + base64url body, or legacy MDxx. _NODE_ID_LITERAL = re.compile(r'^(?:[A-Za-z]{1,6}_[A-Za-z0-9_\-]{6,}|MD[A-Za-z0-9]{6,})$') # -F/-f name=VALUE (captures the value; handles "quoted" and bare) @@ -89,7 +91,7 @@ def classify(cmd, cwd=None, origin=None): # 1. suppressed output on a write if _SUPPRESS.search(cmd): return "deny", ( - "This is a GitHub write with its output discarded (>/dev/null, 2>&1, || true). " + "This is a GitHub write with its output discarded (>/dev/null, &>/dev/null, || true). " "A write's result is exactly what must be read: a mutation can succeed on the server " "while the client reports an error. Run it without the output-discarding tail and read " "the response. See AGENTS.md 'Repository Boundaries and Write Safety'." @@ -121,6 +123,9 @@ def classify(cmd, cwd=None, origin=None): for m in _API_REPO_PATH.finditer(cmd): if "<" not in m.group("owner"): targets.append((m.group("owner").lower(), m.group("repo").lower())) + # Only runs when origin resolves (a git checkout): with no project context there is nothing to + # compare an explicit target against, so this check is skipped and rules 1-2 still apply. A node-id + # target is invisible here regardless - that is what rule 2 guards. if origin: for t in targets: if t != origin: @@ -147,7 +152,10 @@ def classify(cmd, cwd=None, origin=None): ("gh pr view 5 --json reviews", "allow", "gh pr view (read)"), ("return 1 2>/dev/null || exit 1", "allow", "shell guard, not a gh write"), ("git push origin develop", "allow", "normal push (no suppression, no cross-repo)"), - ("git commit -m 'x' && git push >/dev/null 2>&1", "deny", "push with suppressed output"), + ("git commit -m 'x' && git push >/dev/null 2>&1", "deny", "push with discarded output"), + ("gh issue comment 5 --body x 2>&1 | tee out.log", "allow", "bare 2>&1 piped to tee is not suppression"), + ("gh pr comment 5 --body ok 2>&1", "allow", "bare 2>&1 leaves output visible"), + ("gh api repos/ptr727/PlexCleaner/issues/1/comments -f body=x 2>/dev/null", "deny", "stderr discarded on a write"), ] diff --git a/host-setup/agent-safety/install.py b/host-setup/agent-safety/install.py index ce84610c..6e3595f8 100644 --- a/host-setup/agent-safety/install.py +++ b/host-setup/agent-safety/install.py @@ -53,7 +53,8 @@ def main(): # 2. Register in settings.json: exactly one PreToolUse/Bash group carrying our hook command. launcher = hook_launcher() - hook_cmd = f'{launcher} "{hook_dst}"' + # Quote the launcher too: the sys.executable fallback can contain spaces (e.g. C:\Program Files\...). + hook_cmd = f'"{launcher}" "{hook_dst}"' data = {} if settings.exists() and settings.read_text(encoding="utf-8").strip(): data = json.loads(settings.read_text(encoding="utf-8")) From 20f9ffa717deab9c2205340aa37761ce45a5e8ee Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 20 Jul 2026 09:54:45 -0700 Subject: [PATCH 04/17] Tighten node-id detection, harden python selection, title-case + de-semicolon docs (Copilot #367 round 2) Correctness: - The node-id literal detector matched any underscored token, so a legitimate reply body like body="fixed_the_thing_now" would false-deny. It now requires an UPPERCASE prefix plus a >=12-char base64url body, matching real GitHub node ids (PRRT_kwDO...) while ignoring ordinary underscored words. Two cases added; 16/16 pass. - install.py never emits a bare `python` launcher (Python 2 on some systems, which would fail the hook's Python 3 syntax): it uses python3 or this interpreter's absolute path, and guards the version. The .sh and .ps1 wrappers select a verified Python 3 (py -3 / python3 first). Style: README and CLAUDE.md-snippet headings are title case; the docstring semicolon joining two clauses is recast. Co-Authored-By: Claude Opus 4.8 (1M context) --- host-setup/agent-safety/README.md | 8 ++--- host-setup/agent-safety/claude-md-safety.md | 2 +- host-setup/agent-safety/gh-write-guard.py | 10 ++++-- host-setup/agent-safety/install.ps1 | 38 +++++++++++---------- host-setup/agent-safety/install.py | 14 +++++--- host-setup/agent-safety/install.sh | 15 ++++++-- 6 files changed, 53 insertions(+), 34 deletions(-) diff --git a/host-setup/agent-safety/README.md b/host-setup/agent-safety/README.md index 41689f10..2541905f 100644 --- a/host-setup/agent-safety/README.md +++ b/host-setup/agent-safety/README.md @@ -2,7 +2,7 @@ Per-machine, user-account-scoped guards against an agent making a mis-targeted GitHub **write** under the maintainer's identity. Deploy it as the **first thing on any new system** where Claude Code runs with the `gh` credentials logged in (WSL, Linux, macOS, Proxmox, Windows). -## What it installs +## What It Installs Into `~/.claude/` (or `%USERPROFILE%\.claude\` on Windows): @@ -11,7 +11,7 @@ Into `~/.claude/` (or `%USERPROFILE%\.claude\` on Windows): The hook is the mechanical backstop. The CLAUDE.md rules and the carried AGENTS.md rules are the behavioral layer. Prose alone is not enough - the incident happened under prose rules - so both ship. -## Install (idempotent - safe to re-run to update) +## Install (Idempotent - Safe to Re-Run to Update) ```sh # Linux / WSL / macOS / Proxmox @@ -40,7 +40,7 @@ Live end-to-end (in any repo): attempt a discarded-output write and confirm the gh api graphql -f query='mutation{noop}' -F t="PRRT_x" >/dev/null 2>&1 || true # blocked by the hook ``` -## Manual settings.json shape (for reference) +## Manual settings.json Shape (for Reference) The installer writes this. It is here so you can inspect or hand-place it: @@ -54,7 +54,7 @@ The installer writes this. It is here so you can inspect or hand-place it: } ``` -## Scope and limits +## Scope and Limits - **Per-machine.** `~/.claude/` does not travel, so run the installer on each box. This is the rollout that ptr727/ProjectTemplate#365 tracks. - **Precision over recall.** The hook denies the specific dangerous shapes with high confidence rather than gating every write, so it never blocks legitimate work. A shape it does not catch still falls under the behavioral rules. diff --git a/host-setup/agent-safety/claude-md-safety.md b/host-setup/agent-safety/claude-md-safety.md index 52cb1520..5bccb458 100644 --- a/host-setup/agent-safety/claude-md-safety.md +++ b/host-setup/agent-safety/claude-md-safety.md @@ -1,5 +1,5 @@ -## GitHub write safety (any project, every session) +## GitHub Write Safety (Any Project, Every Session) A `gh` / GitHub API write runs under the logged-in identity, so a mis-targeted write acts publicly as that account on someone else's repository - outward-facing and hard to reverse. These rules bound every write (a git push, an API mutation, a comment, a label, a merge) in every session on this machine, including ad-hoc work outside any project. Reads are unrestricted. A committed repo's `AGENTS.md` "Repository Boundaries and Write Safety" states the same rules for its fleet, and the two are kept in sync deliberately, because this file also covers sessions that `AGENTS.md` never reaches. The `gh-write-guard` PreToolUse hook enforces the mechanical half. diff --git a/host-setup/agent-safety/gh-write-guard.py b/host-setup/agent-safety/gh-write-guard.py index 7478cabd..b37363c7 100644 --- a/host-setup/agent-safety/gh-write-guard.py +++ b/host-setup/agent-safety/gh-write-guard.py @@ -6,7 +6,7 @@ matching a known-dangerous pattern. Reads and everything that is not a clear write pass through. Precision over recall by design: it denies the specific shapes that caused the incident, not everything -it cannot parse. A false deny would break the agent; a missed case still falls under the AGENTS.md +it cannot parse. A false deny would break the agent, while a missed case still falls under the AGENTS.md "Repository Boundaries and Write Safety" prose rules. The three denied shapes: 1. a state-changing gh call whose output is discarded (>/dev/null, &>/dev/null, 2>/dev/null, || true) @@ -46,8 +46,10 @@ # Output-discard / force-success tails. Bare `2>&1` is NOT here: it merges stderr into stdout, leaving # the output visible, so it is not suppression (and denying it would break `... 2>&1 | tee log`). _SUPPRESS = re.compile(r">\s*/dev/null|&>\s*/dev/null|2>\s*/dev/null|\|\|\s*(?:true|:|echo)\b") -# A GitHub global node id literal: an uppercase-ish prefix + underscore + base64url body, or legacy MDxx. -_NODE_ID_LITERAL = re.compile(r'^(?:[A-Za-z]{1,6}_[A-Za-z0-9_\-]{6,}|MD[A-Za-z0-9]{6,})$') +# A GitHub global node id literal: an UPPERCASE prefix (PR_, PRRT_, IC_, BOT_, ...) + a long base64url +# body, or a legacy MD... base64 id. The uppercase prefix plus a >=12-char body keeps it from matching +# an ordinary underscored word in a reply body (e.g. body="fixed_the_thing_now", lowercase prefix). +_NODE_ID_LITERAL = re.compile(r'^(?:[A-Z]{1,5}_[A-Za-z0-9_\-]{12,}|MD[A-Za-z0-9]{12,})$') # -F/-f name=VALUE (captures the value; handles "quoted" and bare) _FIELD_ASSIGN = re.compile(r"""(?:-F|-f|--field|--raw-field)\s+[A-Za-z_][\w]*=(?P'[^']*'|"[^"]*"|\S+)""") _EXPLICIT_REPO = re.compile(r"(?:-R|--repo)\s+(?P[^\s'\"]+)") @@ -156,6 +158,8 @@ def classify(cmd, cwd=None, origin=None): ("gh issue comment 5 --body x 2>&1 | tee out.log", "allow", "bare 2>&1 piped to tee is not suppression"), ("gh pr comment 5 --body ok 2>&1", "allow", "bare 2>&1 leaves output visible"), ("gh api repos/ptr727/PlexCleaner/issues/1/comments -f body=x 2>/dev/null", "deny", "stderr discarded on a write"), + ("gh api graphql -f query='mutation{addPullRequestReviewThreadReply(input:{pullRequestReviewThreadId:$t,body:$b}){comment{id}}}' -F t=\"$TID\" -F b=\"fixed_the_underscore_bug_here\"", "allow", "underscored reply body is not a node id"), + ("gh api graphql -f query='mutation{resolveReviewThread(input:{threadId:$t}){thread{isResolved}}}' -F t=\"TODO_fixit\"", "allow", "short all-caps token is not a node id"), ] diff --git a/host-setup/agent-safety/install.ps1 b/host-setup/agent-safety/install.ps1 index ac0f3fe6..d3107878 100644 --- a/host-setup/agent-safety/install.ps1 +++ b/host-setup/agent-safety/install.ps1 @@ -1,18 +1,20 @@ -# Thin wrapper: run the cross-platform installer with the available python (Windows). -# All logic lives in install.py so every OS runs one tested code path. Idempotent; safe to re-run. -# .\install.ps1 -# $env:CLAUDE_HOME = "C:\path"; .\install.ps1 # override the target (testing) -$ErrorActionPreference = "Stop" -$here = Split-Path -Parent $MyInvocation.MyCommand.Path -$script = Join-Path $here "install.py" - -if (Get-Command "python" -ErrorAction SilentlyContinue) { - & python $script @args -} elseif (Get-Command "python3" -ErrorAction SilentlyContinue) { - & python3 $script @args -} elseif (Get-Command "py" -ErrorAction SilentlyContinue) { - & py -3 $script @args -} else { - Write-Error "Python is required and was not found on PATH (tried python, python3, py)." - exit 1 -} +# Thin wrapper: run the cross-platform installer with a Python 3 (Windows). +# All logic lives in install.py so every OS runs one tested code path. Idempotent; safe to re-run. +# .\install.ps1 +# $env:CLAUDE_HOME = "C:\path"; .\install.ps1 # override the target (testing) +$ErrorActionPreference = "Stop" +$here = Split-Path -Parent $MyInvocation.MyCommand.Path +$script = Join-Path $here "install.py" + +# Prefer launchers that are unambiguously Python 3. install.py and the hook use Python 3 syntax, so a +# bare `python` (Python 2 on some systems) is the last resort. +if (Get-Command "py" -ErrorAction SilentlyContinue) { + & py -3 $script @args +} elseif (Get-Command "python3" -ErrorAction SilentlyContinue) { + & python3 $script @args +} elseif (Get-Command "python" -ErrorAction SilentlyContinue) { + & python $script @args +} else { + Write-Error "Python 3 is required and was not found on PATH (tried py -3, python3, python)." + exit 1 +} diff --git a/host-setup/agent-safety/install.py b/host-setup/agent-safety/install.py index 6e3595f8..e42639a1 100644 --- a/host-setup/agent-safety/install.py +++ b/host-setup/agent-safety/install.py @@ -20,15 +20,19 @@ def hook_launcher(): - """A python invocation for the settings.json command. Prefer a bare name on PATH (portable across - machines), else this interpreter's absolute path.""" - for name in ("python3", "python"): - if shutil.which(name): - return name + """A python invocation for the settings.json command. Prefer a bare `python3` (portable and + unambiguously Python 3), else this interpreter's absolute path (guaranteed the Python 3 running the + installer). Never a bare `python`, which is Python 2 on some systems and would fail the hook's + Python 3 syntax.""" + if shutil.which("python3"): + return "python3" return sys.executable def main(): + if sys.version_info < (3, 7): + sys.stderr.write("This installer and the hook require Python 3.7+; run it with python3.\n") + return 1 claude_home = pathlib.Path(os.environ.get("CLAUDE_HOME", pathlib.Path.home() / ".claude")) hooks_dir = claude_home / "hooks" hook_dst = hooks_dir / "gh-write-guard.py" diff --git a/host-setup/agent-safety/install.sh b/host-setup/agent-safety/install.sh index c60e3ad5..64cf7289 100755 --- a/host-setup/agent-safety/install.sh +++ b/host-setup/agent-safety/install.sh @@ -1,10 +1,19 @@ #!/usr/bin/env bash -# Thin wrapper: run the cross-platform installer with the available python (Linux / WSL / macOS / Proxmox). +# Thin wrapper: run the cross-platform installer with a Python 3 (Linux / WSL / macOS / Proxmox). # All logic lives in install.py so every OS runs one tested code path. Idempotent; safe to re-run. # ./install.sh installs to ~/.claude # CLAUDE_HOME=/x ./install.sh overrides the target (testing) set -Eeuo pipefail here="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -py="$(command -v python3 || command -v python || true)" -[ -n "$py" ] || { echo "python3 is required and was not found on PATH." >&2; exit 1; } + +# Pick the first candidate that is actually Python 3 - install.py and the hook use Python 3 syntax, so a +# bare `python` that is Python 2 must be rejected, not handed the script (it would fail on import). +py="" +for c in python3 python; do + if command -v "$c" >/dev/null 2>&1 && "$c" -c 'import sys; raise SystemExit(0 if sys.version_info[0] == 3 else 1)' 2>/dev/null; then + py="$c"; break + fi +done +[ -n "$py" ] || { echo "Python 3 is required and was not found on PATH (tried python3, python)." >&2; exit 1; } + exec "$py" "$here/install.py" "$@" From 75b460db886543f40fa534ebbe5491da85dd70f9 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 20 Jul 2026 09:55:03 -0700 Subject: [PATCH 05/17] Restore install.ps1 to CRLF Rewriting it reset the endings to LF; it follows the .editorconfig [*] CRLF default (and PowerShell is CRLF-native). editorconfig-checker flags the LF form. Co-Authored-By: Claude Opus 4.8 (1M context) --- host-setup/agent-safety/install.ps1 | 40 ++++++++++++++--------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/host-setup/agent-safety/install.ps1 b/host-setup/agent-safety/install.ps1 index d3107878..21b680c5 100644 --- a/host-setup/agent-safety/install.ps1 +++ b/host-setup/agent-safety/install.ps1 @@ -1,20 +1,20 @@ -# Thin wrapper: run the cross-platform installer with a Python 3 (Windows). -# All logic lives in install.py so every OS runs one tested code path. Idempotent; safe to re-run. -# .\install.ps1 -# $env:CLAUDE_HOME = "C:\path"; .\install.ps1 # override the target (testing) -$ErrorActionPreference = "Stop" -$here = Split-Path -Parent $MyInvocation.MyCommand.Path -$script = Join-Path $here "install.py" - -# Prefer launchers that are unambiguously Python 3. install.py and the hook use Python 3 syntax, so a -# bare `python` (Python 2 on some systems) is the last resort. -if (Get-Command "py" -ErrorAction SilentlyContinue) { - & py -3 $script @args -} elseif (Get-Command "python3" -ErrorAction SilentlyContinue) { - & python3 $script @args -} elseif (Get-Command "python" -ErrorAction SilentlyContinue) { - & python $script @args -} else { - Write-Error "Python 3 is required and was not found on PATH (tried py -3, python3, python)." - exit 1 -} +# Thin wrapper: run the cross-platform installer with a Python 3 (Windows). +# All logic lives in install.py so every OS runs one tested code path. Idempotent; safe to re-run. +# .\install.ps1 +# $env:CLAUDE_HOME = "C:\path"; .\install.ps1 # override the target (testing) +$ErrorActionPreference = "Stop" +$here = Split-Path -Parent $MyInvocation.MyCommand.Path +$script = Join-Path $here "install.py" + +# Prefer launchers that are unambiguously Python 3. install.py and the hook use Python 3 syntax, so a +# bare `python` (Python 2 on some systems) is the last resort. +if (Get-Command "py" -ErrorAction SilentlyContinue) { + & py -3 $script @args +} elseif (Get-Command "python3" -ErrorAction SilentlyContinue) { + & python3 $script @args +} elseif (Get-Command "python" -ErrorAction SilentlyContinue) { + & python $script @args +} else { + Write-Error "Python 3 is required and was not found on PATH (tried py -3, python3, python)." + exit 1 +} From 7c427ae70d8d237299987e1aec3adae6d6fa5958 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 20 Jul 2026 09:59:36 -0700 Subject: [PATCH 06/17] Fix the 2>&1 wording in the runbook and a semicolon in install.py (Copilot #367 round 3) The 2>&1-is-not-suppression correction had missed the copilot-instructions runbook comment (the same class, fixed now) and install.py carried a clause-joining semicolon in a user-facing message. Swept both files plus AGENTS.md and the kit for any remaining instance of either - none left. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/copilot-instructions.md | 2 +- host-setup/agent-safety/install.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 500a9a7d..78860f72 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -161,7 +161,7 @@ TID=$(gh api graphql -f query=' | .id' | head -n 1) [ -n "$TID" ] || { echo "no matching unresolved thread on - do not guess an id" >&2; return 1 2>/dev/null || exit 1; } -# Show the mutation's output; never append >/dev/null, 2>&1, or || true to a write. +# Show the mutation's output. Never append >/dev/null, &>/dev/null, or || true to a write. gh api graphql -f query=' mutation($threadId: ID!, $body: String!) { addPullRequestReviewThreadReply(input: { pullRequestReviewThreadId: $threadId, body: $body }) { diff --git a/host-setup/agent-safety/install.py b/host-setup/agent-safety/install.py index e42639a1..8e208219 100644 --- a/host-setup/agent-safety/install.py +++ b/host-setup/agent-safety/install.py @@ -31,7 +31,7 @@ def hook_launcher(): def main(): if sys.version_info < (3, 7): - sys.stderr.write("This installer and the hook require Python 3.7+; run it with python3.\n") + sys.stderr.write("This installer and the hook require Python 3.7+. Run it with python3.\n") return 1 claude_home = pathlib.Path(os.environ.get("CLAUDE_HOME", pathlib.Path.home() / ".claude")) hooks_dir = claude_home / "hooks" From d4d224ddf152574bc079e5a9dbc3cede02599d4a Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 20 Jul 2026 10:08:46 -0700 Subject: [PATCH 07/17] Address Copilot round 4 on the write-safety kit (#367) - install.sh / install.ps1: recast the "Idempotent; safe to re-run" header comment to drop the clause-joining semicolon (house prose rule). - .gitattributes: refresh the stale comment above the per-path Python LF pins to name the audit runner and the agent-safety hook and installer, not just the CI validation entry point (behavior changed, so the prose follows). - README.md: render the issue reference as a clickable link. - gh-write-guard.py: drop two clause-joining semicolons in its own comments to match the same rule the wrappers were held to. Co-Authored-By: Claude Opus 4.8 --- .gitattributes | 3 ++- host-setup/agent-safety/README.md | 2 +- host-setup/agent-safety/gh-write-guard.py | 4 ++-- host-setup/agent-safety/install.ps1 | 2 +- host-setup/agent-safety/install.sh | 2 +- 5 files changed, 7 insertions(+), 6 deletions(-) diff --git a/.gitattributes b/.gitattributes index 2a14e795..b69124f1 100644 --- a/.gitattributes +++ b/.gitattributes @@ -15,7 +15,8 @@ catalog/snippets/husky/pre-commit text eol=lf # Vanilla `.py` follows the CRLF default - Python's universal newlines accept CRLF, and it is # commonly edited on Windows. Pin LF only for a `.py` executed directly via its shebang, by path - -# here the CI validation entry point; do not re-add a blanket `*.py text eol=lf`. +# here the CI validation entry point, the fleet-audit runner, and the agent-safety hook and its +# installer. Do not re-add a blanket `*.py text eol=lf`. spec/validate.py text eol=lf spec/audit.py text eol=lf host-setup/agent-safety/gh-write-guard.py text eol=lf diff --git a/host-setup/agent-safety/README.md b/host-setup/agent-safety/README.md index 2541905f..61590295 100644 --- a/host-setup/agent-safety/README.md +++ b/host-setup/agent-safety/README.md @@ -56,7 +56,7 @@ The installer writes this. It is here so you can inspect or hand-place it: ## Scope and Limits -- **Per-machine.** `~/.claude/` does not travel, so run the installer on each box. This is the rollout that ptr727/ProjectTemplate#365 tracks. +- **Per-machine.** `~/.claude/` does not travel, so run the installer on each box. This is the rollout that [#365](https://github.com/ptr727/ProjectTemplate/issues/365) tracks. - **Precision over recall.** The hook denies the specific dangerous shapes with high confidence rather than gating every write, so it never blocks legitimate work. A shape it does not catch still falls under the behavioral rules. - **Opaque targets are unseen.** The hook cannot see the repository behind a GraphQL node id, which is exactly why rule 2 blocks a *literal* id at all - a captured `$variable` is trusted. Likewise, the cross-origin check only runs when an `origin` can be resolved and the write names an explicit `-R`/`repos//` target. A write from a non-git directory, or one whose target is only a node id, is evaluated by rules 1 and 2 alone. - **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. diff --git a/host-setup/agent-safety/gh-write-guard.py b/host-setup/agent-safety/gh-write-guard.py index b37363c7..54b6eedf 100644 --- a/host-setup/agent-safety/gh-write-guard.py +++ b/host-setup/agent-safety/gh-write-guard.py @@ -50,7 +50,7 @@ # body, or a legacy MD... base64 id. The uppercase prefix plus a >=12-char body keeps it from matching # an ordinary underscored word in a reply body (e.g. body="fixed_the_thing_now", lowercase prefix). _NODE_ID_LITERAL = re.compile(r'^(?:[A-Z]{1,5}_[A-Za-z0-9_\-]{12,}|MD[A-Za-z0-9]{12,})$') -# -F/-f name=VALUE (captures the value; handles "quoted" and bare) +# -F/-f name=VALUE, capturing the value - handles "quoted" and bare _FIELD_ASSIGN = re.compile(r"""(?:-F|-f|--field|--raw-field)\s+[A-Za-z_][\w]*=(?P'[^']*'|"[^"]*"|\S+)""") _EXPLICIT_REPO = re.compile(r"(?:-R|--repo)\s+(?P[^\s'\"]+)") _API_REPO_PATH = re.compile(r"\bgh\s+api\b[^\n|]*?\brepos/(?P[A-Za-z0-9_.\-]+)/(?P[A-Za-z0-9_.\-]+)") @@ -183,7 +183,7 @@ def _main(): try: data = json.load(sys.stdin) except Exception: - sys.exit(0) # not our event shape; do not interfere + sys.exit(0) # not our event shape - do not interfere if data.get("tool_name") != "Bash": sys.exit(0) cmd = (data.get("tool_input") or {}).get("command", "") diff --git a/host-setup/agent-safety/install.ps1 b/host-setup/agent-safety/install.ps1 index 21b680c5..224ce663 100644 --- a/host-setup/agent-safety/install.ps1 +++ b/host-setup/agent-safety/install.ps1 @@ -1,5 +1,5 @@ # Thin wrapper: run the cross-platform installer with a Python 3 (Windows). -# All logic lives in install.py so every OS runs one tested code path. Idempotent; safe to re-run. +# All logic lives in install.py so every OS runs one tested code path. Idempotent, safe to re-run. # .\install.ps1 # $env:CLAUDE_HOME = "C:\path"; .\install.ps1 # override the target (testing) $ErrorActionPreference = "Stop" diff --git a/host-setup/agent-safety/install.sh b/host-setup/agent-safety/install.sh index 64cf7289..d52c88d0 100755 --- a/host-setup/agent-safety/install.sh +++ b/host-setup/agent-safety/install.sh @@ -1,6 +1,6 @@ #!/usr/bin/env bash # Thin wrapper: run the cross-platform installer with a Python 3 (Linux / WSL / macOS / Proxmox). -# All logic lives in install.py so every OS runs one tested code path. Idempotent; safe to re-run. +# All logic lives in install.py so every OS runs one tested code path. Idempotent, safe to re-run. # ./install.sh installs to ~/.claude # CLAUDE_HOME=/x ./install.sh overrides the target (testing) set -Eeuo pipefail From 9325942bc04c9f6519cd587eca9f8789518c2a38 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 20 Jul 2026 10:14:39 -0700 Subject: [PATCH 08/17] Use a reference-style link in the kit README (#367) AGENTS.md requires reference-style links in every markdown doc except the two agent-instruction files. The round-4 fix rendered the issue reference as an inline link; convert it to a reference-style link with the definition at the bottom of the file under a Repo group. Addresses Copilot's low-confidence note on the round-5 review. Co-Authored-By: Claude Opus 4.8 --- host-setup/agent-safety/README.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/host-setup/agent-safety/README.md b/host-setup/agent-safety/README.md index 61590295..ef600624 100644 --- a/host-setup/agent-safety/README.md +++ b/host-setup/agent-safety/README.md @@ -56,7 +56,10 @@ The installer writes this. It is here so you can inspect or hand-place it: ## Scope and Limits -- **Per-machine.** `~/.claude/` does not travel, so run the installer on each box. This is the rollout that [#365](https://github.com/ptr727/ProjectTemplate/issues/365) tracks. +- **Per-machine.** `~/.claude/` does not travel, so run the installer on each box. This is the rollout that [#365][issue-365] tracks. - **Precision over recall.** The hook denies the specific dangerous shapes with high confidence rather than gating every write, so it never blocks legitimate work. A shape it does not catch still falls under the behavioral rules. - **Opaque targets are unseen.** The hook cannot see the repository behind a GraphQL node id, which is exactly why rule 2 blocks a *literal* id at all - a captured `$variable` is trusted. Likewise, the cross-origin check only runs when an `origin` can be resolved and the write names an explicit `-R`/`repos//` target. A write from a non-git directory, or one whose target is only a node id, is evaluated by rules 1 and 2 alone. - **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. + + +[issue-365]: https://github.com/ptr727/ProjectTemplate/issues/365 From 8dfc5ff7ab9dc4d04f0dc4e12825c4730453c87d Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 20 Jul 2026 10:20:59 -0700 Subject: [PATCH 09/17] Fix CLAUDE_HOME tilde expansion; record gh review-request API traps (#367) - install.py: expanduser a CLAUDE_HOME set to a ~/... form so the installer targets the home directory instead of creating a literal ~ dir under cwd (Copilot round-6 finding). Verified with a quoted-tilde CLAUDE_HOME. - copilot-instructions.md: extend the runbook's known-non-working-paths list with three traps verified this session while driving the review loop - the reviewer bot id must go in requestReviews botIds not userIds (a Bot node does not resolve as a User), suggestedActors surfaces copilot-swe-agent not the reviewer, and there is no removePullRequestFromReviewRequest mutation (union:true re-fires without removing). Co-Authored-By: Claude Opus 4.8 --- .github/copilot-instructions.md | 3 +++ host-setup/agent-safety/install.py | 4 +++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 78860f72..0e653d22 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -85,6 +85,9 @@ Known non-working request paths (don't rely on them - use the `requestReviews` m - `POST /requested_reviewers` with `reviewers=[Copilot]` can return 200 but no-op. - `copilot-pull-request-reviewer` as a requested reviewer slug returns 422. +- `requestReviews` with the reviewer's bot node id in **`userIds`** fails with `Could not resolve to User node` - the Copilot reviewer is a **Bot**, so its node id goes in **`botIds`** (as in the mutation above), never `userIds`. +- `suggestedActors(capabilities: [CAN_BE_ASSIGNED])` lists `copilot-swe-agent` (the coding agent), not `copilot-pull-request-reviewer` - do not source the reviewer's bot node id there. Read it from an existing review per step 1 above. +- There is no `removePullRequestFromReviewRequest` mutation, and removing the reviewer to force a fresh pass is unnecessary anyway - `requestReviews` with `union: true` re-fires the review on the current head. ### Verify Review Covered Current Head diff --git a/host-setup/agent-safety/install.py b/host-setup/agent-safety/install.py index 8e208219..4266a438 100644 --- a/host-setup/agent-safety/install.py +++ b/host-setup/agent-safety/install.py @@ -33,7 +33,9 @@ def main(): if sys.version_info < (3, 7): sys.stderr.write("This installer and the hook require Python 3.7+. Run it with python3.\n") return 1 - claude_home = pathlib.Path(os.environ.get("CLAUDE_HOME", pathlib.Path.home() / ".claude")) + # expanduser so a CLAUDE_HOME set to a `~/...` form resolves to the home dir, not a literal `~` dir. + claude_home_env = os.environ.get("CLAUDE_HOME") + claude_home = pathlib.Path(claude_home_env).expanduser() if claude_home_env else pathlib.Path.home() / ".claude" hooks_dir = claude_home / "hooks" hook_dst = hooks_dir / "gh-write-guard.py" settings = claude_home / "settings.json" From 3fd3c5a1c3cf6211cad6be855b30128b3feab85d Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 20 Jul 2026 10:29:09 -0700 Subject: [PATCH 10/17] Actually block the || : force-success tail; align the pattern lists (#367) Copilot round-7 flagged the human-facing suppression examples as omitting patterns the kit blocks. Chasing that surfaced a real enforcement gap: the _SUPPRESS regex ended the alternation group with \b, but a bare `:` is a non-word char, so `|| :` at end of command never matched - the hook did not actually deny a `|| :` force-success tail despite claiming to. Move \b inside the word alternatives (true\b|echo\b|:) so `||:` and `|| :` deny while a `truthy` substring in a body still does not false-match, and a non-gh command is unaffected (the check is write-gated). Align every place that lists the patterns with the enforced set (>/dev/null, 2>/dev/null, &>/dev/null, || true, || :, || echo): the hook deny message and docstring, AGENTS.md rule 3, the copilot-instructions runbook comment, and the CLAUDE.md kit snippet - framed as examples in the behavioral docs so they do not read as an allowlist. Add self-test cases for `|| :` and `|| echo` so the enforcement is proven, not just asserted. Co-Authored-By: Claude Opus 4.8 --- .github/copilot-instructions.md | 3 ++- AGENTS.md | 2 +- host-setup/agent-safety/claude-md-safety.md | 2 +- host-setup/agent-safety/gh-write-guard.py | 10 +++++++--- 4 files changed, 11 insertions(+), 6 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 0e653d22..57001040 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -164,7 +164,8 @@ TID=$(gh api graphql -f query=' | .id' | head -n 1) [ -n "$TID" ] || { echo "no matching unresolved thread on - do not guess an id" >&2; return 1 2>/dev/null || exit 1; } -# Show the mutation's output. Never append >/dev/null, &>/dev/null, or || true to a write. +# Show the mutation's output. Never append an output-discard or force-success tail +# (>/dev/null, 2>/dev/null, &>/dev/null, || true, || :, || echo) to a write. gh api graphql -f query=' mutation($threadId: ID!, $body: String!) { addPullRequestReviewThreadReply(input: { pullRequestReviewThreadId: $threadId, body: $body }) { diff --git a/AGENTS.md b/AGENTS.md index f4dd74e2..4b9d980c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -21,7 +21,7 @@ A state-changing GitHub call is the highest-blast-radius thing an agent does her - **Write only to the current project's own repository.** Every state-changing call targets this project's `origin` and nothing else. A broad or logged-in identity is capability, not permission - a token that *can* reach another repository does not authorize writing to it. Writing to any other repository needs explicit, per-session human permission for that specific repository, and a "harmless test" write is still a write, so there is no probe exception. Reads from anywhere are fine. - **Never fabricate, guess, or reuse an identifier passed to a write.** Every id a state-changing call consumes - a node id, a numeric id, a thread or comment id - is captured from a live query in the **same** session into a variable and passed from there. Do not hand-type an id, guess it, recall it from memory or an earlier session, or copy it from documentation or an example. Ids commonly resolve **globally**, so a wrong-but-valid id does not fail - it writes to the wrong target, in someone else's repository. If a query returns no id, stop rather than invent one to proceed. -- **A write is never a probe, and a write's output is never suppressed.** Never fire a state-changing call to see whether it works: decide it should happen, make it happen, and read the result. Never append output-discarding redirection or a force-success tail to a mutation (`>/dev/null`, `&>/dev/null`, `|| true`, `|| echo`) - the write's output is exactly what must be read. A write that appears to fail is **verified, not assumed harmless** - the operation may have succeeded on the server while the client reported an error - so confirm the actual state before retrying or moving on. +- **A write is never a probe, and a write's output is never suppressed.** Never fire a state-changing call to see whether it works: decide it should happen, make it happen, and read the result. Never append output-discarding redirection or a force-success tail to a mutation (for example `>/dev/null`, `2>/dev/null`, `&>/dev/null`, `|| true`, `|| :`, `|| echo`) - the write's output is exactly what must be read. A write that appears to fail is **verified, not assumed harmless** - the operation may have succeeded on the server while the client reported an error - so confirm the actual state before retrying or moving on. ## Git and Commit Rules diff --git a/host-setup/agent-safety/claude-md-safety.md b/host-setup/agent-safety/claude-md-safety.md index 5bccb458..38dc8a49 100644 --- a/host-setup/agent-safety/claude-md-safety.md +++ b/host-setup/agent-safety/claude-md-safety.md @@ -5,5 +5,5 @@ A `gh` / GitHub API write runs under the logged-in identity, so a mis-targeted w - **Write only to the current project's own repository.** Every state-changing call targets this checkout's `origin` and nothing else. A broad or logged-in identity is capability, not permission. Another repository needs explicit, per-session human permission for that specific repository, and a "harmless test" write is still a write. - **Never fabricate, guess, or reuse an identifier passed to a write.** Every id a write consumes (a node id, a numeric id, a thread or comment id) is captured from a live query in the same session into a variable and passed from there. Ids resolve globally, so a wrong-but-valid id does not fail - it writes to the wrong target in another repository. If a query returns no id, stop rather than invent one. -- **A write is never a probe, and a write's output is never suppressed.** Never fire a state-changing call to see whether it works, and never append `>/dev/null`, `&>/dev/null`, or `|| true` to a mutation. A write that appears to fail is verified, not assumed harmless - it may have succeeded on the server. +- **A write is never a probe, and a write's output is never suppressed.** Never fire a state-changing call to see whether it works, and never append an output-discarding or force-success tail (for example `>/dev/null`, `2>/dev/null`, `&>/dev/null`, `|| true`, `|| :`, `|| echo`) to a mutation. A write that appears to fail is verified, not assumed harmless - it may have succeeded on the server. diff --git a/host-setup/agent-safety/gh-write-guard.py b/host-setup/agent-safety/gh-write-guard.py index 54b6eedf..35bb8ba1 100644 --- a/host-setup/agent-safety/gh-write-guard.py +++ b/host-setup/agent-safety/gh-write-guard.py @@ -9,7 +9,8 @@ it cannot parse. A false deny would break the agent, while a missed case still falls under the AGENTS.md "Repository Boundaries and Write Safety" prose rules. The three denied shapes: - 1. a state-changing gh call whose output is discarded (>/dev/null, &>/dev/null, 2>/dev/null, || true) + 1. a state-changing gh call whose output is discarded or forced to success + (>/dev/null, 2>/dev/null, &>/dev/null, || true, || :, || echo) 2. a GraphQL mutation passing a literal GitHub node id (PRRT_/PR_/BOT_/...) instead of a $variable 3. a gh write with an explicit -R/--repo/repos// target outside the checkout's origin @@ -45,7 +46,7 @@ # --- Risk-pattern detectors -------------------------------------------------------------------------- # Output-discard / force-success tails. Bare `2>&1` is NOT here: it merges stderr into stdout, leaving # the output visible, so it is not suppression (and denying it would break `... 2>&1 | tee log`). -_SUPPRESS = re.compile(r">\s*/dev/null|&>\s*/dev/null|2>\s*/dev/null|\|\|\s*(?:true|:|echo)\b") +_SUPPRESS = re.compile(r">\s*/dev/null|&>\s*/dev/null|2>\s*/dev/null|\|\|\s*(?:true\b|echo\b|:)") # A GitHub global node id literal: an UPPERCASE prefix (PR_, PRRT_, IC_, BOT_, ...) + a long base64url # body, or a legacy MD... base64 id. The uppercase prefix plus a >=12-char body keeps it from matching # an ordinary underscored word in a reply body (e.g. body="fixed_the_thing_now", lowercase prefix). @@ -93,7 +94,8 @@ def classify(cmd, cwd=None, origin=None): # 1. suppressed output on a write if _SUPPRESS.search(cmd): return "deny", ( - "This is a GitHub write with its output discarded (>/dev/null, &>/dev/null, || true). " + "This is a GitHub write with its output discarded or forced to success " + "(>/dev/null, 2>/dev/null, &>/dev/null, || true, || :, || echo). " "A write's result is exactly what must be read: a mutation can succeed on the server " "while the client reports an error. Run it without the output-discarding tail and read " "the response. See AGENTS.md 'Repository Boundaries and Write Safety'." @@ -158,6 +160,8 @@ def classify(cmd, cwd=None, origin=None): ("gh issue comment 5 --body x 2>&1 | tee out.log", "allow", "bare 2>&1 piped to tee is not suppression"), ("gh pr comment 5 --body ok 2>&1", "allow", "bare 2>&1 leaves output visible"), ("gh api repos/ptr727/PlexCleaner/issues/1/comments -f body=x 2>/dev/null", "deny", "stderr discarded on a write"), + ("gh pr close 5 || :", "deny", "force-success no-op tail on a write"), + ("gh pr comment 5 --body x || echo done", "deny", "force-success echo tail on a write"), ("gh api graphql -f query='mutation{addPullRequestReviewThreadReply(input:{pullRequestReviewThreadId:$t,body:$b}){comment{id}}}' -F t=\"$TID\" -F b=\"fixed_the_underscore_bug_here\"", "allow", "underscored reply body is not a node id"), ("gh api graphql -f query='mutation{resolveReviewThread(input:{threadId:$t}){thread{isResolved}}}' -F t=\"TODO_fixit\"", "allow", "short all-caps token is not a node id"), ] From c38d9ed1b3b4a28bd2804adf77ffeebe57a996db Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 20 Jul 2026 10:33:03 -0700 Subject: [PATCH 11/17] Fail gracefully on corrupt settings.json; propagate exit code on Windows (#367) - install.py: catch json.JSONDecodeError when reading an existing settings.json and exit non-zero with a clear message instead of crashing with a traceback, so a user can fix or remove the file and re-run. Verified against a corrupt settings.json: exit 1, plain message, file left untouched. - install.ps1: `exit $LASTEXITCODE` after the launcher block. PowerShell's Stop preference does not trap a native command's non-zero exit, so without this a failed install could be read as success by automation. The bash wrapper already propagates via `exec`. Both are Copilot round-8 findings. Co-Authored-By: Claude Opus 4.8 --- host-setup/agent-safety/install.ps1 | 3 +++ host-setup/agent-safety/install.py | 8 +++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/host-setup/agent-safety/install.ps1 b/host-setup/agent-safety/install.ps1 index 224ce663..06864a5d 100644 --- a/host-setup/agent-safety/install.ps1 +++ b/host-setup/agent-safety/install.ps1 @@ -18,3 +18,6 @@ if (Get-Command "py" -ErrorAction SilentlyContinue) { Write-Error "Python 3 is required and was not found on PATH (tried py -3, python3, python)." exit 1 } + +# Propagate the installer's exit code - a native command's non-zero exit does not stop the script. +exit $LASTEXITCODE diff --git a/host-setup/agent-safety/install.py b/host-setup/agent-safety/install.py index 4266a438..e49b50e2 100644 --- a/host-setup/agent-safety/install.py +++ b/host-setup/agent-safety/install.py @@ -63,7 +63,13 @@ def main(): hook_cmd = f'"{launcher}" "{hook_dst}"' data = {} if settings.exists() and settings.read_text(encoding="utf-8").strip(): - data = json.loads(settings.read_text(encoding="utf-8")) + try: + data = json.loads(settings.read_text(encoding="utf-8")) + 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 pre = data.setdefault("hooks", {}).setdefault("PreToolUse", []) group = next((g for g in pre if g.get("matcher") == "Bash"), None) if group is None: From 0fab30329a7986609e819405e0591f2376303b28 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 20 Jul 2026 10:37:31 -0700 Subject: [PATCH 12/17] Align doc snippets with actual behavior (#367) - README.md: the reference settings.json snippet quotes the launcher ("python3") to match exactly what install.py writes (it quotes the launcher so a sys.executable path with spaces is safe), so a user hand-placing it gets the same command shape. - copilot-instructions.md: the $TID capture prose claimed the jq filters by path and first-comment body, but the shown select filters by path only. State it honestly - the snippet filters by path, and when a file carries more than one unresolved thread (where path alone is ambiguous and head -n 1 would pick the wrong one) add a first-comment-body predicate, shown inline. Both are Copilot round-9 findings. Co-Authored-By: Claude Opus 4.8 --- .github/copilot-instructions.md | 2 +- host-setup/agent-safety/README.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 57001040..fd0af4be 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -147,7 +147,7 @@ gh api graphql -f query=' ' ``` -Reply on a thread, then resolve it. Capture the target thread's id into `$TID` from the listing query above - filter to the thread being answered by its `path` (and, when a file carries more than one thread, its first-comment body), and guard for an empty result so a mutation never runs on a guessed id: +Reply on a thread, then resolve it. Capture the target thread's id into `$TID` from the listing query above - filter to the thread being answered by its `path`, and guard for an empty result so a mutation never runs on a guessed id. When a file carries more than one unresolved thread, `path` alone is ambiguous and `head -n 1` would pick the wrong one, so narrow by first-comment body - the query already fetches `comments(first: 1)` for this - by adding `and (.comments.nodes[0].body | contains(""))` to the `select`: ```sh TID=$(gh api graphql -f query=' diff --git a/host-setup/agent-safety/README.md b/host-setup/agent-safety/README.md index ef600624..62ef8ae7 100644 --- a/host-setup/agent-safety/README.md +++ b/host-setup/agent-safety/README.md @@ -48,7 +48,7 @@ The installer writes this. It is here so you can inspect or hand-place it: { "hooks": { "PreToolUse": [ - { "matcher": "Bash", "hooks": [ { "type": "command", "command": "python3 \"/.claude/hooks/gh-write-guard.py\"" } ] } + { "matcher": "Bash", "hooks": [ { "type": "command", "command": "\"python3\" \"/.claude/hooks/gh-write-guard.py\"" } ] } ] } } From d699ecf7a81c06116af67fd7af827f5e6e8358c1 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 20 Jul 2026 10:44:38 -0700 Subject: [PATCH 13/17] Deny quoted -R cross-repo writes; title-case README H1; verify python is py3 (#367) - gh-write-guard.py: the _EXPLICIT_REPO detector excluded quote chars, so a quoted `-R "owner/repo"` (or `'owner/repo'`) matched nothing and bypassed the cross-origin check entirely - a real enforcement gap of the same class as the || : bug. Allow an optional surrounding quote via a backreference so bare and quoted forms both resolve the target. Added a quoted-cross-origin self-test case; verified quoted/single-quoted/--repo all deny off-origin and a quoted on-origin target still allows. - README.md: title-case the H1 (Agent Write-Safety Kit) per the repo heading convention. - install.ps1: verify a bare `python` fallback is Python 3 before handing it Python 3 syntax (it is Python 2 on some Windows setups, which fails to parse install.py). py -3 and python3 are Python 3 by construction; only the bare `python` last resort needed the guard, matching the bash wrapper. Validated on the Windows box per the kit's stated per-machine self-test caveat - this host has no pwsh. The -R fix is a Copilot round-10 inline finding; the H1 and py3 items are its two low-confidence notes, both correct. Co-Authored-By: Claude Opus 4.8 --- host-setup/agent-safety/README.md | 2 +- host-setup/agent-safety/gh-write-guard.py | 3 ++- host-setup/agent-safety/install.ps1 | 7 +++++++ 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/host-setup/agent-safety/README.md b/host-setup/agent-safety/README.md index 62ef8ae7..6981779a 100644 --- a/host-setup/agent-safety/README.md +++ b/host-setup/agent-safety/README.md @@ -1,4 +1,4 @@ -# Agent write-safety kit +# Agent Write-Safety Kit Per-machine, user-account-scoped guards against an agent making a mis-targeted GitHub **write** under the maintainer's identity. Deploy it as the **first thing on any new system** where Claude Code runs with the `gh` credentials logged in (WSL, Linux, macOS, Proxmox, Windows). diff --git a/host-setup/agent-safety/gh-write-guard.py b/host-setup/agent-safety/gh-write-guard.py index 35bb8ba1..5901f86f 100644 --- a/host-setup/agent-safety/gh-write-guard.py +++ b/host-setup/agent-safety/gh-write-guard.py @@ -53,7 +53,7 @@ _NODE_ID_LITERAL = re.compile(r'^(?:[A-Z]{1,5}_[A-Za-z0-9_\-]{12,}|MD[A-Za-z0-9]{12,})$') # -F/-f name=VALUE, capturing the value - handles "quoted" and bare _FIELD_ASSIGN = re.compile(r"""(?:-F|-f|--field|--raw-field)\s+[A-Za-z_][\w]*=(?P'[^']*'|"[^"]*"|\S+)""") -_EXPLICIT_REPO = re.compile(r"(?:-R|--repo)\s+(?P[^\s'\"]+)") +_EXPLICIT_REPO = re.compile(r"(?:-R|--repo)\s+(?P['\"]?)(?P[^\s'\"]+)(?P=q)") _API_REPO_PATH = re.compile(r"\bgh\s+api\b[^\n|]*?\brepos/(?P[A-Za-z0-9_.\-]+)/(?P[A-Za-z0-9_.\-]+)") @@ -150,6 +150,7 @@ def classify(cmd, cwd=None, origin=None): ("gh api graphql -f query='mutation($t:ID!){resolveReviewThread(input:{threadId:$t}){thread{isResolved}}}' -F t=\"PRRT_kwDOabc123def\"", "deny", "literal node id in a mutation"), ("gh api graphql -f query='mutation($t:ID!){resolveReviewThread(input:{threadId:$t}){thread{isResolved}}}' -F t=\"$TID\"", "allow", "mutation with captured $TID"), ("gh issue comment 5 -R mankatcheung/job-finder --body \"hi\"", "deny", "cross-origin explicit -R"), + ("gh issue comment 5 -R \"mankatcheung/job-finder\" --body \"hi\"", "deny", "cross-origin quoted -R"), ("gh pr create --title x --body y >/dev/null 2>&1", "deny", "suppressed gh pr create"), ("gh api repos/ptr727/PlexCleaner/issues/1/comments -f body=\"ok\"", "allow", "gh api POST to origin"), ("gh api graphql -f query='{repository(owner:\"o\",name:\"r\"){pullRequest(number:1){reviewThreads(first:100){nodes{id}}}}}'", "allow", "graphql READ query"), diff --git a/host-setup/agent-safety/install.ps1 b/host-setup/agent-safety/install.ps1 index 06864a5d..2274fea6 100644 --- a/host-setup/agent-safety/install.ps1 +++ b/host-setup/agent-safety/install.ps1 @@ -13,6 +13,13 @@ if (Get-Command "py" -ErrorAction SilentlyContinue) { } elseif (Get-Command "python3" -ErrorAction SilentlyContinue) { & python3 $script @args } elseif (Get-Command "python" -ErrorAction SilentlyContinue) { + # Verify a bare `python` is Python 3 before handing it Python 3 syntax - it is Python 2 on some setups, + # which would fail to parse install.py. py -3 and python3 above are Python 3 by construction. + & python -c "import sys; sys.exit(0 if sys.version_info[0] == 3 else 1)" 2>$null + if ($LASTEXITCODE -ne 0) { + Write-Error "Found python on PATH but it is not Python 3 (tried py -3, python3, python). Install Python 3." + exit 1 + } & python $script @args } else { Write-Error "Python 3 is required and was not found on PATH (tried py -3, python3, python)." From cf9b1e32319b86a66a47fd5b37ce2ccc90b5083d Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 20 Jul 2026 10:50:06 -0700 Subject: [PATCH 14/17] Do not false-deny a suppression token quoted inside a write body (#367) The _SUPPRESS scan matched anywhere in the command, so a gh issue/pr write whose --body or --title merely mentions `|| true` or `>/dev/null` as text was denied even though the write itself is not suppressed - a real false positive in a repo whose issues and PRs routinely discuss shell commands. Strip quoted argument values before the suppression scan: a real suppression tail is an unquoted shell operator, so stripping quotes never hides an actual footgun, while body text no longer trips the check. The node-id and cross-origin checks still read the original command, so a literal id in a quoted -F is unaffected. Added self-test cases for a quoted-body mention (allow) and a real redirect after a quoted body (deny); the incident still denies. Copilot round-11 low-confidence finding. Co-Authored-By: Claude Opus 4.8 --- host-setup/agent-safety/gh-write-guard.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/host-setup/agent-safety/gh-write-guard.py b/host-setup/agent-safety/gh-write-guard.py index 5901f86f..f2d66e1f 100644 --- a/host-setup/agent-safety/gh-write-guard.py +++ b/host-setup/agent-safety/gh-write-guard.py @@ -47,6 +47,10 @@ # Output-discard / force-success tails. Bare `2>&1` is NOT here: it merges stderr into stdout, leaving # the output visible, so it is not suppression (and denying it would break `... 2>&1 | tee log`). _SUPPRESS = re.compile(r">\s*/dev/null|&>\s*/dev/null|2>\s*/dev/null|\|\|\s*(?:true\b|echo\b|:)") +# A quoted argument value ("..." or '...'). Stripped before the suppression scan so a --body/--title +# that merely mentions `|| true` or `>/dev/null` as text is not mistaken for a real command tail. Real +# suppression tails are unquoted shell operators, so stripping quotes never hides an actual footgun. +_QUOTED_SPAN = re.compile(r"\"[^\"]*\"|'[^']*'") # A GitHub global node id literal: an UPPERCASE prefix (PR_, PRRT_, IC_, BOT_, ...) + a long base64url # body, or a legacy MD... base64 id. The uppercase prefix plus a >=12-char body keeps it from matching # an ordinary underscored word in a reply body (e.g. body="fixed_the_thing_now", lowercase prefix). @@ -91,8 +95,9 @@ def classify(cmd, cwd=None, origin=None): if not _is_gh_write(cmd): return "allow", "" - # 1. suppressed output on a write - if _SUPPRESS.search(cmd): + # 1. suppressed output on a write - scan with quoted argument values removed so a --body/--title + # that only mentions a suppression token as text does not false-deny a legitimate write. + if _SUPPRESS.search(_QUOTED_SPAN.sub("", cmd)): return "deny", ( "This is a GitHub write with its output discarded or forced to success " "(>/dev/null, 2>/dev/null, &>/dev/null, || true, || :, || echo). " @@ -161,6 +166,9 @@ def classify(cmd, cwd=None, origin=None): ("gh issue comment 5 --body x 2>&1 | tee out.log", "allow", "bare 2>&1 piped to tee is not suppression"), ("gh pr comment 5 --body ok 2>&1", "allow", "bare 2>&1 leaves output visible"), ("gh api repos/ptr727/PlexCleaner/issues/1/comments -f body=x 2>/dev/null", "deny", "stderr discarded on a write"), + ("gh issue comment 5 --body \"run make || true to skip errors\"", "allow", "|| true inside a quoted body is not a tail"), + ("gh pr comment 5 --body \"pipe noisy output to >/dev/null\"", "allow", ">/dev/null inside a quoted body is not a redirect"), + ("gh issue comment 5 --body \"see notes\" >/dev/null", "deny", "real redirect after a quoted body still denies"), ("gh pr close 5 || :", "deny", "force-success no-op tail on a write"), ("gh pr comment 5 --body x || echo done", "deny", "force-success echo tail on a write"), ("gh api graphql -f query='mutation{addPullRequestReviewThreadReply(input:{pullRequestReviewThreadId:$t,body:$b}){comment{id}}}' -F t=\"$TID\" -F b=\"fixed_the_underscore_bug_here\"", "allow", "underscored reply body is not a node id"), From fe4f7334feb1d28c30ae2ba42393fa3833bc577a Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 20 Jul 2026 11:02:35 -0700 Subject: [PATCH 15/17] Handle escaped quotes when stripping; mark Verify shell explicitly (#367) - gh-write-guard.py: the quote-stripping regex used a naive "[^"]*" that a \"-escaped quote inside a --body ended early, re-exposing a || true or >/dev/null in the body text and false-denying the write. Use an escape-aware double-quoted span ("(?:\\.|[^"\\])*"); shell single quotes take no escapes, so their form stays literal. Added a self-test with escaped quotes around a suppression token in a body (allow). - README.md: the Verify snippet is POSIX-shell only, so name the heading accordingly and add the Windows PowerShell equivalent, matching the kit's cross-platform claim. Both are Copilot round-12 findings. Co-Authored-By: Claude Opus 4.8 --- host-setup/agent-safety/README.md | 9 ++++++++- host-setup/agent-safety/gh-write-guard.py | 7 +++++-- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/host-setup/agent-safety/README.md b/host-setup/agent-safety/README.md index 6981779a..609f1faa 100644 --- a/host-setup/agent-safety/README.md +++ b/host-setup/agent-safety/README.md @@ -27,13 +27,20 @@ Both are thin wrappers around `install.py`, so every OS runs one tested code pat **Restart Claude Code sessions on the machine afterward** so the new hook and CLAUDE.md load. -## Verify +## Verify (POSIX Shell) ```sh 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) ``` +On Windows PowerShell: + +```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 +``` + Live end-to-end (in any repo): attempt a discarded-output write and confirm the Bash tool is blocked: ```sh diff --git a/host-setup/agent-safety/gh-write-guard.py b/host-setup/agent-safety/gh-write-guard.py index f2d66e1f..5ce846e1 100644 --- a/host-setup/agent-safety/gh-write-guard.py +++ b/host-setup/agent-safety/gh-write-guard.py @@ -49,8 +49,10 @@ _SUPPRESS = re.compile(r">\s*/dev/null|&>\s*/dev/null|2>\s*/dev/null|\|\|\s*(?:true\b|echo\b|:)") # A quoted argument value ("..." or '...'). Stripped before the suppression scan so a --body/--title # that merely mentions `|| true` or `>/dev/null` as text is not mistaken for a real command tail. Real -# suppression tails are unquoted shell operators, so stripping quotes never hides an actual footgun. -_QUOTED_SPAN = re.compile(r"\"[^\"]*\"|'[^']*'") +# suppression tails are unquoted shell operators, so stripping quotes never hides an actual footgun. The +# double-quoted form allows `\"` escapes so an embedded quote does not end the span early; shell single +# quotes take no escapes, so their form is literal. +_QUOTED_SPAN = re.compile(r'"(?:\\.|[^"\\])*"' r"|'[^']*'") # A GitHub global node id literal: an UPPERCASE prefix (PR_, PRRT_, IC_, BOT_, ...) + a long base64url # body, or a legacy MD... base64 id. The uppercase prefix plus a >=12-char body keeps it from matching # an ordinary underscored word in a reply body (e.g. body="fixed_the_thing_now", lowercase prefix). @@ -169,6 +171,7 @@ def classify(cmd, cwd=None, origin=None): ("gh issue comment 5 --body \"run make || true to skip errors\"", "allow", "|| true inside a quoted body is not a tail"), ("gh pr comment 5 --body \"pipe noisy output to >/dev/null\"", "allow", ">/dev/null inside a quoted body is not a redirect"), ("gh issue comment 5 --body \"see notes\" >/dev/null", "deny", "real redirect after a quoted body still denies"), + ("gh issue comment 5 --body \"he said \\\"pipe to >/dev/null\\\" today\"", "allow", "escaped quotes in a body do not end the span early"), ("gh pr close 5 || :", "deny", "force-success no-op tail on a write"), ("gh pr comment 5 --body x || echo done", "deny", "force-success echo tail on a write"), ("gh api graphql -f query='mutation{addPullRequestReviewThreadReply(input:{pullRequestReviewThreadId:$t,body:$b}){comment{id}}}' -F t=\"$TID\" -F b=\"fixed_the_underscore_bug_here\"", "allow", "underscored reply body is not a node id"), From 8353f17d883fe36402c2174445c30d62c66c6306 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 20 Jul 2026 11:06:13 -0700 Subject: [PATCH 16/17] Match the README to the installed CLAUDE.md heading (#367) The README described the installed section as `## GitHub write safety`, but the snippet's heading is `## GitHub Write Safety (Any Project, Every Session)` (title-cased earlier). Quote the actual heading so a user grepping for it finds the real one. Copilot round-13 finding. Co-Authored-By: Claude Opus 4.8 --- host-setup/agent-safety/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/host-setup/agent-safety/README.md b/host-setup/agent-safety/README.md index 609f1faa..ee1062b4 100644 --- a/host-setup/agent-safety/README.md +++ b/host-setup/agent-safety/README.md @@ -7,7 +7,7 @@ Per-machine, user-account-scoped guards against an agent making a mis-targeted G Into `~/.claude/` (or `%USERPROFILE%\.claude\` on Windows): - **`hooks/gh-write-guard.py`** - a PreToolUse hook that denies the three write footguns behind the cross-repo comment incident: a state-changing `gh` call whose output is discarded, a GraphQL mutation passing a **literal** node id instead of a `$variable`, and a `gh` write whose explicit target is outside the checkout's `origin`. Reads and everything else pass through. It fires even in autonomous / bypass-permissions sessions, which is how the incident happened. -- **A `## GitHub write safety` section in `CLAUDE.md`** - the same three rules as behavioral guidance, loaded into every session on the machine (including ad-hoc work outside any project). It mirrors the committed `AGENTS.md` "Repository Boundaries and Write Safety" rules, which only reach fleet repos. +- **A `## GitHub Write Safety (Any Project, Every Session)` section in `CLAUDE.md`** - the same three rules as behavioral guidance, loaded into every session on the machine (including ad-hoc work outside any project). It mirrors the committed `AGENTS.md` "Repository Boundaries and Write Safety" rules, which only reach fleet repos. The hook is the mechanical backstop. The CLAUDE.md rules and the carried AGENTS.md rules are the behavioral layer. Prose alone is not enough - the incident happened under prose rules - so both ship. From 6b377c3169d7f8644b69fdc6b621d4235bd3fa7e Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 20 Jul 2026 11:10:09 -0700 Subject: [PATCH 17/17] Guarantee a single hook registration across multiple Bash groups (#367) The settings merge updated only the first matcher=="Bash" group, so the "exactly one group carries our hook" comment overstated the behavior when settings.json already had more than one Bash group (a re-run could leave a stale entry in another group). Strip our hook from every existing group first, then register it in a single Bash group, so exactly one group carries it regardless of the starting shape. Verified against a settings.json with two Bash groups (one holding a stale entry), a Write group, and unrelated keys: one gh-write-guard entry after install, all other hooks and keys preserved, idempotent on re-run. Copilot round-14 finding. Co-Authored-By: Claude Opus 4.8 --- host-setup/agent-safety/install.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/host-setup/agent-safety/install.py b/host-setup/agent-safety/install.py index e49b50e2..05896972 100644 --- a/host-setup/agent-safety/install.py +++ b/host-setup/agent-safety/install.py @@ -57,7 +57,7 @@ def main(): return 1 print(" hook self-test: PASS") - # 2. Register in settings.json: exactly one PreToolUse/Bash group carrying our hook command. + # 2. Register our hook command in settings.json so exactly one PreToolUse/Bash group carries it. 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}"' @@ -71,13 +71,17 @@ def main(): ) return 1 pre = data.setdefault("hooks", {}).setdefault("PreToolUse", []) + # Strip our hook from every existing group first, so a re-run never leaves a duplicate behind even + # when settings.json already has more than one Bash group. Then register it in a single Bash group. + for g in pre: + hooks_list = g.get("hooks") + if isinstance(hooks_list, list): + hooks_list[:] = [h for h in hooks_list if "gh-write-guard" not in str(h.get("command", ""))] group = next((g for g in pre if g.get("matcher") == "Bash"), None) if group is None: group = {"matcher": "Bash", "hooks": []} pre.append(group) - entries = group.setdefault("hooks", []) - entries[:] = [h for h in entries if "gh-write-guard" not in str(h.get("command", ""))] - entries.append({"type": "command", "command": hook_cmd}) + group.setdefault("hooks", []).append({"type": "command", "command": hook_cmd}) settings.write_text(json.dumps(data, indent=2) + "\n", encoding="utf-8") print(f" settings -> {settings} (PreToolUse/Bash hook registered)")