Repository navigation
Replace the merge workflow guard with a Go hook #721
Description
Problem
The merge workflow guard guard-merge.sh, the 75-line PreToolUse hook under packages/agents/source/skills/merge-pr/claude/scripts/, keeps every change to a pull request behind the one merge entry point. It is authority-bearing, and its rules are shell regular expressions that read the hook payload with jq. This slice ports it to Go, on top of the hook command, the Bash payload support and the strict payload decoding that #719 adds. It belongs to the workflow guards, feature 9 of #588.
Acceptance criteria
- AC1: Given a well-formed hook payload for a Bash command, when the merge guard evaluates it, then the exit status, standard output and standard error equal what the shell guard produced, for allowed and for blocked commands.
- AC2: Given a command, the verb is the first run of lowercase letters and hyphens that directly follows
gh prand whitespace at a command position, as the shell guard's pattern captures it. If that verb is outsidelist,view,diff,statusandchecks, the guard blocks the command; if it is one of those five, it allows the command whatever follows in the chain; if there is no such verb, it allows the command. Sogh pr view 1 && gh pr merge 1is allowed,gh pr merge 1 && gh pr view 1is blocked andgh pr MERGE 1, whose spelling starts with no lowercase letter, has no verb and is allowed, all as today. - AC3: Given a payload that is empty, is not valid JSON, or is valid JSON followed by any further non-whitespace bytes, then the guard exits 2 with a reason on standard error, prints nothing on standard output and never allows.
- AC4: Given commands that chain, quote, substitute or span several lines, including a heredoc body whose line starts with
gh pr, then each verdict equals the shell guard's today, with the first-match and verb-capture behaviour of AC2 and no hardening of chains or of letter case. - AC5: Given a valid payload with no Bash command (the field absent, null or not a string), then the guard allows it with no output, as the shell guard does.
- AC6: The skill's hook registration still fires for the same tool matcher and the same activation lifetime as before the change.
- AC7: Given the Go binary is not installed where the registration runs, then the registered hook denies the call (exit 2) and never allows it.
- AC8: After regeneration, the guard entry point of the skill package and both local deployments are generated from the canonical sources and agree with them.
- AC9: Given a change to the Go code that enforces this guard, including the shared payload decoding and dispatch that control its verdict, then the merge policy classifies it as a golden and owner-protected path exactly as it classifies the shell guard, so an unattended merge needs the machine-configured owner's approval of the exact head.
- AC10: Non-functional: a guard invocation, measured from the start of the registered hook path to exit, takes under 100 ms at the median over 50 runs on the CI runner, for the slowest allowed command of the corpus.
Scope and non-goals
In scope: the merge guard, its forwarding shim, registration and tests.
Non-goal: the commit and pull request guards (#719) and the rebase and resolve-conflicts guards (#722).
Non-goal: loosening or tightening any rule. The only deliberate difference from the shell guard is AC3's refusal of an unreadable payload (the shell guard exits 5 on malformed JSON or on bytes after a valid object, a status that does not block, and exits 0 on an empty payload).
Non-goal: hardening the first-verb behaviour on chains or the lowercase-only verb capture; a separate issue may tighten them.
Out of scope: the merge scripts the guard protects.
Approach
- Add the merge guard as a subcommand of the Go hook command from Replace the commit and pull request workflow guards with Go hooks #719, using its Bash payload type and its strict decoder (the decoder rejects empty input and any bytes after the first JSON value, which is what AC3 needs and why this issue does not add its own). The guard follows the shell contract: reason on standard error, exit 2 to block.
- Record the characterization corpus first, from the shell guard at a pinned commit of main, since the merge Bats suite mostly tests the merge scripts and not the guard decisions: every
gh prverb, unknown and mixed-case verbs, both chain orders, separators;,&&,||,|, a subshell and a newline, quoting, command substitution, agh prline inside a heredoc, andecho gh pr merge. Run the shell guard over it to confirm the recording, then require identical Go results before removing the shell logic. - Keep the script path as a forwarding shim of at most 10 lines that execs the Go command and exits 2 with a reason when the binary is missing, so the registration, matcher and activation lifetime stay as they are. Delete the shell matching logic.
- Protection: Replace the commit and pull request workflow guards with Go hooks #719 extends the golden and owner path patterns to cover the hook command, the guard packages and the shared payload library. This issue adds a test that its new files fall under those patterns, and changes the patterns only if they do not.
- Deliver in the documented order: edit canonical sources, run
mise run generate-skills, thenmise run skill-tokens, thenapm install, and commit what they rewrite. Add todocs/agent-skills-migration.mdthat the merge skill needs the release carrying this subcommand installed before the skill is deployed, or state "No consumer impact" with the reason.
Dependencies
Native blocked-by links: #719 (the hook command, Bash payload support and strict decoding it adds, and the protection of that code), #333 (the Go binaries ship as release assets) and #544 (consumers install those assets, because the shim runs in consumer repositories). #719 in turn waits for #713 and #720 and the earlier features of #588, so this issue inherits that sequence, and the shim is not switched to the Go command until #719 has been rolled out.
Size
XS. One PR for one 75-line guard on top of #719.
Verify plan
- AC1: differential test over the corpus against the recorded shell results, comparing exit status, standard output and exact standard error.
- AC2: table cases for each allowlisted verb, mutating verbs, a missing verb, an all-uppercase verb (
gh pr MERGE 1, allowed as today), a verb with a lowercase first letter and uppercase after it (gh pr mERGE 1, blocked asm), unknown verbs, and both chain orders. - AC3: Go tests that feed an empty payload, non-JSON text, a valid object followed by trailing text and a valid object followed by a second object, asserting exit 2, a non-empty reason on standard error and empty standard output.
- AC4: corpus cases for chaining, quoting, substitution, multi-line input and a heredoc body line that starts with
gh pr. - AC5: Go tests with the command field absent, null, a number and an empty string, asserting exit 0 and no output.
- AC6: comparison of the generated hook registration (matcher, command path, scope) with the registration before the change.
- AC7: integration test running the registered hook with the binary absent, asserting exit 2 and a non-empty reason.
- AC8:
mise run generate-skills,mise run skill-tokensandapm installin that order leave no diff,mise run check-generated-skillspasses, the shim has at most 10 lines, and a search shows the matching logic is gone from the source script. - AC9: a policy test in the style of
tests/skills/agent-merge-pr/policy.bats: a change to the new guard file is refused for an unattended merge without the owner's approval of the exact head, plus a pattern-match check on the new paths. - AC10: a process-level timing script that runs 50 invocations of the registered hook path for the slowest allowed corpus command and reports the median, run in CI.
- Delivery:
mise run checkis green on the exact head, and so aremise run cover-go-checkandmise run cover-py-check; the pull request lists every AC ID with its evidence.
Risks
A guard that allows what it should block weakens the merge path; AC1 to AC5 are the fail-closed checks. A missing binary must not open the guard; AC7 covers it. Lapsed protection of the replacement code is covered by AC9. The undo is reverting the PR, which restores the shell guard and its registration.
Operator authority
Owner-only GitHub action: the repository owner approves the exact head, because the change touches an owner-protected guard path and adds protection for the replacement code.
Activity
tbhb-agent commentedon Oct 25, 2026
Refinement: NOT READY — reconcile compatibility criteria and preserve enforcement protection
Checked
Read the non-empty live body of #721, confirmed parent #588, inspected native blockers #719 and #333, and reviewed siblings #713–#722, the shell guard, shared libraries, hook registration, Go hook infrastructure, and relevant test code. No files changed or mechanical checks run.
Definition of Ready, in order:
AC<n>:entries, including performance criteria. AC2 conflicts with AC4; AC7 combines implementation prescriptions and multiple outcomes.Owner-only GitHub action:prefix and requires exact-head owner approval.Gaps
AC2 contradicts the compatibility promise. AC2 unconditionally blocks non-read-only verbs, while AC4 preserves first-match behavior. For example,
gh pr view 1; gh pr merge 1is allowed because the shell capturesviewand exits immediately (packages/agents/source/skills/merge-pr/claude/scripts/guard-merge.sh:49,packages/agents/source/skills/merge-pr/claude/scripts/guard-merge.sh:56). Qualify AC2 to describe the first matched verb and explicitly verify both chain orders. This preserves the stated non-goal of hardening chains.AC7 violates the required acceptance-criteria shape. It prescribes a shell implementation, a script layout and line limit, and a generation command, while combining several outcomes. Move those implementation constraints into Approach and Verify; express the acceptance outcome as continued compatibility of the existing hook entry point. Retain shell-removal and generation checks as delivery requirements.
Replacement protection must preserve both protection mechanisms. The shell guard matches both
golden_pathsandowner_paths(.agents/agent-skills.conf:4,.agents/agent-skills.conf:9). AC8 and its verification mention only owner-path matching. Golden-path classification separately triggers exact-head owner approval (packages/agents/source/skills/merge-pr/shared/scripts/merge-agent.sh:155). Require equivalent protection for the replacement enforcement code, including shared decoding or dispatch code that controls its verdict. Verify refusal without the required owner approval, alongside pattern matching. Existing policy coverage provides a model (tests/skills/agent-merge-pr/policy.bats:335).Complete the generation and consumer rollout plan. “Regenerate skills” and
check-generated-skillsdo not cover the required generation, token measurement and deployment sequence. Specifymise run generate-skills, thenmise run skill-tokens, thenapm install, committing their outputs. Add consumer adoption guidance identifying the binary version containing the new subcommand, its installation before skill deployment, and protection updates. Ship the Go binaries as release assets installed through mise's github backend #333 publishes assets but explicitly excludes consumer pin and adoption migration; closing it alone does not establish that deployed hooks can invoke this new command.Make AC3’s malformed-input verification cover the actual decoder boundary.
internal/hookio/hookio.go:52decodes one JSON value without checking the remaining input. A valid object followed by invalid trailing bytes can therefore escape the intended invalid-JSON refusal if reused unchanged. Include that case in AC3 verification and state whether Replace the commit and pull request workflow guards with Go hooks #719 supplies strict whole-input decoding or this migration must enforce it.Split
No split required. Keep this to the merge guard, compatibility corpus, forwarding entry point, protection update and adoption guidance. Shared Bash payload infrastructure belongs to #719; rebase guards remain in #722.
Definition of done
All items below remain unchecked because the issue’s AC and Verify plan do not yet cover each complete obligation.
mise run check,mise run cover-go-checkormise run cover-py-check.Round: 1 of 3
tbhb-agent commentedon Oct 25, 2026
Refinement: NOT READY — AC2 still conflicts with compatibility for uppercase verbs
Checked
Read the current #721 body, round 1 verdict, parent #588, native blockers, and relevant siblings. Fetched origin and read code exclusively from
origin/main. No edits, checks, delegation or publication.Definition of Ready, in order:
internal/hookio/hookio.go:52).tests/skills/agent-merge-pr/policy.bats:335andtests/skills/agent-merge-pr/policy.bats:393.Owner-only GitHub action:prefix and requires exact-head approval.Gaps
AC2 needs to distinguish a matched verb from an uppercase token. It requires blocking the first
gh prverb outside the five allowed spellings. However,gh pr MERGE 1is allowed by the shell: its expression captures only[a-z-]+, and no match exits 0 (packages/agents/source/skills/merge-pr/claude/scripts/guard-merge.sh:49). Thus AC2 requires blocking this input while AC1 and AC4 require allowing it. The uppercase case explicitly required by Verify cannot satisfy both.Qualify AC2 around the first regex-matched verb, preserving the existing matching semantics. Give the uppercase fixture its shell-compatible expected result. The ordinary
view-then-mergeand reversed chain examples already match the early return atpackages/agents/source/skills/merge-pr/claude/scripts/guard-merge.sh:56.Split
No split required. Keep the merge guard, corpus, shim, protection verification and adoption guidance together. Shared decoding and dispatch remain prerequisites supplied by #719; description validation belongs to #718.
Definition of done
Ticks indicate coverage in the issue’s plan, not completed implementation evidence.
mise run check,mise run cover-go-checkandmise run cover-py-check.docs/agent-skills-migration.md, or a justified “No consumer impact” statement.Round: 2 of 3
tbhb-agent commentedon Oct 25, 2026
Refinement: READY — AC2 now preserves the shell guard’s verb-capture behavior
Checked
Read the current #721 body, both prior verdicts, parent #588, native blockers and relevant siblings #713–#722. No edits, mechanical checks, delegation or publication.
Definition of Ready, in order:
AC<n>:criteria, within the limit, testable and organized around observable outcomes; AC10 covers performance.The round 2 gap is resolved. The expression captures the first matching
[a-z-]+run and allows when none matches (packages/agents/source/skills/merge-pr/claude/scripts/guard-merge.sh:49). It uses that capture and immediately allows the five listed spellings (packages/agents/source/skills/merge-pr/claude/scripts/guard-merge.sh:53,packages/agents/source/skills/merge-pr/claude/scripts/guard-merge.sh:56).Consequently, AC1, AC2 and AC4 agree:
MERGEalone has no matching verb and allows;mERGEcapturesmand blocks; the first matching allowlisted verb permits either chain suffix. AC2 verification explicitly covers both case examples, every allowlisted verb, missing and unknown verbs, mutating verbs and both chain orders. AC1’s differential comparison supplies exact output and status evidence.Registration and protection verification also match existing code:
packages/agents/source/skills/merge-pr/SKILL.claude.md:6,.agents/agent-skills.conf:4,.agents/agent-skills.conf:9andtests/skills/agent-merge-pr/policy.bats:335.Gaps
None that require implementation guesses or prevent the planned checks from proving their AC.
Split
No split required. Keep the merge guard, corpus, shim, protection verification and adoption guidance together; shared infrastructure remains in #719.
Definition of done
Ticks indicate coverage in the issue’s plan, not completed implementation.
mise run check,mise run cover-go-checkandmise run cover-py-check.Round: 3 of 3
tbhb-agent commentedon Oct 26, 2026
Refinement amendment for the shell-to-Go epic (#588), covering compatibility and protection. The acceptance criteria stay as written, except where noted.