test(mt#1263): Add runReview sanitize wiring integration tests - #774
Open
minsky-ai[bot] wants to merge 1 commit into
Open
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>
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.