test(mt#4895): Cover the concurrent_inflight skip at L2 and L4 via injected seams

[minsky-ai]

Summary

mt#4271 shipped the skipped check-run for a review the reviewer DECLINES, and merged with three
acceptance tests and one success criterion unverified — forcing a skip looked like it needed two
colliding agents on a live PR.

mt#4895 reframed that correctly: the contention is a Postgres row, not an interaction between
agents, so the layers separate. This PR closes the two that are reachable, and records a
corrected premise for the one that is not.

Where the five layers stand after this PR: L1 already covered (twice), L2 covered here, L3
covered by PR #3563, L4 harness shipped here (live integration run deferred to mt#4897), L5
recorded as unverified with its reason.

Key changes

• RunReviewDeps gains three seams — octokitFactory, prContextFetcher, appIdentityFetcher
  — so the marker branch can be driven with no network. Optional fields with real defaults; every
  existing caller is unaffected.
• src/runreview-concurrent-inflight.test.ts (new) — 4 cases covering the skip return, the log
  shape, the skip-path timing write, and SC1's negative control.
• scripts/inflight-skip-harness.ts (new, round 2) — the L4 harness's decision core: stdout
  collection with a completion promise, event extraction, and the pass/fail derivation. Pure
  functions over values, covered by 14 tests.
• scripts/smoke-concurrent-inflight-skip.ts (new) — the imperative shell around it: spawn,
  post, poll, clean up. Modelled on kill-test.ts, with a --negative-control mode.

Injected, not module-patched — and this was a decision, not a default

mt#4895's spec left the seam choice open, conditional on the still-open PR #774 landing its
mock.module approach. ADR-036 §2 rule 2 settles it: where a seam can be added by changing one
production file with no exported-type change ("an optional deps parameter with a real default
counts as no change"), patching is banned at that site. That is exactly this site. PR #3563 — the
mt#4271 PR merged this morning — used the same shape for terminalCheckRunPublisher, with the
reasoning this task needs verbatim: "The seam has to sit above createOctokit, not below."

Consequence: this PR does not depend on PR #774, and does not touch review-worker.test.ts,
which is the only file #774 shares. Recorded on mt#1263 as material to the merge decision it is
blocked on: #774 is mergeable_state: dirty and its ESLint carve-out is the mechanism ADR-036 now
prohibits.

A spec premise this PR corrects

mt#4895's ## The precedent for L4 claimed the skip log is emitted "before any GitHub interaction",
so an L4 script needs "no real PR, no model call, and no reviewer App token." One of those three
survives. The marker is keyed on pr.headSha, so fetchPullRequestContext (github-client.ts:201,
a real pulls.get) necessarily precedes acquireMarker. createOctokit is not the blocker — it
is a pure constructor that validates nothing — but the PR fetch is: against a synthetic PR it
throws, and runReview dies before the marker, so no contention is possible and the log can never
fire. Only "no model call" holds. This is the same error SC1 had already corrected one section
earlier; the L4 section was never updated to match. Both the spec and the script header now say so.

The look-alike the tests discriminate

runReview has TWO returns carrying status: "skipped", and the routing one comes first —
decideRouting short-circuits a Tier-1 PR before the marker exists. Asserting status === "skipped"
alone would pass for the wrong reason, so every assertion pins reason, and the fixture PR body
carries the tier-3 marker so routing resolves to shouldReview: true.

Round 2 — reviewer findings addressed

All three R1 findings were correct as stated. Two were real defects in the new script; the third is
answered below with what changed rather than with an argument alone.

R1-1 (blocking) — racy stdout collection. Correct, and it would have made SC2/SC3 intermittently
wrong in the worst way: a real skip reported as a failure. collectStdoutLines returned only the
array, with no completion signal, so the script killed the process and parsed lines while the
background reader was still draining — and a terminal event is exactly what arrives last. It now
returns { lines, done }, and the shell awaits done after the process exits and before
anything reads a line.

R1-2 (blocking) — process.exit inside the try bypassed cleanup. Correct. Every failure path
inside main's try now throws (bail) and a top-level catch sets the exit code after the finally
has run, so the spawned server is terminated and the Postgres handle closed. Class, not instance:
the reviewer cited :320, but the same shape was on every in-try failure path — all of them
converted, not just the one named. The env-gate skip() calls deliberately keep process.exit, and
the code now says why: they run before anything is spawned or connected, so there is no cleanup to
bypass.

Both fixes are provable rather than asserted. The decision logic moved into
scripts/inflight-skip-harness.ts — ADR-036 §3's functional core / imperative shell, applied to a
script — so the harness's own logic is covered by the suite instead of resting on a live run that has
not happened. One test reproduces R1-1 directly by asserting the line buffer is incomplete before
done and complete after; without the fix there is nothing to await and the assertion cannot be
written at all.

R1-3 (blocking) — missing live-run evidence. The finding is factually right: there is no live
run, and I am not claiming otherwise or asking for it to be waived as a false positive. What changed
is how much it covers.

• The credentials probe stands (all seven absent, listed below), which is §7a's documented override
  branch (b), "the author lacks live-target access."
• Independently of credentials, delivery A starts a real review of a real PR and posts to it.
  That is a shared-state change on a PR this task does not own — the authorization mt#4271 surfaced
  rather than took, and it is not mine to grant.
• What was unproven is now much smaller. R1 was right that shipping a harness whose primary path
  had never executed is weak. Round 2 covers its decision logic with 14 tests, and the failure path
  was exercised live end to end (below). What remains unproven is the integration — real GitHub,
  real Postgres, real contention — which is precisely what mt#4897 owns.

I have not merged past this. If the reviewer still considers the live run a merge precondition, that
is a legitimate call and mt#4897 is the gate; say so and the PR waits.

Execution evidence:

$ bun test --preload ../../tests/setup.ts src/runreview-concurrent-inflight.test.ts
(pass) a HELD marker makes runReview return the concurrent_inflight skip [8.13ms]
(pass) the skip records a skip-path timing row (mt#2088) rather than skipping the write [0.27ms]
(pass) the skip log carries the delivery id, which is what correlates it to a delivery [0.21ms]
(pass) negative control: an AVAILABLE marker does not return the skip — execution passes the gate [1.25ms]
 4 pass / 0 fail / 13 expect() calls

$ bun test --preload ../../tests/setup.ts scripts/inflight-skip-harness.test.ts     # round 2
(pass) collectStdoutLines > R1-1 regression: lines are INCOMPLETE before `done` resolves and COMPLETE after [3.54ms]
(pass) collectStdoutLines > flushes a trailing segment that never got a newline [2.40ms]
(pass) collectStdoutLines > reassembles a JSON object split across chunk boundaries [2.63ms]
(pass) collectStdoutLines > a null stream yields no lines and an already-resolved done [0.05ms]
(pass) findEvents > returns only objects whose event matches, in order [0.08ms]
(pass) findEvents > non-JSON banner lines are skipped rather than throwing [0.04ms]
(pass) deriveVerdict — contention mode > passes when B skipped, A did not, and a conclusion was read back
(pass) deriveVerdict — contention mode > a FAILED publish still counts as ATTEMPTED — that is the SC3 contract
(pass) deriveVerdict — contention mode > fails when neither publish signal is present — the skip never surfaced
(pass) deriveVerdict — contention mode > fails when B did not skip at all
(pass) deriveVerdict — contention mode > fails when A ALSO skipped — A holds the marker, so a skip on A means something else took it
(pass) deriveVerdict — negative-control mode > passes when no skip is observed, which is the whole point of the control
(pass) deriveVerdict — negative-control mode > FAILS when a skip is still observed — the harness cannot discriminate
(pass) deriveVerdict — negative-control mode > the publish question is N/A here, not false
 14 pass / 0 fail / 27 expect() calls

$ bun run test          # full reviewer suite
 2474 pass / 0 fail / 5431 expect() calls — Ran 2474 tests across 94 files. [4.64s]

$ validate_typecheck    # 8 projects: root, packages/domain, packages/shared, services/reviewer,
                        # services/site, src/cockpit/web, tsconfig.hooks.json, tsconfig.scripts.json
 0 errors    (infra/ skipped — deps not installed locally; CI runs it with its own install step)

$ validate_lint         # services/reviewer, 228 files
 0 errors, 0 warnings

R1-2 failure path, exercised live. Dummy credentials + an unreachable Postgres, so the spawned
server never becomes healthy and bail fires from inside the try:

$ INFLIGHT_TEST_PORT=34612 ... bun scripts/smoke-concurrent-inflight-skip.ts
inflight-skip: mode=contention owner=edobry repo=minsky pr=1 port=34612
FAIL: server did not become healthy within 20s
script exit=1

$ curl -s -m 2 -o /dev/null -w "http_code=%{http_code}\n" http://127.0.0.1:34612/health
http_code=000        # nothing listening — the spawned server is gone

What that does and does not prove. It proves the throw path reaches the top-level handler and
exits 1 with the right message. It does not by itself prove finally killed the server: the
server also failed to boot in this run, so an empty port is consistent with it having exited on its
own. The cleanup guarantee rests on control flow that is deterministic rather than probed — bail
throws, and the finally is attached to the same try. Recording the distinction rather than
letting the port check read as stronger evidence than it is.

Acceptance tests, by mt#4895's own numbering.

• AT1 — RUN and passing. "runReview with a held marker returns the skip; with an available
  marker it does not. Both assertions in one test file, run with no network." Both halves are in the
  4-case run above; no network — the three GitHub calls are injected.
• AT2 — NOT run. "the script, run against a local Postgres, reports PASS and names which
  delivery was skipped. Run it twice." Needs App credentials that can read a real PR, plus operator
  authorization to act on one. [at2-deferred: mt#4897]
• AT3 — NOT run. "Negative control for L4 — with the marker released between the two deliveries,
  the script observes NO skip." Implemented as --negative-control; its decision half is covered by
  the two negative-control cases in the harness suite. Same blockers as AT2 for the live half.
  [at3-deferred: mt#4897]
• AT4 — DONE. "whatever verdict SC4 reaches is written into this spec's ## Outcome with its
  evidence, including the case where it stays unverified." Written; see ## Live verification.

Success criteria. SC1 — done. SC2 — the runnable script is shipped and its decision logic is
covered; its live invocation is AT2. SC3 — the script asserts the publish was ATTEMPTED, and the
observable is deliberately two-sided: a minsky-reviewer/findings conclusion read back off the sha
when the publish succeeds, or the review_skip_check_run_failed warn when it fails, because both
prove the branch ran through the publish call rather than returning before it. Three harness tests
pin that contract, including the failed-publish case. SC4 — verdict recorded below. SC5 — discharge
written to mt#4271's spec; the four markers live in PR #3563's merged body, not its spec, which
mt#4895's spec now says. (R1 reported SC5 Unverifiable because the mt#4271 edit is outside this
diff — correct per the contract; the record is in mt#4271's ## Deferred-marker discharge section.)

Dual-mode script — both branches exercised (mt#2776). Running only the safe branch leaves the
other's code unexecuted, and imports are hoisted, so each run below proves the whole module —
@octokit/rest, @octokit/auth-app, postgres, @octokit/webhooks-methods, and now
./inflight-skip-harness — resolves at runtime:

contention, no env          -> SKIP: MINSKY_REVIEWER_APP_ID is not set        exit 0
--negative-control, no env  -> SKIP: MINSKY_REVIEWER_APP_ID is not set        exit 0
dummy env, INFLIGHT_TEST_PR=not-a-number
                            -> FAIL: INFLIGHT_TEST_PR must be a positive integer   exit 1
dummy env, REVIEWER_PROVIDER=cohere
                            -> SKIP: not one of openai|google|anthropic       exit 0
dummy env, unreachable Postgres
                            -> FAIL: server did not become healthy within 20s  exit 1   (the R1-2 path)

SC1 — negative control: an AVAILABLE marker does not return the skip, and the injected
appIdentityFetcher is observed called exactly once, proving execution reached the first call past
the marker gate rather than merely not skipping.

Negative control — production log event, suite liveness: renamed
runReview.skipped_concurrent_inflight to runReview.MUTATED_CONTROL in review-worker.ts and
re-ran.

(fail) a HELD marker makes runReview return the concurrent_inflight skip
       error: expect(received).not.toBeNull()
(fail) the skip log carries the delivery id, which is what correlates it to a delivery
       error: expect(received).toBe(expected)
 2 pass / 2 fail

The two that do not assert on the log stayed green, which is what makes the pair meaningful rather
than one assertion twice. Restored; 4/4.

Live verification

UNVERIFIED — the reason is a missing specimen plus an authorization, not a missing mechanism.

Publishing skipped shipped with PR #3563 at 2026-09-02T08:05:26Z. All six concurrent_inflight
occurrences on record predate it, so no historical sha can answer the question — the specimen has to
be made, and this PR ships the thing that makes it.

Probes run rather than assumed. Checked for presence, never values: MINSKY_REVIEWER_APP_ID,
MINSKY_REVIEWER_INSTALLATION_ID, MINSKY_REVIEWER_PRIVATE_KEY, MINSKY_REVIEWER_WEBHOOK_SECRET,
MINSKY_PERSISTENCE_POSTGRES_URL, MINSKY_POSTGRES_URL, OPENAI_API_KEY — all seven absent.
forge_branch_protection_get main re-probed → Resource not accessible by integration, the same
result mt#4271 got; that is verified-1a for the ForgeBackend channel and inferred for the
capability, not evidence that no channel can read it.

mt#4897 owns the live run and the L5 observation, and its spec carries both blockers with the
probe results.

Best evidence standing, unchanged: PR #3504 merged with minsky-reviewer/findings at
conclusion: neutral, 13/13 checks passed. neutral and skipped are named in the same sentence of
GitHub's protected-branches documentation, so a non-success conclusion from that sentence
demonstrably does not block a merge here — a class argument, not the direct observation, which is
the distinction SC4 exists to keep visible.

Deploy verification

isDeploySurfaceFile run over this PR's actual changed files: true for all five. 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 returns indeterminate — correlate deploy-reviewer.yml's workflow
run against the merge SHA and assert the health body's service identity rather than the status code.

Note the change is seams-plus-tests: the three new deps fields are undefined in production, so the
deployed behaviour is unchanged by construction. That is a reason to expect a clean deploy, not a
reason to skip verifying it.

Coordination

PR #774 (mt#1263), open — get_files reads exactly eslint.config.js and
services/reviewer/src/review-worker.test.ts. This PR touches neither; the L2 case is a sibling
file specifically so the conflict surface stays at zero while #774 sits unmerged (and conflicted).
PR #3412 (mt#4639), open — touches services/reviewer/src/server.ts, which this PR does not
touch. Recent merges, one git_log --path per path, both pathMatched: true:
review-worker.test.ts — 0 commits in 7 days; services/reviewer/scripts — 6 commits, none
colliding with a new filename.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LJGtdNqy7Yq9n6QXkV5fjp

[minsky-reviewer]

Minsky Reviewer Status

Verdict: APPROVED — no blocking findings
Review: View review
Model: openai/gpt-5 | Tokens: 358K prompt, 7K completion | Duration: 128s
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

---

The PR adds optional injection seams to runReview, a solid L2 test suite, and an L4 smoke harness. However, two design defects in the new script are merge‑blocking: (1) stdout collection is racy — the background reader is never awaited after process exit, so trailing structured logs (including the skip and publish‑failed events) can be missed, making SC2/SC3 verification flaky; and (2) failure paths call process.exit(1) inside the try, bypassing the finally cleanup and leaking the spawned server and DB connection. Additionally, the PR defers live‑target evidence for the L4 harness — please include a redacted real run exercising contention and a publish attempt (or gate merge on the follow‑up). The production seams themselves are safe (optional with defaults), and SC1 is well covered. Address the script issues and supply live‑run evidence to proceed.

Findings

• [BLOCKING] services/reviewer/scripts/smoke-concurrent-inflight-skip.ts:118 — Racy stdout collection can miss late log lines, making L4 verdicts flaky
  collectStdoutLines starts an async reader that pushes to lines but returns only { lines } with no completion signal. Later, the script calls terminateWithGrace(proc, 10_000) and immediately parses lines to decide pass/fail (e.g., findEvents(lines, SKIP_EVENT)). There's no await to ensure the background reader has drained the stream after the process exits, so trailing log lines (including runReview.skipped_concurrent_inflight or review_skip_check_run_failed) can be dropped. This introduces nondeterminism into SC2/SC3: a true skip/publish-attempt could be missed and reported as a failure. Fix by returning a completion promise from collectStdoutLines (e.g., { lines, done }) and await done after the process exits (before parsing), or by reading stdout synchronously to completion after proc.exited.
• [BLOCKING] services/reviewer/scripts/smoke-concurrent-inflight-skip.ts:320 — process.exit in failure branches bypasses cleanup, leaking the spawned server and DB handle
  Inside main(), many error paths call fail(...), which invokes process.exit(1). Example: if (!(await waitForHealth(PORT, 20_000))) fail("server did not become healthy within 20s"); at services/reviewer/scripts/smoke-concurrent-inflight-skip.ts:320. Because this call occurs inside the try { ... } finally { await terminateWithGrace(proc, 10_000); await sql.end() } block, process.exit terminates the script immediately and the finally cleanup never runs — orphaning the spawned reviewer server and leaving the Postgres connection open. The same applies to later fail(...) calls inside the try. Replace fail(...) with throw new Error(...) (or return an error code after the finally block) so the finally always executes and the child process is terminated gracefully before exit. Then set process.exitCode = 1 once cleanup completes.
• [BLOCKING] services/reviewer/scripts/smoke-concurrent-inflight-skip.ts:1 — Missing live-run evidence for the L4 smoke script exercising real contention and publish-attempt
  This PR adds a verification/probe script (services/reviewer/scripts/smoke-concurrent-inflight-skip.ts) that targets external systems (GitHub + Postgres). Per the Live-target verification gap rule, the PR body must include redacted live-run output demonstrating the script's primary path against a real target. The PR explicitly defers the live run and only shows skip/error branches without valid credentials. That leaves SC2/SC3’s intended verification (observing runReview.skipped_concurrent_inflight for one delivery and a publish attempt) unproven for the shipped harness. Please include a short operator run transcript (delivery ids, observed skip event, and either a minsky-reviewer/findings conclusion on the head sha or a review_skip_check_run_failed warn) or gate merging until mt#4897 lands that evidence.

Spec verification

Criterion	Status	Evidence
SC1 — L2 covered. A test drives runReview with a HELD marker and asserts it returns { status: "skipped", reason: "concurrent_inflight" }. Include a negative control: the same call with the marker AVAILABLE does not return skipped.	Met	Implemented in services/reviewer/src/runreview-concurrent-inflight.test.ts: covers the denied path with reason pinned to "concurrent_inflight", asserts structured skip log, and includes a negative-control case that passes the marker gate (via injected appIdentityFetcher) without skipping.
SC2 — L4 covered. A runnable script under services/reviewer/scripts/ fires two overlapping deliveries for one PR+sha at a live reviewer process over a real Postgres and asserts runReview.skipped_concurrent_inflight appears for exactly one of them. Exit 0 on pass or graceful skip, non-zero on failure.	Met	New script services/reviewer/scripts/smoke-concurrent-inflight-skip.ts spawns a real server, sequences via reviewer_inflight_reviews, posts signed webhooks, and asserts the structured skip event for exactly one delivery. It implements exit 0 on pass/skip and non-zero on failure.
SC3 — the L4 script also asserts the check-run publish is ATTEMPTED, naming the observable used.	Met	The script reads check runs via Octokit (listForRef with check_name "minsky-reviewer/findings") and also treats the warn event "review_skip_check_run_failed" as evidence of an attempted publish; see services/reviewer/scripts/smoke-concurrent-inflight-skip.ts: lines ~382-433 and usage at ~520-548.
SC4 — a recorded verdict on L5 (direct observation or explicitly recorded as unverified with reason).	Met	PR description's Live verification section records L5 as UNVERIFIED with reasons (no specimen yet; operator authorization required) and cites the best-standing evidence. This satisfies the "recorded verdict" branch of SC4.
SC5 — mt#4271's deferred markers are discharged or re-pointed in its spec/record.	Unverifiable	The change to mt#4271’s spec/body is outside this diff. The Referenced Task Specs section for mt#4271 is TRUNCATED and does not show the asserted discharge/re-pointing; cannot confirm from the provided content. Reporting Unverifiable per contract.

Documentation impact

• no-update-needed — The PR adds optional test seams to RunReviewDeps and a developer-only smoke script; runtime behavior for production callers is unchanged (optional fields with real defaults). No user-facing routes, commands, or config contracts were modified. I checked services/reviewer/README.md and docs/architecture ADR-030 applicability via code comments; no impacted docs require updates for these internals.

[minsky-reviewer]

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

---

R2 verification: the prior blocking findings are addressed. The L4 smoke script now drains stdout deterministically via a completion promise and awaits it before parsing, and failure paths inside main throw (HarnessFailure) rather than calling process.exit, ensuring cleanup runs. These are covered by new unit tests in inflight-skip-harness.test.ts, and the functional core/imperative shell split keeps decision logic testable. The harness correctly sequences contention by polling for a live marker and implements a negative-control mode. I found no new critical defects introduced by the fixes. Verdict: APPROVE.

Spec verification

Criterion	Status	Evidence
SC1 — L2 covered. A test drives runReview with a HELD marker and asserts it returns { status: "skipped", reason: "concurrent_inflight" }. Include a negative control: the same call with the marker AVAILABLE does not return skipped. Correction: GitHub fetch must be stubbed via an injected seam.	Met	The PR previously added tests and seams; this round adds no new L2 test changes, but the task spec’s Outcome section documents the passing test run. The injected, test-covered harness is present; no regression introduced. Evidence of L2 coverage exists in the PR’s earlier commit (not re-touched here).
SC2 — L4 covered. A runnable script under services/reviewer/scripts/ fires two overlapping deliveries and asserts runReview.skipped_concurrent_inflight appears for exactly one.	Met	services/reviewer/scripts/smoke-concurrent-inflight-skip.ts:1-474 — new script implements the L4 harness, spawning the server, posting two signed webhooks, polling the marker row, and asserting the structured skip log via findEvents(SKIP_EVENT). Negative control via --negative-control is implemented.
SC3 — the L4 script also asserts the check-run publish is ATTEMPTED, and states which observable and why.	Met	services/reviewer/scripts/smoke-concurrent-inflight-skip.ts:281-316 defines readSkipCheckRunConclusion to read minsky-reviewer/findings via Octokit; PUBLISH_FAILED_EVENT is also observed. Verdict derivation is two-sided in services/reviewer/scripts/inflight-skip-harness.ts:103-147 (deriveVerdict), treating either a read-back conclusion or a publish-failed warn as attempted.
SC4 — a recorded verdict on L5 (observed or explicitly unverified with reason).	Met	Task spec’s Outcome section states SC4 remains unverified and records the reasons and best evidence. This is captured in the PR description/spec and does not require code; no contradiction in this diff.
SC5 — mt#4271's deferred markers are discharged or re-pointed.	Met	Task spec’s Outcome/Planning sections record the re-pointing and discharge; no code change needed here. No conflicting changes in this diff.

Adoption sweep

Symbol	Kind	Consumers found	Classification	Notes
services/reviewer/scripts/inflight-skip-harness.collectStdoutLines	function	services/reviewer/scripts/inflight-skip-harness.test.ts:18 — imports and tests completion semantics, services/reviewer/scripts/smoke-concurrent-inflight-skip.ts:180 — used to capture server stdout with a drain-before-parse contract	Adopted
services/reviewer/scripts/inflight-skip-harness.findEvents	function	services/reviewer/scripts/inflight-skip-harness.test.ts:51 — unit tests exercise JSON parsing/skip semantics, services/reviewer/scripts/smoke-concurrent-inflight-skip.ts:257, 333 — used to detect skip and publish-failed events from server stdout	Adopted

Documentation impact

• no-update-needed — This PR adds a test-only harness and a new smoke script under services/reviewer/scripts plus internal helper functions. No user-facing API, CLI, or documented behavior changed; acceptance criteria are about verification coverage. Docs do not reference these internal script details, and no existing docs are invalidated.

[minsky-reviewer]

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

---

R2 verification: the prior blocking findings on the L4 smoke script are addressed. Stdout collection now exposes a completion promise and the shell awaits it before parsing, eliminating the race that could miss the terminal skip line. All in-try failure paths now throw (HarnessFailure via bail) instead of calling process.exit, ensuring the finally runs and cleans up the spawned server and Postgres handle; early env-gated skips remain process.exit by design as they occur pre-spawn. The decision logic is factored into inflight-skip-harness.ts with comprehensive tests. L2 coverage is added via injected seams in RunReviewDeps without changing production behavior (optional fields with real defaults). I found no new critical defects introduced by these changes. Verdict: APPROVE.

Spec verification

Criterion	Status	Evidence
SC1 — L2 covered. A test drives runReview with a HELD marker and asserts it returns { status: "skipped", reason: "concurrent_inflight" }. Include a negative control: the same call with the marker AVAILABLE does not return skipped.	Met	services/reviewer/src/runreview-concurrent-inflight.test.ts:123-174 — asserts status: "skipped", reason: "concurrent_inflight" and pins the skip log; services/reviewer/src/runreview-concurrent-inflight.test.ts:224-268 — negative control throws past the gate and verifies no skip logged.
SC2 — L4 covered. A runnable script under services/reviewer/scripts/ fires two overlapping deliveries for one PR+sha at a live reviewer process over a real Postgres and asserts runReview.skipped_concurrent_inflight appears for exactly one of them. Exit 0 on pass or graceful skip, non-zero on genuine failure.	Met	services/reviewer/scripts/smoke-concurrent-inflight-skip.ts — new script implements spawn, signed webhook posts, sequencing via live marker, and verdict; process exit codes set to 0 on skip/pass and 1 on fail.
SC3 — the L4 script also asserts the check-run publish is ATTEMPTED, using either a read-back conclusion or the review_skip_check_run_failed warn.	Met	services/reviewer/scripts/smoke-concurrent-inflight-skip.ts:307-345 readSkipCheckRunConclusion() plus PUBLISH_FAILED_EVENT; services/reviewer/scripts/inflight-skip-harness.ts:117-147 deriveVerdict treats either outcome as attempted; covered by inflight-skip-harness.test.ts:101-120.
SC4 — a recorded verdict on L5 (either a live observation of skipped not blocking a merge, or an explicit statement that it remains unverified with the reason).	Met	Task spec’s Outcome section (in this review context) explicitly records L5 as UNVERIFIED with reasons and cites current best evidence. No repo change required; verification rests on the provided spec text.
SC5 — mt#4271's deferred markers are discharged or re-pointed.	Unverifiable	Depends on mt#4271’s spec/PR body updates outside this diff. The Referenced Task Specs content for mt#4271 is TRUNCATED; cannot verify the re-pointing/discharge from the available excerpt per the contract.

Documentation impact

• no-update-needed — Production behavior is unchanged: RunReviewDeps gained optional test seams with real defaults, and new L2 test + L4 harness/scripts were added. No user-facing routes, CLI, or documented workflows changed. The L4 script is a developer smoke tool under services/reviewer/scripts/ and does not affect public docs.