docs(harness): correct the claim that openhuman cannot resume a parked call (#561) - #777
Conversation
…d call (tinyhumansai#561) Comment only; no behaviour change. This module asserted openhuman "genuinely cannot be resumed here". That is false as a blanket statement, and the assertion is why nobody checked: it reads as a settled fact while being an untested inference. openhuman never claimed impossibility — its own `ToolPolicyDecision::RequireApproval` doc says session execution *currently* treats the variant as fail-closed and that callers able to prompt for approval "may branch on this variant and retry". It also already ships durable approval interrupt-and-resume for delegation, on tinyagents' graph executor. Two separate things are true, and only one of them is a wall: Resolving inline is INCIDENTAL. The block happens in one `wrap_tool` middleware; that hook is async and may call `next.run` zero or more times, which tinyagents pins with `wrap_tool_retries_next_until_success`. Awaiting a verdict there and then dispatching needs no upstream change. Durably suspending is STRUCTURAL. The agent turn runs on `AgentHarness`, which has no checkpointer; `AgentRun` is not `Serialize`; and the turn is a live async task holding the model context, bounded by the run's wall-clock deadline. Which is what settles the design, and is the sentence worth keeping: an in-memory await survives neither a long approval nor a process restart, so against this crate's seven-day standing-grant ceiling it is a leak rather than a mechanism. Re-issuing stays correct here until the turn is checkpointable. Full analysis on tinyhumansai#561, including why even the graph executor's `resume` re-runs its interrupted node rather than continuing a suspended call — so re-dispatch and the durable design are the same kind of mechanism at different grain.
|
@coderabbitai review |
oxoxDev
left a comment
oxoxDev
left a comment
There was a problem hiding this comment.
This is the most valuable kind of correction, and the sentence that earns it is the diagnosis rather than the fix:
the assertion is why nobody checked — it reads as a settled fact while being an untested inference
Four PRs worked around a symptom because a comment asserted a wall that was never verified. Going to openhuman's own doc — which says session execution currently treats the variant as fail-closed, and that callers able to prompt "may branch on this variant and retry after approval is granted" — is the check nobody did.
Splitting the claim into the two things that are actually true is what makes the correction useful rather than merely accurate: resolving inline is incidental (one async wrap_tool middleware that may call next.run zero or more times, pinned upstream by wrap_tool_retries_next_until_success, so await-then-dispatch is representable today with no upstream change), while durably suspending is structural (no checkpointer on AgentHarness, AgentRun is not Serialize). Only the second is a wall, and conflating them is what turned a design constraint into a design assumption.
0 major. 1 follow-through, and it is the point. Approving.
The corrected claim lives in five files; this PR fixes one
src/company/types.rs
src/harness/brain.rs
src/harness/policy.rs ← corrected here
src/policy/consequence.rs
src/policy/judgement.rs
After this lands, four files still assert the thing this PR establishes is false — and the one being corrected is the only one without a behaviour decision resting on it. The two that matter most are untouched:
consequence.rs, onparks_under_auto:Reach::Moneydoes not park, because "openhuman resolves aRequireApprovalinline and never re-dispatches, so a parked search is a search that never happens and an agent with no search invents citations".judgement.rs(#338's arm):web_searchis kept out of the group-only stop on the identical reasoning, and that reasoning is why theReachpairing exists at all.
Both are live decisions about what does not park, justified by the premise being retracted. I am not arguing either conclusion is wrong — an inline await still blocks the turn, so "the operator is asked and the call then proceeds" may land in the same place as today for those tools. But the reason written at both sites is now known to be shakier than it reads, and this repo has spent the day being bitten by exactly that: a justification that stops describing the code, on an exclusion nobody re-derives.
Correcting one copy and leaving four is worse than the status quo in one specific way — a reader who finds judgement.rs has no signal that policy.rs now contradicts it. Either carry the correction to all five, or leave a pointer at the two behaviour-bearing ones saying the premise is qualified and where.
Worth an issue for the substantive question too, separately from the comments: if await-then-dispatch is representable today, is the Reach::Money carve-out still the right shape, or was it a workaround for a wall that is not there? That is a real design question this PR has just unlocked, and it should not live only in a corrected comment.
Before merging: 13 commits behind, one file, comment-only. Nothing else outstanding.
|
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe harness policy documentation now distinguishes inline approval handling from durable suspension. It explains that durable suspension requires checkpointable state, which the harness does not provide, so approved calls are re-issued. ChangesApproval resumption documentation
Estimated code review effort: 1 (Trivial) | ~2 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0056 · 9,576 in / 949 out · 6,312 cached (66%) · z-ai/glm-5.2
critique: $0.0019 · 2,175 in / 509 out · 1,612 cached (74%) · z-ai/glm-5.2
security: $0.0014 · 2,154 in / 115 out · 768 cached (36%) · z-ai/glm-5.2
tests: $0.0010 · 2,069 in / 160 out · 1,549 cached (75%) · z-ai/glm-5.2
description: $0.0013 · 3,178 in / 165 out · 2,383 cached (75%) · z-ai/glm-5.2
What this change touches1 file, +28 -3 across 1 component. It reaches 6 untouched components (60 graph nodes walked). 1 further component left out to keep the diagram readable. flowchart LR
n0["src/harness<br/>1 file +28 -3"]:::changed
n1["src/harness<br/>8 files reached"]:::impacted
n2["src/ports<br/>3 files reached"]:::impacted
n3["src/runtime<br/>2 files reached"]:::impacted
n4["src/company<br/>1 file reached"]:::impacted
n5["src/metering<br/>1 file reached"]:::impacted
n6["src/policy<br/>1 file reached"]:::impacted
n1 -->|19 refs| n2
n1 -->|14 refs| n3
n1 -->|12 refs| n4
n3 -->|4 refs| n2
n1 -->|2 refs| n6
n3 -->|2 refs| n1
n1 -->|1 ref| n5
n2 -->|1 ref| n4
n5 -->|1 ref| n2
n5 -->|1 ref| n4
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed. Grey: untouched, reached through an import or a call. Orange: has findings. Red: has a finding that blocks the merge.
Changed files
|
tinysweeper
Bot
added
the
priority: p3
2 participants
Summary
Comment only. No behaviour change.
src/harness/policy.rsasserted that openhuman "genuinely cannot be resumedhere". That is false as a blanket statement, and the assertion is why nobody
checked — it reads as a settled fact while being an untested inference. Four
PRs (#612, #625, #633, #672) worked around the symptom without anyone verifying
the premise.
openhuman never claimed impossibility. Its own
ToolPolicyDecision::RequireApprovaldoc says session execution currently treats the variant as fail-closed, and
that "callers that can prompt for approval may branch on this variant and retry
after approval is granted". It also already ships durable approval
interrupt-and-resume — for delegation, on tinyagents' graph executor.
The comment now records the two things that are actually true, because only one
of them is a wall:
wrap_toolmiddleware. That hook is
asyncand may callnext.runzero or more times —tinyagents pins exactly that with
wrap_tool_retries_next_until_success.Awaiting a verdict there and then dispatching is representable today with no
upstream change.
AgentHarness,which has no checkpointer;
AgentRunis notSerialize; and the turn is alive async task holding the model context, bounded by the run's wall-clock
deadline.
And the sentence that decides the design, which is the one this correction
exists to put in front of the next reader: an in-memory await survives neither
a long approval nor a process restart, so against this crate's seven-day
standing-grant ceiling it is a leak rather than a mechanism. Re-issuing stays
correct here until the turn is checkpointable.
This PR deliberately does not implement suspend/resume and does not
recommend a shape. There are two candidate designs — a checkpointer on the agent
engine, versus a bounded in-memory await for short approvals — and that is a
human's decision, not one to take on the back of a comment fix.
Full analysis is on #561, including the detail that most
changes how that issue should be scoped: even the graph executor's
resumere-runs its interrupted node rather than continuing a suspended call. So
re-dispatch and the durable design are the same kind of mechanism at
different grain — the cost can be narrowed, but "approving costs a re-dispatch"
probably cannot be driven to zero.
Closes nothing. #561 stays open for the design decision; this only removes the
false premise that was standing in front of it.
API Or Behavior Changes
None. Documentation comment only — no code, no signatures, no tests changed.
Tests
cargo fmt --all -- --check— exit 0cargo check --locked --all-features --all-targets— exit 0cargo clippy --locked --no-deps --features openhuman,tinycortex --all-targets -- -D warnings— exit 0cargo test --features openhuman,tinycortex— exit 0, 3388 passed, 0 failed, 3 ignoredNo revert-and-check, stated deliberately rather than omitted. This adds no
test and changes no behaviour, so there is nothing whose removal could turn a
test red; performing one would be exactly the vacuous green this repo's
process warns about. What was checked instead: the added text introduces no
[...]intra-doc links (verified against the added lines in isolation), sorustdoc has no new reference to resolve and cannot break on it.
cargo fmt --all -- --checkcargo clippy --all-targets -- -D warnings(run as the gated lane:--locked --no-deps --features openhuman,tinycortex --all-targets)cargo build --all-targets(N/A as spelled — covered bycargo check --locked --all-features --all-targets, the stricter lane, which builds every target under every feature)cargo test(run ascargo test --features openhuman,tinycortex)Documentation
This change is the documentation. Nothing under
docs/states theresume/re-dispatch rationale — it lives in this module header, which is where a
reader of the approval path arrives — so nothing there went stale.
Summary by CodeRabbit