feat(source-control): layer convention config across three layers

[kyle-sexton]

Closes #647

Summary

source-control.md was resolved as a single project-level file. It now resolves three layers in the order seam 2 of docs/MIGRATION-PLAYBOOK.md mandates — ~/.claude/source-control.md (user-global) → .claude/source-control.md (team, tracked) → .claude/source-control.local.md (gitignored personal overlay) — merged per key.

The layering rules live in one new bundled reference, plugins/source-control/reference/config-resolution.md, which /commit, /pull-request, and /setup all read instead of each restating them. Shape mirrors plugins/toolchain/reference/resolution-ladder.md.

Per-key override is a recorded deviation, not a silent one. The seam's default is concatenation, which is right for the first-party security-guidance precedent whose layers are prose blocks that genuinely accumulate. Every key here is a scalar or a closed list: two subject_pattern regexes cannot concatenate into a third valid regex, and a concatenated trailer_policy would emit two trailers. This is the seam's sanctioned "genuinely must override … per key, never by dropping the base layer wholesale" case.

/setup apply gains layer=user|team|local (default team); check renders the effective merge as a row per key with the layer that supplied it, rather than reporting the team file's values as if they were the whole convention.

The gitignore guard splits, because one verdict was being applied to layers that need opposite ones:

Layer	Gitignored	Staged
user-global	n/a — outside the worktree, no git command runs against it	never
team	FAIL (hard STOP)	yes
local overlay	required (FAIL when not ignored)	never

The user-global case is not an omission: git check-ignore and git status on a path outside the repository return a meaningless verdict, or a confidently wrong one when the home directory is itself a git repository.

Deviations from the issue's scope list

Scope item 7 ("Fix the bundled-template defect" / "Collapse to a single template") is closed as investigated, premise false — nothing was collapsed. The two files the issue names are not bundled templates:

• They are per-case eval fixtures, declared by the files field of the cases that consume them — skills/commit/evals/evals.json case 7 and skills/setup/evals/evals.json cases 3 and 6.
• Their divergence is load-bearing, not a defect. Commit case 7 is trailer-policy-none-honored-in-pathspec-form; it requires trailer_policy: none to test that --trailer is omitted. Collapsing toward setup's full-trailer value would break that case outright, and a single shared file can only hold one value.
• There is no bundled template to collapse toward. /setup writes the config from a skeleton embedded inline in its own SKILL.md; the plugin ships no template file.

The real default is self-consistent and was verified rather than assumed: commit/SKILL.md defaults to emitting the trailer, and the inline skeleton omits trailer_policy unless the repo has a trailer convention, so the absent case falls through to that default.

This PR instead adds a .claude/source-control.local.md fixture to each of those two skills and three new eval cases covering the layering contract (per-key override, effective-merge rendering, layer routing, and the no-git-commands user-global write) — the fixtures item 7 would have deleted are what make those cases assert anything.

Security criterion 4 flagged for review, not decided here. docs/MIGRATION-PLAYBOOK.md:657-659 ("no reading consumer files outside ${CLAUDE_PROJECT_DIR}") sits in apparent tension with seam 2's blessed ~/.claude/ user-global. The reconciliation this PR assumes is that user-home config is the operator's own declared surface rather than consumer repository data — the first-party security-guidance plugin reads ~/.claude/claude-security-guidance.md on the same basis. Confirming that reading is a review decision, and it is the one thing here worth a second opinion.

Test plan

• scripts/validate-plugins.sh — passes; root README catalog regenerated via node scripts/generate-catalog.mjs after the manifest description changed.
• scripts/run-plugin-tests.sh — all plugin tests passed or were skipped (exit 0).
• markdownlint-cli2 against plugins/source-control/**/*.md with the repo config — 0 errors.
• Every new relative link resolved against the working tree by hand; link-check is a weekly fail: false cron, so it gates nothing at PR time.
• git ls-files --eol on the three new files — i/lf w/lf, matching the * text=auto eol=lf attribute.
• Eval JSON parsed and case ids checked for uniqueness in both changed evals.json files.

Not run: the model-graded eval cases themselves. No first-party runner executes them today (docs/MIGRATION-PLAYBOOK.md §"Evals"), so the new cases are verified as schema-valid fixtures with existing files, not as passing runs.

Related

• docs/governance: consumer-config layering has no Convention registry row — 11 of 15 surfaces diverge from the seam-2 precedence rule #648 — the governing-doc gap this conformance fix sits under; independent, not blocking
• docs/governance: rule needed on three consumer-config layering deviations (standards inversion, code-tidying + songwriting wholesale replace) #649 — deviation rulings, needs-human, deliberately untouched

[claude]

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

---

PR Review: feat(source-control): layer convention config across three layers

• Gather context and load tools
• Read PR diff and changed files
• Review architecture and design decisions
• Review correctness and logic
• Review security alignment
• Review test coverage (eval fixtures)
• Post comprehensive feedback

---

Summary

This PR is well-designed. The three-layer resolution, the per-key override deviation from seam 2's concatenating default, and the gitignore-guard split are all architecturally sound and correctly motivated. One genuine inconsistency needs resolution before merge; the rest are minor.

---

Finding 1 — Inconsistency: config-resolution.md says "warn", setup/SKILL.md says "FAIL" (local overlay not gitignored)

Severity: Needs fix before merge

config-resolution.md line 71:

- **Local overlay NOT gitignored → warn.**

setup/SKILL.md line ~52 (check section):

Present but **not** gitignored → **FAIL**

The shared reference is the authoritative document read by all three skills (/commit, /pull-request, /setup). If it says "warn", then /commit and /pull-request will warn and continue; only /setup would hard-stop. The PR description's intent table says "FAIL when not ignored" for the local overlay, and setup/SKILL.md is consistent with that intent — but config-resolution.md is not.

A model running /commit with a non-gitignored local overlay would read the reference, see "warn", and proceed. That overlay can then surface in git status and be committed to team history — exactly the failure the guard exists to prevent.

Fix: update config-resolution.md line 71 from → warn to → FAIL (and update the description to match setup/SKILL.md's guidance: surface the matching gitignore line, never stage the overlay, note that the convention still resolves). Fix this →

---

Finding 2 — Security criterion 4 tension (flagged; opinion offered)

The PR correctly flags MIGRATION-PLAYBOOK.md:657-659 ("no reading consumer files outside ${CLAUDE_PROJECT_DIR}") against seam 2's sanctioned ~/.claude/<plugin>.md user-global. The reconciliation — "user-home config is the operator's own declared surface, not consumer repository data" — is the right one, and the first-party security-guidance precedent reading ~/.claude/claude-security-guidance.md settles it.

One suggested addition: config-resolution.md currently states the user-global layer as a fact ("the operator's own preference, following them across repos and machines") but doesn't note why it's outside ${CLAUDE_PROJECT_DIR} and still sanctioned. A single sentence pointing to the security-guidance precedent would give future reviewers the same footing the PR description gave this one and prevent the question from recurring on every update. Not a blocker — a documentation hygiene note.

---

Finding 3 — Eval coverage gap: no user-global-only fixture

The three new eval cases cover:

• Overlay overrides team per-key, team supplies the rest (commit eval 8)
• Effective-merge table rendering (setup eval 7)
• Personal-request routing to local layer (setup eval 8)
• User-global write with no git commands (setup eval 9)

There is no case exercising the user-global-only scenario — all three resolution cases present include a team file. A future regression where the user-global layer is silently dropped in the presence of a team file would not be caught. Not a blocker, but worth noting for a follow-on eval pass.

---

Correctness checks (all pass)

Merge semantics: Per-key override is the correct deviation. subject_pattern is a scalar/regex; two values can't concatenate into a third valid regex. trailer_policy: none concatenated with a template would emit both or neither. The deviation is explicitly recorded in config-resolution.md, not silent — satisfying MIGRATION-PLAYBOOK.md:236-237.

pr_title_pattern expansion: config-resolution.md correctly specifies that Same as \subject_pattern`.expands against the **effective**subject_patternafter all layers merge — not the value in its own layer. This prevents a case where the overlay overridessubject_pattern` but the expansion still uses the team layer's value.

Fixture consistency for commit eval 8:

• Team file (commit/.claude/source-control.md): subject_pattern: Conventional Commits, trailer_policy: none
• Overlay (commit/.claude/source-control.local.md): subject_pattern: ^[A-Z]+-\d+: .+
• Expected effective: ticket-prefix pattern + trailer_policy: none from team ✓

Fixture consistency for setup evals 7/8/9:

• Team (setup/.claude/source-control.md): subject_pattern: Conventional Commits, trailer_policy: Co-Authored-By: Claude …
• Overlay (setup/.claude/source-control.local.md): trailer_policy: none
• Eval 7 expects overlay to win trailer_policy; team to supply subject_pattern ✓
• Eval 8 expects new write to source-control.local.md, not touching team file ✓
• Eval 9 expects no git commands against ~/.claude/source-control.md ✓

Gitignore guard split: The three cases are correctly differentiated in setup/SKILL.md — user-global gets no git command, team file gitignored = hard STOP, local overlay gitignored = PASS. The "run the wrong one" risk is explicitly called out.

REPO_ROOT anchoring: Both check and apply resolve REPO_ROOT once and reuse it per-call, with the re-resolve-per-Bash-call instruction preserved. The user-global path ~/.claude/source-control.md is correctly handled without REPO_ROOT.

Version bump: 0.13.1 → 0.14.0 is an appropriate semver minor bump for new behavior (new layer= parameter, new layering logic, new reference file).

Relative links:

• commit/SKILL.md → ../../reference/config-resolution.md resolves to plugins/source-control/reference/config-resolution.md ✓
• pull-request/SKILL.md → ../../reference/config-resolution.md ✓
• pull-request/reference/create.md → ../SKILL.md ✓

---

Overall

The design is sound, the deviation from seam 2's concatenating default is correctly recorded, the gitignore-guard split fixes a real latent bug, and the eval fixtures are schema-valid and internally consistent. The one actionable item before merge is Finding 1 — the "warn" vs "FAIL" inconsistency in config-resolution.md. Findings 2 and 3 are documentation hygiene and coverage notes, not blockers.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dbae64effb

ℹ️ About Codex in GitHub

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

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

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

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

[claude]

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

---

PR Review: feat(source-control): layer convention config across three layers

• Gather context and load tools
• Read PR diff and changed files
• Review architecture and design decisions
• Review correctness and logic
• Review security alignment
• Review test coverage (eval fixtures)
• Post comprehensive feedback

---

This is a well-considered architectural change. The three-layer resolution model, the per-key override deviation, and the gitignore guard split are all sound. The PR description is unusually thorough and self-critical, which makes the review easier. Two correctness gaps surfaced below correspond to the Codex P2 findings and are worth fixing before merge; one security question needs a decision, not just a note.

---

Correctness

1. Rung 2 fallback condition is too narrow — config file present, subject_pattern absent

commit/SKILL.md:26 says rung 2 runs "if no config file exists". But a user-global file that only contributes trailer_policy: none (no subject_pattern) leaves the effective merged config without a subject_pattern. With the current condition, rung 2 never fires — a file exists — so /commit proceeds with no subject convention resolved, silently drifting to whatever the model guesses from context.

The fix is to guard on the effective merged result, not on file presence:

Rung 2 runs when the effective merged config has no subject_pattern (absent from all layers), not merely when no config file exists.

config-resolution.md:39 ("All three absent → this rung yields nothing and resolution falls through") is correct for the all-absent case but doesn't explicitly call out the partial-key-presence case, so the reference doc needs a matching clause too.

Fix this →

2. Already-tracked local overlay check is incomplete

setup/SKILL.md:229,272–278 uses git check-ignore -v (without --no-index) to verify the overlay is gitignored. When a local overlay was accidentally committed and is indexed, git check-ignore consults the index by default and does not report the file as ignored, even when a matching .gitignore pattern exists — because gitignore rules don't apply to tracked files. The skill would report FAIL: not ignored without distinguishing "no rule exists" from "rule exists but file is tracked", so the user gets the wrong remediation (add .gitignore line vs. git rm --cached).

The fix needs two separate probes for the overlay path:

1. git check-ignore --no-index -v "$REPO_ROOT/.claude/source-control.local.md" — checks whether a matching rule exists, irrespective of tracked status.
2. git ls-files -- .claude/source-control.local.md — checks whether the file is indexed.

If the rule exists but the file is indexed: FAIL, and surface git rm --cached .claude/source-control.local.md rather than the .gitignore line. Only both checks passing (rule exists AND not indexed) is a PASS.

This corresponds to the Codex P2 comment at line 277. Fix this →

---

Security

3. Criterion 4 needs a decision, not a note

The PR flags the ~/.claude/ read as an open question (MIGRATION-PLAYBOOK.md:657–659 vs. the user-global seam). The PR author's reconciliation — "user-home config is the operator's own declared surface" — is plausible and consistent with security-guidance reading ~/.claude/claude-security-guidance.md on the same basis.

But the PR description says "Confirming that reading is a review decision, and it is the one thing here worth a second opinion." Since this PR adds a new read path against that surface and the playbook currently says no reading outside CLAUDE_PROJECT_DIR, the decision should be recorded as a deviation ruling in docs/MIGRATION-PLAYBOOK.md (or #649) before merge, not left as a comment in a PR description that will scroll off. The first-party precedent is evidence, but precedent without a ruling means the next plugin author has the same open question.

---

Minor / Nits

Error to stdout instead of stderr (setup/SKILL.md:254): the STOP: line echoes to stdout. It should go to stderr (>&2) to match the pattern of the other error paths in this file and to not pollute stdout in any caller that captures output.

Eval IDs out of order (setup/evals.json): IDs are sequenced 1, 2, 3, 4, 6, 5, 7, 8, 9 — 6 appears before 5. Not functional if the runner queries by id, but surprising on visual inspection and worth fixing for consistency.

config-resolution.md:39 is slightly ambiguous — it says "All three absent → this rung yields nothing and resolution falls through." The plural "all three absent" implies that if any file exists but has no subject_pattern, the rung does not yield nothing. A parenthetical clarifying "a layer that contributes no subject_pattern is equivalent to absent for this key" would close the reading.

---

Eval fixtures — correctness

The three new eval cases are internally consistent with the fixtures:

• commit eval 8: overlay declares subject_pattern (ticket-prefix), team declares trailer_policy: none. Effective merge = ticket-prefix pattern + trailer suppressed. Expected output correctly states overlay wins subject_pattern, team wins trailer_policy. ✓
• setup eval 7: overlay declares only trailer_policy: none, team declares full pattern + trailer template. Effective merge = team's subject_pattern + overlay's trailer_policy: none. ✓
• setup eval 9 (user-global write, no git commands): no files, non-interactive subject_pattern=Conventional Commits. Expectations correctly prohibit git check-ignore/git status/git add. ✓

Setup eval 8 (routes-personal-request-to-local-overlay) supplies .claude/source-control.md as a fixture but not .claude/source-control.local.md — correct, since the scenario is writing the overlay for the first time. The "writes only differing keys" expectation is a good catch for the silent-pinning failure mode.

---

What's well done

The gitignore guard split is the hardest part of this PR and it's correct. Running git check-ignore against ~/.claude/source-control.md on a home directory that is itself a git repo would produce a confidently wrong verdict — the skip with a clear rationale is the right call. The inversion (team: no match = PASS; local: match = PASS) is clearly explained in both the SKILL.md and config-resolution.md.

The pr_title_pattern: Same as subject_pattern deferred expansion (resolve against the effective subject_pattern after all layers merge, not against the pattern in the contributing layer) is correctly documented in config-resolution.md:57 and would be subtle to get wrong.

The per-key override deviation is recorded, not silent — CHANGELOG, PR description, and config-resolution.md all explain why concatenation is wrong for scalar/closed-list keys. That's exactly how a sanctioned deviation should be handled per the playbook.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9eebb8b4f6

ℹ️ About Codex in GitHub

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

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

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

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

[claude]

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

---

PR Review: feat(source-control): layer convention config across three layers

• Gather context and load tools
• Read the latest diff (44ebf12 — "resolve config fall-through per key, not per file")
• Verify which prior findings were addressed
• Review remaining issues
• Post comprehensive feedback

---

The 44ebf12 fix commit addressed all three technical findings raised by the prior review rounds. Here is the full picture of what landed and what remains.

---

What 44ebf12 fixed

Rung 2 per-key fall-through (second Claude review Correctness 1; Codex P2 on commit/SKILL.md)

commit/SKILL.md:26 now says:

if the effective merged config declares no subject_pattern (absent from every layer, which includes the case where layers exist but only contribute other keys)

The old gate ("no config file exists") was wrong: a user-global file that only contributes trailer_policy still counts as "a file exists," so the ladder never fell through even though subject_pattern was unresolved. Fixed correctly.

config-resolution.md:39 now reads:

Each layer is optional, and fall-through is per key, not per file. A key absent from every layer is unresolved even when some layer exists.

Both the authoritative reference and the consuming skill now agree. ✓

---

Two-probe local overlay verification (second Claude review Correctness 2; Codex P2 on setup/SKILL.md)

setup/SKILL.md:276–285 now runs:

IGNORE_MATCH="$(git -C "$REPO_ROOT" check-ignore --no-index -v -- "$OVERLAY")" && HAS_RULE=1 || HAS_RULE=0
TRACKED="$(git -C "$REPO_ROOT" ls-files -- "$OVERLAY")"
if   [ "$HAS_RULE" -eq 1 ] && [ -z "$TRACKED" ]; then echo "OK: ..."
elif [ -n "$TRACKED" ];                            then echo "FAIL: ... git rm --cached ..."
else                                                    echo "FAIL: no ignore rule ..."
fi

--no-index answers "does a matching rule exist?" independent of index state. ls-files detects an already-tracked overlay. The two failure modes now produce different remediations, and the tracked branch takes precedence (recommending the .gitignore line to a user whose overlay is already indexed sends them in a circle). ✓

config-resolution.md:78 matches, replacing the old "warn" with "FAIL, with different remediations." The first Claude review's Finding 1 ("warn" vs "FAIL" inconsistency between the reference and setup/SKILL.md) is resolved. ✓

---

STOP message to stderr + eval ID ordering (second Claude review minors)

setup/SKILL.md:253: echo "STOP: ..." >&2 — correct. ✓

setup/evals.json — IDs now run 1, 2, 3, 4, 5, 6, 7, 8, 9, with eval 5 moved back before eval 6. ✓

---

Remaining item worth noting before merge

Non-interactive apply layer=local doesn't filter keys matching the lower-layer merge

setup/SKILL.md:127–131:

When the invocation carries a subject_pattern= argument, write non-interactively: use it as subject_pattern, set pr_title_pattern to the same, and omit trailer_policy.

The interactive path at line 139 explicitly says "carry only the keys that genuinely differ from the merge below it." The non-interactive path skips that check. A user running apply layer=local subject_pattern=Conventional Commits when the team file already sets subject_pattern: Conventional Commits gets an overlay that silently pins the value — future team changes to that key won't propagate locally, with no visible reason why.

This is the Codex P2 finding that was not addressed in 44ebf12. It is not a blocker — the convention resolves correctly in the present, and the overlay is recoverable — but it is an inconsistency between the two apply paths that the "carry only differing keys" rationale applies to both. Fix this →

---

Security criterion 4 — needs a human ruling before merge

The PR author correctly flags MIGRATION-PLAYBOOK.md:657–659 ("no reading consumer files outside ${CLAUDE_PROJECT_DIR}") against the user-global ~/.claude/source-control.md read added here. The proposed reconciliation — "user-home config is the operator's own declared surface" — is sound and consistent with the security-guidance plugin reading ~/.claude/claude-security-guidance.md on the same basis. But the PR description says explicitly: "Confirming that reading is a review decision, and it is the one thing here worth a second opinion."

If the playbook's prohibition is meant to have a standing exception for ~/.claude/<plugin>.md user-global files (as the seam 2 design implies), that exception should be recorded in docs/MIGRATION-PLAYBOOK.md or #649 before this merges — not so future authors ask the same question every time a new plugin adds a user-global read. This is a process gate, not a code correctness issue.

---

Correctness checks (all pass in current state)

• Merge semantics: per-key override correctly deviates from seam 2's concatenating default; deviation is recorded in config-resolution.md, CHANGELOG, and the PR description. ✓
• pr_title_pattern expansion: resolves against the effective subject_pattern after all layers merge. ✓
• Gitignore guard split: team STOP, local PASS/FAIL (now two-probe), user-global no git command. All three cases are correctly differentiated and the "run the wrong one" risk is called out. ✓
• Fixture consistency: commit eval 8 and setup evals 7/8/9 match their fixtures. ✓
• Eval coverage: no user-global-only scenario; noted by prior reviews as a follow-on gap. ✓
• Version bump: 0.13.1 → 0.14.0 is appropriate for new behavior. ✓
• Relative links: all three skill cross-references resolve correctly. ✓

---

Summary

All technical findings from the prior review rounds are closed. One Codex P2 item (non-interactive layer=local key filtering) was not picked up in the fix commit — note it for a follow-on or address before squash. The security criterion 4 gate needs a human ruling recorded in the playbook before merge; everything else is ready.

[claude]

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

---

PR Review: feat(source-control): layer convention config across three layers

• Gather context and load tools
• Read latest commits and diff (three fixing commits since initial review)
• Verify which prior findings were addressed by the fix commits
• Review remaining open issues
• Post comprehensive updated feedback

---

Three fix commits landed since the initial reviews. This pass verifies what changed and whether anything remains actionable.

---

What the fix commits addressed

44ebf12 — resolve config fall-through per key, not per file

This commit fixes three findings from the prior reviews in one shot:

1. config-resolution.md "warn" vs "FAIL" (Finding 1, review 1). config-resolution.md lines 76–81 now reads:

   Local overlay not ignored, or already tracked → FAIL, with different remediations. … A missing ignore rule is fixed by the .gitignore line; an already-tracked overlay is fixed by untracking it, since an ignore rule never applies to a file already in the index.

   The shared reference is now authoritative and consistent with setup/SKILL.md. ✓

2. Rung 2 fallback condition too narrow (Finding 1, review 2). commit/SKILL.md line 26 now says "absent from every layer, which includes the case where layers exist but only contribute other keys," and config-resolution.md lines 39–43 explicitly states "fall-through is per key, not per file … Never gate that fall-through on file presence." A user-global layer with only trailer_policy no longer silently blocks the subject_pattern fall-through. ✓

3. Already-tracked local overlay check incomplete (Finding 2, review 2). setup/SKILL.md lines 283–293 now runs two independent probes — check-ignore --no-index -v for rule existence and ls-files for index membership — and branches on the pair with correct remediations. "Rule exists, file is tracked" now surfaces git rm --cached rather than the .gitignore line. ✓

   Also in this commit: the team-layer STOP message now goes to stderr (>&2), and the setup eval cases are back in ID order (1–10). Both were minor findings from review 2. ✓

5bf6484 — spell unparsable per the repo dictionary

Spelling fix (unparseable → unparsable) in config-resolution.md. ✓

0c20814 — apply the differs-only overlay rule non-interactively

The Codex P2 finding (review at line 129): the non-interactive apply layer=local subject_pattern=X path wrote keys unconditionally. setup/SKILL.md lines 133–138 now reads:

Writing an overlay layer — user or local — this path still resolves the layers below first and omits any key already equal to that merge. … Writing nothing is the correct outcome when the requested value already holds; say so rather than materializing an empty overlay.

The interview path already did this correctly; the non-interactive path now matches. Eval case 10 (non-interactive-overlay-omits-keys-equal-to-lower-layers) exercises the scenario directly. ✓

---

What remains open

Security criterion 4 — still needs a decision, not a note.

The PR description says "Confirming that reading is a review decision, and it is the one thing here worth a second opinion." The config-resolution.md says ~/.claude/source-control.md is "Their home directory, not consumer repository data," which is the right framing. But no deviation ruling has been added to docs/MIGRATION-PLAYBOOK.md or issue #649 — the open question from review 2 is still open.

The first-party security-guidance precedent (~/.claude/claude-security-guidance.md) settles the substance. The gap is recording that ruling durably so the next plugin author has it in the playbook rather than a PR description. This is not a correctness blocker, but the PR itself explicitly flags it as a decision to record. Worth a follow-on — either a line in MIGRATION-PLAYBOOK.md or a ruling in #649 before the next plugin reaches for a user-home read.

Eval coverage — no user-global-only scenario.

No case tests the path where ~/.claude/source-control.md is the only file present (no team or local file). A regression that silently dropped the user-global layer in the presence of a team file would go undetected. Minor — this can land in a follow-on eval pass.

---

State of the PR

All five actionable findings from prior reviews are resolved. The two remaining items are a documentation ruling (non-blocking, PR author already flagged it) and a coverage note (minor, follow-on). The design is sound — the per-key override deviation is recorded, the gitignore guard split correctly inverts between team and local layers, the two-probe check-ignore --no-index + ls-files logic handles all three states (no rule / rule exists + untracked / rule exists + tracked) with the right remediation for each, and the non-interactive apply path now matches the interview path's differs-only contract.

Ready to merge once the security criterion 4 ruling is tracked somewhere durable.

[kyle-sexton]

Review findings addressed — three commits, plus a note on the security question.

Correctness 1 (rung 2 fallback) — 44ebf12d. Fall-through is now per key rather than per file, in both commit/SKILL.md and config-resolution.md.

Correctness 2 (tracked overlay) — 44ebf12d, and verified against git rather than reasoned about: on a tracked file matching an ignore rule, bare check-ignore -v exits 1 with no output while --no-index -v reports the rule. Two probes now, with git rm --cached as the remediation when the overlay is tracked.

Codex thread 3 (non-interactive overlay pinning) — 0c208149. The differs-only rule now covers the non-interactive path too; eval case 10 pins it.

Nits — all three: STOP: to stderr, setup eval ids back in order, config-resolution.md fall-through clause made explicit.

Security criterion 4 — why this PR does not rule on it

You are right that a PR description is the wrong home for a deviation ruling, and I have not moved it into one. The reason is a constraint rather than a disagreement:

• The ruling belongs to docs/governance: rule needed on three consumer-config layering deviations (standards inversion, code-tidying + songwriting wholesale replace) #649, which carries needs-human. That label's definition is "Human-in-the-loop required; autonomous sessions must not resolve items carrying this." Writing the reconciliation into docs/MIGRATION-PLAYBOOK.md would resolve docs/governance: rule needed on three consumer-config layering deviations (standards inversion, code-tidying + songwriting wholesale replace) #649's substance under a different file name, which is exactly what the label exists to prevent. source-control: tracked config is single-layer — no ~/.claude user-global or *.local.* overlay, diverging from seam 2 #647 and docs/governance: consumer-config layering has no Convention registry row — 11 of 15 surfaces diverge from the seam-2 precedence rule #648 were deliberately scoped so neither blocks on it.
• This PR does not create the precedent it would be ruling on. security-guidance — a first-party plugin — already reads ~/.claude/claude-security-guidance.md, and seam 2 already blesses a ~/.claude/<plugin>.md user-global. source-control is conforming to an existing sanctioned shape, not opening a new read class.

So the honest state is: the tension is real, it is recorded in #647 and here, and the next plugin author does still have the open question — but closing it is a maintainer decision, tracked at #649. If you would rather this PR wait on that ruling, say so and I will hold it; the layering work itself is independent of which way the ruling goes.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0c20814912

ℹ️ About Codex in GitHub

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

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

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

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

[claude]

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

---

PR Review: feat(source-control): layer convention config across three layers

• Gather context and load tools
• Read the latest commit diff (b350eb5 — "treat a non-interactive apply as an update, not a rewrite")
• Verify which prior findings were addressed
• Review remaining Codex P2 findings
• Post comprehensive feedback

---

What b350eb5 fixed

This commit addresses the two Codex P2 findings raised after 0c20814.

Overlay key preservation during non-interactive apply (Codex P2, setup/SKILL.md)

setup/SKILL.md lines 133–138 now says:

A non-interactive write is an update, not a fresh file. The target layer is rewritten in place, so read it first and carry through every key the invocation did not ask to change. … Only a key the invocation explicitly sets may be replaced, and only a key the user explicitly clears may be removed.

The "omit-if-equal" rule at lines 140–147 is correctly scoped to requested keys only — it explicitly says "This applies to the requested keys only; it never licenses dropping an unrelated key the overlay already carries." The two behaviors compose correctly:

• Existing overlay key not mentioned by invocation → carried through (update, not rewrite)
• Requested key already equal to lower-layer merge → omitted from write (avoids silent pinning)
• Both conditions at once (existing trailer_policy: none, requested subject_pattern == team value) → trailer_policy: none preserved, subject_pattern omitted ✓

Eval case 11 (non-interactive-update-preserves-unrequested-overlay-keys) exercises the first scenario directly. ✓

Subject-pattern inference gated on effective resolved value (Codex P2, setup/SKILL.md apply interview + commit/SKILL.md + pull-request/SKILL.md)

setup/SKILL.md lines 159–162:

Infer before asking. Gate this on the resolved value, not on file presence: infer whenever the effective merged subject_pattern is unresolved, which includes the case where layers exist but contribute only other keys.

commit/SKILL.md line 26:

…if the effective merged config declares no subject_pattern (absent from every layer, which includes the case where layers exist but only contribute other keys)…

pull-request/SKILL.md: "When no layer resolves a pr_title_pattern…" — consistent phrasing applied to the PR skill too. ✓

All three consuming skills now gate their fall-through on the effective resolved value, not on file presence. The three phrasings are consistent with each other and with config-resolution.md line 39 ("fall-through is per key, not per file"). ✓

---

Codex P1 — Security criterion 4

The Codex review at 0c20814 raised this as P1 ("Keep config reads inside the project boundary"). Kyle's response correctly explains why this PR cannot and should not close it: the ruling belongs to #649 (needs-human), and writing the reconciliation into docs/MIGRATION-PLAYBOOK.md here would resolve #649's substance under a different file name — exactly what that label exists to prevent.

The substantive case is settled: security-guidance already reads ~/.claude/claude-security-guidance.md on the same basis, and seam 2 explicitly blesses this shape. source-control is conforming to an existing sanctioned surface, not opening a new read class. The only open item is recording the ruling durably, which is #649's job. The Codex P1 rating reflects a policy gap, not a code defect in this PR.

---

Full state of all findings across all review rounds

Finding	Source	Status
config-resolution.md "warn" vs "FAIL"	Review 1, Finding 1	✅ Fixed in 44ebf12
Rung 2 fallback gated on file presence	Review 2, Correctness 1; Codex P2	✅ Fixed in 44ebf12
Tracked overlay check using bare check-ignore	Review 2, Correctness 2; Codex P2	✅ Fixed in 44ebf12
STOP to stderr; eval ID ordering	Review 2, nits	✅ Fixed in 44ebf12
Non-interactive apply omits equal keys	Review 3; Codex P2	✅ Fixed in 0c20814, eval 10
Overlay key preservation during non-interactive update	Codex P2 (after 0c20814)	✅ Fixed in b350eb5, eval 11
Inference gated on file presence in apply interview	Codex P2 (after 0c20814)	✅ Fixed in b350eb5
Security criterion 4 ruling	All reviews; Codex P1	⏳ Deferred to #649 (needs-human) — not resolvable here
User-global-only eval scenario	Reviews 1–3	📋 Minor follow-on

---

Correctness checks (all pass)

• The two new behaviors in b350eb5 compose correctly: the "update, not rewrite" rule and the "omit-if-equal for requested keys only" rule are applied to disjoint key sets when they interact, with no conflict.
• Inference path in all three skills now consistently targets the effective resolved subject_pattern, not file presence.
• Eval case 11 fixtures reference the correct files (.claude/source-control.md and .claude/source-control.local.md), matching the scenario where an overlay already exists.
• config-resolution.md is still the authoritative reference document and remains consistent with all three consuming skills after this commit.

---

Summary

All seven technical findings are resolved. The security criterion 4 ruling is correctly deferred to #649 — this PR cannot and should not close it, and the code change is sound on the existing precedent. The user-global-only eval gap is a follow-on. Ready to merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b350eb55bb

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

[kyle-sexton]

Control tower hold (do-not-merge), 2026-07-20 ~09:50Z. This PR's layering scheme (including the recorded per-key-merge deviation from the seam-2 concatenation default) is exactly the subject of #649 — the governance decision on the three consumer-config layering deviations — which is still status: needs-decision with an operator brief pending (T5, 08:51Z). Conventions are retained Class C: the operator decides #649, then this PR either rides the ruling or adjusts to it. Hold is content-neutral — CI/review proceed; only the merge waits. Cross-noted on #649. [Tower decide-and-log; reversible]

[claude]

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

---

PR Review: feat(source-control): layer convention config across three layers

• Gather context and load tools
• Read the latest commit diff (e8c4fed — "halt setup apply on a local-overlay validation failure")
• Verify the final Codex P2 finding is resolved
• Review remaining open items
• Post comprehensive feedback

---

What e8c4fed fixed

Local-overlay validation failure no longer falls through to step 7 (Codex P2, last round)

setup/SKILL.md lines 302–310:

elif [ -n "$TRACKED" ]; then
  echo "FAIL: $OVERLAY is tracked; untrack it with: git rm --cached $OVERLAY" >&2
  exit 1
else
  echo "FAIL: no ignore rule matches $OVERLAY; add .claude/*.local.* to .gitignore" >&2
  exit 1
fi

Both FAIL branches now exit non-zero, matching the team-layer guard's behaviour. The closing prose at lines 316–322 is updated to document that either guard halts the apply and to give each failure its own remediation. The inline comment at line 299 explains the security invariant clearly: proceeding after a FAIL would report an effective merge over a personal overlay that is still shareable. ✓

The Codex P2 finding — "these branches only print FAIL and the shell call still exits successfully" — is fully resolved. ✓

---

Full state of all findings across all review rounds

Finding	Source	Status
config-resolution.md "warn" vs "FAIL"	Review 1, Finding 1	✅ Fixed in 44ebf12
Rung 2 fallback gated on file presence	Review 2, Correctness 1; Codex P2	✅ Fixed in 44ebf12
Tracked overlay check using bare check-ignore	Review 2, Correctness 2; Codex P2	✅ Fixed in 44ebf12
STOP to stderr; eval ID ordering	Review 2, nits	✅ Fixed in 44ebf12
Non-interactive apply omits equal keys	Review 3; Codex P2	✅ Fixed in 0c20814, eval 10
Overlay key preservation during non-interactive update	Codex P2 (after 0c20814)	✅ Fixed in b350eb5, eval 11
Inference gated on file presence in apply interview	Codex P2 (after 0c20814)	✅ Fixed in b350eb5
Local-overlay validation failure doesn't halt apply	Codex P2 (after b350eb5)	✅ Fixed in e8c4fed
Security criterion 4 ruling	All reviews; Codex P1	⏳ Deferred to #649 (needs-human) — correct
User-global-only eval scenario	Reviews 1–3	📋 Minor — follow-on

---

Correctness checks (all pass in current state)

• Both FAIL branches exit non-zero. The no-rule and already-tracked cases both halt before step 7. No path through the local overlay guard can proceed to an effective-merge report while the overlay is shareable. ✓
• FAIL messages go to stderr. Consistent with the team-layer STOP and standard shell convention. ✓
• Remediation split. The tracked branch takes precedence (add the ignore line to a tracked file → no effect; git rm --cached is the correct first step). ✓
• Inline comment explains the invariant. The comment at line 299 names the hazard — "proceeding here would report an effective merge over a personal file that is still shareable" — so a future editor knows why these branches exit rather than warn and continue. ✓
• Eval cases 10 and 11 are internally consistent with their fixtures and cover the non-interactive update contracts: omit-if-equal to lower-layer merge (eval 10), and preserve unrequested overlay keys (eval 11). ✓

---

Security criterion 4 — deferred correctly

The Codex P1 comment on config-resolution.md:33 was left unresolved by Kyle deliberately: the ruling belongs to #649 (needs-human), and recording it here would resolve #649's substance under a different file name. That reasoning is correct. The thread is appropriately unresolved.

---

Summary

All eight technical findings from all review rounds are resolved. The security criterion 4 ruling is correctly deferred to #649. The user-global-only eval gap remains a minor follow-on. The code itself is ready; the merge gate is the #649 ruling and the control tower hold.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e8c4feda7e

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

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6976d42645

ℹ️ About Codex in GitHub

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

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

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

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

[claude]

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

---

PR Review: feat(source-control): layer convention config across three layers

• Gather context and load tools
• Read latest commits and diff (9072758 — "bind type_list to the effective subject_pattern")
• Verify Codex P2 finding (type_list at merge time) is resolved
• Review any remaining open issues
• Post comprehensive feedback

---

What 9072758 fixed

type_list retained at merge time for custom patterns (Codex P2, config-resolution.md:48, last round)

config-resolution.md lines 60–67 now reads:

type_list is bound to the effective subject_pattern, not merged independently. It is a property of a Conventional-Commits-shaped pattern, so after the layers merge, an inherited type_list is dropped whenever the effective subject_pattern is a custom regex — even when the layer supplying that pattern said nothing about type_list. A user-global Conventional Commits plus its type list, overridden by a team ticket-prefix regex, resolves to the team pattern with no type_list; retaining it per key would leave /commit pre-checking against a vocabulary the effective pattern does not use. The reverse also holds: an effective Conventional-Commits pattern with no type_list in any layer resolves to the bundled 11-type list.

This is the resolution-time counterpart to the write-path fix in 6976d42 — both halves of the problem are now closed. New commit eval case 9 (inherited-type-list-drops-for-an-effective-custom-pattern) pins it.

Fixture consistency for commit eval 9:

• Team file: subject_pattern: Conventional Commits, type_list: build, chore, ci, …, trailer_policy: none
• Overlay: subject_pattern: ^[A-Z]+-\d+: .+, pr_title_pattern: Same as subject_pattern.
• Expected effective: ticket-prefix pattern (from overlay) + type_list absent (effective pattern is custom) + trailer_policy: none (from team) ✓

The case expectation explicitly states "Drops the team layer's type_list because the effective subject_pattern is not Conventional-Commits-shaped" and "Never pre-checks the subject against a type vocabulary the effective pattern does not use" — both are now backed by the reference document. ✓

---

Security criterion 4 — maintainer ruling received

The Codex P1 thread on config-resolution.md:33 was resolved at 2026-07-20T14:58Z with a maintainer ruling: criterion 4's "consumer files" means consuming-repository files, and ~/.claude/ is the operator's own declared surface, not consumer repository data. The wording fix to criterion 4 (narrowing "no absolute paths" to exclude the sanctioned ~/.claude/<plugin>.md surface) is deferred to #648 so the next plugin author has it in the playbook without a PR-description hunt. The ruling is settled; the documentation landing is tracked.

---

Full state of all findings across all review rounds

Finding	Source	Status
config-resolution.md "warn" vs "FAIL"	Review 1, Finding 1	✅ Fixed in 44ebf12
Rung 2 fallback gated on file presence	Review 2, Correctness 1; Codex P2	✅ Fixed in 44ebf12
Tracked overlay check using bare check-ignore	Review 2, Correctness 2; Codex P2	✅ Fixed in 44ebf12
STOP to stderr; eval ID ordering	Review 2, nits	✅ Fixed in 44ebf12
Non-interactive apply omits equal keys	Review 3; Codex P2	✅ Fixed in 0c20814, eval 10
Overlay key preservation during non-interactive update	Codex P2 (after 0c20814)	✅ Fixed in b350eb5, eval 11
Inference gated on file presence in apply interview	Codex P2 (after 0c20814)	✅ Fixed in b350eb5
Local-overlay validation failure doesn't halt apply	Codex P2 (after b350eb5)	✅ Fixed in e8c4fed
type_list retained at merge for custom patterns	Codex P2 (after 6976d42)	✅ Fixed in 9072758, commit eval 9
Security criterion 4 ruling	All reviews; Codex P1	✅ Maintainer ruling received; wording fix deferred to #648
User-global-only eval scenario	Reviews 1–3	📋 Minor — follow-on

---

Correctness checks (all pass in current state)

• type_list binding: The fix in config-resolution.md is symmetric — dropped for custom patterns, defaulted to the bundled 11-type list for Conventional-Commits-shaped patterns with no explicit type_list in any layer. Both directions correct. ✓
• Write-path and resolution-path consistency: 6976d42 fixed the write path (recompute derived keys); 9072758 fixed the resolution path (bind type_list to the effective subject_pattern). Setup eval 12 pins the write path; commit eval 9 pins the resolution path. The two evals use scenarios that exercise the same invariant from different angles. ✓
• Fixture consistency for commit evals 8 and 9: Both use the same fixture files. Case 8 focuses on subject_pattern/trailer_policy per-key override; case 9 focuses on type_list dropping. No conflict — the expected outputs test different aspects of the same merge. ✓
• pr_title_pattern deferred expansion: config-resolution.md:69–70 correctly states expansion against the effective subject_pattern. The overlay fixture declares pr_title_pattern: Same as subject_pattern. explicitly, which is redundant but not incorrect — it resolves to the ticket-prefix pattern either way. ✓
• Setup eval 12 fixture consistency: Team file has subject_pattern: Conventional Commits + type_list. Applying layer=team subject_pattern=^[A-Z]+-\d+: .+ should rewrite with ticket-prefix pattern, drop type_list (derived key), carry trailer_policy: Co-Authored-By: Claude … (independent key). Correctly covered by the expectations in eval 12. ✓

---

Summary

All nine technical findings from every review round are resolved. The security criterion 4 ruling is in. The user-global-only eval gap remains a minor follow-on. The code is complete and correct; the merge gate is the #649 governance decision tracked by the control tower hold.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2a96410d18

ℹ️ About Codex in GitHub

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

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

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

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

[claude]

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

---

PR Review: feat(source-control): layer convention config across three layers

• Gather context and load tools
• Read latest commit diff (d0c29cfe — "make subject_pattern exactly one anchored regex")
• Verify the any-of list removal is consistent across all affected files
• Check for any new issues introduced
• Post comprehensive feedback

---

What d0c29cfe fixed

Undefined "any-of list" affordance removed (Codex P2, config-resolution.md:14, last round)

The previous contract documented subject_pattern as accepting "an any-of list of anchored regexes" but never defined a serialization grammar or an any-matches rule. A consuming repo that stored a multi-line list form would have it handed whole to a regex matcher, producing either invalid-regex errors or false rejections of valid subjects.

Rather than invent a grammar for a form nothing parses, the fix narrows subject_pattern to exactly one value and provides the correct way to express alternatives: regex alternation inside that one regex.

config-resolution.md lines 14–20 now reads:

subject_pattern — required; the literal keyword Conventional Commits, or an anchored regex (^…-style). Exactly one value, never a list and never a plain-language description — a convention with several accepted shapes is expressed as alternation inside the one regex (^(?:feat|fix): .+|^[A-Z]+-\d+: .+), which every consumer already evaluates correctly. A list form would need a serialization grammar and an any-matches rule that nothing here defines, and a reader handing a multi-line value straight to a matcher would reject valid subjects or build an invalid regex.

The change is correctly applied in all four places that previously documented or permitted the list form:

File	Change
config-resolution.md:14–20	Authoritative definition narrowed to exactly one value; alternation cited as the canonical form
commit/SKILL.md:22	Rung 1 drops "or any-of list" — "anything else is already a single anchored regex"
setup/SKILL.md:202–205	Interview guidance replaces "ask for a short any-of list" with "express alternatives as alternation inside one regex"
setup/SKILL.md:225–228	Inline skeleton comment updated — "exactly one anchored regex — never a list"

No remaining "any-of list" references appear in any source-control plugin file. ✓

---

Correctness check: fixture consistency for commit eval 9

Eval case 9 (inherited-type-list-drops-for-an-effective-custom-pattern) exercises the interaction between the any-of-list fix and the type_list binding rule:

• Team file (commit/.claude/source-control.md): subject_pattern: Conventional Commits, type_list: build, chore, ci, …, trailer_policy: none
• Personal overlay (commit/.claude/source-control.local.md): subject_pattern: ^[A-Z]+-\d+: .+, pr_title_pattern: Same as subject_pattern.
• Expected effective merge: ticket-prefix pattern (overlay wins subject_pattern) + type_list absent (effective pattern is custom, not CC-shaped) + trailer_policy: none (team wins)

Both the eval expectations and the fixtures are consistent with the type_list binding rule in config-resolution.md:65–72. ✓

---

No new issues introduced

The change is narrowly scoped to removing a single documented but unimplementable feature. All downstream consumers (/commit rung 1, /setup interview and skeleton, config-resolution.md) are updated consistently, and the authoritative reference doc's treatment of the other keys (type_list, pr_title_pattern, trailer_policy) is untouched.

---

Full state of all findings across all review rounds

Finding	Source	Status
config-resolution.md "warn" vs "FAIL"	Review 1, Finding 1	✅ Fixed in 44ebf12
Rung 2 fallback gated on file presence	Review 2, Correctness 1; Codex P2	✅ Fixed in 44ebf12
Tracked overlay check using bare check-ignore	Review 2, Correctness 2; Codex P2	✅ Fixed in 44ebf12
STOP to stderr; eval ID ordering	Review 2, nits	✅ Fixed in 44ebf12
Non-interactive apply omits equal keys	Review 3; Codex P2	✅ Fixed in 0c20814, eval 10
Overlay key preservation during non-interactive update	Codex P2 (after 0c20814)	✅ Fixed in b350eb5, eval 11
Inference gated on file presence in apply interview	Codex P2 (after 0c20814)	✅ Fixed in b350eb5
Local-overlay validation failure doesn't halt apply	Codex P2 (after b350eb5)	✅ Fixed in e8c4fed
type_list retained at merge for custom patterns (write path)	Codex P2 (after e8c4fed)	✅ Fixed in 6976d42, setup eval 12
type_list retained at merge for custom patterns (resolution path)	Codex P2 (after 6976d42)	✅ Fixed in 9072758, commit eval 9
subject_pattern "any-of list" affordance undefined	Codex P2 (after 9072758)	✅ Fixed in d0c29cfe
Security criterion 4 ruling	All reviews; Codex P1	✅ Maintainer ruling received; wording fix deferred to #648
User-global-only eval scenario	Reviews 1–3	📋 Minor — follow-on

---

All 11 technical findings from all review rounds are resolved. The security criterion 4 ruling is in. The user-global-only eval gap remains a minor follow-on. The code is complete and correct; the merge gate is the #649 governance decision tracked by the control tower hold.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d0c29cfe24

ℹ️ About Codex in GitHub

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

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

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

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

[claude]

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

---

PR Review: feat(source-control): layer convention config across three layers

• Gather context and read latest commit (7d5206b)
• Verify the pr_title_pattern deferral marker fix
• Cross-check all affected files for consistency
• Review for any remaining issues

---

What 7d5206b fixed

pr_title_pattern deferral marker now pinned to the exact canonical spelling (Codex P2, setup/SKILL.md:211, last round)

The interview previously instructed the skill to write same as \subject_pattern`(lowercase s, no trailing period).config-resolution.mddefines only the literal `` Same assubject_pattern. `` as the recognized expansion marker — a different casing or missing period would have been pre-checked as a regex rather than deferred to the effective subject_pattern`.

config-resolution.md:23–25 now reads:

pr_title_pattern — the PR-title shape, or the deferral marker spelled exactly Same as `subject_pattern`. (capital S, backticked key, trailing period). The match is literal: any other casing or punctuation is treated as a pattern in its own right.

setup/SKILL.md:212–215 now says:

write the deferral marker exactly as Same as `subject_pattern`. — capital S, backticked key, trailing period. That literal is what the resolution contract recognizes and expands against the effective subject_pattern; any other casing or punctuation is read as a pattern in its own right and pre-checked as a regex.

Consistency check — all consumers and fixtures verified:

Location	Value	Status
config-resolution.md:24 — definition	Same as `subject_pattern`.	✅ Canonical
config-resolution.md:76 — expansion clause	Same as \`subject_pattern\`.	✅ Canonical
setup/SKILL.md:212 — interview writer instruction	Same as `subject_pattern`.	✅ Canonical (fixed in this commit)
setup/SKILL.md:241 — skeleton placeholder	"Same as \subject_pattern`."`	✅ Canonical
setup/.claude/source-control.md (fixture)	Same as \subject_pattern`.`	✅ Already canonical
commit/.claude/source-control.md (fixture)	Same as \subject_pattern`.`	✅ Already canonical
commit/.claude/source-control.local.md (fixture)	Same as \subject_pattern`.`	✅ Already canonical

The table at setup/SKILL.md:53 displays Same as subject_pattern (no backticks, no period) — this is in an illustrative display rendering of check output where Markdown markup is stripped for a text table, not a stored value. Not an issue.

No new eval case was added for this specific fix — Kyle's note explains why: the three bundled fixtures already used the canonical form; the fix was purely the writer instruction drifting from the contract.

---

Full state of all findings across all review rounds

Finding	Source	Status
config-resolution.md "warn" vs "FAIL"	Review 1, Finding 1	✅ Fixed in 44ebf12
Rung 2 fallback gated on file presence	Review 2, Correctness 1; Codex P2	✅ Fixed in 44ebf12
Tracked overlay check using bare check-ignore	Review 2, Correctness 2; Codex P2	✅ Fixed in 44ebf12
STOP to stderr; eval ID ordering	Review 2, nits	✅ Fixed in 44ebf12
Non-interactive apply omits equal keys	Review 3; Codex P2	✅ Fixed in 0c20814, eval 10
Overlay key preservation during non-interactive update	Codex P2 (after 0c20814)	✅ Fixed in b350eb5, eval 11
Inference gated on file presence in apply interview	Codex P2 (after 0c20814)	✅ Fixed in b350eb5
Local-overlay validation failure doesn't halt apply	Codex P2 (after b350eb5)	✅ Fixed in e8c4fed
type_list retained at merge for custom patterns (write path)	Codex P2 (after e8c4fed)	✅ Fixed in 6976d42, setup eval 12
type_list retained at merge for custom patterns (resolution path)	Codex P2 (after 6976d42)	✅ Fixed in 9072758, commit eval 9
subject_pattern "any-of list" affordance undefined	Codex P2 (after 9072758)	✅ Fixed in d0c29cfe
pr_title_pattern deferral marker exact spelling	Codex P2 (after d0c29cfe)	✅ Fixed in 7d5206b
Security criterion 4 ruling	All reviews; Codex P1	✅ Maintainer ruling received; wording fix deferred to #648
User-global-only eval scenario	Reviews 1–3	📋 Minor — follow-on

---

Summary

All 12 technical findings from every review round are resolved. The 7d5206b commit is narrow and correct — the deferral marker definition and the writer instruction now agree on the exact literal, and all three fixture files were already canonical. The code is complete and correct; the merge gate remains the #649 governance decision tracked by the control tower hold.