Skip to content

fix(tools): stop registering image_gen.imagegen (reserved provider name)#389

Open
andrei-hasna wants to merge 1 commit into
mainfrom
fix/imagegen-reserved-namespace-regression
Open

fix(tools): stop registering image_gen.imagegen (reserved provider name)#389
andrei-hasna wants to merge 1 commit into
mainfrom
fix/imagegen-reserved-namespace-regression

Conversation

@andrei-hasna

Copy link
Copy Markdown
Contributor

Summary

Adds a durable, centralized guard so no first-party / extension / code-mode
tool can ever be assembled under the Responses-API-reserved image_gen
namespace (wire name image_gen.imagegen), which the model rejects with a 400
that fails the entire turn:

400 Invalid Value: 'tools'. Function 'image_gen.imagegen' is reserved for use
by this model and must match the configured schema.

Investigation — what git show 5cc22bc71 did, and where image_gen.imagegen still comes from

5cc22bc71 fix(image-gen): move standalone imagegen off reserved image_gen namespace
renamed the standalone image tool's namespace from the reserved image_gen to
the non-reserved images in two producer sites:

  • codex-rs/ext/image-generation/src/lib.rsIMAGE_GEN_NAMESPACE: "image_gen" -> "images"
  • codex-rs/core/src/tools/spec_plan.rsIMAGE_GEN_NAMESPACE: "image_gen" -> "images"

On current main (0.1.77) this is NOT reverted and there is NO second
producer path.
Verified:

  • Both constants are "images" on origin/main.
  • The only remaining "image_gen" string literals in codex-rs are the two
    RESERVED-name allowlists (app-server .../thread_processor.rs,
    core/src/config/mod.rs) plus the ext regression-test assertion — none are
    producers. The code-mode nested name is derived from the same
    IMAGE_GEN_NAMESPACE constant, so it is images__imagegen, not
    image_gen__imagegen.
  • No session rollout under ~/.codewith/sessions/** contains a tool spec
    named image_gen; the image_gen.imagegen strings there are agent notes.

The live 400 is an old-binary deployment gap, not a main regression. The
affected iapp-factory session runs codewith 0.1.67, the release immediately
before the fix. git show rust-v0.1.67:.../ext/image-generation/src/lib.rs
and .../spec_plan.rs both still have IMAGE_GEN_NAMESPACE = "image_gen";
rust-v0.1.68+ have "images". So the immediate operational fix is to roll
0.1.68+ (ride 0.1.78) onto that box.

Why this PR anyway (the missing guard)

5cc22bc71 fixed the value but left the invariant unguarded: nothing
structurally prevents a first-party / extension / code-mode / MCP tool from
regressing back under a reserved namespace, and the only protection was a
single unit-test assertion on that one tool. The dynamic-tool validator
(validate_dynamic_tools) only covers user-supplied tools, so the
first-party assembly path — the exact path that produced the original
image_gen.imagegen — was never guarded.

Fix

  • codex-tools: new reserved_namespaces module — single source of truth
    for RESERVED_RESPONSES_NAMESPACES, plus is_reserved_responses_namespace
    and is_forbidden_first_party_namespace. web is allowlisted via
    FIRST_PARTY_ALLOWED_RESERVED_NAMESPACES because standalone web search
    (web.run) deliberately advertises the built-in schema; every other reserved
    namespace (notably image_gen) is forbidden for first-party tools.
  • Dedupe the two existing copies of the reserved list (app-server
    validate_dynamic_tools, core validate_multi_agent_v2_tool_namespace) onto
    the shared constant so they can't drift.
  • core/src/tools/spec_plan.rs: namespace_spec_is_safe_for_wire runs on
    the assembled model-visible tool list in
    build_model_visible_specs_and_registry — the single chokepoint where
    first-party, extension, code-mode, and MCP namespace tools converge. A tool
    under a reserved, non-allowlisted namespace fails loudly (debug_assert)
    in debug/CI and is dropped with a warning in release, so a mis-namespaced
    tool degrades gracefully (one tool missing) instead of the API rejecting the
    whole turn.

Tests (added)

  • standalone_imagegen_never_uses_reserved_image_gen_namespace_on_responses_wire
    — assembled Responses-wire turn exposes images.imagegen and never
    image_gen.imagegen, and no assembled namespace tool carries a forbidden
    reserved name.
  • namespace_guard_fails_loud_on_reserved_image_gen_namespace (#[should_panic])
    and namespace_guard_allows_non_reserved_and_allowlisted_namespaces.
  • reserved_namespaces unit tests (image_gen reserved+forbidden; web
    reserved+allowed; list sorted/unique; allowlist ⊆ reserved).

Blacksmith gate (16 vCPU warm box)

  • just test/nextest codex-core (+codex-tools): my 3 new tests pass.
    75 failures are pre-existing sandbox/MCP/approval/shell-snapshot
    environment tests — an origin/main baseline on the same box produced an
    identical 75-failure set (0 regressions, 0 fixed).
  • cargo fmt --all -- --config imports_granularity=Item --check: clean.
  • cargo clippy --tests --all-targets -p codex-tools -p codex-core -p codex-app-server:
    7 warnings, all in pre-existing files (cli/Cargo.toml,
    code-mode/src/runtime/mod.rs, core/src/unified_exec/process_manager_tests.rs,
    proc-macro-error2 dep) — none in changed files.
  • No *.md changed → prettier N/A.

Regression context: #5cc22bc71. Do not merge/tag — rides 0.1.78.

…namespace

The standalone image tool once shipped under the Responses-API-reserved
`image_gen` namespace (wire name `image_gen.imagegen`), which the model
rejects with a 400 that fails the whole turn. Commit 5cc22bc renamed the
tool to the non-reserved `images` namespace, but left the invariant
unguarded: nothing prevents a first-party / extension / code-mode tool from
regressing back under `image_gen` (or another reserved built-in namespace),
and the only protection was a single unit-test assertion on that one tool.

Add a durable, centralized guard:

- codex-tools: new `reserved_namespaces` module — single source of truth for
  `RESERVED_RESPONSES_NAMESPACES`, plus `is_reserved_responses_namespace` and
  `is_forbidden_first_party_namespace`. `web` is allowlisted via
  `FIRST_PARTY_ALLOWED_RESERVED_NAMESPACES` (standalone web search advertises
  the built-in `web.run` schema on purpose).
- Dedupe the two existing copies of the reserved list (app-server
  `validate_dynamic_tools`, core `validate_multi_agent_v2_tool_namespace`)
  onto the shared constant so they can't drift.
- core spec_plan: `namespace_spec_is_safe_for_wire` runs on the assembled
  model-visible tool list. A first-party namespace tool under a reserved,
  non-allowlisted namespace fails loudly (debug_assert) in debug/CI and is
  dropped with a warning in release, so a mis-namespaced tool degrades
  gracefully instead of the API rejecting the entire turn.

Tests: assembled Responses-wire turn exposes images.imagegen and never
image_gen.imagegen; guard allows images/web and fails loud on image_gen;
reserved_namespaces unit tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant