fix: Windows grep outage and test-run telemetry pollution - #1074
fix: Windows grep outage and test-run telemetry pollution#1074anandgupta42 wants to merge 1 commit into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds in-process Windows ripgrep ZIP extraction, staged executable installation, clearer ripgrep errors, and automated test-run telemetry suppression. Explicit telemetry sinks and force overrides remain supported. Tests, CI, and documentation cover both changes. ChangesRipgrep binary reliability
Automated-run telemetry controls
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DownloadSource
participant RipgrepBinary
participant ZipReader
participant FileSystem
DownloadSource->>RipgrepBinary: return ZIP or tarball bytes
RipgrepBinary->>ZipReader: decode ZIP bytes
ZipReader-->>RipgrepBinary: return validated rg.exe bytes
RipgrepBinary->>FileSystem: stage and atomically install executable
sequenceDiagram
participant TestRunner
participant Telemetry
participant AppInsights
TestRunner->>Telemetry: initialize with environment markers
Telemetry->>Telemetry: apply sink, force, and disabled settings
Telemetry->>AppInsights: send or suppress telemetry
Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@packages/core/src/ripgrep.ts`:
- Around line 143-147: Update the ripgrep failure handling around failure() so
the formatted exit-code/stderr message is passed as the Error message string,
while the original ripgrep failure object is supplied as ErrorOptions.cause when
available. Preserve the existing stderr attribution and fallback text in the
formatted message.
In `@packages/core/src/ripgrep/binary.ts`:
- Around line 103-111: Serialize the installation flow in install around each
target using a process-safe per-target lock, covering both ZIP and tar
installation paths. Create a unique staged filename inside the lock instead of
the shared `${target}.tmp`, and keep cleanup, writing, chmod, Windows removal,
and rename within the locked section so concurrent resolvers cannot invalidate
the returned binary.
In `@packages/opencode/test/telemetry/automated-run.test.ts`:
- Around line 71-75: Make the telemetry suite identified by “telemetry:
automated runs never reach the production sink” run serially, using the test
framework’s supported serial describe mechanism. Preserve the existing afterEach
cleanup and test behavior while preventing concurrent access to process.env,
global fetch, and the shared Telemetry instance.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e48ec458-dbbf-4686-af56-826ae01ff4e5
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
docs/docs/reference/telemetry.mdpackages/core/package.jsonpackages/core/src/ripgrep.tspackages/core/src/ripgrep/binary.tspackages/core/test/ripgrep-windows.test.tspackages/opencode/src/altimate/telemetry/index.tspackages/opencode/test/telemetry/automated-run.test.ts
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental pass over
Files Reviewed (1 file)
Previous Review Summaries (8 snapshots, latest commit 334adfe)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 334adfe)Status: No Issues Found | Recommendation: Merge Incremental pass over
Files Reviewed (1 file)
Previous review (commit 8d16513)Status: No Issues Found | Recommendation: Merge Incremental pass over
No redundancy remains to flag; the change itself is the simplification. No new issues on changed code. Files Reviewed (2 files)
Previous review (commit e105f17)Status: No Issues Found | Recommendation: Merge Incremental pass over commit Verified the rationale is sound:
No new issues on changed code. Files Reviewed (1 file)
Previous review (commit 6851f9d)Status: No Issues Found | Recommendation: Merge Incremental pass over commit The CI job mirrors every existing convention in The E2E script correctly reproduces the outage condition and then asserts each invariant the PR claims:
The imported surface matches the current code: Files Reviewed (2 files)
Previous review (commit 59e0482)Status: No Issues Found | Recommendation: Merge Incremental pass over commit A prior warning on Files Reviewed (2 changed files)
Previous review (commit 09af0a5)Status: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Previous review (commit c7b415c)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (1 file)
Fix these issues in Kilo Cloud Previous review (commit bc7ec7a)Status: No Issues Found | Recommendation: Merge Files Reviewed (7 files)
Notes: Both fixes are well-engineered and well-tested.
No blocking findings on changed code. Reviewed by glm-5.2 · Input: 26.4K · Output: 4.8K · Cached: 201.9K Review guidance: REVIEW.md from base branch |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/src/ripgrep/binary.ts">
<violation number="1" location="packages/core/src/ripgrep/binary.ts:110">
P2: A Windows install can delete the only usable cached `rg.exe` and then fail to publish the new binary because `remove(target)` is performed before `rename` and its failure is ignored. A Windows-safe replacement or cross-process install lock would preserve the old target until the new one is committed.</violation>
<violation number="2" location="packages/core/src/ripgrep/binary.ts:186">
P2: Some binary-resolution filesystem failures still escape as defects instead of the new `ripgrep binary resolve failed: …` error because the preceding `Effect.orDie` calls run before this `mapError`. Keeping these operations in the typed error channel (or mapping before converting to defects) would make the resolver's error contract consistent.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // HTTP failures do not — and a resolve failure is memoized by Effect.cached, so it is | ||
| // reported on every subsequent grep of the session. An unattributed message there is | ||
| // exactly what made the Windows outage read as a tool bug rather than a binary problem. | ||
| Effect.mapError((cause) => { |
There was a problem hiding this comment.
P2: Some binary-resolution filesystem failures still escape as defects instead of the new ripgrep binary resolve failed: … error because the preceding Effect.orDie calls run before this mapError. Keeping these operations in the typed error channel (or mapping before converting to defects) would make the resolver's error contract consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/ripgrep/binary.ts, line 186:
<comment>Some binary-resolution filesystem failures still escape as defects instead of the new `ripgrep binary resolve failed: …` error because the preceding `Effect.orDie` calls run before this `mapError`. Keeping these operations in the typed error channel (or mapping before converting to defects) would make the resolver's error contract consistent.</comment>
<file context>
@@ -114,11 +164,31 @@ export namespace RipgrepBinary {
+ // HTTP failures do not — and a resolve failure is memoized by Effect.cached, so it is
+ // reported on every subsequent grep of the session. An unattributed message there is
+ // exactly what made the Windows outage read as a tool bug rather than a binary problem.
+ Effect.mapError((cause) => {
+ const message = cause instanceof Error ? cause.message : String(cause)
+ return /ripgrep/i.test(message) ? cause : new Error(`ripgrep binary resolve failed: ${message}`)
</file context>
| if (process.platform !== "win32") yield* fs.chmod(staged, 0o755) | ||
| // Windows rename fails if the destination exists; the old binary is already unusable | ||
| // by the time we are reinstalling, so dropping it first is safe. | ||
| if (process.platform === "win32") yield* fs.remove(target, { force: true }).pipe(Effect.ignore) |
There was a problem hiding this comment.
P2: A Windows install can delete the only usable cached rg.exe and then fail to publish the new binary because remove(target) is performed before rename and its failure is ignored. A Windows-safe replacement or cross-process install lock would preserve the old target until the new one is committed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/ripgrep/binary.ts, line 110:
<comment>A Windows install can delete the only usable cached `rg.exe` and then fail to publish the new binary because `remove(target)` is performed before `rename` and its failure is ignored. A Windows-safe replacement or cross-process install lock would preserve the old target until the new one is committed.</comment>
<file context>
@@ -48,34 +93,40 @@ export namespace RipgrepBinary {
+ if (process.platform !== "win32") yield* fs.chmod(staged, 0o755)
+ // Windows rename fails if the destination exists; the old binary is already unusable
+ // by the time we are reinstalling, so dropping it first is safe.
+ if (process.platform === "win32") yield* fs.remove(target, { force: true }).pipe(Effect.ignore)
+ yield* fs.rename(staged, target)
+ })
</file context>
bc7ec7a to
c7b415c
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7b415cfae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function isAutomatedRun(): boolean { | ||
| if (process.env.ALTIMATE_TELEMETRY_FORCE === "true") return false | ||
| if (process.env.NODE_ENV === "test") return true | ||
| return Boolean(process.env.BUN_TEST || process.env.VITEST || process.env.JEST_WORKER_ID) | ||
| } |
There was a problem hiding this comment.
For this new private helper, keeping it inside Telemetry adds more code to an export namespace; the package guidance says namespace-private helpers should be non-exported top-level declarations so they remain usable by the file without being part of the namespace pattern. Move isAutomatedRun to top level and call it from doInit instead.
AGENTS.md reference: packages/opencode/AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
| function isAutomatedRun(): boolean { | ||
| if (process.env.ALTIMATE_TELEMETRY_FORCE === "true") return false | ||
| if (process.env.NODE_ENV === "test") return true | ||
| return Boolean(process.env.BUN_TEST || process.env.VITEST || process.env.JEST_WORKER_ID) | ||
| } |
There was a problem hiding this comment.
For this new private helper, keeping it inside Telemetry adds more code to an export namespace; the package guidance says namespace-private helpers should be non-exported top-level declarations so they remain usable by the file without being part of the namespace pattern. Move isAutomatedRun to top level and call it from doInit instead.
Useful? React with 👍 / 👎.
| // Staging next to the target keeps the rename within one filesystem, so it is atomic. | ||
| const install = Effect.fnUntraced( | ||
| function* (target: string, write: (staged: string) => Effect.Effect<void, Error>) { | ||
| const staged = `${target}.tmp` |
There was a problem hiding this comment.
When two processes hit a cold cache at the same time, they both use the same rg(.exe).tmp staging path. One installer can delete or rename the other installer’s temp file; on Windows the later installer can also remove the just-installed target before its own rename, so filepath can fail or leave no cached binary. Use a per-process/random temp name or serialize installs before the final rename.
Useful? React with 👍 / 👎.
| const exit = await run(Effect.result(RipgrepBinary.unzipExecutable(zip))) | ||
|
|
||
| expect(exit._tag).toBe("Failure") | ||
| expect(String((exit as { failure: Error }).failure.message)).toContain("did not contain rg.exe") |
There was a problem hiding this comment.
When unzipExecutable fails, Effect.result(...) returns a Failure exit with the error under exit.cause, not a failure property (the existing tests in this repo check exit.cause). In the missing/empty rg.exe cases this line dereferences undefined.message, so the regression tests fail for the wrong reason instead of asserting the intended diagnostic.
Useful? React with 👍 / 👎.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| } | ||
| // altimate_change end | ||
| return target | ||
| }), |
There was a problem hiding this comment.
[WARNING]: HTTP download failures lost their ripgrep attribution
The outer .pipe(Effect.mapError(...)) that wrapped this resolve effect was removed, so failures outside the install step are no longer prefixed. In particular the HTTP download (the per-call mapError at line 172 only normalizes to Error — it doesn't add "ripgrep"), so a network error, GitHub 403/404, or proxy block surfaces as a raw HttpClientError/ResponseError. Because filepath is Effect.cached, that failure is memoized and re-surfaced on every later grep of the session — the exact "reads as a tool bug" case this attribution exists for. The install/write path is still covered; the download path is not. The PR description still claims "Typed FS/HTTP failures are now wrapped as ripgrep binary resolve failed: …."
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
Thanks — addressed in the latest push. Triage below, including two findings I'm rejecting with evidence. FixedConcurrent installs shared Also fixed the related Windows hazard cubic raised: HTTP download failures lost ripgrep attribution (@kilo-code-bot) — correct, and a real regression from moving attribution into
Tests wrote to the real home directory (cubic) — correct. Non-suppressed cases reach Rejecting — verified against the code
export class Error extends Schema.TaggedErrorClass<Error>()("Ripgrep.Error", {
message: Schema.String,
cause: Schema.optional(Schema.Defect),
}) {}It takes a struct, and
The tests pass; had
Not doing hereA cross-process lock (CodeRabbit's stronger suggestion) is deliberately out of scope. Unique staging plus atomic rename removes the corruption window; the remaining race is two processes each installing a valid, identical binary and one rename winning — which is harmless. A real lock needs a cross-platform advisory-lock primitive we don't have, and this PR is a hotfix for a live Windows outage. Verification: |
|
To use Codex here, create an environment for this repo. |
c7b415c to
09af0a5
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
09af0a5 to
59e0482
Compare
|
One more addressed. Serialize the shared-state test suites (CodeRabbit) — valid, applied. Both new suites mutate process-wide state: the telemetry one touches To be precise about the risk: Bun runs a file's tests sequentially by default and this repo does not pass Verification after this change: |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
59e0482 to
6851f9d
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/ci.yml:
- Line 392: Update the changes filter configuration used by the TypeScript job
condition to include script/windows-ripgrep-e2e.ts in
changes.outputs.typescript, ensuring the job runs when that E2E script changes
while preserving the existing push behavior.
In `@script/windows-ripgrep-e2e.ts`:
- Around line 22-25: Ensure the test body and any failing execFileSync calls in
the Windows ripgrep E2E flow are wrapped in try/finally so the temporary cache
directory is always removed. Move the cleanup currently at the post-test path
into finally, and update fail() to report failure without directly calling
process.exit before cleanup executes.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9fc22fc1-90ce-4a54-99c8-594069523143
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.github/workflows/ci.ymldocs/docs/reference/telemetry.mdpackages/core/package.jsonpackages/core/src/ripgrep.tspackages/core/src/ripgrep/binary.tspackages/core/test/ripgrep-windows.test.tspackages/opencode/src/altimate/telemetry/index.tspackages/opencode/test/telemetry/automated-run.test.tsscript/windows-ripgrep-e2e.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- docs/docs/reference/telemetry.md
- packages/core/package.json
- packages/core/test/ripgrep-windows.test.ts
- packages/core/src/ripgrep.ts
- packages/opencode/test/telemetry/automated-run.test.ts
- packages/opencode/src/altimate/telemetry/index.ts
- packages/core/src/ripgrep/binary.ts
| windows-ripgrep-e2e: | ||
| name: Windows ripgrep E2E | ||
| needs: changes | ||
| if: needs.changes.outputs.typescript == 'true' || github.event_name == 'push' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Run this job when its E2E script changes.
changes.outputs.typescript does not include script/windows-ripgrep-e2e.ts. A pull request that changes only this script skips the job. Add this script to the typescript filter, or add a dedicated output for it.
Proposed fix
typescript:
+ - 'script/windows-ripgrep-e2e.ts'
# altimate_change start — upstream_fix: typecheck every declared TypeScript workspace🤖 Prompt for 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.
In @.github/workflows/ci.yml at line 392, Update the changes filter
configuration used by the TypeScript job condition to include
script/windows-ripgrep-e2e.ts in changes.outputs.typescript, ensuring the job
runs when that E2E script changes while preserving the existing push behavior.
| function fail(message: string): never { | ||
| console.error(`FAIL: ${message}`) | ||
| process.exit(1) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Clean up the temporary cache on failure.
fail() calls process.exit(1), so it bypasses line 110. A thrown execFileSync error also bypasses line 110. Wrap the test body in try/finally, and remove the directory in finally. Avoid direct process.exit() before cleanup runs.
Also applies to: 110-110
🤖 Prompt for 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.
In `@script/windows-ripgrep-e2e.ts` around lines 22 - 25, Ensure the test body and
any failing execFileSync calls in the Windows ripgrep E2E flow are wrapped in
try/finally so the temporary cache directory is always removed. Move the cleanup
currently at the post-test path into finally, and update fail() to report
failure without directly calling process.exit before cleanup executes.
Source: Coding guidelines
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6851f9de94
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (process.env.NODE_ENV === "test") return true | ||
| return Boolean(process.env.BUN_TEST || process.env.VITEST || process.env.JEST_WORKER_ID) |
There was a problem hiding this comment.
This still sends to the production sink when Bun tests are launched with NODE_ENV already set, e.g. NODE_ENV=production bun test: Bun documents that it sets NODE_ENV to test unless it is already set, and in that case BUN_TEST is undefined, so this function returns false and doInit() falls through to DEFAULT_CONNECTION_STRING. Add a repo-owned test marker around the bun test invocations or another Bun-specific guard so pre-set environments do not keep polluting production telemetry.
Useful? React with 👍 / 👎.
6851f9d to
e105f17
Compare
full receipts (1 session)
orchestrator ·
|
| subagent | cost |
|---|---|
| agent-aglm-reviewer-702f70ccae59e3e8 · claude-sonnet-5 | ≥ $0.5405 |
| agent-agemini-reviewer-3cce9c3ba5e51cbe · claude-sonnet-5 | ≥ $0.3922 |
| agent-agpt-reviewer-71fb203724bc62c1 · claude-sonnet-5 | ≥ $0.3835 |
| agent-agrok-converge-26aa059aa135e5b7 · claude-sonnet-5 | ≥ $0.3021 |
| agent-amimo-reviewer-3c3215e6d7373c0d · claude-sonnet-5 | ≥ $0.2653 |
| agent-aglm-converge-0afa307f0a530a99 · claude-sonnet-5 | ≥ $0.2264 |
| agent-aqwen-reviewer-11144dff06f113d3 · claude-sonnet-5 | ≥ $0.2228 |
| agent-akimi-reviewer-7c4c7e2168076ff5 · claude-sonnet-5 | ≥ $0.1852 |
| agent-agrok-reviewer-f684f16a0a212d21 · claude-sonnet-5 | ≥ $0.1676 |
| agent-aqwen-reviewer-c72d3ec92b364a4b · claude-sonnet-5 | ≥ $0.1607 |
| agent-agrok-reviewer-747a9bd7129a2672 · claude-sonnet-5 | ≥ $0.1421 |
| agent-akimi-reviewer-96624d90c821abe9 · claude-sonnet-5 | ≥ $0.1394 |
| agent-adeepseek-reviewer-44ca6585ba35900a · claude-sonnet-5 | ≥ $0.1222 |
| agent-aminimax-reviewer-c8346e3fa415d7ce · claude-sonnet-5 | ≥ $0.1202 |
| agent-aglm5-reviewer-113ee1caa7f489cd · claude-sonnet-5 | ≥ $0.1180 |
Generated by aireceipts
8d16513 to
334adfe
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 334adfe0f9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| yield* fs.remove(target, { force: true }).pipe(Effect.ignore) | ||
| yield* fs.rename(staged, target) |
There was a problem hiding this comment.
When two Windows processes resolve a cold cache concurrently, the first one can publish target after this process already passed the fs.isFile(target) check; if this rename then fails because the destination appeared or is locked by the first process, this retry deletes that valid binary before attempting its own rename. Although the new staging names are unique, the current code still removes a concurrently published target, so a peer can hit ENOENT while spawning rg.exe or this process can memoize a resolve failure when the existing binary was actually usable; prefer treating an existing target as success or serializing the install instead of removing it.
Useful? React with 👍 / 👎.
Two production defects found in Azure telemetry over 2026-07-22 → 2026-08-05. **Windows `grep` broken for 99 machines** `core_failure` showed 328 events across 99 distinct Windows machines (of 617 total) carrying `? is not recognized as an internal or external command, operable program or batch file.` and its German, French, Spanish and Portuguese translations. Present on released 0.9.2, 0.9.3 and 0.9.4. Root cause: ripgrep's Windows release is a zip, and `RipgrepBinary` extracted it via `powershell.exe -Command Expand-Archive`, falling back to the literal string `"powershell.exe"` when neither `powershell.exe` nor `pwsh.exe` resolved. `cross-spawn`'s `parseNonShell()` sets `needsShell = true` when `resolveCommand()` returns undefined and re-spawns through `cmd.exe /d /s /c`, so cmd.exe produced that message. `throw new Error(result.stderr.trim())` made it the error verbatim, and since `RipgrepBinary.filepath` is `Effect.cached`, one failed extraction broke grep for the whole session. Upstream carries the same fragility: anomalyco/opencode#24291 is open, reporting `Expand-Archive` unusable when spawned from the Bun-compiled binary, affecting `grep`, `glob` and `skill`. Their #23457 fix only corrected how paths were passed to PowerShell (the `$args` → inlined-and-escaped form we already carry); it did not remove the dependency on PowerShell being resolvable. Extract the zip in-process with `@zip.js/zip.js`, converging on the approach the `packages/opencode/src/file/ripgrep.ts` shim already uses in production. `unzipExecutable` is exported so archive handling is tested directly, and decodes with `checkSignature: true` — zip.js defaults it off, and a CRC-corrupt download would otherwise be written to the cache and trusted by every later session. Install the binary atomically (stage to `rg.exe.tmp`, then rename). `filepath` trusts the cached binary on existence alone, so an interrupted write previously left a truncated `rg.exe` that every later session reused — the same permanent breakage `checkSignature` guards against, which CRC cannot catch because it is verified before the write. The tar path installs the same way. Attribute resolution failures. Child stderr was reported verbatim, so a shell-level failure was indistinguishable from a tool bug; and because a resolve failure is memoized, it is re-reported on every later grep in the session. The tar branch now names ripgrep, and any typed filesystem or HTTP failure is wrapped as `ripgrep binary resolve failed: …`. Note the blast radius is wider than the `grep` tool: `@opencode-ai/core/ripgrep` also backs the HTTP-API file handlers and `cli/cmd/debug/ripgrep.ts`. **Test runs shipped telemetry to the production resource** 1,020 of 3,135 machine ids in the same window emitted `provider_id="test"` / `cli_version="local"` — test processes that regenerate their machine id every run, inflating install and active-machine counts by roughly a third. `doInit()` gated only on `ALTIMATE_TELEMETRY_DISABLED`. Refuse the baked-in connection string when `NODE_ENV=test`, `BUN_TEST`, `VITEST` or `JEST_WORKER_ID` is set. Keyed on test runners, deliberately not on CI: `altimate-code-actions` wraps this CLI, so every run of that shipped product sets `CI`/`GITHUB_ACTIONS`, and gating on those would blind a real product surface. `bun test` sets `NODE_ENV=test`, which covers CI and developer machines alike. An explicit `APPLICATIONINSIGHTS_CONNECTION_STRING` is always honoured, so suites with their own sink are unaffected; `ALTIMATE_TELEMETRY_FORCE=true` overrides the default-sink refusal, and the existing opt-outs still win over both. Test plan: 8 ripgrep tests — including a layer-level test that drives `filepath` through the Windows zip path with a spawner that fails if invoked, asserting the staged-then-renamed install, and a CRC case that fails without `checkSignature` — plus 15 telemetry-gate tests covering CI-alone-still-reports and one that relies on the real runner's `NODE_ENV`. `turbo typecheck` green, marker guard green, `packages/core` and `packages/opencode` suites show no failures beyond the pre-existing set on `main`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
334adfe to
83961ac
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83961acb83
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Run from packages/core so `effect` and the other deps resolve — they are not root deps. | ||
| - name: Resolve ripgrep with PowerShell unavailable | ||
| working-directory: packages/core | ||
| run: bun run script/windows-ripgrep-e2e.ts |
There was a problem hiding this comment.
On PRs that change packages/core/**, this Windows job installs the core deps but only runs script/windows-ripgrep-e2e.ts; I checked the workflows with rg and there is no invocation of packages/core/test/ripgrep-windows.test.ts or the core test suite, while the main TypeScript job runs bun test from packages/opencode. That leaves the new no-spawn/CRC/empty-archive regression coverage unexecuted in CI, so a broken in-process extractor can still merge as long as this E2E script passes; add the core regression test, or a focused core test pass, to this job.
Useful? React with 👍 / 👎.
Now tested on real Windows — with an important caveatAnswering the direct question: until now, no — nothing here had run on Windows, which was the obvious gap given this fixes a Windows-only failure. There is now a Output from the run on this PR: So on real Windows the new path downloads, extracts in-process, installs atomically and produces a working binary. The staging assertion also covers the atomic-install change reviewers asked for. What this job does not establishI first wrote it to assert that the old PowerShell approach fails under the same conditions, so the run would be a true counterfactual. That assertion failed — and the failure is the useful part: Emptying I have therefore not reproduced the affected machines, and I am not claiming this job proves the fix resolves #1072 for them. The probe is now informational and prints that caveat rather than failing. What carries that argument instead is structural, and it is platform-independent: Other e2e coverage added this roundThe subprocess/e2e pass CI runs separately ( One incidental find: |
1 participant
Issue for this PR
Closes #1072
Closes #1073
Type of change
What does this PR do?
Two defects found while reading Azure telemetry for 2026-07-22 → 2026-08-05.
1.
grepwas broken for 99 Windows machines (#1072).ripgrep's Windows release is a zip, and we extracted it by shelling out to
powershell.exe -Command Expand-Archive, with a literal"powershell.exe"string as thefallback when neither
powershell.exenorpwsh.exeresolved. cross-spawn'sparseNonShell()setsneedsShell = truewheneverresolveCommand()returns undefined andre-spawns through
cmd.exe /d /s /c, so cmd.exe replied'…' is not recognized as an internal or external command. We threw that string verbatim, and sinceRipgrepBinary.filepathisEffect.cached, one failed extraction killedgrepfor the whole session.This extracts the zip in-process with
@zip.js/zip.js, so there is no external shell toresolve. That is the same approach
packages/opencode/src/file/ripgrep.tsalready uses — whichis why
glob/ls/skillnever showed this failure whilegrepdid.Three things came out of review and are included:
checkSignature: true. zip.js defaults CRC verification off, so a corrupt downloaddecodes "successfully", gets written to the cache, and is trusted by every later session.
filepathtrusts the cached binary on existence alone, so an interruptedwrite left a truncated
rg.exethat was reused forever. Both paths now stage torg.exe.tmpand rename. CRC cannot catch this — it is verified before the write.
like a tool bug. A resolve failure is also memoized, so it is re-reported on every later grep.
Typed FS/HTTP failures are now wrapped as
ripgrep binary resolve failed: ….Upstream has the same fragility open as anomalyco/opencode#24291. Their #23457 fix only changed
how paths were passed to PowerShell (the inlined-and-escaped form we already carry); it did not
remove the dependency.
upstream/devstill has the original code.2. Test runs polluted production telemetry (#1073).
1,020 of 3,135 machine ids were test processes (
provider_id="test",cli_version="local"),which regenerate their machine id every run — inflating install and active-machine counts by
about a third.
doInit()now refuses the baked-in connection string underNODE_ENV=test,BUN_TEST,VITESTorJEST_WORKER_ID.Keyed on test runners and deliberately not on CI:
altimate-code-actionswraps this CLI, soevery run of that shipped product sets
CI/GITHUB_ACTIONS, and gating on those would blind areal product surface.
bun testsetsNODE_ENV=test, which covers CI and developer machineswith one condition. An explicit
APPLICATIONINSIGHTS_CONNECTION_STRINGis always honoured, sosuites with their own sink are unaffected;
ALTIMATE_TELEMETRY_FORCE=trueoverrides thedefault-sink refusal, and the existing opt-outs still win over both.
How did you verify your code works?
filepaththrough the Windowszip path with stubbed HTTP/FS and a spawner that throws if invoked — pinning that the
Windows path spawns nothing and that the install is staged-then-renamed. The CRC test was
confirmed to fail without
checkSignature: true.env at all so it relies on the real runner's
NODE_ENV— the assumption the gate rests on.bun turbo typecheckgreen; marker guard green.mainbaseline worktree:packages/core1,038 pass,packages/opencode11,014 pass, with zero failures unique to this branch (the remainingMCP/plugin failures reproduce on
main).Not verified on a real Windows host — I have no Windows runner. The zip decoding, install and
no-spawn behaviour are covered by tests; the end-to-end "grep works on a machine without
PowerShell on PATH" claim is inferred from the call graph, not executed.
Known follow-ups, deliberately not in this PR: bundling
rgso there is no runtime download(upstream wants this too — anomalyco/opencode#31734), unifying the two ripgrep resolvers, and
making
filepathcache only successes so a transient failure stops poisoning the session.Screenshots / recordings
Not a UI change.
Checklist
Summary by cubic
Fixes Windows grep outages by extracting ripgrep in-process (no PowerShell) and installing it atomically, and stops automated tests from sending production telemetry. Adds a Windows CI E2E that resolves and runs
rg.exewith PowerShell removed from PATH (fixes #1072, #1073).Bug Fixes
@zip.js/zip.jswith CRC; atomic install (stage → rename); clearer, named HTTP/extract errors; normalized failure messageripgrep failed with code X: …with a “no output” fallback; Windows-only E2E verifies download/extract/install/exec with PowerShell off PATH.NODE_ENV=test,BUN_TEST,VITEST,JEST_WORKER_ID); CI alone still reports; explicitAPPLICATIONINSIGHTS_CONNECTION_STRINGhonored;ALTIMATE_TELEMETRY_FORCE=trueopt-in; docs updated.Dependencies
@zip.js/zip.js@2.7.62topackages/core.Written for commit 83961ac. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Telemetry
ALTIMATE_TELEMETRY_FORCE=truestill enable telemetry, with disable overrides taking precedence.Documentation
Tests / CI