fix(flows): pin a parked run to the graph it was approved against - #5293
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughFlow runs now store a canonical SHA-256 hash for workflows paused at approval gates. Resume checks reject changed workflows, cancel stale runs, and remove checkpoints. Legacy runs without hashes remain resumable. Storage, types, finalization paths, and tests support the new field. ChangesApproval resume graph pinning
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
|
Review follow-up: widened the pin to cover
So the original graph-only pin left the exact hole this guard exists to close, one level down: park at a gate → user approves → flip The hash now covers 559 flows tests pass, Worth a follow-up sweep (not done here): anything else read off |
11e39a0 to
04818d0
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
graycyrus has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
coderabbitai
Bot
added
bug
rust-core
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04818d0535
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| finish_flow_run_row( | ||
| config, | ||
| thread_id, | ||
| flow_id, | ||
| "cancelled", |
There was a problem hiding this comment.
When a resume request has already loaded the approved graph and flipped the row to running, a second resume that read the row while it was still pending but sees a concurrent graph edit can enter this mismatch branch. finish_flow_run_row updates both running and pending_approval rows, so this path can cancel the first resume's active row and then drop its checkpoint while approved side effects are still executing; since the return value is ignored, it can also record last_status as cancelled after another writer won. Please claim the row before doing the hash refusal, or use a pending-only guarded update and only record/drop on success.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed — you both independently found the same real hazard, and it was a genuine miss on my part: I added exactly this guard to flows_cancel_run earlier in this batch (its ORDER MATTERS note) and then failed to apply the same rule to the new refusal path here.
The refusal now settles the row first and treats the guarded write as the authority. Only when it actually matched does it record_run and drop_checkpoint; otherwise it logs that another resume or cancel owns the run and leaves that runs status and checkpoint untouched. The refusal error is returned either way, since this resume view of the graph is stale and it must never proceed regardless of who owns the row.
Pinned by stale_approval_refusal_does_not_settle_a_run_another_resume_claimed, which claims the run via mark_run_resuming first and asserts the caller learns the writes real verdict rather than assuming the row was still parked.
Also corrected the compute_graph_hash doc that claimed a hash failure is treated like a legacy row with no pin. On the park side None stores no pin, but on the resume side the comparison is Some(expected) != None, so it refuses and drops the checkpoint. Failing closed there is right; the doc claiming fail-open was not.
595 flows tests pass, cargo fmt --check clean.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/openhuman/flows/ops.rs (1)
5945-5972: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving the hashing helpers into their own module.
compute_graph_hashandcanonicalize_jsonform a self-contained, dependency-light unit: aWorkflowGraphplus aboolin, a hex digest out.ops.rsis already close to 6000 lines, and this PR adds roughly 150 more. Extracting these two functions into something likesrc/openhuman/flows/graph_pin.rswould move the pin logic and its unit tests next to each other and would shrinkops.rsslightly.This is a mechanical move with no behavior change, so it can be deferred to a follow-up.
Based on coding guidelines: "Prefer Rust modules of approximately 500 lines or fewer and maintain small, single-responsibility Unix-style modules."
🤖 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 `@src/openhuman/flows/ops.rs` around lines 5945 - 5972, Extract compute_graph_hash and canonicalize_json from ops.rs into a dedicated graph-pin module such as graph_pin.rs, preserving their existing behavior and signatures. Move the related hashing unit tests alongside these helpers, update imports and call sites to use the new module, and remove the original definitions from ops.rs.Source: Coding guidelines
src/openhuman/flows/store.rs (1)
753-785: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a parameter struct for
finish_flow_run.
finish_flow_runnow takes eight positional parameters. The last two,error: Option<&str>andgraph_hash: Option<&str>, share the same type and sit next to each other. A transposed call compiles silently and writes the refusal message into the pin column, or the hash into the user-visible error field. The same shape now repeats infinish_flow_run_rowinsrc/openhuman/flows/ops.rsat lines 5834-5843.All current call sites look correct. This is a defensive change against future edits, so it can be deferred.
One option is a small
FlowRunFinish<'_>struct with named fields, passed by bothfinish_flow_run_rowandfinish_flow_run. Another lower-cost option is a newtype such asGraphPin<'a>(&'a str)so the two arguments stop being interchangeable.🤖 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 `@src/openhuman/flows/store.rs` around lines 753 - 785, Defer this defensive API change; no implementation update is required because all current finish_flow_run and finish_flow_run_row call sites are correct. If addressed later, replace the adjacent error and graph_hash positional arguments with named fields in a shared FlowRunFinish struct used by both functions.
🤖 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 `@src/openhuman/flows/ops.rs`:
- Around line 5940-5944: Correct the documentation near compute_graph_hash to
distinguish park-side and resume-side failures: retain the existing fail-closed
resume behavior when run_record.graph_hash is Some and hashing returns None,
which rejects/cancels the run and drops its checkpoint. Remove the claim that
resume falls back to unknown/allow, while preserving the explanation that None
on the park side means no pin is stored.
- Around line 5127-5151: Use the boolean returned by finish_flow_run_row in the
stale-approval refusal branch as the authority for cleanup. Only call
store::record_run and drop_checkpoint after the guarded write reports success;
if it does not, skip both operations. Preserve returning
GRAPH_CHANGED_SINCE_PARK_ERROR in either case, following the established
handling in flows_cancel_run.
---
Nitpick comments:
In `@src/openhuman/flows/ops.rs`:
- Around line 5945-5972: Extract compute_graph_hash and canonicalize_json from
ops.rs into a dedicated graph-pin module such as graph_pin.rs, preserving their
existing behavior and signatures. Move the related hashing unit tests alongside
these helpers, update imports and call sites to use the new module, and remove
the original definitions from ops.rs.
In `@src/openhuman/flows/store.rs`:
- Around line 753-785: Defer this defensive API change; no implementation update
is required because all current finish_flow_run and finish_flow_run_row call
sites are correct. If addressed later, replace the adjacent error and graph_hash
positional arguments with named fields in a shared FlowRunFinish struct used by
both functions.
🪄 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: CHILL
Plan: Pro
Run ID: d07d09d1-9e9f-46ae-942a-7de80bc3784f
📒 Files selected for processing (7)
src/openhuman/flows/bus.rssrc/openhuman/flows/medulla_bridge_tests.rssrc/openhuman/flows/ops.rssrc/openhuman/flows/ops_tests.rssrc/openhuman/flows/store.rssrc/openhuman/flows/store_tests.rssrc/openhuman/flows/types.rs
04818d0 to
c2778c0
Compare
There was a problem hiding this comment.
graycyrus has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Manual test status — partial, noted for the reviewer. Confirmed by hand in a running app: a node-level Not confirmed by hand: the refusal path itself. The parked run was never resumed in that session (zero It is covered by automated tests: Worth a reviewer's eye on one thing the manual attempt surfaced: it was not obvious in the UI where to approve a parked run as opposed to the save/enable pre-authorization card (#5248). They are different gates — the pre-auth card grants action classes up front, the park suspends a specific node mid-run — and conflating them is easy. If the parked-run approval is hard to reach, that is a UX gap worth its own issue rather than something this PR should absorb. |
A run parks `pending_approval` on, say, a `send` node, and the user sees an approval card describing that node as it existed at park time. `flows_resume` then loaded the flow's CURRENT graph and resumed the old checkpoint against it, with no version pin between the two — so if `save_workflow` (or any other `flows_update`) rewrote that node's args or slug while the run sat parked, the resume fired whatever the NEW config does, under an approval the user never saw. The builder agent holds both `save_workflow` and `resume_flow_run`, so this was reachable in one authoring session. The engine-compatibility gate re-runs at resume, but nothing checked that the graph was the one the checkpoint was actually taken from. Fix: persist a canonical SHA-256 of the graph on the run row when it parks (`flow_runs.graph_hash`, added via the existing `add_column_if_missing` idiom), recompute it at resume, and on mismatch FAIL CLOSED — refuse, settle the run terminally with a clear reason, and drop the checkpoint rather than execute. Hashing is over a key-sorted serialization so the digest is stable regardless of JSON key order. Deliberately NOT blocking `flows_update` while runs are parked: that would let a stale park hold a flow hostage for the full TTL. Failing closed at resume is the chosen trade, and it is documented at the guard. Migration safety: rows written before this guard read back as `graph_hash IS NULL` and are allowed through with a warning, so upgrading mid-park cannot strand in-flight approvals. Pinned by a test. Two pre-existing tests legitimately broke: both simulate a "legacy incompatible graph" by swapping the graph after park, which the new pin now catches first — correctly, since the graph did change. Their fixtures re-pin the hash so the compatibility gate they exist to cover is still reached. `sha2` was already a direct dependency; no Cargo.toml/Cargo.lock change. 558 flows tests pass.
c2778c0 to
f57a60f
Compare
There was a problem hiding this comment.
graycyrus has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
1 participant
Summary
Problem
A run parks
pending_approvalon a node — saysend— and the user sees an approval card describing that node as it existed at park time.flows_resumethen loaded the flow's current graph and resumed the old checkpoint against it:There was no version pin between the checkpoint and the persisted graph, and
flows_updatehas no in-flight/pending-run guard. So ifsave_workflow(or any other update) rewrote that node'sargs/slugwhile the run sat parked, the resume fired whatever the new config does — under an approval the user never saw.This is reachable inside a single authoring session: the
workflow_builderagent holds bothsave_workflowandresume_flow_run.The engine-compatibility gate does re-run at resume, but nothing checked that the graph was the one the checkpoint was actually taken from.
Solution
flow_runs.graph_hash, added through the existingadd_column_if_missingidiom). The digest is computed over a key-sorted serialization, so it is stable regardless of JSON key order.Deliberately not doing: blocking
flows_updatewhile runs are parked. That would let a stale park hold a flow hostage for the entire TTL. Failing closed at resume is the chosen trade, and it is documented at the guard.Migration safety: rows written before this guard read back as
graph_hash IS NULLand are allowed through with a warning, so upgrading while a run is parked cannot strand an in-flight approval. Pinned by a test.Note for reviewers: two pre-existing tests changed, legitimately
flows_resume_marks_an_incompatible_legacy_checkpoint_failedandflows_resume_marks_a_checkpoint_with_an_incompatible_saved_child_failedboth simulate a "legacy incompatible graph" by swapping the graph after park. Under the new pin that swap is now caught first — correctly, since the graph genuinely did change. Their fixtures re-pin the hash to the swapped-in graph so the compatibility gate they exist to cover is still the thing being exercised.Submission Checklist
cargo test --lib openhuman::flows= 558 passed, 0 failedN/A: security hardening of an existing path, no feature rows added/removed/renamed## Related—N/A: no matrix feature rows affectedN/A: no user-facing surface change beyond a refusal messageCloses #NNN—N/A: found by code review, no tracking issue filed yetImpact
graph_hash TEXTcolumn toflow_runsvia the existing idempotent migration idiom. No backfill; NULL is a meaningful "legacy, unknown" value.sha2was already a direct dependency — noCargo.toml/Cargo.lockchange.Related
N/Aflows_resumethe run-lifecycle safetyflows_runalready had #5286 — merge that first. Touchesstore.rs/ops.rs, so it also overlaps fix(flows): close two authorization boundaries in flow-run tools #5287 and docs(flows): fix contract-drift comments and repair the workflow-builder prompt structure #5290; rebase after those land if they merge first.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/flows-resume-graph-pin08a892e5a(plus fix(flows): giveflows_resumethe run-lifecycle safetyflows_runalready had #5286's0b7105fa7as its base)Validation Run
pnpm --filter openhuman-app format:check— N/A, no frontend files changedpnpm typecheck— N/A, no TypeScript changedGGML_NATIVE=OFF cargo test --lib openhuman::flows→ 558 passed, 0 failedGGML_NATIVE=OFF cargo checkclean;cargo fmtappliedapp/src-tauriuntouchedValidation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
Parity Contract
graph_hash IS NULL) resume exactly as before, with a warning log — no in-flight approval is stranded by the upgrade.Duplicate / Superseded PR Handling
Summary by CodeRabbit