Make the CLI Grove's only agent interface, and remove MCP - #68
Open
nicksong-z wants to merge 21 commits into
Open
Make the CLI Grove's only agent interface, and remove MCP#68nicksong-z wants to merge 21 commits into
nicksong-z wants to merge 21 commits into
Conversation
Grove's MCP server was a second interface to a small set of features for users (local devs and coding agents) who already have shell access. It cost a JSON-RPC protocol implementation, `.mcp.json` lifecycle management, an announcements SQLite database, and its own tests and failure modes — without providing a trust or authentication boundary the CLI doesn't already have. The `gw` CLI is now the only first-party agent interface. Removed: - `internal/mcp/` (server + announcements store) and `cmd/mcp.go` - the hidden `gw mcp-serve` command - `.mcp.json` generation on create and cleanup on delete - `models.MCPConfig` / `models.MCPServer` - the `modernc.org/sqlite` dependency tree Added a migration path instead of silent breakage: workspaces created by older versions still carry a `grove` entry pointing at the removed command, so `gw doctor` reports it and `gw doctor --fix` removes just that entry. `internal/workspace/mcpmigrate.go` only claims entries Grove actually wrote (command `gw`, args containing `mcp-serve`) so an external adapter named `grove` and any other server in the file are left untouched. `announce` / `get_announcements` are intentionally not reimplemented; there is no demonstrated usage to preserve. Release binary (darwin/arm64, -s -w): 13,030,674 → 8,951,234 bytes (-31%).
Defines the versioned envelope agents parse, the stable error codes they branch on, and the exit-code classes they react to. No command uses it yet — this commit is the contract plus its documentation so the following commits have one shape to converge on. Design notes: - Error classification lives with the envelope, not in string matching at the CLI boundary. Services return `*machine.Error` carrying a code, a suggested fix, and safe next commands; unclassified errors surface as INTERNAL rather than being guessed onto a neighbouring code. - Exit codes are classes (usage / not-found / conflict / precondition / permission / transient / cancelled) so a shell caller can decide whether to retry without parsing JSON. Only TRANSIENT is retry-safe, and a test enforces that every declared code has a class. - `Emit` is a no-op in text mode, so call sites can be unconditional and human output never gets JSON interleaved into it. - Warnings are collected and attached to the envelope once, then drained, so a degraded-but-successful run is machine-visible instead of stderr-only. - `DetectEarly` reads --format before Cobra parses, so pre-command output like the update notice can be suppressed in machine mode. Cobra stays the source of truth and still rejects invalid values. - Even a marshal failure writes a valid envelope, keeping the "stdout is always parseable JSON" guarantee true on the error path. docs/agent-cli.md documents the envelope, the code/exit table, the compatibility policy, and the create → inspect → sync → delete example.
`--format json` (`-o json`) is now available on every command and turns on machine mode: one envelope on stdout, everything else on stderr, no colors, no prompts, no version notice, and an exit code matching the failure class. Plumbing: - Root registers --format as a persistent flag and validates it in PersistentPreRunE, so a bad value is a structured USAGE error rather than a pflag message. A test asserts every subcommand inherits the flag — an agent can't know which commands opted in. - Execute() reads --format before Cobra parses so the update notice is suppressed in machine mode; otherwise the first line of stdout... would be fine, but the first line of *stderr* would be release news an agent has to filter, and the notice is irrelevant to a non-human caller anyway. - Cobra's own parse failures are mapped to USAGE (exit 2) by marker matching, which is confined to that one boundary. Already-classified errors pass through untouched. - fail() replaces ad-hoc "print red text, exit 1": one envelope or one stderr line plus the fix hint, and the error's exit class. Non-interactivity is enforced where blocking could happen, not just promised: pickers refuse to run in machine mode with a USAGE error, and Confirm/Prompt return their defaults without touching stdin. console.NoColor strips ANSI from stderr diagnostics, and console.Warning now also records into the envelope's warnings array so a degraded-but-successful run is machine-visible. Read commands emit the envelope: list, list --status, ws show, status, doctor, repos, preset list/show, plugin list. Two shape decisions worth noting: - `gw doctor` reporting problems is ok:true with issues in the result. A failed *diagnosis* would be ok:false; findings are the successful output. - `gw repos` with no configured repo dirs is now NOT_INITIALIZED instead of an empty list, so an agent cannot misread it as "this machine has no repos". Status output was refactored so the human table and the envelope both render one workspace.StatusReport — they cannot drift. Its Dirty()/Behind() helpers drive state-dependent next_actions (offer sync only when actually behind). The legacy `--json` flag keeps its exact pre-envelope output for existing scripts and plugins; its help text now marks it deprecated in favour of --format json, and a test pins that it still exists. Service-layer errors are now classified at the point of detection (internal/workspace/errors.go): WORKSPACE_NOT_FOUND, WORKSPACE_EXISTS, REPO_NOT_FOUND, WORKTREE_EXISTS, WORKTREE_DIRTY, GIT_FAILED — each carrying a fix hint and safe next commands. Note: text-mode exit codes are now semantic too (e.g. `gw status missing-ws` exits 3, not 1). That is the point of the contract, and 0 still means success.
create/delete/sync/add-repo/remove-repo/run now report what happened to each repo instead of printing prose and returning a bare error. Multi-repo work is partially failable by nature, so "did it work?" is not answerable with one boolean — the envelope carries a result per repo with a stable outcome from a fixed vocabulary (created, added, already_present, removed, not_found, rebased, up_to_date, skipped, exited, failed) plus a Detail explaining any non-obvious outcome. Service API: CreateWithOpts/Delete/Sync/AddRepos/RemoveRepos/Run return typed results. The positional Create() wrapper keeps its error-only signature since it exists for historical call sites. Failure-vs-data decisions, which are the substance of this commit: - Sync returns ok:true with per-repo outcomes even when a repo could not be rebased. Sibling repos may have advanced, and reporting a command failure would hide that. Only an unreadable workspace is an error. - Create stays all-or-nothing and returns no result on failure, so a caller can never mistake a rolled-back attempt for a half-built workspace. - Delete reports state_removed, distinguishing "gone" from "partially gone" when a worktree could not be removed and the state entry was kept on purpose so `gw doctor` can still find the leftover. - add-repo/remove-repo treat already-present and not-found repos as outcomes rather than errors, so an agent retrying after a partial failure converges. - AddRepos now persists repos that succeeded before a later repo failed; abandoning them left worktrees on disk that state did not know about. - Sync's dirty/undeterminable-upstream cases became explicit skipped outcomes with reasons instead of stderr-only warnings. - A post_create hook failure reports HOOK_FAILED with the workspace in details, so the caller knows the workspace exists and must not retry create. Machine-mode stdout stays a single envelope even while running foreign code: per-repo setup/run hook stdout is redirected to stderr (`gw run` children keep their [repo] prefixes there), since hook output is arbitrary text. Lifecycle hooks already wrote to stderr only. Destructive operations require explicit intent in machine mode: delete, remove-repo, and create --replace demand --force rather than treating a prompt they are forbidden to show as consent. `gw delete` with no NAME is a USAGE error instead of an interactive picker. Behavior preserved deliberately: remove-repo still deletes branches without --force while whole-workspace delete still forces, so unmerged work survives removing a single repo. A test pins that difference now that both paths share deleteRepo().
An agent's first question is "where am I and what can I do?". Answering it previously took several commands (list, status, repos, preset list) plus knowledge of which one to trust. `gw context --format json` answers it once. It reports the workspace containing the cwd (or null), each repo's live branch, base branch, remote, and dirty/ahead/behind state, the configured repo dirs, presets, workspace inventory, and safe next actions. Design decisions: - It is a projection, not a new state model: every field is derived from config.toml, state.json, or a local git query, and nothing is cached. - RepoContext embeds RepoStatus, so `gw context` and `gw status` cannot report different git state for the same repo. A test asserts they agree. - Only local git operations run (no fetch, no PR lookups) so the command is cheap enough to call before every decision, with per-repo collection in parallel like the rest of the service. - A missing config reports initialized:false instead of erroring — an agent's first call is exactly how it should discover Grove needs `gw init`, and the next_actions then point at it. - workspace is null outside a workspace rather than a best guess; that null is the signal that later commands need an explicit name. - Path containment resolves symlinks, prefers the deepest match, and rejects ".." results so a sibling like `feat-other` is never treated as inside `feat`. Running from a repo subdirectory resolves the parent workspace, which is where agents actually work from. - Fields for features that do not exist in core (blueprint identity, preparation/Oven status) are omitted rather than stubbed as null; adding them later is a compatible change under the schema policy. - List-valued fields are always arrays, never null, so clients can iterate without nil checks.
The removed MCP server's real feature was letting agents in parallel workspaces leave each other notes about shared repos. That capability came from a shared store on disk, not from JSON-RPC — so it belongs in the CLI, where every agent with a shell can reach it. This restores announce/get_announcements as `gw announce` and `gw announcements`, and fixes the discovery weakness that made the MCP version go unused (0 rows in messages.db after months of real use): notes now arrive in `gw context` under result.announcements, so an agent receives coordination while orienting instead of having to notice a tool in a list and choose to call it. Storage is a directory of one-file-per-note JSON under ~/.grove/announcements/, not SQLite: - publishing is a single O_EXCL file creation, so concurrent agents cannot clobber each other and no locking is needed — a test runs 24 parallel publishers and asserts none are lost; - reading is a directory scan, unaffected by concurrent writers; - pruning unlinks expired files, which is safe during reads and writes, and runs opportunistically on publish so no background job is required. SQLite serialized writes that file creation already serializes, and cost the ~4 MB dependency tree this epic just removed. Binary stays at 9.1 MB. Contract decisions: - Notes are keyed by normalized repo remote (ssh/https/nested-group forms all collapse to owner/repo), so different worktrees of the same upstream match. Both publish and read derive keys through one function — deriving them differently would mean two agents silently never see each other. - Repos with no remote fall back to their local name, so the feature still works for local-only repos. - A workspace never sees its own notes; that is noise, not coordination. - Coordination is advisory, so an unreadable or corrupt store degrades to zero announcements rather than failing the command the agent was actually running, and unparseable files are left on disk rather than deleted as if they were ours. - Retention is 30 days in the store but 7 days / 20 entries in `gw context`: an old note is history, not something to act on while orienting. AGENTS.md and CLAUDE.md now document the agent interface and these commands, so the discovery path is explicit rather than depending on tool-list osmosis.
Destructive agent operations now have a review step: `gw plan create` and `gw plan delete` describe what would change, and `gw apply` executes a plan that was reviewed. Two properties make a plan worth more than a printed warning: - It is produced by the same validation path as execution. validateCreate is now shared by PlanCreate and CreateWithOpts, so a plan cannot succeed where execution would fail validation — a table test asserts both paths return the same error code for duplicate names, unknown repos, missing branch/repos, and an already-worktreed branch. Create also now fails validation before touching the filesystem rather than mid-provision. - It pins the state it depends on with a fingerprint, and apply recomputes it. A delete plan's fingerprint deliberately includes each repo's dirty flag, so if an agent starts editing between review and apply, the delete is refused with STATE_CHANGED (exit 4) and the work survives. That is the property worth having; a plan that stayed valid while the world moved would be dangerous. Plans enumerate every repository, path, and branch, each marked destructive or not, so "delete workspace x" is never the whole story. Delete plans additionally warn about uncommitted changes and unpushed commits — the things a reviewer most needs to know. Apply rebuilds the create request from the plan document (repo set, source paths, workspace dir from the plan's own path), so a config change or shifted repo discovery between plan and apply cannot silently redirect the work. Ergonomics: apply accepts a bare plan, a saved `--format json` envelope (which is what `> plan.json` actually produces), or `-` for stdin. A saved *failure* envelope is refused rather than parsed into an empty plan, and an unrecognized plan schema_version is refused rather than misread. The plan document is versioned separately from the response envelope since they evolve for different reasons. `gw plan create` requires --repos/--preset/--all instead of falling back to an interactive picker: a plan must be reproducible from its inputs. The transactional execution and rollback semantics underneath belong with the transactional-operations epic (#59); this owns the public reviewable contract.
A readability pass over the code added in this branch. No behavior changes: the full suite passes untouched, and human output for list -s / ws show / repos / context is byte-identical against the previous binary. Deduplication (the substantive part): - state.PathContains is now the single definition of "is this path inside this workspace?". state.FindWorkspaceByPath and gw context each had their own implementation — prefix-matching versus filepath.Rel — which is exactly the kind of divergence that makes two commands disagree about where the caller is. Each keeps its own selection policy (first match for state lookups, deepest match for context, which matters for nested workspaces) but they now share the subtle part: absolute-path resolution, symlink resolution, and rejecting "../" so a sibling like feat-other never counts as inside feat. A table test pins those edge cases. - announce's List and Prune both walked the directory and decoded files; they now share Store.each, so "an announcement we can act on" has one definition. - cmd's home-directory-to-~ rewrite existed in four copies; now shortenPath. Decomposition: - PlanCreate (cyclomatic 13) split into planRepoProvisioning / planBranchProvisioning / effectiveBranchMode / resolveBaseForPlan / planSetupHooks. The three-way branch resolution is the part worth reading on its own, since it has to mirror provisionWorktreeNoFetch. - PlanDelete (10) split into planRepoDestruction / unsavedWorkWarnings, which also makes it symmetric with the create path. - announce.List (19) split into a named filter type plus Store.each, leaving List about assembling and ordering results. - announce.Publish (13) extracted writeNew, removing a nested re-marshal in the ID-collision retry and putting the O_EXCL rationale next to the syscall. Also: dropped a hand-rolled min() that shadowed the builtin, and threaded BranchExists through planRepoProvisioning instead of calling it twice — the first decomposition attempt added a redundant git subprocess per repo. gocyclo -over 20 and staticcheck are clean.
A delete plan asked `rev-list origin/<branch>...HEAD` and ignored the error. For
a branch that was never pushed, origin/<branch> does not exist, so the call
failed and the plan reported no warnings at all:
committed locally, never pushed: work that exists nowhere else
gw plan delete losswork → {"warnings": null}
The risk was inverted. A pushed branch that was merely ahead of its remote got a
warning, while commits that exist nowhere but that worktree — the only truly
unrecoverable case — got silence. A plan whose purpose is to say what will be
destroyed must not be quietest when the stakes are highest.
Now the comparison target depends on whether the remote branch exists: against
origin/<branch> when it does, and against the resolved base branch when it does
not, which is what makes the never-pushed case visible.
Both failure paths also warn instead of staying silent:
- an unreadable worktree ("could not check for uncommitted changes") is not
evidence of a clean one;
- a branch with no remote and no resolvable base still says its commits may
exist only here.
Tests cover never-pushed, pushed-and-ahead, fully-pushed (which must stay quiet
so the warning keeps meaning something), and an unreadable worktree.
Counted with a PATH shim over real git, `gw context` on a two-repo workspace made 20 git invocations, 6 of them redundant: collectRepoStatus resolves the base branch to compute ahead/behind, then repoContexts resolved it again for RepoContext.BaseBranch. Resolution alone is up to three subprocesses (symbolic-ref, then probing origin/main and origin/master). The fix is data flow, not caching: RepoStatus now carries the BaseBranch that ahead/behind were measured against, and RepoContext gets it through the embedded struct. 20 → 14 invocations. That field is worth having on its own — `gw status --format json` reported "ahead: 2, behind: 1" without saying what they were relative to, which is not interpretable by an agent. It is an additive field, compatible under the schema policy in docs/agent-cli.md. Memoizing ResolveBaseBranch was the obvious alternative, so I implemented and measured it: it changed nothing. Every remaining repeat resolution in a single process is separated by a gitops.Fetch, which can change the answer and would have to invalidate the cache anyway (`gw apply` = plan, fetch, provision). I reverted it rather than ship a cache with invalidation coupling and no measured benefit, and left the cost note in gitops.go instead, since the durable problem is that these functions read like cheap accessors and are not.
There were two implementations of "reduce a git remote to owner/repo": gitops.ParseRemoteName (string splitting) and announce.NormalizeRepo (regexes, added in this branch). They agreed on common forms and disagreed on case and on non-URL input — and the announce copy exists to build a *coordination key*, so a disagreement would mean two agents on the same repo silently never seeing each other's notes. announce.NormalizeRepo now delegates the parse and keeps only its own policy: lowercasing (so a case-different remote still matches) and the bare-name fallback for repos with no remote. Two bugs surfaced while consolidating: - IsGitURL did not recognize ssh:// — it required a colon *without* a scheme for the scp-like form, and only allowed https/http/file otherwise. So `gw create -r ssh://git@host/org/repo.git` treated the URL as a repo name and failed with "repo not found" instead of cloning. It now accepts explicit git transports (https, http, ssh, git, git+ssh, file) and still requires "@" for the scp-like shorthand. - ParseRemoteName turned "C:/repos/api" into "repos/api", reading a Windows drive letter as scp-like syntax. It was only harmless because callers happened to gate on IsGitURL first; it nearly became load-bearing when announce delegated to it. It now returns "" for anything IsGitURL rejects. Tests: ssh:// with and without a port, git://, git+ssh://, an unknown scheme, a Windows path, and an absolute path joined the existing IsGitURL table; new tests assert every URL form of one upstream reduces to the same identity, that nested GitLab groups survive, and that non-URLs return "" rather than a mangled guess. Also documented the deliberate duplicate `git worktree list` in the create path: validateCreate fails fast before any directory or fetch exists (and lets a plan report WORKTREE_EXISTS), while provisionWorktreeNoFetch re-checks immediately before mutating to guard against a concurrent gw. Removing a guard that protects a mutation to save one subprocess ahead of a network fetch is not a trade worth making.
models.ToJSON had zero callers while ten sites marshalled inline; a helper nobody calls is an invitation to a second convention rather than a shared one. Its test went with it — the behavior it covered (Workspace round-tripping through JSON) is already covered by TestWorkspaceJSONRoundTrip. preset list, preset show, and plugin list each hand-rolled MarshalIndent + "failed to marshal JSON" + Println behind the legacy --json flag; they now use emitLegacyJSON like list/status/doctor/repos, so the deprecated output path has one implementation. Verified byte-identical output against the previous binary for each command.
Output of `openwiki --update`, covering the MCP removal, the versioned machine contract, structured per-repo results, gw context, announcements, and plan/apply. Generated pages, not hand-written — this also replaces the openwiki pages I hand-edited earlier in this branch, which CLAUDE.md asks us not to do. Adds openwiki/index.md and openwiki/INSTRUCTIONS.md, and drops the tool's temporary _plan.md. docs/agent-cli.md remains the authoritative source for the error codes and exit classes; the wiki links to it rather than restating it.
…erly Two parts: what the suite tests, and where it runs. ## Coverage A machine-contract section drives the whole agent surface through JSON only: - envelope invariants across every read command, as a table-driven loop, so a command added without machine support fails here instead of needing its own hand-written test; - stdout purity, proven by giving a repo a setup hook that writes to stdout — it must appear on stderr and never inside the envelope; - a degraded-but-successful sync: ok stays true, the warning rides in the envelope, and the per-repo outcome names the dirty repo and why it skipped; - error codes mapped to exit classes: WORKSPACE_NOT_FOUND 3, WORKSPACE_EXISTS 4, REPO_NOT_FOUND 3, USAGE 2 (missing flag, bad --format, unknown command), STATE_CHANGED 4; - destructive refusals: delete and remove-repo without --force fail as USAGE and leave the workspace intact; - plan/apply including apply-after-change (and that the new work survives), re-plan surfacing the warning the stale plan lacked, never-pushed commits being named, a saved failure envelope being rejected, and plan | apply over a pipe; - the full create → inspect → sync → add → remove → delete lifecycle parsing only JSON, including running a next_actions command verbatim to prove those strings are executable rather than descriptive; - text mode and legacy --json unchanged. Three real defects surfaced while writing it, each fixed with unit tests: - An invalid --format value produced no envelope at all — bare text on stderr in exactly the case where the caller explicitly asked for machine output. Passing --format now enables machine mode even when the value is rejected, so the rejection is parseable. - The delete fingerprint stored only a boolean "is dirty", so work added to an already-dirty repo did not invalidate a reviewed plan. Since coding agents routinely start from a workspace that already has scratch files, this was the common case, not the edge case. It now fingerprints the actual status output and HEAD commit, which also catches a commit made after review — that would previously have left the worktree clean and slipped through entirely. - My earlier assertion assumed only the dirty repo would skip during sync; a second repo skipped for an unrelated reason. Now asserted on the reason. ## Isolation Sandboxing rested entirely on `export HOME`, which is not enough for git: it prefers $XDG_CONFIG_HOME/git/config when that is set, so on a developer machine with XDG configured `git config --global` in the setup block would have edited their real config. The suite now pins GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM (git 2.32+), clears XDG and GIT_DIR-style variables, keeps TMPDIR and all test artifacts inside the sandbox (several of my earlier additions wrote predictable /tmp paths), reaps child processes on exit so a failed run cannot leave `gw run` processes behind, refuses to rm -rf a path it did not create, and asserts the boundary holds before any test relies on it. A container mode runs the same script — not a variant, which would drift — for what sandboxing cannot give: nothing from a host reachable, a pinned git, Linux filesystem semantics, and a non-root user so permission bugs are not masked. CI now runs the suite on ubuntu and macos (macOS matters: path resolution there goes through the /var -> /private/var symlink) plus the container offline with --network=none. Note: the image build is unverified locally — no Docker daemon on this machine — so the e2e-container job is its first real execution. 218 assertions pass.
…derers Building the CLI with `go build -cover -coverpkg=./...` and running the e2e suite against it turns the bash suite into a coverage report, which doubles as a dead-code detector. It found three things I had added speculatively: - workspace.anyFailed — superseded by FailedRepos, never called. - workspace.ErrWorktreeDirty — never called. - machine.Emitted — added to guard against emitting a second envelope, a guard no command ever needed. Its `emitted` bookkeeping went with it. Removing ErrWorktreeDirty made WORKTREE_DIRTY unreachable: no operation could return it, yet docs/agent-cli.md listed it as a code to branch on. A documented code that cannot occur is worse than a missing one — it invites dead branches in client code — so the code and its table row are gone, and the docs now explain where dirtiness actually surfaces instead: `gw sync` reports a per-repo `skipped` outcome with a reason, `gw plan delete` warns about work that would be destroyed, and applying a plan after a repo changed is STATE_CHANGED. Adding a code back later is a compatible change under the policy; advertising a phantom one is not. The envelope example in the docs now uses a code that can actually be received. The profile also showed printContext, printPlan, and humanizeAge at 0% — the human renderers are a separate path from the envelope and nothing exercised them. The e2e suite now asserts the context summary, the plan table with its destructive markers, and announcements with a relative age. e2e-only coverage 57.1% → 59.0%; 65.0% combined with the unit tests. One fix in kind: the new text-mode announcement assertion polluted the later coordination section, since the announcement store is shared for the whole run. It now publishes about a repo no other section uses, and the coordination assertions filter by workspace instead of asserting exact counts, so a later section publishing a note cannot fail an earlier one. 221 assertions pass.
Applying the same audit to code I did not write. The "load workspaces, bail if
none, build choices, PickOne" block existed four times in create/delete/rename/
add-repo/remove-repo, and the copies had drifted apart in a way that matters now
that error codes are contract:
gw rename → INTERNAL (exit 1)
gw remove-repo → NO_WORKSPACES (exit 3)
gw add-repo → NO_WORKSPACES (exit 3)
Same condition, three different answers, so the code told an agent nothing unless
it already knew which command it had run. All three now go through one helper and
report NO_WORKSPACES with a fix and a next action. (gw delete stays USAGE/2 in
machine mode: its condition is a missing NAME argument, not missing workspaces.)
The `--repos` flag was split and trimmed in six places, and every copy kept empty
entries, so `-r "api,"` produced a repo named "" and the error `repo not found`:
before: gw create x -r "api," -b b → REPO_NOT_FOUND: repo not found
after: gw plan create x -r "api, ,web" -b b → repos [api web]
parseRepoList drops blank entries once for create, plan, add-repo, remove-repo,
preset add, and announce.
Both were found by looking for the pattern behind the earlier PathContains fix —
one concept implemented several times drifts, and the drift shows up as
inconsistent behavior rather than as duplication anyone notices.
createCmd.Run was 240 lines in one closure — the largest readability problem in the repo, and in the command humans and agents use most. gocyclo never flagged it because the complexity lived inside a function literal, so the ceiling of 20 that guards every other function did not apply to the worst offender. It is now a 32-line orchestrator over named steps: resolveCreateRepos (with reposFromPreset / reposFromFlag / cloneRepo / reposInteractively / pickPreset / pickRepos / offerPresetSave), requireKnownRepos, resolveCreateBranch, buildCreateOpts, createSource, replaceCurrentWorkspace, failCreate, and the two hook helpers. Highest remaining complexity in the file is 8. The ordering is the part that needed protecting, so it is now stated at the top of Run rather than being implicit in 240 lines of flow: repos are resolved (and possibly cloned) before validation, the branch is resolved after the name argument is read because it seeds the prompt default, and --replace runs after the new name is known but before anything is created — so a name collision is caught before the old workspace is destroyed. Verified with a 19-invocation golden baseline covering every non-interactive path (explicit repos, --all, --preset, unknown preset, derived name, name from branch, duplicate, unknown repo, missing branch, trailing separators, clone URL, --track, source flags, and three --format json cases): stdout, stderr, and exit codes are byte-identical before and after, modulo timestamps. e2e's 221 assertions still pass. The payoff beyond readability is testability: repo-selection precedence (preset > --all > --repos), provenance assembly, --track mode, and explicit-branch handling now have unit tests, where previously nothing in the file could be tested without running the whole command. buildCreateOpts and createSource are at 100%. Two behavior-preserving details worth noting. Declining the --replace confirmation previously returned from the Run closure; it is now an explicit os.Exit(0), which is the same observable outcome (exit 0, nothing created) from a helper that cannot return to skip the rest. The "Pick manually…" sentinel is now a named constant instead of a string repeated in the comparison and the choice list.
The feature entries were there; the defects found while writing the e2e suite and auditing for duplication were not. Also corrects the plan fingerprint description, which now covers each repo's exact changes and commit rather than a dirtiness flag.
Two artifacts outlive the removed MCP server, and only one was handled: the `grove` entry in each workspace's `.mcp.json`, and the announcements SQLite database at ~/.grove/messages.db. On my own machine the database is still there from May, 16 KB plus a -wal and -shm sidecar, and nothing in Grove can read it now that the driver is gone — so it is dead weight rather than data at risk. `gw doctor` now reports and `--fix` removes it, alongside the .mcp.json entries. But doctor only visits workspaces Grove still tracks, which misses the cases most likely to have leftovers: directories abandoned by workspaces removed from state, and checkouts outside the configured workspace directory. So scripts/cleanup-mcp-migration.sh covers those, and works without upgrading first. Design choices worth stating: - It reports and changes nothing unless given --apply, matching the plan/apply posture of the rest of this work. A cleanup that deletes files on first run is not something to paste from a README. - It removes a `grove` entry only when that entry launches `gw mcp-serve`, so an external MCP adapter that happens to be named `grove` is left alone — the same rule internal/workspace/mcpmigrate.go applies. - A `.mcp.json` that is not valid JSON is skipped and reported, never rewritten. - The file is deleted only when Grove's entry was the only thing in it; otherwise the other servers are preserved. - Without jq it refuses to edit JSON and points at `gw doctor --fix` rather than attempting text surgery on a config file. - It reads workspace_dir from config.toml, since the workspace directory need not live under the Grove home, and accepts extra directories to scan. e2e covers the script end to end: dry run changing nothing, the grove-only file removed, other servers preserved, a foreign adapter untouched, an untracked directory reached, the database deleted, and a second run reporting no work. Plus doctor's own path for the database. 231 assertions.
A plan listed the setup commands it would run, but the fingerprint did not cover
them, so approving a plan approved nothing about the code it would execute:
plan says: touch SETUP_RAN_ORIGINAL ← what the reviewer sees
.grove.toml edited to: touch PWNED
gw apply plan.json → ok: true, PWNED created
Everything else a plan describes is git work Grove controls; run_setup_hook and
run_teardown_hook are arbitrary code from a repo's .grove.toml. Those are exactly
what a reviewer is being asked to approve, so plannedCommands now folds them into
both fingerprints and apply refuses with STATE_CHANGED when they change.
Fingerprinting the plan's own displayed commands, rather than re-deriving what to
hash, means the guarantee tracks the plan: whatever a plan claims it will execute
is what it is pinned to.
This is also the mechanism blueprints need (#61: "changed remote setup commands
invalidate prior approval"), where the commands arrive from a shared or remote
file rather than a local repo — the same check, with a blueprint digest and
resolved commit added to the fingerprint input.
docs/agent-cli.md now states what the fingerprint covers per plan kind, and adds
the section that was missing: when plan/apply is worth the extra round trip. It
is for the gap between deciding and doing — human review, arbitrary setup
commands, a concurrent mutator, or irreversible destruction. For an agent that
plans and applies in the same breath with nothing reading the plan, it is
overhead, and `gw delete --force --format json` already returns per-repo results.
Overselling a two-step as universally better would earn it a reputation as
ceremony.
e2e covers the approval path end to end.
The interactive paths in create were the last 0%-coverage code in the agent surface, and not because nobody wrote tests: they need a terminal, so a non-interactive suite could not reach them at all. Choosing a preset, taking the "pick manually" escape hatch, being offered a preset save and declining it, the branch prompt defaulting to the workspace name — all unverified. Prompter is one interface for every interaction that needs a human: Interactive, PickOne, PickMany, Confirm, Prompt. All 25 call sites across 11 files in cmd/ now go through it rather than reaching into picker and console directly, so "asks a human" has one definition instead of two packages' worth. The production implementation is a pure delegation holding no logic, so substituting it in a test cannot change what the code under test does. Machine-mode enforcement stays in picker and console, where a caller that bypasses this seam still cannot block on input. The test double is strict: an unscripted prompt fails the test rather than returning a zero value, because a silent default would let a flow take a branch nobody wrote a case for and still pass. It answers by prompt substring rather than call order, records what was asked, and rejects a scripted answer that is not among the choices actually offered — a test that "picks" an option the user could never see is testing nothing. That strictness immediately caught a wrong assumption of mine: I wrote a test asserting a sole workspace requires no prompt, which is picker's auto-select behavior, not the cmd layer's. The seam stops at the cmd boundary deliberately, so that shortcut stays picker's responsibility (and its own tests); the test now asserts what the cmd layer offers instead. Coverage of the previously unreachable paths: reposInteractively 100%, pickPreset 87%, offerPresetSave 86%, resolveCreateBranch 89%, pickWorkspaceName/Names 75%. Behavior is unchanged: the 19-invocation create baseline is still byte-identical to the pre-refactor binary, and e2e passes 237 assertions.
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.
Closes #63.
Makes the
gwCLI the only first-party agent interface: a versioned machinecontract, structured results, discovery, coordination, and a review step for
destructive work — and removes the built-in MCP server.
Why not MCP
MCP was a second interface to a small set of features for users who already have
shell access. It cost a JSON-RPC implementation,
.mcp.jsonlifecycle management,a SQLite announcements database, and its own failure modes — without providing a
trust boundary the CLI doesn't already have.
Its one genuine feature was cross-agent coordination, so that came back as CLI
commands (see below) rather than being dropped: coordination came from shared
state on disk, not from the protocol.
What an agent gets
ok,schemaVersion,result/error,warnings,next_actions. One JSON document on stdout; progress, warnings, and hook outputon stderr. 15 stable error codes mapped to 8 semantic exit classes, with a
documented compatibility policy in
docs/agent-cli.md.Multi-repo work is partially failable, so "did it work?" isn't a boolean:
outcomes come from a fixed vocabulary (
created,already_present,rebased,up_to_date,skipped,not_found,failed, …) with a reason attached.gw context— one read-only call answering "where am I and what can I do?".A projection over config, state, and local git; it shares
RepoStatuswithgw statusso the two cannot disagree.gw plan/gw apply— plans enumerate every repo, path, and branch, markdestructive changes, and carry a fingerprint of the state they assume. Apply
recomputes it and refuses with
STATE_CHANGEDrather than executing against aworld that moved.
gw announce/gw announcements— cross-workspace coordination forparallel agents, and recent notes surface in
gw context, so an agent receivescoordination while orienting instead of having to remember a tool exists. That
fixes the reason the MCP version went unused: 0 rows in
messages.dbaftermonths of real use.
machine mode, and destructive commands require
--forcerather than treating aprompt they cannot show as consent.
Human output, interactive pickers, and the legacy
-j/--jsonshapes are unchanged;-jis marked deprecated but still byte-identical for existing scripts.Removals and migration
internal/mcp/,gw mcp-serve,.mcp.jsongeneration,models.MCPConfig, andthe
modernc.org/sqlitedependency tree are gone. Release binary13.03 MB → 9.10 MB (-30%).
Migration is handled rather than assumed:
gw doctor --fixstrips the stalegroveentry (preserving other MCP servers) and deletes the orphaned~/.grove/messages.db.scripts/cleanup-mcp-migration.shcovers what doctorcannot reach — untracked directories and trees outside the workspace dir — and is
dry-run by default.
Defects found and fixed along the way
Most of these were found by writing the tests, not by reading the code:
rev-list origin/<branch>IsGitURLdidn't recognizessh://gw create -r ssh://git@host/org/repo.gitfailed with "repo not found" instead of cloning--formatproduced no envelopegw renamereportedINTERNAL/1 where others reportedNO_WORKSPACES/3-r "api,"produced a repo named""repo not foundParseRemoteName("C:/repos/api")→"repos/api"stateandgw contextcould disagree about where the caller isgw contextresolved each base branch twiceTesting
stdout purity under a hook that writes to stdout, every error-code→exit-class
pair, plan/apply recovery, the JSON-only lifecycle, and the migration script.
go build -cover -coverpkg=./...makes the bashsuite report real numbers — 59% from e2e alone, 65% combined. It doubles as a
dead-code detector; it found three unused helpers I'd added and one documented
but unreachable error code, all removed.
export HOME, which git ignores whenXDG_CONFIG_HOMEis set — so on adeveloper machine with XDG configured it could have modified the real
gitconfig. It now pins
GIT_CONFIG_GLOBAL/GIT_CONFIG_SYSTEM, keeps allartifacts inside the sandbox, reaps child processes, and asserts the boundary
before any test trusts it.
through the
/var→/private/varsymlink) plus offline in a container(
--network=none, non-root).just checkclean: tests, vet, gofmt,gocyclo -over 20, staticcheck.Refactors, kept separate from behavior
Three commits are pure refactors, each verified behavior-preserving:
createCmd.Run: 240 lines → a 32-line orchestrator over named steps.gocyclonever flagged it because the complexity lived in a closure. Verifiedwith a 19-invocation golden baseline (stdout + stderr + exit codes byte-identical).
state.PathContains,announce.Store.each,parseRepoList,pickWorkspaceName,shortenPath— each replacing 2–6 divergent copies.list -s,ws show,repos, andcontextverifiedbyte-identical against the previous binary.
Notes for review
WORKTREE_DIRTYis deliberately not in the code catalog. The issue's exampleuses it, but no operation returns it:
gw syncreports dirtiness as a per-reposkippedoutcome, and applying a plan after a repo changed isSTATE_CHANGED.A documented code that cannot occur invites dead client branches.
ResolveBaseBranchwas implemented, measured, and reverted. Everyremaining repeat resolution is separated by a
Fetchthat must invalidate it, soit bought nothing; the fix was data flow instead. Cost note left in
gitops.go.machine) — the
e2e-containerCI job is its first real run.rollback semantics underneath belong with Epic: Make workspace operations transactional and recoverable #59.
gw grabcomposes this contract.