feat(guardrails): add asserted-path and skill-reference claim guards

[kyle-sexton]

Closes #1270

Summary

Two advisory PostToolUse guards on Write|Edit, siblings of cli-flag-verify — same defect class (a confident specific that was never checked), different oracles.

asserted-path-verify flags a repo-relative path asserted in markdown, in a code span or a link target, that does not exist in the working tree. Deterministic oracle, advisory action: a citation can be deliberately forward-looking and PostToolUse runs after the write, so it cannot tell.

The first-segment gate is what keeps it quiet — a candidate is adjudicated only when its leading directory exists in the repo. So another project's paths, package paths, globs, placeholders, and third-party references never fire. Line and range citation suffixes are stripped, which matters because path.md:576-580 is this repo's dominant citation form.

skill-reference-verify flags a /plugin:skill reference that does not resolve. Declared detect-then-judge, not deterministic: globbing a plugins tree is exact only where the reference is locally owned, so the finding is a prompt for a human verdict — never a determination, never an auto-fix. Gated twice: inert outside a marketplace repo, and within one it adjudicates only a plugin this repo's own manifests own. Resolution goes through manifest name and skill frontmatter name, so a renamed directory still matches.

Both follow the cli-flag-verify pattern exactly: diff-scope only (scan what the call wrote, never re-read from disk), per-guard kill switch defaulting true, statusMessage per handler, telemetry with repo-relative path redaction, lib/hook-utils.sh unchanged.

A third guard was scoped and dropped. A version-versus-manifest guard had no buildable trigger: its only in-repo shape is already covered by check-changelog-parity.sh --check-bump, and the residual prose surface is historical, minimum-floor, and planned version claims that a manifest-compare oracle reads wrong. The full enumeration — checked and unchecked sets — is recorded on #1270 rather than shipped as a guard that never fires correctly.

Boy Scout. README counts were stale before this change: prose said "nine safety guards" and the table omitted block-convention-violation while ten were wired. Counts are now measured against the manifest toggle set, the missing row is present, and a new enforceability-tier section states each guard's oracle class so the detect-then-judge guard cannot be read as deterministic.

Test plan

Contract tests, both run to completion (each hook invocation costs 10-20s on Windows/Git Bash — process spawn plus the bounded stdin read, not hook logic — so a full pass takes 10-20 min locally):

• asserted-path-verify.test.sh — 45/45, exit 0
• skill-reference-verify.test.sh — 38/38, exit 0
• require-jq-notice-isolation.test.sh — 2/2; now proves 11 unique notice keys across 11 hooks, covering both new hooks by glob discovery

Each suite carries MUST-fire, MUST-stay-quiet, kill-switch, empty-stdin, missing-prerequisite, and telemetry cases. Notable stay-quiet coverage: line/range citation suffixes, anchor fragments, globs, placeholders, ellipses, URLs, absolute and home-relative paths, parent escapes, extensionless refs, vendor trees, unbackticked prose, multi-token code spans, non-markdown files, non-Write|Edit tools, files outside CLAUDE_PROJECT_DIR, non-git directories, and — for the reference guard — unowned plugins, manifest-less directories, and uppercase non-command-shaped tokens.

Repo gates, all pass: validate-plugins.sh (incl. catalog), check-silent-skips.sh, check-changelog-parity.sh --check and --check-bump origin/main.

Runtime jq-removal is not portably simulable — an isolated bin dir without jq cannot host bash + coreutils across Git Bash and Linux, the constraint secret-pattern-detection.test.sh and require-jq-notice-isolation.test.sh both already document. Both suites assert the fail-open guard through the shared helper instead, and key uniqueness is proven by the cross-hook test.

Related

Closes #1270, whose scope was amended from three guards to two during the build — see the "Excluded because already covered" section there for the dropped guard's enumeration.

Merge-order note. Three open PRs touch plugins/guardrails/plugin.json and CHANGELOG.md: #1097, #1085, #853. All three are currently mergeStateStatus=DIRTY and unchanged since 2026-07-23, so this PR was rebased onto current main rather than held behind them — whoever rebases those must re-pick a version regardless. 0.15.0 is correct off 0.14.2 on main today. #853 additionally edits guardrails-test-helpers.sh, which both new tests source, so it should re-run these two suites after rebasing.

Root README.md carries a one-line catalog-row change because validate-plugins.sh gates it; regenerated with node scripts/generate-catalog.mjs.

Review status, stated plainly: the two contract suites and the repo gates are verified, and an independent noise measurement across every tracked *.md is in progress to quantify the real-world false-positive rate — the thing docs/conventions/hook-precision/README.md cares most about for an advisory guard. A subagent security review was dispatched and never reported, so this PR is opened to engage the repo's own security-review, review, and cross-vendor review rather than to bypass them. Please do not merge on my say-so alone; two new shell hooks executing on every Write|Edit is a code-execution trust surface.

[claude]

Claude finished @kyle-sexton's task in 3m 33s —— View job

---

PR Review: feat(guardrails): add asserted-path and skill-reference claim guards

• Gather context and understand request
• Read all changed files
• Review asserted-path-verify.sh
• Review skill-reference-verify.sh
• Review test suites
• Review manifest/plugin.json changes
• Review README and CHANGELOG updates
• Synthesize findings

---

This is a thorough, well-considered addition. The design is solid, the test coverage is excellent (45+38 cases), and both guards correctly implement the same diff-scope pattern as cli-flag-verify. Below are my findings, ordered by severity.

---

Documentation bug: per-hook kill switches table is incomplete

The README's "Per-hook kill switches" section lists only 9 options, but the plugin now wires 12 guards with 12 toggles. Three entries are missing from the table:

Missing guard	Missing option key
block-convention-violation	block_convention_gate_enabled
asserted-path-verify	asserted_path_verify_enabled
skill-reference-verify	skill_reference_verify_enabled

The PR description says counts are now "measured against the manifest toggle set," but the kill-switch reference table (where a user looking for the exact option key would go) was not updated to match. block-convention-violation was already present in plugin.json before this change; asserted_path_verify_enabled and skill_reference_verify_enabled are new. All three are missing from the table.

Fix this →

---

normalize_candidate: trailing-colon edge case

asserted-path-verify.sh:91

t="${t%%:[0-9]*}"

In bash globbing, [0-9]* means one required digit followed by any characters — so the strip only fires when at least one digit follows the colon. A token like `docs/file.md:` (trailing colon, no digits) is not stripped, producing the candidate docs/file.md:. On Linux a file named literally file.md: will not exist, so this fires a false positive finding for a token no one would intentionally write. Extremely unlikely in practice, but the test suite has no stay-quiet case for it.

The fix is to also strip a bare trailing colon:

