Conversation
📝 WalkthroughWalkthroughAdds a helper that deduplicates manifest ChangesCopy-file Deduplication Fix
Estimated code review effort: 2 (Simple) | ~10 minutes Related issues: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/lib/install-executor.test.js`:
- Around line 431-495: This test is tied to the live repo layout because it
reads REPO_ROOT and real .opencode/commands and commands files, so its
assertions can drift as files change. Refactor the OpenCode regression case in
install-executor.test.js to use a fixed test fixture or explicit mocked file set
for createManifestInstallPlan, and keep the duplicate-destination and
override-wins checks against that controlled data. Use the existing
test('dedupes copy-file operations that share a destination for OpenCode
installs') block and the plan.operations assertions to anchor the fixture-based
rewrite.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 90a193ad-c09e-4c85-adb8-db8301f4f337
📒 Files selected for processing (2)
scripts/lib/install-executor.jstests/lib/install-executor.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: Greptile Review
- GitHub Check: Test (windows-latest, Node 22.x, pnpm)
- GitHub Check: Test (macos-latest, Node 18.x, bun)
- GitHub Check: Test (windows-latest, Node 20.x, pnpm)
- GitHub Check: Test (windows-latest, Node 18.x, pnpm)
🧰 Additional context used
📓 Path-based instructions (19)
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}
📄 CodeRabbit inference engine (.cursor/rules/common-coding-style.md)
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}: Always create new objects, never mutate existing ones. Use immutable patterns to prevent hidden side effects and enable safe concurrency
Organize code into many small files (200-400 lines typical, 800 lines max) organized by feature/domain rather than by type
Always handle errors explicitly at every level and never silently swallow errors
Always validate all user input before processing at system boundaries
Use schema-based validation where available
Fail fast with clear error messages when validation fails
Never trust external data (API responses, user input, file content)
Ensure code is readable and well-named
Keep functions small (less than 50 lines)
Keep files focused (less than 800 lines)
Avoid deep nesting (more than 4 levels)
Do not use hardcoded values; use constants or configuration instead
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
No hardcoded secrets (API keys, passwords, tokens) - validate before any commit
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}: All user inputs must be validated
Enable CSRF protection on all state-changing endpoints
Verify authentication and authorization for all protected endpoints
Implement rate limiting on all endpoints to prevent abuse
Ensure error messages do not leak sensitive data in responses
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,sql}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Use parameterized queries to prevent SQL injection
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
**/*.{js,ts,jsx,tsx,html,php,java,cs,rb,go}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Implement XSS prevention by sanitizing HTML output
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp,properties,yml,yaml,json,env,config}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
NEVER hardcode secrets in source code - ALWAYS use environment variables or a secret manager
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript-coding-style.md)
**/*.{ts,tsx,js,jsx}: Use spread operator for immutable updates in TypeScript/JavaScript instead of direct mutation
Use async/await with try-catch for error handling in TypeScript/JavaScript
Use Zod for schema-based input validation in TypeScript/JavaScript
No console.log statements in production code; use proper logging libraries instead
**/*.{ts,tsx,js,jsx}: Auto-format JavaScript/TypeScript files using Prettier after edit
Warn aboutconsole.logstatements in edited files
Check all modified files forconsole.logstatements before session ends
**/*.{ts,tsx,js,jsx}: Use the ApiResponse interface pattern with generic type parameter:interface ApiResponse<T> { success: boolean; data?: T; error?: string; meta?: { total: number; page: number; limit: number; } }
Implement custom React hooks following the pattern: export a named function with use prefix, generic type parameters, and proper useEffect cleanup for side effects
**/*.{ts,tsx,js,jsx}: Never hardcode secrets; always use environment variables for sensitive credentials like API keys
Throw an error when required environment variables are not configured to fail fast and ensure security prerequisites are metUse Playwright as the E2E testing framework for critical user flows in TypeScript/JavaScript
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: Write tests before implementation (test-driven development); target 80%+ coverage
Achieve minimum 80% test coverage across all three layers: Unit, Integration, and E2E
Use AAA structure (Arrange / Act / Assert) in tests with descriptive test names that explain behavior under test
Files:
tests/lib/install-executor.test.js
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts,jsx,tsx}: Always create new objects and never mutate in place; return new copies instead
Keep files between 200–400 lines typical, with a maximum of 800 lines
Extract helpers when a file exceeds 200 lines
Handle errors explicitly at every level; never swallow errors silently
Validate all user input before processing; use schema-based validation where available
Never trust external data (API responses, file content, query params); always validate
All user inputs must be validated and sanitized
Error messages must be scrubbed of sensitive internals
Use readable, well-named identifiers in all code
Keep functions under 50 lines
Keep files under 800 lines
Avoid nesting deeper than 4 levels
Implement comprehensive error handling in all code
Do not hardcode values; use constants or environment configuration instead
Do not use in-place mutation; always return new objects or state
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
**/*.{js,ts,jsx,tsx,json,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not hardcode secrets, API keys, passwords, or tokens
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts}: Use parameterized queries for all database writes (no string interpolation)
Auth/authz must be checked server-side for every sensitive path
Rate limiting must be applied to all public endpoints
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
HTML output must be sanitized where applicable
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
**/*.{js,ts,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Required environment variables must be validated at startup
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
**/*.{js,ts,jsx,tsx,py,java,go,rs,kt,cpp,c,fs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx,py,java,go,rs,kt,cpp,c,fs}: Write tests before implementation using TDD workflow: write failing test (RED), implement minimal code (GREEN), then refactor (IMPROVE)
Keep functions small (<50 lines) and files focused (<800 lines, typical 200-400 lines)
Avoid deep nesting (>4 levels)
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
**/*.{js,ts,jsx,tsx,py,java,go,rs,kt}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx,py,java,go,rs,kt}: Never mutate existing objects; always create new objects with changes applied (Immutability requirement)
Handle errors at every level; provide user-friendly messages in UI code and detailed context in server-side logs
Ensure error messages don't leak sensitive data
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
**/*.{jsx,tsx,html,js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Sanitize HTML output to prevent XSS vulnerabilities
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
{package.json,*.config.js,scripts/**/*.js}
📄 CodeRabbit inference engine (CLAUDE.md)
Package manager detection should support npm, pnpm, yarn, and bun, with configuration via CLAUDE_PACKAGE_MANAGER environment variable or project config.
Files:
scripts/lib/install-executor.js
scripts/**/*.js
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure cross-platform support for Windows, macOS, and Linux via Node.js scripts in the scripts/ directory.
Files:
scripts/lib/install-executor.js
{scripts,bin}/**
⚙️ CodeRabbit configuration file
{scripts,bin}/**: Focus on command injection, unsafe subprocess usage, path traversal, SSRF, secret exposure, and missing tests for new CLI behavior.
Files:
scripts/lib/install-executor.js
🔇 Additional comments (2)
scripts/lib/install-executor.js (2)
691-716: Dedupe logic correct — LGTM.Two-pass last-index Map + filter cleanly keeps the last
copy-fileperdestinationPath, leavesmerge-jsonand paths withoutdestinationPathuntouched, and preserves relative ordering. No mutation of the input array — consistent with the immutability guideline for this path.
745-747: LGTM!
| if (test('dedupes copy-file operations that share a destination for OpenCode installs (issue #2414)', () => { | ||
| const homeDir = createTempDir('install-executor-home-'); | ||
| try { | ||
| const plan = createManifestInstallPlan({ | ||
| sourceRoot: REPO_ROOT, | ||
| homeDir, | ||
| target: 'opencode', | ||
| profileId: 'core', | ||
| }); | ||
|
|
||
| // Core fix: OpenCode ships override command files under .opencode/commands/ | ||
| // that shadow the generic commands/ sources. Before the fix both writes | ||
| // were recorded against the same destination, so `doctor` saw perpetual | ||
| // drift and `repair` clobbered the override. Every copy-file destination | ||
| // must now be unique. | ||
| const copyDestinations = plan.operations | ||
| .filter(operation => operation.kind === 'copy-file') | ||
| .map(operation => operation.destinationPath); | ||
| const seenDestinations = new Set(); | ||
| const duplicateDestinations = new Set(); | ||
| for (const destinationPath of copyDestinations) { | ||
| if (seenDestinations.has(destinationPath)) { | ||
| duplicateDestinations.add(destinationPath); | ||
| } | ||
| seenDestinations.add(destinationPath); | ||
| } | ||
| assert.deepStrictEqual( | ||
| [...duplicateDestinations], | ||
| [], | ||
| `copy-file operations must have unique destinations; duplicates: ${[...duplicateDestinations].join(', ')}` | ||
| ); | ||
|
|
||
| // The surviving op must be the last writer: wherever an .opencode/commands | ||
| // override exists it has to win over the generic commands/ source. Checked | ||
| // over every override destination, not a hardcoded index. | ||
| const opencodeCommandOps = plan.operations.filter(operation => ( | ||
| operation.kind === 'copy-file' | ||
| && operation.destinationPath.split(path.sep).join('/').includes('/.opencode/commands/') | ||
| )); | ||
| assert.ok( | ||
| opencodeCommandOps.length > 0, | ||
| 'expected OpenCode command operations to be present' | ||
| ); | ||
| let assertedOverrides = 0; | ||
| for (const operation of opencodeCommandOps) { | ||
| const overrideRelativePath = path.join('.opencode', 'commands', path.basename(operation.destinationPath)); | ||
| if (!fs.existsSync(path.join(REPO_ROOT, overrideRelativePath))) { | ||
| continue; | ||
| } | ||
| assertedOverrides += 1; | ||
| assert.strictEqual( | ||
| operation.sourceRelativePath.split(path.sep).join('/'), | ||
| overrideRelativePath.split(path.sep).join('/'), | ||
| `override must win for ${operation.destinationPath}` | ||
| ); | ||
| } | ||
| assert.ok( | ||
| assertedOverrides > 0, | ||
| 'expected at least one .opencode/commands override to be asserted' | ||
| ); | ||
| } finally { | ||
| cleanup(homeDir); | ||
| } | ||
| })) passed++; else failed++; | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Solid regression test, but depends on live repo file layout.
The test uses REPO_ROOT and real on-disk .opencode/commands/* and commands/* files instead of a fixture, so its assertions are conditional on which override files currently exist (assertedOverrides > 0 guards this). This is more of an integration test than a unit test — acceptable for validating the exact reported bug scenario, but it will silently exercise different code paths as repo content changes over time, and won't fail loudly if all current overrides are removed (only the assertedOverrides > 0 guard would catch full removal).
🤖 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 `@tests/lib/install-executor.test.js` around lines 431 - 495, This test is tied
to the live repo layout because it reads REPO_ROOT and real .opencode/commands
and commands files, so its assertions can drift as files change. Refactor the
OpenCode regression case in install-executor.test.js to use a fixed test fixture
or explicit mocked file set for createManifestInstallPlan, and keep the
duplicate-destination and override-wins checks against that controlled data. Use
the existing test('dedupes copy-file operations that share a destination for
OpenCode installs') block and the plan.operations assertions to anchor the
fixture-based rewrite.
There was a problem hiding this comment.
Addressed in 76b151b. The OpenCode case is now a hermetic unit test of an extracted dedupeCopyFileOperations helper over fixed operation arrays (no REPO_ROOT / live-layout dependency): it asserts last-writer-wins (the .opencode/commands override beats the generic commands/ source) and that merge-json accumulation plus operation order are preserved. This also resolves a CI failure the earlier version hit — targeting opencode in-plan required the git-ignored .opencode/dist payload, which is absent on a fresh CI checkout.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
git log --oneline -5 76b151b8 2>/dev/null
git show 76b151b8 --stat 2>/dev/null | head -50Length of output: 1878
Confirmed via commit 76b151b: the test file's OpenCode case now feeds fixed operation arrays directly into dedupeCopyFileOperations, dropping the REPO_ROOT/live-layout dependency and the associated CI failure on .opencode/dist. This resolves the concern raised.
@gaurav0107 Confirmed — commit 76b151b replaces the REPO_ROOT-dependent OpenCode test with hermetic unit tests exercising dedupeCopyFileOperations against fixed operation arrays, covering last-writer-wins and merge-json/order preservation. This resolves the live-layout fragility flagged and avoids the CI dependency on the git-ignored .opencode/dist payload. Thanks for the fix.
٩(◕‿◕)۶
|
| Filename | Overview |
|---|---|
| scripts/lib/install-executor.js | Adds destination-based de-duplication for manifest copy-file operations while preserving the last writer. |
| tests/lib/install-executor.test.js | Adds tests for duplicate copy destinations and preserving non-copy operations. |
Reviews (2): Last reviewed commit: "fix(install): dedupe copy-file operation..." | Re-trigger Greptile
OpenCode ships override command files under .opencode/commands/ that shadow the generic commands/*.md sources. The manifest install plan recorded both writes against the same destination, so `ecc doctor` reported perpetual drift for the 29 shadowed command files and `ecc repair` "fixed" that phantom drift by copying the generic source over the correct override, corrupting the installed command while never clearing the warning. Dedupe copy-file operations by destination in createManifestInstallPlan, keeping the last writer to match the sequential apply order. install, repair, and doctor all consume this one builder, so a fresh install is clean and a single repair rewrites drifted state green. Fixes affaan-m#2414
ef1d152 to
76b151b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/lib/install-executor.js (1)
691-798: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffFile exceeds the 800-line guideline.
This file is now at 807+ lines, past the stated max. Not solely caused by this PR, but the new helper adds to an already-oversized file. Consider extracting install-plan materialization/dedup helpers into a separate module during a future pass.
As per coding guidelines, "Organize code into many small files (200-400 lines typical, 800 lines max) organized by feature/domain rather than by type."
🤖 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/lib/install-executor.js` around lines 691 - 798, The file is now over the 800-line guideline because the new install-plan helpers were added to an already oversized module. Move the new helper logic, especially dedupeCopyFileOperations and the related createManifestInstallPlan materialization flow, into a separate feature-focused module and keep this file as a thin orchestrator. Preserve the existing behavior and references to resolveInstallPlan, materializeScaffoldOperation, and createStatePreview while reducing the size of install-executor.js.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/lib/install-executor.test.js`:
- Line 17: Add an end-to-end test that exercises createManifestInstallPlan with
a manifest fixture containing overlapping copy destinations (for example generic
and .opencode override sources) and assert that the returned operations and
statePreview reflect the deduped result. Reuse the existing
dedupeCopyFileOperations coverage in tests/lib/install-executor.test.js, but
wire it through the full createManifestInstallPlan path so the regression from
issue `#2414` is verified at the manifest-plan level.
---
Outside diff comments:
In `@scripts/lib/install-executor.js`:
- Around line 691-798: The file is now over the 800-line guideline because the
new install-plan helpers were added to an already oversized module. Move the new
helper logic, especially dedupeCopyFileOperations and the related
createManifestInstallPlan materialization flow, into a separate feature-focused
module and keep this file as a thin orchestrator. Preserve the existing behavior
and references to resolveInstallPlan, materializeScaffoldOperation, and
createStatePreview while reducing the size of install-executor.js.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b82dea05-42a4-49f8-8de5-de2fe20e6262
📒 Files selected for processing (2)
scripts/lib/install-executor.jstests/lib/install-executor.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (20)
- GitHub Check: Test (windows-latest, Node 22.x, pnpm)
- GitHub Check: Test (windows-latest, Node 20.x, pnpm)
- GitHub Check: Test (ubuntu-latest, Node 22.x, bun)
- GitHub Check: Test (windows-latest, Node 18.x, yarn)
- GitHub Check: Test (windows-latest, Node 20.x, npm)
- GitHub Check: Test (ubuntu-latest, Node 18.x, bun)
- GitHub Check: Test (macos-latest, Node 22.x, npm)
- GitHub Check: Test (windows-latest, Node 22.x, npm)
- GitHub Check: Test (macos-latest, Node 20.x, pnpm)
- GitHub Check: Test (ubuntu-latest, Node 18.x, pnpm)
- GitHub Check: Test (windows-latest, Node 18.x, pnpm)
- GitHub Check: Test (ubuntu-latest, Node 22.x, pnpm)
- GitHub Check: Test (macos-latest, Node 20.x, yarn)
- GitHub Check: Test (windows-latest, Node 22.x, yarn)
- GitHub Check: Test (macos-latest, Node 18.x, yarn)
- GitHub Check: Test (windows-latest, Node 20.x, yarn)
- GitHub Check: Test (ubuntu-latest, Node 20.x, bun)
- GitHub Check: Test (windows-latest, Node 18.x, npm)
- GitHub Check: Test (ubuntu-latest, Node 20.x, pnpm)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (19)
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}
📄 CodeRabbit inference engine (.cursor/rules/common-coding-style.md)
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}: Always create new objects, never mutate existing ones. Use immutable patterns to prevent hidden side effects and enable safe concurrency
Organize code into many small files (200-400 lines typical, 800 lines max) organized by feature/domain rather than by type
Always handle errors explicitly at every level and never silently swallow errors
Always validate all user input before processing at system boundaries
Use schema-based validation where available
Fail fast with clear error messages when validation fails
Never trust external data (API responses, user input, file content)
Ensure code is readable and well-named
Keep functions small (less than 50 lines)
Keep files focused (less than 800 lines)
Avoid deep nesting (more than 4 levels)
Do not use hardcoded values; use constants or configuration instead
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
No hardcoded secrets (API keys, passwords, tokens) - validate before any commit
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}: All user inputs must be validated
Enable CSRF protection on all state-changing endpoints
Verify authentication and authorization for all protected endpoints
Implement rate limiting on all endpoints to prevent abuse
Ensure error messages do not leak sensitive data in responses
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,sql}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Use parameterized queries to prevent SQL injection
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
**/*.{js,ts,jsx,tsx,html,php,java,cs,rb,go}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Implement XSS prevention by sanitizing HTML output
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp,properties,yml,yaml,json,env,config}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
NEVER hardcode secrets in source code - ALWAYS use environment variables or a secret manager
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript-coding-style.md)
**/*.{ts,tsx,js,jsx}: Use spread operator for immutable updates in TypeScript/JavaScript instead of direct mutation
Use async/await with try-catch for error handling in TypeScript/JavaScript
Use Zod for schema-based input validation in TypeScript/JavaScript
No console.log statements in production code; use proper logging libraries instead
**/*.{ts,tsx,js,jsx}: Auto-format JavaScript/TypeScript files using Prettier after edit
Warn aboutconsole.logstatements in edited files
Check all modified files forconsole.logstatements before session ends
**/*.{ts,tsx,js,jsx}: Use the ApiResponse interface pattern with generic type parameter:interface ApiResponse<T> { success: boolean; data?: T; error?: string; meta?: { total: number; page: number; limit: number; } }
Implement custom React hooks following the pattern: export a named function with use prefix, generic type parameters, and proper useEffect cleanup for side effects
**/*.{ts,tsx,js,jsx}: Never hardcode secrets; always use environment variables for sensitive credentials like API keys
Throw an error when required environment variables are not configured to fail fast and ensure security prerequisites are metUse Playwright as the E2E testing framework for critical user flows in TypeScript/JavaScript
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: Write tests before implementation (test-driven development); target 80%+ coverage
Achieve minimum 80% test coverage across all three layers: Unit, Integration, and E2E
Use AAA structure (Arrange / Act / Assert) in tests with descriptive test names that explain behavior under test
Files:
tests/lib/install-executor.test.js
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts,jsx,tsx}: Always create new objects and never mutate in place; return new copies instead
Keep files between 200–400 lines typical, with a maximum of 800 lines
Extract helpers when a file exceeds 200 lines
Handle errors explicitly at every level; never swallow errors silently
Validate all user input before processing; use schema-based validation where available
Never trust external data (API responses, file content, query params); always validate
All user inputs must be validated and sanitized
Error messages must be scrubbed of sensitive internals
Use readable, well-named identifiers in all code
Keep functions under 50 lines
Keep files under 800 lines
Avoid nesting deeper than 4 levels
Implement comprehensive error handling in all code
Do not hardcode values; use constants or environment configuration instead
Do not use in-place mutation; always return new objects or state
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
**/*.{js,ts,jsx,tsx,json,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not hardcode secrets, API keys, passwords, or tokens
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts}: Use parameterized queries for all database writes (no string interpolation)
Auth/authz must be checked server-side for every sensitive path
Rate limiting must be applied to all public endpoints
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
HTML output must be sanitized where applicable
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
**/*.{js,ts,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Required environment variables must be validated at startup
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
**/*.{js,ts,jsx,tsx,py,java,go,rs,kt,cpp,c,fs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx,py,java,go,rs,kt,cpp,c,fs}: Write tests before implementation using TDD workflow: write failing test (RED), implement minimal code (GREEN), then refactor (IMPROVE)
Keep functions small (<50 lines) and files focused (<800 lines, typical 200-400 lines)
Avoid deep nesting (>4 levels)
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
**/*.{js,ts,jsx,tsx,py,java,go,rs,kt}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx,py,java,go,rs,kt}: Never mutate existing objects; always create new objects with changes applied (Immutability requirement)
Handle errors at every level; provide user-friendly messages in UI code and detailed context in server-side logs
Ensure error messages don't leak sensitive data
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
**/*.{jsx,tsx,html,js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Sanitize HTML output to prevent XSS vulnerabilities
Files:
tests/lib/install-executor.test.jsscripts/lib/install-executor.js
{package.json,*.config.js,scripts/**/*.js}
📄 CodeRabbit inference engine (CLAUDE.md)
Package manager detection should support npm, pnpm, yarn, and bun, with configuration via CLAUDE_PACKAGE_MANAGER environment variable or project config.
Files:
scripts/lib/install-executor.js
scripts/**/*.js
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure cross-platform support for Windows, macOS, and Linux via Node.js scripts in the scripts/ directory.
Files:
scripts/lib/install-executor.js
{scripts,bin}/**
⚙️ CodeRabbit configuration file
{scripts,bin}/**: Focus on command injection, unsafe subprocess usage, path traversal, SSRF, secret exposure, and missing tests for new CLI behavior.
Files:
scripts/lib/install-executor.js
🔇 Additional comments (5)
scripts/lib/install-executor.js (3)
691-716: Dedup logic is correct.Two-pass last-index-wins approach correctly preserves non-
copy-fileoperations and relative order, and doesn't mutate the input array. Matches the sequential-apply semantics described in the comment.
745-747: LGTM!
807-807: LGTM!tests/lib/install-executor.test.js (2)
432-461: LGTM! Synthetic fixture properly covers the override-wins scenario without relying on live repo layout.
463-483: Good coverage for non-copy-file preservation and ordering.Confirms
merge-jsonwrites to the same destination survive while shadowedcopy-fileentries are dropped, with order intact.
daltino
left a comment
daltino
left a comment
There was a problem hiding this comment.
This PR addresses a clear pain point by introducing a dedupeCopyFileOperations function to eliminate redundant copy-file operations targeting the same destination, ensuring only the last write is preserved. The implementation is cleanly added to the install logic, and testing covers the intended scenario effectively, demonstrating thoughtful improvement without impacting unrelated functionality. Looks good to merge!
|
Quick note: these CI reds aren't from this PR. The leftover Windows and Python Tests reds are separate, already-existing issues (a bash-detection test and a missing pyyaml), not related to this change. |
3 participants
What Changed
Extracted a small pure helper,
dedupeCopyFileOperations, and applied it increateManifestInstallPlanso a plan carries at most onecopy-fileoperationper destination — keeping the last writer, which matches the sequential
apply order that already determines what lands on disk. Every other operation
kind (e.g.
merge-jsonwrites that accumulate into a shared config) is leftuntouched and in order.
Why This Change
For OpenCode installs, 29 command files ship a harness-specific override under
.opencode/commands/<name>.mdthat shadows the genericcommands/<name>.md.The plan recorded both managed
copy-fileoperations against the samedestination
~/.opencode/commands/<name>.md. The override is applied last sothe install is correct on disk, but the stale generic operation leaked into
install-state, which broke lifecycle:
node scripts/ecc.js doctorcompared the generic source against theinstalled override content and reported
drifted-managed-filesfor all 29files — forever.
node scripts/ecc.js repair"repaired" that phantom drift by copying thegeneric
commands/<name>.mdover the correct override, corrupting theinstalled command, and
doctorstill showed the same warnings on the nextrun.
With one operation per destination, a fresh install is clean and
repairrestores the correct override instead of clobbering it.
Testing Done
Reproduced end-to-end against a sandbox
HOME(ecc install --target opencode --profile core):doctor→ 27drifted-managed-files;repair→ "Repaired paths:27" (exit 0) but
doctor→ the identical 27 warnings, and the command fileswere overwritten with the generic variant.
doctor→warnings=0 errors=0 status=ok;repair→ 0 repairs(nothing drifted);
doctor→ stillok.Automated (in
tests/lib/install-executor.test.js):dedupeCopyFileOperationsover fixed operationarrays (no live-repo layout coupling): last writer wins (the
.opencode/commandsoverride beats the generic
commands/source), andmerge-jsonaccumulationplus operation order are preserved. Both fail on
main(helper absent) andpass with this change.
node tests/lib/install-executor.test.js12/12,install-lifecycle30/30,install-targets41/41,selective-install46/46, and the opencode /plugin-manifest suites all green.
npx eslint scripts/lib/install-executor.js tests/lib/install-executor.test.jsclean.
Note: the regression is tested against the pure plan-shaping helper rather than
a
target: opencodeplan build, because the OpenCode adapter requires thegit-ignored compiled
.opencode/distpayload, which is unbuilt on a fresh CIcheckout.
Type of Change
Security & Quality Checklist
lockfile, no generated
.opencode/dist, no unrelated refactors)shadowed write is dropped from install-state
merge-jsonaccumulation into shared config files is unaffected (onlycopy-fileops are collapsed)Documentation
No user-facing docs change required; behaviour now matches the documented
doctor/repaircontract.Fixes #2414