Gate Codex skills flag on CLI support #6520
Conversation
The Codex CLI removed the `skills` feature flag in newer versions , causing `goose configure` to fail with exit code 1 when it unconditionally passes `--enable/--disable skills`. This commit updates the logic to check `codex features list` before applying these flags. This ensures the flags are only passed when the installed CLI explicitly advertises support for them. While simply removing the flag usage was an option, this approach is designed to maintain backward compatibility for environments where the CLI version is pinned (e.g., in automation workflows). This also updates the documentation to clarify that the `skills` flag logic is legacy behavior for older CLIs. Signed-off-by: Yusuke Shimizu <[email protected]>
YusukeShimizu
force-pushed
the
codex-skills-feature-flag
branch
from
4e2b015 to
468d8f8
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses a breaking change in newer versions of the Codex CLI where the skills feature flag was removed (openai/codex#8850). The implementation adds backward compatibility by detecting whether the installed Codex CLI supports the skills feature flag via codex features list and only passing --enable/--disable skills flags when supported.
Changes:
- Added runtime feature detection to check if the Codex CLI supports the
skillsfeature flag - Updated documentation to clarify that
CODEX_ENABLE_SKILLSis a legacy toggle only respected on older CLIs - Added test coverage for the feature detection logic
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/goose/src/providers/codex.rs | Added skills_feature_flag_supported field, feature detection logic via codex features list, and conditional flag passing |
| documentation/docs/guides/cli-providers.md | Updated documentation to clarify skills support is native in newer CLIs and the env var is legacy-only |
| fn test_skills_feature_flag_args_based_on_features_list_support() { | ||
| let stdout = "undo stable false shell_tool stable true skills stable true steer beta false"; | ||
| let skills_supported = CodexProvider::feature_list_contains_feature(stdout, "skills"); | ||
| assert!(skills_supported); | ||
|
|
||
| assert_eq!( | ||
| CodexProvider::skills_feature_flag_args(skills_supported, true), | ||
| Some(("--enable", "skills")) | ||
| ); | ||
| assert_eq!( | ||
| CodexProvider::skills_feature_flag_args(skills_supported, false), | ||
| Some(("--disable", "skills")) | ||
| ); | ||
| } |
There was a problem hiding this comment.
The test only covers the case where the feature is supported. Add a test case for when skills is not in the feature list (i.e., skills_supported = false), which should verify that skills_feature_flag_args returns None. This is critical because the PR's entire purpose is handling newer CLIs where the flag is absent.
| fn feature_list_contains_feature(stdout: &str, feature: &str) -> bool { | ||
| // `codex features list` output is a whitespace-separated sequence of: | ||
| // `<name> <stability> <enabled>` repeated, e.g.: | ||
| // `undo stable false shell_tool stable true ...` | ||
| // | ||
| // We match by token to be robust to both newline- and space-delimited output. | ||
| stdout | ||
| .split_whitespace() | ||
| .collect::<Vec<_>>() | ||
| .windows(3) | ||
| .any(|w| { | ||
| w[0] == feature | ||
| && matches!(w[1], "stable" | "beta" | "experimental") | ||
| && matches!(w[2], "true" | "false") | ||
| }) | ||
| } |
There was a problem hiding this comment.
Add test cases for edge scenarios: empty stdout, feature name appearing but with invalid stability/enabled values, feature name as substring of another word, and stdout with fewer than 3 tokens. The windows(3) call could panic on short input if the slice has fewer than 3 elements.
| stdout | ||
| .split_whitespace() | ||
| .collect::<Vec<_>>() | ||
| .windows(3) | ||
| .any(|w| { | ||
| w[0] == feature | ||
| && matches!(w[1], "stable" | "beta" | "experimental") | ||
| && matches!(w[2], "true" | "false") | ||
| }) |
There was a problem hiding this comment.
If stdout has fewer than 3 tokens after split_whitespace(), the windows(3) iterator will be empty (not panic), but this edge case should be tested. An empty or malformed output from codex features list would silently return false, which is correct behavior but should be explicitly verified in tests.
| stdout | |
| .split_whitespace() | |
| .collect::<Vec<_>>() | |
| .windows(3) | |
| .any(|w| { | |
| w[0] == feature | |
| && matches!(w[1], "stable" | "beta" | "experimental") | |
| && matches!(w[2], "true" | "false") | |
| }) | |
| let mut tokens = stdout.split_whitespace(); | |
| loop { | |
| let name = match tokens.next() { | |
| Some(t) => t, | |
| None => break, | |
| }; | |
| let stability = match tokens.next() { | |
| Some(t) => t, | |
| None => break, | |
| }; | |
| let enabled = match tokens.next() { | |
| Some(t) => t, | |
| None => break, | |
| }; | |
| if name == feature | |
| && matches!(stability, "stable" | "beta" | "experimental") | |
| && matches!(enabled, "true" | "false") | |
| { | |
| return true; | |
| } | |
| } | |
| false |
1 participant
Summary
Addresses compatibility issues with newer versions of the Codex CLI where the
skillsfeature flag has been removed (referenced in openai/codex#8850 ).Previously,
goose configurewould fail with an exit code 1 on newer CLIs because it unconditionally passed the--enable/--disable skillsflags.While simply removing the flag checks was an option, this implementation is explicitly designed to maintain backward compatibility. This ensures stability for automation workflows and environments where CLI versions may be pinned.
Type of Change
AI Assistance
Testing
This change has been tested manually with both new and legacy versions of the Codex CLI to ensure robustness.
Screenshots/Demos (for UX changes)
Before (Crash on Codex CLI 0.85.0):
Before (Success on Codex CLI 0.77.0):
After (Backward Compatibility on Codex CLI 0.77.0):