refactor(code): run /offload as a server operation - #5261
Open
Mason Daugherty (mdrxy) wants to merge 25 commits into
Open
refactor(code): run /offload as a server operation#5261Mason Daugherty (mdrxy) wants to merge 25 commits into
/offload as a server operation#5261Mason Daugherty (mdrxy) wants to merge 25 commits into
Conversation
`/offload` injects a synthetic assistant message carrying a forced `compact_conversation` call directly into graph state, so `awrap_model_call` never runs and no Auto decision plan is checkpointed. `aafter_model` then treated that as an invalid-plan case and raised a HITL interrupt for an action the user had just requested, forcing the `/offload` driver to catch and self-approve its own interrupt. Recognize the seed from trust signals a model cannot forge — the client-set `offload_tool_call_id` run context, the deterministic seed message ID the compaction tool's execution guard already requires, and exact `force=True` args — and pass it through. Any other gated call in the batch keeps the existing manual-review fallback. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
- Log WARNING on the two impossible seed-signal states (no trusted
compaction tool, seed message missing the forced call) so a broken
trust signal no longer degrades silently behind the driver's
blind-approve fallback.
- Reject the bypass when a non-compaction call reuses the authorized
tool-call ID; previously the subset check let a same-ID `execute`
call through.
- Loosen the seed args check to `args.get("force") is True`,
matching `_offload_rejection` and `_decisions_for_interrupt`, and
read the context ID via the existing `_offload_tool_call_id` helper.
- Include the seeded ID in the bypass DEBUG line and drop the
Auto-only wording; the bypass is mode-independent.
- Update `app.py` docstrings: the bypass is the normal path, and
interrupt-self-approve is the fail-closed fallback for graphs
without the Auto HITL middleware.
- Tests: cover message-ID vs tool-call-ID mismatch separately, pin
that a seed-ID'd message carrying another gated tool is reviewed,
and add a positive-control chunk assertion to the integration
recorder. Fix the integration comment (the app runs Manual mode).
Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
github-actions
Bot
added
dcode
Mason Daugherty (mdrxy)
changed the title
/offload as a server operation
Mason Daugherty (mdrxy)
marked this pull request as ready for review
August 3, 2026 19:21
![open-swe[bot]](https://avatars.githubusercontent.com/in/1395372?s=60&v=4){kind=small}
…face failures as errors Replace the bare private-attribute writes on the composite backend with an OffloadServerResources NamedTuple attached through attach_offload_resources / offload_resources_from, keeping the attribute name in one place. The operation graph's node now raises RuntimeError with the COMPACTION_FAILURE_PREFIX on compaction failure and on PreCompact hook veto, so a server-backed /offload reports the real reason instead of a generic internal error or a misleading "already compact" no-op. The client unwraps RemoteException via format_agent_exception, and fires the SessionStart(compact) hook itself since the operation graph produces no tool result to key on. RemoteAgent.for_graph now caches sibling clients per graph name instead of leaking two httpx connection pools per call, and the server graph factories share one cached resources tuple. The driver replays the thread's own state as run input (minus _summarization_event, which the server cannot deserialize) because an empty input leaves a real server run with nothing to compact. Also documents the /offload graph's no-HITL authorization model and the noop-auth writable channels in the threat model.
…eded-compaction-no-hitl # Conflicts: # libs/code/deepagents_code/offload_middleware.py
…eded-compaction-no-hitl # Conflicts: # libs/code/deepagents_code/agent.py
…in failures The `offload` operation graph accepted every channel `_OffloadState` declares as writable run input. `PrivateStateAttr` / `OmitFromInput` are honored by `create_agent`, not by a raw `StateGraph`, so the markers inherited from `CostState` did not restrict anything. Because the driver replayed the whole thread state as input and `_session_cost_usd` reduces with `operator.add`, every `/offload` echoed the checkpointed total back and doubled the thread's persisted spend, compounding across runs; `_session_cost_transfers` (`operator.or_`) resurrected settled subagent transfers, and a local caller on the `noop`-auth port could set the compaction cutoff directly. Declare an explicit `input_schema` carrying only `messages`, so the restriction is enforced by the graph rather than by the client's discipline in what it sends, and replay only `messages`. Inherit `_summarization_event` from `SummarizationState` instead of re-declaring it without the SDK's annotation. The driver also gave up silently in two reachable cases: an interrupt it cannot answer (a `PreToolUse` hook returning an `ask` permission makes the hook middleware raise a plain `HITLRequest`, which the operation graph has no HITL middleware to route) and a hook that outlasts the resume bound. Both left the run paused while the caller read the unchanged event as "nothing to offload". Both now return an error string. The resume bound is shared with the seeded driver rather than duplicated as a second literal. Further fixes: - Mint a fresh forced-tool-call id per `PreCompact` dispatch. The hook `invocation_id` derives from it plus the prompt id, which only rotates on user-prompt submit, so a constant id made two `/offload`s in one turn collide and replay the first decision, including a denial. - Convert hook-dispatch failures to `RuntimeError` so the server's serde allowlist does not replace them with "An internal error occurred". - Stop letting a cost-drain failure discard a committed compaction, which would report "your conversation is unchanged" while leaving an orphaned archive section no `_summarization_event` references. - Report a failed post-run graph rebind instead of only logging it; a later `/goal` or `/rubric` would otherwise fail with no explanation. - Do not let a stopping `SessionStart` hook suppress an already-committed offload and leave the status bar at pre-offload counts. - Derive the `offload` langgraph.json ref from the agent ref's module. The two graphs share one server runtime only because both resolve into the same factory closure, so a hardcoded ref would silently build a second sandbox and MCP session if `graph_ref` were overridden. - Return the graph/backend pair as a named `ServerRuntime`, make the hook middleware non-optional, and narrow the resource accessors to `CompositeBackend`. - Correct THREAT_MODEL DC5 (three producers; the archive guard covers only the forced paths), TB2 (`PreToolUse` is dispatched too), and TB10 (the input surface and the mechanism that restricts it), and drop docstrings describing an Auto-mode approval bypass that no longer exists.
…eded-compaction-no-hitl # Conflicts: # libs/code/deepagents_code/offload_middleware.py # libs/code/tests/unit_tests/test_compact_tool.py
The `/offload` operation graph minted its forced tool-call id with `uuid4()`
inside the node. Answering a hook interrupt re-executes a node from the top,
so the id changed between the request and the resume; `ServerHooksMiddleware`
folds it into the hook `invocation_id`, and `parse_hook_resume_value` rejects a
mismatched id as fatal. Every `/offload` therefore failed outright for anyone
with a `PreCompact`/`PreToolUse` hook configured, and the client's whole
fulfill/resume loop was unreachable. The id is now derived from the
task-scoped `checkpoint_ns`, which LangGraph reuses when it replays a task and
mints fresh for each run -- stable across resumes, distinct across runs.
The post-compaction cost drain awaited `aafter_agent`, which
`CostTrackingMiddleware` does not implement, so it resolved to
`AgentMiddleware`'s empty base method and charged nothing. It now calls
`after_agent` through a thread. Its test patched the async name onto a mock,
materializing a method that does not exist in production.
`stream_completed` was set after both `drain_error` breaks, so a failed drain
plus a failed rebind mounted "Offload finished, but..." alongside "Offload
could not complete". The rebind outcome is now recorded and reported by
`_handle_offload`, which also covers the inverse case the old flag missed: a
stream error that still committed the compaction is reconciled into a success
and now carries the warning.
Also:
- Read the hook outcome under the middleware's own `_PRE_TOOL_STATE_KEY`; a
re-spelling yielded `{}` and compacted through a denial.
- Re-raise `GraphBubbleUp` out of the compaction handler, matching the hook
handler, so control-flow exceptions are not reported as failures.
- Log a failed archive write at the call site; `_aoffload_to_backend` swallows
`_ArchiveReadGuard`'s fail-closed error into a bare `None`.
- Report an empty state re-read as unconfirmed rather than "already compact".
- Log discarded hook fulfillments and tell the user those hooks will re-run.
- Check for `RemoteGraph._validate_client` explicitly so an SDK rename is not
downgraded to a generic rebind warning.
- Make `_OffloadInput` total, name the graph factories, and type the forced
compaction update as `SummarizationEvent` -- which surfaced that the
summary message's `HumanMessage` shape was never verified.
- Correct docstrings claiming the graph cannot create a synthetic tool call
(it does, in memory only), the THREAT_MODEL "no HITL interrupt" claim, and
the conflicting accounts of what an empty run input does.
Pydantic rejects `typing.TypedDict` on Python < 3.12 when langgraph routes the `/offload` operation graph's `input_schema` through its schema inspection, failing CI on 3.11 with PydanticUserError.
`RemoteAgent.astream` only yields the documented `(namespace, mode, data)`
3-tuple when `subgraphs=True`. The operation-graph driver never set it, so a
live server yielded `("updates", {...}, None)`, the unpacking bound `mode` to
the payload dict, and every chunk fell through the `mode != "updates"` filter.
Hook interrupts were therefore never detected on this path: fulfillment never
ran, the run stayed paused server-side, and the client read the unadvanced
event as "your conversation is already compact". Unit tests feed the loop
chunks directly and cannot see this, so the driver now also treats a stream
that ends without any node update as a reported failure.
The run input is authoritative for the `messages` channel against a real
server -- it replaces the conversation rather than merging by ID as the
`add_messages` reducer predicts (an in-process checkpointer does merge, which
is why only the integration test observes it). Streaming `{"messages": []}`
empties an 8-message thread and still reports success. The driver now re-reads
thread state immediately before the run rather than replaying a snapshot taken
before the concurrency guard, refuses to stream an empty replay, and the node
raises instead of rendering the resulting wipe as a no-op. THREAT_MODEL TB10
described the old, incorrect merge semantics.
A chained `/offload` whose absolute cutoff would not advance now returns before
the model call. It previously spent a summarizer request, wrote an archive
section, and replaced the committed event with a summary-of-a-summary that drops
the prior archive's `file_path` -- while the client, which keys its report on
the cutoff moving, told the user nothing had happened. The seeded path's
`_remove_offload_artifacts` had covered this; the operation graph skips it.
Also:
- Report a hook stop on the operation-graph path, and surface a failed graph
rebind from the unconfirmed-result and hook-stop paths, which previously
reported a finished offload while leaving the warning unread.
- Skip the rebind entirely while a run is left suspended, so the paused run
stays addressable.
- Warn instead of silently falling back when `checkpoint_ns` is missing inside
a run; that fallback breaks hook resumes for exactly the users who configure
hooks, and left nothing in the logs.
- Reject an offload/backend pair whose compaction middleware is bound
elsewhere, and log discarded `PreToolUse` `additionalContext`.
- Return `self` from `RemoteAgent.for_graph` for the receiver's own graph
instead of leaking a second connection pool.
- Rename `_drive_legacy_seeded_compaction` to `_drive_local_seeded_compaction`:
it is the sole `/offload` implementation for local and ACP agents, not a
deprecated path.
- Document that `PostToolUse`/`PostToolUseFailure` no longer fire for
`/offload`, that TB2 is still crossed on the local seeded path, that
`offload` is registered only for the default `graph_ref`, and that the
replay's re-serialization is not byte-identical.
Comment on lines
+15127
to
+15137
| try: | ||
| state_values = ( | ||
| await self._get_thread_state_values(thread_id) or state_values | ||
| ) | ||
| except Exception: | ||
| logger.warning( | ||
| "Could not refresh thread state before /offload; replaying " | ||
| "the pre-run snapshot instead", | ||
| exc_info=True, | ||
| ) | ||
| await remote.aensure_thread(dict(config)) |
Contributor
There was a problem hiding this comment.
🟡 Failed refresh can replay stale conversation
The refresh is specifically needed because the original snapshot can miss a turn committed before _set_agent_running(True), and the server treats the replayed messages as an authoritative replacement. On a refresh exception (or an empty result via ... or state_values), this fallback sends that known-stale snapshot anyway, deleting any messages committed in the gap. This is reachable with an externally managed/shared remote thread or any concurrent completion during the initial read. Abort the offload when the fresh state cannot be obtained rather than performing the destructive replay the refresh was intended to prevent.
(Refers to lines 15127-15137)
Your feedback helps Open SWE learn. React with 👍 or 👎 to tell us if this review comment was useful.
The operation-graph driver replays the thread's `messages` as run input, and on a real server that replay replaces the channel. The caller's snapshot is taken before `_set_agent_running(True)`, so a turn committed in the gap (shared/external threads, a concurrent completion) is missing from it. The refresh exists to catch that, but on a refresh exception (or an empty re-read via `or state_values`) the driver fell back to replaying that known-stale snapshot anyway, deleting the messages the refresh was meant to preserve. Abort the offload when current state cannot be obtained rather than performing the destructive replay.
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.
/offloadnow invokes a dedicated server-side compaction graph instead of injecting a synthetic assistant tool call.The server registers an offload graph that shares the normal agent graph’s backend and compaction middleware. The client streams that graph directly, so the operation no longer needs the seeded tool-call authorization path or the Auto/HITL bypass.
The offload tests now cover selecting the named operation graph and verify that its run context carries no synthetic tool-call ID.