|
| 1 | +# openai/codex PR #20319 — Add managed-hooks-only hook requirement |
| 2 | + |
| 3 | +- PR: https://github.com/openai/codex/pull/20319 |
| 4 | +- Head SHA: `7216b2514dd7dfba0c4a80795ae8872ea0034e90` |
| 5 | +- Files touched: 17 files across `app-server-protocol/schema/{json,typescript}` (regenerated), `app-server-protocol/src/protocol/v2.rs`, `app-server/{README.md,src/config_api.rs}`, `cloud-requirements/src/lib.rs` (test fixtures), `config/src/{config_requirements.rs,state.rs}`, `core/src/config/{config_loader_tests.rs,config_tests.rs,mod.rs}`, `hooks/src/engine/{discovery.rs,mod_tests.rs}`, `tui/src/debug_config.rs`, `docs/config.md` (+547/-4, 17 files) |
| 6 | + |
| 7 | +## Specific citations |
| 8 | + |
| 9 | +- New TOML field `allow_managed_hooks_only: Option<bool>` lands on `ConfigRequirementsToml` at `config/src/config_requirements.rs:647`, on `ConfigRequirements` at `:89`, and on `ConfigRequirementsWithSources` at `:696` — and is wire-exposed as nullable `allowManagedHooksOnly: boolean | null` on the v2 `ConfigRequirements` payload at `app-server-protocol/src/protocol/v2.rs:955` and TS `ConfigRequirements.ts:9`. JSON schema additions at `codex_app_server_protocol.schemas.json:7390-7395` and the v2 mirror confirm the nullable boolean shape. |
| 10 | +- The `is_empty()` discipline at `config/src/config_requirements.rs:892` correctly adds `&& self.allow_managed_hooks_only.is_none()` so a `requirements.toml` containing only `allow_managed_hooks_only = false` still counts as configured. The dedicated regression at `:1280-1290` (`deserialize_allow_managed_hooks_only` and `allow_managed_hooks_only_false_is_still_configured`) locks both the `Some(true)` and the implicit `Some(false)` truthiness asserting `!requirements.is_empty()` — this is the canonical "explicit-false-is-not-default" pattern and matches how other allow-list fields here are handled. |
| 11 | +- The mapping at `app-server/src/config_api.rs:287` (`allow_managed_hooks_only: requirements.allow_managed_hooks_only`) and the round-trip test at `:522, :606` (`assert_eq!(mapped.allow_managed_hooks_only, Some(true))`) lock the `requirements.toml → API` projection. |
| 12 | +- The 12+ test-fixture call sites in `cloud-requirements/src/lib.rs` (lines `:1206, :1289, :1323, :1374, :1491, :1572, :1651, :1858, :1899, :1960, :2017, :2076, :2136, :2196, :2286, :2318` — 16 sites in this file alone) all add `allow_managed_hooks_only: None` to existing fixtures. This is the cost of adding a non-`Default`-derivable field to a struct that test fixtures construct with explicit `Field: None` everywhere. Worth flagging as cosmetic friction (see nit #2). |
| 13 | +- `hooks/src/engine/discovery.rs` adds 71 lines (the actual gating logic) and `hooks/src/engine/mod_tests.rs` adds 361 lines (allow_managed_hooks_only behavioral coverage). Per the PR body: when `allow_managed_hooks_only = true`, discovery keeps managed-requirements hooks + managed-config-layer hooks, drops user/project/session `hooks.json` and `[hooks]` entries, drops current unmanaged plugin hooks, and emits "concise startup warnings" for the dropped entries. The 361-line test suite is the right size for the ~5 source/scope axes (managed-requirements, managed-config, user, project, plugin) crossed against (skip / keep) outcomes. |
| 14 | +- `docs/config.md` at `+6 lines` and `app-server/README.md:225-226` (`configRequirements/read` section now lists `allowManagedHooksOnly`) correctly document the requirements-only enforcement: this is policy that *can only* be set in `requirements.toml` or via MDM, not in `config.toml`. The `discovery.rs` change "ignores any `allow_managed_hooks_only` key placed in ordinary `config.toml` layers" enforces that anchoring at the loader. |
| 15 | + |
| 16 | +## Verdict: merge-after-nits |
| 17 | + |
| 18 | +## Concerns / nits |
| 19 | + |
| 20 | +1. **No regression that asserts `config.toml`-placed `allow_managed_hooks_only` is *ignored*** in the diff slice. The PR body says "ignores any `allow_managed_hooks_only` key placed in ordinary `config.toml` layers" — that's the security-relevant property of "policy can only be set in `requirements.toml`/MDM, never escalated by user config". A test that puts `allow_managed_hooks_only = true` in user `config.toml` (no `requirements.toml`) and asserts the loaded `ConfigRequirements.allow_managed_hooks_only.is_none()` would lock the loader-side anchoring. Without it, a future loader refactor that accidentally honors the user-config key silently breaks the threat model. |
| 21 | +2. **16 test-fixture sites needing `allow_managed_hooks_only: None`** is a sign that `ConfigRequirementsToml` has accumulated enough Optional fields to warrant `#[derive(Default)]` plus `..Default::default()` in fixtures. The PR adds `allow_managed_hooks_only: None` to ~16 sites in `cloud-requirements/src/lib.rs` alone; the next field-addition PR will pay the same tax. A small precursor PR adding `Default` and migrating fixtures to spread syntax would amortize this cost across all future requirements-field additions. (Out of scope here, but worth a follow-up issue.) |
| 22 | +3. **The "concise startup warnings" claim in the PR body** is not verifiable from the diff slice — the text doesn't show the warning copy itself. Would benefit from a doc/inline comment in `discovery.rs` showing the exact warning text emitted (so users grepping logs for "managed hooks only" can find it) and a regression that asserts a specific warning is emitted when a user-config hook is dropped. |
| 23 | +4. **Plugin-hooks treated as unmanaged for now** is a forward-compat hazard worth a comment in `discovery.rs`. The PR body says "Current plugin hooks are treated as unmanaged; future managed plugin hooks can opt in through the existing `is_managed` bit". A future plugin author who reads only the source code may not realize their hook is being dropped under `allow_managed_hooks_only = true` until they read the engine code. A `// SAFETY: when plugins gain an is_managed bit, this branch ...` comment would help. |
| 24 | +5. **MDM and "managed config" layer marking** changed in `core/src/config/mod.rs` (+1 line) — the diff slice shows just the +1 line but doesn't include the `state.rs` or layer-marking surface. Worth confirming in the merge that the same predicate (`is_managed`) is now consistently true across MDM, system, and legacy managed layers, not just the loader-side check. A round-trip test that loads an MDM layer and asserts `layer.is_managed_for_hooks() == true` would lock this — the PR mentions it but the assertion is not visible in this diff slice. |
0 commit comments