deprecate legacy notify#20524
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
etraut-openai
left a comment
etraut-openai
left a comment
There was a problem hiding this comment.
Overall, looks good. I think the deprecation warning text could be improved.
|
|
||
| ## Notify | ||
|
|
||
| Codex can run a notification hook when the agent finishes a turn. See the configuration reference for the latest notification settings: |
There was a problem hiding this comment.
We shouldn't be adding documentation to the OSS repo. The docs live in a different (external) repo, and we don't want to try to keep two copies of the docs in sync.
No need to fix in this PR. It looks like others have been adding docs here too. I'll do a separate clean-up pass after this PR lands. I'll also update the AGENTS.md guidance because that's probably causing codex to add docs here.
Four PRs across opencode (3) and codex (1): - anomalyco/opencode#25166: merge-as-is, server.mdx adds /global/config docs - anomalyco/opencode#25143: merge-as-is, ecosystem.mdx adds opencode-swarm row - anomalyco/opencode#25180: merge-after-nits, pre-stream Token.estimate proactive compaction at processor.ts:543-568 + 3 new overflow regex arms - openai/codex#20524: merge-as-is, notify deprecation across README + TOML doc + JSON schema + struct doc + 2 metrics + DeprecationNotice event + 153-line user_notification.rs delete
3 participants
Why
notifyis the remaining compatibility surface from the legacy hook implementation. The newer lifecycle hook engine now owns the active hook system, so we should start steering users away from adding newnotifyconfigs before removing the old path entirely. This also adds a lightweight watchpoint for the deprecation so we can see how much legacy usage remains before the clean drop.What
notifycommand is configuredcodex.notify.configuredwhen a session starts with legacynotifyconfiguredcodex.notify.runwhen the legacy notify path fires after a completed turnnotifyas deprecated in the config schema and repo docscodex-rs/hooks/src/user_notification.rsfile that is no longer compiledNext steps
A follow-up PR can remove the legacy notify path entirely once we are ready for the clean drop. Before then, we can watch
codex.notify.configuredandcodex.notify.runto understand the deprecation impact and remaining active usage. The cleanup PR should then delete thenotifyconfig field, thelegacy_notifyimplementation, the old compatibility dispatch types and callsites that only exist for the legacy path, and the remaining compatibility docs/tests.Testing
cargo test -p codex-hookscargo test -p codex-configcargo test -p codex-core emits_deprecation_notice_for_notify