From 09f840df347764998929fd34522158f2347e5e8e Mon Sep 17 00:00:00 2001 From: scottschreckengaust <345885+scottschreckengaust@users.noreply.github.com> Date: Wed, 29 Jul 2026 01:06:30 +0000 Subject: [PATCH 1/6] =?UTF-8?q?feat(governance):=20ADR-003=20enforcement?= =?UTF-8?q?=20hooks=20=E2=80=94=20commit-msg,=20branch-naming,=20pickup-is?= =?UTF-8?q?sue=20skill=20(#186)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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)/-. 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 --- .pre-commit-config.yaml | 19 ++- CONTRIBUTING.md | 5 +- docs/abca-plugin/README.md | 2 + docs/abca-plugin/hooks/hooks.json | 2 +- docs/abca-plugin/skills/pickup-issue/SKILL.md | 136 ++++++++++++++++++ .../ADR-003-contribution-governance.md | 14 +- .../Adr-003-contribution-governance.md | 14 +- .../docs/developer-guide/Contributing.md | 5 +- scripts/hooks/check-branch-name.mjs | 122 ++++++++++++++++ scripts/hooks/check-branch-name.test.mjs | 76 ++++++++++ scripts/hooks/check-commit-msg.mjs | 110 ++++++++++++++ scripts/hooks/check-commit-msg.test.mjs | 93 ++++++++++++ 12 files changed, 584 insertions(+), 14 deletions(-) create mode 100644 docs/abca-plugin/skills/pickup-issue/SKILL.md create mode 100644 scripts/hooks/check-branch-name.mjs create mode 100644 scripts/hooks/check-branch-name.test.mjs create mode 100644 scripts/hooks/check-commit-msg.mjs create mode 100644 scripts/hooks/check-commit-msg.test.mjs diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 0b2f7a7ca..787642420 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -1,7 +1,7 @@ # Git hooks via https://github.com/j178/prek (installed by `mise run install` in a Git checkout; re-run `mise run hooks:install` after edits). # Config format matches pre-commit; run hooks with `prek` from mise (`mise.toml` [tools]). -default_install_hook_types: [pre-commit, pre-push] +default_install_hook_types: [commit-msg, pre-commit, pre-push] fail_fast: false exclude: ^\.threat-composer/ @@ -22,6 +22,23 @@ repos: - repo: local hooks: + # ADR-003 Tier 0: every commit must reference an issue (Refs/Fixes/Closes #N). + # prek passes the commit-message file path as the positional arg; forward it. + - id: adr003-commit-msg + name: ADR-003 commit-msg issue reference + entry: bash -lc 'cd "$(git rev-parse --show-toplevel)" && node scripts/hooks/check-commit-msg.mjs "$@"' -- + language: system + stages: [commit-msg] + + # ADR-003: feature branch must match (feat|fix|chore|docs)/-*. + # Runs at pre-push (branch name is stable by then); reads the current branch. + - id: adr003-branch-name + name: ADR-003 branch naming convention + entry: bash -lc 'cd "$(git rev-parse --show-toplevel)" && node scripts/hooks/check-branch-name.mjs' + language: system + pass_filenames: false + stages: [pre-push] + - id: gitleaks name: gitleaks (staged) entry: bash -lc 'cd "$(git rev-parse --show-toplevel)" && mise run security:secrets:staged' diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 672bdc1a9..e50423124 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -93,8 +93,11 @@ PRs labeled `auto-approve` are approved automatically by the `auto-approve` work `mise run install` automatically installs [prek](https://github.com/j178/prek) git hooks. These run on every commit and push: +- **commit-msg** - ADR-003 Tier 0: rejects a commit message with no issue reference (`Refs #N` / `Fixes #N` / `Closes #N`). See [ADR-003](./docs/decisions/ADR-003-contribution-governance.md). - **pre-commit** - Whitespace/EOF checks, gitleaks on staged changes, linters (ESLint, Ruff, astro check) for touched files. -- **pre-push** - Security scans (`mise run hooks:pre-push:security`) and tests across all packages (`mise run hooks:pre-push:tests`). +- **pre-push** - ADR-003 branch-name check (branch must match `(feat|fix|chore|docs)/-*`; `main`, `dependabot/*`, and detached `HEAD` are exempt), security scans (`mise run hooks:pre-push:security`), and tests across all packages (`mise run hooks:pre-push:tests`). + +The ADR-003 governance hooks (`commit-msg`, branch-name) are plain Node scripts under `scripts/hooks/`; unit-test them with `node --test scripts/hooks/check-commit-msg.test.mjs scripts/hooks/check-branch-name.test.mjs`. If `prek install` fails with "refusing to install hooks with `core.hooksPath` set", another tool owns your hooks. Either unset it (`git config --unset-all core.hooksPath`) or integrate these checks into your hook manager. diff --git a/docs/abca-plugin/README.md b/docs/abca-plugin/README.md index bc2f98a8c..e08e9eba9 100644 --- a/docs/abca-plugin/README.md +++ b/docs/abca-plugin/README.md @@ -22,6 +22,7 @@ Or add to your project's `.claude/settings.json`: | Skill | Trigger | Description | |-------|---------|-------------| +| `/pickup-issue` | Start work, implement, claim issue | ADR-003 governance gate — verify an approved, assigned issue exists before writing code (hard-fails otherwise) | | `/setup` | First-time setup, prerequisites | Walk through prerequisites, toolchain, and first deployment | | `/deploy` | Deploy, diff, destroy | Deploy, diff, or destroy the CDK stack | | `/onboard-repo` | Add a repository | Onboard a GitHub repo via Blueprint CDK construct | @@ -51,6 +52,7 @@ docs/abca-plugin/ hooks/ hooks.json # SessionStart capability advertisement skills/ + pickup-issue/SKILL.md # ADR-003 governance gate (approved+assigned issue) setup/SKILL.md # First-time setup workflow deploy/SKILL.md # CDK deployment management onboard-repo/SKILL.md # Repository onboarding diff --git a/docs/abca-plugin/hooks/hooks.json b/docs/abca-plugin/hooks/hooks.json index d7e87cbc5..050fbe7d2 100644 --- a/docs/abca-plugin/hooks/hooks.json +++ b/docs/abca-plugin/hooks/hooks.json @@ -3,7 +3,7 @@ "SessionStart": [ { "type": "prompt", - "prompt": "The ABCA plugin is active. Available skills:\n- /setup — first-time setup walkthrough\n- /deploy — deploy, diff, or destroy the CDK stack\n- /onboard-repo — add a GitHub repository\n- /submit-task — submit a coding task (guided or quick mode)\n- /troubleshoot — diagnose deployment, auth, or task issues\n- /status — platform health check (stack, tasks, build)\n\nAvailable agents: cdk-expert (CDK infrastructure), agent-debugger (task failure investigation).\n\nInteractive task commands (suggest when user is monitoring or steering a running task):\n- bgagent watch — stream progress events in real time\n- bgagent nudge \"\" — steer the agent mid-run\n- bgagent trace download — download full execution trace\n- bgagent webhook create/list/revoke — manage webhook integrations\n- --trace flag on submit — enable detailed tracing for a task\n- --verbose flag — HTTP debug output on any command\n\nSuggest relevant skills when the user's request matches." + "prompt": "The ABCA plugin is active. Available skills:\n- /pickup-issue — ADR-003 gate: verify an approved, assigned issue BEFORE implementing (hard-fails otherwise)\n- /setup — first-time setup walkthrough\n- /deploy — deploy, diff, or destroy the CDK stack\n- /onboard-repo — add a GitHub repository\n- /submit-task — submit a coding task (guided or quick mode)\n- /troubleshoot — diagnose deployment, auth, or task issues\n- /status — platform health check (stack, tasks, build)\n\nAvailable agents: cdk-expert (CDK infrastructure), agent-debugger (task failure investigation).\n\nInteractive task commands (suggest when user is monitoring or steering a running task):\n- bgagent watch — stream progress events in real time\n- bgagent nudge \"\" — steer the agent mid-run\n- bgagent trace download — download full execution trace\n- bgagent webhook create/list/revoke — manage webhook integrations\n- --trace flag on submit — enable detailed tracing for a task\n- --verbose flag — HTTP debug output on any command\n\nSuggest relevant skills when the user's request matches." } ] } diff --git a/docs/abca-plugin/skills/pickup-issue/SKILL.md b/docs/abca-plugin/skills/pickup-issue/SKILL.md new file mode 100644 index 000000000..97cdfc818 --- /dev/null +++ b/docs/abca-plugin/skills/pickup-issue/SKILL.md @@ -0,0 +1,136 @@ +--- +name: pickup-issue +description: >- + ADR-003 governance gate — verify an approved, assigned GitHub issue exists + BEFORE writing any code. Invoke before starting implementation. Hard-fails if + there is no valid `approved` issue. Use when the user says "start work", + "implement this", "pick up an issue", "begin the task", "let's build X", + "go ahead and code", "start coding", "claim issue", or directs implementation + without first pointing to an approved issue. +argument-hint: +--- + +# Pick Up an Issue (ADR-003 Governance Gate) + +You are enforcing the ABCA contribution-governance gate defined in +[ADR-003](../../../decisions/ADR-003-contribution-governance.md). **No code is +written until a durable, approved, assigned issue exists.** This is a hard gate, +not advice — if any check below fails, STOP and do not begin implementation. + +> **Why this exists:** The most common governance bypass is treating +> conversational momentum ("yes, go ahead") as authorization. Conversations are +> ephemeral; issues are auditable. This skill forces the check that the branch +> and commit hooks cannot: that the issue is *approved* and *assigned* before a +> single file changes. See ADR-003 "Conversational approval is NOT issue +> approval". + +## When to hard-fail (STOP — do not implement) + +- No issue number was provided and none can be identified. +- The referenced issue does not exist. +- The issue lacks the `approved` label. +- The issue is closed. +- The issue is unassigned, or is assigned to someone other than the acting + identity without declared intentionality (multiple assignees need + intentionality per ADR-003 "Assignments"). + +In any of these cases, respond with the specific failure and the remediation +(create the issue / request the `approved` label from an admin / self-assign), +then STOP. Do NOT create branches, write files, or run implementation commands. + +## Step 1: Identify the issue + +Determine the target issue number from the user's request or the current branch +name (which, per ADR-003, encodes it as `(feat|fix|chore|docs)/-*`). + +```bash +# From an explicit number the user gave, or extract from the branch: +git rev-parse --abbrev-ref HEAD # e.g. feat/186-adr003-hooks -> 186 +``` + +If no issue number can be determined, **hard-fail**: ask the user to create an +issue with acceptance criteria and obtain the `approved` label first. + +## Step 2: Verify the issue is approved and workable + +Query GitHub. The issue must exist, be OPEN, carry the `approved` label, and be +assigned. + +```bash +gh issue view --json number,title,state,labels,assignees \ + --jq '{number,title,state,labels:[.labels[].name],assignees:[.assignees[].login]}' +``` + +Validate the response: + +| Field | Required | Hard-fail if | +|-------|----------|--------------| +| `state` | `OPEN` | closed | +| `labels` | contains `approved` | missing `approved` | +| `assignees` | contains the acting identity | empty (unassigned) or assigned only to others | + +If `assignees` is empty, self-assign before proceeding: + +```bash +gh issue edit --add-assignee @me +# then re-read to confirm sole ownership (self-assignment is not atomic — +# ADR-003 warns concurrent agents may race; verify after claiming) +gh issue view --json assignees --jq '[.assignees[].login]' +``` + +## Step 3: Pre-start synthesis (ADR-003 "Pre-start review") + +Before implementing, synthesize context so the body + thread are unambiguous: + +- **Read the full thread** — body, comments, replies. Surface any inconsistency + between the body (primary directive) and later clarifications. +- **Check for blockers** — any `**UNRESOLVED:** ` in the body or + thread blocks implementation. `**DEFERRED:** — tracked in #N` does + not block. +- **Predecessor validation** — the dependency graph is authoritative: + + ```bash + gh api graphql -f query=' + query($owner:String!,$repo:String!,$num:Int!){ + repository(owner:$owner,name:$repo){ + issue(number:$num){ + title + trackedInIssues(first:20){ nodes{ number title state } } # blockedBy + } + } + }' -f owner= -f repo= -F num= + ``` + + If any blocking issue is OPEN, this issue is **not ready** — hard-fail. +- **Priority evaluation** — if asked to work a lower-priority item while higher + `p0`/`p1` items are unassigned, challenge before proceeding. +- **Cross-reference audit** — search open issues/PRs (including drafts) for + duplicates or conflicts; flag overlaps. + +## Step 4: Final gate + +Only if ALL checks pass: + +1. Comment "Starting implementation." on the issue (the durable start signal). +2. Confirm to the user that the gate passed and implementation may begin. + +```bash +gh issue comment --body "Starting implementation." +``` + +If any check failed, you have already stopped at that step. Do not reach Step 4. + +## Relationship to the git hooks + +This skill is the **agent-workflow** layer of ADR-003 enforcement. It complements +but does not replace the git hooks (which every contributor, human or agent, +also gets): + +- **commit-msg hook** (`scripts/hooks/check-commit-msg.mjs`) — rejects commits + with no `Refs #N` / `Fixes #N` / `Closes #N` reference. +- **branch-name hook** (`scripts/hooks/check-branch-name.mjs`, pre-push) — + rejects branches not matching `(feat|fix|chore|docs)/-*`. + +The hooks catch *unreferenced* work mechanically; this skill catches +*unapproved* work before it starts (the hooks cannot query the `approved` label +without network access at commit time). diff --git a/docs/decisions/ADR-003-contribution-governance.md b/docs/decisions/ADR-003-contribution-governance.md index 06a0d7fdd..b9e73e773 100644 --- a/docs/decisions/ADR-003-contribution-governance.md +++ b/docs/decisions/ADR-003-contribution-governance.md @@ -95,15 +95,19 @@ Prose governance is necessary but insufficient. The following enforcement points | Mechanism | Layer | What it catches | Status | |-----------|-------|-----------------|--------| | AGENTS.md directive | Agent prompt | Explicit instruction: "Do NOT begin implementation without an approved issue, even if the user says 'go ahead' in conversation" | Implemented | -| Branch name convention | Git workflow | Branch must match `(feat|fix|chore|docs)/-*` — rejects branches without issue reference | Planned | -| Commit-msg hook (Tier 0) | Pre-commit | Rejects commits without `Refs #N` or `Fixes #N` | Planned | -| Pre-push hook (Tier 1) | Pre-push | Validates referenced issue exists and has `approved` label via `gh` API | Planned | +| Branch name convention | Pre-push | Branch must match `(feat\|fix\|chore\|docs)/-*` — rejects branches without issue reference | Implemented (#186) — `scripts/hooks/check-branch-name.mjs` | +| Commit-msg hook (Tier 0) | Commit-msg | Rejects commits without `Refs #N` / `Fixes #N` / `Closes #N` | Implemented (#186) — `scripts/hooks/check-commit-msg.mjs` | +| Pre-push hook (Tier 1) | Pre-push | Validates referenced issue exists and has `approved` label via `gh` API | Planned (deferred from #186 — needs network at push time; follow-up) | | Claude Code hook (`PreToolUse: Write`) | Agent runtime | Blocks file creation in governed paths without declared issue context | Planned | -| Skill gate: `pickup-issue` | Agent workflow | Agent must invoke before implementation — hard-fails without valid issue | Planned | +| Skill gate: `pickup-issue` | Agent workflow | Agent must invoke before implementation — hard-fails without valid issue | Implemented (#186) — `docs/abca-plugin/skills/pickup-issue/SKILL.md` | **Transition:** Branch naming and commit-msg rules apply to branches created after the corresponding hooks are deployed. Existing branches (including this PR's) pre-date enforcement. -**Progressive enforcement:** Start with the commit-msg hook (cheapest, catches all contributors). Add pre-push validation next. Skill gates enforce at the agent-workflow level (see ADR-012, proposed, for the skill model). +**Branch-name exemptions:** The branch-name hook exempts `main` (the trunk is not a feature branch), `dependabot/*` (bot-authored upgrade branches), and the detached-`HEAD` sentinel. These are not enumerated elsewhere in this ADR; they are the minimal set required so the trunk and machine-generated branches are not falsely rejected. + +**Commit-msg reference forms:** The Tier 0 hook accepts the GitHub closing-keyword family (`Close`/`Closes`/`Closed`, `Fix`/`Fixes`/`Fixed`, `Resolve`/`Resolves`/`Resolved`) plus `Ref`/`Refs`, each followed by `#N`, `GH-N`, or `owner/repo#N`. A bare `#N` mention with no keyword does **not** satisfy the gate — the reference must be an intentional issue link, not incidental prose. + +**Progressive enforcement:** Start with the commit-msg hook (cheapest, catches all contributors). Add pre-push validation next. Skill gates enforce at the agent-workflow level (see ADR-012, proposed, for the skill model). The pre-push Tier 1 `approved`-label check (row above) is deferred — it needs a network `gh` call at push time, which the offline-capable hook set intentionally avoids; the `pickup-issue` skill covers the `approved`-label gate at the agent-workflow layer in the interim. ## Consequences diff --git a/docs/src/content/docs/decisions/Adr-003-contribution-governance.md b/docs/src/content/docs/decisions/Adr-003-contribution-governance.md index f8938626f..4f4b4f413 100644 --- a/docs/src/content/docs/decisions/Adr-003-contribution-governance.md +++ b/docs/src/content/docs/decisions/Adr-003-contribution-governance.md @@ -99,15 +99,19 @@ Prose governance is necessary but insufficient. The following enforcement points | Mechanism | Layer | What it catches | Status | |-----------|-------|-----------------|--------| | AGENTS.md directive | Agent prompt | Explicit instruction: "Do NOT begin implementation without an approved issue, even if the user says 'go ahead' in conversation" | Implemented | -| Branch name convention | Git workflow | Branch must match `(feat|fix|chore|docs)/-*` — rejects branches without issue reference | Planned | -| Commit-msg hook (Tier 0) | Pre-commit | Rejects commits without `Refs #N` or `Fixes #N` | Planned | -| Pre-push hook (Tier 1) | Pre-push | Validates referenced issue exists and has `approved` label via `gh` API | Planned | +| Branch name convention | Pre-push | Branch must match `(feat\|fix\|chore\|docs)/-*` — rejects branches without issue reference | Implemented (#186) — `scripts/hooks/check-branch-name.mjs` | +| Commit-msg hook (Tier 0) | Commit-msg | Rejects commits without `Refs #N` / `Fixes #N` / `Closes #N` | Implemented (#186) — `scripts/hooks/check-commit-msg.mjs` | +| Pre-push hook (Tier 1) | Pre-push | Validates referenced issue exists and has `approved` label via `gh` API | Planned (deferred from #186 — needs network at push time; follow-up) | | Claude Code hook (`PreToolUse: Write`) | Agent runtime | Blocks file creation in governed paths without declared issue context | Planned | -| Skill gate: `pickup-issue` | Agent workflow | Agent must invoke before implementation — hard-fails without valid issue | Planned | +| Skill gate: `pickup-issue` | Agent workflow | Agent must invoke before implementation — hard-fails without valid issue | Implemented (#186) — `docs/abca-plugin/skills/pickup-issue/SKILL.md` | **Transition:** Branch naming and commit-msg rules apply to branches created after the corresponding hooks are deployed. Existing branches (including this PR's) pre-date enforcement. -**Progressive enforcement:** Start with the commit-msg hook (cheapest, catches all contributors). Add pre-push validation next. Skill gates enforce at the agent-workflow level (see ADR-012, proposed, for the skill model). +**Branch-name exemptions:** The branch-name hook exempts `main` (the trunk is not a feature branch), `dependabot/*` (bot-authored upgrade branches), and the detached-`HEAD` sentinel. These are not enumerated elsewhere in this ADR; they are the minimal set required so the trunk and machine-generated branches are not falsely rejected. + +**Commit-msg reference forms:** The Tier 0 hook accepts the GitHub closing-keyword family (`Close`/`Closes`/`Closed`, `Fix`/`Fixes`/`Fixed`, `Resolve`/`Resolves`/`Resolved`) plus `Ref`/`Refs`, each followed by `#N`, `GH-N`, or `owner/repo#N`. A bare `#N` mention with no keyword does **not** satisfy the gate — the reference must be an intentional issue link, not incidental prose. + +**Progressive enforcement:** Start with the commit-msg hook (cheapest, catches all contributors). Add pre-push validation next. Skill gates enforce at the agent-workflow level (see ADR-012, proposed, for the skill model). The pre-push Tier 1 `approved`-label check (row above) is deferred — it needs a network `gh` call at push time, which the offline-capable hook set intentionally avoids; the `pickup-issue` skill covers the `approved`-label gate at the agent-workflow layer in the interim. ## Consequences diff --git a/docs/src/content/docs/developer-guide/Contributing.md b/docs/src/content/docs/developer-guide/Contributing.md index 86f5f0b3e..40cc3cc95 100644 --- a/docs/src/content/docs/developer-guide/Contributing.md +++ b/docs/src/content/docs/developer-guide/Contributing.md @@ -97,8 +97,11 @@ PRs labeled `auto-approve` are approved automatically by the `auto-approve` work `mise run install` automatically installs [prek](https://github.com/j178/prek) git hooks. These run on every commit and push: +- **commit-msg** - ADR-003 Tier 0: rejects a commit message with no issue reference (`Refs #N` / `Fixes #N` / `Closes #N`). See [ADR-003](/sample-autonomous-cloud-coding-agents/architecture/adr-003-contribution-governance). - **pre-commit** - Whitespace/EOF checks, gitleaks on staged changes, linters (ESLint, Ruff, astro check) for touched files. -- **pre-push** - Security scans (`mise run hooks:pre-push:security`) and tests across all packages (`mise run hooks:pre-push:tests`). +- **pre-push** - ADR-003 branch-name check (branch must match `(feat|fix|chore|docs)/-*`; `main`, `dependabot/*`, and detached `HEAD` are exempt), security scans (`mise run hooks:pre-push:security`), and tests across all packages (`mise run hooks:pre-push:tests`). + +The ADR-003 governance hooks (`commit-msg`, branch-name) are plain Node scripts under `scripts/hooks/`; unit-test them with `node --test scripts/hooks/check-commit-msg.test.mjs scripts/hooks/check-branch-name.test.mjs`. If `prek install` fails with "refusing to install hooks with `core.hooksPath` set", another tool owns your hooks. Either unset it (`git config --unset-all core.hooksPath`) or integrate these checks into your hook manager. diff --git a/scripts/hooks/check-branch-name.mjs b/scripts/hooks/check-branch-name.mjs new file mode 100644 index 000000000..3f95bcced --- /dev/null +++ b/scripts/hooks/check-branch-name.mjs @@ -0,0 +1,122 @@ +#!/usr/bin/env node +/** + * MIT No Attribution + * + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * the Software without restriction, including without limitation the rights to + * use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of + * the Software, and to permit persons to whom the Software is furnished to do so. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +/** + * ADR-003 enforcement: branch-name validation (pre-push). + * + * ADR-003 (docs/decisions/ADR-003-contribution-governance.md, "No branches + * without an Issue" + enforcement table) requires a feature branch to match + * `(feat|fix|chore|docs)/-` — a branch without an issue + * reference is unauthorized work. + * + * Exemptions (branches that are not contributor feature work): + * - `main` — the trunk itself is not a feature branch. + * - `dependabot/*` — bot-authored dependency-upgrade branches. + * - `HEAD` — the sentinel `git rev-parse --abbrev-ref HEAD` + * returns in a detached-HEAD state; do not block. + * ADR-003 does not enumerate exemptions in prose; these are the minimal set + * needed so the trunk and machine-generated branches are not falsely rejected. + * See the PR body for this note. + * + * Wiring: a `pre-push`-type local hook in `.pre-commit-config.yaml`. + * + * Usage: + * node scripts/hooks/check-branch-name.mjs [branch-name] + * When no argument is given, the current branch is read via git plumbing + * (`git rev-parse --abbrev-ref HEAD`). + */ + +import { execFileSync } from 'node:child_process'; +import { fileURLToPath } from 'node:url'; + +const BRANCH_PATTERN = /^(?:feat|fix|chore|docs)\/\d+-.+/; +const EXEMPT_EXACT = new Set(['main', 'HEAD', '']); + +/** + * @param {string} branch + * @returns {boolean} + */ +function isExempt(branch) { + if (EXEMPT_EXACT.has(branch)) return true; + if (branch.startsWith('dependabot/')) return true; + return false; +} + +/** + * @param {string} branch current branch name + * @returns {{ ok: boolean, reason?: string }} + */ +export function validateBranchName(branch) { + const name = branch ?? ''; + if (isExempt(name)) { + return { ok: true }; + } + if (BRANCH_PATTERN.test(name)) { + return { ok: true }; + } + return { + ok: false, + reason: + `Branch "${name}" does not match the ADR-003 convention ` + + '`(feat|fix|chore|docs)/-` ' + + '(e.g. `feat/123-short-description`). A branch without an issue reference ' + + 'is unauthorized work — rename it: `git branch -m `.', + }; +} + +/** + * Resolve the current branch via git plumbing. Fail loud on error rather than + * defaulting to a pass, so a broken git invocation cannot silently disable the + * gate. + * @returns {string} + */ +function currentBranch() { + return execFileSync('git', ['rev-parse', '--abbrev-ref', 'HEAD'], { + encoding: 'utf8', + }).trim(); +} + +function main(argv) { + let branch; + if (argv[2]) { + branch = argv[2]; + } else { + try { + branch = currentBranch(); + } catch (err) { + console.error(`check-branch-name: could not determine current branch: ${err.message}`); + return 2; + } + } + + const result = validateBranchName(branch); + if (result.ok) { + return 0; + } + + console.error(`❌ ${result.reason}`); + console.error(' ADR-003: docs/decisions/ADR-003-contribution-governance.md'); + return 1; +} + +// Run only when invoked directly, not when imported by the test suite. +if (process.argv[1] && fileURLToPath(import.meta.url) === process.argv[1]) { + process.exit(main(process.argv)); +} diff --git a/scripts/hooks/check-branch-name.test.mjs b/scripts/hooks/check-branch-name.test.mjs new file mode 100644 index 000000000..034d09502 --- /dev/null +++ b/scripts/hooks/check-branch-name.test.mjs @@ -0,0 +1,76 @@ +/** + * MIT No Attribution + * + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * the Software without restriction, including without limitation the rights to + * use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of + * the Software, and to permit persons to whom the Software is furnished to do so. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { validateBranchName } from './check-branch-name.mjs'; + +// ADR-003: a feature branch must match `(feat|fix|chore|docs)/-*`. + +test('accepts feat/-', () => { + assert.equal(validateBranchName('feat/186-adr003-hooks').ok, true); +}); + +test('accepts fix/-', () => { + assert.equal(validateBranchName('fix/456-bug-name').ok, true); +}); + +test('accepts chore/ and docs/ prefixes', () => { + assert.equal(validateBranchName('chore/12-tidy').ok, true); + assert.equal(validateBranchName('docs/191-agents-md-split').ok, true); +}); + +test('rejects a prefix outside the allowed set', () => { + const r = validateBranchName('feature/186-thing'); + assert.equal(r.ok, false); + assert.match(r.reason, /feat\|fix\|chore\|docs/); +}); + +test('rejects a branch with no issue number', () => { + const r = validateBranchName('feat/adr003-hooks'); + assert.equal(r.ok, false); +}); + +test('rejects a branch missing the description', () => { + // `-` requires a hyphen + at least one desc char. + assert.equal(validateBranchName('feat/186').ok, false); + assert.equal(validateBranchName('feat/186-').ok, false); +}); + +test('rejects a bare branch name with no prefix', () => { + assert.equal(validateBranchName('my-random-branch').ok, false); +}); + +// Exemptions — branches that are not contributor feature work. +test('exempts main', () => { + assert.equal(validateBranchName('main').ok, true); +}); + +test('exempts dependabot/* branches', () => { + assert.equal(validateBranchName('dependabot/npm_and_yarn/foo-1.2.3').ok, true); +}); + +test('exempts the HEAD detached / empty sentinel', () => { + // git rev-parse --abbrev-ref HEAD returns "HEAD" when detached; do not block. + assert.equal(validateBranchName('HEAD').ok, true); +}); + +test('does not exempt a lookalike prefix (maindev)', () => { + assert.equal(validateBranchName('maindev').ok, false); +}); diff --git a/scripts/hooks/check-commit-msg.mjs b/scripts/hooks/check-commit-msg.mjs new file mode 100644 index 000000000..8a0bd21fc --- /dev/null +++ b/scripts/hooks/check-commit-msg.mjs @@ -0,0 +1,110 @@ +#!/usr/bin/env node +/** + * MIT No Attribution + * + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * the Software without restriction, including without limitation the rights to + * use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of + * the Software, and to permit persons to whom the Software is furnished to do so. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +/** + * ADR-003 Tier 0 enforcement: commit-msg hook. + * + * Rejects a commit whose message carries no issue reference. ADR-003 + * (docs/decisions/ADR-003-contribution-governance.md, enforcement table) + * specifies the rule as "Rejects commits without `Refs #N` or `Fixes #N`". + * We accept the full GitHub closing-keyword family (Closes/Fixes/Resolves and + * their inflections) plus `Refs`/`Ref`, since those are what actually link a + * commit to an issue on GitHub. A bare `#N` mention with no keyword does NOT + * satisfy the gate — the reference must be intentional, not incidental prose. + * + * Wiring: a `commit-msg`-type local hook in `.pre-commit-config.yaml`. prek / + * pre-commit invoke commit-msg hooks with the path to the commit message file + * (`.git/COMMIT_EDITMSG`) as the sole positional argument. + * + * Usage: + * node scripts/hooks/check-commit-msg.mjs + */ + +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; + +// Keyword family that links a commit to an issue on GitHub, plus `Refs`/`Ref`. +// Reference forms accepted after the keyword: `#N`, `GH-N`, or `owner/repo#N`. +const ISSUE_REF = + /\b(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?|refs?)\b\s+(?:[\w.-]+\/[\w.-]+)?(?:#|gh-)\d+/i; + +/** + * Strip git comment lines (those beginning with `#`), which git removes before + * storing the commit. A keyword that only appears in a comment must not count. + * Note: `#N` issue references never start a line with the keyword, so removing + * whole `#`-leading lines cannot hide a legitimate reference. + */ +function stripComments(message) { + return message + .split('\n') + .filter((line) => !line.startsWith('#')) + .join('\n'); +} + +/** + * @param {string} message raw commit message + * @returns {{ ok: boolean, reason?: string }} + */ +export function validateCommitMessage(message) { + const body = stripComments(message ?? ''); + if (ISSUE_REF.test(body)) { + return { ok: true }; + } + return { + ok: false, + reason: + 'Commit message is missing an issue reference. ADR-003 (Tier 0) requires ' + + 'a keyword + issue link, e.g. `Refs #N`, `Fixes #N`, or `Closes #N`. ' + + 'Add one referencing the approved issue this commit implements.', + }; +} + +function main(argv) { + const msgPath = argv[2]; + if (!msgPath) { + console.error( + 'check-commit-msg: no commit message file path given. This hook must be ' + + 'wired as a `commit-msg`-type hook so the message file path is passed.', + ); + return 2; + } + + let message; + try { + message = readFileSync(msgPath, 'utf8'); + } catch (err) { + console.error(`check-commit-msg: could not read ${msgPath}: ${err.message}`); + return 2; + } + + const result = validateCommitMessage(message); + if (result.ok) { + return 0; + } + + console.error(`❌ ${result.reason}`); + console.error(' ADR-003: docs/decisions/ADR-003-contribution-governance.md'); + return 1; +} + +// Run only when invoked directly, not when imported by the test suite. +if (process.argv[1] && fileURLToPath(import.meta.url) === process.argv[1]) { + process.exit(main(process.argv)); +} diff --git a/scripts/hooks/check-commit-msg.test.mjs b/scripts/hooks/check-commit-msg.test.mjs new file mode 100644 index 000000000..d5de1b689 --- /dev/null +++ b/scripts/hooks/check-commit-msg.test.mjs @@ -0,0 +1,93 @@ +/** + * MIT No Attribution + * + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * the Software without restriction, including without limitation the rights to + * use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of + * the Software, and to permit persons to whom the Software is furnished to do so. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { validateCommitMessage } from './check-commit-msg.mjs'; + +// ADR-003 Tier 0: a commit message must carry an issue reference — +// `Refs #N`, `Fixes #N`, or `Closes #N` (case-insensitive, GitHub's own +// closing-keyword family). These accept anywhere in the message body. + +test('accepts a message with Closes #N', () => { + const r = validateCommitMessage('feat(x): thing\n\nBody\n\nCloses #186\n'); + assert.equal(r.ok, true); +}); + +test('accepts a message with Fixes #N', () => { + const r = validateCommitMessage('fix: bug\n\nFixes #42'); + assert.equal(r.ok, true); +}); + +test('accepts a message with Refs #N', () => { + const r = validateCommitMessage('chore: tidy\n\nRefs #7'); + assert.equal(r.ok, true); +}); + +test('rejects a bare (#N) with no keyword even on the subject line', () => { + const r = validateCommitMessage('docs: update guide (#191)'); + // Bare (#N) alone is NOT a governance keyword — must be Refs/Fixes/Closes. + assert.equal(r.ok, false); +}); + +test('accepts the keyword+reference on the subject line', () => { + const r = validateCommitMessage('docs: update guide, Closes #191'); + assert.equal(r.ok, true); +}); + +test('accepts GitHub closing synonyms (Resolves, Close, Fix, Ref)', () => { + for (const kw of ['Resolves', 'Resolve', 'Close', 'Closed', 'Fix', 'Fixed', 'Ref']) { + const r = validateCommitMessage(`feat: x\n\n${kw} #99`); + assert.equal(r.ok, true, `expected "${kw} #99" to pass`); + } +}); + +test('is case-insensitive on the keyword', () => { + const r = validateCommitMessage('feat: x\n\ncloses #12'); + assert.equal(r.ok, true); +}); + +test('rejects a message with no issue reference', () => { + const r = validateCommitMessage('feat: add a thing without any reference'); + assert.equal(r.ok, false); + assert.match(r.reason, /Refs #N|Fixes #N|Closes #N/); +}); + +test('rejects a bare "#186" without a keyword', () => { + const r = validateCommitMessage('feat: mentions #186 in prose but no keyword'); + assert.equal(r.ok, false); +}); + +test('rejects an empty message', () => { + const r = validateCommitMessage(''); + assert.equal(r.ok, false); +}); + +test('ignores comment lines (git # comments) when scanning', () => { + // Lines beginning with '#' are git scissors/comments and are stripped by + // git before the commit is stored; a keyword hidden only in a comment must + // NOT satisfy the gate. + const r = validateCommitMessage('feat: x\n\n# Closes #5 (this is a comment)\n'); + assert.equal(r.ok, false); +}); + +test('accepts owner/repo#N cross-repo references', () => { + const r = validateCommitMessage('fix: x\n\nFixes aws-samples/sample#3'); + assert.equal(r.ok, true); +}); From 233aba63dde42e5e7f43db3743710ff51ce21e62 Mon Sep 17 00:00:00 2001 From: scottschreckengaust <345885+scottschreckengaust@users.noreply.github.com> Date: Wed, 29 Jul 2026 01:11:52 +0000 Subject: [PATCH 2/6] test(governance): wire hook tests into CI build DAG + cover GH-N/CRLF/boundary (#186) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- mise.toml | 8 ++++++++ scripts/hooks/check-commit-msg.test.mjs | 23 +++++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/mise.toml b/mise.toml index 6851e3033..2a27d08cf 100644 --- a/mise.toml +++ b/mise.toml @@ -94,6 +94,13 @@ run = "node --experimental-strip-types scripts/check-coverage-thresholds-sync.ts description = "Test the standalone Jira Forge app-actor proxy" run = "npm test --prefix integrations/jira-forge-app" +[tasks."test:hooks"] +description = "Unit-test the ADR-003 governance hook scripts (#186) via node --test" +# Explicit glob (not the directory) so Node reports real pass/fail — a directory +# arg on Node 22 emits a spurious top-level failure. Runs in the build DAG so the +# governance gate's own regression tests cannot silently rot. +run = "node --test scripts/hooks/*.test.mjs" + [tasks."sync:abca-commands"] description = "Generate commands stubs from .abca/commands for multiple AI assistants" run = "node scripts/sync-abca-commands.mjs" @@ -243,6 +250,7 @@ depends = [ "//cli:build", "//docs:build", ":test:jira-forge-app", + ":test:hooks", ":drift-prevention" ] diff --git a/scripts/hooks/check-commit-msg.test.mjs b/scripts/hooks/check-commit-msg.test.mjs index d5de1b689..3499a44ef 100644 --- a/scripts/hooks/check-commit-msg.test.mjs +++ b/scripts/hooks/check-commit-msg.test.mjs @@ -91,3 +91,26 @@ test('accepts owner/repo#N cross-repo references', () => { const r = validateCommitMessage('fix: x\n\nFixes aws-samples/sample#3'); assert.equal(r.ok, true); }); + +test('accepts the GH-N reference form', () => { + // Documented accepted form (source comment): `#N`, `GH-N`, or `owner/repo#N`. + const r = validateCommitMessage('fix: x\n\nFixes GH-42'); + assert.equal(r.ok, true); +}); + +test('rejects keyword with no separator (Closes#5)', () => { + // The `\s+` between keyword and reference is deliberate — no space, no match. + assert.equal(validateCommitMessage('feat: x\n\nCloses#5').ok, false); +}); + +test('rejects a keyword embedded in a longer word (refixes)', () => { + // Word boundary (\b) prevents matching a keyword inside another token. + assert.equal(validateCommitMessage('feat: x\n\nrefixes #5').ok, false); +}); + +test('handles CRLF line endings (comment stripped, body ref survives)', () => { + // Real COMMIT_EDITMSG files may carry \r\n (Windows / core.autocrlf). + assert.equal(validateCommitMessage('feat: x\r\n\r\nCloses #7\r\n').ok, true); + // A ref that appears ONLY in a CRLF comment line must not count. + assert.equal(validateCommitMessage('feat: x\r\n\r\n# Closes #7\r\n').ok, false); +}); From 33acc259ac3a08fa58c78a9672bdc69d67687604 Mon Sep 17 00:00:00 2001 From: scottschreckengaust <345885+scottschreckengaust@users.noreply.github.com> Date: Wed, 29 Jul 2026 01:19:16 +0000 Subject: [PATCH 3/6] fix(governance): exempt merge/revert/fixup commits from commit-msg gate (#186) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- scripts/hooks/check-commit-msg.mjs | 18 +++++++++++++++++ scripts/hooks/check-commit-msg.test.mjs | 26 +++++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/scripts/hooks/check-commit-msg.mjs b/scripts/hooks/check-commit-msg.mjs index 8a0bd21fc..8c4995346 100644 --- a/scripts/hooks/check-commit-msg.mjs +++ b/scripts/hooks/check-commit-msg.mjs @@ -29,6 +29,13 @@ * commit to an issue on GitHub. A bare `#N` mention with no keyword does NOT * satisfy the gate — the reference must be intentional, not incidental prose. * + * Exempt commits (git-generated, not authored contribution work): merge + * commits (`Merge ...`), reverts (`Revert ...`), and fixup!/squash! commits. + * These carry auto-generated subjects with no issue link; requiring one would + * make a routine `git merge origin/main` — which the contribution guide tells + * contributors to run — impossible. This mirrors the default ignore set of + * conventional commit-msg linters (commitlint et al.). + * * Wiring: a `commit-msg`-type local hook in `.pre-commit-config.yaml`. prek / * pre-commit invoke commit-msg hooks with the path to the commit message file * (`.git/COMMIT_EDITMSG`) as the sole positional argument. @@ -45,6 +52,11 @@ import { fileURLToPath } from 'node:url'; const ISSUE_REF = /\b(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?|refs?)\b\s+(?:[\w.-]+\/[\w.-]+)?(?:#|gh-)\d+/i; +// Git-generated / non-authored commits whose subject lines are auto-produced +// and carry no issue link. Matched on the first non-comment line only. +const EXEMPT_SUBJECT = + /^(?:Merge\b|Revert\b|fixup!|squash!|amend!)/i; + /** * Strip git comment lines (those beginning with `#`), which git removes before * storing the commit. A keyword that only appears in a comment must not count. @@ -64,6 +76,12 @@ function stripComments(message) { */ export function validateCommitMessage(message) { const body = stripComments(message ?? ''); + // Exempt git-generated commits (merge / revert / fixup / squash / amend), + // keyed off the first non-blank content line (the subject). + const subject = body.split('\n').find((line) => line.trim() !== '') ?? ''; + if (EXEMPT_SUBJECT.test(subject.trim())) { + return { ok: true }; + } if (ISSUE_REF.test(body)) { return { ok: true }; } diff --git a/scripts/hooks/check-commit-msg.test.mjs b/scripts/hooks/check-commit-msg.test.mjs index 3499a44ef..86c485c9f 100644 --- a/scripts/hooks/check-commit-msg.test.mjs +++ b/scripts/hooks/check-commit-msg.test.mjs @@ -108,6 +108,32 @@ test('rejects a keyword embedded in a longer word (refixes)', () => { assert.equal(validateCommitMessage('feat: x\n\nrefixes #5').ok, false); }); +test('exempts a merge commit (auto-generated subject, no issue ref)', () => { + // `git merge origin/main` — which CONTRIBUTING tells contributors to run — + // produces this subject with no issue link and must not be blocked. + const r = validateCommitMessage( + "Merge remote-tracking branch 'origin/main' into feat/186-adr003-hooks\n", + ); + assert.equal(r.ok, true); +}); + +test('exempts a revert commit', () => { + const r = validateCommitMessage('Revert "feat: something"\n\nThis reverts commit abc123.'); + assert.equal(r.ok, true); +}); + +test('exempts fixup!/squash!/amend! commits', () => { + for (const prefix of ['fixup! feat: x', 'squash! fix: y', 'amend! chore: z']) { + assert.equal(validateCommitMessage(prefix).ok, true, `expected "${prefix}" to be exempt`); + } +}); + +test('does NOT exempt a normal commit whose body merely mentions "merge"', () => { + // Exemption keys off the SUBJECT line only, not incidental "merge" in prose. + const r = validateCommitMessage('feat: teach the merge helper a trick\n\nno reference here'); + assert.equal(r.ok, false); +}); + test('handles CRLF line endings (comment stripped, body ref survives)', () => { // Real COMMIT_EDITMSG files may carry \r\n (Windows / core.autocrlf). assert.equal(validateCommitMessage('feat: x\r\n\r\nCloses #7\r\n').ok, true); From 201a07ed48b98bedcebc8442c31c7f300a796fec Mon Sep 17 00:00:00 2001 From: scottschreckengaust <345885+scottschreckengaust@users.noreply.github.com> Date: Wed, 29 Jul 2026 01:20:40 +0000 Subject: [PATCH 4/6] docs(governance): note merge/revert commit-msg exemption in ADR-003 (#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 --- docs/decisions/ADR-003-contribution-governance.md | 2 +- .../content/docs/decisions/Adr-003-contribution-governance.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/decisions/ADR-003-contribution-governance.md b/docs/decisions/ADR-003-contribution-governance.md index b9e73e773..f9358ad64 100644 --- a/docs/decisions/ADR-003-contribution-governance.md +++ b/docs/decisions/ADR-003-contribution-governance.md @@ -105,7 +105,7 @@ Prose governance is necessary but insufficient. The following enforcement points **Branch-name exemptions:** The branch-name hook exempts `main` (the trunk is not a feature branch), `dependabot/*` (bot-authored upgrade branches), and the detached-`HEAD` sentinel. These are not enumerated elsewhere in this ADR; they are the minimal set required so the trunk and machine-generated branches are not falsely rejected. -**Commit-msg reference forms:** The Tier 0 hook accepts the GitHub closing-keyword family (`Close`/`Closes`/`Closed`, `Fix`/`Fixes`/`Fixed`, `Resolve`/`Resolves`/`Resolved`) plus `Ref`/`Refs`, each followed by `#N`, `GH-N`, or `owner/repo#N`. A bare `#N` mention with no keyword does **not** satisfy the gate — the reference must be an intentional issue link, not incidental prose. +**Commit-msg reference forms:** The Tier 0 hook accepts the GitHub closing-keyword family (`Close`/`Closes`/`Closed`, `Fix`/`Fixes`/`Fixed`, `Resolve`/`Resolves`/`Resolved`) plus `Ref`/`Refs`, each followed by `#N`, `GH-N`, or `owner/repo#N`. A bare `#N` mention with no keyword does **not** satisfy the gate — the reference must be an intentional issue link, not incidental prose. Git-generated commits (merge, revert, and `fixup!`/`squash!`/`amend!` commits) are exempt — keyed off the subject line — so a routine `git merge origin/main` is not blocked. **Progressive enforcement:** Start with the commit-msg hook (cheapest, catches all contributors). Add pre-push validation next. Skill gates enforce at the agent-workflow level (see ADR-012, proposed, for the skill model). The pre-push Tier 1 `approved`-label check (row above) is deferred — it needs a network `gh` call at push time, which the offline-capable hook set intentionally avoids; the `pickup-issue` skill covers the `approved`-label gate at the agent-workflow layer in the interim. diff --git a/docs/src/content/docs/decisions/Adr-003-contribution-governance.md b/docs/src/content/docs/decisions/Adr-003-contribution-governance.md index 4f4b4f413..4ec27051a 100644 --- a/docs/src/content/docs/decisions/Adr-003-contribution-governance.md +++ b/docs/src/content/docs/decisions/Adr-003-contribution-governance.md @@ -109,7 +109,7 @@ Prose governance is necessary but insufficient. The following enforcement points **Branch-name exemptions:** The branch-name hook exempts `main` (the trunk is not a feature branch), `dependabot/*` (bot-authored upgrade branches), and the detached-`HEAD` sentinel. These are not enumerated elsewhere in this ADR; they are the minimal set required so the trunk and machine-generated branches are not falsely rejected. -**Commit-msg reference forms:** The Tier 0 hook accepts the GitHub closing-keyword family (`Close`/`Closes`/`Closed`, `Fix`/`Fixes`/`Fixed`, `Resolve`/`Resolves`/`Resolved`) plus `Ref`/`Refs`, each followed by `#N`, `GH-N`, or `owner/repo#N`. A bare `#N` mention with no keyword does **not** satisfy the gate — the reference must be an intentional issue link, not incidental prose. +**Commit-msg reference forms:** The Tier 0 hook accepts the GitHub closing-keyword family (`Close`/`Closes`/`Closed`, `Fix`/`Fixes`/`Fixed`, `Resolve`/`Resolves`/`Resolved`) plus `Ref`/`Refs`, each followed by `#N`, `GH-N`, or `owner/repo#N`. A bare `#N` mention with no keyword does **not** satisfy the gate — the reference must be an intentional issue link, not incidental prose. Git-generated commits (merge, revert, and `fixup!`/`squash!`/`amend!` commits) are exempt — keyed off the subject line — so a routine `git merge origin/main` is not blocked. **Progressive enforcement:** Start with the commit-msg hook (cheapest, catches all contributors). Add pre-push validation next. Skill gates enforce at the agent-workflow level (see ADR-012, proposed, for the skill model). The pre-push Tier 1 `approved`-label check (row above) is deferred — it needs a network `gh` call at push time, which the offline-capable hook set intentionally avoids; the `pickup-issue` skill covers the `approved`-label gate at the agent-workflow layer in the interim. From 4bf038f4381ff8b0cfccd734f8a0381c3b3cf363 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 2 Sep 2026 23:13:23 +0000 Subject: [PATCH 5/6] fix(hooks): validate pushed refs, scope commit-msg exemptions, pin governance repo (#679 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ` ` 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 --- .pre-commit-config.yaml | 11 +- docs/abca-plugin/skills/pickup-issue/SKILL.md | 63 +++++- scripts/hooks/check-branch-name.mjs | 185 +++++++++++++++-- scripts/hooks/check-branch-name.test.mjs | 186 +++++++++++++++++- scripts/hooks/check-commit-msg.mjs | 30 ++- scripts/hooks/check-commit-msg.test.mjs | 55 ++++++ 6 files changed, 501 insertions(+), 29 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 787642420..dbc4f5a16 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -31,7 +31,16 @@ repos: stages: [commit-msg] # ADR-003: feature branch must match (feat|fix|chore|docs)/-*. - # Runs at pre-push (branch name is stable by then); reads the current branch. + # Runs at pre-push and validates the refs actually being pushed (local AND + # remote side of each refspec), not merely HEAD — pushing a compliant + # branch to a non-compliant remote name must not slip through. + # + # No stdin/args are forwarded here on purpose: prek's git shim is what git + # invokes, so prek — not the hook — consumes git's ` + # ` lines, and it forwards neither those nor git's + # ` ` argv to the entry. It re-publishes the first + # pushed ref pair as PRE_COMMIT_LOCAL_BRANCH / PRE_COMMIT_REMOTE_BRANCH, + # which the script reads from the environment. See the script header. - id: adr003-branch-name name: ADR-003 branch naming convention entry: bash -lc 'cd "$(git rev-parse --show-toplevel)" && node scripts/hooks/check-branch-name.mjs' diff --git a/docs/abca-plugin/skills/pickup-issue/SKILL.md b/docs/abca-plugin/skills/pickup-issue/SKILL.md index 97cdfc818..99024c841 100644 --- a/docs/abca-plugin/skills/pickup-issue/SKILL.md +++ b/docs/abca-plugin/skills/pickup-issue/SKILL.md @@ -33,11 +33,35 @@ not advice — if any check below fails, STOP and do not begin implementation. - The issue is unassigned, or is assigned to someone other than the acting identity without declared intentionality (multiple assignees need intentionality per ADR-003 "Assignments"). +- The governance repository resolves to a **fork** (see Step 0) — a fork's + issue tracker is writable by the contributor, so its `approved` label is + self-grantable and gates nothing. In any of these cases, respond with the specific failure and the remediation (create the issue / request the `approved` label from an admin / self-assign), then STOP. Do NOT create branches, write files, or run implementation commands. +## Step 0: Resolve the governance repository + +**Every `gh` call below must be pinned with `--repo`.** CONTRIBUTING tells +contributors to "push to a fork and open a PR against `main`", so in the normal +workflow the local `origin` is the contributor's own fork. An unpinned +`gh issue view`/`edit`/`comment` resolves the repository from the git remotes, +which on a fork can land on the contributor's copy — where they can add the +`approved` label themselves and self-bypass this entire gate. + +```bash +# Source of truth for issues and the `approved` label. Override to reuse this +# skill in another repository (see "Adapting this skill" at the end). +REPO="${ABCA_GOVERNANCE_REPO:-aws-samples/sample-autonomous-cloud-coding-agents}" + +# Hard-fail if the governance repo is a fork — its `approved` label is +# self-grantable, so the gate would be decorative. This also stops +# ABCA_GOVERNANCE_REPO from being used as a bypass. +gh repo view "$REPO" --json isFork,nameWithOwner \ + --jq 'if .isFork then error("governance repo \(.nameWithOwner) is a fork") else .nameWithOwner end' +``` + ## Step 1: Identify the issue Determine the target issue number from the user's request or the current branch @@ -57,7 +81,7 @@ Query GitHub. The issue must exist, be OPEN, carry the `approved` label, and be assigned. ```bash -gh issue view --json number,title,state,labels,assignees \ +gh issue view --repo "$REPO" --json number,title,state,labels,assignees \ --jq '{number,title,state,labels:[.labels[].name],assignees:[.assignees[].login]}' ``` @@ -72,10 +96,10 @@ Validate the response: If `assignees` is empty, self-assign before proceeding: ```bash -gh issue edit --add-assignee @me +gh issue edit --repo "$REPO" --add-assignee @me # then re-read to confirm sole ownership (self-assignment is not atomic — # ADR-003 warns concurrent agents may race; verify after claiming) -gh issue view --json assignees --jq '[.assignees[].login]' +gh issue view --repo "$REPO" --json assignees --jq '[.assignees[].login]' ``` ## Step 3: Pre-start synthesis (ADR-003 "Pre-start review") @@ -98,7 +122,7 @@ Before implementing, synthesize context so the body + thread are unambiguous: trackedInIssues(first:20){ nodes{ number title state } } # blockedBy } } - }' -f owner= -f repo= -F num= + }' -f owner="${REPO%%/*}" -f repo="${REPO##*/}" -F num= ``` If any blocking issue is OPEN, this issue is **not ready** — hard-fail. @@ -115,7 +139,7 @@ Only if ALL checks pass: 2. Confirm to the user that the gate passed and implementation may begin. ```bash -gh issue comment --body "Starting implementation." +gh issue comment --repo "$REPO" --body "Starting implementation." ``` If any check failed, you have already stopped at that step. Do not reach Step 4. @@ -134,3 +158,32 @@ also gets): The hooks catch *unreferenced* work mechanically; this skill catches *unapproved* work before it starts (the hooks cannot query the `approved` label without network access at commit time). + +## Adapting this skill to another repository + +The gate itself is generic — only the policy it enforces is ABCA-specific. To +reuse it elsewhere, set one environment variable: + +```bash +export ABCA_GOVERNANCE_REPO=/ +``` + +That repointing is deliberately the *only* runtime knob, because it is the one +with a security consequence: point it at a repository whose labels the +contributor cannot grant themselves (Step 0 hard-fails on a fork for exactly +this reason). Everything else below is policy a forking project edits once in +its own copy of this file: + +| Assumption | Where it appears | ABCA value | +|------------|------------------|------------| +| Governance repo | Step 0 (`$REPO`) | `aws-samples/sample-autonomous-cloud-coding-agents` | +| Approval label | Steps 2, 4 | `approved` | +| Branch-name pattern | Step 1 | `(feat\|fix\|chore\|docs)/-` | +| Blocker markers | Step 3 | `**UNRESOLVED:**` blocks; `**DEFERRED:** … tracked in #N` does not | +| Blocking-dependency source | Step 3 | GraphQL `trackedInIssues` (the dependency graph, not prose) | +| Start signal | Step 4 | a `Starting implementation.` issue comment | +| Governing document | throughout | [ADR-003](../../../decisions/ADR-003-contribution-governance.md) | + +Keep the branch-name pattern in sync with `scripts/hooks/check-branch-name.mjs` +and the issue-reference keywords with `scripts/hooks/check-commit-msg.mjs` — the +skill and the hooks enforce two halves of the same policy. diff --git a/scripts/hooks/check-branch-name.mjs b/scripts/hooks/check-branch-name.mjs index 3f95bcced..fa595ec7a 100644 --- a/scripts/hooks/check-branch-name.mjs +++ b/scripts/hooks/check-branch-name.mjs @@ -37,17 +37,52 @@ * * Wiring: a `pre-push`-type local hook in `.pre-commit-config.yaml`. * + * Which refs get validated + * ------------------------ + * A push does not necessarily publish the branch you have checked out — + * `git push origin feat/123-x:refs/heads/random-name` from a `main` checkout + * publishes a non-conforming remote ref while HEAD reads as the exempt `main`. + * So the ref list is resolved from the push itself, in this precedence order + * (see `resolveBranchesToCheck`): + * + * 1. an explicit `argv[2]` — manual/CI use and the test harness. + * 2. git's native `pre-push` stdin contract, one line per pushed ref: + * ` SP SP SP ` + * Populated only when this script is wired as a bare `.git/hooks/pre-push`. + * Under prek it is always empty (see below), but parsing it keeps the + * script correct in a native-hook install and covers every pushed ref. + * 3. prek / pre-commit `pre-push` environment variables + * (`PRE_COMMIT_LOCAL_BRANCH`, `PRE_COMMIT_REMOTE_BRANCH`) — the wiring + * this repo actually uses. + * 4. `git rev-parse --abbrev-ref HEAD` — no push is in flight, e.g. + * `prek run --all-files --stage pre-push` via `mise run hooks:run`. + * + * Both the local and the remote ref of a push are validated: the local name is + * the branch being worked on, and the remote name is what lands on the remote + * and what a PR is opened from — ADR-003 auditability depends on both. + * + * Known limitation (prek): prek's git shim is + * `exec prek hook-impl --hook-type=pre-push -- "$@"`, so prek — not this + * script — is git's stdin reader. It consumes the ref lines to compute its own + * file list, forwards neither stdin nor git's ` ` + * arguments to the hook entry, and re-publishes only the FIRST pushed ref pair + * as `PRE_COMMIT_*`. A multi-ref push is therefore validated on that first pair + * alone. Reaching every ref would require bypassing prek with a native + * `.git/hooks/pre-push`, which conflicts with prek owning `core.hooksPath`. + * * Usage: * node scripts/hooks/check-branch-name.mjs [branch-name] - * When no argument is given, the current branch is read via git plumbing - * (`git rev-parse --abbrev-ref HEAD`). */ import { execFileSync } from 'node:child_process'; +import { readFileSync } from 'node:fs'; import { fileURLToPath } from 'node:url'; const BRANCH_PATTERN = /^(?:feat|fix|chore|docs)\/\d+-.+/; const EXEMPT_EXACT = new Set(['main', 'HEAD', '']); +const HEADS_PREFIX = 'refs/heads/'; +// git writes an all-zero sha for the absent side of a create/delete. +const NULL_SHA = /^0+$/; /** * @param {string} branch @@ -81,6 +116,95 @@ export function validateBranchName(branch) { }; } +/** + * Reduce a ref to a bare branch name. + * @param {string | undefined} ref + * @returns {string | null} the branch name, or null when the ref is not a + * branch (`refs/tags/*`, `refs/notes/*`, …) and so carries no ADR-003 + * naming obligation. + */ +export function branchFromRef(ref) { + const value = (ref ?? '').trim(); + if (value === '') return null; + if (value.startsWith(HEADS_PREFIX)) return value.slice(HEADS_PREFIX.length) || null; + if (value.startsWith('refs/')) return null; + // Unqualified — git accepts a short name or `HEAD` on the left of a refspec. + return value; +} + +/** + * Parse git's native `pre-push` stdin contract. One line per pushed ref: + * ` SP SP SP ` + * + * A deletion (`git push origin :branch`) carries an all-zero local sha and an + * empty local ref — nothing is being published, so there is nothing to name. + * + * @param {string} stdin + * @returns {{ refLines: number, branches: string[] }} `branches` is the + * deduped set of branch names the push touches — both sides of each + * refspec, which are usually the same name. `refLines` counts well-formed + * lines seen, which distinguishes "no ref list was supplied" from "a ref + * list was supplied and yielded no branches to check". + */ +export function parsePrePushStdin(stdin) { + const branches = new Set(); + let refLines = 0; + for (const line of String(stdin ?? '').split('\n')) { + const parts = line.trim().split(/\s+/).filter(Boolean); + if (parts.length < 4) continue; + refLines += 1; + const [localRef, localSha, remoteRef] = parts; + if (NULL_SHA.test(localSha)) continue; // ref deletion + for (const ref of [localRef, remoteRef]) { + const branch = branchFromRef(ref); + if (branch) branches.add(branch); + } + } + return { refLines, branches: [...branches] }; +} + +/** + * Resolve the branch names a given invocation should validate, and where they + * came from. See the module header for the precedence rationale. + * + * @param {{ + * argv?: string[], + * stdin?: string, + * env?: Record, + * readCurrentBranch?: () => string, + * }} [io] + * @returns {{ source: 'argv'|'stdin'|'env'|'head', branches: string[] }} + */ +export function resolveBranchesToCheck(io = {}) { + const { argv = [], stdin = '', env = {}, readCurrentBranch } = io; + const dedupe = (names) => [...new Set(names)]; + + if (argv[2]) { + return { source: 'argv', branches: [argv[2]] }; + } + + // git's native contract. Authoritative once any ref line is present, even if + // it yields no branches (a tag-only push has no naming obligation) — falling + // through to HEAD there would validate a name this push never touches. + const fromStdin = parsePrePushStdin(stdin); + if (fromStdin.refLines > 0) { + return { source: 'stdin', branches: dedupe(fromStdin.branches) }; + } + + // prek / pre-commit. Same reasoning: the vars being set means a push IS in + // flight, so this wins even when it resolves to nothing checkable. + const { PRE_COMMIT_LOCAL_BRANCH: local, PRE_COMMIT_REMOTE_BRANCH: remote } = env; + if (local || remote) { + return { + source: 'env', + branches: dedupe([branchFromRef(local), branchFromRef(remote)].filter(Boolean)), + }; + } + + // No push in flight — validate the checked-out branch. + return { source: 'head', branches: dedupe([readCurrentBranch().trim()]) }; +} + /** * Resolve the current branch via git plumbing. Fail loud on error rather than * defaulting to a pass, so a broken git invocation cannot silently disable the @@ -93,29 +217,58 @@ function currentBranch() { }).trim(); } -function main(argv) { - let branch; - if (argv[2]) { - branch = argv[2]; - } else { - try { - branch = currentBranch(); - } catch (err) { - console.error(`check-branch-name: could not determine current branch: ${err.message}`); - return 2; - } +/** + * Read git's ref list from fd 0. + * + * Returns '' when no ref list is available — which is the normal case under + * prek (it already consumed stdin) and on a TTY (no push in flight). This is a + * fall-THROUGH, not a fall-back to a pass: `resolveBranchesToCheck` then + * resolves the refs from the environment or from HEAD, so the gate still runs. + * A blocking read on a TTY would hang the hook, hence the isTTY guard. + * @returns {string} + */ +function readRefListFromStdin() { + if (process.stdin.isTTY) return ''; + try { + return readFileSync(0, 'utf8'); + } catch { + // EAGAIN on an empty non-blocking pipe, or fd 0 closed outright. + return ''; + } +} + +function main(argv, io = {}) { + const { + stdin = readRefListFromStdin(), + env = process.env, + readCurrentBranch = currentBranch, + } = io; + + let resolved; + try { + resolved = resolveBranchesToCheck({ argv, stdin, env, readCurrentBranch }); + } catch (err) { + console.error(`check-branch-name: could not determine the branch(es) being pushed: ${err.message}`); + return 2; } - const result = validateBranchName(branch); - if (result.ok) { + const failures = resolved.branches + .map((branch) => validateBranchName(branch)) + .filter((result) => !result.ok); + + if (failures.length === 0) { return 0; } - console.error(`❌ ${result.reason}`); + for (const failure of failures) { + console.error(`❌ ${failure.reason}`); + } console.error(' ADR-003: docs/decisions/ADR-003-contribution-governance.md'); return 1; } +export { main }; + // Run only when invoked directly, not when imported by the test suite. if (process.argv[1] && fileURLToPath(import.meta.url) === process.argv[1]) { process.exit(main(process.argv)); diff --git a/scripts/hooks/check-branch-name.test.mjs b/scripts/hooks/check-branch-name.test.mjs index 034d09502..dbe8932e4 100644 --- a/scripts/hooks/check-branch-name.test.mjs +++ b/scripts/hooks/check-branch-name.test.mjs @@ -19,7 +19,28 @@ import { test } from 'node:test'; import assert from 'node:assert/strict'; -import { validateBranchName } from './check-branch-name.mjs'; +import { + branchFromRef, + main, + parsePrePushStdin, + resolveBranchesToCheck, + validateBranchName, +} from './check-branch-name.mjs'; + +const SHA = 'a'.repeat(40); +const ZERO = '0'.repeat(40); + +/** Run `main` with console.error captured so test output stays readable. */ +function runMain(argv, io) { + const original = console.error; + const lines = []; + console.error = (msg) => lines.push(String(msg)); + try { + return { code: main(argv, io), stderr: lines.join('\n') }; + } finally { + console.error = original; + } +} // ADR-003: a feature branch must match `(feat|fix|chore|docs)/-*`. @@ -74,3 +95,166 @@ test('exempts the HEAD detached / empty sentinel', () => { test('does not exempt a lookalike prefix (maindev)', () => { assert.equal(validateBranchName('maindev').ok, false); }); + +// --------------------------------------------------------------------------- +// Ref resolution (#679 review). A push does not necessarily publish HEAD, so +// the refs actually being pushed drive validation. +// --------------------------------------------------------------------------- + +test('branchFromRef strips refs/heads/ and rejects non-branch namespaces', () => { + assert.equal(branchFromRef('refs/heads/feat/186-x'), 'feat/186-x'); + assert.equal(branchFromRef('refs/tags/v1.2.3'), null, 'a tag is not a branch'); + assert.equal(branchFromRef('refs/notes/commits'), null, 'notes are not a branch'); + assert.equal(branchFromRef('feat/186-x'), 'feat/186-x', 'unqualified short name'); + assert.equal(branchFromRef('HEAD'), 'HEAD', 'HEAD refspec side stays the sentinel'); + assert.equal(branchFromRef(''), null); + assert.equal(branchFromRef(undefined), null); +}); + +test('parsePrePushStdin reads git’s ref-line contract', () => { + const r = parsePrePushStdin(`refs/heads/feat/186-x ${SHA} refs/heads/feat/186-x ${SHA}\n`); + assert.equal(r.refLines, 1); + assert.deepEqual(r.branches, ['feat/186-x']); +}); + +test('parsePrePushStdin covers every ref in a multi-ref push', () => { + const r = parsePrePushStdin( + `refs/heads/feat/186-x ${SHA} refs/heads/feat/186-x ${SHA}\n` + + `refs/heads/chore/7-y ${SHA} refs/heads/chore/7-y ${SHA}\n`, + ); + assert.equal(r.refLines, 2); + assert.deepEqual(r.branches, ['feat/186-x', 'chore/7-y']); +}); + +test('parsePrePushStdin skips a deletion (all-zero local sha)', () => { + // `git push origin :old-branch` publishes nothing, so nothing to name. + const r = parsePrePushStdin(`(delete) ${ZERO} refs/heads/old-branch ${SHA}\n`); + assert.equal(r.refLines, 1, 'the line was seen'); + assert.deepEqual(r.branches, [], 'but yields no branch to validate'); +}); + +test('parsePrePushStdin ignores blank and malformed lines', () => { + const r = parsePrePushStdin('\n\ngarbage\ntwo fields\n'); + assert.equal(r.refLines, 0); + assert.deepEqual(r.branches, []); +}); + +test('resolveBranchesToCheck honours precedence: argv > stdin > env > HEAD', () => { + const stdin = `refs/heads/feat/1-s ${SHA} refs/heads/feat/1-s ${SHA}\n`; + const env = { PRE_COMMIT_LOCAL_BRANCH: 'refs/heads/feat/2-e' }; + const readCurrentBranch = () => 'feat/3-h'; + + assert.deepEqual( + resolveBranchesToCheck({ argv: ['node', 's', 'feat/0-a'], stdin, env, readCurrentBranch }), + { source: 'argv', branches: ['feat/0-a'] }, + ); + assert.deepEqual(resolveBranchesToCheck({ argv: [], stdin, env, readCurrentBranch }), { + source: 'stdin', + branches: ['feat/1-s'], + }); + assert.deepEqual(resolveBranchesToCheck({ argv: [], stdin: '', env, readCurrentBranch }), { + source: 'env', + branches: ['feat/2-e'], + }); + assert.deepEqual( + resolveBranchesToCheck({ argv: [], stdin: '', env: {}, readCurrentBranch }), + { source: 'head', branches: ['feat/3-h'] }, + ); +}); + +test('resolveBranchesToCheck reads BOTH sides of a renaming refspec', () => { + // prek re-publishes the first pushed ref pair as PRE_COMMIT_*. + const r = resolveBranchesToCheck({ + env: { + PRE_COMMIT_LOCAL_BRANCH: 'refs/heads/feat/123-x', + PRE_COMMIT_REMOTE_BRANCH: 'refs/heads/random-name', + }, + }); + assert.equal(r.source, 'env'); + assert.deepEqual(r.branches, ['feat/123-x', 'random-name']); +}); + +test('resolveBranchesToCheck dedupes when local and remote names agree', () => { + const r = resolveBranchesToCheck({ + env: { + PRE_COMMIT_LOCAL_BRANCH: 'refs/heads/feat/186-x', + PRE_COMMIT_REMOTE_BRANCH: 'refs/heads/feat/186-x', + }, + }); + assert.deepEqual(r.branches, ['feat/186-x']); +}); + +test('a tag-only push does not fall through to HEAD', () => { + // Falling through would validate a branch this push never touches. + let headRead = false; + const r = resolveBranchesToCheck({ + env: { PRE_COMMIT_LOCAL_BRANCH: 'refs/tags/v1.2.3' }, + readCurrentBranch: () => { + headRead = true; + return 'some-non-conforming-name'; + }, + }); + assert.equal(r.source, 'env'); + assert.deepEqual(r.branches, [], 'a tag carries no naming obligation'); + assert.equal(headRead, false, 'HEAD must not be consulted'); +}); + +// --- main() end-to-end ----------------------------------------------------- + +test('main rejects a push whose REMOTE ref is non-conforming', () => { + // The reviewer's bypass: HEAD is the exempt `main`, but the ref being + // published is not compliant. Pre-fix this exited 0. + const { code, stderr } = runMain([], { + stdin: `refs/heads/feat/123-x ${SHA} refs/heads/random-name ${SHA}\n`, + env: {}, + readCurrentBranch: () => 'main', + }); + assert.equal(code, 1); + assert.match(stderr, /random-name/); +}); + +test('main accepts a fully compliant push', () => { + const { code } = runMain([], { + stdin: `refs/heads/feat/186-x ${SHA} refs/heads/feat/186-x ${SHA}\n`, + env: {}, + readCurrentBranch: () => 'main', + }); + assert.equal(code, 0); +}); + +test('main reports every offending ref in a multi-ref push', () => { + const { code, stderr } = runMain([], { + stdin: + `refs/heads/feat/186-ok ${SHA} refs/heads/feat/186-ok ${SHA}\n` + + `refs/heads/bad-one ${SHA} refs/heads/bad-two ${SHA}\n`, + env: {}, + readCurrentBranch: () => 'main', + }); + assert.equal(code, 1); + assert.match(stderr, /bad-one/); + assert.match(stderr, /bad-two/, 'both offenders reported, not just the first'); +}); + +test('main falls back to HEAD when no push is in flight', () => { + // `prek run --all-files --stage pre-push` via `mise run hooks:run`. + assert.equal(runMain([], { stdin: '', env: {}, readCurrentBranch: () => 'feat/186-x' }).code, 0); + assert.equal(runMain([], { stdin: '', env: {}, readCurrentBranch: () => 'nope' }).code, 1); +}); + +test('main honours an explicit branch argument', () => { + assert.equal(runMain(['node', 's', 'feat/186-x'], { stdin: '', env: {} }).code, 0); + assert.equal(runMain(['node', 's', 'bogus'], { stdin: '', env: {} }).code, 1); +}); + +test('main exits 2 when the branch cannot be determined', () => { + // Fail loud: a broken git invocation must not silently disable the gate. + const { code, stderr } = runMain([], { + stdin: '', + env: {}, + readCurrentBranch: () => { + throw new Error('not a git repository'); + }, + }); + assert.equal(code, 2); + assert.match(stderr, /not a git repository/); +}); diff --git a/scripts/hooks/check-commit-msg.mjs b/scripts/hooks/check-commit-msg.mjs index 8c4995346..5af434199 100644 --- a/scripts/hooks/check-commit-msg.mjs +++ b/scripts/hooks/check-commit-msg.mjs @@ -30,11 +30,15 @@ * satisfy the gate — the reference must be intentional, not incidental prose. * * Exempt commits (git-generated, not authored contribution work): merge - * commits (`Merge ...`), reverts (`Revert ...`), and fixup!/squash! commits. - * These carry auto-generated subjects with no issue link; requiring one would - * make a routine `git merge origin/main` — which the contribution guide tells - * contributors to run — impossible. This mirrors the default ignore set of - * conventional commit-msg linters (commitlint et al.). + * commits (`Merge branch|branches|remote-tracking branch|tag|commit|pull + * request ...`), reverts (`Revert "..."`), and `fixup! `/`squash! `/`amend! ` + * commits. These carry auto-generated subjects with no issue link; requiring + * one would make a routine `git merge origin/main` — which the contribution + * guide tells contributors to run — impossible. The exemption is keyed to the + * exact forms git emits rather than a bare `Merge`/`Revert` prefix, so an + * authored subject that happens to begin with either word is still gated. + * This mirrors the default ignore set of conventional commit-msg linters + * (commitlint et al.). * * Wiring: a `commit-msg`-type local hook in `.pre-commit-config.yaml`. prek / * pre-commit invoke commit-msg hooks with the path to the commit message file @@ -54,8 +58,22 @@ const ISSUE_REF = // Git-generated / non-authored commits whose subject lines are auto-produced // and carry no issue link. Matched on the first non-comment line only. +// +// Scoped to the exact subject forms git itself emits — NOT any subject that +// merely starts with "Merge" or "Revert". An authored `Merge behavior for +// schema handling` or `Revert reviewer nudge copy` is contribution work and +// must still carry an issue reference. The forms below are: +// `Merge branch 'x'` / `... into y` / `... of ` git merge +// `Merge branches 'a' and 'b'` git merge (octopus) +// `Merge remote-tracking branch 'origin/main' into y` git merge +// `Merge tag 'v1'` / `Merge commit ''` git merge +// `Merge pull request #N from owner/branch` GitHub +// `Revert ""` git revert (always quoted) +// `fixup! ` / `squash! ` / `amend! ` git commit --fixup/--squash +// The required space after the autosquash prefixes keeps an authored +// `squashed the bug` out of the exemption. const EXEMPT_SUBJECT = - /^(?:Merge\b|Revert\b|fixup!|squash!|amend!)/i; + /^(?:Merge (?:branch(?:es)?|remote-tracking branch(?:es)?|tag|commit|pull request)\b|Revert "|(?:fixup|squash|amend)! )/i; /** * Strip git comment lines (those beginning with `#`), which git removes before diff --git a/scripts/hooks/check-commit-msg.test.mjs b/scripts/hooks/check-commit-msg.test.mjs index 86c485c9f..d18db6e8f 100644 --- a/scripts/hooks/check-commit-msg.test.mjs +++ b/scripts/hooks/check-commit-msg.test.mjs @@ -134,6 +134,61 @@ test('does NOT exempt a normal commit whose body merely mentions "merge"', () => assert.equal(r.ok, false); }); +// The exemption is scoped to the subjects git itself emits. An authored subject +// that merely STARTS with "Merge"/"Revert"/"squash" is contribution work and +// must still carry an issue reference (#679 review). +test('exempts every auto-generated merge subject form git emits', () => { + const generated = [ + "Merge branch 'main'", + "Merge branch 'main' into feat/186-adr003-hooks", + "Merge branch 'main' of github.com:aws-samples/sample-autonomous-cloud-coding-agents", + "Merge branches 'a' and 'b'", // octopus merge — plural + "Merge remote-tracking branch 'origin/main'", + "Merge tag 'v1.2.3'", + "Merge commit 'abc1234'", + 'Merge pull request #12 from owner/feat/9-x', // GitHub — has #N but no keyword + ]; + for (const subject of generated) { + assert.equal( + validateCommitMessage(`${subject}\n`).ok, + true, + `expected "${subject}" to be exempt`, + ); + } +}); + +test('does NOT exempt an authored subject that merely starts with Merge/Revert', () => { + const authored = [ + 'Merge behavior for schema handling', + 'Revert reviewer nudge copy', // git revert always quotes: `Revert "..."` + 'Merged the two resolvers', + 'Reverting the earlier approach', + ]; + for (const subject of authored) { + assert.equal( + validateCommitMessage(`${subject}\n\nno reference here`).ok, + false, + `expected "${subject}" to be gated`, + ); + } +}); + +test('does NOT exempt an authored subject resembling an autosquash prefix', () => { + // The required space after fixup!/squash!/amend! keeps these gated. + for (const subject of ['squashed the bug', 'fixup the layout', 'amendment to the guide']) { + assert.equal( + validateCommitMessage(`${subject}\n\nno reference here`).ok, + false, + `expected "${subject}" to be gated`, + ); + } +}); + +test('an exempt merge subject still passes when it does carry a reference', () => { + // Exemption short-circuits; a referenced merge must not regress to a failure. + assert.equal(validateCommitMessage("Merge branch 'main'\n\nRefs #186").ok, true); +}); + test('handles CRLF line endings (comment stripped, body ref survives)', () => { // Real COMMIT_EDITMSG files may carry \r\n (Windows / core.autocrlf). assert.equal(validateCommitMessage('feat: x\r\n\r\nCloses #7\r\n').ok, true); From 4332aee08092bd51e2887a6b5f0bf1e4def2e7f6 Mon Sep 17 00:00:00 2001 From: t Date: Thu, 3 Sep 2026 00:28:50 +0000 Subject: [PATCH 6/6] fix(hooks): fail closed when the pre-push ref list cannot be read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- scripts/hooks/check-branch-name.mjs | 33 ++++++++++++----- scripts/hooks/check-branch-name.test.mjs | 45 ++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 8 deletions(-) diff --git a/scripts/hooks/check-branch-name.mjs b/scripts/hooks/check-branch-name.mjs index fa595ec7a..7c14d6492 100644 --- a/scripts/hooks/check-branch-name.mjs +++ b/scripts/hooks/check-branch-name.mjs @@ -217,6 +217,13 @@ function currentBranch() { }).trim(); } +/** + * The only two fd-0 errnos that mean "there is no ref list here" rather than + * "the read failed": an empty non-blocking pipe, and fd 0 closed outright. + * Everything else is a genuine failure and must not be degraded to ''. + */ +const NO_REF_LIST_ERRNOS = new Set(['EAGAIN', 'EBADF']); + /** * Read git's ref list from fd 0. * @@ -225,27 +232,37 @@ function currentBranch() { * fall-THROUGH, not a fall-back to a pass: `resolveBranchesToCheck` then * resolves the refs from the environment or from HEAD, so the gate still runs. * A blocking read on a TTY would hang the hook, hence the isTTY guard. + * + * An *unexpected* read failure is re-thrown so it reaches `main`, which fails + * closed (exit 2). Reporting it as '' would be a security bug, not a nicety: + * the fall-through ends at HEAD, and on a `main` checkout HEAD is exempt — so a + * masked read failure would pass the very gate this hook exists to enforce. + * @param {{isTTY?: boolean, read?: () => string}} [io] seams for tests * @returns {string} */ -function readRefListFromStdin() { - if (process.stdin.isTTY) return ''; +function readRefListFromStdin(io = {}) { + const { isTTY = process.stdin.isTTY, read = () => readFileSync(0, 'utf8') } = io; + if (isTTY) return ''; try { - return readFileSync(0, 'utf8'); - } catch { - // EAGAIN on an empty non-blocking pipe, or fd 0 closed outright. - return ''; + return read(); + } catch (err) { + if (!NO_REF_LIST_ERRNOS.has(err?.code)) throw err; } + return ''; } function main(argv, io = {}) { const { - stdin = readRefListFromStdin(), env = process.env, readCurrentBranch = currentBranch, + readRefList = readRefListFromStdin, } = io; let resolved; try { + // Read stdin inside the `try` so an unexpected fd-0 failure lands in the + // catch below (exit 2) instead of escaping `main` as an uncaught throw. + const stdin = io.stdin ?? readRefList(); resolved = resolveBranchesToCheck({ argv, stdin, env, readCurrentBranch }); } catch (err) { console.error(`check-branch-name: could not determine the branch(es) being pushed: ${err.message}`); @@ -267,7 +284,7 @@ function main(argv, io = {}) { return 1; } -export { main }; +export { main, readRefListFromStdin }; // Run only when invoked directly, not when imported by the test suite. if (process.argv[1] && fileURLToPath(import.meta.url) === process.argv[1]) { diff --git a/scripts/hooks/check-branch-name.test.mjs b/scripts/hooks/check-branch-name.test.mjs index dbe8932e4..7ecf3f79b 100644 --- a/scripts/hooks/check-branch-name.test.mjs +++ b/scripts/hooks/check-branch-name.test.mjs @@ -23,6 +23,7 @@ import { branchFromRef, main, parsePrePushStdin, + readRefListFromStdin, resolveBranchesToCheck, validateBranchName, } from './check-branch-name.mjs'; @@ -258,3 +259,47 @@ test('main exits 2 when the branch cannot be determined', () => { assert.equal(code, 2); assert.match(stderr, /not a git repository/); }); + +// --- reading the ref list from fd 0 --------------------------------------- + +test('readRefListFromStdin returns "" without reading on a TTY', () => { + // A blocking read on a TTY would hang the hook. + assert.equal( + readRefListFromStdin({ + isTTY: true, + read: () => assert.fail('must not read fd 0 on a TTY'), + }), + '', + ); +}); + +test('readRefListFromStdin degrades to "" only for empty/closed fd 0', () => { + for (const code of ['EAGAIN', 'EBADF']) { + const read = () => { + throw Object.assign(new Error(code), { code }); + }; + assert.equal(readRefListFromStdin({ isTTY: false, read }), ''); + } +}); + +test('readRefListFromStdin re-throws an unexpected read failure', () => { + // Masking it would fall through to HEAD, which is exempt on `main` — i.e. it + // would pass the gate on the strength of a failed read. + const read = () => { + throw Object.assign(new Error('permission denied'), { code: 'EACCES' }); + }; + assert.throws(() => readRefListFromStdin({ isTTY: false, read }), /permission denied/); +}); + +test('main exits 2 when the ref list cannot be read', () => { + const { code, stderr } = runMain([], { + // No `stdin` key, so main reads fd 0 through the (stubbed) reader. + readRefList: () => { + throw Object.assign(new Error('input/output error'), { code: 'EIO' }); + }, + env: {}, + readCurrentBranch: () => 'main', + }); + assert.equal(code, 2); + assert.match(stderr, /input\/output error/); +});