Skip to content

Replace the commit and pull request workflow guards with Go hooks #719

Description

@tbhb-agent

Problem

The commit workflow guard guard-git.sh and the pull request workflow guard guard-gh.sh, the PreToolUse hooks under packages/agents/source/skills/commit/claude/scripts/ and packages/agents/source/skills/pr/claude/scripts/, decide which shell commands an agent may run in those two workflows. They are authority-bearing: each holds the single entry point of its workflow. They are 186 and 77 lines of shell that read the hook payload with jq and match commands with regular expressions, and the Go side has internal/hookio and the guard-markdown command but no support for a Bash command payload. Porting them to Go makes the allow and deny rules testable and drops the shell parsing. This is the first slice of the workflow guards, which are feature 9 of #588 (feature 10 there is the public commit hooks and is not part of this issue). The remaining guards are split into #721 (merge) and #722 (rebase and resolve-conflicts), which build on the hook command and the strict payload decoding that this issue adds.

Acceptance criteria

  1. AC1: Given a well-formed hook payload for a Bash command, when the commit or pull request guard evaluates it, then the exit status, standard output and standard error equal what the shell guard produced, for allowed and for blocked commands.
  2. AC2: Given a commit workflow whose activation state is any of: not in a repository, no activation marker, a marker matching HEAD, a marker not matching HEAD (HEAD moved on), or an unborn HEAD, then the commit guard's verdict equals the shell guard's in each case.
  3. 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.
  4. AC4: Given commands that chain, quote, substitute or span several lines, including a heredoc body that mentions a forbidden verb, then each verdict equals the shell guard's today: the commit guard looks at every command position and ignores heredoc bodies, and the pull request guard decides on the first gh pr verb at a command position, ignores later ones, and reads a heredoc body line that starts with gh pr as a command.
  5. AC5: Given a command whose first gh pr verb at a command position is outside list, view, diff, status and checks, then the pull request guard blocks it; given one of those five, it allows the command whatever follows in the chain; given no gh pr verb, it allows it.
  6. AC6: Each skill's hook registration still fires for the same tool matcher and the same skill activation lifetime as before the change.
  7. AC7: Given the Go binary is not installed where a registration runs, then the registered hook denies the call (exit 2) and never allows it.
  8. AC8: After regeneration, the guard entry points of both skill packages and both local deployments are generated from the canonical sources and agree with them.
  9. AC9: Given a change to the Go code that enforces these guards, including the shared payload decoding and dispatch that control their verdicts, then the merge policy classifies it as a golden and owner-protected path exactly as it classifies the shell guards, so an unattended merge needs the machine-configured owner's approval of the exact head.
  10. 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 in a fixture repository.
  11. AC11: 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 guards do.
  12. AC12: Given any payload that guard-markdown reads today, including a Write payload with wrapped Markdown followed by trailing garbage or by a second JSON value, then its decision and output are unchanged: it still denies the wrapped content and still allows a payload that does not decode.

Scope and non-goals

In scope: the commit and pull request guards, Bash payload support and a strict whole-input decoder for the workflow guards in the hook library, the forwarding shims, their registration and their tests.
Non-goal: making guard-markdown decode strictly or changing any of its behaviour; it keeps the existing lenient decoding and its fail-open path (AC12).
Non-goal: the merge guard (#721), the rebase and resolve-conflicts guards (#722), guard-fix.sh, guard-watch.sh and guard-target.sh; each is a later slice.
Non-goal: loosening or tightening any rule. The only deliberate differences from the shell guards are AC3's refusal of an unreadable payload (the shell guards exit 5 on malformed JSON or on bytes after a valid object, a status that does not block, and exit 0 on an empty payload).
Non-goal: hardening the chain behaviour (AC4 records it as today's behaviour); a separate issue may tighten it.
Out of scope: the workflow scripts the guards protect.

Approach

  1. Extend the hook library's payload with the Bash command field, keeping the Write and Edit fields and their callers as they are (AC11, AC12). Leave the existing decoding function unchanged, because it reads the first JSON value and ignores trailing input, and guard-markdown relies on that: a strict version would send its wrapped-content Write with trailing input down its decode-error path, which allows instead of denies. Add a separate strict decoder beside it that accepts one JSON value and then end of input, and have only the workflow guards call it (AC3). The library is the single strict decoder for Replace the merge workflow guard with a Go hook #721 and Replace the rebase and conflict-resolution guards with Go hooks #722; guard-markdown keeps the lenient one and is not changed.
  2. Add the commit and pull request guards as subcommands of the Go hook command. They follow the shell contract (reason on standard error, exit 2 to block, exit 0 to defer), not the JSON-on-standard-output denial that guard-markdown uses, and unlike guard-markdown they do not fail open on a decoding error.
  3. Build the characterization corpus first: fixtures of command, activation state, exit status, standard output and exact standard error, recorded from the shell guards at a pinned commit of main. Seed it from tests/skills/commit/scripts/commit-workflow.bats and tests/skills/pr/scripts/pr-workflow.bats, which mostly exercise the workflow scripts and not the guard decisions, and add: every git add and git commit spelling the guard names (-A, --all, ., ./path, -C dir, --no-verify, -n inside a flag cluster, -m, --message, --amend), flags after a separator that belong to another command, heredocs with quoted and indented terminators, every read-only and mutating gh pr verb, both chain orders (view then merge, and merge then view), a heredoc body line that starts with gh pr, and the five activation states. Before the shell logic is removed, run the shell guards against the fixtures to confirm they are faithful, then require identical Go results.
  4. Keep each guard 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 (AC6, AC7). The shim is allowed by Replace complex shell with tested Go, starting with gates and merging #588 as a forwarding shim. Delete the shell matching logic.
  5. Extend the golden and owner path patterns in the repository's skills policy so the Go enforcement code, including the hook command, the guard packages and the shared payload library, is protected like the shell guards (AC9). Replace the merge workflow guard with a Go hook #721 and Replace the rebase and conflict-resolution guards with Go hooks #722 add files under the same paths and need no further pattern change.
  6. Deliver in the documented order: edit canonical sources, run mise run generate-skills, then mise run skill-tokens, then apm install, and commit what they rewrite (AC8). Add the consumer steps to docs/agent-skills-migration.md: the release that carries the new subcommand, installed before the skills that call it are deployed.

Dependencies

Native blocked-by links: #333 (the Go binaries ship as release assets), #544 (consumers install those assets, because the shim runs in consumer repositories), #713 (gates, #588 feature 4), #723 (the check judge migration, #588 feature 3) and #720 (the Spike that settles the judge contract, kept as a direct blocker). #588 migrates gates and the merge path first and hooks second; #713 and #723 are the issues that exist for that earlier work, and #713 is itself ordered after #723. Features 5 to 8 of #588 have no issues yet, so the rollout hold is: the shims are not switched to the Go command until those features have shipped, and their issues are added as native blockers when they exist. #721 and #722 are blocked by this issue and inherit the sequence.

Size

S. One PR for two small guards and the payload support. The corpus is the bulk of the diff; if it is too large to review, split per guard before coding.

Verify plan

  • AC1: differential test over the corpus, asserting identical exit status, standard output and exact standard error against the recorded shell results, plus a one-time run of the shell guards over the fixtures before they are removed.
  • AC2: corpus fixtures for each activation state: no repository, absent marker, matching marker, moved HEAD, unborn HEAD.
  • 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 heredoc bodies, with the shell guards' recorded verdicts, including the view-then-merge and merge-then-view orders and a gh pr line inside a heredoc.
  • AC5: table cases for every verb in the allowlist, mutating verbs, a missing verb, an uppercase verb and unknown verbs, in a single command and in chains.
  • AC6: comparison of each skill's generated hook registration (matcher, command path, scope) with the registration before the change.
  • AC7: integration test that runs each registered hook with the binary absent and asserts exit 2 and a non-empty reason.
  • AC8: mise run generate-skills, mise run skill-tokens and apm install in that order leave no diff, and mise run check-generated-skills passes; shim line counts and a search showing the matching logic is gone from the source scripts.
  • AC9: a policy test in the style of tests/skills/agent-merge-pr/policy.bats: a change to each class of Go enforcement file (guard code, hook command, shared payload library) is refused for an unattended merge without the owner's approval of the exact head, and a pattern-match check that the new paths are covered.
  • 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.
  • AC11: Go tests with the command field absent, null, a number and an empty string, asserting exit 0 and no output.
  • AC12: the existing guard-markdown and internal/hookio tests pass unchanged, plus new guard-markdown hook cases that reuse the wrapped-content Write fixture followed by garbage and then by a second JSON value, asserting the same denial output as without trailing input, and a non-decoding payload still allowed with no output; and a case that decodes a Write and an Edit payload through the extended type.
  • Delivery: mise run check is green on the exact head, and so are mise run cover-go-check and mise run cover-py-check; the pull request lists every AC ID with its evidence and says either what changed in docs/agent-skills-migration.md or "No consumer impact" with the reason.

Risks

A guard that allows what it should block weakens every workflow; AC1 to AC5 are the fail-closed characterization checks. A missing binary must not open the guard; AC7 covers it. A guard that decodes differently from the shell could let a malformed payload through; AC3 covers it. Making the shared decoder strict would flip guard-markdown from deny to allow on wrapped content with trailing input; the separate strict decoder and AC12 cover it. Protection of the replacement code could lapse; AC9 covers it. The undo is reverting the PR, which restores the shell guards and their registrations.

Operator authority

Owner-only GitHub action: the repository owner approves the exact head, because the change edits owner-protected guard paths and the skills policy patterns, and adds protection for the replacement code.

Activity

  1. changed the title [-]Port the pull request description validator to Go[/-] [+]Replace four Claude workflow guard scripts with Go hooks[/+] on Oct 25, 2026
  2. updated Priority Standard and Work type Planned on Oct 25, 2026
  3. tbhb-agent commented on Oct 25, 2026

    @tbhb-agent
    ContributorAuthor

    Refinement: NOT READY — resolve behavioral contradictions, rollout protections and scope before implementation

    Checked

    Reviewed #719, parent #588, siblings #713–#720, #333, and native dependency relationships. Confirmed #588 is the native parent; #719 has no native blocked-by links.

    Read the four canonical guards, hook registrations, shared shell library, Go hook infrastructure, generation logic and relevant Bats suites. No checks were executed, files changed, or GitHub content posted.

    Gaps

    1. Problem: Clear motivation and identified implementations. Correct the attribution to “features 9 and 10”: Replace complex shell with tested Go, starting with gates and merging #588 lists workflow guards as feature 9; its feature 10 concerns public commit hooks.

    2. Acceptance criteria: Seven correctly numbered AC<n>: entries satisfy the count limit, but the contract is inconsistent.

      • AC2 conflicts with AC1 and the non-goal prohibiting behavioral changes. Missing commands become empty strings and are allowed by existing guards: packages/agents/source/skills/pr/claude/scripts/guard-gh.sh:41 and packages/agents/source/skills/pr/claude/scripts/guard-gh.sh:54. Empty input likewise does not explicitly deny; malformed JSON can terminate through set -e without the guard’s exit-2 refusal. Choose and document permitted compatibility exceptions.
      • AC3 overstates existing protection. For gh pr view 1 && gh pr merge 1, the PR guard captures the first verb and immediately allows; the merge guard does the same. See packages/agents/source/skills/pr/claude/scripts/guard-gh.sh:54 and packages/agents/source/skills/merge-pr/claude/scripts/guard-merge.sh:49. Define whether these cases retain current verdicts or receive separately authorized hardening.
      • AC5 bundles executable choice, matcher preservation and activation lifetime. AC6 bundles implementation deletion and a generation check. Move implementation details into Approach and separate observable outcomes.
      • AC7 includes a non-functional target, but does not specify whether 50 ms includes process startup and Git/state reads, or identify the measurement environment.
    3. Scope and non-goals: Required wording exists, but Port the rebase conflict-marker scan to Go #714 says guard-resolve.sh is covered by Replace the commit and pull request workflow guards with Go hooks #719 while Replace the commit and pull request workflow guards with Go hooks #719 explicitly excludes it. Assign it consistently. Other sibling implementation scopes are distinct.

    4. Approach: Needs correction against the code.

      • internal/hookio/hookio.go:32 has no Bash command field. The approach must include Bash payload support and preservation of existing Write/Edit callers.
      • hookio.Deny emits JSON on stdout (internal/hookio/hookio.go:109); the shell contract requires stderr and exit 2. guard-markdown also deliberately fails open on decoding errors (cmd/guard-markdown/run.go:101). Explain the separate workflow-guard contract.
      • The cited workflow suites primarily test workflow scripts, not the four guard decision tables. Add explicit characterization fixtures, including the rebase guard.
      • Include activation-state fixtures: no repository, absent marker, matching marker, changed HEAD and unborn HEAD. These govern the commit guard at packages/agents/source/skills/commit/claude/scripts/guard-git.sh:84.
    5. Dependencies: Add native blocked-by Ship the Go binaries as release assets installed through mise's github backend #333, matching the stated prerequisite. Also identify who owns protection of the replacement Go paths before rollout. Existing patterns protect shell guards but do not cover new Go enforcement code (.agents/agent-skills.conf:4, .agents/agent-skills.conf:9), contrary to Replace complex shell with tested Go, starting with gates and merging #588’s explicit requirement. Clarify how the migration respects Replace complex shell with tested Go, starting with gates and merging #588’s gates/merge-before-hooks sequencing.

    6. Size: M is an allowed size, but Replace complex shell with tested Go, starting with gates and merging #588 requires larger Features to split into XS/S implementation PRs before coding. Replace the commit and pull request workflow guards with Go hooks #719 makes splitting conditional on later review difficulty. Resolve that mismatch during refinement.

    7. Verify plan: Every AC ID is cited, but several checks do not prove the complete outcome.

      • AC1 needs activation-state fixtures and assertions for exit status, stdout and exact stderr.
      • AC3 needs explicit mixed-chain and quoting/substitution expectations after resolving compatibility.
      • AC5’s generated matcher inspection cannot prove activation lifetime or safe handling of a missing binary.
      • AC7’s decision benchmark cannot establish process-level latency unless startup and state lookup are included.
      • Add a missing-binary integration check proving denial; the dependency and risk text require it, but no AC/check currently does.
    8. Risks: Authorization regression, missing binaries and revert-based undo are identified. Add loss of owner protection over replacement code and checks for the required mitigation.

    9. Operator authority: None conflicts with the Approach’s required owner-path review and Replace complex shell with tested Go, starting with gates and merging #588’s protection-update requirement. State the required owner action using the applicable reserved phrase; Owner-only GitHub action: already appears in Ship the Go binaries as release assets installed through mise's github backend #333.

    Split

    Split before coding into independently refined XS/S guard migrations, or obtain an explicit revision to #588’s sizing rule. Each migration should include its characterization corpus, replacement enforcement protection, registration change and generated outputs. Keep guard-resolve.sh ownership explicit.

    Definition of done

    These boxes assess coverage already promised by the AC and Verify plan; they do not assert implementation completion.

    • D1 — Unmet: AC IDs are mapped, but merged-head verification and PR evidence with commands/results or links are not required.
    • D2 — Pending: Exact-head independent approval and disposition of findings are not covered.
    • D3 — Unmet: Required checks and owner approval of any loosened gate are not covered.
    • D4 — Unmet: Gitleaks, private-string leak scanning and post leak reviews are absent.
    • D5 — Pending: Commit and squash signing are not covered.
    • D6 — Unmet: Documentation updates or a justified “No doc impact” statement are absent.
    • D7 — Pending: Assisted-by trailers and the provenance label are not covered.
    • D8 — Unmet: “M, one PR” does not establish PR size-limit verification or exact-head owner approval.
    • D9 — Pending: Possible sub-issues are mentioned, but deferred-finding issue requirements and PR Related links are absent.
    • D10 — Pending: Closing reference, Done state and final merge-commit Status: comment are not covered.
    • devtools-1 — Unmet: Neither mise run check nor the two coverage-floor commands are included.
    • devtools-2 — Partially covered: AC6 requires regeneration validation; the ordered generation, token measurement and deployment steps are missing.
    • devtools-3 — Unmet: Consumer rollout is discussed, but adoption instructions or a justified “No consumer impact” statement are not required.
      Round: 1 of 3
  4. changed the title [-]Replace four Claude workflow guard scripts with Go hooks[/-] [+]Replace the commit and pull request workflow guards with Go hooks[/+] on Oct 25, 2026
  5. tbhb-agent commented on Oct 25, 2026

    @tbhb-agent
    ContributorAuthor

    Refinement: NOT READY — split is coherent, but AC structure and verification gaps remain

    Checked

    Confirmed #719’s live body is non-empty. Read round 1 comment 6397102836, parent #588, siblings #713–#722, #333, and native dependency links.

    Read the canonical commit and PR guards, registrations, activation code, shared shell library, Go hook infrastructure, generation code and relevant test sources. No files changed, checks executed, operator commands run, or GitHub content posted.

    Gaps

    1. Problem — closed. The revised body correctly identifies feature 9 of Replace complex shell with tested Go, starting with gates and merging #588 and excludes public commit hooks.

    2. Acceptance criteria — partly closed. Ten correctly numbered ACs satisfy the count limit. The revision explicitly chooses malformed-input refusal, preserves command-less payloads and first-verb behavior, and defines process-level latency including Git/state reads on CI.

      Remaining corrections:

      • AC8 still combines implementation deletion, shim layout/line count and generation success. Round 1 explicitly requested separating outcomes and moving implementation details into Approach. Keep the ten-line shim prescription there and separate observable compatibility and generation outcomes.
      • Qualify AC5’s blanket blocking statement with AC4’s first-matched-verb behavior. gh pr view 1 && gh pr merge 1 remains allowed because the guard captures one verb and returns immediately (packages/agents/source/skills/pr/claude/scripts/guard-gh.sh:54, packages/agents/source/skills/pr/claude/scripts/guard-gh.sh:63). The compatibility decision is now explicit elsewhere; AC5 should express the same contract.
    3. Scope and non-goals — closed. Required wording exists. The two-guard scope is clear, and Port the rebase conflict-marker scan to Go #714 now assigns the separate resolve guard to Replace the rebase and conflict-resolution guards with Go hooks #722.

    4. Approach — closed. It now addresses Bash payload support, preservation of Write/Edit callers, stderr/exit-2 refusals, decoding-error behavior, characterization fixtures and activation states. This matches the existing limitations at internal/hookio/hookio.go:32, internal/hookio/hookio.go:109 and cmd/guard-markdown/run.go:104. The Go hook command is currently a skeleton (cmd/agenthooks/main.go:23), consistent with Replace the commit and pull request workflow guards with Go hooks #719 establishing support for its successors.

    5. Dependencies — partly closed. Native blocked-by Ship the Go binaries as release assets installed through mise's github backend #333 exists, and replacement-path protection is assigned to this issue with owner approval. However, saying the work “follows” gates/merge-before-hooks does not identify when that prerequisite is satisfied. Name the prerequisite work or an explicit rollout hold tied to completion of Replace complex shell with tested Go, starting with gates and merging #588’s earlier stages; add native blockers where that work is represented by issues. Ship the Go binaries as release assets installed through mise's github backend #333 alone establishes binary distribution, not the migration sequence.

    6. Size — closed. S is permitted and fits Replace complex shell with tested Go, starting with gates and merging #588’s requirement for XS/S implementation slices. Two guards plus shared payload support form a coherent PR.

    7. Verify plan — partly closed. Every AC ID is cited. Exact streams/statuses, activation states, mixed command forms, absent binaries, protection matching and process-level timing now have checks.

      Still missing:

      • An explicit regression check for existing Write/Edit callers after changing the shared payload library.
      • The complete ordered generation/deployment verification. check-generated-skills excludes token metadata and does not prove deployment; see tools/generate-skills.sh:25 and tools/generate-skills.sh:30.
      • Repository checks, coverage floors and consumer documentation evidence listed below.

      Registration comparison, combined with AC2’s activation fixtures and unchanged shim paths, addresses the prior lifetime concern.

    8. Risks — closed. Authorization regression, absent binaries, lost replacement-code protection and revert-based undo are covered.

    9. Operator authority — closed. The reserved Owner-only GitHub action: line identifies exact-head owner approval.

    Split

    The split is coherent: #719 owns commit/PR guards and shared Bash support; #721 owns merge; #722 owns rebase/resolve guards. Both successors have native blockers on #719 and #333.

    #714 retains the standalone marker scanner. Its explicit exclusion agrees with #722, and the resolve guard’s independent marker logic supports that boundary (packages/agents/source/skills/resolve-rebase-conflicts/claude/scripts/guard-resolve.sh:120). No remaining sibling scope overlap was found.

    Definition of done

    These assess coverage promised by the AC and Verify plan, not implementation completion. No complete checklist item is covered yet.

    • D1 — Unmet: AC mappings exist; merged-head PR evidence with commands/results or links is not required.
    • D2 — Pending: Exact-head independent approval and disposition of findings are absent.
    • D3 — Partially covered: Exact-head owner approval is stated; required checks and approval of any loosened gate are not fully covered.
    • D4 — Unmet: Gitleaks, private-string scanning and post leak reviews are absent.
    • D5 — Pending: Commit and squash signing are not covered.
    • D6 — Unmet: Documentation updates or justified “No doc impact” evidence are absent.
    • D7 — Pending: Assisted-by trailers and the provenance label are not covered.
    • D8 — Unmet: S sizing does not establish PR size-limit verification or an exact-head exception.
    • D9 — Pending: Deferred-finding issue requirements and PR Related links are absent.
    • D10 — Pending: Closing reference, Done state and final merge-commit Status: comment are absent.
    • devtools-1 — Unmet: mise run check, mise run cover-go-check and mise run cover-py-check are absent.
    • devtools-2 — Partially covered: AC8 checks generated output; ordered generation, token measurement, deployment and committing their results remain unspecified.
    • devtools-3 — Unmet: Consumer adoption instructions or justified “No consumer impact” evidence are absent.

    Round: 2 of 3

  6. tbhb-agent commented on Oct 25, 2026

    @tbhb-agent
    ContributorAuthor

    Refinement: NOT READY — resolve the shared decoder’s effect on Markdown guard compatibility

    Checked

    Read the current #719 body, both earlier verdicts, parent #588, siblings #713–#722, and distribution prerequisites #333 and #544. Confirmed native blockers are #333, #544, #713 and #720; #721 and #722 each depend on #719, #333 and #544.

    Fetched origin and read code exclusively from origin/main at 56543d2914814d0f7e47feb11b4679155a185ab1. No checks, operator commands, edits, delegation or GitHub writes were performed.

    In Definition of Ready order:

    • Problem: Clear and correctly assigned to feature 9 of Replace complex shell with tested Go, starting with gates and merging #588.
    • Acceptance criteria: Twelve correctly numbered criteria, including a measurable non-functional target. One compatibility conflict remains below.
    • Scope and non-goals: Explicit and consistent with the sibling split.
    • Approach: Covers Bash payload support, characterization, forwarding shims, generation and protection.
    • Dependencies: Native links match the body. The hold on features 5–8 and requirement to add their future issues as blockers make the remaining sequence explicit.
    • Size: S fits one implementation PR and Replace complex shell with tested Go, starting with gates and merging #588’s sizing rule.
    • Verify plan: Every AC has a corresponding check; AC12’s proposed checks miss the conflict below.
    • Risks: Authorization regression, missing binaries, protection loss and revert-based undo are covered.
    • Operator authority: The reserved owner-action line identifies exact-head approval.

    The commit activation description matches packages/agents/source/skills/commit/claude/scripts/guard-git.sh:84 and its arming write at packages/agents/source/skills/commit/shared/scripts/preflight.sh:159. The differing heredoc behavior and PR first-match behavior match packages/agents/source/skills/commit/claude/scripts/guard-git.sh:98 and packages/agents/source/skills/pr/claude/scripts/guard-gh.sh:54.

    AC9 correctly assigns replacement-code protection to this issue: current golden and owner patterns at .agents/agent-skills.conf:4 and .agents/agent-skills.conf:9 do not cover the new Go paths.

    Gaps

    1. AC12 conflicts with making the existing shared decoder strict, and its verification would not detect the resulting behavior change.

      internal/hookio/hookio.go:54 decodes the first JSON value without checking the remaining input. Consequently, a Write payload containing wrapped Markdown followed by trailing garbage or another JSON value currently reaches Markdown analysis and produces a denial.

      If that shared decoder begins rejecting trailing input, guard-markdown instead takes its decoding-error path and silently allows the call (cmd/guard-markdown/run.go:105). That changes both the decision and output, contrary to AC12’s promise to preserve the Write/Edit inputs it accepts today.

      Existing tests separately cover wrapped-Write denial and malformed-input allowance (cmd/guard-markdown/run_test.go:162, cmd/guard-markdown/run_test.go:200), but do not combine a previously readable Write/Edit payload with trailing input. Passing those tests unchanged, plus decoding an ordinary Write and Edit, cannot prove AC12.

      State whether Markdown compatibility excludes trailing-input cases and explicitly authorize their changed outcome, or preserve its existing decoding behavior while supplying strict decoding to the workflow guards. Add a regression case using the existing wrapped-content fixture followed by garbage and a second JSON value, asserting the chosen decision and output. This leaves Replace the merge workflow guard with a Go hook #721 and Replace the rebase and conflict-resolution guards with Go hooks #722’s strict-decoding requirement intact.

    Split

    No further split is required. #719 owns commit/PR guards and shared payload support; #721 owns merge; #722 owns rebase/resolve; #718 owns description validation. The standalone marker scanner remains with #714. No scope overlap was found.

    Definition of done

    Checked boxes indicate coverage promised by the issue, not completed implementation.

    • D1 — Pending: Per-AC PR evidence is required, but verification on the merged head with commands/results or links is not explicit.
    • D2 — Pending: Exact-merged-head independent approval and disposition of findings are not specified.
    • D3 — Partially covered: Exact-head checks and owner approval are required; the complete required-check condition remains pending.
    • D4 — Unmet: The three required leak-review components are not specified.
    • D5 — Pending: Commit and squash signing are not specified.
    • D6: Approach requires consumer migration documentation describing the new command and installation order.
    • D7 — Pending: Assisted-by trailers and the provenance label are not specified.
    • D8 — Pending: S sizing is stated; hard-limit verification or an exact-head exception is not specified.
    • D9 — Pending: Follow-up issue requirements, Related links and code references are not specified.
    • D10 — Pending: Closing reference, Done state and final merge-commit Status: comment are not specified.
    • devtools-1: Delivery requires mise run check, mise run cover-go-check and mise run cover-py-check on the exact head.
    • devtools-2: Approach requires the ordered generation, token and deployment sequence and committing its rewrites; AC8 verifies reproducibility and generated output.
    • devtools-3: Approach and Delivery require consumer migration instructions or a justified “No consumer impact” statement.

    Round: 3 of 3

  7. tbhb-agent commented on Oct 25, 2026

    @tbhb-agent
    ContributorAuthor

    Status: redesigned after the three-round cap. Round 3 left one gap: AC12 conflicted with making the shared decoder strict, because a strict decoder would send guard-markdown down its decode-error path and allow a wrapped-content Write with trailing input that it denies today, and no existing test covers that case. The redesign keeps the existing decoding function unchanged for guard-markdown and adds a separate strict decoder that only the workflow guards use (so #721 and #722 still get strict decoding). AC12 now asserts that guard-markdown is unchanged, including the wrapped-content fixture followed by garbage and by a second JSON value, and its Verify plan adds those regression cases. Scope and Risks say so. No split was needed. Refinement restarts at Round 1 of 3.

  8. tbhb-agent commented on Oct 25, 2026

    @tbhb-agent
    ContributorAuthor

    Refinement: NOT READY — Decoder redesign is sound; dependency ordering needs one update

    Checked

    • Problem: Clear and consistent with Replace complex shell with tested Go, starting with gates and merging #588 feature 9.
    • Acceptance criteria: Twelve correctly numbered, testable criteria with matching Verify entries, including performance and compatibility.
    • Redesign: internal/hookio/hookio.go:52 decodes once without checking for trailing input. cmd/guard-markdown/run.go:105 calls that decoder and allows decoding errors at line 110. Existing tests cover wrapped-content denial and malformed-input allowance (cmd/guard-markdown/run_test.go:162, cmd/guard-markdown/run_test.go:200). AC12 adds the decisive regression cases: wrapped Write payloads followed by garbage or another JSON value must retain their denial output.
    • Decoder separation: Scope, Approach and Risks consistently preserve the existing decoder and add a separate strict decoder for workflow guards. This matches Replace the merge workflow guard with a Go hook #721’s strict-decoder dependency and Replace the rebase and conflict-resolution guards with Go hooks #722’s equivalent contract.
    • Guard behavior: Commit arming matches packages/agents/source/skills/commit/claude/scripts/guard-git.sh:84; heredoc removal matches line 98. PR first-match behavior matches packages/agents/source/skills/pr/claude/scripts/guard-gh.sh:54 and its allowlist at line 62. The corpus explicitly preserves these behaviors.
    • Protection: AC9 covers guard code, dispatch and payload decoding under both protection patterns. Current patterns protect shell guards (.agents/agent-skills.conf:4, .agents/agent-skills.conf:9); the proposed policy tests cover the replacement.
    • Scope, Approach, Size, Verify, Risks and Operator authority: Present and actionable. The S-sized PR and split-before-coding provision fit the epic. The rollout hold for features 5–8 is explicit.

    Gaps

    1. Dependencies omit the newly separated implementation Replace the shell check judge in the merge scripts with the Go evaluator #723. Settle the check judge contract and divergence corpus before porting it #720 now settles the check-judge contract and corpus; Replace the shell check judge in the merge scripts with the Go evaluator #723 performs the feature-3 migration. Replace the commit and pull request workflow guards with Go hooks #719 still calls Settle the check judge contract and divergence corpus before porting it #720 “the check judge” and treats its completion as the earlier implementation prerequisite. Its native blockers are Ship the Go binaries as release assets installed through mise's github backend #333, feat(distribution): migrate consumer tool pins to release assets #544, Port the gate runner and pass cache from Python to Go #713 and Settle the check judge contract and divergence corpus before porting it #720; Port the gate runner and pass cache from Python to Go #713 currently blocks on Settle the check judge contract and divergence corpus before porting it #720, so that chain does not enforce completion of Replace the shell check judge in the merge scripts with the Go evaluator #723 either.

      Add Replace the shell check judge in the merge scripts with the Go evaluator #723 to Replace the commit and pull request workflow guards with Go hooks #719’s required ordering and native blockers, or establish an explicit native dependency through Port the gate runner and pass cache from Python to Go #713. Keep Settle the check judge contract and divergence corpus before porting it #720 identified as the contract Spike. Otherwise Replace the commit and pull request workflow guards with Go hooks #719 can become unblocked before the earlier migration required by Replace complex shell with tested Go, starting with gates and merging #588 has shipped.

    Split

    No further split required. #719 supplies the shared payload support and commit/PR guards; #721 and #722 consume that support. #718 owns description validation. No scope overlap found.

    Definition of done

    Ticks indicate coverage in the issue’s plan, not completed implementation evidence.

    • D1 Delivery requires every AC ID and evidence on the exact head; execution remains pending.
    • D2 Pending exact-head independent approval and finding resolution.
    • D3 AC9 and Operator authority cover owner approval; required-check completion remains pending.
    • D4 Pending explicit evidence for all three leak-review requirements.
    • D5 Pending signed-commit and squash evidence.
    • D6 Approach and Delivery require migration documentation and PR documentation evidence.
    • D7 Pending Assisted-by trailers and provenance label.
    • D8 Size is S, with splitting required before coding if the corpus becomes too large.
    • D9 Pending follow-up issues and Related/code references for deferred findings.
    • D10 Pending closing reference, board completion and final merge-status comment.
    • devtools-1 Delivery explicitly requires check, cover-go-check and cover-py-check.
    • devtools-2 Approach and AC8 Verify specify generation, token measurement, deployment and generated-output validation.
    • devtools-3 Approach requires consumer installation and deployment steps in docs/agent-skills-migration.md.

    Round: 1 of 3

  9. tbhb-agent commented on Oct 25, 2026

    @tbhb-agent
    ContributorAuthor

    Refinement: READY — Dependency ordering gap is closed

    Checked

    Reviewed the current #719 body, redesigned round 1 verdict, #588, #713, #720, #721, #722 and #723, and native dependency links.
    In Definition of Ready order:

    1. Problem: Clear motivation and ownership under Replace complex shell with tested Go, starting with gates and merging #588 feature 9.
    2. Acceptance criteria: Twelve matching, numbered AC<n>: criteria define testable outcomes, including non-functional latency. AC3 and AC12 consistently require separate strict and lenient decoding. Existing decoding reads one JSON value (internal/hookio/hookio.go:52); Markdown allows decoding errors (cmd/guard-markdown/run.go:105). AC12’s trailing-input regression cases prove the relevant compatibility boundary.
    3. Scope and non-goals: Commit/PR guards and shared payload support are explicit; sibling guard migrations and behavioral hardening are excluded.
    4. Approach: Actionable characterization, forwarding shims and generation sequence. Commit arming matches packages/agents/source/skills/commit/shared/scripts/preflight.sh:159 and packages/agents/source/skills/commit/claude/scripts/guard-git.sh:84; heredoc handling matches the latter’s line 98. PR first-match behavior matches packages/agents/source/skills/pr/claude/scripts/guard-gh.sh:54 and its allowlist at line 62.
    5. Dependencies: Prose and native blockers agree: Ship the Go binaries as release assets installed through mise's github backend #333, feat(distribution): migrate consumer tool pins to release assets #544, Port the gate runner and pass cache from Python to Go #713, Settle the check judge contract and divergence corpus before porting it #720 and Replace the shell check judge in the merge scripts with the Go evaluator #723. Replace the shell check judge in the merge scripts with the Go evaluator #723 implements the migration whose contract Settle the check judge contract and divergence corpus before porting it #720 settles. Port the gate runner and pass cache from Python to Go #713 natively blocks on Replace the shell check judge in the merge scripts with the Go evaluator #723, confirming the ordering sentence. Keeping Settle the check judge contract and divergence corpus before porting it #720 directly blocking is consistent. The features 5–8 rollout hold and future native-blocker requirement preserve Replace complex shell with tested Go, starting with gates and merging #588’s sequence.
    6. Size: S is coherent; splitting before coding remains available if the corpus becomes too large.
    7. Verify plan: Every AC ID has a corresponding proving check. AC9 covers guard code, dispatch and payload decoding under both protection patterns, addressing their current absence from .agents/agent-skills.conf:4 and .agents/agent-skills.conf:9.
    8. Risks: Authorization regressions, decoding compatibility, missing binaries, protection loss and rollback are addressed.
    9. Operator authority: The reserved owner-action wording requires exact-head approval.

    Gaps

    None that require an implementer to guess or prevent the planned checks from proving the AC.

    Split

    No further split required. #719 supplies commit/PR guards and shared Bash payload support with strict decoding; #721 supplies the merge guard; #722 supplies rebase and conflict-resolution guards.

    Definition of done

    Ticks indicate coverage promised by the issue, not completed implementation.

    • D1 Delivery requires per-AC evidence on the exact head; execution remains pending.
    • D2 — Pending: Exact-head independent approval and finding resolution.
    • D3 — Pending: AC9 and Operator authority cover owner approval; required-check completion remains pending.
    • D4 — Pending: Evidence for gitleaks, private-string scanning and each post’s leak review.
    • D5 — Pending: Signed commits and squash.
    • D6 Approach and Delivery require migration documentation and PR documentation evidence.
    • D7 — Pending: Assisted-by trailers and provenance label.
    • D8 S sizing and splitting before coding if the corpus becomes too large are specified.
    • D9 — Pending: Deferred-finding issues and Related/code references.
    • D10 — Pending: Closing reference, Done state and final merge-commit status.
    • devtools-1 Delivery requires mise run check, mise run cover-go-check and mise run cover-py-check.
    • devtools-2 Approach and AC8 Verify specify ordered generation, token measurement, deployment, committed rewrites and generated-output validation.
    • devtools-3 Consumer installation and deployment steps are required in docs/agent-skills-migration.md.

    Round: 2 of 3

  10. added
    refinement:readyPassed the Definition of Ready; may be picked up
    and removed
    refinement:activeA refinement round is open; not ready for implementation
    on Oct 25, 2026
  11. tbhb-agent commented on Oct 26, 2026

    @tbhb-agent
    ContributorAuthor

    Refinement amendment for the shell-to-Go epic (#588), covering compatibility and protection. The acceptance criteria stay as written, except where noted.

    1. The commit and pull request guard scripts forward to the Go hook when present and run the existing shell guard otherwise. A guard is enforcement, so a present Go hook that errors blocks the command (exit 2) rather than falling back.
    2. Fallback rule from docs: state the N-2 compatibility policy #661, applied here: the caller probes for the Go command without writes. When the command is absent (an older pinned binary), the existing shell implementation runs unchanged. When the command is present and fails, prints partial output or returns an invalid result, the caller refuses and never falls back to repeat a protected action. Configuration, flags, streams, statuses and state formats stay as they are, and this change does not raise the binary floor. A separate XS issue removes the fallback after every supported release pin ships the command. Any acceptance criterion or step that demands deleting the shell logic, or a missing-binary refusal, moves to that issue.
    3. No new complex shell: the fallback is the frozen existing code, not a translation of it.
    4. The replacement Go package and its dispatcher registration are covered by the owner-approval protection patterns before any caller switches to them. That protection is its own Wave 1 issue and is a native dependency of this one.
    5. Generated skill output is regenerated in the same PR, in the documented order.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    refinement:readyPassed the Definition of Ready; may be picked up

    Fields

    Priority

    Standard

    Work type

    Planned

    Projects

    • Status
      Backlog

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions