Skip to content

fix(windows): harden telemetry detached-spawn + fix runtime-loader colon-path (Windows nightly Node 24, GH-363) - #365

Merged
unbraind merged 5 commits into
mainfrom
fix/windows-nightly-node24-telemetry-spawn
Jun 25, 2026
Merged

unbraind merged 5 commits into
mainfrom
fix/windows-nightly-node24-telemetry-spawn

Conversation

@unbraind

Copy link
Copy Markdown
Owner

Summary

Fixes the Windows nightly (Node 24) failure tracked in GH-363 / GH-348 (pm-xaib, root cause of pm-2kkl). The nightly went red on the 2026-06-24 matrix bump from Node 20 → Node 24 with three distinct, deterministic symptoms; this PR fixes all three and hardens the per-PR windows-regression Node 24 oracle so the classes are caught before the nightly.

Root causes & fixes

  1. Telemetry detached-spawn unhandled error → vitest worker crash (root cause of the long-standing Nightly Validation failed: windows-latest / Node 20 #348 "Worker forks emitted error / Worker exited unexpectedly" teardown crash).
    scheduleTelemetryFlush in src/core/telemetry/runtime.ts launched the detached flush worker via spawn() without a child.on("error", …) listener. An asynchronous spawn failure (locked-down Windows runner / EACCES / missing runner) emits an unhandled 'error' event that Node re-throws as an uncaught exception — crashing the foreground CLI and, under tests, the vitest fork worker on teardown. The sibling search dispatchRefreshChild (src/core/search/background-refresh.ts) already had exactly this guard; telemetry was the lone outlier.

    • Added child.on("error", …) that swallows the failure and releases the spawn gate (mirrors background-refresh), so a later flush retries.
    • The unit test that exercised this path deleted VITEST and launched a real, network-touching, untracked node flush child (orphaned on Windows). It now mocks node:child_process like the gold-standard background-refresh.spec.ts, plus a focused regression test that emits an async 'error' and asserts the spawn gate is released.
  2. runtime-loaders.spec.ts colon-path ENOENT (pm-beads + pm-todos).
    The ERR_MODULE_NOT_FOUND path-normalization coverage fixture synthesized a literal C:\pm\project directory segment; the colon is reserved on Windows so mkdir raised ENOENT. The fixture is now platform-aware — on Windows path.join already yields the native-separator divergence; on POSIX a backslash-bearing segment is injected — exercising the loader's backslash-normalizing branch identically on both platforms with no coverage change.

  3. cli.integration.spec.ts "installs runtime dependencies for packed npm package extensions" returned install code 1 (Windows-only npm .cmd shell-spawn / tar / hosted-SDK junction class).
    Added an assertion message that surfaces the install stderr/stdout so the platform-specific failure is diagnosable from CI rather than a bare expected 1 to be 0, and added the case to the windows-regression Node 24 job.

CI hardening

The per-PR windows-regression (Node 24) job now also runs runtime-loaders.spec.ts, telemetry-runtime.spec.ts, and the packed-npm-extension install integration case, so all three Windows classes fail this required check pre-merge instead of only the scheduled nightly. The ci-workflow-contract spec is updated to lock the new commands.

Verification

  • All affected specs pass locally on Linux; telemetry suite spawns zero orphan flush children.
  • Full suite + 100/100/100/100 coverage green; typecheck clean.
  • Windows behavior verified via this PR's windows-regression Node 24 job.

Closes GH-363. Resolves the root cause of GH-348 / pm-2kkl (closed once the next nightly is green).

🤖 Generated with Claude Code

…lon-path (GH-363, pm-xaib)

The Windows nightly (windows-latest) went red on the 2026-06-24 Node 20 -> 24
matrix bump with three deterministic symptoms. Fix all three and expand the
per-PR windows-regression Node 24 oracle so the classes fail pre-merge.

1. Telemetry detached-spawn unhandled error -> vitest worker crash (root cause of
   the long-standing GH-348 "Worker forks emitted error / Worker exited
   unexpectedly" teardown crash, pm-2kkl). scheduleTelemetryFlush launched the
   detached flush worker via spawn() with no child.on("error") listener, so an
   async spawn failure (locked-down Windows runner / EACCES / missing runner)
   re-throws as an uncaught exception that crashes the foreground CLI and the
   vitest fork worker. The sibling search dispatchRefreshChild already had this
   guard; telemetry was the lone outlier. Added child.on("error") that releases
   the spawn gate. The unit test that exercised this also launched a real,
   network-touching, untracked node flush child (orphaned on Windows); it now
   mocks node:child_process like background-refresh.spec.ts, plus a focused
   regression test that emits an async 'error' and asserts the gate is released.

2. runtime-loaders.spec.ts colon-path ENOENT (pm-beads + pm-todos): the path
   normalization fixture synthesized a literal "C:\pm\project" segment; the
   colon is reserved on Windows so mkdir raised ENOENT. The fixture is now
   platform-aware and exercises the loader's backslash-normalizing branch
   identically on both platforms with no coverage change.

3. cli.integration.spec.ts packed-npm install: surface the install stderr/stdout
   in the assertion so the Windows-only npm .cmd shell-spawn / tar / junction
   failure is diagnosable in CI rather than a bare "expected 1 to be 0".

CI: windows-regression (Node 24) now also runs runtime-loaders.spec.ts,
telemetry-runtime.spec.ts, and the packed-npm-extension install case; the
ci-workflow-contract spec locks the new commands.

Full suite green (5096 tests, 100/100/100/100 coverage); typecheck, static and
docs/skills gates pass; manual sandbox smoke test of core flows + the telemetry
detached-flush path verified clean with zero orphan children.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PY9Gnrmrh9EVxGmpBVAv9j

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @unbraind, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@coderabbitai

coderabbitai Bot commented Jun 25, 2026 •

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@unbraind, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 6 minutes and 32 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 credits.

🚦 How do rate 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 see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro

Run ID: 56ed4e7c-57ee-418a-80b4-6671624253e6

📥 Commits

Reviewing files that changed from the base of the PR and between e4c7f3e and e459ca5.

📒 Files selected for processing (12)
  • .agents/pm/history/pm-2kkl.jsonl
  • .agents/pm/history/pm-xaib.jsonl
  • .agents/pm/issues/pm-2kkl.toon
  • .agents/pm/issues/pm-xaib.toon
  • .github/workflows/ci.yml
  • src/cli/commands/extension/install-sources.ts
  • src/core/telemetry/runtime.ts
  • tests/integration/ci-workflow-contract.spec.ts
  • tests/integration/cli.integration.spec.ts
  • tests/unit/core/telemetry/telemetry-runtime.spec.ts
  • tests/unit/extensions/extension-command.spec.ts
  • tests/unit/packages/runtime-loaders.spec.ts
📝 Walkthrough

Walkthrough

The PR updates telemetry flush handling to ignore asynchronous child-process spawn errors, normalizes npm pack and file-alias resolution to native filesystem paths, expands Windows regression coverage for runtime-loader and packed-extension tests, and appends matching GH-348/GH-363 issue history updates.

Changes

Windows regression bundle

Layer / File(s) Summary
Telemetry spawn error gate
src/core/telemetry/runtime.ts, .agents/pm/history/pm-2kkl.jsonl, .agents/pm/issues/pm-2kkl.toon
scheduleTelemetryFlush adds an async child-process error handler that releases the flush gate, and the GH-348 issue records capture the same root cause and fix details.
Telemetry flush tests
tests/unit/core/telemetry/telemetry-runtime.spec.ts
The telemetry runtime spec mocks node:child_process.spawn, asserts detached unref() and error listener wiring, and adds an EventEmitter-based regression for async child errors releasing the lock.
Npm pack path resolution
src/cli/commands/extension/install-sources.ts, .agents/pm/history/pm-xaib.jsonl, .agents/pm/issues/pm-xaib.toon
resolveNpmPackSpec and normalizeNpmLocalFileAliasSpec now return and decode native filesystem paths, and the GH-363 tracker entries record the same path-normalization changes.
Extension path assertions
tests/unit/extensions/extension-command.spec.ts
The extension command tests now expect native-path results from resolveNpmPackSpec and normalizeNpmLocalFileAliasSpec, including decoded file URLs and alias path cases.
Windows regression CI coverage
.github/workflows/ci.yml, tests/integration/ci-workflow-contract.spec.ts, tests/unit/packages/runtime-loaders.spec.ts, tests/integration/cli.integration.spec.ts
The Windows regression job adds runtime-loader, telemetry-runtime, and packed-extension invocations, the workflow contract checks the new step list, the runtime-loader test uses platform-dependent path setup, and the CLI integration assertion now includes failure output.

Sequence Diagram(s)

sequenceDiagram
  participant scheduleTelemetryFlush
  participant spawn
  participant detachedFlushWorker
  scheduleTelemetryFlush->>spawn: spawn detached flush worker
  spawn-->>detachedFlushWorker: child process
  detachedFlushWorker-->>scheduleTelemetryFlush: error event
  scheduleTelemetryFlush->>scheduleTelemetryFlush: release flush gate
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • unbraind/pm-cli#84: Touches the telemetry flush spawn path in src/core/telemetry/runtime.ts and related control-flow around detached child processes.
  • unbraind/pm-cli#311: Updates the windows-regression workflow contract expectations in tests/integration/ci-workflow-contract.spec.ts.
  • unbraind/pm-cli#325: Modifies the same telemetry runtime test area, including platform-specific execution behavior and child-process control.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title names the main Windows telemetry and runtime-loader fixes and stays specific to the PR's primary change.
Description check ✅ Passed The description matches the PR's Windows Node 24 fixes and CI hardening, including telemetry, runtime-loader, and packed npm install regressions.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@unbraind

Copy link
Copy Markdown
Owner Author

@coderabbitai full review

@unbraind

Copy link
Copy Markdown
Owner Author

/gemini review

@unbraind

Copy link
Copy Markdown
Owner Author

@codex review

@coderabbitai

coderabbitai Bot commented Jun 25, 2026 •

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create a Codex account and connect to github.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request addresses several Windows-specific test and runtime issues (GH-348 and GH-363). Specifically, it adds an asynchronous error handler to the detached telemetry flush spawn in src/core/telemetry/runtime.ts to prevent uncaught exceptions from crashing vitest workers during teardown. Additionally, it mocks node:child_process in the telemetry unit tests to avoid spawning real, lingering processes, fixes a Windows path-creation issue in runtime-loaders.spec.ts by avoiding reserved colon characters, improves diagnostics for CLI installation failures, and expands the CI workflow contract to cover these tests. 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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request addresses several Windows-specific test and runtime issues (GH-363 and GH-348). Key changes include adding an 'error' event listener to the detached telemetry flush spawn process in src/core/telemetry/runtime.ts to prevent unhandled exceptions from crashing vitest workers on teardown, mocking node:child_process in telemetry tests to avoid orphan processes, and resolving a Windows path creation issue in runtime-loaders.spec.ts by avoiding reserved colon characters. Additionally, CLI integration tests now provide more detailed error diagnostics on failure. Feedback points out that while the CI workflow contract test was updated to expect new test commands, the actual .github/workflows/ci.yml file was not modified in this PR, which will likely cause the CI build to fail.

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.

Comment on lines +190 to +193
"run: node scripts/run-tests.mjs test -- tests/unit/cli/cli-main-errors.spec.ts tests/unit/core/schema/runtime-schema-path-win32-guard.spec.ts tests/unit/helpers/scriptModule.spec.ts tests/unit/scripts/ tests/unit/packages/runtime-loaders.spec.ts tests/unit/core/telemetry/telemetry-runtime.spec.ts",
"run: node scripts/run-tests.mjs test -- tests/integration/cli.integration.spec.ts -t \"installs runtime dependencies for packed npm package extensions\"",
]);
expect(ciWorkflow.match(/PM_RUN_TESTS_SKIP_BUILD: "1"/g)?.length).toBe(2);
expect(ciWorkflow.match(/PM_RUN_TESTS_SKIP_BUILD: "1"/g)?.length).toBe(3);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The contract test has been updated to expect the new test commands and three occurrences of PM_RUN_TESTS_SKIP_BUILD: "1" in .github/workflows/ci.yml. However, the actual .github/workflows/ci.yml file is not modified in this pull request. Please ensure that .github/workflows/ci.yml is updated to match these new contract expectations, otherwise the CI build will fail.

…cs resolve (GH-363)

Windows-regression (Node 24) surfaced the real packed-npm-extension install
failure via the new diagnostic: `npm pack` received
`name@file:///C:/Users/RUNNER%7E1/...` and opened the literal `RUNNER%7E1`
directory, failing ENOENT. pathToFileURL percent-encodes the Windows 8.3 `~`
short name (and spaces -> %20); pm passed that encoded file URL straight to npm,
which opens it literally. This is a real cross-platform bug — reproduced on
Linux with a space-containing path (`pm install npm:file:///…/pm%20space/pkg`).

resolveNpmPackSpec and normalizeNpmLocalFileAliasSpec now hand npm pack a NATIVE
decoded filesystem path (via fileURLToPath) instead of a percent-encoded file
URL, and decode `file:///abs` alias targets (previously bailed on the `//`
prefix) while leaving `file://host/share` UNC specs untouched. The resolver
contract tests assert native paths, plus a regression asserting an encoded
`%20`/`%7E` alias decodes (no `%` survives). Verified end-to-end on Linux: a
space-containing local package install now succeeds where it previously ENOENT'd.

Full suite green (5096 tests, 100/100/100/100 coverage); typecheck, static and
docs/skills gates pass.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PY9Gnrmrh9EVxGmpBVAv9j
@unbraind

Copy link
Copy Markdown
Owner Author

Pushed 6e9cc0c: the real packed-npm-extension install failure is now root-caused and fixed. The Windows-regression diagnostic revealed npm pack was handed name@file:///C:/Users/RUNNER%7E1/... and opened the literal %7E path → ENOENT. pathToFileURL percent-encodes the Windows 8.3 ~ short name (and spaces → %20); resolveNpmPackSpec/normalizeNpmLocalFileAliasSpec now hand npm pack a native decoded path instead. Reproduced + fixed end-to-end on Linux with a space-containing path.

Re: the earlier /gemini review note on ci-workflow-contract.spec.ts claiming .github/workflows/ci.yml is unmodified — that is a false positive: this PR's first commit (09a9524) does modify ci.yml (the two new windows-regression test steps), and the contract test passes against the committed workflow locally and in CI.

Please re-review the full diff:
@coderabbitai full review
/gemini review
@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create a Codex account and connect to github.

@coderabbitai

coderabbitai Bot commented Jun 25, 2026 •

Copy link
Copy Markdown

@unbraind Thanks — I’ll re-review the full diff with that context in mind, especially the Windows npm pack fix around decoded native paths in resolveNpmPackSpec / normalizeNpmLocalFileAliasSpec, and your note that the earlier ci-workflow-contract.spec.ts concern about .github/workflows/ci.yml being unmodified was a false positive.

✅ Action performed

Full review finished.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request addresses several Windows-specific nightly test failures and a long-standing teardown crash. Specifically, it updates install-sources.ts to pass native filesystem paths instead of percent-encoded file URLs to npm pack to prevent ENOENT errors on paths containing spaces or ~ characters. It also hardens the detached telemetry flush spawn by adding an asynchronous error listener to prevent vitest worker crashes, mocks child processes in telemetry tests to avoid orphan processes, and fixes a test fixture directory creation failure on Windows. Feedback on these changes highlights a potential runtime crash on Windows when calling fileURLToPath with root-relative paths that do not start with a drive letter, suggesting a try-catch fallback.

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.

// Windows 8.3 `~` short name escaped to `%7E` — fails ENOENT on every platform
// (GH-363). `file:///abs` and `file:/abs` are absolute URLs decoded via
// fileURLToPath; a bare relative target resolves against cwd.
const absolutePath = target.startsWith("/") ? fileURLToPath(`file:${target}`) : path.resolve(cwd, target);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

On Windows, calling fileURLToPath with a URL path that does not start with a drive letter (e.g., file:/Users/runner) throws a TypeError [ERR_INVALID_FILE_URL_PATH]. Since target can be a root-relative path starting with a single / on Windows, calling fileURLToPath directly here without a guard can crash the CLI process.

To prevent this runtime crash, wrap the fileURLToPath call in a try-catch block and fall back to path.resolve(cwd, target) if parsing fails.

  let absolutePath: string;
  try {
    absolutePath = target.startsWith("/") ? fileURLToPath("file:" + target) : path.resolve(cwd, target);
  } catch {
    absolutePath = path.resolve(cwd, target);
  }

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@tests/unit/core/telemetry/telemetry-runtime.spec.ts`:
- Around line 1743-1770: Restore all test-mutated telemetry env vars in the
try/finally cleanup for startTelemetryCommand in telemetry-runtime.spec.ts:
capture and restore process.env.PM_GLOBAL_PATH, and also preserve/reset
PM_TELEMETRY_INLINE_FLUSH and PM_TELEMETRY_FLUSH_CHILD alongside the existing
VITEST, VITEST_WORKER_ID, and NODE_ENV handling. Use the same pattern already in
the test’s finally block so the invalid temp global root does not leak into
later tests.
🪄 Autofix (Beta)

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: Repository UI (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3f021d1a-df50-47cf-8ba1-c3fb93a71bca

📥 Commits

Reviewing files that changed from the base of the PR and between e4c7f3e and 6e9cc0c.

📒 Files selected for processing (12)
  • .agents/pm/history/pm-2kkl.jsonl
  • .agents/pm/history/pm-xaib.jsonl
  • .agents/pm/issues/pm-2kkl.toon
  • .agents/pm/issues/pm-xaib.toon
  • .github/workflows/ci.yml
  • src/cli/commands/extension/install-sources.ts
  • src/core/telemetry/runtime.ts
  • tests/integration/ci-workflow-contract.spec.ts
  • tests/integration/cli.integration.spec.ts
  • tests/unit/core/telemetry/telemetry-runtime.spec.ts
  • tests/unit/extensions/extension-command.spec.ts
  • tests/unit/packages/runtime-loaders.spec.ts

…mini, GH-363)

Addresses the Gemini HIGH review finding: fileURLToPath throws
ERR_INVALID_FILE_URL_PATH on Windows for a driveless absolute path (e.g. a
POSIX-style `file:/Users/...` supplied on Windows, or the regression fixture's
`file:///opt/...`), which would crash `pm install` instead of resolving the
spec. normalizeNpmLocalFileAliasSpec now decodes an absolute file: URL target
with URL + decodeURIComponent (platform-independent, never throws) and strips
the leading slash before a Windows drive letter, so npm pack always receives a
decoded native path. Relative targets still resolve against cwd.

The resolver contract tests assert platform-independent decoded literals and add
a drive-letter case covering the leading-slash strip.

Full suite green (5096 tests, 100/100/100/100 coverage); typecheck, static and
docs/skills gates pass; space-path install re-verified end-to-end on Linux.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PY9Gnrmrh9EVxGmpBVAv9j
@unbraind

Copy link
Copy Markdown
Owner Author

Pushed 31ca73f addressing review feedback:

@gemini-code-assist (HIGH, install-sources.ts): Confirmed real — fileURLToPath throws ERR_INVALID_FILE_URL_PATH on Windows for a driveless absolute path (verified via Node's {windows:true} mode). normalizeNpmLocalFileAliasSpec now decodes the file-URL target with URL + decodeURIComponent (platform-independent, never throws) and strips the leading slash before a Windows drive letter, so npm pack always gets a decoded native path. Added a drive-letter contract test.

@coderabbitai (telemetry-runtime.spec.ts env restore): This is already handled — the suite-level afterEach (lines 65-117) restores PM_GLOBAL_PATH to its captured original and resets PM_TELEMETRY_INLINE_FLUSH/PM_TELEMETRY_FLUSH_CHILD after every test. The per-test finally deliberately restores only VITEST/VITEST_WORKER_ID/NODE_ENV, which the afterEach does not touch. So there is no cross-test leak. Happy to add redundant restoration if preferred, but it would duplicate the existing teardown.

Re-review the full diff please:
@coderabbitai full review
/gemini review
@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create a Codex account and connect to github.

@coderabbitai

coderabbitai Bot commented Jun 25, 2026 •

Copy link
Copy Markdown

@unbraind Thanks for the detailed follow-up — that makes sense.

I’m withdrawing my earlier telemetry-runtime.spec.ts env-restore concern given your explanation that the suite-level afterEach already restores PM_GLOBAL_PATH / resets PM_TELEMETRY_INLINE_FLUSH and PM_TELEMETRY_FLUSH_CHILD, while the per-test finally only covers VITEST, VITEST_WORKER_ID, and NODE_ENV.

The Windows path note is also helpful — especially the fileURLToPath failure mode for driveless absolute paths and the decoded native-path handling in normalizeNpmLocalFileAliasSpec.

I’ll re-review the full diff with that updated context.


✏️ Learnings added
Learnt from: unbraind
URL: https://github.com/unbraind/pm-cli/pull/365

Timestamp: 2026-06-25T10:09:11.556Z
Learning: In `tests/unit/core/telemetry/telemetry-runtime.spec.ts`, the suite-level `afterEach` restores `PM_GLOBAL_PATH` and resets `PM_TELEMETRY_INLINE_FLUSH` and `PM_TELEMETRY_FLUSH_CHILD` after every test; per-test `finally` blocks in that file intentionally restore only `VITEST`, `VITEST_WORKER_ID`, and `NODE_ENV`, so the earlier cross-test env-leak concern does not apply.

Learnt from: unbraind
URL: https://github.com/unbraind/pm-cli/pull/365

Timestamp: 2026-06-25T10:09:11.556Z
Learning: In `src/cli/commands/extension/install-sources.ts`, `normalizeNpmLocalFileAliasSpec` should not rely on `fileURLToPath` for driveless absolute file-URL targets on Windows because it can throw `ERR_INVALID_FILE_URL_PATH`; the safer approach used here is `URL` plus `decodeURIComponent`, then stripping the leading slash before a Windows drive letter so `npm pack` receives a decoded native filesystem path.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

✅ Action performed

Full review finished.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request addresses several Windows-specific and cross-platform issues. It resolves a teardown crash (GH-348) by adding an 'error' event listener to the detached telemetry flush spawn and mocking node:child_process in its unit tests to prevent orphan processes. It also fixes an npm installation regression (GH-363) by ensuring resolveNpmPackSpec and normalizeNpmLocalFileAliasSpec return native decoded filesystem paths instead of percent-encoded file URLs, preventing ENOENT errors on paths containing spaces or Windows 8.3 short names. Finally, it fixes a test fixture in runtime-loaders.spec.ts where a reserved colon character caused mkdir to fail on Windows. No review comments were provided, and I have no further feedback.

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.

…eRabbit)

Addresses the CodeRabbit review note: the non-inline flush-scheduler tests
mutate PM_GLOBAL_PATH, PM_TELEMETRY_INLINE_FLUSH and PM_TELEMETRY_FLUSH_CHILD in
addition to VITEST/VITEST_WORKER_ID/NODE_ENV. The suite afterEach already resets
these, but the per-test finally now snapshots and restores every mutated key via
a small snapshotEnv helper so each scheduler test is fully self-contained and
cannot leak into a sibling test regardless of teardown order.

Full suite green (5096 tests, 100/100/100/100 coverage); typecheck, static and
docs/skills gates pass.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PY9Gnrmrh9EVxGmpBVAv9j
@unbraind

Copy link
Copy Markdown
Owner Author

Pushed a39e0a8 applying the CodeRabbit env-restore note: the non-inline flush-scheduler tests now snapshot and restore every mutated env var (PM_GLOBAL_PATH, PM_TELEMETRY_INLINE_FLUSH, PM_TELEMETRY_FLUSH_CHILD alongside VITEST/VITEST_WORKER_ID/NODE_ENV) via a small snapshotEnv helper, so each test is fully self-contained. 5096 tests, 100/100/100/100 coverage.

@coderabbitai full review
/gemini review

@coderabbitai

coderabbitai Bot commented Jun 25, 2026 •

Copy link
Copy Markdown

@unbraind Thanks — I’ll re-review the full diff with the updated snapshotEnv-based env restoration in tests/unit/core/telemetry/telemetry-runtime.spec.ts in mind.

✅ Action performed

Full review finished.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request addresses several cross-platform and Windows-specific issues. It ensures that local npm package paths and file aliases are resolved to native filesystem paths rather than percent-encoded file URLs, preventing npm pack from failing with ENOENT on paths with spaces or Windows 8.3 short names. It also hardens the detached telemetry flush spawn by adding an error listener to swallow asynchronous spawn failures, avoiding vitest worker crashes on teardown. Additionally, it fixes a Windows test failure caused by a reserved colon character in a directory fixture and mocks child processes in telemetry tests to prevent orphan processes. Feedback is provided to wrap the URL parsing and percent-decoding logic in a try-catch block to prevent potential synchronous exceptions from crashing the CLI.

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.

Comment on lines +393 to +395
const decodedPath = decodeURIComponent(new URL(`file:${target}`).pathname);
const nativePath = /^\/[A-Za-z]:/.test(decodedPath) ? decodedPath.slice(1) : decodedPath;
return `${packageName}@${nativePath}`;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The URL parsing and percent-decoding logic using new URL and decodeURIComponent can throw synchronous exceptions (such as TypeError for invalid URLs or URIError for malformed percent-encoded sequences). To prevent these exceptions from crashing the CLI, wrap this block in a try-catch statement and gracefully fall back to returning the original spec if parsing or decoding fails.

  try {
    const decodedPath = decodeURIComponent(new URL("file:" + target).pathname);
    const nativePath = /^\/[A-Za-z]:/.test(decodedPath) ? decodedPath.slice(1) : decodedPath;
    return packageName + "@" + nativePath;
  } catch {
    return spec;
  }

…g (Gemini)

Addresses the Gemini review finding: decodeURIComponent throws a URIError on a
malformed percent sequence (e.g. `%ZZ`), so `pm install npm:name@file:///…/%ZZ/…`
would crash the CLI with an uncaught exception. normalizeNpmLocalFileAliasSpec
now wraps the URL parse + decode in try/catch and returns the spec untouched on
failure, letting npm surface a clear error. Added a malformed-percent contract
test covering the fallback.

Full suite green (5096 tests, 100/100/100/100 coverage); typecheck, static and
docs/skills gates pass.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PY9Gnrmrh9EVxGmpBVAv9j
@unbraind

Copy link
Copy Markdown
Owner Author

Pushed e459ca5 applying the Gemini robustness note: normalizeNpmLocalFileAliasSpec now wraps the URL parse + decodeURIComponent in try/catch and returns the spec untouched on a URIError (malformed percent-encoding like %ZZ), so a bad spec yields a clear npm error instead of crashing the CLI. Added a malformed-percent contract test. 5096 tests, 100/100/100/100 coverage.

@coderabbitai full review
/gemini review

@coderabbitai

coderabbitai Bot commented Jun 25, 2026 •

Copy link
Copy Markdown

@unbraind Thanks — I’ll re-review the full diff with the latest robustness change in mind, especially the normalizeNpmLocalFileAliasSpec fallback for malformed percent-encoding and the added contract test.


✏️ Learnings added
Learnt from: unbraind
URL: https://github.com/unbraind/pm-cli/pull/365

Timestamp: 2026-06-25T10:41:08.282Z
Learning: In `src/cli/commands/extension/install-sources.ts`, `normalizeNpmLocalFileAliasSpec` now treats malformed percent-encoding defensively by wrapping `URL` parsing and `decodeURIComponent` in try/catch and returning the original spec on `URIError`, so the CLI does not crash and npm can surface a clearer install error; this behavior is covered by a malformed-percent contract test.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

✅ Action performed

Full review finished.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request addresses several Windows-specific nightly CI failures (GH-363 and GH-348). It fixes a teardown worker crash by adding an error listener to the detached telemetry flush spawn and mocking child processes in unit tests. It also resolves an npm install issue by passing native, percent-decoded filesystem paths to npm pack instead of percent-encoded file URLs, which previously failed on paths containing spaces or Windows 8.3 short names. Additionally, a test fixture in runtime-loaders.spec.ts was updated to avoid using a reserved colon character on Windows. Feedback points out a potential bug in normalizeNpmLocalFileAliasSpec where absolute Windows paths like file:///C:/path could be incorrectly treated as UNC paths and bypass decoding, and provides a code suggestion to resolve this.

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.

return `${packageName}@${pathToFileURL(absolutePath).href}`;
// `file://host/share` (exactly two leading slashes) is a UNC / network spec,
// not a local path — leave it for npm to resolve.
if (target.startsWith("//") && !target.startsWith("///")) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This check for UNC paths appears to have a bug on Windows. A file: URL for an absolute Windows path like file:///C:/path will result in a target of //C:/path. This condition will evaluate to true and incorrectly return the original spec, treating it as a UNC path. This will prevent it from being correctly decoded later in the function.

This would cause the new test case you added for Windows paths with percent-encoding to fail. A more specific check is needed to differentiate a UNC hostname from a Windows drive letter.

Suggested change
if (target.startsWith("//") && !target.startsWith("///")) {
if (target.startsWith('//') && !target.startsWith('///') && !/^[a-zA-Z]:/.test(target.slice(2))) {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, but this is a false positive — a slash miscount. For an absolute path the file URL is file:///C:/path (three slashes: file: + empty authority // + /C:/path). After stripping the @file: marker, target is ///C:/path (three leading slashes), not //C:/path. So target.startsWith('///') is true, making !target.startsWith('///') false, and the UNC branch is correctly not taken — the drive path falls through to the decode.

Empirically, the added contract test normalizeNpmLocalFileAliasSpec('alias@file:///C:/Temp/pm%7Eshort/pkg') returns 'alias@C:/Temp/pm~short/pkg' and passes in CI (Gates coverage + Windows regression Node 24 both green). The //C:/path two-slash form is a non-standard file://host shape (host=C:) and is intentionally left for npm. Keeping the current check.

@unbraind
unbraind merged commit 6f88efc into main Jun 25, 2026
14 checks passed
@unbraind
unbraind deleted the fix/windows-nightly-node24-telemetry-spawn branch June 25, 2026 10:51
unbraind added a commit that referenced this pull request Jun 25, 2026
… and regenerate CHANGELOG

pm-xaib resolved: runtime-loader colon-path, telemetry detached-spawn error
handler, and npm-pack native-path fixes verified on the windows-latest/Node 24
regression job. pm-2kkl (teardown worker crash) stays open with the root-cause
fix documented, pending confirmation by the next scheduled nightly.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PY9Gnrmrh9EVxGmpBVAv9j
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Nightly Validation failed: windows-latest / Node 24

1 participant