Skip to content

fix(claude-config): fix audit findings in audit-instructions, unhobble and permission lint - #5311

Merged
kyle-sexton merged 23 commits into
mainfrom
fix/audit-claude-config
Sep 29, 2026
Merged

kyle-sexton merged 23 commits into
mainfrom
fix/audit-claude-config

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Refs: #3563
Refs: #3568
Refs: #4027
Refs: #4094
Refs: #4113
Refs: #4114
Refs: #4115
Refs: #4116
Refs: #4583
Refs: #4600
Refs: #4656

Summary

Fixes the plugins/claude-config findings from the audit of the unattended Cursor PR run. Every change is inside plugins/claude-config/; hooks/exec-bash.mjs is untouched.

Fix

In-place changelog corrections:

No issue is closed by this PR. #4094 stays on Refs because its 14-item checklist is not verified complete here, and the others are decision-held or have leftovers in other groups.

Verification

Run in /home/kyle/worktrees/ccp-fix-claude-config after merging origin/main (aebb9bb):

  • All 34 plugins/claude-config/**/*.test.sh suites: exit 0 (includes emit-findings.test.sh, finding-ids.test.sh, finding-identity.test.sh, permission-plane-lint.test.sh, permission-state.test.sh, audit-engine.test.sh, lane-runs.test.sh, instruction-files.test.sh).
  • bash scripts/check-detector-findings-crosswalk.sh --check: OK, 38 rule rows.
  • bash scripts/check-changed-skills.sh origin/main: 4 skills checked, 0 failed.
  • bash scripts/check-changelog-parity.sh --check --check-order, --check-bump origin/main, --check-preserved origin/main (190 headings compared): pass.
  • bash scripts/validate-plugins.sh: all manifests and the catalog validated.
  • shellcheck on every changed .sh file (the SC2016 hit on the emit-findings.test.sh rule fixture line is suppressed with a reason) and check-evals-quality.sh on the changed evals pass; check-skill.sh warnings that remain were present on main.

#5154 review outcome (#4027):

  • Claims checked: the defaultMode masking claim, the permissions key shapes the lint reads, and the auto-mode version boundary, each against Claude Code docs pages fetched 2026-09-29.
  • Evidence: the masking claim holds only under a stated condition, now carried in the text; the auto version boundary had no source and is removed; permission-state.sh reproduced the invalid-json misclassification when the last permissions key was a string.
  • Defect fixed: the per-key jq -e check counted only the last output, so C2-defaultMode and C5-disableType could not fire on real files. New tests in permission-state.test.sh fail against the old reader and pass now.

Related

Audit report: .work/audit/REPORT.md findings by issue number: #3563, #3568 (row 3c), #4027 (3b), #4094 (3b), #4113 (3b), #4114, #4115, #4116, #4583, #4600, #4656 (3c).

Cross-group requests (not done here):

Cross-group requests received and not applied here:

Issue operations done: #4027 and #4115 reopened with decision packets, packets on #3568 and #4027 extended, comments on #4113, #4116, #4656, #4583 and #3563.

🤖 Generated with Claude Code

https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB

kyle-sexton and others added 16 commits September 29, 2026 01:06
The I15 ledger link used six ../ segments and resolved outside the repo;
an installed plugin does not carry docs/, so name the path in code font.

Refs #3568

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB
…s and I32 tiers

I31 and I33 now admit any file inside a skill directory (I33 excludes
SKILL.md) plus context/, reference/, and references/ files, and I33 names
the nearest ancestor SKILL.md as its hub. I32 is CRITICAL under plugins/
and IMPORTANT on a user or project surface. The persist-admission text in
criteria.md and persist-findings.md names the scanner and lane intakes.

Refs #4116, #4656

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB
…g-ids

surface_of() accepted every file under $HOME as a user: surface, so a row
naming ~/.ssh/config or ~/.claude/.credentials.json got a finding id whose
anchor hashed a line of that file. Claude Code reads CLAUDE.md, CLAUDE.local.md
and AGENTS.md from the working directory and every directory above it, and
follows imports to absolute paths, so the user surface stays home-wide. It now
admits only instruction-file shapes: any markdown file, and settings.json,
settings.local.json or hooks.json inside a .claude tree or the resolved
CLAUDE_CONFIG_DIR. Anything else is refused as
surface-not-an-instruction-file. The scope carries its four-part record in the
code. relativize_in_repo in emit-findings.sh is unchanged.

Tests cover accepted ancestor, AGENTS.md, import-target, settings, plugin
hooks.json and relocated-config-dir shapes, refused non-instruction files, and
symlink chains inside home, from the repository into home, and to a
non-instruction target.

Refs #4116

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB
I31, I33 and I34 cover every file a skill loads, not only its markdown, so the
home-directory instruction shapes also admit any file beneath a skills/
directory inside a .claude tree or the resolved CLAUDE_CONFIG_DIR, plugin cache
included. A non-markdown file under a skills/ directory elsewhere in home stays
refused, as do credentials, transcripts and dotfiles.

Refs #4116

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB
…d, point at the owners

Refs #4113

