Add orchestration DTO snapshots (#73) - #96
Merged
leynos merged 24 commits intoMay 28, 2026
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
OverviewThis PR completes issue Key ChangesSnapshot Coverage for Orchestration DTOsAdded canonical snapshot tests in
Validation and Invariant TestsAdded strict DTO validation tests in new files
Usage Rollup RefinementRefactored
Orchestration Result BuildersIntroduced
Property-Based TestingAdded Hypothesis property tests across multiple files:
Integration TestingEnhanced
Test Harness Updates
Design Document References
ValidationAll validation commands passed sequentially (as noted in the ExecPlan):
WalkthroughAdd deterministic ShowNotesEntry/ShowNotesResult helpers and serialisation snapshots; update ExecutionPlan serialisation so generate_show_notes requires ChangesShow notes snapshot test coverage
Sequence Diagram(s)sequenceDiagram
participant Test
participant Orchestrator as StructuredPlanningOrchestrator
participant ToolExec as ShowNotesToolExecutor
participant ShowNotesLLM as ShowNotesLLM
participant Result as GenerationOrchestrationResult
Test->>Orchestrator: orchestrate(request)
Orchestrator->>ToolExec: request show-notes generation (planned action)
ToolExec->>ShowNotesLLM: invoke model with prompt containing template_structure (model=gpt-4o-mini)
ShowNotesLLM-->>ToolExec: return structured show-notes JSON
ToolExec-->>Orchestrator: return ShowNotesResult (entries, usage, tei_locator)
Orchestrator-->>Result: aggregate planner and action usage, embed show_notes_result
Result-->>Test: return final orchestration result
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 19 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (19 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideAdds syrupy-based snapshot tests for orchestration DTO serialisation, including new coverage for show-notes DTOs and updated execution plan snapshots to track required_inputs and nested orchestration results. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
marked this pull request as ready for review
May 13, 2026 23:23
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. tests/test_generation_orchestration_snapshots.py Comment on lines +159 to +194 def test_generation_orchestration_result_with_show_notes_snapshot(
snapshot: SnapshotAssertion,
) -> None:
planned = PlannedAction(
action_id="a1",
action_kind=ActionKind.GENERATE_SHOW_NOTES,
rationale="Generate listener-facing notes from canonical TEI.",
model_tier=ModelTier.EXECUTION,
required_inputs=("script_tei_xml",),
)
plan = ExecutionPlan(
plan_version="1",
selected_planning_model="gpt-4.1",
selected_execution_model="gpt-4o-mini",
steps=(planned,),
)
show_notes = make_show_notes_result(
entries=(make_show_notes_entry(),),
)
action_done = ActionExecutionResult(
action_id="a1",
action_kind=ActionKind.GENERATE_SHOW_NOTES,
model_tier=ModelTier.EXECUTION,
model="gpt-4o-mini",
summary="Generated one show-notes entry.",
usage=show_notes.usage,
show_notes_result=show_notes,
)
result = GenerationOrchestrationResult(
plan=plan,
action_results=(action_done,),
planner_usage=LLMUsage(input_tokens=12, output_tokens=8, total_tokens=20),
total_usage=LLMUsage(input_tokens=52, output_tokens=33, total_tokens=85),
)
serialised = dataclasses.asdict(result)
assert serialised == snapshot❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lodyai
Bot
force-pushed
the
issue-73-syrupy-snapshot-tests-for-orchestration-outputs
branch
from
May 19, 2026 20:41
24e56ca to
9860436
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_generation_orchestration_snapshots.py`:
- Around line 214-219: When total_usage is None, compute it from planner_usage
and action_usage instead of using a fixed LLMUsage; sum the input_tokens,
output_tokens, and total_tokens from planner_usage and action_usage (treat any
missing planner_usage or action_usage as zero for each token field) and assign
total_usage = LLMUsage(input_tokens=sum_input, output_tokens=sum_output,
total_tokens=sum_total); if both planner_usage and action_usage are absent keep
the existing hardcoded fallback LLMUsage(11,22,33). Ensure you update the block
that currently sets total_usage to reference planner_usage and action_usage by
name so callers who override only one usage still produce correct totals.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: abe035cf-f64e-419e-8724-42a57d21e50a
📒 Files selected for processing (3)
tests/__snapshots__/test_generation_orchestration_snapshots.ambrtests/test_generation_orchestration_snapshots.pytests/test_orchestration_orchestrator.py
Owner
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Excess Number of Function Argumentstests/test_generation_orchestration_snapshots.py: _make_orchestration_result What lead to degradation?_make_orchestration_result has 7 arguments, max arguments = 4 Why does this problem occur?Functions with many arguments indicate either a) low cohesion where the function has too many responsibilities, or b) a missing abstraction that encapsulates those arguments. How to fix it?Start by investigating the responsibilities of the function. Make sure it doesn't do too many things, in which case it should be split into smaller and more cohesive functions. Consider the refactoring INTRODUCE PARAMETER OBJECT to encapsulate arguments that refer to the same logical concept. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_generation_orchestration_snapshots.py`:
- Around line 339-349: Modify the
test_generation_orchestration_fixture_totals_partial_usage_overrides test to
parametrize input cases so it covers both action-only and planner-only partial
overrides: use pytest.mark.parametrize on the test to pass either
action_usage=_make_orchestration_result(action_usage=LLMUsage(...)) or
planner_usage=_make_orchestration_result(planner_usage=LLMUsage(...))
(referencing the test name
test_generation_orchestration_fixture_totals_partial_usage_overrides, helper
_make_orchestration_result, and LLMUsage) and assert result.total_usage equals
the supplied LLMUsage in each param case; this ensures both partial branches
(action vs planner) are exercised without changing other test logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c38111a1-bafd-41a6-a19b-b3cd4e382b15
📒 Files selected for processing (1)
tests/test_generation_orchestration_snapshots.py
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/test_generation_orchestration_snapshots.py (2)
193-202:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace the seven-argument fixture helper with a parameter object.
Unblock the CodeScene gate at Line 193 by collapsing the helper signature into a single spec object and removing the
too-many-argumentssuppression.Refactor diff
+@dataclasses.dataclass(frozen=True, slots=True) +class _OrchestrationResultSpec: + rationale: str = "test" + required_inputs: tuple[str, ...] = () + action_summary: str = "test" + action_usage: LLMUsage | None = None + show_notes_result: ShowNotesResult | None = None + planner_usage: LLMUsage | None = None + total_usage: LLMUsage | None = None + -# pylint: disable-next=too-many-arguments def _make_orchestration_result( - *, - rationale: str = "test", - required_inputs: tuple[str, ...] = (), - action_summary: str = "test", - action_usage: LLMUsage | None = None, - show_notes_result: ShowNotesResult | None = None, - planner_usage: LLMUsage | None = None, - total_usage: LLMUsage | None = None, + spec: _OrchestrationResultSpec | None = None, ) -> GenerationOrchestrationResult: + spec = _OrchestrationResultSpec() if spec is None else spec🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_generation_orchestration_snapshots.py` around lines 193 - 202, The helper function _make_orchestration_result currently accepts seven separate parameters (rationale, required_inputs, action_summary, action_usage, show_notes_result, planner_usage, total_usage); replace this signature with a single parameter object (e.g., a dataclass or dict named OrchestrationResultSpec or spec) and update the body to read properties from that spec, then adjust all callers in tests to pass a single spec instance and remove the `too-many-arguments` suppression; keep the original defaults by providing default values on the spec fields and preserve the return type GenerationOrchestrationResult in the function (still named _make_orchestration_result) so other references remain valid.
323-330:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winParametrize the partial-override test to cover planner-only input.
Exercise the missing planner-only branch at Line 323 and keep the test aligned with its own “whichever usage values callers supply” contract.
Test diff
-def test_generation_orchestration_fixture_totals_partial_usage_overrides() -> None: +@pytest.mark.parametrize( + ("kwargs", "expected"), + [ + ( + {"action_usage": LLMUsage(input_tokens=5, output_tokens=7, total_tokens=12)}, + LLMUsage(input_tokens=6, output_tokens=9, total_tokens=15), + ), + ( + {"planner_usage": LLMUsage(input_tokens=2, output_tokens=3, total_tokens=5)}, + LLMUsage(input_tokens=12, output_tokens=23, total_tokens=35), + ), + ], +) +def test_generation_orchestration_fixture_totals_partial_usage_overrides( + kwargs: dict[str, LLMUsage], + expected: LLMUsage, +) -> None: """Verify total usage is derived from whichever usage values callers supply.""" - result = _make_orchestration_result( - action_usage=LLMUsage(input_tokens=5, output_tokens=7, total_tokens=12), - ) - - assert result.total_usage == LLMUsage(6, 9, 15) + result = _make_orchestration_result(**kwargs) + assert result.total_usage == expectedAs per coding guidelines "
**/test_*.py: Use pytest idioms: prefer fixtures over setup/teardown methods, parametrize broadly, avoid unnecessary mocks".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_generation_orchestration_snapshots.py` around lines 323 - 330, The test test_generation_orchestration_fixture_totals_partial_usage_overrides only exercises an action-only override; parametrize it to also exercise a planner-only override by adding a case that calls _make_orchestration_result(planner_usage=LLMUsage(...)) and assert result.total_usage equals the expected sum (matching the test's “whichever usage values callers supply” behavior); keep other existing cases (action-only, both) in the param list and use pytest.mark.parametrize to feed (planner_usage, action_usage, expected_total) into the test so the planner-only branch in _make_orchestration_result is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@tests/test_generation_orchestration_snapshots.py`:
- Around line 193-202: The helper function _make_orchestration_result currently
accepts seven separate parameters (rationale, required_inputs, action_summary,
action_usage, show_notes_result, planner_usage, total_usage); replace this
signature with a single parameter object (e.g., a dataclass or dict named
OrchestrationResultSpec or spec) and update the body to read properties from
that spec, then adjust all callers in tests to pass a single spec instance and
remove the `too-many-arguments` suppression; keep the original defaults by
providing default values on the spec fields and preserve the return type
GenerationOrchestrationResult in the function (still named
_make_orchestration_result) so other references remain valid.
- Around line 323-330: The test
test_generation_orchestration_fixture_totals_partial_usage_overrides only
exercises an action-only override; parametrize it to also exercise a
planner-only override by adding a case that calls
_make_orchestration_result(planner_usage=LLMUsage(...)) and assert
result.total_usage equals the expected sum (matching the test's “whichever usage
values callers supply” behavior); keep other existing cases (action-only, both)
in the param list and use pytest.mark.parametrize to feed (planner_usage,
action_usage, expected_total) into the test so the planner-only branch in
_make_orchestration_result is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 071b738e-6c1b-4c43-a7fc-b1c9c5f8c872
📒 Files selected for processing (1)
tests/test_generation_orchestration_snapshots.py
Owner
|
@coderabbitai Have the following now been resolved?
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 14971
Both suggestions are fully addressed. ✅ |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Owner
|
@coderabbitai review |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_generation_orchestration_snapshots.py`:
- Line 8: The file contains a file-level lint suppression "# pylint:
disable=too-many-lines" and is over the allowed size; remove the file-level
suppression and split the large test module into smaller modules/fixtures so
each file is under 400 logical lines: extract helper functions/classes (the
surrounding test helpers referenced near line 8) into one or more new modules
(e.g., tests/helpers/*.py or fixtures modules) and convert shared setup into
pytest fixtures, update imports in
tests/test_generation_orchestration_snapshots.py to import those
helpers/fixtures, ensure no file-level "# pylint: disable=..." remains and
instead apply targeted, minimal inline suppressions only where absolutely
necessary, and run tests/linter to confirm everything still passes (this change
also applies to the full range covered by the original file 1-528).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bb69ff23-66ce-416a-87a8-c3c534aaf148
📒 Files selected for processing (6)
tests/__snapshots__/test_generation_orchestration_snapshots.ambrtests/_orchestration_property_support.pytests/test_generation_orchestration_snapshots.pytests/test_guest_bios_properties.pytests/test_orchestration_langgraph_properties.pytests/test_orchestration_orchestrator.py
Move snapshot DTO builders and planner-format helpers into a private test support module so the snapshot test module stays below the lint line limit. Remove the file-level `too-many-lines` suppression and keep the existing snapshot assertions unchanged.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/_generation_orchestration_snapshot_support.py`:
- Around line 21-202: Rename the module-level test helpers to be private by
prefixing their names with an underscore (e.g., UnusedLLMPort -> _UnusedLLMPort,
PlannedActionKwargs -> _PlannedActionKwargs, OrchestrationResultSpec ->
_OrchestrationResultSpec, make_show_notes_entry -> _make_show_notes_entry,
make_show_notes_result -> _make_show_notes_result, make_orchestration_result ->
_make_orchestration_result, valid_plan_payload -> _valid_plan_payload,
valid_plan_step -> _valid_plan_step, plan_payload_with_step_field ->
_plan_payload_with_step_field, plan_payload_without_step_field ->
_plan_payload_without_step_field, capture_plan_format_error ->
_capture_plan_format_error) and update the import site in
tests/test_generation_orchestration_snapshots.py to import those underscored
names; keep docstrings and signatures unchanged. Ensure all internal references
within this file use the new underscored names so references like
StructuredGenerationPlanner._parse_plan and the test import continue to resolve.
- Around line 24-31: The generate static method currently raises RuntimeError
for the impossible await path; change it to raise AssertionError instead to
reflect a broken test-harness invariant. Update the raise in generate
(signature: async def generate(request: LLMRequest) -> LLMResponse) to raise
AssertionError(msg) using the same msg that references
StructuredGenerationPlanner.build_prompt so the error retains context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5f5eb640-3503-4381-9f5c-f74d6248db69
📒 Files selected for processing (2)
tests/_generation_orchestration_snapshot_support.pytests/test_generation_orchestration_snapshots.py
Prefix the extracted snapshot support helpers with underscores so the module signals that they are local test implementation details. Raise `AssertionError` from the unreachable LLM shim path to report a broken test harness invariant with the existing contextual message.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Expand the support module docstring to describe the fixture families, planner payload builders, and their relationship to the orchestration snapshot tests.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Cover snapshot fixture validation boundaries with Hypothesis-generated invalid timestamps, whitespace-only values, and non-string DTO inputs. Keep the snapshot module under the line limit by placing these checks in a focused validation property test module.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_generation_orchestration_validation_properties.py (1)
67-125: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winParametrize duplicated PlannedAction rejection tests.
Collapse the duplicated test bodies into parametrized cases so the boundary intent stays explicit and maintenance cost drops.
Proposed refactor
+import pytest @@ -@given(value=_WHITESPACE_STRINGS) -@settings(max_examples=25) -def test_planned_action_rejects_arbitrary_whitespace_rationale( - value: str, -) -> None: - """Verify rationale validation rejects arbitrary whitespace strings.""" - with pytest.raises(ValueError, match="rationale must be a non-empty string"): - PlannedAction( - action_id="a1", - action_kind=ActionKind.GENERATE_SHOW_NOTES, - rationale=value, - model_tier=ModelTier.EXECUTION, - required_inputs=("script_tei_xml",), - ) - - -@given(value=_WHITESPACE_STRINGS) -@settings(max_examples=25) -def test_planned_action_rejects_arbitrary_whitespace_required_inputs( - value: str, -) -> None: - """Verify required input validation rejects arbitrary whitespace strings.""" - with pytest.raises(ValueError, match="required_inputs must be a non-empty string"): - PlannedAction( - action_id="a1", - action_kind=ActionKind.GENERATE_SHOW_NOTES, - rationale="test", - model_tier=ModelTier.EXECUTION, - required_inputs=(value,), - ) +@pytest.mark.parametrize( + ("field_name", "error_match"), + ( + ("rationale", "rationale must be a non-empty string"), + ("required_inputs", "required_inputs must be a non-empty string"), + ), +) +@given(value=_WHITESPACE_STRINGS) +@settings(max_examples=25) +def test_planned_action_rejects_arbitrary_whitespace_fields( + field_name: str, + error_match: str, + value: str, +) -> None: + """Verify PlannedAction rejects arbitrary whitespace-only field values.""" + kwargs = { + "action_id": "a1", + "action_kind": ActionKind.GENERATE_SHOW_NOTES, + "rationale": "test", + "model_tier": ModelTier.EXECUTION, + "required_inputs": ("script_tei_xml",), + } + if field_name == "rationale": + kwargs["rationale"] = value + else: + kwargs["required_inputs"] = (value,) + with pytest.raises(ValueError, match=error_match): + PlannedAction(**kwargs) @@ -@given(value=_INVALID_DTO_FIELD_TYPES) -@settings(max_examples=25) -def test_planned_action_rejects_invalid_rationale_types(value: object) -> None: - """Verify rationale validation rejects arbitrary non-string values.""" - with pytest.raises(ValueError, match="rationale must be a non-empty string"): - PlannedAction( - action_id="a1", - action_kind=ActionKind.GENERATE_SHOW_NOTES, - rationale=typ.cast("str", value), - model_tier=ModelTier.EXECUTION, - required_inputs=("script_tei_xml",), - ) - - -@given(value=_INVALID_DTO_FIELD_TYPES) -@settings(max_examples=25) -def test_planned_action_rejects_invalid_required_input_types(value: object) -> None: - """Verify required input validation rejects arbitrary non-string items.""" - with pytest.raises(ValueError, match="required_inputs must be a non-empty string"): - PlannedAction( - action_id="a1", - action_kind=ActionKind.GENERATE_SHOW_NOTES, - rationale="test", - model_tier=ModelTier.EXECUTION, - required_inputs=typ.cast("tuple[str, ...]", (value,)), - ) +@pytest.mark.parametrize( + ("field_name", "error_match"), + ( + ("rationale", "rationale must be a non-empty string"), + ("required_inputs", "required_inputs must be a non-empty string"), + ), +) +@given(value=_INVALID_DTO_FIELD_TYPES) +@settings(max_examples=25) +def test_planned_action_rejects_invalid_field_types( + field_name: str, + error_match: str, + value: object, +) -> None: + """Verify PlannedAction rejects arbitrary invalid field types.""" + kwargs = { + "action_id": "a1", + "action_kind": ActionKind.GENERATE_SHOW_NOTES, + "rationale": "test", + "model_tier": ModelTier.EXECUTION, + "required_inputs": ("script_tei_xml",), + } + if field_name == "rationale": + kwargs["rationale"] = typ.cast("str", value) + else: + kwargs["required_inputs"] = typ.cast("tuple[str, ...]", (value,)) + with pytest.raises(ValueError, match=error_match): + PlannedAction(**kwargs)As per coding guidelines, "
**/*.py: Replace duplicate tests with@pytest.mark.parametrize" and "**/test_*.py: ... parametrize broadly".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_generation_orchestration_validation_properties.py` around lines 67 - 125, Collapse duplicated property-specific tests into parametrized variants: replace the two whitespace tests (test_planned_action_rejects_arbitrary_whitespace_rationale and test_planned_action_rejects_arbitrary_whitespace_required_inputs) with a single `@pytest.mark.parametrize` test (e.g., test_planned_action_rejects_arbitrary_whitespace_fields) that accepts field_name and error_match, builds a kwargs dict for PlannedAction and injects value into either "rationale" or "required_inputs"; do the same for the invalid-type pair (test_planned_action_rejects_invalid_rationale_types and test_planned_action_rejects_invalid_required_input_types) into one parametrized test (e.g., test_planned_action_rejects_invalid_field_types) that casts value appropriately and asserts the same ValueError matches, keeping references to PlannedAction, "rationale", and "required_inputs" to locate the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/test_generation_orchestration_validation_properties.py`:
- Around line 67-125: Collapse duplicated property-specific tests into
parametrized variants: replace the two whitespace tests
(test_planned_action_rejects_arbitrary_whitespace_rationale and
test_planned_action_rejects_arbitrary_whitespace_required_inputs) with a single
`@pytest.mark.parametrize` test (e.g.,
test_planned_action_rejects_arbitrary_whitespace_fields) that accepts field_name
and error_match, builds a kwargs dict for PlannedAction and injects value into
either "rationale" or "required_inputs"; do the same for the invalid-type pair
(test_planned_action_rejects_invalid_rationale_types and
test_planned_action_rejects_invalid_required_input_types) into one parametrized
test (e.g., test_planned_action_rejects_invalid_field_types) that casts value
appropriately and asserts the same ValueError matches, keeping references to
PlannedAction, "rationale", and "required_inputs" to locate the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 51c3403e-7227-4ff0-8ddd-4d66f8b9ebba
📒 Files selected for processing (1)
tests/test_generation_orchestration_validation_properties.py
Collapse duplicated `PlannedAction` rejection checks into parametrized Hypothesis tests. Keep the boundary cases explicit while reducing the repeated DTO construction in the validation property module.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
|
@coderabbitai Have the following now been resolved? tests/test_generation_orchestration_validation_properties.py (1)
Proposed refactor+import pytest
@@
-@given(value=_WHITESPACE_STRINGS)
-@settings(max_examples=25)
-def test_planned_action_rejects_arbitrary_whitespace_rationale(
- value: str,
-) -> None:
- """Verify rationale validation rejects arbitrary whitespace strings."""
- with pytest.raises(ValueError, match="rationale must be a non-empty string"):
- PlannedAction(
- action_id="a1",
- action_kind=ActionKind.GENERATE_SHOW_NOTES,
- rationale=value,
- model_tier=ModelTier.EXECUTION,
- required_inputs=("script_tei_xml",),
- )
-
-
-@given(value=_WHITESPACE_STRINGS)
-@settings(max_examples=25)
-def test_planned_action_rejects_arbitrary_whitespace_required_inputs(
- value: str,
-) -> None:
- """Verify required input validation rejects arbitrary whitespace strings."""
- with pytest.raises(ValueError, match="required_inputs must be a non-empty string"):
- PlannedAction(
- action_id="a1",
- action_kind=ActionKind.GENERATE_SHOW_NOTES,
- rationale="test",
- model_tier=ModelTier.EXECUTION,
- required_inputs=(value,),
- )
+@pytest.mark.parametrize(
+ ("field_name", "error_match"),
+ (
+ ("rationale", "rationale must be a non-empty string"),
+ ("required_inputs", "required_inputs must be a non-empty string"),
+ ),
+)
+@given(value=_WHITESPACE_STRINGS)
+@settings(max_examples=25)
+def test_planned_action_rejects_arbitrary_whitespace_fields(
+ field_name: str,
+ error_match: str,
+ value: str,
+) -> None:
+ """Verify PlannedAction rejects arbitrary whitespace-only field values."""
+ kwargs = {
+ "action_id": "a1",
+ "action_kind": ActionKind.GENERATE_SHOW_NOTES,
+ "rationale": "test",
+ "model_tier": ModelTier.EXECUTION,
+ "required_inputs": ("script_tei_xml",),
+ }
+ if field_name == "rationale":
+ kwargs["rationale"] = value
+ else:
+ kwargs["required_inputs"] = (value,)
+ with pytest.raises(ValueError, match=error_match):
+ PlannedAction(**kwargs)
@@
-@given(value=_INVALID_DTO_FIELD_TYPES)
-@settings(max_examples=25)
-def test_planned_action_rejects_invalid_rationale_types(value: object) -> None:
- """Verify rationale validation rejects arbitrary non-string values."""
- with pytest.raises(ValueError, match="rationale must be a non-empty string"):
- PlannedAction(
- action_id="a1",
- action_kind=ActionKind.GENERATE_SHOW_NOTES,
- rationale=typ.cast("str", value),
- model_tier=ModelTier.EXECUTION,
- required_inputs=("script_tei_xml",),
- )
-
-
-@given(value=_INVALID_DTO_FIELD_TYPES)
-@settings(max_examples=25)
-def test_planned_action_rejects_invalid_required_input_types(value: object) -> None:
- """Verify required input validation rejects arbitrary non-string items."""
- with pytest.raises(ValueError, match="required_inputs must be a non-empty string"):
- PlannedAction(
- action_id="a1",
- action_kind=ActionKind.GENERATE_SHOW_NOTES,
- rationale="test",
- model_tier=ModelTier.EXECUTION,
- required_inputs=typ.cast("tuple[str, ...]", (value,)),
- )
+@pytest.mark.parametrize(
+ ("field_name", "error_match"),
+ (
+ ("rationale", "rationale must be a non-empty string"),
+ ("required_inputs", "required_inputs must be a non-empty string"),
+ ),
+)
+@given(value=_INVALID_DTO_FIELD_TYPES)
+@settings(max_examples=25)
+def test_planned_action_rejects_invalid_field_types(
+ field_name: str,
+ error_match: str,
+ value: object,
+) -> None:
+ """Verify PlannedAction rejects arbitrary invalid field types."""
+ kwargs = {
+ "action_id": "a1",
+ "action_kind": ActionKind.GENERATE_SHOW_NOTES,
+ "rationale": "test",
+ "model_tier": ModelTier.EXECUTION,
+ "required_inputs": ("script_tei_xml",),
+ }
+ if field_name == "rationale":
+ kwargs["rationale"] = typ.cast("str", value)
+ else:
+ kwargs["required_inputs"] = typ.cast("tuple[str, ...]", (value,))
+ with pytest.raises(ValueError, match=error_match):
+ PlannedAction(**kwargs)As per coding guidelines, " 🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
fd 'test_generation_orchestration_validation_properties.py' --exec cat {}Length of output: 4470
The four original single-field test functions are gone. The implementation also goes one step further than the proposed refactor by annotating the Both items are resolved. ✅ |
Replace pre-constrained usage strategies with independent input, output, and total tuples so property tests can detect incorrect total rollups. Import the shared `_PlannedActionKwargs` TypedDict in validation property tests, and derive aggregated `total_tokens` from summed input and output counts in `_sum_usage` and snapshot fixtures. Co-authored-by: Cursor <cursoragent@cursor.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Document why aggregated total_tokens is derived from summed input and output counts, and emit structured warnings when provider-reported totals differ at the component or aggregate level. Co-authored-by: Cursor <cursoragent@cursor.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_orchestration_usage.py`:
- Around line 50-55: The test uses a bare assert comparing aggregate_fields to
the expected dict; change it to include a descriptive assertion message by
updating the assertion for aggregate_fields (the dict comparison) to use
Python's assert ... , "message" form — e.g. assert aggregate_fields == { ... },
"Aggregate fields mismatch: expected X but got Y" — so that failures show a
clear message; reference the existing variable name aggregate_fields and update
that specific assert statement accordingly.
- Line 21: Update the bare assertion that compares total_usage to LLMUsage in
tests/test_orchestration_usage.py to include a descriptive failure message;
specifically modify the assert using the left-hand expression total_usage and
expected LLMUsage(input_tokens=13, output_tokens=12, total_tokens=25) so it
becomes assert <comparison>, "<descriptive message indicating actual vs expected
total_usage or which part failed>" to improve test diagnostics.
- Line 48: The bare assertion in tests/test_orchestration_usage.py uses assert
all(event[0] == "warning" for event in logged_events) without a message; update
that assertion to include a descriptive failure message (e.g., "Expected all
logged events to be warnings, got: {logged_events}") so failures are
informative—modify the assertion in the test function that references
logged_events accordingly.
- Around line 79-80: Replace the two bare asserts in
tests/test_orchestration_usage.py with assertions that include descriptive
messages: for the first assert comparing total_usage to
LLMUsage(input_tokens=13, output_tokens=12, total_tokens=25) add a message like
"unexpected LLM token usage: {total_usage}" and for the second assert checking
logged_events is empty add a message like "expected no logged events but found
{logged_events}"; update the assertions that reference total_usage,
LLMUsage(...) and logged_events accordingly so failures show clear context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6c940578-44b0-4b4f-bb81-e9e738ec2c07
📒 Files selected for processing (2)
episodic/orchestration/_usage.pytests/test_orchestration_usage.py
Add descriptive failure messages to usage aggregation unit tests so mismatched totals, warning levels, and aggregate fields are easier to diagnose. Co-authored-by: Cursor <cursoragent@cursor.com>
leynos
deleted the
issue-73-syrupy-snapshot-tests-for-orchestration-outputs
branch
May 28, 2026 17:43
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch adds committed syrupy snapshots for orchestration DTO serialisation
so plan-version, action-kind, and nested show-notes output regressions are
visible in CI.
Closes #73.
Review walkthrough
Validation
pytest tests/test_generation_orchestration_snapshots.py --snapshot-update: passed viauv run; generated three snapshots and updated the canonical execution-plan snapshot.make check-fmt: passed.make test: passed, 480 passed and 3 skipped.make lint: passed.make typecheck: passed.Summary by Sourcery
Tests: