Conversation
The nightly cli_bundle_launch job has never passed. It called Bun.build in-process from bun test, where the bundler does not rewrite ./src/cli.js to src/cli.tsx, so it could not resolve the CLI entry own import. The same build run as the prepack subprocess resolves it, so build the way npm pack actually builds. Spawn the Bun running the test rather than node_modules/bun/bin/bun: npm 11.16 no longer runs install scripts, so the bun postinstall never materialises that binary in CI. Render the diagnostics Bun attaches to a failed build. Bun.build rejects with an AggregateError whose own message is the constant "Bundle failed"; reporting only that left the nightly failure undiagnosable. Refs #3061
The nightly has failed every night since 2026-08-01 across four independent jobs. This addresses the remaining three. windows_ci carried six portability defects, all of which asserted POSIX behaviour on a platform where the production code deliberately behaves differently: - the credential-write-lock suite shelled out to `ps -o lstart=` for every platform, though production gates that probe to darwin/linux/freebsd and returns null elsewhere. It now mirrors the gate and fabricates an approximate owner record where no canonical source exists, so the test still runs on Windows rather than being skipped. Its M4 sabotage also moved to replacing the lock file with a directory: the old "remove the dir, write a file" trick raises ENOTDIR on POSIX but ENOENT on Windows, where readOwner reads it as "absent" and logs nothing, silently hiding the release failure the test exists to catch. - toolRegistryFactory compared a path built from os.tmpdir() (an 8.3 short form on the runner) against the tool's resolved long path; the workspace root is now realpath'd so both sides speak the same path. - getPty's vi.doMock was bypassed on Windows, where node-pty is a real installed native addon, so the assertions received the genuine module. File-level vi.mock intercepts at module-graph build time instead. - process-run asserted an ENOENT message; Bun on Windows says "Executable not found in $PATH" for the same condition. - the quota-guard integration tests spawned their nested vitest run with process.execPath, which is Bun here. Vitest is a Node tool whose forks pool assumes a Node runtime, so the child died on Windows and the run published no sentinel. The child is now pinned to Node, and the assertions report the child's own output so a nested run that dies for an unrelated reason is no longer indistinguishable from one that failed for the intended reason. Dispatching the nightly on this branch also surfaced a regression that landed after the failing run: the shell timeout-ceiling suite builds a background invocation unconditionally, but ShellTool rejects is_background on win32 outright, so build() threw before the clamp behaviour was reached. credential-proxy-server.stop() destroyed each connection abruptly. A Windows named-pipe peer does not reliably observe that as a close, so the client kept believing it was connected and its next request hung to the timeout instead of reporting the loss. It now half-closes with the existing endAndDestroyAfter helper, which sends EOF before force-destroying. macos_ci failed on a race in the OCR canary: the monitor forwarded the upstream 200 correctly (every telemetry assertion passed), but the client's response callback and the request-level reset competed for one first-wins resolve. The status is now captured when the response object is delivered and a later reset cannot overwrite it. e2e_full (windows) failed because the rig workspace lives under .integration-tests/, which the repo root .gitignore excludes, so file discovery resolved the enclosing repo and hid every workspace file from the agent — "Listed 0 item(s). (5 ignored)". The model then refused to run a script it could not see. The workspace is now its own git root. Fixes #3061
The second branch dispatch (run 31039119968) turned E2E Windows, the CLI bundle and every other leg green, and narrowed the rest to two things. windows_ci: only getPty remained. The previous attempt was aimed at the wrong runner. packages/core runs exclusively under Bun, and getPty short-circuits to the Bun.Terminal adapter on Bun POSIX, so these three cases only ever EXECUTE on Windows — the one platform where substituting the backend in the module registry silently fails, because Bun's runner cannot reset the registry and evaluates a module-mock factory once, eagerly. That is why they passed everywhere a developer would look and failed only in CI. getPty now takes injectable backend loaders, so the selection logic is driven the same way on every runtime and the cases run locally under Vitest too. macos_ci: with the canary race fixed, the job reached a step that failure had been masking. "Run scripts-assignment Bun manifest" invokes a workspace that #3014 deleted from the Bun test manifest while leaving the step behind, so it could never pass. Both of its test files are already covered by the scripts-tests glob root and ran green in the same job's harness step, so the dead step goes rather than the workspace coming back. From review: - scripts/bun-build.config.ts rendered a diagnostic's source position only for instanceof Error values, but Bun attaches BuildMessage/ResolveMessage objects, which are not Errors — so the file/line/column was always thrown away. Confirmed by probing Bun 1.3.14 directly. - The CLI bundle test's artifact check was an opaque existsSync assertion even though the build can exit 0 having built nothing, and its process renderer printed the literal "null" for the streams spawnSync nulls out on a spawn failure — both precisely when the diagnostic matters most. - The nested-vitest launcher accepted node.cmd, which spawnSync cannot execute without shell:true; on a Windows box where a .cmd shim shadows node.exe the child would have failed to launch at all. - The credential-proxy shutdown comment claimed a Windows named-pipe half-close that libuv does not implement. What actually fixes it is ordering: server.close() waits for the connection count to reach zero, so stop() can no longer return before the peer observes the close. The socket is also paused before ending, since end() leaves the read side open and an in-flight frame would otherwise still mutate the token store mid-shutdown, and endAndDestroyAfter now guards writableEnded against a double end(). - The canonical start-time platform list is exported from the credential lock and imported by its tests, so the mirror cannot drift. - Windows gained the complement of the is_background gate: it now asserts the rejection rather than merely skipping the case. Fixes #3061
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (19)
📝 WalkthroughWalkthroughThe PR fixes cross-platform test execution, makes PTY backends injectable, improves credential proxy and lock lifecycle handling, preserves Bun build diagnostics, and stabilizes OCR failure assertions. ChangesCross-platform reliability fixes
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
the
maintainer:e2e:ok
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/bun-build.config.ts`:
- Around line 193-203: Update renderDiagnostic to read the diagnostic.message
property for object diagnostics and return that string when available, while
preserving the existing position suffix formatting. Retain String(diagnostic) as
the fallback when message is absent or not a string, and keep primitive/null
handling unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 85936146-4c19-4a3b-bd47-f4b78a4e7c89
⛔ Files ignored due to path filters (1)
project-plans/issue-3061-nightly-green.mdis excluded by!project-plans/**
📒 Files selected for processing (14)
.github/workflows/nightly.ymlintegration-tests/run_shell_command.windows.test.tspackages/core/src/config/toolRegistryFactory.test.tspackages/core/src/utils/getPty.test.tspackages/core/src/utils/getPty.tspackages/providers/src/auth/proxy/credential-proxy-server.tspackages/storage/src/secure-store/credential-write-lock.tspackages/storage/test-bun/credential-write-lock.bun.tspackages/test-utils/src/process-run.test.tspackages/test-utils/src/quota-guard-vitest-integration.test.tspackages/tools/src/__tests__/shell-timeout-bounds.test.tsscripts/bun-build.config.tsscripts/tests/issue-2999-cli-bundle.bun.test.tsscripts/tests/ocr-concurrency-canary-2673.test.ts
💤 Files with no reviewable changes (1)
- .github/workflows/nightly.yml
# Conflicts: # scripts/tests/ocr-concurrency-canary-2673.test.ts
Contributor
WalkthroughThis PR changes 28 file(s).
Changes
Magnitude🎯 3 (L) RelatedNo related items found. Walkthrough generated by LLxprt PR Review. Planner issue: #2256 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
OpenCodeReview — automatic reviews suspendedAutomatic OCR reviews are suspended for this PR after 2 of 2 automatic reviews. To get more reviews you can:
|
main moved substantially during the nightly repair and two of this branch's changes were affected directly: - #3078 implemented real Windows background shell jobs via Start-Process and removed ShellTool's win32 rejection. The timeout-clamp test therefore no longer needs a Windows gate (or the complementary rejection assertion added from review); it runs on every platform again. - #3080 independently fixed half of the OCR canary race by retaining the status observed at the response callback. Taking main's version is correct, but it is not sufficient: reverting this branch's 50ms upstream-crash delay on top of it still failed 4 of 8 runs because the reset can preempt the HTTP parser before any response object exists. Keeping the delay passed 10 of 10. The merge also confirms #3080's CLI asset-staging work composes with the publish-path bundle test and build diagnostics here. Address all three open review threads: - prefer Bun's explicit diagnostic.message when present instead of relying on a custom diagnostic object's toString(), retaining String() as fallback; - surface git-init status/stderr in the Windows E2E instead of silencing the command that establishes the workspace boundary; - strip syntactic quotes from Windows PATH entries before looking for node.exe. Refs #3061
The merged-head nightly turned the last failures into deterministic evidence: - Windows PowerShell 5.1 parsed the BOM-less UTF-8 script as CP932, corrupting its Japanese filename literal before the shell command even ran. Give the script a UTF-8 BOM. - Config's missing client-factory contract was checked only after parser and registry startup, allowing a simple dependency error to exceed the Windows test budget. Validate the required factory before initialization side effects. - Windows log-cap enforcement killed the child before recording why. The exit event could therefore finalize the job first, as failed but without a reason. Claim a capping phase around taskkill so cap enforcement owns that transition. - Graceful proxy shutdown waited for the three-second force timer on every cooperative connection because a peer can keep its write half open after our FIN. Destroy immediately after the FIN is flushed and retain the timer solely as a fallback. These fixes preserve the behavioral assertions rather than widening timeouts or weakening platform coverage. Refs #3061
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-24.x-ubuntu-latest' artifact from the main CI run. |
Windows named-pipe close notification can arrive one event-loop turn after the server's close callback. Yield that turn before stop resolves so a client cannot start a request using stale connected state and wait for the request timeout. Refs #3061
Windows surfaced two more Unix assumptions in the full nightly suite: - realpath reports ENOENT when an existing parent component is a file. Walk the parent chain so the trust dialog still distinguishes a malformed path from a genuinely missing folder. - fchmod is not supported for these Windows file handles. The capability transport is a Docker/Podman path whose Windows host permissions are not Unix modes, so retain the restrictive create modes and explicit fchmod enforcement on POSIX only. Refs #3061
10 tasks
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
The Nightly Tests workflow has failed every night since 2026-08-01 (last green
2026-07-31). Issue #3061 reports the run in which four jobs failed at once:
cli_bundle_launch,windows_ci,macos_ciande2e_fullon Windows. Theyare four independent problems, not one. This makes the nightly pass.
Most changes align tests and harnesses with the production platform contract.
The production changes are narrowly tied to failures the full nightly exposed:
credential-proxy shutdown ordering, fail-fast dependency validation, atomic
Windows shell-job cap transitions, Windows trust-path classification, and
capability-file handling on Windows hosts.
The thing worth a reviewer's attention: the overriding rule for this work was
that no test may be neutered to make CI green. The only platform gates mirror real production gates, and the one broadened error
matcher accepts Bun's Windows spelling while preserving its behavioral checks.
Windows also gained coverage it did not have before: the credential-lock M4 test
was silently not exercising its release-failure path there.
Dive Deeper
1.
cli_bundle_launch— a job that had never passed. Added by #3013, whichmerged after the 2026-08-04 nightly, so 2026-08-05 was its first run. It called
Bun.buildin-process frombun test, where Bun's bundler does not rewrite./src/cli.jstosrc/cli.tsx, so it could not resolve the CLI entry's ownimport. The same build run as a subprocess —
bun scripts/bun-build.config.ts --cli-only,which is literally what
packages/cli'sprepackruns at publish time —succeeds, so the test now builds the way
npm packbuilds.Two further defects in the same job: the test spawned
node_modules/bun/bin/bun,which npm 11.16 never materialises now that it stops running install scripts by
default; and the failure was undiagnosable —
Bun.buildrejects with anAggregateErrorwhose own message is the constant stringBundle failed, andboth the test and the build script reported only
error.message. That is whythe root cause could not be read off the nightly log at all and had to be
reproduced locally. Diagnostics now render the aggregate's contents, the build
logs, and subprocess status/signal/stdout/stderr.
2.
windows_ci— portability defects and platform races.ps -o lstart=invoked on aplatform whose production counterpart is gated to darwin/linux/freebsd; a path
built from
os.tmpdir()(an 8.3 short form on the runner) compared against aresolved long path; a backend substituted in the module registry by a technique
the runner cannot honour; a nested
vitestrun launched withprocess.execPath,which is Bun here, though vitest's forks pool assumes Node; an
ENOENTmessageassertion where Bun on Windows says
Executable not found in $PATH; a locksabotage that produced ENOENT rather than a non-ENOENT error on Windows, so the
release-failure path under test was never reached there; and Windows-specific shutdown/cap/filesystem semantics. Main moved during the
work: #3078 added real Windows background jobs, so this PR discarded its
superseded temporary rejection test and retains main's implementation.
3.
macos_ci— a race, and then a step the race was hiding. The OCR canary'smonitor behaved correctly (every telemetry assertion passed); only the client
harness raced, letting a post-header connection reset overwrite an
already-observed 200. With that fixed the job reached
Run scripts-assignment Bun manifest, which invokes a workspace #3014 deletedfrom
scripts/bun-test-manifest.tswhile leaving the step behind — it couldnever have passed. Both of its test files are already covered by the
scripts-testsglob root and ran green in the same job's harness step, so thedead step is removed rather than the workspace restored.
4.
e2e_full(windows) — an agent that could not see its own workspace. Thetranscript's decisive line is
Listed 0 item(s). (5 ignored): the rig workspacelives under
.integration-tests/, which the repo root.gitignoreexcludes, sofile discovery resolved the enclosing repository and applied its ignore rules to
the workspace. The model reasonably refused to run a script it believed did not
exist. The workspace is now its own git root.
Reviews. An independent review caught that the first
getPtyfix could notwork — it was aimed at Vitest, but
packages/coreruns exclusively under Bun,and because
getPtyshort-circuits on Bun POSIX those three cases only everexecute on Windows, the one platform where registry substitution silently
fails. The branch nightly then confirmed exactly that.
getPtynow takesinjectable backend loaders, so the selection logic is driven identically on every
runtime and the cases run locally too. Open Code Review additionally caught that
the nested-vitest launcher accepted
node.cmd, whichspawnSynccannot executewithout
shell: true.Reviewer Test Plan
The acceptance gate is the full Nightly Tests workflow dispatched on the branch.
Progressive runs converted opaque failures into specific fixes:
After main #3080 landed, its response-status fix alone failed 4 of 8 local canary
runs. Retaining this branch's 50 ms upstream-crash delay passed 10 of 10, so the
final code deliberately includes both parts.
To exercise locally:
To see the diagnostics that were missing, break a CLI import deliberately and run
the bundle test: it should now name the unresolved specifier rather than saying
Bundle failed.Testing Matrix
Windows and Linux coverage is from the dispatched nightly runs above rather than
a local machine; macOS is local plus nightly. Full local suite: typecheck, lint,
lint:eslint-guard, build, format and tests, plus thebun scripts/start.ts --profile-load stepfun-37smoke.Linked issues / bugs
Fixes #3061
Related follow-ups found while investigating, deliberately not fixed here:
packages/auth/src/lock-owner.tscarries the timezone-misread bug that itspackages/storagetwin was fixed for, and its only covering test is skippedboth by platform and by
'bun' in process.versionsin a package that runsexclusively under Bun — so it never executes anywhere.
.gitignoreto a workspacenested inside it, so a user whose workspace sits in an ignored directory sees
an empty project with no explanation. Fixed here only for the one integration
test that tripped over it.
Summary by CodeRabbit
Bug Fixes
Developer Experience