fix(docs-hygiene): speed audit-noise detect hot path (0.14.4)

[kyle-sexton]

Summary

• Hoist and export convention-root resolution once per detect.sh run so AUDIT_NOISE_CONTRACT_ROOT is not lost to command-substitution subshells (auditor F6: configured contract root bare reviews//handoffs/ no longer falsely exempt).
• Restructure the per-line hot path to use nameref helpers (detect_shapes_into, shape_tier_into, trim_excerpt) instead of 2+ $() forks per markdown line.
• Add --offset / --limit so repo-wide orchestration can chunk without a per-file shell loop; document in SKILL.md clean-tree defaults.
• Bump docs-hygiene to 0.14.4.

Test plan

• bash plugins/docs-hygiene/skills/audit-noise/scripts/detect.test.sh — 48/48 pass (includes F6 contract-bare regression + chunk affordance)
• Perf smoke: single non-CHANGELOG scan path no longer pays per-line parse-concern-value.sh forks; 40×885-line corpus ~7.8s in this environment

Closes #2741

Related

• Base of the docs-hygiene stack (fix(docs-hygiene): close audit-noise scanner exemption gaps (0.14.5) #2794 → docs(docs-hygiene): share clean-tree fallback across audit skills (0.14.6) #2795 → docs(docs-hygiene): process derivability route-to-sibling batch (0.14.7) #2798 → fix(docs-hygiene): compress caveman/taxonomy audit remediations (0.15.0) #2805).

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[claude]

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

---

Security review (/review:security-review)

• Gather context (diff, changed files, repo guidelines)
• Invoke /review:security-review skill and follow its instructions
• Post findings

Skip gate hit: no security-relevant surface.

This PR (b2e47ac) touches only:

• plugins/docs-hygiene/skills/audit-noise/scripts/detect.sh
• plugins/docs-hygiene/skills/audit-noise/scripts/lib/noise-shapes.sh
• detect.test.sh, SKILL.md, CHANGELOG.md, plugin.json (version bump)

These are a local, offline bash markdown-linter (audit-noise) that reads the
repo's own tracked .md files and prints findings — no network calls, no
secrets/credential/token handling, no authN/authZ logic, and no GitHub Actions
workflow or permissions changes.

I checked specifically for the classes this lane hunts for:

• Injection: the new --offset/--limit flags are validated with
  ^[0-9]+$ before any arithmetic/comparison use
  (detect.sh#L73-L76),
  so non-integer input hits the exit 2 unknown-arg path rather than reaching
  [[ ... -ge ... ]]/$(( )). No eval, no unquoted expansion into a command
  context.
• New namerefs (local -n) in detect_shapes_into / shape_tier_into /
  trim_excerpt (noise-shapes.sh):
  the target variable name is always a hardcoded literal at each call site
  (shapes, tier, excerpt), never derived from file content or CLI args,
  so there's no nameref-injection surface.
  Line content itself only ever reaches [[ =~ ]] regex matches and printf '%s' — never a command-substitution or eval context.
• Env hoist: AUDIT_NOISE_ROOTS_PATTERN / AUDIT_NOISE_CONTRACT_ROOT are
  now exported once per run instead of being recomputed per line
  (noise-shapes.sh#L26-L43).
  Values originate from the repo's own .claude/topic-docs.yaml
  (author-controlled config, not PR/attacker input), same trust level as
  before — this is a correctness fix (auditor F6), not a trust-boundary change.
• No workflow (.github/**) files, dependency pins, or permissions are
  touched by this PR.

No CRITICAL / IMPORTANT / SUGGESTION findings to report.

[github-actions]

Last security-reviewed head: b2e47ac27856906238b2ac42d4f82c06260f31e2. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b2e47ac278

ℹ️ About Codex in GitHub

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

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

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

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

[claude]

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

---

Code review — /review:code-review

• Gather context (PR diff, changed files)
• Read CLAUDE.md / REVIEW.md guidelines
• Review detect.sh changes
• Review noise-shapes.sh changes
• Review detect.test.sh changes
• Review SKILL.md / CHANGELOG / plugin.json
• Post findings

Summary

Scope note: this repo has a claude-security-review.yml workflow, so per REVIEW.md this lane stays out of security findings (none applicable here anyway — read-only markdown scanner over repo-local content).

The core fix is sound. I traced through the actual bug mechanics rather than just trusting the description:

• F6 fix (contract-root loss to subshells): confirmed. In the pre-PR code, roots="$(audit_noise_convention_roots_pattern)" inside audit_noise_line_has_ghost_ref ran in a nested command-substitution subshell within the already-subshelled audit_noise_detect_shapes "$line" call — so AUDIT_NOISE_CONTRACT_ROOT assignments never escaped even one level up, let alone into the top-level process. The fix's hoist-and-export in detect.sh (calling audit_noise_resolve_convention_roots directly, no $()) genuinely fixes this, and the regression test at detect.test.sh:310-323 exercises the exact scenario.
• Hot-path nameref rewrite: audit_noise_detect_shapes_into / audit_noise_shape_tier_into / trim_excerpt's nameref form correctly eliminate the per-line forks; I checked for nameref name-collision hazards (a classic bash footgun) between caller locals (shapes, tier, excerpt) and callee nameref names (_audit_noise_*_out) — no collisions.
• --offset/--limit chunk slicing: traced the slice loop by hand against the detect.test.sh:353 chunk test (offset=1/limit=1 over 3 files) — logic is correct, including the zero-selection early exit.
• Author-claimed 48/48 test pass and the perf smoke numbers in the PR description are unverified by me — I don't have a shell available in this lane to run detect.test.sh myself, so I traced the assertion count (51 assert_* calls minus 3 function definitions = 48) statically instead, which is consistent with the claim but not a substitute for actually running it.

Finding posted inline

🔴 detect.sh:43-50 — --offset/--limit as the last CLI token with no value causes shift 2 to silently no-op (bash: shift is a no-op when n > $#, and the non-zero exit isn't checked), leaving the same token in $1 forever — an infinite spin loop, not a clean exit 2. This is new code added specifically for the programmatic/orchestration use case the PR targets, which is exactly where a templating bug could drop a flag's value. See the inline comment for a concrete guard. (The pre-existing --paths-file arm has the identical hazard, but it's untouched by this diff so I left it out of scope.)

[github-actions]

Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

[claude]

Claude finished @cursor[bot]'s task in 5m 2s —— View job

---

Code review — /review:code-review

• Gather context (PR diff at HEAD 36cca2f6, changed files, repo guidelines)
• Invoke /review:code-review skill and follow its instructions
• Post findings

Summary

Per REVIEW.md, this repo carries a claude-security-review workflow, so this lane stays out of security findings (a prior security-review pass already covered this PR).

This head (36cca2f6) is a follow-up commit on top of the previously-reviewed b2e47ac, addressing the two P2/Important findings both Codex and I raised in the earlier rounds:

• Missing-value hang (detect.sh:37-43): the new require_opt_value() helper guards --paths-file/--offset/--limit, checking $# -lt 2 before touching $2 (so it's safe under set -u) and rejecting a following token that looks like another flag ($2 == -*). I traced this by hand against the reported hazard (detect.sh --offset with nothing after it) — the loop now hits exit 2 instead of spinning on the same unconsumed token. Fixed correctly.
• Octal misparse (detect.sh:89-91): OFFSET=$((10#$OFFSET)) / LIMIT=$((10#$LIMIT)) force base-10 interpretation after the ^[0-9]+$ regex validation, so --offset 08 no longer hits "value too great for base" and silently falls through to scanning the entire target list. Fixed correctly.

One finding posted inline on detect.sh:89-91: both fixes are sound, but neither gets a regression test, unlike every other fix in this PR (F6 contract-bare, chunk affordance, negative-offset all got dedicated assert_exit/assert_contains cases). Both bugs are in the "green CI, silently wrong" class — worth closing before merge, not blocking.

Note: I don't have a shell available in this lane to run detect.test.sh myself (confirmed — script execution is blocked in this sandbox), so the fixes above are verified by static trace only, consistent with the prior review round's stated limitation.

[claude]

Nit (test coverage): this fixup commit resolves both prior review findings (missing-value guard at require_opt_value() line 37, and this leading-zero decimal-normalization) correctly — traced by hand, both are sound. But unlike the rest of this PR, which added a dedicated regression test per fix (F6 contract-bare, chunk affordance, negative-offset), neither new fix gets a test:

• No test exercises --offset/--limit/--paths-file as the last CLI token with no following value (the hang scenario require_opt_value guards against).
• No test exercises a leading-zero value like --offset 08 (the octal-misparse scenario the 10#$OFFSET strip guards against).

Both are exactly the "green CI, silently wrong" class — a hang wouldn't fail a test that doesn't invoke that path, and the octal bug degrades to scanning the whole list without erroring, so a regression here wouldn't be caught by the existing suite. Since detect.test.sh already has the harness for this (assert_exit, chunk fixtures at the block above), two more assert_exit/assert_contains cases would close the gap.