From 33659aa1fbab08d0a5694b11fea79c33d023803f Mon Sep 17 00:00:00 2001 From: Adam Warski Date: Fri, 28 Aug 2026 10:30:02 +0000 Subject: [PATCH 1/3] Parse the three destination flags into one RunTarget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--worktree` cannot be combined with `--skip-branch` or `--keep-changes`. Until now three independent `Flag` fields on `OrcaArgs` could hold either pair, so the refusal had to be repeated at runtime: once in `OrcaArgs.parse`, and again in `flow()` for an `OrcaArgs` built by hand. The flags now become one `RunTarget` — `NewBranch(Uncommitted)`, `CurrentBranch(Uncommitted)` or `Worktree`, which carries neither a branch mode nor an `Uncommitted`. The five legal states are the only ones that can be written, so the check in `flow()` is gone. mainargs still needs raw `Flag` fields to parse argv, so those live on `RawArgs`, private to the package. `OrcaArgs.parse` is its only consumer and converts through `RunTarget.from` — the single place a refused pair is worded and rejected. `RunTarget.toArgv` renders the flags back, next to the parser that reads them, so the shell no longer spells them out itself. --- AGENTS.md | 3 +- README.md | 35 ++++---- runner/src/main/scala/orca/OrcaArgs.scala | 64 +++++-------- runner/src/main/scala/orca/RunTarget.scala | 90 +++++++++++++++++++ runner/src/main/scala/orca/flow.scala | 30 +++---- .../scala/orca/runner/FlowLifecycle.scala | 18 ++-- runner/src/test/scala/orca/OrcaArgsTest.scala | 31 ++++--- .../src/test/scala/orca/RunTargetTest.scala | 21 +++++ .../scala/orca/runner/FlowLifecycleTest.scala | 60 +++++++++---- 9 files changed, 240 insertions(+), 112 deletions(-) create mode 100644 runner/src/main/scala/orca/RunTarget.scala create mode 100644 runner/src/test/scala/orca/RunTargetTest.scala diff --git a/AGENTS.md b/AGENTS.md index 31a5c6c5..12bcf82d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -44,7 +44,8 @@ site rather than rewiring modules. The user-facing surface lives in `package orca` (the `flow` entry, the tool accessors — including `planningAgent`/`codingAgent`/`reviewAgent`, the backend-agnostic role accessors (ADR 0020) — `stage`/`display`/`fail`, -`JsonData`, `OrcaArgs`). Implementations live in +`JsonData`, `OrcaArgs` and the `RunTarget` its flags parse into). +Implementations live in focused subpackages: `orca.tools` (os-backed git/gh/fs impls + their traits), `orca.agents` + `orca.backend` (LLM SPI, `SessionSupport`, conversation driver), `orca.subprocess` (subprocess shim), `orca.sweep` diff --git a/README.md b/README.md index f1202243..3b01984a 100644 --- a/README.md +++ b/README.md @@ -325,31 +325,36 @@ Each `flow(...)` run is bound to exactly one feature branch and one progress log - **Start:** stash a dirty working tree with a warning (recover with `git stash pop`); create + checkout the feature branch; write and commit the progress log - header. `--skip-branch` (`OrcaArgs.skipBranch`) binds the run to the CURRENT + header. The three flags below reach a flow as one `OrcaArgs.target` + (`RunTarget`), which has no case for a combination orca refuses. + `--skip-branch` (`RunTarget.CurrentBranch`) binds the run to the CURRENT branch instead of creating one — for continuing work already planned on a branch — refusing on a protected branch or detached HEAD. On a FRESH `--skip-branch` run a dirty tree is tolerated, not stashed: uncommitted or untracked files (e.g. plan files left by a planning harness) stay in place for the flow, and get swept into the first stage's commit. `--keep-changes` - (`OrcaArgs.keepChanges`) does the same on a FRESH run in either branch mode — - in normal mode the files survive branch creation and reach the new branch in - that first stage commit. With neither flag, a dirty tree on a fresh run is put - to the user: stash (the default), keep, or abort; with no terminal to ask, it - stashes. A run that already has a progress log — a resume, or one too broken - to read — always stashes and ignores `--keep-changes`, so an interrupted - stage's partial work can't leak into the stage that re-runs. - `--worktree` (`OrcaArgs.worktree`) runs the whole flow in + (`Uncommitted.Keep` on either branch case) does the same on a FRESH run in + either branch mode — in normal mode the files survive branch creation and + reach the new branch in that first stage commit. With neither flag, a dirty + tree on a fresh run is put to the user: stash (the default), keep, or abort; + with no terminal to ask, it stashes. A run that already has a progress log — + a resume, or one too broken to read — always stashes and ignores + `--keep-changes`, so an interrupted stage's partial work can't leak into the + stage that re-runs. + `--worktree` (`RunTarget.Worktree`) runs the whole flow in `.orca/worktrees/` of this repository — a second checkout, keyed on the same prompt hash as the progress log, created on the first run and reused by every later one for that task. It isolates the run: two tasks can run at once without sharing a checkout or a branch. Uncommitted work does NOT come along — a worktree is made from a commit — so `--worktree` is refused with - `--skip-branch` and with `--keep-changes`. The first run in a worktree pays a - cold build (no build outputs, no dependencies, none of the untracked local - config a project may need), an editor or indexer that ignores `.gitignore` - will see the second checkout, and orca never removes it. The run starts on an - `orca-worktree-` branch orca also never deletes, so full cleanup is `git - worktree remove .orca/worktrees/` **and** `git branch -d + `--skip-branch` and with `--keep-changes`: `RunTarget.Worktree` carries + neither a branch mode nor an `Uncommitted`, so the pair is refused while argv + is parsed and has no representation after that. The first run in a worktree + pays a cold build (no build outputs, no dependencies, none of the untracked + local config a project may need), an editor or indexer that ignores + `.gitignore` will see the second checkout, and orca never removes it. The run + starts on an `orca-worktree-` branch orca also never deletes, so full + cleanup is `git worktree remove .orca/worktrees/` **and** `git branch -d orca-worktree-`; a re-run of the task refuses rather than moving that branch if it has gained commits since. Sharp edge: kept files are unprotected until that first stage commit — a diff --git a/runner/src/main/scala/orca/OrcaArgs.scala b/runner/src/main/scala/orca/OrcaArgs.scala index d7e962a2..a894ec64 100644 --- a/runner/src/main/scala/orca/OrcaArgs.scala +++ b/runner/src/main/scala/orca/OrcaArgs.scala @@ -2,8 +2,12 @@ package orca import mainargs.{Flag, ParserForClass, arg} -/** Parsed command-line arguments for the `orca` entry point. */ -case class OrcaArgs( +/** The argv shape mainargs parses: one raw flag per `--`-spelled option, + * including the `--worktree` combinations orca refuses. [[OrcaArgs.parse]] is + * its only consumer, and turns the three run-destination flags into a + * [[RunTarget]] — so nothing beyond the parse boundary holds them separately. + */ +private[orca] case class RawArgs( @arg(positional = true, doc = "task description") userPrompt: String = "", @arg(doc = "print a stack trace if the flow aborts") @@ -20,53 +24,31 @@ case class OrcaArgs( worktree: Flag = Flag() ) -object OrcaArgs: - given ParserForClass[OrcaArgs] = ParserForClass[OrcaArgs] - - private val worktreeWithSkipBranchRefusal: String = - "--worktree cannot be combined with --skip-branch: --skip-branch runs on " + - "the branch checked out now, and git will not check that branch out a " + - "second time in a new worktree" - - private val worktreeWithKeepChangesRefusal: String = - "--worktree cannot be combined with --keep-changes: --keep-changes works " + - "on uncommitted files, which stay behind in the invoking checkout — a " + - "worktree is created from a commit and starts clean" - - /** Why these flags cannot be combined, or `None` when they can — asked of a - * whole `OrcaArgs`, so neither [[parse]] nor `flow()` spells out a triple of - * same-typed booleans that a transposition would silently reorder. - */ - private[orca] def worktreeRefusal(args: OrcaArgs): Option[String] = - worktreeRefusal( - worktree = args.worktree.value, - skipBranch = args.skipBranch.value, - keepChanges = args.keepChanges.value - ) +/** Parsed command-line arguments for the `orca` entry point. */ +case class OrcaArgs( + userPrompt: String = "", + verbose: Boolean = false, + target: RunTarget = RunTarget.NewBranch(Uncommitted.Stash) +) - /** The same question over loose booleans, for the shell — it holds - * `FlowFlags`, not an `OrcaArgs`, and refuses the pair before it spawns a - * flow at all. What must not drift is which pairs are refused, not only how - * each refusal reads. - */ - private[orca] def worktreeRefusal( - worktree: Boolean, - skipBranch: Boolean, - keepChanges: Boolean - ): Option[String] = - if !worktree then None - else if skipBranch then Some(worktreeWithSkipBranchRefusal) - else if keepChanges then Some(worktreeWithKeepChangesRefusal) - else None +object OrcaArgs: + private given ParserForClass[RawArgs] = ParserForClass[RawArgs] /** Parse the given argv or return a human-readable error — including for a * contradictory `--worktree` pair, refused here so it fails at parse, before * the banner and before anything touches git. */ def parse(args: Seq[String]): Either[String, OrcaArgs] = - summon[ParserForClass[OrcaArgs]] + summon[ParserForClass[RawArgs]] .constructEither(args.toList) - .flatMap(parsed => worktreeRefusal(parsed).toLeft(parsed)) + .flatMap: raw => + RunTarget + .from( + worktree = raw.worktree.value, + skipBranch = raw.skipBranch.value, + keepChanges = raw.keepChanges.value + ) + .map(OrcaArgs(raw.userPrompt, raw.verbose.value, _)) /** Overload for scala-cli flow scripts, whose top-level `args` is * `Array[String]`. Throws `OrcaFlowException` on a parse failure. diff --git a/runner/src/main/scala/orca/RunTarget.scala b/runner/src/main/scala/orca/RunTarget.scala new file mode 100644 index 00000000..7891d882 --- /dev/null +++ b/runner/src/main/scala/orca/RunTarget.scala @@ -0,0 +1,90 @@ +package orca + +/** What a run does with uncommitted and untracked files it finds in the working + * tree at start (`--keep-changes` asks for [[Uncommitted.Keep]]). + */ +enum Uncommitted: + /** Stash them, so the run starts from committed content. */ + case Stash + + /** Leave them in place for the flow to work on and commit. */ + case Keep + +/** Where a run's work goes: which branch it commits onto, and — in the cases + * where the question arises at all — what happens to uncommitted files. + * + * This is what `--skip-branch`, `--keep-changes` and `--worktree` become once + * argv is parsed. `--worktree` combines with neither of the other two (a + * worktree is created from a commit, so it starts clean and checks out a + * branch of its own), so [[Worktree]] carries no `Uncommitted` and is not a + * branch mode: the refused combinations have no representation here, and + * [[RunTarget.from]] — the only way in from raw flags — is where they are + * refused. + */ +enum RunTarget: + /** A branch orca creates in the invoking checkout — the default. */ + case NewBranch(uncommitted: Uncommitted) + + /** The branch checked out now; the flow commits onto it (`--skip-branch`). */ + case CurrentBranch(uncommitted: Uncommitted) + + /** A separate checkout under `.orca/worktrees/` (`--worktree`). */ + case Worktree + + /** Runs on the branch already checked out, instead of creating one. */ + def skipBranch: Boolean = this match + case CurrentBranch(_) => true + case NewBranch(_) | Worktree => false + + /** Leaves uncommitted files in the tree instead of stashing them. */ + def keepChanges: Boolean = this match + case NewBranch(uncommitted) => uncommitted == Uncommitted.Keep + case CurrentBranch(uncommitted) => uncommitted == Uncommitted.Keep + case Worktree => false + + /** The flags [[OrcaArgs]] parses back, in the order the shell appends them + * after `--` when it spawns a flow child. Rendering lives next to the parser + * so both directions of one flag spelling are in a single file. + */ + def toArgv: Seq[String] = this match + case NewBranch(Uncommitted.Stash) => Nil + case NewBranch(Uncommitted.Keep) => Seq("--keep-changes") + case CurrentBranch(Uncommitted.Stash) => Seq("--skip-branch") + case CurrentBranch(Uncommitted.Keep) => + Seq("--skip-branch", "--keep-changes") + case Worktree => Seq("--worktree") + +object RunTarget: + + private val worktreeWithSkipBranchRefusal: String = + "--worktree cannot be combined with --skip-branch: --skip-branch runs on " + + "the branch checked out now, and git will not check that branch out a " + + "second time in a new worktree" + + private val worktreeWithKeepChangesRefusal: String = + "--worktree cannot be combined with --keep-changes: --keep-changes works " + + "on uncommitted files, which stay behind in the invoking checkout — a " + + "worktree is created from a commit and starts clean" + + /** The single conversion from the raw flags an argv parser produces: a flow's + * own argv ([[OrcaArgs.parse]]) and `orca run`'s (the shell has its own + * parser for the same three flags) both come through here, so neither the + * wording of a refusal nor the set of refused pairs can drift. A refused + * pair is a message, never a value — which is what keeps it out of every + * type below this point. + */ + def from( + worktree: Boolean, + skipBranch: Boolean, + keepChanges: Boolean + ): Either[String, RunTarget] = + val uncommitted = + if keepChanges then Uncommitted.Keep else Uncommitted.Stash + if !worktree then + Right( + if skipBranch then CurrentBranch(uncommitted) + else NewBranch(uncommitted) + ) + else if skipBranch then Left(worktreeWithSkipBranchRefusal) + else if keepChanges then Left(worktreeWithKeepChangesRefusal) + else Right(Worktree) diff --git a/runner/src/main/scala/orca/flow.scala b/runner/src/main/scala/orca/flow.scala index 69d8106c..9d651d21 100644 --- a/runner/src/main/scala/orca/flow.scala +++ b/runner/src/main/scala/orca/flow.scala @@ -120,9 +120,10 @@ private enum FlowOutcome: * reused after, and everything below it — git, the progress log, the session * manifest — uses that directory instead. A refusal (no repository, no * commits, something orca did not create already at the path) ends the run - * before any of that starts. `--worktree` cannot be combined with `skipBranch` - * or `keepChanges`; the pair is refused here as well as in `OrcaArgs.parse`, - * since an `OrcaArgs` built by hand never passed through the parser. + * before any of that starts. `--worktree` combines with neither + * `--skip-branch` nor `--keep-changes`, and `RunTarget` — what `args` carries + * those three flags as — has no case for either pair, so the refusal happens + * once, converting argv (`OrcaArgs.parse`). * * Overrides default to `None` so the runtime can build the default lazily — * `TerminalInteraction` in particular takes the resolved `workDir`, which @@ -173,17 +174,13 @@ def flow( // the session manifest below; everything downstream is handed `workDir` // explicitly, so it is the single value to change. def resolveRunDir(): Either[String, os.Path] = - OrcaArgs - .worktreeRefusal(args) - .toLeft(()) - .flatMap: _ => - if !args.worktree.value then Right(workDir) - else - // Resolution can throw as well as refuse — a symlinked or unwritable - // `.orca`, a git that won't start. One `Left` shape for every outcome - // keeps the reporting below the only way out. - try WorktreeRun.resolve(workDir, args.userPrompt) - catch case NonFatal(e) => Left(TextUtil.throwableMessage(e)) + if args.target != RunTarget.Worktree then Right(workDir) + else + // Resolution can throw as well as refuse — a symlinked or unwritable + // `.orca`, a git that won't start. One `Left` shape for every outcome + // keeps the reporting below the only way out. + try WorktreeRun.resolve(workDir, args.userPrompt) + catch case NonFatal(e) => Left(TextUtil.throwableMessage(e)) // The run proper. Everything under here uses `dir`, never `workDir`. def runIn(dir: os.Path): FlowOutcome = @@ -263,7 +260,8 @@ def flow( FlowOutcome.Failed case Right(dir) => val where = - if args.worktree.value then s"$dir (worktree)" else dir.toString + if args.target == RunTarget.Worktree then s"$dir (worktree)" + else dir.toString flowLog.info( "orca {} starting (workDir={})", OrcaBanner.version, @@ -315,7 +313,7 @@ private[orca] def runFlow( wiring: FlowWiring = FlowWiring(), pricing: PriceList = Pricing.default )(body: FlowControl ?=> Unit): Unit = - val debug = OrcaDebug.enabled || args.verbose.value + val debug = OrcaDebug.enabled || args.verbose // Acquire both guards before `supervised:` (neither needs an `Ox` scope) so a // violation is caught before any git mutation. See [[FlowLock]] for the // two-layer rationale and release-ordering symmetry. diff --git a/runner/src/main/scala/orca/runner/FlowLifecycle.scala b/runner/src/main/scala/orca/runner/FlowLifecycle.scala index 48d956b8..2ad6ef13 100644 --- a/runner/src/main/scala/orca/runner/FlowLifecycle.scala +++ b/runner/src/main/scala/orca/runner/FlowLifecycle.scala @@ -524,8 +524,8 @@ object FlowLifecycle: val dirtyCount = git.dirtyPaths().size val facts = DirtyTreeFacts( ownLogPresent = ownLog != ProgressStore.LoadResult.Absent, - skipBranch = args.skipBranch.value, - keepChanges = args.keepChanges.value, + skipBranch = args.target.skipBranch, + keepChanges = args.target.keepChanges, dirtyCount = dirtyCount ) DirtyTreePolicy.decide(facts, tty, ask) match @@ -548,7 +548,7 @@ object FlowLifecycle: WorkspaceWrite ): UntrackedFiles = if dirtyCount > 0 then - if args.keepChanges.value then + if args.target.keepChanges then emit( OrcaEvent.Step( "ignoring --keep-changes: this task already has a progress " + @@ -614,8 +614,8 @@ object FlowLifecycle: /** Shared by [[bindBranch]]'s corrupt-log and absent-log arms: resolve + * create a fresh branch via [[freshRun]], then mint [[BranchMode]] from - * `args.skipBranch` (skip-branch mode never creates a branch, so it reads - * as `Reused`). + * `args.target` (skip-branch mode never creates a branch, so it reads as + * `Reused`). */ private def freshBinding( startBranch: String, @@ -644,7 +644,7 @@ object FlowLifecycle: BranchBinding( branch, startBranch, - if args.skipBranch.value then BranchMode.Reused + if args.target.skipBranch then BranchMode.Reused else BranchMode.Created, headAtBinding ) @@ -913,7 +913,7 @@ object FlowLifecycle: * "main" this time. [[createFreshBranch]] applies the same policy to a * git-level collision. * - * `args.skipBranch` skips all of this: the run binds to `startBranch` + * A `CurrentBranch` target skips all of this: the run binds to `startBranch` * verbatim via [[reuseCurrentBranch]] instead. */ private def freshRun( @@ -931,7 +931,7 @@ object FlowLifecycle: emit: OrcaEvent => Unit )(using InStage, WorkspaceWrite): FeatureBranch = val branch = - if args.skipBranch.value then + if args.target.skipBranch then reuseCurrentBranch(startBranch, protectedBranches) else val strategy = @@ -970,7 +970,7 @@ object FlowLifecycle: branch = branch.value, promptHash = ProgressStore.hashPrompt(args.userPrompt), branchMode = - if args.skipBranch.value then BranchMode.Reused + if args.target.skipBranch then BranchMode.Reused else BranchMode.Created, userPrompt = Some(args.userPrompt), flowName = flowName, diff --git a/runner/src/test/scala/orca/OrcaArgsTest.scala b/runner/src/test/scala/orca/OrcaArgsTest.scala index c07c3f3d..6a0fdd43 100644 --- a/runner/src/test/scala/orca/OrcaArgsTest.scala +++ b/runner/src/test/scala/orca/OrcaArgsTest.scala @@ -1,11 +1,14 @@ package orca -import mainargs.Flag - class OrcaArgsTest extends munit.FunSuite: test("parses an empty argv into defaults (empty prompt, verbose off)"): - assertEquals(OrcaArgs.parse(Nil), Right(OrcaArgs("", Flag()))) + assertEquals( + OrcaArgs.parse(Nil), + Right( + OrcaArgs("", verbose = false, RunTarget.NewBranch(Uncommitted.Stash)) + ) + ) test("a single positional argument becomes userPrompt"): val result = OrcaArgs @@ -13,7 +16,7 @@ class OrcaArgsTest extends munit.FunSuite: .toOption .getOrElse(fail("expected successful parse")) assertEquals(result.userPrompt, "implement feature X") - assertEquals(result.verbose.value, false) + assertEquals(result.verbose, false) test("--verbose flag sets verbose = true"): val result = OrcaArgs @@ -21,27 +24,33 @@ class OrcaArgsTest extends munit.FunSuite: .toOption .getOrElse(fail("expected successful parse")) assertEquals(result.userPrompt, "do the thing") - assertEquals(result.verbose.value, true) + assertEquals(result.verbose, true) - test("--skip-branch flag sets skipBranch = true; absent defaults to false"): + test("--skip-branch targets the current branch; absent, a new one"): val result = OrcaArgs .parse(Seq("--skip-branch", "do the thing")) .toOption .getOrElse(fail("expected successful parse")) assertEquals(result.userPrompt, "do the thing") - assertEquals(result.skipBranch.value, true) + assertEquals(result.target, RunTarget.CurrentBranch(Uncommitted.Stash)) + assertEquals( + OrcaArgs.parse(Seq("do the thing")).map(_.target), + Right(RunTarget.NewBranch(Uncommitted.Stash)) + ) + + test("--keep-changes keeps uncommitted files on the chosen branch"): assertEquals( - OrcaArgs.parse(Seq("do the thing")).map(_.skipBranch.value), - Right(false) + OrcaArgs.parse(Seq("--keep-changes", "do the thing")).map(_.target), + Right(RunTarget.NewBranch(Uncommitted.Keep)) ) - test("--worktree flag sets worktree = true"): + test("--worktree targets a worktree"): val result = OrcaArgs .parse(Seq("--worktree", "do the thing")) .toOption .getOrElse(fail("expected successful parse")) assertEquals(result.userPrompt, "do the thing") - assertEquals(result.worktree.value, true) + assertEquals(result.target, RunTarget.Worktree) test("--worktree with --skip-branch is refused, naming both flags"): assertRefused(Seq("--worktree", "--skip-branch", "x"), "--skip-branch") diff --git a/runner/src/test/scala/orca/RunTargetTest.scala b/runner/src/test/scala/orca/RunTargetTest.scala new file mode 100644 index 00000000..43ac86bb --- /dev/null +++ b/runner/src/test/scala/orca/RunTargetTest.scala @@ -0,0 +1,21 @@ +package orca + +class RunTargetTest extends munit.FunSuite: + + test("toArgv renders each destination as the flags OrcaArgs parses back"): + assertEquals( + List( + RunTarget.NewBranch(Uncommitted.Stash), + RunTarget.NewBranch(Uncommitted.Keep), + RunTarget.CurrentBranch(Uncommitted.Stash), + RunTarget.CurrentBranch(Uncommitted.Keep), + RunTarget.Worktree + ).map(_.toArgv), + List( + Nil, + Seq("--keep-changes"), + Seq("--skip-branch"), + Seq("--skip-branch", "--keep-changes"), + Seq("--worktree") + ) + ) diff --git a/runner/src/test/scala/orca/runner/FlowLifecycleTest.scala b/runner/src/test/scala/orca/runner/FlowLifecycleTest.scala index f6785738..0519d3bf 100644 --- a/runner/src/test/scala/orca/runner/FlowLifecycleTest.scala +++ b/runner/src/test/scala/orca/runner/FlowLifecycleTest.scala @@ -6,7 +6,9 @@ import orca.{ FlowContext, OrcaArgs, OrcaDir, + RunTarget, StackSettings, + Uncommitted, WorkspaceWrite, runFlow, stage, @@ -49,7 +51,6 @@ import orca.tools.{ UntrackedFiles, Worktrees } -import mainargs.Flag import ox.supervised import ox.either.orThrow @@ -129,7 +130,7 @@ class FlowLifecycleTest extends munit.FunSuite: animated = false ) flow( - args = OrcaArgs(prompt, worktree = Flag(true)), + args = OrcaArgs(prompt, target = RunTarget.Worktree), stackSettings = Some(StackSettings.empty), claude = Some(_ => StubAgent.claude), workDir = workDir, @@ -704,7 +705,10 @@ class FlowLifecycleTest extends munit.FunSuite: val _ = writeForeignLog(workDir, branch = "my-work") val thrown = intercept[orca.OrcaFlowException]: val _ = FlowLifecycle.setup( - args = OrcaArgs("a brand new task", skipBranch = Flag(true)), + args = OrcaArgs( + "a brand new task", + target = RunTarget.CurrentBranch(Uncommitted.Stash) + ), agent = StubAgent.claude, git = git, workDir = workDir, @@ -881,7 +885,8 @@ class FlowLifecycleTest extends munit.FunSuite: val prompt = "skip-branch starting commit" val store = ProgressStore.default(workDir, prompt) val setup = FlowLifecycle.setup( - args = OrcaArgs(prompt, skipBranch = Flag(true)), + args = + OrcaArgs(prompt, target = RunTarget.CurrentBranch(Uncommitted.Stash)), agent = StubAgent.claude, git = git, workDir = workDir, @@ -2216,7 +2221,8 @@ class FlowLifecycleTest extends munit.FunSuite: animated = false ) flow( - args = OrcaArgs(prompt, skipBranch = Flag(true)), + args = + OrcaArgs(prompt, target = RunTarget.CurrentBranch(Uncommitted.Stash)), stackSettings = Some(StackSettings.empty), claude = Some(_ => StubAgent.claude), workDir = workDir, @@ -2266,7 +2272,10 @@ class FlowLifecycleTest extends munit.FunSuite: animated = false ) runFlow( - args = OrcaArgs(prompt, skipBranch = Flag(true)), + args = OrcaArgs( + prompt, + target = RunTarget.CurrentBranch(Uncommitted.Stash) + ), stackSettings = Some(StackSettings.empty), wiring = FlowWiring(claude = Some(_ => StubAgent.claude)), workDir = workDir, @@ -2313,7 +2322,10 @@ class FlowLifecycleTest extends munit.FunSuite: animated = false ) runFlow( - args = OrcaArgs(prompt, skipBranch = Flag(true)), + args = OrcaArgs( + prompt, + target = RunTarget.CurrentBranch(Uncommitted.Stash) + ), stackSettings = Some(StackSettings.empty), wiring = FlowWiring(claude = Some(_ => StubAgent.claude)), workDir = workDir, @@ -2353,7 +2365,10 @@ class FlowLifecycleTest extends munit.FunSuite: animated = false ) runFlow( - args = OrcaArgs(prompt, skipBranch = Flag(true)), + args = OrcaArgs( + prompt, + target = RunTarget.CurrentBranch(Uncommitted.Stash) + ), stackSettings = Some(StackSettings.empty), wiring = FlowWiring(claude = Some(_ => StubAgent.claude)), workDir = workDir, @@ -2437,7 +2452,8 @@ class FlowLifecycleTest extends munit.FunSuite: animated = false ) flow( - args = OrcaArgs(prompt, skipBranch = Flag(true)), + args = + OrcaArgs(prompt, target = RunTarget.CurrentBranch(Uncommitted.Stash)), stackSettings = Some(StackSettings.empty), claude = Some(_ => StubAgent.claude), workDir = workDir, @@ -2502,7 +2518,8 @@ class FlowLifecycleTest extends munit.FunSuite: animated = false ) flow( - args = OrcaArgs(prompt, skipBranch = Flag(true)), + args = + OrcaArgs(prompt, target = RunTarget.CurrentBranch(Uncommitted.Stash)), stackSettings = Some(StackSettings.empty), claude = Some(_ => StubAgent.claude), workDir = workDir, @@ -2572,7 +2589,8 @@ class FlowLifecycleTest extends munit.FunSuite: animated = false ) flow( - args = OrcaArgs(prompt, skipBranch = Flag(true)), + args = + OrcaArgs(prompt, target = RunTarget.CurrentBranch(Uncommitted.Stash)), stackSettings = Some(StackSettings.empty), claude = Some(_ => StubAgent.claude), workDir = workDir, @@ -2608,7 +2626,8 @@ class FlowLifecycleTest extends munit.FunSuite: animated = false ) flow( - args = OrcaArgs(prompt, skipBranch = Flag(true)), + args = + OrcaArgs(prompt, target = RunTarget.CurrentBranch(Uncommitted.Stash)), stackSettings = Some(StackSettings.empty), claude = Some(_ => StubAgent.claude), workDir = workDir, @@ -2658,7 +2677,8 @@ class FlowLifecycleTest extends munit.FunSuite: animated = false ) flow( - args = OrcaArgs(prompt, skipBranch = Flag(true)), + args = + OrcaArgs(prompt, target = RunTarget.CurrentBranch(Uncommitted.Stash)), stackSettings = Some(StackSettings.empty), claude = Some(_ => StubAgent.claude), workDir = workDir, @@ -2701,7 +2721,7 @@ class FlowLifecycleTest extends munit.FunSuite: animated = false ) flow( - args = OrcaArgs(prompt, keepChanges = Flag(true)), + args = OrcaArgs(prompt, target = RunTarget.NewBranch(Uncommitted.Keep)), stackSettings = Some(StackSettings.empty), claude = Some(_ => StubAgent.claude), workDir = workDir, @@ -2781,7 +2801,7 @@ class FlowLifecycleTest extends munit.FunSuite: animated = false ) flow( - args = OrcaArgs(prompt, keepChanges = Flag(true)), + args = OrcaArgs(prompt, target = RunTarget.NewBranch(Uncommitted.Keep)), stackSettings = Some(StackSettings.empty), claude = Some(_ => StubAgent.claude), workDir = workDir, @@ -2923,7 +2943,8 @@ class FlowLifecycleTest extends munit.FunSuite: animated = false ) runFlow( - args = OrcaArgs(prompt, keepChanges = Flag(true)), + args = + OrcaArgs(prompt, target = RunTarget.NewBranch(Uncommitted.Keep)), stackSettings = Some(StackSettings.empty), wiring = FlowWiring(claude = Some(_ => StubAgent.claude)), workDir = workDir, @@ -2962,7 +2983,7 @@ class FlowLifecycleTest extends munit.FunSuite: animated = false ) flow( - args = OrcaArgs(prompt, keepChanges = Flag(true)), + args = OrcaArgs(prompt, target = RunTarget.NewBranch(Uncommitted.Keep)), stackSettings = Some(StackSettings.empty), claude = Some(_ => StubAgent.claude), workDir = workDir, @@ -3003,7 +3024,8 @@ class FlowLifecycleTest extends munit.FunSuite: animated = false ) flow( - args = OrcaArgs(prompt, skipBranch = Flag(true)), + args = + OrcaArgs(prompt, target = RunTarget.CurrentBranch(Uncommitted.Stash)), stackSettings = Some(StackSettings.empty), claude = Some(_ => StubAgent.claude), workDir = workDir, @@ -3503,7 +3525,7 @@ class FlowLifecycleTest extends munit.FunSuite: animated = false ) flow( - args = OrcaArgs(prompt, keepChanges = Flag(true)), + args = OrcaArgs(prompt, target = RunTarget.NewBranch(Uncommitted.Keep)), stackSettings = Some(StackSettings.empty), claude = Some(_ => StubAgent.claude), workDir = workDir, From 9bf0b36e48bdf774129133abe9f0032ea2158ef4 Mon Sep 17 00:00:00 2001 From: Adam Warski Date: Fri, 28 Aug 2026 10:30:07 +0000 Subject: [PATCH 2/3] Give the shell one run target instead of three flags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `FlowFlags` carried the same three independent booleans as `OrcaArgs`, built at four launch sites, so any of them could hand the flow child a combination it would refuse after the spawn. It now carries a `RunTarget`, which cannot express one. The shell's own `RunTarget` (menu-only, same three destinations) is dropped for the shared type. `orca run` converts its argv flags with `RunTarget.from` and refuses a bad pair before resolving anything, as before — but now that conversion is the only way to reach a launch path, rather than a check a call site could skip. --- adr/0021-orca-shell.md | 6 +++ shell/src/main/scala/orca/shell/Main.scala | 34 ++++++------- .../src/main/scala/orca/shell/MainMenu.scala | 48 +++++++------------ .../orca/shell/actions/AuthorAction.scala | 6 +-- shell/src/main/scala/orca/shell/cli/Cli.scala | 11 +++-- .../main/scala/orca/shell/cli/RunCli.scala | 27 +++++------ .../scala/orca/shell/run/FlowLauncher.scala | 42 +++++++--------- .../src/test/scala/orca/shell/MainTest.scala | 41 ++++++---------- .../orca/shell/actions/AuthorActionTest.scala | 5 +- .../orca/shell/actions/RunActionTest.scala | 5 +- .../orca/shell/run/FlowLauncherTest.scala | 33 ++++++------- 11 files changed, 109 insertions(+), 149 deletions(-) diff --git a/adr/0021-orca-shell.md b/adr/0021-orca-shell.md index a1d7da5a..aeaeafe4 100644 --- a/adr/0021-orca-shell.md +++ b/adr/0021-orca-shell.md @@ -905,6 +905,12 @@ harness/model/yolo flag exists for either. > (`OrcaArgs.worktreeRefusal`), so neither the wording nor the set of refused > pairs can drift. +> **Amendment (2026-08-28).** That shared decision is now `RunTarget.from`, +> which returns the run's destination as one value instead of an optional +> refusal string. Both parsers convert their raw flags through it, so a refused +> pair cannot be carried past argv — `FlowFlags` holds a `RunTarget`, and every +> launch path takes one. + Both entry points call a shared `orca.shell.actions` package (`FlowResolution`, `RunAction`, `ViewAction`, `EditAction`, `AuthorAction`, `SessionAction`, `ConfigAction`, `StackAction`): each takes fully-resolved parameters and does diff --git a/shell/src/main/scala/orca/shell/Main.scala b/shell/src/main/scala/orca/shell/Main.scala index ec8ed96e..1cfc084c 100644 --- a/shell/src/main/scala/orca/shell/Main.scala +++ b/shell/src/main/scala/orca/shell/Main.scala @@ -1,6 +1,7 @@ package orca.shell import org.jline.terminal.Terminal +import orca.{RunTarget, Uncommitted} import orca.settings.GlobalSettings import orca.shell.actions.{ AuthorAction, @@ -390,12 +391,7 @@ object Main: target <- promptRunTarget(ui) do val opts = RunAction.RunOptions( - flags = FlowFlags( - verbose = false, - skipBranch = target.skipBranch, - keepChanges = false, - worktree = target.worktree - ), + flags = FlowFlags(verbose = false, target = target), fallback = FallbackPolicy.Ask(ui) ) runAction(flow, task, opts, workDir, terminal).discard @@ -408,13 +404,13 @@ object Main: * ([[RunAction.run]]/[[orca.shell.run.FlowLauncher]]): no branch prompt — * the resume happens on the current branch by design, and a resumed log's * `bindBranch` (`FlowLifecycle`) ignores `skipBranch` entirely, so the - * all-false flags passed here are exactly as correct as any other value - * would be — `worktree` included: the run is launched IN `run.dir`, the - * directory its log was found in, which is what makes this a resume. The - * flag would instead re-derive a path from the task text, which is the same - * directory only when the log happened to be in an orca-made worktree of - * that exact prompt. `runAction` is injectable, [[AuthorAction]]-style, so a - * test can record the call instead of spawning a real subprocess. + * default target passed here is exactly as correct as any other would be — + * `Worktree` included: the run is launched IN `run.dir`, the directory its + * log was found in, which is what makes this a resume. That target would + * instead re-derive a path from the task text, which is the same directory + * only when the log happened to be in an orca-made worktree of that exact + * prompt. `runAction` is injectable, [[AuthorAction]]-style, so a test can + * record the call instead of spawning a real subprocess. */ private[shell] def resumeInterruptedRun( ui: ShellUi, @@ -438,9 +434,7 @@ object Main: RunAction.RunOptions( flags = FlowFlags( verbose = false, - skipBranch = false, - keepChanges = false, - worktree = false + target = RunTarget.NewBranch(Uncommitted.Stash) ), fallback = FallbackPolicy.Ask(ui) ) @@ -467,15 +461,15 @@ object Main: * `CurrentBranch` is skip-branch mode (ADR 0018 amendment): the * handoff-from-harness case, where the user already planned work on a branch * carrying plan files. `Worktree` is `--worktree`, which orca refuses - * together with `--skip-branch` (`OrcaArgs.worktreeRefusal`) — one choice - * cannot express that pair, where two independent confirms could. - * `private[shell]` so a scripted-UI test can drive it directly. + * together with `--skip-branch` — one choice cannot express that pair, where + * two independent confirms could. `private[shell]` so a scripted-UI test can + * drive it directly. */ private[shell] def promptRunTarget(ui: ShellUi): Option[RunTarget] = ui.select( "Where should this run's work go?", MainMenu.runTargetChoices, - preselect = Some(RunTarget.NewBranch) + preselect = Some(RunTarget.NewBranch(Uncommitted.Stash)) ) match case UiOutcome.Cancelled => None case UiOutcome.Selected(target) => Some(target) diff --git a/shell/src/main/scala/orca/shell/MainMenu.scala b/shell/src/main/scala/orca/shell/MainMenu.scala index 6bba8be9..678a3fd5 100644 --- a/shell/src/main/scala/orca/shell/MainMenu.scala +++ b/shell/src/main/scala/orca/shell/MainMenu.scala @@ -1,5 +1,6 @@ package orca.shell +import orca.{RunTarget, Uncommitted} import orca.shell.resume.InterruptedRun import orca.shell.ui.Choice import orca.util.TextUtil @@ -20,32 +21,6 @@ private[shell] enum MenuItem: private[shell] enum ChangeMode: case Hand, Agent -/** Where a run's work goes, asked via [[MainMenu.runTargetChoices]] once the - * flow and the task text are settled. - * - * One choice on one axis, rather than a branch confirm followed by a worktree - * confirm: the two answers are not independent — orca refuses `--worktree` - * together with `--skip-branch` — and asking them separately makes the illegal - * pair expressible, leaving prompt order to prevent it. A worktree run creates - * a branch of its own, so all three cases here are branch-creating or not in - * exactly one way. - */ -private[shell] enum RunTarget: - /** A branch orca creates in this checkout — the default. */ - case NewBranch - - /** The branch checked out now; the flow commits onto it (`--skip-branch`). */ - case CurrentBranch - - /** A separate checkout under `.orca/worktrees/` (`--worktree`). */ - case Worktree - - /** Runs on the branch already checked out, instead of a new one. */ - def skipBranch: Boolean = this == RunTarget.CurrentBranch - - /** Runs in a worktree rather than the invoking checkout. */ - def worktree: Boolean = this == RunTarget.Worktree - private[shell] object MainMenu: /** Fixed ADR §3 order. Conditional items are ABSENT when inapplicable, never @@ -116,15 +91,24 @@ private[shell] object MainMenu: Choice(ChangeMode.Hand, "By hand — open in your editor") ) - /** [[RunTarget]]'s rows, in the order they are offered. `NewBranch` leads - * because it is the default, and the position is load-bearing rather than - * cosmetic: `ConsoleUiShell` cannot honor `preselect`, so the first row is - * what the cursor starts on and what Enter picks. + /** Where a run's work goes, offered as one choice on one axis rather than a + * branch confirm followed by a worktree confirm: the answers are not + * independent — orca refuses `--worktree` with `--skip-branch` — and asking + * separately would leave prompt order to prevent a pair [[RunTarget]] has no + * case for. The menu never keeps uncommitted files, so every row stashes. + * + * `NewBranch` leads because it is the default, and the position is + * load-bearing rather than cosmetic: `ConsoleUiShell` cannot honor + * `preselect`, so the first row is what the cursor starts on and what Enter + * picks. */ val runTargetChoices: List[Choice[RunTarget]] = List( - Choice(RunTarget.NewBranch, "A new branch in this checkout"), Choice( - RunTarget.CurrentBranch, + RunTarget.NewBranch(Uncommitted.Stash), + "A new branch in this checkout" + ), + Choice( + RunTarget.CurrentBranch(Uncommitted.Stash), "The branch checked out now — the flow commits onto it" ), Choice( diff --git a/shell/src/main/scala/orca/shell/actions/AuthorAction.scala b/shell/src/main/scala/orca/shell/actions/AuthorAction.scala index b5823e12..f0934b57 100644 --- a/shell/src/main/scala/orca/shell/actions/AuthorAction.scala +++ b/shell/src/main/scala/orca/shell/actions/AuthorAction.scala @@ -1,7 +1,7 @@ package orca.shell.actions import org.jline.terminal.Terminal -import orca.OrcaDir +import orca.{OrcaDir, RunTarget, Uncommitted} import orca.shell.ShellVersion import orca.shell.create.{ AuthoringSandbox, @@ -139,9 +139,7 @@ private[shell] object AuthorAction: sandbox, FlowFlags( verbose = false, - skipBranch = false, - keepChanges = false, - worktree = false + target = RunTarget.NewBranch(Uncommitted.Stash) ), terminal ) diff --git a/shell/src/main/scala/orca/shell/cli/Cli.scala b/shell/src/main/scala/orca/shell/cli/Cli.scala index 0b1d3805..af613538 100644 --- a/shell/src/main/scala/orca/shell/cli/Cli.scala +++ b/shell/src/main/scala/orca/shell/cli/Cli.scala @@ -2,9 +2,10 @@ package orca.shell.cli import mainargs.{Flag, ParserForMethods, Renderer, Util, arg, main} import org.jline.terminal.Terminal +import orca.RunTarget import orca.settings.GlobalSettings import orca.shell.WorktreeScan -import orca.shell.run.{FlowFlags, LaunchResult} +import orca.shell.run.LaunchResult import orca.shell.ui.ShellUi import orca.subprocess.TtyProbe @@ -167,11 +168,11 @@ private[shell] object Cli: RunCli.run( flowRef = flow, task = task, - flags = FlowFlags( - verbose = verbose.value, + verbose = verbose.value, + target = RunTarget.from( + worktree = worktree.value, skipBranch = skipBranch.value, - keepChanges = keepChanges.value, - worktree = worktree.value + keepChanges = keepChanges.value ), honorPin = honorPin.value, workDir = os.pwd, diff --git a/shell/src/main/scala/orca/shell/cli/RunCli.scala b/shell/src/main/scala/orca/shell/cli/RunCli.scala index 22360f06..05de95af 100644 --- a/shell/src/main/scala/orca/shell/cli/RunCli.scala +++ b/shell/src/main/scala/orca/shell/cli/RunCli.scala @@ -1,6 +1,6 @@ package orca.shell.cli -import orca.OrcaArgs +import orca.RunTarget import orca.shell.actions.{FlowResolution, RunAction} import orca.shell.run.{FallbackPolicy, FlowFlags, FlowLauncher} @@ -13,36 +13,33 @@ import Cli.{actionFailure, complete, usageFailure, withTerminal} */ private[cli] object RunCli: - /** A contradictory `--worktree` pair is refused first, before anything is - * resolved or spawned, saving a `scala-cli` start and its dependency - * resolution. The flow child refuses it too, on this same shared decision, - * and stays the authority — this only makes the answer immediate. + /** `target` arrives unvalidated — a `Left` is the refusal + * [[orca.RunTarget.from]] returned for a contradictory `--worktree` pair. It + * is refused first, before anything is resolved or spawned, saving a + * `scala-cli` start and its dependency resolution; the flow child refuses + * the same argv on the same shared decision and stays the authority, this + * only makes the answer immediate. Below the refusal only the validated + * target exists, so no launch path can be handed a pair orca refuses. */ def run( flowRef: String, task: Option[String], - flags: FlowFlags, + verbose: Boolean, + target: Either[String, RunTarget], honorPin: Boolean, workDir: os.Path, tty: Boolean ): Int = complete: for - _ <- OrcaArgs - .worktreeRefusal( - worktree = flags.worktree, - skipBranch = flags.skipBranch, - keepChanges = flags.keepChanges - ) - .toLeft(()) - .left - .map(usageFailure) + runTarget <- target.left.map(usageFailure) resolved <- FlowResolution .resolve(flowRef, workDir) .left .map(actionFailure) taskText <- readTask(task, tty, readAllStdin).left.map(usageFailure) yield withTerminal: terminal => + val flags = FlowFlags(verbose, runTarget) val result = if honorPin then FlowLauncher.runHonoringPin( diff --git a/shell/src/main/scala/orca/shell/run/FlowLauncher.scala b/shell/src/main/scala/orca/shell/run/FlowLauncher.scala index 54c0d9ce..c7e11728 100644 --- a/shell/src/main/scala/orca/shell/run/FlowLauncher.scala +++ b/shell/src/main/scala/orca/shell/run/FlowLauncher.scala @@ -1,6 +1,7 @@ package orca.shell.run import org.jline.terminal.Terminal +import orca.RunTarget import orca.shell.ShellVersion import orca.shell.flows.BuiltInFlows import orca.shell.ui.{ShellOutput, ShellUi, UiOutcome} @@ -27,16 +28,12 @@ private[shell] enum FallbackPolicy: * adjacent same-typed positional Booleans that a call site could silently * transpose without a compile error. * - * `worktree` combines with neither `skipBranch` nor `keepChanges`; a builder - * that can produce either pair has to ask `OrcaArgs.worktreeRefusal` (as - * [[orca.shell.cli.RunCli]] does), or the flow child refuses it after a spawn. + * `target` is the run's destination as one [[orca.RunTarget]] rather than the + * three flags it renders to, so the combinations orca refuses (`--worktree` + * with `--skip-branch` or `--keep-changes`) cannot be handed to a launch path + * at all. */ -private[shell] case class FlowFlags( - verbose: Boolean, - skipBranch: Boolean, - keepChanges: Boolean, - worktree: Boolean -) +private[shell] case class FlowFlags(verbose: Boolean, target: RunTarget) /** Runs a selected flow as a `scala-cli run` child inheriting the shell's * terminal (ADR 0021 §2). By default the shell forces its own orca version via @@ -84,16 +81,16 @@ private[shell] object FlowLauncher: .getOrElse(Seq.empty) /** `scala-cli run --quiet --verbose [--dep ...] --workspace -- - * [--verbose] [--skip-branch] [--keep-changes] [--worktree]`. The - * `--verbose` before `--` is scala-cli's own ([[loggingArgs]]); the one - * after is the flow's, parsed by its own `OrcaArgs` — which spells its flags - * `--verbose`/`--skip-branch`/`--keep-changes`/`--worktree`, so they land - * after `--` alongside the task text, not before it. `--workspace` relocates - * scala-cli's own `.scala-build`/`.bsp` build metadata to `workspaceDir` - * ([[resolveWorkspaceDir]]) instead of next to `flow` — load-bearing for a - * Project-tier flow, whose script lives inside the user's own repo - * (`/.orca/flows/.sc`), same pollution class the `orca` shim's - * own `--workspace` fixes (ADR 0021 §1 amendment). + * [--verbose] []`. The `--verbose` before `--` is + * scala-cli's own ([[loggingArgs]]); everything after `--` is the flow's + * own, parsed by its `OrcaArgs`, so it lands alongside the task text rather + * than before it — the destination flags come from + * [[orca.RunTarget.toArgv]], which is where their spelling lives. + * `--workspace` relocates scala-cli's own `.scala-build`/`.bsp` build + * metadata to `workspaceDir` ([[resolveWorkspaceDir]]) instead of next to + * `flow` — load-bearing for a Project-tier flow, whose script lives inside + * the user's own repo (`/.orca/flows/.sc`), same pollution class + * the `orca` shim's own `--workspace` fixes (ADR 0021 §1 amendment). * * Requires `task` to be non-blank — `Main.promptTask` re-prompts on blank * input before this is ever called, so an empty task here means a caller @@ -111,17 +108,12 @@ private[shell] object FlowLauncher: "task text must be non-blank — Main.promptTask re-prompts before calling" ) val verboseArgs = if flags.verbose then Seq("--verbose") else Seq.empty - val skipBranchArgs = - if flags.skipBranch then Seq("--skip-branch") else Seq.empty - val keepChangesArgs = - if flags.keepChanges then Seq("--keep-changes") else Seq.empty - val worktreeArgs = if flags.worktree then Seq("--worktree") else Seq.empty Seq("scala-cli", "run", flow.toString) ++ loggingArgs ++ depArgs(orcaVersion) ++ Seq("--workspace", workspaceDir.toString) ++ Seq("--", task) ++ - verboseArgs ++ skipBranchArgs ++ keepChangesArgs ++ worktreeArgs + verboseArgs ++ flags.target.toArgv /** The compile probe's argv — same `--workspace` treatment as [[argv]], and * for the same reason: without it, the probe (run whenever the forced diff --git a/shell/src/test/scala/orca/shell/MainTest.scala b/shell/src/test/scala/orca/shell/MainTest.scala index a6cba9cf..4d3b43d2 100644 --- a/shell/src/test/scala/orca/shell/MainTest.scala +++ b/shell/src/test/scala/orca/shell/MainTest.scala @@ -1,7 +1,7 @@ package orca.shell import org.jline.terminal.{Terminal, TerminalBuilder} -import orca.StackSettings +import orca.{RunTarget, StackSettings, Uncommitted} import orca.runner.manifest.{ ManifestOutcome, ManifestSession, @@ -567,20 +567,26 @@ class MainTest extends munit.FunSuite: // --- promptRunTarget (where the run's work goes, asked as one choice) --- + private val newBranch = RunTarget.NewBranch(Uncommitted.Stash) + test("promptRunTarget: the three destinations are offered, new branch first"): - val ui = RecordingSelectUi(UiOutcome.Selected(RunTarget.NewBranch)) - assertEquals(Main.promptRunTarget(ui), Some(RunTarget.NewBranch)) + val ui = RecordingSelectUi(UiOutcome.Selected(newBranch)) + assertEquals(Main.promptRunTarget(ui), Some(newBranch)) assertEquals( ui.recordedChoices.head.map(_.value), - List(RunTarget.NewBranch, RunTarget.CurrentBranch, RunTarget.Worktree) + List( + newBranch, + RunTarget.CurrentBranch(Uncommitted.Stash), + RunTarget.Worktree + ) ) test( "promptRunTarget: a new branch leads the rows and is marked preselected" ): - val ui = RecordingSelectUi(UiOutcome.Selected(RunTarget.NewBranch)) - assertEquals(Main.promptRunTarget(ui), Some(RunTarget.NewBranch)) - assertEquals(ui.recordedPreselect, Some(Some(RunTarget.NewBranch))) + val ui = RecordingSelectUi(UiOutcome.Selected(newBranch)) + assertEquals(Main.promptRunTarget(ui), Some(newBranch)) + assertEquals(ui.recordedPreselect, Some(Some(newBranch))) test("promptRunTarget: cancelling aborts the run"): assertEquals( @@ -588,14 +594,6 @@ class MainTest extends munit.FunSuite: None ) - test("RunTarget: each destination maps to one flag pair, never both"): - // The pair orca refuses (`--worktree` with `--skip-branch`) has no case - // that produces it — which is the point of asking once rather than twice. - assertEquals( - RunTarget.values.toList.map(t => (t.skipBranch, t.worktree)), - List((false, false), (true, false), (false, true)) - ) - // --- runFlow (the interactive launch path) --- test("runFlow: the chosen destination reaches the launcher's flags"): @@ -632,14 +630,7 @@ class MainTest extends munit.FunSuite: ) assertEquals( recorded, - Some( - FlowFlags( - verbose = false, - skipBranch = false, - keepChanges = false, - worktree = true - ) - ) + Some(FlowFlags(verbose = false, target = RunTarget.Worktree)) ) // --- editFlow / createNewFlow / createForkFlow (ADR 0021 §6/§9 amendment: @@ -1033,9 +1024,7 @@ class MainTest extends munit.FunSuite: Some( worktree -> FlowFlags( verbose = false, - skipBranch = false, - keepChanges = false, - worktree = false + target = RunTarget.NewBranch(Uncommitted.Stash) ) ) ) diff --git a/shell/src/test/scala/orca/shell/actions/AuthorActionTest.scala b/shell/src/test/scala/orca/shell/actions/AuthorActionTest.scala index 929566db..cdafcf53 100644 --- a/shell/src/test/scala/orca/shell/actions/AuthorActionTest.scala +++ b/shell/src/test/scala/orca/shell/actions/AuthorActionTest.scala @@ -1,6 +1,7 @@ package orca.shell.actions import org.jline.terminal.{Terminal, TerminalBuilder} +import orca.{RunTarget, Uncommitted} import orca.shell.ShellVersion import orca.shell.create.{CreateTarget, CreateTier} import orca.shell.flows.{BuiltInFlows, DiscoveredFlow, FlowOrigin} @@ -136,9 +137,7 @@ class AuthorActionTest extends munit.FunSuite: call.flags, FlowFlags( verbose = false, - skipBranch = false, - keepChanges = false, - worktree = false + target = RunTarget.NewBranch(Uncommitted.Stash) ) ) assertEquals(call.fallback, FallbackPolicy.Ask(NoPromptUi)) diff --git a/shell/src/test/scala/orca/shell/actions/RunActionTest.scala b/shell/src/test/scala/orca/shell/actions/RunActionTest.scala index 623facda..5a9ec273 100644 --- a/shell/src/test/scala/orca/shell/actions/RunActionTest.scala +++ b/shell/src/test/scala/orca/shell/actions/RunActionTest.scala @@ -1,5 +1,6 @@ package orca.shell.actions +import orca.{RunTarget, Uncommitted} import orca.shell.flows.{DiscoveredFlow, FlowOrigin} import orca.shell.run.{FallbackPolicy, FlowFlags, LaunchResult} @@ -19,9 +20,7 @@ class RunActionTest extends munit.FunSuite: val flags = FlowFlags( verbose = true, - skipBranch = false, - keepChanges = true, - worktree = false + target = RunTarget.NewBranch(Uncommitted.Keep) ) val recording = RecordingLaunch() diff --git a/shell/src/test/scala/orca/shell/run/FlowLauncherTest.scala b/shell/src/test/scala/orca/shell/run/FlowLauncherTest.scala index b19924d5..28af7301 100644 --- a/shell/src/test/scala/orca/shell/run/FlowLauncherTest.scala +++ b/shell/src/test/scala/orca/shell/run/FlowLauncherTest.scala @@ -1,5 +1,7 @@ package orca.shell.run +import orca.{RunTarget, Uncommitted} + class FlowLauncherTest extends munit.FunSuite: private val flow = os.root / "home" / "u" / "flow.sc" @@ -7,14 +9,12 @@ class FlowLauncherTest extends munit.FunSuite: /** Only the fields under test, defaulted off. Deliberately test-local: * production `FlowFlags` stays without defaults so every real construction - * site keeps stating each flag. + * site keeps stating what it launches. */ private def flags( verbose: Boolean = false, - skipBranch: Boolean = false, - keepChanges: Boolean = false, - worktree: Boolean = false - ): FlowFlags = FlowFlags(verbose, skipBranch, keepChanges, worktree) + target: RunTarget = RunTarget.NewBranch(Uncommitted.Stash) + ): FlowFlags = FlowFlags(verbose, target) test("argv forces --dep with a release version, before --workspace/--"): val result = FlowLauncher.argv( @@ -104,7 +104,7 @@ class FlowLauncherTest extends munit.FunSuite: flow, Some("0.0.18"), "do the thing", - flags(skipBranch = true), + flags(target = RunTarget.CurrentBranch(Uncommitted.Stash)), workspaceDir ) assertEquals( @@ -134,7 +134,10 @@ class FlowLauncherTest extends munit.FunSuite: flow, None, "do the thing", - flags(verbose = true, skipBranch = true), + flags( + verbose = true, + target = RunTarget.CurrentBranch(Uncommitted.Stash) + ), workspaceDir ) assertEquals( @@ -161,7 +164,7 @@ class FlowLauncherTest extends munit.FunSuite: flow, Some("0.0.18"), "do the thing", - flags(keepChanges = true), + flags(target = RunTarget.NewBranch(Uncommitted.Keep)), workspaceDir ) assertEquals( @@ -187,13 +190,13 @@ class FlowLauncherTest extends munit.FunSuite: ) test( - "argv adds --worktree (OrcaArgs's exact flag spelling) after -- when set" + "argv adds --worktree (OrcaArgs's exact flag spelling) after the flow's own --verbose" ): val result = FlowLauncher.argv( flow, None, "do the thing", - flags(worktree = true), + flags(verbose = true, target = RunTarget.Worktree), workspaceDir ) assertEquals( @@ -208,12 +211,13 @@ class FlowLauncherTest extends munit.FunSuite: workspaceDir.toString, "--", "do the thing", + "--verbose", "--worktree" ) ) test( - "argv adds every flow flag in a fixed order after --: verbose, skip-branch, keep-changes, worktree" + "argv adds every flag a single run can carry, in a fixed order after --" ): val result = FlowLauncher.argv( flow, @@ -221,9 +225,7 @@ class FlowLauncherTest extends munit.FunSuite: "do the thing", flags( verbose = true, - skipBranch = true, - keepChanges = true, - worktree = true + target = RunTarget.CurrentBranch(Uncommitted.Keep) ), workspaceDir ) @@ -241,8 +243,7 @@ class FlowLauncherTest extends munit.FunSuite: "do the thing", "--verbose", "--skip-branch", - "--keep-changes", - "--worktree" + "--keep-changes" ) ) From e177307180a4a89703810a113cb0b256cecdb60f Mon Sep 17 00:00:00 2001 From: Adam Warski Date: Fri, 28 Aug 2026 10:40:19 +0000 Subject: [PATCH 3/3] Match on the run target, and test the pair that is allowed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the two commits before this one: - `flow()` asked `args.target == RunTarget.Worktree`; it now matches, so a new case cannot silently take the in-checkout path. - `--skip-branch --keep-changes` is the one combination of two flags orca allows, and it is now its own constructor — so it gets a parse test. - ADR 0018 named the three `OrcaArgs` fields the flags used to become. --- adr/0018-stage-bound-flow-runtime.md | 6 ++++++ runner/src/main/scala/orca/RunTarget.scala | 5 +++-- runner/src/main/scala/orca/flow.scala | 13 +++++++------ runner/src/test/scala/orca/OrcaArgsTest.scala | 8 ++++++++ shell/src/main/scala/orca/shell/Main.scala | 12 ++++++------ 5 files changed, 30 insertions(+), 14 deletions(-) diff --git a/adr/0018-stage-bound-flow-runtime.md b/adr/0018-stage-bound-flow-runtime.md index d27a7d0c..c184adf0 100644 --- a/adr/0018-stage-bound-flow-runtime.md +++ b/adr/0018-stage-bound-flow-runtime.md @@ -454,6 +454,12 @@ the wrong branch. > `--skip-branch` (git will not check the current branch out a second time in > a new worktree). Both refusals happen in `OrcaArgs.parse`, before setup, the > banner, or any git call. + + > **Amendment (2026-08-28).** The three flags above no longer reach a run as + > separate fields: `OrcaArgs.target` is one `RunTarget` — + > `NewBranch(Uncommitted)`, `CurrentBranch(Uncommitted)` or `Worktree`, which + > carries neither — so a refused pair has no representation past + > `OrcaArgs.parse`, which is where it is still refused. - **R5** — On **successful** exit the progress-log file is removed in a final commit, and a feature branch left with no changes other than the progress log is deleted (throwaway-branch cleanup). That removal commit is also pushed, but only diff --git a/runner/src/main/scala/orca/RunTarget.scala b/runner/src/main/scala/orca/RunTarget.scala index 7891d882..e07357a1 100644 --- a/runner/src/main/scala/orca/RunTarget.scala +++ b/runner/src/main/scala/orca/RunTarget.scala @@ -31,12 +31,13 @@ enum RunTarget: /** A separate checkout under `.orca/worktrees/` (`--worktree`). */ case Worktree - /** Runs on the branch already checked out, instead of creating one. */ + // Views for the two setup decisions that turn on a single axis (which branch + // to bind, what to do with a dirty tree). Derived, so no caller can set one + // without the case that implies it. def skipBranch: Boolean = this match case CurrentBranch(_) => true case NewBranch(_) | Worktree => false - /** Leaves uncommitted files in the tree instead of stashing them. */ def keepChanges: Boolean = this match case NewBranch(uncommitted) => uncommitted == Uncommitted.Keep case CurrentBranch(uncommitted) => uncommitted == Uncommitted.Keep diff --git a/runner/src/main/scala/orca/flow.scala b/runner/src/main/scala/orca/flow.scala index 9d651d21..9c361455 100644 --- a/runner/src/main/scala/orca/flow.scala +++ b/runner/src/main/scala/orca/flow.scala @@ -173,9 +173,9 @@ def flow( // Where the run happens. This settles before the directory's first consumer, // the session manifest below; everything downstream is handed `workDir` // explicitly, so it is the single value to change. - def resolveRunDir(): Either[String, os.Path] = - if args.target != RunTarget.Worktree then Right(workDir) - else + def resolveRunDir(): Either[String, os.Path] = args.target match + case RunTarget.NewBranch(_) | RunTarget.CurrentBranch(_) => Right(workDir) + case RunTarget.Worktree => // Resolution can throw as well as refuse — a symlinked or unwritable // `.orca`, a git that won't start. One `Left` shape for every outcome // keeps the reporting below the only way out. @@ -259,9 +259,10 @@ def flow( System.err.println(s"[orca] $message") FlowOutcome.Failed case Right(dir) => - val where = - if args.target == RunTarget.Worktree then s"$dir (worktree)" - else dir.toString + val where = args.target match + case RunTarget.Worktree => s"$dir (worktree)" + case RunTarget.NewBranch(_) | RunTarget.CurrentBranch(_) => + dir.toString flowLog.info( "orca {} starting (workDir={})", OrcaBanner.version, diff --git a/runner/src/test/scala/orca/OrcaArgsTest.scala b/runner/src/test/scala/orca/OrcaArgsTest.scala index 6a0fdd43..d17b80cd 100644 --- a/runner/src/test/scala/orca/OrcaArgsTest.scala +++ b/runner/src/test/scala/orca/OrcaArgsTest.scala @@ -44,6 +44,14 @@ class OrcaArgsTest extends munit.FunSuite: Right(RunTarget.NewBranch(Uncommitted.Keep)) ) + test("--skip-branch with --keep-changes: current branch, files kept"): + assertEquals( + OrcaArgs + .parse(Seq("--skip-branch", "--keep-changes", "do the thing")) + .map(_.target), + Right(RunTarget.CurrentBranch(Uncommitted.Keep)) + ) + test("--worktree targets a worktree"): val result = OrcaArgs .parse(Seq("--worktree", "do the thing")) diff --git a/shell/src/main/scala/orca/shell/Main.scala b/shell/src/main/scala/orca/shell/Main.scala index 1cfc084c..aa895f58 100644 --- a/shell/src/main/scala/orca/shell/Main.scala +++ b/shell/src/main/scala/orca/shell/Main.scala @@ -405,12 +405,12 @@ object Main: * the resume happens on the current branch by design, and a resumed log's * `bindBranch` (`FlowLifecycle`) ignores `skipBranch` entirely, so the * default target passed here is exactly as correct as any other would be — - * `Worktree` included: the run is launched IN `run.dir`, the directory its - * log was found in, which is what makes this a resume. That target would - * instead re-derive a path from the task text, which is the same directory - * only when the log happened to be in an orca-made worktree of that exact - * prompt. `runAction` is injectable, [[AuthorAction]]-style, so a test can - * record the call instead of spawning a real subprocess. + * the worktree axis included: the run is launched IN `run.dir`, the + * directory its log was found in, which is what makes this a resume. + * `Worktree` would instead re-derive a path from the task text, which is the + * same directory only when the log happened to be in an orca-made worktree + * of that exact prompt. `runAction` is injectable, [[AuthorAction]]-style, + * so a test can record the call instead of spawning a real subprocess. */ private[shell] def resumeInterruptedRun( ui: ShellUi,