fix(instruction-placement): walk nested AGENTS.md paths and scan frontmatter hooks in the shim rule

[kyle-sexton]

Closes #5794
Closes #5800

Summary

The migrate skill's shim-droppable rule (conditions A to F in
plugins/instruction-placement/skills/migrate/reference/shim-droppable.md) missed two blockers,
both found by Codex on #5765:

• fix(instruction-placement): migrate shim rule checks blockers only beside a nested AGENTS.md, not on every ancestor #5800, condition A: a CLAUDE.md, .claude/CLAUDE.md or CLAUDE.local.md in a directory
  between the repository root and a nested AGENTS.md (for example svc/CLAUDE.local.md above
  svc/deep/AGENTS.md) was not checked, so the rule could recommend dropping a shim it should keep.
• fix(instruction-placement): migrate shim rule F ignores InstructionsLoaded hooks in skill and agent frontmatter #5794, condition F: InstructionsLoaded hooks declared in skill, command or agent
  hooks: frontmatter were not inventoried.

Current docs confirm frontmatter hooks can register InstructionsLoaded, so #5794 is a real fix,
not a no-change close. hooks.md, "Hooks in skills and agents": "hooks can be defined directly in
skills and subagents using frontmatter, in the same configuration format as settings-based hooks".
sub-agents.md, "Hooks in subagent frontmatter": "All hook events are supported." Skill hooks are
registered on invocation and run "for the rest of the session". Subagent hooks run while the
subagent runs, or for the whole session under --agent.

Fix

• shim-droppable.md row A now covers every directory from the root to each nested AGENTS.md.
  A new "The nested path walk for condition A" section runs ip_entry_points_on_path
  (scripts/lib/discover.sh:279-288) for each nested AGENTS.md, adds the readability check that
  function lacks, and includes the svc/CLAUDE.local.md worked example. A non-shim FOUND row,
  or any UNREADABLE row, fails A.
• Row F now lists frontmatter hooks. A new "The frontmatter scan for condition F" section has a
  fm_scan snippet that reads only the frontmatter of skills/*/SKILL.md, commands/*.md and
  agents/**/*.md across the repository (root and nested .claude/), <config> (which honors
  CLAUDE_CONFIG_DIR), <plugins>, the managed settings directory and per-session plugin
  directories. An unreadable scope prints UNREADABLE. That row, a HIT, and any source not on
  disk here (claude.ai-synced skills, --agents JSON, managed-deployed subagents, --plugin-url)
  fail F until the operator answers for it.
• The "Nested AGENTS.md, per mode" verdict names the full path.
• sources.md gains two four-part records: "Frontmatter hooks and InstructionsLoaded" (hooks,
  sub-agents and skills pages, fetched 2026-10-02) and "Blockers between the root and a nested
  AGENTS.md" (memory page, fetched 2026-10-02). The memory page does not say whether an
  intermediate file stops a nested load, so the rule treats it as a blocker (fail-closed).
• No script implements conditions A to F (cutover-check.sh and remove-shims.sh grade other
  gates), so the rule text changes. The walk the rule uses as its model gets a regression test.
• instruction-placement 0.18.2 → 0.18.3, with a CHANGELOG entry.

Verification

• discover.test.sh: 3 new cases. ip_entry_points_on_path lists every entry point from
  svc/deep to the root, nearest first; it reports the intermediate svc/CLAUDE.local.md; it
  leaves out a sibling directory's file. 57 cases, 0 failures.
• scripts/affected-tests.sh --run: discover, cutover-check (73) and remove-shims (87) all pass.
• Both doc snippets were run on a fixture. The walk printed FOUND .../svc/CLAUDE.local.md.
  fm_scan hit a skill and a CRLF agent that declare InstructionsLoaded, ignored a body-only
  mention, and printed UNREADABLE for a chmod 000 root. Over this machine's real <config>
  and plugins roots it found no hits.
