feat(governance): ADR-003 enforcement hooks — commit-msg, branch-naming, pickup-issue skill (#186) - #679
feat(governance): ADR-003 enforcement hooks — commit-msg, branch-naming, pickup-issue skill (#186)#679scottschreckengaust wants to merge 11 commits into
Conversation
Self-review (
|
✅ Acceptance summary (for the reviewer)#186 — ADR-003 enforcement hooks, implementing the enforcement table rows that were marked "Planned".
Two real defects fixed during self-review: the PR body
🔀 Merge guidance (for the reviewer)Independent — no ordering constraint. Cluster 🤖 orchestrator note (agent) — promotion is orchestrator-driven; merge remains a human action. |
28e9a8a to
d2f6035
Compare
…ng, pickup-issue skill (#186) Implements the three "Planned" ADR-003 enforcement rows (docs/decisions/ ADR-003-contribution-governance.md enforcement table) as offline-capable, no-new-dependency hooks + a Claude Code skill: - commit-msg hook (Tier 0): scripts/hooks/check-commit-msg.mjs rejects a commit whose message carries no issue reference (Refs/Fixes/Closes #N, the GitHub closing-keyword family + Ref/Refs). Wired as a commit-msg-type local hook; `commit-msg` added to default_install_hook_types. A bare `#N` with no keyword does not satisfy the gate. - branch-name hook (pre-push): scripts/hooks/check-branch-name.mjs rejects a branch not matching (feat|fix|chore|docs)/<issue-number>-<desc>. Exempts `main`, `dependabot/*`, and the detached-HEAD sentinel (ADR did not enumerate exemptions; minimal set documented in the ADR + PR body). - pickup-issue skill: docs/abca-plugin/skills/pickup-issue/SKILL.md — an agent-workflow gate that hard-fails without an approved, assigned issue before implementation. Advertised in the plugin SessionStart hook + README. Both scripts are plain Node ESM with node --test unit tests (23 cases, adversarial: missing ref, wrong prefix, missing issue number, comment-only ref). No new hook-runner or third-party tool added — reuses the existing prek/ pre-commit framework and Node. Deferred AC (flagged, not dropped): the pre-push Tier 1 `approved`-label `gh` check remains "Planned" — it needs a network call at push time, which this offline hook set intentionally avoids. The pickup-issue skill covers the approved-label gate at the agent-workflow layer in the interim. Docs: flipped the three implemented ADR-003 rows Planned -> Implemented (#186), documented exemptions + reference forms; updated CONTRIBUTING.md hooks list; regenerated Starlight mirrors via docs:sync. Closes #186 Co-authored-by: Claude Opus 4.8 <[email protected]>
…/boundary (#186) Addresses the /review_pr blocking finding: the new hook unit tests were orphaned — no CI job or mise task ran scripts/hooks/*.test.mjs, so the Tier-0 governance regexes could silently regress. - Add mise task `test:hooks` (node --test scripts/hooks/*.test.mjs; explicit glob avoids Node 22's spurious directory-arg failure) and add it to the `build` DAG so `mise run build` (which CI runs) executes it. - Add the cheap test cases the reviewers flagged: GH-N reference form (a documented-but-untested accepted form), no-separator rejection (Closes#5), word-boundary rejection (refixes), and CRLF handling (body ref survives, comment-only ref does not). 23 -> 27 tests, all green. Refs #186
…te (#186) The Tier 0 commit-msg hook rejected any message without an issue reference, including git-generated merge and revert commits. That made a routine `git merge origin/main` — which CONTRIBUTING instructs contributors to run to refresh a branch — impossible to complete. Exempt auto-generated subjects (Merge/Revert/fixup!/squash!/amend!), keyed off the subject line only so a normal commit that merely mentions "merge" in prose still requires a ref. Mirrors the default ignore set of conventional commit-msg linters. Refs #186. Co-authored-by: Claude Opus 4.8 <[email protected]>
…186) Document the commit-msg hook's exemption for git-generated commits so the ADR matches the implementation. Regenerated the Starlight mirror. Refs #186. Co-authored-by: Claude Opus 4.8 <[email protected]>
d2f6035 to
201a07e
Compare
|
Really like the shape of this — hooks + skill + ADR docs moving together, and the test coverage on the pure
Otherwise LGTM once these are handled — nice work. |
…vernance repo (#679 review) Addresses the three findings from the #679 review. 1. Branch name — validated only `git rev-parse --abbrev-ref HEAD`, so `git push origin feat/123-x:refs/heads/random-name` from a `main` checkout published a non-conforming remote ref while HEAD read as the exempt `main`. `resolveBranchesToCheck` now derives the names from the push itself, with precedence argv > git's native pre-push stdin > prek's PRE_COMMIT_* vars > HEAD, and validates BOTH sides of every refspec (the local name is the work, the remote name is what a PR is opened from). Tag-only pushes and deletions resolve to "nothing to check" rather than falling through to HEAD, which would validate a branch the push never touched. The review's remedy as stated — forward stdin/args from the hook entry — cannot work under this wiring. prek's git shim is what git invokes, so prek, not the hook, is git's stdin reader: it consumes the ref lines to build its own file list, forwards neither them nor git's `<remote-name> <remote-url>` argv to the entry, and re-publishes only the FIRST ref pair as PRE_COMMIT_LOCAL_BRANCH / PRE_COMMIT_REMOTE_BRANCH. Confirmed with a probe hook in a throwaway repo: the entry saw `ARGS:[]` and empty stdin. The stdin parser is kept anyway so a native `.git/hooks/pre-push` install covers every ref, and prek's first-pair-only limitation is documented in the script header and in `.pre-commit-config.yaml` instead of being papered over. 2. Commit message — the exemption matched any subject merely starting with `Merge` or `Revert`, so authored work like `Merge behavior for schema handling` or `Revert reviewer nudge copy` skipped the issue-reference requirement entirely. Now scoped to the exact subject forms git and GitHub generate, plus the `fixup! ` / `squash! ` / `amend! ` autosquash prefixes (trailing space required, so `squashed the bug` stays subject to the gate). `branch(es)?` is kept because git's octopus merge writes `Merge branches 'a' and 'b'`. Replaying both regexes over all 313 subjects on `main` flips exactly one commit from exempt to gated — `merge origin/main into mise-migration; drop docs/yarn.lock` — authored prose rather than a generated merge subject, i.e. the bypass this closes. 3. pickup-issue skill — unpinned `gh issue` calls resolved the repository from the git remotes. CONTRIBUTING tells contributors to work from a fork, so those calls could land on the contributor's own copy, where they can grant themselves the `approved` label and self-bypass the gate. Every call is now pinned with `--repo "$REPO"`, defaulting to aws-samples/sample-autonomous-cloud-coding-agents, and a new Step 0 hard-fails when the resolved governance repo is a fork. Reuse in other repositories: `ABCA_GOVERNANCE_REPO` is the single runtime knob, deliberately the only one, because it is the one with a security consequence — and Step 0's fork check keeps it from becoming a bypass in its own right. The remaining assumptions (approval label, branch pattern, blocker markers, blocking-dependency source, start signal, governing document) are documented as a knob table that a forking project edits once in its own copy. Tests: 50 across the two hook suites (`mise run test:hooks`), covering ref resolution precedence, both refspec sides, multi-ref and deletion pushes, the review's exact bypass scenario, and every merge/revert/autosquash subject form. Refs #186
|
All three findings are fixed in 1 — one correction on the suggested remedy. The bypass is real and now closed, but "read the ref list from stdin / forward stdin and args" can't work under this wiring. prek's installed git shim is So the fix reads the refs from a precedence chain (
2 — agreed, and slightly wider than proposed. Two additions to the suggested pattern. Keeping Worth recording how this was checked — replaying the old and new regexes over all 313 commit subjects on Lowercase verb, semicolon, editorial clause — authored prose, not a subject git generated (git would have written 3 — default pinned, plus the reuse answer. Every On adopting gh repo view "$REPO" --json isFork,nameWithOwner \
--jq 'if .isFork then error("governance repo \(.nameWithOwner) is a fork") else .nameWithOwner end'Verified both branches against live repos — a genuine fork exits 1 with Minor: the review cites Disclosure on this push. |
The `catch` in `readRefListFromStdin` degraded every fd-0 read failure to '', which `semgrep.ts-silent-success-masking` (AI004) flagged as a new finding on this branch. It is a real bypass, not a lint nit: '' means "no ref list", so the resolver falls through to HEAD — and on a `main` checkout HEAD is exempt. A masked read failure would therefore *pass* the branch-name gate this PR exists to enforce. Only EAGAIN (empty non-blocking pipe) and EBADF (fd 0 closed) now degrade to '' — the two cases that genuinely mean "nothing on stdin", which is the normal state under prek and on a TTY. Any other errno is re-thrown, and the read moved inside `main`'s `try` so it surfaces as the existing clean exit 2 rather than an uncaught throw. Verified the rule is live rather than trusting a clean scan: it reports the finding against the pre-fix file and none against the new one. Hook suites 50 -> 54. Refs #186
Round 2 — findings addressed; one design questionAll three items from my earlier review are genuinely fixed, and two of the fixes are better than what I suggested. The remaining question is not about the implementation. It is about whether "every PR references an issue" should be enforced as an absolute, which I no longer think it should. The escape hatch already exists, undocumentedBoth hooks are client-side. That matters on ADR-003's own terms. The ADR's stated rationale for requiring an issue is that governance needs "a durable, reviewable artifact… not a transient conversation." A gate whose only escape is Where an issue is net-negativeThree classes, and the argument is not convenience: Mechanical changes — lockfile re-resolves, formatter churn, the generated Starlight mirrors this PR itself regenerates via Unblocking a red trunk — the gate's cost is paid while PR precedes issue — worth keeping the issue here, since that is where rationale lives at a stable URL, and an issue written from a finished, verified PR has known-true acceptance criteria rather than guessed ones. But demanding it before the branch exists is ceremony, and the remedy at push time is the expensive one: a PR is bound to its head ref, so renaming means close and reopen, losing the review threads. Suggested shapeEnumerate the exempt cases in ADR-003 rather than leaving them to a maintainer's judgment. Rule-based fits the grain of what you have already built — The condition is that an exemption must be derivable from the diff, not self-asserted, or
Only the last row has a soft edge, and I would leave it soft rather than build follow-up machinery around it: the PR body says why there is no issue, and that is the artifact. This also argues for where the real gate lives. A required PR check — body links an open approved issue, or the diff satisfies an enumerated exemption — is unbypassable, fires once, and fires when the work is already asking for a reviewer's time. The hooks fire before you know whether the work is even PR-worthy, and are skipped with a flag. Worth noting that the Tier 1 Out-of-scope findings — codify what this PR already doesA related gap: nothing in ADR-003 says what to do when you are on issue #N and find something else. The "Notes / pre-existing findings (NOT fixed here — out of scope)" section in this PR description is the right answer, and it deserves a name in the ADR — a Findings section, each item with The distinction the ADR is missing is that a blocker is not out of scope. The stale-base merge in this very PR is the example: had that needed its own approved issue first, the approved work would have been unlandable pending a governance round-trip. Scope-freeze should read as covering deliverables, not incidental prerequisites. Smaller points on the current headThe commit-msg hook does not cross-check against the branch number — please keep it that way, deliberately.
Also minor: only To be clear on the ask: the three findings are resolved and I am not asking for more implementation. What I would like agreed before this merges is whether "no PRs without an issue" ( |
Summary
Implements the three ADR-003 governance enforcement mechanisms that were marked Planned — a Tier 0 commit-msg hook, a pre-push branch-name check, and a
pickup-issueClaude Code skill — as offline-capable, no-new-dependency hooks reusing the existing prek/pre-commit framework and Node.Closes #186
Root cause
ADR-003 (
docs/decisions/ADR-003-contribution-governance.md, enforcement table) defines governance rules that were prose-only and unenforced. Three table rows were Planned with no implementation:pickup-issue.pre-commit-config.yamlhad no commit-msg hook, no branch-naming hook, anddefault_install_hook_typeswas[pre-commit, pre-push](nocommit-msgtype).The fix
1. commit-msg hook (Tier 0) —
scripts/hooks/check-commit-msg.mjsRefs #NorFixes #N". Accepts the full GitHub closing-keyword family (Close/Closes/Closed,Fix/Fixes/Fixed,Resolve/Resolves/Resolved) plusRef/Refs, each followed by#N,GH-N, orowner/repo#N. A bare#Nwith no keyword does not pass (must be an intentional link, not incidental prose). Git comment lines (#-leading) are stripped before scanning.fixup!/squash!/amend!) are exempt, keyed off the subject line only — so a routinegit merge origin/main(which CONTRIBUTING tells contributors to run) is not blocked. Mirrors the default ignore set of conventional commit-msg linters. A normal commit that merely mentions "merge" in prose still requires a reference.commit-msg-type local hook;commit-msgadded todefault_install_hook_types.2. branch-name hook (pre-push) —
scripts/hooks/check-branch-name.mjs(feat|fix|chore|docs)/<issue-number>-<desc>(ADR-003 "No branches without an Issue"). Reads the current branch via git plumbing (git rev-parse --abbrev-ref HEAD); fails loud rather than defaulting to a pass.main(the trunk is not a feature branch),dependabot/*(bot-authored upgrade branches), and the detached-HEADsentinel. ADR-003 does not enumerate exemptions in prose — this is the minimal set needed so the trunk and machine-generated branches are not falsely rejected. Documented in the ADR "Branch-name exemptions" note + here.pre-push-type local hook.3.
pickup-issueskill —docs/abca-plugin/skills/pickup-issue/SKILL.mdhooks/hooks.json) and README skills table.Deferred AC (flagged, not dropped)
The pre-push Tier 1
approved-labelghcheck (a separate ADR-003 table row) remains Planned. It needs a networkghAPI call at push time, which this offline-capable hook set intentionally avoids; thepickup-issueskill covers theapproved-label gate at the agent-workflow layer in the interim. The ADR row and the "Progressive enforcement" note now say so explicitly. Recommend a follow-up issue. (ThePreToolUse: WriteClaude Code hook row also remains "Planned" — out of scope for #186.)Testing
Unit tests (Node's built-in
node --test, no new framework) — 31 cases, adversarial coverage (missing ref, wrong prefix, missing issue number, comment-only ref, bare#N, CRLF, GH-N, cross-repo, keyword-boundary, and the merge/revert/fixup exemptions):Hooks proven to fire (dogfooded on this very branch/commits):
Dogfood: every commit on this branch references #186 (or is the exempt merge commit) and passes the
adr003-commit-msghook; the branchfeat/186-adr003-hookspasses the branch-name hook.Gates: hook unit tests PASS (50/50) ·
mise //docs:build+drift-preventionPASS ·security:sast(semgrep, 0 findings) /security:gh-actions(zizmor, 0 findings) PASS with zero findings in the new scripts · gitleaks overorigin/main..HEADclean.Notes / pre-existing findings (NOT fixed here — out of scope)
#672main, so theorigin/main..HEADdiff previously showed 8.github/workflows/*.ymlfiles as spurious "downgrades" ofactions/checkout,jdx/mise-action,actions/setup-node, andconfigure-aws-credentialspins. This was an artifact of the stale base, not an authored change — resolved by mergingorigin/main(which brought PR chore(deps): actions: bump the all-actions group across 1 directory with 4 updates #672's newer pins in). The diff now contains only the 13 in-scope files.ec2:DescribeAvailabilityZonesnot authorized (local Isengard role lacks EC2 perms for the VPC AZ lookup) — environmental, not code; my diff touches zerocdk/files. Passes in CI. Re-verified for the review round: with the lookup satisfied locally (-c 'agentcore:availabilityZones=["us-east-1b","us-east-1c"]'for this repo's resolver, plus the built-inavailability-zonesprovider seeded into the gitignoredcdk/cdk.context.json),cdk synth -qexits 0 with zero errors and zero cdk-nag Error findings. Every othermise run buildtask passes:test:hooks50/50,cdk:test,cli:test,agent:test,cdk:eslint,cli:eslint,agent:typecheck,docs:build,docs:link-check, and all four drift checks (types-sync,constants-sync,coverage-thresholds-sync,transitive-pin-sync).Dependencies / related
bash/nodeunder the existing preklanguage: systemframework).docs/decisions/ADR-003+CONTRIBUTING.md; Starlight mirrors regenerated viamise //docs:sync.Review round 2 — fixes for the #679 review
All three findings addressed. Hook suites now 50/50 (
mise run test:hooks).1. Branch-name hook validated HEAD, not the refs being pushed
git push origin feat/123-x:refs/heads/random-namefrom amaincheckout published a non-conforming remote ref whilegit rev-parse --abbrev-ref HEADread as the exemptmain→ exit 0.resolveBranchesToChecknow derives the names from the push itself:argv[2]<local-ref> <local-sha> <remote-ref> <remote-sha>).git/hooks/pre-pushinstall — covers every pushed refPRE_COMMIT_LOCAL_BRANCH/PRE_COMMIT_REMOTE_BRANCHgit rev-parse --abbrev-ref HEADprek run --all-files --stage pre-push)Both sides of every refspec are validated — the local name is the work, the remote name is what lands on the remote and what a PR is opened from. Tag-only pushes and ref deletions resolve to "nothing to check" rather than falling through to HEAD, which would validate a branch the push never touched.
Correction to the review's suggested remedy. "Read the ref list from stdin / forward stdin and args" cannot work under this wiring. prek's git shim is
exec prek hook-impl --hook-type=pre-push -- "$@", so prek, not the hook, is git's stdin reader: it consumes the ref lines to compute its own file list and forwards neither them nor git's<remote-name> <remote-url>argv to the entry. It re-publishes only the first ref pair asPRE_COMMIT_LOCAL_BRANCH/PRE_COMMIT_REMOTE_BRANCH. Confirmed with a probe hook in a throwaway repo — the entry sawARGS:[]and empty stdin.So the env-var path is what actually closes the bypass here. The stdin parser is retained anyway so a native
.git/hooks/pre-pushinstall covers every pushed ref, and prek's first-pair-only limitation is documented in the script header and in.pre-commit-config.yaml(whose old comment claiming the script "reads the current branch" was itself misleading) rather than papered over. Reaching every ref under prek would mean bypassing prek with a native hook, which conflicts with prek owningcore.hooksPath.2. Commit-msg exemptions were far too broad
The old pattern exempted any subject merely starting with
MergeorRevert, so authored work skipped the issue-reference requirement. Now scoped to the exact subject forms git and GitHub generate, plus the autosquash prefixes with a required trailing space:/^(?:Merge (?:branch(?:es)?|remote-tracking branch(?:es)?|tag|commit|pull request)\b|Revert "|(?:fixup|squash|amend)! )/iMerge branch 'x'/… into y/… of <url>,Merge branches 'a' and 'b'(octopus — the review's suggested pattern would have broken this, hencebranch(es)?),Merge remote-tracking branch 'origin/main' into y,Merge tag 'v1',Merge commit '<sha>',Merge pull request #N from owner/branch,Revert "<subject>"(git revert always quotes), andfixup!/squash!/amend!.Merge behavior for schema handling,Revert reviewer nudge copy(both from the review),Merged the two configs,Reverting the nudge copy,squashed the bug,fixup the layout,amendment to the guide.Differential check against real history. Replaying the old and new regexes over all 313 commit subjects on
main: 11 were exempt under the old pattern, and exactly one flips to gated —merge origin/main into mise-migration; drop docs/yarn.lock (root lockfile). That is authored prose (lowercase verb, semicolon, editorial clause), not a subject git generated; git would have writtenMerge remote-tracking branch 'origin/main' into mise-migration. It is precisely the bypass class this closes. The other 10 are all GitHub'sMerge pull request #N from …. Retro-safe:commit-msgonly fires on new commits, so reclassifying a historical subject breaks nothing.3.
pickup-issueskill — unpinnedghcalls could resolve to the contributor's forkCONTRIBUTING tells contributors to "push to a fork and open a PR against
main", so in the normal workflow the localoriginis the contributor's fork. An unpinnedgh issue view/edit/commentresolves the repository from the git remotes and can therefore land on their own copy — where they can add theapprovedlabel themselves and self-bypass this entire gate. Fixed:Every
ghcall pinned with--repo "$REPO"(4gh issuecalls, plus the GraphQL predecessor query which now takes-f owner="${REPO%%/*}" -f repo="${REPO##*/}").REPOdefaults toaws-samples/sample-autonomous-cloud-coding-agents.New Step 0 hard-fails when the resolved governance repo is a fork — a fork's issue tracker is writable by the contributor, so its
approvedlabel is self-grantable and gates nothing:Verified both branches against live repositories: a genuine fork →
error: governance repo … is a fork, exit 1;aws-samples/…→ prints the name, exit 0. (jq'serror()makesghexit non-zero, so the guard fails closed without a separateif.)Reusing this skill in another repository — answering the review's question directly:
ABCA_GOVERNANCE_REPOis the single runtime knob.It is deliberately the only runtime knob, because it is the one with a security consequence: it must point at a repository whose labels the contributor cannot grant themselves, which is exactly what Step 0's fork check enforces — so the knob cannot become a bypass in its own right. Everything else is policy a forking project edits once in its own copy of
SKILL.md, now documented as an explicit knob table: governance repo, approval label, branch-name pattern, blocker markers, blocking-dependency source, start signal, and governing document — with a note to keep the branch pattern in sync withcheck-branch-name.mjsand the issue-reference keywords withcheck-commit-msg.mjs, since the skill and the hooks enforce two halves of the same policy.Minor
scripts/hooks/check-commit-msg.mjs:603; the file is 128 lines. The exemption regex was at:57-58.🤖 Generated with Claude Code