fix(join): gate on provisioned agents, and support multi-agent containers - #454
Merged
Conversation
Adds CanonicalAgent, KnownAgentNames, and ValidateAgent so an unknown agent: value (e.g. a hallucinated or project-shaped string) is caught and cleared with a warning instead of silently disabling agent-specific behavior. Also adds ui.Writer() to pair with ui.SetWriter, since SetWriter has no "reset to default" sentinel — passing nil leaves the writer nil and panics on the next Warn/Error/Info call. Tests capture the prior writer via ui.Writer() and restore it instead.
copilot and pi's ConfigureAgent hooks unconditionally overwrite cfg.Agent with their own name before ResolveAgentField's conflict check runs, so a conflicting agent: field in moat.yaml silently produced no warning for those two commands (the check compared the provider name against itself). Snapshot cfg.Agent before ConfigureAgent runs and restore it before resolving, so the warning fires uniformly across all six provider commands.
Add the AgentRuntime interface (DefaultDependencies, NetworkHosts, CredentialGrant) so agent providers can be provisioned into a container declaratively via moat.yaml agents: (Task 10). CredentialGrant is a static per-provider constant, not a delegation to the existing GetCredentialName() store-probing funcs — those answer which credential is configured right now, not which grant an agent needs. codex maps to the openai grant (not codex, where GetCredentialName would resolve). pi deliberately does not implement the interface: its grant is resolved per-invocation from flags, config, and the store, so there is no static answer. Also fix registry_test.go: several tests call Clear() on the shared provider registry without restoring it, permanently wiping the real provider registrations (populated once via init()) for the rest of the test binary. This only surfaced now because the new runtime_test.go hard-asserts real registrations exist, running after registry_test.go alphabetically. Snapshot/restore the registry around each Clear().
Expands moat.yaml's agents: list into the dependencies, grants, and network rules each named agent needs. Unlike agent:, unknown entries are a hard error since a silently dropped agent leaves the container short a credential and its firewall rules, surfacing only much later as an opaque join refusal.
- inferJoinCandidates now sorts by CreatedAt (newest-first) before partitioning into local/widened, since manager.List() iterates a map and is otherwise unordered per call. Without this, the picker's slice-index numbering shuffled between invocations, letting a remembered selection attach to a different run (and, when widened, a different workspace's grants). - pickJoinRun gains an anyRunning parameter so the zero-candidate error can distinguish nothing running at all (start a run) from running runs that exist but can't host this agent (recreate one with it in moat.yaml's agents: list) - these imply different next steps. - Strengthened the single-WIDENED-candidate test to assert on the picker's actual output instead of just its return value, so deleting the widened guard is caught.
Provisions a real container via moat.yaml's agents: [claude, codex], asserts both land in the persisted joinable_agents set, and drives real `moat join <run> claude` / `moat join <run> codex` through the real binary to prove both get past the capability gate and reach a live process in the same container. TestJoinHeadless only covers the single-agent case.
The negative "cannot host" check alone passes vacuously if the moat join subprocess never launches or the joined process crashes before printing anything. Add a fatal non-empty-output check alongside it so the test requires real evidence a live process was reached, not just the absence of the literal rejection string.
…dential `agents: [claude, ...]` injected "claude" straight into cfg.Grants, which buildGrants treats as an explicit user grant and uses to suppress an auto-detected credential. A user whose Anthropic credential is stored as an API key under `anthropic` (a supported, documented fallback) would have it discarded in favor of the OAuth-only "claude" grant the expansion added, forcing a login they never asked for even though their existing credential already worked. ExpandAgents now returns derived grants instead of mutating cfg.Grants, and buildGrants takes them as a fourth, lowest-precedence input: they never suppress an auto-detected grant, and are skipped when an equivalent credential (the claude/anthropic pair) is already present. moat run merges the returned grants into its own default-grants logic, since it has no buildGrants precedence chain of its own.
moat wt loads moat.yaml and is documented as following "the same pattern as moat run", but it never called ExpandAgents or ResolveAgentField — the two hooks every other run-creating entry point (moat run, the provider verbs via RunProvider) uses. Two consequences: `agents:` was a silent no-op under `moat wt` (no extra deps, grants, or network rules, so `moat join` refused with advice the user had already followed), and an invalid `agent:` value kept silently disabling container memory defaults, implied dependencies, and language-server support — the original bug this branch set out to fix, just unfixed on this one path. Both calls are added in the same relative positions run.go uses: ExpandAgents after the worktree config reload and before the grants-defaulting block, ResolveAgentField right before ExecuteRun. runWorktree has no existing unit tests (neither does run.go's runAgent) — it isn't unit-testable in its current shape without a git repo, run manager, and cobra command harness. Verified the ordering by reading against run.go's placement instead.
`moat join <run> openai` is valid — provider.GetAgent resolves it through the openai->codex alias — but every remedy message interpolated the raw argument, so the suggested fix was `moat openai`, which is not a command. The diagnosis half of each message keeps the string the user typed, so they recognize what they asked for; the remedy half (`moat <name>` / `agents: [<name>]`) now uses the canonical name resolved via agent.Name(). Adds an `openai` row to TestValidateJoinAgent. A prior review believed the `j.IdentifiesAs(a)` branch in the membership loop was unreachable and proposed deleting it; this case proves it's load-bearing — `moat join <run> openai` is accepted only through that branch, since agentArg stays "openai" while JoinableAgents holds "codex".
…ide of agents: The multi-agent guide and moat.yaml reference both claimed that with no agent: set, moat run launches agents[0] in the foreground. It doesn't: moat run with no `-- command` and no `command:` in moat.yaml runs /bin/bash (internal/run/manager_create.go). agents[0] only backfills agent:, which drives agent-specific defaults (container memory, implied dependencies, language-server support) — nothing constructs an agent invocation from it. This branch exists partly because agent: was mis-documented as something it wasn't; the docs now describe agents[0]'s actual effect and point at the agent's own verb (moat claude) for running it in the foreground. Also documents that --grant on moat run / moat wt replaces moat.yaml's configured grants rather than adding to them, including grants agents: derives — previously undocumented, and now the first case where it silently drops something a user's moat.yaml declares. This is existing flag behavior; changing it to union is out of scope here.
The Unreleased summary paragraph covered only the Codex/TTY work and never mentioned agents: or the moat join capability-gating fix, even though it's a headline feature of this release.
ExpandAgents returns derived grants instead of appending them to cfg.Grants (a33e6f6) so buildGrants can't wrongly treat them as explicit and suppress an auto-detected credential. But nothing wrote the returned grants back, so two downstream readers that consult cfg.Grants directly silently stopped seeing agent-derived grants: Config.ShouldSyncCodexLogs/ShouldSyncGeminiLogs (dropping the session-transcript host mount) and buildLocalMCPConfig's grant validation (hard-failing run creation for a local MCP server's declared grant). Add AppendDerivedGrants and call it after grant precedence resolution completes at all three call sites, so the write-back can't re-enter buildGrants' suppression logic.
Contributor
📦 Previous Review (superseded by newer review)Claude finished @dpup's task in 11m 59s —— View job Code reviewReviewed the diff against Five findings posted as inline comments, roughly in severity order:
Nothing security-sensitive stood out — the proxy/credential paths aren't touched here, and the new Docs ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- Gate the not-in-`agents:` warning on the no-verb path. With a CLI verb, RunProvider provisions that agent's dependencies, grants, and network hosts independently of `agents:`, so the warning's claim was false there, not just misleadingly worded — and it fired alongside the conflict warning. - Add provider.AgentAliases() and include agent aliases in KnownAgentNames, so `openai` appears in the valid-values list that already accepts it. - Extract the ExpandAgents -> grants-merge -> AppendDerivedGrants sequence into intcli.ApplyAgentDefaults, shared by `moat run` and `moat wt`. The ordering invariant is now documented once instead of duplicated at both call sites. - Add a drift guard tying agentCLIDep to the AgentRuntime implementors, so a new provider that forgets an entry fails loudly instead of becoming silently unjoinable. - Fix noun agreement in the picker's single-candidate non-TTY error. Note: pre-existing failure in internal/deps.TestRegistryGithubBinaryURLsExist not addressed by this PR.
Contributor
📦 Previous Review (superseded by newer review)Claude finished @dpup's task in 2m 5s —— View job Review in progress
|
Four findings from a clean-room review of the multi-agent join work. KnownAgentNames advertises the provider aliases (openai, google) as valid `agent:` values and ValidateAgent accepts them, but ResolveAgentField never normalized them. Every downstream consumer matches with strings.HasPrefix(cfg.Agent, "<canonical>") — isAIAgent's container-memory default, agentImpliedDependencies, the language_servers gate, copilot init, pi staging — and "openai" prefix-matches none of them. So an alias moat itself advertises passed validation and then silently switched those defaults off, which is the degradation this code exists to prevent. `agents: [openai]` reached the same place via the Agents[0] backfill. ResolveAgentField now canonicalizes on the way out; ValidateAgent stays pure validation. resolveProviderAgentField (and its moat run / moat wt equivalents) sat after the dry-run return, so --dry-run — the flag people reach for to check a moat.yaml before committing to a run — was the one path that never surfaced the bad-`agent:` warning. Moved above the return at all three call sites. The `doc:` tag on `agents:` claimed the first entry is "the foreground agent" for moat run, contradicting both doc pages and manager_create.go, which defaults to /bin/bash when no command is configured. That tag is rendered into moat init's LLM prompt via GenerateSchemaReference, i.e. the same docs-as-bad-value-generator mechanism this branch fixes for `agent:`. The Agent doc tag and KnownAgentNames also listed different "valid" sets; a new drift guard asserts every accepted name appears in the tag.
Contributor
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.
Why
moat joinrefused to attach to containers that were demonstrably running the agent. The gate comparedmoat.yaml'sagent:field against two hardcoded strings — and that field was unvalidated and actively mis-documented.docs/content/reference/02-moat-yaml.mddescribed it as a free-form identifier defaulting toname, so users andmoat init's config generator both wrote project-shaped values likeagent: vibrant-code.A bogus value silently disabled five behaviors beyond join: container memory defaults, implied dependencies, language-server support, copilot init, and pi staging.
Two further causes surfaced while fixing it:
moat.yaml'sname:is copied into every run's name — so any project settingname:gives all its runs one name. Joining by name then hard-errored withmatches 2 runs.JoinableAgent, somoat join <run> codexfailed even in a codex-started run.What changed
Part A — join gates on what was actually provisioned. A
joinable_agentsset (agents whose config was staged ∩ agents whose CLI was installed) is computed at create time and persisted to run metadata.agent:is validated against the provider registry, the CLI verb is authoritative over it, and run resolution filters to running runs.Part B — one container, several agents. A new
provider.AgentRuntimeinterface with a staticCredentialGrant(), and amoat.yamlagents:list that expands into each agent's dependencies, grant, and network rules. Codex gains aJoinableAgentimplementation.Part C —
moat join <agent>. The run is inferred from running runs in the current workspace, capability-filtered, and sorted newest-first. An interactive picker handles ambiguity, and always prompts when the search widened past the current workspace — attaching there borrows another run's credentials.Behavior changes worth knowing
moat joinrejects 3+ positional arguments (RangeArgs(1,2));MinimumNArgs(2)silently ignored extras.--grantonmoat run/moat wtstill overrides rather than unions, so it discardsagents:-derived grants. Documented rather than changed — altering flag semantics deserves its own decision.Notable fixes found along the way
moat copilotandmoat pinever warned on a conflictingagent:, because theirConfigureAgenthooks overwritecfg.Agentbefore validation ran.internal/provider/registry_test.gocalledClear()on the global registry without restoring it, wiping init-time registrations for the rest of the test binary. Invisible until a test hard-asserted instead of skipping.moat wtwas missing both config hooks entirely —agents:was a no-op there and the originalagent:bug was unfixed on that path.PopulateStagingDirwrites a placeholder; the proxy injects the real key). It now has thecodex-clidependency fallback the other four already had. This changes image hashes for projects withcodex-cli, so expect one rebuild.Testing
38 packages green; lint clean.
TestDualAgentJoin_E2Eprovisions a real dual-agent container and joins it as both agents, verified against live containers.internal/deps.TestRegistryGithubBinaryURLsExistfails in sandboxes without GitHub network access — pre-existing, and this branch has zero changes to that package.Known follow-ups
AppendDerivedGrantscall sites — deleting any one leaves the suite green.run.NewManager()probes for gVisor unconditionally, somoat joinneeds a sandbox escape hatch despite never creating a container.runJoinaccumulated four tasks' worth of edits; extractingresolveJoinTargetwould also make the picker wiring testable.🤖 Generated with Claude Code