[codex] Honor role-defined spawn service tiers#22169
Conversation
jif-oai
left a comment
There was a problem hiding this comment.
Now that a role-provided service_tier can override the explicit spawn argument, can we surface that in the role guidance too?
| .await; | ||
| let mut config = | ||
| build_agent_spawn_config(&session.get_base_instructions().await, turn.as_ref())?; | ||
| if let Some(service_tier) = args.service_tier.as_ref() { |
There was a problem hiding this comment.
I don't think this survives role application. apply_role_to_config() rebuilds Config from the layer stack
This should be preserved through the role reload
| .await; | ||
| let mut config = | ||
| build_agent_spawn_config(&session.get_base_instructions().await, turn.as_ref())?; | ||
| if let Some(service_tier) = args.service_tier.as_ref() { |
| requested_service_tier: Option<&str>, | ||
| ) -> Result<(), FunctionCallError> { | ||
| let Some(candidate_service_tier) = requested_service_tier.or(parent_service_tier) else { | ||
| let requested_service_tier_is_effective = |
There was a problem hiding this comment.
This makes validation of the explicit spawn argument role-dependent. Invalid request like service_tier=wtf stops failing if the selected role overwrite the field
I think we should always error. Mainly in 0-shot
| let Some(candidate_service_tier) = config | ||
| .service_tier | ||
| .clone() | ||
| .or_else(|| parent_service_tier.map(str::to_string)) |
There was a problem hiding this comment.
(found by Codex)
This only falls back to the parent before support validation. If the role contributes an unsupported child tier, we never retry a still-valid parent tier; we clear the field below and spawn with no tier instead
| } | ||
|
|
||
| if requested_service_tier.is_none() { | ||
| if !requested_service_tier_is_effective { |
There was a problem hiding this comment.
Silently drops unsupported role-authored tiers. Same comment as before
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d64b20f1e
ℹ️ 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".
| } | ||
| let (preserve_current_profile, preserve_current_provider) = | ||
| preservation_policy(config, &role_layer_toml); | ||
| let preserve_current_service_tier = role_layer_toml.get("service_tier").is_none(); |
There was a problem hiding this comment.
Honor profile-scoped role service tiers
This only checks for a top-level service_tier, but role files can select or define profiles, and ConfigProfile has its own service_tier; Config::load_config_with_layer_stack only reads config_profile.service_tier when the service-tier override is None. With a role such as profile = "tiered" plus [profiles.tiered] service_tier = "priority" (or a role update to the active profile), this line preserves the parent/request tier as an override, so the profile-defined role tier is ignored instead of being honored.
Useful? React with 👍 / 👎.
| let service_tier_note = service_tier | ||
| .map(|service_tier| { | ||
| format!( | ||
| "\n- This role's service tier is set to `{service_tier}` and overrides any spawn request service tier." |
There was a problem hiding this comment.
Under validation constraints right?
Why
Custom agent roles are ordinary config layers, so a role file can already express
service_tierjust like other config values. The spawned-agent tier path needs to preserve that effective role config and follow the same precedence pattern as model/reasoning.What changed
service_tieronto the child config before role application, so a role config layer can override it just like role-defined model/reasoning settings do.Validation
just fmtgit diff --check