Skip to content

fix(mt#2312): Name the cause of zero check_runs, and stop blaming behind - #3058

Merged
edobry merged 2 commits into
mainfrom
task/mt-2312
Aug 17, 2026
Merged

fix(mt#2312): Name the cause of zero check_runs, and stop blaming behind#3058
edobry merged 2 commits into
mainfrom
task/mt-2312

Conversation

@minsky-ai

@minsky-ai minsky-ai Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Summary

The merge gate's CI-presence floor (mt#1309) denied every zero-check_runs PR with the same message: "This is the GitHub Actions webhook-miss class", prescribing an empty-commit nudge. Zero check_runs has two causes and they need opposite recoveries. When the cause is a merge conflict, GitHub never built refs/pull/N/merge, so no pull_request workflow could dispatch — the nudge cannot work, and it re-heads the branch, invalidating any existing reviewer APPROVE and costing another round.

That is not hypothetical: mem#537 records three recurrences (R1 2026-06-04, R2 and R3 on 2026-08-16/17), each of which paid both costs.

The gate now reads the PR's own merge state on the zero path and names the cause.

What planning falsified before any code was written

Two premises this task asserted turned out to be false. Both would have shipped as defects.

behind does not suppress CI dispatch

The spec's SC2 grouped mergeable_state ∈ {dirty, behind, unknown} as "unmergeable", and both memories it was written from (mem#321, mem#537) say GitHub "cannot form refs/pull/N/merge" for dirty or behind.

Measured: PR #3042's base was main's tip at 02:24:45Z. By 02:41:52Z main was 13 commits ahead of that base (compare/84255d5a...02361c4f6ahead_by: 13). The push at 02:43:36Z — with the branch 13 commits behind — dispatched the full 20-check set. Four other open PRs read in the same window (#3050, #3051, #3052, #2945) were all behind, all mergeable: true, all carrying 13–20 check runs.

Only a real conflict stops the merge ref forming. behind has mergeable: true; it blocks the merge button under branch protection, which is a different thing. Grouping it with dirty would send an agent to session_update for what is actually a webhook miss — the same class of wrong answer this gate exists to stop.

blocked is the trap in the other direction, and the spec had it right: with required checks configured, a genuine webhook miss presents as blocked precisely because the required checks are missing. It is a symptom of the zero, not a cause.

So the discriminator is mergeable alone. mergeable_state is reported for triage and never decides. This matches what mt#4182 shipped for session_pr_checks (eabb96d2b, merged earlier today), which branches on mergeable === false and explicitly rejected a broader predicate for the same reason. The two surfaces must not disagree about which state means "CI could not have dispatched."

Both memories are corrected in this task's scope (mem#537 ## The cheap discriminator, mem#321's 2026-06-05 banner).

The merge state is not "one cheap added field"

The spec said the PR object is already fetched, so mergeability is "one cheap added field/call." The gate resolves its PR through resolvePrRefByBranchfetchPrMetaByBranch, which runs gh pr list --head. Probed directly:

$ gh pr list --repo edobry/minsky --head task/mt-4191 --json number,mergeable,mergeStateStatus
[{"mergeStateStatus":"UNKNOWN","mergeable":"UNKNOWN","number":3052}]
$ gh pr view 3052 --repo edobry/minsky --json mergeable,mergeStateStatus
{"mergeStateStatus":"BEHIND","mergeable":"MERGEABLE"}

The list endpoint does not compute mergeability — GitHub runs that as a background job which a GET on the individual PR starts. Extending PR_META_JSON_FIELDS (the route the spec proposed) would have returned UNKNOWN on every call: a change that compiles, typechecks, ships, and silently classifies every PR as inconclusive forever, with no error anywhere.

The change

classifyZeroCheckRuns(state) — a pure function, three outcomes:

merge state cause prescribed recovery
mergeable === false unmergeable-branch session_update + resolve; explicitly warns off the empty commit and names its cost (re-heads the branch, invalidates APPROVE)
mergeable === true (any mergeable_state) webhook-miss the pre-mt#2312 message, unchanged
mergeable === null, or the read failed inconclusive lists both causes and their opposite recoveries rather than asserting either

readMergeState is the IO half, kept separate so the classifier stays pure and directly testable. It re-reads once when mergeable is null, because the first GET is what starts GitHub's job — without that, the common case would report inconclusive and the discriminator would be useless.

getMergeState is passed to evaluateCheckRunsPresence as a thunk, so the extra single-PR read happens only on the zero path — which is already denying the merge. An ordinary merge pays nothing. A test asserts this by passing a thunk that throws.

fetchPullRequestMergeStateRaw joins the existing raw fetchers in pr-context.ts, with the gh pr list finding recorded in its doc comment so the cheaper-looking route is not re-attempted.

The test file was split — require-review-before-merge.test.ts crossed the 1500-line max-lines ceiling once these landed, so the zero-path tests live in require-review-before-merge.zero-check-runs.test.ts.

Execution evidence:

$ bun test --preload ./tests/setup.ts --timeout=15000 \
    ./.minsky/hooks/require-review-before-merge.test.ts \
    ./.minsky/hooks/require-review-before-merge.zero-check-runs.test.ts
 149 pass
 0 fail
 307 expect() calls
Ran 149 tests across 2 files. [58.00ms]

AT1 — a conflicted PR (mergeable: false, dirty) classifies merge-conflict; the message contains session_update and Do NOT push an empty commit, and does NOT contain wake the webhook.
AT2 — a behind but mergeable PR classifies webhook-miss, not unmergeable. This is the regression test for the corrected premise.
AT3 — clean classifies webhook-miss (unchanged).
AT4 — blocked classifies webhook-miss (unchanged) — the required checks are missing because CI never ran.
AT5 — unresolved mergeability classifies inconclusive; the message contains both session_update and empty commit.
AT6 — see ## Live verification below. The unit half is a fixture of PR #3031's shape; the live half exercises the real gh api call and parse.

SC1 — the read is a single-PR GET. fetchPullRequestMergeStateRaw calls gh api repos/<repo>/pulls/<n>; the gh pr list route is ruled out in its doc comment with the probe output above.
SC2 — covered by AT1.
SC3 — covered by AT2, AT3, AT4. The webhook-miss text is byte-identical to the pre-change message on the assertions the mt#1309 tests already pinned (mt#1309 / PR #763 lineage, noFiles/noStage, /merge-coordination step 7a), which all still pass.
SC4 — readMergeState re-reads once on null; parseMergeStateResponse treats null as a KNOWN not-yet-computed value rather than a parse failure (its own test), and an unresolved second read renders inconclusive (AT5).
SC5 — classifyZeroCheckRuns is pure and unit-tested per branch, plus a test asserting mergeable_state alone never decides (same string, opposite mergeable, opposite result).

Full suite, typecheck and lint:

$ MINSKY_PREPUSH_FULL_SUITE=1 bun scripts/run-tests-gated.ts
Ran 13988 tests across 942 files. [196.30s]
run-tests-gated.ts: all test steps passed.

$ validate_typecheck  → 0 errors across 8 projects
$ validate_lint       → 0 errors, 0 warnings across 3720 files

Negative control 1 — the discriminator is load-bearing:

classifyZeroCheckRuns was short-circuited to return "webhook-miss" (the pre-mt#2312 behavior) and the suite re-run. 8 tests went red, all of them mt#2312's; every pre-existing mt#1309 assertion still passed, which is correct — the webhook-miss path is unchanged for a mergeable PR.

(fail) classifyZeroCheckRuns (mt#2312) > AT1: a conflicted PR is the merge-conflict cause
(fail) classifyZeroCheckRuns (mt#2312) > AT5: unresolved mergeability is inconclusive, not guessed at
(fail) classifyZeroCheckRuns (mt#2312) > an unreadable merge state is inconclusive
(fail) classifyZeroCheckRuns (mt#2312) > mergeable_state alone never decides — only `mergeable` does
(fail) evaluateCheckRunsPresence zero-path messages (mt#2312) > the conflict message prescribes session_update and warns OFF the empty commit
(fail) evaluateCheckRunsPresence zero-path messages (mt#2312) > the conflict message names the invalidated-approval cost
(fail) evaluateCheckRunsPresence zero-path messages (mt#2312) > the inconclusive message lists BOTH causes and their opposite recoveries
(fail) evaluateCheckRunsPresence zero-path messages (mt#2312) > an unreadable merge state surfaces the fetch error in the inconclusive message
 141 pass / 8 fail

Negative control 2 — the behind correction is load-bearing:

Control 1 does not discriminate AT2/AT3/AT4: they assert webhook-miss, which that control returns unconditionally, so all three passed while proving nothing about the premise this task exists to correct. So a second control was run, implementing the spec's original premise — {dirty, behind} both treated as unmergeable. Exactly the three behind-specific tests went red, and nothing else.

(fail) classifyZeroCheckRuns (mt#2312) > AT2: a BEHIND but mergeable PR is a webhook miss, NOT unmergeable
(fail) classifyZeroCheckRuns (mt#2312) > mergeable_state alone never decides — only `mergeable` does
(fail) evaluateCheckRunsPresence zero-path messages (mt#2312) > a behind-but-mergeable PR still gets the webhook-miss recovery
 146 pass / 3 fail

Both controls were restored and the suite re-verified green.

What the controls do not buy, stated rather than assumed: they prove the tests can fail for the reverted behavior. The defect CLASS is "a denial message that asserts one cause for an ambiguous symptom." This PR covers the zero-check_runs member. The sibling members in the same file — the required-checks gate's "no matching run" denial (mt#1938) also prescribes the webhook-wake recovery unconditionally — are NOT covered here and remain as they were; that is a separate surface with its own tests, out of scope for this task.

Live verification

The unit tests all feed the classifier a hand-built MergeState, so nothing above exercises the actual gh api invocation or the --jq expression — the part most likely to be wrong in a way typechecking cannot see. Run against the live API from the session:

$ bun -e 'import { readMergeState, classifyZeroCheckRuns } from "./.minsky/hooks/require-review-before-merge.ts"; ...'
PR #3052: {"known":true,"mergeable":true,"mergeableState":"behind"} -> webhook-miss
PR #3051: {"known":true,"mergeable":true,"mergeableState":"behind"} -> webhook-miss

The fetch, the parse and the classification all work end-to-end against real PRs — and the result independently re-confirms the corrected premise: a live behind PR classifies as a webhook miss, which is the right answer, because both of those PRs carry a full check set.

AT6's conflicted-PR half is not live-exercised: producing it means deliberately conflicting a real PR against main, which is a destructive change to shared state that this task's scope does not authorize. The conflicted shape is covered by AT1's fixture, which is PR #3031's shape verbatim — an observed real occurrence recorded in mem#537 R2.

Scope note

.claude/hooks/** is regenerated from .minsky/hooks/** and recompiled in this PR (bun run src/cli.ts compile, verified by git status plus a grep for the new symbol in the generated output, not by the exit code).

edobry and others added 2 commits August 16, 2026 23:38
…hind`

The merge gate reported every zero-check_runs PR as the "GitHub Actions
webhook-miss class" and prescribed an empty-commit nudge. When the real cause
is a merge conflict that nudge cannot work — GitHub never built the merge ref,
so no pull_request workflow ever dispatched — and it re-heads the branch,
invalidating an existing APPROVE. Three recurrences (mem#537 R1/R2/R3) each
paid both costs.

The gate now reads the PR's own merge state on the zero path and picks one of
three messages: unmergeable (session_update + resolve, explicitly warning off
the nudge), webhook-miss (unchanged), or inconclusive (both causes listed).
The read is passed as a thunk so it is taken only on that path, which is
already a denial.

Two premises the spec asserted were false, and planning falsified both:

`behind` does NOT suppress CI dispatch. The spec, mem#321 and mem#537 all
grouped it with `dirty`. Measured: PR #3042 was 13 commits behind main at its
02:43:36Z push and GitHub dispatched all 20 checks; four other open `behind`
PRs each carry a full check set. Only `mergeable === false` stops the merge
ref forming, which is also what mt#4182 shipped for session_pr_checks. Both
memories are corrected.

The merge state is NOT "one cheap added field" on the call the gate already
makes. `gh pr list` does not compute mergeability and returns UNKNOWN for
every row; only a single-PR GET does, and that GET is what starts the job.
Folding it into PR_META_JSON_FIELDS would have compiled, shipped, and
classified every PR as inconclusive.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@minsky-reviewer

minsky-reviewer Bot commented Aug 17, 2026

Copy link
Copy Markdown

Minsky Reviewer Status

Verdict: APPROVED — no blocking findings
Review: View review
Model: openai/gpt-5 | Tokens: 625K prompt, 10K completion | Duration: 158s
Mode: normal

Commands

  • /review — request a fresh review

@minsky-ai minsky-ai Bot added the authorship/co-authored Co-authored by human and AI agent label Aug 17, 2026

@minsky-reviewer minsky-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2


Overall, this PR cleanly introduces a mergeability-based discriminator for zero check_runs and wires it into the presence floor via a thunk, with strong unit coverage and spec/docs updated. I verified the single-PR fetch (gh api repos/.../pulls/<n>) addition, the pure classifier, the bounded re-read on null, and the callsite changes. Non-blocking notes: (1) the synchronous 2s sleep may slightly degrade UX on the deny path — consider configurability; (2) evaluateCheckRunsPresence’s exported signature changed — ensure no external consumers are missed; (3) user-facing messages embed raw mergeable_state strings — consider quoting/normalizing. Otherwise, the change aligns with the task’s success criteria and appears internally coherent. Nice work.

Findings

  • [NON-BLOCKING] .minsky/hooks/require-review-before-merge.ts:145 — Synchronous 2s sleep on deny path may degrade UX; consider making delay configurable or asynchronous
    readMergeState uses Bun.sleepSync(MERGE_STATE_RECHECK_DELAY_MS) with MERGE_STATE_RECHECK_DELAY_MS = 2_000 (.minsky/hooks/require-review-before-merge.ts:145-164). While this only triggers on the zero-check_runs deny path, it blocks the hook process synchronously for 2 seconds on every inconclusive-first-read. Consider:
  • Making the delay configurable for test and ops tuning, or
  • Using an async retry (if/when this code ever runs in a larger process) or a shorter delay, given GitHub’s mergeability job is usually fast.
    Not blocking per spec (bounded, deny path only), but worth noting for responsiveness.
  • [NON-BLOCKING] .minsky/hooks/require-review-before-merge.ts:325 — Exported function signature changed; potential external consumers may now break or call without the new thunk
    evaluateCheckRunsPresence now takes a fourth parameter getMergeState: () => MergeState (.minsky/hooks/require-review-before-merge.ts:114-164, 325-363). Internal call sites in this file were updated, and tests were adapted. However, since evaluateCheckRunsPresence is exported and used in tests, any other module importing it (if any exist) would now fail to compile or, in looser call sites, call it without the thunk. [NEEDS VERIFICATION] I could not sweep the entire repo for external imports; if there are any, they will need to be updated to pass a thunk (use a mergeable stub for mt#1309-only tests). If there are no external consumers, ignore this.
  • [NON-BLOCKING] .minsky/hooks/require-review-before-merge.ts:338 — User-facing denial messages embed raw mergeable_state strings without normalization/guarding
    In the merge-conflict and webhook-miss branches, the reason conditionally appends , mergeable_state: ${state.mergeableState} when present (.minsky/hooks/require-review-before-merge.ts:338-370). Since mergeable_state is an undocumented, UI-facing string from GitHub that can vary (e.g., capitalization or new values), consider quoting it or prefixing with “as reported by GitHub” to reduce implied contract surface. Not blocking — diagnostic only — but helps avoid future brittleness if operators begin to key off these strings.

Spec verification

Criterion Status Evidence
When the gate finds zero check_runs on HEAD, it reads the PR's own merge state before emitting a diagnosis. The read must be a single-PR GET (gh api repos/<owner>/<repo>/pulls/<n> or gh pr view <n>). Met Implemented via fetchPullRequestMergeStateRaw calling gh api repos/${repo}/pulls/${prNumber} with --jq (see .minsky/hooks/pr-context.ts:734-771). Wired into the gate through readMergeState and invoked only on the zero path via the new thunk parameter to evaluateCheckRunsPresence (see .minsky/hooks/require-review-before-merge.ts:114-164 and 325-363, 1503-1510).
If the PR is unmergeable — REST mergeable: false / mergeable_state: dirty — the denial reason names the merge-conflict cause and prescribes session_update + conflict resolution, NOT the empty-commit-webhook-wake recovery. Met classifyZeroCheckRuns returns "merge-conflict" when mergeable === false (see .minsky/hooks/require-review-before-merge.ts:86-103, 131-139). evaluateCheckRunsPresence's merge-conflict branch prescribes session_update and explicitly warns "Do NOT push an empty commit" (see .minsky/hooks/require-review-before-merge.ts:343-356). Tests assert message content (see .minsky/hooks/require-review-before-merge.zero-check-runs.test.ts:113-146).
If the PR is mergeable (mergeable: true, any mergeable_state including behind, blocked, unstable, clean), the existing webhook-miss message is retained unchanged. blocked in particular must NOT be treated as unmergeable. Met classifyZeroCheckRuns maps any mergeable === true to "webhook-miss", ignoring mergeable_state (see .minsky/hooks/require-review-before-merge.ts:120-139). The webhook-miss message path remains the same text lineage (see .minsky/hooks/require-review-before-merge.ts:357-370). Tests cover behind, clean, and blocked mapping to webhook-miss and retaining the webhook-wake recovery (see .minsky/hooks/require-review-before-merge.zero-check-runs.test.ts:78-103 and 148-167).
mergeable is null until computed; the gate re-reads once after a short delay before classifying; if still unresolved, the message is inconclusive and lists both causes. Met readMergeState performs a single re-read after Bun.sleepSync(MERGE_STATE_RECHECK_DELAY_MS) when the first read returns mergeable: null (see .minsky/hooks/require-review-before-merge.ts:145-164). The inconclusive branch in evaluateCheckRunsPresence lists both causes and opposite recoveries (see .minsky/hooks/require-review-before-merge.ts:371-386). Tests assert inconclusive messaging content, including both recoveries and surfacing of fetch error (see .minsky/hooks/require-review-before-merge.zero-check-runs.test.ts:169-186).
Pure helper (classifyZeroCheckRuns(mergeState) or similar) is unit-tested for each branch. Met classifyZeroCheckRuns is a pure function exported from .minsky/hooks/require-review-before-merge.ts (lines 106-139) and covered by dedicated tests verifying all branches AT1–AT5 and tie-break behavior (see .minsky/hooks/require-review-before-merge.zero-check-runs.test.ts:60-110).

Adoption sweep

Symbol Kind Consumers found Classification Notes
fetchPullRequestMergeStateRaw function .minsky/hooks/require-review-before-merge.ts: imports and calls via readMergeState, /.claude/hooks/require-review-before-merge.ts: imports and calls via readMergeState (generated copy) Adopted New raw fetcher is wired into the zero-check_runs path only, per spec. No other consumers required.

Documentation impact

  • updated-in-pr — The gate’s behavior and rationale were updated in .minsky/hooks/SPEC.md §10 to describe the new cause-specific diagnosis, the single-PR mergeability read, and the decision on mergeable vs mergeable_state (see .minsky/hooks/SPEC.md:209-230). This matches the implementation in .minsky/hooks/require-review-before-merge.ts.
    Affected: .minsky/hooks/SPEC.md

@edobry
edobry merged commit f66803c into main Aug 17, 2026
13 checks passed
@edobry
edobry deleted the task/mt-2312 branch August 17, 2026 03:50

@minsky-reviewer minsky-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2


Prior round had no blocking findings. This iteration preserves the same implementation: a single-PR mergeability fetch, a pure classifyZeroCheckRuns discriminator, and thunk-wired invocation only on the zero-check_runs path, with generated .claude/ copies updated and tests split to stay under max-lines. I re-verified the success criteria against the current spec, and the behavior matches: conflict cases prescribe session_update, mergeable cases retain the webhook-miss recovery, and unresolved mergeability yields an inconclusive message. No new defects surfaced in the updated diff. Documentation in .minsky/hooks/SPEC.md was updated accordingly. Approving.

Spec verification

Criterion Status Evidence
When the gate finds zero check_runs on HEAD, it reads the PR's own merge state before emitting a diagnosis. The read must be a single-PR GET (gh api repos/<owner>/<repo>/pulls/<n> or gh pr view <n>). Met Implemented via fetchPullRequestMergeStateRaw calling gh api repos/${repo}/pulls/${prNumber} --jq '{mergeable: .mergeable, mergeable_state: .mergeable_state}' (see .minsky/hooks/pr-context.ts:734-771). Wired through readMergeState and invoked only on the zero path by passing a thunk into evaluateCheckRunsPresence (see .minsky/hooks/require-review-before-merge.ts:118-164 and :1505-1512).
If the PR is unmergeable — REST mergeable: false / mergeable_state: dirty — the denial reason names the merge-conflict cause and prescribes session_update + conflict resolution, NOT the empty-commit-webhook-wake recovery. Met classifyZeroCheckRuns returns "merge-conflict" when mergeable === false (.minsky/hooks/require-review-before-merge.ts:146-177). evaluateCheckRunsPresence’s merge-conflict branch prescribes session_update and warns "Do NOT push an empty commit" (.minsky/hooks/require-review-before-merge.ts:287-307). Tests assert this content (.minsky/hooks/require-review-before-merge.zero-check-runs.test.ts:109-133).
If the PR is mergeable (mergeable: true, any mergeable_state including behind, blocked, unstable, clean), the existing webhook-miss message is retained unchanged. blocked in particular must NOT be treated as unmergeable. Met classifyZeroCheckRuns maps any mergeable === true to "webhook-miss" and ignores mergeable_state (.minsky/hooks/require-review-before-merge.ts:169-177). The webhook-miss message path retains the prior text lineage with the same recovery instructions (.minsky/hooks/require-review-before-merge.ts:308-325). Tests cover behind, clean, and blocked mapping to webhook-miss (.minsky/hooks/require-review-before-merge.zero-check-runs.test.ts:61-90, 135-151).
mergeable is null until computed; the gate re-reads once after a short delay before classifying; if still unresolved, the message is inconclusive and lists both causes rather than asserting either. Met readMergeState performs a single re-read after Bun.sleepSync(MERGE_STATE_RECHECK_DELAY_MS) when the first read returns mergeable: null (.minsky/hooks/require-review-before-merge.ts:179-205). The inconclusive branch in evaluateCheckRunsPresence lists both recoveries and surfaces the fetch error when applicable (.minsky/hooks/require-review-before-merge.ts:326-344). Tests assert null handling and inconclusive messaging (.minsky/hooks/require-review-before-merge.zero-check-runs.test.ts:33-45, 153-186).
Pure helper (classifyZeroCheckRuns(mergeState) or similar) is unit-tested for each branch. Met classifyZeroCheckRuns is a pure exported helper (.minsky/hooks/require-review-before-merge.ts:146-177) with dedicated tests covering AT1–AT5 and tie-break behavior (.minsky/hooks/require-review-before-merge.zero-check-runs.test.ts:47-107).

Adoption sweep

Symbol Kind Consumers found Classification Notes
fetchPullRequestMergeStateRaw function .minsky/hooks/require-review-before-merge.ts:198 — readMergeState calls fetchPullRequestMergeStateRaw(...), /.claude/hooks/require-review-before-merge.ts:204 — generated copy readMergeState calls fetchPullRequestMergeStateRaw(...) Adopted New raw fetcher is wired into the zero-check_runs path only via readMergeState. No other consumers required per spec.

Documentation impact

  • updated-in-pr — The behavior change is documented in .minsky/hooks/SPEC.md §10, which now describes the cause-specific diagnosis and single-PR mergeability read (see .minsky/hooks/SPEC.md:209-230 in the diff). This matches the implementation in require-review-before-merge.ts. No other docs appear to describe this gate’s denial text or discriminator.
    Affected: .minsky/hooks/SPEC.md

@minsky-reviewer minsky-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2


Verification pass: the prior rounds had no blocking findings, and this iteration preserves the same mergeability-based discriminator and thunk-wired fetch, mirrored consistently in both .minsky/ and generated .claude/ trees. The single-PR mergeability read (fetchPullRequestMergeStateRaw) is correctly added and consumed only on the zero-check_runs path; classifyZeroCheckRuns remains pure and is comprehensively tested, including the corrected behind premise and the inconclusive path. Documentation in .minsky/hooks/SPEC.md is updated to match the behavior. I found no new critical defects introduced by this change. Approving.

Spec verification

Criterion Status Evidence
When the gate finds zero check_runs on HEAD, it reads the PR's own merge state before emitting a diagnosis. The read must be a single-PR GET (gh api repos/<owner>/<repo>/pulls/<n> or gh pr view <n>). Met Implemented via fetchPullRequestMergeStateRaw calling gh api repos/${repo}/pulls/${prNumber} --jq '{mergeable: .mergeable, mergeable_state: .mergeable_state}' in .minsky/hooks/pr-context.ts:734-771 and mirrored in .claude/hooks/pr-context.ts:737-774. Wired into the presence floor through readMergeState and passed as a thunk to evaluateCheckRunsPresence only on the zero path (.minsky/hooks/require-review-before-merge.ts:1505-1512, .claude/hooks/require-review-before-merge.ts:1508-1515).
If the PR is unmergeable — REST mergeable: false / mergeable_state: dirty — the denial reason names the merge-conflict cause and prescribes session_update + conflict resolution, NOT the empty-commit-webhook-wake recovery. Met classifyZeroCheckRuns returns "merge-conflict" when mergeable === false (.minsky/hooks/require-review-before-merge.ts:118-139). In that branch, evaluateCheckRunsPresence prescribes session_update and warns "Do NOT push an empty commit" (.minsky/hooks/require-review-before-merge.ts:286-306). Tests assert message content in .minsky/hooks/require-review-before-merge.zero-check-runs.test.ts:118-147.
If the PR is mergeable (mergeable: true, any mergeable_state including behind, blocked, unstable, clean), the existing webhook-miss message is retained unchanged. blocked in particular must NOT be treated as unmergeable. Met classifyZeroCheckRuns maps any mergeable === true to "webhook-miss", ignoring mergeable_state (.minsky/hooks/require-review-before-merge.ts:118-139). The webhook-miss message path retains the existing recovery text (.minsky/hooks/require-review-before-merge.ts:307-326). Tests cover behind, clean, and blocked mapping to webhook-miss and keeping the webhook-wake recovery (.minsky/hooks/require-review-before-merge.zero-check-runs.test.ts:149-167).
mergeable is null until computed; the gate re-reads once after a short delay before classifying; if still unresolved, the message is inconclusive and lists both causes rather than asserting either. Met readMergeState performs a single re-read after Bun.sleepSync(MERGE_STATE_RECHECK_DELAY_MS) when the first read returns mergeable: null (.minsky/hooks/require-review-before-merge.ts:141-164). The inconclusive branch in evaluateCheckRunsPresence lists both recoveries and surfaces fetch errors (.minsky/hooks/require-review-before-merge.ts:327-349). Tests assert inconclusive messaging content and error surfacing (.minsky/hooks/require-review-before-merge.zero-check-runs.test.ts:169-187).
Pure helper (classifyZeroCheckRuns(mergeState) or similar) is unit-tested for each branch. Met classifyZeroCheckRuns is a pure function exported in .minsky/hooks/require-review-before-merge.ts:118-139 and covered by dedicated tests verifying all branches AT1–AT5 and tie-break behavior (.minsky/hooks/require-review-before-merge.zero-check-runs.test.ts:60-116).

Adoption sweep

Symbol Kind Consumers found Classification Notes
fetchPullRequestMergeStateRaw function .minsky/hooks/require-review-before-merge.ts:141 — parsed via readMergeState calling fetchPullRequestMergeStateRaw, /.claude/hooks/require-review-before-merge.ts:145 — parsed via readMergeState calling fetchPullRequestMergeStateRaw Adopted New raw fetcher is wired into the zero-check_runs path only, per spec; no other consumers required.

Documentation impact

  • updated-in-pr — The change alters the gate’s denial messaging and behavior for zero check_runs and updates .minsky/hooks/SPEC.md §10 accordingly to document the cause-specific diagnosis and single-PR mergeability read. I reviewed the updated section (.minsky/hooks/SPEC.md:209-230) and it matches the implementation in .minsky/hooks/require-review-before-merge.ts. No other docs assert contradictory behavior.
    Affected: .minsky/hooks/SPEC.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

authorship/co-authored Co-authored by human and AI agent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant