fix(workflows): an approval must not repeat an outward call, finish a run that is waiting, or hide what it is approving (#846) - #854
Conversation
… run that is waiting, or hide what it is approving (#846) Three defects on the workflow approval path. **A continuation repeated an outward call.** #438's requirement was never "stop re-executing" — a paused tinyflows run is settled, so approving is a re-run — it was that an approval must never send twice. #496 met that for the half the host routes itself (an `output` node's report) and could not reach the half the engine performs mid-run. A new outward-call ledger rides the approval card and the continuation input like #496's, and the node is rewritten before the run starts to replay its recorded result rather than call again. **A run reported Finished with its approval outstanding.** The console read only parked *deliveries*, so a run stopped at a gate — which never reaches an output node, and so has none — fell through every branch to "Finished". **The card never said what it was approving.** A call description existed only for policy-raised gates, so on a `full`-tier company every workflow card named a node id and showed the engine's resume payload. The host now describes an authored gate too, and the console labels the card by its tool.
|
@coderabbitai review |
|
Warning Review limit reached
Next review available in: 47 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 Plus Run ID: 📒 Files selected for processing (11)
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 |
|
`typecheck:unit` covers `test/unit`, and the fixture built its delivery rows from an invented shape — `target: null` where the wire says `target?: string`, plus two fields that do not exist. Vitest strips types, so it passed locally and failed the Console lane, which is exactly the blind spot that step exists for.
|
Pushed The Vitest strips types rather than checking them, so it went green locally and red in CI. That is precisely the blind spot Now typed against the real No Rust changed, and none of the revert-proofs in the PR body are affected. |
|
@coderabbitai review |
|
|
CI is green, including CodeRabbit has not reviewed this PR. Requested three times over ~50 minutes; every response was a rate-limit notice on the shared account, with no review content:
So there are no CodeRabbit findings outstanding — not "none found", but none produced. Flagging that explicitly so a green checks list is not mistaken for a clean automated review. Worth a re-trigger by whoever picks this up if you want that pass before merging. Human review is the one that matters for this diff anyway, and the part I would point at first is the Not merging or approving. |
|
The CI evidence is useful. It does not replace review coverage. I will request a new review, with focus on the
|
oxoxDev
left a comment
oxoxDev
left a comment
There was a problem hiding this comment.
Correcting the issue in both directions, and having the correction widen the work, is the part that makes this trustworthy:
- the citation was wrong (#438 was closed by #496, not #624), which changes the diagnosis from "the ledger regressed" to "the ledger's reach was narrower than the exposure";
- the issue's own example — a
send/publish/repo_publishnode after two gated steps — is not reachable today, because a workflowtool_callreaches only the four families the invoker wires and every slug in them isEffectGroup::Otherbar a billed read; - and the thing that is reachable is one the issue did not name: an
http_requestnode with a mutating method, arbitrary address, every company, right now.
Shipping the guard for the unreachable class anyway — "the guard that must exist before a send-capable namespace is wired, or wiring one silently re-opens #438 on the day it lands" — is the right call. That is the failure mode this repo has hit repeatedly: a constraint that holds today, is not written down as a guard, and quietly stops holding when something adjacent changes.
The root cause is stated precisely and matches what #777 established one layer over: a paused tinyflows run is settled, not suspended, so resuming is necessarily a re-run from the trigger. Given that, replaying a recorded result is the only correct answer — you cannot decline a call a node already made, so the continuation must not make it again.
Why the translation and not the invoker is the strongest paragraph. ToolInvoker::invoke receives (slug, args, conn) — no node id, no run state — which gate.rs already records as why the approval gate could not live there, and the same fact rules out a node-keyed skip inside it. And the alternative is worse for a reason easy to miss: keying on (tool, args) "would miss every send whose body an upstream agent node regenerates, which is most of them". Node identity is the only stable key, and the translation is the only place that holds both node ids and the trigger input.
I went looking for whether the replay sentinel is forgeable, since a slug that answers from its own arguments is the obvious thing to abuse. the_replay_sentinel_is_answered_without_a_grant_and_reaches_nothing pins the property that matters: it touches no toolbelt, so the worst an author — or an agent using create_workflow — can do by invoking it directly is fabricate a value they could have written as a literal anyway. Not an escalation, and the test names it rather than leaving it to be re-derived.
0 major. 1 question. Approving.
Question — what does the gate make of the rewritten node?
The node is rewritten to the sentinel before the run starts, and the ledger is stripped from the gate's dedupe identity. What I could not establish is what apply_policy_gates then classifies that node as on the continuation.
The sentinel is not in the declaration table, so consequence_of should fall to the undeclared path — and #660's judgement arm stops an undeclared tool as StopReason::Undeclared on the harness side. If the workflow gate reaches the same conclusion, the continuation would park the replay of a call the operator has already approved, which is a loop rather than a leak but an unpleasant one.
I suspect this is handled — stripping the ledger from the dedupe identity suggests the gate's view of the node was thought about carefully — but it is the one interaction where the fix meets the guard I reviewed on #660, and it deserves an explicit sentence or a test rather than being inferred from the absence of a failure.
Before merging: MERGEABLE / CLEAN. Worth confirming the gated lane is green before landing given this touches the workflow gate, which is the lane that compiles those tests.
3 participants
Summary
Three defects on the workflow approval path, in the order they matter.
1. Approving repeated an outward call the run had already made
This is a gap in a narrower fix, not a regression — and the citation in the issue was wrong.
#846 says #438 was closed by #624. The timeline says otherwise: #438 was closed by #496
("approving a paused run does not re-send what it already delivered"); #624 closed #435 (threaded
approvals). That is not pedantry, because it changes the diagnosis:
#496 deliberately kept re-run-on-approve. A paused tinyflows run is settled, not suspended —
the engine finishes and reports the gates it stopped at — so resuming is necessarily a re-run from
the trigger. #438's written requirement was never "stop re-executing"; it was "an approval must
never cause a message to be sent to a person twice." #496 met that by adding a delivery ledger.
Nothing here calls #496 broken, and a reviewer reading this as "the ledger regressed" should read
it as "the ledger's reach was narrower than the exposure."
The reach: #496 guards what the host routes with its own hands — an
outputnode's report,dispatched by
deliver_outputsafter the engine settles — because that is the half the host candecline after the fact. A call made by a node, mid-run, through a capability has no post-hoc
dispatch to decline. Put one upstream of a gate and the first run calls, the operator approves, and
the continuation calls again. Same lineage, same exposure, the other mechanism.
The severity claim in the issue needed correcting too, and the correction widens the work rather
than shrinking it. The issue's example — "put a
send,publishorrepo_publishnode after twogated steps and this delivers it twice" — is not reachable today. A workflow
tool_callcan onlyinvoke the four families the invoker wires (shell, code, web, search), and every slug in them is
declared
EffectGroup::Otherexceptweb_search, a billed read.send/publish/repo_publishareagent-turn families workflows do not wire at all.
What is reachable today is an
http_requestnode with a mutating method — arbitrary address,every company, right now. So this guards both, and neither half is speculative:
http_requestGET/HEADtool_callconsequence_ofgroup is outward, lessReach::MoneyMechanism. An outward-call ledger, built on #496's terms: recorded from the settled run's own
output, unioned down the lineage, carried on the approval card, threaded into the continuation input,
and stripped from the gate's dedupe identity. On the continuation the node is rewritten before the
run starts to invoke a host-private sentinel slug carrying its recorded result; the invoker answers
that slug from its arguments and touches no toolbelt.
Why the translation and not the invoker.
ToolInvoker::invokereceives(slug, args, conn)— nonode id, no run state — which
src/workflows/gate.rsalready records as the reason the approvalgate could not live there. The same fact rules out a node-keyed skip inside it. And node identity is
not negotiable: keying on
(tool, args)instead would miss every send whose body an upstream agentnode regenerates, which is most of them. The translation is where the host holds both the node ids
and the trigger input, and where
apply_policy_gatesalready writes per-node decisions.The
http_requestarm rewrites the node's kind totool_call. Safe because the two kinds alreadyagree about the only thing that leaves a node — every capability node wraps its result in the same
{ json, text, raw }envelope, andenvelope::wrapis a pure function of the recorded value, soreplaying reconstructs the node's output rather than approximating it. Flagging it explicitly as the
one part of this diff I would most like a second pair of eyes on.
The recorded result is JSON-encoded into a single string.
tinyflows::expr::resolvewalks nodeconfig before the node runs and evaluates every leaf beginning with
=; a recorded result isarbitrary data from a counterparty.
serde_json::to_stringnever yields a document starting with=,so it is inert by construction rather than by escaping rules. Pinned by a test.
2. A run reported
Finishedwith its approval outstanding19 of the 26 runs in the reproduction ended
Finished — this run routed no reportswhile their gatesat undecided.
Root cause:
runToneand the run row read only parked deliveries. A run stopped at a gate neverreaches an output node, so it has no deliveries, no error, no cancellation, and is not
running—the engine settled it. Every branch reads "fine" on its own, so it fell through to the green "ok" and
the "Finished" line.
pendingApprovalshas been on the wire since #395 and nothing read it for therun's state.
The run object really is settled; what has not happened is the work, and the work is what the operator
is asking about. So this reports the waiting state rather than claiming the run holds open — and names
the nodes, because a scheduled run that silently did nothing is the failure that makes this matter and
the fix is a click.
3. The card never said what it was approving
GatedCallwas produced only bypolicy_gates, so it existed only for a node the company's policystopped. An authored
requires_approvalproduced none — and on afull-tier company, where thepolicy stops nothing, that is every workflow card. The operator was asked to authorise
fetch_bbcand shown
{"items":[{"json":{}}],"port":null}.#372 made this exact complaint about the chat surface and #375 fixed it there by carrying the call's
own arguments. The information was already on the host in that case and it is already on the host in
this one:
call_ofhas read the slug and args since #460, and only the reason was everpolicy-specific. So an authored gate is now described from the graph and gains everything except the
sentence nobody wrote.
tool_callnodes also gain a destination (host only — #614 gavehttp_requestone and left
tool_callwithout, which is why a parkedweb_fetchnamed no host).Console-side, the card is labelled by its tool ("Fetch a web page") and the payload block shows the
call instead of the resume record. The dropped keys are dropped for stated reasons — notably
input,which is what read as a description and was not one, and which carries the accumulating
{"approvals":[...]}list an operator reasonably misreads as one card covering several gates.Coordination with #842 / #848 (batching): deliberately untouched. All card changes are in
frontend/src/lib/language.ts(pure functions) and not inapproval-card.tsx, specifically sothis does not collide with W2's PR. Nothing here groups or consolidates cards.
API Or Behavior Changes
No HTTP/GraphQL API changes. No route, no wire type gained a required field.
Wire (additive, all optional). The
workflow.approveeffect payload gainsperformed(theoutward-call ledger) and
args(the gated call's arguments);toolandtargetare now written forauthored gates as well as policy-raised ones.
argsis credential-redacted host-side by the existingdisplay_payloadprojection that already redacts a chat card's payload — this adds no new redactionrule and cannot bypass the current one. An older console ignores every one of them; this console treats
absence as "old host" and renders exactly the pre-#846 card, which is pinned by a test.
Journal / replay. The trigger input gains a reserved
__opencompany_delivered-style sibling,__opencompany_performed, written only when there is something to suppress — so a first run's payloadkeeps byte-identical shape. Both reserved keys are stripped before two parked gates are compared, or
every continuation gate would read as a new decision and stack a duplicate card.
Behaviour.
http_request, or an outward-classifiedtool_call,that an earlier run in its lineage already made — it replays the recorded result.
chip and the status dot.
Limits, stated rather than hidden
shell,curl,git_operationsare not guarded. All three areEffectGroup::Otherand allthree can reach a counterparty. The host cannot tell a
git logfrom agit push, and replayingevery shell node's recorded stdout would change the behaviour of the most common effectful node in a
workflow on the strength of a guess. Filed as workflows: a continuation still repeats a shell / curl / git_operations call that already reached a counterparty #850, with three candidate routes, rather than
folded in here.
answer N invocations without inventing which. Refused rather than approximated — and surfaced as a
run notice, so the operator learns before they approve. fix(workflows): approving a paused run does not re-send what it already delivered (#438, #440) #496 took the same side of the same trade
for a partial delivery fan-out.
silently-clipped receipt as though it were whole is worse. Also surfaced as a notice.
web_fetch/web_searchstill re-run. The three re-executed fetches in the reproduction stillre-execute, which is the issue's own reading of them: idempotent reads whose cost is latency and
tokens.
web_searchis excluded by theReach::Moneycarve-out rather than by its group — it isdeclared
EffectGroup::Spendfor billing and is a read.run_resumableinstalls a no-op observer and a process-local in-memory checkpointer and takes no cancellation token,
so it composes with neither the per-node progress trail (workflows: a running workflow shows no progress until it finishes #371) nor the stop signal (workflows: cancelling a run drops the engine future, because tinyflows cannot observe and cancel at once #398), and an
approval arriving after a restart would find no checkpoint. Engine work in a vendored crate, exactly
as workflows: resuming an approved run re-executes upstream nodes, so a delivery can be sent twice #438 and fix(workflows): approving a paused run does not re-send what it already delivered (#438, #440) #496 both recorded. This guard is forward-compatible: keyed on the node, and it withdraws
to nothing when the ledger is empty.
Tests
cargo fmt --all -- --checkcargo clippy --all-targets -- -D warnings— ran as CI does, both lanes:--locked --no-depsdefault, and
--locked --no-deps --features openhuman,tinycortex. Both clean.cargo build --all-targets— ran ascargo check --locked --all-features --all-targetspluscargo check --locked --features openhuman,tinycortex --all-targets. Both clean. (check, notbuild: nothing here is a linker-visible change, and the gated lane's test binaries link in CI.)cargo test—cargo test --locked --features openhuman,tinycortex --lib: 3610 passed, 0failed, 3 ignored. Needs
RUST_MIN_STACK=16777216locally, orharness::brain::tests::a_cancelled_hand_off_returns_the_card_to_todo_as_a_cancellationoverflows the default thread stack — an untouched module, and a local artifact rather than a
code defect.
git diff --stat Cargo.lock— empty.npm ci,npm run typecheck,npm run build— all clean.npx vitest run: 11 new tests pass.57 failures in
tour-resume/connection-registry/desktop-bridgeare pre-existing —verified identical with this branch stashed.
Every new test was proven to fail with its fix reverted — 11 of 11
Each fix was reverted individually and the named test re-run:
performed_ledgerdrops the lineage unionan_outward_call_is_made_once_across_two_gatescontinuation_inputstops threading the ledgeran_outward_call_is_made_once_across_two_gateswithout_ledgerstops stripping the reserved keythe_outward_ledger_is_not_part_of_a_gates_identityreplay_performedbecomes a no-opa_recorded_node_is_rewritten_to_replay,a_recorded_expression_string_is_not_evaluatedthe_replay_sentinel_is_answered_without_a_grant_and_reaches_nothingMoneycarve-out and theGETarmreads_are_not_recorded_whatever_they_costan_oversized_result_is_refused_rather_than_clippeda_fan_out_is_refused_and_surfacedapprovalActionloses the workflow-tool rungnames the tool rather than the mechanismpayloadLinesdumps the raw resume payloadshows the call's arguments…,hides the engine's resume payload…awaitingCountforgets the gatescounts a parked gate as awaiting…,counts parked gates and parked reports togetherOne test initially failed to fail, and was rewritten. The first version of
an_outward_call_is_made_once_across_two_gatespassed with the lineage union reverted: the ledgerwas surviving anyway, because a card copies the paused run's whole trigger input and that input already
carried the key. The union only decides anything when a later run performs a new outward call, since
a non-empty ledger replaces rather than merges. The test now does exactly that, and fails on the
first node's entry when the union is reverted. Recording it because a test that cannot fail is worse
than no test.
What is NOT proven, stated rather than claimed as coverage
No test drives a real outward call end-to-end through a whole lineage. Two independent blockers,
both pre-existing: the SSRF
url_guardrefuses loopback forhttp_requestregardless of the company'sallowlist (already recorded in
runner.rs's cancel test, which had to use an agent node for the samereason), and no wired
tool_callslug is outward, so there is nothing to count at a transport. It iscovered at each seam instead — including the real
WorkflowToolInvoker, constructed with zerogrants, answering the sentinel and still refusing an ungranted
shellin the same test, which is whatproves the arm sits above the fail-closed check without widening anything.
The run-history JSX branch has no component test. Its logic (
awaitingCount,runTone) is coveredand revert-proven; the markup around it is not. This repo has no component-test harness for that view.
Documentation
No spec doc changes. The reasoning lives at the code it governs:
src/workflows/replay.rs's module docscarry the seam argument, the two rules and every limit above, beside the tests that pin them.
Closes #846