feat(agent): pass function-tool strict through serialization - #99
Conversation
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Original prompt from dennis.jeong
|
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
cortex review —
|
There was a problem hiding this comment.
⚠️ APPROVE withheld by policy — PR author @devin-ai-integration[bot] is not a member of OpenRouterTeam (association: CONTRIBUTOR). Review posted as COMMENT; a maintainer must approve out-of-band.
cortex panel verdict: comment — details in the consolidated review comment.
LukasParke
left a comment
There was a problem hiding this comment.
Adversarial review of the strict pass-through. The core change is correct — strict flows through all six tool() config shapes, the serializer's ?? null preserves the prior default, and the built-in task tool's hardcoded strict: null remains intentional. Three gaps found (confirmed with maintainer):
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Downgrade notice: Perry's maintainer app is not authorized to post APPROVE verdicts on OpenRouterTeam. This review is posted as COMMENT with the reviewer identity. A human reviewer should approve this PR — the assessment below supports approval.
Perry's Review
Verdict: ✅ LGTM
Risk: 🟢 Low
Estimated impact: 🟩 Low — callers who don't set strict see no change; those who do get an additive feature. Worst case: strict doesn't work for a specific provider dialect, surfaced as a provider error.
PR: OpenRouterTeam/typescript-agent#99 — feat(agent): pass function-tool strict through serialization
All five prior review threads are resolved, CI is green (lint, typecheck, unit-tests, e2e-tests, structural-gate), and the implementation is complete and consistent across every tool kind.
What changed. convertToolsToAPIFormat hardcoded strict: null on every function tool, silently dropping a caller's strict: true (DEV-786). This PR adds strict?: boolean | null to BaseToolFunction and every tool() config shape, copies it onto the built tool's function in all factory branches, and serializes it via tool.function.strict ?? null — preserving the existing default while passing through true and false.
Coverage of all tool kinds verified:
- Regular tools — spread syntax in
tool.ts(line 742) - Generator tools — explicit
if (config.strict !== undefined)(line 703) - Manual tools — same pattern (line 645)
- HITL tools — same pattern (line 586)
- Unified
runtools —assignCommonToolFields(line 846) - Agent tools —
optionalFieldsloop inagentToolBuilder(line 337)
Design decision on preflight. An earlier revision added schema preflight validation for strict tools; commit 25df967 removed it in favor of faithful passthrough. This is the correct call — the SDK forwards the caller's Zod schema unchanged and lets the provider validate its own strict dialect, propagating errors unchanged. The README and JSDoc guide users toward .nullable() over .optional() for strict tools.
Test coverage. Five new tests pin strict: true (regular, unified run, agent), strict: false (not coerced to null — guards the ?? vs || distinction), and the null default. One inline suggestion below asks for a test covering the generator/manual/HITL copy path.
Risk assessment
| Dimension | Severity | Risk | Reasoning |
|---|---|---|---|
| Implementation risk | 🟩 | Low | Additive change — optional field + null ?? null default. All seven tool-config types and all six factory copy paths covered. |
| Premise risk | 🟩 | Low | The hardcoded strict: null bug is real and verified in the diff; passthrough with ?? null is the correct fix. |
| Estimated impact | 🟩 | Low | Callers who don't set strict see no change; those who do get an additive feature. Worst case: strict doesn't work for a specific provider dialect, surfaced as a provider error. |
| Risk Factor | Severity | Risk | Reasoning |
|---|---|---|---|
| Reversibility | 🟩 | Low | Fully reversible — removing the field or reverting to null restores prior behavior. |
| Detectability | 🟩 | Low | Provider 400 errors on strict schemas are immediately visible. |
| Blast radius | 🟩 | Low | Only affects tool-call argument generation for callers who opt into strict: true. |
| Data integrity | None | No persisted state is touched. | |
| Financial exposure | None | No billing or payment paths. | |
| Security and privacy exposure | None | No credentials, auth, or tenant isolation involved. | |
| Propagation | 🟩 | Low | Serialized tools flow into API requests only; no downstream records or decisions absorb the change. |
| Availability | None | The change cannot affect whether anything serves. | |
| Recovery cost | 🟩 | Low | A revert or field removal fully recovers. |
| Time to correct | 🟩 | Low | Any issue is a one-commit fix in the serializer or factory. |
Summary
Fixes DEV-786:
convertToolsToAPIFormathardcodedstrict: nullon every function tool, silently dropping a caller'sstrict: true— so providers never schema-constrained tool-call argument generation (OpenAI structured-outputs style) on the root model loop.strict?: boolean | nulladded toBaseToolFunctionand to everytool()config shape (regular, generator, manual, HITL, shared-context, unifiedrun); the factory copies it onto the built tool'sfunction.const apiTool: APITool = { type: 'function', name: tool.function.name, description: tool.function.description || null, - strict: null, + strict: tool.function.strict ?? null, parameters: convertZodToJsonSchema(tool.function.inputSchema), };Unit tests pin both
strict: truepass-through and thenulldefault; changeset included (minor).Note on the other DEV-786 bun-patch absorption items: the HITL
toolCallthreading intobuildExecuteCtxand theinvokeOnResponseReceivedfix already exist onmain(0.8.0), so this PR only adds thestrictgap. Threadingt.function.strictin openrouter-web'sbuildCallModelInputis a follow-up there after the SDK bump.API example
Link to Devin session: https://openrouter.devinenterprise.com/sessions/347b0632ce814dcebd12a6e5a70de122
Strict schema behavior
The SDK forwards the caller-generated schema unchanged and propagates provider validation errors. OpenAI-style strict mode requires every object property to be listed in
required; use Zod.nullable()for conceptually optional values because.optional()allows the key to be omitted. The option is supported by regular, unifiedrun, andtool.agent()builders.