• markdownlint-cli2 on the changed markdown: 0 issues. check-purged-em-dashes.sh: clean.
  check-changed-skills.sh origin/main: migrate PASS (SKILL.md unchanged at 472 lines).
  validate-plugin-contracts.mjs: passes. check-changelog-parity.sh --check-bump origin/main
  and --check-order: pass.

Related

• feat(instruction-placement): decide CLAUDE.md shim removal against the built-in AGENTS.md loader #5765: built the A-F rule. Both findings come from its review, deferred at the round cap.
• instruction-placement:migrate should account for the built-in AGENTS.md loader #5736.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-02T15:45:55.306708Z	e8a3b7b	Manual request

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[claude]

Claude finished @kyle-sexton's task in 59s —— View job

---

Security review: no issues found

• Gather context (PR diff, changed files)
• Check skip gate
• Review changed files for security-relevant surface
• Check instruction-surface deletions against protected classes
• Post findings (or confirm no issues)

Scope reviewed (gh pr diff 5823, head 8279ae8): all 5 changed files —
plugin.json (version bump), CHANGELOG.md, discover.test.sh, and the two reference docs
shim-droppable.md and sources.md.

Findings: none.

This PR is documentation and test-fixture changes to the migrate skill's shim-droppable
decision procedure, plus a version/changelog bump. There is no executable production code path,
no auth/authz logic, no network or untrusted-input handling, and no secret/credential handling.

• discover.test.sh adds test fixtures under mktemp -d and asserts against ip_entry_points_on_path; no injection or untrusted-input surface.
• shim-droppable.md and its fm_scan snippet are reference shell snippets an operator runs by hand over their own repository/config during a migration decision — not shipped automation processing attacker-controlled input. Variable quoting in both snippets is correct ("$root", "$d", "$f", etc.); find/awk output is read with IFS= read -r.

Instruction-surface lens: this diff only tightens conditions A and F (widens what counts as a
blocker, adds new sources to check) — it does not delete, narrow, or soften any existing guardrail,
so the protected-class register (present at docs/conventions/instruction-exception-register/README.md)
does not apply here.

No GitHub Actions workflow files are touched.

[claude]

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

---

Code review complete

• Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
• Review changed files for correctness and maintainability
• Check for inline findings
• Post final summary

Scope reviewed (per the authoritative 5-file list): plugin.json, CHANGELOG.md, discover.test.sh, shim-droppable.md, sources.md in plugins/instruction-placement.

What I checked directly (not just author-claimed):

