Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion plugins/guardrails/.claude-plugin/plugin.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"$schema": "https://json.schemastore.org/claude-code-plugin-manifest.json",
"name": "guardrails",
"version": "0.20.0",
"version": "0.21.0",
"description": "Twelve safety guards that block secret/credential writes, hardcoded machine-specific paths, git hook-bypass attempts, irreversible git operations (force-push, reset --hard, worktree-wide checkout/restore discards), Bash file-write workarounds that circumvent Write/Edit hooks, multi-line `git commit -m` messages (an actual-newline `-m` mangles across shells; single-line `-m` passes), commit subjects and gh pr create titles that violate the repo's tracked team convention (when one is declared in .claude/source-control.md), (advisory) hallucinated CLI flags, (advisory) /plugin:skill references that do not resolve, (advisory) markdown citing a repo path the repo's own history shows was removed, (advisory, opt-in) un-throttled Workflow fan-out that risks burst 529s, and (advisory, opt-in) direct gh pr create calls bypassing this marketplace's own pull-request skill — each independently toggleable.",
"author": {
"name": "Melodic Software",
Expand Down
84 changes: 84 additions & 0 deletions plugins/guardrails/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,90 @@
All notable changes to the `guardrails` plugin are documented here. Format follows
[Keep a Changelog](https://keepachangelog.com/en/1.1.0/); this plugin uses semantic versioning.

## [0.21.0]

### Fixed

- **`block-dangerous-git`'s hash-width probe ignored a wrapper's chdir, so an unsafe
`--force-with-lease` passed.** A lease expectation is judged against the hash width of the
repository the push will run in, and the probe replays git's own repository-locating globals to
find it. It could not replay a WRAPPER's relocation: `collect_git_locating_opts` reads only the
slice between the git word and the subcommand — as it must, since that walk cannot know which of
`env`'s or `sudo`'s options take a value — so `env -C <sha256-repo> git push
--force-with-lease=main:<40-hex>` probed the invoking SHA-1 directory, read the 40-hex expectation
as an immutable object id, and allowed the push. Where git actually runs, that same word is an
ordinary movable ref name, which is the exact hole `--force-with-lease` exists to close.
`hook::git_resolve_index` already records the relocation in `HOOK_GIT_RESOLVED_WRAPPER_DIRS` — it
is the only parser that can tell a real `env -C <dir>` from the `-C` in `env -u -C git`, which
moves nothing — and the probe now replays it as leading `-C` words, ahead of git's own, so the two
compose in execution order under git's rules rather than being modelled. Covered for `env -C`,
`env --chdir=`, `sudo -D`, the composition with git's own `-C`, and the `env -u -C` non-chdir.
**Acceptance behavior changes** (hence a minor bump): a wrapped push whose lease is a movable name
where git runs is refused where it was allowed, and one whose lease is a real object id there is
allowed where the misplaced probe refused it.

- **`skill-reference-verify` reported references an Edit never touched, missed short substring
edits, and could spend its whole 30-second timeout on one large Edit.** Partial-Edit
reconstruction located the hunk by line and then filtered the whole line by word token. All three
defects were that filter: an untouched broken reference sharing a physical line with the hunk was
readmitted by any word it happened to share (`legacy` in both the edited prose and
`` `/alpha:ghost-legacy` ``); an Edit replacing fewer than four lowercase characters produced no
token at all, so reconstruction gave up and every such edit went uncovered; and locating spent two
full-file `grep` processes per hunk line, which a thousand-line Edit turned into a timeout — an
advisory lost entirely, after delaying the tool call to get there. Reconstruction now reads the
file once and keeps only the inline-code spans whose extent OVERLAPS the located anchor. Overlap
is exact where the token filter was approximate, and has no minimum length to clear. The
occurrence-uniqueness gate is unchanged: an anchor that cannot say which occurrence the edit
landed on is still dropped rather than unioned.

The timeout half needed both halves of its cost removed. Dropping the subprocesses left the
per-line RESCAN, which is anchors TIMES file size — measured on a Windows/Git Bash host, a
thousand span-free hunk lines still cost 82 s against the 30-second budget. The hunk is written to
disk contiguously, so it is now located WHOLE: one scan for the entire edit, and the span set is
the same one the per-line walk produced, since a line anchor's extent is the text the edit wrote
on that line and the whole hunk's extent is the union of exactly those. The same measurement is
now 11 s at a thousand lines and 11 s at four thousand — the hunk-size term is gone. Locating
whole is also strictly better scoping: a hunk whose every line repeats but whose whole text does
not used to be dropped as ambiguous line by line, and now resolves to the one place it names. The
per-line walk survives as a fallback for a hunk that is no longer on disk verbatim — another
PostToolUse hook reformatting the file between the write and this read is the realistic cause.

Measuring the scan itself then contradicted the bound that had been placed on it. One scan is not
linear in file size, it is QUADRATIC — 0.07 s at 32 KiB, 0.24 s at 64, 1.07 s at 128, 3.94 s at
256 on the same host, because bash's `%%` pattern strip walks the string rather than indexing it.
A 4 MiB file, which the previous cap allowed, is ~18 minutes for a SINGLE scan, so the guard's
worst case had never actually been bounded, only moved. Reconstruction now stops above 128 KiB,
and the fallback's anchor cap falls along that same curve instead of being a flat count — 58
anchors at 32 KiB, 14 at 64, 3 at 128. Above the cap the direct hunk scan is unaffected, so a
complete reference is still reported and only partial-edit recovery stops, which is this guard's
permitted failure direction. Both numbers are calibrated end to end against the hook rather than
from the isolated curve, which understates the cost: it times an anchor that matches near the end,
where one strip walks the file and the second is free, while a no-match strip walks it twice and
the whole-hunk probe pays a scan before the fallback runs at all. Covered by a case that puts a
large file and the fallback path TOGETHER, which neither the timing case (whole-hunk fast path)
nor the correctness cases (three lines) reached. That case asserts what the cap DOES rather than
how long it takes — one reference inside the cap is still reported, one past it is not — because a
wall-clock bound there measures the host: the same fixture read 21 s loaded and a smaller one 23 s,
against an isolated scan of ~1 s at that size. A timing assertion that noisy fails on load and
passes on a regression that happens to run on a quiet box.

- **`skill-reference-verify` reported a valid command as unresolved when its plugin declared custom
skill paths.** Resolution hard-coded `plugins/<plugin>/skills/`, but a manifest's `skills` key
holds a path or array of paths that ADD to that directory, and a declared path may point straight
at a directory holding `SKILL.md` ([Plugins
reference](https://code.claude.com/docs/en/plugins-reference), "Path behavior rules"). A skill
loaded from a declared location now resolves, as does the documented single-skill layout (a root
`SKILL.md` with no `skills/` subdirectory and no `skills` key). That layout is honoured under its
stated conditions only — a root `SKILL.md` beside a populated `skills/` is not loaded by Claude
Code, so accepting it would suppress the advisory for a command that does not exist.

The advisory's own text was wrong the same way the resolution had been: it named
`plugins/<plugin>/skills/` as the place searched, whatever the manifest declared. The message now
lists the directories the search actually covered, built from the same `skill_roots` the
resolution used. The advisory ends by telling the reader to confirm against the tree, and pointing
them at the wrong part of it is the one instruction that cannot survive being wrong. A plugin
using the conventional layout still reads exactly as before.

## [0.20.0]

### Changed
Expand Down
17 changes: 16 additions & 1 deletion plugins/guardrails/hooks/block-dangerous-git.sh
Original file line number Diff line number Diff line change
Expand Up @@ -264,13 +264,28 @@ lease_expect_is_immutable() {
# git word and the subcommand. Two-word options whose value is consumed the same
# way hook::git_resolve_subcommand consumes it, so the walk cannot desynchronize
# from the parser's own.
#
# The slice starts AT the git word, as it must: this walk cannot know which of a
# wrapper's own options take a value, so reading `env -u -C git …` as git's `-C`
# would relocate the probe on an option that belongs to `env`'s `-u`. That
# scoping is what makes the replay below necessary — a wrapper's chdir is a real
# relocation the slice deliberately cannot see, and hook::git_resolve_index is
# the single parser that can tell the two apart.
#
# A wrapper's chdir happens before git starts, so git's own locating options
# compose onto it: it is replayed as LEADING `-C` words, which git applies
# cumulatively in argv order, and the composition then falls out of git's own
# rules rather than being modelled here.
# shellcheck disable=SC2329 # reached via the hook::bash_parse_segments callback chain
collect_git_locating_opts() {
local gi="$1" sub_idx="$2"
shift 2
local -a w=("$@")
local j=$((gi + 1))
local j=$((gi + 1)) wdir
git_locating_opts=()
for wdir in ${HOOK_GIT_RESOLVED_WRAPPER_DIRS[@]+"${HOOK_GIT_RESOLVED_WRAPPER_DIRS[@]}"}; do
git_locating_opts+=(-C "$wdir")
done
while ((j < sub_idx)); do
case "${w[j]}" in
-C | --git-dir | --work-tree | --namespace)
Expand Down
21 changes: 21 additions & 0 deletions plugins/guardrails/hooks/block-dangerous-git.test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,27 @@ run "git --git-dir=<sha256-repo>/.git push --force-with-lease=main:<40-hex> (blo
run "git -C <not-a-repo> push --force-with-lease=main:<40-hex> (width undeterminable, fail-closed block)" "git -C $NOT_A_REPO push --force-with-lease=main:$SHA1_OID origin main" 2
run "git -c x=y -C <sha256-repo> push --force-with-lease=main:<64-hex> (config value skipped, not mistaken for a path, allowed)" "git -c x=y -C $REPO_SHA256 push --force-with-lease=main:$SHA256_OID origin main" 0

# A WRAPPER's chdir moves git just as git's own -C does, and the width probe must
# follow it. The locating-option walk is deliberately scoped to `[git, subcommand)`
# — it cannot know which wrapper options take a value — so the relocation reaches
# it only through hook::git_resolve_index, the one parser that can tell `env -C
# <dir>` (a real chdir) from the `-C` in `env -u -C git` (the operand of -u).
# Losing it probed the INVOKING directory, where a 40-hex lease reads as an object
# id while the push runs where it is a movable ref name.
run "env -C <sha256-repo> git push --force-with-lease=main:<40-hex> (40-hex is a name where git runs, blocked)" "env -C $REPO_SHA256 git push --force-with-lease=main:$SHA1_OID origin main" 2
run "env -C <sha256-repo> git push --force-with-lease=main:<64-hex> (object id where git runs, allowed)" "env -C $REPO_SHA256 git push --force-with-lease=main:$SHA256_OID origin main" 0
run_in "$REPO_SHA256" "env -C <sha1-repo> git push --force-with-lease=main:<64-hex> (64-hex is a name where git runs, blocked)" "env -C $REPO_SHA1 git push --force-with-lease=main:$SHA256_OID origin main" 2
run "env --chdir=<sha256-repo> git push --force-with-lease=main:<40-hex> (long form, blocked)" "env --chdir=$REPO_SHA256 git push --force-with-lease=main:$SHA1_OID origin main" 2
run "sudo -D <sha256-repo> git push --force-with-lease=main:<40-hex> (sudo's chdir, blocked)" "sudo -D $REPO_SHA256 git push --force-with-lease=main:$SHA1_OID origin main" 2
# The mirror image: an option that only LOOKS like a chdir must not move the probe.
# GNU env's `-u NAME` consumes the next word, so the `-C` in `env -u -C git` is
# the variable name and git never moves — the lease is judged where it stands.
run "env -u -C git push --force-with-lease=main:<40-hex> (-C is -u's operand, no chdir, allowed)" "env -u -C git push --force-with-lease=main:$SHA1_OID origin main" 0
# A wrapper chdir composes AHEAD of git's own -C, in that order: env relocates
# before git starts, so a relative `-C` resolves against the wrapper's directory.
run "env -C <sha256-parent> git -C <basename> push --force-with-lease=main:<64-hex> (composed, allowed)" "env -C $TEST_TMPDIR git -C repo-sha256 push --force-with-lease=main:$SHA256_OID origin main" 0
run "env -C <sha256-parent> git -C <basename> push --force-with-lease=main:<40-hex> (composed, blocked)" "env -C $TEST_TMPDIR git -C repo-sha256 push --force-with-lease=main:$SHA1_OID origin main" 2

# The width probe is the guard's only subprocess, and a command may carry many
# lease expectations. Counting real git invocations catches the cache being lost
# to a subshell — a per-expectation probe would spawn one git each and push a
Expand Down
Loading