fix(mt#4271): Publish a skipped check-run when the reviewer declines to run

[minsky-ai]

Summary

runReview returns { status: "skipped", reason: "concurrent_inflight" } from review-worker.ts:508 — before review-finalize.ts, the only path that publishes on the reviewed and errored outcomes. So a refusal set no check-run conclusion at all, and session_pr_wait-for-review reported reviewerCheckRunState: { status: "absent" }.

That is byte-identical to genuine reviewer silence, which is a documented bypass-merge condition. An agent walking the documented ladder honestly arrives at "reviewer absent >5 minutes" on a PR the bot REFUSED rather than missed — mem#1093, PR #3107: three consecutive waits over ~35 minutes, every one reading absent, on a healthy service. Nothing bad happened only because both bypass paths' preconditions incidentally held.

This is the last leg of ADR-030 follow-up 1. review-finalize already published on the errored outcome; mt#4881 took the thrown-review leg this morning; this takes the skip.

Key changes

• publishTerminalCheckRunSafe (server.ts) — one function for both terminal outcomes that never reach review-finalize.ts: a review that THREW (mt#4881) and one that was DECLINED (this task). One function because it was one defect: neither set a conclusion, so both read as absent. Fail-open — a check-run write must never affect the review path, and fork PRs legitimately lack the permission.
• buildCheckRunPayload gains skipReason → conclusion skipped. The branch sits above the blockingCount derivation deliberately: a declined review carries no findings, so falling through would emit a green success asserting the code was reviewed when nothing looked at it. The round-1 negative control below confirms that is exactly what happens without it.
• The summary rules the bypass out in words rather than describing the state, because its reader is partway down a ladder whose next documented step is a bypass merge. The reason-specific remedy is asserted only for the reason it is true of (see Round 2).
• /implement-task §9's zero-review branch gains a step that reads the check-run conclusion before retriggering. The old ordering wasted a round: skipped means declined, failure means ran-and-failed (mt#4881), and only absent is consistent with silence — and even then, confirm through mt#4118's delivery record, whose verdict.isSilence is the authoritative discriminator.

Nothing below the reviewer changes. packages/domain/src/repository/github-checks-run.ts:84 already accepts skipped, and mt#1307 (DONE) taught pr-watch the full conclusion enum — so the one downstream consumer of a new conclusion value already understands it.

Planning audit, the corrected line references, and the gate walk: mt#4271.

Why skipped, with the vendor citation

minsky-reviewer/findings is a required check (resolution-note-guard.ts:19, :232), so the conclusion must not block the PR. GitHub, About protected branches, verbatim:

Required status checks must have a successful, skipped, or neutral status before collaborators can make changes to a protected branch.

Both skipped and neutral satisfy a required check — they are symmetric for this purpose, contrary to the spec's original framing that they "are not symmetric and [this] has changed over time." skipped is chosen on semantics: the state being reported is a skip. failure is ruled out because it would turn a transient refusal into a merge blocker, which is worse than the silence this ends. The REST reference (docs.github.com/en/rest/checks/runs) enumerates the values but says nothing about branch-protection interaction, so it is not the page that answers this.

Round 2 — reviewer findings addressed

Both blocking findings from review 5086970933 were correct.

R1-1 (blocking) — no test proved the server's skip branch actually invokes the publisher. This is the caller-direction check /implement-task §7 item 8 names, and I had skipped it: the payload builder was unit-tested and the wiring was not — precisely the shape that ships a publisher nobody calls while every payload test stays green, reproducing the absent state this change exists to end.

The seam has to sit above createOctokit, not below: the real path mints an App installation token no test holds, so a seam placed after that call is unreachable and would verify nothing. createApp gains an optional terminalCheckRunPublisher, undefined in production.

Class, not instance: mt#4881's publishFailureCheckRunSafe had the identical untested-wiring gap two functions up, so both now route through one publishTerminalCheckRunSafe rather than only mine gaining a seam. That is a behavior-preserving refactor of code merged this morning; its own suite (failure-alert.test.ts) passes unchanged.

R1-2 (blocking) — the skip summary asserted concurrent_inflight's remedy for every reason. "Clears on a new head; a retrigger is refused" is true of one reason and nothing else yet; stating it for a future one — a permission, a rate limit — would send an operator confidently the wrong way, which is the same shape of error as the absent this whole change fixes, one layer up. The remedy is now conditional; the reason-independent half stays unconditional.

R1-3 (non-blocking) — brittle phrasing assertions. Narrowed from four exact phrases to the two carrying the contract, and replaced the rest with a test of the actual invariant: the remedy appears for concurrent_inflight and not for another reason.

Testing

Typecheck: 0 errors across 8 projects, validated against the session workspace. Lint: 0 errors, 0 warnings over 4,316 files.

Execution evidence:

$ bun --cwd services/reviewer test --preload ../../tests/setup.ts \
      src/check-run-publisher.test.ts src/server.test.ts src/failure-alert.test.ts
 103 pass
 0 fail
 216 expect() calls
Ran 103 tests across 3 files. [495.00ms]

# the ten cases this task adds, all passing:
(pass) publishCheckRun: octokitOverride seam > skip path publishes with conclusion 'skipped'
(pass) buildCheckRunPayload: declined-to-run path > conclusion is 'skipped' — not 'failure', which would block a required check
(pass) buildCheckRunPayload: declined-to-run path > conclusion is NOT 'success' — a declined review must not read as reviewed
(pass) buildCheckRunPayload: declined-to-run path > the summary names the reason and marks the state as not-silence
(pass) buildCheckRunPayload: declined-to-run path > the concurrent_inflight remedy is asserted ONLY for that reason
(pass) buildCheckRunPayload: declined-to-run path > no round parenthetical when the round is unknown
(pass) buildCheckRunPayload: declined-to-run path > a real failure outranks a skip when both are somehow set
(pass) buildCheckRunPayload: declined-to-run path > negative control: the same params WITHOUT skipReason still derive normally
(pass) terminal check-run publication wiring > a DECLINED review publishes a skip check-run carrying the reason
(pass) terminal check-run publication wiring > a REVIEWED review publishes no terminal check-run — finalize owns that path

$ bun scripts/run-related-tests.ts services/reviewer/src/server.ts \
      services/reviewer/src/check-run-publisher.ts .minsky/skills/implement-task/skill.ts
 65 pass
 0 fail
5 related test file(s) passed: .minsky/hooks/deploy-surface-detector.test.ts,
  .minsky/hooks/require-deploy-verification-before-merge.test.ts,
  services/reviewer/src/check-run-publisher.test.ts,
  services/reviewer/src/server.test.ts,
  tests/domain/implement-task-expected-head-sha.test.ts

Acceptance tests, by the spec's own numbering.

• AT4 — RUN and passing. "A review that proceeds normally still publishes its findings check-run exactly as before, with the same conclusion it produces today." Covered by the pre-existing suite (success / neutral / failure derivations, annotation mapping, convergence summaries) plus two explicit cases: negative control: the same params WITHOUT skipReason still derive normally → conclusion: "success", and a REVIEWED review publishes no terminal check-run.
• AT1, AT2, AT3 — UNVERIFIED pre-merge, and not for want of trying. All three require a check-run actually published by the DEPLOYED reviewer against a contended in-flight marker. Two probes, both negative: publishing one needs the reviewer App's installation token, which the service mints internally via createOctokit(cfg) and the agent does not hold; and forge_branch_protection_get on main returns Resource not accessible by integration — this integration cannot read the required-checks list at all. Deferred to the §10 post-deploy live exercise, not treated as done. [at1-deferred: mt#4271] [at2-deferred: mt#4271] [at3-deferred: mt#4271]

Success criteria. SC1 (vendor citation recorded in ## Context, choice justified against the must-not-block requirement) — done, quoted above and in the spec. SC2 (the skip publishes a check-run naming the reason) — implemented, unit-covered, and as of round 2 its wiring is covered too; its live half rides with AT1. SC4 (a live marker still produces this state, an expired one no longer can) — composes with mt#4267, DONE; nothing here touches the acquire path. SC5 (§9's ladder names this state as distinct from silence, against PR #3114's merged text) — done, written against the current merged §9.

SC3 is the one I cannot fully satisfy, and it says so explicitly: "The published conclusion does not fail the required minsky-reviewer/findings check — demonstrated against branch protection, not asserted." Branch protection is unreadable from here (probe above). The strongest evidence available, and it is live rather than asserted: PR #3504 merged at 2026-08-31T02:12:38Z with its head a6633cb92dcd6d04b86c2bde55bef3ef399297ed carrying minsky-reviewer/findings at conclusion: neutral, and forge_check_runs_list on that sha reports 13/13 passed, 0 failed. A non-success conclusion from the same documented sentence already does not block a merge in this repo. That is a class argument, not the direct skipped observation SC3 asks for — hence [sc3-deferred: mt#4271], discharged at §10.

Negative control — round 1, the conclusion branch: reverted the whole skipReason branch from buildCheckRunPayload's derivation, back to failureSummary || blockingCount > 0 ? "failure" : deriveConclusion(levels). Three tests went red, and the received value is the dangerous one rather than merely absent — Expected: "skipped" Received: "success", i.e. a green check-run claiming a review that never ran. Restored.

Negative control — round 2, the wiring: removed the publishSkipCheckRunSafe call from the skip branch in server.ts — the whole of R1-1's fix at the call site — and re-ran. The new wiring test failed with exactly the gap the reviewer named:

Expected length: 1
Received length: 0
(fail) terminal check-run publication wiring (mt#4271) > a DECLINED review publishes a skip check-run carrying the reason
 26 pass
 1 fail

Restored; 103/103. Note the sibling control stayed green through this, which is what makes the pair meaningful rather than one assertion twice.

Live verification

UNVERIFIED — the live exercise is deferred to §10 post-deploy, because the code that publishes the check-run only runs inside the deployed reviewer service and requires a contended in-flight marker to reach. This integration is NOT confirmed working until that §10 exercise runs and succeeds.

Probes run rather than assumed: the reviewer App's installation token is minted inside the service (createOctokit(cfg)) and is not reachable from the agent; forge_branch_protection_get main → Resource not accessible by integration. Neither AT1's publish nor SC3's branch-protection read is available pre-merge.

What §10 will do: after the reviewer deploy lands, force a concurrent_inflight skip (two triggers against one head while the marker is held) and assert (a) a minsky-reviewer/findings check-run exists on that sha with conclusion: skipped, (b) its output names concurrent_inflight, (c) session_pr_wait-for-review reports a reviewerCheckRunState that is not absent, and (d) the PR's merge state is not blocked by it.

Deploy verification

isDeploySurfaceFile run over this PR's actual changed files: true for services/reviewer/src/check-run-publisher{,.test}.ts and services/reviewer/src/server{,.test}.ts; false for the two skill files. This is a reviewer-service deploy-surface PR and does not claim [no-deploy-impact].

Post-merge I will run deployment_wait-for-latest for reviewer with notBefore = the merge timestamp and expectCommitSha = the merge SHA, read buildIdentity, and — since the reviewer is an image-source service where that comes back indeterminate — correlate deploy-reviewer.yml's workflow run against the merge SHA and assert the health body's service identity. Then the §10 live exercise above.

Coordination

PR #3412 (mt#4639) is open and touches services/reviewer/src/server.ts, which this PR also touches; its 313-file list was read via get_files and it does not touch check-run-publisher.ts or review-worker.ts. Its changes are mechanical err.message → getLoggableErrorSummary rewrites at log sites and do not occupy the skip branch — a rebase risk, not a correctness one. PR #774 (mt#1263) touches only review-worker.test.ts and eslint.config.js; its new runReview end-to-end tests all take the normal review path with the marker uncontended, so the skip return is untouched by them.

🤖 Generated with Claude Code

https://claude.ai/code/session_01U6kmDQENKCHPspxL5CZ5ks

[minsky-reviewer]

Minsky Reviewer Status

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

Commands

• /review — request a fresh review

[minsky-reviewer]

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

---

Solid targeted change that adds a skipped check-run for declined reviews and updates the implement-task ladder accordingly. The payload builder and publisher are well-covered by unit tests, but two issues block merge: (1) there’s no integration test that proves the server’s skip branch actually invokes the new publisher — the critical wiring remains unverified; and (2) the skip-summary hardcodes concurrent_inflight-specific advice, which will be wrong if any other skip reason is introduced. I also flagged a non-blocking brittleness in tests that over-assert specific summary phrasing. SC3 (live branch-protection behavior) is deferred per the PR body and marked Not Met in spec verification; either provide post-deploy evidence or amend the spec. If you address the wiring test and condition the summary on the actual reason, this is good to go.

Findings

• [BLOCKING] services/reviewer/src/server.ts:590 — No automated test coverage that skipped path publishes a check-run (server wiring unverified)
  The behavioral change hinges on publishSkipCheckRunSafe() being invoked when runReview returns { status: "skipped" } (see services/reviewer/src/server.ts:590-599). While there are unit tests for buildCheckRunPayload and for publishCheckRun with skipReason, there is no test exercising the server’s skip branch to prove the new publisher is actually called. Given prior regressions around terminal-outcome publishing, a wiring-gap here would silently revert to reviewerCheckRunState: { status: "absent" } despite the new payload code existing.

Please add a server.test.ts (or extend the existing one) case that forces the skip path and asserts that a check-run publish is attempted. Suggested approaches:

• Inject a seam or spy for publishCheckRun or for the Octokit used by publishSkipCheckRunSafe (mirroring the existing octokitOverride seam used in check-run-publisher.test.ts).
• Alternatively, expose/inject a publisher callback into createApp that can be observed in tests.

Absent this, the critical behavior change remains unverified at the integration point.

• [BLOCKING] services/reviewer/src/check-run-publisher.ts:201 — Summary text hardcodes "concurrent_inflight" even when skipReason could be other values
  In buildCheckRunPayload's skip branch (services/reviewer/src/check-run-publisher.ts:201-210), the summary concatenates:

• A dynamic prefix naming skipReason, AND

• A hard-coded sentence: "A concurrent_inflight refusal clears on a new head; a retrigger is refused while the marker is held."

This bakes concurrent_inflight semantics into the message regardless of the actual skipReason. If a future skip reason is introduced (e.g., permissions, rate limiting, etc.), the summary will assert the wrong remedy and mislead operators.

Requested fix:

• Either tailor the remedy sentence conditionally when skipReason === "concurrent_inflight", or
• Remove the concurrent_inflight-specific advice from the generic skip path and keep the summary semantically accurate for all reasons.

Tests also assume this exact wording (see check-run-publisher.test.ts "the summary rules out the bypass explicitly"), so please update/extend tests accordingly.

• [NON-BLOCKING] services/reviewer/src/check-run-publisher.test.ts:470 — Tests assert highly specific summary phrasing — brittle against harmless copy edits
  The test "the summary rules out the bypass explicitly, and names what clears it" (services/reviewer/src/check-run-publisher.test.ts:470-492) asserts multiple exact substrings such as "NOT reviewer silence", "not a bypass condition", and "new head". These enforce editorial wording rather than contract. Minor copy edits (tone, punctuation, or reflow) would break tests without changing semantics.

Suggestion (non-blocking): narrow the assertions to the essential signals the system consumes (e.g., includes skipReason, sets title to include skipped, and conclusion is skipped). Consider exposing a structured field for the remedy if that must be asserted, rather than anchoring on free-text.

Spec verification

Criterion	Status	Evidence
The GitHub Checks API conclusion question above is answered with a vendor-doc citation recorded in ## Context, and the chosen value is justified against the requirement that a concurrent_inflight skip must NOT block the PR.	Met	PR description’s Context section quotes GitHub’s protected-branches doc verbatim and records the rationale for choosing skipped over neutral (see PR body “## Context — Third-party citation”). The code comments in services/reviewer/src/check-run-publisher.ts:120-147 also embed the citation and justification.
A concurrent_inflight skip publishes a check-run naming the reason, so session_pr_wait-for-review's reviewerCheckRunState stops reporting absent for that state.	Met	Implementation adds skipReason plumbing and sets conclusion "skipped" with summary/title naming the reason in services/reviewer/src/check-run-publisher.ts:179-223. The server wires the skip branch to publish via publishSkipCheckRunSafe in services/reviewer/src/server.ts:277-346 and invokes it on result.status === "skipped" at :590-599.
The published conclusion does not fail the required minsky-reviewer/findings check — demonstrated against branch protection, not asserted.	Not Met	Pre-merge, no live branch-protection verification is included. The PR body explicitly defers this to a post-deploy exercise (§10). While unit tests cover payload shape, the criterion requires a live demonstration against branch protection. Please provide post-deploy evidence or amend the spec to defer SC3.
A live in-flight marker still produces this state; an EXPIRED one no longer can, because mt#4267 fixed the acquire path. The two tasks compose rather than overlap.	Unverifiable	This criterion depends on runtime behavior across the marker-acquire path from mt#4267. The current diff does not include live verification or tests that simulate expiry vs. live markers; no repo artifact demonstrates it. Without a runnable environment, this cannot be verified from the diff alone.
/implement-task §9's ladder names this state as distinct from reviewer silence, written against PR #3114's merged text.	Met	Both .minsky/skills/implement-task/skill.ts:624-638 and the generated .claude/skills/implement-task/SKILL.md:627-644 add a step 2 that inspects the check-run conclusion first and distinguishes skipped/failure from absent, naming the concurrent_inflight remedy and deferring retrigger appropriately.

Documentation impact

• no-update-needed — This PR changes internal reviewer-service behavior and the implement-task skill text. The docs are generated from .minsky/skills/implement-task/skill.ts, and the PR updates both the source and generated SKILL.md accordingly. No separate public documentation files appear to require updates.

[minsky-reviewer]

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

---

Re-verification against the current spec and diff: both prior BLOCKING findings are addressed. The skip path now explicitly publishes a check run with conclusion 'skipped' and names the reason; the server wiring calls the publisher on the declined branch and is covered by a dedicated test. The payload builder gained skipReason, produces 'skipped' (not 'success'/'failure'), and its summary clearly distinguishes this state from silence; tests assert the ordering and content. The implementation remains fail-open and uses a safe test seam; no regressions surfaced. I find no new critical defects introduced by the fixes. Spec criteria that require live post-deploy verification are correctly deferred; unit/integration coverage for in-repo behavior is present. Verdict: APPROVE.

Spec verification

Criterion	Status	Evidence
The GitHub Checks API conclusion question above is answered with a vendor-doc citation recorded in ## Context, and the chosen value is justified against the requirement that a concurrent_inflight skip must NOT block the PR.	Met	Task spec Context section quotes GitHub 'About protected branches' verbatim naming 'skipped' and 'neutral' as acceptable for required checks; code comments in services/reviewer/src/check-run-publisher.ts:109-133 reiterate the same citation and rationale ('minsky-reviewer/findings' is required; 'failure' would block).
A concurrent_inflight skip publishes a check-run naming the reason, so session_pr_wait-for-review's reviewerCheckRunState stops reporting absent for that state.	Met	services/reviewer/src/server.ts:602-616 calls publishSkipCheckRunSafe(...) in the skip branch; services/reviewer/src/check-run-publisher.ts:93-116 adds skipReason and conclusion 'skipped'; summary includes the reason and states 'NOT reviewer silence' (lines ~170-214). Tests cover both the wiring and payload: services/reviewer/src/server.test.ts:370-430 and services/reviewer/src/check-run-publisher.test.ts:445-572.
The published conclusion does not fail the required minsky-reviewer/findings check — demonstrated against branch protection, not asserted.	Unverifiable	This criterion requires post-deploy live verification against GitHub branch protection. The diff contains unit/integration tests only; no in-repo artifact can prove live branch-protection behavior. The PR body records a deferral to post-deploy (§10).
A live in-flight marker still produces this state; an EXPIRED one no longer can, because mt#4267 fixed the acquire path. The two tasks compose rather than overlap.	Unverifiable	This depends on live behavior of the reviewer’s marker acquisition and expiry (mt#4267). The diff adds skip check-run publishing only; no in-repo test demonstrates acquisition vs expiry composition. No live evidence is included.
/implement-task §9's ladder names this state as distinct from reviewer silence, written against PR #3114's merged text.	Unverifiable	The current diff portion shows only services/reviewer/* changes since the last review. Any skill text/rung edits to .minsky/skills/implement-task/skill.ts are not present in this incremental diff segment; reviewer cannot verify from the shown files alone.

Adoption sweep

Symbol	Kind	Consumers found	Classification	Notes
services/reviewer/src/server.ts: TerminalCheckRunPublication	type	services/reviewer/src/server.test.ts: import type { TerminalCheckRunPublication }, services/reviewer/src/server.ts: internal use in publishTerminalCheckRunSafe signature and terminalCheckRunPublisher seam	Adopted	New exported type used by tests via createApp seam; no external consumers required by spec.

Documentation impact

• no-update-needed — Behavioral change is internal to the reviewer service’s check-run publishing: a skipped review now emits a GitHub check run with conclusion 'skipped'. No public CLI, API, or user-facing docs are changed or invalidated. ADR-030 already designates the Checks API as the status/liveness channel; this PR aligns with it. I checked .minsky/skills/implement-task/skill.ts presence in the repo but this incremental diff segment did not include its edits; regardless, no existing docs are made false by adding a non-blocking 'skipped' conclusion on declined runs.

[minsky-reviewer]

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

---

Re-verified the fixes against the current spec and diff. The prior BLOCKING issues are addressed: the server’s skipped-review branch now publishes a terminal check-run via a safe, testable seam; buildCheckRunPayload accepts skipReason, yields conclusion: "skipped" (not success/failure), and crafts a summary that distinguishes this state from reviewer silence with reason-specific guidance only when appropriate. Tests cover both the payload and the server wiring. The refactor to a unified publishTerminalCheckRunSafe preserves failure behavior and remains fail-open. Skill guidance was updated to read the check-run conclusion before retriggering. I found no new critical defects introduced by these changes. Verdict: APPROVE.

Spec verification

Criterion	Status	Evidence
The GitHub Checks API conclusion question above is answered with a vendor-doc citation recorded in ## Context, and the chosen value is justified against the requirement that a concurrent_inflight skip must NOT block the PR.	Met	services/reviewer/src/check-run-publisher.ts:96-117 — inline docstring cites GitHub protected-branches doc and justifies choosing "skipped"; PR/task spec Context also records the citation and reasoning.
A concurrent_inflight skip publishes a check-run naming the reason, so session_pr_wait-for-review's reviewerCheckRunState stops reporting absent for that state.	Met	services/reviewer/src/server.ts:613-621 — skip branch now calls publishSkipCheckRunSafe; services/reviewer/src/check-run-publisher.ts:270-298 plumbs skipReason to payload builder; tests at services/reviewer/src/check-run-publisher.test.ts:445-470 and services/reviewer/src/server.test.ts:393-442 assert publication and content.
The published conclusion does not fail the required minsky-reviewer/findings check — demonstrated against branch protection, not asserted.	Unverifiable	Live branch-protection behavior cannot be verified from repo code; the task spec explicitly defers this to post-deploy (§10). No in-repo artifact can demonstrate protected-branch acceptance pre-merge.
A live in-flight marker still produces this state; an EXPIRED one no longer can, because mt#4267 fixed the acquire path. The two tasks compose rather than overlap.	Unverifiable	This requires live interaction with the marker acquisition/expiry path; the current diff only adds reporting. No in-repo test exercises live TTL behavior; verification depends on mt#4267 behavior outside this PR.
/implement-task §9's ladder names this state as distinct from reviewer silence, written against PR #3114's merged text.	Met	.minsky/skills/implement-task/skill.ts:627-640 (and generated .claude/skills/implement-task/SKILL.md:627-642) — step 2 now reads the check-run conclusion before retriggering, distinguishing skipped/failure/absent and naming the remedy for concurrent_inflight.

Adoption sweep

Symbol	Kind	Consumers found	Classification	Notes
services/reviewer/src/server.ts: TerminalCheckRunPublication	type	services/reviewer/src/server.test.ts:18 — imported for testing published events array shape, services/reviewer/src/server.ts:245 — used as parameter type for publishTerminalCheckRunSafe	Adopted	Exported primarily to support test seam verification for terminal check-run publication (mt#4271).

Documentation impact

• updated-in-pr — User-facing skill guidance was updated to distinguish skipped/failure/absent states and to read the check-run conclusion before retriggering. See .minsky/skills/implement-task/skill.ts:624-640 and the generated .claude/skills/implement-task/SKILL.md:627-642 modified in this PR.
  Affected: .minsky/skills/implement-task/skill.ts, .claude/skills/implement-task/SKILL.md

[minsky-reviewer]

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

---

Re-verified the fix commit against the current spec: the previously blocking gaps are closed. The skip path now publishes a terminal check-run with conclusion "skipped" and reasoned summary via a unified, fail-open publisher; server wiring is exercised by a dedicated test seam; payload builder handles skipReason with correct ordering to avoid false "success"/"failure". Skill guidance was updated to read the check-run conclusion before retriggering. I found no new critical defects introduced by these changes. APPROVE.

Spec verification

Criterion	Status	Evidence
The GitHub Checks API conclusion question above is answered with a vendor-doc citation recorded in ## Context, and the chosen value is justified against the requirement that a concurrent_inflight skip must NOT block the PR.	Met	Implementation selects conclusion: "skipped" (services/reviewer/src/check-run-publisher.ts:179-189, 201-229), with inline vendor-quote rationale in the new skipReason docblock (services/reviewer/src/check-run-publisher.ts:93-121) and summary comments. The PR body/spec records the citation to GitHub’s protected-branches doc.
A concurrent_inflight skip publishes a check-run naming the reason, so session_pr_wait-for-review's reviewerCheckRunState stops reporting absent for that state.	Met	The skip branch now calls publishSkipCheckRunSafe(...) (services/reviewer/src/server.ts:613-622) which routes to publishTerminalCheckRunSafe (services/reviewer/src/server.ts:245-299, 670-691). The payload builder sets conclusion: "skipped" and includes skipReason in title/summary (services/reviewer/src/check-run-publisher.ts:201-229). Covered by tests: publish path (services/reviewer/src/server.test.ts:382-423) and payload content (services/reviewer/src/check-run-publisher.test.ts:445-569).
The published conclusion does not fail the required minsky-reviewer/findings check — demonstrated against branch protection, not asserted.	Unverifiable	Live protected-branch behavior cannot be validated from repo code; no in-repo branch-protection config is present. The PR description documents pre-merge probe limitations and defers live verification (AT2). No repo artifact can confirm this pre-merge.
A live in-flight marker still produces this state; an EXPIRED one no longer can, because mt#4267 fixed the acquire path. The two tasks compose rather than overlap.	Unverifiable	This criterion depends on live runtime behavior with a held/expired marker; the diff introduces no change to marker acquisition/expiry. Verification requires a deployed reviewer and controlled contention, which is outside the repo. Pre-merge validation is not possible here.
/implement-task §9's ladder names this state as distinct from reviewer silence, written against PR #3114's merged text (see the collision note below), not against the version that preceded it.	Met	Skill guidance updated to read the check-run conclusion before retriggering and distinguishes skipped/failure/absent with remedies (/.minsky/skills/implement-task/skill.ts:624-642). The generated SKILL.md reflects the same (/.claude/skills/implement-task/SKILL.md:627-645).

Adoption sweep

Symbol	Kind	Consumers found	Classification	Notes
services/reviewer/src/server.ts — TerminalCheckRunPublisher	type	services/reviewer/src/server.test.ts: imports type TerminalCheckRunPublication and provides a publisher callback to createApp(), services/reviewer/src/server.ts: used as optional DI param to createApp()	Adopted	New exported test seam type for terminal check-run publication; used by tests via createApp() injection. No external wiring required by spec.

Documentation impact

• no-update-needed — Behavioral change is limited to the reviewer service’s internal check-run publication and the agent skill guidance. The skill’s generated docs (.claude/skills/implement-task/SKILL.md) and source (.minsky/skills/implement-task/skill.ts) were updated in this PR to instruct reading the check-run conclusion before retriggering and to distinguish skipped/failure/absent states. No other public-facing docs appear to describe the reviewer’s skipped state semantics; ADR-030 remains accurate. Therefore, documentation updates ship within this PR and no further docs need updating.