Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e667824d0
ℹ️ 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".
codex-rs/core/src/config/mod.rs
Outdated
| let features = apply_feature_requirements( | ||
| configured_features, | ||
| requirements.feature_requirements.clone(), | ||
| )?; |
There was a problem hiding this comment.
Re-apply feature requirements after runtime flag updates
feature_requirements are only enforced once during Config construction here, but the running app can later mutate config.features directly (for example via AppEvent::UpdateFeatureFlags in tui/src/app.rs, which calls enable/disable without checking requirements). In a managed environment this allows users to re-enable features that were pinned to false by enterprise requirements after startup, so the new policy is bypassable for the active session.
Useful? React with 👍 / 👎.
08aa186 to
44b4b91
Compare
5842091 to
db5ef09
Compare
codex-rs/core/src/config/mod.rs
Outdated
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn feature_requirements_override_default_feature_values() -> std::io::Result<()> { |
There was a problem hiding this comment.
I’m a bit confused. I see that we reject/raise errors during validation, but this test is about overriding. What is the intended behavior in this PR — when to reject vs override?
Personally, I think override should be the general approach if possible.
There was a problem hiding this comment.
The intended behavior is override for effective runtime state, but reject for explicit config writes. We normalize the loaded/in-memory feature set so enterprise requirements always win, but we reject persisted conflicting writes so we don’t save misleading config that can never take effect.
Why
Enterprises can already constrain approvals, sandboxing, and web search through
requirements.tomland MDM, but feature flags were still only configurable as managed defaults. That meant an enterprise could suggest feature values, but it could not actually pin them.This change closes that gap and makes enterprise feature requirements behave like the other constrained settings. The effective feature set now stays consistent with enterprise requirements during config load, when config writes are validated, and when runtime code mutates feature flags later in the session.
It also tightens the runtime API for managed features.
ManagedFeaturesnow follows the same constraint-oriented shape asConstrained<T>instead of exposing panic-prone mutation helpers, and production code can no longer construct it through an unconstrainedFrom<Features>path.The PR also hardens the
compact_resume_forkintegration coverage on Windows. After the feature-management changes,compact_resume_after_second_compaction_preserves_historywas overflowing the libtest/Tokio thread stacks on Windows, so the test now uses an explicit larger-stack harness as a pragmatic mitigation. That may not be the ideal root-cause fix, and it merits a parallel investigation into whether part of the async future chain should be boxed to reduce stack pressure instead.What Changed
Enterprises can now pin feature values in
requirements.tomlwith the requirements-sidefeaturestable:Only canonical feature keys are allowed in the requirements
featurestable; omitted keys remain unconstrained.ConfigRequirementsToml, threaded it through source-preserving requirements merge and normalization incodex-config, and made the TOML surface use[features](while still accepting legacy[feature_requirements]for compatibility).featureRequirementsfromconfigRequirements/read, regenerated the JSON/TypeScript schema artifacts, and updated the app-server README.ManagedFeatures, backed byConstrainedWithSource<Features>, and changed its API to mirrorConstrained<T>:can_set(...),set(...) -> ConstraintResult<()>, and result-returningenable/disable/set_enabledhelpers.ManagedFeatures; callers that need those behaviors now mutate a plainFeaturesvalue and reapply it throughset(...), so the constrained wrapper remains the enforcement boundary.ManagedFeatures. Non-test code now creates it through the configured feature-loading path, andimpl From<Features> for ManagedFeaturesis restricted to#[cfg(test)].core_test_supportBazel target to include the bundled core model-catalog fixtures in its runtime data, so helper code that resolvescore/models.jsonthrough runfiles works in remote Bazel test environments.compact_resume_after_second_compaction_preserves_historyinside an explicit 8 MiB test thread and Tokio runtime worker stack, following the existing larger-stack integration-test pattern, to keep the Windowscompact_resume_forktest slice from aborting while a parallel investigation continues into whether some of the underlying async futures should be boxed.Verification
cargo test -p codex-configcargo test -p codex-core feature_requirements_ -- --nocapturecargo test -p codex-core load_requirements_toml_produces_expected_constraints -- --nocapturecargo test -p codex-core compact_resume_after_second_compaction_preserves_history -- --nocapturecargo test -p codex-core compact_resume_fork -- --nocapturecodex-coretests/allbinary withRUST_MIN_STACK=262144forcompact_resume_after_second_compaction_preserves_historyto confirm the explicit-stack harness fixes the deterministic low-stack repro.cargo test -p codex-corecodex/test_stdio_serverbinaries or hit existingsearch_toolwiremock mismatches.Docs
developers.openai.com/codexshould document the requirements-side[features]table for enterprise and MDM-managed configuration, including that it only accepts canonical feature keys and that conflicting config writes are rejected.