Enable Codex structured output in Rig Codex adapter - #283
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — commenting with a few targeted improvements.
📋 Key Themes & Highlights
Key Themes
- Missing edge-case tests: the
null/undefinedfinalResponse path and the string-branch-with-schema path are untested. - Implicit contract broadening:
outputSchemais now forwarded to all engines viaAgentAskOptions, not just Codex — worth documenting so future engine authors know they can safely ignore it.
Positive Highlights
- ✅ Clean integration: the structured-output path follows the existing
askOptionspattern without a bespoke bypass. - ✅ Good test coverage of the happy path (schema forwarding + structured response serialisation).
- ✅ Minimal footprint — the change is surgical and doesn't touch unrelated code.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 32.2 AIC · ⌖ 4.31 AIC · ⊞ 6.3K
Comment /matt to run again
| if (typeof turn.finalResponse === "string") { | ||
| return turn.finalResponse; | ||
| } | ||
| return JSON.stringify(turn.finalResponse); |
There was a problem hiding this comment.
[/tdd] Missing edge case: finalResponse could be null — JSON.stringify(null) returns "null", which propagates silently as a valid response but will fail downstream schema validation.
💡 Suggested test + guard
mocks.run.mockResolvedValueOnce({ finalResponse: null });
await expect(runtimeAgent.ask('x', {})).rejects.toThrow(); // or resolves.toBe('')Consider throwing if finalResponse is nullish when an outputSchema was requested, or explicitly document that null stringifies to "null".
| context.signal ? { signal: context.signal } : undefined, | ||
| { | ||
| ...(context.signal ? { signal: context.signal } : {}), | ||
| outputSchema: toJsonSchema(context.outputSchema), |
There was a problem hiding this comment.
[/codebase-design] outputSchema is now always passed (even when undefined from toJsonSchema) to every engine, not just Codex. Engines that don't use it will silently ignore it, but this couples the generic AgentAskOptions contract to a Codex-specific feature.
💡 Context
The Copilot engine (line 564 in rig.ts) discards askOptions.outputSchema today because it only reads signal. That is safe now, but any future engine that spreads askOptions directly into its SDK call could accidentally forward an unknown field. Consider documenting that engines are free to ignore outputSchema, or narrowing the propagation to engines that declare support.
| mocks.run.mockResolvedValueOnce({ finalResponse: { summary: "done" } }); | ||
|
|
||
| await expect(runtimeAgent.ask("summarize", { outputSchema })).resolves.toBe(JSON.stringify({ summary: "done" })); | ||
|
|
There was a problem hiding this comment.
[/tdd] The test only covers the structured-object path. Add a test confirming that a plain string finalResponse is returned as-is (no double-serialisation) when outputSchema is also provided — this is the boundary between the two branches in the production code.
💡 Suggested test
it('returns string finalResponse unchanged when outputSchema is set', async () => {
const runtimeAgent = await codexEngine()({ model: 'gpt-5-codex' });
mocks.run.mockResolvedValueOnce({ finalResponse: '{"summary":"done"}' });
await expect(
runtimeAgent.ask('summarize', { outputSchema: { type: 'object' } })
).resolves.toBe('{"summary":"done"}');
});
Rig’s Codex adapter was treating Codex responses as plain text, so it did not leverage Codex’s native schema-constrained output path. This change routes Rig’s output schema into Codex per turn and normalizes structured responses back into Rig’s existing parse/validate flow.
Runtime schema propagation
AgentAskOptionswithoutputSchema.toJsonSchema(context.outputSchema)intoruntimeAgent.ask(...)so engines can consume structured-output metadata.Codex adapter integration
codexEngineto forwardoutputSchematothread.run(prompt, { ... }).turn.finalResponsetoJSON.stringify(...).Behavioral coverage
finalResponsenormalization.outputSchema.