Keep development and production ide installs isolated - #68
Conversation
📝 WalkthroughWalkthroughThe PR separates production and development channels with distinct Tauri metadata, loopback ports, runtime lifecycle handling, signed ChangesDevelopment channel isolation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant run.sh
participant ide-dev
participant OpenPathAPI
run.sh->>ide-dev: Check development API reachability
run.sh->>OpenPathAPI: POST /api/open-path with bearer token
OpenPathAPI-->>run.sh: Return handoff result
run.sh->>ide-dev: Stop or start development instance
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
There was a problem hiding this comment.
Pull request overview
This PR isolates development (ide-dev) and production (ide) installs so running/building from a checkout cannot stop, overwrite, or reuse the Homebrew-installed production app, including separating bundle identifiers, app state, and loopback API ports.
Changes:
- Split production vs development identity/config: new
ide-devbundle identifier, separate Tauri dev config overlay, and fixed dev loopback port17878(production remains17877). - Reworked launch/install tooling so
ideand Finder Quick Action remain pinned to/Applications/ide.app, whileide-devcontinues to target the checkout runner and dev loopback endpoint. - Added regression harness/validations for channel isolation behaviors and updated the lockfile to address audit advisories.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/tauri.ts | Updates hosted dev-server API base to the dev loopback port. |
| src/tauri.test.ts | Adjusts tests to assert the dev loopback base and related edge cases. |
| src-tauri/tauri.dev.conf.json | Adds a development-only Tauri overlay with distinct identifier/name and no file associations. |
| src-tauri/src/lib.rs | Plumbs the app bundle identifier into HTTP server startup config. |
| src-tauri/src/http_server.rs | Uses fixed per-channel loopback ports (17877/17878) and validates IDE_LOOPBACK_PORT. |
| scripts/validate-tauri-config.mjs | Extends config validation to enforce development overlay invariants. |
| scripts/validate-launch-runners.mjs | Shifts runner validation to focus on channel isolation and non-mutation of production. |
| scripts/validate-finder-quick-action.mjs | Ensures generated Finder Quick Action targets production app and avoids dev-tooling. |
| scripts/validate-channel-isolation.mjs | New harness validating port/process ownership, cleanup behavior, and install-path safety. |
| scripts/install-macos-finder-quick-action.sh | Changes Finder runner to hand off to production loopback or open production app bundle. |
| scripts/install-cli-command.sh | Pins ide launcher to /Applications/ide.app and links ide-dev to checkout run.sh. |
| run.sh | Updates dev runner to use a dev-only loopback port and refuse interfering with unrelated listeners. |
| package-lock.json | Updates PostCSS/nanoid dependencies in response to audit advisory. |
| docs/development.md | Documents production vs development install behavior, ports, and new packaging/install flow. |
| dev-install.sh | Simplifies to a compatibility wrapper delegating to build.sh. |
| build.sh | Builds/signs/installs ide-dev.app to an isolated path with safety checks and verification. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address reviewer comments: - Keep MCP endpoint fallbacks on the active channel port
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
scripts/validate-launch-runners.mjs (3)
21-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSurface the child harness output on failure.
With
stdio: "pipe"and no try/catch, a failingvalidate-channel-isolation.mjsthrowsCommand failed: node …and the harness's own assertion message (which names the failing scenario) is buried inerror.stdout/error.stderr. Usestdio: "inherit", or catch and re-throw with the captured streams.♻️ Proposed change
execFileSync(process.execPath, [channelIsolationHarness], { cwd: rootDir, - stdio: "pipe", + stdio: "inherit", });🤖 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 `@scripts/validate-launch-runners.mjs` around lines 21 - 24, Update the execFileSync invocation in validate-launch-runners.mjs to use stdio: "inherit" so validate-channel-isolation.mjs output, including assertion details, is visible directly when the child harness fails.
122-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFinder-runner assertions are duplicated across two harnesses. Both files assert the same set of strings against the same generated
open-from-finder.sh, and both carry byte-identicalassertNotIncludes/assertOrderedhelpers; the shared root cause is the absence of a common assertion module, so any change to the runner requires editing two places.
scripts/validate-launch-runners.mjs#L122-L134: extract the Finder-runner assertion block (and theassertIncludes/assertNotIncludes/assertOrderedhelpers) into a shared module, e.g.scripts/lib/assert.mjs, and import it here.scripts/validate-finder-quick-action.mjs#L41-L55: import the shared assertion block/helpers instead of re-declaring them, keeping only this harness's unique setup.🤖 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 `@scripts/validate-launch-runners.mjs` around lines 122 - 134, The Finder-runner assertions and assertion helpers are duplicated across both harnesses. In scripts/validate-launch-runners.mjs lines 122-134, extract the shared Finder-runner assertion block and assertIncludes/assertNotIncludes/assertOrdered helpers into a common module such as scripts/lib/assert.mjs, then import and reuse them; in scripts/validate-finder-quick-action.mjs lines 41-55, remove the duplicate definitions and import the shared module while preserving only its unique setup.
152-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe forbidden-string list is exact-match and easy to bypass unintentionally.
Each entry must be spelled character-for-character, so a quoting change (
ditto "$DEV_APP_BUNDLE" /Applications/ide.app), a variable rename, orrm -rf /Applications/ide.appall pass this guard. Matching on the target instead — e.g. a regex for a write/destructive command whose argument is/Applications/ide.app, pluscom.gordonbeeming.ide(non-.dev) quit/pkill — makes the canary hold up as the scripts evolve.🤖 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 `@scripts/validate-launch-runners.mjs` around lines 152 - 162, Strengthen assertNoProductionMutation so it detects production-app mutations by target rather than exact command strings. Replace the brittle forbidden list with checks covering write or destructive commands targeting /Applications/ide.app, including quoting or variable variations, and checks for quitting or killing the non-development com.gordonbeeming.ide application while preserving allowed development-app operations.run.sh (1)
240-255: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: silence SC2034 for the unused loop counters.
attemptis only a repeat counter; shellcheck flags it. Renaming to_attempt(orfor _ in {1..20}) keeps the lint clean across all four wait loops.🤖 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 `@run.sh` around lines 240 - 255, Rename the unused loop counter variable attempt to _attempt (or use _) in all four wait loops, including the loops around running_dev_app_reachable, so ShellCheck no longer reports SC2034 while preserving the existing retry behavior.Source: Linters/SAST tools
build.sh (1)
118-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid
codesign --deepfor signing development apps.Apple discourages
--deepfor signing because it can apply the same entitlements across nested code and miss properly signed bundles. Sign inner code items first, then sign the outer app bundle with the desired--identifier;--deepremains useful for verification, but not for thesecodesign --signactions.🤖 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 `@build.sh` around lines 118 - 133, Update sign_dev_app so neither signing action passes --deep to codesign. Sign nested code items explicitly before signing the outer app bundle with --identifier "$DEV_BUNDLE_IDENTIFIER", while preserving verify_dev_app_signature for deep verification.
🤖 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 `@build.sh`:
- Around line 27-51: Update the validation around resolve_dev_install_parent and
the parent comparison near the install path checks to canonicalize both the
requested parent and its existing ancestor before comparing them. Reject only
when canonical resolution shows the path escapes or aliases away from the
requested parent, while allowing legitimate symlinked prefixes such as /tmp or
$HOME; preserve the existing safety checks for non-directory components.
In `@scripts/validate-channel-isolation.mjs`:
- Around line 176-178: Update the harness setup around binDir and runShell so
harness.binDir is prepended to the PATH passed to every shell script, while
preserving the existing environment and avoiding reliance on the parent process
PATH. Ensure the resulting PATH uses the harness-specific bin directory before
system entries so all command stubs resolve first.
---
Nitpick comments:
In `@build.sh`:
- Around line 118-133: Update sign_dev_app so neither signing action passes
--deep to codesign. Sign nested code items explicitly before signing the outer
app bundle with --identifier "$DEV_BUNDLE_IDENTIFIER", while preserving
verify_dev_app_signature for deep verification.
In `@run.sh`:
- Around line 240-255: Rename the unused loop counter variable attempt to
_attempt (or use _) in all four wait loops, including the loops around
running_dev_app_reachable, so ShellCheck no longer reports SC2034 while
preserving the existing retry behavior.
In `@scripts/validate-launch-runners.mjs`:
- Around line 21-24: Update the execFileSync invocation in
validate-launch-runners.mjs to use stdio: "inherit" so
validate-channel-isolation.mjs output, including assertion details, is visible
directly when the child harness fails.
- Around line 122-134: The Finder-runner assertions and assertion helpers are
duplicated across both harnesses. In scripts/validate-launch-runners.mjs lines
122-134, extract the shared Finder-runner assertion block and
assertIncludes/assertNotIncludes/assertOrdered helpers into a common module such
as scripts/lib/assert.mjs, then import and reuse them; in
scripts/validate-finder-quick-action.mjs lines 41-55, remove the duplicate
definitions and import the shared module while preserving only its unique setup.
- Around line 152-162: Strengthen assertNoProductionMutation so it detects
production-app mutations by target rather than exact command strings. Replace
the brittle forbidden list with checks covering write or destructive commands
targeting /Applications/ide.app, including quoting or variable variations, and
checks for quitting or killing the non-development com.gordonbeeming.ide
application while preserving allowed development-app operations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 51e6259d-5d62-475a-a4ff-1ce90c753c78
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
build.shdev-install.shdocs/development.mdrun.shscripts/install-cli-command.shscripts/install-macos-finder-quick-action.shscripts/validate-channel-isolation.mjsscripts/validate-finder-quick-action.mjsscripts/validate-launch-runners.mjsscripts/validate-tauri-config.mjssrc-tauri/src/http_server.rssrc-tauri/src/lib.rssrc-tauri/tauri.dev.conf.jsonsrc/tauri.test.tssrc/tauri.ts
Address reviewer comments: - Allow safe development install paths through symlinked prefixes - Run channel-isolation scripts with harness command stubs first
|
@coderabbitai review |
✅ Action performedReview finished.
|
Address reviewer comments: - Guard macOS development packaging paths - Protect canonical Applications aliases
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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: 0b8ca80709
ℹ️ 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".
| ensure_dev_port_available | ||
|
|
||
| exec npm run tauri:dev | ||
| exec npm run tauri:dev -- --config src-tauri/tauri.dev.conf.json |
There was a problem hiding this comment.
When a developer runs the still-documented npm run tauri:dev, package.json expands it to plain tauri dev, so neither this --config argument nor IDE_LOOPBACK_PORT=17878 is applied. That launch therefore uses the base production identifier and port 17877, putting it back in the production single-instance and app-data channel and defeating the isolation this change promises. Make the npm entry point itself apply the development overlay, rather than applying it only in run.sh.
Useful? React with 👍 / 👎.
| const result = runShell(path.join(checkout, "build.sh"), [], harness, "build-success", buildEnv); | ||
| assertStatus(result, 0, "stubbed development build/install"); |
There was a problem hiding this comment.
On Linux or Windows, this unconditionally runs the copied build.sh, whose new uname -s != Darwin guard exits before any of the harness stubs execute; consequently npm run launch:check always fails on those supported development platforms. The harness should either skip this macOS-only install case off Darwin or stub the platform and macOS-only copy operation.
Useful? React with 👍 / 👎.
Why
I want development builds from this checkout to leave the Homebrew-installed production app alone. Running or packaging
ide-devshould not stop production, replace/Applications/ide.app, reuse production state, or bind production's loopback port.What changed
ide-devits own bundle identifier, app-data directory, installed app bundle, and fixed loopback API port.idelauncher and Finder Quick Action pinned to the production app whileide-devcontinues to run this checkout.Testing
./run-tests.shSummary by CodeRabbit