refactor(providers): extract shared output schema builders into _schema.py by hertznsk · Pull Request #311 · microsoft/conductor

hertznsk · 2026-07-16T21:08:28Z

Summary

Four providers (Claude, Copilot, Hermes, Claude Agent SDK) contained near-duplicate logic for building provider output schemas. This PR extracts the shared logic into a new private module src/conductor/providers/_schema.py; each provider keeps a thin wrapper preserving its exact API contract, error types, and messages.

What changed

Shared Builder Core (src/conductor/providers/_schema.py): Implements build_json_schema_field/properties, build_prompt_schema_field/properties, build_hermes_legacy_prompt_schema, and custom exception SchemaDepthError.
Claude Wiring: Integrated the shared schema builder for Claude tool definitions.
Hermes Wiring: Wired the Hermes provider to use the new shared legacy prompt schema builder.
Copilot Wiring: Integrated the shared schema builder for Copilot prompt schema generation.
Claude Agent SDK Wiring: Wired Claude Agent SDK output format generation through the shared schema builder.
Golden Regression Tests: Added tests/test_providers/test_output_schema.py containing 9 golden tests.

Behavior preservation

Golden Tests: A suite of 9 tests in tests/test_providers/test_output_schema.py pins full wrapper outputs against literals captured from the pre-refactor code to ensure byte-for-byte correctness and prevent regressions.
Hermes Legacy Quirks: Preserved specific legacy quirks of the Hermes provider (description fallback, omitting the required field inside array-item objects, collapsed array-of-array items) in a dedicated legacy builder, including exact depth counting for array object items (test_build_prompt_schema_array_item_depth_parity).

Explicitly out of scope

No tool_choice changes.
No description fallback removal.
No Hermes recursion fix.
No additionalProperties: false additions.
No OutputField schema extensions.
No validate_output changes.
No public API, workflow YAML schema, or provider capabilities changes.
No new dependencies introduced.

Test plan

All tests run via make test: 4146 passed (1 pre-existing failure in test_large_create_tool_call_does_not_truncate caused by a local Copilot SDK authorization/enterprise-policy error, which also fails on the main branch).
Type checks and lint run via make check: clean execution with only 3 pre-existing ty warnings.
Examples validated via make validate-examples: all validated successfully.

…builder

…ed builder

Jason Robert (jrob5756)

Nice extraction overall, and the golden tests actually pin real pre-refactor output (I checked). Two things I'd want fixed before this merges: the depth check on Hermes array items now fires unconditionally, which tightens the depth limit for non-object items and breaks the zero-behavior-change goal, and there are a few leftover methods in claude.py and copilot.py that nothing calls anymore. Smaller doc nits below.

Jason Robert (jrob5756) · 2026-07-17T21:55:23Z

+ Raises:
+ SchemaDepthError: When the depth limit is exceeded.
+ """
+ _check_depth(depth, max_depth)


This check now runs for every array item, but the pre-refactor Hermes code only checked depth when it actually recursed into object properties. Array items of type array or a scalar never hit a depth check before.
I reproduced the regression: a schema with 10 nested objects ending in an array-of-strings field succeeds on main but raises ValidationError("Schema nesting depth exceeds maximum of 10 levels") here. That's a real behavior change for valid schemas right at the boundary.
Suggested change
_check_depth(depth, max_depth)
iffield.type=="object"andfield.properties:
_check_depth(depth, max_depth)
Move the check inside the object branch below so it only fires when recursing, same as before.

Fixed in 7a6d1f5 — moved _check_depth inside the object branch per your suggestion, so it only fires when recursing, exactly matching pre-refactor semantics. Verified against main: your repro (10 nested objects ending in array-of-strings) now passes, and 11 object levels still raises. Added a regression test test_build_prompt_schema_non_object_array_item_at_boundary_accepted pinning this boundary alongside the existing array-item parity test.

Jason Robert (jrob5756) · 2026-07-17T21:55:23Z

- return properties
+ ) from exc

 def _build_single_field_schema(self, field: OutputField, depth: int = 0) -> dict[str, Any]:


Nothing calls this anymore. _build_json_schema_properties delegates fully to build_json_schema_properties in _schema.py now, which handles array items internally through build_json_schema_field instead of coming back through this method. Checked src/ and tests/, no references outside the definition. Safe to delete.

Deleted in e1cc0cd, together with the now-unused build_json_schema_field import. Confirmed no references left in src/ or tests/.

Jason Robert (jrob5756) · 2026-07-17T21:55:23Z

- }
+ ) from exc

 def _build_prompt_field_schema(


_build_prompt_field_schema and _build_prompt_item_schema (line 1569) are both unreachable now. _build_prompt_schema calls build_prompt_schema_properties from _schema.py directly, which recurses on its own instead of going through these wrappers. The docstrings claim tests call them directly, but none do. Delete both, or if you want to keep them as an extension point, add tests and fix the docstring.

Deleted both in e1cc0cd. The docstrings claiming tests called them directly were indeed wrong — nothing referenced them, so removal was the right call over keeping them as an extension point.

Jason Robert (jrob5756) · 2026-07-17T21:55:23Z

- return schema
+ Kept as a thin wrapper for tests that call it directly.
+ """
+ from conductor.providers._schema import build_prompt_schema_field


