fix(claude-config): report-only plugin drift posture with a dependency carve-out and CR-safe keys

[kyle-sexton]

No related issue: closes handoff items 20260924-110000 and 20260925-080000; the large remainder (D1-D8 and two engine gate defects) is tracked in #4514.

Summary

The claude-config audit stops asking operators to record an explicit true or false for every
catalog plugin, and stops writing false keys itself. The two drift scripts also carry plugin and
marketplace keys as JSON from end to end, so a key that ends in a carriage return is no longer lost.
The operator approved the posture on 2026-09-25: Q3 and Q5 as vetted, and the Q4 dependency
carve-out reading false keys from all three scopes. Release 0.49.0.

Fix

• Q3. audit-engine.sh no longer emits the drift-new finding. Instead it emits one ok
  inventory row per marketplace, counting catalog plugins that have no enabledPlugins entry in
  any scope.
• Q4. The disabled-plugin finding is now an ok inventory row for every false key in the
  user, project and local files, and the row says when a higher-scope true shadows the key. One
  case stays a finding: a false that wins the local > project > user merge and that an enabled
  plugin declares as a direct dependency. That is a warning finding, dependency-disabled:<key>.
  Dependencies are read from each enabled plugin's .claude-plugin/plugin.json. The manifest is
  optional. An unresolved install path, an invalid manifest, or a non-array dependencies produces
  a not-inspectable row.
• Q5 and Q8. fix-plugin-drift.sh --yes never adds a key, and NEW upstream plugins are listed
  for reporting only. An orphan removal proceeds only when the key's live value is exactly false.
  It moves to manual review when a lower-precedence scope file holds true, or when that file
  cannot be read. The plan says that other developers' scopes and managed settings were not
  checked.
• Q2 and defect 4 (coverage). check-plugin-drift prints a Not diffed: line listing the keys
  whose marketplace the audited file does not declare. The engine reports the same gap as an
  E/drift skip row.
• CR-safe keys. Keys are carried as JSON end to end, and no tr -d '\r' or jq -R touches a
  key. The engine compares keys in jq, and it runs jq with MSYS argument conversion disabled.
• Exit code. check-plugin-drift exits 1 only when it finds an orphan or a rename.
• Backup. Each backup is a new mktemp name, <settings>.bak.<stamp>.<random>. No existing
  path is written through, which closes the non-regular-file window.
• Test isolation. The test suites derive a POSIX TMPDIR, guard their temp directories, and
  point HOME and CLAUDE_CONFIG_DIR at fixtures.
• consentReceipt. Keys that Claude Code or a plugin writes as consent receipts are recorded in
  skills/audit/reference/consent-receipts.json, shaped {"consentReceipt": {"<owner>": [records]}}.
  • The owner is either a plugin@marketplace id or the reserved id claude-code.
  • Each record gives the key, the scopes it applies to, what it means, and a four-part record.
    The first entry is claude-code -> skipWorkflowUsageWarning, scoped to the user file.
  • A listed key that the docs do not cover gets an ok row, A/consent-receipt, in place of the
    undocumented-key finding, but only when all of these hold:
    • the key is a top-level key;
    • it sits in a scope the record declares;
    • a plugin owner is enabled, or for claude-code, the binary search did not find the name
      missing.
  • Otherwise the finding stays and says why. A binary search that finds the name missing reads
    "stale consent receipt".
  • A missing or invalid record file produces one not-inspectable row. A key that is ambiguous
    because a carriage-return twin is present is not labeled. A record whose fields contain a
    control character makes the file invalid.
  • An existing .claude/audit-pass.md suppression of undocumented-key:<key> stops matching once
    that key is labeled. CHANGELOG says so.
• Wording. The Q15 sweep covers SKILL.md, context/, evals, README and setup. The dependency
  claims carry a four-part record.

Verification

These results were measured on Windows 11 with Git Bash and native jq 1.8.2.

• Reproduced on base 47555b5 before the fix:
  • A directory-source marketplace keyed mk\r was reported as mk (no-repo) SKIP.
  • TMPDIR='C:/...' bash fix-plugin-drift.test.sh failed 6 of 182 checks, in cases 29 and 30.
• Red first: the new tests failed against the pre-change scripts:
  • check suite: 16 failures
  • fix suite: 182 pass, 73 fail
  • engine: 229 pass, 31 fail
• Green on head (check and engine re-run on 5d050a6 after the last wording commit):
  • check-plugin-drift.test.sh: 96/96, and the same with a C:/ TMPDIR.
  • fix-plugin-drift.test.sh: 271 pass, 0 fail, 1 skip, both with and without a C:/ TMPDIR. The skip is case 40: chmod cannot clear a directory's search bit on this host.
  • audit-engine.test.sh: 270/270.
  • check-doc-citations.test.sh: 23/23.
  • shellcheck is clean on every touched script.
  • check-changelog-parity.sh --check-bump 47555b587 passes.
  • The added lines contain no U+2014.
  • check-changed-skills.sh fails the audit skill only on check-hook-coverage.test.sh (6/93). That suite fails the same 6 checks on untouched base on this host, and this PR does not touch it.
• Fresh-context verifier (rationale withheld): AC1 to AC7, AC9 and AC10 each PASS. It rebuilt the original symptoms with its own fixtures: the CR marketplace is now audited, the CR key alone is removed, and --yes adds no key.
• Reviews: a code reviewer and a security reviewer found 15 items between them, and the review-fix commit 9b7cb82 fixed all of them. A simplification and prose pass followed. Its wording items were applied in 5d050a6. Its behavior-neutral code simplifications were declined, so that verified code is not reshaped this late.
• Review-thread fixes (5a80ec2):
  • The backup is now written on an exclusive open.
  • Citation slugs are validated before any path is built from them.
  • check-doc-citations.test.sh: 35/35. Its new case 9 failed 12 checks before the fix.
  • fix-plugin-drift.test.sh: 277 pass, 0 fail, 4 skip, both with and without a C:/ TMPDIR. The skips are symlink and mode-bit variants that this host cannot run.
  • CI is green on 5a80ec2.
• consentReceipt (2183f96, f0c801b):
  • Engine case 46 failed 10 of 18 checks on the old engine; the 3 new security-fix checks were
    also red first.
  • audit-engine.test.sh: 294/294 on f0c801b.
  • shellcheck, the portability check and changelog parity all pass.
  • A fresh-context security review of the delta passed AC8.
• Merge-gate fixes (11a9ddf, 0af73ff):
  • A record file that lists one key under two owners is invalid, and a stale claude-code
    verdict is terminal. The new checks in cases 46 and 47 failed 5 of 31 on f0c801b.
  • Every drift and engine suite now clears or overrides every inherited settings-path variable
    before its first case. Every --yes run in the fix suite goes through guard_apply, which
    aborts the suite for any path outside its temp dir.
  • A decoy settings file exported as CLAUDE_SETTINGS_FILE was byte-identical after both drift
    suites ran.
  • Suite counts:
    • audit-engine.test.sh: 301/301.
    • fix-plugin-drift.test.sh: 287 pass, 0 fail, 4 skip, both with and without a C:/ TMPDIR.
    • check-plugin-drift.test.sh: 96/96.
• CI green before 5a80ec2: 9953293 fixed a GNU-only \b in a test, the one CI lint failure on 5d050a6.
• skill-evidence block absent: the installed source-control and claude-ops predate feat(source-control): seat the mandatory reviews on the operator's session and retire the OAuth review lanes #4210, so no ledger sha is stamped; the owed reviews ran as nested agents: code reviewer, security reviewer, simplification and prose (AI-slop and markdown-noise) pass, skill check script (check-changed-skills.sh), fresh-context verifier.

Related

• Closes handoff items 20260924-110000 (derive-from-docs remainder after fix(claude-config): derive audit criteria from fetched upstream docs #4419) and 20260925-080000
  (fix-plugin-drift remainder after fix(claude-config): fix-plugin-drift reports skipped runs and refuses stale plans and concurrent writes #4490). claude-config audit: derive the remaining criteria from docs (D1-D8) and two engine gate defects #4514 carries D1-D8, sr_section rescans, and the
  U+0000 key split. It also carries stale "skips directory sources" text in docs/cloud-sessions.md
  and scripts/check-plugin-catalog-enablement.sh, which sit outside this plugin.
• Unattended interview. Open questions took the item's or the operator's default. An answer with
  no default was challenged by one fresh-context validator with the rationale withheld, and its
  verdict was adopted:
  • Defect 4 acknowledgement (OQ1): CONFIRMED with conditions, adopted. The census keeps its
    narrow scope, and the coverage line replaces a machine-wide census. The coverage row names the
    local file's keys too. The row never double-reports keys whose marketplace is unregistered.
  • consentReceipt (OQ2): built as reading 2, a receipt record per plugin@marketplace. The
    operator wrote "consent-receipt key named consentReceipt, keyed by plugin id".
    • The first validator challenged the check-slug reading, which added nothing.
    • The loop coordinator then ruled the question a design choice rather than user-reserved, and
      chose reading 2.
    • A second fresh-context validator challenged the proposed shape, and its verdict was adopted
      in full:
      • consentReceipt becomes the JSON key.
      • A claude-code receipt must not bypass the binary tie-breaker, so an absent name is
        reported as stale.
      • Each record declares its scopes, because one developer's consent must not label a key in
        the shared project file.
      • Matching is by top-level pointer.
      • There is no consumer extension surface, because .claude/audit-pass.md already accepts
        undocumented keys.
    • The reserved owner is claude-code, with no @, so it can never collide with a real plugin
      id.
    • A security review of the delta passed AC8. Its two low and info items (a carriage-return twin
      key, and control characters in record fields) were fixed. Its third item, disclosure of the
      pre-existing SETTINGS_AUDIT_ENGINE_* test overrides, was declined as out of scope: that
      class predates this PR, and the new override adds no reach.
  • Q8 guard (OQ4): CHALLENGED, adopted in the validator's second form. A lower-precedence
    true blocks the removal, and the plan names the scopes it could not check.
  • Offline flag. None, per the operator. --docs-dir covers docs only when both pages are
    supplied, and the drift catalogs still fetch unless SETTINGS_AUDIT_ENGINE_SKIP_DRIFT=1.
• Deferred (not measured here). Live behavior of a disabled dependency on a real Claude Code
  install. The absent-entry reading keeps the vet's range (observed disabled on 2.1.278 to
  2.1.282), and this PR did not re-measure it.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

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

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-26T09:09:13.997177Z	5d050a6	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 finished @kyle-sexton's task in 4m 40s —— View job

---

Security review

• Load /review:security-review skill instructions
• Gather PR diff and context (origin/main...HEAD)
• Run skip-gate check (PR is open, has a security-relevant surface: shell scripts that fetch, parse, and write settings/JSON/docs)
• Review changed files for security-relevant issues
• Adversarially validate candidate findings
• Post findings via inline comments and summarize here

Reviewed the full diff (git diff origin/main...HEAD, 18 files) at HEAD 5d050a699c6bd73b75e893cb9253a23b10308898, focused on the three shell scripts doing the real work: audit-engine.sh, check-plugin-drift.sh, and fix-plugin-drift.sh (settings read/write, backup, and the JSON/CR-safe key handling), plus check-doc-citations.sh.

Most of this PR is itself security-hardening (JSON-safe key handling to close the CRLF-key data loss bug, mktemp-based non-clobbering backups, symlink/TOCTOU guards on the settings-file replace, curl --globoff plus an owner/name allowlist regex on source.repo before it's put in a URL). That existing work looks sound and I did not find a way to defeat it.

1 finding, IMPORTANT:

• Path traversal via unsanitized slug in check-doc-citations.sh's page_file() — inline comment on check-doc-citations.sh#L98. slug is read verbatim from a manifest row (--manifest, default doc-citations.tsv) with no format check in the script. This PR newly allows / in slug (the plugins/install / plugins/dependencies rows) and newly adds mkdir -p "$(dirname "$f")" before the fetch, so a manifest row with a ../ segment in its slug resolves $FETCH_DIR/$slug.md outside the temp dir and curl -o (or the --docs-dir/fixture-dir read paths) follows it. The only guard is a test assertion (check-doc-citations.test.sh case 6) about the shipped manifest's character set — not a runtime check in the script, and it doesn't apply to --manifest <file> or --docs-dir. Suggested fix: validate slug against a safe pattern (reject .. segments / leading /) inside check-doc-citations.sh itself.

No other findings met the high-signal bar (exploitable path in this diff, not pre-existing/style/generic advice).

[claude]

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

---

Claude code review in progress

• Gather context (PR body, diff, CLAUDE.md/AGENTS.md instructions)
• Invoke /review:code-review and load its criteria
• Review the three core scripts (audit-engine.sh, check-plugin-drift.sh, fix-plugin-drift.sh) — running in background
• Review docs/manifest/test changes for consistency
• Adversarially verify candidate findings
• Post inline comments for confirmed findings anchored to changed lines
• Post summary comment with cross-file findings and job run link

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5d050a699c

ℹ️ About Codex in GitHub

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

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

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

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

[claude]

Claude code review in progress

• Gather context (PR body, diff, CLAUDE.md/AGENTS.md instructions)
• Invoke /review:code-review and load its criteria
• Review the three core scripts (audit-engine.sh, check-plugin-drift.sh, fix-plugin-drift.sh)
• Review docs/manifest/test changes for consistency
• Adversarially verify candidate findings
• Post inline comments for confirmed findings anchored to changed lines
• Post summary comment with cross-file findings and job run link

[claude]

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

---

Security review

• Load /review:security-review skill instructions
• Gather PR diff and context (origin/main...HEAD, HEAD 9953293366c36499750ebef0e27ffd65fada002e)
• Run skip-gate check (PR is open; security-relevant surface: shell scripts fetching, parsing, and writing settings/JSON/docs)
• Review changed files for security-relevant issues
• Adversarially validate candidate findings
• Post findings and summarize here

Re-ran the full security review for this push. The only commit since the last review (5d050a6 → 9953293) is test(claude-config): use a portable word boundary in the drift JQ_DEFS check, a one-line regex change in check-plugin-drift.test.sh with no security-relevant surface. The two previously-reported findings are still present in the code at this HEAD and remain unresolved, so I'm not filing new duplicate inline threads — summarizing both here against the current commit instead of re-opening the same lines.

1. IMPORTANT — path traversal via unsanitized slug in check-doc-citations.sh's page_file()
permalink
slug is read verbatim from a manifest row (--manifest, default reference/doc-citations.tsv) with no runtime format check in the script. This PR allows / in slug (the plugins/install / plugins/dependencies rows) and adds mkdir -p "$(dirname "$f")" before the fetch. A row with a ../ segment in its slug makes f="$FETCH_DIR/$slug.md" resolve outside the mktemp -d'd $FETCH_DIR, and the new mkdir -p creates the parent directories for it before curl -fsSL -o "$f" ... writes there; the same unsanitized join at $DOCS_DIR/$slug.md / $FIXTURE_DIR/$slug.md (lines 88, 92-93) lets a crafted slug read/probe an arbitrary path via --docs-dir or the fixture env var. The only guard is check-doc-citations.test.sh case 6, a CI assertion about the shipped manifest's character set (^[a-z-]+(/[a-z-]+)*\t.+$) — not a runtime check in check-doc-citations.sh, and it doesn't apply to --manifest <file> pointed elsewhere. Confirmed still present at this HEAD; no validation was added in the intervening commit. Suggested fix: validate slug against a safe pattern (reject .. segments / leading /) inside check-doc-citations.sh itself, not only in the test suite.

2. IMPORTANT — backup write reopens the mktemp path by name, reintroducing a symlink race (flagged by Codex, still open)
permalink
BACKUP=$(mktemp "$SETTINGS.bak.$STAMP.XXXXXX") creates the backup file exclusively, but the descriptor mktemp opened is not reused: cat "$SNAPSHOT" >"$BACKUP" reopens the path by name for the write. If another process with write access to the settings directory can unlink $BACKUP and replace it with a symlink in the window between the two opens, the redirection follows the symlink and writes the verbatim settings snapshot — which the script's own comments say can hold tokens and permission rules — to an attacker-chosen target. The subsequent [[ ! -f "$BACKUP" || -L "$BACKUP" ]] || ! cmp check only detects the substitution after the write already landed, so it doesn't prevent the leak, only reports it. This is the same class of TOCTOU window the PR's summary says this exact backup logic was written to close ("no existing path is written through, which closes the non-regular-file window"); the mktemp call closes the creation race but not the reopen-to-write race. Confirmed still present at this HEAD — this is Codex's P2 comment on the same lines from the prior push, unaddressed since. Suggested fix: write through the descriptor mktemp (or an explicit O_EXCL/noclobber open) already holds, e.g. redirect into a file descriptor opened once (exec {fd}>"$BACKUP"; cat "$SNAPSHOT" >&"$fd") rather than reopening $BACKUP by pathname.

No other findings met the high-signal bar (exploitable path in this diff, not pre-existing/style/generic advice). The rest of the diff's security-relevant hardening — CR-safe key handling via JSON end-to-end, the owner/name allowlist regex on source.repo before it's placed in a URL, curl --globoff, and the other backup/replace symlink guards — looks sound and unchanged since the last review.

[claude]

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

---

Security review

• Load /review:security-review skill instructions
• Run skip-gate check (PR is open; security-relevant surface: shell scripts writing settings backups and fetching/parsing docs by manifest-supplied path)
• Gather PR diff (origin/main...HEAD, 18 files) at HEAD 5a80ec2
• Review changed files, focused on the last push (commit 5a80ec2) against the two previously open findings
• Adversarially validate candidate findings
• Post summary

Re-reviewed after the push since the last security review (9953293 → 5a80ec2, one commit: fix(claude-config): write the drift backup on an exclusive open and validate citation slugs). This commit targets exactly the two findings still open from the prior rounds. Both are now fixed:

1. Path traversal via unsanitized slug in check-doc-citations.sh — FIXED.
check-doc-citations.sh#L106-L120 now validates every manifest row's slug against ^[a-z0-9_-]+(/[a-z0-9_-]+)*$ in a pass over the whole manifest that runs to completion (exiting 2 and naming the row on the first bad one) before page_file() is ever called, so no ../, leading /, or otherwise-shaped slug reaches the $FETCH_DIR/$slug.md, $DOCS_DIR/$slug.md, or $FIXTURE_DIR/$slug.md joins. Since the allowed character set excludes ., a .. segment can't match regardless of the segment-boundary logic — the fix isn't just filtering that one case, it structurally can't produce one. This also closes the gap I'd flagged in the prior round: the guard now lives in the script itself and applies to --manifest <file> pointed anywhere, not just a test assertion about the shipped TSV. New test case 9 in check-doc-citations.test.sh exercises exactly that gap (--manifest pointed at a crafted file) with ../escape, /escape, alpha/../../escape, and a case-sensitivity probe (Alpha), asserting exit 2 and that no OK line is ever printed for the bad row.

2. Backup-write symlink race in fix-plugin-drift.sh — FIXED.
fix-plugin-drift.sh#L744-L769 no longer reopens the backup path by name for the write. mktemp -u only names the path; the actual write goes through one exec 3>"$BACKUP" under set -C (noclobber) and umask 077, so bash opens with O_CREAT|O_EXCL — this fails (refusing, not following) if anything, including a symlink, already sits at that path at open time, which covers a link planted either before the pre-check or in the window between the check and the open. Immediately after opening, [[ -f /dev/fd/3 && ! -L "$BACKUP" && /dev/fd/3 -ef "$BACKUP" ]] confirms the path still resolves to the same inode the fd holds before anything is written. Critically, the actual write (cat "$SNAPSHOT" >&3) targets the already-open fd, not the pathname — so even if an attacker swaps the path for a symlink after this check succeeds, the write is unaffected; it always lands in the inode captured at the exclusive open, never in whatever the path resolves to at write time. This is the fix the prior review asked for ("write through the descriptor mktemp already holds... rather than reopening $BACKUP by pathname"). New test case 41 in fix-plugin-drift.test.sh simulates the exact race with an mktemp shim that plants a hard link, a symlink, and a dangling symlink at the generated name at the moment it's produced, and asserts: exit 2, no "Applied", settings byte-identical, the decoy target never written through, and the planted link left untouched.

No new security-relevant findings in this push — the diff outside these two fixes is limited to changelog/docs text describing the same change and matching test coverage.

Status: both previously-reported findings are resolved. No open findings.

[claude]

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

---

Claude code review in progress

• Gather context (PR body, diff, CLAUDE.md/AGENTS.md instructions)
• Invoke /review:code-review and load its criteria
• Review the new commit (5a80ec2, the exclusive-open backup write and citation-slug validation)
• Fresh pass on Q3/Q4/Q5/Q8 logic and the JSON-key refactor in the three core scripts — running in background
• Adversarially verify candidate findings
• Post inline comments for confirmed findings anchored to changed lines
• Post summary comment with cross-file findings and job run link

[claude]

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

---

Security review

• Load /review:security-review skill instructions
• Run skip-gate check (PR is open; security-relevant surface: shell scripts fetching, parsing, and writing settings/JSON/docs)
• Gather PR diff (origin/main...HEAD, 19 files) at HEAD 2183f96
• Review changed files, focused on the last push (commit 2183f96, the new consent-receipts labeling feature) against the two previously open findings
• Adversarially validate candidate findings
• Post summary

Re-reviewed after the push since the last security review (5a80ec2 → 2183f96, one commit: feat(claude-config): label CLI and plugin consent receipts from a per-plugin record). This touches audit-engine.sh, audit-engine.test.sh, the new reference/consent-receipts.json, and matching docs — it does not touch check-doc-citations.sh or fix-plugin-drift.sh, so both previously-reported findings stay resolved:

1. Path traversal via unsanitized slug in check-doc-citations.sh — still fixed as of 5a80ec2 (check-doc-citations.sh#L106-L120); unchanged in this push.
2. Backup-write symlink race in fix-plugin-drift.sh — still fixed as of 5a80ec2 (fix-plugin-drift.sh#L744-L769); unchanged in this push.

New commit (2183f96) review — no findings.

The new cr_load/cr_match/ejq path in audit-engine.sh#L857-L951 reads the shipped (or test-overridden via SETTINGS_AUDIT_ENGINE_CONSENT_RECEIPTS_FILE) reference/consent-receipts.json, validates its shape in a static jq program (E_DEFS/valid/rec) before trusting any field, and only ever interpolates the resulting strings ($k, $owner, .meaning, etc.) into row's jq -cn --arg ... calls — never into a shell command, eval, glob, or filesystem path. The one place a key-derived string reaches a shell metacharacter context is the pre-existing BIN_HAS binary search (audit-engine.sh#L953-969), which this commit doesn't touch and which already gates the leaf through BIN_WORD='^[A-Za-z_][A-Za-z0-9_]{3,}$' before use in grep -aqE.

Checked whether the new label could be used to mask a real undocumented-key finding an attacker controls: the match requires the key to literally equal an entry in the repo-shipped consent-receipts.json (not attacker-writable from a settings file or plugin manifest) and the file's scope to be in that record's declared scopes. A plugin-owned record additionally requires the owner to read true in the merged enabledPlugins, computed via the same JSON-safe enabled jq def used elsewhere in this file. There's no way for an untrusted settings/plugin value to forge a new consent-receipt entry or widen an existing one's scope — the record file is the only source of truth and it isn't reachable through data this script treats as untrusted. This is a reporting/labeling change in an offline audit tool, not a new trust boundary.

The one asymmetry I looked at closely — the claude-code owner path labels a key as ok with note="binary not checked" (not a finding) whenever BIN_SEARCH != searched, i.e. it doesn't require a positive binary confirmation to suppress the undocumented-key finding — matches the documented behavior in this same commit (validation-categories.md: "a binary that was not searched labels with 'binary not checked'") and is covered by test case 46's claude-shim sub-case. It's a false-negative-tolerance decision already made and documented by the authors, not an exploitable path (there's no untrusted input that flips BIN_SEARCH), so it doesn't meet this lane's high-signal bar.

Status: no new findings. Both previously-reported findings remain resolved.

[claude]

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

---

Claude code review

• Gather context (PR body, diff, CLAUDE.md/AGENTS.md instructions)
• Invoke /review:code-review and load its criteria
• Review the new commit (2183f96, consent-receipt labeling) against the three core scripts
• Spot-check the two previously-fixed findings (path traversal, backup symlink race) remain fixed at HEAD
• Adversarially verify candidate findings
• Post summary comment with cross-file findings and job run link

Scope of this pass

This lane already ran several full rounds on this PR (Codex + Claude code/security reviews through commits 5d050a6 → 9953293 → 5a80ec2), and both prior findings — the check-doc-citations.sh slug path traversal and the fix-plugin-drift.sh backup symlink race — were fixed in 5a80ec2 and independently confirmed fixed by the security lane. I spot-checked both are still intact at HEAD 2183f96 (unchanged since): check-doc-citations.sh still validates every manifest slug against ^[a-z0-9_-]+(/[a-z0-9_-]+)*$ before any path is built, and fix-plugin-drift.sh still writes the backup through the exec 3>"$BACKUP" exclusive descriptor under set -C/umask 077 rather than reopening the path by name.

The only commit since the last full review is 2183f96 (consent-receipt labeling), which is genuinely new, unreviewed logic, so I focused there.

Consent-receipt labeling (audit-engine.sh)

Read through cr_load()/cr_match() (plugins/claude-config/skills/audit/scripts/audit-engine.sh lines ~857–952) and the new consent-receipts.json reference file:

• The "wanted" gate (KP_KEY[$i] == KP_LEAF[$i]) correctly restricts the file load to top-level undocumented keys only — KP_KEY/KP_LEAF already holds only undocumented keys (populated at check_keys line 817-818 only when SR_KEY lookup misses), so the docs' claim "read only when an undocumented top-level key exists" checks out against the code, and cr_match's own "$k" == "$leaf" guard independently keeps a permissions.* leaf from ever matching, matching test case 46's assertion.
• The shape validation (rec/valid in the embedded jq) requires all five record fields non-empty and a single owner@marketplace shape (^[^@]+@[^@]+$, which structurally rejects more than one @) or the literal claude-code, and the bash-side "${f[0]:-}" != "ok" || (({#f[@]}-1) % 6) != 0 check correctly detects both a totally malformed file (empty f) and a well-formed-but-wrong-shape one.
• ejq/e_scope/E_DEFS/E_ARGS were relocated earlier in the file (not duplicated — confirmed only one definition of each remains) so cr_load can reuse the same enabled (merged enabledPlugins) computation the dependency-disabled check (category E) uses; SURF_USER/SURF_SETTINGS/SURF_LOCAL and USER_OK/PROJECT_OK/LOCAL_OK are all defined well before first use at the new call sites, so there's no ordering hazard from the move.
• CR-safety: the reference file's own CRLF is legitimately stripped before parsing (it's a static repo file, not user settings, so the PR's CR-preservation concern doesn't apply to it), while the settings key ($k) it's matched against still arrives via the existing CR-safe JSON path — the new test case ("a carriage-return twin of the key is never labeled") checks exactly this and matches the code's behavior on inspection.
• Test coverage (audit-engine.test.sh case 46) exercises the scope-mismatch message, the stale/binary-not-checked/binary-lacks-name branches, the disabled-owner branch, and both a missing and a shape-invalid record file — no gaps I could find in the branches cr_match actually has.

No correctness issues found in this commit.

Caveat on verification: this session's Bash tool only has git/gh available (invoking the scripts or their test suites directly required approval that isn't obtainable here), so the above is static review only — I did not independently execute audit-engine.test.sh or reproduce the PR's reported 270/270 pass. That count is author-claimed, not independently verified by me.

[claude]

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

---

Claude code review

• Gather context (PR body, diff, CLAUDE.md/AGENTS.md instructions)
• Invoke /review:code-review and follow its instructions
• Review the delta since the last full code-review pass on this lane (commit f0c801b, "refuse ambiguous or control-character consent receipts")
• Adversarially verify candidate findings
• Post summary comment with cross-file findings and job run link

Scope of this pass

This lane already completed full code-review passes through commit 2183f96 with no findings, and full security-review passes through 2183f96 with both prior findings (citation-slug path traversal, backup symlink race) confirmed fixed and unchanged since. The only commit since then is f0c801b, which touches audit-engine.sh, audit-engine.test.sh, and CHANGELOG.md (3 files, +38/-11). I focused this pass entirely on that commit.

f0c801b: consent-receipt ambiguity/control-character handling

This commit changes cr_load()/cr_match() in audit-engine.sh lines 862-964 to (1) never label a key as a consent receipt when a settings scope also holds a carriage-return variant of that key, and (2) treat a control character in a consent-receipt record's fields or owner id as making the whole record file invalid (not-inspectable), rather than silently shifting the NUL-delimited field parse.

Since the mechanism here (a JSON key that differs from another only by an embedded CR, compared via nkeys($s;$k): [$s | obj | keys[] | select(gsub("\r";"") == $k)] | length) is easy to get backwards, I didn't just read it — I reproduced the actual byte-level behavior with tr/jq directly (the sandbox here permits ad hoc tr/jq invocations but not running the repo's own scripts, so I couldn't execute audit-engine.test.sh itself and can't independently confirm the author's reported 270+/270+ pass count):

• Confirmed the settings-fixture line printf '%s\n' '{"...":true,"...\r":true}' (used in the new test cases) writes a literal backslash-r JSON escape into the file, not a raw CR byte — so tr -d '\r' (used throughout this script to sanitize input before jq parses it) does not collapse it pre-parse, and jq correctly decodes it into two distinct string keys differing only by a trailing CR character. That's what lets check_keys() legitimately emit two separate KP_KEY entries for this fixture, and what makes nkeys() correctly return 2 for that scope — confirming CR_IN excludes it and CR_AMB includes it, exactly as the new code intends.
• Separately confirmed that a real malformed raw CR byte in a key (the actual Windows-CRLF-corruption pattern this PR fixes elsewhere, e.g. the mk\r marketplace-name bug) is rejected by jq as invalid JSON when parsed directly, and is silently collapsed to a single key by the existing tr -d '\r' pre-parse step used by both check_keys() and e_scope() — so the two-keys-from-one-file scenario this commit defends against can only arise from a well-formed escaped \r in the source JSON, not from the raw-byte corruption pattern. That's a materially different trigger than the comment at fix-plugin-drift.sh... sorry, audit-engine.sh line 934-936 implies ("a key read through check_keys has lost any carriage return it carried"), but I could not find a case where the discrepancy changes the script's actual output — it's a pre-existing, unedited-by-this-commit comment describing the general CR-stripping behavior, not a new claim this commit makes, and every code path I traced against it (both the ambiguous-duplicate-key case and the lone-CR-variant case) produces the result the new tests assert. Not filing this as a finding; noting it here in case it's useful for a future wording pass.
• Traced the control character in a record field path: plain (explode | all(. >= 32)) correctly rejects NUL, LF, and other control characters in key/meaning/basis/as_of/recheck/owner-id, routing to the existing not-inspectable fallback rather than letting a stray NUL shift the f[] array's NUL-delimited field boundaries. The % 7 divisibility check and the i += 7 extraction loop are consistent with the 7 fields now emitted per record (key, owner, scopes, meaning, on, in, amb), and I didn't find an off-by-one in the new CR_AMB field's index (f[i+6]).
• cr_match()'s new CR_AMB branch runs before the CR_IN scope check and does continue rather than return, so a record matching the ambiguous key by name still lets the loop fall through to "no match" (return 1) rather than mis-attributing a why reason from a different scope — this looked like the likeliest place for a false "ok" label to leak through, and it doesn't.

No correctness issues found in this commit. The new test cases (audit-engine.test.sh case 46 additions) match the behavior I traced by hand.

Caveat on verification: as in the prior review round on this PR, this session's Bash tool doesn't have permission to execute the repository's own shell scripts (only git, gh, and ad hoc coreutils/jq are usable without an approval prompt this environment doesn't resolve), so I could not run audit-engine.test.sh and cannot independently verify the author's claimed pass counts — my confirmation above is from manually reproducing the relevant tr/jq byte-level behavior in isolation, not from running the suite.

[claude]

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

---

Security review

• Load /review:security-review skill instructions
• Run skip-gate check (PR open; security-relevant surface: shell/jq logic parsing settings and a consent-receipts record file)
• Gather PR diff (origin/main...HEAD) and isolate what's new since the last reviewed commit (2183f96)
• Review the new commit (f0c801b, "refuse ambiguous or control-character consent receipts")
• Adversarially validate candidate findings
• Post summary

Re-reviewed after the push since the last security review (2183f96 → f0c801b, one commit touching only audit-engine.sh, audit-engine.test.sh, and CHANGELOG.md). This commit doesn't touch check-doc-citations.sh or fix-plugin-drift.sh, so both previously-reported and previously-fixed findings stay resolved and unaffected:

1. Path traversal via unsanitized slug in check-doc-citations.sh — fixed since 5a80ec2, unchanged.
2. Backup-write symlink race in fix-plugin-drift.sh — fixed since 5a80ec2, unchanged.

New commit (f0c801b) review — no findings

The change is confined to cr_load()/cr_match() in audit-engine.sh, an offline audit tool reading local settings files and a repo-shipped record file. Two things changed, both closing gaps the commit message names accurately:

• Ambiguity guard. nkeys($s; $k) (L897) counts how many of a scope's raw JSON keys collapse to $k once \r is stripped (mirroring how check_keys itself reads keys). When more than one key in a scope collapses to the same record key — e.g. skipWorkflowUsageWarning next to skipWorkflowUsageWarning\r — CR_AMB now flags that scope and cr_match (L933-937) refuses to label either variant, falling through to the pre-existing undocumented-key finding instead of silently picking one. This closes a real false-negative (a CR-variant key could previously ride along unlabeled as if vetted); it doesn't open a new trust gap since the fallback is the stricter, pre-existing path.
• Control-character rejection. plain (L887) requires every codepoint in key/meaning/basis/as_of/recheck/owner to be >= 32, so a \u0000 (or any C0 control character) in a record field can no longer land in the NUL-delimited f[] array the bash side splits with read -r -d '' (L884) — a record with such a field now fails valid/rec and the whole file takes the existing not-inspectable fallback rather than desynchronizing the field count. The field-count check itself was updated in step (% 7 instead of % 6, L900) to match the new CR_AMB field, and the read loop's stride (L907) matches consistently.
• Checked obj (E_DEFS, L410) safely defaults non-objects to {}, so nkeys can't crash on a scope that isn't an object; plain's explode | all(. >= 32) operates on Unicode codepoints from jq's own JSON string type, not raw bytes, so there's no multi-byte/encoding trick to smuggle a low codepoint past the check.
• All of this is jq-internal string comparison and array indexing — no new shell interpolation, path construction, or command execution was introduced, and the two new test cases (audit-engine.test.sh case 46 additions) exercise exactly the ambiguous-twin and control-character scenarios described.

No security-relevant issues found in this commit.

[claude]

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

---

Claude code review

• Gather context (PR body, diff, CLAUDE.md/AGENTS.md instructions)
• Invoke /review:code-review and follow its criteria
• Identify delta since the last full code-review pass (commit f0c801b) — commits 11a9ddf and 0af73ff
• Review both new commits against code-review criteria (security findings stay on the dedicated security lane, which already ran on this HEAD)
• Adversarially verify candidate findings
• Post summary comment with cross-file findings and job run link

Scope of this pass

This lane already completed a full code-review pass through commit f0c801b with no findings (and a security-review pass through the same commit with both prior findings confirmed fixed). Two commits landed since then:

• 11a9ddf — reject a consent receipt listed under two owners, and make a stale claude-code verdict terminal for its key.
• 0af73ff — test-only: isolate the drift/engine suites from inherited SETTINGS_AUDIT_*/CLAUDE_PROJECT_DIR/settings-path env vars, and add a guard_apply that aborts the fix-suite if an --yes run would ever target a path outside the suite's temp dir.

I reviewed both in full against audit-engine.sh/audit-engine.test.sh/fix-plugin-drift.test.sh/check-plugin-drift.test.sh.

11a9ddf (audit-engine.sh cr_load/cr_match):

• The new jq clause ([.consentReceipt | to_entries[] | .key as $o | .value[] | {k: .key, $o}] | group_by(.k) | all(map(.o) | unique | length == 1)) groups by settings-key across all owners and requires a single owner per key — correctly invalidates the whole file (not-inspectable, no label) when the same key appears under two owners, which is what the tests (case 46) assert.
• Because the file-validity check already guarantees a unique owner per key, hoisting the "stale claude-code record is terminal" check out of the per-record loop and into an early, explicit scope/AMB/SCOPES-gated pass ahead of the loop is behavior-preserving, not just a structural move: I traced both the old (in-loop) and new (hoisted) code paths and they produce the same CR_WHY/return value for every combination of AMB, IN, and SCOPES gates — the earlier round's design ("no other owner can label the key once claude-code is stale") was already implied by owner-uniqueness, so this is a correct simplification rather than a behavior change needing new scrutiny.
• Docs (CHANGELOG.md, validation-categories.md) were updated to match, and new tests (case 46 duplicate-owner file, case 47 driving cr_match directly in both owner orders since a valid file can never reach that branch) cover exactly this.

0af73ff (test isolation + guard_apply):

• Verified every --yes/apply invocation in fix-plugin-drift.test.sh is preceded by a guard_apply call (directly, or via run_fix_apply/run_fix_internal, which now call it internally) against the correct settings path for that case — no --yes run in the suite is unguarded.
• The new guard_probe test (lines ~1409-1429) correctly exercises guard_apply in a child shell so its abort can't kill the suite itself, and its three "outside" fixtures (${TEST_TMPDIR}x/..., $TEST_TMPDIR/../elsewhere/..., /settings.json) each defeat the "$1" == "$TEST_TMPDIR"/* glob or the /../ exclusion for the reason the test intends, plus one in-bounds case that should pass.

No correctness issues found in either commit. Nothing here needs an inline comment — I found no candidate findings to bring to adversarial verification.

Caveat on verification: this session's Bash tool doesn't have permission to execute the repository's own shell scripts/test suites (only git, gh, and ad hoc coreutils/jq are usable without an approval prompt this environment doesn't resolve), so this is static review by reading the diffs and manually tracing the bash/jq control flow, not independent execution — the author-claimed pass counts in the PR body are unverified by me.
· branch claude-config-audit-derive-from-docs-rem

[claude]

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

---

Security review

• Gather context (PR body, diff, prior review comments, CLAUDE.md/AGENTS.md instructions)
• Invoke /review:security-review and follow its instructions
• Review the delta since the last full security-review pass (commit f0c801b → HEAD 0af73ff)
• Adversarially verify candidate findings
• Post summary

Two commits landed since the last full security-review pass on this PR (f0c801b): 11a9ddf ("reject a consent receipt listed under two owners") and 0af73ff ("isolate the drift and engine suites from inherited settings paths"). Both previously-fixed findings from earlier rounds remain resolved and untouched by either commit:

1. Path traversal via unsanitized slug in check-doc-citations.sh — fixed since 5a80ec2, not touched by either new commit.
2. Backup-write symlink race in fix-plugin-drift.sh — fixed since 5a80ec2, not touched by either new commit.

0af73ff — no production surface

This commit only edits the three *.test.sh harnesses (adds guard_apply isolation, clears inherited SETTINGS_AUDIT_*/CLAUDE_PROJECT_DIR, pins HOME/CLAUDE_CONFIG_DIR/CLAUDE_SETTINGS_FILE to fixtures). guard_apply is defined only in fix-plugin-drift.test.sh and is a test-suite safety rail, not part of audit-engine.sh, check-plugin-drift.sh, or fix-plugin-drift.sh. Nothing here changes what ships or runs against real settings.

11a9ddf — reviewed, no findings

This is the only production change: audit-engine.sh L857-976.

• Duplicate-owner guard (L890-895): the new valid clause groups every {key, owner} pair across all owners in consentReceipt and requires each key's owner set to be a singleton. A record file that lists the same key under two owners (e.g. claude-code and own@mkt) now fails validation up front, producing the pre-existing not-inspectable row rather than silently picking one owner's verdict — this is a stricter, fail-secure change (traced by hand; the jq is pure internal-string logic over a repo-shipped reference file, no injection surface).
• Stale-record terminal check moved earlier (L936-946): when the installed binary was searched and lacks the key's name, any matching claude-code-owned record now returns 1 (no label, CR_WHY names the stale reason) before the normal per-owner loop runs, rather than only inside the owner == "claude-code" branch of that loop as before. Because the duplicate-owner guard above makes it impossible for a valid file to list the same key under claude-code and another owner, this early-return path can't currently be reached with two competing owners for the same key in real data — the commit's own test (case 47) has to eval cr_match directly and hand-construct the arrays to exercise the "two owners, either order" case, exactly as the commit message states. The change is fail-secure in both directions: it only ever causes cr_match to withhold a label (return 1, falling through to the existing undocumented-key finding), never to grant one it wouldn't have granted before.
• Confirmed cr_match's caller (L1000) treats return 1 as "not labeled, keep the finding" — so nothing in this change can cause a real undocumented key to be hidden from the report; both new checks only make the tool more conservative.
• CR_FILE/its env override remain reachable only via the repo-shipped reference file or a test-only override, matching the trust-boundary analysis from the prior consent-receipt review rounds (2183f96, f0c801b) — no new attacker-controlled input reaches this jq or bash logic.

Status: no findings. Both previously-reported issues remain fixed; the two new commits introduce no new security-relevant regressions — one has no production surface, and the other only tightens (never loosens) the consent-receipt labeling gate.

[claude]

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

---

Claude code review

• Gather context (PR body, diff, CLAUDE.md/AGENTS.md instructions, prior review history)
• Invoke /review:code-review and follow its criteria
• Check the delta since the last full code-review pass (commit 0af73ff → HEAD bddfcae)
• Skip gate applied — no new review surface
• Post summary

Skip gate: no new review surface

bddfcae is an empty commit (ci: re-run CI on the verified tree) — git diff 0af73ff bddfcae is empty. This lane already completed a full code-review pass through 0af73ff (the previous commit) with no findings: prior review covered 11a9ddf and 0af73ff and found no correctness issues, following earlier full passes through 5d050a6 → 9953293 → 5a80ec2 → 2183f96 → f0c801b that also cleared with no findings (each new commit reviewed on arrival). That review still fully applies to this HEAD's tree since nothing changed.

Per /review:code-review's skip gate ("This head already has a successful review from this lane that still applies"), stopping here rather than re-deriving the same review. No inline comments to post.

For reference, the two issues raised earlier in this PR's review history (path traversal via unsanitized slug in check-doc-citations.sh, and the backup-write symlink race in fix-plugin-drift.sh) were both fixed in commit 5a80ec2 and independently confirmed fixed — see prior review comments on this PR.
· branch claude-config-audit-derive-from-docs-rem

[claude]

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

---

Security review

• Gather context (PR body, diff, prior review comments, CLAUDE.md/AGENTS.md instructions)
• Invoke /review:security-review and follow its instructions exactly
• Check the delta since the last full security-review pass (HEAD 0af73ff → bddfcae)
• Apply skip gate
• Post summary

Skip gate: no new security-relevant surface

bddfcae3369eb41afbd2d4fd16ab51f4204c8904 is an empty commit (git diff 0af73ff..bddfcae is empty). Its own message explains why: the previous CI run on this PR came from a body-edit event, which skips every lane, and ci-status treats an all-skipped run as a failure — this commit only restarts CI on the unchanged tree, per bddfcae.

Per this lane's skip gate: "This head already has a successful security review that still applies." The last full security-review pass on this PR ran through commit 0af73ff and found no findings, with both earlier-reported issues confirmed fixed and unchanged since 5a80ec2:

1. Path traversal via unsanitized slug in check-doc-citations.sh's page_file() — fixed, slug validated against ^[a-z0-9_-]+(/[a-z0-9_-]+)*$ before any path is built (check-doc-citations.sh#L106-L120).
2. Backup-write symlink race in fix-plugin-drift.sh — fixed, snapshot is written through an exec 3>"$BACKUP" exclusive descriptor under set -C/umask 077 rather than reopening the pathname (fix-plugin-drift.sh#L744-L769).

Since the tree at bddfcae is byte-identical to 0af73ff, both remain fixed and no new review is warranted.

Status: no findings. Skipped — no code change since the last completed pass.
· branch claude-config-audit-derive-from-docs-rem