Stage-bound flow runtime: resumable committing stages, sessions, capability gating#21
Open
adamw wants to merge 80 commits into
Open
Stage-bound flow runtime: resumable committing stages, sessions, capability gating#21adamw wants to merge 80 commits into
adamw wants to merge 80 commits into
Conversation
ADR 0018 records the full design (resumable committing stages bound to a branch + progress log; FlowContext/FlowControl/InStage capabilities; per-backend session existence probes; idempotent GitHub effects; replaces ADR 0013's Plan-persistence resume). plan-stage-runtime-impl.md is the task-level implementation plan (Epics A-G). ADR 0013 marked superseded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…, tuples Adds hand-written `JsonData` instances for `String`, `Int`, `Long`, `Boolean`, `Double`, `Unit`, `Option[A]`, `List[A]`, `(A, B)`, and `(A, B, C)` so that stage-bound flow scripts can use these types as return values without requiring a `Mirror` (which `JsonData.derived` needs). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add two JsonData givens as per Epic A task A2:
- `given [B <: BackendTag]: JsonData[SessionId[B]]` in `BackendTag.scala`,
delegating to `JsonData.given_JsonData_String` (avoids infinite-loop the
compiler detects when `summon[JsonData[String]]` sees this given as a
candidate inside the opaque-alias file).
- `given JsonData[PlanLike]` in `object PlanLike` (Plan.scala): a hand-written
`ConfiguredJsonValueCodec` using a `{"type":"<Name>","value":{…}}` envelope.
`JsonCodecMaker.make[PlanLike]` and `ConfiguredJsonValueCodec.derived` both
fail to add a discriminator at encode time for this sealed trait, so the
codec is written manually using the existing Plan/PlanWithBrief sub-codecs.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
JsonData.derived[PlanLike] confirmed inconsistent (encoder omits discriminator, decoder requires it) — hand-rolled codec is retained. Removes unused JsonCodecMaker import, makes the decoder key-order tolerant (buffers value bytes when type arrives later, ignores unknown keys). Removes redundant asInstanceOf brief assertion in tests; adds negative test for unknown discriminator. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Defines the two capability types from ADR 0018 §2.2: - FlowControl (marker trait extending FlowContext): authority to start a stage - InStage (opaque type, private[orca] constructor): in-stage mutation token Includes positive subtyping tests and a negative compile test (package orcacaps) verifying InStage.unsafe is inaccessible outside the orca package. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ests InStage lived in flow but tools cannot import from flow (circular dependency). Move it to tools/src/main/scala/orca/InStage.scala. Keep FlowControl in flow (renamed Capabilities.scala → FlowControl.scala). Move InStage tests to tools; simplify CapabilitiesTest to one focused subtyping test reusing TestFlowContext; strengthen negative test to assert error mentions access/visibility restriction. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…sLog) Adds the three immutable case classes for the stage-bound flow runtime progress log (Epic C, ADR 0018 §2.4), each deriving JsonData for JSON round-trip support. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds ProgressStore trait and OsProgressStore implementation (C2 of Epic C / ADR 0018 §2.4). Stores ProgressLog as JSON at <workDir>/.orca/progress-<12hexchars>.json; upserts entries by id; returns None for absent or corrupt files without throwing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds BranchNamingStrategy trait + object with security-critical slug (git-ref-safe, never empty, never starts with '-', SHA-256 fallback), issue(handle, prefix) and fromText(text) factory strategies; 24 munit tests covering all slug security properties and both strategies. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Rewrite `stage` to commit + resume against a progress log, and bind each `flow(...)` run to a feature branch + log (ADR 0018 §2.1/§2.4/§2.5). - stage[T: JsonData](name, commitMessage)(body: InStage ?=> T)(using FlowControl): resume-by-id (decode-or-rerun), run with InStage, then append entry + force-add log + one commit. Add `display` (Step only). - FlowControl gains progressStore / featureBranch / nextOccurrence (ConcurrentHashMap + AtomicInteger counter). - GitTool: forceAdd (git add -f) + resetHard (git reset --hard). - ProgressStore: expose `path` and `hashPrompt`. - flow: mandatory leading `llm`, FlowControl body, setup (stash, branch, header commit) + success/failure teardown. - ReviewLoop internal stages -> display (run under the caller's stage). - Adapt FlowTest/FlowCompilesTest/ReviewLoop tests; add StageRuntimeTest (resume replays once; crash-mid-flow keeps stage 1 committed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1. Robust teardown: body-success teardown now runs OUTSIDE the body's catch so a cleanup-commit failure cannot trigger flowTeardownFailure (resetHard). The log-file removal is best-effort (swallows NoSuchFileException); checkout(startBranch) is in a `finally` so a cleanup error never strands the user on the feature branch. 2. Remove dead featureBranch: FlowControl.featureBranch was never read. Removed from FlowControl trait, DefaultFlowContext (ctor + withDefaults), TestFlowControl, and the CapabilitiesTest stub. 3. Simplify forceAdd: GitTool.forceAdd(paths: Seq[os.Path]) → forceAdd (path: os.Path); both call sites passed a single path. Updated OsGitTool, Flow.recordAndCommit, and flowSetup. 4. Best-effort log removal: os.remove in flowTeardownSuccess now swallows NoSuchFileException (file already gone is harmless). 5. Lifecycle tests: runner/FlowLifecycleTest covers success teardown (branch + log file), failure teardown (branch + clean tree + earlier commit), and setup resume (stage body runs only once across two calls). Note: flow() calls System.exit(1) on body failure, so the failure and resume tests build the aborted-run git state manually. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…it failure Final-review polish on the CORE task: the success-teardown's cleanup commit is now wrapped so a genuine commit failure on an already-successful run is swallowed (the progress file is untracked on the branch we return to), matching the documented "cosmetic" intent. Also drop the stale featureBranch mention from the FlowControl scaladoc. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…018 §2.8) Collapse PlanLike/Plan/PlanWithBrief into one always-briefed Plan whose brief comes from the planner structured output (no .briefed turn). Remove the hand-rolled JsonData[PlanLike] codec, the .orca/plan-<hash>.md persistence/recovery API (recover, recoverOrCreate, persistComplete, implementTaskLoop, loadOrGenerate, defaultPath, hashUserPrompt), keeping the generation grid + .reviewed. parse/render now round-trip the brief and are cosmetic-only; the stage log is the sole resume mechanism. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Plan.taskPrompt now returns task.description verbatim when brief is empty, avoiding a stray leading separator line - PlanTest pins both cases: non-empty brief prepends brief+separator, empty brief yields description unchanged - JsonDataGivensTest adds a SessionId[BackendTag.ClaudeCode.type] round-trip, restoring the coverage that was lost when PlanJsonDataTest was deleted in the F1 task Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rack strays Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Whole-branch review follow-ups:
- runner/flow.scala: extract a `private[orca] runFlow` lifecycle seam that
propagates a body failure (exit-free); `flow` keeps the CLI `System.exit(1)`.
Adds genuine end-to-end FlowLifecycleTest coverage of the crash→in-place-resume
path (stage one replays once; body skipped). Resume from an arbitrary branch /
returning to header.startingBranch noted as deferred recovery hardening (R30/R32).
- ProgressStore.appendEntry: document the writeHeader precondition on the trait.
- BranchNamingStrategy.slug: drop dead `replaceAll("-+","-")`, hoist slug(prefix)
in `issue`, clamp maxLen to a lower bound.
- JsonData: note the lenient Unit decoder (Option[Unit] caveat).
- Plan.parse: reconcile docstring with its tolerance of a missing ## Brief.
- Flow.recordAndCommit: comment the deliberate runtime InStage mint.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds two LLM affordances needed by example flows and deferred features: - `LlmTool.cheap: LlmTool[B]` with a default `this` and per-backend overrides: ClaudeTool→haiku, CodexTool→mini, GeminiTool→flash, OpencodeTool→anthropicHaiku, PiTool inherits the default. - `FlowContext.llm: LlmTool[?]` accessor that returns the flow's leading model (the one passed to `flow(args, llm, ...)`). Wiring: `runFlow` passes `leadingLlm = llm` to `DefaultFlowContext.withDefaults`; the context stores and exposes it. All FlowContext implementers updated (DefaultFlowContext, TestFlowContext, TestFlowControl). 7 new tests (6 cheap + 1 llm accessor). Zero warnings. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Review follow-up: ClaudeTool/CodexTool/OpencodeTool/GeminiTool.cheap now return their concrete type (matching haiku/mini/flash/anthropicHaiku) so .cheap chains backend-specific methods without a downcast. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…get-or-create - Add SessionRecord(index, id, seed) to ProgressLog (with lenient codec to tolerate absent sessions field in old logs — strictCodecConfig would reject it) - Add ProgressStore.upsertSession (by index, last wins, no git commit) - Add FlowControl.nextSessionOccurrence() (independent AtomicInteger counter) - Implement nextSessionOccurrence in DefaultFlowContext and TestFlowControl - Add llm.session(seed)(using FlowControl): SessionId[B] extension in Session.scala — pure get-or-create keyed by occurrence index; resumes recorded id on replay Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…purity Review follow-ups: collapse the resume path's exists+find(...).get into one find (no fragile .get); reword the 'pure' doc to 'no LLM call, no commit' since the minted id is a fresh UUID. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds `LlmBackend.sessionExists(session): Boolean` (default `false`) plus non-destructive, best-effort overrides for claude (on-disk `.jsonl` file under `~/.claude/projects/<cwd-slug>/`), codex (glob `rollout-*-<id>.jsonl` under `~/.codex/sessions/`), gemini (`gemini --list-sessions` via the existing `CliRunner` seam), and opencode (`GET /session/<id>` via the existing `OpencodeHttp` seam). Pi inherits the default `false`. Each probe wraps its IO in `try/catch NonFatal => false` and is covered by per-module tests that exercise the present-true / absent-false / unavailable-false paths using injected temp dirs, stub CLI runners, or fake HTTP. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…); best-effort tests + fidelity docs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…n not live Expose sessionExists on LlmTool (default false; BaseLlmTool delegates to backend.sessionExists so all five concrete tools automatically relay the D1 probe). Add llm.runSeeded(prompt, session)(using FlowControl): when the backend session isn't live, prepends the recorded seed (from the progress log's SessionRecord) and a progress preamble (completed stage names) before calling llm.autonomous.run; if the session is live, runs prompt verbatim. Missing seed or log → no seed, not a throw. Seven targeted tests (TDD). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ten tests
- composePrimedPrompt: `---` separator emitted ONLY when context (preamble or
seed) is non-empty; no-seed + no-preamble path now returns prompt verbatim
- lookupSeed: filter empty-string seeds so `Some("")` is treated as absent
- "no recorded seed" test: asserts equality to bare prompt (not just contains)
- New test: no seed + completed stages -> preamble present, no stray separator
- Ordering assertion in lost-session test: preamble < seed < prompt by index
- sessionExists delegation tests: replaced tautological stub-override tests
with StubBasedTool (extends BaseLlmTool) backed by StubBackend (extends
LlmBackend), exercising real BaseLlmTool -> backend delegation path
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace the old planning/session API across all 6 example scripts:
- remove Plan.briefed, Plan.implementTaskLoop, inner Implementation stage
- use Plan.autonomous/interactive.from(...).value (brief always present)
- seed implementer session with plan.brief via claude.session(seed=)
- run each task in its own stage via claude.runSeeded + reviewAndFixLoop
- implement-enhanced: add .reviewed(claude), plan.taskPrompt(task), push+PR stages
- epic: claude.opus for planning, allReviewers(codex) for cross-backend review
- issue-pr: BranchNamingStrategy.issue, assessThenPlan, Verdict match, Option[Plan] stage result
- issue-pr-bugfix: Triage stage result, R8 push-after-edit in separate stages,
waitForBuild outside stage, PrHandle stage result
API fixes to support stage result types:
- Triage enum: add derives JsonData (needed for stage("Triage") return type)
- PrHandle case class: add derives JsonData (needed for stage("Push + open PR") return type)
FlowCompilesTest: add 6 compile-check defs mirroring each example's distinct
shape (ADR 0018 §3 canaries). All compile clean with zero warnings.
Known gap: `claude` accessor requires FlowContext, so `flow(args, claude)` does
not compile in standalone .sc files. Examples show the intended API; the shapes
are verified via FlowCompilesTest with a `leadModel` stub.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ainst new API The `flow(args, llm: LlmTool[?])` leading-model parameter was uncallable: the only way to name a model is the `claude`/`codex`/… accessor, which needs an in-scope FlowContext not present at the `flow(...)` argument position. Change it to a selector `leadModel: FlowContext => LlmTool[?] = _.claude` resolved against the built context, and reorder the lifecycle so the ProgressStore and DefaultFlowContext are built BEFORE branch setup (which needs `ctx.llm`). `DefaultFlowContext.llm` becomes a lazy val resolving the selector. Teardown / resume (bodySucceeded gate, success checkout-back, failure resetHard+stay) are unchanged. Rewrite the 6 examples + FlowCompilesTest to the real shapes (`flow(OrcaArgs(args))`, `_.codex` selector, issue branchNaming), update the other flow tests to the selector arg, and update ADR 0018 §2.5/§3. Examples compile clean via scala-cli against the local publishLocal snapshot. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…Model selector warning
- issue-pr.sc: move Rejection gh.writeComment into its own stage("Comment:
rejection") so it is checkpointed and not double-posted on resume; assess
stage returns (Option[Plan], String) tuple (both have JsonData) to carry
the rejection body across stage boundaries.
- FlowCompilesTest: mirror the new two-stage pattern in issuePrFlowShape canary.
- issue-pr-bugfix.sc / implement-enhanced.sc: add code comment near
gh.createPr(...).orThrow noting crash-safe idempotency depends on ADR §2.7
R24 (external-effect-idempotency task).
- Triage.scala: update docstring — Sessioned wrapper is available but flows
start a fresh seeded implementer session (.value discards the triage session);
add comment on Testable.branchName that the runtime (BranchNamingStrategy),
not this field, names the actual feature branch (field kept for LLM wire).
- flow.scala / DefaultFlowContext.scala: add WARNING scaladoc that leadModel
selector must not read ctx.llm (lazy val self-reference = infinite loop);
safe selectors are _.claude, _.codex, _.gemini, _.opencode, _.pi.
- All example version pins bumped to 0.0.14+28-eb1a8993+20260624-0842-SNAPSHOT.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e probes (R22) Persist the learned client→server session mapping in the progress log and rehydrate it on resume, so codex/gemini/opencode (server-id backends) continue the right server thread and probe the server id across a resume — closing D1's documented gap where gemini/opencode probed the client id (always false in prod). - SessionRecord gains serverId: Option[String] = None (back-compat: decodes to None when absent; the lenient ProgressLog codec already tolerates it). - SessionRegistry.serverFor read accessor (ClientToServer → mapped server id; ClaimedOnce → Some(client) iff claimed). - LlmBackend.serverFor (default None; overridden in codex/gemini/opencode); LlmTool.serverSessionId / registerServerSession surfaced via BaseLlmTool. - runSeeded persists the learned server id after a run (upsert only when changed). - flow lifecycle rehydrates the map into the leading model after setup, before the body (multi-tool flows are a documented limitation). - gemini/opencode sessionExists now resolve serverFor(client) and probe the SERVER id; None mapping → false. claude/pi unaffected (client == wire id). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…esume ClaudeBackend tracked its --session-id->--resume claim in an in-memory ClaimedOnce registry but never wired serverFor/registerSession to it (codex and pi do). So the claim was lost across processes: on resume, dispatchFor returned Fresh and re-issued --session-id for an id claude already has on disk, failing with 'Session ID ... is already in use'. Wire both hooks (claude's sessions are durable on disk, unlike pi's temp ones), so persistServerId records the claimed id and rehydrateSessions re-claims it -> a resumed task uses --resume. Found by an end-to-end implement.sc recovery run (crash after task 1, re-run). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ssion preamble Two issues surfaced by the end-to-end implement.sc runs: - cheapOneShot returned a literal ``` when the cheap model wrapped its one-line reply in a code fence, so commits landed titled ```. Skip fence lines and take the first non-blank line. - the session progress preamble claimed '(resuming an interrupted run)' even on the first task after an earlier stage in the same run. Make the wording neutral. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
codex exec resume rejects --sandbox <mode> and --full-auto ('unexpected
argument') — a resumed session inherits the sandbox it was created with; only
--dangerously-bypass-approvals-and-sandbox is still accepted. execResume reused
exec's sandboxArgs, so every resumed turn (a fix iteration, a follow-up task on
the same session, or a cross-process resume) failed. Add resumeSandboxArgs that
keeps only the accepted bypass flag.
Found by an end-to-end codex implement.sc recovery run (crash after task 1).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Moderate conciseness pass so the README reads top-to-bottom for a newcomer: - fix the resume description (progress log is at a branch-INDEPENDENT path) - dedupe the Sessions section against the capability model; tighten the dense claude tool cell; merge authoring rules 4+5 - collapse the deep reference behind <details> (Data structures list, prompt defaults, glyph legend, Ollama setup) — ~80 lines hidden by default No content removed; reference is one click away. Completeness + accuracy unchanged (verified against the live backend runs).
README: the capability machinery (FlowContext/FlowControl/InStage) is a
compile-time correctness tool, not a user feature — replace the 'capability
model' section with a behavioural note ('side effects happen inside stages; the
compiler enforces it'), and drop the using-clauses from the flow-method
signatures. Sessions/ToolSet/autoApprove stay (real user features).
AGENTS.md: refresh the stale layout (all five backends, correct subpackages,
FlowLifecycle), and add a 'stage-bound runtime' section documenting the
invariants an editor must respect — the InStage gating, progress-log/recovery,
and the per-backend session model (ClaimedOnce vs ClientToServer + the
serverFor/registerSession persist-rehydrate contract that just had two bugs).
Fix integration-test paths, fixture names, and the publishLocal module list.
Calling a gated method outside a stage produced the cryptic 'No given instance of type orca.InStage was found for parameter x$2 of method commit' — naming an internal type a flow author shouldn't need to know (and which the README now deliberately hides). Add @implicitNotFound to InStage so the error instead reads 'side-effecting calls ... must be made inside a stage(...) body. Move this call into a stage.' Kept the zero-cost opaque type (verified the annotation applies to it on Scala 3.8.3 with a clean build). Pinned the message in InStageNegativeTest. Also drop a stray '(using InStage)' from examples/implement.sc's comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ntext; misc TODO cleanups - FlowControl/FlowContext: @implicitNotFound so calling stage(...)/session(...) or a tool accessor outside a flow(...) body yields a 'must be inside flow(...)' instruction instead of 'No given instance of type orca.FlowControl'. - TextWrap: private[orca] (internal util, not public API). - cheapOneShot: moved from the flow-module extension (CheapCall.scala, deleted) to a method on LlmTool, where it belongs (resolves the CheapCall TODO). - LlmTool: drop ADR requirement-number refs from comments and clarify the client-vs-server session-id question (one type, distinguished by role).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
All `flow(OrcaArgs(...))` call sites now pass a required `leadModel` selector. Example scripts and README swap `claude.haiku` cheap-work call sites to `claude.cheap`. `withCheapModel` added to each backend's method list in the README Built-in tools table. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…maining TODOs - LlmTool.cheap now resolves a withCheapModel override (LlmConfig.cheapModel) before the backend default; opencode's default is provider-matched (openai-led → gpt-5-mini, else anthropic haiku) so incidental work doesn't pull in a second provider's auth. WithCheapModelTest covers the override. - ProgressStore: document why the log is rewritten whole (structured document with in-place upserts, small + bounded) rather than appended as JSONL.
defaultCheap read the provider prefix directly instead of OpencodeModel.split (which throws on a bare id) — cheap feeds branch naming / commit messages and must never break a flow. README opencode row notes cheap is provider-matched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…h for PR flows A no-PR flow (implement.sc) used to return HEAD to the starting branch on success, leaving the work on a feature branch the user wasn't on. Now the default is to STAY on the feature branch, so you end on the work. A throwaway branch (no substantive changes) is still deleted with HEAD back on start. Flows that open a PR pass returnToStartBranch = true to switch back afterward (issue-pr.sc, issue-pr-bugfix.sc, implement-enhanced.sc). Updated R3 + the docs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…s placement, example layout) - StreamConversation: reader/stderr workers are virtual threads (Thread.ofVirtual) — they live blocked on I/O, which is what virtual threads are for; always daemon, so no setDaemon. - AskUserInput: private[mcp] (only the in-file handler references it; the server's ServerName/ToolSlug/ToolTimeout/Hint stay private[orca] — backends consult them). - AutonomousTextCall: moved from LlmTool.scala to LlmCall.scala, beside the other call classes (AutonomousLlmCall/InteractiveLlmCall). - examples/issue-pr-bugfix.sc: the flow now reads top-to-bottom as the high-level logic; the three pipeline helpers are hoisted to top-level defs below the flow (threading issue/session). Compiles via scala-cli. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Examples hardcoded 'claude' even though LlmTool is a coding agent/harness (each drives many models), not a model. Address the implement.sc TODO: rename the flow selector leadModel -> agent, and add a top-level 'agent' accessor that resolves the lead inside a flow body so bodies are backend-agnostic (switch the selector, the whole flow follows). The accessor returns a concretely-typed LlmTool[B] even though the runtime holds the lead erased as ctx.llm: flow supplies one stable Lead carrier given whose B type member pins the backend, and "def agent(using l: Lead): LlmTool[l.B]" hands it back -- so a session from agent.session threads into agent.runSeeded and the reviewers. flow stays non-generic; existing bodies auto-wrap under the new "Lead ?=> FlowControl ?=> Unit" body type (no call-site churn beyond the rename). Converted the autonomous, tier-agnostic examples (implement.sc, implement- enhanced.sc) + the README to 'agent'. Tier-pinned (epic/issue-pr use claude.opus; bugfix uses claude.sonnet) and interactive (implement-interactive needs CanAskUser[B], per-backend) flows stay claude-committed. Updated R31 + docs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
adamw
force-pushed
the
stage-bound-flow-runtime
branch
from
282e121 to
7840757
Compare
… orca.agents
Completes the 'agent' rename: LlmTool was doubly inaccurate -- it isn't an LLM
(it's a harness over one, driving several models) and "Tool" collides with the
agent's actual tools (ToolSet/withTools/ToolUse). The codebase already leaned
this way (AgentInput/agentName/AgentTurnFailed), and 'agent' is now the in-body
accessor, so the type follows.
Types (whole-identifier renames; GitTool/FsTool/ToolSet/ToolUse untouched):
- LlmTool -> Agent, BaseLlmTool -> BaseAgent
- {Claude,Codex,Opencode,Pi,Gemini}Tool -> *Agent (+ their Default* impls)
- LlmConfig -> AgentConfig, LlmCall family -> AgentCall, LlmBackend -> AgentBackend,
LlmResult -> AgentResult
- lintLlm param -> lintAgent; test stubs StubLlm/etc -> StubAgent/etc
Package: orca.llm -> orca.agents (dir + resources moved; PromptResource paths
updated). NOTE: the package is 'agents' (plural), not 'agent' -- a singular
'orca.agent' package collides with the 'def agent' accessor in package orca
('agent is already defined as package'). Plural reads fine: the package holds the
Agent types, the singular accessor returns the one lead.
Kept as-is: the lowercase 'llm' member/param names (FlowContext.llm: Agent[?] is
the runtime's erased handle; the typed view is the 'agent' accessor). All 762
tests pass; all 6 examples compile; docs (README/AGENTS/ADR) updated.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…aming FlowContext.llm was the last vestige of the old naming -- an Agent typed `llm`, exposed on the public context. Remove it. Its only hard consumer was the in-stage default-commit-message generator (the other three -- branch naming, session rehydration, the body's Lead carrier -- live in runFlow and now take a local `val lead = agent(ctx)`). Threading the lead to the commit-gen instead would force `stage(...)(using Lead)`, which is viral (every helper that calls stage would need it). So the context exposes just the one capability that path needs: FlowContext.llm: Agent[?] -> private[orca] def cheapOneShot(prompt, fallback)(using InStage): String backed by a private `lead` in DefaultFlowContext. The raw lead is no longer on the context; bodies reach it via the typed `agent` accessor, the runtime threads its own erased handle. The selector-recursion foot-gun (`_.llm`) disappears with it. Consistency (the point): purge `llm` from every identifier that holds an agent -> `agent` (BranchNamingStrategy.resolve, Session, review params, summarisePr, Plan.from, …), `llmDriven` -> `agentDriven`, `llmCommitMessage` -> `defaultCommitMessage`, the DefaultFlowContext selector param -> `agentSelector`. Distinct, consistent names now: `agent` (selector + accessor + any agent value), `Lead` (carrier), `lead` (runtime's local erased handle), `cheapOneShot` (internal capability). All 762 tests pass; all 6 examples compile; ADR R31 + docs updated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…type member)
Replaces the `Lead` carrier + `FlowContext.cheapOneShot` workaround with a single
consistent mechanism: `agent` is now an ordinary accessor over `FlowContext`,
exactly like `claude`/`git`.
How: `flow` is generic over the leading agent's backend tag `B`, inferred from the
selector (`_.claude` => ClaudeCode) and NEVER written by callers — so
`flow(OrcaArgs(args), _.claude):` is unchanged, no type parameter surfaced. `B` is
captured at construction into a type MEMBER `FlowContext { type LeadB }` (a member,
not a parameter, so the whole `using FlowContext` surface stays unparametrised and
the body stays `FlowControl ?=> Unit`). `def agent: Agent[LeadB]` is then concretely
typed, so `agent.session` threads into `agent.runSeeded` and the reviewers.
Removed:
- the `Lead` sealed trait + carrier given, and `(using Lead)` from the accessor;
- `FlowContext.cheapOneShot` (the runtime + the in-stage commit path now use
`ctx.agent.cheapOneShot` directly).
Net win: one capability family (FlowContext/FlowControl/InStage), no 4th capability,
helpers that drive the lead take plain `(using FlowControl)` not `(using Lead)`, and
the friendly @implicitNotFound errors + user-facing syntax are untouched. Examples
unchanged; all 762 tests pass; all 6 examples compile. R31 + docs updated.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…hModel Public-API clarity pass on the methods a flow author calls. 1. Sessions — `newSession` is now `private[orca]`. It was internal-only (used by ReviewLoop for ephemeral per-reviewer ids) and just an alias for SessionId.fresh, but sat next to `session(seed)` as a second, indistinguishable way to mint a session — and silently defeated resume. Scoping it out leaves one clear public method: `session(seed)` (resume-safe), or just omit the session arg for a one-off. Updated FlowCompilesTest + doc refs. 3. Cheap family — `cheapOneShot` is now `private[orca]` (runtime-only: branch names, commit messages). The public cheap surface collapses to `cheap` (the cheaper agent) + `withCheapModel` (the builder that configures it) — a value and a builder, clearly distinct. 4. `withModel(Model)` is now public, on the four single-provider backend traits (ClaudeAgent/CodexAgent/GeminiAgent/PiAgent; BaseAgent supplies the impl). Backend-specific by design — the model id is — so it lives on the concrete agents, not the agnostic `Agent`. Fixes the codex footgun (its doc recommended `withConfig(AgentConfig(model=...))`, which wipes other config). Test stubs of the backend traits gained the method. 2. Docs — the `agent` accessor scaladoc and a new "Coding agent tools" README intro spell out the two ways to drive a model: backend-agnostic `agent` vs a concrete accessor + tier (`claude.opus`, `codex.mini`), incl. why `agent.opus` won't compile and the backend-typed-SessionId caveat. FlowContext.LeadB leads with an author-facing sentence. Removed `newSession` from the README method tables; added `withModel(Model)`. Correctness: Flow.scala referenced the removed `FlowContext.cheapOneShot` — now `fc.agent.cheapOneShot`. All 762 tests pass; all 6 examples compile. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
From a public-API review focused on flow-author clarity: - agent accessor: document the chaining order — name the model/tier first, then constraints (claude.opus.withReadOnly), since only the tier accessors return the concrete agent (builder-first like claude.withReadOnly.opus won't compile). - Session.session: note the key is positional (n-th call), like stage — keep session(...) calls unconditional + stably ordered across runs or resume shifts. - flow/runFlow: add codex/gemini override params (the DefaultFlowContext factory already accepted them; flow only exposed claude/opencode/pi — asymmetric). - Agent.name: document it (the TokensUsed agent axis, set by withName). - FlowControl @implicitNotFound: also name runSeeded (needs FlowControl too). Considered and left: the two-arg opencode withModel (README-advertised), the per-call config replace-vs-merge (documented, only reachable from backend tests), the cheap/defaultCheap/withCheapModel trio (defaultCheap is subclass-only). The full fix for builder-chaining order-independence (~40 trait overrides + stub churn) isn't worth it when every example already uses the working order. All 762 tests pass; examples compile. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace copy-pasted boilerplate across the five agent backends with three focused, behaviour-preserving helpers (rather than one leaky base class, since opencode is HTTP and claude/pi diverge from codex/gemini): - Conversations.drainAndCommit: the shared autonomous drain + AgentTurnFailed- preserving rewrap + commit-on-success, used by claude/codex/gemini/pi. - backend.AskUserEchoes: the ask_user echo-suppression id bookkeeping that was reimplemented three times (the per-backend tool matcher stays at the call site). - backend.SubprocessSpawn.open: the spawn+build two-level teardown bracket (the most leak-prone duplication), used by the four subprocess backends. AskUserSession now extends AutoCloseable so it threads through the bracket's resource list. The partial-stream dedup flag was deliberately NOT unified — the three backends scope it differently (per-turn / per-message / per-part). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…WireId The persisted SessionRecord.serverId and the serverFor/serverSessionId/ registerServerSession SPI never had two meanings — they had ONE misnamed meaning: "the wire id to resume against" (the same notion as Dispatch.wireId). The name implied a distinct server-minted id, which is true only for codex/ gemini/opencode; for claude the value IS the client id, and for pi there is none. Two scaladocs were outright wrong about this (one claimed claude keeps it None; another lumped pi with claude). Rename the read/persist surface to resumeWireId (registerServerSession → registerResumeWireId, SessionRecord.serverId → resumeWireId, persistServerId → persistResumeWireId) and fix the docs; the write primitives commitSuccess/ registerSession keep their accurate names. No behaviour change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… var reviewAndFixLoop held a method-scope `var state: ReviewLoopState` that its `evaluate()` thunk reassigned as a side effect — invisible to `fixLoop`, which drove the iteration. Extract a private generic `fixLoopWithState[S]` driver that threads caller state through its tailrec loop (carried in `Step.Continue`); `fixLoop` becomes a thin Unit-specialised delegate (public API unchanged), and `evaluate` is now `ReviewLoopState => (ReviewResult, ReviewLoopState)`. No var, no behaviour change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
git/gh return a uniform non-zero exit for both recoverable and fatal failures, so the recoverable-vs-fatal split must read their human-readable stderr (gh has no machine-readable "PR already exists"). Pull the four scattered, inline substring checks into named, documented, unit-tested predicates (OsGitTool.isPushRejection / isWorktreeAlreadyPresent, OsGitHubTool. isPrAlreadyExists / isNoCommitsToPr — the gh ones case-fold internally). Make the gh read-vs-mutate idempotency distinction explicit: split the primitive into runGhResult (raw CliResult, the single funnel) → runGh (abort+stdout) → ghRead (retry, idempotent) / ghMutate (no retry, mutating); createPr funnels through runGhResult. No behaviour change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sessions are keyed by a purely positional index, so a reordered or conditionally-skipped agent.session(...) call silently resumes the wrong recorded session. (Stages are keyed by name#occurrence and already fail-safe — a mismatch re-runs.) On the resume path, if the recorded seed at the index differs from this call's seed — the most likely symptom of a shifted call sequence — emit a warning rather than resume silently. The recorded session is still returned (re-seed remains the safe fallback, ADR 0018 §2.6). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ontract Plan.parse (a ~130-line hand-rolled inverse of render, plus PlanParseException) was used only by its own round-trip tests — resume reads the stage log, never a rendered plan (ADR 0018 §2.8). Delete it and its tests; keep Plan.render (used by Sessioned.reviewed to format the self-review prompt) with direct render tests. Document the Usage token-axis normalisation contract at the type: inputTokens is the total INCLUSIVE of cache-served tokens, cachedInputTokens <= inputTokens. All three cache-aware backends already normalise to this from different wire shapes; the per-backend arithmetic stays at each construction site. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the hand-managed daemon-thread StreamConversation base with
ForkedConversation, where every worker is an Ox fork bound to a per-turn
`supervised` scope and the code reads in execution order. The conversation's
result is the reader fork's return value (no `outcomeRef` CAS), teardown is a
lexical `finally conv.cancel()` that destroys the subprocess before the scope
joins its forks, and the bespoke EventQueue becomes an Ox Channel. The
`start()`-last-in-constructor invariant and `ensureStarted` guard are gone
(workers spawn lazily on first surface touch, after construction). All five
backends migrate to it; StreamConversation and its tests are deleted.
Key points:
- AgentBackend.runInteractive gains `(using Ox)`; AgentCall wraps interactive
drive in `supervised { … finally conv.cancel() }`. runAutonomous self-scopes.
- CliProcess.destroyForcibly (SIGKILL backstop) added so a cancel always unblocks
a reader blocked on a non-interruptible read — required now that the scope must
join the fork (daemon threads tolerated a stuck reader; structured forks don't).
- Reader fork catches-and-returns an Outcome (never throws), with a `cancelled`
flag checked before a read-exception so Ctrl-C surfaces as Left(cancelled).
- stderr-drain join runs before the failure outcome is computed, preserving
BufferedStderrDiagnostics' diagnostic context.
- Conversation.sendUserMessage dropped (it had zero call sites).
- opencode keeps its SSE source, native ask-user HTTP replies, and `/abort`.
Validated live end-to-end with implement.sc (autonomous) and
implement-interactive.sc: happy-path and crash-then-resume for both, including a
real ask_user round-trip and resume replaying the plan without re-prompting.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
OpencodeServer drained the shared `opencode serve` subprocess's stdout/stderr with two hand-managed daemon threads — the last raw `new Thread` in the tree. The documented blocker was that an Ox fork would hang scope teardown: a fork blocked on a non-interruptible `readLine` is joined before `releaseAfterScope` runs the kill that would unblock it (confirmed from Ox 1.0.2: `interruptAllAndJoinUntilCompleted` runs in the scope body's finally, `runFinalizers` after). The conversation refactor already solved this generally — destroy the process in a body `finally`, before the scope joins. Apply the same here: - OpencodeServer drains via `fork`/`forkDiscard` and exposes `shutdown()`, which `destroyForcibly()`s the process (EOFing the drains so the scope joins) and closes the client. Idempotent; no-op if never started. The bind-failure path also destroys-before-join so the stderr-tail wait can't hang. - OpencodeBackend.shutdown() forwards to the (lazily-created) server. - DefaultFlowContext.close() runs context-owned teardown; the runner calls it in the flow body's `finally` (before the supervised scope joins), next to interaction.close() — the established body-finally teardown point. No more raw threads in main sources. Covered by a new OpencodeServerTest case that leaves the drains blocked and asserts shutdown unblocks them (the scope joins without hanging); the live path stays under the gated integration test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review caught a regression: OpencodeServer.shutdown used PID-only `destroyForcibly`, but a launch wrapper (`ollama launch opencode -- serve`) forks the real serve child, which inherits the stdout/stderr pipe write-ends. Killing only the wrapper PID orphans that child, leaving the pipes open, so the drain forks' non-interruptible reads never EOF and the scope join hangs — the very deadlock the structured refactor set out to avoid (default single-process launcher was unaffected). Add `CliProcess.destroyForciblyTree` (SIGKILL descendants + root, the analogue of `sendSigIntTree`) and use it in `shutdown` and the bind-failure cleanup. Also: log (don't silently swallow) drain-fork read errors, document shutdown's synchronous-handoff assumption, and correct the test comment about what it proves. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Second review round (no critical bugs) cleanups: - Remove `CliProcess.sendSigIntTree` + its OsProcCliRunner override: dead since opencode shutdown moved to `destroyForciblyTree` (zero callers). Drops the now-unused `StreamConverters` import. - OpencodeServer.start: structurally close the (runner-can't-hit) shutdown- before-`processRef` race — re-check `stopped` after forking the drains and tree-destroy if it was latched, so a missed kill can't strand the drains. - Document the `descendants()` tree-kill assumption (catches a wrapper that stays the live parent; not a double-forking/daemonizing launcher — none exist). - Fix two stale scaladocs (OpencodeServer single-instance guarantee is the lazy `sharedServer`; ForkedConversation's EventQueue is a channel facade, not a StreamConversation back-compat shim). - Add OsProcCliRunnerTest: a real subprocess forks a child holding the pipes; asserts `destroyForciblyTree` reaps the descendant. Fails if the method ever regresses to the PID-only default — the regression the first round caught. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Implements ADR 0018 — a stage-bound flow runtime. Each
flow(...)run binds to one feature branch + one progress log;stage(...)is the committing, resumable unit of work; mutations are gated behind a compile-time capability.What this delivers
stage[T: JsonData](name)(body)commits its work + a progress-log entry as one commit, records a JSON-serialisable result, and is skipped on re-run — so interrupting a flow costs at most the in-progress stage.display(...)is progress-only;fail(...)aborts.git.commit/push,fs.write,gh.createPr/writeComment,llm.*.run) require anInStagetoken that only a stage body provides — the compiler enforces that side effects happen inside a stage (proven by a negative compile test).FlowControl <: FlowContextgates starting a stage.llm.session(seed)get-or-create (recorded in the log) +llm.runSeeded(prompt, session)— continues a live backend conversation or re-seeds (seed + progress preamble) when an existence probe says it's gone; the client→server map is persisted + rehydrated across resume. Best-effort per-backend probes (claude/codex/gemini/opencode; pi none).Planis a single always-briefed type; the old.orca/plan-<hash>.mdrecovery machinery is retired (the stage log subsumes it).gh.createPrreuses an existing open PR;gh.upsertCommentupdates a marked comment instead of duplicating.shortenPrompt, default), llm-generated commit messages, throwaway-branch auto-delete.Quality
sbt testgreen across all 8 modules (746+ tests), zero warnings under-Wunused:all/-Wvalue-discard/-Wnonunit-statement, scalafmt clean.Known follow-ups (triaged as acceptable in the final review)
gh.upsertCommentmatches by marker substring (a write-access attacker could pre-place it) — author-filter is a possible v2.flow()callsSystem.exit(1)on body failure (pre-existing); the exit-freerunFlowseam is used for failure-path tests.Supersedes ADR 0013 (persistent plans).
🤖 Generated with Claude Code