• Traced ip_entry_points_on_path in discover.sh and confirmed it only does [[ -f "$entry" ]] with no readability/executability check — so shim-droppable.md's claim that the function "skips an unreadable directory silently" is accurate, and the doc's added readability loop is not redundant.
• Walked the new discover.test.sh case by hand against the function's actual loop order (nearest directory first, walking up via dirname to root) — the expected assert_lists ordering (svc/deep/CLAUDE.md, svc/deep/.claude/CLAUDE.md, svc/CLAUDE.local.md, root CLAUDE.md) matches the implementation, and the assert_has/assert_lacks argument order matches the helper signatures in the file (assert_has(label, list, item), assert_lists(label, expected, actual)).
• Checked the fm_scan snippet's find -path '*/skills/*/SKILL.md' ... patterns: since find -path doesn't use FNM_PATHNAME, * legitimately spans /, so nested plugin paths (<plugins>/foo/skills/bar/SKILL.md) are still matched — not a coverage gap.
• Checked the apparent asymmetry where fm_scan is invoked with <managed-settings-dir>/.claude/skills but no managed-settings agents/commands path — this is intentional, not a gap: the doc's closing paragraph explicitly places "subagents ... deployed through managed settings" in the unknown/operator-answer bucket, consistent with sources.md's background note.
• Verified TOC anchors added in sources.md (#frontmatter-hooks-and-instructionsloaded, #blockers-between-the-root-and-a-nested-agentsmd) match GitHub's heading-slug algorithm for the new ## headings.
• Confirmed the version bump (0.18.2 → 0.18.3) and CHANGELOG.md entry are consistent and there are no other stale 0.18.2 references in the plugin.

Not independently verified: this sandbox's permission system required approval to execute discover.test.sh (and other shell invocations) that I could not obtain in this run, so I could not independently re-run the test suite or the doc snippets on a fixture. The PR body's claims of "57 cases, 0 failures" and the fixture runs of both doc snippets are author-claimed and unverified by me — I instead verified correctness by reading discover.sh's implementation and hand-tracing the new test's expected output against it, which lines up.

Findings: none blocking. This is a tightly-scoped, well-cross-referenced doc + test fix. The two conditions (A's nested-path walk, F's frontmatter scan) match the upstream doc quotes cited in sources.md, the new test case's expectations match the existing ip_entry_points_on_path implementation, and I didn't find a logic gap in either bash snippet after tracing both by hand.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8279ae8075

ℹ️ 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 6s —— View job

---

Code review complete

• Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
• Invoke /review:code-review per custom instructions
• Review changed files, focusing on what changed since the last review (commit 8279ae8)
• Post final summary

Scope reviewed: diff origin/main...HEAD (head 6d94e3f), all 5 changed files in plugins/instruction-placement. The two prior Claude comments on this PR already reviewed commit 8279ae8; this pass focuses on what changed since then — commit 4479f97 ("scan --add-dir roots and report unstat-able roots in the shim rule"), which responds directly to the two Codex P2 comments on this PR, plus the merge commit 6d94e3f (confirmed clean — no conflict markers, no duplicated headings).

What I checked directly:

• Hand-traced the new fm_gone() helper in shim-droppable.md against both Codex findings:
  • "Include --add-dir skill roots in the scan": the fm_scan invocation now adds <each --add-dir directory> as a scan root, and sources.md documents the permissions-page basis for what --add-dir loads. Resolved.
  • "Report inaccessible scan roots instead of skipping them": previously [ -e "$d" ] || continue silently skipped any root an unreadable ancestor made unstat-able. The new code only takes that silent-skip path for ?-prefixed optional roots, and only when fm_gone positively confirms the path is gone (walks up to the nearest existing ancestor and requires that ancestor be searectable with -x). When the blocking ancestor itself isn't searchable, fm_gone returns false and the root falls through to UNREADABLE, which is exactly the reported gap. Mandatory (non-prefixed) roots were never subject to the silent-skip branch. I traced both the single-missing-level and multi-level-missing cases by hand against the implementation and didn't find a path that still silently drops an unresolvable root.
• Verified ip_entry_points_on_path in discover.sh:279-288 against the new regression test case in discover.test.sh — the walk order (nearest directory first, via dirname to root) matches the test's expected list exactly, and assert_has/assert_lacks/assert_lists call sites match their helper signatures.
• Confirmed no leftover merge artifacts (conflict markers, duplicate ## headings) from merging origin/main into the branch.
• Confirmed the 0.18.3 → 0.18.4 version bump and the squashed CHANGELOG.md entry accurately describe the final state (including the --add-dir fix), not just the pre-fix state.

Not independently verified (no tool execution available to me in this run): the PR body's test-suite and fixture-run claims, and this session could not execute discover.test.sh or the doc's bash snippets to confirm empirically — I verified by reading the implementation and tracing the new logic by hand instead, which checks out.

Findings: none blocking. This follow-up commit closes both open Codex P2 comments with a principled fix (expected-vs-optional root distinction, existence vs. provable-absence distinction) rather than a surface patch, and I didn't find a new gap in the updated logic.

[claude]

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

---

Security review: no issues found

• Gather context (PR diff, changed files)
• Invoke /review:security-review skill and follow its procedure
• Post findings (or confirm no issues)

Scope reviewed (git diff origin/main...HEAD, head 6d94e3f): all 5 changed files —
plugin.json (version bump),
CHANGELOG.md,
discover.test.sh,
and the two reference docs
shim-droppable.md and
sources.md.

Findings: none.

This is documentation + test-fixture changes to the migrate skill's shim-droppable decision
procedure, plus a version/changelog bump. There is no executable production code path, no
auth/authz logic, no network or untrusted-input handling, and no secret/credential handling. The
fm_scan/fm_gone/nested-path-walk shell snippets in shim-droppable.md are reference commands
an operator runs by hand over their own repository/config during a migration decision, not shipped
automation processing attacker-controlled input; variable quoting is correct throughout
("$root", "$d", "$f", etc.), and find/awk output is read with IFS= read -r.

This head (6d94e3f) already incorporates the follow-up commit
(4479f97) that addresses both P2 findings from Codex's earlier review on this PR — --add-dir
roots are now included in the fm_scan invocation list, and an unreadable/unresolvable root
(optional or expected) now prints UNREADABLE via the new fm_gone helper instead of being
silently skipped. I re-traced that logic against the current diff and it holds: an optional root
(?-prefixed) is skipped only when fm_gone proves it provably absent via a searchable ancestor;
every other missing-or-unreadable case falls through to UNREADABLE.

Instruction-surface lens: this diff only tightens conditions A and F (widens what counts as a
blocker, adds new sources to check) — it does not delete, narrow, or soften any existing guardrail,
so the protected-class register
(docs/conventions/instruction-exception-register/README.md) does not apply here.

No GitHub Actions workflow files are touched.

[kyle-sexton]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6d94e3f147

ℹ️ 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".

[kyle-sexton]

@codex review

[claude]

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

---

Security review: no issues found

• Gather context (PR diff, changed files, custom instructions)
• Invoke /review:security-review skill and follow its procedure exactly
• Review changed files for security-relevant surface
• Check instruction-surface deletions against the protected-class register
• Post findings (or confirm no issues)

Scope reviewed (git diff origin/main...HEAD, head
e8a3b7b): all 5 changed files —
plugin.json (version bump),
CHANGELOG.md,
discover.test.sh,
shim-droppable.md and
sources.md.

Findings: none.

This remains documentation + test-fixture changes to the migrate skill's shim-droppable
decision procedure, plus a version/changelog bump. No executable production code path, no
auth/authz logic, no network or untrusted-input handling, and no secret/credential handling.

What's new since the last reviewed head (6d94e3f): this head's sole content commit
(e8a3b7b) responds to Codex's two latest comments on the previous head. I hand-traced both
fixes in shim-droppable.md:

• Symlinked scan roots (P2): fm_scan now canonicalizes each root with cd -P -- "$d" && pwd -P
  before searching, and scans with find -L, so a symlinked --add-dir/--plugin-dir root, or a
  symlinked skill/agent/command file inside a scanned tree, is now traversed and matched as a
  regular file rather than silently skipped — closing the gap the prior review flagged. fm_gone's
  added [ -L "$p" ] && return 1 correctly keeps a dangling symlink root out of the
  "provably absent, skip silently" path for optional roots, so a broken link still surfaces as
  UNREADABLE rather than being treated as absence. A GNU find -L symlink loop causes a non-zero
  exit (the loop warning is a nonfatal error that still sets find's exit status), which the || printf 'UNREADABLE' fallback catches, matching the doc's own description of that case. I did not
  find a path this leaves silently unscanned.
• Volatile hook mechanics restated in the skill body (P1): the condition-F prose in
  shim-droppable.md now states only the decision ("counts as a hook source whenever it might
  run... a pointer that comes to say something narrower does not loosen the scan until that record
  changes") and defers the mechanics (which components/events/locations) to the new
  sources.md records. This is a doc-authoring/freshness fix, not a security weakening: condition
  F's actual scan behavior and its fail-closed defaults are unchanged.

Instruction-surface lens: re-checked against
docs/conventions/instruction-exception-register/README.md
(present in this checkout, read directly). The diff only moves prose between two doc files and
tightens what conditions A and F check — it does not delete, narrow, or soften any enforced rule,
and none of the moved/reworded text matches a Gate 0 protected class (irreversible-action,
secret-handling, data-integrity, external-publication, legal-compliance, agent-authority), so the
register does not apply here.

Execution note: this sandbox's permission system declined approval for filesystem operations
needed to build a live fixture (symlink creation, removing a scratch test directory) outside basic
reads/greps, so I verified the symlink-handling fix by hand-tracing the shell logic against GNU
find/cd -P semantics rather than by running it — consistent with how this gap in tool access was
noted on the prior two heads of this PR.

No GitHub Actions workflow files are touched.

[claude]

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

---

Code review complete

• Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
• Invoke /review:code-review and follow its instructions
• Review changed files, focusing on what changed since the last review (commit 6d94e3f → e8a3b7b)
• Post final summary

Scope reviewed: git diff origin/main...HEAD at head e8a3b7b, all 5 changed files in plugins/instruction-placement. The prior three Claude passes on this PR already covered 8279ae8 and 6d94e3f in depth (including discover.test.sh, which this commit does not touch). This pass focuses on what changed in e8a3b7b — the fix for both outstanding Codex P1/P2 comments on this PR.

What I checked directly:

• Hand-traced the new fm_gone()/fm_scan() logic in shim-droppable.md L141–L179 against the "Handle symlinked roots" P2: each root is now canonicalized (cd -P -- "$d" && pwd -P) before find -L walks it, so a symlinked --add-dir/--plugin-dir root or a linked skill subtree is scanned at its target instead of silently matching nothing. I traced four cases by hand against the updated fm_gone: a valid root, a truly-absent optional root (fm_gone true → silently skipped, correct for "optional"), a dangling-symlink root whether optional or expected (the new [ -L "$p" ] && return 1 makes fm_gone false, so it falls through to UNREADABLE instead of being treated as absent), and an unresolvable/unreadable root (cd -P fails → UNREADABLE). I didn't find a path where a dangling link or unresolvable root is now silently dropped.
• For the in-tree symlink-loop case, find -L exits non-zero when it hits a filesystem loop (a documented GNU find behavior), so the || printf 'UNREADABLE\t%s\n' "$d" after the find call fires even though some HIT/file rows may already have been printed before the loop was hit — consistent with the doc's "every root not fully read" framing elsewhere in this same function (I was not able to empirically re-verify this exact GNU find exit-status detail in this sandbox — ln/mkdir outside the repo checkout require approval I didn't have in this run — so I'm flagging that this one piece is reasoned from documented find behavior rather than independently executed).
• Checked the "Keep volatile hook mechanics out of the skill body" P1 against sources.md L438–L471 and the matching shim-droppable.md L127–L139: both records now state only the rule's own decision plus live pointers ("read live at the pointers... a pointer that comes to say something narrower does not loosen the scan"), with the specifics that used to be restated (which events frontmatter hooks accept, when they're registered, what --add-dir loads, what the memory page says about intermediate directories) removed in favor of the Recheck trigger bullet. This matches .claude/rules/skill-bodies-state-current-rules.md's "pointer, as-of date, recheck trigger, no restated specifics" shape.
• Confirmed the CHANGELOG entry (still under the unreleased [0.18.4] heading, not a new version bump — correct, since 0.18.4 covers this PR's whole fix) and plugin.json (0.18.4, unchanged by this commit) stay consistent with each other.
• Confirmed no merge artifacts and that the two Codex review threads on this PR are both now marked resolved by the author with matching commit SHAs.

Not independently verified: PR-body-claimed fixture runs for this commit's change (dangling-link, symlink-loop, chmod-000 cases) are author-claimed; I verified by hand-tracing the shell logic against discover.sh/the snippet itself rather than executing it, for the reason noted above.

Findings: none blocking. This commit closes both remaining Codex findings with logic that holds up under hand-tracing, and I did not find a new gap it introduces.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Chef's kiss.

Reviewed commit: e8a3b7b728

ℹ️ 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".