Skip to content

feat(claude-config): add the cross-surface instruction-conflict checks and an OPINION tier - #1316

Merged
kyle-sexton merged 22 commits into
mainfrom
feat/audit-pass-criteria
Jul 26, 2026
Merged

feat(claude-config): add the cross-surface instruction-conflict checks and an OPINION tier#1316
kyle-sexton merged 22 commits into
mainfrom
feat/audit-pass-criteria

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

The criteria payload for the audit-pass effort: new checks in the two catalogs that already own their surfaces, rather than a new catalog.

No linked issue.

What lands

  • claude-config:audit-instructions gains I12 and I13, plus an OPINION tier for rules that ship marked and default-off. I12 is the cross-surface instruction-conflict check — the one finding in this effort with no incumbent. It is scoped by routing rather than by assertion: claude-memory:audit's C6 already owns contradictions within the memory layer, so I12 takes cross-layer, non-memory, and managed-tier conflicts and does not duplicate it.
  • claude-memory:audit's C3 (Content Placement) gains three gaps closed in one revision. They land as one edit rather than three checks, because the routing table answers one question and three checks would emit three findings for one misplaced section.

Two corrections absorbed late, both worth calling out

  • Imports are resolved before comparison. A detector that reads CLAUDE.md without expanding @path imports is reading a different surface than the model sees — imported files are expanded into context at launch. Without this, every @docs/foo.md import is invisible to the check.
  • AGENTS.md is excluded by reachability, not by filename. Claude Code does not read a bare AGENTS.md, but the official docs steer repositories toward @AGENTS.md or ln -s AGENTS.md CLAUDE.md, and both make it live. A filename-based exclusion would have silently dropped the conflict class in precisely the repositories following official advice. Confirmed empirically: with the import in place, /context enumerates AGENTS.md as its own loaded Memory Files row.

The one empirical claim in the C3 revision is carried with explicit provenance rather than stated as documented behavior.

Verification

check-changelog-parity.sh --check and --check-bump, check-skill-portability.sh, check-silent-skips.sh, and validate-plugin-contracts.mjs pass on the rebased branch. skill-quality:check on audit-instructions: PASS, 0 errors, 0 warnings — 188/500 lines, description 666/1536 chars, all 6 base-ref trigger phrases preserved, markdownlint clean, and its script test passes.

Merge-order note

This branch takes claude-memory to 0.5.0. #1315 carries a 0.4.1 for the same plugin that is not on main yet. Whichever merges second needs the other's changelog entry inserted in order — the resolution is mechanical, but it is not a clean auto-merge.

Review focus

The catalog text was written by one context and has not had an independent read. I12's surface list and the Type B allowlist are the two places where a fresh reviewer is most likely to find something: Type B conflicts (one absolute, one conditional, same act) ship only with a curated safety-critical allowlist, and that dependency is meant to be binding rather than advisory.

Related

🤖 Generated with Claude Code

kyle-sexton and others added 3 commits July 25, 2026 10:22
…on-audit catalog

Two new checks and three edits to existing ones, per the proportionality
gate's D1-D4 and D6 dispositions.

I12 (cross-surface instruction conflict) detects two live instructions
that cannot both be satisfied where no official layering rule already
picks a winner. It is scoped by routing around the incumbent rather than
duplicating it: claude-memory:audit C6 already reviews contradictions
inside the memory layer, so a contradiction wholly inside that layer is
C6's and I12 does not report it. A contradiction with a side outside the
memory layer, or any side in the managed-policy tier, is I12's.
Remediation splits by scope and never defaults to deletion. Findings are
pairwise and name both participating locations.

I13 (definition-site locality) is a different axis from I3: I3 is load
timing, I13 is locality, and an instruction can be correctly deferred and
still misplaced. OPINION-tier and off by default.

The stopping condition on I6 and I8 withholds trim proposals against
high-consequence areas. It is OPINION-tier but enabled by default,
because a suppressor defaulted off deletes the only bound those two
checks have.

The catalog gains the OPINION enablement policy the tier never had, and
YAML frontmatter carrying version 1.1.0 in place of the body-prose line.
I3's remediation now qualifies its destination (@path imports do not
defer) and prices the move; I9's names the interface destination.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…in C3

One consolidated revision to check C3 (Content Placement), not three
checks: the rows answer one question, so separate filing would emit three
findings on one misplaced section.

The destination set predated auto memory as a destination - the plugin
audits it as a first-class entity in M1-M4 but never routed content to
it. It now carries a row, stating that Claude writes it and that asking
Claude to remember something lands there rather than in CLAUDE.md.

@path imports are named as a non-destination: imported files load at
launch, so a split into imports reorganizes and saves nothing, and the
same holds for an import inside a path-scoped rule. Every move
recommendation is now priced against the compaction table in
reference/official-guidance.md. Both facts were already recorded in that
file and no check cited them.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…irical claim

Three corrections from review of the I12/C3 landing.

I12's comparison set is now resolved before it is compared: @path imports
expanded and symlinks followed. Imported files load at launch, so a
detector reading only the importing file compares a different surface
than the model sees, and every @docs/foo.md import would be invisible to
it. AGENTS.md is affirmatively excluded with the reason recorded - Claude
Code reads CLAUDE.md, not AGENTS.md, so a stock install never loads one
and flagging it would false-positive on repos that keep one for other
tools; its content enters only through an import.

C3's import-inside-a-path-scoped-rule clause was the only claim in either
catalog resting on a first-party repro rather than a cited page. It moves
out of the routing table into a provenance-marked paragraph, following
RD1's idiom, so the update action cannot overwrite it with doc-sourced
text and its recheck trigger is a re-run rather than a doc re-fetch.

Adds the audit-instructions eval that was missing for I3's tightened
destination rule - the edit changed remediation behavior with no case
exercising it.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a5ce478453

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

Comment thread plugins/claude-config/skills/audit-instructions/reference/criteria.md Outdated
Comment thread plugins/claude-config/skills/audit-instructions/reference/criteria.md Outdated
Comment thread plugins/claude-config/skills/audit-instructions/evals/evals.json Outdated
kyle-sexton added a commit that referenced this pull request Jul 25, 2026
…1318 ship

Task #21, walked against the gate's own body in MIGRATION-PLAYBOOK.md rather
than against this plan's seven-word summary of it, and against the two PRs'
shipped files rather than their descriptions. Recording a pass on a criterion
nobody substantiated would have been the same read-the-incumbent's-body failure
this effort keeps recording, applied to the gate itself.

One criterion fails. Security-review criterion 4 forbids ../ reach-outs, and
audit-pass carries three — SKILL.md:163, run-contract.md:104, and
exclusion-set.md:72 — all pointing at docs/conventions/finding-suppression,
which #1318 creates at the repository root. Each resolves in this repository and
to nothing in an installed plugin. That is this plan's own horizontal-decoupling
grounding finding landing on the artifact it was written to protect, and the
playbook lists it first under the caveats that break a working in-repo skill.
The convention document is the right home for the keys; the pointer is what has
to change.

One criterion cannot close. The plugin-acceptance security review still owes an
adversarial injection fixture corpus and verification that the exclusion-set
derivation is not itself an injection target. Its hook, MCP, telemetry,
provenance, and PATH surfaces are all clean — audit-pass ships none of them — so
the residual is entirely the runtime behavior the threat model covers.

Five criteria pass with evidence. Repo-agnosticism is real rather than assumed:
the exclusion set derives every class and names the empty case for a target with
no registry, and there are zero hardcoded repo or machine identifiers. No
userConfig is the correct answer rather than an omission, because the ownership
table assigns by kind and audit-pass has no personal-or-administrator scalar.

Two divergences from this branch's design are recorded as findings against the
implementation lane. The shipped run-contract carries the superseded
(surface, check, anchor, claim) tuple, which cannot express the pairwise finding
D1 exists to produce, and has no liveness-basis concept at all — so Assertion
1.1 ships unscoped and P1 ships without its liveness clause. Neither is a gate
criterion; both would make the shipped contract fail its own idempotence claim.

The no-PII pass is today's, with the hazard named forward: when the liveness
basis lands it must take the scope-prefixed form surface already takes, or a
report that may be redirected into the target tree carries the operator's home
path.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… skill

Both catalogs recommend moving only-sometimes-relevant content to a skill,
and both priced that move only against compaction. That undercounts it.
A skill's body defers, but its listing entry does not: skill descriptions
are loaded into context so Claude knows what is available, and the combined
`description` and `when_to_use` text is truncated at 1,536 characters in
the listing to bound that cost. So "move it to a skill" moves part of the
cost into the always-loaded tier rather than out of it, and a remediation
that omits the entry reports a saving larger than the one it delivers.

The cost is charged only where it is real. A new skill adds an entry; a
move into a skill that already exists adds none, and pricing that case
would overstate the cost in exactly the way this fix exists to prevent.
The one field that keeps a description out of context is
`disable-model-invocation: true`, which also makes the skill
user-invocable only — named with its consequence, since recommending it
silently is the capability change the compaction clause already refuses.
`user-invocable: false` does not remove the description, and
`skillOverrides` does not reach plugin skills, so neither is an out.

Stated as a cost on the recommendation, not as a Detect clause or a
threshold: whether a corpus is over its listing budget is a separate
check and a separate work item.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

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


PR Review — feat(claude-config): cross-surface conflict checks and OPINION tier

  • Gather context and understand the PR
  • Read changed files
  • Evaluate I12's surface list and routing logic
  • Evaluate C3 revision and import-resolution logic
  • Review Codex findings for validity
  • Post final review

Summary

The design is sound: the routing convention, OPINION-tier policy, stopping condition, and import-expansion requirement are all well-reasoned. Three findings below, in descending severity — two confirm Codex's P1/P2, and one is new.


Finding 1 — C6 scope gap invalidates I12's routing for nested and user-scope CLAUDE.md (confirmed P1)

Severity: Moderate

I12's routing rule says: "A contradiction wholly inside the memory layer… is C6's." But C6's actual discovery (in audit.md Step 3) only cross-references CLAUDE.md (found by find . -maxdepth 1) against .claude/rules/ and CLAUDE.local.md. It does not scan:

  • Nested CLAUDE.md files deeper than the project root
  • User-scope ~/.claude/CLAUDE.md

I12's comparison set explicitly includes "CLAUDE.md at every scope — managed policy, user, project root, nested, CLAUDE.local.md." So a contradiction between ~/.claude/CLAUDE.md and ./subdir/CLAUDE.md — both firmly inside the memory layer — is routed to C6, but C6 never reads either file. The conflict is silently dropped.

The fix is either: (a) expand C6's Step 1 discovery to include all nested CLAUDE.md files and user-scope CLAUDE.md before the cross-reference step, or (b) tighten I12's routing to delegate to C6 only for "project-root CLAUDE.md, CLAUDE.local.md, and project rules" contradictions, retaining nested and user-scope conflicts as I12's responsibility. Option (b) is lower-risk since it doesn't require changing the incumbent skill.


Finding 2 — Eval ID 8 conflict pair can be satisfied simultaneously (confirms Codex P2)

Severity: Low

evals.json ID 8 expects I12 to flag:

CLAUDE.md: "leave documentation as appropriate" vs SKILL.md: "DO NOT add comments"

These can both be satisfied — you can update a README without adding code comments. Per I12's own threshold, a finding requires "two live instructions that cannot both be satisfied." This example doesn't clear that bar. The eval trains the skill toward a false positive in exactly the case I12 explicitly says not to flag.

A genuinely incompatible replacement:

  • CLAUDE.md: "Add inline comments to explain every non-trivial decision"
  • SKILL.md: "DO NOT add comments — comment-free code is the standard here"

These cannot coexist — one demands inline comments, the other forbids them.

Fix this →


Finding 3 — I12's behavioral tier doesn't compose with Phase C's verify-pass protocol

Severity: Low / design note

I12 is tagged behavioral — which the criteria defines as "ground truth is observed model behavior, so findings ship as proposals verified by the delete-and-watch loop." But instruction-conflict detection is a textual comparison task, not a behavior observation. More importantly, Phase C dispatches subagents prompted to refute: "would removing this instruction cause Claude to make mistakes?" That question doesn't apply to conflict resolution — you're not proposing to remove an instruction, you're asking which of two conflicting instructions to reconcile. A Phase C verifier asked about an I12 finding has no coherent adversarial frame.

