[hooks] userpromptsubmit - hook before user's prompt is executed#14626
[hooks] userpromptsubmit - hook before user's prompt is executed#14626eternal-openai merged 10 commits intomainfrom
Conversation
0a31f07 to
f91c2e5
Compare
…sion_start to also use developer message for this
2902d26 to
8003348
Compare
| turn_id: Option<String>, | ||
| parse: fn(&ConfiguredHandler, CommandRunResult, Option<String>) -> ParsedHandler<T>, | ||
| ) -> Vec<ParsedHandler<T>> { | ||
| let results = join_all( |
There was a problem hiding this comment.
This still launches every handler concurrently, so a blocking user-prompt-submit hook cannot prevent later handlers from running/injecting context. That broke the pre-submit stop semantics + conflicts with the reported Sync execution mode
There was a problem hiding this comment.
I'm under the impression that the code matches the spec here -- the spec doesn't have a concept of sequential sync, all hooks are run in parallel all the time (to save time, etc). What sync vs async means is that async hooks are run purely in the background, i.e. the thread doesn't wait for them to complete (we haven't implemented this yet). Sync means that we block the loop until all sync hooks complete. Agree that the naming in the code could be a bit more clear, but I'm leaning towards leaving this as-is
| for pending_input in accepted_pending_input { | ||
| record_pending_input(&sess, &turn_context, pending_input).await; | ||
| } | ||
| record_additional_contexts(&sess, &turn_context, blocked_pending_input_contexts).await; |
There was a problem hiding this comment.
If a later queued prompt is blocked after an earlier queued prompt was accepted, this injects the blocked prompt’s context into the accepted follow-up request?
There was a problem hiding this comment.
Yes, that was a detail that I was paying attention to. The spec indeed wants us to accept additionalContext regardless of block, so: if queued prompt A is accepted, queued prompt B is blocked, and B returns additionalContext, we record A, then record B’s additional context, then build the next sampling request. So yes, B’s context can influence the follow-up request that contains A
Basically the mechanics of additionalContext is that it's added no matter what, if the hooks sets it
| _ => false, | ||
| }, | ||
| HookEventName::Stop => true, | ||
| HookEventName::UserPromptSubmit | HookEventName::Stop => true, |
There was a problem hiding this comment.
UserPromptSubmit matchers are parsed and validated, but this makes them unconditional at runtime. Either honor matchers or reject them in config
There was a problem hiding this comment.
Great catch, addressing...
There was a problem hiding this comment.
UserPromptSubmit matchers were already ignored at runtime, but discovery was still parsing/validating/storing them, which meant an invalid matcher could still warn or prevent registration. I changed discovery to ignore matchers for UserPromptSubmit (and Stop) as well
This matches what the spec wants:
UserPromptSubmit, Stop, [...] don’t support matchers and always fire on every occurrence. If you add a matcher field to these events, it is silently ignored.
codex-rs/core/src/hook_runtime.rs
Outdated
| let request = codex_hooks::SessionStartRequest { | ||
| session_id: sess.conversation_id, | ||
| cwd: turn_context.cwd.clone(), | ||
| transcript_path: sess.current_rollout_path().await, |
There was a problem hiding this comment.
is the file at the current_rollout_path guaranteed to exist at this point? it seems like it's only materialized when the prompt is recorded below by record_user_prompt_and_emit_turn_item
There was a problem hiding this comment.
Thanks! Fixed it so that we make sure to materialize the transcript before the hook runs
There was a problem hiding this comment.
havent looked into this too deeply, but i want to be sure we aren't reverting intentional work. it seems from this pr that defering the materialization until the first UserMessage was intentional-- is it ok to basically undo that?
There was a problem hiding this comment.
Yeah @etraut-openai would love to have your thoughts here, but @sayan-oai I guess it's not exactly a revert, because this only runs if hooks require it -- and I'm not sure how to get around that requirement without breaking the spec. I'm also contemplating how closely we want to match the transcript style that's used in the industry, since ours is fairly different of course -- so maybe one day this becomes a different transcript mechanism altogether
| PendingInputHookDisposition::Blocked { | ||
| additional_contexts, | ||
| } => { | ||
| record_additional_contexts(self, &turn_context, additional_contexts).await; |
There was a problem hiding this comment.
the similar pending_input loop in run_turn seems to break + requeue remaining items on Blocked, but this one seems to continue. why are they different?
There was a problem hiding this comment.
@charley-oai, why do we drain all pending items on task_finished and persisting them in conversation history? I see this pr. Just curious if this is behavior we want to continue with prompt-submission turn hooks.
There was a problem hiding this comment.
Yeah, thank you Sayan & Charley for helping me understand and untangle this. I think the reef that I hit surfing through here is that:
- Previously this was introduced to handle a kind of weird race between turn ending & queued messages coming in. So my read here is that we wanted to at least persist those loose-hanging prompts into history, then if the model ever started up that thread again, it would catch em?
- But my little catch-22 here is that this mechanism would bypass hooks by doing so
So it sounds like we need to either (a) make sure this race can't happen in a different way, like automatically starting the next real turn or (b) make some kind of new concept of queue persistence so that the next time the thread gets started, it'll run through the normal hook processing pipeline. But let me know if I'm catching the wave wrong
There was a problem hiding this comment.
Alright, thanks @charley-oai -- I'm going to descope that from this PR then and come back to fixing this issue in its own PR, seems like it would be a bit much
| let response_item: ResponseItem = initial_input_for_turn.clone().into(); | ||
| sess.record_user_prompt_and_emit_turn_item(turn_context.as_ref(), &input, response_item) | ||
| .await; | ||
| record_additional_contexts(&sess, &turn_context, additional_contexts).await; |
There was a problem hiding this comment.
developer messages are dropped on remote compaction, so additional context will disappear. is it ok for this to disappear from model history?
There was a problem hiding this comment.
Hmm, thanks for telling me about that! I'll think about it some more, but my instinct is that that's okay:
- various hooks are likely to run again after compaction
- developers can use pre/post compaction hooks to ensure what they want gets reinserted
There was a problem hiding this comment.
yeah i think this is non-blocking; we can see if the model becomes confused.
developers can use pre/post compaction hooks to ensure what they want gets reinserted
fair, but i dont think many users will be aware of the quirks of dev messages and the compaction lifecycle :)
sayan-oai
left a comment
There was a problem hiding this comment.
thanks for digging into the details to make sure this lands well!
Sample hook for testing - if you write "[block-user-submit]" this hook will stop the thread:
example run
.codex/config.toml
.codex/hooks.json
.codex/hooks/user_prompt_submit_demo.py