t="${t%%:[0-9]*}"
t="${t%:}"          # strip bare trailing colon

---

skill_resolves: sed scan is not frontmatter-bounded

skill-reference-verify.sh:96

fname=$(sed -n '1,40{s/^name:[[:space:]]*…/\1/p;}' "$sd" 2>/dev/null | head -1)

The scan covers lines 1–40 without stopping at the --- closing delimiter of the YAML frontmatter. If a skill's frontmatter name is missing but its body contains a left-margin name: field within the first 40 lines, that body value would resolve the skill name — making the hook believe the skill exists under an incorrect name. This is a false-negative risk: skill_resolves() returns true when it shouldn't.

The hardening is to stop at the closing ---:

fname=$(awk '/^---/{if(NR>1){exit}} /^name:[[:space:]]*/{…; exit}' "$sd" 2>/dev/null)

or to restrict the sed address to lines between --- delimiters. Low likelihood, but it affects correctness of the oracle.

---

shopt -u nullglob does not restore prior state

skill-reference-verify.sh:71-73

shopt -s nullglob
manifests=("$PLUGINS_DIR"/*/.claude-plugin/plugin.json)
shopt -u nullglob

If the sourced hook-utils.sh or a calling environment had nullglob already set, this unconditionally disables it for the rest of the hook process. In the current codebase hooks run in isolated subshells so this is not a live bug, but it's fragile — if nullglob matters to hook-utils.sh globs or future additions, this silently breaks them. The defensive fix is to save and restore:

local _nullglob; _nullglob=$(shopt -p nullglob)
shopt -s nullglob
manifests=(…)
eval "$_nullglob"

---

Minor: emit_tokens scans only single-backtick spans

asserted-path-verify.sh:76

The grep pattern '`[^`]+`' only matches single-backtick code spans; double-backtick spans ( docs/nope.md ) silently skip. This is an intentional design tradeoff (CommonMark full conformance would complicate the regex significantly), but it's worth noting that a path cited inside double-backtick spans will not fire the guard. The dominant citation form in this repo is single-backtick, so the impact is low.

---

Strengths

• Diff-scope discipline is correct. Both hooks scan only what the current tool call wrote (new_string for Edit, content for Write), never re-reading from disk. This is the right design for PostToolUse advisory guards and is consistent with cli-flag-verify.
• First-segment gate eliminates the dominant false-positive class. Requiring that the leading path segment be a real directory before adjudicating is exactly the right heuristic — another project's paths, package paths, and illustrative examples all fail this gate cleanly.
• Double gate in skill-reference-verify is appropriately conservative. Plugins-root gate (marketplace check) + plugin-scope gate (ownership check) ensures the hook stays silent in all consuming contexts where it cannot be authoritative.
• normalize_candidate handles the right exclusion set. URLs, absolute paths, home-relative, parent-escaping, globs, placeholders, ellipses, vendor trees, extensionless ambiguous refs, and multi-token spans are all correctly dropped.
• Advisory tier and detect-then-judge labeling are correctly applied. The enforceability tier section in README and the in-hook advisory message both correctly frame skill-reference-verify as a human-verdict prompt, not a determination. This addresses the risk the PR description flags explicitly.
• Telemetry has proper PII redaction. The file_rel stripping (falling back to basename if absolute) is the right pattern; no developer username or absolute path leaks into the telemetry envelope.
• Test suite is comprehensive. MUST FIRE, MUST STAY QUIET, kill switch, empty stdin, missing prerequisite, and telemetry sections cover the cases that matter. Notable: the non-git-directory fallback test documents that the fallback root is a real adjudication path, not dead code.
• Manifest name-vs-directory divergence is tested. The beta-dir/beta and legacy-dir/renamed-command cases in skill-reference-verify.test.sh prove the manifest resolution path, not just the happy directory-name path.

---

Summary: The two blocking issues to address before merge are the kill-switch table gap in the README (documentation correctness that users would hit immediately when looking for option names) and the sed frontmatter-boundary issue in skill_resolves (oracle correctness, low probability but clean to fix). The trailing-colon and nullglob issues are both hardening items worth capturing.

[claude]

Claude finished @kyle-sexton's task in 5m 16s —— View job

---

Security Review — PR #1284

• Load tools
• Read PR diff and changed files
• Analyze asserted-path-verify.sh for security issues
• Analyze skill-reference-verify.sh for security issues
• Analyze hook manifest (hooks.json) and plugin manifest changes
• Post findings

---

Summary

No CRITICAL or IMPORTANT findings. Both new hooks follow the same security posture as their sibling cli-flag-verify: all user-controlled content flows through printf '%s' → jq → bash [[ ]] string comparisons, with no eval, no unquoted variables in exec positions, and jq --arg used for all output JSON construction. One SUGGESTION-level filter gap is documented below.

---

Findings

SUGGESTION — normalize_candidate filter gap: terminal .. segments (HIGH confidence)

File: plugins/guardrails/hooks/asserted-path-verify.sh, lines 116–123

  # Escapes the tree; not resolvable against the repo root.
  ../* | */../*) return 0 ;;

The two patterns guard against ../foo (leading) and foo/../bar (interior), but not against terminal .. segments — e.g. `docs/..`.

Trace for `docs/..` in a code span written to a .md file:

1. emit_tokens extracts docs/..
2. normalize_candidate "docs/..":
   • passes [[ "$t" == */* ]] (has /)
   • does not match ../* (doesn't start with ../)
   • does not match */../* (no / after the ..)
   • last="${t##*/}" → ..; .. matches *.* (the dot-dot contains a dot), so the bare-directory guard passes
   • returns docs/..
3. first_segment_is_local "docs/..": seg="docs" → checks [[ -d "$REPO_ROOT/docs" ]] → true
4. [[ -e "$REPO_ROOT/docs/.." ]] → resolves to $REPO_ROOT → exists → silently passes (false negative)

Security impact: Zero in the current implementation. The only operation performed on the resolved path is a read-only existence check ([[ -e … ]]), so path traversal here cannot cause reads, writes, or command execution outside the repo. The practical consequence is a false-negative: a markdown citation like `docs/..` is not flagged as suspicious. Since the hook is advisory-only, this is a correctness gap, not a security defect.

