Repository navigation
feat(guard-workflow): add a Go guard for pull request commands - #1047
tbhb-agent wants to merge 1 commit into
Conversation
tbhb-agent
added
change:feat
![tbhb-dev-reviews[bot]](https://avatars.githubusercontent.com/u/288164213?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed head: 4f165f4
Review: CHANGES REQUESTED
Loosens: no
Owner-tier: no
The live body of #328 and all four comments were read, including the final Refinement: READY verdict. This head implements the deliberately narrowed four-guard slice, with one malformed-payload gap below. ALL of #328's live ACs are not met: AC8 fails for a valid JSON prefix followed by invalid trailing input, and AC11 therefore does not prove the complete contract. No live AC is assigned to a later slice; the other guards and rollout are explicit non-goals.
| AC | Met on reviewed head | Evidence |
|---|---|---|
| AC1 | Yes | internal/guard/guard.go:24, cmd/guard-workflow/run_test.go:29, cmd/guard-workflow/run_test.go:123: five read verbs, silent status 0. |
| AC2 | Yes | internal/guard/guard.go:25, cmd/guard-workflow/run_test.go:32: mutations/unknown verbs denied, publishing reason, first-match chains preserved. |
| AC3 | Yes | internal/guard/guard.go:35, cmd/guard-workflow/run_test.go:141: common verdicts and distinct publishing/squash entry points. |
| AC4 | Yes | internal/guard/guard.go:16, internal/guard/guard.go:56, cmd/guard-workflow/run_test.go:38, cmd/guard-workflow/run_test.go:64, cmd/guard-workflow/run_test.go:77: command positions, whitespace, quotes/heredocs and whole-text flags. |
| AC5 | Yes | internal/guard/guard.go:39, internal/guard/guard.go:72, cmd/guard-workflow/run_test.go:44: checks watching and run watching retain their skill-specific decisions. |
| AC6 | Yes | internal/guard/guard.go:62, cmd/guard-workflow/run_test.go:52: log sweep denied unless the whole text contains --job. |
| AC7 | Yes | internal/guard/guard.go:67, cmd/guard-workflow/run_test.go:58: force forms denied unless the whole text contains the lease flag. |
| AC8 | No | cmd/guard-workflow/run.go:33 uses the first-value decoder at internal/hookio/hookio.go:55; trailing invalid bytes escape decoding. Existing absent/empty/truncated/non-Bash cases are asserted at cmd/guard-workflow/run_test.go:155, but omit this case. |
| AC9 | Yes | internal/hookio/hookio.go:129, cmd/guard-workflow/run.go:41, cmd/guard-workflow/shell_parity_test.go:37: exact stderr/status/stdout comparisons for the denial corpus. |
| AC10 | Yes | internal/guard/imports_test.go:19 parses every non-test source file and allows only fmt/regexp/strings; cmd/guard-workflow/run_test.go:173 repeats the corpus. Rule source has no external effects. |
| AC11 | No, complete contract; matrix itself green | All four native test jobs succeeded, but cmd/guard-workflow/run_test.go:158 omits the malformed-suffix case that fails AC8 on each platform. |
Findings:
- P2 — Treat malformed trailing input as undecodable before judging its command.
cmd/guard-workflow/run.go:33callshookio.Decode, whose singleDecoder.Decodeatinternal/hookio/hookio.go:55accepts a complete first object without inspecting the remainder. For example,{"tool_input":{"command":"gh pr merge 1"}} trailingreaches the PR rule and exits 2. The frozen PR shell was exercised on that exact input: it exits 5 on the jq parse error with no denial header, so it is non-blocking under #328's stated contract. AC8 requires undecodable input to return silent status 0. Validate the complete workflow input and return 0 for an invalid suffix, with a regression assertion; preserve the existing decoder behavior for guard-markdown. This is a tightening, not a loosening, but it violates the behavior-preserving scope. - P2 — Complete the slice's verification and deferred-work record. The PR description's Verification section explicitly leaves AC11 without a command/result/link, groups AC4 through AC7 rather than citing each ID separately, and supplies neither the required No doc impact statement nor a No consumer impact statement with its rationale. The deferred caller flip documented at
internal/guard/reasons.go:6has no issue reference there or in Related. Related correctly says Related to #328 and contains no closing reference; retain that relationship, add the missing evidence/declarations, and link filed follow-ups for the outstanding guard/rollout work. D1, D6 and D9 remain unmet.
Later slices and issue coverage: the commit guard has an overlapping filed issue, #719; rebase/conflict guards have #722. The proposed fix-prose guard slice has no dedicated filed issue found: #327 explicitly excludes arming/release/guards and leaves them with #328. The release-guard port likewise has no dedicated filed issue found: #811 explicitly leaves surviving hook rules with #328. General asset production and installation are filed as #333 and #544, and replacement-path protection as #753; those do not file the complete guard-workflow caller flip/removal slice. #719/#721 contain older overlapping rollout plans with different malformed-input policy, so they are not an exact substitute for the new deferred plan. The five proposed follow-ups are therefore not all filed. Merging this PR leaves #328 open; it does not close the original guard migration.
Scope and parity:
cmd/guard-workflow/shell_parity_test.go:1limits the executable shell oracle to Darwin/Linux. It needsgoandbashon PATH, and the unchanged shells additionally needcatandjq(packages/agents/source/skills/pr/claude/scripts/guard-gh.sh:41). It does not invoke uv, Python, mise, gh or git. Go subprocesses use the existing module/toolchain and build cache. The native Darwin/Linux Go-only jobs passed with these dependencies. Windows does not compile this oracle file; its portable acceptance, payload, parity-of-verdict and import tests run natively with Go. Not.Skipand no weakened existing assertions were introduced.- All nine changed files have zero nolint directives and zero matches for
.agents/agent-skills.conf:5loosening_markers. The frozen source/generated shell guards, existing Bats suites andtools/test-guards.share unchanged. No mechanical gates were re-run for this review. - No changed path matches golden_paths, constitutional_paths, gate_config_paths or owner_paths in
.agents/agent-skills.conf. The new Go command's name does not make it match the guard*.sh protections. Default CODEOWNERS routing to @tbhb applies, but no supplied owner-tier path is changed. #753 remains the open prerequisite for protecting replacement enforcement before rollout. - Over-engineering: none found. The skill argument selects the four required behaviors; the rule package, binary glue, hookio block helper and import check each implement the issue's stated approach or an AC. No additional option/configuration layer was added.
Size: scc 4.1.0, additions plus deletions from origin/main...4f165f4e664ec0acc07b8040562f1fa2b69aa987. Code includes tests and Go string literals; prose below means source comments. No standalone prose file changed. Columns are added/deleted lines.
| Changed path | Status | Owner-tier | Code +/- | Prose +/- | Blank +/- |
|---|---|---|---|---|---|
cmd/guard-workflow/main.go |
A | No | 5/0 | 3/0 | 3/0 |
cmd/guard-workflow/run.go |
A | No | 40/0 | 3/0 | 7/0 |
cmd/guard-workflow/run_test.go |
A | No | 177/0 | 2/0 | 13/0 |
cmd/guard-workflow/shell_parity_test.go |
A | No | 55/0 | 5/0 | 6/0 |
internal/guard/guard.go |
A | No | 61/0 | 8/0 | 7/0 |
internal/guard/imports_test.go |
A | No | 32/0 | 2/0 | 4/0 |
internal/guard/reasons.go |
A | No | 57/0 | 3/0 | 9/0 |
internal/hookio/hookio.go |
M | No | 10/0 | 2/1 | 1/0 |
internal/hookio/hookio_test.go |
M | No | 15/0 | 0/0 | 2/0 |
| Total | 9 files | No | 452/0 | 28/1 | 52/0 |
532 additions + 1 deletion = 533 physical changed lines: 452 code, 29 prose/comments, 52 blanks. This is below 800 code and 2000 prose; no size P1. The reported 680 code is also below the limit, but it is not this independent diff count; counting the nine complete post-change files with the specified scc returns 675 code.
Checks were read once: every reported context was COMPLETED SUCCESS, including test (ubuntu-24.04), test (ubuntu-24.04-arm), test (macos-15), test (windows-2025) and coverage. Mergeable: MERGEABLE; mergeStateStatus: BLOCKED; draft: false. Coverage: SUCCESS (combined-report job). CodeQL was re-read independently: COMPLETED SUCCESS, zero annotations, no new alerts in changed code; both analyze (go) and analyze (actions) also succeeded. A green combined-report job is not evidence that the separate Go coverage-floor command ran.
Definition of done for #328 (Feature):
- D1 Unmet: AC8 gap and missing explicit per-AC/AC11 evidence in the PR description; see findings 1 and 2.
- D2 Unmet: this independent review requests changes on the exact head; findings need resolution or an accepted filed disposition.
- D3 Every reported required-check context succeeded on this head; no gate loosening or owner-tier edit.
- D4 gitleaks succeeded; the private-strings leak scanner was invoked separately on each of the nine exact-head changed files and every invocation returned clean/0. This review body was inspected for leaks and scanned before posting through gh-post.
- D5 Current commit is GitHub-verified, reason valid. Final squash signature is not yet available.
- D6 Unmet: no documentation changes and no explicit No doc impact explanation in Verification; see finding 2.
- D7 Current commit has Assisted-by trailers and the PR has provenance:agent-assisted; final squash trailer remains pending.
- D8 452 changed code lines and 29 changed comment/prose lines fit 800/2000 without an exception.
- D9 Unmet: not all proposed follow-ups are filed or listed under Related; see the later-slice inventory and finding 2.
- D10 Intentionally pending: this slice relates to #328 and leaves it open. No merge commit, closing reference, Done transition or final Status comment exists for completion of #328.
- devtools-1 The CI components are green, but the PR supplies no exact-head evidence for the complete mise run check plus separate cover-go-check/cover-py-check requirement. Those gates were not re-run during review.
- devtools-2 Not applicable: no skill source, generated package or deployment changed.
- devtools-3 Unmet declaration: no No consumer impact statement in Verification. The prose already explains that no skill calls the binary, which supplies the rationale for recording it.
Round: 1 of 3
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 <tony@tonyburns.net>
4f165f4 to
1bf8f21
Compare
There was a problem hiding this comment.
Reviewed head: 1bf8f21
Review: CHANGES REQUESTED
Loosens: no
Owner-tier: no
Read #328's live body and all four comments, including Refinement: READY, the round-1 review, the description and the complete exact-head diff. No mechanical gates were rerun.
Round-1 disposition: F1's original immediate malformed suffix is fixed by cmd/guard-workflow/run.go:33, with regressions at cmd/guard-workflow/run_test.go:161 and cmd/guard-workflow/shell_parity_test.go:55. Complete-input validation remains incomplete as detailed below. F2's requested separate AC1–AC11 Verification rows, matrix evidence, “No doc impact” and “No consumer impact” explanations are now present. The earlier deferred-work filing/listing concern remains unproven: Related still lists only #328, and internal/guard/reasons.go:6 has no follow-up issue reference.
| AC | Met on this head | Evidence |
|---|---|---|
| AC1 | Yes | internal/guard/guard.go:31; cmd/guard-workflow/run_test.go:29, cmd/guard-workflow/run_test.go:123: five read verbs, silent status 0. |
| AC2 | Yes | internal/guard/guard.go:25, internal/guard/guard.go:36; cmd/guard-workflow/run_test.go:32: mutating/unknown verbs denied, first-match behavior retained. |
| AC3 | Yes | internal/guard/guard.go:35; cmd/guard-workflow/run_test.go:141: identical verdicts and distinct entry points. |
| AC4 | Yes | internal/guard/guard.go:16, internal/guard/guard.go:53; cmd/guard-workflow/run_test.go:38, cmd/guard-workflow/run_test.go:64, cmd/guard-workflow/run_test.go:77: command positions and whole-text flags. |
| AC5 | Yes | internal/guard/guard.go:39, internal/guard/guard.go:72; cmd/guard-workflow/run_test.go:44: watching forms and plain checks. |
| AC6 | Yes | internal/guard/guard.go:62; cmd/guard-workflow/run_test.go:52: log sweep and job exception. |
| AC7 | Yes | internal/guard/guard.go:67; cmd/guard-workflow/run_test.go:58: force forms and lease exception. |
| AC8 | No | internal/hookio/hookio.go:67 validates at most one additional JSON value; malformed text after that is ignored. See F1. |
| AC9 | Yes, denial protocol | internal/hookio/hookio.go:150; cmd/guard-workflow/shell_parity_test.go:37: exact stderr/status/stdout comparisons for the denial corpus. |
| AC10 | Yes | internal/guard/imports_test.go:19; cmd/guard-workflow/run_test.go:174: source import inspection and repeat-run determinism. Rule source has no external effects. |
| AC11 | No, complete contract | All four native matrix jobs succeeded; cmd/guard-workflow/run_test.go:155 and cmd/guard-workflow/shell_parity_test.go:55 omit the remaining malformed-suffix case. |
Verification cites every AC ID separately. Green matrix results establish the implemented test corpus, not the missing AC8 case.
Findings:
- P2 — F1 remains incomplete: consume the entire input before accepting it.
internal/hookio/hookio.go:66-70decodes only one trailing value and returns success when that value is valid, without checking the remaining bytes. For example,{"tool_input":{"command":"gh pr merge 1"}} {} xdecodes the command and then{}, leavingxunread.cmd/guard-workflow/run.go:33consequently reaches the rule and returns status 2 underpr, whereas the frozen shell's whole-input jq parse fails onxand the AC8 exception requires silent status 0. This follows directly from the decoder control flow; no test or gate was executed for this review. Validate the complete remainder and add regression coverage with a valid intervening value before the malformed suffix, while preservingDecodefor its existing caller.internal/hookio/hookio_test.go:185currently tests only immediately malformed suffixes.
Over-engineering: none found. The skill selector, rule package, protocol helper and import test are required by the issue's Approach or ACs. No unnecessary guard, option or configuration layer was added. No t.Skip, nolint, weakened assertion or gate loosening was introduced. Existing shell guards and enforcement configuration are unchanged.
Size: scc 4.1.0 on added/deleted diff text against base fef5535. Counts include tests; prose means source comments. No standalone prose changed. Each changed path was independently scanned with the private-strings leak scanner, one file per invocation: all nine returned 0 with zero findings.
| Changed path | Owner-tier | Code +/- | Prose +/- | Blank +/- |
|---|---|---|---|---|
cmd/guard-workflow/main.go |
No | 5/0 | 3/0 | 3/0 |
cmd/guard-workflow/run.go |
No | 40/0 | 3/0 | 7/0 |
cmd/guard-workflow/run_test.go |
No | 178/0 | 2/0 | 13/0 |
cmd/guard-workflow/shell_parity_test.go |
No | 78/0 | 6/0 | 7/0 |
internal/guard/guard.go |
No | 61/0 | 8/0 | 7/0 |
internal/guard/imports_test.go |
No | 32/0 | 2/0 | 4/0 |
internal/guard/reasons.go |
No | 57/0 | 3/0 | 9/0 |
internal/hookio/hookio.go |
No | 29/3 | 6/2 | 3/0 |
internal/hookio/hookio_test.go |
No | 27/0 | 0/0 | 3/0 |
Total: 507 added + 3 deleted code = 510, 33 added + 2 deleted prose = 35, 56 blanks; 601 physical changed lines. Below 800 code / 2000 prose. Default CODEOWNERS routing applies, but no changed path matches the supplied owner-tier categories or .agents/agent-skills.conf protected patterns. .agents/review-instructions.md is absent at this head.
Checks were read once: every reported context was COMPLETED SUCCESS, including all four test platforms, coverage, gitleaks and CodeQL. Mergeability: MERGEABLE; merge state: BLOCKED; draft: false.
Definition of done for #328 (Feature):
- D1 Verification now cites all AC IDs with command/result or check evidence, but AC8 remains unmet and final merged-head verification is pending.
- D2 This independent exact-head review requests changes; F1 requires resolution. Final merged-head approval is pending.
- D3 All reported checks succeeded on this head; no gate loosening needs approval.
- D4 Gitleaks succeeded; each of the nine changed exact-head files returned zero findings from the private-strings scan. This post was inspected for leaks and is submitted through the scanning publisher.
- D5 Current commit is GitHub-verified with reason valid; the final squash signature is pending.
- D6 Verification explicitly says “No doc impact” and explains that no caller invokes the new binary.
- D7 Current commit has Assisted-by trailers and the PR has provenance:agent-assisted; the final squash trailer is pending.
- D8 510 changed code and 35 changed prose lines fit the limits.
- D9 Deferred-work filing/listing from round 1 remains unproven. Related names only #328; the later caller flip at
internal/guard/reasons.go:6has no issue reference. - D10 Pending: #328 remains open, Related is non-closing, and no merge commit, Done transition or final Status comment exists. This is the bounded implementation slice; rollout is explicitly excluded.
- devtools-1 CI is green, but exact-head evidence for the complete
mise run checkand both separate coverage-floor commands is not supplied..github/workflows/ci.yml:180runs binary coverage and.github/workflows/ci.yml:248merges reports; these do not establish the separate floor commands. No gates were rerun. - devtools-2 Not applicable: no skill sources or generated/deployed skill output changed.
- devtools-3 Verification explicitly says “No consumer impact” and explains that no deployed caller registers or runs the binary.
Round: 2 of 3
Summary
guard-workflowis a standalone hook binary that judges a Bash tool call for thepr,merge-pr,watch-pr, andfix-prskills and denies the commands those skills route through their wrapping scripts. No skill invokes the binary yet.Why
Both the
prandmerge-prguards must agree wherever both are live, yet each repeats the same command-position matching and the same read-onlyghallowlist. A Go rule follows the layoutguard-markdownestablished. Tests compare it with the shell guards. A later change can then switch the hooks without altering a verdict. This slice leaves the shell guards and the skill declarations that call them as they were.Invalid JSON immediately after the first value makes decoding fail, as it does in the shell guards. The call then runs, because the guard treats the payload as unreadable.
Verification
The commands below ran on the head of this pull request.
go test ./cmd/guard-workflow -run 'TestAcceptance/AC1/' -count=1go test ./cmd/guard-workflow -run 'TestAcceptance/AC2/' -count=1prandmerge-pragreementgo test ./cmd/guard-workflow -run TestAC3Parity -count=1go test ./cmd/guard-workflow -run 'TestAcceptance/AC4/' -count=1go test ./cmd/guard-workflow -run 'TestAcceptance/AC5/' -count=1gh run viewlogsgo test ./cmd/guard-workflow -run 'TestAcceptance/AC6/' -count=1git pushforce formsgo test ./cmd/guard-workflow -run 'TestAcceptance/AC7/' -count=1go test ./internal/guard ./cmd/guard-workflow -run 'TestAC10' -count=1test (ubuntu-24.04),test (ubuntu-24.04-arm),test (macos-15)andtest (windows-2025)jobsPATH=<go dir>:/usr/bin:/bin go test ./cmd/guard-workflow ./internal/guard ./internal/hookio -count=1GOOS=windows go vet ./cmd/guard-workflow ./internal/guard ./internal/hookiogo test ./cmd/guard-markdown ./internal/hookio -count=1mise run lint-goNo doc impact: the binary has no caller yet, and the docs describe nothing that changed.
No consumer impact: nothing deployed to consumers registers or runs the binary, and the shell guards they use are unchanged.
Risk
The new binary has no caller yet, and the shared decoder that
guard-markdownuses keeps its behavior, because the strict check is a separate function that only this guard calls. When someone registers it directly, a defect could allow a command the shell guard denies or deny one it allows, and the denial text is what the agent sees. The parity test runs the shell guards through bash and compares verdicts and stderr bytes. Reverting the change removes the binary and the package.Compatibility: none
Loosens: no
Related
Related to #328