feat(source-control): well-known default path for neutral convention SSOT (F1–F4)

[kyle-sexton]

Summary

Resolves the neutral commit-convention SSOT (from #1141) by a fixed 3-rung precedence, identical
on the drafting and enforcement surfaces:

1. explicit convention_source pointer — relocation override, path stays repo-owned
2. well-known default docs/conventions/source-control/commit-convention.yml when present — the
   marketplace's own dogfooded docs/conventions/<concern>/ layout; common case reads ONE
   tool-agnostic file, no markdown pointer-parse, nothing in agent-rewritable prose to sever
3. markdown-H2 in .claude/source-control.md — legacy / back-compat

Full back-compat: absent both a pointer and the well-known file, resolution is unchanged.

Findings folded in (inbox item 20260723-163434):

• F2 — the well-known default path (lib/resolve-convention-pattern.sh + synced guardrails mirror + config-resolution.md).
• F1 — setup recommends the neutral SSOT as the default when a second enforcement consumer (commit-msg hook / CI title check) is detected.
• F3 — setup check gains drift probes: broken pointer/neutral file → FAIL; resolved neutral file shadowing stale markdown-H2 → WARN.
• F4 — neutral-YAML preamble trimmed to a 1–2 line header.

Design + re-anchor rationale (reuse-or-replace, recheck-against-upstream, reason-dont-recite, point-dont-copy): docs/topics/commit-convention-well-known-path/design-resolution.md.

Scope note (honest): F3 closes silent severance for well-known-default repos; a repo that relocates via convention_source and then loses the pointer still falls through per-key to markdown — check surfaces it, but prevention there is bounded by design.

Version bumps: source-control 0.24.0 → 0.25.0; guardrails 0.13.0 → 0.14.0 (forced by the sync --check-bump gate — vendored resolver changed).

Test plan

• bash lib/resolve-convention-pattern.test.sh — 56/56 (added 8 well-known-path cases: resolves-with-no-pointer, wins-over-markdown, explicit-overrides-well-known, fail-closed on broken well-known, per-key markdown fallback, back-compat).
• scripts/sync-resolve-convention-pattern.sh --check — mirror byte-identical.
• scripts/sync-resolve-convention-pattern.sh --check-bump origin/main — guardrails bumped.
• scripts/check-changelog-parity.sh --check-bump origin/main — both CHANGELOGs updated.
• shellcheck clean on the resolver + mirror.

Related

• Closes source-control setup: neutral convention SSOT well-known default path + config surface (F1–F4) #1184
• Inbox item: 20260723-163434-source-control-setup-convention-default-and-config-surface
• Iterates on source-control: neutral tool-agnostic convention SSOT — author-directed reopen of #913 #1141 (neutral convention SSOT); reopens the commit-convention seam's V1 "no well-known search" decision (both recorded reasons engaged).

🤖 Generated with Claude Code

[claude]

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

---

Code Review: feat(source-control): well-known default path for neutral convention SSOT (F1–F4)

• Gather context and understand request
• Review lib/resolve-convention-pattern.sh (core resolver changes)
• Review lib/resolve-convention-pattern.test.sh (test coverage)
• Review plugins/source-control/skills/setup/SKILL.md (F1/F3 changes)
• Review plugins/source-control/skills/setup/reference/apply-convention.md
• Review plugins/source-control/reference/config-resolution.md
• Review guardrails sync mirror
• Review documentation and changelogs
• Post findings

---

Summary

Overall: LGTM with minor observations. The implementation is correct, security-hardened, and thoroughly documented. The 3-rung precedence is consistently mirrored across the resolver and all documentation surfaces, and the fail-closed contract is maintained for the well-known path. Minor observations below, none of which block merge.

---

Core resolver logic (lib/resolve-convention-pattern.sh lines 54–151)

The well-known probe is clean:

readonly WELL_KNOWN_NEUTRAL="docs/conventions/source-control/commit-convention.yml"
...
if [[ -z "$ptr" && -f "$repo_root/$WELL_KNOWN_NEUTRAL" ]]; then
  ptr="$WELL_KNOWN_NEUTRAL"
fi

The constant value docs/conventions/source-control/commit-convention.yml safely clears all the rejection patterns in the subsequent case block (/*, [A-Za-z]:*, *\\*, *..*). Critically, the well-known path then travels through the same symlink/canon/dialect checks as any explicit pointer — no shortcut that could create a privilege difference between the two rungs. The fail-closed contract (broken well-known file → exit 1, never fall back to stale markdown) is correctly maintained. ✓

One note on the *..* pattern: it rejects any path whose string representation contains .. as a substring, which is broader than just ..-as-a-path-segment (e.g., v1..v2.yml would be rejected). This is intentionally conservative/safe and doesn't affect the well-known constant, but worth noting if a consumer ever tries to relocate via convention_source to a file with .. in its basename.

---

Security

No issues found across the security surface:

Check	Result
Symlink file escape (-L test)	✓ Caught, disabled
Symlinked directory segment (pwd -P canon check)	✓ Caught, disabled
POSIX absolute paths (/*)	✓ Rejected
Windows drive-letter paths ([A-Za-z]:*)	✓ Rejected
.. traversal (*..*)	✓ Rejected
Non-posix-ere dialect	✓ Fail-closed
grep injection in yaml_value / check_neutral_key	✓ All k values are hardcoded literals (subject_pattern, pr_title_pattern, dialect, convention_source)

---

Diagnostic message cosmetics (minor)

When the well-known path fails (e.g., wrong dialect), the error still says:

resolve-convention-pattern: convention_source file declares dialect 'pcre'; enforcement disabled.

A user whose repo has docs/conventions/source-control/commit-convention.yml but no explicit ## convention_source pointer will see a message referencing convention_source they never set. Functionally correct (enforcement is disabled as intended), but could prompt a confused "I never wrote convention_source?" question.

A small prefix like well-known convention file vs convention_source file would distinguish the two origins without changing the exit path. Not a blocker — the F3 check drift probe will surface this condition explicitly with better context before it hits a blocked commit in practice.

Consider this improvement →

---

Test coverage (lib/resolve-convention-pattern.test.sh lines 279–330)

The new section covers the cases that matter:

Test	Coverage
Well-known present, no pointer, no markdown → resolves	✓
Well-known + markdown → well-known wins (rung 2 > 3)	✓
Explicit pointer + well-known → explicit wins (rung 1 > 2)	✓
Well-known with broken dialect → exit 1 (fail closed)	✓
Well-known with empty key + stale markdown → exit 1 (fail closed)	✓
No well-known, no pointer, markdown present → markdown back-compat	✓
Well-known file omits a key → per-key markdown fallback	✓

There is no dedicated test for a symlinked well-known file — the existing symlink tests at lines 258–277 cover the explicit-pointer path. The general symlink guard in the resolver is shared and path-agnostic, so the behavior is correct; it's just not exercised for the well-known rung specifically. Worth a TODO if the test suite ever grows a CI symlink fixture.

Minor: the PR description says "8 well-known-path cases" but counting assert_eq/assert_exit calls in the new section yields 9 assertions (the broken-dialect and empty-key scenarios each emit two assertions). Not a bug.

---

F1 / F3 in SKILL.md and apply-convention.md

F1 (recommend neutral SSOT when second consumer detected) is correctly gated on inference step 2's hook detection, not on file presence — the gotcha section at line 209 ("Gate inference on the resolved value, never file presence") applies symmetrically here. The text at SKILL.md line 162–166 and apply-convention.md lines 344–348 are consistent.

F3 drift probes (SKILL.md lines 91–105): the two-condition design is sound. The WARN condition ("neutral file resolves AND markdown still carries the same key") correctly points at apply to retire the duplicate rather than silently inheriting the stale value. The FAIL condition ("broken pointer/neutral file") closes the gap where enforcement was silently disabled.

---

Cross-surface consistency

The 3-rung precedence is stated identically in:

• lib/resolve-convention-pattern.sh (enforcement resolver, both source and guardrails mirror)
• docs/conventions/commit-convention/README.md
• plugins/source-control/reference/config-resolution.md (drafting resolver)
• SKILL.md

No drift between surfaces. ✓

Guardrails mirror (plugins/guardrails/hooks/resolve-convention-pattern.sh) is byte-identical to the lib source. ✓

Version bumps are appropriate: source-control 0.24.0 → 0.25.0 (new feature) and guardrails 0.13.0 → 0.14.0 (forced by resolver change). ✓

---

[claude]

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

---

Security Review — PR #1185

• Gather PR diff and changed files
• Analyze shell scripts for injection surfaces (command, path, variable expansion)
• Check file path handling and trust boundaries
• Review plugin manifests for supply-chain / version issues
• Post findings

---

Summary

No CRITICAL or IMPORTANT issues. The security posture of the resolver is strong: the new well-known-path probe inherits all existing safety gates (absolute-path rejection, ..-traversal guard, symlink-file check, canonical-directory containment). One pre-existing SUGGESTION surfaces because the new well-known-path code path now exercises it via a second trigger.

---

SUGGESTION — Glob metacharacter expansion in canonical-directory containment check

Confidence: LOW · Pre-existing; newly exercised by the well-known-path code path this PR adds.

lib/resolve-convention-pattern.sh line 181 (identical in the guardrails mirror):

if [[ -z "$canon_root" || -z "$canon_dir" || "$canon_dir/" != "$canon_root/"* ]]; then

In [[ ]], the RHS of != is treated as a glob pattern. If the physical canonical repo-root path (pwd -P) happens to contain glob metacharacters — for instance a directory literally named myrepo[abc] — the RHS becomes /path/to/myrepo[abc]/* where [abc] is interpreted as a character class. A canon_dir of /path/to/myrepob (a sibling directory, outside the repo) would then match the pattern and bypass the containment check.

Exploitation chain for the well-known path (new in this PR):

1. Consumer repo root has a canonical path containing […] in a directory name (e.g. /ci/builds/project[dev]).
2. An attacker with write access to the repo creates docs/conventions/source-control/ as a symlink to a directory under /ci/builds/projectd/ (a sibling that matches the character class).
3. The neutral YAML file at that location is a plain regular file — passes the [[ -L "$NEUTRAL_FILE" ]] check.
4. cd "$(dirname "$NEUTRAL_FILE")" && pwd -P resolves to the symlinked target outside the repo.
5. The glob comparison incorrectly accepts the out-of-tree path, and the gate enforces a pattern from outside the repository.

Why confidence is LOW: pwd -P on typical systems returns paths without [, ], *, or ?; directory names with glob metacharacters are unusual enough that no realistic consumer is likely affected. Additionally, step 1 requires the attacker to also control the directory naming of wherever CI clones the repo.

Safe alternative — string prefix comparison without glob expansion:

canon_prefix="${canon_root}/"
if [[ -z "$canon_root" || -z "$canon_dir" || \
      "${canon_dir:0:${#canon_prefix}}" != "$canon_prefix" ]]; then

This is a pure string-prefix test; no characters in canon_root are ever interpreted as glob metacharacters.

---

Everything else reviewed — no issues

Area	Assessment
WELL_KNOWN_NEUTRAL constant (line 59)	readonly, no metacharacters, passes all downstream guards intact
Well-known probe (-f "$repo_root/$WELL_KNOWN_NEUTRAL")	Probes existence only; sets ptr to the constant, then goes through the same lexical + symlink + canon-dir safety chain as any explicit pointer
yaml_value's grep -E "^${k}:" (line 127)	$k is always one of dialect, subject_pattern, or pr_title_pattern — all hardcoded at call sites after the validated $key gate at lines 73–79; no user-controlled string reaches the regex
ERE safety-net grep -E -- "$value" </dev/null (line 285)	-- blocks flag injection; /dev/null input means ReDoS cannot trigger; exit-code gate handles invalid expressions
awk -v key="$k" in h2_value (line 100)	Value passed as an awk variable, not interpolated into awk source; file content never executed as awk code
Symlink check (lines 175–178)	Correctly rejects a symlink at the file level; the canon-dir check (with the suggestion above) handles symlinked directories
Plugin manifests (plugin.json)	Version bumps only (0.24.0→0.25.0, 0.13.0→0.14.0); no new dependencies, no new permissions, no pinning regressions
SKILL.md / apply-convention.md	Model instructions; shell command templates use $REPO_ROOT from git rev-parse --show-toplevel (trusted) with fixed paths — no injection surface

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d84323f3ce

ℹ️ 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 31s —— View job

---

Review — PR #1185 (follow-up commit 928cd9b)

• Gather context and understand request
• Review the follow-up commit diff vs prior review commit
• Analyze the git-tracked guard implementation
• Analyze the canon-prefix hardening
• Cross-check test coverage for the new guards
• Post findings

---

Summary

All three review items (Codex P1, security-review suggestion, Codex P2) are correctly implemented. No new issues introduced. Ready to merge.

---

Codex P1 — git-tracked guard ✓

lib/resolve-convention-pattern.sh line 157–159:

if [[ -z "$ptr" && -f "$repo_root/$WELL_KNOWN_NEUTRAL" ]] &&
  git -C "$repo_root" ls-files --error-unmatch -- "$WELL_KNOWN_NEUTRAL" >/dev/null 2>&1; then
  ptr="$WELL_KNOWN_NEUTRAL"
fi

The >/dev/null 2>&1 suppression is correct: --error-unmatch exits 1 for untracked files (already the right effect); the 2>&1 swallows the "did not match" message and any "not a git repo" error from when git isn't available. Both cases fall through to the markdown H2 — "fail toward prior behavior, never toward the hole." ✓

The && compound is inside an if condition, so set -e doesn't apply to git ls-files failure here. ✓

The policy-floor rationale in the comment (line 150–156) correctly articulates why rung 2 needs a stronger trust signal than rung 3 (markdown has a tracked opt-in by definition; the well-known path does not).

Two new regression tests cover the critical cases:

Test	Behavior verified
untracked file ignored -> markdown wins	untracked YAML at well-known path + tracked markdown → markdown wins
untracked file + no markdown -> exit 1	untracked YAML + no markdown → no enforcement (exit 1, not activated)

The track() helper (git init -q || true && git add -- "$path") is correct: it uses || true so tests don't fail on a re-init, and git add with a relative path in the repo root is precise. One observation: this means six existing well-known tests now require a real git binary at test time — this was already implied by the newrepo fixture pattern but now it's load-bearing. If CI ever runs these in a minimal container without git, the tests that call track() will silently skip (the || true suppresses errors) and then fail because ls-files won't find the file. Worth a note in the test file header if the suite ever runs in such an environment, though not a blocker given this repo's CI setup.

---

Security suggestion — canon-prefix hardening ✓

lib/resolve-convention-pattern.sh line 194–196:

canon_prefix="$canon_root/"
canon_dir_slash="$canon_dir/"
if [[ -z "$canon_root" || -z "$canon_dir" || "${canon_dir_slash:0:${#canon_prefix}}" != "$canon_prefix" ]]; then

Correct. The bash substring expansion ${canon_dir_slash:0:${#canon_prefix}} extracts exactly ${#canon_prefix} bytes from canon_dir_slash and compares them as a plain string — no glob characters in canon_root can be interpreted as a pattern. The two intermediate variables are script-level (not function-local), but they're only used in this one check and don't shadow anything. ✓

Guardrails mirror confirmed byte-identical to the lib source. ✓

---

Codex P2 — eval case 18 ✓

The updated expectation in evals.json now requires:

• The skill writes at the well-known default path without a convention_source pointer.
• A pointer is written only for non-default relocation.
• The verification step confirms the file is staged/tracked (not just present), closing the loop with the new policy floor.

The rename from apply-neutral-ssot-migration-retires-duplicates to apply-neutral-ssot-well-known-default-retires-duplicates is precise. ✓

---

One open cosmetic observation (carry-over, not a blocker)

Noted in the prior review: when the well-known path fails (e.g. wrong dialect), the diagnostic still reads convention_source file declares dialect '...' even when no explicit convention_source was set. The follow-up commit didn't address this — consistent with the prior review's assessment that F3 check surfacing the condition first makes this low-priority. Confirming it remains the only open non-blocking item.

[claude]

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

---

Security Review — PR #1185 (HEAD: 928cd9b)

• Gather context and understand request
• Fetch PR diff (HEAD~2...HEAD covering both commits d84323f and 928cd9b)
• Review shell scripts for injection/path/logic/TOCTOU vulnerabilities
• Review git-tracking gate and canon-prefix fix
• Review test coverage for new security-critical paths
• Post findings

---

Summary

No CRITICAL or IMPORTANT issues. The fixup commit (928cd9b) correctly addresses both security findings from the prior review pass (the Codex P1 untracked-file override and the glob-metacharacter SUGGESTION on the canon-prefix check). One low-confidence SUGGESTION remains on the git ls-files call path; one observation on the TOCTOU window is included for completeness and is mitigated by downstream fail-closed checks.

---

Addressed by 928cd9b

Prior finding	Status
Codex P1 — untracked/gitignored file at well-known path could silently override tracked markdown policy or disable enforcement	✓ Fixed — git ls-files --error-unmatch gate added at line 157–159; tests for untracked-with-markdown and untracked-no-markdown both confirm fail-closed/fall-through behavior
Previous Claude SUGGESTION — glob metacharacter in != "$canon_root/"* pattern comparison	✓ Fixed — replaced with pure string prefix test at lines 194–196; ${canon_dir_slash:0:${#canon_prefix}} != $canon_prefix is literal byte comparison with no glob interpretation

---

Remaining: SUGGESTION — git ls-files with leading-dash repo_root

Confidence: LOW

lib/resolve-convention-pattern.sh line 158:

git -C "$repo_root" ls-files --error-unmatch -- "$WELL_KNOWN_NEUTRAL" >/dev/null 2>&1

If $repo_root began with a - (a directory named -foo), git -C -foo would attempt to interpret -foo as a flag sequence; git would error out with an invalid option message and exit non-zero. The condition would fail, the well-known rung would be skipped, and resolution would fall through to the markdown H2 — the safe prior behavior. This is not exploitable for policy override, but an adversarially named repository root could cause the well-known rung to be silently bypassed when it shouldn't be.

In practice $repo_root is supplied by git rev-parse --show-toplevel from the calling hook, which returns an absolute path starting with / — leading-dash is not a realistic concern. Adding -- before "$repo_root" is not possible here since -C consumes the next argument positionally (not ---separated), but the same hardening is achievable by validating the absolute path before the git call:

if [[ -z "$ptr" && -f "$repo_root/$WELL_KNOWN_NEUTRAL" && "$repo_root" == /* ]] &&
  git -C "$repo_root" ls-files --error-unmatch -- "$WELL_KNOWN_NEUTRAL" >/dev/null 2>&1; then

The "$repo_root" == /* guard also aligns with the TEAM_FILE construction at line 82, which implicitly assumes an absolute path.

---

TOCTOU between git ls-files and downstream checks (not a vulnerability)

Between git ls-files returning success and the subsequent [[ ! -f "$NEUTRAL_FILE" ]] / [[ -L "$NEUTRAL_FILE" ]] / canon-dir checks, the file could theoretically be replaced. Each mutation is caught fail-closed:

• File deleted: [[ ! -f ]] catches it → exit 1 ✓
• Replaced with symlink: [[ -L ]] catches it → exit 1 ✓
• Replaced with a regular file pointing to canon-dir outside repo: substring check catches it → exit 1 ✓

No TOCTOU exploitation path exists; the downstream chain handles every post-ls-files mutation safely.

---

Everything else reviewed — no issues

Area	Result
WELL_KNOWN_NEUTRAL constant (line 59)	readonly; docs/conventions/source-control/commit-convention.yml contains no metacharacters; passes all downstream lexical guards (/* | [A-Za-z]:* | *\\* | *..*) intact
Well-known probe (-f "$repo_root/$WELL_KNOWN_NEUTRAL")	Double-quoted — no word splitting, no glob expansion; safe
git ls-files --error-unmatch -- "$WELL_KNOWN_NEUTRAL"	$WELL_KNOWN_NEUTRAL is a readonly constant, -- guards against flag injection; git exits non-zero on any failure (git unavailable, not a repo, untracked) — all fail toward the prior markdown-H2 behavior
Canon-prefix fix (${canon_dir_slash:0:${#canon_prefix}})	Pure string substring comparison; no pattern characters in canon_root are ever interpreted as globs
yaml_value grep -E "^${k}:"	$k is always a hardcoded literal at all call sites; no user-controlled string reaches the regex
ERE safety-net grep -E -- "$value" </dev/null	-- blocks flag injection; /dev/null eliminates ReDoS; exit-code gate handles invalid expressions
awk -v key="$k" in h2_value	Value passed as awk variable, not interpolated into awk source
Symlink check (line 184)	Regular-file-only requirement on the neutral file stands for both the explicit-pointer and well-known rungs
Plugin manifests	Version bumps only (0.24.0→0.25.0, 0.13.0→0.14.0); no new permissions, no new remote dependencies
Guardrails mirror	Byte-identical to lib source (confirmed by sync script); the same analysis applies
SKILL.md / apply-convention.md	Model instructions with no shell execution surfaces; git rev-parse --show-toplevel usage is trusted and fixed-path
evals.json	Eval expectation update only; no execution surface

[kyle-sexton]

Also applied the security-review hardening (advisory): the symlink canon check now uses a pure string-prefix comparison instead of a "$canon_root/"* glob, so a glob metacharacter in the physical repo path can't be interpreted rather than compared literally. (928cd9b)