Conversation
gt-oai
force-pushed
the
gt/hooks-1
branch
11 times, most recently
from
eb4aa5d to
917a82c
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://waybackassets.bk21.net/20260318054711im_/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: 917a82c82a
ℹ️ 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".
| command | ||
| .current_dir(&payload.cwd) | ||
| .stdin(Stdio::null()) |
There was a problem hiding this comment.
Setting current_dir to the turn’s cwd changes how relative notify commands are resolved. If a user configures notify = ["./scripts/notify.sh"] (relative to the directory where Codex was started), this now resolves relative to the per-turn working directory instead, so the hook can fail with “No such file or directory” after the user switches to a different repo or deletes the cwd. The legacy notifier didn’t change current_dir, so this is a behavior regression that breaks existing setups depending on relative paths.
Useful? React with 👍 / 👎.
codex-rs/core/src/hooks/mod.rs
Outdated
| return; | ||
| }; | ||
|
|
||
| for hook in hooks { |
There was a problem hiding this comment.
The order matter so we should probably document it at some point
There was a problem hiding this comment.
will document in the docs
codex-rs/core/src/hooks/mod.rs
Outdated
| pub(crate) fn new(config: &Config) -> Self { | ||
| let after_agent_hooks = get_notify_hook(config).into_iter().collect(); | ||
| Self { | ||
| hooks: HashMap::from([(HookEventKind::AfterAgent, after_agent_hooks)]), |
There was a problem hiding this comment.
We have a finite set of possible hooks (after/before agent, after/before session, ...). Instead of using a HashMap with a finite enum, should we have a well defined structure? It would make this easier to read and to document
There was a problem hiding this comment.
removed the map and replaced with strongly-typed struct field.
gt-oai
force-pushed
the
gt/hooks-1
branch
7 times, most recently
from
2d6eb8d to
ec59a3d
Compare
| last_assistant_message: last_agent_message.clone(), | ||
| }); | ||
| sess.hooks() | ||
| .dispatch(crate::hooks::HookPayload { |
There was a problem hiding this comment.
Maybe we should think about a timeout on those hooks. Otherwise a badly configured hook will prevent the turn from finishing. Ideally we can make it configurable
Can be in a follow-up PR
There was a problem hiding this comment.
yup timeout is in the design doc
| pub(crate) async fn dispatch(&self, hook_payload: HookPayload) { | ||
| // TODO(gt): support interrupting program execution by returning a result here. | ||
| for hook in self.hooks_for_event(&hook_payload.hook_event) { | ||
| let outcome = hook.execute(&hook_payload).await; |
There was a problem hiding this comment.
It would be cool to measure and log hook failures
| // TODO(gt): support interrupting program execution by returning a result here. | ||
| for hook in self.hooks_for_event(&hook_payload.hook_event) { | ||
| let outcome = hook.execute(&hook_payload).await; | ||
| if matches!(outcome, HookOutcome::Stop) { |
There was a problem hiding this comment.
I guess it's on purpose that the Stop does not actually stop anything but the dispatch of the other hooks?
There was a problem hiding this comment.
not right now, will propagate the error up in future PRs
2 participants
This introduces a
Hooksservice. It registers hooks from config and dispatches hook events at runtime.N.B. The hook config is not wired up to this yet. But for legacy reasons, we wire up
notifyfrom config and power it using hooks now. Nothing about thenotifyinterface has changed.I'd start by reviewing
hooks/types.rsSome things to note: