feat(codex): tell the user the plan tool is opt-in instead of forcing it on - #976
Open
kaizhou-lab wants to merge 2 commits into
Open
feat(codex): tell the user the plan tool is opt-in instead of forcing it on#976kaizhou-lab wants to merge 2 commits into
kaizhou-lab wants to merge 2 commits into
Conversation
… it on codex 0.152.0 made `update_plan` opt-in: UpdatePlanToolConfig lost its default = "default_true" (openai/codex a9519cbc, #41744; the same commit flips "default": true to false under that key in the generated config.schema.json). Re-checked against rust-v0.153.4 on 2026-09-09: still false. The model is therefore never offered the tool, no plan notification is emitted, and AionUi's ConversationPlanBar simply does not appear -- it renders nothing at all when there are no entries, so there is no error and nothing to notice. The obvious fix was to pass `-c tools.update_plan.enabled=true` when spawning the app-server, and that is what #961 did. It is the wrong fix. `codex --help` is explicit that `-c` overrides "a configuration value that would otherwise be loaded from ~/.codex/config.toml", so it silently overrides a user who turned the tool off on purpose. It is also not the same kind of setting as the shell_environment_policy overrides beside it: those are compatibility requirements the host must control, this is a preference upstream made opt-in deliberately. And what is lost by leaving it alone is a live progress view, not correctness -- the turn still runs and still answers. So AionUi says it once and leaves the choice with the user. session_drift_notice now returns every notice one version probe justifies rather than at most one. Both notices are one-shot per session and both need the same version string, so probing twice would be waste and a second already_reported key. The notice is version-gated, not config-gated. Reading the effective value means an extra app-server config/read round-trip -- AppToolsConfig is an untyped object in the v2 schema, so the value does come back; it is reachable, just not free. The cost of not reading it is one Info line for someone who already enabled the tool; the cost of reading it is a handshake on every session open. Does NOT unblock the codex version bump on its own: live_codex_produces_a_plan still asserts a plan frame, which cannot appear on 0.152+ unless the tool is enabled, and there is currently no supported way for the test to enable it. Writing to the operator's CODEX_HOME was tried and abandoned -- that directory also holds auth.json, and copying credentials into a temp home breaks token refresh ("your access token could not be refreshed because you have since logged out"). The operator's real login was verified intact afterwards. That test needs its own change and its own decision; see the PR body.
…fered
live_codex_produces_a_plan asserts a plan frame, which cannot exist on codex
>= 0.152.0 unless the user has opted into the update_plan tool. AionUi
deliberately does not opt in for them (see the CODEX_PLAN_OPT_IN notice in the
previous commit), so on such a machine the test was reporting an intentional
product decision as a regression.
The guard is targeted, not blanket. It skips ONLY when both are true: the
running codex is >= 0.152.0, and this machine's config does not enable the
tool. An operator who HAS enabled it still gets the full assertions, and older
codex still gets them because the tool was on by default there. The only case
that skips is the one where the frame genuinely cannot exist, and the skip
message names the setting, the file, and why AionUi will not write it.
Version parsing goes through aionui_session::parse_cli_version, so "0.152.0"
means the same thing here as in the backend. The config check is deliberately
crude -- any `enabled = true` under a `[tools.update_plan]` header -- because a
false negative only skips a test while a false positive would fail one, so the
crude check errs in the safe direction.
Verified all three branches against real binaries:
0.153.4, tool not enabled -> SKIPPED in 0.11s with the explanation
0.151.0, tool on by default -> runs, FRAME-TYPES includes `plan`, ok in 28.63s
0.153.4, config enables it -> does NOT skip (0 SKIPPED lines)
Pointing the test at a throwaway CODEX_HOME was implemented first and abandoned:
that directory also holds auth.json, and copying credentials into an alternate
home breaks token refresh ("your access token could not be refreshed because you
have since logged out or signed in to another account"). The operator's real
login was verified intact afterwards and the change reverted rather than shipped
unverified. This guard only READS the config; it never writes it.
Coverage cost, stated plainly: on a machine with the tool off, the codex plan
translation path (plan notification -> SessionEvent::Plan -> ConversationPlanBar)
is no longer exercised. Enabling the tool locally restores it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces #961. codex 0.152.0 made the
update_plantool opt-in; this tells the user rather than overriding their config.What changed upstream
UpdatePlanToolConfiglost itsdefault = "default_true"— openai/codexa9519cbc, "Make the update_plan tool opt-in" (#41744). The same commit flips the default under that key in the generatedcodex-rs/core/config.schema.json. Re-checked againstrust-v0.153.4on 2026-09-09: stillfalse.The model is never offered the tool, no plan notification is emitted, and
ConversationPlanBarrenders nothing at all (it returns null when there are no entries) — so there is no error and nothing for the user to notice.Why #961's approach was wrong
#961 passed
-c tools.update_plan.enabled=truewhen spawning the app-server.codex --helpis explicit about what that does:So it silently overrides a user who turned the tool off on purpose. It is also not the same kind of setting as the two overrides beside it —
shell_environment_policy.inherit=all/include_only=[]are compatibility requirements the host must control;update_planis a preference upstream made opt-in deliberately.And the stake is smaller than #961 claimed: what is lost is a live progress view, not correctness. The turn still runs and still answers. #961's "the plan card stays permanently empty" was also inaccurate — the bar does not appear at all.
What this does instead
One Info notice, once per session, when the detected codex is ≥ 0.152.0, naming the setting and the file.
session_drift_noticenow returns every notice a single version probe justifies instead of at most one: both are one-shot per session and both need the same version string, so probing twice would be waste and a secondalready_reportedkey.Version-gated, not config-gated, on purpose. Reading the effective value means an extra app-server
config/readround-trip —AppToolsConfigis an untyped object in the v2 schema, so the value does come back; it is reachable, just not free. The cost of not reading it is one Info line for someone who already enabled the tool; the cost of reading it is a handshake on every session open. Happy to switch if that trade looks wrong.The live test: a targeted skip (second commit)
live_codex_produces_a_planasserts a plan frame, which cannot exist on 0.152+ unless the user opted in — so on such a machine it was reporting an intentional product decision as a regression.The guard skips only when both are true: codex ≥ 0.152.0 and this machine's config does not enable the tool. An operator who has enabled it still gets the full assertions; older codex still gets them because the tool was on by default. The skip message names the setting, the file, and why AionUi will not write it.
All three branches verified against real binaries:
FRAME-TYPESincludesplan, ok in 28.63sVersion parsing goes through
aionui_session::parse_cli_version, so0.152.0means the same here as in the backend. The config check is deliberately crude — anyenabled = trueunder a[tools.update_plan]header — because a false negative only skips a test while a false positive would fail one.Coverage cost, stated plainly: on a machine with the tool off, the codex plan translation path (plan notification →
SessionEvent::Plan→ConversationPlanBar) is no longer exercised. Enabling the tool locally restores it.An approach that was tried and abandoned
Pointing the test at a throwaway
CODEX_HOMEseeded with the setting. That directory also holdsauth.json, and copying credentials into an alternate home breaks token refresh:The operator's real login was verified intact afterwards and the change reverted rather than shipped unverified. The guard that shipped only reads the config; it never writes it.
Tests
cargo test -p aionui-session --lib:cli_version16/16 (two new: the 0.152.0 boundary, and that the notice is codex-only and carries the fix rather than just the diagnosis),codex_conn145/145,antigravity111/111. Clippy clean on both crates, fmt clean.AionUi side
The notice ships an English fallback string, so it works untranslated. Localising it needs a
conversation.agentTip.codes.CODEX_PLAN_OPT_IN.bodyentry in AionUi.Source change — left for human review, no auto-merge.