fix(claude-memory,session-flow): quote-aware topic-docs seam parser via shared helper by kyle-sexton · Pull Request #756 · melodic-software/claude-code-plugins

kyle-sexton · 2026-07-20T20:32:02Z

Summary

The topic-docs memory_dir seam was parsed inline at two sites with a naive ${val%%#*}-first strip — the same naive-string-op class the #469/#470 whitespace fixes closed, left as a review-deferred residual. It truncated a legitimately quoted value containing # (memory_dir: "a#b" → a) because it stripped a "comment" before resolving surrounding quotes, and an absent concern file collapsed straight to .work without consulting the contract's rung-2 (a save-point convention declared in CLAUDE.md / .claude/rules).

Per the issue's acceptance criteria, both sites now delegate to one shared parser rather than re-forking the duplicated block.

Fix

lib/parse-concern-value.sh (new) — the single seam parser. Order is quote-resolution first, then a quote-aware comment strip (unquoted #… only; a#b / .work/#topic preserved), then whitespace-trim, then trailing-slash-normalize. Takes <concern-file> <key> [fallback]; the fallback is the caller-resolved rung-2 location (prose is an inference source, not a machine key — the agent resolves it and passes it in). Emits empty only when nothing resolves, so the caller owns the documented .work default.
Both consumers wired through it: claude-memory orphan-rule-check.sh (real script) and the session-flow retro HANDOFF_DIR snippet (retro/context/session.md). The non-interactive orphan check degrades to .work and notes that inferred/interactive resolution is the calling skill's job, rather than silently collapsing rung 2.
Materialization + drift gate: copies are synced from lib/ into each plugin (installed plugins are cache-isolated, so each must be self-contained — the lib/hook-utils.sh precedent). New scripts/sync-parse-concern-value.sh (--check / --check-bump) + a parse-concern-value-sync CI job keep the copies byte-identical and enforce version bumps when the lib changes. The two copies live at different relpaths, so the generic cross-plugin drift check does not cluster them — a dedicated gate is required.

Verification

All run locally (Git Bash):

lib/parse-concern-value.test.sh — 19/19 pass, incl. regressions: double/single-quoted a#b and .work/#topic keep #; quoted value with trailing comment drops the comment but keeps the inner #; absent/empty key returns the supplied fallback; held behavior (unquoted trailing # comment stripped, whitespace trimmed, interior quoted space preserved, trailing slash normalized).
orphan-rule-check.test.sh — 14/14 pass, incl. new Case 9: a ref inside a quoted ".scratch#dir" tier is correctly excluded (the old %%#* truncated to .scratch, masking the orphan).
scripts/sync-parse-concern-value.sh --check — copies match source.
scripts/check-cross-plugin-source-drift.sh --check — no unregistered/drifted clusters.
shellcheck + shfmt -d — clean on all new/edited scripts.
scripts/validate-plugins.sh — all manifests + catalog valid.

Closes #482

…ia shared helper The topic-docs `memory_dir` seam was parsed inline at two sites with a naive `${val%%#*}`-first strip that truncated a legitimately quoted value containing `#` (`"a#b"` -> `a`) and stripped comments before resolving quotes. Extract a single shared `parse-concern-value.sh` helper (quote-resolution first, then whitespace-anchored comment strip, trim, trailing-slash normalize) materialized from `lib/` into both consuming plugins, and route the claude-memory orphan-rule-check and the session-flow retro `HANDOFF_DIR` snippet through it. The helper takes the caller-resolved rung-2 location (a save-point convention declared in CLAUDE.md / .claude/rules) as an input argument, so an absent concern file no longer collapses straight to `.work`: prose stays an inference source the agent resolves, not a machine key the script parses. A dedicated scripts/sync-parse-concern-value.sh (--check / --check-bump) plus a parse-concern-value-sync CI gate keep the plugin copies byte-identical to the source, mirroring the hook-utils sync pattern. Co-Authored-By: Claude Opus <noreply@anthropic.com>

claude · 2026-07-20T20:32:17Z

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

PR Review — fix(claude-memory,session-flow): quote-aware topic-docs seam parser via shared helper

Summary

The fix is correct and well-structured. The root bug (naive %%#* stripping before quote resolution) is genuinely addressed, the shared helper's parse order is right (quote → comment → trim → slash), and the test suite directly pins the regression. The sync/drift gate mirrors the established hook-utils pattern and the CI integration is complete.

Two findings below worth addressing before merge, plus several minor notes.

Finding 1 — `sed` key injection: no validation on the `$key` argument

File: lib/parse-concern-value.sh line 104

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

$key is interpolated directly into the sed expression. The inline comment says "keys are [a-z_]+ identifiers, no metachars," which is true for every current call site (memory_dir is a fixed literal). But the helper is documented as a general-purpose seam parser and is intended to gain more consumers (per #474). A key containing sed regex metacharacters (., *, [) would silently produce incorrect output rather than an error, and a caller-controlled key with an injection string could alter the substitution.

A simple guard at the top of the file would close this permanently:

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

Alternatively, quote the key with \Q...\E if using GNU sed, but the POSIX [a-z_]+ guard is cleaner and self-documenting.

Fix this →

Finding 2 — `--check-bump` manifest path derivation is undocumented and fragile

File: scripts/sync-parse-concern-value.sh line 54

manifest="${copy%%/skills/*}/.claude-plugin/plugin.json"

This assumes every consumer copy lives at <plugin-root>/skills/<skill-name>/.... That assumption is correct for both current entries, but it is load-bearing, silent, and will compute a wrong manifest path for any future consumer whose skill lives at a different depth (e.g., <plugin>/hooks/… or a flat layout). Unlike the copies array—which is an explicit list with an "add a consumer by adding its path" comment—this derivation has no such signal.

A comment noting the assumed layout contract would prevent a silent failure when a new consumer is added:

# Assumes consumer paths follow <plugin-root>/skills/<name>/…
manifest="${copy%%/skills/*}/.claude-plugin/plugin.json"

This mirrors what the sync-hook-utils.sh script presumably documents (if that script has a similar derivation). Low urgency, but given this is a drift gate whose silent failure would allow a version-unbumped plugin to ship a stale lib to consumers, the assumption should be visible.

Fix this →

Minor notes (no action required)

Fallback passthrough is intentionally not normalized: The caller-supplied fallback is emitted raw without passing through strip_value. This is correct by design (the caller owns the default), but a caller passing ".notes/" would get an unnormalized trailing slash. The existing test at lib/parse-concern-value.test.sh line 77 ("fallback is trailing-slash-agnostic passthrough") documents this behavior. Fine as-is, but future consumers should be aware.

subshell + sed in strip_value's unquoted branch (line 92) is the right call—the comment-strip pattern ([[:space:]]#.*$) can't be expressed portably in pure Bash parameter expansion without extglob. No change needed.

jq dependency in --check-bump is undeclared in the script but available on ubuntu-24.04 GitHub-hosted runners. Consistent with the hook-utils precedent and low risk. Acceptable.

ci-status correctly gates on parse-concern-value-sync at line 613 — the new lane is fully wired into the required-check aggregate.

Session.md ${DECLARED_SAVEPOINT:-} idiom is intentional: the prose instructs the agent to infer and substitute the value. The empty default produces an empty-string argument, which correctly propagates to parse-concern-value.sh as "no fallback," and the .work default is applied by the subsequent ${MEMORY_DIR:-.work}. The two-step default is correct and matches the resolution order in the helper's contract.

Test coverage verdict

The 19 unit tests in lib/parse-concern-value.test.sh and the 14 integration tests in orphan-rule-check.test.sh are well-targeted. Case 9 (hash inside a quoted memory_dir) directly pins the regression. The whitespace test (Case 8) and override test (Case 6) cover known historical failure modes. No gaps found.

claude · 2026-07-20T20:32:20Z

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

Security Review — PR #756

Gather context and read PR diff
Review lib/parse-concern-value.sh (new shared parser)
Review scripts/sync-parse-concern-value.sh (new sync/drift gate)
Review .github/workflows/ci.yml (new CI job)
Review plugin consumers (orphan-rule-check.sh, session.md)
Synthesize and post findings

Scope: all 13 changed files reviewed. No critical or important security findings. One suggestion documented below.

Findings

SUGGESTION — Unvalidated `key` parameter interpolated into `sed` expression (CONFIDENCE: High)

File: lib/parse-concern-value.sh, line 104

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

$key is interpolated directly into the sed expression without any runtime validation. The comment at line 103 notes that keys are [a-z_]+ identifiers with no metacharacters, but this is documentation — nothing enforces it at runtime.

Practical impact today: none. Every current call site passes the hardcoded literal memory_dir (orphan-rule-check.sh:68, session.md:52). No code execution path exists through this alone because:

A key containing / mangles the sed delimiter but produces wrong output, not command execution.
GNU sed's e flag (which would execute the replacement as shell code) cannot be smuggled into the flags position through $key injection alone — the template's trailing //p fixes the flags to p.

Latent risk: if a future caller ever passes a dynamic or user-supplied key, a value containing / silently corrupts the extraction (producing the wrong field value rather than an error), and a value containing & or \ in the replacement position could produce unexpected output. The helper's contract does not document this constraint for callers.

Suggested hardening — add a guard before the sed call:

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

This enforces the documented invariant at the API boundary rather than relying on callers to respect it. Fix this →

Areas reviewed with no findings

Area	Result
CI workflow trigger (`push` + `pull_request`, not `pull_request_target`)	Safe — fork PRs cannot access secrets
`github.base_ref` in new CI job (line 270)	Safe — isolated to `env:`, never directly interpolated in `run:` block; consistent with all prior `BASE_REF` usages in the file
Workflow permissions	Safe — `contents: read` at workflow level, `persist-credentials: false` on all checkouts, no token usage in new job
`$SCRIPT_DIR` resolution and child-script execution in `orphan-rule-check.sh`	Safe — resolved from `BASH_SOURCE[0]`, not user-controlled input
`git grep` pathspec with `$memory_dir` interpolation (`orphan-rule-check.sh:91`)	Safe — the entire pathspec is a single double-quoted argument; malformed content produces a git error, not code execution
File path handling (`concern_file` arg)	Safe — read-only (`-f` check + `sed`), no write path; no privilege escalation beyond existing repo access
`scripts/sync-parse-concern-value.sh --check-bump` (`git show "$base:$manifest"`)	Safe — `$base` is a git ref argument used as a git argument, not interpolated into shell
Action pins in new CI job	Safe — uses same SHA-pinned `actions/checkout` and `actions/setup-node` as all other jobs

No objections to merging on security grounds. The suggestion above is optional hardening for a latent API-contract concern, not a blocker.

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

claude · 2026-07-20T20:38:55Z

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

PR Review — fix(claude-memory,session-flow): quote-aware topic-docs seam parser via shared helper

Gather context and understand request
Diff the PR changes (HEAD: 52ed944)
Review lib/parse-concern-value.sh (shared parser logic)
Review lib/parse-concern-value.test.sh (test coverage)
Review consumer wiring and sync script
Check if previous findings were addressed
Synthesize and post review

Latest commit (`52ed944`)

Trivial CHANGELOG-only fix: resolved MD038 (space-in-code-span) in the claude-memory [0.2.2] entry — ` # comment` → `# comment`. No functional or structural change.

Overall verdict

The fix is correct and well-structured. The root bug (naive ${val%%#*} stripping before quote resolution) is fully addressed. The shared helper's parse order — quote-resolution first, then whitespace-anchored comment strip, then trim, then slash normalize — is exactly right. Test coverage directly pins the regression (Case 9 in the integration suite; the four regression cases in the unit suite). The sync/drift gate mirrors the established hook-utils pattern and CI integration is complete.

Two findings from the previous review cycles remain unaddressed. Documented below.

Finding 1 — `$key` interpolated into `sed` without runtime guard (still open)

File: lib/parse-concern-value.sh line 104 (and identical copies in both plugin trees)

# sed anchors on the literal key; keys are `[a-z_]+` identifiers, no metachars.
raw_line=$(sed -n "s/^${key}:[[:space:]]*//p" "$concern_file" | head -1)

The inline comment documents the invariant but does not enforce it. Every current call site passes the hardcoded literal memory_dir, so there is no live risk today. The concern is forward-looking: this helper is explicitly intended to gain more consumers (per #474), and a caller passing a key with sed metacharacters (., *) would silently produce wrong output rather than an error. Both the PR review and security review from the prior CI run flagged this; the latest commit did not address it.

Suggested guard (after line 64, before resolved=""):

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

This converts a silent contract violation into a loud one at the API boundary, which is especially important now that the helper is being socialized to future consumers. Note that adding the guard to lib/parse-concern-value.sh will require re-syncing both plugin copies and bumping the consuming plugin versions.

Fix this →

Finding 2 — Manifest path derivation undocumented in sync script (still open)

File: scripts/sync-parse-concern-value.sh line 54

manifest="${copy%%/skills/*}/.claude-plugin/plugin.json"

This assumes every entry in copies[] follows the layout <plugin-root>/skills/<skill-name>/.... The assumption holds for both current entries. It is load-bearing but silent: a future consumer whose skill lives at a different depth (e.g. <plugin>/hooks/…) would compute a wrong manifest path and the --check-bump gate would silently pass for that plugin with no version bump required.

The copies array already documents itself with "Add a consumer by adding its path here." The derivation line deserves the same signal:

# Assumes each consumer path follows <plugin-root>/skills/<name>/…
manifest="${copy%%/skills/*}/.claude-plugin/plugin.json"

Fix this →

Verified clean

Area	Verdict
Parse order in `strip_value` (quote → comment → trim → slash)	Correct
`#` adjacent to non-space preserved in unquoted path (`.work/#topic`)	Correct — `[[:space:]]#.*$` pattern requires leading space
Absent concern file degrades to fallback, then empty	Correct — `-f` check gates the sed path
`orphan-rule-check.sh` delegation (line 68–69)	Correct — no fallback passed; caller owns the `.work` default
`session.md` two-step resolution (`DECLARED_SAVEPOINT:-` → `MEMORY_DIR:-.work`)	Correct — matches helper's resolution contract
Fallback passthrough not normalized (by design)	Documented in test line 77; callers must pass clean values
CI job wired into `ci-status` `needs:` (line 613)	Confirmed
`fetch-depth: 0` on `parse-concern-value-sync` for base-ref resolution	Correct
`github.base_ref` in `env:`, not interpolated into `run:`	Safe
`jq` dependency in `--check-bump`	Acceptable — available on `ubuntu-24.04`, consistent with hook-utils precedent
Regression test Case 9 (quoted `".scratch#dir"` preserved)	Directly pins the reported bug

Test coverage

19 unit tests in lib/parse-concern-value.test.sh and 14 integration tests in orphan-rule-check.test.sh cover the regression, the held behaviors, fallback resolution, and the absent-file path. Coverage is well-targeted and sufficient for the scope of this fix.

claude · 2026-07-20T20:38:56Z

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

Security Review — PR #756

Gather context and understand request
Read PR diff and changed files
Review lib/parse-concern-value.sh (new shared parser)
Review scripts/sync-parse-concern-value.sh (drift gate)
Review .github/workflows/ci.yml (new CI job)
Review plugin consumers and other changed files
Synthesize and post findings

Scope: all 13 changed files reviewed. No critical or important security findings. One suggestion carried forward from the prior automated security review, confirmed here.

Finding — `$key` interpolated into `sed` expression without runtime validation

Severity: SUGGESTION | Confidence: High

File: lib/parse-concern-value.sh, line 104

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

$key is double-quote interpolated directly into the sed substitution expression. The inline comment on line 103 documents that keys are [a-z_]+ identifiers with no metacharacters — but this is documentation, not enforcement. Every current caller passes the hardcoded literal memory_dir, so there is no exploitable path today. The risk is latent: when a future caller passes a dynamic or user-influenced key, a value containing / will silently corrupt the sed delimiter and produce wrong output instead of an error, and a value containing \ or & in sed's replacement context could produce unexpected results. Because the helper is explicitly designed to gain more consumers (per #474), the API boundary is real and worth sealing.

A guard at the top of the validated-args block closes this permanently and self-documents the constraint for callers:

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

Fix this →

Areas reviewed with no findings

Area	Result
Workflow trigger (`push` + `pull_request`, not `pull_request_target`)	Safe — fork PRs cannot access secrets
`github.base_ref` usage in new `parse-concern-value-sync` CI job (line 270)	Safe — isolated to `env:`, never directly interpolated in a `run:` block; matches the pattern used by every other bump-gate job
Workflow permissions (`contents: read` at workflow level)	Safe — no token widening introduced
`persist-credentials: false` on all checkouts in new job	Safe — matches repo convention
`$SCRIPT_DIR` resolution in `orphan-rule-check.sh`	Safe — derived from `BASH_SOURCE[0]`, not user-controlled input
`concern_file` path (`${repo_root}/.claude/topic-docs.yaml`)	Safe — constructed from `git rev-parse --show-toplevel`, a trusted source; sed reads only, no write path
`$memory_dir` in `git grep` pathspec (`orphan-rule-check.sh:91`)	Benign — advisory-only check (exits 0 always); pathspec is behind `--` so no option injection; worst case is incorrect file exclusion, not privilege escalation
`git show "$base:$manifest"` in `sync-parse-concern-value.sh` (line 57)	Safe — `$base` comes from `origin/$BASE_REF` where `BASE_REF` is set in `env:` from `github.base_ref`, not inline in `run:`; branch names are constrained by git
`jq` usage for version parsing in `--check-bump`	Safe — `jq` does not execute shell commands from its input
Action pins in new CI job	Safe — `actions/checkout` pinned to same SHA as all other jobs in the file
`ci-status` aggregate gates on `parse-concern-value-sync` (line 613)	Safe — new lane correctly wired into required-check aggregate

No objections to merging on security grounds. The suggestion above is optional hardening for a latent API-contract concern already flagged by the prior automated review — not a blocker.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 52ed9448e8

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

…am-parsing # Conflicts: # plugins/claude-memory/CHANGELOG.md

claude · 2026-07-20T21:26:16Z

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

Security Review — PR #756

Gather PR diff and identify changed files
Review lib/parse-concern-value.sh (new shared parser)
Review scripts/sync-parse-concern-value.sh (drift gate)
Review .github/workflows/ci.yml (new CI job)
Review plugin consumers (orphan-rule-check.sh, session.md)
Synthesize and post findings

Scope: all 13 changed files reviewed against origin/main. No critical or important security findings. One suggestion carried forward from three prior automated security review passes, confirmed here and still unaddressed.

Finding — `$key` interpolated into `sed` expression without runtime validation

Severity: SUGGESTION | Confidence: High

File: lib/parse-concern-value.sh, line 104

# sed anchors on the literal key; keys are `[a-z_]+` identifiers, no metachars.
raw_line=$(sed -n "s/^${key}:[[:space:]]*//p" "$concern_file" | head -1)

$key is interpolated directly into the sed substitution expression. The comment on line 103 documents the [a-z_]+ invariant, but nothing enforces it at runtime.

Practical impact today: none — every current caller passes the hardcoded literal memory_dir (orphan-rule-check.sh:68, session.md:52). No code execution path exists: GNU sed's e flag cannot be smuggled through $key interpolation alone because the template fixes the flag position to p. A key containing / causes sed delimiter confusion and produces wrong output rather than an error; \ or & in the key could corrupt the substitution result.

Latent risk: the helper is explicitly designed to gain more consumers (per #474). When a future caller passes a dynamic key, a value with sed metacharacters silently produces incorrect output instead of a loud failure. Three prior automated security reviews on this PR have flagged this; it remains unaddressed after four commits.

Suggested guard (after line 64, before resolved=""):

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

This enforces the documented invariant at the API boundary and self-documents the constraint for future callers. Adding the guard to lib/parse-concern-value.sh will require re-syncing both plugin copies and bumping the consuming plugin versions (enforced by the --check-bump gate in CI).

Fix this →

Areas reviewed with no findings

Area	Result
Workflow trigger (`push` + `pull_request`, not `pull_request_target`)	Safe — fork PRs cannot access secrets
`github.base_ref` in new `parse-concern-value-sync` job	Safe — isolated to `env:` as `BASE_REF`, never directly interpolated in a `run:` block; matches the pattern used by every other bump-gate job
Workflow permissions (`contents: read` at top level, no widening in new job)	Safe — no new permission grants introduced
`persist-credentials: false` on checkout in new job	Safe — matches repo convention
`$SCRIPT_DIR` resolution in `orphan-rule-check.sh`	Safe — derived from `BASH_SOURCE[0]`, not user input
`$concern_file` path construction (`${repo_root}/.claude/topic-docs.yaml`)	Safe — `repo_root` from `git rev-parse --show-toplevel`, a trusted source; sed reads only, no write or exec path
`$(concern_file)` content used in sed output — no `eval`, `source`, or exec sink	Safe — `strip_value` only applies string operations to the extracted line; file content cannot cause code execution
`$memory_dir` in `git grep` pathspec (`orphan-rule-check.sh:91`)	Benign — script is advisory-only (always exits 0); pathspec is behind `--`, so injection would produce git errors or incorrect exclusions, not privilege escalation
`git show "$base:$manifest"` in `sync-parse-concern-value.sh --check-bump`	Safe — `$base` is `origin/$BASE_REF` where `BASE_REF` comes from `env:`, not inline `run:` interpolation; passed as a git argument, not to a shell
`jq` usage for version parsing	Safe — `jq` does not execute shell commands from its input
Action pins in new CI job	Safe — `actions/checkout` pinned to same SHA (`9c091bb2...`) as all other jobs in the file
`ci-status` aggregate gates on `parse-concern-value-sync` (line 613)	Correct — new lane is fully wired into the required-check aggregate

No objections to merging on security grounds. The suggestion above is optional hardening for a latent API-contract concern — not a blocker. Three prior automated security reviews reached the same conclusion.

claude · 2026-07-20T21:26:17Z

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

PR Review — fix(claude-memory,session-flow): quote-aware topic-docs seam parser via shared helper

Gather context and understand request
Read lib/parse-concern-value.sh — check if prior findings addressed
Read scripts/sync-parse-concern-value.sh — check if prior findings addressed
Review latest commits and overall state
Synthesize and post final review

Latest commits

The branch now has three commits: the substantive fix (efa2414), a trivial changelog typo fix (52ed944), and a merge from origin/main (563e540) that brings in upstream changes unrelated to this fix. The merge commit touches no files changed by this PR.

Status of open findings

Two findings from prior review cycles remain open in the current HEAD (563e540).

Finding 1 — `$key` interpolated into `sed` without runtime guard (still open)

File: lib/parse-concern-value.sh line 104

# sed anchors on the literal key; keys are `[a-z_]+` identifiers, no metachars.
raw_line=$(sed -n "s/^${key}:[[:space:]]*//p" "$concern_file" | head -1)

The inline comment documents but does not enforce the [a-z_]+ constraint. All three review passes flagged this — PR review, security review, and the re-review after the docs-only 52ed944 commit. No guard has been added.

Impact today: zero — every current call site passes the hardcoded literal memory_dir. The concern is API surface for future consumers (explicitly planned per #474). A key containing / silently corrupts the sed delimiter and returns wrong output instead of an error.

Suggested guard (after line 64, before resolved=""):

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

Adding this requires re-syncing both plugin copies (they are byte-identical copies of the lib) and bumping both consuming plugin versions — the --check-bump CI gate will catch this if skipped.

Fix this →

Finding 2 — Manifest path derivation undocumented in sync script (still open)

File: scripts/sync-parse-concern-value.sh line 54

manifest="${copy%%/skills/*}/.claude-plugin/plugin.json"

This correctly derives the plugin root for both current entries but the %%/skills/* assumption — that every consumer path follows <plugin-root>/skills/<name>/… — is load-bearing and silent. A consumer whose skill lives at a different depth (e.g. <plugin>/hooks/…) would compute a wrong manifest path and the --check-bump gate would pass silently, skipping the version-bump requirement for that plugin. The copies array has an explicit "Add a consumer by adding its path here" comment; the derivation line deserves the same signal.

Suggested addition (one line above line 54):

# Assumes each consumer path follows <plugin-root>/skills/<name>/…

Fix this →

Weighing in on the Codex finding

File: plugins/session-flow/skills/retro/context/session.md lines 52–55

Codex flags that if DECLARED_SAVEPOINT is set to the handoff directory rather than the memory root, the snippet appends /handoffs a second time and silently misses the chain. Looking at the prose at line 49:

a save-point convention you inferred from CLAUDE.md / .claude/rules (rung 2 — pass it as DECLARED_SAVEPOINT; prose is an inference source, not a machine key)

The surrounding documentation uses "save-point" and "memory_dir" interchangeably in the broader handoff skill, and the PR description explicitly says fallback is "the caller-resolved rung-2 location" (i.e. the memory root, not the handoffs subdir). The contract is sound, but the inline comment doesn't make explicit that DECLARED_SAVEPOINT must be the memory root (not the handoffs/ subdirectory). An agent inferring from a CLAUDE.md that says "handoffs live in .claude/handoffs" could plausibly set DECLARED_SAVEPOINT=.claude/handoffs and get HANDOFF_DIR=.claude/handoffs/handoffs.

The fix is a one-word clarification in the comment — "the memory root you inferred" vs just "the save-point convention you inferred":

# rung 2: pass the MEMORY ROOT you inferred from CLAUDE.md / .claude/rules as
# DECLARED_SAVEPOINT (not the handoffs subdirectory — /handoffs is appended below)

This is lower urgency than Finding 1 but worth addressing given the ambiguity an agent would face when the consuming repo's CLAUDE.md describes the handoff location rather than the memory root.

Fix this →

Verified clean

Area	Verdict
Root bug: quote-resolution before comment-strip in `strip_value`	Fixed correctly
`#` adjacent to non-space preserved in unquoted path (`.work/#topic`)	Correct — `[[:space:]]#.*$` requires leading space
Absent concern file degrades to fallback, then empty	Correct — `-f` check gates the sed path
`orphan-rule-check.sh` delegation	Correct — caller owns the `.work` default
`session.md` two-step default (`DECLARED_SAVEPOINT:-` → `MEMORY_DIR:-.work`)	Correct resolution order
Regression test Case 9 (quoted `".scratch#dir"` preserved)	Directly pins reported bug
CI `parse-concern-value-sync` job wired into `ci-status`	Confirmed
Merge commit (`563e540`) touches none of the 13 changed files	Confirmed — upstream sync only

Overall verdict

The core fix is correct, well-tested, and structurally sound. All three open items are forward-looking quality concerns rather than bugs in the current fix:

Finding 1 (key guard) — optional hardening, zero live risk today, high value before claude-memory/session-flow/docs-hygiene: memory_dir/contract_dir config-value parsing is hand-rolled and copy-pasted across ≥4 files, not a shared helper #474 adds consumers
Finding 2 (manifest comment) — one-line documentation gap with no functional impact
Codex finding (DECLARED_SAVEPOINT clarity) — low-urgency wording improvement

The PR is ready to merge as-is if the team is comfortable deferring these to a follow-up. Addressing Finding 1 before adding new consumers (the planned #474 work) would be the natural forcing function.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 563e540cfd

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

…allback A comment-only YAML value (`memory_dir: # use default`, YAML-null) was taken literally by parse-concern-value.sh: the extraction strips the post-colon whitespace, so `raw` begins with `#`, and the unquoted comment strip only fired on a whitespace-*preceded* `#`. The value resolved to `# use default` instead of falling through to the caller fallback. Impact was fail-open in a check: orphan-rule-check stopped excluding the default `.work/` tier and could let a `.work` reference mask a real orphan; retro's handoff-chain search resolved a bogus `# use default/handoffs`. Treat a `#` that starts the unquoted value as a comment (YAML-null) so resolution degrades to the fallback / documented default. Synced to both plugin copies; added red-then-green regression cases. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HZqwoLTHPpPRzLpRGzbKRq

claude · 2026-07-20T23:38:51Z

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

PR Review — fix(claude-memory,session-flow): quote-aware topic-docs seam parser via shared helper

Gather context and understand request
Diff HEAD against origin/main to identify changes in latest commit
Review lib/parse-concern-value.sh — check if prior findings addressed
Review scripts/sync-parse-concern-value.sh — check if manifest comment added
Review plugins/session-flow/skills/retro/context/session.md — check DECLARED_SAVEPOINT clarity
Review plugin copies and changelogs for sync correctness
Synthesize findings and post review

Latest commit (`ba183d5`)

This commit addresses the Codex P2 finding (comment-only scalar treated as a literal directory) and is correct. The unquoted branch of strip_value now applies two patterns in a single sed call:

-    val="$(printf '%s' "$val" | sed 's/[[:space:]]#.*$//')"
+    val="$(printf '%s' "$val" | sed -e 's/^#.*$//' -e 's/[[:space:]]#.*$//')"

The s/^#.*$// expression correctly treats a # that starts the value as a comment (YAML-null), so memory_dir: # use default resolves to empty → fallback → .work, rather than keeping # use default as a literal path. The existing s/[[:space:]]#.*$// expression is preserved for trailing inline comments on real values. The two patterns compose correctly: a value that is # comment hits the first; path # comment hits the second; path#embedded hits neither.

Two regression tests added (lines 76–79 in lib/parse-concern-value.test.sh) pin both behaviors:

comment-only with a fallback → fallback
comment-only with no fallback → empty

Both plugin copies were synced and verified byte-identical (diff clean). Both plugin versions were bumped (claude-memory 0.2.3, session-flow 0.10.4). Changelogs were updated with clear descriptions of both the original seam fix and the comment-only follow-up.

Open findings (three prior review cycles, still unaddressed)

Finding 1 — `$key` interpolated into `sed` without runtime guard

File: lib/parse-concern-value.sh line 106 (and both plugin copies)

# sed anchors on the literal key; keys are `[a-z_]+` identifiers, no metachars.
raw_line=$(sed -n "s/^${key}:[[:space:]]*//p" "$concern_file" | head -1)

The inline comment documents the [a-z_]+ invariant; no runtime guard enforces it. Live risk today: zero — every caller passes the hardcoded literal memory_dir. The concern is forward-looking: the helper is explicitly designed to gain more consumers (per #474), and a caller passing a key containing / would silently corrupt the sed delimiter and produce wrong output instead of a loud error.

Suggested guard (after line 64, before resolved=""):

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

This converts a silent contract violation into a load error at the API boundary, which is especially important now that the helper is being socialized to future consumers. Adding the guard requires re-syncing both plugin copies and bumping versions — the --check-bump CI gate enforces this.

Fix this →

Finding 2 — Manifest path derivation undocumented in sync script

File: scripts/sync-parse-concern-value.sh line 54

manifest="${copy%%/skills/*}/.claude-plugin/plugin.json"

This %%/skills/* strip assumes every consumer path follows <plugin-root>/skills/<name>/…. The assumption is correct for both current entries, but it's load-bearing and silent: a future consumer at <plugin>/hooks/… or a flat layout would compute a wrong manifest path and the --check-bump gate would silently pass without requiring a version bump. The copies array already has an "Add a consumer by adding its path here" comment; this derivation line should carry the same signal.

Suggested comment (one line above line 54):

# Assumes each consumer path follows <plugin-root>/skills/<name>/…

Fix this →

Finding 3 — `DECLARED_SAVEPOINT` must be the memory root, not the handoffs subdirectory

File: plugins/session-flow/skills/retro/context/session.md lines 48–55

# concern-file memory_dir key -> a save-point convention you inferred from
# CLAUDE.md / .claude/rules (rung 2 — pass it as DECLARED_SAVEPOINT; prose is an
# inference source, not a machine key) -> the plugin default .work.
MEMORY_DIR=$(bash "${CLAUDE_PLUGIN_ROOT}/skills/retro/scripts/parse-concern-value.sh" \
  .claude/topic-docs.yaml memory_dir "${DECLARED_SAVEPOINT:-}")
MEMORY_DIR="${MEMORY_DIR:-.work}"
HANDOFF_DIR="$MEMORY_DIR/handoffs"

DECLARED_SAVEPOINT is used as the memory root, and /handoffs is appended on line 55. The comment describes it only as "a save-point convention you inferred" — an agent whose consuming repo's CLAUDE.md describes the handoff location (e.g., "handoffs live in .claude/handoffs") could plausibly set DECLARED_SAVEPOINT=.claude/handoffs, producing HANDOFF_DIR=.claude/handoffs/handoffs and silently missing the chain. A one-word clarification in the comment closes the ambiguity:

# concern-file memory_dir key -> the MEMORY ROOT you inferred from CLAUDE.md /
# .claude/rules (rung 2 — pass as DECLARED_SAVEPOINT; /handoffs is appended below,
# so pass the root, not the handoffs subdirectory itself) -> the plugin default .work.

Fix this →

Verified clean

Area	Verdict
Comment-only value (`memory_dir: # use default`) now falls to fallback	Fixed in `ba183d5` — `s/^#.*$//` applied before space-preceded strip
Quoted `a#b` and `.work/#topic` still preserve `#`	Correct — neither new pattern touches non-leading, non-space-preceded `#`
`strip_value` parse order (quote → comment → trim → slash)	Correct and unchanged
Both plugin copies byte-identical to `lib/parse-concern-value.sh`	Verified (`diff` clean)
Plugin versions bumped (`claude-memory` → 0.2.3, `session-flow` → 0.10.4)	Confirmed
Regression tests added for comment-only behavior (lines 76–79)	Confirmed; correctly pin the new behavior
`ci-status` gates on `parse-concern-value-sync`	Confirmed (CI job wired in; prior review verified)

Overall verdict

The core fix (quote-aware comment strip) is correct, the Codex P2 regression (comment-only scalar) is correctly addressed in ba183d5, and all sync/drift/CI machinery is in order. The three remaining open findings are forward-looking quality concerns — not bugs in the current code:

Finding 1 (key guard) is zero-risk today; becomes the natural claude-memory/session-flow/docs-hygiene: memory_dir/contract_dir config-value parsing is hand-rolled and copy-pasted across ≥4 files, not a shared helper #474 forcing function
Finding 2 (manifest comment) is one documentation line; no functional impact
Finding 3 (DECLARED_SAVEPOINT clarity) is a wording gap in agent-facing prose

The PR is mergeable as-is. Addressing Finding 1 before new consumers land in #474 remains the recommended path.

claude · 2026-07-20T23:38:54Z

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

Security Review — PR #756

Gather PR diff and identify changed files
Review lib/parse-concern-value.sh (new shared parser)
Review scripts/sync-parse-concern-value.sh (drift gate)
Review .github/workflows/ci.yml (new CI job)
Review plugin consumers (orphan-rule-check.sh, session.md)
Synthesize and post findings

Scope: all 13 changed files reviewed, including the latest commit (ba183d5) which addressed the Codex comment-only YAML finding. No critical or important security findings. One suggestion carried forward from four prior automated security review passes; no new issues introduced by the latest commit.

Finding — `$key` interpolated into `sed` expression without runtime validation

Severity: SUGGESTION | Confidence: High

File: lib/parse-concern-value.sh, line 106

# sed anchors on the literal key; keys are `[a-z_]+` identifiers, no metachars.
raw_line=$(sed -n "s/^${key}:[[:space:]]*//p" "$concern_file" | head -1)

$key is interpolated directly into the sed substitution expression. The comment on line 105 documents the [a-z_]+ invariant but nothing enforces it at runtime.

Practical impact today: none. Every current call site passes the hardcoded literal memory_dir. GNU sed's e flag (which would execute the replacement as shell code) cannot be smuggled through $key alone because the template fixes the flags position to p. A key containing / causes sed delimiter confusion and produces wrong output rather than an error; \ or & in the key could corrupt the substitution result.

Latent risk: the helper is explicitly designed to gain more consumers (per #474). When a future caller passes a dynamic key, a value with sed metacharacters silently produces incorrect output instead of a loud failure. Four prior automated security reviews on this PR have flagged this; the latest commit (ba183d5) resolved the Codex comment-only finding but did not add the guard.

Suggested guard (after line 64, before resolved=""):

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

This enforces the documented invariant at the API boundary. Adding the guard to lib/parse-concern-value.sh requires re-syncing both plugin copies and bumping the consuming plugin versions — the --check-bump CI gate will catch this if skipped.

Fix this →

Latest commit (`ba183d5`) — security assessment

The latest commit correctly resolves the Codex finding: adding -e 's/^#.*$//' as a first pass in the unquoted branch of strip_value() treats a leading # as a YAML-null comment, so memory_dir: # use default degrades to the caller's fallback rather than resolving to the literal string # use default. The fix is applied inside printf '%s' "$val" | sed ... — $val at that point is the raw extracted line from the concern file; no exec surface exists in the sed pipeline (no e flag, no exec sink).

Areas reviewed with no findings

Area	Result
Workflow trigger (`push` + `pull_request`, not `pull_request_target`)	Safe — fork PRs cannot access secrets
`github.base_ref` in `parse-concern-value-sync` job	Safe — isolated to `env:` as `BASE_REF`, never directly interpolated in a `run:` block; matches the pattern used by every other bump-gate job
Workflow permissions (`contents: read` at top level, no widening in new job)	Safe
`persist-credentials: false` on checkout in new job	Safe — matches repo convention
`$SCRIPT_DIR` resolution in `orphan-rule-check.sh`	Safe — derived from `BASH_SOURCE[0]`, not user-controlled input
`concern_file` path construction (`${repo_root}/.claude/topic-docs.yaml`)	Safe — `repo_root` from `git rev-parse --show-toplevel`, a trusted source; read-only sed, no write or exec path
`strip_value()` sed subshell on line 94 (`ba183d5` two-expression form)	Safe — `printf '%s' "$val"` pipes to sed with no `e` flag or exec sink; file content cannot cause code execution
`$memory_dir` in `git grep` pathspec (`orphan-rule-check.sh:91`)	Benign — pathspec behind `--`, script always exits 0; worst case is wrong exclusion, not privilege escalation
`git show "$base:$manifest"` in `sync-parse-concern-value.sh --check-bump` (line 57)	Safe — `$base` is `origin/$BASE_REF` where `BASE_REF` comes from `env:`, not inline `run:` interpolation
`jq` usage for version parsing (line 57)	Safe — `jq` does not execute shell commands from its input
Action pins in new CI job	Safe — `actions/checkout@9c091bb2...` pinned to same SHA as all other jobs
`ci-status` aggregate gates on `parse-concern-value-sync` (line 613)	Correct — new lane fully wired into the required-check aggregate

No objections to merging on security grounds. The suggestion above is optional hardening for a latent API-contract concern — not a blocker. Four prior automated security reviews reached the same conclusion.

…o shared parse-concern-value helper (#847) ## 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 #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 - #482 / #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. - #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. - #470 / #469 / #432 / #365 / #369 — the broader tracker-seam-resolution class; explicitly excluded from this PR's scope per the operator's ratified decision. ## Test plan - [x] `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. - [x] 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). - [x] `scripts/sync-parse-concern-value.sh --check` — all 3 plugin copies match `lib/parse-concern-value.sh`. - [x] `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). - [x] `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. - [x] `shellcheck` + `shfmt -d` — clean on all new/edited scripts. - [x] `scripts/validate-plugins.sh` — all manifests + catalog valid. Closes #474 --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

kyle-sexton added the automated Opened by automation. label Jul 20, 2026

kyle-sexton mentioned this pull request Jul 20, 2026

ops: loop-lane telemetry — running per-lane status (babysit / triage / work) #502

Open

docs(claude-memory): fix MD038 space-in-code-span in changelog entry

52ed944

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

kyle-sexton marked this pull request as ready for review July 20, 2026 20:42

chatgpt-codex-connector Bot reviewed Jul 20, 2026

View reviewed changes

Comment thread plugins/session-flow/skills/retro/context/session.md

Merge remote-tracking branch 'origin/main' into fix/482-memory-dir-se…

563e540

…am-parsing # Conflicts: # plugins/claude-memory/CHANGELOG.md

chatgpt-codex-connector Bot reviewed Jul 20, 2026

View reviewed changes

Comment thread lib/parse-concern-value.sh Outdated

kyle-sexton mentioned this pull request Jul 20, 2026

lane sweep: triage — week of 7/20 (rolling paper-cut batch) #657

Open

77 tasks

kyle-sexton merged commit 7f92b14 into main Jul 20, 2026
22 checks passed

kyle-sexton deleted the fix/482-memory-dir-seam-parsing branch July 20, 2026 23:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(claude-memory,session-flow): quote-aware topic-docs seam parser via shared helper#756

fix(claude-memory,session-flow): quote-aware topic-docs seam parser via shared helper#756
kyle-sexton merged 4 commits into
mainfrom
fix/482-memory-dir-seam-parsing

kyle-sexton commented Jul 20, 2026

Uh oh!

claude Bot commented Jul 20, 2026 •

edited

Loading

Uh oh!

claude Bot commented Jul 20, 2026 •

edited

Loading

Uh oh!

claude Bot commented Jul 20, 2026 •

edited

Loading

Uh oh!

claude Bot commented Jul 20, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

claude Bot commented Jul 20, 2026 •

edited

Loading

Uh oh!

claude Bot commented Jul 20, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

claude Bot commented Jul 20, 2026 •

edited

Loading

Uh oh!

claude Bot commented Jul 20, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

kyle-sexton commented Jul 20, 2026

Summary

Fix

Verification

Related

Uh oh!

claude Bot commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review — fix(claude-memory,session-flow): quote-aware topic-docs seam parser via shared helper

Summary

Finding 1 — sed key injection: no validation on the $key argument

Finding 2 — --check-bump manifest path derivation is undocumented and fragile

Minor notes (no action required)

Test coverage verdict

Uh oh!

claude Bot commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security Review — PR #756

Findings

SUGGESTION — Unvalidated key parameter interpolated into sed expression (CONFIDENCE: High)

Areas reviewed with no findings

Uh oh!

claude Bot commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review — fix(claude-memory,session-flow): quote-aware topic-docs seam parser via shared helper

Latest commit (52ed944)

Overall verdict

Finding 1 — $key interpolated into sed without runtime guard (still open)

Finding 2 — Manifest path derivation undocumented in sync script (still open)

Verified clean

Test coverage

Uh oh!

claude Bot commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security Review — PR #756

Finding — $key interpolated into sed expression without runtime validation

Areas reviewed with no findings

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

claude Bot commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security Review — PR #756

Finding — $key interpolated into sed expression without runtime validation

Areas reviewed with no findings

Uh oh!

claude Bot commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review — fix(claude-memory,session-flow): quote-aware topic-docs seam parser via shared helper

Latest commits

Status of open findings

Finding 1 — $key interpolated into sed without runtime guard (still open)

Finding 2 — Manifest path derivation undocumented in sync script (still open)

Weighing in on the Codex finding

Verified clean

Overall verdict

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

claude Bot commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review — fix(claude-memory,session-flow): quote-aware topic-docs seam parser via shared helper

Latest commit (ba183d5)

Open findings (three prior review cycles, still unaddressed)

Finding 1 — $key interpolated into sed without runtime guard

Finding 2 — Manifest path derivation undocumented in sync script

Finding 3 — DECLARED_SAVEPOINT must be the memory root, not the handoffs subdirectory

Verified clean

Overall verdict

Uh oh!

claude Bot commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

claude Bot commented Jul 20, 2026 •

edited

Loading

Finding 1 — `sed` key injection: no validation on the `$key` argument

Finding 2 — `--check-bump` manifest path derivation is undocumented and fragile

claude Bot commented Jul 20, 2026 •

edited

Loading

SUGGESTION — Unvalidated `key` parameter interpolated into `sed` expression (CONFIDENCE: High)

claude Bot commented Jul 20, 2026 •

edited

Loading

Latest commit (`52ed944`)

Finding 1 — `$key` interpolated into `sed` without runtime guard (still open)

claude Bot commented Jul 20, 2026 •

edited

Loading

Finding — `$key` interpolated into `sed` expression without runtime validation

claude Bot commented Jul 20, 2026 •

edited

Loading

Finding — `$key` interpolated into `sed` expression without runtime validation

claude Bot commented Jul 20, 2026 •

edited

Loading

Finding 1 — `$key` interpolated into `sed` without runtime guard (still open)

claude Bot commented Jul 20, 2026 •

edited

Loading

Latest commit (`ba183d5`)

Finding 1 — `$key` interpolated into `sed` without runtime guard

Finding 3 — `DECLARED_SAVEPOINT` must be the memory root, not the handoffs subdirectory

claude Bot commented Jul 20, 2026 •

edited

Loading

Finding — `$key` interpolated into `sed` expression without runtime validation

Latest commit (`ba183d5`) — security assessment