Skip to content

Repository navigation

Replace the rebase and conflict-resolution guards with Go hooks #722

New issue
New issue
Open
Feature
Open
Replace the rebase and conflict-resolution guards with Go hooks#722
Feature
Labels
refinement:readyPassed the Definition of Ready; may be picked up

Description

@tbhb-agent
tbhb-agent
opened on Oct 25, 2026

Problem

The rebase workflow guard guard-rebase.sh (194 lines, under packages/agents/source/skills/rebase/claude/scripts/) and the conflict-resolution guard guard-resolve.sh (161 lines, under packages/agents/source/skills/resolve-rebase-conflicts/claude/scripts/) police git stash, rebase, push, staging and conflict-side checkout during a rebase. They are authority-bearing shell regular expressions over the hook payload, and guard-resolve.sh also refuses a staging command while conflict markers remain. This slice ports both to Go on top of the hook command, Bash payload support and strict payload decoding from #719. It belongs to the workflow guards, feature 9 of #588. The standalone marker scanner marker-scan.sh is a different script and stays with #714.

Acceptance criteria

  1. AC1: Given a well-formed hook payload for a Bash command, when the rebase or the conflict-resolution 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 rebase command that is a stash pop or apply without a named stash entry, a --skip, a --continue (blocked directly whatever the index holds), a rebase start that lacks the pinned settings the guard requires (rebase.updateRefs=false together with either rebase.autoSquash=false or --exec), or a bare --force or -f push without --force-with-lease, then the rebase guard blocks it with the shell guard's reason; and a start that carries those settings, a --abort, a --quit, an --edit-todo, a --show-current-patch, and a stash command naming an entry stay allowed.
  3. AC3: Given a checkout, restore or checkout-index command that takes a side with --ours, --theirs or --stage=2 or --stage=3, a whole-tree staging command (-A, --all or .), or a staging command naming an existing regular file that carries a directional conflict-marker line (<<<<<<<, >>>>>>> or ||||||| at the start of a line followed by a space or the end of the line), then the conflict-resolution guard blocks it with the shell guard's reason and names the offending files; a bare ======= line does not count.
  4. AC4: Given a staging command naming a file whose path ends skills/resolve-rebase-conflicts/SKILL.md, or whose conflict-markers Git attribute is documented, then that file is not treated as an offender; given the attribute lookup fails or returns another value, the file is not exempt.
  5. AC5: Given a payload that is empty, is not valid JSON, or is valid JSON followed by any further non-whitespace bytes, then each guard exits 2 with a reason on standard error, prints nothing on standard output and never allows.
  6. AC6: Given a valid payload with no Bash command (the field absent, null or not a string), then each guard allows it with no output, as the shell guards do.
  7. AC7: Given commands that chain, quote, substitute or span several lines, including a heredoc body that mentions a forbidden command, then each verdict equals the shell guard's today: both guards ignore heredoc bodies, search for a git command at each command boundary, and split staged file names on whitespace, so a quoted name with a space is read as separate words.
  8. AC8: Each skill's hook registration still fires for the same tool matcher and the same activation lifetime as before the change.
  9. AC9: Given the Go binary is not installed where a registration runs, then the registered hook denies the call (exit 2) and never allows it.
  10. AC10: After regeneration, the guard entry points of both skill packages and both local deployments are generated from the canonical sources and agree with them.
  11. AC11: 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.
  12. AC12: Non-functional: a guard invocation, measured from the start of the registered hook path to exit, takes under 250 ms at the median over 50 runs on the CI runner, for the slowest allowed corpus command of each guard, which for the conflict-resolution guard is a staging command naming 10 existing files in a fixture repository.

Scope and non-goals

