chore(#458): whitelist sync type across branch/commit/PR validators#460
Conversation
The /release-sync skill documents a `sync`-typed shape (branch `sync/...`, commit `sync: ...`, PR title `sync(#N): ...`) but `sync` was absent from all three type whitelists and the hooks' hardcoded fallbacks — so every sync artifact was rejected, forcing the v2.2.0 release sync to retitle everything to `chore`. - Add `sync` to branch.type_whitelist, commit.type_whitelist, and pr.title_type_whitelist in project-config.defaults.json - Align the three hardcoded fallback strings with their config lists (each gains `sync`; commit fallback was also missing `spike`, pr fallback was missing `release`+`spike` — fixed the drift) - Exempt `sync/main-to-dev-after-vN.N.N` branches from the ticket-id requirement (same narrow exception as `release/vN.N.N` — the release being synced is the ticket) - New regression test test_sync_type_whitelisted.sh (10 cases: behavioural branch+commit acceptance + control + static config/fallback assertions) - Note the whitelisting in release-sync SKILL.md Refs #458
atlas-apex
left a comment
There was a problem hiding this comment.
Code Review: PR #460
Commit: 28bc67be566c9c777a3a72af3668f29d434c3b54
Summary
Whitelists the sync conventional-commit type across all three validators (branch / commit / PR-title), so the /release-sync skill's documented sync/-branch, sync:-commit, sync(#N):-PR-title shape actually passes the gates it ships with. Closes the self-inconsistency that forced the v2.2.0 release sync to retitle every artifact to chore. Also brings the three hardcoded fallback strings into line with their config whitelists (fixing pre-existing spike/release drift) and adds a narrow ticket-id exemption for sync/main-to-dev-after-vN.N.N branches, mirroring the existing release/vN.N.N exemption.
Checklist Results
- ✅ Architecture & Design: Pass
- ✅ Code Quality: Pass
- ✅ Testing: Pass (10/10, 15/15, 9/9, 8/8 verified locally)
- ✅ Security: Pass (no over-reach; control case proves the gate still blocks unknown types)
- ✅ Performance: N/A (validator/config only)
- ✅ PR Description & Glossary: Pass
- ✅ Summary Bullet Narrative: Pass (bold lead + what + why on every bullet)
- ✅ Technical Decisions (AgDR): N/A — implements AgDR-0052; no new decision
- ✅ Adopter Handbooks: N/A (no handbooks loaded for this diff)
Verification performed
- Exemption regex narrowness —
^sync/main-to-dev-after-v[0-9]+\.[0-9]+\.[0-9]+(-rc[0-9]+)?$exempts only the canonical shape. Confirmed it rejectssync/arbitrary-branch,sync/main-to-dev-after-v2.2(incomplete semver),sync/main-to-dev-after-v2.2.0-extra(trailing junk), andsync/main-to-dev-after-vfoo. Structurally identical to the siblingrelease/vN.N.Nregex immediately above it. Appropriately narrow — no accidental blanketsync/exemption. - Fallback ⇄ config alignment — all three fallback strings now match their config whitelists token-for-token:
- branch:
feature|fix|refactor|chore|docs|test|spike|ci|build|perf|sync✓ - commit:
feat|fix|refactor|test|docs|chore|style|perf|build|ci|revert|spike|sync✓ - pr:
feat|fix|docs|style|refactor|perf|test|build|ci|chore|revert|release|spike|sync✓
Thespike(commit) andrelease+spike(PR) additions are genuine pre-existing drift fixes — the config already carried those types; only the bare-checkout fallback lagged. Correct to fix while here.
- branch:
- No over-reach — diffing config against base
devshows the sole config change issyncadded to each of the three whitelists. No other gate loosened, no unintended exemption introduced. - Tests pass — ran all five suites from the repo root:
test_sync_type_whitelisted.sh→ 10/10test_validate_branch_name_pushref.sh→ 15/15test_validate_commit_format_heredoc.sh→ 9/9test_validate_pr_create_head.sh→ 8/8
Issues Found
None.
Suggestions
- Test control case is meaningful but could be slightly stronger. The
wibble:control proves an arbitrary unknown type still blocks the commit validator — good. Since the whole point of this PR is thesynctoken specifically, a complementary negative on the branch validator (e.g. a baresync/foothat is NOT themain-to-dev-after-vN.N.Nshape must still fall through to the normal type-or-ticket-id path) would directly lock in the exemption's narrowness. Not blocking — the regex narrowness is self-evident from the anchored pattern and I verified it manually above. - Re: the PR-title validator gap you flagged. Substituting a static assertion (config + fallback contain
sync) for a behavioural test is acceptable here: the PR-title hook additionally performs ticket-existence checks requiring network, so a hermetic behavioural case isn't free. The static assertion guards the regression that actually occurred (the missing token), and the shared type-regex construction is identical across all three hooks. Reasonable trade-off.
Verdict
APPROVED
The fix is surgical, the regex is correctly narrow, the fallback/config alignment is exact, and the test guards the regression at both the behavioural and static layers. No AgDR required — this implements the already-recorded AgDR-0052 skill design and resolves a self-inconsistency rather than making a new decision. Note this PR targets dev (release-cut model), so GitHub auto-close won't fire on Refs #458; close #458 manually on dev-merge per the release-cut norm.
🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: 28bc67be566c9c777a3a72af3668f29d434c3b54
atlas-apex
left a comment
There was a problem hiding this comment.
Code Review: PR #460
Commit: 28bc67be566c9c777a3a72af3668f29d434c3b54
Summary
Whitelists the sync conventional-commit type across all three validators (branch / commit / PR-title), so the /release-sync skill's documented sync/-branch, sync:-commit, sync(#N):-PR-title shape actually passes the gates it ships with. Closes the self-inconsistency that forced the v2.2.0 release sync to retitle every artifact to chore. Also brings the three hardcoded fallback strings into line with their config whitelists (fixing pre-existing spike/release drift) and adds a narrow ticket-id exemption for sync/main-to-dev-after-vN.N.N branches, mirroring the existing release/vN.N.N exemption.
Checklist Results
- ✅ Architecture & Design: Pass
- ✅ Code Quality: Pass
- ✅ Testing: Pass (10/10, 15/15, 9/9, 8/8 verified locally)
- ✅ Security: Pass (no over-reach; control case proves the gate still blocks unknown types)
- ✅ Performance: N/A (validator/config only)
- ✅ PR Description & Glossary: Pass
- ✅ Summary Bullet Narrative: Pass (bold lead + what + why on every bullet)
- ✅ Technical Decisions (AgDR): N/A — implements AgDR-0052; no new decision
- ✅ Adopter Handbooks: N/A (no handbooks loaded for this diff)
Verification performed
- Exemption regex narrowness —
^sync/main-to-dev-after-v[0-9]+\.[0-9]+\.[0-9]+(-rc[0-9]+)?$exempts only the canonical shape. Confirmed it rejectssync/arbitrary-branch,sync/main-to-dev-after-v2.2(incomplete semver),sync/main-to-dev-after-v2.2.0-extra(trailing junk), andsync/main-to-dev-after-vfoo. Structurally identical to the siblingrelease/vN.N.Nregex immediately above it. Appropriately narrow — no accidental blanketsync/exemption. - Fallback ⇄ config alignment — all three fallback strings now match their config whitelists token-for-token:
- branch:
feature|fix|refactor|chore|docs|test|spike|ci|build|perf|sync✓ - commit:
feat|fix|refactor|test|docs|chore|style|perf|build|ci|revert|spike|sync✓ - pr:
feat|fix|docs|style|refactor|perf|test|build|ci|chore|revert|release|spike|sync✓
Thespike(commit) andrelease+spike(PR) additions are genuine pre-existing drift fixes — the config already carried those types; only the bare-checkout fallback lagged. Correct to fix while here.
- branch:
- No over-reach — diffing config against base
devshows the sole config change issyncadded to each of the three whitelists. No other gate loosened, no unintended exemption introduced. - Tests pass — ran all five suites from the repo root:
test_sync_type_whitelisted.sh→ 10/10test_validate_branch_name_pushref.sh→ 15/15test_validate_commit_format_heredoc.sh→ 9/9test_validate_pr_create_head.sh→ 8/8
Issues Found
None.
Suggestions
- Test control case is meaningful but could be slightly stronger. The
wibble:control proves an arbitrary unknown type still blocks the commit validator — good. Since the whole point of this PR is thesynctoken specifically, a complementary negative on the branch validator (e.g. a baresync/foothat is NOT themain-to-dev-after-vN.N.Nshape must still fall through to the normal type-or-ticket-id path) would directly lock in the exemption's narrowness. Not blocking — the regex narrowness is self-evident from the anchored pattern and I verified it manually above. - Re: the PR-title validator gap you flagged. Substituting a static assertion (config + fallback contain
sync) for a behavioural test is acceptable here: the PR-title hook additionally performs ticket-existence checks requiring network, so a hermetic behavioural case isn't free. The static assertion guards the regression that actually occurred (the missing token), and the shared type-regex construction is identical across all three hooks. Reasonable trade-off.
Verdict
APPROVED
The fix is surgical, the regex is correctly narrow, the fallback/config alignment is exact, and the test guards the regression at both the behavioural and static layers. No AgDR required — this implements the already-recorded AgDR-0052 skill design and resolves a self-inconsistency rather than making a new decision. Note this PR targets dev (release-cut model), so GitHub auto-close won't fire on Refs #458; close #458 manually on dev-merge per the release-cut norm.
🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: 28bc67be566c9c777a3a72af3668f29d434c3b54
…#460) The /release-sync skill documents a `sync`-typed shape (branch `sync/...`, commit `sync: ...`, PR title `sync(#N): ...`) but `sync` was absent from all three type whitelists and the hooks' hardcoded fallbacks — so every sync artifact was rejected, forcing the v2.2.0 release sync to retitle everything to `chore`. - Add `sync` to branch.type_whitelist, commit.type_whitelist, and pr.title_type_whitelist in project-config.defaults.json - Align the three hardcoded fallback strings with their config lists (each gains `sync`; commit fallback was also missing `spike`, pr fallback was missing `release`+`spike` — fixed the drift) - Exempt `sync/main-to-dev-after-vN.N.N` branches from the ticket-id requirement (same narrow exception as `release/vN.N.N` — the release being synced is the ticket) - New regression test test_sync_type_whitelisted.sh (10 cases: behavioural branch+commit acceptance + control + static config/fallback assertions) - Note the whitelisting in release-sync SKILL.md Refs #458 Co-authored-by: me2resh <ahmed.abdelaliem@gmail.com>
Summary
syncconventional-commit type across all three validators (branch / commit / PR-title) so the/release-syncskill's documented shape —sync/branch,sync:commit,sync(#N):PR title — actually passes the gates. Before this,syncwas absent from every whitelist, so the v2.2.0 release sync had to retitle every artifact tochore(the merge commit only slipped through becausegit merge -mbypasses the commit-format hook).sync; while there, fixed pre-existing drift — the commit fallback was missingspike, the PR fallback was missingrelease+spike. A bare checkout (no jq/config) now honours the same types the config declares, so config and fallback can't silently diverge.sync/main-to-dev-after-vN.N.Nbranches from the ticket-id requirement — the same narrow exceptionrelease/vN.N.Nalready has. These branches don't carry a ticket id because the release being synced is the ticket. The PR title still references a live OPEN ticket viasync(#N):(validate-pr-create checks ticket existence independent of the type).test_sync_type_whitelisted.sh, 10 cases): behavioural acceptance ofsync-typed branch + commit input, a control case proving an unknown type still blocks, and static assertions thatsyncis present in all three config whitelists AND all three hardcoded fallbacks.Why
Surfaced while running
/release-sync v2.2.0(the first real exercise of the skill end-to-end against production state). The skill's SKILL.md prescribes async-typed shape, but the framework's own validators rejected it — a self-inconsistency where the documented happy path was impossible to follow. This closes the gap so the next release sync runs as written.Root-cause ticket: #458. (Sibling ticket #459 tracks a separate
/release-syncissue —--squashdefeating ancestry closure — not addressed here.)Testing
bash .claude/hooks/tests/test_sync_type_whitelisted.sh— 10/10 passbash .claude/hooks/tests/test_validate_branch_name_pushref.sh— 15/15 (no regression)bash .claude/hooks/tests/test_validate_commit_format_heredoc.sh— 9/9 (no regression)bash .claude/hooks/tests/test_validate_pr_create_head.sh— 8/8 (no regression)bash .claude/hooks/tests/test_validate_pr_create_upstream.sh— 7/7 (no regression)sync/main-to-dev-after-v2.2.0branch, async: ...commit, and async(#N): ...PR title all pass their validators; an unknown type still blocksGlossary
synctype/release-syncfor its main→dev sync artifacts. Now whitelisted alongsidefeat/fix/release/etc..claude/project-config.defaults.json(branch.type_whitelist,commit.type_whitelist,pr.title_type_whitelist) and enforced by the branch / commit / PR-title validators.release/vN.N.N) where a branch may skip the{type}/{TICKET-ID}-{desc}shape. Extended tosync/main-to-dev-after-vN.N.N.Refs #458