[Hooks] Schema changes only - AppServer protocol additions & Hooks I/O schema#13408
[Hooks] Schema changes only - AppServer protocol additions & Hooks I/O schema#13408eternal-openai wants to merge 5 commits intomainfrom
Conversation
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
eternal-openai
requested review from
pakrym-oai and
pkomlev
and removed request for
pkomlev
jif-oai
left a comment
jif-oai
left a comment
There was a problem hiding this comment.
OOC, why fully dropping the previous hooks implementation (not drop here but mostly incompatible with what you're building here)
IMO the implementation direction was quite good
| .send_server_notification(ServerNotification::ItemCompleted(notification)) | ||
| .await; | ||
| } | ||
| EventMsg::HookStarted(_) => {} |
There was a problem hiding this comment.
why having them if we don't emit them?
This should be mapped to your hook/completed, ...
| turn: Turn { | ||
| id: event_turn_id, | ||
| items: vec![], | ||
| hook_runs: vec![], |
There was a problem hiding this comment.
huh? This sounds strange to never populate this no?
| | EventMsg::ShutdownComplete | ||
| | EventMsg::DeprecationNotice(_) | ||
| | EventMsg::ItemStarted(_) | ||
| | EventMsg::HookStarted(_) |
There was a problem hiding this comment.
Any reason not to persist them? My guess would be that this is quite relevant
There was a problem hiding this comment.
Persistence is handled by HookRunSummary in the larger PR, so that only the summaries of what the hook returned are persisted
| return; | ||
| } | ||
|
|
||
| self.ensure_turn().hook_runs.push(run); |
There was a problem hiding this comment.
This thing will check the current turn if there is one, otherwise create a mock turn in the history in order to push the run
But you don't seem to be filtering out non-turn based hooks such as "session start" for example. So why would we attach this one to a turn?
There was a problem hiding this comment.
Start session imo does have a turn attached, it's the first prompt that's sent when starting or resuming a session. I think the mechanics are simpler doing it this way with context additions
| { | ||
| SchemaSettings::draft07() | ||
| .with(|settings| { | ||
| settings.option_add_null_type = false; |
There was a problem hiding this comment.
I don't understand why do we have this but you also redefine your own NullableString? Someting feels a bit wrong here...
As a result you can see that your JSON schema can put null when an Option is None but schemars will actually not include them...
There was a problem hiding this comment.
Thanks, I had removed NullableString for now, since it would throw warnings in this version of the PR
| #[derive(Debug, Clone, Copy, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)] | ||
| #[serde(rename_all = "snake_case")] | ||
| pub enum HookRunStatus { | ||
| Running, |
There was a problem hiding this comment.
The Running state is never used and I don't find any gates for race on subscription/execution + this is never reported to the user
2 participants
This is just the schema + in development feature flag set up from #13276 to make review a bit easier
Testing
Details
codex_hooksfeature flag: introduces an under-development feature gate, off by default.SessionStartandStop) so hook contracts live incodex-rsEventMsg::HookStarted: emitted when a hook run begins.EventMsg::HookCompleted: emitted when a hook run finishes.HookEventName: identifies which hook event ran.HookHandlerType: identifies what kind of handler ran.HookExecutionMode: describes how the handler was executed.HookScope: describes where the hook applies.HookRunStatus: captures the final outcome of the run.HookOutputEntry: represents one normalized output row from a hook.HookRunSummary: represents the full normalized, displayable, and persistable result of one hook run.hook/startedhook/completedTurn.hookRunsso completed hook runs can be attached to a turnmain.