In scope: the two guards above, including the conflict-marker refusal in guard-resolve.sh, their forwarding shims, registrations and tests.
Non-goal: marker-scan.sh and the rebase scripts that call it (#714), the commit and pull request guards (#719) and the merge guard (#721).
Non-goal: loosening or tightening any rule. The only deliberate difference from the shell guards is AC5'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 command-boundary behaviour or the whitespace splitting of file names; a separate issue may tighten it.
Out of scope: guard-fix.sh, guard-watch.sh and guard-target.sh.

Approach

  1. Add both guards as subcommands 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 (which supplies AC5), following the shell contract: reason on standard error, exit 2 to block.
  2. Record the characterization corpus first, from the shell guards at a pinned commit of main, with a fixture repository per case, since the workflow Bats suites do not table the guard decisions. It covers the rebase guard (stash with and without an entry name, a seven-hex-digit stash id, --skip, --continue with a clean and with an unmerged index, starts with and without each pinned setting, -c and -C arguments before the verb, forced pushes with and without a lease) and the resolve guard (checkout, restore and checkout-index with each side flag and --stage value, whole-tree staging, a marker file, a file with ======= only, a file exempted by path, a file exempted by attribute, a failed attribute read, a quoted file name with a space, a missing file, and a directory). Both guards get heredoc cases with quoted and indented terminators. Run the shell guards over the corpus to confirm the recording, then require identical Go results before removing the shell logic.
  3. The marker refusal in the resolve guard keeps today's behaviour; it does not call the scanner of Port the rebase conflict-marker scan to Go #714.
  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. Delete the shell matching logic.
  5. 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.
  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. Add to docs/agent-skills-migration.md that the two skills need the release carrying these subcommands installed before they are 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 shims run 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 shims are not switched to the Go command until #719 has been rolled out.

Size

S. One PR for two guards; if the corpus is too large to review, split per guard before coding.

Verify plan

  • AC1: differential test over the corpus against the recorded shell results, comparing exit status, standard output and exact standard error, for both guards.
  • AC2: table cases for each rebase form listed, with a fixture repository for --continue with a clean and with an unmerged index (both blocked), and each pinned-settings combination for starts.
  • AC3: table cases for each side flag, --stage value and whole-tree form, and a fixture with directional marker files, a ======= file and a clean file, checking the offender list in the reason.
  • AC4: fixtures for the exempt path, the documented attribute, another attribute value and a failed lookup.
  • AC5: 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, to each guard, asserting exit 2, a non-empty reason on standard error and empty standard output.
  • AC6: Go tests with the command field absent, null, a number and an empty string, asserting exit 0 and no output.
  • AC7: corpus cases for chaining, quoting, substitution, multi-line input, heredoc bodies and a quoted file name with a space.
  • AC8: comparison of each skill's generated hook registration (matcher, command path, scope) with the registration before the change.
  • AC9: integration test running each registered hook with the binary absent, asserting exit 2 and a non-empty reason.
  • AC10: mise run generate-skills, mise run skill-tokens and apm install in that order leave no diff, mise run check-generated-skills passes, the shims have at most 10 lines, and a search shows the matching logic is gone from the source scripts.
  • AC11: a policy test in the style of tests/skills/agent-merge-pr/policy.bats: a change to each 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.
  • AC12: a process-level timing script that runs 50 invocations per guard of the registered hook path for the slowest allowed corpus command and reports the median, run in CI.
  • 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.

Risks

A guard that allows what it should block weakens the rebase path, and a rebase can then discard work; AC1 to AC7 are the fail-closed checks. A missing binary must not open a guard; AC9 covers it. Lapsed protection of the replacement code is covered by AC11. 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 touches owner-protected guard paths and adds protection for the replacement code.

Activity

  1. github-project-automation
    added this to devtoolson Oct 25, 2026
  2. github-project-automation
    moved this to Backlog in devtoolson Oct 25, 2026
  3. tbhb-agent
    marked this as blocked by 2 issues on Oct 25, 2026
  4. tbhb-agent
    set
    Priority
    to
    Standard
    on Oct 25, 2026
  5. tbhb-agent
    added the
    Feature
    issue type on Oct 25, 2026
  6. tbhb-agent
    added a parent issue on Oct 25, 2026
  7. tbhb-agent
    set
    Work type
    to
    Planned
    on Oct 25, 2026
  8. tbhb-agent
    added
    refinement:activeA refinement round is open; not ready for implementation
    on Oct 25, 2026

  9. tbhb-agent commented on Oct 25, 2026

    @tbhb-agent
    tbhb-agent
    on Oct 25, 2026
    ContributorAuthor

    Refinement: NOT READY — compatibility contract and verification need correction

    Checked

    Gaps

    1. Acceptance criteria: AC2 contradicts preserved behavior. It requires blocking a rebase “start outside the workflow entry point,” but the shell explicitly allows manually started rebases with pinned settings, including its gate-sweep exception (packages/agents/source/skills/rebase/claude/scripts/guard-rebase.sh:131). Direct --continue is refused unconditionally, without inspecting unmerged paths (packages/agents/source/skills/rebase/claude/scripts/guard-rebase.sh:121). Correct AC2 and its verification cases to preserve these exceptions and test continuation with both clean and unmerged indexes.

    2. Acceptance criteria: outcome and implementation requirements are mixed. AC8 combines removal of shell logic, a shim line limit, and a generation check. Move the shim design and line limit into Approach, and the generation command into Verify. AC4 also combines malformed-input refusal and command-less-payload allowance; give those outcomes separate criteria. Keep matching numbering and no more than 12 ACs.

    3. Approach and Verify: the compatibility corpus needs explicit coverage of the distinctive rules. Add cases for:

      • Heredoc stripping, which both guards implement (packages/agents/source/skills/rebase/claude/scripts/guard-rebase.sh:54).
      • checkout-index --stage=2/3, which the resolve guard refuses (packages/agents/source/skills/resolve-rebase-conflicts/claude/scripts/guard-resolve.sh:76).
      • Built-in documentation exemptions, attribute exemptions and failed attribute reads, directional markers versus =======, and quoted filenames under today’s splitting behavior (packages/agents/source/skills/resolve-rebase-conflicts/claude/scripts/guard-resolve.sh:124).

      “With and without leftover markers” does not establish these compatibility boundaries. Also clarify that first-verb behavior is preserved only where observed: these guards search command boundaries and contain heredoc preprocessing, rather than uniformly inspecting one initial verb.

    4. Dependencies: epic sequencing is not enforced. Replace complex shell with tested Go, starting with gates and merging #588 requires gates and the merge path before hooks. The declared native prerequisites are accurate, but Replace the commit and pull request workflow guards with Go hooks #719 itself has only Ship the Go binaries as release assets installed through mise's github backend #333 as a native blocker. Add the relevant preceding-work dependency or an explicit rollout hold that preserves the epic’s ordering.

    5. Verify: tighten malformed-input and performance checks. AC4’s plan should assert exit 2 and a non-empty stderr reason for each guard, including valid JSON followed by invalid trailing input. The existing decoder reads only one JSON value (internal/hookio/hookio.go:52). AC10 needs a defined command/repository workload and resolve-path count; attribute lookup occurs per candidate file, so an unspecified invocation gives no reproducible performance boundary.

    Split

    No mandatory split. Two guards remain a coherent S change after #719 supplies the shared infrastructure. Split per guard only if the characterization corpus exceeds one reviewable PR.

    Definition of done

    Unchecked items are unmet in the current AC/Verify plan or pending implementation evidence.

    • D1 Pending — every AC has a Verify entry, but merged-head verification and PR evidence are not required explicitly.
    • D2 Pending — exact-head owner approval is specified; independent implementation review and finding disposition remain pending.
    • D3 Unmet — owner approval is specified, but passing required checks on that head is absent from Verify.
    • D4 Unmet — gitleaks, the private-string leak scan, and post leak reviews are absent.
    • D5 Unmet — signed commits, including the squash, are absent.
    • D6 Unmet — no documentation check or justified “No doc impact” statement.
    • D7 Unmet — Assisted-by trailers and the provenance label are absent.
    • D8 Pending — S sizing is declared; PR size-limit verification or exact-head exception is absent.
    • D9 Unmet — deferred findings lack the required follow-up and Related-section procedure.
    • D10 Unmet — closing reference, Done state, and final merge-commit status comment are absent.
    • devtools-1 Unmet — Verify omits mise run check, mise run cover-go-check, and mise run cover-py-check.
    • devtools-2 Partially covered — AC8 checks generated skills, but the ordered generate-skills → skill-tokens → apm install sequence and committing its results are missing.
    • devtools-3 Unmet — these deployed hooks change consumer requirements; adoption steps or a justified “No consumer impact” statement are missing.

    Round: 1 of 3

  10. tbhb-agent
    marked this as blocked on Oct 25, 2026

  11. tbhb-agent commented on Oct 25, 2026

    @tbhb-agent
    tbhb-agent
    on Oct 25, 2026
    ContributorAuthor

    Refinement: READY — compatibility contract and verification are sufficiently defined

    Checked

    Gaps

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

    Split

    No mandatory split. Two guards remain a coherent S change once #719 supplies shared infrastructure. Split per guard if the characterization corpus exceeds one reviewable PR.

    Definition of done

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

    • D1 Pending — exact-head checks and evidence for every AC are planned; merged-head author verification remains pending.
    • D2 Pending — independent approval of the exact merged head and finding disposition remain delivery obligations.
    • D3 Delivery requires checks green on the exact head; Operator authority requires exact-head owner approval.
    • D4 Unmet in the issue-specific plan — the three leak-review obligations are not enumerated.
    • D5 Pending — signed commits and squash require delivery evidence.
    • D6 Approach requires migration documentation explaining the new binary prerequisite, or a justified impact statement.
    • D7 Pending — Assisted-by trailers and the provenance label require delivery evidence.
    • D8 Pending — S sizing is declared; actual PR limits or an approved exception remain to be verified.
    • D9 Pending — follow-up issues, Related references and links from deferred code remain delivery obligations.
    • D10 Pending — closing reference, Done state and final merge-commit status comment follow implementation.
    • devtools-1 Delivery explicitly requires mise run check, mise run cover-go-check and mise run cover-py-check.
    • devtools-2 Approach specifies generate-skills → skill-tokens → apm install and committing rewritten output; AC10 verifies generation.
    • devtools-3 Approach requires consumer installation and deployment steps in docs/agent-skills-migration.md, or a justified “No consumer impact” statement.

    Round: 2 of 3

  12. tbhb-agent
    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

  13. tbhb-agent commented on Oct 26, 2026

    @tbhb-agent
    tbhb-agent
    on Oct 26, 2026
    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 rebase and conflict-resolution guard scripts forward to the Go hooks when present and run the existing shell guards otherwise. 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

    Type

    Feature

    Fields

    Priority

    Standard

    Work type

    Planned

    Projects

    • devtools
      Status
      Backlog

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions