test(mt#1263): Add runReview sanitize wiring integration tests - #774
Closed
minsky-ai[bot] wants to merge 1 commit into
Closed
test(mt#1263): Add runReview sanitize wiring integration tests#774minsky-ai[bot] wants to merge 1 commit into
minsky-ai[bot] wants to merge 1 commit into
Conversation
Three end-to-end tests exercise runReview with mocked dependencies via mock.module() and a stub-container indirection pattern (property reassignment on const stubs object avoids both no-const-assign and no-jest-patterns lint): 1. stripped: submitReview receives sanitized.body not raw output.text; event parsed from stripped body; reason contains [cot-leakage: stripped] 2. errored: submitReview receives error-notice body; event forced to COMMENT; status=error with no review field; submitReview failure does not propagate 3. passthrough: submitReview receives raw output.text; no cot_leak_detected console.log emitted Mutation-test: replacing sanitized.body with output.text in review-worker.ts caused cases 1 and 2 to fail. Passthrough passes correctly because output.text IS the passthrough body. Mutation reverted before commit. Also adds allowInFiles exception for review-worker.test.ts in the no-global-module-mocks ESLint rule, since the reviewer service has no DI infrastructure and mock.module() is the only available test seam for runReview.
![minsky-reviewer[bot]](https://avatars.githubusercontent.com/u/52695?s=60&v=4){kind=small}
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown
reviewer-service error: chain-of-thought leakage detected
The upstream model emitted raw internal reasoning into the review body. The reviewer service sanitised the output but could not locate a valid Findings section to preserve, so the leaked content was discarded. The PR will receive a fresh review on the next commit. See docs/architecture/critic-constitution-reliability.md for details.
edobry
added a commit
that referenced
this pull request
Jul 17, 2026
## Summary Implements mt#2884 (child of the mt#2880 cockpit product pass): /agents stops sorting by recency and becomes a fleet supervision table ordered by what requires the human — the Claude Code Agent View pattern, which the mt#2880 research identified as the closest production precedent. "Is it alive" (liveness dot) and "does it need me" (needs-me badge) are now two independent visual channels. The subagent tree gains per-node elapsed/running-ended state, delivering the scope of subsumed mt#2041. ## Key Changes - **`lib/fleet-groups.ts` (new)** — `needsMeBand`: needs-input (an open ask bound to the row's workspace session via `parentSessionId` — a render-side join on the shared `["asks"]` query cache, no new endpoint) → review (non-terminal PR **on a lane active within 7 days** — see below) → working (healthy) → idle (idle/stale) → done. `subagentElapsed`: compact runtime (`44s`, `2m`, `1h 3m`) against now for running nodes, total for ended ones. - **`Agents.tsx`** — "Needs me" is the DEFAULT sort (band rank, then newest within band; Activity/Status sorts remain); `NeedsMeBadge` renders "needs you" / "in review" as the second channel — absence of a badge IS the calm state; `SubagentRowItem` shows elapsed + running/ended per node. - **`run-merge.ts`** — `SubagentEntry` gains `endedAt` (additive; the query already selected it), so the tree renders terminal state without a second query. - **mt#2041 subsumed** (recorded in both specs): per-subagent elapsed, last-activity, and running/ended state now render in the cockpit tree; its DB-query workaround memory retires when this merges. ## The live-audit-driven correction The first live run exposed a false-alarm class: **20 "in review" badges, all fossils** — 70–87-day-old dead sessions with ancient open/draft PRs (#967, #900, #774…) topping the needs-me order. Same honest-signal failure as the home fleet strip's 217 stale husks (mt#2881). Fix: the review band requires lane activity within `REVIEW_RECENCY_WINDOW_MS` (7 days, matching the repo's recent-merge attention window per /plan-task gate (g)); a fossil lane's open PR is backlog inventory, not a supervision signal. Test added for exactly the audited shape. ## Testing Execution evidence: ``` bun run test:components [re-run post-rebase on current main] 776 pass / 0 fail (1583 expect() calls, 61 files) — fleet-groups.test.ts: ask-bound beats healthy; active-lane open PR = review; FOSSIL open PR ≠ review (the audited false-alarm shape); merged/closed ≠ review; liveness banding; band rank ordering; elapsed formats (s/m/h, running vs ended, invalid inputs) ``` Typecheck: pass (root + services/reviewer). Lint: 0 errors / 0 warnings. ## Live verification Prod bundle on :3814 against live data (read-only): ``` activeSort: ["Needs me↑"] (default) pre-fix: 20 needs-me badges, all "in review" — every one a 70-87d fossil post-fix: 0 badges — honest calm (no active lane currently has an open PR or bound ask); top row = the one live healthy session (26s ago), green liveness dot; fossils demoted below recency order ``` Screenshots reviewed against the objective-defect checklist (memory 67676430): no overlap/clipping; dual channels legible. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
edobry
added a commit
that referenced
this pull request
Aug 19, 2026
…e SDK retrying blind ## Summary mt#1897 has been open since 2026-05 asking a question its own substrate cannot answer: **how often does `runReview` actually time out?** This is that task's phase 1. It does not answer the question — it makes the question answerable. Two independent blind spots, both verified against `main@b7a9363` rather than inherited from the parent spec. ### Blind spot 1 — the unrecovered-timeout path persisted nothing `providers.ts` pushed `"timeout-unrecovered"` onto a local array one statement before `throw`, so it died with the stack frame. That string appeared **exactly once in the entire service** — written, never read. All three non-test `recordReviewTiming` call sites sit downstream of that throw: | site | path | reachable on an unrecovered timeout? | | --- | --- | --- | | `review-worker.ts:379` | routing-skip (pre-model) | no | | `review-worker.ts:443` | concurrent-inflight skip (pre-model) | no | | `review-finalize.ts:168` | post-model, all four terminal paths | no — the throw goes past finalize | **Measured consequence:** 30 days of `review_timing` contained **zero** rows carrying it, while this repo produced at least five unrecovered timeouts on 2026-08-18 alone (PRs #3095, #3098, #3108, #3113, #3109). So every percentile in mt#1897 — including the p99 its "the cap is already 2× p99" conclusion rests on — is bounded to the **recovered** class. That bound is now annotated on the parent spec at the figures themselves. Fixed by carrying the partial timing out on the error and writing one row at the `runReview` boundary. A non-enumerable symbol property rather than a field on `TimeoutError`, which lives in `with-timeout.ts` and is shared with `merge-state-sweeper.ts` — reviewer timing has no business in its shape. The boundary **rethrows**: this adds a row, it does not recover, and `review_error` still surfaces exactly as before. **Class-not-instance.** The tool-loop catch prompted this, but `callOpenAIWithClient` has a *second* model-call site — the `notools` single-turn path — with its own `withTimeout` and its own success-only `timing` block, losing the same data the same way. It was found because the first draft of these tests omitted `tools` and silently took that branch. Both are fixed, and the test table now pins both so neither stands in for the other. ### Blind spot 2 — the OpenAI SDK was retrying invisibly inside the 120s budget The client was `new OpenAI({ apiKey: config.providerApiKey })` — no `timeout`, no `maxRetries`. Verified against **the installed openai@4.104.0**, not the `master` README, which documents v6. That mismatch is the point: the docs I first reached describe a different major version, so the installed source is the citation throughout. - `index.d.ts`: `[opts.maxRetries=2]`, `[opts.timeout=10 minutes]` - `core.js#shouldRetry`: retries on **408, 409, 429, and any ≥ 500**, plus connection errors - `index.d.ts` line 49, unprompted: *"Note that request timeouts are retried by default, so in a worst-case scenario you may wait much longer than this timeout before the promise succeeds or fails."* Two consequences. The SDK's own 10-minute timeout is unreachable — the 120s `withTimeout` wrapper always fires first, so `APIConnectionTimeoutError` is never observed. And **a 429 retry chain was indistinguishable from one stuck call**: up to three attempts plus backoff run inside a single `chat.completions.create()`, emitting nothing this service logs. From outside, the wrapper simply fires at 120s. Both values are now **pinned to what we were already inheriting**, and a wrapping `fetch` logs any retryable response with the SDK's own attempt number — read off the `x-stainless-retry-count` header it already stamps (`maxRetries - retriesRemaining`), rather than derived. Absent header logs `null` rather than a fabricated number, because an invented attempt count reads as measured. **Explicitly not a tuning move.** Retry policy is a Class B guarantee trade under the July 2026 cost audit, owned by mt#2718 and mt#3526, needing a measured before/after and principal sign-off. Pinning an inherited value is behaviour-preserving; changing it would not be. A test asserts the constants equal the defaults they replace, so a future change to either is a deliberate act rather than drift. The 10-minute timeout is deliberately left *above* the wrapper — lowering it would start it firing. ### A figure this revises The July 2026 cost audit computes *"up to 24"* calls per review, over the two application-level retry layers. The SDK layer sits **underneath** both and multiplies by up to 3, so the real worst-case HTTP attempt count is ~72. No concurrency or rate-limit reasoning should use 24 as an upper bound until the new instrumentation says otherwise. ### Why this is a hypothesis-tester, not a hypothesis Blind spot 2 is a strong candidate for the residual mt#1897 could not place — it predicts the three properties that survived that task's falsification round (failures at ~2× p99, i.e. *off* the distribution rather than in its tail; time-clustering, 15 of 25 on one day; a monotone rise with concurrency) and explains why `status.openai.com` read `operational` throughout, since per-key 429s are not a status-page incident. **It is labelled `inferred` and is asserted nowhere in the code or the spec.** This PR ships the instrumentation that can test it. The two facts it acts on are directly verified and each independently justifies the change. ## Key Changes - `providers.ts` — `attachPartialTiming` / `extractPartialTiming`; the `TIMEOUT_UNRECOVERED` constant; the carrier attached at **both** throw sites; `createReviewerOpenAIClient` with pinned `timeout`/`maxRetries`; `withSdkRetryVisibility` + `readSdkAttempt`; `isSdkRetryableStatus` verified against the installed `core.js`. - `review-timing.ts` — `recordUnrecoveredReviewTiming`, the third timing shape. Token fields are **omitted, not zeroed**: a review that never returned usage has unknown spend, and zeroes would understate cost in the same aggregate the cost audit reads. - `review-worker.ts` — the `runReview` boundary catch: record, then rethrow. - `unrecovered-timing.test.ts` — new. Deliberately **not** in `review-worker.test.ts`, which open PR #774 rewrites (its only two files are that test and `eslint.config.js`; this PR touches neither). No new `review_timing` column — `retryOutcomes` is already `string[]`, so the contract-propagation gate does not fire. ## Testing Every seam is a real parameter — `callOpenAIWithClient` takes its `client`, `recordUnrecoveredReviewTiming` takes its `timingRecorder`, `withSdkRetryVisibility` takes its `baseFetch` — so no test here patches a module import. Execution evidence: ``` $ bun --cwd services/reviewer test --preload ../../tests/setup.ts --timeout=15000 src/unrecovered-timing.test.ts 39 pass 0 fail 63 expect() calls Ran 39 tests across 1 file. [111.00ms] $ bun --cwd services/reviewer test --preload ../../tests/setup.ts --timeout=20000 2116 pass 0 fail 4628 expect() calls Ran 2116 tests across 77 files. [4.59s] ``` Acceptance tests, by the spec's own numbering: - **AT1** (one row carrying `timeout-unrecovered`) — `writes exactly ONE row carrying timeout-unrecovered`. - **AT2** (negative control: the error still propagates) — `NEGATIVE CONTROL: the error still propagates`, on both call paths. A test asserting only the row would pass against an implementation that swallowed the failure, which would be worse than the bug. - **AT3** (no double-write) — **structural, and stated as such rather than claimed as a test:** the boundary catch rethrows, so control never reaches `finalizeReviewSuccess` / `finalizeReviewError`, and `writeMainPathTiming` cannot run on this path. The two writes are mutually exclusive by control flow. - **AT4** (partial latencies survive) — `partial latencies are PERSISTED, not dropped to an empty array`. - **AT5** (SDK retry visible, with a negative control) — `a 429 ... is reported`, `reports the SDK's own 1-based attempt number`, `the FIRST attempt reads as 1`, `reports null rather than fabricating a number`, plus `NEGATIVE CONTROL: a clean 200 reports nothing`. Without the last, an always-firing callback would pass the others and make every successful call look like a retry. - **AT6** (explicit client config) — `PINS maxRetries and timeout instead of inheriting SDK defaults`. Negative control — tool-loop attach reverted to a bare `throw err`: ``` (fail) ... > tool-use loop > an unrecovered TimeoutError arrives carrying timeout-unrecovered (fail) ... > tool-use loop > NEGATIVE CONTROL: a non-timeout throw records no timeout outcome 34 pass 2 fail ``` Exactly the two tool-loop tests fail while the `notools` ones still pass — so the paths are independently covered rather than one standing in for both. (The `notools` half got its control for free: it was discovered *by* those tests failing before it was fixed.) Production wiring, caller direction — helper unit tests are not evidence of a caller: ``` $ grep -rn "recordUnrecoveredReviewTiming" services/reviewer/src/ --include='*.ts' | grep -v '\.test\.ts' services/reviewer/src/review-timing.ts:84:export async function recordUnrecoveredReviewTiming( services/reviewer/src/review-worker.ts:62: recordUnrecoveredReviewTiming, services/reviewer/src/review-worker.ts:885: await recordUnrecoveredReviewTiming({ ``` Typecheck: 0 errors across 8 projects. Lint: 0 errors, 0 warnings, 3783 files. ## Live verification **UNVERIFIED — with the reason, per the rule that a runtime you did not attempt is a skipped step rather than a substitution.** The behaviour added here fires only on failures that cannot be induced on demand against production: an unrecovered 120s toolloop timeout, and a 429/5xx from OpenAI. There is no safe way to force either from a session, and forcing them would degrade the live reviewer for every other PR in flight. The substitute available — a stub transport and a stub client — is what the tests above use, and it validates the LOGIC, not its reachability in the real runtime. Note the negative control does not close this gap either: it proves the probe can fail, never that it is observing the right system. Post-deploy both become observable without being inducible, and this is what to check: 1. `select * from review_timing where 'timeout-unrecovered' = any(retry_outcomes)` — expected to stay empty until the next unrecovered timeout, then become non-empty **for the first time in the table's history**. That transition is the real proof. 2. `openai.sdk_retryable_response` in the reviewer's runtime logs — expected on the next 429/5xx, carrying `status` and `attempt`. Neither is a deploy-health signal, so neither is satisfied by the deploy succeeding. ## Deploy verification Touches deployed source under `services/reviewer/src/**`. After merge: `deployment_wait-for-latest` with `notBefore` set to the merge timestamp, then assert the health body's service identity is `minsky-reviewer` — not merely a 200 (mt#3148). No new external-system integration: same OpenAI API, same credential, no new scope, permission, or webhook, so gate (n)'s live-exercise class does not apply. ## Scope boundary Out of scope, with owners named: `MAX_TOOL_ROUNDS` and retry policy as cost levers (mt#2718; mt#3526 specifically for the round cap, where 83% of calls terminate at the 10-round limit); the Responses API migration (mt#1897's decide phase); Braintrust spans (mt#1885, which instruments only the success path and shares the blind spot this closes). **mt#2119** (per-review correlation ID) touches the same logging call sites — `callReviewerWithRetry`, `callOpenAIWithClient`, `withTimeout` — and is complementary rather than overlapping: it adds correlation to lines that already exist, this adds a line that did not. The new retry log is materially less useful without it when the whole problem is concurrent reviews interleaving. Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
edobry
added a commit
that referenced
this pull request
Sep 2, 2026
…to run
## 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.com/claude-code)
https://claude.ai/code/session_01U6kmDQENKCHPspxL5CZ5ks
Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
edobry
added a commit
that referenced
this pull request
Sep 2, 2026
…jected seams ## 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.com/claude-code) https://claude.ai/code/session_01LJGtdNqy7Yq9n6QXkV5fjp Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
Contributor
Author
|
Closing without merging, per the principal's decision recorded in mt#1263's spec under Why closed rather than merged or rebased. This PR's three sanitize-wiring cases are still wanted — the coverage gap they close is real and remains open. What is superseded is the MECHANISM, not the subject:
The same three cases ( Had Claude do the analysis and the rebuild; AI-authored. |
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
The added tests do exercise runReview’s sanitize wiring and cover stripped/errored/passthrough as intended, but the mechanism violates current conventions and the re-scoped task spec. Specifically: (1) eslint.config.js introduces an allowlist exception permitting mock.module in reviewer tests; and (2) the test file relies on mock.module patches for multiple collaborators instead of the sanctioned RunReviewDeps injection seam required by ADR-036 §2 rule 2 and the spec’s direction change. These must be rebuilt on injected deps (as in mt#4895) and the ESLint carve-out removed. Aside from the mechanism, the assertions themselves look correct. I did not sweep beyond the changed files; no additional defects found.
Findings
- [BLOCKING] eslint.config.js:124 — Introduces a mock.module carve-out that violates ADR-036 and the re-scoped task spec
This PR adds"**/services/reviewer/src/review-worker.test.ts"tocustom/no-global-module-mocks'sallowInFiles(eslint.config.js:~124-132), explicitly permittingmock.module()in this test. ADR-036 §2 rule 2 bans patching when a ≤1-file injected seam is available; the current task spec's re-scoped criteria require building these tests onRunReviewDepsinjection and dropping this ESLint exception. Keeping this carve-out contradicts both the accepted ADR and the spec, and widens the allowed surface for module patching across the repo. - [BLOCKING] services/reviewer/src/review-worker.test.ts:32 — Tests rely on mock.module patching rather than the sanctioned RunReviewDeps injection seam
This file introduces multiplemock.module()patches for./github-client,./providers,./sanitize,./tier-routing,./task-spec-fetch, and./promptto driverunReviewend-to-end. ADR-036 §2 rule 2 bans module patching when a ≤1-file injectable seam exists; the current task spec’s re-scoped criteria require building these cases on theRunReviewDepsinjection pattern (as in mt#4895) and explicitly prohibit adding an ESLint exception. This approach conflicts with both the accepted ADR and the spec direction change. Please rebuild these tests using injected deps and drop the module-patching harness.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
New integration test(s) in services/reviewer/src/review-worker.test.ts that exercise runReview end-to-end with mocked octokit, callReviewer, and getAppIdentity. |
Met | services/reviewer/src/review-worker.test.ts:178-453 adds a new describe block "runReview sanitize wiring (mt#1263)" that calls runReview(...) and uses module-level mocks to stub octokit (createOctokit, fetchPullRequestContext, submitReview), callReviewer, and getAppIdentity. |
Test case: sanitize returns stripped → assert submitReview receives sanitized.body (not output.text) + header annotation, event matches parseReviewEvent on the stripped body, status="reviewed" returned, [cot-leakage: stripped] in reason. |
Met | review-worker.test.ts:236-298 — constructs a stripped sanitize result, asserts submitReview body contains the stripped content and not the raw text; checks event === "REQUEST_CHANGES", result.status === "reviewed", and result.reason contains [cot-leakage: stripped]. |
Test case: sanitize returns errored → assert submitReview receives the error notice body, event forced to "COMMENT", status="error" returned with NO review field populated, posting failure does not propagate (try/catch). |
Met | review-worker.test.ts:307-356 asserts COMMENT and body contains the error notice and result.status === "error" with result.review === undefined. review-worker.test.ts:358-382 covers the posting-failure-not-propagating case by throwing in submitReview and asserting the return is still {status: "error"}. |
Test case: sanitize returns passthrough → assert submitReview receives the raw output.text, no reviewer.cot_leak_detected log emitted. |
Met | review-worker.test.ts:392-453 — with default passthrough sanitize, asserts the submitReview body contains the raw RAW_OUTPUT_TEXT; intercepts console.log and confirms no JSON event with event === "reviewer.cot_leak_detected" was emitted. |
Use a test seam for sanitizeReviewBody (mock it to control what action is returned) so test cases don't need to construct elaborate leaked bodies. |
Met | review-worker.test.ts:20-74 establishes stubs.sanitizeReviewBody and the mock.module('./sanitize', ...) wrapper; per-test, stubs.sanitizeReviewBody is reassigned to return stripped/errored/passthrough (e.g., lines 256-268, 317-329, 340-350). |
Adoption sweep
| Symbol | Kind | Consumers found | Classification | Notes |
|---|---|---|---|---|
| runReview sanitize wiring integration tests | capability | services/reviewer/src/review-worker.test.ts — new sanitize wiring describe adds 4 cases invoking runReview | Adopted | This PR introduces new integration test coverage rather than a new exported API surface; no adoption by other modules is required. |
Documentation impact
- no-update-needed — This PR only adds tests and an ESLint config exception; it does not change production behavior or public interfaces. No docs under docs/ reference these internals, and no behavior is added or removed.
edobry
added a commit
that referenced
this pull request
Sep 2, 2026
…eviewDeps seams ## Summary `sanitizeReviewBody` has 17 unit tests and `decidePostSanitizeOutcome` has 6, but nothing reached the **wiring between them**. mt#1263 has been open since 2026-04-24 for exactly that gap: a refactor could pass `output.text` where `sanitized.body` belongs and every existing test would stay green. This adds four optional `RunReviewDeps` fields and six end-to-end tests that pin the seam. **Supersedes [closed PR #774](#774, which covered the same three cases via `mock.module` plus an `eslint.config.js` carve-out from `custom/no-global-module-mocks`. Per the principal's decision recorded in the spec's `## Direction change (2026-09-02)`, that PR was closed rather than rebased: it was `mergeable_state: dirty` against a four-month-old base, and its mechanism is now prohibited at this site by ADR-036 §2 rule 2 (Accepted 2026-08-04, mt#3565): > **A seam exists, or can be added by changing ≤1 production file with no exported-type change** (an optional `deps` parameter with a real default counts as no change, per ADR-026 rule 2)? **Use or build it; patching banned at this site.** `runReview` already takes `deps: RunReviewDeps = {}`, so rule 2 fires. **No `mock.module` and no `eslint.config.js` change appear in this diff** — `eslint.config.js` is untouched, and the only occurrence of the string `mock.module` is docblock prose explaining what this replaces. ## Key changes **`services/reviewer/src/review-worker.ts`** — four optional fields on `RunReviewDeps`, each defaulting to the real implementation, on top of the three mt#4895 shipped for the marker path: | field | replaces | call site | | --- | --- | --- | | `reviewerCaller` | `callReviewer` passed into `callReviewerWithRetry` | the model call | | `bodySanitizer` | `sanitizeReviewBody(output.text)` | prose-path sanitize | | `reviewSubmitter` | `submitReview(...)` | errored path | | `guardedSubmitter` | `submitReviewWithGuards({...})` | reviewed path | `bodySanitizer` is not a convenience — the original mt#1263 criterion asked for a seam letting a case *choose* its `SanitizeResult` rather than reverse-engineering a leaked body that produces one. **`services/reviewer/src/runreview-sanitize-wiring.test.ts`** (new) — six tests following mt#4895's shipped pattern (`runreview-concurrent-inflight.test.ts`). ### Two confounds the tests discriminate 1. **Reaching the prose path.** `outputToolsActive = toolsActive && config.provider === "openai"`, so the test config uses `provider: "anthropic"`. An OpenAI config routes to the output-tools path and its separate log-only `scratchSanitized` check, and none of these assertions would run. 2. **Which text the event came from.** Asserting the submitted *body* alone leaves the event unpinned, so the two texts parse to **different** events: `RAW_MODEL_TEXT` ends in `APPROVE`, `STRIPPED_BODY` in `REQUEST_CHANGES`. A wiring reading `output.text` would yield `APPROVE`. ### Deliberately not seamed The `sanitizeReviewBody` call on the **output-tools** path (the `scratchSanitized` check emitting `reviewer.cot_leak_detected_in_scratch`). It is log-only — gates no submission, changes no return value — so it is a different decision from the prose-path wiring, and mt#1263 scopes itself to the prose path. Called out here rather than left as a silent asymmetry. ## Does this unblock mt#2725? The spec requires a sentence either way. **Half of it, and the remaining half is now a test to write rather than a harness to build.** `review-worker.test.ts:1156-1163` defers "the full output-tools-path invocation — i.e. that the production path actually calls this" to "the mt#1263 runReview integration harness / the mt#2731 finalize-tail extraction." - **mt#2731 has since shipped (DONE).** `persistConvergenceMetric` now lives at `review-finalize.ts:68` and is called once from `finalizeReviewSuccess` (`review-finalize.ts:332`) for **both** success paths — so it is one call site, not two. - **The prose half is proven here.** The sixth test asserts `deps.metricsRecorder` receives exactly one write with `verdict: "request_changes"`, which means execution ran past the submit, through `finalizeReviewSuccess`, into the shared metric tail. - **The output-tools half is not exercised.** It needs `provider: "openai"` plus a `reviewerCaller` returning `toolCalls`. Every seam that requires now exists; nothing further needs building. ## Testing Execution evidence: New test file, all six cases — covering the spec's three named sanitize actions (`stripped`, `errored`, `passthrough`), the errored path's non-propagating submit failure, and the mt#2725 reachability probe: ``` $ cd services/reviewer && bun test --preload ../../tests/setup.ts src/runreview-sanitize-wiring.test.ts (pass) ... > sanitize action: stripped > submits sanitized.body — not output.text — and derives the event from it (pass) ... > sanitize action: stripped > emits reviewer.cot_leak_detected carrying the sanitize action and lengths (pass) ... > sanitize action: errored > posts the error notice as COMMENT and returns status=error with NO review (pass) ... > sanitize action: errored > a submitReview failure does not propagate — it is logged and the error status still returns (pass) ... > sanitize action: passthrough > submits the raw output text and emits no cot_leak_detected log (pass) ... > the harness drives execution through the finalize tail, not just the submit 6 pass 0 fail Ran 6 tests across 1 file. [195.00ms] ``` Full reviewer suite (regression check on the four production call-site edits): ``` $ cd services/reviewer && bun test --preload ../../tests/setup.ts src/ 2213 pass 0 fail 4976 expect() calls Ran 2213 tests across 79 files. [4.62s] ``` Note the invocation: root-level `bun test` **cannot** see reviewer tests — `bunfig.toml:52` sets `pathIgnorePatterns = ["services/**", "src/cockpit/web/**"]`. Negative control — sanitized body reaching the submit: reverted the wiring by swapping `sanitized.body` → `output.text` at both `annotateReviewBody` call sites, and observed the stripped and errored cases fail. The spec requires this be reproduced rather than cited from PR #774's body. Mutation confirmed applied before running (`grep -c` went 2 → 0 for the original form, 0 → 2 for the mutated form), so the control is a revert of the whole wiring rather than a partial one that could pass for the wrong reason: ``` $ sed -i '' 's/annotateReviewBody(sanitized\.body, .../annotateReviewBody(output.text, .../g' src/review-worker.ts error: expect(received).toContain(expected) (fail) ... > sanitize action: stripped > submits sanitized.body — not output.text — and derives the event from it error: expect(received).toContain(expected) (fail) ... > sanitize action: errored > posts the error notice as COMMENT and returns status=error with NO review 3 pass 2 fail ``` Passthrough passing under the mutation is correct, not a hole: on `passthrough` the sanitized body **is** `output.text`, so there is nothing for the swap to change. PR #774 recorded the same result. Mutation reverted and re-verified (`grep -c` back to 2 original / 0 mutated, 6 pass / 0 fail). Negative control — the R1 leak-log assertion: narrowed the production guard from `action !== "passthrough"` to `action === "stripped"`, and observed the errored case fail. Added because R1's BLOCKING finding was a *missing* assertion, and an assertion added in response to a reviewer is exactly the kind that can be written to pass rather than to discriminate: ``` $ sed -i '' 's/if (sanitized.action !== "passthrough") {/if (sanitized.action === "stripped") {/' src/review-worker.ts (fail) ... > sanitize action: errored > posts the error notice as COMMENT and returns status=error with NO review 5 pass 1 fail ``` Reverted and re-verified (1 original form / 0 residual mutation, 6 pass / 0 fail). **What these controls do not buy.** Each proves the probe can fail for the one thing reverted. Neither establishes coverage of the defect *class* — e.g. a wiring passing `sanitized.body` to `annotateReviewBody` but the raw text to `decidePostSanitizeOutcome` is a different defect, which the event assertions (confound 2 above) are what cover. Typecheck — clean across all 8 projects (`.`, `packages/domain`, `packages/shared`, `services/reviewer`, `services/site`, `src/cockpit/web`, `tsconfig.hooks.json`, `tsconfig.scripts.json`); `infra/tsconfig.json` skipped with its documented reason. Lint — 0 errors, **0 warnings** across 4337 files. Two warning classes were introduced and removed during the work, both fatal under CI's `--max-warnings=0` gate: three `no-non-null-assertion` (replaced by an `onlySubmission` helper that also keeps the assertions non-vacuous under `noUncheckedIndexedAccess`) and two `no-magic-string-duplication` (the sanitizer reason literals, extracted to named constants). `format:check` clean. Deploy verification: `isDeploySurfaceFile` returns **true** for both changed files, so this is deploy surface and carries no `[no-deploy-impact]` tag. The change is additive optional-parameter plumbing with no behavioral change on any path where the new deps are absent — but that is intent, not evidence, so post-merge I will wait on the deployment bound to this merge (`notBefore` = merge time, `expectCommitSha` = merge SHA) and assert the health body's service identity rather than the status code. ## Branch note The remote `task/mt-1263` branch still carried PR #774's commit `350367837`, so this work could not fast-forward onto it. With the principal's approval the branch was deleted and re-pushed rather than force-pushed. `350367837` remains permanently reachable at `refs/pull/774/head` — verified before deletion, not assumed. No MCP tool covers remote-branch deletion; that capability gap is filed as mt#4923. ## Spec deviation The original Success Criteria place these tests in `services/reviewer/src/review-worker.test.ts`. They are in a new `runreview-sanitize-wiring.test.ts` instead, matching mt#4895's shipped pattern that the re-scoped criteria direct this task to follow. `review-worker.test.ts` is already 1832 lines, past the repo's 1500-line error ceiling, so adding ~300 lines would deepen an existing violation. Had Claude do the analysis and the implementation; AI-authored. Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds integration tests for the runReview sanitize wiring from mt#1212 / PR #758. Four end-to-end test cases cover the three SanitizeResult.action values through mock.module() stubs.
Key Changes
Testing
50/50 tests pass. Mutation test confirmed: replacing sanitized.body with output.text caused cases 1 and 2 to fail. Mutation reverted before commit.
Had Claude implement this; AI-authored code.