fix(fleet): let ps/install work on a roster-v2 fleet, and refuse add/remove honestly (#1237 piece A) #1243

Merged
fred merged 3 commits from fix/1237-fleet-v2-dispatch into next 2026-08-16 18:06:36 +00:00
Collaborator

Closes part of #1237.

What was broken

On a greenfield box with a roster-v2 fleet, mosaic fleet ps, install, install-systemd, add and remove all failed with:

Fleet roster has unknown field(s): generation.

They parse the roster through the v1 parser, which rejects v2 outright. install is what places the systemd units, so a fresh v2 box could never get a seat installed at all.

What this does

  • ps / install / install-systemd — read the roster through a narrow read-only view (FleetRosterReadModel: version, socket name, holder session, and per agent name/alias/runtime) that is filled by parseRosterV2 on v2 and the v1 loader on v1.
  • install on v2 writes no generated env. The reconciler owns fleet/agents/<name>.env.generated through projectRosterV2AgentGeneratedEnv. Writing it here too — necessarily through the v1 generateAgentEnvValues mapping — would be a second projection mapping writing one file, which is exactly the #791 single-SSOT invariant. install prints which command does own it.
  • add / remove refuse on v2, naming the real two-step sequence (create/delete --expected-generation, then apply) and where to read the current generation.
  • [email protected] gets ConditionPathExists for the agent's generated env.

Why not project v2 down to a v1 roster

That was the first design and it is wrong. A downshifted FleetRoster is accepted by generateAgentEnvValues, so it would become a third writer of the generated env through the v1 mapping. The read model is deliberately too small to write a roster or an env file back from, so the misuse is structurally unavailable rather than discouraged.

Why add/remove refuse instead of dispatching

I described this as a stable rename to the reviewer three times and it is not. The v1 pair edits the roster and drives systemd; the v2 pair changes desired state without runtime actions and leaves convergence to apply. add collects four fields; AGENT_REQUEST_KEYS (fleet-agent-crud-command.ts:35-48) needs eleven. Two of the invented ones would be provider and toolPolicy — as scooby put it, "silently defaulting a tool policy for an operator is not a convenience, it is choosing an agent's privilege surface without telling them."

The boot window, and why the Condition is here

Caught by scooby in review, not by me. install enables the unit with plain systemctl --user enable (no --now), the unit is WantedBy=default.target, Type=oneshot, no Restart=, and install now writes no env on v2. So install → reboot → (no apply yet) would run ExecStart against an absent env file and fail every seat unit once.

Measured on canary rather than argued:

start rc Result ExecMainStatus state
with the condition 0 success inactive, ConditionResult=no, journal "skipped, unmet condition check"
condition removed by drop-in, nothing else changed 1 exit-code 64 failed, "agent environment rejected: code=missing-file"

So the window was real — the launcher hard-fails on a missing generated env — and a skipped unit is the honest state for "enabled but not yet configured". systemd re-evaluates on every start, so the seat comes up on the next start once the reconciler has written env. On v1 it is a no-op, since install writes env.

Evidence

Canary (mosaic-sbx-canary, roster v2 generation 3), installed 0.0.50-next.2413 vs this branch, same roster, side by side:

command 0.0.50-next.2413 this branch
fleet ps rc=1 unknown-field rc=0, 3 agents listed
fleet install --no-enable rc=1 unknown-field rc=0
fleet remove <name> rc=1 unknown-field rc=1, refusal naming delete + apply
fleet add <name> ... rc=1 unknown-field rc=1, refusal naming create + apply

The #791 negative was measured too: the six existing *.env.generated files were untouched by install (mtimes 20+ minutes older than the run).

Gates: typecheck 0 errors, eslint 0, prettier clean, fleet specs 382 passed. New spec fleet-roster-v2-dispatch.spec.ts is 10/10 with the fix and 9/10 red against origin/next (the 10th passes there for an unrelated reason and is annotated in the file). Full suite: only src/mutator-gate/mutator-gate.acceptance.spec.ts fails, which is pre-existing on origin/next — verified by running the suite against a baseline, and its failure count varies run to run.

Read this before verifying

A correct fix here shows install rc=0 and start rc=0 and still no live seat. #1240 (tmux is absent and neither the installer nor doctor mentions it) is upstream of this; #1241 (start reports lifecycle: complete over dead panes) and the missing agent runtime are downstream. A dead pane after this change is not a regression in this PR.

Scope

This is Piece A only: making the v1-only commands work on a v2 roster. Piece B — which producer emits v2, and what upgrade does to existing v1 installs — is a design decision I am not taking unilaterally.

Review

Reviewed over git comms by scooby, who cannot file a Gitea review from fomo-lin and will not borrow a token, so their sign-off is carried here. They found the boot window (adopted), pushed back on the read model and then withdrew it with a structural argument (a view that reuses the producer's parser cannot drift in the parse dimension, and #791 forbids two projection mappings writing one file — this writes nothing), and withdrew their earlier green-light on add/remove dispatch once I corrected the false premise I had given them.

Two self-inflicted defects are in the history rather than squashed out, because both are the same failure mode as the bug itself — a check that passed without measuring anything:

  • 67f5014c dropped the ConditionPathExists line. git checkout origin/next -- <file> (my own red-check) writes the index, not just the working tree; copying my version back restored the tree only, so the commit took the index. The spec reads the working tree, so it stayed green against a HEAD that no longer had the guard. Restored in 6f5b4c3d with the per-commit occurrence counts recorded.
  • The v2 refusals were bare throws, which reach the CLI top level uncaught and print the guidance under a Node stack trace. The message is the entire point of a refusal. Found on canary; the unit tests asserted the text and passed either way. They now go through command.error().
Closes part of #1237. ## What was broken On a greenfield box with a roster-v2 fleet, `mosaic fleet ps`, `install`, `install-systemd`, `add` and `remove` all failed with: ``` Fleet roster has unknown field(s): generation. ``` They parse the roster through the v1 parser, which rejects v2 outright. `install` is what places the systemd units, so a fresh v2 box could never get a seat installed at all. ## What this does - **`ps` / `install` / `install-systemd`** — read the roster through a narrow read-only view (`FleetRosterReadModel`: version, socket name, holder session, and per agent name/alias/runtime) that is filled by `parseRosterV2` on v2 and the v1 loader on v1. - **`install` on v2 writes no generated env.** The reconciler owns `fleet/agents/<name>.env.generated` through `projectRosterV2AgentGeneratedEnv`. Writing it here too — necessarily through the v1 `generateAgentEnvValues` mapping — would be a second projection mapping writing one file, which is exactly the #791 single-SSOT invariant. `install` prints which command does own it. - **`add` / `remove` refuse on v2**, naming the real two-step sequence (`create`/`delete --expected-generation`, then `apply`) and where to read the current generation. - **`[email protected]` gets `ConditionPathExists`** for the agent's generated env. ## Why not project v2 down to a v1 roster That was the first design and it is wrong. A downshifted `FleetRoster` is accepted by `generateAgentEnvValues`, so it would become a third writer of the generated env through the v1 mapping. The read model is deliberately too small to write a roster or an env file back from, so the misuse is structurally unavailable rather than discouraged. ## Why `add`/`remove` refuse instead of dispatching I described this as a stable rename to the reviewer three times and it is not. The v1 pair edits the roster **and** drives systemd; the v2 pair changes desired state without runtime actions and leaves convergence to `apply`. `add` collects four fields; `AGENT_REQUEST_KEYS` (fleet-agent-crud-command.ts:35-48) needs eleven. Two of the invented ones would be `provider` and `toolPolicy` — as scooby put it, "silently defaulting a tool policy for an operator is not a convenience, it is choosing an agent's privilege surface without telling them." ## The boot window, and why the Condition is here Caught by scooby in review, not by me. `install` enables the unit with plain `systemctl --user enable` (no `--now`), the unit is `WantedBy=default.target`, `Type=oneshot`, no `Restart=`, and install now writes no env on v2. So `install` → reboot → (no `apply` yet) would run `ExecStart` against an absent env file and fail every seat unit once. Measured on canary rather than argued: | | start rc | Result | ExecMainStatus | state | |---|---|---|---|---| | with the condition | 0 | success | — | inactive, `ConditionResult=no`, journal "skipped, unmet condition check" | | condition removed by drop-in, nothing else changed | 1 | exit-code | 64 | **failed**, "agent environment rejected: code=missing-file" | So the window was real — the launcher hard-fails on a missing generated env — and a skipped unit is the honest state for "enabled but not yet configured". systemd re-evaluates on every start, so the seat comes up on the next start once the reconciler has written env. On v1 it is a no-op, since install writes env. ## Evidence Canary (`mosaic-sbx-canary`, roster v2 generation 3), installed `0.0.50-next.2413` vs this branch, same roster, side by side: | command | 0.0.50-next.2413 | this branch | |---|---|---| | `fleet ps` | rc=1 unknown-field | **rc=0**, 3 agents listed | | `fleet install --no-enable` | rc=1 unknown-field | **rc=0** | | `fleet remove <name>` | rc=1 unknown-field | rc=1, refusal naming `delete` + `apply` | | `fleet add <name> ...` | rc=1 unknown-field | rc=1, refusal naming `create` + `apply` | The #791 negative was measured too: the six existing `*.env.generated` files were untouched by `install` (mtimes 20+ minutes older than the run). Gates: typecheck 0 errors, eslint 0, prettier clean, fleet specs 382 passed. New spec `fleet-roster-v2-dispatch.spec.ts` is 10/10 with the fix and **9/10 red against `origin/next`** (the 10th passes there for an unrelated reason and is annotated in the file). Full suite: only `src/mutator-gate/mutator-gate.acceptance.spec.ts` fails, which is pre-existing on `origin/next` — verified by running the suite against a baseline, and its failure count varies run to run. ## Read this before verifying **A correct fix here shows `install` rc=0 and `start` rc=0 and still no live seat.** #1240 (tmux is absent and neither the installer nor `doctor` mentions it) is upstream of this; #1241 (`start` reports `lifecycle: complete` over dead panes) and the missing agent runtime are downstream. A dead pane after this change is not a regression in this PR. ## Scope This is Piece A only: making the v1-only commands work on a v2 roster. Piece B — which producer emits v2, and what `upgrade` does to existing v1 installs — is a design decision I am not taking unilaterally. ## Review Reviewed over git comms by **scooby**, who cannot file a Gitea review from fomo-lin and will not borrow a token, so their sign-off is carried here. They found the boot window (adopted), pushed back on the read model and then withdrew it with a structural argument (a view that reuses the producer's parser cannot drift in the parse dimension, and #791 forbids two projection mappings *writing* one file — this writes nothing), and withdrew their earlier green-light on `add`/`remove` dispatch once I corrected the false premise I had given them. Two self-inflicted defects are in the history rather than squashed out, because both are the same failure mode as the bug itself — a check that passed without measuring anything: - `67f5014c` dropped the `ConditionPathExists` line. `git checkout origin/next -- <file>` (my own red-check) writes the index, not just the working tree; copying my version back restored the tree only, so the commit took the index. The spec reads the working tree, so it stayed green against a HEAD that no longer had the guard. Restored in `6f5b4c3d` with the per-commit occurrence counts recorded. - The v2 refusals were bare `throw`s, which reach the CLI top level uncaught and print the guidance under a Node stack trace. The message is the entire point of a refusal. Found on canary; the unit tests asserted the text and passed either way. They now go through `command.error()`.
fred added 3 commits 2026-08-16 04:37:22 +00:00
On a roster-v2 fleet, `ps`, `install`, `install-systemd`, `add` and `remove`
all failed in the v1 parser. The consequence was that a greenfield v2 box could
never get its unit templates placed, so nothing downstream could start.

The read-only commands get a narrow version-agnostic view of the roster
(version, socket name, holder session, and per agent name/alias/runtime).
This is deliberately not a v2 -> v1 downshift. A downshifted FleetRoster would
be accepted by generateAgentEnvValues, which would make a third writer of
fleet/agents/<name>.env.generated through the v1 mapping and break the #791
single-SSOT invariant that projectRosterV2AgentGeneratedEnv is documented to
hold. The view is too small to write a roster or an env file back from, so that
misuse is unavailable rather than merely discouraged.

So on a v2 roster `install` places the tool files and the unit templates,
enables the units, and writes no generated env at all. Env belongs to `apply`
and `regen`, both already v2-native.

That change alone would have traded an init-time failure for a boot-time one.
`install` enables mosaic-agent@<name>.service (WantedBy=default.target) without
starting it, so a reboot between `install` and the first `apply` would run
ExecStart against an absent env file and fail every seat unit, further from its
cause. The unit template now carries

  ConditionPathExists=%h/.config/mosaic/fleet/agents/%i.env.generated

which skips an enabled-but-unconfigured unit cleanly and starts it on the next
start once the reconciler has written env. On v1 it is a no-op, since v1
`install` writes env itself. Found in review by scooby.

`add` and `remove` are not routed to `create` and `delete`. They are different
operations: the v1 pair edits the roster and drives systemd, the v2 pair is
documented as changing desired state without runtime actions. `add` also
collects four fields where a v2 agent requires eleven, so routing it would mean
inventing an operator's provider, alias, reasoning and tool policy. On v2 both
now fail with the real two-step sequence instead.

Tests: 8 new, 7 of which are red before this change. Includes the greenfield
case scooby asked for — `ps` on a fresh v2 install with nothing running is rc=0
and lists every agent stopped, since that is the command an operator runs to
find out why there is no seat.

Note for anyone verifying this: a correct fix here shows `install` rc=0 and
`start` rc=0 and still no live seat. #1240 (tmux absent) is upstream, #1241
(start reports lifecycle-complete over dead panes) and the missing agent
runtime are downstream. A dead pane after this change is not a regression here.

Refs #1237, #791, #1240, #1241
Two follow-ups from the canary red->green run and scooby's review.

1. The v2 refusal in `add`/`remove` was a bare `throw`, which reaches the CLI
   top level uncaught and prints the guidance under a Node stack trace. The
   message *is* the point of the refusal, so it now goes through
   `command.error()` — the same clean path the roster-config error uses.
   Caught on canary, not in review: the unit tests asserted the message text
   and passed either way.

2. The unit-template test asserted only that ConditionPathExists is present.
   Presence is not effect. Added two tests for the parts that can drift in
   code while that assertion still passes: the condition resolving to exactly
   the file the fleet writes (%h/%i rendered against a real install), and the
   launcher genuinely failing on an absent generated env (exit 64,
   `missing-file`) — which is what makes the condition load-bearing rather
   than decorative.

systemd is not available in the suite, so the effect itself was measured on
canary (2026-08-16), roster v2 generation 3:

  with the condition:    start rc=0, Result=success, ConditionResult=no,
                         journal "skipped, unmet condition check"
  condition removed by
  drop-in, nothing else: start rc=1, Result=exit-code, ExecMainStatus=64,
                         unit failed, "agent environment rejected: missing-file"

Canary red->green for the three commands, same v2 roster, side by side:

  fleet ps               0.0.50-next.2413 rc=1  ->  branch rc=0 (3 agents listed)
  fleet install          0.0.50-next.2413 rc=1  ->  branch rc=0
  fleet remove <name>    0.0.50-next.2413 rc=1  ->  branch rc=1, refusal naming
                                                    delete + apply

All three previously failed with "Fleet roster has unknown field(s):
generation." The #791 negative was measured too: the six existing
*.env.generated files were untouched by `install` (mtimes 20+ minutes older
than the run).

Gates: typecheck 0, eslint 0, prettier clean, fleet specs 382 passed, new spec
10/10 with the fix and 9/10 red against origin/next (the 10th passes there for
an unrelated reason and is annotated as such). Full suite: only
mutator-gate.acceptance.spec.ts fails, pre-existing on origin/next.

Still true and still worth saying: a correct fix here shows install rc=0 and
start rc=0 and STILL no live seat. #1240 (tmux absent) is upstream, #1241
(start reports lifecycle-complete over dead panes) and the missing agent
runtime are downstream.

Refs #1237
Reviewed-by: scooby (by git comms; cannot file a Gitea review from fomo-lin)
Self-inflicted and worth recording rather than quietly amending.

To prove the new tests were red without the fix I ran
`git checkout origin/next -- <fleet.ts> <[email protected]>`. That writes
the *index*, not just the working tree. Copying my versions back afterwards
restored the working tree only, so the unit file sat staged-as-origin/next and
modified-in-tree, and the next commit (67f5014c) committed the index — silently
removing the ConditionPathExists line that 463745e3 had added.

Nothing caught it. The spec reads the file from the working tree, so it stayed
10/10 green against a HEAD that no longer had the guard. Found by reading
`git status` after the push, not by any gate.

Verified by content, not by assumption:
  origin/next  0 occurrences
  463745e3     1
  67f5014c     0   <- the regression
  this commit  1

Refs #1237
Author
Collaborator

Included in the greenfield composite run (e2e-compose) — see the measured table in the comment on #1244. Note this run did not exercise the v2-roster dispatch specifically: mosaic fleet init --write emits roster.yaml and the install/start path accepted it without hitting the v1/v2 branch. Piece B (which producer emits v2, and what upgrade does) remains an owner decision and is untouched by this evidence.

Included in the greenfield composite run (`e2e-compose`) — see the measured table in the comment on #1244. Note this run did not exercise the v2-roster dispatch specifically: `mosaic fleet init --write` emits `roster.yaml` and the install/start path accepted it without hitting the v1/v2 branch. Piece B (which producer emits v2, and what `upgrade` does) remains an owner decision and is untouched by this evidence.
fred merged commit 14cb9c6a1e into next 2026-08-16 18:06:36 +00:00
Sign in to join this conversation.