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()) +}