The file restated the lane fraction and bytes per token that SKILL.md Lane
sizing owns, carried a stale "emits I28 and I29 today" persist paragraph,
and restated the finding tuple that reference/finding-identity.md owns.
After turning each of those into a pointer, the only content no other file
held was the I33 by plugin layout. That moves into SKILL.md Phase D, and the
file is deleted rather than kept as a second home. SKILL.md and eval case 25
now point at Lane sizing, Run files and resume, reference/finding-identity.md
and context/persist-findings.md.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB
…laim and the I33 dispatch rule

Drop ticket back-references from lane sizing and state the one partition rule. Give the
subagent-window claim a four-part record and say where the lane model window comes from and
what to pass when it does not resolve. Extend the read-only contract to the lane reports and
run-state writes, state that I33 rows sit outside the per-lane verifier batches and count as
one dispatch, assert it in eval 28, and restore line headroom.

Refs #4114, #4115, #4656

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB
…sk row

Refs #4600

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB
…dence

An empty strip ledger now licenses deleting an editorial candidate only. A
consequential rule the ledger did not defend goes back to a Deletion watch or
is restored; only a closed watch makes its removal permanent, matching the
attribution spec. The re-add grammar sentence now states the one-row versus
two-row threshold asymmetry as this skill's rule. The watch is named in the
mutating-step rail and the description triggers, the watch section says where
qualifying sessions run and how they are counted, and criteria.md points its
delete-and-watch sites at Deletion tiers. Eval 18 uses an unambiguous
non-protected rule; eval 22 covers the empty-ledger case.

Refs #3563

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB
…convention oracle test, readd refusal

Refs #4094

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB
… argument

status prints register holds, confounds and the PR URL with the source of each;
readd gets a by-hand ledger-grouping report against the two-row gate; decide is
documented as a presence-gated composition. Adds evals 27 to 30.

Refs #4094

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB
…ly permissions files

Refs #4027.

Independent review of the C2-defaultMode bypassPermissions change that reached
main in #5154 without review.

Defects fixed
- permission-state.sh classified a settings file invalid-json when the LAST key
  under `permissions` was a string, because `jq -e` reads only the last output of
  a per-key check. `{"permissions":{"defaultMode":"bypassPermissions"}}` was
  rejected and never scanned, so C2-defaultMode (and C5-disableType) could not
  fire on real files, only on hand-written records. The check now tests only the
  allow, ask, and deny lists. Tests cover the reader and the reader-into-lint path
  and fail against the old reader.
- Both C2-defaultMode findings now say to remove the value from the named file:
  an ignored project or local value still hides a user-scope defaultMode
  (auto falls to the built-in default, bypassPermissions to Manual).
- The "v2.1.142 and later" boundary for auto had no source on any current page
  and is removed from the finding, the code comment, and the criteria row, which
  now records the absence and where it was checked.
- The "acceptEdits, plan, and dontAsk still apply here" clause left the finding:
  the basis limits it to terminal sessions.

Claims checked, fetched 2026-09-29 as whole raw pages
- project and local settings ignore auto and bypassPermissions: settings-reference
  permissions.defaultMode Scope, permission-modes which-mode-a-session-starts-in.
- bypassPermissions became ignored in v2.1.257: settings-reference Scope
  ("Before v2.1.257, bypassPermissions took effect from any file") and the
  changelog 2.1.257 entry.
- the session starts in Manual: permission-modes, same section.
- user and managed settings read both values: permission-modes
  switch-permission-modes.
- other values apply from any file: permission-modes, same section.

Tests added
- plan, dontAsk, default, and manual in project scope raise no finding.
- bypassPermissions fires in local and startdir-local, not in user or managed.
- reader-into-lint over real files for the string-only shape.

No defect found in docs/conventions/permission-rule-hygiene/README.md or
babysit-prs/reference/safety.md; neither restates the project-scope claim beyond
what the pages support.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB
…shape check without history

Refs #4027.

The C2-defaultMode findings said an ignored project or local value makes the
session start in the built-in default or Manual. That holds only when no
higher-ranked settings file and no --permission-mode sets a mode ("settings
precedence decides"), so both findings and the criteria row now carry that
condition.

The criteria row keeps the as-of date and the absence record for the auto
version boundary; the lint and reader comments and the reader test state the
rule in present tense and point to the row. The reader test also asserts that
{"permissions":{}} is present.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB
…nhobble state

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB
…d where 0.51.7 shipped

Refs #4027

In-place changelog corrections:
- 0.51.18: body replaced by a pointer; it repeated the 0.51.16 Fixed entry (released by #5159) and the 0.51.10 Changed entry (released by #5161)
- 0.51.17: body replaced by a pointer; it repeated the 0.51.15 entry (released by #5156)
- 0.51.7: states it shipped through #5154 (commit 9c2db71), not #5059 (closed unmerged)
- File header: notes that 0.51.8 to 0.51.9 and 0.51.11 to 0.51.14 were reserved by parallel branches and never released

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB
Refs #4027, #4094, #4113, #4114, #4115, #4116, #3563, #3568, #4600, #4656

In-place changelog corrections:
- 0.51.18: body replaced by a pointer; it repeated the 0.51.16 Fixed entry (released by #5159) and the 0.51.10 Changed entry (released by #5161)
- 0.51.17: body replaced by a pointer; it repeated the 0.51.15 entry (released by #5156)
- 0.51.7: states it shipped through #5154 (commit 9c2db71), not #5059 (closed unmerged)
- File header: notes that 0.51.8 to 0.51.9 and 0.51.11 to 0.51.14 were reserved by parallel branches and never released

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB
@github-actions

github-actions Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

PR body contract — issue linkage

This PR body conforms to the issue-linkage contract. Nothing to do.

kyle-sexton and others added 2 commits September 29, 2026 11:15
…a without volatile detail

I21 and the audit effort-pin row named Opus 4.7 as the only non-high default and cited a
first-run hold the model-config page no longer carries; both follow the 2026-09-28 read of the
model-config and effort pages (Opus 5.5 and Sonnet 5.5 default to medium in Claude Code). The
unhobble state-write gotcha and its eval stop restating block-hook-bypass exit code and heredoc
coverage and defer to the hook.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB
@kyle-sexton
kyle-sexton marked this pull request as ready for review September 29, 2026 15:46
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Codex Review Summary

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

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-29T15:59:35.949850Z 5cb0e0d Draft marked ready
ℹ️ About Codex in GitHub

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

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

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

@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

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


Security review

Reviewed all 25 changed files (gh pr diff 5311, base origin/main). No security issues found.

No findings. This PR is documentation/skill-instructions and test-script maintenance for
plugins/claude-config; every .sh change touches a detector/lint script, not application logic
that handles untrusted input, credentials, or trust boundaries. Two changes are worth calling out
as security-positive, since they're exactly the kind of thing this lane watches for:

  • finding-ids.sh (skills/audit-instructions/scripts/finding-ids.sh, surface_of/instruction_shape) narrows what counts as a "surface" under $HOME. Previously any file under the home directory could be hashed into a finding anchor; now only instruction-file shapes (*.md, or settings.json/settings.local.json/hooks.json/skills/* inside a .claude tree or CLAUDE_CONFIG_DIR) qualify, and ~/.ssh/config, ~/.claude/.credentials.json, transcripts, and dotfiles are explicitly refused as surface-not-an-instruction-file. This reduces exposure to reading sensitive files, not the reverse.
  • permission-state.sh (classify_json_file) fixes a real detection gap: the old jq -e per-key check only kept the last key's type verdict, so a settings.json whose last permissions key was a string (e.g. {"permissions":{"defaultMode":"bypassPermissions"}}) was misclassified invalid-json and silently skipped — meaning C2-defaultMode/C5-disableType findings for a real bypassPermissions/disableAutoMode pin could go undetected. The new check validates allow/ask/deny explicitly regardless of key order, with tests that fail against the old reader.

Instruction-surface deletion lens applied: the diff removes/rewords several standing rules
(unhobble's "silence is a deletion warrant" language, execution-and-report.md's deletion,
SKILL.md wording trims). None weaken an enforced guardrail:

  • unhobble's ledger-silence rule is tightened, not loosened — an empty ledger now licenses deleting only an "editorial" candidate; a consequential rule the ledger didn't defend must go through a Deletion watch or be restored (skills/unhobble/SKILL.md, Phase 4 step 4 and "Deletion watch"). The "Security posture is out of scope" carve-out (policy-classified hooks kept, never stripped by default) is unchanged.
  • context/execution-and-report.md is deleted, but its two contracts (lane sizing/resume, report identity) are relocated into SKILL.md and reference/finding-identity.md, not dropped — content moves, nothing is unenforced.
  • The block-hook-bypass restatement removed from unhobble/SKILL.md's Gotchas is a restated-elsewhere case (the guardrails plugin's own hook and README still enforce/describe the block); no mechanism was weakened, only a redundant description of one.

Nothing in this PR touches hooks/exec-bash.mjs, credential handling, network calls, or GitHub
Actions workflow files.

@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

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


Claude finished the code review

  • Gather context (PR body, changed files, custom instructions)
  • Invoke /review:code-review per custom instructions
  • Post inline findings (none met the bar — see below)
  • Final summary

Review scope

This is the CI code-review lane (/review:code-review): correctness and maintainability findings only, scoped to files changed in this PR, high-signal bar (a careful senior reviewer would block/flag, with a concrete failure scenario).

I focused on the code-bearing diffs, since the bulk of this PR (roughly 750 of ~840 added lines) is prose rewording in SKILL.md/reference/*.md/README.md/CHANGELOG.md with no executable logic:

  • scripts/emit-findings.sh — the new hub_of(file, loc, ...) ancestor-SKILL.md walk and the widened in_rule_surfaces/rule_tier logic (I31/I33 surface widening, I32 tier-by-arm). Traced the base/h prefix arithmetic and the loop's empty-h exit condition; both check out for every path shape the new test Case 18 (emit-findings.test.sh) exercises, and I didn't find an input shape in real usage (relative, repo-rooted paths) that breaks it.
  • scripts/finding-ids.sh — the new instruction_shape() gate and its two-case surface_of/site_of refusal plumbing (surface-not-an-instruction-file vs surface-outside-repository-and-home). Consistent with the new Case 6b tests, including the symlink-resolves-before-shape-test cases.
  • scripts/permission-state.sh — verified the described defect is real and the fix is correct: the old to_entries[] | .value | type piped through as $kt produced one boolean per permissions key, and jq -e's exit status is keyed to the last emitted value, so a trailing string key (e.g. defaultMode) after an array-typed rule list masked the check. The new [.permissions.allow, .permissions.ask, .permissions.deny] | all(...) collapses this to a single boolean and is exercised by the new "trailing string key must not rescue a rule list of the wrong type" test.
  • scripts/permission-plane-lint.sh, scripts/audit-engine.sh (the $lane_note addition — confirmed in scope on all four call sites), scripts/instruction-files.sh (comment-only) — no issues.

I also cross-checked the rewritten prose in criteria.md, finding-identity.md, persist-findings.md, conflict-criteria.md, and the unhobble SKILL.md additions (oracle test, product-surface class, readd refusal gate, ledger grouping, decide) for internal consistency with the code-level changes and with each other; nothing contradicted.

Result

No findings meet this lane's bar. Nothing here is a correctness bug I could reproduce, and the two behavior changes with real logic risk (the permission-state.sh jq fix and the emit-findings.sh I31/I33/I32 relay widening) are both backed by new tests that fail against the prior behavior, which is what I'd otherwise be asking for.
· branch

@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: 5cb0e0dcdd

ℹ️ 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
kyle-sexton and others added 2 commits September 29, 2026 12:08
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…llback

permission-state.sh now marks a settings file invalid-json when
permissions.defaultMode is not a string, with a test. The disable* keys stay
with the C5-disableType lint, which reports a mistyped one. The audit-instructions
200000-token lane-window fallback carries its recheck trigger.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

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


Claude finished the code review

  • Gather context (PR body, changed files, custom instructions)
  • Invoke /review:code-review per custom instructions
  • Post inline findings
  • Final summary

Review scope

CI code-review lane (/review:code-review): correctness and maintainability only, scoped to files changed in this PR (git diff origin/main...HEAD), reviewed at HEAD 5be292f. Security is out of scope here (a separate security lane already ran on this PR).

The latest commit (5be292f) correctly addresses both prior Codex findings on this PR:

  • The defaultMode-type-validation P1/P2 items: permission-state.sh's classify_json_file now rejects a non-string defaultMode as invalid-json, with a new test ({"permissions":{"defaultMode":{},"allow":[...]}} → invalid-json) that reproduces the exact shape from the review comment. The disable* keys are deliberately left to the separate C5-disableType lint (verified: emit_file_conf still forwards their raw JSON type via conf records regardless of shape, so C5-disableType's conf[k] != "\"disable\"" check still fires on a mistyped value — this isn't a gap, it's a different, already-correct enforcement path).
  • The 200000-token lane-window fallback in audit-instructions/SKILL.md now carries the full four-part verification record (Claim/Basis/As of/Recheck), matching this repo's skill-bodies-state-current-rules convention.

Findings (posted inline)

  1. emit-findings.sh:331-340 (hub_of) — wrong I33 remediation target when the script runs outside the repo root. The new ancestor-walk hub search derives base by checking whether file (CWD-relative) ends with loc (repo-relative) — backwards. relativize_in_repo() actually builds loc = <git-prefix>/file, so loc ends with file, not the reverse, whenever the invocation CWD isn't the repo root. The guard then never matches, base falls back to "", and every ancestor probe is opened as a repo-root-relative path against a non-root CWD — the real hub SKILL.md is never found even when it exists, and the Action text falls back to "the surface that points at this file." Traced through a concrete example (running from plugins/claude-config/skills/audit-instructions/). None of the new tests catch this because every fixture cds to the repo root first, where file == loc trivially passes.
  2. emit-findings.sh:364-368 (in_rule_surfaces) — still under-admits I31/I33 per criteria.md's own surface text. criteria.md scopes both rules over "a file a memory surface points the model at to read," not only skills/<name>/... and context/reference/references directories, but the predicate only recognizes the latter. This PR's own Case 18 fixture (docs/notes.md) demonstrates the gap by asserting it gets declined rather than closing it. This was already flagged in a review comment on this PR (Codex, P2) and remains open at this HEAD — confirmed independently against criteria.md.

Everything else reviewed checked out: finding-ids.sh's new instruction_shape() gate (traced the .md-anywhere-under-$HOME vs. JSON/skills/-must-be-in-.claude-tree distinction against its own four-part record — consistent), the permission-plane-lint.sh wording update, and the audit-engine.sh $lane_note plumbing (all four call sites consistent, new Case 3b test exercises the right branch).
· branch

@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

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


Security review

  • Gather context (PR body, changed files, custom instructions)
  • Invoke /review:security-review and follow it exactly
  • Post findings
  • Final summary

Reviewed the full diff against origin/main (git diff origin/main...HEAD, 25 files, matches the PR's changed-file list) at HEAD 5be292f, scoped to plugins/claude-config/ per this lane's criteria: logic/trust-boundary flaws, injection surfaces, and the instruction-surface deletion lens (checked against docs/conventions/instruction-exception-register/README.md, which is present in this checkout).

1 finding, IMPORTANT, posted inline on permission-state.sh#L288:

The latest commit (5be292f) adds a defaultMode-must-be-a-string check to classify_json_file(). That's the right fix for the Codex P2 comment on the prior revision, but it has a side effect: a malformed-defaultMode file now classifies invalid-json, and emit_file_conf() — which emits the disableAutoMode config record permission-plane-lint.sh's C5-disableType check depends on — only runs when status is present (permission-state.sh#L390-L392). So a settings file with both a malformed defaultMode and a mistyped disableAutoMode (e.g. {"permissions":{"defaultMode":{}},"disableAutoMode":true}) now has its disableAutoMode misconfiguration silently dropped from the audit, where before this commit it was correctly caught — disableAutoMode:true is exactly the "operator believes auto mode is disabled and it is not" case the script's own comments call out as the worst-case lock-out failure. This also makes the commit's own comment and CHANGELOG line ("The disable* keys are left to the C5-disableType lint, which reports a mistyped one") false for this compound case; no test in permission-state.test.sh exercises both defects in one fixture.

Other things checked, no findings:

  • finding-ids.sh's narrowed instruction_shape()/surface_of() home-directory gate: narrows what's admitted (refuses ~/.ssh/config, credentials, dotfiles), no widening of exposure.
  • emit-findings.sh's widened I31/I33 surface admission and I32 tier-by-arm split: relay/reporting logic only, no injection or trust-boundary surface (the existing Codex P2 comment about under-admitting memory-linked files is an accuracy gap, not a security issue, so out of this lane's scope).
  • Instruction-surface deletions (execution-and-report.md removed, unhobble's "silence as deletion warrant" reworded, block-hook-bypass restatement trimmed): checked each against the instruction exception register's six Gate-0 classes (irreversible-action, secret-handling, data-integrity, external-publication, legal-compliance, agent-authority). None match — content is relocated (not dropped) or the rule is tightened, not loosened, and the security posture carve-out (policy-classified hooks never stripped by default) is unchanged.
  • No GitHub Actions files, no secrets/credential handling, no hooks/exec-bash.mjs changes (confirmed untouched, matching the PR description).
    · branch

Comment thread plugins/claude-config/skills/audit-permission-state/scripts/permission-state.sh Outdated
Comment thread plugins/claude-config/skills/audit-instructions/scripts/emit-findings.sh Outdated
kyle-sexton added a commit that referenced this pull request Sep 29, 2026
…suites (#5316)

Closes #3599

## Summary

Fixes from the 2026-09-29 audit of the Cursor agent's PRs that touch CI
(group "ci"), one commit per change, all under `.github/`,
`.gitleaksignore` and `scripts/gitleaks-*`:

- Gitleaks: the local scoped-scan fork replaced the shared composite.
The composite is re-pinned to v0.30.1 and the fork and its test are
deleted (#3599).
- Windows lane: the kindle-dedrm Pester step can fail; the exec-form
launcher tests and two of the three remaining #3683 host-skip suites
(markdown-format, cloud-bootstrap-plugins) now run on Windows (#3703,
#3686, #3683). The third, audit_skill_visibility, fails on windows-2025
for a cause outside `.github/` and is dropped (#5323).
- ci.yml: the UNMAPPED fallback runs the outside-node suites, the
prerequisite-probe suite is wired, and the shell filter is widened
(#3703, #4240, #4139).
- Workflow comments point at their trackers instead of restating pins
and remedies (#4130, #4670, #4144, #5323), and the gitleaks comment no
longer implies a red dispatch.
- test-linux installs hash-locked numpy and opencv so the animation
suites run instead of skipping (#4594).
- The silent-revert canary calls the shared base-ref predicate instead
of its own copy (#3413).

## Fix

- `aa6a6230d` `lint` job:
`melodic-software/ci-workflows/.github/actions/gitleaks@35880dcb`
(v0.30.1) with `scan-mode: git` and `log-opts` unset. A pull request
scans its own commits (`<base>..HEAD`), a push scans main's full
history, a dispatch scans every ref present in the clone. Only this step
moves; the other composites stay at v0.27.1. `.gitleaksignore` keeps the
two `04f4bcd5` fingerprints (reachable from main) and drops the five for
commits no origin ref reaches.
- `fafdab6af`, `bf21ed285`, `b748b5dd4`, `b2b689b52` `test-windows.yml`:
the kindle-dedrm step fails the lane on a failing test; the launcher
steps and the markdown-format and cloud-bootstrap suites run on
`windows-2025`; the skill-visibility step added by `b748b5dd4` is
dropped by `b2b689b52`.
- `430e0d953` `ci.yml`: outside-node runner on the UNMAPPED fallback,
`check-prerequisite-probes.test.sh` as a hard-failing `test-linux` step,
wider shell filter.
- `747b59154` comment-only changes in workflows.
- `c2e18d24a` `.github/requirements-ci-animation.txt` (hash-locked)
installed with `requirements-ci.txt` in one `--require-hashes` command,
plus `ANIMATION_REQUIRE_DEPS=1`. The install stays (measured below).
- `85ee97b36` comment-only: the gitleaks step comment says a manual
dispatch scans every ref present in the clone; the `test-windows.yml`
comment names #5323.
- `dee4d13fa` `silent-revert-canary.yml`: "Resolve the pushed range"
sources `scripts/lib/changed-files.sh` and calls
`changed_files::verify_base` in place of the inline `git rev-parse
--verify --quiet "<ref>^{commit}"`. The fallback (warn, scan the head
commit) is unchanged. Applies the scripts group's request (c).
- `2a619570a` `ci.yml`: the "Install locked plugin test toolchains" step
diffs the `name==version` pins of `plugins/animation/requirements.txt`
against `.github/requirements-ci-animation.txt` and fails when they
differ (Codex review: Dependabot's `/.github` entry would bump only the
CI copy).
- Requests received from other groups: the animation, architecture,
scripts (a, b) and claude-config requests were already met by the
commits above or by issue operations; playbooks became a decision packet
and hook-launcher was not triggered. Each outcome is under Related.

## Verification

Static checks, on head `85ee97b36`:

- `git merge origin/main` (earlier, into `5ca5fe296`): clean, no
conflicts, none of its commits in the group's paths.
- `actionlint` and online `zizmor --persona=regular`: no findings.
- `bash scripts/check-lane-coverage.sh --check`: all 5 lanes reachable
from `ci-status.needs`, 66 gate steps fed to the aggregator, 2 opted
out.
- `bash scripts/check-lane-coverage.test.sh`: PASS=40 FAIL=0.
- `bash scripts/check-docs-only-gate.sh --check`: passes.
- `bash scripts/check-changelog-parity.sh --check --check-order`:
passes.
- `bash scripts/validate-plugins.sh`: all manifests and the catalog
validated. No plugin files changed, so no version bumps or changelog
entries.

On head `dee4d13fa`, which changes only `silent-revert-canary.yml`,
re-run: `actionlint` over every workflow, `zizmor --offline
--persona=regular` on the canary, `check-lane-coverage.sh --check` (66
gate steps fed, 2 opted out) and `check-shell-portability.sh
origin/main`: all clean. The checks above were not re-run; the commit
changes no other file.

On head `2a619570a`, which changes only `ci.yml` (the pin check): equal
pins pass and a one-sided `numpy` edit fails when the step's diff is run
locally; `actionlint`, `zizmor --offline --persona=regular`,
`check-lane-coverage.sh --check` (66 gate steps fed, 2 opted out),
`check-lane-coverage.test.sh` (PASS=40), `check-docs-only-gate.sh
--check` and `check-shell-portability.sh origin/main` are clean.

The migrated step body was run in a scratch repository against five
`before` values. A valid parent sha took range mode with no warning; 40
zeros, an empty value, an unknown sha and a non-ref each took commit
mode with one warning. A dispatch of the canary on this branch,
[36589890903](https://github.com/melodic-software/claude-code-plugins/actions/runs/36589890903)
(head `dee4d13fa`), succeeded in every step; its empty `before` takes
the commit-mode branch, so it proves the `source` line works on the
runner and the scratch run covers the rest.

#3599 acceptance criterion 1 (hygiene passes on a PR that does not touch
`resolve-convention-home.sh`): met. This PR's own `lint` run,
[36579146249](https://github.com/melodic-software/claude-code-plugins/actions/runs/36579146249)
(head `5ca5fe296`, `pull_request`): "Scan for secrets" success, 8
commits scanned (`<base>..HEAD`), no leaks found. The run on the newest
head `85ee97b36`,
[36582778889](https://github.com/melodic-software/claude-code-plugins/actions/runs/36582778889),
is green (`ci-status` success) and its scan of 20 commits found no
leaks. The `ci` dispatch below, whose `--all` scan ran in a CI clone,
scanned 3192 commits and also found no leaks, so a manual dispatch does
not go red on other refs. The 8 findings a local all-refs scan reports
come from local-only `refs/remotes/pr/*` refs that a CI clone does not
hold (correction posted on #3599).

Windows lane, two dispatches of `test-windows.yml` on this branch (the
only run of these steps on a real Windows host):

-
[36528002813](https://github.com/melodic-software/claude-code-plugins/actions/runs/36528002813),
red. Step "Test the skill-visibility audit on Windows" failed with 2
failures, both `AssertionError: 'unreadable' != 'read'`; the record was
`bash exited 1 sourcing the lib: no stderr` for
`plugins/claude-ops/lib/managed-scope.sh`. The cause is in a claude-ops
script, not in `.github/`, so the step was dropped (`b2b689b52`) instead
of committed red, and the defect is filed as #5323 (before this it lived
only in the comment at `test-windows.yml:205`).
-
[36529242934](https://github.com/melodic-software/claude-code-plugins/actions/runs/36529242934),
green, about 10.5 minutes (06:04:46 to 06:15:19) against
`timeout-minutes: 20`, which stays unchanged. Per step:
- kindle-dedrm Pester (`Run.Exit` set): 11 passed, 0 failed, 0 skipped.
- markdown-format: PASS=168 FAIL=0 SKIPPED=4. The host probe fired: four
counted `SKIP (host: cygpath rewrites ...)` lines, plus the uncounted
symlink-escape skip.
- cloud-bootstrap-plugins: PASS=79 FAIL=0, no probe skip; the stubbed
`claude` ran on Git Bash.
- exec-form launcher, resolver test (`node --test
lib/exec-bash.resolver.test.mjs`): 1 pass, 0 fail.
- exec-form launcher, `plugins/guardrails/hooks/exec-bash.test.sh`:
passed (stdin, exit 2, Git Bash resolution, hooks.json shape, 8
dispatcher rows).

`ci` dispatch
[36577675603](https://github.com/melodic-software/claude-code-plugins/actions/runs/36577675603)
(head `c2e18d24a`), which measures the animation install and the
gitleaks step:

- `lint` success; "Scan for secrets" scanned 3192 commits, no leaks
found.
- `test-linux` legs 0, 1 and 3 green. Leg 2 red only on
`plugins/planning/tests/interview-defenses.test.sh` (`SKILL.md
frontmatter is unchanged` digest check). That is outside this group's
paths and the suite failed the same way at the branch's base; a snapshot
of origin/main `9ef496019` passes it (PASS=154 FAIL=0), so the next base
merge clears it.
- `Install locked plugin test toolchains`: 11 to 14 s per leg with the
animation wheels, against 6 to 8 s per leg on today's main push runs
36580419086 and 36580578535 (one 15 s sample). Leg wall time: 162 to 281
s against 141 to 288 s on those runs, so the install adds about 5 s and
the wall-time spread is main's own run-to-run spread. No leg is more
than 60 s slower, so the install stays, and the animation group's
`ANIMATION_REQUIRE_DEPS=1` request stands.

## Related

- Audit report: `.work/audit/REPORT.md` findings for #3599, #3703,
#3686, #3683, #4240, #4139, #4130, #4670, #4144, #4594.
- Refs #3703, #3686, #3683, #4240, #4139, #4130, #4670, #4144, #4594,
#5323, #3413 (the owning groups finish those).
- Decision packet for the owner: #5333 (a PR body that says the
merge-hold phrase holds nothing; options A to D, nothing implemented).
- Requests received from other groups, checked against origin/main
`f2f421f7b`:
- playbooks (a merge-hold guard for the #5154 failure): not implemented,
decision packet #5333. The label is the convention's only hold, the
reading step is in the ci-workflows composite, a `GITHUB_TOKEN` label
does not re-run `ci-status`, and the phrase appears in 63 PR bodies (22
merged). The lane half went to source-control.
- hook-launcher (test-windows steps after the probe): not triggered. No
claude-ops re-land PR is open; the YAML-reader and probe steps are
byte-identical to origin/main and the launcher steps here follow them.
- architecture (five dead `.gitleaksignore` lines): already applied in
`aa6a6230d`; both commits are unreachable from origin/main.
- tracker (#4670): already applied. #4670 is reopened with operator
steps; the ci-workflows issue is operator step 3, so none was filed.
- animation (hash-locked numpy and opencv, `ANIMATION_REQUIRE_DEPS=1`):
already applied in `c2e18d24a`, measured above.
- scripts (a) the prerequisite-probe suite: already applied in
`430e0d953` as a hard-failing step like its four `scripts/lib`
neighbours; without `continue-on-error` the lane-coverage gate would
report an unfed gate, and it passes. (b) #3413: already commented,
follow-up #5321. (c) the canary predicate: applied in `dee4d13fa`.
- claude-config (#4094): already applied. #4094 is open and its comment
names #5311 (`Refs: #4094`, unmerged draft) for items 5, 6, 8, 9 and 10
to 12.
- Cross-group requests sent: core-docs (docs/ci-runner-routing.md
time-to-green job list and contract-only remedy wording), conventions
(token-leak-amplification suite, two rationale edits), scripts
(gate-entry.test.sh guard, verb-contract gate test, then a ci.yml
self-test step in a second pass), guardrails (#4527; #3683: this PR adds
two of the three remaining host-skip suites, markdown-format and
cloud-bootstrap-plugins, so read their Windows results above;
audit_skill_visibility did not land), claude-ops (#5323:
managed-scope.sh under Git Bash on windows-2025, plus restoring the
dropped test-windows step when fixed), hook-launcher (#3708 and
backslash-root case), animation (`ANIMATION_REQUIRE_DEPS` must exit 1
instead of skipping), playbooks (hygiene.md residue-dissolve note),
claude-config (#4094).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…eAutoMode, find the I33 hub from a subdirectory

Revert the defaultMode type gate in permission-state.sh: it made a file with a
malformed defaultMode invalid-json, which skipped emit_file_conf and dropped the
mistyped disableAutoMode the C5-disableType lint reads. A test now covers both
defects in one file.

hub_of() in emit-findings.sh probed repo-relative paths from the caller's
directory, so a relative path from a subdirectory never found the hub. It now
probes from the repository root, with a test that fails without the change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

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


Security review

  • Gather context (PR body, changed files, custom instructions)
  • Invoke /review:security-review and follow it exactly
  • Post findings
  • Final summary

No findings. This is a re-review at HEAD ea8e674 after two follow-up commits (5be292f, ea8e674) that resolve both prior findings from this lane and the code-review lane on this PR:

  1. disableAutoMode masking (the IMPORTANT finding posted earlier on this PR) — permission-state.sh#L275-L285. Commit 5be292f had added a defaultMode-must-be-a-string gate to classify_json_file(), which caused a file with a malformed defaultMode to classify invalid-json — skipping emit_file_conf() entirely and silently dropping the file's disableAutoMode conf record that permission-plane-lint.sh's C5-disableType check depends on (the exact lock-out scenario: {"permissions":{"defaultMode":{}},"disableAutoMode":true} would no longer surface the mistyped lock-out switch). Commit ea8e674 reverts the defaultMode type gate, restoring the file's present classification so emit_file_conf always runs. Verified against the new test in permission-state.test.sh#L168-L175, which fixtures both defects in one file and asserts the scope stays present and disableAutoMode true is still read. Traced the jq filter directly — confirmed only allow/ask/deny are checked now, defaultMode's shape no longer gates classification.

  2. hub_of() wrong-directory bug (correctness/audit-integrity finding posted earlier on this PR) — emit-findings.sh#L331-L334. The base fallback (when file doesn't end with loc, i.e. the script runs from a repo subdirectory) now uses the absolute repo_root_pwd/repo_root instead of "". Traced REPO_ROOT/REPO_ROOT_PWD (lines 222-236): both are always absolute (git rev-parse --show-toplevel, or a pwd resolved from it), so every ancestor SKILL.md probe in the walk is now an absolute path regardless of invocation CWD. Confirmed by the new subdirectory-invocation test in emit-findings.test.sh#L898-L902.

No new security-relevant issues in the rest of the diff (plugins/claude-config/ only, hooks/exec-bash.mjs untouched). Re-checked the full changed-file set against this lane's criteria (injection surfaces, trust-boundary/authz logic, credential handling, instruction-surface deletions against the instruction exception register) — consistent with the earlier passes on this PR (5cb0e0d, 5be292f), which is unchanged for the ~20 files this commit didn't touch.

One remaining open item, not a security finding: in_rule_surfaces() in emit-findings.sh still under-admits some I31/I33 memory-linked files per criteria.md's own surface text (flagged by Codex and the code-review lane, acknowledged by the author as a scoped-out audit-completeness gap, not addressed in this commit). Under-admission only omits a persisted repair proposal — it doesn't emit an incorrect one or widen any exposure, and no standing guardrail is removed by it, so it's out of this lane's scope per its own criteria (security issues only).
· branch

@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

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


Claude finished the code review

  • Gather context (PR body, changed files, custom instructions)
  • Invoke /review:code-review per custom instructions
  • Post inline findings (none — see below)
  • Final summary

Review scope

CI code-review lane (/review:code-review), triggered by synchronize. HEAD is ea8e674, one commit ahead of the last-reviewed 5be292f. That commit (ea8e674) is scoped entirely to fixing the two open findings from this lane's prior review round on this PR, so I focused verification there rather than re-reviewing the whole diff (already covered in prior rounds).

Verified fixes

  1. permission-state.sh disableAutoMode masking (IMPORTANT, previously flagged) — fixed correctly. The commit reverts the defaultMode-type gate added in 5be292f entirely, so a malformed defaultMode (e.g. an object) no longer makes the file classify invalid-json; the file stays present, emit_file_conf still runs, and a mistyped disableAutoMode beside it is still emitted as a conf record for C5-disableType to catch. Traced permission-plane-lint.sh's C2-defaultMode check (lines 197-205): it only string-compares conf[k] against "\"auto\""/"\"bypassPermissions\"", so a non-string defaultMode (now serialized via tojson, e.g. "{}") neither crashes nor produces a false positive — it's simply not flagged by C2-defaultMode, which is the accepted trade-off from this thread. New test (permission-state.test.sh) covers {"permissions":{"defaultMode":{}},"disableAutoMode":true} → present + conf user settings disableAutoMode true, which reproduces the exact compound case from the prior finding.

  2. emit-findings.sh hub_of() backwards subdirectory check (previously flagged) — fixed correctly. Traced the new fallback: when file (CWD-relative) doesn't end with loc (repo-relative) — i.e., the invocation CWD isn't the repo root — base now falls back to repo_root_pwd (or repo_root) plus /, an absolute path, instead of the empty string it fell back to before. Walked the concrete failure case from the original finding (running from plugins/claude-config/skills/audit-instructions/ with file=scripts/finding-ids.sh, loc=plugins/claude-config/skills/audit-instructions/scripts/finding-ids.sh): base now resolves to the absolute repo root, and the ancestor probe correctly opens <repo_root>/plugins/claude-config/skills/audit-instructions/SKILL.md via getline, regardless of CWD. The new emit-findings.test.sh case (cd "$SURFREPO/$SURFSKILL" then --from-lane with a relative path) exercises exactly this scenario.

Both fixes are consistent with the existing pattern elsewhere in the file (source_line() already opens file relative to the real CWD; relativize_in_repo() already prefers repo_root_pwd over repo_root for the symlink case), so nothing here is a one-off.

Not re-flagged

The third open comment (in_rule_surfaces() under-admitting memory-linked files outside skills//context/reference/references shapes) was addressed with reasoning by the author (no code change): under-admission only omits a persisted repair proposal — it's counted and visible as outside-rule-surfaces, never silently dropped, and never emits a wrong remediation. That's a scope/interface decision, not a correctness bug this lane's bar would block on, so I'm not re-raising it.

Not verified

I could not execute the test suites in this environment (bash tool execution was denied by the sandbox), so the above is verified by code trace only, not by running permission-state.test.sh / emit-findings.test.sh. The PR description's own Verification section (34 suites passing) is author-claimed and not independently confirmed by me.

No findings.
· branch

@kyle-sexton
kyle-sexton merged commit 3184ee4 into main Sep 29, 2026
19 checks passed
@kyle-sexton
kyle-sexton deleted the fix/audit-claude-config branch September 29, 2026 17:06
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