Fix migrated hook path rewriting#20144
Conversation
|
@codex review |
![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: a2fa2725ff
ℹ️ 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".
alexsong-oai
force-pushed
the
alexs/hooks-migration
branch
from
a2fa272 to
fd2336e
Compare
alexsong-oai
force-pushed
the
alexs/hooks-migration
branch
from
fd2336e to
dd6f8cd
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0491fc67b
ℹ️ 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".
| fn is_shell_path_boundary(ch: char) -> bool { | ||
| ch.is_whitespace() || matches!(ch, ';' | '|' | '&' | '<' | '>' | '(' | ')') | ||
| } |
There was a problem hiding this comment.
replace_unquoted_hook_paths relies on shell_path_start, but is_shell_path_boundary does not treat = as a boundary. For commands like HOOK=${CLAUDE_PROJECT_DIR}/.claude/hooks/check.py python3 "$HOOK", rewriting starts at byte 0 and replaces the whole assignment token with a quoted path, dropping HOOK= and breaking execution.
Useful? React with 👍 / 👎.
| return Ok(effective); | ||
| }; | ||
| let local_settings = settings_dir.join("settings.local.json"); | ||
| let Some(local_settings) = read_external_settings(&local_settings)? else { |
There was a problem hiding this comment.
effective_external_settings now propagates parse errors from settings.local.json. A malformed local file will abort config/MCP/plugin detection and import even when settings.json is valid. This introduces a migration failure mode that did not exist before local-file merging was added.
Useful? React with 👍 / 👎.
| command.contains(&source_hooks_backslash_path) | ||
| || command.contains(&format!("%{project_dir_env_var}%")) | ||
| || command.contains(&format!("$env:{project_dir_env_var}")) |
There was a problem hiding this comment.
looks_like_windows_hook_command returns true if %{PROJECT_DIR}% or $env:{PROJECT_DIR} appears anywhere, and rewrite_hook_command then skips all rewriting. Mixed POSIX commands can keep stale .claude/hooks paths (e.g., python3 /repo/.claude/hooks/check.py && echo %CLAUDE_PROJECT_DIR%) and break after migration.
Useful? React with 👍 / 👎.
| fn is_static_hook_path_suffix(suffix: &str) -> bool { | ||
| !suffix.is_empty() | ||
| && !suffix | ||
| .chars() | ||
| .any(|ch| matches!(ch, '\\' | '$' | '`' | '*' | '?' | '[')) | ||
| } |
There was a problem hiding this comment.
is_static_hook_path_suffix blocks globs/vars but allows { and }. A command like .claude/hooks/{lint,fmt}.sh is treated as static and rewritten to one single-quoted literal path, which disables brace expansion and can make valid hook commands fail after migration.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea252450d4
ℹ️ 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".
| { | ||
| let path_start = shell_path_start(&rewritten, source_hooks_start); | ||
| let path_end = shell_path_end(&rewritten, source_hooks_start + source_hooks_path.len()); | ||
| if is_assignment_value_start(&rewritten, path_start) { |
There was a problem hiding this comment.
Avoid skipping every source path that appears immediately after =. The is_assignment_value_start guard was meant for env assignments, but it also blocks valid rewrites in arguments like --script=${CLAUDE_PROJECT_DIR}/.claude/hooks/check.py. Those commands retain stale .claude/hooks paths after migration and can fail once legacy hooks are removed.
Useful? React with 👍 / 👎.
| !suffix.is_empty() | ||
| && !suffix | ||
| .chars() | ||
| .any(|ch| matches!(ch, '\\' | '$' | '`' | '*' | '?' | '[' | '{' | '}')) |
There was a problem hiding this comment.
is_static_hook_path_suffix rejects any \\ in the suffix, so commands like python3 ${CLAUDE_PROJECT_DIR}/.claude/hooks/my\ script.py are never rewritten. The migrated command keeps the old .claude/hooks path and breaks once legacy hooks are removed. This is a regression for valid shell-escaped filenames with spaces.
Useful? React with 👍 / 👎.
2 participants
Summary
.claude/hooks/segment..claude/settings.local.jsonover.claude/settings.jsonfor config, MCP, and plugin migration so local scope matches Claude settings precedence.descriptionfrontmatter, including README-style docs under.claude/commands.Root Cause
The previous hook rewrite handled
.claude/hooks/as a substring replacement. For absolute source commands, that left the original project-root prefix before the newly quoted.codex/hooksdirectory, producing invalid commands likeproject/'project/.codex/hooks'/script.sh.The migration also only used project
settings.jsonfor config/MCP/plugin decisions, so local settings such asdisabledMcpjsonServerscould be ignored even though Claude gives local settings higher precedence than project settings.Validation
just fmtcargo test -p codex-external-agent-migrationcargo test -p codex-app-server external_agent_configjust fix -p codex-external-agent-migrationjust fix -p codex-app-servergit diff --check