refactor(cli): replace hand-rolled parsers with effect/unstable/cli - #52
Open
FreshlyBrewedCode wants to merge 4 commits into
Open
FreshlyBrewedCode wants to merge 4 commits into
FreshlyBrewedCode wants to merge 4 commits into
Conversation
FreshlyBrewedCode
force-pushed
the
33-parse-cli-with-effect-cli
branch
from
September 20, 2026 12:58
77daec6 to
8de9136
Compare
FreshlyBrewedCode
added this pull request to stack #58
September 20, 2026 13:00
This was referenced Sep 22, 2026
FreshlyBrewedCode
added a commit
that referenced
this pull request
Sep 23, 2026
Commit a507d1e's conflict resolution against 33-parse-cli-with-effect-cli silently reverted PR #52: src/cli.ts's import.meta.main block regressed to the pre-#52 hand-rolled USAGE/parseFlags/usageError/parseArgs parser, even though src/cli-commands.ts's factoryCommand (the effect/unstable/cli command tree) and its tests kept passing in isolation — so CI stayed green while the shipped binary silently lost generated help, typed flag validation, and --wizard/--completions. Restore the base branch's entrypoint (import { factoryCommand } from "./cli-commands"; Command.run(factoryCommand, ...)) while keeping this PR's actual new work intact: the ManagedRuntime/AgentRuntimeLayer composition root in runCli, which resolves the adapter from factory.config.ts (falling back to opencodeAdapter) and threads a ManagedRuntime into startRun. Also restore `prepareWorkspace: options.clone !== undefined`, which the same bad merge had silently dropped from runCli's startRun call. cli-commands.ts's runCommand was hardcoding `adapter: opencodeAdapter` on every `factory run` invocation, which bypassed runCli's config-driven adapter resolution and defeated issue #36's "runtime selectable from factory.config.ts" criterion for the direct-run path. Drop that override so runCli's own fallback (options.adapter ?? config.agent.adapter ?? opencodeAdapter) decides. Add tests that exercise src/cli.ts's actual import.meta.main entrypoint (the same path bin/factory.js runs in production), not just factoryCommand in isolation: a static check that the file contains no hand-rolled parser, and spawned-process checks that --help renders effect/unstable/cli's generated help and that `serve --port abc` is rejected by the typed Int flag. Without these, this class of regression can pass CI again undetected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replace the four hand-rolled argument parsers with a single command tree built from effect/unstable/cli. Each command (init, serve, start, runs, log, run) is defined with typed flags and positional arguments. The new argv tests feed argv directly to the parser via Command.runWith rather than bypassing it with structured options. Flag.Int rejects non-numeric values for --port, Flag.Boolean handles --watch correctly, and all required/optional flags are validated by the framework.
…ed parsers Replace the import.meta.main dispatch block with Command.run(factoryCommand). Remove the USAGE constant, parseFlags, parseArgs, parseStartArgs, parseServeArgs, and usageError — help text is now generated from the command definitions, flag values are validated before reaching command bodies, and no process.exit call remains inside a parsing function. The command body functions (runCli, startCli, listRunsCli, logRunCli, watchSse) are untouched — they are wrapped at the Effect boundary in cli-commands.ts. Exit codes are preserved: 0 completed, 1 failed, 130 cancelled.
…rrors Command.run(factoryCommand) fails with CliError.ShowHelp both when no subcommand is given and for genuine parse/validation errors, so the blanket `.catch(() => process.exit(1))` was mapping bare `factory` (and anything else that only renders help) to exit 1 instead of the pre-effect/unstable/cli behaviour of exit 0. ShowHelp.errors distinguishes the two cases — empty means "help was all that happened" — so only map that case to exit 0; a populated errors array (bad flag value, unknown flag, missing argument, ...) still exits 1. Avoid delegating to Runtime.defaultTeardown/makeRunMain for this: it calls process.exit(0) on any successful Effect completion, which would kill `factory serve` right after it starts its long-lived HTTP server. Adds subprocess-spawned regression tests in cli-argv.test.ts pinning the no-args/--help/-h exit codes and confirming --port abc and an unknown flag still exit non-zero — the exit-code mapping lives in cli.ts's import.meta.main block, so it's only observable by running the binary.
effect@4.0.0-rc.115 only exports test/noop constructors for Stdio/Terminal/ FileSystem/ChildProcessSpawner (Stdio.layerTest, FileSystem.layerNoop, Terminal.make, ChildProcessSpawner.make) — no @effect/platform-node or @effect/platform-bun equivalent is installed, so there is no real platform layer to swap CliEnvLayer for. Record why the test-fixture shapes are used for the real binary and which paths would break if they were ever exercised, so the next reader doesn't have to re-derive it.
FreshlyBrewedCode
force-pushed
the
33-parse-cli-with-effect-cli
branch
from
September 23, 2026 07:08
a52788c to
90186ba
Compare
This branch has not been deployed
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
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.
Part of #32 · Closes #33
src/cli.tshand-rolled four argument parsers that disagreed with each other:parseFlagswalksargv at stride 2 and structurally cannot represent a boolean flag, which is why
factory startneeded a second, different parser;
init's flags were parsed inline in theimport.meta.mainblock; and
parseServeArgspassed an unvalidatedNumber(portRaw)straight toBun.serve, so--port abcbecameNaN. BecauseusageErrorcalledprocess.exitinside a parser, none of itcould be tested — every existing CLI test bypassed argv and called the command functions directly
with structured options. This PR replaces all four parsers and the hand-maintained
USAGEstringwith a single
effect/unstable/clicommand tree, per ADR 0009 §5.What changed
src/cli-commands.tsdefinesfactoryCommand— a rootCommandwith six subcommands(
init,serve,start,runs,log,run), each with typedFlag/Argumentdefinitions,descriptions, and defaults matching the old
USAGEstring.src/cli.tsdropsUSAGE,usageError,parseFlags,parseArgs,parseStartArgs, andparseServeArgsentirely. Theimport.meta.mainblock now runsCommand.run(factoryCommand, …)under a
CliEnvLayer(FileSystem.layerNoop,Path.layer,Stdio.layerTestseeded with realprocess.argv, aTerminalshim, and aChildProcessSpawnershim).runCli/startCli/listRunsCli/logRunCli— the command bodies — are unchanged.--portis nowFlag.Int, so a non-numeric value is rejected at parse time with a clear errorinstead of reaching
Bun.serveasNaN.Command.runfails withCliError.ShowHelpboth for genuine parse errors andfor "no subcommand given" / explicit
--help(help renders from the command definition eitherway).
ShowHelp.errorsdistinguishes the two — empty means help was all that happened. Theimport.meta.maincatch now only callsprocess.exit(1)when that array is non-empty, so barefactory,--help, and-hexit0again (matching the pre-effect/unstable/clibehaviour),while
--port abc, unknown flags, and missing required args/flags still exit1.src/cli-argv.test.ts(28 tests) feeds argv arrays straight into aCommand, coveringdefaults, required flags/arguments, the
--watchboolean, the--clone/--git-name/--git-emailgroup,--portrejecting non-numeric input, generated--helpat the root and persubcommand, unknown-flag rejection, and — since the exit-code mapping lives in
cli.ts'simport.meta.mainblock, only observable by actually running the binary — a subprocess-spawnedsuite pinning the no-args/
--help/-hexit code and confirming--port abc/unknown flags stillexit non-zero.
Notes for reviewers
Command/Flag/Argumentdefinitions (withCommand.withDescription/Flag.withDescription) rather than hand-maintained, and the libraryadds global flags the old CLI never had (
--version,--wizard,--completions,--log-level) — out of scope for Parse CLI arguments with effect/unstable/cli #33, not regressions.CliEnvLayerinsrc/cli.tsreuses the effect/cli library's own test-fixture layer shape(
Stdio.layerTest,Terminal.makewithreadInput/readLine: Effect.die, aChildProcessSpawnerthat dies on use) as the production entrypoint's environment. I checkedwhether a real platform layer exists to swap it for:
effect@4.0.0-rc.115exports no such thing— no
@effect/platform-node/@effect/platform-bunequivalent is installed, andStdio,Terminal,FileSystem, andChildProcessSpawnerin this rc only ship test/noop constructors.So
CliEnvLayerstays as-is; I added a comment on it explaining why and which paths (--wizard,anything reading stdin, anything shelling out through
ChildProcessSpawner) would hit a bareEffect.die("unused")if ever exercised. Everything this PR's commands do goes throughConsolefor output and realprocess.argvfor input, so it's unaffected.process.exitcalls that remain (cli-commands.ts, pluslogRunCli's incli.ts, plus thesingle
process.exit(1)in theimport.meta.maincatch) are outside argv parsing — satisfiesthe "no
process.exitin a parsing function" criterion.Effect.runPromiseExit+Runtime.defaultTeardown/makeRunMain(effect's usual main-entrypoint helper): that teardowncalls
process.exit(0)on any successfulEffectcompletion, butfactory serve's handlereffect resolves right after starting the long-lived HTTP server — forcing an exit there would
kill the daemon immediately after startup. The fix stays inside the existing
.catch()shapeinstead, so a successful run still falls through to whatever keeps (or doesn't keep) the process
alive on its own.
Verification
bun run check(format:check + lint + typecheck +bun test) passes locally: 289 pass, 0 fail.factory,--help,-h,serve --help,serve --port abc,serve --nope, unknown subcommand, missing required flag/argument,$FACTORY_URLfallback,--clone/--git-name/--git-email) and confirmed exit codes andstdout/stderr routing match the pre-
effect/unstable/cliCLI.factory serve --port 0and confirmed it stays listening (doesn't exit afterstartup) under the new exit-code handling.
bun run test:e2e.Stack
33-parse-cli-with-effect-cli(issue Parse CLI arguments with effect/unstable/cli #33) ← you are here34-domain-errors-as-tagged-errors(issue Represent domain failures as Schema.TaggedError #34)35-move-chunk-interpretation-into-adapter(issue Move chunk interpretation into the agent adapter #35)37-move-headless-permissions(issue Move headless permission setup out of the workspace allocator #37)36-agent-runtime-service(issue Add an Effect composition root and make the agent runtime a service #36)38-move-singletons-into-layers(issue Move the daemon's remaining singletons into layers #38)Stack created with GitHub Stacks CLI • Give Feedback 💬