Skip to content

Let spawn_agent carry an output schema - #10

Closed
thedancingdeveloper wants to merge 3 commits into
fork/mainfrom
fork/wi-1130-output-schema
Closed

thedancingdeveloper wants to merge 3 commits into
fork/mainfrom
fork/wi-1130-output-schema

Conversation

@thedancingdeveloper

Copy link
Copy Markdown

Summary

  • Adds an optional output_schema argument to spawn_agent v1 and v2.
  • The value is copied into the child's first-turn TurnStartOptions::final_output_json_schema and is not re-validated here.
  • The tool schema advertises it as an unconstrained JSON value, and the v2 spec test checks that it carries no type.

WI-1130.

Test plan

  • cargo test -p codex-core --lib tools::handlers::multi_agents_spec::tests::spawn_agent_tool_v2_requires_task_name

@thedancingdeveloper thedancingdeveloper left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: no blockers (head 4f1781f)

This is the smallest-patch version WI-1130 asked for: +44 lines in 7 files.

  • output_schema is added to v1 and v2 SpawnAgentArgs and to both specs.
  • It's copied into SpawnAgentOptions.final_output_json_schema, then into the child's first TurnStartOptions (control/spawn.rs), which is the same path as exec --output-schema.

The untyped property is safe. The spawn tools are strict: false (multi_agents_spec.rs), and the MCP-only sanitize_json_schema doesn't touch built-in specs, so the model sees an unconstrained JSON value, not a string.

Non-blocking:

  • The only test listed is the v2 spec test. Add one plumbing test that a spawn with output_schema reaches the child's TurnStartOptions.final_output_json_schema, so a rebase can't silently drop the wiring.
  • Mention in the tool description that providers enforcing strict json_schema need strict-compatible schemas (additionalProperties: false, all properties required).

Merge condition: fork-ci run 37935237022 on this head is green (it includes cargo check --workspace, which catches struct-literal breakage).

@thedancingdeveloper thedancingdeveloper left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: blocker (head 4f1781f)

fork-ci run 37935237022 fails cargo check --workspace with E0559: HookHandlerConfig::Command has no field fail_closed (core/src/session/tests.rs:2000). That fail_closed: false line is #6's (WI-1129) and leaked into this WI-1130 branch, which is the 1-line core/src/session/tests.rs change in this diff. Against fork/main, where #6 isn't merged, it doesn't compile.

Fix: drop that line from #10 so it stands alone on fork/main, push, and let fork-ci go green. Keep #10 independent of #6; they'll merge in either order.

Everything else in my earlier review of this PR stands.

@thedancingdeveloper thedancingdeveloper left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: no blockers (head cdd2dc9)

The only change since 4f1781f removes the stray fail_closed: false that had leaked in from #6. #10 now stands alone on fork/main. My earlier no-blocker review of the WI-1130 content holds. Merge condition: fork-ci run 37937347118 on this head is green. Under the current fork-ci, which still uses -D warnings, pre-existing upstream lint debt in codex-core could fail it. If it does, merge #11 first and re-run.

@thedancingdeveloper

Copy link
Copy Markdown
Author

Epic lead: run 37937347118 failed in the clippy step on upstream lint debt (unused imports in core/tests/suite/openai_file_mcp.rs:47 and scenarios.rs:42), files #10 doesn't touch. That's exactly what #11 fixes. After #11 merges, re-run fork-ci on this head; no code change is needed. The review stands at cdd2dc9.

@thedancingdeveloper thedancingdeveloper left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: no blockers (head 02f541e)

This is a rebase onto fork/main 219664f (which includes #11). git range-diff against the reviewed cdd2dc9 shows both commits identical (=), so the earlier review stands. Merge condition: fork-ci green on this head, under the changed-files clippy rule.

@thedancingdeveloper

Copy link
Copy Markdown
Author

Epic lead: run 37947101661 aborts cargo test -p codex-core with fatal runtime error: stack overflow. That's fork-ci, not this PR: upstream runs tests with RUST_MIN_STACK: "8388608" (rust-ci.yml:158), and fork-ci doesn't set it. Fix requested from codex-fork-infra. Re-run this PR once it's on fork/main. The review at 02f541e stands.

@thedancingdeveloper thedancingdeveloper left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: no blockers (head a4fb68f)

This is a pure rebase onto fork/main feaf20e, which includes #15's RUST_MIN_STACK. git range-diff shows both commits identical (=) to the reviewed 02f541e, so the review stands. Merge condition: fork-ci green on this head. The codex-core stack overflow should now be gone.

@thedancingdeveloper
thedancingdeveloper force-pushed the fork/wi-1130-output-schema branch from a4fb68f to 559cc29 Compare October 9, 2026 19:10

@thedancingdeveloper thedancingdeveloper left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: no blockers (head 559cc29)

This is a rebase onto fork/main ef97503, which includes #17's workspace bin build. git range-diff against the previously reviewed head shows every commit identical (=): 2 commits. Merge condition: fork-ci green on this head.

thedancingdeveloper added a commit that referenced this pull request Oct 9, 2026
cargo test -p runs the app-server and core integration suites without the
serialization, retries and slow timeouts in codex-rs/.config/nextest.toml,
so #6 and #10 fail tests that are unrelated to their changes. Mirror
rust-ci-full-nextest-platform.yml: install bubblewrap, install cargo-nextest
0.9.103 from the same taiki-e/install-action pin, and run `cargo nextest run
-p` so that config applies. RUST_MIN_STACK and the workspace bin build stay.

The cli skip moves from a cargo --skip to a nextest filterset read from
.github/fork-ci-known-failures.txt. That file lists only tests shown to fail
on unmodified fork/main, each with the baseline run that proved it, and it
starts empty until that baseline exists.
@thedancingdeveloper
thedancingdeveloper force-pushed the fork/wi-1130-output-schema branch from 559cc29 to d802c6e Compare October 9, 2026 22:13
@thedancingdeveloper
thedancingdeveloper force-pushed the fork/wi-1130-output-schema branch from 9b56b8f to f59fc41 Compare October 10, 2026 04:27
An optional output_schema on spawn_agent v1 and v2 is copied into the
child's TurnStartOptions.final_output_json_schema. The tool schema
accepts any JSON value and the child is not re-validated here.
fail_closed belongs to the fail-closed hook change and is not on
fork/main, so this branch must build without it.
The collaboration namespace hash moved when spawn_agent gained an output schema. Parent tool hashes that include that namespace moved with it.
@thedancingdeveloper

Copy link
Copy Markdown
Author

Superseded: these commits were cherry-picked into #22 (fork features, merged to fork/main as c2d74d2) and ship in release fork-c2d74d2d9. Closing so the work isn't merged twice. The branch is kept.

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