Suggested fix (if desired): extend the filter to also reject a path that resolves its final segment to ..:

  # Also catches terminal `..` (e.g. `docs/..`)
  */..) return 0 ;;
  ../* | */../*) return 0 ;;

---

Positive security observations

The remainder of the threat surface I checked was clean:

Area	Assessment
Command injection via SCAN_CONTENT	printf '%s' "$SCAN_CONTENT" | grep | sed — content is stdin-piped, never interpolated into shell command strings; no injection path
JSON output construction	All user-controlled values go through jq --arg (both hooks) or hook::json_escape (jq-less skip-notice path); no hand-built JSON with user data
Skill ref parsing	grep -oE ' `/[a-z][a-z0-9-]*:[a-z][a-z0-9-]*`'strictly gates plugin and skill names to[a-z0-9-]` before any filesystem lookup; no traversal possible via a crafted ref
Manifest name → PLUGIN_DIR map	Key is from jq -r .name; used only in lookup where $plugin is regex-constrained to [a-z0-9-], so a malicious manifest name (e.g. ../../etc) can never be retrieved
sed in skill_resolves	Pattern is hard-coded; $sd is the file path argument (quoted), not interpolated into the pattern; extracted fname is constrained to [A-Za-z0-9_-] by the pattern
Telemetry sink execution	"$sink" is operator-configured via HOOK_TELEMETRY_SINK (pre-existing in hook-utils.sh); not derived from hook input; content piped to it is jq-built with controlled fields
CLAUDE_PROJECT_DIR boundary	hook::read_file_path rejects files outside the project root (symlinks resolved before comparison); attacker-written files can't move the hook's scope
shopt nullglob guard	Correctly scoped around the manifests=() glob; ((${#manifests[@]} > 0)) gate properly handles the empty-repo case

---

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d45eecdddd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude finished @kyle-sexton's task in 3m 41s —— View job

---

Security Review — PR #1284

• Gather context
• Read PR diff and changed files
• Analyze asserted-path-verify.sh
• Analyze skill-reference-verify.sh
• Analyze manifest changes
• Post findings

---

No CRITICAL or IMPORTANT security findings. Both hooks follow the same secure-by-design pattern as their siblings: all user-controlled hook input flows through jq for extraction, filesystem operations are read-only existence tests inside [[ ]] bash keywords (immune to word-splitting and globbing), and all JSON output is constructed with jq --arg/--argjson. The full threat surface I checked:

Surface	Verdict
SCAN_CONTENT → grep/sed	Piped as stdin, never interpolated into command strings. No injection path.
SCAN_CONTENT → normalize_candidate	All use is inside [[ ]] comparisons or bash string-ops (%%, ##, #, %). No exec surface.
$REPO_ROOT/$target in [[ -e ]]	[[ -e ]] is a shell keyword — no glob expansion, no word splitting. Read-only existence test.
$REPO_ROOT/$first_seg in [[ -d ]]	Same: shell keyword, read-only.
jq -r extractions	All passed to --arg/--argjson on output; or used only in [[ ]] tests.
plugin/skill from emit_refs	Regex [a-z][a-z0-9-]* strictly gates what can be extracted before any filesystem lookup. No traversal characters possible.
Manifest name → PLUGIN_DIR array key	Crafted name values with traversal chars (e.g. ../../etc) can never be looked up because the ref-extraction regex only produces [a-z][a-z0-9-]* lookup keys. Dead entry, never reached.
sed in skill_resolves	Pattern is hard-coded. $sd is a quoted file-path argument from a filesystem glob — not interpolated into the pattern. Extracted fname constrained to [A-Za-z0-9_-]+ by the capture group.
"$pdir"/skills/*/SKILL.md glob	$pdir is a filesystem path from manifest discovery (not from hook JSON). The glob runs after nullglob is disabled; `[[ -f "$sd" ]]
HOOK_TELEMETRY_SINK exec	Operator-configured (settings.json env var), not derived from any hook input. Not a user-controlled injection surface.
Output context (hook::ctx_append chain)	Flushed via jq -n --arg ctx in hook::emit_additional_context. Arbitrary string content is safely escaped by jq.
Telemetry path values	MISSING/UNRESOLVED arrays piped through jq -R . and jq -s .. No hand-built JSON with user-controlled paths.
CLAUDE_PROJECT_DIR boundary	hook::read_file_path (in hook-utils.sh) rejects files outside the project root with symlink-resolved comparison. Attacker-written files cannot relocate the hook's scope.
source "$(dirname "${BASH_SOURCE[0]}")/hook-utils.sh"	BASH_SOURCE[0] is the script's own path (set by the shell, not from hook input). Not user-controllable.

---

SUGGESTION — terminal .. segment passes normalize_candidate (confirmed from prior review)

Confidence: HIGH | Security impact: NONE

asserted-path-verify.sh:116

The existing filter rejects ../* and */../* but not a terminal .. segment (e.g. `docs/..`). Such a token passes normalize_candidate, passes the first-segment gate (docs/ exists), and then [[ -e "$REPO_ROOT/docs/.." ]] resolves to the repo root — which exists — so the hook stays silent. This is a false negative (missed advisory finding), not a security defect: the only operation performed on the resolved path is a read-only [[ -e ]] test. No read, write, or command execution outside the repo is possible. The fix is a correctness improvement:

t="${t%%:[0-9]*}"
t="${t%:}"        # strip bare trailing colon (edge case)
# …
*/..) return 0 ;; # terminal .. resolves outside — drop before first-segment gate
../* | */../*) return 0 ;;

---

SUGGESTION — shopt -u nullglob unconditionally clears prior state (confirmed from prior review)

Confidence: HIGH | Security impact: NONE

skill-reference-verify.sh:71–73

shopt -s nullglob
manifests=("$PLUGINS_DIR"/*/.claude-plugin/plugin.json)
shopt -u nullglob

If nullglob was already set in the sourced environment, this unconditionally unsets it for the rest of the process. Hooks currently run in isolated subshells so this has no live impact, but it is fragile. The save-and-restore pattern eliminates the brittleness without any security consequence:

local _nullglob; _nullglob=$(shopt -p nullglob)
shopt -s nullglob
manifests=(…)
eval "$_nullglob"

---

Positive security observations

Both hooks exhibit the patterns this codebase's security review expects for PostToolUse advisory hooks:

• No eval of user-controlled content anywhere in either hook.
• No unquoted variables in exec positions. Every external command invocation (jq, grep, sed, git, cygpath) receives its arguments as properly quoted strings.
• Fail-open on missing prerequisites. hook::require_jq exits 0 rather than blocking when jq is absent; the jq-less skip-notice path uses hook-authored strings only, not user input.
• Narrow extraction regex in emit_refs ([a-z][a-z0-9-]*) prevents any traversal or injection character from reaching a filesystem lookup.
• PII redaction in telemetry is correct: file_rel falls back to basename if it can't be made repo-relative, preventing developer usernames embedded in absolute paths from reaching the telemetry sink.
• Manifest name → PLUGIN_DIR isolation. Because the ref-extraction regex and the array lookup key are both constrained to [a-z][a-z0-9-]*, a crafted manifest with a traversal name value creates a dead PLUGIN_DIR entry that is never reachable via any syntactically-valid skill reference.

[claude]

Claude finished @kyle-sexton's task in 3m 17s —— View job

---

PR Review: feat(guardrails): add asserted-path and skill-reference claim guards

• Gather context and understand request
• Read all changed files at HEAD
• Review asserted-path-verify.sh (latest commit)
• Review skill-reference-verify.sh (latest commit)
• Check whether prior review findings were addressed
• Review test suites
• Synthesize findings

---

Latest commit (e944d32) — what was fixed

The shellcheck commit correctly resolved all three CI failures:

ShellCheck finding	Fix
SC2178/SC2128 — local seg collides with the local -a seg array declared in hook-utils.sh (sourced and resolved by ShellCheck)	Renamed to first_seg throughout first_segment_is_local() with a collision note at the site
SC2249 — normalize_candidate's case had no default branch	Added *) ;; with a clear comment
SC2016 — single-quoted test fixtures containing $/{} and backticks flagged as unexpanding	# shellcheck disable=SC2016 added file-wide in both test files with rationale

All three fixes are correct and the rename produces no behavior change.

---

Outstanding issues — none were addressed in the latest commit

These carry over from the two prior reviews and the Codex pass. Listed by severity:

1. README kill-switch table incomplete — documentation correctness

plugins/guardrails/README.md:97–108

The "Per-hook kill switches" table has 9 rows. plugin.json defines 12 userConfig boolean toggles. Three are absent:

Missing guard	Missing option key
block-convention-violation	block_convention_gate_enabled
asserted-path-verify	asserted_path_verify_enabled
skill-reference-verify	skill_reference_verify_enabled

A user looking up how to disable one of the two new guards would find no option key. block-convention-violation predates this PR and was already missing.

Fix this →

---

2. skill_resolves sed scan not frontmatter-bounded — oracle correctness

plugins/guardrails/hooks/skill-reference-verify.sh:96

fname=$(sed -n '1,40{s/^name:[[:space:]]*…/\1/p;}' "$sd" 2>/dev/null | head -1)

The 1,40 range is not bounded by the closing --- frontmatter delimiter. If a SKILL.md's frontmatter name is absent but its body contains a bare name: foo within the first 40 lines, the hook treats the body value as the skill name and returns true — a false negative. Harden by stopping at the closing ---:

fname=$(awk 'NR==1{next} /^---/{exit} /^name:[[:space:]]*/{ … ; exit }' "$sd" 2>/dev/null)

or restrict the sed address to between frontmatter delimiters.

---

3. normalize_candidate trailing-colon false positive — filter completeness

plugins/guardrails/hooks/asserted-path-verify.sh:92

t="${t%%:[0-9]*}"

:[0-9]* requires at least one digit after the colon. A token ending in a bare colon (`docs/file.md:`) is not stripped, so docs/file.md: reaches the filesystem check, fails to exist, and fires a spurious finding. The fix is one extra line:

t="${t%%:[0-9]*}"
t="${t%:}"          # bare trailing colon

Very unlikely in practice; no test covers it.

---

4. shopt -u nullglob does not restore prior state — brittleness

plugins/guardrails/hooks/skill-reference-verify.sh:71–73

shopt -s nullglob
manifests=(…)
shopt -u nullglob

If the calling environment (or hook-utils.sh) had nullglob set, this unconditionally clears it for the rest of the hook process. Hooks run in isolated subshells today so this is not a live bug, but it's fragile against future changes. Defensive pattern:

local _nullglob; _nullglob=$(shopt -p nullglob)
shopt -s nullglob
manifests=(…)
eval "$_nullglob"

---

5. skill_resolves directory-name branch does not require a SKILL.md — false negative

plugins/guardrails/hooks/skill-reference-verify.sh:92

[[ -d "$pdir/skills/$skill" ]] && return 0

A directory that exists but contains no SKILL.md (not a valid skill in Claude Code's discovery) causes this branch to return true, silencing the guard. The for sd in "$pdir"/skills/*/SKILL.md loop below correctly scans only SKILL.md-bearing directories; the fast-path above does not. Suggested fix:

[[ -f "$pdir/skills/$skill/SKILL.md" ]] && return 0

---

6. Link targets with optional titles are not scanned — false negative (minor)

plugins/guardrails/hooks/asserted-path-verify.sh:78

printf '%s' "$SCAN_CONTENT" | grep -oE '\]\([^)[:space:]]+\)'

[^)[:space:]] rejects any space, so ](docs/missing.md "title") (a CommonMark titled link) is not extracted — the destination is never checked. The destination must be parsed separately from the optional title:

grep -oE '\]\([^)[:space:]"]+([[:space:]][^)]+)?\)'

or capture just the unspaced prefix before the first space. Titled links are uncommon in this repo's citation style but are valid Markdown, and a cited file in a titled link is still an assertion.

---

Summary

The shellcheck commit was correct and necessary — those three findings were real. The six items above are all carry-overs or addenda from the prior reviews; none were introduced by the latest commit. The table gap (#1) and the skill_resolves SKILL.md check (#5) are the cleanest to fix and would be the most visible to consumers. Items #2–#4 are hardening; #6 is low-impact given this repo's citation style.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e944d326e7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude finished @kyle-sexton's task in 4m 6s —— View job

---

PR Review — latest commit 55585e1

• Gather context — reviewing commit 55585e1 (fix: correct five claim-guard defects found in review)
• Read changed files at HEAD
• Assess which prior findings were addressed
• Check new fixes for correctness
• Synthesize findings

---

What 55585e1 fixed

Five defects from the prior reviews were all correctly addressed, with regression tests for each:

Defect	Fix
Titled link destinations ([x](dest)) silently skipped	emit_tokens now extracts the whole (…) run, then strips the optional title with s/[[:space:]].*$//
Link destinations resolved only against repo root, not the document	Dual-base loop: tries DOC_DIR first, then REPO_ROOT; code spans stay repo-root-only
Percent-encoded destinations (docs/my%20file.md) not decoded	maybe_percent_decode() decodes valid %HH sequences, then re-applies the traversal guard post-decode
skill_resolves returned true for a bare skills/<name>/ directory with no SKILL.md	skill-reference-verify.sh:96 changed from [[ -d … ]] to [[ -f …/SKILL.md ]]
YAML inline comment on frontmatter name: (name: foo # note) broke extraction	Comment-strip pipe added before the value-capture sed pass

The test additions are correctly scoped: each covers one repro case, and the link-resolution tests include the tricky counter-cases (code span stays repo-root-relative from a nested document; link resolving against neither base still fires).

---

Outstanding — not addressed in 55585e1

1. README kill-switch table still missing three rows — documentation correctness

plugins/guardrails/README.md:97–108

The kill-switch table lists 9 rows; plugin.json defines 12 toggles. Three entries are absent — two of them new to this PR:

Missing guard	Missing option key
block-convention-violation	block_convention_gate_enabled
asserted-path-verify	asserted_path_verify_enabled
skill-reference-verify	skill_reference_verify_enabled

A consumer trying to disable one of the new guards would find no option key to use. This is the cleanest fix before merge.

Fix this →

---

2. skill_resolves sed scan crosses the frontmatter boundary — oracle correctness

plugins/guardrails/hooks/skill-reference-verify.sh:102

fname=$(sed -n '1,40p' "$sd" 2>/dev/null |
  sed -E 's/[[:space:]]+#.*$//' |
  sed -nE 's/^name:…/p' | head -1)

1,40p passes lines 1–40 without stopping at the closing --- of the YAML frontmatter. A SKILL.md with no name: in its frontmatter but a bare name: foo in its body within the first 40 lines would be treated as having name: foo, causing skill_resolves to return true for a non-existent command — a false negative. Harden by stopping at the second ---:

fname=$(awk 'NR==1{next} /^---/{exit} 1' "$sd" 2>/dev/null |
  sed -E 's/[[:space:]]+#.*$//' |
  sed -nE 's/^name:[[:space:]]*['\''"]?([A-Za-z0-9_-]+)['\''"]?[[:space:]]*$/\1/p' |
  head -1)

---

3. Trailing-colon false positive in normalize_candidate — filter completeness

plugins/guardrails/hooks/asserted-path-verify.sh:118

t="${t%%:[0-9]*}"

:[0-9]* requires at least one digit. A token ending in a bare colon — `docs/file.md:` — is not stripped, so docs/file.md: reaches [[ -e ]], fails to exist, and fires a spurious finding. One extra line closes it:

t="${t%%:[0-9]*}"
t="${t%:}"   # bare trailing colon

No test covers this case. Very unlikely in practice but easy to close.

---

4. shopt -u nullglob unconditionally clears prior state — brittleness

plugins/guardrails/hooks/skill-reference-verify.sh:71–73

shopt -s nullglob
manifests=(…)
shopt -u nullglob

If nullglob was set before this block (e.g., by hook-utils.sh), it is unconditionally cleared for the rest of the hook process. Not a live bug today since hooks run in isolated subshells, but fragile against future additions. Save-and-restore:

local _nullglob; _nullglob=$(shopt -p nullglob)
shopt -s nullglob
manifests=(…)
eval "$_nullglob"

---

5. Terminal .. segment still passes normalize_candidate — false negative

plugins/guardrails/hooks/asserted-path-verify.sh:142

The case guards cover ../* (leading) and */../* (interior) but not a terminal .. — e.g. `docs/..`. Such a token passes normalize_candidate, passes the first-segment gate (docs/ exists), and then [[ -e "$REPO_ROOT/docs/.." ]] resolves to the repo root (which exists), so resolved=1 and the hook stays silent. This is a false negative, not a security defect (the only operation on the resolved path is a read-only [[ -e ]] test). Fix:

*/..) return 0 ;;   # terminal .. resolves outside tree
../* | */../*) return 0 ;;

---

Minor: same path reported twice when cited as both code span and link

With the new CHECKED["$kind|$cand"] key, the same physical path can appear twice in MISSING if it is cited as both a code span and a link destination in the same write — both dedup buckets are distinct. The output would then say "2 asserted path(s)" and list the path twice. Unlikely in practice; easily closed by deduplicating MISSING before the output loop or using a flat-string CHECKED key regardless of kind.

---

Summary

Five defects from prior reviews are correctly closed, each with a regression test. The five items above are carry-overs from those reviews. Item 1 (README kill-switch table) is the most visible to consumers and remains the single clean blocker; the rest are hardening items. The fixes themselves introduce no new correctness or security issues — maybe_percent_decode is correct, the dual-base resolution is correct, and the post-decode traversal re-application covers the encoded-escape case the test suite now exercises.

[claude]

Claude finished @kyle-sexton's task in 4m 52s —— View job

---

Security Review — PR #1284 (HEAD: 55585e1)

• Load tools
• Get PR diff and enumerate changed files
• Read asserted-path-verify.sh at HEAD
• Read skill-reference-verify.sh at HEAD
• Analyze new surfaces in fix commit (55585e1)
• Check manifest/hooks.json changes
• Post findings

---

No CRITICAL or IMPORTANT security findings. The fix commit (55585e1) addressed five correctness defects and introduced two new code surfaces — maybe_percent_decode and the link-kind/base-resolution refactor — both analyzed below. The threat surface I checked:

Area	Assessment
SCAN_CONTENT → grep -oE ' `[^` ]+ `` '	Content is stdin-piped, never interpolated into command strings. No injection path.
SCAN_CONTENT → grep -oE '\]\([^)]*\)' (now [^)]* permits spaces)	Content is still stdin-piped data. The extracted text goes through sed as stdin, then into a while read variable. No exec surface.
normalize_candidate with glob metacharacter gate	Explicitly rejects $, (, ), `
maybe_percent_decode → printf '%b'	Analyzed in detail below.
Post-decode traversal guard	Re-applied on decoded $cand before any filesystem test. %2e%2e/path → decoded ../path → caught by ../*.
[[ -e "$base/$target" ]] / [[ -d "$base/$first_seg" ]]	Shell keywords — no glob expansion, no word splitting, read-only existence tests.
skill_resolves sed pattern	Hard-coded patterns; $sd is a quoted file-path argument from a controlled glob; extracted fname constrained to [A-Za-z0-9_-]+.
emit_refs grep pattern	[a-z][a-z0-9-]* strictly gates extracted plugin/skill tokens before any filesystem lookup.
JSON construction in emit_tel	All user-controlled values go through jq -R . / jq --arg / jq --argjson. No hand-built JSON.
source "$(dirname "${BASH_SOURCE[0]}")/hook-utils.sh"	BASH_SOURCE[0] is shell-set (not from hook input). Standard pattern.
HOOK_TELEMETRY_SINK exec	Operator-configured via env var (pre-existing in hook-utils.sh). Not derived from hook input.
CLAUDE_PROJECT_DIR boundary	hook::read_file_path (hook-utils.sh) rejects files outside the project root. Not altered by this PR.

---

maybe_percent_decode — detailed analysis

asserted-path-verify.sh:94–105

maybe_percent_decode() {
  local s="$1"
  [[ "$s" == *%* ]] || { printf '%s' "$s"; return 0; }
  [[ "$s" =~ ^([^%]|%[0-9A-Fa-f]{2})*$ ]] || { printf '%s' "$s"; return 0; }
  printf '%b' "${s//%/\\x}"
}

Three potential concerns examined:

1. printf '%b' with pre-existing backslash sequences. If the original string contains literal \n, \t, etc. before any %, those pass the regex ([^%] matches \) and then printf '%b' interprets them. Result: the decoded value may contain newlines or other control characters. This is passed to [[ -e "$base/$target" ]] — a shell keyword — so no command execution is possible. The practical outcome is a false negative (the path doesn't exist as named; the guard stays silent). No security impact.

2. %2e%2e traversal via percent-encoding. docs/%2e%2e/etc/passwd.md survives normalize_candidate (it has literal / separators), is decoded by maybe_percent_decode to docs/../etc/passwd.md, and then the post-decode traversal guard fires on */../*: continue. The guard is correctly re-applied after decoding.

3. Double-encoding bypass: %252e%252e/path. %25 decodes to %, so one round produces %2e%2e/path — not ../path. The post-decode guard doesn't fire (no literal ../). However, first_seg="%2e%2e" and no directory named %2e%2e exists in any real repo, so the first-segment gate blocks adjudication entirely. The only effect is a false negative (no advisory fires); the filesystem check is never reached. No security impact.

4. Null byte via %00. %00 passes the hex regex. printf '%b' '\x00...' in bash truncates at the null byte, silently passing a truncated path to the existence check. This is at most a false negative. No security impact.

In all cases maybe_percent_decode output reaches only [[ ]] keyword tests — immune to word splitting, globbing, and command expansion. The printf '%b' decode cannot cause command execution regardless of input.

---

SUGGESTION — normalize_candidate terminal .. segment (confirmed, low confidence it matters)

asserted-path-verify.sh:142

../* | */../*) return 0 ;;

A terminal .. (docs/..) passes normalize_candidate, passes the first-segment gate (docs/ exists), and [[ -e "$REPO_ROOT/docs/.." ]] resolves to the repo root — which exists — so the guard stays silent. Security impact: zero — the only operation is a read-only [[ -e ]] test. This is a false-negative correctness issue, already confirmed by both prior reviews.

Fix if desired:

*/..)  return 0 ;;   # terminal .. resolves to parent — treat as traversal
../* | */../*) return 0 ;;

---

SUGGESTION — shopt -u nullglob does not restore prior state (no live impact)

skill-reference-verify.sh:71–73

Unchanged from prior reviews. Hooks run in isolated subshells so this has no live impact. The defensive save/restore pattern (local _nullglob; _nullglob=$(shopt -p nullglob); …; eval "$_nullglob") removes the brittleness.

---

Positive observations

• Five correctness fixes from prior reviews are correctly addressed. No new security surface was introduced in addressing them.
• The link-kind/base-resolution refactor (code| vs link| prefix) does not create a new injection surface: | is itself a rejected character in normalize_candidate's glob-metacharacter gate, so a content-injected | in a path candidate is dropped before the %%|* / #*| split in the while loop.
• The YAML-comment stripping in skill_resolves uses a hard-coded sed pattern; the comment content is never used as a command or pattern.
• All five findings from the latest commit were correctness defects, not security defects. The fixes don't alter the security posture.

---

Bottom line: Two independent prior security reviews and this one reach the same conclusion: no CRITICAL or IMPORTANT security issues. The two SUGGESTION-level items (terminal .., nullglob restore) carry no live security impact and were already documented. Safe to merge from a security standpoint pending the open correctness and documentation items from the code reviews.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 55585e1e1e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude finished @kyle-sexton's task in 6m 26s —— View job

---

PR Review — latest commit 343c227

• Gather context and understand request
• Read changed files at HEAD (343c227)
• Assess which prior findings were addressed
• Check new fixes for correctness
• Check remaining open items
• Synthesize findings

---

What 343c227 fixed

Three Codex findings from the prior review round were correctly addressed, each with regression tests:

Finding	Fix
Parent-relative links never checked (Codex)	lexical_normalize() + DOC_REL (via git --show-prefix) + resolve_within_repo() canonicalize ../ link destinations against the document's directory; paths that resolve outside the repo are silently skipped; code spans retain the repo-root-relative convention
Directory name treated as alias for a renamed skill (Codex)	skill_resolves() now returns true on a direct directory match ONLY when the frontmatter declares no name, or the declared name equals the directory. If a SKILL.md declares name: renamed in a skills/legacy-dir/ directory, /plugin:legacy-dir correctly fires
Argument-bearing skill invocations unchecked (Codex)	emit_refs() now extracts the leading /plugin:skill token from any backtick span rather than requiring an exact match; `/alpha:audit --dry-run <target>` → /alpha:audit

The lexical_normalize implementation is correct: it iterates path segments under IFS=/, collapses . and empty segments, returns 1 when a .. would ascend past the input's own root (where ((${#out[@]})) is zero), and produces an empty string when the result is the repo root — caught by the [[ -n "$cand" ]] || continue guard in the caller.

The Windows/MSYS path-discrepancy rationale for using git --show-prefix rather than string surgery is well-explained and the fallback (empty DOC_REL outside a work tree) is correct.

The skill_resolves logic handles all four scenarios correctly: no-frontmatter-name (directory-name wins), frontmatter-name-matches-directory (both paths work), frontmatter-name-differs (only frontmatter name resolves, directory name fires), and name-only-in-for-loop (a skill whose directory name doesn't match but frontmatter does).

---

Outstanding items — none were addressed in 343c227

1. README kill-switch table still missing three rows — documentation correctness

plugins/guardrails/README.md:97–108

The table lists 9 rows; plugin.json defines 12 toggles. Three entries are absent — two new to this PR:

Missing guard	Missing option key
block-convention-violation	block_convention_gate_enabled
asserted-path-verify	asserted_path_verify_enabled
skill-reference-verify	skill_reference_verify_enabled

A consumer wanting to disable one of the new guards would find no option key. This is the clearest remaining blocker before merge.

Fix this →

---

2. Terminal .. false negative — gap widened by the ../ refactor

Code span path: asserted-path-verify.sh:246–249

Link path: asserted-path-verify.sh:239

The prior code blanket-rejected ../* and */../* in normalize_candidate. Now links go through resolve_within_repo only when the condition [[ "$cand" == *../* || "$cand" == ../* ]] is true — which requires a / after the ... A terminal .. segment (docs/..) has no following slash, so it fails the condition on both legs.

Trace for `docs/..` as a code span:

1. normalize_candidate "docs/.." — passes all filters: no metacharacters, .. matches *.* so the bare-extensionless guard doesn't fire, returns docs/..
2. Code-span branch: docs/.. matches neither ../* nor */../* → continues
3. first_seg = "docs" → gated (if docs/ exists)
4. [[ -e "$REPO_ROOT/docs/.." ]] → resolves to the repo's parent, which exists → resolved=1 → silent (false negative)

Same trace holds for [text](docs/..) as a link: the condition at line 239 evaluates false, resolve_within_repo is never called, and the fallback existence test resolves to the parent directory.

The refactor that enabled proper ../ link canonicalization did not extend the link condition to cover the terminal case. The fix is two-line:

# Link branch condition at line 239 — also catch terminal `..`
if [[ "$cand" == *../* || "$cand" == ../* || "$cand" == */.. || "$cand" == ".." ]]; then

