refactor(providers): type NativeTool.inputSchema so provider drift becomes a compile error - #3309
Conversation
…o converters NativeTool.inputSchema was Record<string, unknown> documented as canonical JSON Schema, but only a flat object of string / string-enum / boolean properties with a `required` list was ever supported. Both providers re-derived that subset by hand and threw their own copy of the same error, so a field kind added on one side and missed on the other loaded under one provider and threw at spawn time under the other. The subset now lives in NativeToolProperty / NativeToolInputSchema, and each converter maps it with an exhaustive switch whose `never` default turns a new field kind into a compile error in both converters. The runtime schema throws are gone because the type makes them unrepresentable. A single conformance test drives both converters from one shared fixture and asserts they accept and reject the same value inputs.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe PR replaces free-form native-tool JSON schemas with a typed input schema. Claude and Pi convert the shared shape into Zod and TypeBox schemas. A conformance test verifies matching validation and emitted schema details. ChangesNative tool schema
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Refactor · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The schema conversion change preserves the supported native-tool behavior across Claude and Pi, with shared conformance coverage for validation and emitted schemas. No actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 5 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review report — #3309 (round 3, light — CI recheck)VerdictReady. Action: none. The branch head has not moved since the prior cursor Accepted contractSource: supplied work order (
Settled design (also part of the contract): discriminated union on Acceptance criteria carried by the work order: a new kind without a converter case fails Explicit boundaries (plan "Out of scope"): no provider-specific behavior in Reviewed head SHA
(PR #3309, base FindingsNo open Critical, Important, or Suggestion findings. No rejected findings this round. Prior findingsThe prior report's review-coverage section is authoritative for the concerns the accepted review
R1 verification (re-checked this round)
The delta is empty and touches no production path, so R1's status is unchanged and no new finding DiscoveriesAccepted discoveries: 1 (adjacent, non-blocking) — "Windows CI times out on unrelated
If you are an agent reading this: open discoveries.md and surface each discovery to your human. Review coverageContinuation mode. One continuation reviewer (this report) read the prior The delta adds no user-facing surface — it is empty — so no gated-off lens was re-evaluated. Evidence this review could not obtain: the Windows CI leg for this head remains red on an |
The conformance test only asserted parse success for the Claude converter, and Zod descriptions never affect parsing, so a dropped `.describe()` would have stayed green while manage_run's model-visible parameter documentation disappeared. Read the emitted JSON Schema back through `z.toJSONSchema` and assert the descriptions, matching the structural check the Pi branch already had.
1 participant
Problem and outcome
NativeTool.inputSchemawasRecord<string, unknown>, documented as "canonical JSON Schema", but only a flat object ofstring, string-enum, andbooleanproperties plusrequiredwas ever supported. That subset lived nowhere in a type — it was re-derived by two hand-written walkers, one per provider, each with its own copy of the sameunsupported type/ empty-enumthrows. A field kind added on one side and not the other produces aNativeToolthat loads under Claude and throws under Pi at spawn time, and no test catches it (issue #3303).type-checkin each converter that lacks a case for it, so the two providers cannot drift silently. Both converters map one typed contract, and one conformance test drives both from the same fixture.manage_run's parameter surface is unchanged on both providers, and Pi still builds enums withStringEnumfor Google/Vertex compatibility (PR fix(pi): emit StringEnum tool params so manage_run works on Vertex #3299).types.ts; no otherNativeToolproducer or consumer changes.Review guidance
@archon/providers/typesis an exported boundary.packages/providers/src/types.ts:528—NativeToolProperty,NativeToolInputSchema, anddefineNativeToolInputSchemaare the load-bearing change; everything else maps to them.types.ts→claude/native-tools.tsandcommunity/pi/native-tools.ts(exhaustiveswitchonkind) →native-tools-conformance.test.ts→core/orchestrator/manage-run-tool.ts(the only producer).packages/providers/package.jsonswaps the two deleted test files for the new conformance file intestGroups.manage-run-tool.tsis a mechanical retype: same eight keys, samerequired: ['action'], descriptions verbatim.Solution
NativeToolPropertyis a discriminated union on an explicitkind(string/enumwith a non-empty[string, ...string[]]tuple /boolean), andNativeToolInputSchemais a flatpropertiesmap plus arequiredkey list.defineNativeToolInputSchemaconstrainsrequiredentries tokeyof P & stringand returns the erased interface, so a misspelled required key is a compile error whileNativeToolstays non-generic. A generic interface would be invariant inPand break therequestOptions.nativeToolsassignment inpackages/core/src/orchestrator/orchestrator-agent.ts.Each converter replaces its JSON-Schema walker with an exhaustive
switchonkindwhosedefaultis the repo's existingconst unreachable: neveridiom. That is the mechanism behind the acceptance criterion: adding a fourth kind producesTS2322at bothneversites. The old object guard, per-kind throw, empty-enum throw, andisStringhelper are deleted, because the type makes each of those states unrepresentable.Rejected alternatives: discriminating on JSON-Schema
type(plain strings and string-enums both carrytype: 'string'); keeping a runtime guard for non-TypeScript callers (there is no non-TypeScript producer — the sole producer is@archon/core, in-process and never deserialized); extracting one provider-neutral walker (the only provider-specific content is the mapping, and the compiler already enforces parity per converter).Behavior change
NativeToolwith an unsupported field kind compiled and reached the provider, which threw at spawn time.bun run type-checkat the producer.native tool schema: unsupported type ...(orenum ... must be non-empty strings) inside the provider at spawn.native tool schema: unhandled field kind ...backstop instead.Architecture
Changed seams
@archon/providers/types → claude converterRecord<string, unknown>becomesNativeToolInputSchema; walker replaced by exhaustiveswitchpackages/providers/src/claude/native-tools.ts:18@archon/providers/types → pi converterStringEnumretained for enum propertiespackages/providers/src/community/pi/native-tools.ts:7@archon/core → @archon/providers/typesINPUT_SCHEMAbuilt withdefineNativeToolInputSchema; keys,required, and descriptions unchangedpackages/core/src/orchestrator/manage-run-tool.ts:82@archon/providers/types(exported subpath)NativeTool.inputSchemanarrows fromRecord<string, unknown>toNativeToolInputSchemapackages/providers/src/types.ts:574Validation
bun --filter @archon/providers type-check— exit 0 — the typed contract and both converters compile.bun test packages/providers/src/native-tools-conformance.test.ts— 4 pass, 0 fail — one fixture accepted and rejected identically by the Zod and Ajv validators; Pi still emits a JSON-Schema string enum; Claude still emits per-property descriptions; both builders accept the typed schema.bun --filter @archon/core type-check,bun run type-check(workspace) — exit 0 — the retyped producer compiles.bun run lint,bun run test,bun run validate— exit 0 — full suite and aggregate gate.| { kind: 'number'; description?: string }toNativeToolPropertyproducedTS2322at bothconst unreachable: never = propsites; the change was reverted and not committed.Delivery considerations
NativeTool.inputSchemais an exported contract subpath; an out-of-repo producer must build the typed shape withdefineNativeToolInputSchema. The in-repo producer is the only one and is updated here.packages/providers/src/types.ts:552,manage-run-tool.ts:82native tool schema: unhandled field kind ...instead of silently mis-converting.claude/native-tools.ts:28,pi/native-tools.ts:18cfb18f11eand8dbf2d8faLinks
Summary by CodeRabbit
New Features
Bug Fixes