fix(cli): keep pre-commit hooks on external installations - #789
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 570b1daafc
ℹ️ 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".
| [runtime, cli, installation].some( | ||
| (path) => !isOutsidePath(relative(worktree, path)), |
There was a problem hiding this comment.
When the target is a linked Git worktree, this checks only that worktree’s top-level path. An installation in another linked worktree therefore passes, while git rev-parse --git-path hooks/pre-commit resolves to the shared hook directory in the main worktree. The resulting shared hook is pinned to repository-controlled code that a checkout or deletion of the sibling worktree can replace or break, affecting commits in every linked worktree and recreating the vulnerability this change is intended to prevent. Check the installation and runtime against every path reported by git worktree list --porcelain, not only the target worktree.
AGENTS.md reference: sdk/typescript/AGENTS.md:L22-L23
Useful? React with 👍 / 👎.
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
The marker-less migration check is broad enough to overwrite a custom hook. Since the managed comment is optional and both captured paths only need to be absolute, a hook like exec '/abs/node' '/abs/custom-script' scan . --working-tree --fail-on-severity high is treated as generated and replaced. That changes the previous “leave custom hooks alone” behavior. Can we require stronger provenance for the marker-less case, e.g. identify the old Codex Security CLI/package path before overwriting?
2 participants
Summary
Generated pre-commit hooks need an installation that stays stable as the repository changes. Require the CLI installation and runtime to live outside the target worktree, and let users refresh previously generated hooks from that installation.
Changes
Testing
bun test --timeout 30000 tests-ts/cli.test.ts tests-ts/cli-launcher.test.ts: 159 passed.pnpm run types: passed.pnpm run format: passed.pnpm run test --seed 12345andpnpm run test: both passed on Linux (2,243 passed, 41 skipped in each run).Risk and rollout
install-hooknow returns exit code 2 for repository-local installations. Install the CLI and runtime outside the repository, then reruninstall-hookwith the existing severity threshold to refresh an old generated hook. Package updates alone do not rewrite hooks. No commands, flags, or defaults are added.Public disclosure review