Conversation
…ixes #2683) The Release nightly preflight failed because Vitest's default file parallelism ran subprocess-heavy harness files concurrently, letting independent cleanup tests that take exclusive mkdir locks collide (EEXIST). Serializing test files (not tests within a file) removes the cross-file contention while preserving per-file concurrency. Adds a behavioral regression that fails with EEXIST under parallel files and passes once serialized (mutation-tested by flipping fileParallelism back to true).
github-actions
Bot
added
the
maintainer:e2e:ok
Contributor
📝 WalkthroughWalkthroughChangesFile parallelism handling
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
LLxprt PR Review – PR #2689Issue AlignmentResolves #2683. The release failure was caused by Side Effects
Code Quality
Tests and Coverage
VerdictReady. Small, focused fix with real tests that directly addresses the release nondeterminism described in #2683. |
Contributor
OpenCodeReview — PR #2689
|
GitHub is migrating npm registry metadata for repository URLs from the deprecated git:// protocol to https://, package by package. generate-notices.js read each dependency's repository.url verbatim, so the checked-in NOTICES.txt drifts whenever a dependency's published metadata flips. Because the npm prepare lifecycle re-runs generate-notices.js during 'bun install', CI regenerated NOTICES.txt with the new protocol and the 'git diff --exit-code' formatter gate failed nondeterministically. The failure blocked PR #2689's entire lint/test matrix: lint_javascript failing cascades to a skipped lint gate, which skips the whole test matrix. Canonicalize git://github.com/ to https:// in the generator so the emitted text is identical regardless of which protocol the installed package advertises. Update the checked-in artifact to the canonical form. The generator ran main() at import time, making it unimportable. Extract the normalization into an exported pure function and guard the entry point behind an ESM isMain check so it can be unit tested. The normalization is covered by a focused test (mutation-verified: a no-op implementation fails the canonicalize case).
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Comment on lines
+66
to
+68
| watcher = watch(${JSON.stringify(sentinelDir)}, () => { | ||
| if (existsSync(readyOther)) finish(); | ||
| }); |
Contributor
There was a problem hiding this comment.
The generated watcher in
crossBarrierdoes not handleerrorevents fromfs.watch. If the sentinel directory is deleted or becomes inaccessible during the test, the watcher emits an unhandlederrorevent that crashes the nested Vitest process with an unhandled exception, causing the test to fail with an opaque error instead of a clean assertion failure. Add anwatcher.on('error', (err) => { ... })handler that treats watcher errors as a failure condition and resolves the promise accordingly.
…uced The original diagnosis (EEXIST from file parallelism colliding on exclusive-lock mkdir) was wrong. The nightly log (run 30139165201) contains zero EEXIST and zero timeout errors. All 47 failures were assertion errors (expected status 0, got 1) — the signature of subprocesses being killed at vitest's 5s default timeout, which makes execFileSync throw, the catch block return status 1, and the assertion fail. PR #2682 (already merged) fixed the actual root cause by adding testTimeout: 30000 to scripts/tests/vitest.config.ts. Main (run 30146622275) passes all four Test matrix jobs with file parallelism enabled. The fileParallelism: false setting was both unnecessary (the root cause was the missing timeout, not parallelism) and harmful (serializing 113 subprocess-heavy test files into one worker caused accumulated resource pressure that made TERM-signal tests timeout on macOS CI runners). Reverts scripts/tests/vitest.config.ts to match main and removes the behavioral regression test that asserted fileParallelism: false.
acoliver
changed the title
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. |
4 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.
Summary
Fixes #2683
The nightly release workflow (run 30139165201) failed because
npm run test:cireported 47 test failures in the script harness.Root cause: vitest 5s default timeout (already fixed by #2682)
The nightly ran on commit
3a12f02e5, wherescripts/tests/vitest.config.tsset notestTimeoutorfileParallelism. This left vitest at its 5s default — far too short for the subprocess-heavy assign-remediation tests, which spawn bash scripts, a Python fake-gh adapter, and jq pipelines.When the 5s timeout fired mid-
execFileSync, the subprocess was killed. The catch block returnedstatus: 1, and assertions likeexpect(result.status).toBe(0)failed withexpected 1 to be +0. This produced 47 assertion failures across 12 test files — not EEXIST, not timeouts, but killed-subprocess side effects.PR #2682 (merged 2026-07-25T05:56Z) fixed this by adding
testTimeout: 30000andhookTimeout: 30000to the script harness config. Main (run 30146622275) passes all four Test matrix jobs (ubuntu/macOS × fallback/keyring).Additional fix in this PR: NOTICES.txt formatter gate determinism
While investigating, a separate, pre-existing CI failure was found: the
Lint (Javascript)formatter gate was failing nondeterministically becauseNOTICES.txtdrifted when npm registry metadata flips a dependency'srepository.urlfrom the deprecatedgit://github.com/protocol tohttps://github.com/. Thepreparelifecycle hook regenerates NOTICES.txt duringbun install, and the checked-in file fell out of sync.Fix: Added
normalizeGitHubRepositoryUrl()togenerate-notices.jsto canonicalizegit://github.com/→https://github.com/regardless of which spelling the installed metadata advertises. This makes generation deterministic across npm metadata changes. The function was extracted as an exported pure function with an ESM main-guard so the script is importable for testing. Updated the checked-in NOTICES.txt to match.What was reverted
An initial approach set
fileParallelism: falseto serialize the 113 test files. This was based on a misdiagnosis (EEXIST) — the nightly log contains zero EEXIST errors. The serialization was harmful: it forced all subprocess-heavy files into one worker, causing accumulated resource pressure that made TERM-signal tests timeout on macOS CI runners (the very regression this PR was trying to fix). Reverted to match main.Changes
packages/vscode-ide-companion/scripts/generate-notices.jsnormalizeGitHubRepositoryUrl()helper; wire into generation path; add ESM main-guard for testabilitypackages/vscode-ide-companion/scripts/generate-notices.test.jspackages/vscode-ide-companion/NOTICES.txtgit://github.com/→https://github.com/URL canonicalizationsVerification