[EPD-196] Add strict mode + machine-readable outcome for restore_from_state_id misses#6540
Open
joaomdmoura wants to merge 5 commits into
Open
Conversation
…_state_id misses - Add opt-in raise_on_missing_state flag to kickoff, kickoff_async, akickoff, stream_events, and astream: when True and the restore_from_state_id lookup misses (or no persistence backend is configured), kickoff raises a clear ValueError instead of silently running a fresh session. - Expose flow.last_restore_succeeded (None = no restore requested, True = hydrated, False = miss) so API wrappers can detect silent restore misses without parsing logs. - Default behavior is unchanged: misses still fall back silently. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughFlow persistence APIs now support strict missing-state handling through ChangesRestore handling
OpenAI import test handling
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Flow
participant PersistenceBackend
Caller->>Flow: kickoff with restore_from_state_id
Flow->>Flow: reset last restore outcome
Flow->>PersistenceBackend: load_state(restore_from_state_id)
alt state found
PersistenceBackend-->>Flow: persisted state
Flow->>Flow: hydrate state and set outcome True
else state missing or persistence unavailable
PersistenceBackend-->>Flow: no state
Flow->>Flow: set outcome False
Flow-->>Caller: raise ValueError when strict mode is enabled
end
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
- stream_events/astream now pre-validate restore_from_state_id when raise_on_missing_state=True, so the ValueError surfaces at session creation time instead of only while consuming frames (Bugbot finding). - Extract _missing_state_error/_no_persistence_error builders shared by the eager check and the runtime fork block so messages stay in sync. - Add streaming tests: eager raise via stream_events, kickoff(stream=True), astream, kickoff_async(stream=True); valid-id strict streaming still hydrates and reports last_restore_succeeded=True. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/crewai/tests/test_flow_persistence.py (1)
508-512: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsistently assert the error message for strict restore failures.
Capture
excinfofor thekickoff_asynccall to verify the error message, maintaining consistency with the otherpytest.raisesblocks in these tests. This prevents the test from accidentally passing on an unrelatedValueError.🛠️ Proposed refactor
- with pytest.raises(ValueError): + with pytest.raises(ValueError) as excinfo: await flow2.kickoff_async( restore_from_state_id="no-such-uuid", raise_on_missing_state=True, ) + assert "no-such-uuid" in str(excinfo.value)🤖 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 `@lib/crewai/tests/test_flow_persistence.py` around lines 508 - 512, Update the pytest.raises block around flow2.kickoff_async to capture the raised exception and assert that its message identifies the missing state ID, matching the message-validation pattern used by nearby tests.
🤖 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.
Nitpick comments:
In `@lib/crewai/tests/test_flow_persistence.py`:
- Around line 508-512: Update the pytest.raises block around flow2.kickoff_async
to capture the raised exception and assert that its message identifies the
missing state ID, matching the message-validation pattern used by nearby tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e253e3f2-d597-4986-94ef-5f07bb2fc8b6
📒 Files selected for processing (2)
lib/crewai/src/crewai/flow/runtime/__init__.pylib/crewai/tests/test_flow_persistence.py
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/crewai/src/crewai/flow/runtime/init.py
…heck first - _ensure_restorable_state now sets last_restore_succeeded=False before raising, matching the runtime path, so streaming callers failing in the pre-check see the miss signal instead of None or a stale True. - stream_events/astream validate the from_checkpoint/restore_from_state_id mutual exclusion before the strict restore pre-check, so the conflict ValueError wins over the restore-miss one (matching non-streaming precedence). Conflict check extracted to _check_restore_conflict and reused by kickoff/kickoff_async. - Failing-first tests for the stale signal (incl. prior-success staleness and no-persistence path) and for conflict precedence on both stream entry points. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lib/crewai/tests/test_flow_persistence.py (2)
586-603: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
matchparameter inpytest.raisesfor conciseness.Instead of manually extracting and asserting on the exception message, you can use the
matchparameter inpytest.raisesfor a more idiomatic and concise assertion.♻️ Proposed refactor
- with pytest.raises(ValueError) as excinfo: + with pytest.raises(ValueError, match="Cannot combine `from_checkpoint` and `restore_from_state_id`"): flow.stream_events( from_checkpoint=CheckpointConfig(), restore_from_state_id="no-such-uuid", raise_on_missing_state=True, ) - msg = str(excinfo.value) - assert "Cannot combine" in msg - assert "from_checkpoint" in msg flow2 = StreamConflictFlow(persistence=persistence) - with pytest.raises(ValueError) as excinfo2: + with pytest.raises(ValueError, match="Cannot combine `from_checkpoint` and `restore_from_state_id`"): flow2.astream( from_checkpoint=CheckpointConfig(), restore_from_state_id="no-such-uuid", raise_on_missing_state=True, ) - assert "Cannot combine" in str(excinfo2.value)🤖 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 `@lib/crewai/tests/test_flow_persistence.py` around lines 586 - 603, Update the two pytest.raises blocks for flow.stream_events and flow2.astream to use the match parameter with a regex covering the expected “Cannot combine” and “from_checkpoint” text, then remove the manual exception capture, message conversion, and separate assertions.
539-567: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert specific exception messages in
pytest.raises.Catching a generic
ValueErrorwithout validating its message could accidentally pass if an unrelatedValueErroris raised (e.g., due to an initialization error). Consider using thematchparameter to assert the specific error message to make the test more robust.♻️ Proposed refactor
- with pytest.raises(ValueError): + with pytest.raises(ValueError, match="No flow state found"): flow2.stream_events( restore_from_state_id="no-such-uuid", raise_on_missing_state=True, ) assert flow2.last_restore_succeeded is False # Fresh instance failing in the astream pre-check: False, not None. flow3 = EagerSignalFlow(persistence=persistence) - with pytest.raises(ValueError): + with pytest.raises(ValueError, match="No flow state found"): flow3.astream( restore_from_state_id="no-such-uuid", raise_on_missing_state=True, ) assert flow3.last_restore_succeeded is False # No-persistence eager failure records the miss too. class NoPersistenceStreamFlow(Flow[TestState]): `@start`() def step(self): self.state.counter += 1 flow4 = NoPersistenceStreamFlow() - with pytest.raises(ValueError): + with pytest.raises(ValueError, match="no persistence backend"): flow4.stream_events( restore_from_state_id="some-uuid", raise_on_missing_state=True, ) assert flow4.last_restore_succeeded is False🤖 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 `@lib/crewai/tests/test_flow_persistence.py` around lines 539 - 567, Update each pytest.raises(ValueError) assertion in the persistence restore tests for flow2, flow3, and flow4 to use the match parameter with the expected missing-state error message. Keep the existing exception type and last_restore_succeeded assertions unchanged.
🤖 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.
Nitpick comments:
In `@lib/crewai/tests/test_flow_persistence.py`:
- Around line 586-603: Update the two pytest.raises blocks for
flow.stream_events and flow2.astream to use the match parameter with a regex
covering the expected “Cannot combine” and “from_checkpoint” text, then remove
the manual exception capture, message conversion, and separate assertions.
- Around line 539-567: Update each pytest.raises(ValueError) assertion in the
persistence restore tests for flow2, flow3, and flow4 to use the match parameter
with the expected missing-state error message. Keep the existing exception type
and last_restore_succeeded assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e4bd6c59-02c5-4516-8aed-a94ffab958ce
📒 Files selected for processing (2)
lib/crewai/src/crewai/flow/runtime/__init__.pylib/crewai/tests/test_flow_persistence.py
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/crewai/src/crewai/flow/runtime/init.py
…es mocks Not caused by this PR's change — a latent leak in test_openai_completion_module_is_imported exposed by pytest-randomly's per-run ordering (CI seed 756721157 placed it before the azure responses tests on the same xdist worker of the py3.10 shard). Mechanism: the test monkeypatch.delitem's crewai.llms.providers.openai.completion from sys.modules and re-imports it, which rebinds the parent package's `completion` attribute to the new module object. monkeypatch teardown restores only the sys.modules entry, leaving the package attribute pointing at the stale re-imported module. On Python 3.10, `from crewai.llms.providers.openai.completion import OpenAICompletion` resolves through the parent package attribute while mock.patch targets the sys.modules entry, so the azure responses tests' OpenAICompletion patch silently missed and a real delegate was built (last_response_id None, reset_chain never called). Python 3.11+ resolves via sys.modules, hiding the leak. Fix: record the package attribute with monkeypatch.setattr alongside the delitem so teardown restores both bindings consistently. Reproduced on pristine origin/main (py3.10, -n0, culprit test followed by the three azure responses tests) and verified green after the fix, including a replay of the failing CI worker's exact 181-test order. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
The CI failures in Mechanism (reproduced on pristine
Fix: record the parent package attribute with 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ad0523b. Configure here.
…eated - stream_events/astream (and thus kickoff/kickoff_async with stream=True) now reset last_restore_succeeded to None before returning the session, matching kickoff_async's reset-at-start semantics. Previously the property held the prior kickoff's stale True/False until frames were consumed (Bugbot round 3). - The eager strict-raise path is unchanged: with raise_on_missing_state a miss still sets the signal to False before raising at session creation. - Document streaming timing on the last_restore_succeeded property. - Failing-first sync + async tests: stale True from a prior restore reads None right after obtaining a new session, then False after consumption. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
kickoff(restore_from_state_id=...)with an unknown state id used to log a warning and silently run a brand-new session — invisible to callers behind a remote API.raise_on_missing_state: bool = Falseflag tokickoff,kickoff_async,akickoff,stream_events, andastream: when True, a restore miss (or missing persistence backend) raisesValueError: No flow state found for restore_from_state_id: '<id>'instead of silently dropping context.flow.last_restore_succeeded(None= no restore requested,True= hydrated,False= miss) so wrappers like the AMP deployment API can detect silent misses programmatically.test_flow_persistence.pyand a docs note in the flow-state guide.🤖 Generated with Claude Code
Note
Medium Risk
Touches persisted-state hydration at kickoff (including streaming), which can change failure modes for remote APIs when strict mode is enabled; defaults preserve prior silent-fallback behavior.
Overview
Adds opt-in strict handling when forking persisted flow state via
restore_from_state_id: newraise_on_missing_state=Falseonkickoff,kickoff_async,akickoff,stream_events, andastreamraisesValueErroron a missing snapshot or missing persistence instead of silently starting fresh.Introduces
flow.last_restore_succeeded(None/True/False) so API wrappers can detect silent misses without log scraping. Streaming entry points reset the flag at session creation, run an eager pre-check when strict mode is on, and document when to read the signal after consuming frames.Refactors restore conflict checks into shared helpers; default silent-miss behavior is unchanged. Docs note the new flag and property; persistence tests cover sync/async/streaming paths. A small OpenAI import test fix keeps
sys.modulesand packagecompletionin sync after re-import.Reviewed by Cursor Bugbot for commit 6e81617. Bugbot is set up for automated code reviews on this repo. Configure here.