Repository navigation
Conversation
 [Medium impact] The PR appears safe to merge; no new actionable issue was established. SummaryThe PR allows Linux serve to replace an exact legacy bare-
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
Walkthrough
Priority: Severity of issue fixed: Medium Merge Risk:
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check | The description explains the problem, scope, behavior, trade-offs, linked issue, and test evidence. However, it does not follow the repository template and omits required sections such as ELI5, What C… | Rewrite the description using all required template headings. Include the missing information, mark Visual Proof as N/A with a reason if applicable, complete the testing and checklist items, and state the AI Disclosure and Agent skill upstr… | |
| Docstring Coverage | Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (3 passed)
-
Check name Status Explanation Title check ✅ Passed The title clearly and concisely describes the main change: replacing the legacy bare Orca AppImage wrapper. Linked Issues check ✅ Passed The implementation addresses issue #18985.installLinuxBareOrcaDispatcherreplaces an owned dispatcher or an exact legacy AppImage wrapper. The replacement executes the bundled or extracted `orca-id…Out of Scope Changes check ✅ Passed The changes stay within issue #18985. Production changes only expand bare-orcadispatcher ownership checks for the legacy AppImage wrapper and preserve user-owned files. The added tests verify this …
Full details: Description check
-
Explanation
The description explains the problem, scope, behavior, trade-offs, linked issue, and test evidence. However, it does not follow the repository template and omits required sections such as ELI5, What Changed, Why, Visual Proof or N/A, AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist.
Resolution
Rewrite the description using all required template headings. Include the missing information, mark Visual Proof as N/A with a reason if applicable, complete the testing and checklist items, and state the AI Disclosure and Agent skill upstream boundary status.
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Autopilot is currently an internal CodeRabbit preview.
Comment @coderabbitai help to get the list of available commands.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — reviewed the full diff at f2b35055 (2 files): the bare-orca dispatcher's replacement predicate and its new regression tests.
- Serve now reclaims the legacy wrapper — the pre-check and the displaced-file branch of
publishDispatchercall a newisReplaceableDispatcher, which accepts our own dispatcher (line-2 marker) or an exact legacy AppImage wrapper, so an unedited~/.local/bin/orcano longer survivesserveand no longer routes through AppRun's--no-sandbox. - User scripts stay safe — any other file, including a hand-edited legacy wrapper, still returns
skipped-foreign. - Tests pin both directions —
replaces the legacy AppImage wrapperfails pre-fix (pre-fix returnedskipped-foreign), andleaves a hand-edited legacy wrapper in placeguards the new clobber risk.
I verified buildLegacyAppImageCliWrapper is byte-faithful to the wrapper first shipped in 3647b53e00 (identical APPIMAGE_CLI_SCRIPT, shell body, and quoteShell), so the exact-match criterion should match real users' on-disk wrappers. Ran the two touched test files: 15 passed.
ℹ️ Serve replacement is broader than the installer's legacy-command removal
installLinuxBareOrcaDispatcher replaces any exact legacy wrapper regardless of which AppImage it names — the new test even asserts replacement when the caller passes appImagePath: null and the wrapper points at a different file. The desktop installer deliberately scopes the same cleanup to the current AppImage (cli-command-installation.ts:215, resolve(resolvedTarget) === resolve(this.appImagePath)), and cli-installer.test.ts encodes that ("only when it names the current AppImage"). This is likely intentional — the serve dispatcher is single-slot and always points at the running install, and the wrapper is byte-identical Orca output — but it is worth confirming that overwriting a wrapper naming a different Orca install is the desired behavior rather than matching the installer's stricter scope.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
f2b3505 to
84b88b0
Compare
Sync update (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 3b4e6ba0-4f46-43f4-a0ac-3032f5b4d043
📥 Commits
Reviewing files that changed from the base of the PR and between f2b35055c12d6c08b49b56c19d9794a14b3812d2 and 84b88b03caa19b4b6037bc504cc68d50504b7261.
📒 Files selected for processing (2)
src/main/cli/linux-bare-orca-dispatcher.test.tssrc/main/cli/linux-bare-orca-dispatcher.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
| if (!target) { | ||
| return false | ||
| } | ||
| return trustedAppImagePath === null || target === trustedAppImagePath |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Check callers that pass appImagePath: null or run without AppImage identity.
rg -nP --type=ts -C3 'installLinuxBareOrcaDispatcher\s*\(' -g '!**/*.test.ts'Repository: stablyai/orca
Length of output: 1660
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- dispatcher source ---'
cat -n src/main/cli/linux-bare-orca-dispatcher.ts | sed -n '1,290p'
printf '%s\n' '--- production caller context ---'
cat -n src/main/startup/main-process-runtime-launch.ts | sed -n '1,230p'
printf '%s\n' '--- affected tests ---'
cat -n src/main/cli/linux-bare-orca-dispatcher.test.ts | sed -n '210,285p'
printf '%s\n' '--- relevant PR diff ---'
git diff --unified=30 dc08ffeba9603c5b48b9b100d0a7f89b021e6d1b 84b88b03caa19b4b6037bc504cc68d50504b7261 -- src/main/cli/linux-bare-orca-dispatcher.ts src/main/startup/main-process-runtime-launch.tsRepository: stablyai/orca
Length of output: 38118
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- runtime identity files ---'
fd -t f 'appimage-runtime-identity' src
printf '%s\n' '--- runtime identity implementation ---'
identity=$(fd -t f 'appimage-runtime-identity' src | head -n 1)
cat -n "$identity" | sed -n '1,260p'
printf '%s\n' '--- runtime identity references ---'
rg -n -C4 'resolveAppImageRuntimeIdentity|hasAppImagePathEnvironment|appImagePath: null' src --type tsRepository: stablyai/orca
Length of output: 43654
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- runtime identity implementation ---'
cat -n src/main/appimage-runtime-identity.ts | sed -n '1,230p'
printf '%s\n' '--- packaged AppImage contract ---'
cat -n src/main/cli/cli-install-location.ts | sed -n '80,135p'Repository: stablyai/orca
Length of output: 9713
Require a trusted AppImage path before replacing a legacy wrapper.
isReplaceableDispatcher accepts any exact legacy wrapper when trustedAppImagePath is null. The production caller passes no appImagePath. Outside an AppImage, runtime identity is null, and the bundled launcher fallback still runs. A wrapper that points to another AppImage can then be replaced.
🐛 Suggested fix
- return trustedAppImagePath === null || target === trustedAppImagePath
+ return trustedAppImagePath !== null && target === trustedAppImagePathUpdate the test at src/main/cli/linux-bare-orca-dispatcher.test.ts:246-268 to pass a trusted AppImage path, or document the null behavior as intentional.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return trustedAppImagePath === null || target === trustedAppImagePath | |
| return trustedAppImagePath !== null && target === trustedAppImagePath |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — re-reviewed the delta since the prior pullfrog review at f2b35055 (2 files, 2 commits): the legacy-wrapper replacement predicate was narrowed to the AppImage this process is running, plus two new regression tests.
- Replacement scoped to the trusted AppImage —
isReplaceableDispatchernow takestrustedAppImagePathand reclaims an exact legacy wrapper only when itsAPPIMAGE=target equals the running AppImage (or no identity is available), so a wrapper edited to name a different AppImage is left asskipped-foreign. - New
trustedLegacyAppImagePath— prefers a caller-suppliedappImagePath(includingnull) and otherwise derives it fromresolveAppImageRuntimeIdentity; the value is threaded through both the pre-check and the displaced-file branch ofpublishDispatcher. - Tests pin the new boundary — a wrapper whose
APPIMAGE=path was edited stays in place, while one naming the running AppImage is still replaced.
This addresses greptile's original P1 (a hand-edited APPIMAGE path being overwritten). I ran src/main/cli/linux-bare-orca-dispatcher.test.ts and src/main/cli/legacy-appimage-cli-wrapper.test.ts: 17 passed. The null trusted-path wildcard applies only to byte-exact Orca-written wrappers and matches the documented overwrite behavior for a non-AppImage packaged serve.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
84b88b0 to
ea3d921
Compare
The pre-rename wrapper execs the AppImage, and AppRun injects --no-sandbox into Electron's node mode. Serve now overwrites that exact wrapper with the extracted launcher and still leaves a user's own orca script alone. Fixes stablyai#18985
A packaged serve was rebuilding the expected wrapper from the APPIMAGE= line inside the file, so a path the user changed still looked like one Orca wrote and was replaced. Co-authored-by: Cursor <cursoragent@cursor.com>
ea3d921 to
8b381c7
Compare
Description
On Linux,
~/.local/bin/orcacan still be the AppImage wrapper Orca wrote before the command was renamed toorca-ide. That wrapper execs the AppImage. AppRun then injects--no-sandboxinto Electron's node mode, so every bareorcacommand fails withbad option: --no-sandboxwhileorca-ideworks. Serve treated that wrapper as someone else's script and left it in place.Focused fix
In scope: when installing the bare-
orcadispatcher, replace a file only if it is the current dispatcher or an exact legacy AppImage wrapper.Out of scope: AppRun's sandbox flag, the
orca-ideinstaller, and any script the user edited.Preserves
A user's own
orcascript, including a legacy wrapper they have edited, is still left untouched. The replacement execs the extractedorca-idelauncher and does not pass--no-sandbox.Evidence
node node_modules/vitest/vitest.mjs run --config config/vitest.config.ts --cache false src/main/cli/linux-bare-orca-dispatcher.test.ts— 13 passed.The new cases cover replacing the exact legacy wrapper and keeping a hand-edited one. A restricted-userns Linux host was not rechecked.
User-regression-tradeoffs
An unedited wrapper Orca previously installed at
~/.local/bin/orcais overwritten on the next serve start. A script that is not that exact wrapper stays as it is.Fixes #18985