feat: split a step into model-call, per-tool, and seal activities. - #7
Draft
moedash wants to merge 33 commits into
Draft
feat: split a step into model-call, per-tool, and seal activities.#7moedash wants to merge 33 commits into
moedash wants to merge 33 commits into
Conversation
L2 splits one step into a model activity plus one activity per tool call, all appending to the same aggregate. The fence has to admit every writer holding the token the model attempt claimed, and still reject the attempt that one superseded.
The model call and the tools it asks for were one unit, so a durable executor could not put a retry policy, an approval or a budget between them. runModelCall performs the attempt, records each call as Tool.Called and hands it back with the provider's own callID, leaving the step open for whoever runs it. The prologue and the continuation decision are extracted rather than copied, so the whole-step path and the model-only path cannot drift.
runToolCall settles a single call from what the log already holds, so a dispatcher can give each one its own retry policy, timeout and approval gate instead of sharing the provider attempt's. A duplicate dispatch sees the settled result and does nothing. A retry whose side effect may already have run is reported to the model as an unknown outcome unless the tool declares itself repeatable, which is the rule the crash-resume path already follows.
The end snapshot and the file diff have to be taken after the tools have run, which is no longer the process that called the model. sealStep does that from the log: it closes any call the dispatcher never settled, then publishes Step.Ended with the settlement the attempt handed over. A repeat sees the step already closed and returns the same loop decision without publishing again, so a retry cannot end the turn a step early. The crash-resume path now shares the continuation tail rather than keeping its own copy of it.
OPENCODE_TEMPORAL_STEPPED=1 runs each step as the provider attempt, one activity per tool call, and a seal. The model-to-tools loop is workflow code now, so a retry policy, a timeout or an approval can sit between the model asking for a tool and the tool running. Each call also gets its own bounds, rather than sharing one timeout with the attempt and every other tool of the step. The supervisor is untouched: wake, interrupt, idle and continue-as-new only ever saw one runTurnStep, so the stepped mode supplies a different one. The mode rides the workflow input so a rollover keeps it. Only the model call claims the event log; the tool and seal activities publish under the token it hands back, or a step's writers would fence each other out.
Records the owner-token rule and the retry rule, since both are easy to get wrong and neither is visible from the call sites, and states the cost plainly: the model no longer overlaps with its own tools.
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
Hey! Your PR title Please update it to start with one of:
Where See CONTRIBUTING.md for details. |
The overlap loss is min(stream tail after the first tool call, tool duration), within about 10ms across a five-point sweep, and zero when the tool call is the last thing in the stream. The extra hand-offs cost about 5ms each on loopback, so roughly 10ms per step, and that grows with the distance between worker and namespace. The bench dials a mock model and a sleeping tool so the number is the overlap rather than provider variance. It is opt-in because it sleeps, and it runs on the harness's live clock: under the TestClock both the tool and the stream tail would wait forever.
|
This pull request has been automatically closed because it was not updated to meet our contributing guidelines within the 2-hour window. Feel free to open a new pull request that follows our guidelines. |
The bench showed the overlap loss is min(tail, tool duration) but left the tail itself dialled by hand. Against the live API it is 0 to 77ms across gpt-4o-mini, gpt-5-mini and gpt-5, and no model emitted any text after asking for its first tool. A single tool call is the end of the stream, so there is nothing to overlap; a tail shows up only when several tools are asked for at once. Timed from the point the runner actually forks. Measuring from when a call's id first appears rather than when its arguments finish overstated the tail by two to five times.
Fan-out, interrupt and approvals are now verified live rather than asserted: four concurrent tool activities in one step sharing the log owner, an interrupt that stops the turn and leaves the session serving, and an approval that holds only the tool waiting on it while the model call is already done. Compaction is not covered and says so. A regression test for it passed with the bug deliberately put back, so it was proving nothing and is gone rather than left as a false assurance. Also records that live streaming does not cross a process boundary. That is a property of running a standalone worker, not of stepped mode: the wake is published in-process, so a tail cannot see another process's commits. The durable log is still complete and replay returns everything.
moedash
changed the title
|
This pull request has been automatically closed because it was not updated to meet our contributing guidelines within the 2-hour window. Feel free to open a new pull request that follows our guidelines. |
Running the conformance suite against stepped mode for the first time failed 2 of 4. A provider turn that publishes no content at all never mints an assistant message, and the whole-step path only survives that because it mints one inside Step.Ended. The seal runs in another process with no publisher, so it searched the projection, found nothing, and left the turn open. The attempt now mints the message and carries its id, the same rule the tool call ids already followed. Also adds a fork-local CI workflow. Upstream's test workflow asks for Blacksmith runners this fork cannot schedule, so every run of it here has been cancelled with no runner, on every branch including dev. Nothing in this fork has ever had a CI signal. The new one is Linux-only and skips the whole-monorepo test task, because @opencode-ai/app fails a locale assertion on the untouched base branch and a job that is red on arrival is worth nothing.
|
This pull request has been automatically closed because it was not updated to meet our contributing guidelines within the 2-hour window. Feel free to open a new pull request that follows our guidelines. |
The conformance suite catches this, but it needs a dev server and an opt-in variable, so nothing in CI would have noticed the fix being reverted. Verified by putting the bug back: the test fails, and passes again once the mint returns.
The wake is published in-process and subscribers register in that same process's map, so a standalone worker's whole turn was invisible to a tail on the HTTP server: a live subscriber saw one event, the only one serve writes itself, and then silence. The tail now also re-reads on a tick, default one second and settable to 0. In-process commits still wake it instantly, so nothing that already worked got slower, and a tick with nothing new reads no rows. The test builds a second service over the same database, which is what a worker is to the server: its commits cannot reach this process's wake map, so the tail only sees them because of the tick.
A compacting step re-enters the attempt through a transition defect, and that recursion has to carry the defer flag or the step quietly runs its tools inline. Verified by dropping the flag again: the test fails. Getting compaction to fire at all took three tries, so the setup is commented. The epoch has to be created before the history is seeded (it records the sequence it was made at and the runner reads only past it), and the seeded turn has to sit in a band: over the request headroom so the attempt compacts, under context minus the summary output so compaction does not bail on its own guard.
Deleting a live session's whole working directory between two turns and watching the next turn's tool activity rebuild it from snapshot packs is the cross-host mechanism actually working, rather than asserted. Affinity stays an unimplemented optimization on top of it. The zombie window was described as a mitigation rather than a guarantee. That understated it: the settled check really is a read then a write, but a retry refuses to run a non-idempotent tool at all, so the case that would matter cannot happen. What remains is a race over which truthful outcome the model is told.
…y is. The queue name is derived from the session's directory and only workers serving that directory poll it, so a step does not pay to rebuild a tree a worker already holds. Keyed on location.directory rather than the project root, because that is the tree that has to be present, and resolved through realpath so two spellings of one tree cannot put the client and the worker on separate queues. Off by default, because it gives up the reconstruction fallback: with it on, a session whose tree has no worker waits instead of being served elsewhere. Verified all three ways, including that the work waits and then drains when the right worker comes back.
A review caught that stepped mode drops two things the log can't reconstruct. A provider error left `needsContinuation` behind, so the seal re-derived "keep going" from tool parts and the supervisor called a provider that had just failed, up to the step ceiling. And a declined permission came out of `runToolCall` as an ordinary failure, which the dispatcher swallows, so the agent carried on past a refusal. Both now travel with the result, the same rule the assistant message id already followed. The seal's "already closed" test also checks the target message rather than any in-flight one, so a retried seal can't publish a second `Step.Ended`. Other fixes from the same review: the durable tail keeps the wake as what ends it, `runToolCall` does a point read instead of decoding the whole session, the emit is uninterruptible so an interrupt can't drop a result for a tool that ran, a deleted session is a no-op rather than a run error, and the worktree queue key no longer depends on whether the path resolves locally, which differed between client and worker.
The runner raised a decline as an interrupt, but the dispatcher only rethrew on cancellation. A halt crosses the boundary as an ApplicationFailure, so isCancellation is false for it, allSettled swallowed it and the step sealed anyway. The agent kept going past a refusal, and resume resolved instead of surfacing the tagged error. The durable tail's termination is now tested. It needs the layer in its own scope and the consumer forked into an outer one, or closing the test scope kills the consumer first and a hung tail looks like a passing one. Also scopes the tool point read to its session, and stops the README claiming three unit-tested behaviours were verified live.
The bug was a mismatch between the failure `boundary.ts` raises and what the dispatcher recognises, so a test that injects its own predicate can't catch it coming back. The positive case now runs the real boundary and feeds the result to the real predicate. Change either side and it fails.
Comments and prose reflowed, code wrapped by hand in the shared files so the review fixes stay readable in the diff. Running prettier at 100 over `llm.ts` would have moved 231 lines that have nothing to do with this work. Seven long lines are left, all string literals where a break would hurt more than help.
A second review found the queue keyed on the session's directory while `worktrees.ensure` rebuilds the project tree. One tree split into a queue per subfolder, so a session started from a subdirectory waited on a queue nobody polls. Keys on the project worktree now. `step.ended` was the one event with no projector guard, and a step's writers share an owner token, so a late seal attempt could overwrite the end snapshot and file diff with a different instant. Also drops `toolPartOf`, dead since the point read, tightens a type guard that asserted an assistant message from an id match alone, and fixes the README where it described `realpath` resolution the code doesn't do.
A tool whose output could not be stored had already run, so retrying repeated the side effect and the seal finally reported the call as interrupted. The whole-step path reports the real reason and keeps going.
The attempt number counts every way a dispatch can die, including the ones that never reached the tool, so a call nobody had touched was reported to the model as an unknown outcome. Publishing Tool.Called at dispatch makes a running call mean exactly that, and a fenced dispatch now dies before the tool runs rather than after.
A tree was only rebuilt when it was missing, so a worker that ran an earlier step served files from that step. The store's newest capture is the target now, and a host-local note says whether this tree is behind it or holds work nothing else has.
The contract only covered turns with no tools, so the piece the split actually changes, one activity per call instead of one for the step, had no scenario in the suite that both modes have to pass.
A full-width turbo run had several tsgo processes killed for memory on a hosted runner, which reads as a code failure.
The tick was on for every deployment, so a single process paid one query per second per subscribed session for events that cannot exist. The durable executor's composition root asks for it now, and OPENCODE_EVENT_POLL_MS overrides either way.
The boundary read any interrupt with no cancellation as the user declining, so a runner that stopped itself for another reason reported a decision nobody made. A dispatch raises the refusal under its own type now, and the whole-step path states that its interrupt means the same thing.
A whole step closes the tools it opened on its way out; a call that is its own activity had nobody to do it, so an interrupted turn left the call recorded as running until the next prompt. A cancellation that hands the call to another attempt still leaves it alone.
A dispatch that decided not to run a tool, or lost its result, read as an ordinary success everywhere an operator looks. The outcome each one already returns says which call it was.
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.
Issue for this PR
Issues are disabled on this fork, so there is nothing to link. This is fork-internal work on the Temporal executor (
packages/temporal), not a change aimed at upstream.Type of change
What does this PR do?
It splits one step of a session turn into three activities instead of one, behind
OPENCODE_TEMPORAL_STEPPED=1. Off by default.Until now the smallest durable unit was a step: one model call plus every tool it asked for, in a single activity. Nothing could sit between the model asking for a tool and the tool running, so per-tool retry policies, per-tool timeouts, approval gates and budgets had nowhere to live.
SessionRunner.runModelCallperforms the attempt, records each call asTool.Called, and hands the calls back instead of running them.runToolCallsettles one call.sealSteptakes the end snapshot, diffs it against the start, and publishesStep.Ended. The loop between them is workflow code.The supervisor is untouched. It only ever called one
runTurnStep, so the stepped mode supplies a different one and reuses wake, interrupt, idle self-termination and continue-as-new as they are. The mode rides the workflow input, so a session that rolls over keeps it.Two things are load-bearing and were easy to get wrong:
One owner token per step, not per activity. The event log fences a publish behind the current owner, so a step's writers have to share one token. Only
runModelCallclaims; the other two publish under the token it returns. Minting per activity execution (correct when the activity is the whole step) would make them fence each other out.A call that already started is not silently repeated. Whether a side effect may have happened is read off the log:
runToolCallpublishesTool.Calledbefore it runs the tool, so a call the log shows as running is one a dispatch was already inside. Then only a tool declaringidempotentruns again; anything else reaches the model as an unknown outcome. Re-running agit pushthat may already have landed is the worse failure.The attempt number would answer the same question far less precisely: it counts every way a dispatch can die, including the ones that never reached the tool, so a call nobody had touched came back as an unknown outcome. Publishing the call at dispatch also puts the fence in front of the side effect, where it used to sit behind it.
Two smaller changes ride along because the work exposed them:
event.ts: a durable tail can now also re-read on a tick. The wake is published in-process, so a standalone worker's whole turn used to be invisible to a tail on the HTTP server. It is off unless a composition root asks for it, and only the durable executor's does: one process wakes its own subscribers, so a tick there would be a query per second per subscribed session for events that cannot exist.OPENCODE_EVENT_POLL_MSoverrides either way..github/workflows/test-fork.yml: upstream'stestworkflow asks for Blacksmith runners this fork cannot schedule, so every run of it here is cancelled with no runner, on every branch includingdev. Nothing in this fork had a CI signal.Three review rounds found things the log can't reconstruct, fixed here.
A provider error used to leave
needsContinuationbehind, so the seal re-derived "keep going" from the tool parts and the supervisor called a provider that had just failed.A declined permission needed fixing on both sides. The runner now raises it as an interrupt, and the workflow tells that failure apart from a tool that merely failed. A halt crosses the activity boundary as an
ApplicationFailure, soisCancellationis false for it andallSettledused to swallow it. The step sealed and the agent kept going past the refusal.A tool whose output could not be stored had already run. Failing the dispatch retried the side effect and the seal finally reported the call as interrupted, a reason the model cannot act on. The real reason reaches the model now, the way the whole-step path already reported it.
An interrupted turn left its tool call recorded as running until the next prompt, where a whole step closes the tools it opened on its way out. The dispatch closes its own call now, unless the cancellation means this attempt is being handed to another one, which has to find the call as it was left.
A declined permission crossed the boundary as a bare interrupt, so the boundary read every interrupt with nothing cancelling it as a refusal, including a runner that stopped itself for another reason. The refusal is named now, and the whole-step path, which still halts its local loop with an interrupt, says so where the classification happens.
A worktree was only rebuilt when it was missing, so a worker that ran an earlier step served files from that step. The store's newest capture is the target now. Two rules bound what that may touch: a host-local note says whether this tree is behind the store or holds a capture that never shipped, and only a tree built from packs is moved, so a developer's own checkout is logged and left alone. Two workers running tools of the SAME step on two hosts still cannot see each other's writes; affinity is what keeps a step's tools on one tree.
How did you verify your code works?
Live against a Temporal dev server and
gpt-5-mini, plus unit and contract tests.Live:
runModelCall,runToolCall,sealStep, thenrunModelCall,sealStep. The tool ran once.readcalls in one step became four concurrentrunToolCallactivities, started within 2ms of each other and overlapping, all four results durable, zero activity failures. A fenced write would have died and shown as a failed activity, so this is the shared owner token holding under real concurrency.echo >> counter.txtin one step andsleep 60in the next, then the serve process killed with the sleep in flight. On a fresh worker the interrupted call came back as attempt 2,counter.txtstill held one line, the model was told "The outcome of this tool call is unknown", ran the sleep itself and answered. Re-run after the dispatch evidence moved from the attempt number to the log, with the same result.sleep 90in flight,POST /interruptreturned 204, the child died, the call was closed as "Tool execution interrupted" by the interrupt itself rather than by the next turn's entry check, the workflow stayed running, and the next prompt answered normally.ActivityTaskCancelRequestedand the workflow moved on in the same second, completing two seconds later with that activity still running. The stop costs one workflow task, not the slowest tool's cleanup.*.envparks on the default agent'saskrule. While it waited,runModelCallwas already completed andrunToolCallwas the only outstanding activity. Under the whole-step mode the model call sits in that same activity for the whole human wait.role=clientserve plus a standalone worker: with theevent.tstick, a live subscriber now sees the worker'sstep.started,tool.called,tool.successandstep.ended. Before it saw one event.Tests:
packages/coreandpackages/temporalsuites pass,bun typecheckclean across every package.SessionExecutionconformance suite passes in both Temporal modes and in the local coordinator. Worth calling out: stepped mode failed two scenarios the first time it ran, on a provider turn that publishes no content at all, so no assistant message was ever minted and the seal left the turn open. That bug is fixed here and has its own unit test.Step.Ended.runToolCalldoes a point read instead of decoding the whole session. The durable tail keeps the wake as what ends it. The worktree queue key no longer depends on whether a path resolves locally, which differed between client and worker.min(stream tail after the first tool call, tool duration); against the live API that tail is 0-77ms acrossgpt-4o-mini,gpt-5-miniandgpt-5, and no model emitted any text after asking for its first tool.Screenshots / recordings
Not a UI change.
Checklist