picatz / picatz/flowstate

tools/shipcheck, tools/hooks/mergeguard: follow-ups from the REST fallback review

Open
#1,950 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Go
Stars
9
Forks
0
Avg merge
3h 3m
Merged PRs (30d)
509

Description

The exact-head review of #1948 returned PASS with five non-blocking notes. None of them changes the merge decision on that change, so they were deferred here rather than producing another head. Recorded against e1a95d9.

1. Classic branch protection is decided from a field whose visibility is unverified

classicReviewRules in tools/shipcheck/rest.go reads /repos/{owner}/{repo}/branches/{branch} and treats protection.enabled == false as "no classic protection", reading the protection document only when it is true. The comment claims a branch whose protection document is unreadable is an error rather than a silent "no requirement". That holds only if protection.enabled is visible as true to a token that cannot read the protection document, which is unverified.

Falsifiable check: read /repos/{owner}/{repo}/branches/main with a non-admin token against a repository whose main has classic protection with required_approving_review_count >= 1. If enabled comes back false there, shipcheck reports no review requirement where GraphQL reports REVIEW_REQUIRED.

Impact today is nil: this repository uses rulesets with classic protection off, and the failure direction is a PASS on a pull request GitHub itself would refuse to merge, not a merge that bypasses a rule.

2. The reviews list is walked twice per run on the REST path

restReviews is paged once for loadReviewsREST and again for reviewDecisionREST, doubling the reviews API calls whenever the fallback runs. One walk shared by both would halve it.

3. Dead statement in a hook test

tools/hooks/mergeguard/rest_test.go calls server.Client() and discards the result.

4. Pre-existing nil pull request in the hook's GraphQL decode

In tools/hooks/mergeguard/main.go, graphQLResponse.Data.Repository.PullRequest is a value struct, unlike threadPage in tools/shipcheck, which uses a pointer and errors on nil. A GraphQL 200 carrying "pullRequest": null with no errors array would decode to zero threads and allow a merge. The reviewer could not construct a reachable path, since GitHub returns an errors entry alongside that null, and this predates the REST work. Matching shipcheck's pointer-and-error shape would make the fail-closed behavior structural rather than dependent on GitHub's error reporting.

5. flowstate-pr-tidy resolves gate-relevant state by prose rule

The subagent may resolve review threads, which is exactly the evidence mergeguard and shipcheck read. Its "resolve only when the disposition is visible on the thread" rule is model judgment, not a mechanism. It adds no capability the main agent lacks, but it concentrates a gate-relevant mutation in an agent whose correctness is prose-enforced. Worth deciding whether that wants a check.

Acceptance

Items 2 and 3 are mechanical. Item 4 is a small structural change with a test that feeds a null pull request. Item 1 needs the probe above before any code change; if the probe shows the gap is real, the fix is to read the protection document whenever the branch document does not prove protection is off, and to error when it is unreadable. Item 5 is a decision, not a defect.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with tools/shipcheck/rest.go and tools/hooks/mergeguard/main.go, then inspect tools/hooks/mergeguard/rest_test.go. Run the stated REST protection probe and add the null-pull-request test described in the acceptance criteria; verify the reviews list is walked once and remove the discarded server.Client() call. Item 5 requires a separate maintainer decision.

Written by the indexing model from the issue text.

Assessment

Tech stack
github, go, graphql
Domain
ci-cd, devtools, security
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.