fix(ci+installers): harden permission checks and guard completion/profile writes#1247
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds writability checks in filesystem helpers, completion installers, and file locking, updates related tests for permission-restricted paths, and pins GitHub Actions Node.js to ChangesFilesystem Writability Checks
CI Node.js Version Pin
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/completions/installers/fish-installer.ts`:
- Around line 143-145: The uninstall check in fish-installer’s target removal
path is too strict because it requires the completion file itself to be writable
before unlinking; adjust the precondition in the targetPath/targetDir check so
only the parent directory’s writability is enforced, and keep the unlink path in
FishInstaller aligned with POSIX delete semantics by relying on targetDir rather
than FileSystemUtils.canWriteFile(targetPath).
In `@src/core/completions/installers/powershell-installer.ts`:
- Around line 415-417: The uninstall path in powershell-installer.ts is checking
both the target file and its parent directory for writability, but `uninstall()`
should only gate on the containing directory because `fs.unlink()` depends on
directory permissions, not the file mode. Update the `uninstall()` logic around
`FileSystemUtils.canWriteFile` to remove the target file check and rely on the
directory-derived path (`path.dirname(targetPath)`) while keeping the existing
error handling for unwritable locations.
In `@src/utils/file-system.ts`:
- Around line 20-25: The permission check in canWriteFile is too weak for
directories because W_OK alone can pass on a write-only directory even though
POSIX operations still fail without execute/search access. Update canWriteFile
to treat directories differently by also requiring execute/search permission
before returning true, while keeping the existing behavior for regular files,
and use the same helper path around fs.access and hasOwnerGroupOrOtherWriteBit
to keep the preflight guard accurate.
🪄 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: CHILL
Plan: Pro
Run ID: a2757577-54fc-4603-8325-dff8e67e5e19
📒 Files selected for processing (7)
.github/workflows/ci.ymlsrc/core/completions/installers/fish-installer.tssrc/core/completions/installers/powershell-installer.tssrc/core/completions/installers/zsh-installer.tssrc/core/file-state.tssrc/utils/file-system.tstest/core/completions/installers/zsh-installer.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/core/completions/installers/bash-installer.test.ts`:
- Around line 167-168: The current assertion only checks for a generic install
failure message, so update this test to verify the permission-specific
diagnostic emitted by the new preflight path instead of just any “Failed to
install” text. Use the existing result?.message check in the bash installer test
to assert the exact permission-related wording returned by the installer, so
regressions to a generic write failure will be caught.
In `@test/core/completions/installers/powershell-installer.test.ts`:
- Line 262: The tests in powershell-installer.test.ts are mutating
process.env.OPENSPEC_NO_AUTO_CONFIG and only deleting it, which leaves global
state changed for later cases. Update the affected test(s) by saving the
original OPENSPEC_NO_AUTO_CONFIG value before mutation and restoring it in
cleanup, or by using a shared afterEach in the test suite so each case returns
the environment to its prior state. Use the test blocks that currently touch
process.env.OPENSPEC_NO_AUTO_CONFIG as the fix points.
🪄 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: CHILL
Plan: Pro
Run ID: 7f0cd3ea-709d-45ef-add5-8213a2cd82f9
📒 Files selected for processing (8)
src/core/completions/installers/bash-installer.tssrc/core/completions/installers/fish-installer.tssrc/core/completions/installers/powershell-installer.tssrc/utils/file-system.tstest/core/completions/installers/bash-installer.test.tstest/core/completions/installers/fish-installer.test.tstest/core/completions/installers/powershell-installer.test.tstest/utils/file-system.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/completions/installers/fish-installer.ts
- src/utils/file-system.ts
alfred-openspec
approved these changes
Jun 24, 2026
alfred-openspec
left a comment
alfred-openspec
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the permission-guard changes and follow-up fixes. The implementation now checks parent-dir writability for uninstall paths, preserves PowerShell env state in tests, and covers the POSIX mode-bit/root-CI edge cases.\n\nVerified locally:\n- corepack pnpm@9 exec vitest run test/core/completions/installers/bash-installer.test.ts test/core/completions/installers/zsh-installer.test.ts test/core/completions/installers/fish-installer.test.ts test/core/completions/installers/powershell-installer.test.ts test/core/file-state.test.ts test/utils/file-system.test.ts → 198 passed\n- corepack pnpm@9 exec tsc --noEmit && corepack pnpm@9 lint → passed, with only the preexisting src/core/references.ts unused eslint-disable warning\n\nShip it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
fs.access(..., W_OK)can report writable for root, causing permission-sensitive tests and lock-create logic to pass unexpectedly..zshrc, PowerShell profile, completion scripts, lock files) without pre-validating writability, which can hide errors or produce confusing diagnostics in CI.20is ambiguous; pin to the package-engine floor20.19.0to avoid platform/engine differences when reproducing failures.Description
fs.accessviahasWritableModeAndAccess()and updateFileSystemUtils.canWriteFile()to prefer mode-bit checks on POSIX, preserving Windows behavior (file:src/utils/file-system.ts).acquireFileLock()so permission errors produce the correctcreate-faileddiagnostic (file:src/core/file-state.ts).FileSystemUtils.canWriteFile()before mutating files or unlinking, for Fish, PowerShell and Zsh installers (files:src/core/completions/installers/fish-installer.ts,src/core/completions/installers/powershell-installer.ts,src/core/completions/installers/zsh-installer.ts)./rootpaths (file:test/core/completions/installers/zsh-installer.test.ts).20.19.0for all Node setup steps in.github/workflows/ci.ymlto reduce CI variability.Testing
corepack pnpm@9 exec vitest run test/core/completions/installers/bash-installer.test.ts test/core/completions/installers/zsh-installer.test.ts test/core/completions/installers/fish-installer.test.ts test/core/completions/installers/powershell-installer.test.ts test/core/file-state.test.tsand observed all focused tests pass (permission-related failures are fixed).corepack pnpm@9 run build && corepack pnpm@9 exec tsc --noEmit && corepack pnpm@9 lintsucceeded; lint reports one preexisting warning insrc/core/references.ts.corepack pnpm@9 testwas exercised during investigation; earlier full runs in this constrained environment still exhibited unrelated, long-running e2e/timeouts (environment/order-sensitive) but the root permission-related failures addressed here no longer repro in focused coverage.Codex Task
Summary by CodeRabbit