Repository navigation
Fix Windows nightly test portability - #325
Conversation
Stabilize the nightly and coverage test surfaces by normalizing eval path assertions, guarding chmod-based permission-denial coverage to POSIX, retrying telemetry OTLP temp cleanup while detached workers exit, and giving the MCP dynamic package error-path spec enough time under full coverage. Track the remediation in pm-83rt with linked files, focused linked tests, full 100/100/100/100 coverage evidence, manual temp-dir smoke evidence, changelog regeneration, Sentry/telemetry verification, and related closed Windows-nightly lineage.
Reviewer's GuideMakes Windows nightly tests portable by guarding POSIX-only permission/FS behaviors, normalizing path-based assertions, stabilizing telemetry/mcp integration tests with retry/timeout tweaks, and recording the work in pm-83rt plus the changelog. Flow diagram for stabilized Windows nightly tests and telemetry/MCP adjustmentsflowchart TD
A[Windows nightly test run] --> B[Normalize eval path assertions]
B --> C[Guard POSIX-only chmod-denial coverage]
C --> D[Retry telemetry OTLP temp cleanup]
D --> E[Extend MCP dynamic package error-path timeout]
E --> F[Stable cross-platform nightly and coverage CI]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR fixes Windows nightly CI failures by adding a shared ChangesWindows Nightly CI Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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 |
|
To use Codex here, create a Codex account and connect to github. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses Windows nightly test failures by resolving cross-platform assumptions. Key changes include making path assertions separator-neutral in eval-command.spec.ts, skipping POSIX-specific chmod and write-warning tests on Windows, increasing the timeout for MCP dynamic package actions, and introducing a retry loop with delay during temporary directory cleanup in telemetry-otel-nonblocking.spec.ts. The reviewer suggests increasing the retry attempts and delay step in the cleanup logic to make it even more robust on slow CI environments.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| for (let attempt = 0; attempt <= 5; attempt += 1) { | ||
| try { | ||
| await rm(context.tempRoot, { recursive: true, force: true }); | ||
| return; | ||
| } catch (error) { | ||
| if (attempt === 5) { | ||
| throw error; | ||
| } | ||
| await delay(50 * (attempt + 1)); | ||
| } | ||
| } |
There was a problem hiding this comment.
On slow CI environments (especially windows-latest on GitHub Actions), the detached background telemetry process might take more than 750ms to fully release file locks and exit. Increasing the retry attempts to 10 and the delay step to 100ms provides a more robust ~5.5-second window to prevent flaky test failures during cleanup.
| for (let attempt = 0; attempt <= 5; attempt += 1) { | |
| try { | |
| await rm(context.tempRoot, { recursive: true, force: true }); | |
| return; | |
| } catch (error) { | |
| if (attempt === 5) { | |
| throw error; | |
| } | |
| await delay(50 * (attempt + 1)); | |
| } | |
| } | |
| for (let attempt = 0; attempt <= 10; attempt += 1) { | |
| try { | |
| await rm(context.tempRoot, { recursive: true, force: true }); | |
| return; | |
| } catch (error) { | |
| if (attempt === 10) { | |
| throw error; | |
| } | |
| await delay(100 * (attempt + 1)); | |
| } | |
| } |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The platform checks for POSIX-only behavior (e.g.,
process.platform !== "win32"anditOnPosix) are currently scattered; consider centralizing them in a small test helper (e.g.,isPosix()/itPosix) to keep the intent consistent and reduce duplication. - In the OTLP non-blocking cleanup retry loop, it might be helpful to surface some context (e.g., last error message or attempt count) in the thrown error to make diagnosing persistent cleanup failures easier when the retries are exhausted.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The platform checks for POSIX-only behavior (e.g., `process.platform !== "win32"` and `itOnPosix`) are currently scattered; consider centralizing them in a small test helper (e.g., `isPosix()` / `itPosix`) to keep the intent consistent and reduce duplication.
- In the OTLP non-blocking cleanup retry loop, it might be helpful to surface some context (e.g., last error message or attempt count) in the thrown error to make diagnosing persistent cleanup failures easier when the retries are exhausted.
## Individual Comments
### Comment 1
<location path="tests/unit/core/telemetry/telemetry-runtime.spec.ts" line_range="32" />
<code_context>
installSemanticFetchMock,
} from "../../../helpers/semanticFetchMock.js";
+const itOnPosix = process.platform === "win32" ? it.skip : it;
+
function createSeedItem(
</code_context>
<issue_to_address>
**suggestion:** Consider moving `itOnPosix` into a shared test helper to keep platform-gated tests consistent.
This logic is now duplicated here and in `tests/unit/core/search/search-cache.spec.ts`. To avoid drift and keep platform-gated tests maintainable, consider a shared helper (e.g. `tests/helpers/platform.ts` with `export const itOnPosix = process.platform === 'win32' ? it.skip : it;`) and reuse it across tests.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Get PR details
gh pr view 325 --repo unbraind/pm-cli --json title,body,files,additions,deletions,commits
echo "---FILES CHANGED---"
gh pr diff 325 --repo unbraind/pm-cliLength of output: 41737
Review: Fix Windows nightly test portabilityThe changes are clean, minimal, and well-targeted. Each fix directly addresses a specific failure category from nightly run 27898852339. A few observations:
|
There was a problem hiding this comment.
Code Review
This pull request resolves Windows nightly CI test failures by making path assertions separator-neutral, skipping or guarding POSIX-specific chmod permission tests on Windows, increasing timeouts for MCP dynamic package actions, and introducing a retry mechanism with backoff when cleaning up temporary directories in telemetry integration tests. The reviewer suggested increasing the retry limit and backoff duration in the telemetry cleanup loop to further prevent flaky test failures on slow Windows runners.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| for (let attempt = 0; attempt <= 5; attempt += 1) { | ||
| try { | ||
| await rm(context.tempRoot, { recursive: true, force: true }); | ||
| return; | ||
| } catch (error) { | ||
| if (attempt === 5) { | ||
| throw error; | ||
| } | ||
| await delay(50 * (attempt + 1)); | ||
| } | ||
| } |
There was a problem hiding this comment.
On slow CI environments (especially Windows runners), the detached background telemetry process might take longer than 750ms to fully exit and release file locks on the temporary directory. Increasing the retry limit to 10 attempts with a slightly larger backoff will make the cleanup significantly more robust and prevent flaky test failures.
| for (let attempt = 0; attempt <= 5; attempt += 1) { | |
| try { | |
| await rm(context.tempRoot, { recursive: true, force: true }); | |
| return; | |
| } catch (error) { | |
| if (attempt === 5) { | |
| throw error; | |
| } | |
| await delay(50 * (attempt + 1)); | |
| } | |
| } | |
| for (let attempt = 0; attempt <= 10; attempt += 1) { | |
| try { | |
| await rm(context.tempRoot, { recursive: true, force: true }); | |
| return; | |
| } catch (error) { | |
| if (attempt === 10) { | |
| throw error; | |
| } | |
| await delay(100 * (attempt + 1)); | |
| } | |
| } |
Centralize POSIX test gating in a shared test helper and reuse it in chmod-dependent test coverage. Increase telemetry OTLP temp cleanup retries and throw a contextual final cleanup error with the original cause. Record review follow-up verification on pm-83rt.
|
Review feedback addressed in f968e1d. Please re-review the current head. @codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Get the latest commits and diff
gh pr view 325 --repo unbraind/pm-cli --json commits,headRefOid,title,state
echo "---LATEST DIFF---"
gh pr diff 325 --repo unbraind/pm-cliLength of output: 43064
|
There was a problem hiding this comment.
Code Review
This pull request resolves Windows nightly test failures by making path assertions separator-neutral, guarding POSIX-specific chmod and permission tests, and adding a retry mechanism for telemetry OTLP temp directory cleanup. The review feedback suggests improving test robustness in eval-command.spec.ts by explicitly asserting that readFileMock has been called before accessing its mock calls, preventing cryptic TypeErrors on failure.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const [targetPath] = readFileMock.mock.calls[0]; | ||
| expect(targetPath.replaceAll(path.sep, "/")).toContain("custom/eval.json"); |
There was a problem hiding this comment.
If runEval fails and does not call readFileMock, accessing readFileMock.mock.calls[0] directly will throw a cryptic TypeError instead of a clear assertion failure. Adding an explicit expect(readFileMock).toHaveBeenCalled() assertion first will make test failures much easier to diagnose. Avoid adding redundant defensive checks like optional chaining (?.[0]) once the assertion guarantees the call has occurred.
expect(readFileMock).toHaveBeenCalled();
const targetPath = readFileMock.mock.calls[0][0];
expect(targetPath.replaceAll(path.sep, "/")).toContain("custom/eval.json");References
- Avoid adding redundant defensive checks (such as null/undefined guards or type checks) for fields that are typed as non-optional and guaranteed to be populated by the system. Doing so introduces unreachable branches and inconsistency with the rest of the codebase where the field is consumed unguarded.
| const [targetPath] = readFileMock.mock.calls[0]; | ||
| const normalizedTargetPath = targetPath.replaceAll(path.sep, "/"); |
There was a problem hiding this comment.
Similarly, if runEval fails to call readFileMock, accessing readFileMock.mock.calls[0] directly will throw a cryptic TypeError. Adding an explicit expect(readFileMock).toHaveBeenCalled() assertion first will make test failures much easier to diagnose. Avoid adding redundant defensive checks like optional chaining (?.[0]) once the assertion guarantees the call has occurred.
| const [targetPath] = readFileMock.mock.calls[0]; | |
| const normalizedTargetPath = targetPath.replaceAll(path.sep, "/"); | |
| expect(readFileMock).toHaveBeenCalled(); | |
| const targetPath = readFileMock.mock.calls[0][0]; | |
| const normalizedTargetPath = targetPath.replaceAll(path.sep, "/"); |
References
- Avoid adding redundant defensive checks (such as null/undefined guards or type checks) for fields that are typed as non-optional and guaranteed to be populated by the system. Doing so introduces unreachable branches and inconsistency with the rest of the codebase where the field is consumed unguarded.
|
@coderabbitai review Retrying after the rate-limit cooldown. Please review current head f968e1d. |
|
No new issues found at this head. The PR is approved and good to merge. ✅ Action performedComments resolved. Approval is disabled; enable |
Address Gemini review feedback by asserting the eval golden-set read mock was called before reading mock.calls[0]. Record focused eval verification, build, and static-quality evidence on pm-83rt.
|
Gemini eval-test feedback addressed in 9645438. Please re-review current head. @codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== PR commits ==="
gh pr view 325 --repo unbraind/pm-cli --json commits,headRefOid,title,state
echo ""
echo "=== Latest diff ==="
gh pr diff 325 --repo unbraind/pm-cliLength of output: 45817
|
There was a problem hiding this comment.
Code Review
This pull request resolves Windows nightly CI test failures by eliminating platform-specific assumptions. It introduces a platform helper (tests/helpers/platform.ts) to conditionally skip POSIX-only tests (such as those relying on chmod permissions) on Windows and wraps platform-specific logic in conditional blocks. Additionally, it normalizes path separators in eval-command.spec.ts to be separator-neutral, increases the timeout for MCP dynamic package actions, and implements a retry mechanism with delays for telemetry OTLP temporary directory cleanup to prevent race conditions. There are no review comments provided, so I have no feedback to provide on the review itself.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Summary
Verification
Review Agents
@codex review
/gemini review
@gemini-code-assist
@coderabbitai full review
@sourcery-ai review
Summary by Sourcery
Improve test suite portability and stability across platforms, particularly Windows, and document the fix in the changelog.
Bug Fixes:
Enhancements:
Tests: