Cover packages/*/test-bun/ in npm run typecheck (Fixes #2995) - #3360
Open
acoliver wants to merge 7 commits into
Open
Cover packages/*/test-bun/ in npm run typecheck (Fixes #2995)#3360acoliver wants to merge 7 commits into
acoliver wants to merge 7 commits into
Conversation
* Harden dependencies and ZIP extraction (Fixes #3324) Replace extract-zip with staged, resource-bounded yauzl extraction that rejects symlinks, traversal, path aliases, and publication collisions. Raise vulnerable direct and transitive dependency floors in both lockfiles and preserve the reviewed CodeQL false-positive dispositions. * Make ZIP collision test portable Assert the exact preexisting directory entry instead of reopening it with alternate casing, which is not valid on case-sensitive Linux filesystems. * Verify streamed ZIP entry sizes * Record security review completion
…etention (#3335 #3339 #3340 #3341) (#3346) * Plan: bound runaway model output across four retention defects (#3335 #3339 #3340 #3341) * Bound aggregate subagent output with a MAX_OUTPUT terminate mode (#3335) A per-response cap does not constrain a loop. Telemetry from the incident shows a subagent reaching turn 253 of the 1000-turn default while emitting the full 16,384-token ceiling on consecutive turns, entirely inside every existing bound. Adds SubagentTerminateMode.MAX_OUTPUT, RunConfig.max_output_tokens_total, and a subagent-max-output-tokens-total ephemeral. Cumulative output is tracked from provider usage metadata, falling back to a character estimate so providers that omit usage cannot make the budget unenforceable. The derived default is clamped: turns times the model output ceiling reproduces the very bound that failed here, so it is capped at 2M aggregate output tokens. * Document the subagent aggregate output budget (#3335) Explains why a turn cap alone does not bound a looping subagent, and why the derived default is clamped rather than left as max_turns times the model output ceiling. * Count generated characters without JSON.stringify, and include reasoning (#3335) The token estimate ran JSON.stringify over every chunk's blocks, adding a per-delta allocation to the exact hot path this change exists to make cheaper, and inflating the count with JSON syntax. Sums text, thinking and tool-call argument lengths directly instead. Reasoning is counted because it is generated output: the profile behind this issue ran high reasoning effort, where reasoning dwarfs visible text, so counting only visible text would leave the budget unenforceable for that exact shape. * Draft the PR body for the runaway-output work (#3335) * Record review findings and open checks for the runaway-output branch (#3335) * Measure the debugResponses cap regression: 5661x slower than a ring buffer (#3339) The landed cap front-splices and rebuilds the streamId index on every chunk once at the cap. At the cap turn.ts actually uses (1024), 200k chunks cost 13.2s and 199M reindex operations versus 2ms for a ring buffer. That converts the memory blowup into a CPU stall, which is a worse failure mode: the runaway hangs rather than crashes, and it costs this on the normal path too. * Amortise the debugResponses trim instead of splicing every chunk (#3339) Trimming on every chunk past the cap costs an O(cap) front-splice plus an O(cap) index rebuild per chunk. At the cap in use that measured 12.2s and 407M operations for 200k chunks, converting the memory blowup this bound exists to prevent into a CPU stall that a runaway would hit as a hang rather than a crash. Letting retention reach twice the cap and dropping a full cap in one batch amortises both to O(1) per chunk: the same stream costs 23ms and 397k operations, 544x faster for 1026x fewer operations, still bounded and still retaining the newest chunks. Sizes the runaway test above the high-water mark so it actually exercises the bound, asserts the absolute bound rather than only relative to stream length, and exports the cap so the test cannot drift from the real value. * Record CLI review findings: control assertions good, bound assertion weak (#3340) * Record provider review: caps sized well, one quadratic byte check (#3341) Three of four byte-accounting sites measure the delta and accumulate, which is correct. The vercel SSE line check re-measures the whole accumulated buffer on every read, making the guard quadratic in exactly the pathological case it exists to catch. Records the O(1) length pre-check remedy. * Make the retention guards scale-invariant and prove it (#3340 #3341) The vercel SSE guard re-measured the whole accumulated buffer on every read, making it O(n^2) in the no-newline case it exists to catch. Adds an O(1) length pre-check: UTF-8 length is bounded by three times String.length, so the cheap comparison settles the common case without scanning. The CLI bound test asserted retention landed under the limit for one fixture size, which would still pass if retention scaled with the stream. Adds a test that doubles the input and asserts retention does not materially move, which is the property that actually matters, and drops an assertion on the fixture's own length. Exports the fence constants so tests bind to real values, and documents why the 512 KiB threshold sits where it does relative to the largest response any catalog model can produce. * Bound provider stream parsers and the pending-response buffer (#3339 #3340 #3341) Provider parsers accumulate untrusted network data, so these get hard byte caps that raise a typed ProviderStreamProtocolError naming the limit: tool-call arguments (OpenAI Responses, OpenAI Chat collector, Anthropic), SSE incomplete-line buffers, reasoning capture, and the qwen text buffer. Caps sit at 8-16 MiB against a ~512 KiB largest legitimate response, so ordinary traffic cannot reach them. The error is non-retryable and non-failover: a malformed stream would be malformed on the next backend too. Replaces OpenAIStreamProcessor.allChunks, which retained every raw SDK chunk so three log lines could read .length, with a counter. Counts the Kimi section tokens incrementally instead of re-matching the whole buffer on every delta, which was quadratic while a section stayed open. Wires an AbortSignal through createReasoningCaptureFetch into the detached vercel reasoning parser, which previously stopped only on end-of-stream or a read error and kept accumulating after cancellation. Forces a size-based split when an unclosed code fence would otherwise pin the pending-response split point, synthesizing the opening fence and language on the retained tail so it renders as a code-block continuation. No Ink or renderer change was needed: MarkdownDisplay already flushes an open code block at end of input, so the committed half was already correct. * Fix two regressions the budget work introduced (#3335) Both were caught by the full suite and confirmed against main, where the same tests pass. The interactive loop ran the whole termination check right after counting a turn's output, which re-evaluated max_turns mid-turn. A subagent on its last allowed turn then stopped before its pending tool calls were handled, so tool results were silently dropped. Only the output budget needs a mid-turn check: it exists to stop a runaway before another request goes out. Splits checkOutputBudget out and calls only that after the turn, leaving the turn and time limits at the top of the loop where they were. Run-config resolution had been moved after runtime assembly so it could read maxOutputTokens off the isolated SettingsService. A launch that failed during assembly then dereferenced a runtime that did not exist, masking the real error. The profile already carries the value, so this reads it from there and restores the original ordering rather than adding a guard for a runtime that should never have been required this early. Adds tests pinning that the mid-turn check ignores an exhausted turn budget, that the top-of-loop check still enforces it, and that the budget itself still trips. * Extract the retention and text-buffer concerns to satisfy lint (#3339 #3341) Both files had grown past the 800-line ceiling and both had a function past the 80-line ceiling, because the bounds were bolted onto classes that were already at their limits. Moves the diagnostic-retention bound, the thinking-block collapse, and the index that makes the collapse cheap out of Turn into TurnDebugResponses, which owns that state rather than leaving Turn to carry the bookkeeping. Turn re-exports MAX_DEBUG_RESPONSE_CHUNKS so existing importers are unaffected. Moves the Kimi section counting and the bounded text append out of OpenAIStreamProcessor into openaiTextBuffer. Drops a dead undefined-check on a retained chunk: the thinking index is rebuilt whenever chunks are dropped, so a recorded location always addresses a live chunk, and the check was both unreachable and the reason the surrounding loop nested four deep. * Record the ripgrep flake as load-induced, with the evidence * Bring the PR body up to date with the decisions made since drafting * Measure the budget against the incident's own telemetry (#3335) 8,138 responses from the affected session: p50 122 output tokens, mean 339, ceiling 16,384. The budget is ~16,000 turns of typical output, so max_turns binds thousands of turns earlier and the budget cannot degrade normal runs. Against the incident it trips at turn 122 versus the 253 actually reached, since every sampled response for the runaway subagent sat at the ceiling. But a runaway made of typical 122-token turns would use 1.5% of the budget over the same 253 turns and would never trip it. Records that limitation rather than implying the budget is a general runaway guard: the retention work is what bounds memory in that case. * Correct the budget docs with measured figures and state what it misses Replaces the estimated 500-token-per-turn figure with the measured distribution (p50 122, mean 339 across 8,138 responses), which puts the budget at ~16,000 turns of ordinary work rather than ~4,000. Adds the limitation: the budget measures volume, so a loop of small responses never approaches it, and the retention limits are what bound memory there. Points at #3344. Documents that reasoning counts, and that the budget is checked mid-turn while the turn and time limits are checked at the start of a turn, so a subagent on its last allowed turn still runs the tool calls it requested. * Record the test-audit gate result: no new findings on touched files * Record the full verification result: suite green apart from the known flake * Probe fence handling across backtick counts and record the blind spot (#3340) Measured retention for an unclosed fence followed by 800 KiB of body, on this branch and on main. 3, 4 and 5 backticks all drop from retaining the whole response to a bounded 64 KiB tail, and the longer fences recover their marker and language correctly. Six or more backticks are not seen as opening a block: scanFence matches exactly three, so the next three read as a close and parity flips straight back. main measures identically, so this is pre-existing rather than introduced. Retention is 0 so there is no leak, but such a block renders as prose. Fixing it means tracking fence run-length instead of parity, which is unrelated to the memory bound this PR is about. * Fix the review findings on the new stream bounds (#3339 #3340 #3341) The detached vercel reasoning parser now throws where it previously swallowed everything, and its promise was stored with no rejection handler. Nothing guarantees the consumer reaches its await: the SDK stream can throw first, the signal can abort, or the generator can go un-iterated. That made an unobserved rejection, which is a process-level crash. The outcome is captured on the buffer and rethrown by the code that awaits it, where a caller exists to handle it. The SSE byte guard only measured the trailing incomplete remainder, so appending a newline bypassed it entirely: the line then arrived complete and went straight to JSON.parse unmeasured. Every line is now checked. Adds a test whose only difference from the existing one is the terminating newline. TurnDebugResponses rebuilt a replaced chunk by spreading the *incoming* chunk, which overwrote the retained chunk's finishReason, usage and metadata with values belonging to a later position in the stream. It now spreads the chunk being replaced. The forced-split guard only avoided landing on a low surrogate. Landing on a high surrogate tears the pair the other way, leaving an unpaired half at the end of the committed text. Both directions are handled. * Extract the SSE line-limit loop to keep nesting under the ceiling * Record the real fence run instead of reconstructing it (#3340) Two review findings had the same root: the scanner matched exactly three backticks and then tried to recover the true fence from the header with a regex. The regex restricted the info string to word characters, but CommonMark allows any run of non-backticks, so c++, objective-c and c# failed to match. On failure the continuation fell back to three backticks with no language, and a literal triple backtick inside the retained tail would then close it early and invert fence parity for the rest of the stream. Header capture also ran before scanFence could defer. When a delta ended on a backtick, scanPos stayed put and the same character was captured again on the next delta, corrupting the header and triggering the same fallback. The scanner now measures the whole backtick run at detection time and defers while the run is still arriving, so only the info string is derived from a pattern. Header capture happens after the consume decision. This also fixes a blind spot that predates this branch: a six-backtick fence read as open-then-close, so the block was never seen as open. Measured, it now bounds like the others, retaining 65,536 rather than the whole response. * Record the real fence run instead of reconstructing it (#3340) Two review findings had the same root: the scanner matched exactly three backticks and then tried to recover the true fence from the header with a regex. The regex restricted the info string to word characters, but CommonMark allows any run of non-backticks, so c++, objective-c and c# failed to match. On failure the continuation fell back to three backticks with no language, and a literal triple backtick inside the retained tail would then close it early and invert fence parity for the rest of the stream. Header capture also ran before scanFence could defer. When a delta ended on a backtick, scanPos stayed put and the same character was captured again on the next delta, corrupting the header and triggering the same fallback. The scanner now measures the whole backtick run at detection time and defers while the run is still arriving, so only the info string is derived from a pattern. Header capture happens after the consume decision. This also fixes a blind spot that predates this branch: a six-backtick fence read as open-then-close, so the block was never seen as open. Measured, it now bounds like the others, retaining 65,536 rather than the whole response. * Close two more bypasses found in review (#3335 #3341) The Responses tool-call cap was enforced on the initial arguments and on each delta, but the terminal event could replace the accumulated value wholesale without a check. A provider that sends the whole payload only in function_call_arguments.done therefore skipped the per-call limit entirely, left bounded only by the much larger SSE line limit. The terminal payload is now measured too. The orchestrator gated the budget on `> 0`, which discarded a deliberate 0 and produced no budget at all: the opposite of what 0 asks for. Only the unlimited sentinel should omit the budget. That sentinel was also written as a bare -1 in two files that have to agree, so it is now a single exported constant with the reasoning attached, and both sites use it. Adds tests that 0 stops immediately, that the sentinel does not enforce even at a billion tokens, and keeps the existing exceeded-budget case. * Move the terminal tool-call guard into the limits module Keeps parseResponsesStream under the file-length ceiling and puts the guard with the other byte limits, where the next parser that needs it will find it. * Close the review findings on bypasses and boundaries (#3335 #3339 #3341) The SSE byte cap was fixed in the vercel parser but not in the OpenAI Responses parser, which had the identical defect: it split on newlines and measured only the unfinished remainder, so a complete oversized line ending in a newline was parsed unmeasured. A probe delivered 8,388,609 bytes in one line without error. Both parsers now share one guard in the limits module, so the next one cannot drift from it. The aggregate budget stopped only once the total exceeded the budget, so a run landing exactly on it was allowed one more request. At the incident's response size that overshoot is a whole extra maximum-length response. It now stops on reaching the budget. Diagnostic retention could lose the newest state of a thinking span. A continued span is replaced at its recorded position and trimming runs immediately after, so when that position sat in the half about to be dropped, the update was written straight into the discarded region while its sibling text survived. Replacement is now skipped when the recorded home is inside the pending drop, and the span is re-appended into the retained window instead. Adds a regression test crossing the trim boundary with a continued span, which the existing tests could not catch because they exercise collapse and trimming separately. * Cover the complete-line SSE bypass that the tests were blind to The existing test only exercised an oversized unfinished line, which was the one branch the guard already checked. Adds the completed-line case, whose only difference is a terminating newline. * Do not synthesize a code fence the renderer never opened (#3340) The scanner toggles fence state on any backtick run, including one inline in prose, while the renderer only honours a fence that begins a line. Before the forced split that mismatch only affected where text was committed. With the split it became visible: the retained tail was reopened with a synthesized fence, so ordinary prose containing inline backticks rendered as a code block. Line-anchoring the scanner itself was the wrong fix. Its split points are required to match the batch helper exactly, and there is a test enforcing that equivalence which the change broke. The scanner keeps its behaviour; it now also records whether the opening run was one the renderer would recognise, and the forced split synthesizes a continuation fence only when it was. An inline run still bounds retention, it just does not reopen a block that never existed. Measured: an inline run in prose retains 65,536 rather than the whole response and no longer gains a synthetic opening. A real fence is unchanged. * Stop miscounting reasoning and zero usage reports (#3335) Both defects push the budget away from the truth in opposite directions. Counting summed every thinking block. Providers that carry a streamId re-emit the entire accumulated thought on each delta, so an N-character thought was counted roughly N squared over two times. On a high-reasoning profile, which is the family the incident came from, that inflates the total enough to stop legitimate work early. Spans with a streamId are now tracked by their latest length and contribute once. Thinking without a streamId is a true increment and still sums, so incremental providers are not undercounted. A usage report of zero was treated as authoritative. Some providers normalise a missing completion count to zero, which made the run stop being counted at all: the one way this accounting can fail open. A zero report alongside output that plainly exists now falls back to the character estimate. An accurate report is still trusted outright. Taking the larger of report and estimate was the first attempt and it was wrong: real tokenizers pack code and JSON far tighter than four characters per token, so the estimate would routinely override a correct provider count and stop runs early. Existing budget tests caught that. * Simplify the usage-report guard to satisfy strict null checks * Record the review findings deliberately left for follow-up * Bring the PR body up to date with what review changed * Record the fake-timer cross-file failures as pre-existing Identical 23 pass / 11 fail on this branch and on main when the two files run together; the term file passes 12/12 alone. * Record that all four suite failures vanish at directory scale 903 pass / 0 fail on this branch and on main for the directory containing every one of them. * Read completion tokens from the neutral event only (#3335) The interactive path read candidatesTokenCount off the UsageMetadata event. That is a Gemini-shaped key, and the agents package is required to stay provider-neutral, which the agents-neutral gate enforces in CI. The Finished event already carries the same figure as neutral UsageStats, so the non-neutral branch is removed rather than special-cased. A provider that reports usage only through the other event now falls back to the character estimate, which is the intended behaviour for an absent report. Caught by CI, not locally: this gate is npm run gate:agents-neutral and is not part of npm run lint. * Fix three defects found in PR review (#3335 #3339 #3340) The interactive path summed cumulative reasoning. The stateful counter added earlier only covered the non-interactive loop, so the interactive one still counted an N-character span about N squared over two times. That inflates the aggregate budget and stops legitimate work on a high-reasoning profile, which is the same failure the non-interactive fix was for. Thoughts carrying a subject are now tracked by latest length; thoughts without one still sum. pendingDropCount disagreed with trim. It guarded on `<` where trim uses `<=`, and added a spurious `+ 1`. At exactly CAP * 2 it claimed 1,025 pending drops while trim discarded none, so tryReplaceThinkingBlock treated live chunks as doomed, dropped valid stream ids from the index, and appended thinking spans instead of replacing them. That defeats the linear-space collapse this class exists to provide. The guard and the arithmetic now match trim exactly. The surrogate guard tore the pair before the one it protected. A high surrogate at the split point already keeps its low partner, because the partner sits at candidate + 1 inside the retained tail. Moving to candidate - 1 pushes the boundary into the previous pair whenever that character is a low surrogate, splitting it and rendering a replacement character on both sides. Handling both halves was the wrong instinct; only the low-surrogate case needs adjusting. Not changed: the claim that the detached parser rejection is swallowed. The handler awaits parsePromise and rethrows captureBuffer.parseError immediately after, which is the surfacing path that finding asks for. * Close two more budget bypasses found in review (#3335 #3339) A non-finite explicit budget disabled enforcement while looking configured. checkOutputBudget tests total >= budget, which is false for both Infinity and NaN, and Math.floor followed by Math.max(0, ...) preserves both. Either value therefore turned the aggregate budget off completely. Non-finite input now falls back to the derived default, so a bad explicit value cannot be more permissive than supplying none. Code blocks were not counted. The counter handled text, thinking and tool calls, but code is model-generated and a runaway can emit it exclusively, which would have skipped the budget entirely. Tool responses and media stay excluded on purpose: they carry tool results and inputs the model did not produce, and charging a large tool result against a budget meant to stop runaway generation would stop healthy runs. That reasoning is now recorded next to the code. Also tied the retention test's stream length to MAX_DEBUG_RESPONSE_CHUNKS rather than a hardcoded 3000, so raising the constant cannot silently stop the test exercising the trim path, and covered the empty-report-with-empty-output case. * Cancel the abandoned tee branch instead of just unlocking it (#3341) The reasoning parser reads one branch of a tee. Releasing its reader without cancelling leaves that branch live, so as the SDK drains the other branch the tee queues every subsequent chunk for the abandoned one. On the byte-limit path that retains the remainder of the response, which is the opposite of what the limit exists to do: the guard would bound the parser's own buffer and then leak the same data somewhere else. Cleanup order also changed. finalized is set first so a consumer waiting on it is released even if a later cleanup step throws. Also corrected two comments in openaiTextBuffer: appendBufferedText only appends, it does not emit, and the invariant that textBufferBytes must track textBuffer exactly is now stated where someone changing the buffer will see it. Both were wrong in ways that invite a future change to bypass the cap. * Stop discarding tool calls from the last allowed turn (#3335) The non-interactive loop re-checked the turn and time limits between receiving a response and dispatching its tool calls. A subagent on its final allowed turn would emit tool calls and have them silently thrown away. The interactive loop was corrected earlier in this branch and the two had been left disagreeing about when a run ends, which is worse than either behaviour on its own. Both now re-check only the output budget at that point. The loop head still enforces turn and time, so this costs one dispatch and keeps the result instead of discarding it. Both reviewers raised this independently. It was recorded as pre-existing, which was true and beside the point: fixing one path and not the other made the inconsistency mine. The test drives the real non-interactive loop with max_turns 1 and a self_emitvalue call on that turn, then asserts the emitted variable arrived. Verified against the previous implementation, where it fails. * Do not trade the tee leak for a deadlock (#3341 #3339) Awaiting the tee-branch cancellation was wrong. cancel() on one branch can stay pending while the sibling is still open, and the SDK may leave its stream open after an abort, so awaiting here would keep parsePromise pending and hang vercelStreamHandler, which awaits it. Releasing the lock is what has to happen synchronously; the cancellation lands whenever the tee allows. This is the same mistake as the earlier retention cap, which stopped a crash by introducing a hang: a bound that blocks is not a bound. pendingDropCount also has to project the appended chunk. The replacement runs before push appends, so judging pending drops from the current length is one short: at exactly the high-water mark it reported nothing doomed, the append tipped the total over, and trim discarded the chunk that had just been updated in place, losing the newest thinking value. No regression test accompanies the second fix. The one I wrote failed against both the old and the new arithmetic, so it was evidence of nothing, and a test that looks like proof without being proof is worse than none. It is removed and the gap is recorded in REVIEW-NOTES. * Cover the trim-boundary case properly (#3339) The earlier attempt at this test failed against both the old and the new arithmetic, so it was removed and the gap recorded rather than shipped as false evidence. The cause was mine: it drove the whole Turn, where the chunk shape did not reach the code under test the way I assumed. Driving TurnDebugResponses directly makes the boundary exact. At the high-water mark nothing is doomed yet, and the final chunk carries both a continuation and a sibling, so the sibling is appended and trim runs inside the same push. Measured both ways: previous arithmetic thoughts retained = [] projected arithmetic thoughts retained = ["NEWEST"] The previous code did not merely mislocate the thought, it lost it entirely. The test fails against that implementation, so it pins the fix. The coverage-gap note is removed because the gap is closed. * Bound retained tool-call fragments, not just their bytes (#3341) A byte budget does not bound object count. Empty and one-byte deltas cost almost nothing against 16 MiB while each still costs a retained object and lengthens the duplicate scan, which is linear per fragment. A peer emitting them indefinitely grows memory and CPU without ever tripping the byte cap, which is the same shape as the original incident: a limit that the pathological case walks straight past. Two changes. A fragment carrying no identity and no payload is not stored at all; that is not a limit, just refusing to record noise. And retained fragments per call are capped at 500,000, reported through the same error type as the byte limits so callers cannot tell them apart. Streaming a maximum-length tool call one token at a time is on the order of 128,000 fragments, so the cap sits far above any legitimate call and only catches the degenerate case. Coalescing adjacent fragments, the other half of the review finding, is not done here: it changes how calls are reconstructed and deserves its own change. The count bound removes the unbounded growth either way. Directory-scale run measured with and without this change: 2,084 failures both times, so the providers isolation issue is unrelated. Pass count moves 4348 to 4352, which is these four tests.
…sues (Fixes #3064) (#3352) * Stamp milestone, ci/cd label, and Bug type on auto-created failure issues (Fixes #3064) Five workflows open an issue when something fails, and they stamped triage metadata inconsistently, so the issues fell out of the release view and had to be filtered by hand. Only nightly and evals-nightly carried a milestone (from #3149); release and the OCR infrastructure notifier carried just the ci/cd label; smoke-test carried nothing at all. None of the five set an issue type. Every site now creates its issue with the ci/cd label, the open milestone whose title matches the version in main's package.json, and issue type Bug. Where a workflow reuses a long-lived tracking issue instead of creating one (nightly, evals-nightly, the OCR notifier), it re-applies milestone and type to that issue rather than only stamping on first creation. gh issue create has no --type flag, so the type is applied over REST after creation via `gh api -X PATCH repos/OWNER/REPO/issues/N -f type=Bug`, parsing the issue number out of the URL create prints. apply_issue_type always returns 0 and warns on failure: the nightly notifier deliberately bans both `| true` and `|| true`, and metadata must never sink a failure notification. The milestone and type helpers are inlined per workflow rather than sourced from a shared script. Three of these notify jobs have no checkout step, which is why the existing resolve_milestone reads package.json over the API instead of from disk; sharing would mean adding five pinned sparse checkouts plus a contents: read grant to the privileged workflow_run-triggered OCR notifier. The function bodies are byte-identical across all five sites. Guard every array expansion with the `${ARR[@]+"${ARR[@]}"}` alternate form. Expanding an empty array as "${ARR[@]}" under `set -u` is an unbound-variable error on bash 3.2, still the /bin/bash on macOS, so the milestone fail-soft path shipped in #3149 aborted the notifier there. It survived only because CI runs bash 5. Tests execute the real `run:` script extracted from each workflow against a stateful fake gh on PATH, asserting recorded argv and fake-API state rather than workflow source text. Sixty cases cover, per site, the happy path, milestone resolution boundaries (no match, fetch failure, missing version, exact-title-only, second-page pagination), type-application boundaries (failed PATCH, unparseable create output), and the recurring-issue path. The nightly shell scanner learns the guarded expansion so its repository- targeting assertions keep covering the guarded call sites instead of silently degrading, with tests for both the resolving and fail-closed cases. Verification: typecheck, lint, build, actionlint with CI's ignore set, and the affected test files are green. The batch of workflow-related suites shows an identical 101 failures / 2 errors before and after this change; those are a pre-existing cross-file interference in the repo suite. Plan in project-plans/issue3064-auto-issue-metadata.md. * Fix milestone resolution, evals reuse path, and test-harness fidelity (Refs #3064) Review of the first commit found the milestone resolver could never work. Every site ran 'gh api --paginate --slurp ... --jq ...', which gh rejects outright: 'the --slurp option is not supported with --jq or --template'. The resolver fails soft, so all five workflows carried on and created their issue with no milestone at all. That defect shipped in #3149 and this branch had copied it to three more workflows. Resolution now pipes the slurped pages to a real jq and passes the version with --arg, so the value no longer has to survive a round of shell quoting. Verified live against this repository. The evals notifier could also reuse an issue without stamping it. Its inner create_issue_once race check returns the number of an issue that appeared after the outer search, in which case CREATE_ARGS never applied. Both reuse paths now go through one annotate_issue helper, which validates the reference before calling the API so an unparseable one cannot burn four retries and three sleeps. The tests passed against all of this, so the harness was the larger problem. The fake gh now models the option validation real gh performs (--slurp with --jq, and --slurp without --paginate, both rejected with gh's exit status), emits the real --slurp page-of-pages shape instead of running jq itself, requires a title and a body on issue create, records the PATCH fields, and accepts an issue URL wherever gh accepts one. The harness substitutes GitHub expressions in the run body rather than only in env, throws on an expression it does not model instead of silently yielding an empty string, and invokes bash with the runner's actual flags per the step's shell. Assertions now require the whole PATCH contract rather than just the issue number. Two mutation checks confirm the suite is no longer vacuous: reintroducing the --slurp/--jq combination fails 5 tests, and changing type=Bug to type=Task fails 2. Both previously passed. A new test compiles the embedded Python so an escaping slip in the fake surfaces directly instead of as a retried gh error. 251 tests pass across the new suite and every modified pre-existing suite; typecheck, eslint, prettier, YAML parse and actionlint with CI's ignore set are clean. * Guarantee the ci/cd label and harden the fake gh (Refs #3064) Open code review round one findings. smoke-test.yml and release.yml passed --label ci/cd to gh issue create without first guaranteeing the label exists. gh hard-fails an unknown label, and both steps run under the runner's default bash -e, so a missing ci/cd label would have lost the entire failure notification and skipped the type stamp with it. Both now use the same ensure_label helper as nightly and evals-nightly, which creates the label or verifies it and degrades to an unlabelled issue rather than to no issue. All five notifiers now share one label strategy: create-or- verify in four, create-then-retry-without-labels in the OCR notifier. The fake gh returned an empty object and exit 0 for any endpoint or subcommand it did not model, so a workflow could call something unmodelled and still go green. Unmodelled api endpoints, issue subcommands, label subcommands and top-level commands now fail loudly; gh label create is modelled properly, including the already-exists failure ensure_label depends on. The harness now asserts python3 and jq are present and names the missing one, rather than surfacing their absence as a blank assertion after the notifier retried past a non-zero gh. Spawn errors are folded into stderr instead of being flattened to status 1 with no output. The || fallback resolver folds over every operand instead of destructuring the first two and dropping the tail of a || b || c. evals-nightly's annotate_issue said it was skipping the type when it was also skipping the milestone. The OCR notifier's annotate_existing_issue now wraps its issue edit in retry_gh like every other issue mutation in these scripts. resolveExpression moved its fixed context lookups into a table and split out the fallback chain, bringing complexity back under the limit rather than raising the threshold. Rejected: the suggestion to share the helpers via a script fetched at runtime for release.yml and smoke-test.yml. Those two do have checkouts, but the other three notifier jobs do not, so it would leave two divergent mechanisms for the same logic instead of one duplicated one. Mutation checks re-run after the refactor: mutating smoke-test to type=Task and to the rejected --slurp/--jq form fails 5 tests. 303 tests pass across the new suite and every modified pre-existing suite; typecheck, eslint, prettier, actionlint with CI's ignore set, and the build are clean. * Record the review outcomes and the corrected milestone resolution in the plan (Refs #3064) * Close the duplicate-issue window and harden release interpolation (Refs #3064) PR review round: CodeRabbit and the PR-side open code review. nightly.yml retried gh issue create directly. Creation is not idempotent, so a request that reached GitHub but reported failure to the client would have the retry open a second 'Nightly workflow failed' issue. It now uses the same create_issue_once guard evals-nightly already had, which re-searches for the title before each attempt. Both nightly reuse paths -- the outer search and the inner race check -- now run through one annotate_issue helper, so an issue that won the race still receives the milestone CREATE_ARGS never applied to it. release.yml interpolated GitHub expressions straight into the run body. They now travel through env, so a value carrying shell metacharacters cannot alter the script. smoke-test.yml took REF from github.event.inputs.ref alone, which is empty on push runs and titled the issue 'Smoke test failed on @ <date>' with no revision. It now uses the same || github.sha fallback as the checkout step. The fake gh accepted gh label create and returned success without recording the label, so a later label list would not see it. It now persists, and a new issuesVisibleAfterListCalls fixture models an issue opened by a concurrent run: invisible to the first N searches, visible afterwards. That drives a new test, registered only for the two sites that actually guard the window, asserting no duplicate create and that the winning issue still gets milestone and type. Removing the guard from nightly fails it. release-process-b.test.ts asserted retry_gh gh issue create. Updated to the guarded form rather than dropped: it now requires retry_gh create_issue_once, requires the inner gh issue create, and forbids the unguarded retry. 305 tests pass across the new suite and every modified pre-existing suite, 3 skipped for sites without a race guard; typecheck, eslint, prettier, YAML parse, actionlint with CI's ignore set, and the build are clean. * Fail closed when the duplicate lookup itself fails (Refs #3064) CodeRabbit follow-up on the create_issue_once guard. The lookup swallowed its own failure with '|| found=""', so a failed gh issue list was indistinguishable from a confirmed absence and fell through to create. That reopens exactly the duplicate window the guard exists to close: if a create reached GitHub but reported failure, and the recheck then errored, the retry would open a second issue. The lookup now returns failure instead, so retry_gh rechecks before creating. Exhausting the attempts aborts without creating, which matches the policy the outer search already applies ('aborting to avoid duplicates'): a missed notification is preferable to duplicate tracking issues. Applied to evals-nightly as well, which had the same swallow. The guard bodies stay byte-identical between the two workflows. Covered by a new test, registered only for the two guarded sites, driving a new issue-list failure fixture in the fake gh: no create is attempted and the step exits non-zero. It carries an explicit 30s budget because retry_gh burns three 5s sleeps exhausting its attempts. 307 tests pass across the new suite and every modified pre-existing suite, 6 skipped for sites without a race guard; typecheck, eslint, prettier, YAML parse, actionlint with CI's ignore set, and the build are clean. * Scope the fake gh label-list failure key to label listing (Refs #3064) CodeRabbit follow-up. The issue-list failure knob added in the previous commit was applied to both handle_issue and handle_label, because a blind string replace matched the identical list-branch head in each. A failOn rule for issue listing therefore also failed label listing, and no rule could target label listing on its own, so the two failure paths could not be isolated in a test. The label branch now keys off label/list with its own message.
Bun-native test files under packages/*/test-bun/ were invisible to `npm run typecheck`: every package tsconfig included only index.ts and src/**, so a production interface change would surface as a late, low-signal runtime failure instead of a type error. Each package that owns a test-bun/ directory now has a dedicated tsconfig.test-bun.json child project (extends the package tsconfig, noEmit, incremental off; storage drops composite, cli adds ESNext.Disposable to lib) chained fail-fast into the package's typecheck script. The root typecheck already fans out through workspaces, so the build tsconfigs stay untouched. Turning the checker on surfaced latent errors in never-typechecked fixtures; all are fixed without changing runtime behavior: stale getAuthToken stubs removed against the RuntimeProvider contract, a generateContentStream IIFE matching the declared async-generator return shape, content-part discriminants, the contract-documented -1 "no ceiling" timeout value, mock/closure-narrowing casts, and Bun-global access through globalThis-typed local interfaces per the jspBootstrapStartup precedent. bun-test-corrections.d.ts gains the missing expect.fail declaration (present in the runtime, absent from bun-types@1.3.14). A guard test in scripts/tests/ pins the wiring: every packages/*/ test-bun file must be covered by its package's include globs and fail-fast typecheck chain, so new gaps cannot reopen silently.
github-actions
Bot
added
the
maintainer:e2e:ok
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughAdded dedicated Bun test typecheck projects across packages, corrected Bun test declarations and fixtures, typed CLI Bun runtime access, and added repository-wide coverage checks for package and root typecheck wiring. ChangesBun test typecheck coverage
Estimated code review effort: 3 (Moderate) | ~30 minutes Merge Risk: ⚪ Minimal · up to This change adds Bun test files to package typechecking with compile-time-only corrections and a guard test to preserve the wiring; reported checks are green, so no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
WalkthroughBefore this PR, Release Notes
Changes
Magnitude🎯 4 (XL) Related
Pre-merge Checks
Walkthrough generated by LLxprt PR Review. Planner issue: #2256 |
Contributor
OpenCodeReview — automatic reviews suspendedAutomatic OCR reviews are suspended for this PR after 2 of 2 automatic reviews. To get more reviews you can:
OpenCodeReview — PR #3360
|
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.
TLDR
npm run typechecknever saw the Bun-native test files underpackages/*/test-bun/because every package tsconfig included onlyindex.tsandsrc/**. This PR gives each of the five packages that own atest-bun/directory (agents, cli, providers, storage, tools) a dedicatedtsconfig.test-bun.jsonchild project, chained fail-fast into the package'stypecheckscript — the issue's Option 2, so the build tsconfigs stay untouched. Turning the checker on surfaced a handful of latent type errors in those never-typechecked fixtures; all are fixed with compile-time-only changes (verified runtime-inert), and a guard test pins the wiring so the gap cannot reopen silently.Dive Deeper
Config shape (per package, extending
./tsconfig.json):noEmit,incremental: false(avoidstsBuildInfoFilecollision with the main pass),include: ["../../bun-test-corrections.d.ts", "test-bun/**/*.ts"]. Two deliberate overrides: storage setscomposite: false(its parent is composite); cli addsESNext.Disposabletolibbecause a nested@types/node@20hoisting artifact ships aSymbol.asyncDisposepolyfill that conflicts with core'sasyncIterator.tswhen the test-bun program (which, unlike cli's main program, pulls that file in transitively) resolves the older types. agents re-declarestypes(node,bun-types/test,bun-types/test-globals) because its parent loadsbun:testtypes via an include entry the child's include replaces. Full bun-types is intentionally NOT loaded anywhere: it overrides globalfetchand breaks transitively-checked production code.Script wiring: each package's script is now
tsc --noEmit && tsc --noEmit -p tsconfig.test-bun.json— fail-fast&&, no||. The roottypecheckalready fans out throughtypecheck --workspaces --if-present, so no rootpackage.jsonchange and no central list to maintain.Latent-error fixes (all runtime-inert, per-file rationale in comments):
agents/generatingModelStamp.issue2511.bun.ts: dropped stalegetAuthTokenstubs (not on theRuntimeProvidercontract, never invoked);generateContentStreamas an async IIFE matching the declaredPromise<AsyncGenerator>shape (never called — non-streaming path); content parts get thetype: 'text'discriminant (dispatches to the branch that produces a deeply-equal block).agents/subagentAnthropicTextSettings.issue1738.bun.ts: the fixtures deliberately feed out-of-type ephemeral settings to pin the runtime classifier's tolerance (anthropic bork during subagentic execution #1738); typed viaas unknown as ProfileEphemeralSettingsso the intent is explicit instead of an inference error.cli/profileAuthKeyNameIssue2916.bun.ts:fileURLToPath(import.meta.url)instead ofimport.meta.dir, andBun.spawn/sleep/servereached throughglobalThis-typed local minimal interfaces — the established repo pattern (packages/cli/src/observation/jspBootstrapStartup.test.ts) since package configs deliberately loadbun-types/testbut not the globalBunnamespace. Only observable delta: a fail-fast module-load throw if the Bun global is absent (unreachable underbun test).cli/sandbox-env.bun.ts:readFileSyncmock implementation cast to the real overload shape.cli/steerKey.{darwin,win32}.bun.ts:expect(x as string | null)widening at the assertion — control-flow analysis cannot see that the capturing callback ran (precedent:AppContainer.cancel-race.test.tsx).tools/shell-tool-signal-format.bun.ts:timeoutSeconds: -1(the contract-documented "no ceiling" value) instead ofundefined; both spellings resolve identically throughresolveTimeoutand the field is not exercised by the signal-format assertions.bun-test-corrections.d.ts: gains the missingexpect.faildeclaration (present in the Bun runtime, absent from bun-types@1.3.14; one call site instorage/test-bun/secure-store.runtime-replaced.bun.ts). This is the sanctioned ambient-corrections file for exactly this kind of gap.Guard test (
scripts/tests/test-bun-typecheck-coverage.bun.test.ts, registered intsconfig.scripts.json): discovers everypackages/*/test-bundirectory recursively, asserts each package has a child config extending./tsconfig.jsonwhose include globs cover every discovered file, asserts the fail-fast&& tsc --noEmit -p tsconfig.test-bun.jsonchaining (rejects;/||weakenings), and asserts the root script still fans out via workspaces. Runs in CI via thescripts/testsshard.Known follow-ups (deferred, out of scope here): the guard checks
includebut not inheritedexclude(no base exclude matchestest-bun/**today);expect.failshould be dropped from the corrections file once bun-types ships it.Reviewer Test Plan
npm run typecheck— all five-p tsconfig.test-bun.jsonpasses execute and exit 0.const __probe: number = 'x';to anypackages/*/test-bun/*.tsfile and re-runnpm run typecheck— it must fail; remove it and it must pass again (this was run locally for one file per package, all five caught).bun test ./scripts/tests/test-bun-typecheck-coverage.bun.test.ts— 12/12 pass. Temporarily rename anypackages/*/tsconfig.test-bun.jsonor drop the-pfrom a package script and the guard fails.npm run test— full suite green (edited suites unchanged: CLI 716/716 files, 9216 pass / 0 fail).cd packages/cli && npx tsc --noEmit -p tsconfig.test-bun.json --listFilesOnly | grep test-bunshows every fixture in the program.Testing Matrix
macOS (darwin-arm64) local verification: full
npm run test(exit 0),npm run lint(exit 0),npm run typecheck(exit 0, both passes per package confirmed in the log),npm run format(exit 0),npm run build(exit 0), guard test 12/12, error-injection probes 5/5.steerKey.win32.bun.tscannot execute in this checkout due to a pre-existing@ast-grep/napinative-binding resolution failure (verified identical on the pristine file via stash-test; passes inside the full CLI runner and is unchanged at runtime by this PR). The stepfun-37 smoke test reached the remote API and failed with a deterministic upstream400 you have no active step plan subscriptionafter successful CLI startup — an account condition unrelated to this diff; noting it here for transparency.Linked issues / bugs
Fixes #2995
Related to #2951 / #2992 (whose review surfaced the gap)
Summary by CodeRabbit
Tests
Chores