fix(docs-hygiene,session-flow): wire remaining memory_dir consumers to shared parse-concern-value helper

[kyle-sexton]

Summary

Final wiring of the ratified bounded-widen scope on #474: fold the last two verified hand-rolled memory_dir/contract_dir copies into #482's already-landed parse-concern-value.sh helper (merged via #756).

• plugins/docs-hygiene/skills/audit-noise/scripts/lib/noise-shapes.sh — audit_noise_convention_roots_pattern hand-rolled the same naive ${val%%#*}-first strip claude-memory + session-flow: memory_dir seam parsing residuals — '#' truncation in quoted values, absent-config save-point fallback #482 fixed at its two hot sites (truncates a quoted value at an interior #), plus a defect unique to this copy: ${val//[[:space:]]/} collapsed all interior whitespace, and there was no quote-stripping. Now resolves both keys through the shared helper, materialized to plugins/docs-hygiene/skills/audit-noise/scripts/lib/parse-concern-value.sh (byte-identical + mode-identical, 100755, to the two existing copies) and registered in scripts/sync-parse-concern-value.sh's copies array — the same upstream-SSOT + sync-materialization pattern as the two prior sites (installed plugins are cache-isolated and cannot source a sibling's file).
• plugins/session-flow/skills/handoff/context/structure.md L116 — replaced the placeholder note (# the concern file's memory_dir when set — resolve it first, no mechanism named) with a pointer to the shared helper via the retro skill's Phase 1.1 as the worked call form. Doc note only: the handoff skill has no script of its own to rewire.

Per the operator's ratified bounded-widen decision (2026-07-19T23:26Z) and the tower's 2026-07-21 decision comment on #474, the broader tracker-seam-resolution class (#470/#469/#432/#365/#369) stays explicitly OUT of scope.

Version bumps stack past two independently-merged sibling PRs that landed while this one was open: docs-hygiene 0.8.0 → 0.8.2 (stacks past #846's 0.8.1, an exact version collision resolved by re-numbering this PR's entry on top and preserving #846's CHANGELOG entry underneath); session-flow 0.11.0 → 0.12.1 (stacks past a merged 0.12.0 that added the orient skill).

Related

• claude-memory + session-flow: memory_dir seam parsing residuals — '#' truncation in quoted values, absent-config save-point fallback #482 / fix(claude-memory,session-flow): quote-aware topic-docs seam parser via shared helper #756 — landed the shared parse-concern-value.sh helper and its first two consumers; this PR extends it to the remaining two per the ratified bounded-widen scope.
• feat(review,docs-hygiene): wire documentation-existence admission pre-check #846 — merged concurrently with this PR and independently bumped docs-hygiene to 0.8.1; this PR's version was restacked to 0.8.2 on top, both CHANGELOG entries preserved.
• fix(claude-memory): resolve memory_dir from topic-docs seam in orphan-rule-check #470 / fix(session-flow): derive retro HANDOFF_DIR from resolved memory_dir #469 / session-flow: topic-docs placement contract resolved via hardcoded raw.githubusercontent URL instead of bundled copy #432 / work-items: github adapter should resolve a consumer-local gh-bot.sh wrapper independent of adapter location #365 / work-items: define how consumer-local / shadowed adapters resolve the shared seam lib/ #369 — the broader tracker-seam-resolution class; explicitly excluded from this PR's scope per the operator's ratified decision.

Test plan

• bash plugins/docs-hygiene/skills/audit-noise/scripts/detect.test.sh — 34/34 pass, including a new regression case: a quoted memory_dir: ".scratch#dir/" # trailing comment now correctly resolves to .scratch#dir (flags .scratch#dir/foo/ as a concrete slice) instead of the old parser's silent truncation.
• Manual before/after demonstration of the two defects the old parser had:
  • Quoted + interior # + trailing comment + trailing slash: old [.scratch] (wrong — truncated inside the quotes) vs new [.scratch#dir] (correct).
  • Unquoted value with interior whitespace: old [mydir] (wrong — collapsed) vs new [my dir] (correct, surrounding whitespace trimmed only).
• scripts/sync-parse-concern-value.sh --check — all 3 plugin copies match lib/parse-concern-value.sh.
• scripts/check-cross-plugin-source-drift.sh --check — no unregistered/drifted clusters (the three copies have distinct within-plugin relpaths, so the generic drift check doesn't need to cluster them; the dedicated sync gate already covers them).
• scripts/check-changelog-parity.sh --check and --check-bump origin/main — both touched plugins (docs-hygiene 0.8.0→0.8.2, session-flow 0.11.0→0.12.1) carry matching CHANGELOG entries.
• shellcheck + shfmt -d — clean on all new/edited scripts.
• scripts/validate-plugins.sh — all manifests + catalog valid.

Closes #474

---

Co-authored-by: Claude Fable 5 noreply@anthropic.com

[claude]

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

---

PR Review — fix(docs-hygiene,session-flow): wire remaining memory_dir consumers to shared parse-concern-value helper

• Gather context and understand the PR
• Review the PR diff
• Review changed files in detail
• Analyze correctness, security, and alignment with CLAUDE.md guidelines
• Post findings

---

Verdict: approve with minor observations. The change is clean, correctly scoped, and follows the project's established patterns well. No blockers found.

---

What this does

Replaces the hand-rolled parser in noise-shapes.sh's audit_noise_convention_roots_pattern (three-bug combination: %%#*-first strip truncating quoted values at interior #, //[[:space:]]/ collapsing all interior whitespace, no quote unstripping) with a call to the shared helper, materialized as a byte-identical copy with correct 100755 mode. The session-flow change is docs-only.

---

Correctness ✓

Byte-identity verified. The new copy at plugins/docs-hygiene/skills/audit-noise/scripts/lib/parse-concern-value.sh is byte-for-byte identical to lib/parse-concern-value.sh (SSOT) and has the correct 100755 permissions — matching the two prior copies.

Sync script registration is correct. The new entry in scripts/sync-parse-concern-value.sh is at the right path and follows the existing array format.

The lib_dir derivation is safe. Using $(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) is the correct pattern for finding co-located scripts robustly.

strip_value logic is sound. Quote-first resolution (the core fix), CRLF guard, leading/trailing whitespace trim idiom, and trailing-slash normalization are all correct. The case structure correctly preserves # inside quoted values while stripping #-initiated comments from unquoted values.

---

Test coverage — adequate, one gap noted

The regression test added at detect.test.sh L272–290 is well-constructed and directly reproduces the main defect (quoted memory_dir with interior # and trailing comment).

Minor gap: The PR description claims testing of the unquoted interior-whitespace defect ("old [mydir] (wrong) vs new [my dir] (correct)"), but no corresponding test case was added to detect.test.sh. This defect is covered by parse-concern-value.sh's own tests in the other plugins, so it's not unverified — but a local test in detect.test.sh would give the noise-shapes.sh call-site a complete regression suite. Low priority. Add one →

---

Security observations

Low — key interpolated into sed pattern without runtime validation (parse-concern-value.sh L106):

raw_line=$(sed -n "s/^${key}:[[:space:]]*//p" "$concern_file" | head -1)

The comment documents the assumption ("keys are [a-z_]+ identifiers, no metachars"), and all callers in this repo pass hardcoded safe keys. There is no actual attack surface here since the result is captured, not eval'd. A [[ "$key" =~ ^[a-z_]+$ ]] || exit 2 guard after the existing argument-presence check would make the contract machine-enforced rather than prose-only — worth considering for the SSOT the next time lib/parse-concern-value.sh is touched.

Not a blocker for this PR.

---

Minor notes

set -o pipefail + head -1 SIGPIPE (L39, L106): with pipefail active, sed ... | head -1 will produce a non-zero pipeline exit (SIGPIPE/141) if the concern file has more than one matching key. This is benign in the current script (no set -e, and raw_line captures correctly before head exits), but it's a trap for a future -e addition. Pre-existing in the SSOT — not introduced here.

CHANGELOG heading style: docs-hygiene uses ### Fixed (ATX headings), session-flow uses Changed: (plain label). Each is consistent with its own prior history, so no action needed.

---

Summary

All three defects in the old inline parser are correctly eliminated. The materialized copy is byte-identical with correct permissions, sync registration is clean, version bumps are correct, and the regression test is solid. Approve.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[claude]

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

---

Security Review — PR #847

• Gather PR diff and changed files
• Read parse-concern-value.sh (new file — primary attack surface)
• Read noise-shapes.sh (modified — shell parsing logic)
• Read detect.test.sh (modified — test harness)
• Read sync-parse-concern-value.sh (modified — sync script)
• Read doc/manifest changes
• Assess for injection, path traversal, unsafe evals, secret handling, and logic flaws
• Post findings

---

Findings

SUGGESTION — Low exploitability | Confidence: CONFIRMED

Unvalidated key parameter interpolated into sed substitution pattern

parse-concern-value.sh, line 106:

raw_line=$(sed -n "s/^${key}:[[:space:]]*//p" "$concern_file" | head -1)

$key is embedded directly into a double-quoted sed substitution without escaping or format validation. The comment on line 105 documents the invariant ("keys are [a-z_]+ identifiers, no metachars") but does not enforce it.

What can actually go wrong:

• / in key — breaks the sed delimiter (s/^foo/bar://p becomes a syntax error); sed exits non-zero; raw_line is empty; the fallback fires silently. Not exploitable, but the failure is silent.
• & or \ in key — these are sed replacement metacharacters, not pattern metacharacters, so they don't affect the match side. Safe.
• Newline in key — undefined sed behavior; likely a benign error.

Important: no shell code execution is possible here. sed pattern injection does not reach the shell. All callers today pass hardcoded literals (memory_dir, contract_dir), so the PR as written is safe. The risk is strictly forward-looking: if a future consumer passes a caller-derived key, silent fallback-instead-of-error behavior would be surprising.

Suggested guard at the top of the argument-validation block:

if [[ ! "$key" =~ ^[a-z_]+$ ]]; then
  echo "parse-concern-value: key must match [a-z_]+: $key" >&2
  exit 2
fi

Fix this →

---

Clean areas

Everything else reviewed cleanly:

• $concern_file path handling — always double-quoted as a shell word (filename argument to sed, guarded by [[ -f "$concern_file" ]]). No traversal surface beyond what the caller controls via AUDIT_NOISE_REPO_ROOT, which is a plugin-operator variable, not end-user input.
• strip_value() subprocess — $val is piped via printf '%s' to sed stdin, never interpolated into the pattern. Safe.
• lib_dir execution path in noise-shapes.sh — derived from ${BASH_SOURCE[0]} (the script's own location, not user input); cd … && pwd normalizes it. The resulting "$lib_dir/parse-concern-value.sh" invocation is properly quoted.
• Regex-escaping before pattern use — noise-shapes.sh line 31 correctly sanitizes val through sed "s/[.[\\*^\$()+?{|]/\\\\&/g" before embedding it in the pattern alternation. The parser fix feeds into this escaping layer correctly.
• sync-parse-concern-value.sh — git show "$base:$manifest" uses a hardcoded copies[] array for $manifest derivation; $base comes from a CLI argument and is properly quoted. The jq call uses -r with a safe filter. No injection surface in the CI gate.
• Test harness — all heredocs use <<'EOF' (no shell interpolation in fixture content); AUDIT_NOISE_REPO_ROOT is set to controlled tmpdir paths.
• Manifests and CHANGELOG — version bump and doc changes carry no executable surface.

---

No CRITICAL or IMPORTANT findings. The single SUGGESTION above is a forward-hardening item; the PR is safe to merge as-is given all current callers pass hardcoded keys.

[claude]

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

---

PR Review — fix(docs-hygiene,session-flow): wire remaining memory_dir consumers to shared parse-concern-value helper

• Gather PR context and understand the change
• Review PR diff (current HEAD vs origin/main)
• Review changed files in detail
• Analyze correctness, security, and alignment with CLAUDE.md
• Post findings

---

Verdict: approve. Two prior Claude reviews covered this thoroughly; the change is correct and the outstanding suggestions from those reviews remain low-priority. Direct verification on the working tree confirms what the diff shows.

---

What's here

This PR replaces the three-bug hand-rolled parser in noise-shapes.sh's audit_noise_convention_roots_pattern with a call to the shared parse-concern-value.sh helper, materialized as a co-located copy under scripts/lib/. The session-flow change is docs-only.

---

Verified on the working tree

Byte-identity and permissions — confirmed. Ran diff and ls -la directly:

-rwxr-xr-x  plugins/docs-hygiene/skills/audit-noise/scripts/lib/parse-concern-value.sh
-rwxr-xr-x  plugins/claude-memory/skills/audit/scripts/parse-concern-value.sh
-rwxr-xr-x  plugins/session-flow/skills/retro/scripts/parse-concern-value.sh

All three copies are byte-for-byte identical to lib/parse-concern-value.sh. Mode 100755 on all three matches the SSOT and the prior copies.

Sync script registration — correct. The new entry in scripts/sync-parse-concern-value.sh L22 follows the existing array format, and the --check-bump manifest derivation (${copy%%/skills/*}/.claude-plugin/plugin.json) resolves correctly to plugins/docs-hygiene/.claude-plugin/plugin.json for the new path.

lib_dir derivation when sourced — correct. noise-shapes.sh L23 uses $(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd). When a script is sourced, ${BASH_SOURCE[0]} is the sourced file (not the caller's $0), so this correctly resolves to scripts/lib/ — the directory where the co-located parse-concern-value.sh copy lives.

CHANGELOG/version consistency — correct. docs-hygiene bumps 0.8.1 → 0.8.2 and session-flow bumps 0.12.0 → 0.12.1; both CHANGELOGs carry matching entries.

Note: The PR description says "docs-hygiene 0.8.0→0.8.1, session-flow 0.11.0→0.11.1" — those version numbers reflect the branch state before two origin/main merges landed additional bumps. The description is stale; the code is correct.

---

Open items from prior reviews — status

Item	Status
Low — key interpolated into sed pattern without validation (prior security review)	Not addressed — was explicitly filed as a forward-hardening SUGGESTION, not a blocker. Still recommended if the SSOT is ever touched again.
Missing unquoted interior-whitespace test in detect.test.sh (prior review)	Not added — the SSOT's own tests cover this path; absence is low priority. The quoted-interior-# regression case was added.

Neither item blocks merge. Both were explicitly called out as non-blocking in the prior reviews.

---

No new issues found

The implementation is scoped, the fix is correct, and the materialization pattern matches the two prior sites exactly.

[claude]

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

---

Security Review — PR #847

• Get PR diff and identify changed files
• Read parse-concern-value.sh (new file — primary shell execution surface)
• Read noise-shapes.sh (modified — shell parsing logic)
• Read sync-parse-concern-value.sh (modified — CI gate script)
• Read detect.test.sh (modified — test harness)
• Read manifest and doc changes
• Assess for injection, path traversal, unsafe evals, credential handling, and logic flaws
• Post findings

---

Findings

No CRITICAL or IMPORTANT findings.

---

SUGGESTION — Low exploitability | Confidence: CONFIRMED

Unvalidated key interpolated into sed substitution pattern — now present in three copies

parse-concern-value.sh L106:

raw_line=$(sed -n "s/^${key}:[[:space:]]*//p" "$concern_file" | head -1)

$key is interpolated into the sed substitution without delimiter escaping or format validation. The comment on L105 documents the invariant ("keys are [a-z_]+ identifiers, no metachars"), but does not enforce it.

Current blast radius is zero. Both call sites in noise-shapes.sh use hardcoded loop literals (memory_dir, contract_dir). There is no caller-derived or user-derived key path in the current codebase. No shell code execution is possible via sed substitution injection — the result is captured, not eval'd.

What breaks with a bad key:

• A key containing / (the sed delimiter) causes a sed syntax error → exit non-zero → raw_line is empty → silent fallback to defaults. Not exploitable, but the failure is silent.
• A key containing & or \ affects only the replacement side of s///, not the pattern — no impact on match behavior.
• A key containing newlines produces undefined sed behavior on most platforms.

This finding is pre-existing in the SSOT (lib/parse-concern-value.sh) and was already noted in the prior security review comment on this PR. This PR correctly copies the file byte-for-byte, so no new risk is introduced — but the materialization of a third copy makes the forward-hardening item more load-bearing if the SSOT is ever patched without updating the copy contract.

Suggested guard in the SSOT (apply via scripts/sync-parse-concern-value.sh so all copies receive it):

if [[ ! "$key" =~ ^[a-z_]+$ ]]; then
  echo "parse-concern-value: key must match [a-z_]+: $key" >&2
  exit 2
fi

Fix all three copies →

---

Clean areas

Everything else reviewed cleanly:

• $concern_file path handling — double-quoted throughout; passed as a filename argument to sed (never interpolated into its pattern); guarded by [[ -f "$concern_file" ]]. The value is always ${AUDIT_NOISE_REPO_ROOT:-.}/.claude/topic-docs.yaml — an operator-configured root, not end-user document content. No path traversal surface beyond what the plugin operator controls.
• strip_value() subshell — $val is passed via printf '%s' "$val" | sed (argument, not pattern), so file content is never exec'd or pattern-injected. Quote-peeling uses pure bash parameter expansion — no eval.
• Resolved value → regex pattern — noise-shapes.sh L31 correctly applies sed "s/[.[\\*^\$()+?{|]/\\\\&/g" to escape the resolved value before embedding it in the $BASH_REMATCH pattern. A malicious topic-docs.yaml value cannot inject a regex that commands shell execution.
• lib_dir derivation — $(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) is derived from the script's own path, not any user input. The resulting "$lib_dir/parse-concern-value.sh" invocation is properly quoted.
• sync-parse-concern-value.sh --check-bump — $base (CLI arg) and $manifest (derived from hardcoded copies[] array) are both properly quoted. git show "$base:$manifest" does not exec arbitrary content; its output is piped to jq -r '.version // empty' — safe.
• detect.test.sh fixture — <<'EOF' (single-quoted heredoc) prevents shell expansion of fixture content. AUDIT_NOISE_REPO_ROOT is set to a controlled $TEST_TMPDIR path.
• No .github/workflows changes — no GitHub Actions attack surface in this diff.
• No secrets or credentials — no tokens, API keys, or credentials introduced.
• Byte-identity confirmed — the new copy at plugins/docs-hygiene/skills/audit-noise/scripts/lib/parse-concern-value.sh is byte-for-byte identical to lib/parse-concern-value.sh (SSOT), verified by reading both files.

The PR is safe to merge as-is. The single SUGGESTION above is a forward-hardening item for the SSOT — not a blocker.

[kyle-sexton]

Tower merge-sit (T4 dark, stall protocol): CLEAN, 22/22 green, 0 unresolved threads, conflict recomposition verified (both CHANGELOG entries preserved, versions stacked 0.12.1/0.8.2). This was generated by AI (control tower, session 6).