# Code-span branch at line 247 — terminal `..` should also escape
case "$cand" in
../* | */../* | */..) continue ;;
*) ;;
esac

Noted in all three prior review passes. Unlikely input in practice; the only consequence is a false negative (advisory stays silent). No security impact — the only operation on the resolved path is a read-only [[ -e ]] test.

---

3. skill_frontmatter_name sed scan not frontmatter-bounded — oracle correctness

skill-reference-verify.sh:91–96

skill_frontmatter_name() {
  sed -n '1,40p' "$1" 2>/dev/null |
    sed -E 's/[[:space:]]+#.*$//' |
    sed -nE 's/^name:…/p' |
    head -1
}

1,40p scans the first 40 lines without stopping at the closing --- YAML delimiter. A SKILL.md with no name: in its frontmatter but a bare name: foo within the first 40 body lines would cause skill_resolves to return true for a command that doesn't exist — a false negative. Stop at the second ---:

skill_frontmatter_name() {
  awk 'NR==1{next} /^---/{exit} 1' "$1" 2>/dev/null |
    sed -E 's/[[:space:]]+#.*$//' |
    sed -nE 's/^name:[[:space:]]*"?'"'"'?([A-Za-z0-9_-]+)"?'"'"'?[[:space:]]*$/\1/p' |
    head -1
}

