Uh oh!
There was an error while loading. Please reload this page.
fix(providers): align Copilot and Hermes prompt schemas on the shared recursive builder - #317
Conversation
… recursive builder Known-red until the follow-up test commit: legacy fallback/golden tests in test_copilot.py, test_hermes.py, test_output_schema.py -- rewritten immediately after.
…rompt-schema parity Regenerate Copilot and Hermes EXPECTED_* literals using the shared recursive builder. Rewrite legacy Hermes pinning tests to match new semantics: no description fallback, required in array<object> items, recursive array<array> items, and exact shared depth counting. Update the Copilot nested-schema assertion and add a parse-recovery test proving the embedded schema contains no fallback descriptions. Claude and SDK literals remain byte-identical.
…er to satisfy ruff format
…ocstrings - Restore TestClaudeOutputSchemaGolden (accidentally removed in b9071a9) and its imports. - Update stale golden test docstrings to match the no-fallback, fully recursive behavior. - Fix 'Claude Agent Agent SDK' typo.
…removed parameter name
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
Thanks for contributing once again! Everything looks great, nothing really blocking. But a few things to look at. The comments below are mostly cleanup you exposed by finishing the job so thoroughly. If you take a look I can merge right away!
| @@ -107,26 +107,18 @@ def build_json_schema_properties( | |||
| def build_prompt_schema_field( | |||
There was a problem hiding this comment.
Now that field_name/description_fallback are gone, build_prompt_schema_field/build_prompt_schema_properties are identical to build_json_schema_field/build_json_schema_properties above (lines 45-104) - same depth check, same shape, same recursion. Only two call sites are left (copilot.py:1658, hermes.py:652). Given the PR title is literally about aligning on one shared builder, it seems worth finishing the job: drop this pair and point both callers at build_json_schema_properties instead. Not asking for it in this PR necessarily, just flagging since the diff makes the duplication obvious.
There was a problem hiding this comment.
Good catch - they're identical right now, but deliberately so, and I'd like to keep the pair. I'm planning a follow-up PR that adds additionalProperties: false to the json_schema flavor only (Claude's emit_output tool schema and claude-agent-sdk's output_format), where the API actually enforces it. The prompt flavor is serialized as text into Copilot/Hermes prompts, where the extra key would just add noise with no enforcement value - so the two flavors will intentionally diverge, and collapsing them now would mean re-splitting them in that follow-up. The build_prompt_schema_* names also document the intent at the two call sites. Happy to revisit the structure in that follow-up if you'd prefer something like one parameterized private builder with two thin public wrappers.
| _build_prompt_schema({"matrix": nested_array_chain(_MAX_SCHEMA_DEPTH + 1)}) | ||
| class TestHermesSharedPromptSchema: |
There was a problem hiding this comment.
This new class and TestHermesBuildPromptSchema above (line 839) cover almost the same ground - same _chain_schema helper copy-pasted, and 4-5 of the 6 tests are near-identical bodies (no-fallback description, array required, array-of-arrays recursion, the depth-8/9 boundary). The class docstring even calls out that it's "complementing" the other suite, so this looks intentional rather than an oversight, but it means two places to update if this behavior ever changes again. Worth folding into one class and keeping only the two tests that are actually unique (test_build_prompt_schema_exceeds_max_depth's message/suggestion pin, and test_nested_array_object_descriptions_preserved).
There was a problem hiding this comment.
Done - folded into TestHermesBuildPromptSchema. Kept the two unique tests you flagged (the exact message/suggestion pin already lived in the surviving class, and test_nested_array_object_descriptions_preserved moved over); the near-identical bodies and the duplicated _chain_schema helper are gone.
| """Negative guard proving the golden tests are sensitive to behavioral changes.""" | ||
| def test_mutated_copilot_literal_does_not_match(self) -> None: | ||
| """If the builder output is mutated (e.g., a description fallback is changed), the | ||
| golden assertion must fail so regressions are caught.""" | ||
| """If an explicit description in the builder output is mutated, the golden assertion | ||
| must fail so regressions are caught.""" | ||
| provider = _make_copilot_provider() | ||
| actual = _serialize(provider._build_prompt_schema(rich_schema)) | ||
| mutated = EXPECTED_COPILOT_RICH_SCHEMA.replace( | ||
| '"description": "The number_scalar field"', | ||
| '"description": "The number_scalar field (mutated)"', | ||
| '"description": "A string scalar field with a description"', | ||
| '"description": "A string scalar field with a description (mutated)"', | ||
| ) | ||
| assert actual != mutated |
There was a problem hiding this comment.
mutated here is built by string-replacing the expected literal, not by touching the real builder or its output. So actual != mutated will always be true, no matter what _build_prompt_schema actually does - it's checking that two different strings are different, not that the golden test would catch a regression. As written it doesn't add coverage beyond what TestCopilotOutputSchemaGolden already gives you.
If the goal is a real mutation guard, it'd need to run the builder against a schema that's deliberately different from rich_schema and confirm that output no longer matches the golden string, e.g.:
| """Negative guard proving the golden tests are sensitive to behavioral changes.""" | |
| deftest_mutated_copilot_literal_does_not_match(self) ->None: | |
| """Ifthebuilderoutputismutated (e.g., adescriptionfallbackischanged), the | |
| goldenassertionmustfailsoregressionsarecaught.""" | |
| """Ifanexplicitdescriptioninthebuilderoutputismutated, thegoldenassertion | |
| mustfailsoregressionsarecaught.""" | |
| provider=_make_copilot_provider() | |
| actual=_serialize(provider._build_prompt_schema(rich_schema)) | |
| mutated=EXPECTED_COPILOT_RICH_SCHEMA.replace( | |
| '"description": "The number_scalar field"', | |
| '"description": "The number_scalar field (mutated)"', | |
| '"description": "A string scalar field with a description"', | |
| '"description": "A string scalar field with a description (mutated)"', | |
| ) | |
| assertactual!=mutated | |
| deftest_golden_assertion_detects_missing_description(self) ->None: | |
| """Aschemawhoseonlydifferencefrom`rich_schema`isadroppeddescription | |
| mustnotsatisfytherich-schemagoldenassertion.""" | |
| provider=_make_copilot_provider() | |
| schema_without_description= { | |
| **rich_schema, | |
| "string_scalar": OutputField(type="string"), | |
| } | |
| actual=_serialize(provider._build_prompt_schema(schema_without_description)) | |
| assertactual!=EXPECTED_COPILOT_RICH_SCHEMA |
Or just drop the class if that's more effort than it's worth for what it buys you.
There was a problem hiding this comment.
Rewritten as you suggested - the guard now runs the builder against a schema whose only difference from rich_schema is a dropped description and asserts the output no longer matches EXPECTED_COPILOT_RICH_SCHEMA. Much better as a real negative test, thanks for the snippet.
| inside array-item objects, collapsed array-of-array items) and the depth boundary. | ||
| The wrapper delegates to the shared recursive builder in | ||
| conductor.providers._schema and converts SchemaDepthError into ValidationError | ||
| with the exact legacy message and suggestion. These tests pin the shared |
There was a problem hiding this comment.
"legacy" is stale here - there's no legacy-specific message anymore, hermes.py:648 was already updated in this same PR to drop the word. Small thing, but it'll read as if a separate legacy format still exists.
| withtheexactlegacymessageandsuggestion. Thesetestspintheshared | |
| withtheexactmessageandsuggestion. Thesetestspintheshared |
There was a problem hiding this comment.
Fixed - dropped "legacy" here and in test_build_prompt_schema_exceeds_max_depth below; both now say "shared".
| @@ -752,7 +743,8 @@ def test_build_prompt_schema_rich_matches_baseline(self) -> None: | |||
| def test_build_prompt_schema_missing_descriptions_matches_baseline(self) -> None: | |||
| """Copilot prompt schema wrapper must be byte-for-byte identical to the pre-refactor | |||
There was a problem hiding this comment.
This still claims byte-for-byte identity with the "pre-refactor baseline," but the whole point of this test is that the output changed from that baseline (the fallback descriptions are gone). Might be clearer as something like "must match the current prompt schema output for a schema without descriptions - fields without an explicit description emit no description key," so a future reader doesn't assume this is a frozen behavior-preservation check.
There was a problem hiding this comment.
Reworded - the docstring now describes the current behavior (no description key for fields without an explicit description) instead of claiming identity with the old baseline. Also caught the same stale claim in the module docstring and the rich-schema test.
| @@ -763,13 +755,14 @@ class TestHermesOutputSchemaGolden: | |||
| def test_build_prompt_schema_rich_matches_baseline(self) -> None: | |||
| """Hermes prompt schema wrapper must be byte-for-byte identical to the pre-refactor | |||
There was a problem hiding this comment.
Same issue as the Copilot one above (line 745) - this and the missing-descriptions test right below it (line 763) both say "identical to the pre-refactor baseline" while describing exactly the behavior this PR changed (recursive array-of-arrays, no fallback description). Worth rewording both to describe current behavior rather than parity with the old implementation.
There was a problem hiding this comment.
Both reworded to describe current behavior rather than parity with the old implementation.
hertznsk
commented
Jul 21, 2026
Thank you for your thorough review! I will make corrections tomorrow or the day after tomorrow. |
…ale golden-test wording - Merge TestHermesSharedPromptSchema into TestHermesBuildPromptSchema, keeping only the unique tests (nested description preservation) - Replace the vacuous golden mutation guard with a real negative test that runs the builder against a schema with a dropped description - Reword Copilot/Hermes golden-test docstrings that claimed parity with the pre-refactor baseline while pinning behavior this PR changed
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## main #317 +/- ##
=======================================
Coverage ? 89.99% =======================================
Files ? 75 Lines ? 13281 Branches ? 0 =======================================
Hits ? 11952 Misses ? 1329 Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
LGTM. Approved!
Summary
Builds on #311, which extracted the shared output-schema builders into
_schema.py. This PR flips the two prompt-injection providers (Copilot and Hermes) onto the single shared recursive builder and deletes the legacy Hermes-only code path — one builder, one behavior, no dead code.Two parity fixes:
Copilot and Hermes prompt schemas no longer fall back to
The {field} fieldfor fields without an explicitdescription. The shared schema builder already omitted the fallback for Claude; Copilot and Hermes now use the same behavior, so an absentdescriptionproduces a JSON Schema object without a synthetic description. This keeps prompts shorter and consistent across providers.Hermes now uses the shared recursive prompt-schema builder (the same one Copilot uses). Previously Hermes used its own hand-rolled builder that only recursed through object properties and collapsed nested arrays. It now delegates to the shared recursive builder, which correctly handles scalar types, arrays of arrays, and arbitrary nesting:
array<object>items include bothpropertiesANDrequired,array<array>items recurse, and explicit descriptions survive at every nesting level.Deleted:
build_hermes_legacy_prompt_schema,_build_hermes_legacy_item_schema, and thedescription_fallback/field_nameparameters from the shared prompt-schema builders. Claude and Claude Agent SDK are untouched (already aligned, JSON-Schema flavor).Migration note
Hermes prompt schemas now use the shared recursive builder. The shared depth limit now applies to every array item: scalar/array-item nesting that legacy Hermes never depth-checked at all (it only checked object-item properties, and collapsed array-of-arrays) may now raise
ValidationError: Schema nesting depth exceeds maximum of 10 levelsat depth >10; reduce nesting or split the schema. If model adherence degrades on fields without descriptions, add an explicitdescription.Test plan
requiredinarray<object>items, recursivearray<array>, exact depth-10/11 boundary counting), then the implementation, then golden literals regenerated from actual builder output.TestGoldenMutationGuardretargeted to mutate an explicit description (its old mutation target disappeared with the fallback).make check✅make validate-examples✅ full suite green except one pre-existing environmental failure (test_large_create_tool_call_does_not_truncate, requires a Copilot enterprise entitlement — untouched by this PR).