refactor(codex): unify top-level allow-list, fix TOML vs JSON/YAML drift [skip changelog] - #968
Merged
Merged
Conversation
… drift [skip changelog] Closes #966. The Codex config top-level allow-list was maintained twice: `KNOWN_TOP_LEVEL_KEYS` + `KNOWN_TABLE_KEYS` in schemas/codex.rs (TOML backend) and `KNOWN_CONFIG_TOP_LEVEL_KEYS` in rules/codex.rs (JSON/YAML backend). They had already drifted, so the same key was accepted by one backend and flagged by the other: - `debug` (v0.129 `[debug]` table) and `include_apply_patch_tool`: valid as TOML, false-positive `CDX-CFG-006` as JSON/YAML. - `js_repl_node_path` / `js_repl_node_module_dirs`: valid as JSON/YAML, false-positive `CDX-004` as TOML. Fix: one source of truth. `schemas/codex.rs::is_known_top_level_key` (over `KNOWN_TOP_LEVEL_KEYS` ∪ `KNOWN_TABLE_KEYS`) is now consulted by both `detect_unknown_keys` (TOML) and `collect_unknown_codex_keys` (JSON/YAML). Deleted the duplicate `KNOWN_CONFIG_TOP_LEVEL_KEYS`; added the two js_repl scalars to the schemas list. Lenient union (102 keys) - strictly fewer false positives, both backends now agree. Incidentally removes the duplication gemini-code-assist flagged on #964. Nested allow-lists (features/tui/mcp_servers/shell_env/permissions/apps) already live only on the rules side and are untouched. Regression tests: `test_codex_unified_allowlist_fixes_json_toml_drift` (JSON accepts `debug`) and `test_codex_unified_allowlist_fixes_toml_drift` (TOML accepts `js_repl_node_path`).
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request unifies the top-level configuration key allow-lists for TOML and JSON/YAML backends by centralizing the validation logic in crates/agnix-core/src/schemas/codex.rs. It removes the redundant KNOWN_CONFIG_TOP_LEVEL_KEYS constant from rules/codex.rs and introduces a shared is_known_top_level_key function to prevent configuration drift. Additionally, the js_repl_node_module_dirs and js_repl_node_path keys were added to the schema to ensure they are recognized across all formats. I have no feedback to provide.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a behavioral drift in Codex config validation by unifying the “known top-level keys” allow-list so TOML and JSON/YAML backends can’t disagree on whether a key is valid.
Changes:
- Introduces
schemas::codex::is_known_top_level_keyas the shared predicate overKNOWN_TOP_LEVEL_KEYS ∪ KNOWN_TABLE_KEYS. - Updates both TOML unknown-key detection (
detect_unknown_keys) and JSON/YAML unknown-key collection (collect_unknown_codex_keys) to consult the shared predicate, removing the duplicatedKNOWN_CONFIG_TOP_LEVEL_KEYS. - Adds regression tests covering previously divergent keys (
debug/include_apply_patch_toolvsjs_repl_*).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/agnix-core/src/schemas/codex.rs | Adds shared top-level-key predicate, folds js_repl_* scalars into the schema list, and adds TOML regression test. |
| crates/agnix-core/src/rules/codex.rs | Removes duplicated JSON/YAML top-level allow-list, switches to shared predicate, and adds JSON regression test + comment updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
avifenesh
added a commit
that referenced
this pull request
May 24, 2026
The Codex catch-up (#964/#959), the requirements.toml validator (#967/#965), the Cursor/OpenCode baseline bumps (#963/#960/#961), and the allow-list drift fix (#968/#966) all merged with [skip changelog] but are user-facing (new validator + rules, new recognized config keys, fixed false positives). Add the Added/Changed/Fixed entries that should have accompanied them. CI-only changes (#958 watcher fix, #962 changelog gate) remain intentionally unlisted.
3 tasks
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.
Summary
Closes #966. This is a behavior fix, not just dedup. The Codex config top-level allow-list was maintained in two places that had already drifted, so the same key was accepted by one backend and flagged as unknown by the other:
debug(v0.129[debug]table),include_apply_patch_tooljs_repl_node_path,js_repl_node_module_dirsFix
One source of truth:
schemas/codex.rs::is_known_top_level_key(overKNOWN_TOP_LEVEL_KEYS∪KNOWN_TABLE_KEYS) is now consulted by bothdetect_unknown_keys(TOML) andcollect_unknown_codex_keys(JSON/YAML). The duplicateKNOWN_CONFIG_TOP_LEVEL_KEYSconst is deleted; the twojs_replscalars are folded into the schemas list. Lenient union (102 keys) - strictly fewer false positives, and the two backends can no longer disagree.Incidentally removes the duplication
gemini-code-assistflagged on #964. The nested allow-lists (features/tui/mcp_servers/shell_environment_policy/permissions.network/apps) already live only on the rules side (the schemas parser only walks top-level) and are untouched.Note on current state
This unifies to a lenient union; it does not audit whether all 102 keys are still valid in
rust-v0.133.0. Filed a separate follow-up for that audit (linked below). Net effect here is purely to stop the two backends from disagreeing.Test plan
cargo test -p agnix-core- all passtest_codex_unified_allowlist_fixes_json_toml_drift(JSON acceptsdebug/include_apply_patch_tool),test_codex_unified_allowlist_fixes_toml_drift(TOML acceptsjs_repl_*)cargo clippyclean,cargo fmt --checkclean--checkclean (no rule-count change - refactor only)