Low likelihood in practice; the current SKILL.md convention puts name: in line 2.

---

4. Trailing-colon false positive in normalize_candidate — filter completeness

asserted-path-verify.sh:118

t="${t%%:[0-9]*}"

:[0-9]* requires at least one digit. A token ending in a bare colon (`docs/file.md:`) passes normalize_candidate, reaches [[ -e ]], fails, and fires a spurious finding. One additional line closes it with no test needed to demonstrate the behavior:

t="${t%%:[0-9]*}"
t="${t%:}"   # bare trailing colon

Very unlikely in practice.

---

5. shopt -u nullglob unconditionally clears prior state — brittleness

skill-reference-verify.sh:71–73

Hooks run in isolated subshells today so this has no live impact, but a save-and-restore is the defensive pattern:

local _nullglob; _nullglob=$(shopt -p nullglob)
shopt -s nullglob
manifests=(…)
eval "$_nullglob"

---

Positive observations on 343c227

• lexical_normalize is correctly contained. The function processes a relative prefix only, never an absolute path — resolve_within_repo is responsible for prepending DOC_REL before calling it. The ((${#out[@]})) || return 1 guard correctly detects any path that would ascend past the relative root.
• git --show-prefix over string surgery is the right call. The Windows/MSYS path-universe mismatch is a real portability constraint and the comment documents it precisely.
• link_rooted kind correctly collapses the dual-base logic. Once a link destination has been canonicalized to a repo-relative path, changing kind to link_rooted routes the existence test through the REPO_ROOT-only base, preventing the doc-dir base from being tried again for a path that no longer needs it.
• New tests are scenario-first. Each new case names a concrete MUST-FIRE or MUST-STAY-QUIET scenario and the fixture clearly explains why the outcome is expected. The ghost bare-directory, legacy-dir alias, and argument-bearing invocation cases cover the three exact scenarios the Codex findings identified.
• CLAUDE_PLUGIN_OPTION_STDIN_READ_TIMEOUT=30 addition is correct. The timeout is a stdin-read bound for the advisory hook, not a logic timeout, and raising it prevents false FAIL on slow CI boxes without affecting what the test exercises.

---

Summary

343c227 correctly addresses the three Codex findings from the prior round (parent-relative links, directory aliases, argument-bearing invocations) with clear implementation and test coverage. Five carry-over items from the earlier review passes remain open. The README kill-switch table gap is the one a consumer would hit immediately; the rest are hardening items. No new correctness or security issues were introduced by this commit.

[claude]

Claude finished @kyle-sexton's task in 4m 15s —— View job

---

Security Review — PR #1284 (HEAD: 343c227)

• Gather context
• Read PR diff (latest commit vs prior reviewed 55585e1)
• Read changed files at HEAD
• Analyze new surfaces introduced in 343c227
• Post findings

---

No CRITICAL or IMPORTANT security findings. The new commit (343c227) introduces three new code surfaces — lexical_normalize, resolve_within_repo, and the relaxed emit_refs extraction — all analyzed below. The prior two SUGGESTION items persist unchanged.

New surfaces in 343c227

The full threat surface I checked:

Surface	Assessment
lexical_normalize — for seg in $p (unquoted)	See detailed analysis below
git -C "$DOC_DIR" rev-parse --show-prefix	$DOC_DIR is derived from hook::read_file_path-verified $FILE; argument is quoted; fixed command. No injection path.
resolve_within_repo "$cand"	Return value 1 (escape detected) causes continue; the lexical_normalize failure is the containment test. Correct.
emit_refs — grep -oE '\[^` + "`]+" + ' then sed constraint	The broadened grep extracts arbitrary span content, but the second sed hard-codes the pattern to [a-z][a-z0-9-]*:[a-z][a-z0-9-]*. Plugin and skill names reaching filesystem lookups remain constrained to the same character class as before. No new traversal surface.
skill_resolves refactor — skill_frontmatter_name "$direct"	$direct is $pdir/skills/$skill/SKILL.md where both $pdir (from manifest glob) and $skill ([a-z][a-z0-9-]* by emit_refs constraint) are safe. The function's sed patterns are hard-coded; the extracted name is constrained to [A-Za-z0-9_-]+. No injection path.
%2e%2e%2f traversal via decoded link	maybe_percent_decode converts to ../, which triggers resolve_within_repo → lexical_normalize; the function returns 1 when out would underflow past root → continue. Correctly contained.
Double-encoded traversal %252e%252e	One-round decode produces literal %2e%2e (not ..); first_seg="%2e%2e" — no real directory → first-segment gate blocks. False negative at most; no security impact.
unset 'out[-1]' in lexical_normalize	Single-quoted, no expansion. Correct bash idiom for removing the last array element. Safe.
printf '%s%s' "$lead" "${out[*]}"	local IFS=/ at function scope makes ${out[*]} join with /. Quoted. No injection.

---

SUGGESTION — lexical_normalize: unquoted $p in for seg in $p permits glob expansion of path segments

Confidence: HIGH | Security impact: NONE

asserted-path-verify.sh:184

lexical_normalize() {
  local p="$1" seg out=() lead=""
  [[ "$p" == /* ]] && lead=/
  local IFS=/
  for seg in $p; do   # ← unquoted: word-splits on IFS=/ AND glob-expands each segment

for seg in $p with unquoted $p applies both IFS-splitting on / and pathname expansion on the resulting words. After maybe_percent_decode, a percent-encoded glob character (%2a → *) could survive normalize_candidate's metacharacter filter (which scans the pre-decode form) and reach lexical_normalize as a literal * segment.

For example: a link destination docs/%2a.md passes normalize_candidate (no literal * before decoding), is decoded to docs/*.md, and if the path contains .. triggers resolve_within_repo → lexical_normalize "DOC_REL/docs/*.md". Inside lexical_normalize, *.md expands against the current working directory. Matched filenames become elements of out[]; unmatched (with nullglob off) stays literal.

Security impact: zero. The out array feeds only printf '%s%s' "$lead" "${out[*]}", whose result feeds only [[ -e "$base/$target" ]] — a shell keyword immune to word-splitting, globbing, and command expansion. No command execution path exists regardless of what glob expansion produces.

The practical consequence is a correctness oddity: a percent-encoded * in a link destination could cause the existence check to run against an unexpected resolved path. Quoting "$p" in the for loop, combined with explicit splitting using read -ra or a parameter expansion, would eliminate the ambiguity:

local IFS=/ _segs
read -ra _segs <<<"$p"
for seg in "${_segs[@]}"; do …

---

Persistent items from prior reviews (not addressed in 343c227, not new)

Terminal .. segment false negative (asserted-path-verify.sh:239): [[ "$cand" == *../* || "$cand" == ../* ]] does not match a terminal .. (e.g. docs/..). Such a candidate passes through to [[ -e "$REPO_ROOT/docs/.." ]] → resolves to $REPO_ROOT → exists → silent. False negative, no security impact (read-only [[ -e ]] test).

shopt -u nullglob unconditional clear (skill-reference-verify.sh:73): clears prior state rather than restoring it. Not a live bug (hooks run in isolated subshells); save-and-restore would be more defensive. No security impact.

---

Positive security observations

The 343c227 changes do not weaken the security posture established in 55585e1:

• Post-decode traversal guard is correctly applied. %2e%2e%2f decodes to ../, enters resolve_within_repo, and lexical_normalize returns 1 when the path would escape the repo root — causing continue. The containment test operates on the decoded form, so encoded-escape bypass attempts are correctly caught.
• Absolute-form re-check is applied post-decode. The /* | [A-Za-z]:[/\\]*) guard runs after maybe_percent_decode, so a %2f-prefixed absolute path is also caught.
• emit_refs relaxation is bounded by the second sed. The grep now extracts any backtick content, but only the leading /[a-z][a-z0-9-]*:[a-z][a-z0-9-]* token survives the sed filter. No traversal character can reach a filesystem lookup via this path.
• skill_resolves is stricter, not weaker. Directory names are no longer treated as aliases for declared name: values, which closes a false-negative channel without introducing any new exec surface.
• No eval of user-controlled content anywhere in either hook. All user data flows through jq extraction, [[ ]] tests, or the character-constrained sed patterns. This is unchanged from prior commits.

Bottom line: Three prior security reviews and this one reach the same conclusion — no CRITICAL or IMPORTANT security issues. Safe to merge from a security standpoint pending the open documentation and correctness items noted in the concurrent code reviews.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 343c227ce0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Check bare sibling link destinations

When a nested document writes a sibling link such as [API](api.md) or [API](./api.md), stripping ./ leaves no slash and this check discards the destination before the document-relative base is considered. This repository uses bare-filename relative links extensively, so a newly misspelled or deleted sibling target never produces the advertised advisory; allow extension-bearing link destinations without a slash and resolve them against DOC_DIR.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Scan reference-style link definitions

When Markdown uses a reference link such as [guide][docs] followed by [docs]: docs/missing.md, this extractor sees neither a parenthesized destination nor a path code span, so the locally owned missing target is never checked. Reference-style links are valid Markdown link targets and should be extracted from their definition lines alongside inline links.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Keep directory names as skill command segments

When a marketplace contains a malformed skill whose frontmatter name differs from its directory, this logic rejects the real directory-named command and accepts the frontmatter value instead. Fresh evidence in the final tree contradicts the earlier alias suggestion: plugins/skill-quality/scripts/check-skill.sh:194-215 states that Claude namespaces a skill by its directory and rejects mismatched frontmatter. Thus /alpha:legacy-dir receives a false advisory while nonexistent /alpha:renamed stays silent; resolve commands by the directory and treat the mismatch as a separate skill-validity defect.

Useful? React with 👍 / 👎.

[kyle-sexton]

Superseded by #1319. Closing rather than updating, for two reasons.

This branch could not be force-pushed. #853 merged and bumped guardrails to 0.14.3 — exactly the collision this PR's body predicted — so the branch had to be rebased, which rewrote every SHA. Force-pushing was blocked by policy in the authoring environment, so the rebased work went to a fresh branch instead. #1319 carries all six commits from here plus the narrowing below.

The scope narrowed from two guards to one, on measurement. A full-corpus sweep of all 975 tracked markdown files, each fed to the hook as a real payload:

guard	files firing	findings	true positives
asserted-path-verify	231 / 975 = 23.7%	389	0
skill-reference-verify	33 / 975 = 3.4%	63	4

asserted-path-verify is withdrawn. Its oracle never misfired — every one of the 389 findings was a scoping problem, 72% of them consumer-project config paths (.claude/** and similar) that a doc describes for a consuming repo and that correctly do not exist in a marketplace. Its first-segment gate passed only because this repo happens to carry same-named top-level directories. Fixing the three dominant causes still left ~4% firing at zero true positives. The measurement is preserved on #1314 for rescoping.

skill-reference-verify ships, with CHANGELOGs excluded as append-only historical records — 89% of its noise was rename entries the hook's own advisory calls correct as written. That moves it to 0.51% firing at 57% precision, keeping all four genuine findings.

The review history here is worth keeping. Thirteen findings were raised on this PR and every single one was real — a shellcheck failure from local seg colliding with hook-utils.sh's local -a seg (the rcfile sets external-sources=true, so ShellCheck resolves the sourced library), titled and bracketed link destinations being skipped, percent-encoded destinations tested literally, document-relative resolution, a bare skills/ directory counting as a skill, a YAML inline comment defeating name: extraction, a renamed skill's directory silently acting as an alias, argument-bearing invocations going unscanned, and parent-relative links rejected wholesale. Zero false alarms across all thirteen.

Continue at #1319.