feat(source-control): add machine-readable commit-convention enforcement seam#925
Conversation
|
Claude finished @kyle-sexton's task in 3m 12s —— View job PR Review:
|
|
Claude finished @kyle-sexton's task in 3m 0s —— View job Security Review — PR #925
SummaryNo critical or important security findings. The PR follows correct security patterns throughout: SHA-pinned actions, FindingsSUGGESTION — Incomplete PCRE detection in
|
| Pattern | Verdict |
|---|---|
pull_request (not pull_request_target) |
✓ safe trigger |
${{ github.base_ref }} via env: indirection, not direct run: interpolation |
✓ no script injection |
| All action refs SHA-pinned | ✓ |
persist-credentials: false on checkout |
✓ |
repo_root + key never eval'd or used as a command |
✓ |
| User-controlled file content flows through awk/sed data paths only | ✓ |
normalize_ere pipes to sed stdin (not as a sed script) |
✓ |
copies array hardcoded in sync script (not user-controllable) |
✓ |
origin/ prefix on $BASE_REF in git commands prevents option injection |
✓ |
set -uo pipefail throughout |
✓ |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e271002ac0
ℹ️ 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".
e271002 to
934e787
Compare
|
Claude finished @kyle-sexton's task in 3m 58s —— View job PR Review:
|
| Construct | Example | Risk |
|---|---|---|
| Inline flags | (?i:feat|fix) |
Not detected; (?i: → (i: after (?:→( normalization — silently wrong |
| Mode modifiers | (?i)^feat: .+ |
Not detected; emitted as-is, invalid ERE |
| Atomic group | (?>feat|fix) |
Not detected; emitted as-is, invalid ERE |
| PCRE comment | (?#type)^feat |
Not detected; emitted as-is |
The (?i: case is the most dangerous because normalize_ere transforms it: (?i:feat|fix) → (i:feat|fix). This is valid ERE syntax (a non-special group with a literal i: prefix), so it compiles without error but enforces the wrong pattern — i:feat would be required rather than case-insensitive feat. This is worse than fail-open; it's mis-enforcement.
Suggested additions to is_non_ere:
# PCRE inline flags — (?i, (?m, (?s, (?x — no ERE equivalent
[[ "$p" == *'(?i'* || "$p" == *'(?m'* || "$p" == *'(?s'* || "$p" == *'(?x'* ]] && return 0
# Atomic group, PCRE comment
[[ "$p" == *'(?>'* || "$p" == *'(?#'* ]] && return 04. Unnecessary shellcheck disable=SC2016 — misleading
File: lib/resolve-convention-pattern.sh:51
SC2016 warns about backticks inside double-quoted strings. PR_DEFERRAL uses single quotes; SC2016 would never fire here. The disable directive has no effect and misleads readers.
5. Dangling — see note in newrepo helper comment — minor
File: lib/resolve-convention-pattern.test.sh:36–37
# newrepo "<team body>" [local-body] [does NOT create user-global — see note]There is no "note" in the file. The reason is clear from context (enforcement is team-tracked-only so testing a user-global overlay is purposeless), but the dangling forward-reference will confuse a future editor. Inline the explanation:
# newrepo "<team body>" [local-body] (no user-global: enforcement reads team-tracked layer only)PR description accuracy note
The PR claims "19/19" tests pass. grep -c 'assert_eq\|assert_exit' returns 21 because two of those matches are the function definitions themselves — the actual test count is 19 as stated. No issue, just confirming the count is correct.
What's solid
- Policy-floor is correctly enforced at the layer boundary:
h2_valuereadsTEAM_FILEonly; local and user-global overlays are never consulted by the resolver. The test at line 100–102 explicitly verifies this invariant. is_non_ereruns beforenormalize_ere: prevents a PCRE construct from being mangled into accidentally-valid ERE that enforces the wrong thing. The ordering is load-bearing.- CC keyword → one canonical ERE:
raw_valueowns the expansion in one place; no per-hook re-definition possible. - Unresolved →
exit 1, stdout empty: the[[ -n "$value" ]] || exit 1guard at line 144 is clean and covers both absent-file and absent-key paths. - CI job is correctly wired: SHA-pinned checkout,
persist-credentials: false,BASE_REFvia env var (no script-injection),fetch-depth: 0for bump checking, correct placement inci-status.needs. - Empty-array idiom:
${copies[@]+"${copies[@]}"}throughout — correct underset -uwhen the array is intentionally empty. sync-resolve-convention-pattern.shstructure: The intentionally-empty copies list with a commented placeholder for feat(guardrails): CC-layer content gate — commit subject + PR title #914 is the right scaffold: no premature copies to drift before the consumers exist.
|
Claude finished @kyle-sexton's task in 4m 20s —— View job Security Review — PR #925
SummaryNo CRITICAL or IMPORTANT findings. The architecture is sound: FindingsSUGGESTION — Incomplete PCRE detection in
|
| Pattern | Verdict |
|---|---|
pull_request trigger (not pull_request_target) |
✓ |
${{ github.base_ref }} via env: indirection, not direct run: interpolation |
✓ |
"origin/$BASE_REF" in git diff/git show — prefixed, no option injection |
✓ |
All action refs SHA-pinned (actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0) |
✓ |
persist-credentials: false on checkout |
✓ |
copies array hardcoded in sync script (not user-controllable) |
✓ |
key validated to subject_pattern|pr_title_pattern before awk -v key="$k" |
✓ |
normalize_ere pipes to sed stdin (not as a sed script) |
✓ |
h2_value uses [[ -f "$file" ]] || return 0 before awk |
✓ |
set -uo pipefail throughout; set -euo pipefail in sync script |
✓ |
| No secrets, tokens, or credentials referenced | ✓ |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 934e787807
ℹ️ 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".
934e787 to
337c83f
Compare
|
Claude finished @kyle-sexton's task in 4m 6s —— View job PR Review:
|
| Construct | Example | Outcome |
|---|---|---|
| Inline flag group | (?i:feat|fix) |
Not detected; normalize_ere sees (?i: ≠ (?: so leaves it intact; resolver exits 0 with invalid ERE |
| Mode modifier | (?i)^feat: .+ |
Same — not detected, emitted as-is |
| Atomic group | (?>feat|fix) |
Same |
| PCRE comment | (?#type)^feat |
Same |
The result is the resolver exits 0 (signalling "enforce this"), but the emitted pattern is invalid ERE. A consuming hook that does [[ $commit_msg =~ $pattern ]] or grep -E "$pattern" gets exit status 2 — an unhandled error path that may silently fail-open on all commits, bypassing the gate with no diagnostic.
Fix: Extend is_non_ere before the final return 1:
# PCRE inline flags and atomic group/comment — no ERE equivalent
[[ "$p" == *'(?i'* || "$p" == *'(?m'* || "$p" == *'(?s'* || "$p" == *'(?x'* ]] && return 0
[[ "$p" == *'(?>'* || "$p" == *'(?#'* ]] && return 02. normalize_ere produces wrong ERE for shorthand escapes inside bracket expressions
File: lib/resolve-convention-pattern.sh:116
p="$(printf '%s' "$p" | sed -e 's/\\d/[0-9]/g' -e 's/\\w/[[:alnum:]_]/g' -e 's/\\s/[[:space:]]/g')"The substitutions are global and have no bracket-expression awareness. A common team pattern like ^[\w-]+$ becomes ^[[[:alnum:]_]-]+$. In POSIX ERE, the outer [ opens the bracket expression and the inner [[:alnum:]_] is parsed as a POSIX class inside it — but the leading [ of [[:alnum:]_] is treated as a literal [ character added to the class, making the pattern match [, a-z, A-Z, 0-9, _, and - instead of just the intended word characters and hyphen. This is a silent semantic difference: the resolver exits 0 with a structurally valid but semantically wrong ERE.
The same issue applies to \d inside brackets ([\d-] → [[0-9]-]) and \s.
The fix requires either bracket-context tracking (complex) or a simpler approach: if \w, \d, or \s appears within a bracket expression […], reject it as non-enforceable and emit a diagnostic. For the common case where these escapes appear outside brackets, the current normalization is correct.
Important
3. No ERE compile-time validation before emitting the pattern
File: lib/resolve-convention-pattern.sh:151
The resolver's documented contract is: exit 0 → the pattern is a valid ERE ready for grep -E / [[ =~ ]]. But a consumer pattern that passes is_non_ere (e.g., unbalanced bracket ^[unclosed, unrecognized escape \p{Lu}, or a pattern stranded by the bracket-context issue in finding #2) will be emitted with exit 0 without any compile probe. Downstream hooks will then receive exit status 2 from the regex engine — behavior that isn't covered by the current test suite.
A zero-overhead guard before the final printf:
printf '' | grep -qE "$normalized" 2>/dev/null || {
printf 'resolve-convention-pattern: %s normalized to an invalid ERE; enforcement disabled.\n' "$key" >&2
exit 1
}This makes "exit 0 = valid ERE" a guarantee rather than a best-effort claim.
Minor
4. Raw GitHub URL in config-resolution.md — serves Markdown source, not rendered page
File: plugins/source-control/reference/config-resolution.md:94
[commit-convention enforcement seam](https://raw.githubusercontent.com/melodic-software/claude-code-plugins/main/docs/conventions/commit-convention/README.md)raw.githubusercontent.com serves Markdown source text, not the rendered view. It also hardcodes main. A relative path works correctly in GitHub's rendered view and survives branch renames:
[commit-convention enforcement seam](../../../docs/conventions/commit-convention/README.md)(The file is at plugins/source-control/reference/config-resolution.md; three ../ levels up reaches docs/conventions/commit-convention/README.md.)
5. Unnecessary shellcheck disable=SC2016 — misleads future readers
File: lib/resolve-convention-pattern.sh:51
SC2016 warns about backticks inside double-quoted strings. PR_DEFERRAL uses single quotes ('...'), so ShellCheck never triggers SC2016 here. The disable directive has no effect and misleads a reader into thinking suppression was required.
6. Dangling — see note in test helper comment
File: lib/resolve-convention-pattern.test.sh:36
# newrepo "<team body>" [local-body] [does NOT create user-global — see note]There is no "note" in this file. Inline the reason:
# newrepo "<team body>" [local-body] (no user-global: enforcement reads team-tracked layer only)7. Missing test: CC keyword + pr_title_pattern deferral exercised together
File: lib/resolve-convention-pattern.test.sh:104–106
The existing deferral test uses a custom ERE for subject_pattern. The path where pr_title_pattern defers to a Conventional Commits keyword subject_pattern (exercising CC expansion through the deferral branch) is not covered. Since this is the combination most likely to appear in real consumer configs, a dedicated case would close the gap before #914/#919 vendor the resolver.
What's solid
- Policy-floor is correctly enforced at the layer boundary.
h2_valuereadsTEAM_FILEonly; the test at line 100–102 explicitly verifies local overlays can never enforce. is_non_ereruns beforenormalize_ere. The ordering prevents lookaround / backreference /\bfrom being mangled into accidentally-valid ERE.- CC keyword → one canonical ERE in one place.
raw_valueowns the expansion; no per-hook re-definition possible. - Unresolved → exit 1, empty stdout. The
[[ -n "$value" ]] || exit 1guard covers both absent-file and absent-key. ${copies[@]+"${copies[@]}"}empty-array idiom throughout the sync script — correct underset -u.- CI job structure mirrors
parse-concern-value-syncexactly: SHA-pinned checkout,persist-credentials: false,BASE_REFvia env var,fetch-depth: 0, correctly wired intoci-status.needs.
|
Claude finished @kyle-sexton's task in 3m 18s —— View job Security Review — PR #925
SummaryNo CRITICAL findings. Two IMPORTANT issues and two SUGGESTION-level issues. Both IMPORTANT findings affect enforcement correctness — one is new to this review, one was raised in prior reviews and remains unaddressed. Both can produce silent mis-enforcement (an incorrect pattern emitted with exit 0 and no diagnostic), which is more dangerous than the fail-open case. FindingsIMPORTANT — Negated PCRE shorthand classes (
|
| Construct | Example | ERE behavior |
|---|---|---|
| Inline flag group | (?i:feat|fix) |
Not caught; emitted as-is; invalid ERE → consuming hook errors |
| Mode modifier | (?i)^feat: .+ |
Not caught; emitted as-is; invalid ERE |
| Atomic group | (?>feat|fix) |
Not caught; emitted as-is; invalid ERE |
| PCRE comment | (?#type)^feat |
Not caught; emitted as-is; invalid ERE |
Scenario (worst case): Team sets subject_pattern: (?i:feat|fix): .+ expecting case-insensitive enforcement. is_non_ere returns 1 (not a non-ERE construct). normalize_ere does not transform (?i: — the (?: → ( substitution at line 115 looks for exactly (?: (three chars (, ?, :) and (?i: doesn't match. The resolver emits (?i:feat|fix): .+ with exit 0. A consuming hook hands this to grep -E or [[ =~ ]]; both reject it with exit status 2 (invalid ERE). If the hook treats any non-zero as pass, every commit clears the gate silently.
Add to is_non_ere:
# PCRE inline flags, atomic group, comment — no POSIX ERE equivalent
[[ "$p" == *'(?i'* || "$p" == *'(?m'* || "$p" == *'(?s'* || "$p" == *'(?x'* ]] && return 0
[[ "$p" == *'(?>'* || "$p" == *'(?#'* ]] && return 0(This finding was raised in the two prior reviews on commits e271002 and 934e787 and is still unaddressed in HEAD 337c83f.)
SUGGESTION — normalize_ere substitutions are context-blind inside bracket expressions | confidence: medium
File: lib/resolve-convention-pattern.sh L116
The \w/\d/\s → POSIX class substitutions run globally, including inside bracket expressions. [\w-]+ → [[[:alnum:]_]-]+. The outer [ and the inner [ that begins [:alnum:] form a nested bracket expression; GNU grep ERE accepts this but parses it differently from the intended word-character class, and BSD regex (bash on macOS) may reject it with exit status 2, failing-open or failing-closed depending on the consuming hook. The failure is silent — exit 0 was already returned.
(Also raised in both prior reviews, still unaddressed.)
SUGGESTION — No compile-time ERE validation before emitting with exit 0 | confidence: medium
File: lib/resolve-convention-pattern.sh L151
printf '%s\n' "$(normalize_ere "$value")"
exit 0Exit 0 means "enforce this." Any construct that evades the string-match detection in is_non_ere (a new PCRE feature, an unbalanced bracket, \p{Lu}) is emitted with exit 0 and no validation that the result is actually compilable by grep -E or bash's [[ =~ ]]. A one-line probe before emit would make exit 0 a guarantee rather than a claim:
normalized="$(normalize_ere "$value")"
if ! printf '' | grep -qE "$normalized" 2>/dev/null; then
printf 'resolve-convention-pattern: %s resolved to an invalid ERE; enforcement disabled.\n' "$key" >&2
exit 1
fi
printf '%s\n' "$normalized"(Also raised in both prior reviews, still unaddressed.)
Confirmed clean
| Pattern | Verdict |
|---|---|
pull_request trigger (not pull_request_target) |
✓ |
${{ github.base_ref }} via env: indirection, not direct run: interpolation |
✓ |
"origin/$BASE_REF" in git diff / git show — prefixed, no option injection |
✓ |
actions/checkout@9c091bb… SHA-pinned |
✓ |
persist-credentials: false |
✓ |
copies array hardcoded in sync script (not user-controllable) |
✓ |
key validated to subject_pattern|pr_title_pattern before awk -v key= |
✓ |
normalize_ere pipes to sed stdin (not as a sed script) |
✓ |
h2_value checks [[ -f "$file" ]] before awk |
✓ |
set -uo pipefail throughout; set -euo pipefail in sync script |
✓ |
| No secrets, tokens, or credentials referenced | ✓ |
| New CI job mirrors established SHA-pinned pattern | ✓ |
337c83f to
75a97f5
Compare
|
Claude finished @kyle-sexton's task in 4m 19s —— View job PR Review:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75a97f535b
ℹ️ 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 finished @kyle-sexton's task in 3m 8s —— View job Security Review — PR #925
SummaryNo CRITICAL findings. Two IMPORTANT issues and three SUGGESTION-level issues. All five affect enforcement correctness: a pattern that should be rejected is instead emitted with exit 0, causing the consuming hook to either silently mis-enforce the wrong rule or fail-open with no diagnostic. Neither IMPORTANT finding was introduced in this commit — both were carried forward from prior pushes to this branch and all three prior Claude security reviews flagged them. They remain unaddressed at HEAD FindingsIMPORTANT — Negated PCRE shorthand classes (
|
| Construct | Example | Behavior when emitted |
|---|---|---|
| Inline flag group | (?i:feat|fix) |
normalize_ere does not match (?i: (only matches (?: exactly); emitted as-is; invalid ERE |
| Mode modifier | (?i)^feat: .+ |
Not detected; emitted as-is; invalid ERE |
| Atomic group | (?>feat|fix) |
Not detected; emitted as-is; invalid ERE |
| PCRE comment | (?#type)^feat |
Not detected; emitted as-is; invalid ERE |
Worst-case scenario: Team sets subject_pattern: (?i:feat|fix): .+ expecting case-insensitive enforcement. is_non_ere returns 1; normalize_ere changes (?: → ( but (?i: starts with (?i, not (?:, so the pattern is not transformed and is emitted as (?i:feat|fix): .+ with exit 0. A consuming hook passes this to grep -E or [[ =~ ]]. GNU grep returns exit 2 (invalid regex); bash [[ =~ ]] errors similarly. If the hook interprets any non-zero as "no match" and passes the commit, the gate is silently bypassed with no diagnostic to the consumer.
# Add to is_non_ere:
[[ "$p" == *'(?i'* || "$p" == *'(?m'* || "$p" == *'(?s'* || "$p" == *'(?x'* ]] && return 0
[[ "$p" == *'(?>'* || "$p" == *'(?#'* ]] && return 0(Raised in all three prior Claude security reviews at e271002, 934e787, and 337c83f; unaddressed at HEAD 75a97f5.)
SUGGESTION — PCRE absolute anchors \A, \z, \Z silently enforce against literal characters | confidence: medium
File: lib/resolve-convention-pattern.sh L103–L118
\A (PCRE start-of-string), \z (PCRE end-of-string, no trailing newline), and \Z (PCRE end-of-string, tolerates trailing newline) are not detected by is_non_ere and not normalized by normalize_ere. POSIX ERE has no equivalent anchors; both GNU grep and bash [[ =~ ]] treat them as the literal characters A, z, and Z.
Scenario: A consumer writes \Afeat: .+\z. is_non_ere returns 1; normalize_ere leaves the escapes unchanged; the resolver emits \Afeat: .+\z with exit 0. The consuming hook enforces this against commit subjects: it now requires a subject that contains Afeat: followed by any characters ending in z — a semantically wrong gate with no diagnostic.
# Add to is_non_ere:
[[ "$p" == *'\A'* || "$p" == *'\z'* || "$p" == *'\Z'* ]] && return 0SUGGESTION — normalize_ere substitutions are context-blind inside bracket expressions | confidence: medium
File: lib/resolve-convention-pattern.sh L116
The \w/\d/\s substitutions run globally and do not track whether the match site is already inside a bracket expression. [\w-]+ becomes [[[:alnum:]_]-]+: the outer [ and the [ that opens [:alnum:] nest, which GNU grep ERE accepts but interprets differently from the intended character class. Bash [[ =~ ]] on macOS (BSD regex) may reject it with exit status 2. Either failure is silent — exit 0 was already returned.
(Raised in all three prior Claude security reviews; unaddressed at HEAD 75a97f5.)
SUGGESTION — No compile-time ERE validation before emitting with exit 0 | confidence: medium
File: lib/resolve-convention-pattern.sh L151
printf '%s\n' "$(normalize_ere "$value")"
exit 0Exit 0 means "enforce this." Any construct that evades is_non_ere's string-match detection is emitted with exit 0 and no validation that the result compiles as a valid ERE. A one-line probe before emit would make exit 0 a guarantee:
normalized="$(normalize_ere "$value")"
if ! printf '' | grep -qE "$normalized" 2>/dev/null; then
printf 'resolve-convention-pattern: %s resolved to an invalid ERE; enforcement disabled.\n' "$key" >&2
exit 1
fi
printf '%s\n' "$normalized"This would have caught the \D/\W/\S case and the (?i: case above as defense-in-depth, even before the dedicated is_non_ere checks are added.
(Raised in all three prior Claude security reviews; unaddressed at HEAD 75a97f5.)
Confirmed clean
| Pattern | Verdict |
|---|---|
pull_request trigger (not pull_request_target) |
✓ |
${{ github.base_ref }} via env: indirection, not direct run: interpolation |
✓ |
"origin/$BASE_REF" in git diff/git show — prefixed, no option injection |
✓ |
actions/checkout@9c091bb… SHA-pinned |
✓ |
persist-credentials: false on checkout |
✓ |
copies array hardcoded in sync script (not user-controllable) |
✓ |
key validated to subject_pattern|pr_title_pattern before awk -v key= |
✓ |
normalize_ere pipes to sed stdin (not as a sed script) |
✓ |
h2_value checks [[ -f "$file" ]] before awk |
✓ |
set -uo pipefail throughout; set -euo pipefail in sync script |
✓ |
| No secrets, tokens, or credentials referenced | ✓ |
| New CI job mirrors established SHA-pinned pattern | ✓ |
git show "$base:$manifest" — $base is a git ref from env var, not user content |
✓ |
75a97f5 to
31d182f
Compare
|
Claude finished @kyle-sexton's task in 3m 18s —— View job PR Review:
|
|
Claude finished @kyle-sexton's task in 3m 58s —— View job Security Review — PR #925
SummaryNo CRITICAL or IMPORTANT findings at HEAD Prior findings — now resolvedThe four blocking issues identified across prior reviews are confirmed fixed in the current HEAD:
The FindingsSUGGESTION —
|
| Pattern | Verdict |
|---|---|
pull_request trigger (not pull_request_target) |
✓ |
${{ github.base_ref }} via env: indirection, not direct run: interpolation |
✓ |
"origin/$BASE_REF" in git commands — prefixed, no option injection |
✓ |
actions/checkout@9c091bb… SHA-pinned |
✓ |
persist-credentials: false |
✓ |
copies array hardcoded (currently empty) — not user-controllable |
✓ |
key validated to subject_pattern|pr_title_pattern before awk -v key= |
✓ |
normalize_ere pipes to sed stdin (not as a sed script) |
✓ |
grep -E -- "$normalized" uses -- to prevent option injection |
✓ |
h2_value checks [[ -f "$file" ]] before awk; file content flows data-only |
✓ |
set -uo pipefail throughout; set -euo pipefail in sync script |
✓ |
| No secrets, tokens, or credentials referenced | ✓ |
| New CI job mirrors established SHA-pinned pattern | ✓ |
git show "$base:$manifest" — $base is a git ref from env var, not user content |
✓ |
\D/\W/\S, \A/\Z/\z, bracket-context shorthands — all detected and rejected |
✓ |
| Compile-time ERE probe covers PCRE inline flags and other detection gaps | ✓ |
| Unresolved → exit 1, stdout empty — never falls back to CC default | ✓ |
| Team-tracked layer only — local/user-global overlays cannot reach the gate | ✓ |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31d182fb59
ℹ️ 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".
31d182f to
c75c2fc
Compare
|
Claude finished @kyle-sexton's task in 3m 7s —— View job PR Review:
|
| Prior finding | Resolution in c75c2fc |
|---|---|
\D/\W/\S silently emit mis-enforcing ERE |
General \\[a-zA-Z] loop; *) return 0 catches them |
\A/\Z/\z absolute anchors |
Same loop |
\t/\n/\r/\f/\v control escapes (Codex finding) |
Same loop; explicit test added at line 96 |
\p{Lu} Unicode property escapes (last security review) |
Same loop; \p triggers *) return 0 |
[\w-] bracket-context shorthand → broken ERE |
bracket_shorthand regex (\[[^]]*\\[dwsDSW]) |
| No compile-time ERE validation | grep -E -- "$normalized" </dev/null probe at line 177 |
Open findings (all minor — none blocking)
1. PCRE inline flags fall through to compile probe — diagnostic quality gap
File: lib/resolve-convention-pattern.sh:103–128
(?i:feat|fix), (?i), (?>…), (?#…) contain no \letter sequences, so the new while loop exits cleanly and is_non_ere returns 1 (not detected). normalize_ere doesn't transform (?i: (only (?: matches). The compile probe at line 177 catches them — grep -E returns exit 2 on invalid ERE — and the resolver correctly exits 1. The safety contract is upheld. The gap is that the consumer sees the generic diagnostic:
resolve-convention-pattern: subject_pattern does not compile as POSIX ERE after normalization; enforcement disabled.
rather than the specific PCRE-construct message that is_non_ere emits. Adding explicit detection:
[[ "$p" == *'(?i'* || "$p" == *'(?m'* || "$p" == *'(?s'* || "$p" == *'(?x'* ]] && return 0
[[ "$p" == *'(?>'* || "$p" == *'(?#'* ]] && return 0would give consumers actionable guidance. A test exercising the compile-probe path for an inline-flag pattern would also close the coverage gap. This is a polish item, not a correctness issue.
2. Unnecessary shellcheck disable=SC2016 — misleading comment
File: lib/resolve-convention-pattern.sh:51
# shellcheck disable=SC2016 # backticks are a literal part of the deferral marker, not a substitution
readonly PR_DEFERRAL='Same as `subject_pattern`.'SC2016 warns about backtick substitution inside double-quoted strings. PR_DEFERRAL uses single quotes — SC2016 cannot fire here regardless. The directive has no effect and misleads a reader into thinking suppression was required. Raised in every prior review; still present.
3. Raw GitHub URL in config-resolution.md — serves source, not rendered page
File: plugins/source-control/reference/config-resolution.md:94
[commit-convention enforcement seam](https://raw.githubusercontent.com/melodic-software/claude-code-plugins/main/docs/conventions/commit-convention/README.md)raw.githubusercontent.com serves raw Markdown text, not the rendered view. It also hardcodes main. A relative path renders correctly in GitHub and is branch-agnostic (file is 3 levels up from plugins/source-control/reference/):
[commit-convention enforcement seam](../../../docs/conventions/commit-convention/README.md)Raised in every prior review; still present.
4. Dangling — see note in newrepo helper comment
File: lib/resolve-convention-pattern.test.sh:36
# newrepo "<team body>" [local-body] [does NOT create user-global — see note]No "note" exists in this file. The intent is clear from context (enforcement reads the team-tracked layer only, so a user-global overlay is irrelevant to test), but the forward reference will confuse a future editor. Raised in every prior review; still present.
# newrepo "<team body>" [local-body] (no user-global: enforcement reads team-tracked layer only)5. Missing test: pr_title_pattern deferral when subject_pattern is the CC keyword
File: lib/resolve-convention-pattern.test.sh:131–133
The deferral test uses a custom regex for subject_pattern. The path where pr_title_pattern: Same as \subject_pattern`.defers to a CC-keywordsubject_pattern(exercising CC expansion *through* the deferral branch) is not covered. This is the most likely real-world config for consumers of#914/#919`. Raised in every prior review; still absent.
r="$(newrepo $'## subject_pattern\nConventional Commits\n\n## pr_title_pattern\nSame as `subject_pattern`.')"
assert_eq "pr deferral + CC keyword -> CC_ERE" "$CC_ERE" "$(run "$r" pr_title_pattern)"What's solid
- General
\\[a-zA-Z]catch-all is the right abstraction. Enumerating by allowlist (\d/\w/\sare the only normalizable shorthands) is more robust than a blocklist — any future PCRE\letterescape is rejected by default without a code change. is_non_ereruns beforenormalize_ere: prevents PCRE constructs from being mangled into accidentally-valid ERE that enforces the wrong thing.- Compile probe (
grep -E -- "$normalized" </dev/null) makes exit-0 a meaningful guarantee. Covers PCRE inline flags and any undetected constructs as defense-in-depth. - Policy-floor contract (
h2_valuereadsTEAM_FILEonly; unresolved → exit 1, never CC default; local overlay test at line 125–129). - Bracket-context shorthand detection via variable-held regex is the correct pattern to avoid
[[ ]]early-close. ${copies[@]+"${copies[@]}"}empty-array idiom in sync script — correct underset -u.- CI job mirrors established pattern: SHA-pinned checkout,
persist-credentials: false,BASE_REFvia env var, correctci-status.needswiring.
Summary
All blocking and important issues identified across six prior reviews are resolved in c75c2fc. The five remaining items are all minor polish. The seam contract (exit 0 = valid enforceable ERE; exit 1 = no enforcement; never CC fallback; team-tracked layer only) is correctly implemented and safe for #914/#919 to vendor.
|
Claude finished @kyle-sexton's task in 4m 7s —— View job Security Review — PR #925
SummaryNo CRITICAL or IMPORTANT findings at HEAD Prior findings — confirmed fixed
FindingsSUGGESTION — PCRE
|
| Construct | Matched by is_non_ere? |
Caught by compile probe? |
|---|---|---|
(?i:feat|fix) |
No | Yes — grep -E on glibc ERE rejects it (exit 2) |
(?i)^feat: .+ |
No | Yes — same |
(?>feat|fix) |
No | Yes — same |
(?#comment)^feat |
No | Yes — same |
On Linux runners with glibc regex (ubuntu-24.04), all four cases exit 2 from the compile probe and are correctly handled. The safety contract (exit 0 ↔ valid ERE) holds on this platform.
The residual risk is diagnostic quality only: a consumer who sets subject_pattern: (?i:feat|fix) sees the generic "does not compile as POSIX ERE after normalization" message rather than the informative is_non_ere diagnostic that names the specific construct and explains how to fix it. This is not a security gap — the exit-1 outcome is correct — but it degrades the consumer DX compared to the explicit checks.
If adding explicit detection, extend is_non_ere before the final return 1:
# PCRE inline flag groups / atomic group / comment — no ERE equivalent
[[ "$p" == *'(?i'* || "$p" == *'(?m'* || "$p" == *'(?s'* || "$p" == *'(?x'* ]] && return 0
[[ "$p" == *'(?>'* || "$p" == *'(?#'* ]] && return 0Confirmed clean
| Pattern | Verdict |
|---|---|
pull_request trigger (not pull_request_target) |
✓ |
${{ github.base_ref }} via env: indirection, not direct run: interpolation |
✓ |
"origin/$BASE_REF" in git commands — prefixed; no option injection from branch name |
✓ |
actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 SHA-pinned |
✓ |
persist-credentials: false on checkout |
✓ |
copies array hardcoded (currently empty) — not user-controllable |
✓ |
key validated to subject_pattern|pr_title_pattern before awk -v key= |
✓ |
User file content ($p) flows through printf '%s' | sed and printf '%s' | grep — data paths only, no eval or command construction |
✓ |
grep -E -- "$normalized" uses -- to prevent option injection from pattern |
✓ |
normalize_ere uses bash parameter expansion (${p//'(?:'/'('}) and sed stdin pipe — no code execution from pattern content |
✓ |
h2_value checks [[ -f "$file" ]] before awk; awk -v key="$k" with validated key |
✓ |
Catch-all \letter loop covers \D/\W/\S, \A/\Z/\z, \t/\n/\r/\f/\v, \p/\P (Unicode properties), and all other undeclared \letter escapes |
✓ |
Compile probe on glibc ERE catches (?i:…), (?>…), (?#…) that bypass explicit string checks |
✓ |
set -uo pipefail throughout; set -euo pipefail in sync script |
✓ |
| No secrets, tokens, or credentials referenced | ✓ |
New CI job mirrors established SHA-pinned pattern; correctly placed in ci-status.needs |
✓ |
| Unresolved → exit 1, stdout empty — never falls back to CC default | ✓ |
| Team-tracked layer only — local/user-global overlays cannot reach the gate | ✓ |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c75c2fc752
ℹ️ 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".
c75c2fc to
9c089fc
Compare
|
Claude finished @kyle-sexton's task in 2m 58s —— View job PR Review:
|
| Prior finding | Status |
|---|---|
\D/\W/\S negated shorthands silently emitted as literal-letter mis-enforcement |
✅ Fixed — catch-all grep -oE '\\[a-zA-Z]' loop, *) return 0 |
\A/\Z/\z absolute anchors silently emitted |
✅ Fixed — same loop |
\t/\n/\r/\f/\v control escapes silently emitted |
✅ Fixed — same loop; test at line 97–99 |
\p{Lu} Unicode property escapes pass is_non_ere and compile probe |
✅ Fixed — \p triggers *) return 0 in the loop |
[\w-] bracket-context shorthands → broken double-nested ERE |
✅ Fixed — bracket_shorthand regex at lines 120–121 |
Escaped-backslash ambiguity (\\d = literal, not class) |
✅ Fixed — double_backslash check at lines 113–114 |
| No compile-time ERE validation — exit 0 could mean invalid ERE | ✅ Fixed — grep -E -- "$normalized" </dev/null probe at lines 183–187 |
The seam contract (exit 0 ↔ valid enforceable POSIX ERE; exit 1 = no enforcement; never CC fallback; team-tracked layer only) is correctly implemented and safe for #914/#919 to vendor.
Open findings (all minor — none blocking)
1. PCRE inline flags still fall through to compile probe — diagnostic quality
File: lib/resolve-convention-pattern.sh:103–134
(?i:feat|fix), (?i), (?m), (?s), (?x), (?>…), and (?#…) contain no \letter sequences, so the catch-all loop exits without triggering and is_non_ere returns 1. normalize_ere does not transform (?i: (only (?: matches exactly). The compile probe at lines 183–187 catches them — grep -E exits 2 on invalid ERE, and the resolver correctly exits 1.
Safety contract is maintained. The gap is diagnostic quality only: a consumer who configures subject_pattern: (?i:feat|fix) sees the generic "does not compile as POSIX ERE after normalization" message rather than the informative is_non_ere message that names the specific construct. Adding explicit detection before return 1:
# PCRE inline flag groups / atomic group / comment — no ERE equivalent
[[ "$p" == *'(?i'* || "$p" == *'(?m'* || "$p" == *'(?s'* || "$p" == *'(?x'* ]] && return 0
[[ "$p" == *'(?>'* || "$p" == *'(?#'* ]] && return 02. Unnecessary shellcheck disable=SC2016 — misleading comment
File: lib/resolve-convention-pattern.sh:51
# shellcheck disable=SC2016 # backticks are a literal part of the deferral marker, not a substitution
readonly PR_DEFERRAL='Same as `subject_pattern`.'SC2016 warns about backtick substitution inside double-quoted strings. PR_DEFERRAL uses single quotes — SC2016 cannot fire on a single-quoted assignment regardless. The directive has no effect and misleads a reader into thinking suppression was required. Present in every prior review; still unaddressed.
3. Raw GitHub URL in config-resolution.md — serves Markdown source, not rendered page
File: plugins/source-control/reference/config-resolution.md:94
[commit-convention enforcement seam](https://raw.githubusercontent.com/melodic-software/claude-code-plugins/main/docs/conventions/commit-convention/README.md)raw.githubusercontent.com serves Markdown source text, not the rendered view. Following the link in GitHub's PR or file viewer presents raw text. It also hardcodes main, which will break on forks or if the default branch changes. A relative path renders correctly in GitHub and is branch-agnostic (the file is 3 levels up from plugins/source-control/reference/):
[commit-convention enforcement seam](../../../docs/conventions/commit-convention/README.md)Present in every prior review; still unaddressed.
4. Dangling — see note in newrepo helper comment
File: lib/resolve-convention-pattern.test.sh:36
# newrepo "<team body>" [local-body] [does NOT create user-global — see note]No "note" exists in this file. The reason is clear from context (enforcement reads the team-tracked layer only, so testing a user-global overlay is purposeless), but the forward reference will confuse a future editor. Present in every prior review; still unaddressed.
# newrepo "<team body>" [local-body] (no user-global: enforcement reads team-tracked layer only)5. Missing test: pr_title_pattern deferral when subject_pattern is the CC keyword
File: lib/resolve-convention-pattern.test.sh:137–138
The deferral test at line 137 uses ^ABC-[0-9]+: .+ as subject_pattern. The combined path — pr_title_pattern: Same as \subject_pattern`.deferring to aConventional Commitskeywordsubject_pattern(triggering CC keyword expansion *through* the deferral branch) — is untested. This is the most likely real-world config for consumers of#914/#919`. Present in every prior review; still absent.
# --- pr_title_pattern deferral + CC keyword expansion ---
r="$(newrepo $'## subject_pattern\nConventional Commits\n\n## pr_title_pattern\nSame as `subject_pattern`.')"
assert_eq "pr deferral + CC keyword -> CC_ERE" "$CC_ERE" "$(run "$r" pr_title_pattern)"What's solid
- Catch-all
\\[a-zA-Z]loop inis_non_ereis the right abstraction. Allowlist (\d/\w/\snormalizable; everything else rejected) is more robust than any blocklist — any future PCRE\letterescape is rejected by default without a code change. is_non_ereruns beforenormalize_ere: the ordering prevents lookaround/backreference/etc from being mangled into accidentally-valid ERE that enforces the wrong thing.- Bracket-context shorthand detection via variable-held regex (
'\[[^]]*\\[dwsDSW]') is the correct pattern to avoid[[ ]]early-close by[^]]. - Escaped-backslash guard (
double_backslashat line 113) correctly excludes patterns like^foo\\d$where the double-backslash makes theda literal character, not a class. - Compile probe (
grep -E -- "$normalized" </dev/null) makes exit-0 a meaningful guarantee and covers PCRE inline flags plus any future detection gap as defense-in-depth. - Policy-floor contract (
h2_valuereadsTEAM_FILEonly; unresolved → exit 1; never CC default; local overlay explicitly tested at line 132–134). ${copies[@]+"${copies[@]}"}empty-array idiom in sync script — correct underset -u.- CI job structure mirrors the established
parse-concern-value-syncpattern: SHA-pinned checkout,persist-credentials: false,BASE_REFvia env var,fetch-depth: 0, correctci-status.needswiring.
Summary
No new issues at HEAD 9c089fc. All blocking and important findings from eight prior reviews remain resolved. Five minor polish items (2 misleading comments, 1 DX link, 1 doc comment, 1 test gap) are unchanged since the previous round — none affect the seam contract or the safety of #914/#919 vendoring this resolver. The architecture is sound and the implementation is correct.
|
Claude finished @kyle-sexton's task in 3m 42s —— View job Security Review — PR #925
SummaryNo CRITICAL or IMPORTANT findings at current HEAD. All IMPORTANT vulnerabilities raised across the prior seven security reviews remain fixed. One SUGGESTION-level gap carries over unchanged from the most recent review. Prior findings — confirmed still fixedAll fixes introduced in the
FindingsSUGGESTION — PCRE
|
| Pattern | Verdict |
|---|---|
pull_request trigger (not pull_request_target) |
✓ |
${{ github.base_ref }} via env: indirection, not direct run: interpolation |
✓ |
"origin/$BASE_REF" in git commands — shell-variable expansion only, branch names cannot break double-quoted strings |
✓ |
actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 SHA-pinned |
✓ |
persist-credentials: false on checkout |
✓ |
Workflow-level permissions: contents: read; new job inherits it, no override |
✓ |
copies array hardcoded (currently empty) — not user-controllable |
✓ |
key validated to subject_pattern|pr_title_pattern before awk -v key= |
✓ |
User file content ($p) flows through printf '%s' | sed and printf '%s' | grep data paths only — no eval, no command construction |
✓ |
grep -E -- "$normalized" uses -- to prevent option injection from pattern content |
✓ |
normalize_ere uses bash parameter expansion and sed stdin pipe — no code execution from pattern content |
✓ |
grep -E -- "$normalized" </dev/null compile probe — empty input, no data-plane exposure |
✓ |
Catch-all \\[a-zA-Z] loop covers \D/\W/\S, \A/\Z/\z, \b/\B, control escapes, \p/\P, and all other undeclared \letter forms |
✓ |
Compile probe on glibc ERE catches PCRE (?i:…), (?>…), (?#…) as defense-in-depth |
✓ |
Double-backslash guard (\\ in pattern → return 0) prevents \\d being translated as if it were the \d class |
✓ |
set -uo pipefail throughout; set -euo pipefail in sync script |
✓ |
| No secrets, tokens, or credentials referenced | ✓ |
New CI job mirrors established SHA-pinned pattern; correctly placed in ci-status.needs |
✓ |
git show "$base:$manifest" — $base from env var; $manifest from hardcoded copies array |
✓ |
| Unresolved → exit 1, stdout empty — never falls back to CC default | ✓ |
| Team-tracked layer only — local/user-global overlays cannot reach the gate | ✓ |
9c089fc to
a0d8faf
Compare
|
Claude finished @kyle-sexton's task in 3m 3s —— View job PR Review:
|
| Prior finding | Status |
|---|---|
\D/\W/\S negated shorthands → silent mis-enforcement |
✅ Fixed — \\[a-zA-Z] loop rejects all non-normalizable \letter escapes |
\A/\Z/\z absolute anchors → silent mis-enforcement |
✅ Fixed — same catch-all loop |
\t/\n/\r/\f/\v control escapes → silent mis-enforcement |
✅ Fixed — same loop; test at L96–99 |
\\d (escaped backslash) → mistranslated by normalize_ere |
✅ Fixed — double_backslash check at L114–115; test at L101–104 |
\d/\w/\s inside bracket expressions → broken double-nested ERE |
✅ Fixed — bracket_shorthand regex at L121–122 |
| No compile-time ERE validation — exit 0 could mean invalid ERE | ✅ Fixed — grep -E -- "$normalized" </dev/null probe at L184–188 |
The (?i:…), (?>…), and (?#…) PCRE constructs have no \letter sequences so they pass the \\[a-zA-Z] loop, but the compile probe catches them — grep -E returns exit 2 on all three and the resolver correctly exits 1. The exit-0 contract is upheld.
The Codex c75c2fc finding about \\d (escaped backslash before d) is also resolved: the double_backslash check at L114–115 rejects any pattern containing \\ before normalization runs, and the test at L101–104 covers it directly.
Commit message accuracy note
The commit message says "19 tests." The test file now contains 26 assertions (the count grew across the fix iterations). The final ran $CASE_NUM cases output will print 26, not 19. The contract is solid; the description is stale.
Open findings — all minor, none blocking
1. Unnecessary shellcheck disable=SC2016 — misleading comment
File: lib/resolve-convention-pattern.sh:51
# shellcheck disable=SC2016 # backticks are a literal part of the deferral marker, not a substitution
readonly PR_DEFERRAL='Same as `subject_pattern`.'SC2016 fires on backticks inside double-quoted strings. PR_DEFERRAL uses single quotes — ShellCheck cannot trigger SC2016 here regardless. The directive has no effect and will mislead future readers into thinking it was load-bearing. Raised in every prior review; still present.
2. Dangling — see note in newrepo helper comment
File: lib/resolve-convention-pattern.test.sh:36
# newrepo "<team body>" [local-body] [does NOT create user-global — see note]No "note" exists in this file. The reason is clear from context (enforcement reads the team-tracked layer only, so a user-global overlay is irrelevant to test), but the dangling forward-reference will confuse a future editor. Raised in every prior review; still present.
Suggested replacement:
# newrepo "<team body>" [local-body] (no user-global: enforcement reads team-tracked layer only)3. Raw GitHub URL in config-resolution.md — serves source, not rendered page
File: plugins/source-control/reference/config-resolution.md:94
[commit-convention enforcement seam](https://raw.githubusercontent.com/melodic-software/claude-code-plugins/main/docs/conventions/commit-convention/README.md)raw.githubusercontent.com serves Markdown source text, not the rendered view. It also hardcodes main. A relative path renders correctly in GitHub and is branch-agnostic (file is 3 levels up from plugins/source-control/reference/):
[commit-convention enforcement seam](../../../docs/conventions/commit-convention/README.md)Raised in every prior review; still present.
4. PCRE inline flags reach the compile probe — diagnostic quality gap
File: lib/resolve-convention-pattern.sh:103–135
(?i:feat|fix), (?i), (?>…), (?#…) contain no \letter sequences, so the \\[a-zA-Z] loop exits cleanly and is_non_ere returns 1. normalize_ere does not transform (?i: (only (?: matches exactly). The compile probe at L184 catches them — grep -E returns exit 2 — and the resolver correctly exits 1 with no enforcement. The safety contract is maintained.
The only gap is diagnostic quality: consumers see:
resolve-convention-pattern: subject_pattern does not compile as POSIX ERE after normalization; enforcement disabled.
rather than the specific is_non_ere message that names the construct and explains how to fix it. Adding explicit checks:
# PCRE inline flags / atomic group / comment — no ERE equivalent
[[ "$p" == *'(?i'* || "$p" == *'(?m'* || "$p" == *'(?s'* || "$p" == *'(?x'* ]] && return 0
[[ "$p" == *'(?>'* || "$p" == *'(?#'* ]] && return 0A test case for at least one inline-flag pattern would additionally confirm compile-probe coverage for this class.
5. Missing test: pr_title_pattern deferral when subject_pattern is the CC keyword
File: lib/resolve-convention-pattern.test.sh:136–138
The deferral test (L136–138) uses a custom ERE (^ABC-[0-9]+: .+). The path where pr_title_pattern: Same as \subject_pattern`.defers to aConventional Commitskeywordsubject_pattern` — exercising CC keyword expansion through the deferral branch — is untested. This is the most likely real-world consumer config.
r="$(newrepo $'## subject_pattern\nConventional Commits\n\n## pr_title_pattern\nSame as `subject_pattern`.')"
assert_eq "pr deferral + CC keyword -> CC_ERE" "$CC_ERE" "$(run "$r" pr_title_pattern)"What's solid
- General
\\[a-zA-Z]catch-all inis_non_ereis the right abstraction — allowlist (\d/\w/\sare normalizable; everything else is rejected) is more robust than a blocklist, and any future PCRE\letterescape is covered without a code change. double_backslashguard at L113–115 prevents\\d(literal backslash + d, not the PCRE class) from being mistranslated bynormalize_ere. Test at L101–104 covers it.bracket_shorthandguard at L121–122 correctly uses variable indirection to hold the regex — avoids[^]]closing the[[ ]]prematurely. Covers both lowercase and uppercase shorthand escapes.- Compile probe (
grep -E -- "$normalized" </dev/null) makes exit-0 a meaningful guarantee. The(($? >= 2))idiom correctly distinguishes "no match / exit 1" (valid ERE, no enforcement) from "compile error / exit 2+" (invalid ERE, enforce nothing). Covers PCRE inline flags and any future detection gap. - Policy-floor:
h2_valuereadsTEAM_FILEonly;[[ -n "$value" ]] || exit 1covers both absent-file and absent-key; test at L130–134 explicitly verifies a local overlay can never enforce. - CC keyword expansion in one place:
raw_valueowns theConventional Commits→CC_EREexpansion; no per-hook re-definition possible. - CI job structure: SHA-pinned checkout,
persist-credentials: false,BASE_REFvia env var (no script-injection),fetch-depth: 0for bump checking, correctly wired intoci-status.needs. ${copies[@]+"${copies[@]}"}empty-array idiom throughout sync script — correct underset -uwhen the array is intentionally empty.
Summary
All blocking and important issues identified across eight prior reviews are resolved at HEAD a0d8faf. The five open items above are cosmetic (1, 2, 3), a diagnostic-quality improvement (4), and a test coverage gap (5). The seam contract — exit 0 = valid enforceable POSIX ERE; exit 1 = no enforcement; never CC fallback; team-tracked layer only — is correctly implemented and safe for #914/#919 to vendor.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0d8faf6ae
ℹ️ 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 finished @kyle-sexton's task in 4m 46s —— View job Security Review — PR #925
SummaryNo CRITICAL or IMPORTANT findings. All IMPORTANT vulnerabilities raised across the eight prior security reviews remain fixed at HEAD Prior findings — confirmed still fixedAll fixes introduced through the
PCRE inline flags ( FindingsSUGGESTION —
|
| Pattern | Verdict |
|---|---|
pull_request trigger (not pull_request_target) |
✓ |
${{ github.base_ref }} via env: indirection, not direct run: interpolation |
✓ |
"origin/$BASE_REF" in git commands — prefixed; branch names cannot inject options through double-quoted shell variable expansion |
✓ |
actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 SHA-pinned |
✓ |
persist-credentials: false on checkout |
✓ |
Workflow-level permissions: contents: read; new job inherits, no override |
✓ |
copies array hardcoded (currently empty) — not user-controllable |
✓ |
key validated to subject_pattern|pr_title_pattern before awk -v key= |
✓ |
User file content ($p) flows through printf '%s' | sed and printf '%s' | grep data paths only — no eval, no command construction |
✓ |
grep -E -- "$normalized" uses -- to prevent option injection from pattern starting with - |
✓ |
normalize_ere uses bash parameter expansion and sed stdin pipe — no code execution from pattern content |
✓ |
grep -E -- "$normalized" </dev/null compile probe — empty input, no data-plane exposure, exit-code-only signal |
✓ |
Catch-all \\[a-zA-Z] loop covers \D/\W/\S, \A/\Z/\z, all control escapes, \p/\P, and all other undeclared \letter forms |
✓ |
Compile probe on glibc ERE catches (?i:…), (?>…), (?#…) as defense-in-depth |
✓ |
Double-backslash guard prevents \\d from being translated as if it were the \d shorthand class |
✓ |
git show "$base:$manifest" — $base from env var; $manifest from hardcoded copies array |
✓ |
set -uo pipefail throughout; set -euo pipefail in sync script |
✓ |
| No secrets, tokens, or credentials referenced | ✓ |
New CI job mirrors established SHA-pinned pattern; correctly placed in ci-status.needs |
✓ |
| Unresolved → exit 1, stdout empty — never falls back to CC default | ✓ |
| Team-tracked layer only — local/user-global overlays cannot reach the gate | ✓ |
…ent seam Seams-first foundation for hard-gating commit subjects and PR titles (#914, #919): a shared, zero-dependency resolver that reads the effective enforcement pattern from the consumer's tracked convention and hands hooks a POSIX-ERE regex. - lib/resolve-convention-pattern.sh + 19 tests: team-tracked-layer-only reads (policy-floor by construction), ERE dialect normalization with non-ERE rejection, CC-keyword expansion owned in one place, unresolved -> no enforcement (never the bundled Conventional Commits default). - scripts/sync-resolve-convention-pattern.sh + CI job: the sync scaffold the guardrails consumers (#914, #919) register into. - docs/conventions/commit-convention/: concern owner-doc (enforcement vs drafting), registered in the PLUGIN-PHILOSOPHY convention registry. - config-resolution.md: drafting-vs-enforcement reconciliation section. - consumer-config-layering: correct the stale source-control row (all-three layering landed in #660; drop the in-flight #647 note). - MIGRATION-PLAYBOOK: codify swim-lane execution. Refs #913 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
a0d8faf to
c93de64
Compare
|
Claude finished @kyle-sexton's task in 5m 15s —— View job Security Review — PR #925
ContextThis review covers HEAD Prior findings — all resolvedEvery IMPORTANT finding raised across the prior eight security reviews is confirmed closed in the current HEAD:
FindingsNo CRITICAL or IMPORTANT findings. SUGGESTION —
|
| Pattern | Verdict |
|---|---|
pull_request trigger (not pull_request_target) |
✓ |
${{ github.base_ref }} via env: indirection, not direct run: interpolation |
✓ |
"origin/$BASE_REF" in git commands — quoted shell variable, no option injection |
✓ |
actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 SHA-pinned |
✓ |
persist-credentials: false on checkout |
✓ |
Workflow-level permissions: contents: read; new job inherits, no override |
✓ |
copies array hardcoded (currently empty) — not user-controllable |
✓ |
key validated to subject_pattern|pr_title_pattern before awk -v key= |
✓ |
File content flows through awk data paths only — no eval, no command construction |
✓ |
grep -E -- "$value" uses -- to prevent option injection from pattern content |
✓ |
grep -E -- "$value" </dev/null — empty input, no data-plane exposure |
✓ |
[[ "$p" == *'(?'* ]] — catches ALL PCRE (? constructs including (?i:, (?>, (?# |
✓ |
[[ "$p" =~ \\[a-zA-Z0-9] ]] — catches all \letter/\digit escapes: \d–\S, \A–\z, \b/\B, \t/\n, \p/\P, \1–\9, and any future \letter PCRE escape |
✓ |
Compile probe catches anything that evades string checks (unbalanced brackets, PCRE verbs (*ACCEPT)) |
✓ |
normalize_ere removed — entire class of bracket-context translation bugs is gone |
✓ |
set -uo pipefail throughout; set -euo pipefail in sync script |
✓ |
| No secrets, tokens, or credentials referenced | ✓ |
New CI job mirrors established SHA-pinned pattern; correctly placed in ci-status.needs |
✓ |
| Unresolved → exit 1, stdout empty — never falls back to CC default | ✓ |
| Team-tracked layer only — local/user-global overlays cannot reach the gate | ✓ |
Summary
The redesign — reject, never translate — is the right call and closes the entire class of normalization vulnerabilities that occupied prior reviews. The two-check is_non_ere (*'(?'* + \\[a-zA-Z0-9]) combined with the compile probe provides robust, defence-in-depth rejection. No CRITICAL or IMPORTANT findings at this HEAD.
|
Claude finished @kyle-sexton's task in 4m 29s —— View job PR Review:
|
| Prior finding | How resolved |
|---|---|
\D/\W/\S negated shorthands silently emit mis-enforcing ERE |
\\[a-zA-Z0-9] catches all \letter forms |
\A/\Z/\z absolute anchors silently emitted |
Same — \A, \Z, \z all match \\[a-zA-Z0-9] |
\t/\n/\r/\f/\v control escapes silently emitted |
Same — all match \\[a-zA-Z0-9] |
\\d (escaped backslash + d) mis-translated by normalize_ere |
normalize_ere removed; the escaped form also matches \\[a-zA-Z0-9] via the trailing \d (test at L108–110) |
\d/\w/\s inside bracket expressions → broken double-nested ERE |
normalize_ere removed; [\w-] is now caught by \\[a-zA-Z0-9] matching \w (test at L113–115) |
PCRE inline flags (?i:…), (?i), (?>…), (?#…) not caught by is_non_ere |
All caught by *'(?'* — the glob matches any pattern containing the substring (? |
\p{Lu} Unicode property escapes passing the compile probe |
\p matches \\[a-zA-Z0-9] |
(?: normalization context-blind inside bracket expressions |
normalize_ere removed entirely; no normalization occurs |
| No compile-time ERE validation before emitting with exit 0 | Compile probe at L156–160 still present |
Open findings — all minor, none blocking
1. Unnecessary shellcheck disable=SC2016 — misleading comment
File: lib/resolve-convention-pattern.sh:51
# shellcheck disable=SC2016 # backticks are a literal part of the deferral marker, not a substitution
readonly PR_DEFERRAL='Same as `subject_pattern`.'SC2016 fires on backticks inside double-quoted strings. PR_DEFERRAL uses single quotes — ShellCheck cannot trigger SC2016 here regardless of content. The directive has no effect and misleads a reader into thinking suppression was required. Raised in every prior review; still present.
2. Dangling — see note in newrepo helper comment
File: lib/resolve-convention-pattern.test.sh:36
# newrepo "<team body>" [local-body] [does NOT create user-global — see note]No "note" exists in this file. The intent is clear from context (enforcement reads the team-tracked layer only, so a user-global overlay is irrelevant to test), but the forward reference will confuse a future editor. Raised in every prior review; still present.
# newrepo "<team body>" [local-body] (no user-global: enforcement reads team-tracked layer only)3. Raw GitHub URL in config-resolution.md — serves source text, not rendered page
File: plugins/source-control/reference/config-resolution.md:94
[commit-convention enforcement seam](https://raw.githubusercontent.com/melodic-software/claude-code-plugins/main/docs/conventions/commit-convention/README.md)raw.githubusercontent.com serves Markdown source text, not the rendered page. It also hardcodes main, which breaks on forks or if the default branch changes. A relative path renders correctly in GitHub and is branch-agnostic (the file is three levels up from plugins/source-control/reference/):
[commit-convention enforcement seam](../../../docs/conventions/commit-convention/README.md)Raised in every prior review; still present.
4. Missing test: pr_title_pattern deferral when subject_pattern is the CC keyword
File: lib/resolve-convention-pattern.test.sh:143–144
The deferral test uses a custom ERE (^ABC-[0-9]+: .+) as subject_pattern. The path where pr_title_pattern: Same as \subject_pattern`.defers to aConventional Commitskeywordsubject_pattern— exercising CC keyword expansion *through* the deferral branch — is not covered. This is the most common real-world config combination for consumers of#914/#919`. Raised in every prior review; still absent.
# --- pr_title_pattern deferral + CC keyword expansion ---
r="$(newrepo $'## subject_pattern\nConventional Commits\n\n## pr_title_pattern\nSame as `subject_pattern`.')"
assert_eq "pr deferral + CC keyword -> CC_ERE" "$CC_ERE" "$(run "$r" pr_title_pattern)"5. Stale test count in commit message and PR description
The commit message says "19 tests" and the PR description says "19/19 pass." The test file now contains 27 assertions (after the fix iterations through prior review rounds). The actual ran N cases output at test completion will print 27, not 19. No code change needed, but the PR description should be updated to reflect the current count before merge.
6. *'(?'* glob may false-positive on literal (? inside bracket expressions — minor DX note
File: lib/resolve-convention-pattern.sh:111
[[ "$p" == *'(?'* ]] matches any pattern containing the substring (?, including inside character classes where ( and ? are literal characters. A pattern like ^[(? ]+: .+ (commits starting with (, ?, or space) is valid POSIX ERE but would be rejected with the "PCRE-only construct" diagnostic. The failure mode is conservative (enforcement disabled, not mis-enforcement), and such a pattern is extremely unlikely in practice. Worth documenting in the header comment if not fixing, so a future author knows the limitation:
# NOTE: *'(?'* catches literal `(?` anywhere, including inside `[...]`.
# A bracket expression like `[(? ]` is valid ERE but will be rejected.
# This is intentional: unambiguous ERE should not look like a PCRE group.What's solid
-
Reject-only is the right call. Attempting PCRE→ERE translation by string rewriting was the source of every important finding across nine reviews. Removing
normalize_ereeliminates an entire class of bugs — incorrectly translated bracket expressions, context-blind substitutions, escaped-backslash ambiguity. The new contract is simpler: the team writes POSIX ERE; if they don't, enforcement is disabled with a clear diagnostic. -
*'(?'*+\\[a-zA-Z0-9]is comprehensive. The two-check design covers every PCRE-only construct identified across all prior reviews (inline flags, atomic groups, PCRE comments, lookaround, named groups, all shorthand classes, all anchors, control escapes, backreferences, Unicode property escapes) without needing per-construct detection. Any future PCRE\letteror(?...)form is rejected automatically. -
Compile probe at L156–160 closes the remaining gap. Patterns that pass
is_non_ere(no(?sequence, no\letter) but are structurally invalid ERE (e.g., unbalanced^[$) are caught bygrep -E -- "$value" </dev/null. Exit 0 guarantees the emitted pattern is both PCRE-free and ERE-compilable on the CI platform. -
Policy-floor contract maintained:
h2_valuereadsTEAM_FILEonly; unresolved → exit 1, stdout empty; never falls back toCC_ERE(test at L125–129). -
CC keyword expansion in one place:
raw_valueowns theConventional Commits→CC_EREmapping; no per-hook re-derivation possible. -
Test isolation:
mktemp -dper repo with a trap cleanup; no inter-test state. -
CI job mirrors the established
parse-concern-value-syncpattern: SHA-pinned checkout,persist-credentials: false,BASE_REFvia env var,fetch-depth: 0, correctci-status.needswiring.
Summary
All blocking and important findings from the prior nine reviews are resolved in the current HEAD. The implementation is significantly simpler and more robust than what those reviews tracked. Six minor open items above — three are cosmetic (1, 2, 3), one is a test gap (4), one is a stale count in the PR description (5), and one is a documentation-only edge case (6). None affect the seam contract or the safety of #914/#919 vendoring this resolver.
|
All codex review threads addressed by the resolver simplification (latest push): the enforcement resolver no longer translates PCRE→ERE (that string-rewriting was the source of every finding — shorthand-in-bracket, POSIX-class brackets, |
…file (#1055) ## Summary The team-tracked `.claude/source-control.md` that `/source-control:setup apply` writes lands in shared history with no signal for a teammate who does not run these plugins — audit finding f6 on #912, declared folded into #913 but never delivered by PR #925 (its file list contains no setup-skill change). - The `apply` template now opens with a self-describing preamble: read by the source-control plugin (and the guardrails commit-convention gate where installed), inert without them, and a **drafting aid, not team-wide enforcement** — tool-agnostic enforcement for every committer is a commit-msg hook or CI check. - The `apply` report for a team write states the same draft-aid vs enforcement distinction. - The header is template-owned: a reconfiguration run rewrites it in place, never appends a second copy. - Preamble safety proven, not assumed: a new regression case in `lib/resolve-convention-pattern.test.sh` shows a preambled file resolves identically to a bare one (the parse contract reads only the first non-empty body line under a `## <key>` H2). source-control `0.17.0` → `0.17.1` with CHANGELOG entry. ## Test plan - [x] `bash lib/resolve-convention-pattern.test.sh` — 28 cases, 0 failed (new preamble case included) - [x] `bash scripts/check-changelog-parity.sh --check-bump main` — pass - [x] `bash scripts/check-changed-skills.sh main` — setup: PASS (0 errors) ## Related - Closes #1046 - Refs #912 (audit umbrella, finding f6), #913 (claimed the fold), #925 (seam PR that shipped without it) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…1071) ## Summary Audit finding f4 (#912): the tracked convention config enforced nothing on its own — any direct `git commit` / `gh pr create` path skipped it, so the file was load-bearing only at skill draft time. This delivers the CC-layer content gate, the zero-setup half of the f4 design (`#919`'s opt-in commit-msg hook is the depth layer and ships separately). **New guard `block-convention-violation.sh`** (PreToolUse, `Bash|PowerShell`): - Validates the **commit subject** of the canonical stdin form — first non-empty line of the Bash heredoc / PowerShell here-string body — against the team-tracked `subject_pattern`, and the **`gh pr create --title`/`-t` value** against `pr_title_pattern` (incl. the `` Same as `subject_pattern`. `` deferral). - Reads patterns via the **vendored enforcement resolver** (`resolve-convention-pattern.sh`, synced from `lib/` — first registered consumer in `scripts/sync-resolve-convention-pattern.sh`, per the commit-convention seam #913/PR #925). - **Unresolved = no enforcement** — no team-tracked pattern / non-ERE pattern → no-op; never gates against the bundled Conventional Commits default (#912 contract 2). - **Never blocks `gh pr create` itself** — only a present-and-violating title; the documented inline fallback stays usable. - **Inherits `block-noncanonical-commit`'s exemption taxonomy** — `--amend`, `-C`/`-c`, `--fixup`/`--squash`, `-F <path>`, sequencer-in-progress — so rebases/merges are never content-gated. - **Declared bypass coverage** (out of scope, documented in the hook header): `gh pr edit --title`, `--fill`, direct API calls, babysit retitles, non-heredoc stdin producers. - Kill switch `block_convention_gate_enabled` (default true). PowerShell traffic reduces through the bundled classifier first; unparsable PS never reaches a content decision here. guardrails `0.10.3` → `0.11.0` with CHANGELOG entry. Grounded against the official hooks reference (matcher semantics, PreToolUse payload): <https://code.claude.com/docs/en/hooks>. ## Test plan - [x] New 25-case contract suite `block-convention-violation.test.sh` — unresolved/PCRE no-ops, subject block/allow, full exemption taxonomy, sequencer, non-heredoc skip, `--title` forms incl. deferral + edit/out-of-scope, PowerShell here-string forms, kill switch - [x] Sibling suites unchanged: block-noncanonical-commit 90/0, block-dangerous-git 251/0, flag-commit-pr-skill-bypass 27/0 - [x] `scripts/sync-resolve-convention-pattern.sh --check` — copy matches source - [x] `scripts/check-changelog-parity.sh --check-bump main` — pass - [x] shellcheck clean ## Related - Closes #914 - Refs #912 (locked contract), #913 / #925 (seam + resolver), #919 (depth layer, ships last), #960 (PowerShell classifier this gate composes with) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…t enforcement (#1077) ## Summary Audit f1's depth layer / f4's backstop (#912): a git `commit-msg` hook enforcing the team-tracked subject pattern **regardless of tool or shell** — editor commits, `git commit -F <file>`, IDE integrations, humans outside Claude — where the Claude-Code-layer gates see nothing. **New installable template** `lib/git-hooks/commit-msg-convention.sh` + **`/guardrails:setup apply install-commit-msg`** (explicit opt-in; bare `apply` still writes nothing): - **Chain-or-refuse.** Managed repos (`core.hooksPath`, lefthook, husky, pre-commit) → refused with the manager-side remediation. An existing `commit-msg` hook is never overwritten: chained as `commit-msg.pre-guardrails` (runs first, its rejection is final) or the install refuses. Covers the operator's machine-local commit-msg gate. - **Sentinel-marked** (`guardrails-commit-msg-convention`) so convention-inference tooling excludes the installed hook as a signal — it is derived FROM the tracked config; counting it would echo-cycle. Sentinel re-install is idempotent. - **Personal `.git/hooks/` lane only.** `core.hooksPath`, hook-manager configs, and tracked files are never touched — the committed team lane is a human PR decision, and `core.hooksPath` changes are exactly the shape `block-no-verify` refuses (interaction resolved by not going there). - **Unresolved = no enforcement** (never the bundled CC default); resolver copy removed → fail open, never block blind; `fixup!`/`squash!`/`amend!` exempt (autosquash). - **Deadlock designed out.** The rejection message instructs fixing the subject and never suggests `--no-verify` (which `block-no-verify` refuses in-session anyway); in Claude sessions the CC-layer gate (#914/PR #1071) blocks first, making this hook the cross-tool backstop. Reads the same resolver contract as the CC-layer gate (copied beside the hook at install time — an installed consumer-repo hook cannot participate in the sync seam, so it carries its own unedited copy). guardrails `0.11.0` → `0.12.0` with CHANGELOG entry. ## Test plan - [x] New 15-case contract suite `lib/git-hooks/commit-msg-convention.test.sh` — unresolved/PCRE pass-through, enforce block/allow, comment-line skipping, autosquash exemptions, empty message, resolver-removed fail-open, chain rejection-final + pass-through, sentinel presence - [x] `scripts/check-changed-skills.sh main` — setup skill PASS (0 errors) - [x] `scripts/check-changelog-parity.sh --check-bump main` — pass - [x] shellcheck clean; exec bits set ## Related - Closes #919 - Refs #912 (locked contract), #913/#925 (resolver seam), #1071 (CC-layer gate this backstops), #915/#960 (tool-bypass class this closes for non-CC surfaces) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
Seams-first foundation (#912) for hard-gating commit subjects and PR titles. Gives a zero-dependency guardrails hook a machine-readable path to the effective enforcement pattern without changing the consumer-facing markdown surface (which just landed in #660).
What lands
lib/resolve-convention-pattern.sh+ 19 tests — the single artifact every enforcement hook sources. Reads the effectivesubject_pattern/pr_title_patternand emits a POSIX-ERE regex.scripts/sync-resolve-convention-pattern.sh+ CI job — the sync scaffold the guardrails consumers (feat(guardrails): CC-layer content gate — commit subject + PR title #914, feat(guardrails): opt-in git commit-msg hook for tool-agnostic subject enforcement #919) register into (copies list intentionally empty until then).docs/conventions/commit-convention/— concern owner-doc (enforcement vs drafting), registered in the PLUGIN-PHILOSOPHY convention registry.config-resolution.md— drafting-vs-enforcement reconciliation section.consumer-config-layering— correct the stalesource-controlrow (all-three layering landed in feat(source-control): layer convention config across three layers #660; drop the in-flight source-control: tracked config is single-layer — no ~/.claude user-global or *.local.* overlay, diverging from seam 2 #647 note).MIGRATION-PLAYBOOK— codify swim-lane execution.Locked contract (binds #914/#919)
(?:->(,\d->[0-9],\w/\s; rejects un-expressible PCRE (lookaround/backref/\b) -> no enforcement + visible note..claude/source-control.mdonly; gitignored/user-global overlays are drafting-only and can never weaken a gate.Verification
bash lib/resolve-convention-pattern.test.sh-> 19/19 passshellcheck --rcfile=.shellcheckrcclean on all three scriptsscripts/sync-resolve-convention-pattern.sh --checkpassesRelated
Closes #913