feat(setup): support custom Git hooks directories - #93
Conversation
Deploying rstack-cli with
|
| Latest commit: |
06a74d3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://09d32a08.rstack-cli.pages.dev |
| Branch Preview URL: | https://chenjiahan-feat-setup-custom.rstack-cli.pages.dev |
📝 WalkthroughWalkthroughThe setup CLI adds a 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rstack/src/setup/install.ts (1)
78-94: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject absolute hooks directories.
At repository root,
--hooks-dir /tmp/hooksmakes Line 80 configure Git with/tmp/hooks/_(an absolute path), while Line 90 writes to<cwd>/tmp/hooks/_. Git therefore does not run the generated hooks. Validate that the normalized directory is relative, including leading/or\, drive-rooted, and UNC paths; add a regression test.Proposed fix
const prefix = removeLineEnding(prefixResult.stdout).replaceAll('\\', '/'); const resolvedHooksDir = hooksDir.replaceAll('\\', '/'); +if (path.posix.isAbsolute(resolvedHooksDir) || path.win32.isAbsolute(hooksDir)) { + return fail('invalid-hooks-dir', 'Hooks directory must be relative to the current directory.'); +} const hooksPath = `${prefix}${resolvedHooksDir}/_`;🤖 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 `@packages/rstack/src/setup/install.ts` around lines 78 - 94, Validate the normalized hooks directory before constructing hooksPath or writing files, rejecting absolute paths including leading slash or backslash, drive-rooted paths, and UNC paths. Apply this validation in the setup flow around resolvedHooksDir and return the existing failure form for invalid input; preserve relative-directory behavior, and add a regression test covering an absolute hooks directory such as /tmp/hooks.
🤖 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.
Outside diff comments:
In `@packages/rstack/src/setup/install.ts`:
- Around line 78-94: Validate the normalized hooks directory before constructing
hooksPath or writing files, rejecting absolute paths including leading slash or
backslash, drive-rooted paths, and UNC paths. Apply this validation in the setup
flow around resolvedHooksDir and return the existing failure form for invalid
input; preserve relative-directory behavior, and add a regression test covering
an absolute hooks directory such as /tmp/hooks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4f447f4a-9a55-48b8-93c8-b7108ef0da4b
📒 Files selected for processing (4)
packages/rstack/src/setup/index.tspackages/rstack/src/setup/install.tspackages/rstack/tests/cli/setup/index.test.tspackages/rstack/tests/setup/install.test.ts
![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: 06a74d3320
ℹ️ 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".
|
|
||
| const prefix = removeLineEnding(prefixResult.stdout).replaceAll('\\', '/'); | ||
| const resolvedHooksDir = hooksDir.replaceAll('\\', '/'); | ||
| const hooksPath = `${prefix}${resolvedHooksDir}/_`; |
There was a problem hiding this comment.
When a user passes an absolute value such as --hooks-dir /tmp/hooks, this constructs an absolute core.hooksPath, but the later path.join(cwd, resolvedHooksDir, '_') writes the generated files beneath the current repository (for example, /repo/tmp/hooks/_). Setup therefore reports success while Git looks in /tmp/hooks/_ and runs no generated hooks. Although the help describes the option as relative, the parser currently accepts this value, so validate and reject absolute paths before installation.
Useful? React with 👍 / 👎.
| ); | ||
| } | ||
|
|
||
| const prefix = removeLineEnding(prefixResult.stdout).replaceAll('\\', '/'); |
There was a problem hiding this comment.
On POSIX, backslash is a valid filename character rather than a path separator. If setup runs from a directory such as a literal front\end, Git returns that exact prefix, but this replacement changes it to front/end; the files are still written beneath the original cwd, while core.hooksPath points to a nonexistent directory, so hooks silently stop running. Only apply separator normalization on platforms where backslash is actually a separator.
Useful? React with 👍 / 👎.
1 participant
Summary
This PR lets
rs setupinstall Git hooks in a custom cwd-relative directory and from nested projects. It adds--hooks-dir, derives the repository-relativecore.hooksPathfrom Git's current prefix, and normalizes Windows path separators. Integration coverage verifies root and nested installation, paths with spaces, and real hook execution.