This duplicates the module-level import of build_prompt_schema_properties/SchemaDepthError from the same module at line 26 (and it's repeated again at line 1574). No circular-import issue here, might as well hoist build_prompt_schema_field into that existing import.

Resolved in e1cc0cd — both local imports went away with the methods themselves, since nothing else used build_prompt_schema_field here.

Jason Robert (jrob5756) · 2026-07-17T21:55:23Z

+ A JSON-Schema fragment dictionary.
+
+ Raises:
+ _SchemaDepthError: When the depth limit is exceeded.


The Raises: section names _SchemaDepthError with a leading underscore, but the class is just SchemaDepthError (defined at line 16, correctly referenced at lines 39 and 272). Same typo at lines 97, 135, 188, and 225.

Fixed in 7a6d1f5 — all five occurrences now read SchemaDepthError (lines 62, 97, 135, 188, 225).

Jason Robert (jrob5756) · 2026-07-17T21:55:23Z

+ This matches the legacy Hermes provider behavior: descriptions fall back
+ to ``"The {field_name} field"`` at the top level, but array items do not
+ receive a fallback description. Unlike the generic prompt builder, object
+ items inside arrays are emitted with ``properties`` but no ``required``


This says array-of-array items collapse "without further recursion or description," but _build_hermes_legacy_item_schema keeps the description when the item has one (line 277). Only the recursion into nested items actually gets dropped. Worth tightening the wording so it matches what's actually pinned.

Tightened in 7a6d1f5 — both docstrings now say the collapse drops recursion but keeps an explicit item description, matching what's pinned.

Jason Robert (jrob5756) · 2026-07-17T21:55:23Z

+"""Golden regression tests for provider output-schema wrappers.
+
+These tests pin the exact full-wrapper output of each provider's schema builder
+against pre-refactor literals captured in Task 0. Any behavioral change in the


This points at "Task 0" and a .omo/scripts/capture_schema_baseline.py script, neither of which exists in this repo or its history. I regenerated the baselines from main myself and they check out, so the values are fine, there's just no way for anyone else to reproduce them from what's actually checked in. Worth either committing the capture script somewhere or rewording this to explain how the values were verified instead of pointing at a script that isn't there.

Reworded in e1cc0cd — the module docstring no longer references "Task 0", and the schema comment no longer points at .omo/scripts/capture_schema_baseline.py. The values themselves stand as pinned literals verified against main (you independently regenerated them too); the docstring now states that directly instead of pointing at a script that isn't checked in.

Jason Robert (jrob5756) · 2026-07-17T21:55:23Z

+ def test_build_prompt_schema_depth_limit_enforced(self) -> None:
+ """Excessively nested schemas raise ValidationError with the pinned message.
+
+ This is a targeted regression guard for PR-2a: the Copilot provider


"PR-2a" reads like an internal task label from planning rather than anything tied to this repo's PR numbers, so it won't mean much to anyone reading this later. The rest of the docstring already explains the invariant fine without it.

Removed in e1cc0cd — "PR-2a" was indeed an internal planning label, the docstring now stands on the invariant alone.

…rray items The shared _build_hermes_legacy_item_schema checked depth for every array item, but the pre-refactor Hermes builder only checked depth when recursing into object properties. A schema with 10 nested objects ending in an array-of-strings (or array-of-arrays) field passed on main but raised ValidationError here. Move the depth check inside the object-item branch so it fires only on recursion, matching legacy semantics exactly. Adds a regression test pinning the boundary: 10 nested objects with a non-object array leaf is accepted, 11 object levels still raises. Also fixes the _SchemaDepthError docstring typo (the class is SchemaDepthError) and tightens the array-of-array collapse wording: recursion is dropped, but an explicit item description is kept. Addresses PR microsoft#311 review comments by jrob5756.

…nces Delete ClaudeProvider._build_single_field_schema and CopilotProvider ._build_prompt_field_schema/_build_prompt_item_schema: after the shared builder wiring nothing calls them (checked src/ and tests/). This also removes the duplicated local imports of build_prompt_schema_field inside the two Copilot methods and the now-unused build_json_schema_field import in claude.py. Reword test docstrings that pointed at non-existent artifacts: the "Task 0" baseline reference and the .omo/scripts/capture_schema_baseline.py comment in test_output_schema.py, and the internal "PR-2a" label in test_copilot.py. Addresses PR microsoft#311 review comments by jrob5756.

hertznsk · 2026-07-18T00:03:13Z

All review comments addressed in 7a6d1f5 (Hermes depth boundary + docstring fixes) and e1cc0cd (dead code removal + stale references). Full provider suite: 800 passed; make check clean (3 pre-existing ty warnings, unchanged from main).

One open question for Jason Robert (@jrob5756): the golden tests in tests/test_providers/test_output_schema.py were essential to prove zero behavior change for this refactor, and they just paid off again catching the Hermes depth boundary. But going forward they may add maintenance burden — they pin byte-for-byte serialized output, so any intentional schema change (e.g. adding additionalProperties: false, removing the description fallback) would require mechanically rewriting the literals with noisy diffs. Do you think they should stay as-is once this PR lands, or should they eventually be degraded to narrower structural assertions (keeping targeted quirk tests like the Hermes depth/parity ones), or even removed after the emit-output refactor series is complete? Curious about your take on the right long-term shape.

Codecov Comments Bot (codecov-commenter) · 2026-07-20T12:04:52Z

Codecov Report

❌ Patch coverage is 97.67442% with 2 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@d52d883). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/conductor/providers/claude_agent_sdk.py	77.77%	2 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #311 +/- ##
=======================================
Coverage ? 89.80% =======================================
Files ? 73 Lines ? 13059 Branches ? 0 =======================================
Hits ? 11728 Misses ? 1331 Partials ? 0

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Jason Robert (jrob5756)

LGTM. Approved!

hertznsk added 6 commits July 16, 2026 23:01

refactor(providers): add shared output schema builder core
10dd24b

refactor(providers): wire Claude tool schema through shared builder
d50d9b0

refactor(providers): wire Hermes prompt schema through shared legacy …
c4a7df1
…builder

refactor(providers): wire Copilot prompt schema through shared builder
852b043

refactor(providers): wire Claude Agent SDK output format through shar…
bf63d9b
…ed builder

test(providers): add golden regression tests for shared schema builder
a231a88

hertznsk mentioned this pull request Jul 17, 2026
Idea: evaluate Pydantic AI as a shared inner agent runtime for direct OpenAI and Anthropic providers #315
Closed
18 tasks

Jason Robert (jrob5756) reviewed Jul 17, 2026
View reviewed changes

hertznsk added 2 commits July 18, 2026 03:01

Jason Robert (jrob5756) approved these changes Jul 20, 2026
View reviewed changes

Jason Robert (jrob5756) merged commit 5585b5c into microsoft:mainJul 20, 2026
10 checks passed

hertznsk mentioned this pull request Jul 20, 2026
fix(providers): align Copilot and Hermes prompt schemas on the shared recursive builder #317
Merged

Jason Robert (jrob5756) mentioned this pull request Jul 20, 2026
chore(release): cut 0.1.23 #320
Merged

hertznsk mentioned this pull request Jul 26, 2026
fix(executor): validate nested array item output schemas recursively #337
Merged

	_check_depth(depth, max_depth)
	iffield.type=="object"andfield.properties:
	_check_depth(depth, max_depth)

Uh oh!

Conversation

hertznsk commented Jul 16, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What changed

Behavior preservation

Explicitly out of scope

Test plan

Uh oh!

Jason Robert (jrob5756) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hertznsk commented Jul 18, 2026

Uh oh!

Codecov Comments Bot (codecov-commenter) commented Jul 20, 2026

Codecov Report

Uh oh!

Jason Robert (jrob5756) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hertznsk commented Jul 16, 2026•
edited
Loading