fix(console): flags after the subcommand were ignored — watch --relaunch now actually relaunches, kill --force now forces - #401
Merged
Conversation
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.
Behaviour change
After this release,
lacquer console … watch --relaunchreally relaunches. That includes the form fleet-ops' README documents,./console.sh watch --relaunch. Every Failed session in the sessions file is dispatched again, and each relaunch uses usage. Until now that command printed the status and exited 0 without relaunching anything, so anything that ran it on a schedule was doing nothing.MaxFailedLaunches), a record is held and not relaunched again. Blocked and Missing sessions are still only reported.watch --relaunch --dry-run. That now works on either side ofwatchas well.kill <name> --forcenow forces. Before,--forceafter the name was ignored, and an Alive session was refused as if it had not been passed.--dry-run kill xused to kill for real, anddispatch proj t --dry-rnlaunched for real witht --dry-rnas the task (the RED run below caught exactly that). An unknown subcommand (console wacth) is now an error; before, it showed the dashboard. An extra argument towatch,kill, orinbox resolve/listis an error too.--roster,--roles,--sessionsand--inboxare still accepted by every subcommand, because fleet-ops'console.shpasses the first three on every call.Why
Go's
flagpackage stops at the first argument that isn't a flag. Every console subcommand is such an argument, so any flag written after it was never parsed. #399 madedispatch/dispatch-rolerefuse a known flag after the subcommand, and reportedwatch --relaunch/kill --forceas still broken. Reproduced: with a Failed record,watch --relaunch --dry-runprinted only the status, while--relaunch --dry-run watchattempted the relaunch.What
cmd/lacquer/main.go:parseConsoleArgsparses console flags wherever they appear: before the subcommand, after it, or among its arguments. It uses the flag package's own syntax:-nameor--name, a value after=or as the next argument, and no separate value for a bool. Soconsole --relaunch watchandconsole watch --relaunchmean the same thing.--ends the flags. An unknown flag is an error; it never becomes an argument. This replaces feat(console): dispatch into a PM-assigned worktree (--worktree) or name the created branch (--branch) instead of an unused dispatch-* worktree #399'strailingFlagrefusal.consoleSubcommandresolves the subcommand (""for the dashboard,inbox add, …). It refuses an unknown subcommand or an argument the subcommand would not read.consoleFlagScope/checkConsoleFlagScopelist which subcommands each flag applies to, and refuse a flag set for any other. A flag missing from the table is refused everywhere (fail closed), andTestConsoleEveryFlagHasADeclaredScopechecks that every flagconsole -hlists has an entry. This takes over feat(console): dispatch into a PM-assigned worktree (--worktree) or name the created branch (--branch) instead of an unused dispatch-* worktree #399's "--worktree/--branchonly with dispatch" check.--type --title --body --ref --project,--all) are now console flags scoped toinbox add/inbox list, so they also work on either side.runInboxAdd/runInboxListtake the parsed values.lacquer helpandsite/.../reference/commands.mdnow say flags work on either side, list which ones every subcommand accepts, and explain--. The--livenote ("runwatch --relaunchonce to relaunch") needed no change: its advice is correct now, and wasn't before.Dispatch: flags after the project are now parsed, not refused
I chose to parse them, with
--as the only way to put a flag-looking word in a task:dispatch proj "do it" --dry-runis a dry run. feat(console): dispatch into a PM-assigned worktree (--worktree) or name the created branch (--branch) instead of an unused dispatch-* worktree #399's reason for refusing was that a trailing--dry-runmust never become task text and launch for real. Parsing it as a flag meets that directly, and so does refusing it. The difference is that the parsed form does what the user meant.dispatch proj "do it" --dry-runwould launch for real with--dry-runin the task. So I rejected it.-is read as a flag unless it comes after--. If it isn't a known flag, that's an error, and the message points to--:dispatch proj -- fix the --verbose flag. The task never quietly absorbs a flag. Text inside one quoted argument ("fix --verbose handling") was never at risk, because only whole arguments are read as flags.Tests
In
cmd/lacquer/console_flag_order_test.go. Every test goes throughrun(), using #399'snewPlaceFleet:gittestrepos, a fakeclaudeandtmuxon PATH, and a tempHOME. Nothing real is launched or killed.--relaunch=true, file flags after too). All must print the same dry-run relaunch, launch nothing, and leave the sessions file unchanged.--relaunch=falsemust not relaunch. A realwatch --relaunchmust launch once and replace the failed record.working).kill projis refused.--forcebefore, after, between, and--force=truemust each kill it: the job dir is removed and the record dropped.--mode/--branch/--dry-run/--roster/--roles/--worktreeon either side, and among the task words, must all print identical output. Nothing launches, nothing is recorded, and no worktree is created.--: words after--are the task, flag-looking ones included. Without--, an unknown flag in the task is an error that names--. After--,--dry-runis task text (the fake launch receivestry --dry-run).list --allwith--inboxand their own flags on either side.--dry-run/--relaunchwithkill,--forcewithwatch,--modewithdispatch-role,--relaunchwithdispatch, inbox flags on the wrong inbox subcommand,--typewithwatch,--forcewith the dashboard); a missing value, a bad bool, an unknown subcommand, and extra arguments. Each must exit non-zero and name the offending word. Across all of them there must be no launch, the Alive job must still be there, and the sessions file and inbox must be unchanged.TestConsoleDispatchRefusesAmbiguousOrMisplacedPlacement(feat(console): dispatch into a PM-assigned worktree (--worktree) or name the created branch (--branch) instead of an unused dispatch-* worktree #399): its three "flag after dispatch is refused" cases now cover allowed behaviour, tested positively above. They are replaced by--worktreeafterwatchand--branch=afterkill, which must still be refused.RED on main (fc481d1): every new test failed. The refusal test also showed
claude launched 2 time(s): on main,dispatch proj t --dry-rnanddispatch-role pm-bg -xboth launched for real.Mutation testing (CLAUDE.md rule 2)
For each mutation: break
main.go, rungo test ./cmd/lacquer -run 'TestConsole|TestInbox', restore withgit checkout, then confirmgit statusis clean. All 18 were killed by a named test. M10 and M17 broke the build on the first pass, which doesn't count, so both were rewritten to compile and run again.--not a terminatorTestConsoleDispatchDoubleDashEndsTheFlags--drops what followsTestConsoleDispatchDoubleDashEndsTheFlagsTestConsoleWatchRelaunchAfterTheSubcommandRelaunches,TestConsoleKillForceAfterTheSubcommandForces=valueTestConsoleWatchRelaunchAfterTheSubcommandRelaunches,TestConsoleRefusesAnUnknownOrMisplacedFlagTestConsoleRefusesAnUnknownOrMisplacedFlag,TestConsoleDispatchDoubleDashEndsTheFlagsTestConsoleRefusesAnUnknownOrMisplacedFlagTestConsoleRefusesAnUnknownOrMisplacedFlagTestConsoleRefusesAnUnknownOrMisplacedFlag,TestConsoleDispatchRefusesAmbiguousOrMisplacedPlacement--dry-runscoped tokilltooTestConsoleRefusesAnUnknownOrMisplacedFlagTestConsoleEveryFlagHasADeclaredScopeTestConsoleRefusesAnUnknownOrMisplacedFlagTestConsoleRefusesAnUnknownOrMisplacedFlag--remedy in the errorTestConsoleDispatchDoubleDashEndsTheFlags--forcescoped towatchTestConsoleKillForceAfterTheSubcommandForces,TestConsoleRefusesAnUnknownOrMisplacedFlaginbox listignores--allTestConsoleInboxFlagsParseOnEitherSide,TestConsoleInboxAddListResolveRoundTrips--typescoped everywhereTestConsoleRefusesAnUnknownOrMisplacedFlagVerification
Run locally the way
.github/workflows/ci.ymlruns it, after merging origin/main (#400, v1.41.0):gofmt -l .produced no output.go vet ./...andgo build ./...passed.go test <all but internal/shipped> -race -timeout 15mwithLACQUER_TEST_REQUIRE_TMUX=1exited 0: 41 packages, 39 ok, 2 with no tests.go test ./internal/shipped/ -timeout 20m: ok (85.2s).No detector package changed and no
profiles/*/workflows/changed, so the fleet dry-run and proven-on sections don't apply. VERSION is untouched.