Add orchestration property tests (#72) - #97
Merged
leynos merged 19 commits intoMay 25, 2026
Merged
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 (4)
Hypothesis Property-Based Tests for Episodic OrchestrationAdds comprehensive Hypothesis-based property tests, shared test scaffolding, and supporting refactors for the episodic/orchestration slice to address issue Test coverage added
Implementation and refactoring
Documentation and execplan
Review feedback, fixes and test additions
Test & CI status
Files of interest
ClosingCloses issue WalkthroughCentralise Hypothesis strategies/fakes and invalid-plan generators; add planner-format snapshots and property tests; implement checkpoint payload serialisation, suspend/resume and GenerationGraphState; add optional synchronous finish_callback to LangGraph; update tests to use shared support. ChangesOrchestration Property-Based Test Coverage
Sequence DiagramsequenceDiagram
participant Request as GenerationOrchestrationRequest
participant Planner as StructuredGenerationPlanner
participant Graph as LangGraph
participant Checkpoint as CheckpointPort
participant Executor as ToolExecutor
participant FinishCB as finish_callback
Request->>Planner: plan(request)
Planner->>Graph: provide PlannerResult
alt checkpoint_port provided (suspend)
Graph->>Checkpoint: save_or_reuse(checkpoint_payload)
Checkpoint-->>Graph: return SuspendedWorkflowResult
else direct execute path
Graph->>Executor: execute(planned_action)
Executor-->>Graph: return ActionExecutionResult
Graph->>FinishCB: invoke(orchestration_result)
FinishCB-->>Graph: returns or raises (exception logged)
end
Graph-->>Request: return GenerationOrchestrationResult or SuspendedWorkflowResult
Possibly related PRs
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Reviewer's GuideAdds property-based tests (Hypothesis) to validate orchestration behaviour across planning, execution, and LangGraph flows, including enum normalisation, model-tier boundaries, malformed planner payload handling, and event ordering/token aggregation invariants. 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 resolved.
This comment was marked as resolved.
Sorry, something went wrong.
leynos
marked this pull request as draft
May 14, 2026 00:03
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-72-hypothesis-property-based-tests-for-episodic-orchestration
branch
from
May 15, 2026 10:46
6a63526 to
af454cc
Compare
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 15, 2026 14:16
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Have the following now been resolved? tests/test_orchestration_properties.py (1)
Proposed fix-@given(model_tier=st.sampled_from([t for t in ModelTier if t != ModelTier.EXECUTION]))
-@settings(max_examples=len(ModelTier))
-@pytest.mark.asyncio
-async def test_planned_action_model_tier_rejection_for_all_non_execution_tiers(
- model_tier: ModelTier,
-) -> None:
+@pytest.mark.parametrize(
+ ("model_tier",),
+ [(tier,) for tier in ModelTier if tier is not ModelTier.EXECUTION],
+)
+@pytest.mark.asyncio
+async def test_planned_action_model_tier_rejection_for_all_non_execution_tiers(
+ model_tier: ModelTier,
+) -> None: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: Length of output: 1571
`@pytest.mark.parametrize`(
"model_tier",
[tier for tier in ModelTier if tier is not ModelTier.EXECUTION],
)
`@pytest.mark.asyncio`
async def test_planned_action_model_tier_rejection_for_all_non_execution_tiers(
model_tier: ModelTier,
) -> None:The |
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: 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 `@episodic/orchestration/langgraph.py`:
- Around line 639-641: The finish-callback invocation (finish_callback(state))
must be isolated so exceptions raised by the callback do not prevent returning
the already-computed result; wrap the finish_callback call in a try/except that
catches exceptions, logs or handles the error, and then continue to return
result unconditionally. Locate the finish_callback(state) call in the function
where result and state are computed and replace it with a guarded call that
preserves returning result even if finish_callback fails.
In `@tests/test_orchestration_properties.py`:
- Around line 12-15: The current oversized property-test module uses a
file-level suppression "pylint: disable=too-many-lines"; split it into smaller
focused test modules (e.g., config_model_tier_tests, planner_format_error_tests,
graph_invariants_tests), move shared domain-specific strategies/fakes/fixtures
into a shared helper (or conftest) so each new test module can reuse them,
ensure every new test file stays below the 400 logical-line limit, remove the
file-level "pylint: disable=too-many-lines" suppression, and update imports/test
discovery to reference the new module names so all tests still run.
🪄 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: dfe3323f-7c6b-4550-ac57-9b907c0cc535
📒 Files selected for processing (2)
episodic/orchestration/langgraph.pytests/test_orchestration_properties.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.
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_orchestration_properties.py`:
- Around line 471-477: The test uses Hypothesis sampling (st.sampled_from +
`@settings`(max_examples=10)) but filtering ModelTier to non-EXECUTION yields a
single value and produces duplicate runs; replace the Hypothesis decorators with
pytest.parametrization: remove `@given` and `@settings` and add
`@pytest.mark.parametrize`(("model_tier",), [(tier,) for tier in ModelTier if tier
is not ModelTier.EXECUTION]) above the test function
test_planned_action_model_tier_rejection_for_all_non_execution_tiers so the test
deterministically runs once per non-EXECUTION ModelTier value.
🪄 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: ce0ac05a-cc5b-4df4-bc5e-3849f2ffde02
📒 Files selected for processing (2)
tests/test_generation_orchestration_snapshots.pytests/test_orchestration_properties.py
Owner
Author
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. ❌ Failed checks (2 errors, 5 warnings)
|
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.
Owner
Author
|
@coderabbitai please create a new GitHub issue for the following:
|
4 tasks
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 46
It documents the problem, the proposed resolution (change the callback signature to |
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: 3
🤖 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 `@episodic/orchestration/langgraph.py`:
- Around line 633-637: Update the docstring for the finish_callback parameter in
langgraph.py to precisely state its runtime contract: the callback is invoked as
finish_callback(state) where state is a copied GenerationGraphState whose
orchestration_result field has been populated (not the original mutable state);
keep that callback exceptions are logged and propagated to the caller. Ensure
the text references the exact symbol names finish_callback,
GenerationGraphState, and orchestration_result so readers know which object is
copied and which field is set.
In `@tests/test_orchestration_graph_invariant_properties.py`:
- Around line 180-213: The test duplicates planner/tool setup; replace the
inline Construction of PropGraphPlanner and PropGraphToolExecutor with calls to
the existing helper functions _planner_result() and _tool_result(): locate where
planner is built using PropGraphPlanner(...) and where tool_executor is built
using PropGraphToolExecutor(...) and change them to reuse the returned
PlannerResult and ActionExecutionResult helpers by passing _planner_result()
into PropGraphPlanner (or directly assigning the helper result if it already
returns the planner) and _tool_result() into PropGraphToolExecutor (or assign
the helper result), removing the repeated literal fields so the test uses the
shared helpers instead of rebuilding equivalent objects.
In `@tests/test_orchestration_planner_format_properties.py`:
- Around line 37-39: Two module-level tests (including
test_planning_response_format_error_for_arbitrary_non_object_json and the other
test at lines 68-70) should be moved into a test class (e.g.,
TestStructuredGenerationPlannerFormatProperties) so related property tests are
grouped; wrap both functions as methods of that class, keep their names
unchanged and still prefixed with test_, and ensure any fixtures (like noise)
remain as method parameters and imports/async markers are preserved.
🪄 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: 0c823e2c-ebf3-4de8-972f-72be60a209de
📒 Files selected for processing (10)
docs/developers-guide.mddocs/execplans/2-4-1-structured-output-planning-and-tool-calling-execution.mddocs/roadmap.mdepisodic/orchestration/langgraph.pytests/_orchestration_property_support.pytests/test_orchestration_config_model_tier_properties.pytests/test_orchestration_graph_invariant_properties.pytests/test_orchestration_langgraph_properties.pytests/test_orchestration_planner_format_properties.pytests/test_orchestration_properties.py
Wrap the structured generation planner format property tests in a pytest collection class while preserving their decorators and test bodies.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Share graph construction and callback recording between the direct and suspend finish-callback invariant tests without changing their assertions.
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.
Adjust the extracted finish-callback helper to accept the already-derived correlation id instead of the full graph state. Keep the graph builder below the CodeScene function-size threshold while preserving the result-only callback contract.
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: 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 `@episodic/orchestration/langgraph.py`:
- Around line 635-639: The current return type tuple[typ.Any, str] for
_build_execute_node is too broad—define a precise callable type: add a union
type ExecuteNodeResult = dict[str, tuple[dto.ActionExecutionResult, ...]] |
dict[str, dto.SuspendedWorkflowResult] and an ExecuteNodeFn =
typ.Callable[[GenerationGraphState], typ.Awaitable[ExecuteNodeResult]] then
change _build_execute_node signature to return tuple[ExecuteNodeFn, str]; update
any internal references and imports (typ, GenerationGraphState, dto) so the
inner async node callables conform to ExecuteNodeFn and the function return
annotation uses the new types.
In `@tests/_orchestration_property_support.py`:
- Around line 174-179: The current usage_strategy constructs LLMUsage with
independent input_tokens, output_tokens, and total_tokens which can be
inconsistent; change usage_strategy to build total_tokens from the sampled
component counts so total_tokens = input_tokens + output_tokens (i.e., use
st.builds with a combining factory that accepts sampled input_tokens and
output_tokens and returns LLMUsage(input_tokens=..., output_tokens=...,
total_tokens=input+output)). Update the usage_strategy reference (LLMUsage and
usage_strategy) so generated examples are internally consistent.
🪄 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: bd0f4d79-e948-4f62-a419-557d3a5ccc6a
📒 Files selected for processing (13)
docs/developers-guide.mddocs/execplans/2-4-1-structured-output-planning-and-tool-calling-execution.mddocs/roadmap.mdepisodic/orchestration/_dto.pyepisodic/orchestration/langgraph.pytests/__snapshots__/test_generation_orchestration_snapshots.ambrtests/_orchestration_property_support.pytests/test_generation_orchestration_snapshots.pytests/test_orchestration_config_model_tier_properties.pytests/test_orchestration_graph_invariant_properties.pytests/test_orchestration_langgraph_properties.pytests/test_orchestration_planner_format_properties.pytests/test_orchestration_properties.py
Move graph state, checkpoint payload serialization, and suspend/resume logic out of `langgraph.py` into private orchestration modules. Keep the existing `langgraph` private helper names importable for tests and other callers while reducing the module below the CodeScene file-size limit.
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: 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 `@episodic/orchestration/_checkpoint_payload.py`:
- Around line 150-180: The current construction coerces malformed step entries
and required_inputs by casting steps and doing str(item); instead validate each
step is a mapping/object and enforce required_inputs as list[str] or raise
TypeError with context: inside the generator that builds dto.PlannedAction for
each element of steps, first assert the step is a dict/object (reject non-object
steps), use _required_string/_required_enum/_require_field as before but ensure
_require_field(..., "required_inputs") returns a list and then validate every
item is a str (do not call str(item)); if any check fails raise TypeError
including the field name and "plan step" context so callers can see which step
and field is invalid (update references: PlannedAction, _required_string,
_required_enum, _require_field, and the steps generator).
In `@episodic/orchestration/_graph_state.py`:
- Around line 14-15: Add a full NumPy-style docstring for the public dataclass
GenerationGraphState that opens with a one-line summary, followed by extended
description and structured sections: Parameters (listing and typing each
dataclass field and its semantics), Attributes (if different from Parameters),
Raises (if any invariants), Notes (behavioral/contract details for graph-node
authors), and Examples (minimal usage). Make sure to name the class
GenerationGraphState in the docstring, document each public field exactly as
declared in the dataclass with types and expected values/constraints, and
explain the orchestration semantics (initialize → plan → execute → finish) and
how nodes should read/modify the state.
🪄 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: dbdb694a-fa4a-47f4-b232-0a86c511f23b
📒 Files selected for processing (5)
episodic/orchestration/__init__.pyepisodic/orchestration/_checkpoint_payload.pyepisodic/orchestration/_checkpoint_resume.pyepisodic/orchestration/_graph_state.pyepisodic/orchestration/langgraph.py
Add a precise execute-node protocol, generate internally consistent LLM usage values in property tests, and reject malformed checkpoint plan step payloads instead of coercing them. Expand `GenerationGraphState` documentation with the graph traversal contract for node authors.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Correct the developer guide to describe `finish_callback` as a domain-result hook and document the public `GenerationGraphState` export separately. Add focused concurrent direct-path coverage so shared callback behaviour stays explicit, and document that shared mutable callbacks own their synchronisation.
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: 3
🤖 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 `@docs/developers-guide.md`:
- Around line 842-844: Update the callback-concurrency paragraph by replacing
British spellings "serialise" and "synchronisation" with the standardized -ize
forms "serialize" and "synchronization" to conform to the repo's en-GB-oxendict
(-ize) guideline; ensure the sentence reads "...The graph does not serialize
concurrent invocations of a shared callback; callbacks that mutate shared state
must provide their own synchronization."
In `@episodic/orchestration/langgraph.py`:
- Around line 226-228: Docstrings in episodic/orchestration/langgraph.py use
en-GB oxendict spellings like "serialise"/"synchronise"/"synchronisation" in the
callback contract; update those to the project's en-GB-oxendict-approved forms
"serialize"/"synchronize"/"synchronization" instead. Locate the callback
contract text in the docstring around the graph execution callback (mentions
"already-computed graph result" and "invoked synchronously in the graph
execution context") and perform the string substitutions there and in the other
occurrences noted (also around lines 311-314) so all instances of
serialise/synchronise/synchronisation are replaced with
serialize/synchronize/synchronization. Ensure only docstrings/comments are
changed and run tests/lint afterwards.
In `@tests/test_orchestration_graph_invariant_properties.py`:
- Around line 317-319: The test currently asserts positional equality between
observed_results and the list comprehension [state["orchestration_result"] for
state in states], which flakes under concurrent runs; change this to
order-insensitive comparison by asserting multiset equivalence (e.g., compare
collections.Counter(observed_results) ==
collections.Counter([state["orchestration_result"] for state in states])) while
keeping the existing length check (len(observed_results) ==
expected_invocations) and the non-None assertion (all(result is not None for
result in observed_results)); update the assertion that references
observed_results and states to use Counter (or another multiset comparison)
instead of direct list equality.
🪄 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: f742faae-2c72-4785-bf93-695e9f43a832
📒 Files selected for processing (3)
docs/developers-guide.mdepisodic/orchestration/langgraph.pytests/test_orchestration_graph_invariant_properties.py
Normalize the finish callback concurrency prose to Oxford `-ize` forms and make the concurrent callback assertion order-insensitive so scheduling order cannot affect the invariant test.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Expand the private checkpoint module docstrings so their role in the LangGraph suspend and resume flow is clear. Document `resume_generation_orchestration` in the developer guide and record the `finish_callback` observation contract in ADR-005.
leynos
deleted the
issue-72-hypothesis-property-based-tests-for-episodic-orchestration
branch
May 25, 2026 22:53
This was referenced May 26, 2026
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 Hypothesis coverage for the structured generation orchestration contracts raised in issue #72. It exercises enum normalisation across mixed
ActionKindinputs, rejects every non-executionModelTier, broadens planner format-error preservation across malformed JSON and invalid plan objects, and checks LangGraph ordering and token aggregation invariants.Review follow-up changes expand the module documentation, make graph event recording an explicit injected dependency, bound generated malformed payloads, and pin representative
PlanningResponseFormatErrormessages with Syrupy snapshots.Closes #72.
Review walkthrough
Validation
make check-fmt: passedmake lint: passedmake test PYTEST_XDIST_WORKERS=1: passed, 481 passed, 3 skippedmake typecheck: passedNotes
hypothesiswas already present in the dev dependency group, so no dependency or lockfile change was required. One full-suite run hit a transientpy-pglitefixture setup timeout in an unrelated reference-document pagination case; the isolated case passed immediately, and the subsequent full-suite retry passed.