Make Apps guidance react to MCP availability - #30226
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec22727c83
ℹ️ 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".
| previous: PreviousSectionState<'_, Self::Snapshot>, | ||
| ) -> Option<Box<dyn ContextualUserFragment>> { | ||
| if !self.available | ||
| || matches!(previous, PreviousSectionState::Known(previous) if *previous) |
There was a problem hiding this comment.
When Apps are available, then a later turn records apps_instructions: false, the original <apps_instructions> developer fragment still remains in history because context is append-only. If Apps become available again, this condition treats the persisted false snapshot as needing a fresh render even though the retained fragment is already present, so the model receives duplicate Apps guidance and an unnecessary context/cache change.
AGENTS.md reference: AGENTS.md:L93-L96
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
this requires thrash in connector enablement status across turns which seems niche.
There was a problem hiding this comment.
connector enablement status
Like a user needs to toggle it on and off right? Flapping connectivity wouldn't trigger this?
There was a problem hiding this comment.
flapping connectivity can trigger it, but only if the current MCP tool inventory loses every accessible, enabled connector. there's a shared cache so small drops shouldn't cause flaps, but a full inventory drop can still produce true -> false -> true. in this edge case we just get duplicate guidance.
so yea tldr pretty much user toggle is the main case, which is also not common
aabbc2f to
4f4b7ab
Compare
ec22727 to
578ed02
Compare
4f4b7ab to
32c5958
Compare
97fcaa3 to
220e947
Compare
a2850b2 to
99c7ecb
Compare
220e947 to
5be8b9d
Compare
5be8b9d to
b26d39e
Compare
b26d39e to
e17469a
Compare
Why
Generic Apps guidance is emitted only while building static initial context. If the Apps MCP is unavailable then and recovers later in the same turn, its tools can become usable without the model receiving the guidance for using them.
What
apps_instructionsWorld State sectionlist_all_tools()read rather than adding a request-wide cache; a reconnect between the two reads can differ for one request and reconciles on the next requestApps and plugin guidance now render after the remaining static host-skills block, with Apps before Plugins.
Testing
just fmtjust test -p codex-core apps_instructionsjust test -p codex-core apps_guidance_appears_after_background_recovery_within_a_turnjust test -p codex-core drop_last_n_user_turns_trims_context_updates_above_rolled_back_turn