From 1bf8f21a9c128963001a3c10e8784ae3f1d3d1b1 Mon Sep 17 00:00:00 2001 From: "tbhb.dev Agent" Date: Sun, 1 Nov 2026 08:59:37 -0500 Subject: [PATCH] feat(guard-workflow): add a Go guard for pull request commands The guard hooks that keep pull request, merge, watch, and fix workflows behind their wrapping scripts each repeat the same command-position matching in Bash. A rule written once in Go, with tests that compare it to the shell guards, prepares those skills to share one definition when their hooks adopt it. No skill invokes the binary yet. The shell guards remain the active hooks. Related to #328. Assisted-by: codex:gpt-6.1-sol Assisted-by: claude-code:claude-sonnet-5-5 Signed-off-by: Tony Burns --- cmd/guard-workflow/main.go | 11 ++ cmd/guard-workflow/run.go | 50 ++++++ cmd/guard-workflow/run_test.go | 193 ++++++++++++++++++++++++ cmd/guard-workflow/shell_parity_test.go | 91 +++++++++++ internal/guard/guard.go | 76 ++++++++++ internal/guard/imports_test.go | 38 +++++ internal/guard/reasons.go | 69 +++++++++ internal/hookio/hookio.go | 43 +++++- internal/hookio/hookio_test.go | 30 ++++ 9 files changed, 596 insertions(+), 5 deletions(-) create mode 100644 cmd/guard-workflow/main.go create mode 100644 cmd/guard-workflow/run.go create mode 100644 cmd/guard-workflow/run_test.go create mode 100644 cmd/guard-workflow/shell_parity_test.go create mode 100644 internal/guard/guard.go create mode 100644 internal/guard/imports_test.go create mode 100644 internal/guard/reasons.go diff --git a/cmd/guard-workflow/main.go b/cmd/guard-workflow/main.go new file mode 100644 index 00000000..ab51804b --- /dev/null +++ b/cmd/guard-workflow/main.go @@ -0,0 +1,11 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Tony Burns + +// Command guard-workflow judges hook commands for the four gh-family skills. +package main + +import "os" + +func main() { + os.Exit(Run(os.Args[1:], streams{in: os.Stdin, out: os.Stdout, err: os.Stderr})) +} diff --git a/cmd/guard-workflow/run.go b/cmd/guard-workflow/run.go new file mode 100644 index 00000000..3b738a91 --- /dev/null +++ b/cmd/guard-workflow/run.go @@ -0,0 +1,50 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Tony Burns + +package main + +import ( + "fmt" + "io" + + "github.com/tbhb-dev/devtools/internal/guard" + "github.com/tbhb-dev/devtools/internal/hookio" +) + +const exitBlocked = 2 + +type streams struct { + in io.Reader + out io.Writer + err io.Writer +} + +// Run returns the hook exit status, and an allowed call writes to neither stream. +func Run(args []string, s streams) int { + if len(args) != 1 { + return usage(s.err) + } + skill := args[0] + switch skill { + case "pr", "merge-pr", "watch-pr", "fix-pr": + default: + return usage(s.err) + } + payload, err := hookio.DecodeStrict(s.in) + if err != nil { + return 0 + } + reason := guard.Reason(skill, payload.ToolInput.Command) + if reason == "" { + return 0 + } + if blockErr := hookio.Block(s.err, skill, reason); blockErr != nil { + fmt.Fprintln(s.err, "guard-workflow:", blockErr) + } + return exitBlocked +} + +func usage(w io.Writer) int { + fmt.Fprintln(w, "usage: guard-workflow ") + return exitBlocked +} diff --git a/cmd/guard-workflow/run_test.go b/cmd/guard-workflow/run_test.go new file mode 100644 index 00000000..5cdf942f --- /dev/null +++ b/cmd/guard-workflow/run_test.go @@ -0,0 +1,193 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Tony Burns + +package main + +import ( + "bytes" + "encoding/json" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type commandCase struct { + ac, skill, command string + blocked bool + entry string +} + +func commandCases() []commandCase { + cases := make([]commandCase, 0, 138) + for _, skill := range []string{"pr", "merge-pr"} { + entry := "create-pr.sh" + if skill == "merge-pr" { + entry = "squash-merge.sh" + } + for _, verb := range []string{"list", "view", "diff", "status", "checks"} { + cases = append(cases, commandCase{"AC1", skill, "gh pr " + verb, false, ""}) + } + for _, verb := range []string{"create", "edit", "ready", "merge", "close", "comment", "future-verb"} { + cases = append(cases, commandCase{"AC2", skill, "gh pr " + verb + " 1", true, entry}) + } + cases = append(cases, + commandCase{"AC2", skill, "gh pr view 1; gh pr merge 1", false, ""}, + commandCase{"AC2", skill, "gh pr merge 1; gh pr view 1", true, entry}, + commandCase{"AC4", skill, `echo "gh pr merge 1"`, false, ""}, + commandCase{"AC4", skill, "cat <<'EOF'\ngh pr merge 1\nEOF", true, entry}, + commandCase{"AC4", skill, `echo "; gh pr merge 1"`, true, entry}, + ) + } + cases = append(cases, + commandCase{"AC5", "watch-pr", "gh pr checks 1", false, ""}, + commandCase{"AC5", "watch-pr", "gh pr checks 1 --watch", true, "watch-checks.sh"}, + commandCase{"AC5", "watch-pr", "gh pr checks 1 --fail-fast", true, "watch-checks.sh"}, + commandCase{"AC5", "watch-pr", "gh run watch 1", true, "watch-checks.sh"}, + commandCase{"AC5", "fix-pr", "gh pr checks 1", false, ""}, + commandCase{"AC5", "fix-pr", "gh pr checks 1 --watch", true, "watch-pr skill"}, + commandCase{"AC5", "fix-pr", "gh pr checks 1 --fail-fast", true, "watch-pr skill"}, + commandCase{"AC5", "fix-pr", "gh run watch 1", false, ""}, + commandCase{"AC6", "fix-pr", "gh run view 1 --log", true, "diagnose.sh"}, + commandCase{"AC6", "fix-pr", "gh run view 1 --log-failed", true, "diagnose.sh"}, + commandCase{"AC6", "fix-pr", "gh run view 1 --job 2 --log", false, ""}, + commandCase{"AC6", "fix-pr", "gh run view 1 --log-failed --job=2", false, ""}, + commandCase{"AC6", "fix-pr", "gh run view 1", false, ""}, + commandCase{"AC6", "fix-pr", "gh run view 1 --logging", false, ""}, + commandCase{"AC7", "fix-pr", "git push --force origin HEAD", true, "--force-with-lease"}, + commandCase{"AC7", "fix-pr", "git push -f", true, "--force-with-lease"}, + commandCase{"AC7", "fix-pr", "git push --force-with-lease", false, ""}, + commandCase{"AC7", "fix-pr", "git push --force --force-with-lease", false, ""}, + commandCase{"AC7", "fix-pr", "git push --forceful", false, ""}, + commandCase{"AC7", "fix-pr", "git push -foo", false, ""}, + commandCase{"AC4", "watch-pr", `gh pr checks 1; echo "--watch"`, true, "watch-checks.sh"}, + commandCase{"AC4", "fix-pr", "gh pr checks 1\ncat < + +That script checks that the message names this pull request, that the +independent review cleared the exact bytes on disk at the head about to +merge, that the message passes the commit-msg gates, that the pull +request is open, ready, mergeable, and green, and that somebody +confirmed the merge. It then merges pinned to the reviewed head, +removes the drafts, and deletes the branch. + +Calling gh directly writes a message onto the default branch that +nothing reviewed, and nothing lints a squash commit afterwards.` + readingNote + +const watchChecksReason = `This skill wraps the watch so each check becomes one event rather than +one turn: + +` + " " + `Monitor({ +` + " " + `command: "bash .claude/skills/watch-pr/scripts/watch-checks.sh ", +` + " " + `description: "checks on pull request ", +` + " " + `timeout_ms: 1800000, +` + " " + `persistent: false, +` + " " + `}) + +The script emits PASS, SKIP, and FAIL lines as they settle, then exits. +A single 'gh pr checks' without --watch is fine for a one-off look.` + +const watchRunReason = `Watching a run directly reports a workflow rather than the pull request, +and it blocks a turn to do it. Use the skill's watcher instead: + +` + " " + `bash .claude/skills/watch-pr/scripts/watch-checks.sh ` + +const diagnoseReason = `Sweeping the failing steps is what the skill's own diagnosis does, in one +call, with the reproducing task named beside each failure: + +` + " " + `bash .claude/skills/fix-pr/scripts/diagnose.sh + +Run that first. Once you need more than the tail it printed, a single +job's full log stays available: + +` + " " + `gh run view --job --log` + +const forceReason = `A bare force push overwrites whatever the remote has, including commits +this worktree never saw. Use the lease instead: + +` + " " + `git push --force-with-lease origin HEAD + +It refuses when the remote moved since your last fetch, which is exactly +the case a bare --force would destroy.` + +const fixWatchReason = `Waiting on checks belongs to the watch-pr skill, which streams each result +as an event rather than blocking a turn. Invoke that skill instead.` diff --git a/internal/hookio/hookio.go b/internal/hookio/hookio.go index fa08cb65..af6a18de 100644 --- a/internal/hookio/hookio.go +++ b/internal/hookio/hookio.go @@ -27,9 +27,10 @@ const preToolUse = "PreToolUse" var ErrNoReason = errors.New("hookio: refusing to deny without a reason") // ToolInput holds the fields of the tool call under inspection. The set is -// the union across Write and Edit; a field absent from the incoming payload +// the union across Write, Edit and Bash; a field absent from the incoming payload // decodes to its zero value. type ToolInput struct { + Command string `json:"command"` FilePath string `json:"file_path"` Content string `json:"content"` OldString string `json:"old_string"` @@ -48,15 +49,36 @@ type Payload struct { // stays testable without touching the filesystem. type ReadFileFunc func(string) ([]byte, error) -// Decode reads one JSON payload. +// Decode reads one JSON payload and ignores anything after it. func Decode(r io.Reader) (Payload, error) { - var p Payload - if err := json.NewDecoder(r).Decode(&p); err != nil { - return Payload{}, fmt.Errorf("hookio: decoding payload: %w", err) + p, _, err := decode(r) + return p, err +} + +// DecodeStrict reads one JSON payload and rejects text after it that isn't +// JSON. The frozen shell guards parse the whole input, so text after the first +// value makes the payload unreadable there and must here too. +func DecodeStrict(r io.Reader) (Payload, error) { + p, decoder, err := decode(r) + if err != nil { + return Payload{}, err + } + var trailing json.RawMessage + if trailErr := decoder.Decode(&trailing); trailErr != nil && !errors.Is(trailErr, io.EOF) { + return Payload{}, fmt.Errorf("hookio: decoding trailing input: %w", trailErr) } return p, nil } +func decode(r io.Reader) (Payload, *json.Decoder, error) { + var p Payload + decoder := json.NewDecoder(r) + if err := decoder.Decode(&p); err != nil { + return Payload{}, nil, fmt.Errorf("hookio: decoding payload: %w", err) + } + return p, decoder, nil +} + // Resolve recovers the file content the tool call would produce. // // Write supplies the whole document, which doesn't need reconstruction. @@ -123,3 +145,14 @@ func Deny(w io.Writer, reason string) error { } return nil } + +// Block writes the shell skill guards' stderr protocol. The caller exits 2. +func Block(w io.Writer, skill, reason string) error { + if strings.TrimSpace(reason) == "" { + return ErrNoReason + } + if _, err := fmt.Fprintf(w, "Blocked by the %s skill guard.\n\n%s\n", skill, reason); err != nil { + return fmt.Errorf("hookio: writing block: %w", err) + } + return nil +} diff --git a/internal/hookio/hookio_test.go b/internal/hookio/hookio_test.go index 18c2f167..e245f20e 100644 --- a/internal/hookio/hookio_test.go +++ b/internal/hookio/hookio_test.go @@ -173,3 +173,33 @@ func TestDeny(t *testing.T) { assert.Empty(t, buf.String()) }) } + +func TestDecodeCommand(t *testing.T) { + t.Parallel() + p, err := hookio.Decode(strings.NewReader(`{"tool_name":"Bash","tool_input":{"command":"gh pr view 1"}}`)) + require.NoError(t, err) + assert.Equal(t, "gh pr view 1", p.ToolInput.Command) +} + +func TestDecodeStrictTrailingInput(t *testing.T) { + t.Parallel() + first := `{"tool_name":"Bash","tool_input":{"command":"gh pr merge 1"}}` + for _, trailing := range []string{"x", "{", "}", "garbage text"} { + _, err := hookio.DecodeStrict(strings.NewReader(first + trailing)) + require.Error(t, err, trailing) + _, err = hookio.Decode(strings.NewReader(first + trailing)) + require.NoError(t, err, trailing) + } + _, err := hookio.DecodeStrict(strings.NewReader(first + "\n")) + require.NoError(t, err) +} + +func TestBlock(t *testing.T) { + t.Parallel() + var out bytes.Buffer + require.NoError(t, hookio.Block(&out, "pr", "Use the entry point.")) + assert.Equal(t, "Blocked by the pr skill guard.\n\nUse the entry point.\n", out.String()) + out.Reset() + require.ErrorIs(t, hookio.Block(&out, "pr", " \n"), hookio.ErrNoReason) + assert.Empty(t, out.String()) +}