Agent write-safety: repo-scope + never-fabricate-id + never-suppress-write rules, runbook fix, and a portable enforcement kit (#364/#365/#366) - #367
Conversation
…tgun (#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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds defense-in-depth guidance and tooling to prevent mis-targeted GitHub writes by autonomous agents (repo boundary enforcement, never-fabricate-IDs, and never-suppress-write-output), addressing the cross-repo comment incident described in #364/#365/#366.
Changes:
- Add a new carried
AGENTS.mdsection ("Repository Boundaries and Write Safety") and include it in the spec’s carried-sections allowlist. - Fix the Copilot runbook thread-reply example to require capturing thread IDs from live queries and to avoid suppressing mutation output.
- Introduce a per-machine “agent write-safety kit” (hook + installer + CLAUDE.md snippet) under
host-setup/agent-safety/.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
spec/files.json |
Adds the new write-safety section to the carried-sections baseline. |
AGENTS.md |
Introduces the repo-scope / never-fabricate-id / never-suppress-write rules as a carried section. |
.github/copilot-instructions.md |
Updates the reply/resolve runbook to capture IDs from live queries and to show write outputs. |
host-setup/agent-safety/README.md |
Documents installation and verification for the per-machine safety kit. |
host-setup/agent-safety/install.sh |
Bash wrapper to run the cross-platform installer. |
host-setup/agent-safety/install.ps1 |
PowerShell wrapper to run the cross-platform installer on Windows. |
host-setup/agent-safety/install.py |
Cross-platform, idempotent installer that deploys and registers the hook and CLAUDE.md block. |
host-setup/agent-safety/gh-write-guard.py |
PreToolUse hook to deny specific high-risk GitHub write patterns. |
host-setup/agent-safety/claude-md-safety.md |
Marker-delimited CLAUDE.md block with the same safety rules for all sessions. |
host-setup/agent-safety/.markdownlint-cli2.jsonc |
Local markdownlint override for the snippet’s intentional H2 start. |
.gitattributes |
Pins LF endings for new shebang-executed Python entrypoints. |
.editorconfig |
Pins LF endings for the agent-safety kit’s Python scripts. |
…it 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) <noreply@anthropic.com>
…emicolon 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
host-setup/agent-safety/gh-write-guard.py:100
- The deny reason's examples don't match what the suppression detector actually blocks.
_SUPPRESSalso matches2>/dev/nulland|| echo/|| :, so the message should describe the broader class (discarding output or forcing success) rather than listing an incomplete subset.
return "deny", (
"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'."
)
…pilot #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) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
host-setup/agent-safety/gh-write-guard.py:12
- The docstring's example list says the discarded-output detector covers
|| trueonly, but_SUPPRESSalso matches other force-success tails like|| :and|| echo(line 48). Aligning the docstring with the implementation avoids confusion about which patterns are denied.
1. a state-changing gh call whose output is discarded (>/dev/null, &>/dev/null, 2>/dev/null, || true)
host-setup/agent-safety/gh-write-guard.py:100
- The deny reason lists only
>/dev/null,&>/dev/null, and|| true, but the suppression detector also denies2>/dev/nulland other force-success tails (see_SUPPRESSon line 48). Update the message to either includee.g.or list a representative set that matches the implementation so users understand why a command was blocked.
"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'."
)
- 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 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
host-setup/agent-safety/README.md:59
- AGENTS.md requires reference-style links in Markdown docs (except AGENTS.md and .github/copilot-instructions.md). This README uses an inline link to issue #365; please convert it to a reference-style link and define the reference at the bottom of the file.
- **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.
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 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
host-setup/agent-safety/README.md:1
- Top-level heading is not in title case. AGENTS.md documentation conventions require headings to follow the title-case-with-short-bind-words rule (AGENTS.md:123).
# Agent write-safety kit
…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 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
host-setup/agent-safety/gh-write-guard.py:100
- The deny message lists only some of the suppression / force-success patterns that actually trigger a deny. The guard also denies
2>/dev/nulland|| echo/|| :, so the user-facing reason should include those to avoid confusion when a command is blocked.
"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'."
)
AGENTS.md:24
- This list of output-suppression examples is used as concrete guidance, but it omits other discard patterns the repo's safety kit actively blocks (notably
2>/dev/nulland|| :). Including them here helps keep the behavioral rule aligned with the mechanical enforcement and avoids readers treating the examples as an allowlist.
- **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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
host-setup/agent-safety/README.md:1
- The H1 heading is not in title case (repo doc convention). Update it so the hyphenated compound is capitalized and the heading reads as title case.
# Agent write-safety kit
host-setup/agent-safety/install.ps1:16
- The PowerShell wrapper falls back to running
install.pywithpythonwithout verifying it is Python 3. Ifpythonis Python 2 (still common on some Windows setups), the script will fail with a SyntaxError before install.py can print the friendly version check message.
} elseif (Get-Command "python" -ErrorAction SilentlyContinue) {
& python $script @args
… 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 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
host-setup/agent-safety/gh-write-guard.py:49
- The
_SUPPRESSregex is described as detecting output-discard/force-success "tails", but it currently matches these tokens anywhere in the command string. That can false-deny legitimategh issue/pr ... --body "... || true ..."or comments containing>/dev/nullin the body, even when the write itself is not suppressed. Anchoring this detector to end-of-command keeps the check focused on the actual footgun while reducing false positives.
# 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|:)")
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 <noreply@anthropic.com>
) - 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Addresses the cross-repo comment incident (#364, #365, #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 (mankatcheung/job-finder#15) and the write succeeded under the maintainer's identity, while the suppressed output read as a harmless failure.Three root causes -> three rules, applied in three guard layers.
1. Carried template rule (travels to the fleet)
New
AGENTS.mdsection## Repository Boundaries and Write Safety(after Foundational Principles, beside the existing never-force-push safety rules), added to the carried-sections allowlist inspec/files.json:2. Runbook fix (
.github/copilot-instructions.md, carried whole)The reply/resolve example modeled the footgun: a disembodied
-F threadId="PRRT_..."literal disconnected from thereviewThreads(first:100)query above it. Rewritten to capture$TIDfrom that live query with an empty-result guard, use-F threadId="$TID", show the mutation output, and confirmisResolvedbefore closing. Added an intra-doc cross-reference to the new section. (TherequestReviewsexample already captured its ids from live queries.)3. Portable per-machine safety kit (
host-setup/agent-safety/, not carried)The prose layers are necessary but not sufficient - the incident happened under a "Verification Discipline" section. This adds the mechanical backstop and makes it deployable everywhere as the first thing on a new system:
gh-write-guard.py- a PreToolUse hook that DENIES the three write footguns (suppressed-output mutation, literal-node-id mutation, cross-origin explicit target). Reads pass through. It fires even in autonomous / bypass-permissions sessions - the exact condition of the incident.--selftestruns an 11-case decision matrix.claude-md-safety.md- the same rules as host-scope guidance, appended into~/.claude/CLAUDE.mdso every session on a machine inherits them, including ad-hoc work outside any project.install.py+ thininstall.sh/install.ps1wrappers - one tested cross-platform installer: self-tests the hook before registering it, merges the settings.json entry without clobbering other keys, updates the CLAUDE.md block in place. Fully idempotent.This is the rollout #365 tracks; its per-machine checklist becomes "run
install.sh/install.ps1".Deliberately out of scope
spec/audit.pychange: section carriage is intent-checked, not mechanized; a literal-id grep on a whole-carried runbook is fragile. Raised separately: "Verification Discipline" is also absent from the carried-sections allowlist - a follow-up, not bundled here.Verification (driven, not assumed)
--selftest: 11/11 incl. the exact incident (deny), the empty-id guard idiom (allow), reads (allow).hookSpecificOutput/permissionDecision: "deny"for the incident command; reads and non-Bash tools pass through; malformed input never interferes.settings.jsonmerge preserves other keys.spec/validate.py, markdownlint (whole-repo**/*.md), editorconfig-checker: clean.Note on the section-name/anchor lockstep: the copilot-instructions cross-reference
#repository-boundaries-and-write-safetyand thespec/files.jsonsectionsentry both depend on the section title; rename them together if ever changed.🤖 Generated with Claude Code