The practical effect: behavioral tier on I12 implies the Phase D report table will show "behavioral" in the Tier column, which could mislead a consumer into thinking the conflict finding needs a delete-and-watch experiment rather than human reconciliation judgment. mechanical would be the cleaner fit for the detection side (the conflict itself is pattern-detectable), even if the resolution is a judgment call. Alternatively, adding a clarifying note in the I12 entry that the verify pass is skipped for conflict findings (since there's nothing to delete-and-watch) would resolve the confusion without changing the tier tag.


I12 surface list — no issue found

Per the PR's review-focus request: the surface list is complete and correctly ordered. Managed policy, user, project root, nested, CLAUDE.local.md, rules, skill bodies, agent definitions, prompt-type hooks, output styles. The AGENTS.md exclusion by reachability (enters only through import expansion) is the right call — a filename-based exclusion would have dropped the whole conflict class for repos following the official @AGENTS.md pattern. Import expansion + symlink resolution before comparison is also correct.

C3 revision — no issue found

The three-gap revision (auto-memory as destination, @path imports as non-destination, move pricing) is clean. The empirical provenance marking on the path-scoped-rule import behavior is the right call; the update action protection note is appropriate.

Merge-order note acknowledged

The 0.4.1 / 0.5.0 collision with #1315 is correctly flagged. The resolution is mechanical but not auto-mergeable; whoever merges second will need to hand-insert the other's changelog entry.


@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 2m 18s —— View job


Security Review

  • Gather context
  • Read changed files
  • Analyze for security vulnerabilities
  • Post findings

Scope reviewed: all 9 files changed in this PR (plugin manifests, changelogs, SKILL.md, criteria.md, evals.json for both plugins). Scripts (instruction-scan.sh, instruction-scan.test.sh) were not modified and are out of scope.


No security vulnerabilities found

This PR is entirely prose and configuration — audit criteria text, eval definitions, and version bumps. No new shell execution paths, no workflow files, no external dependencies, no secrets handling, and no GitHub Actions patterns were introduced. The relevant security properties of the existing system are preserved and, in several places, explicitly reinforced by the new criteria text.

Specific checks:

Surface Finding
Plugin manifests (plugin.json) Version bumps only; no new permissions, dependencies, or capabilities
GitHub Actions No workflow files touched
Shell execution instruction-scan.sh not modified; the call signature in SKILL.md (bash "${CLAUDE_PLUGIN_ROOT}/...") is unchanged
External egress No new outbound calls; audit output writes only to ${CLAUDE_PLUGIN_DATA} (plugin-isolated)
Credential/secret handling None present or added
Supply chain No new action pins or package dependencies

I12 managed-policy handling is correct. The new check reads managed-policy CLAUDE.md for conflict detection but the criteria explicitly prohibit proposing any edit to that tier — neither to the policy side nor to the consumer side on the strength of the conflict alone. The remediation section correctly routes this as an organizational decision.

Symlink resolution scope (I12's "resolve symlinks before comparing") mirrors what Claude Code itself does at session launch, so no new attack surface is opened. The audit reads whatever surfaces Claude Code would load; it produces a local-only report with no exfiltration path.

OPINION-tier defaults are set conservatively: withholding rules (stopping condition on I6/I8) default on; emitting rules (I13) default off. Disabling a suppressor makes the audit more aggressive, not more conservative — the documentation correctly explains this inversion.

Report-only contract is maintained and strengthened. SKILL.md continues to state no --fix flag, all diffs are human-applied artifacts, and the new I12 remediation block adds an explicit "never a default deletion" guard.


kyle-sexton added a commit that referenced this pull request Jul 25, 2026
… moved

The proportionality pass downgraded the state key's repo-identity half and
deferred per-lane input digests, but only the gate document said so. The re-run
contract still presented both as settled, unqualified — which re-created the
same two-documents-disagree defect the pass was closing. Fixed at the contract
end, pointing at the gate's reasoning rather than restating it.

Assertion 5.2 is now explicitly owed only under the per-lane form. Assertion 5.1
holds under both, because a tree-wide refuse-to-resume check closes the P1 hole
just as completely; what per-lane digests buy is partial resume, and its value
is unmeasured until Phase 10 has lane costs.

Also records a cross-PR semver collision the acceptance-gate walk surfaced.
#1316 and #1318 both take claude-config from 0.9.2 to 0.10.0. Each passes
changelog-parity on its own branch; the pair does not, and the second to merge
lands a version already claimed with a duplicate changelog heading. Invisible
from either branch and surfacing only at the second merge, so it is recorded
where both lanes can see it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 78aac8351c

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

Comment thread plugins/claude-config/skills/audit-instructions/reference/criteria.md Outdated
Comment thread plugins/claude-config/skills/audit-instructions/reference/criteria.md Outdated
Comment thread plugins/claude-config/skills/audit-instructions/SKILL.md Outdated
… changed

The previous commit added doc-sourced content to both catalogs — the skill
listing entry's cost, quoted from a page neither catalog had cited before —
and left the date stamps reading 2026-07-24. Under either reading of the
field, last edited or last reconciled against official docs, both were
wrong the moment that commit landed.

A staleness stamp that its own edits do not move is worse than none: it
reports a recheck that did not happen. The catalog versions stay at 1.1.0
and 1.3.0 because neither has shipped — the plugin manifests carrying them
are unreleased on this branch, so the listing-entry change is part of the
same revision, not a successor to it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

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


PR Review — feat(claude-config): cross-surface conflict checks and OPINION tier

  • Gather context and understand the PR
  • Read changed files at current HEAD
  • Evaluate new commit (date stamps)
  • Check whether prior review findings have been addressed
  • Post final review

New commit (1d6e446) — clean

The commit corrects date stamps in both criteria catalogs from 2026-07-24 to 2026-07-25. The commit message correctly diagnoses the cause: commit 78aac83 added doc-sourced content (the skill listing-entry cost, drawn from a page neither catalog had previously cited) and did not advance the staleness date. The version fields (1.1.0 / 1.3.0) correctly stay unchanged because these versions are unreleased on this branch. No content changes; nothing new to flag.


Outstanding findings — all carry forward from the earlier review pass

None of the prior findings were addressed in commits 78aac83 or 1d6e446. They remain open.


Finding A — Eval 2 expectation is stale: I6-I11 should be I6-I13 (Codex P2, now confirmed)

evals.json line 25 requires:

"On memory-layer surfaces runs only the model-era checks I6-I11, not the I1-I5 hygiene checks"

SKILL.md line 44 now says:

"this skill runs only the model-era checks I6–I13"

A run that follows the current contract by running I12 and I13 on memory surfaces fails the eval. A run that satisfies the eval skips them. One side is wrong — it's the eval.

Fix this →


Finding B — Eval 8 instructions can be satisfied simultaneously (Codex P2, prior Finding 2)

evals.json ID 8, lines 93–94:

CLAUDE.md: "leave documentation as appropriate" vs SKILL.md: "DO NOT add comments"

You can update a README without adding code comments — both instructions satisfied simultaneously. I12's own threshold requires "two live instructions that cannot both be satisfied." This pair clears no threshold and trains toward a false positive.

A genuinely incompatible replacement:

  • CLAUDE.md: "Add an inline comment to every non-trivial decision"
  • SKILL.md: "DO NOT add comments — comment-free code is the standard"

Fix this →


Finding C — C6 scope gap drops nested and user-scope contradictions (Codex P1, prior Finding 1)

I12's routing rule (criteria.md line 244):

"A contradiction wholly inside the memory layer… is C6's."

C6's discovery (audit.md Step 3) compares only root-level CLAUDE.md against .claude/rules/ and CLAUDE.local.md. It does not read nested CLAUDE.md files or ~/.claude/CLAUDE.md. I12's comparison set names both as memory-layer surfaces and routes them to C6 — which never reads either. A contradiction between ~/.claude/CLAUDE.md and a nested CLAUDE.md is wholly inside the memory layer, routed to C6, and silently dropped.

Fix options: (a) expand C6's Step 1 discovery to cover nested and user-scope CLAUDE.md, or (b) retain those surfaces in I12 and narrow the routing to "project-root CLAUDE.md, CLAUDE.local.md, and project rules only." Option (b) avoids changing an incumbent skill.


Finding D — I12 behavioral tier doesn't compose with Phase C (prior Finding 3)

I12 is tagged behavioral — meaning "ground truth is observed model behavior, verified by the delete-and-watch loop." Instruction-conflict detection is textual, not behavioral. More critically, Phase C's verify pass prompts subagents: "would removing this instruction cause Claude to make mistakes?" That adversarial frame doesn't apply to a conflict finding — you're not proposing to remove an instruction, you're asking which of two conflicting instructions to reconcile. A Phase C subagent has no coherent basis for refuting it.

mechanical fits the detection side cleanly (textual pattern comparison). If behavioral is intentional for the resolution judgment, adding a note that Phase C is skipped for I12 findings (since there's nothing to delete-and-watch) would prevent the report's Tier column from misleading a consumer about what verification step applies.


Finding E — AGENTS.md / official-guidance.md data conflict (Codex P2, second round, not yet adjudicated)

official-guidance.md line 218 shows:

CLAUDE.md / AGENTS.md | Full load | Re-injected | —

That table row asserts AGENTS.md gets a full session-start load — independently, not only through an import. The PR's design excludes bare AGENTS.md from I12's comparison set on the documented grounds that "Claude Code does not read a bare AGENTS.md," confirmed empirically via /context with an import in place. But the table in the snapshot predates that test and makes the opposite claim.

The two facts cannot both be correct. Either official-guidance.md:218 needs to be updated to reflect the correct behavior (bare AGENTS.md is not loaded; only imported or symlinked AGENTS.md is), or I12's exclusion needs to be revisited. The empirical test cited in the PR is the strongest signal here, but the inconsistency inside the repository's own guidance snapshot is worth resolving before merge regardless of which direction.


Items confirmed no-issue (unchanged from prior review)


@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

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


Security Review — PR #1316

  • Gather PR context
  • Read changed files (all 9)
  • Analyze for security vulnerabilities
  • Post findings

Scope: 9 changed files — two plugin.json version bumps, two CHANGELOG.md additions, SKILL.md (argument-hint and routing text), evals/evals.json for both plugins, and reference/criteria.md for both plugins. No workflow files, no scripts, no package manifests.

No security vulnerabilities found.

This PR is entirely prose and configuration. Specific checks:

Surface Finding
plugin.json (both plugins) Version bumps only; no new permissions, capabilities, or dependencies
CHANGELOG.md (both plugins) Documentation only
GitHub Actions No workflow files touched
Shell execution The only shell invocation in SKILL.md is the pre-existing bash "${CLAUDE_PLUGIN_ROOT}/..." line — unchanged by this PR. The new --opinion and --no-stopping-condition flags are behavioral controls consumed by Claude's prose logic, not passed to a shell.
External egress New URLs in criteria.md are reference links for human readers, not programmatic fetch targets added to any execution path
Credential / secret handling None present or introduced
Supply chain No new action pins or package dependencies
Path traversal All path references use ${CLAUDE_PLUGIN_ROOT} and ${CLAUDE_PLUGIN_DATA} — plugin-scoped env vars, no ../ reach-outs

I12 managed-policy and import-expansion handling — no new attack surface. I12 instructs Claude to read managed-policy CLAUDE.md files for conflict detection, but the criteria explicitly prohibit proposing any edit to that tier. The @path import expansion and symlink resolution requirement mirrors what Claude Code already does at session launch — no new read paths are opened that Claude Code wouldn't traverse on its own. The report-only contract is maintained and the audit output writes only to ${CLAUDE_PLUGIN_DATA} with no exfiltration path.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1d6e4462fc

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

Comment thread plugins/claude-config/skills/audit-instructions/reference/criteria.md Outdated
Comment thread plugins/claude-config/skills/audit-instructions/reference/criteria.md Outdated
Comment thread plugins/claude-config/skills/audit-instructions/reference/criteria.md Outdated
… published URL

`audit-permission-grants` reached the permission-rule-hygiene convention
through a `../` relative link, in both `SKILL.md` and
`reference/criteria.md`. An installed plugin runs from an isolated cache
containing only the plugin's own tree, so the link normalized to a path
above the cache root and resolved to nothing. The skill directed a read
that cannot succeed in installed form.

It survived because it resolves in a full-repo checkout, which is the
shape every author sees. Verified against the live cache rather than on
paper: the installed root holds `.claude-plugin`, `.in_use`, `skills`,
`CHANGELOG.md`, and `README.md` — no `docs/` — and the normalized target
does not exist.

The convention's published URL is the fix, not a copy. Hand-copying the
principle, the three anti-patterns, and the correct pattern into the
plugin would create a second owner for rules the convention already owns
and guarantee drift. The URL form is what sibling plugins already use for
marketplace conventions, and it resolves from the isolated cache, which a
repo-relative path cannot.

Nothing about a run depended on the reach-out: each check's Recommend line
already carries the fix in the form the report needs. Both files now say
so, so the convention is named as the doctrine's owner rather than
implied to be a runtime dependency.

The reference file carried the same defect as the skill body and is fixed
with it. A sweep scoped to skill bodies and agent definitions does not
reach a skill's own reference tree, and one instruction surface fixed
while its sibling still points nowhere is not a fix.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 2m 13s —— View job


PR Review — feat(claude-config): cross-surface conflict checks and OPINION tier

  • Gather context and understand the PR
  • Read new commit (ae01bc7) changes
  • Check outstanding findings from prior review passes
  • Post final review

New commit (ae01bc7) — clean

The commit replaces ../../../../docs/conventions/permission-rule-hygiene/README.md relative paths in audit-permission-grants with the published raw GitHub URL, and adds a sentence making explicit that criteria.md carries everything the skill needs at run time — the convention is doctrine's owner, not a runtime fetch. This is the right fix: a skill running from the plugin cache cannot follow ../../../../ relative paths, since the cache layout does not include the repo's root docs/ tree. No concerns.


Outstanding findings — none have been addressed since the prior review

Five findings from the second review pass carry forward unchanged. Summarized below; the prior review thread has the full detail and permalink anchors.


Finding A — Eval 2 I6-I11 is stale; should be I6-I13 (Codex P2)

evals.json: eval ID 2 (scope-boundary-routes-out) still reads in both expected_output and its second expectation:

"On memory-layer surfaces runs only the model-era checks I6-I11"

SKILL.md line 44 now says I6–I13. A run that follows the current contract by running I12 and I13 on memory surfaces fails this eval. One side is wrong — it's the eval.

Fix this →


Finding B — Eval 8 instructions can be satisfied simultaneously (Codex P2)

evals.json ID 8: the prompt pair is:

CLAUDE.md: "leave documentation as appropriate" vs SKILL.md: "DO NOT add comments"

You can update a README without adding code comments — both instructions are simultaneously satisfiable. I12's own threshold requires "two live instructions that cannot both be satisfied." This pair clears no threshold and trains toward a false positive.

A genuinely incompatible replacement:

  • CLAUDE.md: "Add an inline comment to every non-trivial decision"
  • SKILL.md: "DO NOT add comments — comment-free code is the standard"

Fix this →


Finding C — C6 scope gap silently drops nested and user-scope contradictions (Codex P1)

criteria.md I12 routing: "A contradiction wholly inside the memory layer… is C6's." C6's actual discovery in claude-memory:audit reads only the project-root CLAUDE.md against .claude/rules/ and CLAUDE.local.md. It does not scan nested CLAUDE.md files or ~/.claude/CLAUDE.md. I12's comparison set names both as memory-layer surfaces and routes them to C6 — which never reads either. A contradiction between ~/.claude/CLAUDE.md and a nested CLAUDE.md is wholly inside the memory layer, routed to C6, and silently dropped.

Fix options: (a) expand C6's Step 1 discovery to cover all nested and user-scope files before delegating, or (b) narrow I12's routing to "project-root CLAUDE.md, CLAUDE.local.md, and project rules only" and retain nested / user-scope conflicts in I12. Option (b) avoids changing an incumbent skill.


Finding D — I12's behavioral tier doesn't compose with Phase C's verify pass

criteria.md I12 entry: Tier behavioral. Phase C dispatches subagents asked "would removing this instruction cause Claude to make mistakes?" That question has no coherent frame for a conflict finding — you're not proposing to remove an instruction, you're asking which of two contradictory instructions to reconcile. The behavioral tag on the report's Tier column implies a delete-and-watch experiment that has no applicable subject here.

mechanical fits the detection side cleanly (textual comparison). Alternatively, a note in I12 that the Phase C verify pass is skipped for conflict findings (there is nothing to delete-and-watch) would prevent the Tier label from misleading consumers without changing the classification.


Finding E — official-guidance.md:218 contradicts the PR's AGENTS.md exclusion (Codex P2)

official-guidance.md line 218:

CLAUDE.md / AGENTS.md | Full load | Re-injected | —

This asserts that bare AGENTS.md gets an independent full load. The PR's design — confirmed empirically via /context with an import in place — excludes bare AGENTS.md from I12's comparison set precisely because Claude Code does not read it without an import or symlink. The two claims cannot both be correct. The inconsistency inside the repository's own guidance snapshot is worth resolving before merge regardless of which direction: either update official-guidance.md:218 to say bare AGENTS.md is not loaded (only imported or symlinked), or revisit I12's exclusion.


Items confirmed no-issue (unchanged)

  • New commit ae01bc7 (permission-grants URL fix): clean.
  • I12 surface list and Type B allowlist: complete and correctly ordered.
  • I13 definition and OPINION-tier enablement logic: correctly inverted for suppressors vs emitters.
  • Stopping condition design: correct.
  • C3 revision in claude-memory:audit: clean.
  • Merge-order note with fix(setup): close the setup-corpus audit's findings and two falsified reference claims #1315: acknowledged.
  • Security posture: no concerns (confirmed in prior security review passes).

@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

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


Security Review — PR #1316

  • Gather PR diff
  • Read changed files
  • Analyze for security vulnerabilities
  • Post findings

Scope: all 11 changed files — two plugin.json version bumps, two CHANGELOG.md additions, two SKILL.md updates, two evals/evals.json additions, and three reference/criteria.md expansions. No workflow files, no scripts, no package manifests.

No security vulnerabilities found.

This PR is entirely prose and configuration. Specific checks:

Surface Finding
plugin.json (both plugins) Version bumps only — no new permissions, capabilities, or dependencies
CHANGELOG.md (both plugins) Documentation only
GitHub Actions No workflow files touched
Shell execution The only shell invocations in the SKILL.md files are the pre-existing bash "${CLAUDE_PLUGIN_ROOT}/..." lines — unchanged by this PR. The new --opinion and --no-stopping-condition flags are behavioral controls consumed by Claude's prose logic, not interpolated into a shell command
External egress No new programmatic fetch targets; the raw GitHub URL added to audit-permission-grants/criteria.md is a documentation reference explicitly excluded from runtime use by the criteria text ("a run never depends on fetching the convention")
Credential / secret handling None present or introduced
Supply chain No new action pins or package dependencies
Path traversal All path references use ${CLAUDE_PLUGIN_ROOT} and ${CLAUDE_PLUGIN_DATA} — plugin-scoped env vars, no ../ reach-outs

I12 import-expansion and symlink-resolution — no new attack surface. I12 instructs Claude to expand @path imports and resolve symlinks before comparing surfaces. This mirrors what Claude Code already does at session launch; the audit reads the same file graph the runtime reads. An operator who can write a malicious @path import into CLAUDE.md already has write access to the repository and has already affected the runtime — the audit check introduces no new read path that the runtime does not traverse on its own.

Managed-policy handling is correct. I12's criteria explicitly prohibit proposing any edit to the managed-policy tier, and the remediation block adds an explicit "never a default deletion" guard for both sides of any conflict. No write path to a managed surface exists.

OPINION-tier defaults are set conservatively. Suppressor rules (stopping condition on I6/I8) default on; emitting rules (I13) default off. Disabling a suppressor makes the audit more aggressive — the documentation correctly explains this inversion, and neither direction opens a security concern.

Unpinned raw GitHub URL — noted, not a vulnerability. audit-permission-grants/criteria.md now references https://raw.githubusercontent.com/melodic-software/claude-code-plugins/main/... (unversioned, pointing at main). Two explicit mitigations are in the same file: the criteria carry the fix language inline ("Each check's Recommend line below carries the fix in the form the report needs") and the SKILL.md states the convention is "not a runtime dependency." The URL is a documentation pointer for humans, not a programmatic fetch target. This warrants a note for future maintainers — if ever converted to a live fetch, pin to a commit SHA rather than main — but it is not a current vulnerability.

Report-only contract maintained. SKILL.md continues to state no --fix flag, all diffs are human-applied, and I12's remediation block adds an explicit "never a default deletion" guard. No automated write path exists.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ae01bc7c11

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

Comment thread plugins/claude-config/skills/audit-instructions/SKILL.md Outdated
Comment thread plugins/claude-config/skills/audit-instructions/reference/criteria.md Outdated
Comment thread plugins/claude-config/skills/audit-instructions/reference/criteria.md Outdated
Comment thread plugins/claude-config/skills/audit-instructions/reference/criteria.md Outdated
…ctions

Address the Codex review threads on the cross-surface conflict check:

- Cede memory-layer conflicts to claude-memory C6 only for the surfaces C6
  actually discovers; user-scope and nested memory conflicts stay in I12.
- Inventory managed policy and out-of-scope I12 counterparts read-only in
  Phase A, and extract prompt-hook text from .claude/settings.local.json.
- Run I12 in one dedicated cross-surface lane instead of per-surface lanes.
- Drop MCP servers from the shadowed-definition report; nothing inventories
  MCP configuration here.
- Add a no-change report representation so the managed-policy no-edit rule
  no longer contradicts the per-finding diff contract.
- Filter the comparison set by co-activation; correct the disjoint-scope
  exemption to two conditional instructions.
- Constrain I13 move destinations to surfaces Claude loads.
- Fix the stale I6-I11 eval strings and replace the co-satisfiable pair in
  the conflict eval.
- Correct the claude-memory guidance snapshot that claimed Claude Code
  loads AGENTS.md, per https://code.claude.com/docs/en/memory
@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

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


PR Review — feat(claude-config): cross-surface conflict checks and OPINION tier

  • Gather context and understand the PR
  • Read new commit (45b88fe) changes
  • Check which prior findings are addressed
  • Evaluate any new issues
  • Post final review

New commit (45b88fe) — substantive, closes four of five outstanding findings

The commit addresses the bulk of the open review thread: C6 routing tightened, cross-surface conflict lane added, co-activation filter, disjoint-scope exemption corrected, no-change representation for managed-policy findings, I13 destination constrained to loaded surfaces, MCP servers dropped from the shadow report, managed policy and out-of-scope counterparts inventoried read-only, local-settings hooks included, eval strings updated, and the official-guidance snapshot corrected. The scope-filter logic ("narrows which side may produce findings, not which are read") is cleanly stated and handles the scoped-invocation gap that Codex flagged.


Findings closed since the prior review pass

Prior finding Status
A — Eval 2 I6-I11 stale, should be I6-I13 Closed. Both strings updated; the expectation now correctly parenthesizes (I13 only under --opinion).
B — Eval 8 instructions co-satisfiable Closed. Replaced with "every public function must carry a doc comment" vs "DO NOT add comments to any code you touch" — these cannot coexist on any public function the skill touches.
C — C6 scope gap drops nested and user-scope contradictions Closed. Routing now reads: "wholly inside the memory surfaces C6 actually inventories — project-root CLAUDE.md/CLAUDE.local.md and the project .claude/rules/ tree." User-scope and nested contradictions stay in I12 with an explicit "cede only what the incumbent can see" note.
Eofficial-guidance.md:218 contradicted the AGENTS.md exclusion Closed. Table row updated to CLAUDE.md only; a new paragraph records the authoritative source and explains that AGENTS.md loads only through an import or symlink, never as its own surface.

One finding remains open

Finding D — I12's behavioral tier doesn't compose with Phase C's verify framing

criteria.md line 220: Tier behavioral

SKILL.md Phase C, lines 152–162: "prompted to refute: 'would removing this instruction cause Claude to make mistakes? Argue that it is still load-bearing.'"

The behavioral tag is unchanged and Phase C's verify prompt has no adaptation for conflict findings. "Would removing this instruction cause mistakes?" has no coherent subject when the finding is a conflict — you are not proposing to remove an instruction, you are asking which of two contradictory instructions to reconcile. If the verifier argues that both are load-bearing, it confirms the conflict rather than resolving it.

The new no-change representation removes managed-policy I12 findings from the diff contract, but it does not route them out of Phase C or adapt its framing. For non-managed-policy I12 findings (which do carry a reconciliation proposal), Phase C fires with the wrong question.

Two clean resolutions remain available: (a) change I12's tier to mechanical — the detection is textual comparison, and the reconciliation proposal is a rewrite that Phase C can evaluate with an adapted prompt — or (b) add a sentence in Phase C exempting I12 conflict findings from the delete-and-watch framing and replacing the verify question for that lane with "are these two instructions genuinely incompatible on any input the consuming surface covers?" Neither requires a large diff.

Fix this →


Items confirmed no-issue

  • Cross-surface conflict lane design (dedicated lane, not per-surface): clean. The "one lane regardless of how many surfaces are in scope" statement removes the previous ambiguity about re-deriving duplicate findings.
  • Co-activation filter: correct. Output styles and agent definitions are now explicitly excluded from cross-activation comparison.
  • Disjoint-scope exemption: fixed correctly. Two conditional instructions with disjoint conditions vs the prior formulation (one conditional + one unconditional, which always applies).
  • I13 destination constraint: correct. The "pointer on a loaded surface" safety net for subjects whose definition site is not loaded is the right call.
  • Eval 8 replacement pair: the new pair ("every public function must carry a doc comment" vs "DO NOT add comments to any code you touch") is genuinely incompatible and correctly exercises I12's threshold.
  • Managed-policy no-change representation: resolves the prior contradiction cleanly.
  • MCP shadow report dropped: correct — nothing inventories MCP configuration in this skill.
  • Official-guidance.md AGENTS.md correction: accurate and sourced to the docs URL.
  • Merge-order note with fix(setup): close the setup-corpus audit's findings and two falsified reference claims #1315: acknowledged; resolution remains mechanical.
  • Security posture: unchanged from the prior security review passes; no new concerns.

@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

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


Security Review — PR #1316

  • Gather PR diff
  • Read changed files (all 12)
  • Analyze for security vulnerabilities
  • Post findings

Scope: 12 changed files — two plugin.json version bumps, two CHANGELOG.md additions, two SKILL.md updates, two evals/evals.json additions, three reference/criteria.md expansions, and one reference/official-guidance.md update. No workflow files, no scripts, no package manifests were touched.

No critical or important security vulnerabilities found.

This PR is entirely prose and configuration. Specific checks:

Surface Finding
plugin.json (both plugins) Version bumps only — no new permissions, capabilities, or dependencies
CHANGELOG.md (both plugins) Documentation only
GitHub Actions No workflow files touched — no pull_request_target, workflow_run, or script injection surface
Shell execution SKILL.md shell calls use bash "${CLAUDE_PLUGIN_ROOT}/..." with a harness-provided env var, not user-interpolated input. The new --opinion and --no-stopping-condition flags are prose-interpreted behavioral controls, not interpolated into shell strings
Credential / secret handling SKILL.md line 102 explicitly guards against leaking hook text: "Extract the prompt text only; never carry a command line, token, or other secret-bearing value out of a settings file into the report." Correct
Supply chain No new action pins or package dependencies
Path traversal All path references use ${CLAUDE_PLUGIN_ROOT} and ${CLAUDE_PLUGIN_DATA} — plugin-scoped harness vars, no ../ reach-outs
Managed-policy reads I12 reads managed-policy CLAUDE.md read-only for conflict comparison. The criteria explicitly forbid any proposed edit to that tier. No write path exists
@path import expansion I12 expands imports and resolves symlinks before comparison. This mirrors what Claude Code does at session launch — no new read paths opened that the runtime wouldn't traverse on its own

SUGGESTION — Unversioned raw.githubusercontent.com URL (confidence: high; not a current vulnerability)

File: plugins/claude-config/skills/audit-permission-grants/reference/criteria.md, line 9

The criteria file references the permission-rule-hygiene convention at:

https://raw.githubusercontent.com/melodic-software/claude-code-plugins/main/docs/conventions/permission-rule-hygiene/README.md

This URL is pinned to main (mutable). The same file explicitly mitigates the runtime-fetch risk: "Each check's Recommend line below carries the fix in the form the report needs, so a run never depends on fetching the convention." The URL is a documentation pointer for human readers, not a programmatic fetch target in any execution path — so this is not a current vulnerability.

The risk materializes only if the URL is ever promoted to a runtime fetch. If that happens, main-branch content could be changed between skill invocations, injecting different "recommended" text into the audit report without a version gate. The mitigation for that future path is to pin to a commit SHA rather than main. No action required now, but worth recording for future maintainers before any live-fetch upgrade.


Report-only contract maintained and strengthened. SKILL.md continues to state no --fix flag. I12's remediation block adds an explicit "never a default deletion" guard for both sides of any conflict, and the no-change representation for managed-policy conflicts correctly exempts those findings from the per-finding diff contract rather than forcing a fabricated edit.

OPINION-tier defaults are set correctly. Suppressor rules (stopping condition on I6/I8) default on; emitting rules (I13) default off. The documentation correctly explains this inversion — disabling a suppressor makes the audit more aggressive, not more conservative.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 45b88fe4c2

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

Comment thread plugins/claude-config/skills/audit-instructions/SKILL.md
Comment thread plugins/claude-config/skills/audit-instructions/SKILL.md Outdated
…s and plugin cache

Managed-settings prompt hooks and live upstream-owned instruction text
(plugin-cache skill bodies and agent definitions, managed materializations)
can hold one side of a cross-surface conflict, so Phase A now reads them for
comparison. Ownership is unchanged: they propose nothing and route upstream.
@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

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


Security Review — PR #1316

  • Gather PR diff (focus on latest commit 87da6a1)
  • Read changed files
  • Analyze for security vulnerabilities
  • Post findings

Scope: 12 changed files — two plugin.json version bumps, two CHANGELOG.md additions, two SKILL.md updates (audit-instructions and audit-permission-grants), two evals/evals.json additions, three reference/criteria.md expansions, and one reference/official-guidance.md update. No workflow files, no scripts, no package manifests were touched.

Prior security reviews covered commits through 45b88fe. This pass focuses on the new commit 87da6a1, which widened Phase A's read-only inventory to include managed-settings prompt hooks and upstream-owned plugin-cache skill bodies.

No critical or important security vulnerabilities found.


Surface-by-surface checks

Surface Finding
plugin.json (both plugins) Version bumps only — no new permissions, capabilities, or dependencies
CHANGELOG.md (both plugins) Documentation only
GitHub Actions No workflow files touched
Shell execution The only shell call in SKILL.md is the pre-existing bash "${CLAUDE_PLUGIN_ROOT}/..." using a harness-provided env var. The new --opinion and --no-stopping-condition flags are prose-interpreted behavioral controls, not interpolated into shell strings
External egress No new programmatic fetch targets. The raw.githubusercontent.com/main/... URL in audit-permission-grants/criteria.md is a documentation pointer explicitly excluded from runtime use: "a run never depends on fetching the convention"
Credential / secret handling See note below
Supply chain No new action pins or package dependencies
Path traversal All path references use ${CLAUDE_PLUGIN_ROOT} and ${CLAUDE_PLUGIN_DATA} — plugin-scoped harness vars, no ../ reach-outs

New in 87da6a1 — read-only inventory widened to managed hooks and plugin cache

Managed-settings prompt-hook text. Phase A now extracts prompt-type hook text from managed settings for I12 comparison. The SKILL.md correctly carries over the prompt-text-only guardrail at line 117–118:

"Extract managed hook text under the same prompt-text-only, no-secrets handling as the other settings scopes."

The hook schema is consistent across all settings scopes (a hooks block with type: "prompt" entries each having a prompt string field), so "same handling" is unambiguous — there is no schema ambiguity that could cause the model to extract adjacent fields containing tokens or other secrets. The managed-policy finding still carries the no-change representation and the report persists only to ${CLAUDE_PLUGIN_DATA} with no exfiltration path.

Upstream-owned plugin-cache skill bodies and agent definitions. These are already live instruction text the model sees when those skills are invoked. Reading them for I12 conflict comparison does not open any read surface that the runtime hasn't already traversed. The read-only ownership flag is unchanged: a finding on plugin-cache content routes to the owning repository's tracker and proposes no in-place edit.


Pre-existing design pattern — noted, not a new finding

Phase C references a cross-vendor advisor ("e.g. the OpenAI Codex plugin") as an optional verifier. This was present before 87da6a1 and the prior security reviews confirmed it. The consent model is correct: the advisor fires only "when one is installed and set up" and "per its own docs," which presupposes the operator has already accepted that plugin's data terms. No new exposure is opened by this PR.

Report-only contract maintained. SKILL.md states no --fix flag throughout. I12's managed-policy case adds an explicit "never a default deletion" guard. No automated write path exists on any surface inventoried by this PR.


Four review findings on the cross-surface conflict pass.

The pass received Phase A's filesystem inventory and read presence in the
tree as liveness. Liveness is a session property: the launch directory picks
which ancestors are candidates, claudeMdExcludes merges across every settings
layer, omitting project from --setting-sources skips project rules, and
--add-dir with CLAUDE_CODE_ADDITIONAL_DIRECTORIES_CLAUDE_MD adds live memory
files no tree walk sees. Phase A now resolves those controls; what it cannot
resolve is marked liveness-unresolved rather than graded.

A prompt-type hook's text goes to a separate evaluator model and never enters
this session's context, so comparing that prose manufactured conflicts between
two models that each satisfy their own instructions. The hook now enters the
comparison set as the act it gates under its event and matcher. That also
closes the UNVERIFIED residency row.

Eval 13 rewarded saying an @path in an agent definition loads at launch, which
the catalog's own I13 contradicts; it now requires the reason that applies.

The claude-memory C3 eval left the CLAUDE.md remainder unspecified while
demanding it be moved. The prompt now says what the remainder is, and the
expectation requires the destination to be justified by relevance rather than
by the line budget.
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


PR Review — feat(claude-config): cross-surface conflict checks and OPINION tier

  • Gather context and understand the PR
  • Check latest commits since last review pass
  • Review changed files at current HEAD
  • Check whether Finding D and new Codex P2s have been addressed
  • Post final review

New commit (4edbe90) — closes all four open Codex P2s and Finding D

The commit closes every outstanding item from the prior review pass.


Finding D — closed in 4edbe90

SKILL.md Phase C, lines 218–222:

"A conflict pair takes a different refutation, because the removal prompt cannot falsify it: both sides are usually load-bearing, so 'argue it is still needed' defends both and demotes the finding untested. Refute a pair on its own gates — same observable, or two sharing a keyword? does any resident text already arbitrate? is there a prompt that fires both?"

This resolves the Phase C framing mismatch via option (b) — the refutation questions map directly to gates 2, 4, and 5, giving a verifier a coherent frame for a conflict pair rather than the removal question. The behavioral tier label on I15 remains in criteria.md, but Phase C's explicit guidance prevents the label from misleading a consumer about the verification procedure. Closed.


Four Codex P2s closed since the prior review pass

Codex finding Status
"Resolve effective liveness before running I15" (criteria.md) Closed. conflict-criteria.md now owns the five controls — launch directory, merged claudeMdExcludes, --setting-sources, --add-dir / CLAUDE_CODE_ADDITIONAL_DIRECTORIES_CLAUDE_MD, and declined external-import approvals — all sourced from the memory docs. Surfaces excluded or not-yet-inventoried are marked liveness-unresolved rather than silently dropped or falsely graded. Phase A carries matching instructions to resolve before pairing and name the resolved controls in the tier-transparency line.
"Exclude evaluator-only prompt directions from I15" (criteria.md) Closed. conflict-criteria.md now has a dedicated "A prompt hook's text is not an instruction" section, sourced verbatim from the hooks docs. A type: "prompt" handler text goes to a separate evaluator model and never enters the main session's context — comparing that prose against a skill or CLAUDE.md manufactured conflicts between two independent models. The hook enters the comparison set as the act it gates (the event + matcher), never the prose. The worked example ("git push --force after rebase" against a PreToolUse force-push deny) is correct.
"Correct the agent-import explanation in eval 13" (evals.json) Closed. Eval 13's second expectation now requires the reason that actually applies on the agent-definition surface: @path carries no import meaning in an agent definition, so the referenced file would never load — not "imported files load at launch," which is the memory-layer behavior. The distinction is real: the old reason was correct on CLAUDE.md and wrong on an agent body; the new reason is correct on the surface being audited.
"Do not require moving the unspecified CLAUDE.md remainder" (claude-memory/evals.json) Closed. The eval prompt now specifies what the remainder is ("a walkthrough of our Terraform module layout that only matters when I am working under infra/"), and the expectation requires the destination to be justified by relevance (the content is only sometimes relevant while the surface always loads) rather than by the line budget (removing the running log already fixes that). No longer trains toward a move justified only by excess length.

New content evaluated — no issues found

Liveness section in conflict-criteria.md: Comprehensive and correctly sourced. All five controls are documented with verbatim quotes from the memory docs. The liveness-unresolved marking policy prevents silent false-clean results on out-of-session inventories. The self-referential note that Phase A cannot reach plugins/ in the marketplace context is correctly documented as a known coverage gap tracked at #1421 rather than silently omitted.

Hook-as-gate section: The treatment is correct and the worked example is genuine. The "A prompt hook's text is not an instruction to the main session" heading correctly anchors the scope of what enters the comparison set (the gate) versus what doesn't (the evaluator's prose).

conflicts scope argument: Clean addition. "Phase A plus Phase B2 only, so a scheduled routine can compose it on its own budget" is a useful composition handle and correctly describes what the scope does.

Eval 13 distinction (@path on agent body vs. memory layer): The new expectation is precise in a way the old one was not — the old text could have been satisfied by a run that cited the wrong surface behavior and still passed. The new expectation names the failure mode ("silently removes the gotchas from every dispatch") rather than just the mechanism.


All prior findings — remain closed

  • Finding A (eval 2 I6-I11 stale): closed in 45b88fe4, unchanged.
  • Finding B (eval 8 co-satisfiable): closed in 45b88fe4, unchanged.
  • Finding C (C6 scope gap drops nested/user-scope contradictions): closed in 45b88fe4, unchanged.
  • Finding D (I15 tier + Phase C framing): closed in 4edbe90, as above.
  • Finding E (official-guidance.md AGENTS.md contradiction): closed in 45b88fe4, unchanged.

Version consistency — correct

claude-config is at 0.13.0 in both plugin.json and the CHANGELOG heading. claude-memory is at 0.5.0 in both. The version ladder accounts for the merges from #1349 (I12-I14, 0.10.0), #1318 (audit-pass, 0.12.0), and the intervening bumps.


No issues found. The PR is clean.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


Security Review — PR #1316

  • Gather PR diff and changed files
  • Read changed files
  • Analyze for security vulnerabilities
  • Post findings

Scope: 14 changed files — two plugin.json version bumps, two CHANGELOG.md additions, one README.md update, SKILL.md for both audit-instructions and audit-permission-grants, two evals/evals.json additions, three reference/criteria.md expansions, one new reference/conflict-criteria.md, and one reference/official-guidance.md update. No workflow files, no scripts, no package manifests were touched. The conflict-scan.sh and instruction-scan.sh scripts are referenced but not modified in this PR.

No critical or important security vulnerabilities found.

This PR is entirely prose and configuration — audit criteria text, eval definitions, conflict adjudication rules, and version bumps. Specific checks:

Surface Finding
plugin.json (both plugins) Version bumps only — no new permissions, capabilities, or dependencies
CHANGELOG.md (both plugins) Documentation only
GitHub Actions No workflow files touched
Shell execution The only shell calls in SKILL.md are bash "${CLAUDE_PLUGIN_ROOT}/..." — harness-provided env vars, no user-interpolated input. The conflict-scan.sh reference in Phase B2 is new prose but the script itself is unchanged (no diff)
External egress See SUGGESTION below for the one carry-forward URL concern
Credential / secret handling See analysis below — strengthened in the latest commit
Supply chain No new action pins or package dependencies
Path traversal All path references use ${CLAUDE_PLUGIN_ROOT} and ${CLAUDE_PLUGIN_DATA} — plugin-scoped harness vars, no ../ reach-outs

Secret handling — strengthened by latest commit (4edbe90)

SKILL.md lines 103–110:

"Extract the prompt text only; never carry a command line, token, or other secret-bearing value out of a settings file into the report. What is compared is the gate, not the prose: a prompt hook's text goes to a separate evaluator model, never into this session's context, so it enters the comparison set as the act it blocks under its event and matcher."

The latest commit tightened hook text handling: the hook enters the comparison set as the constraint it imposes (which act it gates + its event/matcher conditions), not its raw prose. This correctly prevents evaluator-directed content — including format directives, personas, or output-format instructions the evaluator's prompt contains — from reaching the conflict comparison. A prior incarnation would have compared that raw prose against main-session instructions, which could have pulled sensitive evaluator configuration into the audit report. The current formulation closes that gap cleanly.

The "prompt text only and no secret-bearing values" constraint also applies to the upstream-owned plugin hooks/hooks.json reading and the org-managed-settings hook reading, consistently.

Managed-policy reads — no write path

Org-managed policy CLAUDE.md and managed settings hook text are inventoried read-only for I15 conflict detection. The criteria explicitly prohibit any proposed edit to the managed-policy tier, and the no-change representation in Phase D means managed-policy findings never carry a manufactured diff. No write path exists.

Liveness resolution — correct direction for security

The latest commit adds a liveness-resolution requirement: Phase A must resolve claudeMdExcludes, --setting-sources, and --add-dir inputs before grading pairs. Unresolvable values are marked liveness-unresolved rather than graded. This is the security-correct direction — it prevents false positives from dead surfaces while ensuring out-of-session boundaries are transparent rather than silently treated as live.

Plugin cache reading — enablement-gated, version-resolved

Plugin skill bodies and agent definitions are inventoried only from the cache of a plugin whose effective enabledPlugins resolves enabled, and only from the install record selected for the current project. A disabled plugin's cache and superseded version directories are treated as unloadable. This prevents manufacturing conflict findings from text no session loads.


SUGGESTION (carry-forward from prior reviews) — unversioned raw.githubusercontent.com URL

Confidence: high · Not a current vulnerability

plugins/claude-config/skills/audit-permission-grants/reference/criteria.md line 9:

https://raw.githubusercontent.com/melodic-software/claude-code-plugins/main/docs/conventions/permission-rule-hygiene/README.md

This URL is pinned to main (mutable). The same file explicitly mitigates the runtime-fetch risk: "Each check's Recommend line below carries the fix in the form the report needs, so a run never depends on fetching the convention." The URL is a documentation pointer for human readers, not a programmatic fetch target in any execution path. Not a current vulnerability; the risk materializes only if ever converted to a live fetch — at which point, pin to a commit SHA rather than main.

This file is unchanged in this PR; the observation carries from prior security review passes.


Report-only contract maintained. SKILL.md states no --fix flag. I15's managed-policy case carries an explicit "never manufacture a diff to satisfy the table" guard. No automated write path exists on any surface inventoried by this PR. Report output writes only to ${CLAUDE_PLUGIN_DATA}/audit-instructions/last-audit.md with no exfiltration path.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Merge with main changed what this PR ships — please read before merging

main moved twice under this branch in claude-config:

So this PR's headline feature — its own I15 — has already landed on main from a sibling PR, in a more developed form. The five conflicted files were two independent implementations of the same check.

How I resolved it

main's I15 wins; this branch's is dropped. It is the merged incumbent, it carries a dedicated adjudication file with five gates, a residency table, and a precedence table, and shipping two I15s for one concern is exactly the silent-second-way this repo's conventions forbid. Composing the two texts would have produced a check with two disagreeing definitions.

Concretely dropped from this branch: its I15 catalog row, its SKILL.md cross-surface conflict lane, and the C6 routing paragraph — main's conflict-criteria.md supersedes that last one and is more accurate, since it explicitly corrects the "wholly inside the memory layer" rule this branch stated by routing on the population C6 actually enumerates.

What this PR still uniquely ships, all kept:

  • I16: Definition-site locality and the OPINION tier it needs — emitting rules default off behind --opinion and info-capped, withholding rules default on.
  • The stopping condition bounding I6 and I8, with --no-stopping-condition.
  • Phase A's read-only inventory tier — org-managed policy, upstream-owned-but-live plugin text, out-of-scope counterparts. main's Phase B2 needs exactly this to have a second side to compare against, so the two compose rather than collide.
  • The no-change representation in the report contract.
  • The claude-memory C3 revisions, and the I3 / I9 / authority-gloss changes.
  • Four review-round fixes (the resolved threads above), two of which correct main's merged conflict-criteria.md: the effective-liveness gate, and prompt hooks entering the comparison set as the gate they impose rather than as prose — which also closes that file's UNVERIFIED residency row.

Version: claude-config0.13.0 (main released 0.12.0 while this branch was open; the branch's own 0.11.0 section is now 0.13.0 and 0.12.0 is preserved verbatim). claude-memory stays at 0.5.0 over main's 0.4.1, which merged cleanly.

The PR title still says "add the cross-surface instruction-conflict checks", which is now only half true — the OPINION tier and I16 are the checks that remain new. Retitle before squash-merging if the commit subject matters.

Gates run locally on the merged tree: check-changelog-parity.sh --check and --check-bump origin/main, check-skill-portability.sh origin/main, check-silent-skips.sh origin/main, validate-plugin-contracts.mjs (43 setup skills, 2089 files), markdownlint-cli2 on every changed markdown file, and both evals.json files parse.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4edbe90ad2

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

Comment thread plugins/claude-config/skills/audit-instructions/SKILL.md
Comment thread plugins/claude-config/skills/audit-instructions/SKILL.md Outdated
Comment thread plugins/claude-config/skills/audit-instructions/evals/evals.json Outdated
…t style

Three review findings, all coverage gaps in the conflict pass's corpus.

Auto memory was excluded outright for routing, but conflict-criteria.md assigns
every pair involving it to I15 precisely because claude-memory's C6 does not
read MEMORY.md — so a MEMORY.md-versus-CLAUDE.md contradiction was audited by
neither skill. It now enters the read-only tier, loaded content only.

A plugin can ship an output style, and one with force-for-plugin applies
whenever the plugin is enabled, overriding the user's outputStyle. The
user/project scans cannot reach the plugin cache, so the active style could be
missing from the corpus entirely. Phase A now resolves which style is actually
active and inventories that one.

Eval 7 expected memory-layer contradictions to be reported unchecked when
claude-memory is absent, contradicting the fallback contract that keeps the
pair as an I15 finding. The routing avoids duplicate findings; it does not
drop the only one.
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 2m 20s —— View job


PR Review — feat(claude-config): add the cross-surface instruction-conflict checks and an OPINION tier

  • Gather context and understand the PR
  • Read the new commit (52d3eaf) changes
  • Check which prior Codex P2 findings are addressed
  • Evaluate new content for correctness
  • Post final review

New commit (52d3eaf) — closes all three Codex P2s from the prior round

The commit addresses the three P2s Codex raised on 4edbe90 and is clean on all three.


Three Codex P2 findings closed since the prior review pass

Prior Codex finding Status
"Add auto-memory to the read-only conflict inventory" (SKILL.md:124) Closed. MEMORY.md at the effective autoMemoryDirectory location now enters the read-only tier, capped at the first 200 lines or 25KB matching the co-residency table. The rationale is stated correctly: claude-memory's C6 does not read MEMORY.md, so excluding it outright left a MEMORY.md-vs-CLAUDE.md contradiction audited by neither skill. Ownership unchanged: findings route to claude-memory rather than editing the file.
"Inventory active output styles shipped by plugins" (SKILL.md) Closed. The upstream-owned bullet now includes "the active output style when a plugin supplies it", with a correct resolution algorithm: force-for-plugin style from the enabled set wins first, then the outputStyle setting value (which may itself name a plugin-supplied style). The URL cited for force-for-plugin behavior matches the output-styles doc. Only the active style enters the corpus — inactive plugin styles remain excluded, consistent with the co-residency table's "other styles on disk are inventoried but never loaded."
"Preserve the I15 fallback when claude-memory is absent" — eval 7 (evals.json) Closed. Eval 7's expected_output and third expectation now correctly require the I15 fallback when claude-memory is not installed, matching the existing fallback contract at conflict-criteria.md lines 64–67: "when that plugin is not installed, keep it as a finding so nothing is silently dropped." The commit message correctly diagnoses the bug: routing exists to avoid two findings for one pair, not to lose the only one.

New content evaluated — no issues found

Auto-memory bullet placement: Correctly placed in the read-only tier (after the editable exclusion clause for auto memory at line 121). The two are not contradictory: the editable exclusion removes it from the proposal set; the read-only bullet adds it to the comparison set. Both are required for I15 to work.

Output-style resolution logic: The force-for-pluginoutputStyle resolution order is correct and matches the output-styles doc. The "only the active style is resident" constraint correctly prevents inactive plugin styles from entering the corpus.

Eval 7 first expectation precision: The expectation now says "root-level project memory surfaces" (rather than just "memory surfaces"), which correctly narrows to C6's actual population per the boundary table in conflict-criteria.md. A ~/.claude/CLAUDE.md-vs-.claude/rules/ pair would correctly stay in I15, not be routed to C6.

CHANGELOG entry: Accurate. The three fixes are narrated correctly and the routing-vs-dropping distinction is cleanly stated.


All prior findings — remain closed

  • Finding A (eval 2 I6-I11 stale): closed in 45b88fe4, unchanged.
  • Finding B (eval 8 co-satisfiable): closed in 45b88fe4, unchanged.
  • Finding C (C6 scope gap drops nested/user-scope contradictions): closed in 45b88fe4, unchanged.
  • Finding D (I15 tier + Phase C framing): closed in 4edbe90, unchanged.
  • Finding E (official-guidance.md AGENTS.md contradiction): closed in 45b88fe4, unchanged.

No issues found. The PR is clean.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


Security Review — PR #1316

  • Fetch PR diff and identify changed files
  • Read changed files
  • Analyze for security vulnerabilities
  • Post findings

Scope: 14 changed files — two plugin.json version bumps, two CHANGELOG.md additions, one README.md update, SKILL.md for both audit-instructions and audit-permission-grants, two evals/evals.json additions, three reference/criteria.md expansions, one new reference/conflict-criteria.md, and one reference/official-guidance.md update. No workflow files, no scripts, no package manifests were touched.

No critical or important security vulnerabilities found.

This PR is entirely prose and configuration. Specific checks:

Surface Finding
plugin.json (both plugins) Version bumps only — no new permissions, capabilities, or dependencies
CHANGELOG.md (both plugins) Documentation only
GitHub Actions No workflow files touched
Shell execution SKILL.md shell calls use bash "${CLAUDE_PLUGIN_ROOT}/..." with a harness-provided env var; the referenced scripts (instruction-scan.sh, conflict-scan.sh) are unchanged by this PR. New --opinion and --no-stopping-condition flags are prose-interpreted behavioral controls, not interpolated into shell strings
External egress No new programmatic fetch targets. The raw.githubusercontent.com URL in audit-permission-grants/reference/criteria.md is carried forward from prior state; the file explicitly excludes it from runtime use
Credential / secret handling See analysis below
Supply chain No new action pins or package dependencies
Path traversal All execution paths use ${CLAUDE_PLUGIN_ROOT} and ${CLAUDE_PLUGIN_DATA} — plugin-scoped harness vars

Secret handling — new settings.local.json hook extraction (SUGGESTION · confidence: medium)

File: plugins/claude-config/skills/audit-instructions/SKILL.md, lines 103–110

Phase A now inventories prompt hook text from .claude/settings.local.json alongside project and user settings.json. The audit-permission-grants/reference/criteria.md already states that settings.local.json "may hold tokens" and is "parsed for its permissions.allow array only — never read or echoed wholesale." The audit-instructions SKILL.md carries the same spirit via a prose constraint: "Extract the prompt text only; never carry a command line, token, or other secret-bearing value out of a settings file."

The constraint is model-enforced prose rather than a mechanical boundary. settings.local.json is the most credential-prone settings scope (per-machine API keys and tokens commonly reside there alongside hook configuration). If the model misapplies the constraint, hook-adjacent secret-bearing fields could reach the audit report — which persists to ${CLAUDE_PLUGIN_DATA}/audit-instructions/last-audit.md (local-only, no exfiltration path).

This is not a new attack surface for this PR — the same pattern was already approved for project and user settings.json in prior commits — but the local settings file is higher risk than either of those two scopes. No action is strictly required; noting it so future work can evaluate whether an explicit schema-constrained extraction (rather than prose-only guidance) is warranted.


../../../../ relative path crosses plugin boundary (SUGGESTION · confidence: high · not exploitable)

File: plugins/claude-config/skills/audit-instructions/reference/conflict-criteria.md, line 38

[`plugins/claude-memory/skills/audit/reference/criteria.md`](../../../../claude-memory/skills/audit/reference/criteria.md)

The CLAUDE.md plugin-form-safe rule reads: "Installed plugins run from an isolated cache — reference only files inside the plugin via ${CLAUDE_PLUGIN_ROOT}; persist state in ${CLAUDE_PLUGIN_DATA}. No ../ reach-outs." This link navigates four directories up from reference/ to reach a sibling plugin's criteria.md. In an installed plugin cache, following it would cross claude-config's plugin root boundary into claude-memory's.

In practice the risk is bounded: the target file is documentation (no secrets, no execution), the report output path is local-only, and this is a markdown hyperlink in prose rather than an active fetch instruction to the model. It does not open an information-exfiltration path. The finding is a plugin-isolation design rule deviation, not an exploitable vulnerability — it is worth resolving with a ${CLAUDE_PLUGIN_ROOT}-relative reference if the target file is co-packaged, or converting to prose if it is just a human-facing cross-reference.


Carry-forward observation (unchanged from prior security reviews)

File: plugins/claude-config/skills/audit-permission-grants/reference/criteria.md, line 9

The permission-rule-hygiene convention URL remains https://raw.githubusercontent.com/melodic-software/claude-code-plugins/main/... (mutable, main-pinned). Not a current vulnerability — the same file carries the fix text inline and explicitly states "a run never depends on fetching the convention." Risk materializes only if ever promoted to a runtime fetch; at that point, pin to a commit SHA.


Report-only contract maintained and strengthened. SKILL.md states no --fix throughout. I15's managed-policy remediation block adds "never manufacture a diff to satisfy the table." No automated write path exists on any inventoried surface. Output writes to ${CLAUDE_PLUGIN_DATA} only.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

contradiction is real whether or not this repository may edit either side. Resolve `@path` imports
and symlinks to their targets before pairing, so an imported file is compared as part of the
surface importing it rather than as a separate one.

P2 Badge Restrict import expansion to surfaces that support it

Fresh evidence after the earlier import-expansion thread is the surviving blanket instruction here: when an agent or skill body contains @path, I13 itself says the syntax is inert on those surfaces (criteria.md:275-280), but this rule still directs I15 to resolve it before pairing. That can make an unread reference file appear resident and produce a false conflict, so expand imports only for the CLAUDE.md-family surfaces that implement them.

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

Comment thread plugins/claude-config/skills/audit-instructions/SKILL.md Outdated
Comment thread plugins/claude-config/skills/audit-instructions/evals/evals.json Outdated
…eval 8

Auto memory is on by default, but autoMemoryEnabled: false at any settings
scope or CLAUDE_CODE_DISABLE_AUTO_MEMORY=1 turns it off, and a MEMORY.md left
on disk from before is then neither loaded nor written. Phase A resolves that
state before inventorying the file — the same gate the plugin-cache surfaces
carry, for the same reason.

Eval 8 asked the run to say which side to change for a skill body against a
CLAUDE.md, which the precedence table classifies as unresolved: the skills page
states no authority relation between the two, and silence is not a winner. The
eval now requires an unresolved verdict with both anchors quoted and the choice
left to the operator.
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


PR Review — feat(claude-config): add the cross-surface instruction-conflict checks and an OPINION tier

  • Gather context and understand the PR
  • Read new commits since last review pass
  • Check which Codex P2s from prior round are addressed
  • Evaluate new content for correctness
  • Post final review

New commit (3714d36) — closes two of three Codex P2s from the prior round

The commit addresses two of the three P2s Codex raised on 52d3eafe and is clean on both.


Two Codex P2 findings closed

Prior Codex finding Status
"Skip auto memory when it is effectively disabled" Closed. Phase A now gates MEMORY.md on the resolved enabled state — autoMemoryEnabled: false at any settings scope or CLAUDE_CODE_DISABLE_AUTO_MEMORY=1 turns it off, and a MEMORY.md left on disk from before is then neither loaded nor written. The rationale is correctly stated: pairing live instructions against text no session sees is the same defect as reading a disabled plugin's cache.
"Keep the unresolved conflict eval from choosing a side" (eval 8) Closed. Eval 8's expected_output and third expectation now require an unresolved verdict rather than naming a winner. The precedence table classifies skill body versus any CLAUDE.md as unresolved — the skills page states no authority relation between them — so naming a side would invent precedence the docs decline to state. The mechanism route (hook, permissions.deny, disallowed-tools) is correctly offered as an option, never a verdict.

One finding remains open

Codex P2 — Import expansion still applies blanket to all surfaces (criteria.md:352–354, not addressed in 3714d36)

criteria.md lines 352–354:

"Resolve @path imports and symlinks to their targets before pairing, so an imported file is compared as part of the surface importing it rather than as a separate one."

This is a blanket instruction with no surface restriction. I13 (criteria.md line 275–280) explicitly says: "Import syntax is a property of the CLAUDE.md family; on a skill or agent surface the @ is inert." The co-residency table confirms skill bundled reference/ and context/ files load "only when Claude reads it" — they are not part of the skill body's live instruction set.

If Phase B2 applies the blanket expansion rule to a skill body that contains @reference/rules.md, it will pull in the content of that reference file and compare it against CLAUDE.md as if it were live instruction text — but it was never loaded. The result is a false conflict against material that is only in context when Claude explicitly navigates to the reference file.

The fix is a one-line qualification: "Resolve @path imports and symlinks for memory-layer surfaces (CLAUDE.md at every scope, CLAUDE.local.md, and .claude/rules/) before pairing; on skill bodies, agent definitions, hooks, and output styles, @path is inert — an @path reference there does not expand to additional comparison text."

Fix this →


Items confirmed no-issue

  • Auto memory gating logic: correct. Resolves the env-var-wins-over-setting precedence (matches the sibling stateless resolver), and correctly states that a MEMORY.md left on disk from before auto memory was disabled is not loadable.
  • Eval 8 replacement: the unresolved verdict requirement is correctly sourced — "the skills page states no authority relation between them, and silence is not a winner" is the right anchor. The mechanism route offered as an option (hook / permissions.deny / disallowed-tools) is accurate.
  • All prior findings A–E and all prior Codex P2s except the one above: remain closed; no regression.
  • Merge-order note with fix(setup): close the setup-corpus audit's findings and two falsified reference claims #1315: acknowledged; resolution remains mechanical.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 2m 29s —— View job


Security Review — PR #1316

  • Gather PR diff (focus on latest commit 3714d36)
  • Read changed files
  • Analyze for security vulnerabilities
  • Post findings

Scope: 14 changed files. Prior security reviews covered the full diff through commit 52d3eaf; this pass focuses on the new commit 3714d36, which adds an auto-memory enabled-state gate and corrects eval 8. No workflow files, scripts, or package manifests were touched.

No critical or important security vulnerabilities found.


New in 3714d36 — auto-memory liveness gate

SKILL.md lines 131–144

Phase A now resolves the effective enabled state before reading MEMORY.md. The resolution algorithm:

  • CLAUDE_CODE_DISABLE_AUTO_MEMORY=1 → off
  • CLAUDE_CODE_DISABLE_AUTO_MEMORY=0 → on, overriding autoMemoryEnabled: false
  • Unset → settings-scope precedence, default true

This is the correct security direction: a MEMORY.md left on disk after auto memory was disabled is stale — reading it unconditionally would pair live instructions against text no session loads or writes. The gate prevents that without introducing any new read surface; it only constrains which path is inventoried.

The resolution algorithm matches the sibling stateless skill's resolver. No new read paths, no new write paths, no new execution surfaces.


Eval 8 fix — no security impact

The eval now requires an unresolved verdict instead of asking the run to name a winning side. The change is behavioral correctness only; no execution path is involved.


Carry-forward observations (unchanged from prior security reviews)

Surface Finding
plugin.json (both plugins) Version bumps only — no new permissions, capabilities, or dependencies
CHANGELOG.md (both plugins) Documentation only
GitHub Actions No workflow files touched
Shell execution The only shell calls in SKILL.md are bash "${CLAUDE_PLUGIN_ROOT}/..." using harness-provided env vars; --opinion and --no-stopping-condition are prose-interpreted behavioral controls, not shell-interpolated
External egress No new programmatic fetch targets
Credential / secret handling settings.local.json extraction is bounded by the prose constraint "prompt text only; never carry a command line, token, or other secret-bearing value"; audit-permission-grants/reference/criteria.md separately states the file is "never read or echoed wholesale (it may hold tokens)"
Supply chain No new action pins or package dependencies
Path traversal ${CLAUDE_PLUGIN_ROOT} and ${CLAUDE_PLUGIN_DATA} throughout — plugin-scoped harness vars

SUGGESTION (carry-forward) — cross-plugin relative path in conflict-criteria.md.
Line 38: ../../../../claude-memory/skills/audit/reference/criteria.md. This is a markdown hyperlink for human readers, not a fetch target, so it is not exploitable. In an installed plugin cache the path would not resolve across the plugin root boundary — this violates the repo's own plugin-form-safe rule ("No ../ reach-outs"), but has no security consequence. Noted for future maintainers.

SUGGESTION (carry-forward) — unversioned raw.githubusercontent.com URL.
audit-permission-grants/reference/criteria.md line 9: pinned to main. Not a current vulnerability — the file carries the fix text inline and the SKILL.md states "a run never depends on fetching the convention." Risk materializes only if ever promoted to a live fetch; at that point, pin to a commit SHA.

Report-only contract maintained throughout. SKILL.md states no --fix. I15's managed-policy case carries "never manufacture a diff to satisfy the table." No automated write path exists on any surface inventoried by this PR. Output writes only to ${CLAUDE_PLUGIN_DATA}/audit-instructions/last-audit.md.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3714d36cfe

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

Comment thread plugins/claude-config/skills/audit-instructions/SKILL.md
The known limit still said Phase A never reaches plugins/ and that
agent-versus-memory pairs have no second side. The read-only inventory tier
makes that false for every installed, enabled plugin, so two executable
instructions disagreed about whether the same data is available.

Narrowed to what remains true: a marketplace repository's plugins/ authoring
tree is plugin source, not an installed plugin, and nothing there loads into
the session being audited. The tier-transparency line reports that limit only.

Also restores the section heading this file lost when the effective-liveness
gate was inserted above it.
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


PR Review — feat(claude-config): add the cross-surface instruction-conflict checks and an OPINION tier

  • Gather context and read latest commit
  • Check whether open findings from last review pass are addressed
  • Evaluate new content for correctness
  • Post final review

New commit (5d1f4fe) — clean, closes the last Codex P2

The commit corrects the stale plugin-source limitation in conflict-criteria.md. The old text said "Phase A never reaches plugins/" and that "agent-versus-memory pairs have no second side" — both of which the read-only inventory tier (added in 87da6a16) made false for every installed, enabled plugin. The new ## Known limit: section correctly narrows the limitation to what is still true: a marketplace repository's own plugins/** authoring tree is plugin source, not an installed plugin, and nothing there loads into the session being audited. Installed plugin caches are covered; the authoring tree is not. The CHANGELOG entry accurately narrates the fix and the distinction. The section heading restoration (lost when the liveness gate was inserted above it) is correct.


One finding remains open

Blanket @path expansion in the I15 comparison set

criteria.md lines 352–354:

"Resolve @path imports and symlinks to their targets before pairing, so an imported file is compared as part of the surface importing it rather than as a separate one."

This blanket statement does not restrict @path expansion to CLAUDE.md-family surfaces. I13 (lines 275–280) explicitly states "Import syntax is a property of the CLAUDE.md family; on a skill or agent surface the @ is inert." Expanding @path on skill bodies or agent definitions would make unloaded reference files appear resident to I15 and generate false conflicts.

The prior version of this entry (before the 25e3d05 merge with main) had two correctly qualified bullets:

  • "Resolve before comparing — imports only where the surface implements them."
  • "An @path-shaped reference in a skill body or agent definition is not an import."

Both were lost in the merge conflict resolution. The fix is to qualify the expansion instruction so it reads something like: "Resolve @path imports for memory-layer surfaces (CLAUDE.md at every scope, CLAUDE.local.md, and .claude/rules/) before pairing; resolve symlinks everywhere. On skill bodies, agent definitions, hooks, and output styles, @path is inert — an @path reference there does not expand to additional comparison text."

Fix this →


Items confirmed no-issue

  • New commit 5d1f4fe (plugin-source limitation): the installed-vs-authoring distinction is correct and accurately sourced. The tier-transparency instruction ("report that narrower limit only") prevents the report from understating coverage.
  • All prior findings A–E and all prior Codex P2s except the one above: remain closed; no regression.
  • Merge-order note with fix(setup): close the setup-corpus audit's findings and two falsified reference claims #1315: acknowledged; resolution remains mechanical.
  • Security posture: unchanged from prior security review passes.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 2m 31s —— View job


Security Review — PR #1316

  • Gather PR diff (focus on latest commit 5d1f4fe)
  • Read changed files (all 14)
  • Analyze for security vulnerabilities
  • Post findings

Scope: 14 changed files — two plugin.json version bumps, two CHANGELOG.md additions, one README.md update, SKILL.md for both audit-instructions and audit-permission-grants, two evals/evals.json additions, three reference/criteria.md expansions, one reference/conflict-criteria.md update, and one reference/official-guidance.md update. No workflow files, no scripts, no package manifests were touched.

No critical or important security vulnerabilities found.

This PR is entirely prose and configuration. Specific checks:

Surface Finding
plugin.json (both plugins) Version bumps only — no new permissions, capabilities, or dependencies
CHANGELOG.md (both plugins) Documentation only
GitHub Actions No workflow files touched
Shell execution The only shell calls in SKILL.md are bash "${CLAUDE_PLUGIN_ROOT}/..." using harness-provided env vars; instruction-scan.sh and conflict-scan.sh are referenced but not modified in this PR. The new --opinion and --no-stopping-condition flags are prose-interpreted behavioral controls, not interpolated into shell strings
External egress No new programmatic fetch targets; the raw.githubusercontent.com/main/... URL in audit-permission-grants/reference/criteria.md is a carry-forward from prior state (see SUGGESTION below)
Credential / secret handling Prose constraint "prompt text only; never carry a command line, token, or other secret-bearing value" applies uniformly across all settings scopes; settings.local.json is "parsed for its permissions.allow array only — never read or echoed wholesale (it may hold tokens)"
Supply chain No new action pins or package dependencies
Path traversal All execution paths use ${CLAUDE_PLUGIN_ROOT} and ${CLAUDE_PLUGIN_DATA} — plugin-scoped harness vars

Latest commit 5d1f4fe — clean

The commit modifies only two files: CHANGELOG.md and conflict-criteria.md. The conflict-criteria.md change reorganizes the "Known limit" section to correctly distinguish between installed plugin cache (now covered by the read-only inventory tier) and the marketplace's plugins/ authoring tree (still out of scope). This is a documentation correctness fix; no new read paths, write paths, or execution surfaces are introduced.

Plugin-cache reading — no new attack surface

Phase A's read-only tier reads enabled plugin cache skill bodies, agent definitions, hooks/hooks.json prompt handler text, and the active output style. All are gated on effective enabledPlugins and the selected install record. An operator who enables a plugin has already accepted that plugin's content into their session — the audit reads the same file graph the runtime reads. The read-only/upstream-routing contract is unchanged and explicitly stated: no proposed edit may touch plugin-cache content.

${CLAUDE_CONFIG_DIR} env var — standard harness path

SKILL.md line 96-97 instructs resolving ${CLAUDE_CONFIG_DIR:-~/.claude} as the user config root. This is a standard harness-provided environment variable; no new attack surface is opened.

Report-only contract maintained

SKILL.md states no --fix flag. I15's managed-policy case carries "never manufacture a diff to satisfy the table." The no-change representation for managed-policy and upstream-owned findings explicitly exempts those from the diff requirement. No automated write path exists on any inventoried surface. Output writes only to ${CLAUDE_PLUGIN_DATA}/audit-instructions/last-audit.md.


SUGGESTION (carry-forward from prior reviews) — ../../../../ relative path in conflict-criteria.md

Confidence: high · Not exploitable

plugins/claude-config/skills/audit-instructions/reference/conflict-criteria.md line 38:

[`plugins/claude-memory/skills/audit/reference/criteria.md`](../../../../claude-memory/skills/audit/reference/criteria.md)

Four directory traversals from reference/ toward a sibling plugin. In an installed plugin cache the path would not resolve across the plugin root boundary. This is a markdown hyperlink for human readers only, not a programmatic fetch target — no exfiltration path and nothing executable follows from it. It does not open a security vulnerability. However, it violates the repo's own plugin-form-safe rule ("No ../ reach-outs") documented in CLAUDE.md. Converting to prose or citing the sibling's public URL would close the design-rule gap. Unchanged since a prior security review round.

SUGGESTION (carry-forward from prior reviews) — unversioned raw.githubusercontent.com URL

Confidence: high · Not a current vulnerability

plugins/claude-config/skills/audit-permission-grants/reference/criteria.md line 9: https://raw.githubusercontent.com/melodic-software/claude-code-plugins/main/... pinned to main (mutable). Explicitly mitigated in the same file: "Each check's Recommend line below carries the fix in the form the report needs, so a run never depends on fetching the convention." Not a current vulnerability; risk materializes only if ever promoted to a programmatic fetch — at which point, pin to a commit SHA rather than main.


@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5d1f4fe105

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

Comment thread plugins/claude-config/skills/audit-instructions/SKILL.md Outdated
Comment thread plugins/claude-config/skills/audit-instructions/SKILL.md Outdated
Auto memory's enabled state was read from any single scope's false. It is
resolved by precedence: CLAUDE_CODE_DISABLE_AUTO_MEMORY is authoritative
wherever set (=1 off, =0 on even against autoMemoryEnabled: false), and with
it unset settings precedence decides, defaulting to on. Reading a lower-scope
false as decisive would drop a MEMORY.md a higher scope re-enabled.
/claude-memory:stateless owns that resolver.

The residency table also made every agent-definition-versus-MEMORY.md pair
guaranteed, but the main conversation's auto memory is not loaded into
subagents — only a fork inherits it. That pair now fails gate 1, while the two
real ones stay: a fork, and a subagent's own memory directory against the
definition it runs under.
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 2m 8s —— View job


PR Review — feat(claude-config): add the cross-surface instruction-conflict checks and an OPINION tier

  • Gather context and understand the PR
  • Read latest commit (5722cee) changes
  • Check whether open findings from last review pass are addressed
  • Evaluate new content for correctness
  • Post final review

New commit (5722cee) — closes both Codex P2s from the prior round

The commit addresses the two P2s Codex raised on 5d1f4fe and is clean on both.


Two Codex P2 findings closed since the prior review pass

Prior Codex finding Status
"Resolve auto-memory state using effective precedence" (SKILL.md) Closed. Phase A's auto-memory bullet now resolves by the canonical precedence order: CLAUDE_CODE_DISABLE_AUTO_MEMORY is authoritative wherever set (=1 off, =0 on, even against autoMemoryEnabled: false); with it unset, settings precedence (managed > local > project > user) decides, defaulting to on. The prior implementation read any single scope's false as decisive — a lower-scope false would have dropped a MEMORY.md that a higher-scope setting re-enabled. The bullet correctly delegates to /claude-memory:stateless as the canonical resolver and notes its status action surfaces disagreements.
"Exclude main auto memory from agent-definition pairs" (conflict-criteria.md) Closed. The residency table now marks MEMORY.md as "Every main session — not in a subagent, except a fork," sourced to the memory docs: "The main conversation's auto memory isn't loaded into subagents; the exception is a fork." conflict-criteria.md gains a dedicated paragraph explaining the exception, the guaranteed-pairs set is narrowed to exclude MEMORY.md from agent-definition pairings, and the two real cases are explicitly preserved: a fork (which inherits the parent) and a subagent that enables its own memory field (which contradicts the definition it runs under, but via the subagent's own directory). The CHANGELOG entry accurately narrates both causes and the correct disposition.

One finding remains open

Blanket @path expansion not restricted to memory-layer surfaces (criteria.md:352–354)

criteria.md lines 352–354:

"Resolve @path imports and symlinks to their targets before pairing, so an imported file is compared as part of the surface importing it rather than as a separate one."

This instruction has no surface restriction. I13 (lines 272–298) states explicitly: "Import syntax is a property of the CLAUDE.md family; on a skill or agent surface the @ is inert." If Phase B2 applies the blanket expansion to a skill body containing @reference/rules.md, it will pull in that file's content as if it were live instruction text — but it was never loaded. The result is a false I15 conflict against material no session sees.

This finding was raised in the review of 3714d36, carried through 5d1f4fe, and is unchanged in 5722ceecriteria.md was not touched by this commit.

The fix is a one-sentence qualification:

"Resolve @path imports for memory-layer surfaces (CLAUDE.md at every scope, CLAUDE.local.md, and .claude/rules/) before pairing; resolve symlinks everywhere. On skill bodies, agent definitions, hooks, and output styles, @path is inert — an @path reference there does not expand to additional comparison text."

Fix this →


Items confirmed no-issue

  • Auto-memory precedence: CLAUDE_CODE_DISABLE_AUTO_MEMORY resolution correctly mirrors the canonical resolver; the =0 override case (on, even against false in settings) is explicitly named, which is the edge the prior implementation missed.
  • Subagent auto-memory exclusion: The "two real pairs" preservation — fork and subagent's own memory field — is correctly scoped. A subagent's own memory directory is distinct from the main conversation's, so the exclusion does not sweep away real pairs that should still be reported.
  • Guaranteed-pairs update: The narrowed set (agent definition against any of {user CLAUDE.md, project CLAUDE.md, unscoped rules, MEMORY.md} except MEMORY.md) is correctly stated at conflict-criteria.md:144-146.
  • CHANGELOG narrative: Accurate. Both fixes are correctly described with their causes, and the "two pairs that remain real" are named.
  • All prior findings A–E and all prior Codex P2s except the @path item above: remain closed; no regression.
  • Merge-order note with fix(setup): close the setup-corpus audit's findings and two falsified reference claims #1315: acknowledged; resolution remains mechanical.
  • Security posture: no new concerns; this commit's changes are prose corrections to liveness rules, with no new read or execution surfaces.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 2m 14s —— View job


Security Review — PR #1316

  • Gather PR diff
  • Read changed files
  • Analyze for security vulnerabilities
  • Post findings

Scope: 14 changed files — two plugin.json version bumps, two CHANGELOG.md additions, one README.md update, SKILL.md for both audit-instructions and audit-permission-grants, two evals/evals.json additions, three reference/criteria.md expansions, one reference/conflict-criteria.md update, and one reference/official-guidance.md update. No workflow files, no scripts, no package manifests were touched. Prior security reviews covered commits through 5d1f4fe1; this pass focuses on the latest commit 5722cee.

No critical or important security vulnerabilities found.


New commit 5722cee — clean

The commit corrects two things:

  1. Auto-memory precedence resolverSKILL.md lines 131–148 now resolve the effective enabled state by precedence (CLAUDE_CODE_DISABLE_AUTO_MEMORY authoritative, then settings scopes highest-first, default on). No new read surfaces are opened; the change narrows when MEMORY.md is inventoried, not what is inventoried. Fixing a "false from any scope turns it off" bug makes the gate more correct, not more permissive.

  2. Agent-definition vs. main MEMORY.md pair removed from guaranteed pairsconflict-criteria.md lines 100–107 correctly document that main-conversation auto memory is not loaded into non-fork subagents. Removing this pair from the guaranteed-residency set reduces false positives; it introduces no new read path or write path.


Surface-by-surface checks (full PR)

Surface Finding
plugin.json (both plugins) Version bumps only — no new permissions, capabilities, or dependencies
CHANGELOG.md (both plugins) Documentation only
GitHub Actions No workflow files touched
Shell execution SKILL.md shell calls use bash "${CLAUDE_PLUGIN_ROOT}/..." with harness-provided env vars; instruction-scan.sh and conflict-scan.sh are referenced but not modified in this PR. The --opinion and --no-stopping-condition flags are prose-interpreted behavioral controls, not interpolated into shell strings
Credential / secret handling "Extract the prompt text only; never carry a command line, token, or other secret-bearing value" constraint applies uniformly; settings.local.json is "parsed for its permissions.allow array only — never read or echoed wholesale (it may hold tokens)"; prompt hook text enters the comparison set as the act it gates rather than as raw prose — correct
Supply chain No new action pins or package dependencies
Path traversal All execution paths use ${CLAUDE_PLUGIN_ROOT} and ${CLAUDE_PLUGIN_DATA} — plugin-scoped harness vars
Managed-policy reads Read-only for conflict comparison; criteria explicitly forbid any proposed edit to the managed-policy tier; no-change representation exempts those findings from the diff contract
Report output Writes only to ${CLAUDE_PLUGIN_DATA}/audit-instructions/last-audit.md — local-only, no exfiltration path

SUGGESTION (carry-forward from prior reviews) — ../../../../ relative path in conflict-criteria.md

Confidence: high · Not exploitable

plugins/claude-config/skills/audit-instructions/reference/conflict-criteria.md line 38:

[`plugins/claude-memory/skills/audit/reference/criteria.md`](../../../../claude-memory/skills/audit/reference/criteria.md)

Four directory traversals toward a sibling plugin. In an installed plugin cache this path would not resolve across the plugin root boundary. The target file is a documentation reference for human readers, not a programmatic fetch target — there is no execution path following this link. The finding violates the repo's own plugin-form-safe rule ("No ../ reach-outs" in CLAUDE.md) but carries no exploitable consequence. Converting to prose or a public URL would close the design-rule gap.

SUGGESTION (carry-forward from prior reviews) — unversioned raw.githubusercontent.com URL

Confidence: high · Not a current vulnerability

plugins/claude-config/skills/audit-permission-grants/reference/criteria.md line 9:

https://raw.githubusercontent.com/melodic-software/claude-code-plugins/main/docs/conventions/permission-rule-hygiene/README.md

Pinned to main (mutable). Explicitly mitigated in the same file: "Each check's Recommend line below carries the fix in the form the report needs, so a run never depends on fetching the convention." Not a current vulnerability — risk materializes only if ever promoted to a live programmatic fetch, at which point the URL should be pinned to a commit SHA rather than main.


Report-only contract maintained. SKILL.md states no --fix flag. I15's managed-policy case carries "never manufacture a diff to satisfy the table." The no-change representation for managed-policy and upstream-owned findings explicitly exempts those from the diff contract. No automated write path exists on any inventoried surface.

@kyle-sexton
kyle-sexton merged commit 049a4b9 into main Jul 26, 2026
27 checks passed
@kyle-sexton
kyle-sexton deleted the feat/audit-pass-criteria branch July 26, 2026 02:20

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5722cee390

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

Comment on lines +132 to +133
(the highest-precedence scope that sets `autoMemoryDirectory`, otherwise
`~/.claude/projects/<project>/memory/`). **Resolve the effective enabled state first, by

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Honor CLAUDE_CONFIG_DIR for the default memory store

When CLAUDE_CONFIG_DIR is set and no scope supplies autoMemoryDirectory, this fallback scans ~/.claude/projects/... even though the entire config tree, including projects/, moves under CLAUDE_CONFIG_DIR (the sibling resolver documents this in plugins/claude-memory/skills/stateless/scripts/scope-report.sh:46-48). The conflict corpus consequently misses the live MEMORY.md and may inspect stale memory under the old root; derive the default from the already-resolved config root.

Useful? React with 👍 / 👎.

Comment on lines +127 to +129
removes them from every dispatch rather than deferring them. Name a destination the agent itself
reaches — a skill the agent's definition invokes or preloads, or text kept in the definition — and
never a `paths:`-scoped rule.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Exclude preloads from load-deferring destinations

When the agent lists the destination skill in its skills: preload, the skill's full body is injected into every dispatch (reference/conflict-criteria.md:90), so the sometimes-relevant content remains present during every unrelated use of the general-purpose agent. That contradicts the new requirement at lines 110-112 that an I3 destination must actually defer loading; permit only conditional runtime invocation here, or report that no safe deferral is available.

Useful? React with 👍 / 👎.

Comment on lines +105 to +107
Two pairs remain real and should not be swept away with it: a **fork** does inherit the parent, and a
subagent that enables its own `memory` can contradict the definition it runs under, but that is the
subagent's own memory directory, not the main conversation's.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Add agent-specific memory to the comparison corpus

After correctly excluding the main conversation's MEMORY.md, this retains conflicts with memory enabled by an agent's memory field, but Phase A inventories only the main project's effective auto-memory entrypoint (SKILL.md:131-148) and never enumerates each agent's separate memory directory. An agent-definition-versus-own-memory conflict therefore cannot reach B2 despite being declared real here; inventory the loaded portion of each enabled agent memory read-only, subject to the effective auto-memory gate.

Useful? React with 👍 / 👎.

Comment on lines +154 to +157
- **Upstream-owned instruction text that is nonetheless live** — skill bodies and agent definitions
from the cache of an **enabled** plugin, `type: "prompt"` handler text in an enabled plugin's
`hooks/hooks.json` (a plugin is a supported hook location and `prompt` a supported handler type, so
that text is as live as a settings-configured hook), **the active output style when a plugin

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Apply hook disablement before inventorying plugin hooks

When the effective settings set disableAllHooks: true, an enabled plugin's prompt handler remains on disk but cannot run; plugin enablement alone therefore does not establish that its gate is live. Because the new liveness procedure resolves only the listed memory/session controls, B2 can report conflicts against a gate that is disabled for the session. Resolve effective hook enablement as well and omit disabled prompt handlers from the corpus.

Useful? React with 👍 / 👎.

kyle-sexton added a commit that referenced this pull request Aug 9, 2026
…instructions and setup (#2003)

## Summary

Discharges nine verifier-confirmed review findings against
`claude-config`'s `audit-instructions`
and `setup` skills, and bumps the plugin to `0.21.10` with a changelog
entry.

Seven of the nine are instruction-surface defects in
`audit-instructions` — a skill that audits other
people's instruction surfaces and was violating its own rules on three
of them. One is a scanner
false negative reproduced and fixed with tests. One is a prerequisite
the docs understated by two
skills.

## What changed, per finding

| # | Complaint | Discharge |
|---|---|---|
| 1 | The skill forbids hardcoding `~/.claude`, then hardcodes it |
`SKILL.md:45`, `:75`, `:197`, `:219-222` now resolve against the user
root Phase A establishes |
| 2 | I3 rejects `@path` imports as non-deferring, then names a
`skills:` preload as a valid destination | `criteria.md:196-201` strikes
the preload; only conditional runtime invocation qualifies |
| 3 | A subagent's own `memory` is graded real but never inventoried |
Inventory bullet at `SKILL.md:216-229`; co-residency row at
`conflict-criteria.md:98` |
| 4 | The liveness gate resolved a closed five-input list with no hook
enablement | `SKILL.md:175-185` and `conflict-criteria.md:270-292`
resolve `disableAllHooks` per scope plus `allowManagedHooksOnly` |
| 5 | A nested project memory pair routed to a check that cannot
discover the file | `SKILL.md:413-417` narrows the boundary to
**root-level** project; `.claude/rules/**` deliberately unchanged |
| 6 | `BOUNDARY_ERE` omits `and`, dropping a real conflict | `COORD_ERE`
/ `COORD_HEAD_ERE` at `conflict-scan.sh:131-143`, three new test cases |
| 7 | The requirements list names only `jq`/`curl` | `README.md:163-177`
and `setup/SKILL.md:28-33,46-52,106-108` name `awk`/`sort` across all
three skills that use them |
| 8 | I14's startup set omits `./.claude/CLAUDE.md` |
`criteria.md:612-621` covers both supported root locations |
| 9 | I14's supporting-document exemption ignores startup `@path`
imports | `criteria.md:636-642` resolves imports first, to four hops |

### Finding 6, reproduced

`conflict-scan.sh --count` on the finding's exact strings:

| Case | `origin/main` | this branch |
|---|---|---|
| "Always use `Read` and never use `Bash`" vs "Never use `Read`" | 0 |
**1** |
| "Always use `Read` but never use `Bash`" vs "Never use `Read`" | 1 | 1
|

A **bare** `and` boundary would also return 1 for the first row while
creating a new false negative:
"Never use `Bash` and `Grep`" is one directive over two objects, and
cutting at the coordinator strips
the `never` governing `Grep`. The boundary therefore requires a polarity
token after `and`, and is
consumed asymmetrically — a leading window resumes after the coordinator
alone so that token still
classifies its entity. Case 35 is the must-not-flag test and fails under
a bare-`and` boundary.

### Citations re-verified against the live docs

Every citation this batch introduced was diffed character-for-character
against the raw markdown of
`hooks`, `memory`, and `sub-agents` (fetched 2026-08-08). Three did not
survive and were corrected:

- **`disableAllHooks` has no documented "own settings level and below"
cascade.** The docs say
"Disable all hooks", with exactly one carve-out: set in user, project,
or local settings it cannot
reach managed hooks. The earlier wording invented a directional cascade.
Rewritten.
- **The `skills:` citation was a splice** of the frontmatter-table
sentence and the body sentence,
quotable as neither. Replaced with the real one: "The full content of
each listed skill is injected
  into the subagent's context at startup."
- **Imports recurse "with a maximum depth of four hops"** — the text
pointed at a "hop limit" the page
  never names. Now states the figure and quotes the wording.

Also corrected: the `AGENTS.md` import is *recommended* (a symlink is a
co-equal alternative, and the
import is mandated only on Windows), not prescribed; subagent memory
paths take upstream's
`<name-of-agent>` placeholder; and the auto-memory gate now names what
the subagent actually loses —
"the memory instructions or the memory tool access".

### Also fixed in passing

`conflict-scan.test.sh:378` had unescaped backticks inside a
double-quoted assertion message, so the
suite ran `and` as a command (`and: command not found` on stderr) and
printed the label with a hole in
it. Exit status stayed 0, so no gate saw it. Now single-quoted, matching
the file's existing idiom.

### Deliberately not changed

- `${CLAUDE_CONFIG_DIR:-~/.claude}` — that form *is* the correct
resolver, not a hardcode.
- `~/.claude` inside quoted upstream text (`criteria.md:651`,
`conflict-criteria.md:111`) — altering a
  quotation is a worse defect than the one it would resolve.
- `.claude/rules/**` routing to `claude-memory`'s C6 — C6's rules
discovery is recursive, so nested
  rules do not leak. Finding 5 is narrower than filed.

## Testing

- `conflict-scan.test.sh` — 46/46, no stderr noise
- All 7 `claude-config` plugin tests — PASS
- `check-changelog-parity.sh` `--check` / `--check-bump origin/main` /
`--check-order` — PASS
- `check-changed-skills.sh origin/main` — 4 skills, 0 failed
(`audit-instructions` 281/500 lines)
- `check-shell-portability.sh` / `check-skill-portability.sh` — PASS
- `shellcheck`, `markdownlint-cli2`, `typos` — clean
- `validate-plugins.sh`, `check-contract-slice-prune.sh`,
`check-contract-clause-coverage.py`,
`check-silent-skips.sh`, `check-cross-plugin-source-drift.sh`,
`check-skill-leaf-names.sh`,
  `check-orphaned-fixtures.sh` — PASS

## Related

Discharges review findings filed on #1316, #1343, and #1349.

No linked issue

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
kyle-sexton added a commit that referenced this pull request Aug 12, 2026
…ckRollup running-check pitfall (#2513)

## Summary

Lands the two PR-lifecycle facts #1436 recorded from driving
#1393/#1316/#1318/#1322 to merge, in the document that was prescribing
the expensive path:

- **Stale-branch recovery now defaults to merge-forward.**
`monitor.md`'s mergeable pre-check (§3.1 step 1) and stale-branch
recovery rule (§3.2) both prescribed "force-push with lease" — but
auto-mode permission classifiers commonly deny force-push, and the
recorded consequence was a fresh branch + fresh PR + closing the old one
per rebase (the #1315#1377#1393 churn, with every review thread
re-opened on the successor). Merging the default branch *into* the PR
branch resolves staleness and pushes **fast-forward** — no force-push —
and under a squash-only default branch the merge commits collapse to one
commit on merge, so linear-history requirements stay satisfied. Verified
in the issue's own record: #1393 landed that way and #1318 was
merge-forwarded five times without needing a new branch. Rebase stays
available as the exception for projects requiring a linear PR branch
where force-push is actually permitted.
- **`statusCheckRollup` reports a running check as `conclusion: ""`
(empty string), not `null`.** The complement-shaped filter (`conclusion
!= null and != "SUCCESS"`) therefore counts every in-progress check as a
failure — the exact misreport in the issue (two "failing" checks that
were simply still running). The multi-PR scan section (§3.0.6, the one
place this skill reads `statusCheckRollup`) now documents the pitfall
with value-positive jq selectors for "failed" and "still running".

Version `0.53.11` → `0.53.14` (patch; `0.53.12`/`0.53.13` are claimed by
in-flight PRs #2450/#2453/#2483/#2510 and #2469 — skipping past them per
the #1746 collision pattern).

## Test plan

- `npx markdownlint-cli2@0.23.2` on both edited markdown files — 0
issues.
- Docs-only change to skill reference text; no scripts or hooks touched.
The jq forms added are the ones from the issue, verified against `gh pr
view --json statusCheckRollup` semantics.

## Related

Fixes #1436

Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
cursor Bot pushed a commit that referenced this pull request Aug 12, 2026
…ckRollup running-check pitfall (#2513)

## Summary

Lands the two PR-lifecycle facts #1436 recorded from driving
#1393/#1316/#1318/#1322 to merge, in the document that was prescribing
the expensive path:

- **Stale-branch recovery now defaults to merge-forward.**
`monitor.md`'s mergeable pre-check (§3.1 step 1) and stale-branch
recovery rule (§3.2) both prescribed "force-push with lease" — but
auto-mode permission classifiers commonly deny force-push, and the
recorded consequence was a fresh branch + fresh PR + closing the old one
per rebase (the #1315#1377#1393 churn, with every review thread
re-opened on the successor). Merging the default branch *into* the PR
branch resolves staleness and pushes **fast-forward** — no force-push —
and under a squash-only default branch the merge commits collapse to one
commit on merge, so linear-history requirements stay satisfied. Verified
in the issue's own record: #1393 landed that way and #1318 was
merge-forwarded five times without needing a new branch. Rebase stays
available as the exception for projects requiring a linear PR branch
where force-push is actually permitted.
- **`statusCheckRollup` reports a running check as `conclusion: ""`
(empty string), not `null`.** The complement-shaped filter (`conclusion
!= null and != "SUCCESS"`) therefore counts every in-progress check as a
failure — the exact misreport in the issue (two "failing" checks that
were simply still running). The multi-PR scan section (§3.0.6, the one
place this skill reads `statusCheckRollup`) now documents the pitfall
with value-positive jq selectors for "failed" and "still running".

Version `0.53.11` → `0.53.14` (patch; `0.53.12`/`0.53.13` are claimed by
in-flight PRs #2450/#2453/#2483/#2510 and #2469 — skipping past them per
the #1746 collision pattern).

## Test plan

- `npx markdownlint-cli2@0.23.2` on both edited markdown files — 0
issues.
- Docs-only change to skill reference text; no scripts or hooks touched.
The jq forms added are the ones from the issue, verified against `gh pr
view --json statusCheckRollup` semantics.

## Related

Fixes #1436

Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant