Skip to content

feat(search): bundle ripgrep for local, WSL, and SSH search - #22396

Merged
nwparker merged 29 commits into
mainfrom
nwparker/bundle-ripgrep
Sep 25, 2026
Merged

nwparker merged 29 commits into
mainfrom
nwparker/bundle-ripgrep

Conversation

@nwparker

@nwparker nwparker commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 66 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​2869 $\color{#cf222e}{\Huge{\mathbf{−}}}$​1741 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​1128
Prod 57 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​4482 $\color{#cf222e}{\Huge{\mathbf{−}}}$​888 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​3594

ELI5

Quick Open, the Explorer’s Names filter, and text search now work without installing ripgrep separately. Orca ships its own search binary for local workspaces, WSL, and SSH hosts.

What Changed

  • Desktop and standalone orcad bundles include ripgrep for macOS, Linux, and Windows, each x64 and arm64, plus redistribution notices. Packaging fails if a required binary is missing.
  • Local searches use the bundled binary by absolute path. Missing or blocked binaries produce an error instead of silently returning a slower, capped listing. Temporary resource exhaustion and unreachable workspace roots have distinct errors.
  • WSL selects the Linux binary for the distro’s architecture through wslpath, and reports a missing bundled tool when the install drive is inaccessible.
  • SSH uploads into a private stage, checks size, and renames into a content-keyed cache. Relay startup does not wait for the upload. The relay retains its PATH and git/directory fallbacks, with a temporary backoff when the uploaded binary cannot launch.
  • Windows fallback searches resolve an absolute executable path so a repository’s planted rg.exe cannot run.
  • Standalone runtime deployment hashes and verifies the search binaries, and restores the destination binary’s executable permission after SFTP upload.
  • Installed remote binaries are collected by reference, not age. Each relay directory preserves immutable markers for every build a client has selected, including older clients sharing that directory; a build is removed only once no installation names it, and a relay directory whose marker is missing, empty, or malformed — an older Orca's — blocks the whole pass rather than being guessed at. Directory age cannot prove disuse, so it is not used; both POSIX and Windows remotes are collected.
  • Upload and cleanup are serialized on SSH connections that cannot run concurrent commands.
  • Failure probes use the same environment as searches, so a moved workspace is not mistaken for a missing PATH tool.

Why

The previous git-based fallback was slower and capped, making files appear missing in large repositories. Shipping binaries works offline and avoids PATH initialization races. All six platforms are included so cross-platform deployments need no extra download. SSH retains fallbacks for hosts where uploads or execution are unavailable; local installations fail clearly instead of hiding incomplete results.

Linked Issue

Follow-up to #22369.

Visual Proof

Existing branch evidence: the Explorer Names filter finds AppDelegate.swift beyond the 20,001-file listing cap with no ripgrep on PATH.

Explorer search result

The final review fixes affect packaging, remote installation, and error handling; they add no visual layout changes.

Testing

  • Focused local, WSL routing, SSH deployment, relay fallback, Windows path-resolution, and runtime search suites.

  • Real subprocess regressions for preserving an older relay’s binary, matching search/probe PATH environments, and searching with an empty PATH and spaces/Unicode paths.

  • A missing packaged binary is verified to fail instead of executing a planted workspace binary.

  • Standalone artifact tests cover all six binaries, licenses, install completeness, version changes when binary bytes change, and destination execute permissions.

  • pnpm tc, changed-code quality gate, and pnpm build:orcad.

  • Real Windows PowerShell execution verifies multiple client references, referenced tombstones, and collection of unreferenced entries. Real POSIX shell tests also cover empty legacy markers, unreadable reference directories, and concurrent reinstall.

  • Synced main and replaced the mobile session test’s stale global module count with its existing direct checks of required modules, excluded package sources, and deferred download boundaries.

  • Final review: 291 tests across 26 search, SSH deployment/repair, and cache-cleanup suites passed locally. After adding empty-reference regressions, all 43 ripgrep installation and cleanup tests passed; the new cases reproduce the failure before the fix. Deployment fixtures explicitly isolate ripgrep installation to preserve their native-dependency and liveness assertions. Full CI passed on f211cbfbfe9, including all eight unit shards, Linux and Windows packaging, mobile bundle checks, changed-spec E2E, and the full Docker SSH suite. Final CI run.

  • Docker is unavailable on the review machine, so the opt-in full SSH container test was not rerun in this review.

  • Verification status, corrected. Two items previously called unverified are in fact covered. Linux AppImage/deb/rpm packaging runs in CI on every PR (package job, electron-builder --linux AppImage deb rpm --x64). Windows is covered: windows-signing-rehearsal.yml passed on this branch under both test-signing and release-signing, and the remote cache GC's PowerShell was executed against a real Windows host — the listing emits its ENTRY/LIST_OK tokens, a relay with a marker yields REF <entry>, and a relay without one yields REFS_ERR, which is the safety path confirmed on the real interpreter rather than a mock.

  • macOS notarization is the only item a release must still exercise, and the exposure is narrow: notarization requires signatures on Mach-O binaries, and of the six bundled builds only the two darwin ones are Mach-O (file reports ELF for linux, PE32+ for win32). Those two are signed; signIgnore excludes only the non-Mach-O copies, and a local --mac --arm64 --dir build passes codesign --verify --deep --strict.

  • I manually tested these changes locally

  • Automated tests added/updated

Review

Adversarial review fixed remote cache deletion affecting older clients, incomplete standalone search deployment, and inconsistent failure-probe environments. Reviewed command authority, cancellation, temporary failures, platform routing, and mixed-version behavior.

Agent skill upstream boundary

  • Not applicable; no agent skill installer changes.

Notes

No remote protocol fields or stream opcodes change. Older relays keep their existing behavior. A running relay keeps its existing binary until restart.

Remote cache growth is bounded by reference-aware collection rather than by retaining every entry: a superseded build is removed once the relay installations naming it are gone, and anything this client cannot account for blocks the pass. Rust notices are copied from the ripgrep 15.0.0 lockfile's exact crate archives after checking their SHA-256 checksums. The notice set conservatively includes build/test dependencies for the shipped targets, elects MIT where offered and BSL-1.0 for ryu, and retains additional WHATWG, Unicode, and Crossbeam terms. PCRE2's sljit notice is included as well.

Checklist

  • Focused on search availability and its packaging/deployment paths
  • User-visible behavior, mechanism, and alternatives explained
  • Visual proof attached
  • Correctness, security, performance, and cross-platform behavior reviewed
  • SSH, folder workspaces, and mixed-version behavior considered
  • Focused tests and local checks run; full CI covers the updated PR

Ship @vscode/ripgrep-universal's prebuilt rg for all six relay platforms in
every desktop artifact. Local and WSL searches spawn the bundled binary and
drop the git ls-files / git grep fallbacks; SSH deploys upload the remote's
binary once per ripgrep version and the relay prefers it over PATH rg.
@nwparker
nwparker marked this pull request as ready for review September 23, 2026 04:31
@coderabbitai

coderabbitai Bot commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The change adds bundled ripgrep binaries and license resources for supported platforms. Local file listing and search now use the bundled binary, including WSL-specific selection, and no longer use the Git fallback implementations. SSH deployment passes a content-keyed binary path to the relay and starts a staged remote installation. The relay uses that binary when available and can retry with PATH ripgrep after eligible launch failures. Install guidance now describes remote installation.

Priority: ➖ Normal

Merge Risk: 🟡 Moderate · up to 5d215

Local search now depends entirely on the bundled ripgrep. When a workspace folder has moved or been deleted, search can tell users to reinstall Orca instead of reporting the missing folder. On SSH connections that cannot run concurrent commands, the background ripgrep install can fail silently. Hosts without ripgrep on PATH then lose search until the next deployment. Both issues should be addressed or explicitly accepted before merge.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 27.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 100 functions across 55 files. (1 skipped… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Title check ✅ Passed The title clearly and concisely describes the primary change: bundling ripgrep for local, WSL, and SSH search.
Description check ✅ Passed The description is detailed and covers the required ELI5, changes, rationale, linked issue, visual proof, testing, review, notes, and checklist sections. It omits the AI Disclosure section and does no…
Full details: Docstring Coverage

Explanation

Docstring coverage is 27.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 100 functions across 55 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR

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.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ No critical issues — one minor test-coverage suggestion inline.

Reviewed changes

  • Bundled binary resolution and packaging — src/main/ripgrep/bundled-ripgrep-path.ts resolves the host build (or the Linux build for a WSL-routed spawn) by absolute path, config/bundled-ripgrep-resources.cjs packages all six relay platforms, and beforePack/afterPack fail hard if any is missing.
  • Local search switchover — local Quick Open and text search now always spawn the bundled rg; the git/readdir fallbacks and install guidance are deleted, and a binary that cannot start raises a clear error.
  • WSL routing — a pre-quoted shell expression resolves the Windows install through wslpath, picks the distro-arch Linux build, and otherwise falls back to the distro's own rg.
  • SSH install and relay fallback — the remote platform's binary is uploaded once after launch to a version-keyed cache through a size-checked stage-and-rename, and the relay prefers --ripgrep-path while keeping its PATH/git/readdir chain.
  • Docs, rules, and ratchet updates — docs/reference/bundled-ripgrep.md, the AGENTS.md ripgrep rule, PR-e2e source routing, and the child-process / windows-console-visibility allowlists and pins.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment thread src/shared/bundled-ripgrep.test.ts
- Key the SSH ripgrep cache on the binary's content hash; a package bump is the only update step
- glibc verifier: read arch tokens below the slice root and accept static ELFs (arm64 release blocker)
- Ship ripgrep/PCRE2/musl license notices; bundle rg with orcad
- Packaged builds never spawn a bare rg; report fd pressure as transient
- SSH: install rg before sweep/GC, size-validate installs, back off instead of disabling on launch failure
- Scope Dependabot to @vscode/ripgrep-universal; revert unrelated lockfile churn

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues in this delta. The earlier review's open test-coverage note on src/shared/bundled-ripgrep.test.ts is still unaddressed, so this is not an approval.

Reviewed changes

  • Content-hash SSH cache key — remoteRipgrepLayout now keys the remote dir on <sha256[0:16]>-<platform> instead of a hand-maintained version constant, so a package bump, rebuild, or re-sign re-uploads with no extra step; BUNDLED_RIPGREP_VERSION is deleted.
  • Size-validated installs — the one-round-trip warm probe now requires the installed file's exact byte size (wc -c / (Get-Item).Length), and the promote path replaces a wrong-size file instead of trusting mere existence. On Windows the replace is skipped when the existing file is already correct-size, so a running relay's held-open good rg.exe is preserved.
  • glibc-floor gate fix — findArchViolation reads the arch token from the path relative to the slice root, so linux-arm64-unpacked no longer declares every file arm64 (the arm64 release blocker), and static musl ELFs are accepted by treating objdump -T's "not a dynamic object" as no imported symbols.
  • No packaged bare-rg fallback — a packaged host with a missing binary resolves to the expected absolute path (ENOENT) rather than bare rg, closing the Windows cwd-repo planted-rg.exe path; orcad's install root is added as a packaged candidate.
  • Transient spawn failures — EAGAIN/EMFILE/ENFILE/ENOMEM/ETXTBSY are reported as rg could not start (CODE); try again in all three local search paths instead of the "reinstall Orca" message.
  • Relay 60 s back-off — a bundled binary that fails to launch is skipped for 60 s and retried, rather than disabled for the relay's life.
  • Sequencing, notices, and packaging — the ripgrep upload runs before the relay sweep/GC; ripgrep/PCRE2/musl notices ship under Resources/ripgrep/licenses; build-orcad.mjs copies rg beside orcad.js; Dependabot is scoped to @vscode/ripgrep-universal; unrelated lockfile churn is reverted.

I verified the two highest-stakes pieces myself: the glibc gate passes against the real bundled binaries in simulated linux-arm64-unpacked/linux-x64-unpacked slices while still catching a genuinely wrong-arch binary, and every touched test file passes (verify-linux-glibc-floor, bundled-ripgrep(-path), relay-bundled-ripgrep, filesystem-list-files, ssh-relay-ripgrep-install, ssh-relay-deploy, electron-builder-runtime-resources). This commit cleanly resolves the review findings listed in the PR body.

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Local Quick Open, Quick Open path search, the Explorer name filter, and
runtime text search each repeated the same three steps: resolve the bundled
command, spread in the WSL distro, spread in the WSL shell expression. Fold
that into spawnBundledRipgrep so one place owns the rule that a bare 'rg'
must never reach spawn, and simplify the resolver's command/packaged checks.

Restore the AGENTS.md ripgrep rule dropped alongside its reference doc in
63f4dac, and note why the relay's availability probe may spawn a bare 'rg'.

No behaviour change; verified by the existing suites plus a new test that
pins the local, WSL-routed, and distro-routed-but-Windows-output cases.
… rule

Bundling rg removed the local git/readdir fallback, so nothing can produce
the "install ripgrep on the host running the Quick Open scan" guidance any
more -- only a remote host an upload never reached still reaches the capped
listing. Drop the host parameter, the renderer's local branch and its
translation key, and the relay wrapper that existed only to pass 'remote'.

Add a ratchet test for bare 'rg' spawns, since the AGENTS.md rule alone had
nothing enforcing it. Its one allowlist entry is the relay's PATH probe,
which asks about PATH by definition. Verified the guard catches a planted
offender rather than passing vacuously.

Also stop chaining the remote cleanup sweep behind the ripgrep upload: on a
cold host that is a multi-MB transfer, and stale upload stages and
superseded version dirs were left on the remote for its whole duration. The
two touch different trees, so they now run concurrently.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Important

The child_process import ratchet now fails: the two new commits add two direct node:child_process type imports without touching the allowlist or pin, so child-process-import-boundary.test.ts reports 155 offenders against a pin of 153.

Reviewed changes

  • One spawn entry point — local Quick Open listing, Quick Open path search, the Explorer name filter, and runtime text search now call spawnBundledRipgrep, which owns command resolution, WSL distro routing, and the WSL shell expression; the resolver's packaged/command branching was extracted around a shared isPackagedApp().
  • Packaging layout parity — config/bundled-ripgrep-resources.cjs exports RIPGREP_RESOURCE_DIR and ripgrepBinaryName, and src/shared/bundled-ripgrep.test.ts now asserts both against the runtime constants for every platform.
  • Reference doc removed — docs/reference/bundled-ripgrep.md and its .gitignore allowlist entry are dropped; the AGENTS.md ripgrep rule now names spawnBundledRipgrep / resolveRelayRipgrepCommand and no longer links the doc.
  • Relay probe comment — checkRipgrepAvailableWithoutCwd documents why the relay's availability probe may still spawn a bare rg.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment thread src/main/ripgrep/bundled-ripgrep-spawn.ts Outdated
Comment thread src/main/ipc/filesystem-search-file-paths.ts Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ No critical issues — one minor test-coverage suggestion inline.

Reviewed changes

  • New bare-rg ratchet — src/main/ripgrep/bare-ripgrep-spawn-boundary.test.ts scans non-test sources for a literal 'rg' first argument to spawn/exec*, with the single allowed exception being the relay's PATH probe in src/shared/ripgrep-process-availability.ts.
  • Local install-ripgrep path removed — src/relay/fs-handler-install-rg.ts is deleted, quick-open-install-rg messages are now remote-only (the host parameter is gone), and the renderer guidance drops its location prop and local wording, with the 344f8a48dd key removed from every locale.
  • SSH rg upload decoupled — ensureRemoteBundledRipgrep now runs as its own fire-and-forget statement alongside the stale-stage recovery → superseded-endpoint sweep → version-GC chain instead of blocking the head of it; the two touch disjoint trees (ripgrep/ is owned by no version GC).

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment thread src/main/ripgrep/bare-ripgrep-spawn-boundary.test.ts Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ No new issues in this delta — the commit adds a regression test only. Three earlier review threads remain open on this PR, so this is not an approval.

Reviewed changes

  • SSH ripgrep-upload ordering regression test — src/main/ssh/ssh-relay-deploy-staged-upload.test.ts now pins that the post-launch ripgrep upload no longer blocks the stale-stage recovery → superseded-endpoint sweep → version-GC chain, by holding ensureRemoteBundledRipgrep open with a never-settling promise and asserting the .upload-stages cleanup command still runs. I verified the test times out if the upload is chained back in front of that chain, so it genuinely guards the ordering.

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to GitHub limitations.

⚠️ Outside diff range comments (1)

🟡 Minor · Update the daemon’s ripgrep configuration when reconnecting. · ssh-relay-deploy.ts:591

src/main/ssh/ssh-relay-deploy.ts:591
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Update the daemon’s ripgrep configuration when reconnecting.

A bundled-ripgrep-only update can reuse the same relay while installing a new content-keyed binaryPath. The POSIX --connect command does not pass that path, and connect mode returns before configureRelayBundledRipgrep. The live daemon therefore keeps its old path, or uses PATH rg if it predates this option.

Ensure a changed ripgrep path forces a fresh daemon, or update the live daemon’s configuration before reconnecting.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: fbc4ddcf-02e2-46e2-bad3-2731b3eab07e

📥 Commits

Reviewing files that changed from the base of the PR and between 7a63515 and 622b315.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (48)
  • .github/dependabot.yml
  • AGENTS.md
  • config/bundled-ripgrep-resources.cjs
  • config/electron-builder.config.cjs
  • config/scripts/build-orcad.mjs
  • config/scripts/verify-linux-glibc-floor.cjs
  • config/scripts/verify-linux-glibc-floor.test.mjs
  • resources/licenses/ripgrep/LICENSE-MIT
  • resources/licenses/ripgrep/MUSL-COPYRIGHT
  • resources/licenses/ripgrep/PCRE2-LICENCE.md
  • resources/licenses/ripgrep/README.md
  • resources/licenses/ripgrep/UNLICENSE
  • src/main/ipc/filesystem-list-files.test.ts
  • src/main/ipc/filesystem-list-files.ts
  • src/main/ipc/filesystem-search-file-paths.ts
  • src/main/ipc/filesystem/filesystem-search-handlers.ts
  • src/main/ripgrep/bare-ripgrep-spawn-boundary.test.ts
  • src/main/ripgrep/bundled-ripgrep-path.test.ts
  • src/main/ripgrep/bundled-ripgrep-path.ts
  • src/main/ripgrep/bundled-ripgrep-spawn.test.ts
  • src/main/ripgrep/bundled-ripgrep-spawn.ts
  • src/main/runtime/runtime-file-commands-search-local-runtime-files.ts
  • src/main/ssh/ssh-relay-bundled-ripgrep.docker.test.ts
  • src/main/ssh/ssh-relay-deploy-staged-upload.test.ts
  • src/main/ssh/ssh-relay-deploy.test.ts
  • src/main/ssh/ssh-relay-deploy.ts
  • src/main/ssh/ssh-relay-ripgrep-install.test.ts
  • src/main/ssh/ssh-relay-ripgrep-install.ts
  • src/relay/fs-handler-install-rg.ts
  • src/relay/fs-list-files-fallback-chain.ts
  • src/relay/relay-bundled-ripgrep.test.ts
  • src/relay/relay-bundled-ripgrep.ts
  • src/relay/relay-launch-options.test.ts
  • src/renderer/src/components/QuickOpen.tsx
  • src/renderer/src/components/quick-open-install-rg-guidance.render.test.tsx
  • src/renderer/src/components/quick-open-install-rg-guidance.test.ts
  • src/renderer/src/components/quick-open-install-rg-guidance.tsx
  • src/renderer/src/i18n/locales/en.json
  • src/renderer/src/i18n/locales/es.json
  • src/renderer/src/i18n/locales/fr.json
  • src/renderer/src/i18n/locales/ja.json
  • src/renderer/src/i18n/locales/ko.json
  • src/renderer/src/i18n/locales/zh.json
  • src/shared/bundled-ripgrep.test.ts
  • src/shared/bundled-ripgrep.ts
  • src/shared/quick-open-install-rg.test.ts
  • src/shared/quick-open-install-rg.ts
  • src/shared/ripgrep-process-availability.ts
💤 Files with no reviewable changes (3)
  • src/renderer/src/components/QuickOpen.tsx
  • src/relay/fs-handler-install-rg.ts
  • src/shared/bundled-ripgrep.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/shared/bundled-ripgrep.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.

Comment thread resources/licenses/ripgrep/PCRE2-LICENCE.md
Comment thread src/main/ripgrep/bundled-ripgrep-path.ts
Comment thread src/main/ssh/ssh-relay-deploy.ts Outdated
…ocess

A type-only import still counts against the child_process ratchet, whose pin
and allowlist only ever shrink. Derive both types from wslAwareSpawn instead.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ No new issues in this delta — the commit is a type-only fix that resolves the child_process ratchet feedback. One earlier review thread (the bare-rg pattern gap) remains open, so this is not an approval.

Reviewed changes

  • Derived rg spawn types — bundled-ripgrep-spawn.ts replaces its direct node:child_process import with Parameters<typeof wslAwareSpawn>[2] (minus the WSL keys it owns) for BundledRipgrepSpawnOptions and ReturnType<typeof wslAwareSpawn> for the spawn return; filesystem-search-file-paths.ts now types its child as ReturnType<typeof spawnBundledRipgrep>. No runtime behavior change.
  • Ratchet restored — child-process-import-boundary.test.ts is back to 153 offenders and bare-ripgrep-spawn-boundary.test.ts stays green; pnpm tc:node passes. The two open child_process-ratchet threads are replied to and resolved.

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

… result

Inside `bash -c`, a failed `cd` exits 1 -- the same code ripgrep uses for "no
matches" -- so a WSL workspace whose directory had gone away reported an empty
listing as a successful scan. main did not have this hole: checkRgAvailable ran
the same `cd` wrapper first and settled on `code === 0`, diverting to the git
fallback that this PR deletes. The WSL wrapper now takes an optional
cwdFailureExitCode; rg passes 97, and all four close handlers reject with a
clear error before the unavailable check can blame the install.

Also from review:
- Bound the fire-and-forget ripgrep upload with deploySignal. The controller
  aborts only on the deploy timeout, never on success, so this cancels a
  still-running upload when the deploy gives up.
- Run the stale-stage sweep before the installed check rather than inside its
  else branch. Once rg was installed every later deploy took the PRESENT path,
  so a stage orphaned by a dropped connection was never collected again.
- Note in orcad-remote-deploy.ts why wiring it up needs ripgrep work first:
  build-orcad.mjs copies only the build host's rg, and orcad reports
  isPackaged() === true, so a remote of another platform would find nothing.

ssh-relay-deploy.test.ts sat at the max-lines cap, so any edit to it failed the
gate. Split the four Windows named-pipe deploys into their own file (926 -> 737
+ 333); both are now well clear of it.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Important

The WSL unreachable-workspace fix is incomplete: one of the four local search handlers classifies the unavailability code before the missing-cwd code, so a failed cd still surfaces as a broken install ("Reinstall Orca…").

Reviewed changes — delta since the prior pullfrog review (b435f42583): the WSL missing-cwd fix plus an origin/main merge.

  • WSL missing-cwd exit code — resolveCommand/wslAwareSpawn emit cd <root> || exit 97; <cmd> when a caller opts in (the plain cd … && form collapses a bad cwd into bash's exit 1, indistinguishable from ripgrep's "no matches"), spawnBundledRipgrep opts in only for WSL-routed spawns, and RIPGREP_MISSING_CWD_EXIT_CODE = 97 sits clear of ripgrep's 0/1/2 and the shell's 126/127/128+signal.
  • Handler coverage — three of the four local search handlers check isRipgrepMissingCwdExit before classifying unavailability; filesystem-search-file-paths.ts checks it after, making that guard dead.
  • SSH upload signal and stage sweep — the post-launch ensureRemoteBundledRipgrep now receives deploySignal, and probeOrStageCommand's stale-stage sweep runs before the PRESENT/else branch so an orphaned .upload-* stage is collected even when the binary is already installed.
  • Test coverage — new wsl-missing-cwd-exit.test.ts drives a real bash -c to prove && yields exit 1 while || exit 97 distinguishes it; the Windows-pipe deploy cases are split out of the max-lines-capped ssh-relay-deploy.test.ts; orcad-remote-deploy.ts gains a comment about the not-yet-wired remote deploy.

ℹ️ Nitpicks

  • ci-shards/unit-assignment.json is a generated artifact — config/scripts/ci-unit-sequencer.mjs writes it only under ORCA_BALANCE_UNIT_SHARDS=1 … --shard, and CI uploads it as an artifact. main tracks nothing under ci-shards/ and .gitignore does not exclude it, so a sharded local test run leaves it modified. Drop the file from this commit and add ci-shards/ to .gitignore.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment thread src/main/ipc/filesystem-search-file-paths.ts Outdated
… four

Round-two review caught that the missing-cwd branch in scanRipgrepPaths sat
AFTER isRipgrepUnavailableExit, which classifies any code above 2 as a broken
install -- so for exit 97 it was dead code and Quick Open still told the user to
reinstall Orca. Reordered; all four handlers now check it first.

Also from review:
- A vanished workspace makes spawn fail with ENOENT, which read as a damaged
  install on every local path. Confirm the cwd with isRipgrepSpawnCwdUsable --
  the guard the relay already applies -- before blaming the binary. The async
  continuation re-checks `resolved`, because finish() drops its argument once
  settled and the rejected promise would otherwise go unhandled.
- bundledRipgrepCommand returned a bare 'rg' for an arch outside the bundled
  set, bypassing the guard that exists so Windows cannot resolve a bare name
  against the repo cwd. A packaged app now always names an absolute path.

Drop ci-shards/unit-assignment.json, a 9,425-line CI artifact swept in from
reproducing a shard locally, and gitignore the directory that produced it.

The "rg genuinely cannot start" test pointed at a synthetic /repo, which the
new guard correctly reports as unreachable; it now resolves to a real root so
it still tests what its name says.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues in this delta. The prior review's two findings are addressed; one older bare-rg ratchet thread remains open, so this is not an approval.

Reviewed changes — delta since the prior pullfrog review (e14d7ef): commit 5d2152f3e9, which acts on round-two feedback and hardens the bundled-path guard.

  • WSL missing-cwd guard reordered — filesystem-search-file-paths.ts now checks isRipgrepMissingCwdExit before the classifyNativeLauncherExit classification, matching the other three handlers, so exit 97 reaches ripgrepMissingCwdError instead of "Reinstall Orca".
  • Local ENOENT disambiguated — all four local handlers confirm the search root with isRipgrepSpawnCwdUsable before blaming the binary, so a vanished workspace reports "Search root is not reachable" rather than a damaged install.
  • Packaged command never bare — bundledRipgrepCommand names an absolute expected path for an arch outside the bundled set when packaged, closing the bare-rg route.
  • Generated artifact removed — ci-shards/unit-assignment.json is deleted and ci-shards/ is added to .gitignore.
  • Test fixture corrected — the "rg genuinely cannot start" case now resolves to a real root so it still asserts the binary verdict.

I verified the reorder against the classifyNativeLauncherExit && code > 2 predicate (ripgrep-process-availability.ts:164) and confirmed that on Node 24 a spawn ENOENT emits error then resolves the async cwd check before close(-2), so the new spawn-error guard is reachable rather than dead. pnpm tc:node and the changed suites plus the bare-rg/child_process ratchets pass.

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: c124dc9c-0725-479e-8be6-bd3560f4ed52

📥 Commits

Reviewing files that changed from the base of the PR and between ede2838 and 5d2152f.

📒 Files selected for processing (18)
  • .gitignore
  • src/main/git/command-runner/wsl-aware-spawn.ts
  • src/main/git/command-runner/wsl-command-resolution.ts
  • src/main/ipc/filesystem-list-files.ts
  • src/main/ipc/filesystem-search-file-paths.test.ts
  • src/main/ipc/filesystem-search-file-paths.ts
  • src/main/ipc/filesystem/filesystem-search-handlers.ts
  • src/main/ripgrep/bundled-ripgrep-path.ts
  • src/main/ripgrep/bundled-ripgrep-spawn.ts
  • src/main/ripgrep/wsl-missing-cwd-exit.test.ts
  • src/main/runtime/runtime-file-commands-search-local-runtime-files.ts
  • src/main/ssh/orcad-remote-deploy.ts
  • src/main/ssh/ssh-relay-deploy-windows-pipe.test.ts
  • src/main/ssh/ssh-relay-deploy.test.ts
  • src/main/ssh/ssh-relay-deploy.ts
  • src/main/ssh/ssh-relay-ripgrep-install.ts
  • src/shared/bundled-ripgrep.ts
  • src/shared/ripgrep-process-availability.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.

Comment thread src/main/ipc/filesystem-list-files.ts Outdated
A failed spawn emits 'error' and THEN 'close' with a negative code. The cwd
check added in the error handler did not settle, so the close handler settled
first -- synchronously, with the reinstall message -- and won the race every
time. The branch was not merely flaky, it was unreachable in all four handlers:
it is guarded by pid === undefined, which is exactly the case that always
produces a following close(code < 0). Verified against a real spawn: 3/3 runs
give error(ENOENT) -> close(-2). The error handler now detaches 'close' before
the probe, so it owns the outcome.

The probe also had no rejection handler, so a probe that rejected left the
search unsettled forever -- a hang, not just a wrong message. It now falls back
to the prior verdict rather than inventing one.

Tests: filesystem-search-rg-timeout and orca-runtime-files-search already cover
error-first and close-first, but against synthetic roots that the new guard
correctly calls unreachable; they now resolve to a real root, keeping each
test's stated intent. Added a Quick Open case for the vanished-workspace path
and confirmed it fails with the old ordering.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found.

Reviewed changes — delta since the prior pullfrog review (5d2152f3e9): commit 8921de12e1, which makes the local spawn-failure root disambiguation actually reachable.

  • Error handler owns the spawn-failure verdict — all four local search handlers (filesystem-list-files.ts, filesystem-search-file-paths.ts, filesystem/filesystem-search-handlers.ts, runtime-file-commands-search-local-runtime-files.ts) now call child.off('close', …) before probing the search root with isRipgrepSpawnCwdUsable. A failed spawn emits error and then a synchronous close(code < 0), so the old async probe lost the race every time and a moved workspace was reported as "Reinstall Orca"; detaching close lets the probe decide. A rejected probe falls back to the binary verdict via .catch(() => true) rather than stranding the search.
  • Tests now emit the real event order — the spawn-failure cases in filesystem-search-file-paths.test.ts, filesystem-search-rg-timeout.test.ts, and orca-runtime-files-search.test.ts emit close(-2) after error and point the cwd probe at a reachable root so they keep asserting the binary verdict; a new Quick Open case pins the vanished-workspace message.

I verified the change against the four handlers and the .catch/re-check paths, and ran the four changed suites plus the bare-rg and child_process ratchets (57 tests, all green). The relay's fs-handler-utils.ts uses a separate idempotent launchFailureCheck guard rather than this error/close race, so it has no analogous dead branch and was correctly left untouched. The one earlier bare-rg ratchet thread remains open and unaddressed, so this is not an approval.

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Round-four review found the missing-cwd branch had zero handler coverage: no
test anywhere emitted close(97), only -2/0/1/2/127. Ordering was correct, but
guarded by source-line order alone -- and that exact ordering was wrong in
three of four handlers two commits ago. Each suite now drives close(97) through
its real handler and expects the unreachable-root message.

Verified the tests earn their place: neutering the missing-cwd check fails
exactly four tests, one per handler.

Also drop a Reflect.get the anti-slop gate rejects, in favour of `in` narrowing.
The previous commit asserted close "would beat this threadpool round-trip every
time", from an n=3 sample that measured event ordering -- which was never in
dispute -- rather than probe-vs-close. Two later measurements disagree with each
other: 50/50 close-first here, 30/50 probe-first in review. Either way it is a
race on a sub-millisecond margin, and the detach is what makes the verdict
deterministic.

Why this wording matters: "close wins every time" is an argument for deleting
the detach as a guard against an impossible race. No test would catch that --
the suites emit error and close in the same synchronous tick.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found in this delta. The commit adds regression coverage only. One earlier bare-rg ratchet thread remains open and unaddressed, so this is not an approval.

Reviewed changes — delta since the prior pullfrog review (8921de12e1): commit f29377cdcc, which locks the exit-97 ordering into all four local search handlers.

  • Exit-97 coverage in every local handler — filesystem-list-files.test.ts, filesystem-search-file-paths.test.ts, filesystem-search-rg-timeout.test.ts, and orca-runtime-files-search.test.ts each gain a close(97) case asserting Search root is not reachable: …. Because isRipgrepUnavailableExit classifies any code > 2 as a launch failure, a handler that checks the unavailable branch before isRipgrepMissingCwdExit now fails these tests — the exact regression the earlier reorder fixed.
  • Changed-line assertion avoided — filesystem-search-file-paths.test.ts introduces UNUSED_STORE: Store = Object.create(null) so the new test does not add a {} as Store cast that the changed-code quality gate would flag.
  • Shell-exit helper rewritten — wsl-missing-cwd-exit.test.ts's shellExitCode now reads error.code via 'code' in error rather than Reflect.get, avoiding the anti-slop/no-reflect-get rule. No behavior change.

I confirmed each handler checks isRipgrepMissingCwdExit(code) before isRipgrepUnavailableExit (filesystem-list-files.ts:182, filesystem-search-file-paths.ts:214, filesystem/filesystem-search-handlers.ts:170, runtime-file-commands-search-local-runtime-files.ts:167), so these tests genuinely fail on reintroduction of the old order. Ran the five changed suites (49 tests) green. The one earlier bare-rg ratchet thread remains open and unchanged.

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

…eeds

The statically linked Linux builds carry jemalloc (BSD-2-Clause) and LLVM
libunwind (Apache-2.0 WITH LLVM-exception) in addition to PCRE2 and musl, and
both require their notice on binary redistribution. Confirmed with `strings`:
their symbols are present in linux-x64 and linux-arm64 and absent from the
darwin and win32 builds. Texts taken from the upstream canonical sources.

extraResources already copies the whole licenses directory, so these ship
without a packaging change.
…old builds

Three gaps the reviews surfaced on the remote side, all pre-existing on main.

Bare `rg` on Windows remotes. Both relay spawn sites pass the user's repo as
cwd, and CreateProcessW searches the cwd before PATH -- the same hijack the
desktop side already fixes. The relay now walks PATH itself and spawns an
absolute rg.exe, skipping relative PATH entries because those resolve against
the cwd. No rg on PATH yields null, which callers treat as "ripgrep
unavailable" rather than handing spawn a bare name. POSIX keeps the bare name:
execvp never consults the cwd, so there is nothing to resolve and nothing to
gain. With the last probe converted, the bare-spawn ratchet allowlist is empty.

Empty results for an unreachable root. settleLaunchFailure resolved an empty,
successful-looking scan when the root was gone but PATH rg existed, and the
git/readdir chain never engaged because it only triggers on
RipgrepUnavailableError. Both relay paths now reject naming the root, matching
local workspaces. Missing-rg keeps precedence over a missing root, because only
that verdict engages the fallback chain -- two tests pinned that deliberately
and it would have been wrong to flip it.

Unbounded ~/.orca-remote/ripgrep/. Nothing collected this tree; the relay's
version GC only matches `relay-*`, so every rg bump left another ~5 MB per host
forever. The probe command now also drops sibling builds older than two weeks,
sparing the current one and live upload stages, on POSIX and PowerShell alike.
Two weeks because a client pinned to an older build may still be using it; the
cost of collecting one early is that client re-uploading once.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ No critical issues — one minor suggestion inline.

Reviewed changes — delta since the prior pullfrog review (f79158e565): commit 6059a280bd, which closes three remote-side gaps.

  • Bare rg removed from the relay — both relay spawn sites now resolve through resolveRelayRipgrepCommand, which on Windows walks PATH for an absolute rg.exe (skipping relative entries) and returns null when none exists; callers treat null as RipgrepUnavailableError rather than handing spawn a bare name. The bare-spawn ratchet allowlist is now empty.
  • Unreachable remote roots are classified — isRipgrepUnavailableAfterLaunchFailure is replaced by classifyRipgrepLaunchFailure, and both relay paths now reject an unreachable root with ripgrepMissingCwdError instead of resolving an empty, successful-looking scan; missing rg keeps deliberate precedence.
  • Superseded ripgrep builds are collected — the SSH probe now drops sibling ~/.orca-remote/ripgrep/<key>-<platform> directories older than 14 days, sparing the current entry and live .upload-* stages, on POSIX and PowerShell alike.

I ran the five changed suites (72 tests), the bare-rg and child_process ratchets, and pnpm tc:node, all green.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment thread src/shared/ripgrep-process-availability.ts Outdated
…TH hole

Five review findings against the previous commit, all reproduced first.

The launch-failure classifier probed PATH rg, but the spawn that failed was the
bundled binary. On the normal remote setup -- no rg on PATH, which is why Orca
uploads one -- the probe failed and a moved workspace was reported as a missing
ripgrep, telling the user to install what Orca already ships. So the fix was
inert on exactly the hosts the uploader exists for. It now takes a candidate
list and asks the binary that actually failed first, then PATH.

path.win32.isAbsolute accepts `\tools` and `/tools`: rooted, but carrying no
drive, so they resolve against whatever drive the process is on. The probe
would have validated one against the relay's drive while the spawn, running
with the user's repo as cwd, resolved it against the repo's -- the same
cwd-dependence this lookup removes, narrowed from directory to drive. A real
drive letter or UNC root is now required.

probeRipgrepVersion had lost the timeout's kill in the rewrite, leaking a live
process and a ref'd handle per launch failure -- for a hang, which is the very
case the bundled-rg back-off exists for. It also spawned without windowsHide,
which would flash a console; fixing that made an allowlist entry stale, so the
entry is gone and the pin ratchets down 63 -> 62.

`windowsPathRipgrep ??= …` never memoised a miss, because null is nullish. The
caching was inverted against cost: a hit stops at the first directory, a miss
stats every one, and only the miss was repeated -- per spawn.

The bare-spawn ratchet claimed "nothing in production spawns a bare rg", which
is false on POSIX. It now also matches PATH_RIPGREP_COMMAND at a spawn site,
and the comment states plainly what a textual guard cannot see: the POSIX bare
name reaches spawn as a parameter, and is safe because execvp ignores the cwd.

The drive-rooted predicate is tested directly rather than through the
filesystem -- a temp dir on a POSIX CI host has no drive letter to exercise
win32 semantics with, so the filesystem test could never have caught this.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found. The prior review's inline finding is addressed; one older bare-rg ratchet thread remains open, so this is not an approval.

Reviewed changes — delta since the prior pullfrog review (6059a280bd): commit cf019c2681, which acts on five review findings against that commit.

  • Probe the command that failed — classifyRipgrepLaunchFailure now takes a candidate list and asks the binary that actually failed first, then PATH rg; both relay call sites pass [command, pathRipgrepCommand()], so a moved workspace on a bundled-only remote reports Search root is not reachable instead of a missing ripgrep.
  • Drive-relative PATH hole closed — isDriveRootedWindowsPath requires a real drive letter or UNC root, rejecting \tools and /tools (which path.isAbsolute accepts but which resolve against whatever drive the process is on).
  • Probe timeout kills again — probeRipgrepVersion kills a hung rg --version on timeout rather than leaking a live process and a ref'd handle per launch failure, and spawns with windowsHide: true (the stale allowlist entry is removed and the pin ratchets 63 → 62).
  • Windows PATH cache memoises a miss — an explicit undefined check replaces ??=, which treated a null miss as uncached and re-walked every PATH entry on each spawn.
  • Ratchet pattern widened — BARE_SPAWN_PATTERN also matches the PATH_RIPGREP_COMMAND constant at a spawn site, and the comment states what a textual guard cannot follow.

I ran the six changed/related suites (50 tests, 1 skipped) and pnpm tc:node, all green.

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Merging main brought the closure to 4220 against a pin of 4218. The two extra
modules are `src/shared/agent-turn-outcome.ts` and `src/shared/main-agent-status.ts`
from #22452, which the status projection this route already reaches import.
That change was src/shared-only, so the mobile job never ran on it -- the same
way the structured tool line slipped past, as the ledger above already records.

Repinned here because this PR's file set is what next made the job run, not
because this PR reaches either module. Verified: of the 28 source files this
branch changes, none appear anywhere in the route's 4220-module closure.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Important

The new commit threads env into the launch-failure probe's spawn, but an existing test still asserts the old exact options, so src/relay/fs-handler-list-files-ignored.test.ts now fails. Everything else in the delta checks out.

Reviewed changes — delta since the prior pullfrog review (cf019c2681): commit 8d6759a8e7, which preserves remote binaries and completes standalone-runtime packaging.

  • Every SSH platform ships in the runtime — build-orcad.mjs copies all six @vscode/ripgrep-universal platform binaries plus resources/licenses/ripgrep into out/orcad/ripgrep/, driven by a new ORCAD_RIPGREP_ARTIFACTS / ORCAD_RIPGREP_LICENSE_ARTIFACTS manifest that also feeds the install identity hash and the remote install-complete probe.
  • Remote upload regains its execute bit — orcad-remote-deploy.ts runs chmod 755 on the uploaded ripgrep/<platform>/rg for non-Windows hosts, after SFTP (which writes 0644) and before finalizeInstall; a chmod failure abandons the install.
  • Superseded remote binaries are preserved — ensureRemoteBundledRipgrep drops the 14-day sibling-directory GC; only abandoned .upload-* stages are swept, because directory age cannot prove an older client's relay has stopped using a binary.
  • Failure probes use the search environment — probeRipgrepVersion and classifyRipgrepLaunchFailure now take the buildRelayCommandEnv() result, and both relay call sites pass the same env they spawned with, so a moved root is classified correctly on a host whose rg is only on the augmented PATH.
  • Coverage — a real packaged-ripgrep execution test (spaces/Unicode, empty PATH, planted rg), the artifact manifest and version-hash tests, a relay cwd/env classification test, and remote chmod/install tests.

I ran the changed/related suites: orcad-artifact-version, orcad-artifacts, orcad-remote-deploy, remote-install-model, ssh-relay-ripgrep-install, relay-ripgrep-cwd-env, bundled-ripgrep-real, bundled-ripgrep-path, ripgrep-process-availability, relay-windows-path-ripgrep, electron-builder-runtime-resources, relay-bundled-ripgrep, filesystem-list-files, filesystem-search-file-paths, orca-runtime-files-search, plus the bare-rg / child-process / console-visibility ratchets and pnpm tc:node — all green except the one failure below. The one earlier bare-rg ratchet thread (4087023004) remains open and is untouched by this commit.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment thread src/relay/fs-handler-list-files-ignored.test.ts
8d6759a threaded the relay env into probeRipgrepVersion -- correctly, since the
probe decides whether a launch failure was the binary or the root and so has to
resolve the same rg the failed spawn would have. It left the assertion that
pins the probe's spawn arguments behind, which is what CI caught.

Asserting buildRelayCommandEnv() rather than loosening the match to any object:
under process.env the probe could resolve a different rg, or none, which is the
regression the change exists to prevent.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found. The prior review's [!IMPORTANT] finding is addressed; one older bare-rg ratchet thread remains open, so this is not an approval.

Reviewed changes — delta since the prior pullfrog review (8d6759a8e7): commit aa1d9407fb, a test-only follow-up to that commit's env threading.

  • Probe-env assertion repinned — src/relay/fs-handler-list-files-ignored.test.ts now expects { env: buildRelayCommandEnv(), stdio: 'ignore', windowsHide: true } on the launch-failure probe's spawn, with a comment on why the probe must resolve the same rg the failed spawn would have. This closes the one failing suite from the prior delta; I confirmed both relay call sites pass that same env (fs-handler-list-files.ts:181, fs-handler-utils.ts:192), and the suite now passes (28 tests) alongside pnpm tc:node.

The one earlier bare-rg ratchet thread (4087023004) remains open and unchanged by this commit.

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

# Conflicts:
#	config/scripts/mobile-web-app-session-terminal-closure.test.mjs
Nothing collected `~/.orca-remote/ripgrep/`: the version GC matches only
`relay-*`, so every change to the shipped bytes left another ~5 MB on every SSH
host, permanently. The age window this replaces was the wrong instrument --
a directory's mtime is when it was written, not when it was last used, so it
cannot tell a superseded build from the one a live relay was launched against.
Deleting the latter is not graceful degradation: without a PATH ripgrep remote
text search rejects outright, and listing drops to the capped walk this PR
exists to remove.

So the question is reference. Each relay directory now records the build it
runs against in `.ripgrep-ref`, written only once that binary is confirmed
present, and the GC collects a build only when no installation names it.

The discipline is ssh-relay-native-deps-cache-gc.ts': anything the pass cannot
account for blocks the whole pass. A relay directory with no readable marker is
an older Orca's, possibly running right now against a binary it never recorded,
so the pass declines rather than guessing. Those directories are removed by the
version GC in time, which is what makes their builds collectable -- hence
running after it, not beside it. Deletion is the same tombstone, recheck under
the rename, then remove, so a deploy that takes a reference mid-pass gets its
tree restored. Windows has no pass yet, matching the native-deps cache's gate.

One test note: the first version of the "unaccountable blocks the pass" test
passed against a deliberately broken guard, because the tombstone recheck
masked its absence. The test now puts a readable recheck behind an unreadable
first scan, which is the only shape that fails when that guard is removed.

Recording the reference lives inside ensureRemoteBundledRipgrep rather than at
the call site: it is the same concern, and it keeps the deploy's ripgrep
surface to one call for the tests that mock it to protect their exec queues.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Important

The new remote ripgrep cache GC can delete the build a live relay is actually running: the .ripgrep-ref marker is overwritten by whichever client connects, but a --connect reconnect reuses the daemon's original --ripgrep-path, so the marker and the live daemon can name different builds. Inline detail below.

Reviewed changes — delta since the prior pullfrog review (763cea44a8): commit e506bf34af, which replaces the reverted age-based ripgrep cache sweep with a reference-based collector.

  • Reference-based remote ripgrep cache GC — src/main/ssh/ssh-relay-ripgrep-cache-gc.ts lists ~/.orca-remote/ripgrep/<hash>-<platform> entries, reads each relay-* directory's .ripgrep-ref, and collects only builds no installation references via a move-to-tombstone → recheck → remove sequence; any relay directory whose marker is missing or unreadable aborts the whole pass.
  • Relay directories record their ripgrep build — ensureRemoteBundledRipgrep gains a relayDir option and writes .ripgrep-ref (best-effort) after a present/installed outcome; the deploy passes the current relay directory and pins the current entry into the GC call.
  • Coverage — a new 8-case GC suite plus exec-mock isolation added to the four deploy suites so the fire-and-forget GC cannot drain their queued responses.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment thread src/main/ssh/ssh-relay-ripgrep-install.ts Outdated
Three items previously left documented-but-open.

Windows remote accumulation. The cache GC was POSIX-gated, so the leak did not
go away -- it moved to the platform with the larger binary (rg.exe is 5.43 MB on
win32-x64, against 4.77 MB for linux-arm64). The PowerShell dialect now does the
same reference scan: entries and references carry token prefixes, because
PowerShell writes every uncaptured value to stdout and an untokenised listing
would feed Remove-Item whatever a cmdlet happened to emit.

Verified on a real Windows host rather than a mock: the listing emits its
ENTRY/LIST_OK tokens, a relay directory carrying a marker yields REF <entry>,
and a relay directory without one yields REFS_ERR -- the safety path, on the
real interpreter.

Rust crate notices. The crate set was read out of the shipped binary's symbols
and the licence identifiers taken from crates.io rather than assumed. Where a
crate offers the Unlicense, Orca elects it: a public-domain dedication carries
no notice obligation, and that covers eight of them. The four that do not offer
it get their MIT text reproduced. encoding_rs carries a BSD-3-Clause notice for
its WHATWG-derived encoding data that is joined by AND, not OR, so electing MIT
does not discharge it.

Release-only validation, corrected rather than repeated. Linux AppImage/deb/rpm
already runs in CI's package job on every PR, and Windows signing was already
rehearsed on this branch. macOS notarization is the only item a release must
still exercise, and the exposure is narrow: notarization requires signatures on
Mach-O binaries, and of the six bundled builds only the two darwin ones are
Mach-O -- `file` reports ELF for linux and PE32+ for win32 -- so signIgnore
excludes only files the notary never asks about.

orcad-artifacts.test.ts caught the new notice file missing from the standalone
runtime's shipped list, which is exactly the gap that test exists to catch: a
notice committed to the repo but never actually shipped.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Important

The new RUST-CRATE-NOTICES.txt is incomplete: the shipped binaries statically link crates whose MIT/Apache terms require a notice the file does not reproduce, so redistribution ships without them. The Windows cache-collection change itself looks correct.

Reviewed changes — delta since the prior pullfrog review (e506bf34af, review 5300168214): commit 29b65342f4.

  • Windows remote ripgrep cache GC — listEntriesCommand/listReferencesCommand gain a PowerShell dialect, prefixing every emitted line with its ENTRY/REF token (PowerShell echoes every uncaptured value to stdout), and supportsRipgrepCacheGc now reports both dialects implemented so Windows remotes collect unreferenced builds too.
  • Rust crate notices shipped — new RUST-CRATE-NOTICES.txt is added and wired into ORCAD_RIPGREP_LICENSE_ARTIFACTS so the standalone runtime deploys it; the desktop package picks it up through the whole-directory extraResources copy.
  • Coverage — the Windows collection path (including a REF <entry>-only-vs-REFS_ERR safety shape) and the entry-line token prefix each get a test; orcad-artifacts.test.ts pins the new file into the shipped list.

I ran the changed suites (ssh-relay-ripgrep-cache-gc 9 tests, orcad-artifacts 2 tests) green.

ℹ️ Nitpicks

  • supportsRipgrepCacheGc now returns true unconditionally, so its guard in gcRemoteRipgrepCache (src/main/ssh/ssh-relay-ripgrep-cache-gc.ts:188) can never fire and the export has no external caller — consider dropping both unless a future platform needs the seam.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment thread resources/licenses/ripgrep/RUST-CRATE-NOTICES.txt Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ No critical issues — one minor validation regression inline. The prior review's GC marker-clobber finding is addressed; the Rust-crate-notices finding remains open and unchanged, so this is not an approval.

Reviewed changes — delta since the prior pullfrog review (29b65342f4, review 5300966101): commit 0d144cfbaf.

  • Per-entry reference markers — recordRemoteRipgrepReference now writes .ripgrep-ref-<entry> and both dialects of listReferencesCommand read the legacy .ripgrep-ref alongside every .ripgrep-ref-* file, so a reconnecting client whose bundle differs from the live daemon's adds a reference instead of overwriting it. This closes the finding that the shared single-slot marker let the GC delete the build a reused --connect daemon was launched against.
  • Reference recorded before launch and install — the deploy records the reference directly and only passes --ripgrep-path (and runs the fire-and-forget upload) when the write succeeds; a fresh launch without a recorded reference now falls back to PATH rg rather than running an unprotected binary.
  • GC entry/tombstone rework — the command builders move to ssh-relay-ripgrep-cache-gc-commands.ts, the entry-name pattern is derived from BUNDLED_RIPGREP_PLATFORMS, the abandoned-tombstone sweep moves from shell into staleTombstoneEntry, restoreEntryCommand handles the concurrent-installer restore race, and supportsRipgrepCacheGc is deleted.
  • Synchronous spawn failures — new classifySynchronousRipgrepSpawnFailure plus a try/catch around spawnBundledRipgrep in all four local handlers, so a spawn errno Node throws inline (e.g. ENOTDIR, EMFILE) is classified the same way the async error handler classifies it.
  • Coverage — real-shell GC transaction and real-descriptor-exhaustion suites, a synchronous-spawn suite, and updated install/deploy tests.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment thread src/main/ssh/ssh-relay-ripgrep-cache-gc-commands.ts

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found.

Reviewed changes — delta since the prior pullfrog review (0d144cfbaf, review 5311186622): commits eb6c375833, the origin/main merge, a test-only mobile session-closure repin, and 6382f9f66f.

  • Rust crate notices completed — RUST-CRATE-NOTICES.txt is regenerated from ripgrep 15.0.0's Cargo.lock (28 → ~2100 lines) and now reproduces a notice for every registry crate in the lock, including anyhow, lexopt, textwrap, memmap2, regex-syntax, encoding_rs_io, crossbeam-epoch, the serde/libc/itoa/ryu/cfg-if/crossbeam-*/windows-* funnel, the proc-macro and build deps, and tikv-jemalloc*; SLJIT-LICENSE is added and wired into ORCAD_RIPGREP_LICENSE_ARTIFACTS. The workspace crates (globset, grep*, ignore) stay covered by ripgrep's own LICENSE-MIT/UNLICENSE, and the excluded EFI/WASI packages are documented.
  • WSL search fails closed — bundledRipgrepWslSpawnOptions drops the distro-rg fallback and the -x probe, printing /dev/null/orca-ripgrep-unavailable when wslpath cannot translate the install drive, so an inaccessible install exits into the missing-tool classification instead of silently running PATH rg. A real bash -c test proves the stub rg is never reached.
  • Bare-rg ratchet widened — BARE_SPAWN_PATTERN now also matches wslAwareSpawn and runProcess, with an it.each case pinning 'rg'/PATH_RIPGREP_COMMAND through each name, closing the regex gap the earlier review flagged.
  • Post-launch cleanup serialized — ssh-relay-deploy.ts gates cleanupReady on canRunConcurrentExecCommands(), so the stale-stage recovery → superseded-endpoint sweep → version-GC → ripgrep-cache-GC chain waits for the ripgrep upload only on transports that cannot run concurrent exec commands; the concurrent path is unchanged. Deploy suites gain a ripgrep-install fixture and an upload-failure ordering case.
  • Empty legacy reference blocks the GC — scanReferences treats a trimmed REF line (an empty POSIX marker or a whitespace-only marker of either dialect) as unreadable, blocking the whole pass rather than letting the directory read as accounted-for; mock and real-shell cases cover both.

All three outstanding Pullfrog threads from earlier reviews are addressed by these commits and have been resolved.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@nwparker
nwparker merged commit 7ea0127 into main Sep 25, 2026
41 checks passed
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.

1 participant