fleet/fleetd
3
0
Fork 0
Code Issues 73 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fleetd #424: make architect-slot identity checks read fleet.architects live #428

Closed
agent wants to merge 0 commits from worker/424-architect-slot-hot-038b41-7 into main
pull from: worker/424-architect-slot-hot-038b41-7
merge into: fleet:main
Conversation 2 Commits - Files Changed -
agent commented 2026-09-10 07:05:57 +02:00
Member
Copy Link
Copy Source

fleetd #424 — revoking an architect slot does not revoke it

The defect

MemberRegistry flattened fleet.architects into a frozen map at construction and never
re-read config. reserve()/requireSlotFor() (the two spawn-side checks) matched against that
frozen snapshot forever, so removing an architect slot from config never actually refused a later
spawn asking for it — an architect is materially more privileged than a worker, and this left a
"revoked" privilege open with no warning. ConfigRef also told the operator an architects-only
edit was "already applied" through the wrong consumer (CompositePeerLauncher, which only reads it
for placement, not identity).

The fix — binding semantic

Implemented the lead's ruling: config governs what may be bound next; it never retroactively
unbinds a live session.

  • MemberRegistry.live(Supplier<FleetConfig.Fleet>) — a new static factory — re-flattens
    fleet.architects/developers/reviewers on every slots()/slotsFor() call. reserve() and
    requireSlotFor() both read through slotsFor, so both are now live with no restart. The old
    single-arg MemberRegistry(FleetConfig.Fleet) constructor is kept (frozen) for tests and for a
    fixed/code-built config.
  • A slot removed from config while a terminal is bound to it keeps that binding — nothing is
    unbound, nothing is killed.
  • To keep the bound terminal's identity too — CallerResolver.resolve calls
    roleForSlot/nameForSlot on every request from a bound pane to confirm it is still an architect
    slot — every successful bind now caches the slot's Entry into a new boundEntries map.
    roleForSlot/nameForSlot/profileForSlot/isSlot fall back to that cache when the slot is no
    longer live in config. unbind clears the cached entry in the same critical section it clears the
    binding, so a slot that's both unbound and gone from config stops being "known".
  • Fleetd.java now wires MemberRegistry.live(() -> config.get().fleet()) instead of the frozen
    new MemberRegistry(cfg.fleet()).
  • ConfigRef: corrected the fleet.leaders split-key message and the class doc's Hot bullet —
    architects is hot for two independent consumers now (CompositePeerLauncher for placement,
    MemberRegistry for identity), not only the one the message used to name. An architects-only
    edit still reports nothing beyond "config reloaded" — that's honest now that the key really is
    fully hot for both consumers, so no change was needed to that reporting branch itself, only to the
    wording of the fleet: split message.

Tests (MemberRegistryLiveTest, new)

Real ConfigRef.reload() against a @TempDir file, asserting Outcome.applied(), never two
frozen registries compared in memory:

  • requireSlotFor refuses a removed slot / allows an added slot (2 tests, both directions).
  • reserve refuses a removed slot / allows an added slot (2 tests, both directions, tested
    separately from requireSlotFor).
  • A bound architect survives its slot's removal by reload: binding intact, snapshot() unchanged,
    AND roleForSlot/nameForSlot keep answering ARCHITECT/"designer" — while requireSlotFor/
    reserve refuse the same profile for a new spawn.

Mutation-proofing (each applied, run, confirmed failing, then reverted)

  • A — froze requireSlotFor to a construction-time snapshot only →
    requireSlotForRefusesAProfileWhoseSlotWasRemovedByReload failed ("Expected
    IllegalArgumentException to be thrown, but nothing was thrown") and its added-slot mirror failed
    too (unexpected IllegalArgumentException).
  • B — froze reserve the same way →
    reserveRefusesAProfileWhoseSlotWasRemovedByReload failed the same way, and
    reserveAllowsAProfileWhoseSlotWasAddedByReload errored with "no free architect slot for profile
    'sonnet'".
  • C — dropped the boundEntries fallback so a removed slot's identity is slots()-only →
    anArchitectAlreadyBoundToASlotSurvivesTheSlotsRemovalByReload failed on
    roleForSlot("architect:designer"): expected: <ARCHITECT> but was: <null>.

All three restored before the final build below.

Build

mvn clean install (from fleetd/, no root pom): Tests run: 1510, Failures: 0, Errors: 0,
Skipped: 0 — BUILD SUCCESS.

Out of scope (per ticket)

  • fleet.developers/fleet.reviewers inside MemberRegistry stay dead data — untouched.
  • Ticket #425 (the frozen defaultProfile) — CompositePeerLauncher.defaultProfile, FleetMcp's
    "default" field, SessionManager.acquireWithWorktree — untouched.
  • No ConfigRef coverage-test exclusion was added.

Caveat for review

entryFor() (backing roleForSlot/nameForSlot/profileForSlot) reads live config first and
falls back to the cached bound entry only when the slot is no longer live. If a live config entry
changes shape for the same qualified key while a session is bound under the old identity (as
opposed to being removed outright), the live value wins — that case is not addressed here; the
ticket's scope was specifically survival-through-removal.

## fleetd #424 — revoking an architect slot does not revoke it ### The defect `MemberRegistry` flattened `fleet.architects` into a frozen map at construction and never re-read config. `reserve()`/`requireSlotFor()` (the two spawn-side checks) matched against that frozen snapshot forever, so removing an architect slot from config never actually refused a later spawn asking for it — an architect is materially more privileged than a worker, and this left a "revoked" privilege open with no warning. `ConfigRef` also told the operator an `architects`-only edit was "already applied" through the wrong consumer (`CompositePeerLauncher`, which only reads it for placement, not identity). ### The fix — binding semantic Implemented the lead's ruling: **config governs what may be bound next; it never retroactively unbinds a live session.** - `MemberRegistry.live(Supplier<FleetConfig.Fleet>)` — a new static factory — re-flattens `fleet.architects`/`developers`/`reviewers` on every `slots()`/`slotsFor()` call. `reserve()` and `requireSlotFor()` both read through `slotsFor`, so both are now live with no restart. The old single-arg `MemberRegistry(FleetConfig.Fleet)` constructor is kept (frozen) for tests and for a fixed/code-built config. - A slot removed from config while a terminal is bound to it **keeps that binding** — nothing is unbound, nothing is killed. - To keep the bound terminal's **identity** too — `CallerResolver.resolve` calls `roleForSlot`/`nameForSlot` on every request from a bound pane to confirm it is still an architect slot — every successful `bind` now caches the slot's `Entry` into a new `boundEntries` map. `roleForSlot`/`nameForSlot`/`profileForSlot`/`isSlot` fall back to that cache when the slot is no longer live in config. `unbind` clears the cached entry in the same critical section it clears the binding, so a slot that's both unbound and gone from config stops being "known". - `Fleetd.java` now wires `MemberRegistry.live(() -> config.get().fleet())` instead of the frozen `new MemberRegistry(cfg.fleet())`. - `ConfigRef`: corrected the `fleet.leaders` split-key message and the class doc's Hot bullet — `architects` is hot for **two independent consumers** now (`CompositePeerLauncher` for placement, `MemberRegistry` for identity), not only the one the message used to name. An `architects`-only edit still reports nothing beyond "config reloaded" — that's honest now that the key really is fully hot for both consumers, so no change was needed to that reporting branch itself, only to the wording of the `fleet:` split message. ### Tests (`MemberRegistryLiveTest`, new) Real `ConfigRef.reload()` against a `@TempDir` file, asserting `Outcome.applied()`, never two frozen registries compared in memory: - `requireSlotFor` refuses a removed slot / allows an added slot (2 tests, both directions). - `reserve` refuses a removed slot / allows an added slot (2 tests, both directions, tested separately from `requireSlotFor`). - A bound architect survives its slot's removal by reload: binding intact, `snapshot()` unchanged, AND `roleForSlot`/`nameForSlot` keep answering ARCHITECT/"designer" — while `requireSlotFor`/ `reserve` refuse the same profile for a *new* spawn. ### Mutation-proofing (each applied, run, confirmed failing, then reverted) - **A** — froze `requireSlotFor` to a construction-time snapshot only → `requireSlotForRefusesAProfileWhoseSlotWasRemovedByReload` failed ("Expected IllegalArgumentException to be thrown, but nothing was thrown") and its added-slot mirror failed too (unexpected `IllegalArgumentException`). - **B** — froze `reserve` the same way → `reserveRefusesAProfileWhoseSlotWasRemovedByReload` failed the same way, and `reserveAllowsAProfileWhoseSlotWasAddedByReload` errored with "no free architect slot for profile 'sonnet'". - **C** — dropped the `boundEntries` fallback so a removed slot's identity is `slots()`-only → `anArchitectAlreadyBoundToASlotSurvivesTheSlotsRemovalByReload` failed on `roleForSlot("architect:designer")`: `expected: <ARCHITECT> but was: <null>`. All three restored before the final build below. ### Build `mvn clean install` (from `fleetd/`, no root pom): **Tests run: 1510, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS.** ### Out of scope (per ticket) - `fleet.developers`/`fleet.reviewers` inside `MemberRegistry` stay dead data — untouched. - Ticket #425 (the frozen `defaultProfile`) — `CompositePeerLauncher.defaultProfile`, `FleetMcp`'s `"default"` field, `SessionManager.acquireWithWorktree` — untouched. - No `ConfigRef` coverage-test exclusion was added. ### Caveat for review `entryFor()` (backing `roleForSlot`/`nameForSlot`/`profileForSlot`) reads live config first and falls back to the cached bound entry only when the slot is no longer live. If a *live* config entry changes shape for the same qualified key while a session is bound under the old identity (as opposed to being removed outright), the live value wins — that case is not addressed here; the ticket's scope was specifically survival-through-*removal*.
agent added 1 commit 2026-09-10 07:05:57 +02:00
fleetd #424: make architect-slot identity checks read fleet.architects live
CI / contract (pull_request) Successful in 1m15s
Details
CI / build (pull_request) Successful in 1m32s
Details
ce74e164c6 
MemberRegistry used to flatten fleet.architects into an unmodifiable map at
construction, so removing (revoking) an architect slot from config never
took effect: reserve()/requireSlotFor() kept granting spawns against the
frozen snapshot forever, while ConfigRef told the operator "already
applied" for the wrong consumer.

- MemberRegistry gains a live constructor (MemberRegistry.live(Supplier))
  that re-flattens fleet.architects/developers/reviewers on every
  slots()/slotsFor() call, so reserve() and requireSlotFor() (which both
  read through slotsFor) govern the NEXT spawn with no restart. The frozen
  single-arg constructor is kept for tests and fixed/code-built configs.
- Binding rule: config governs what may be bound next; it never
  retroactively unbinds a live session. A slot removed from config while a
  terminal is bound to it keeps that binding. To keep the bound terminal's
  IDENTITY too (CallerResolver.resolve reads roleForSlot/nameForSlot on
  every request), every successful bind now caches the slot's Entry into a
  new boundEntries map; roleForSlot/nameForSlot/profileForSlot/isSlot fall
  back to it when the slot is no longer live, and unbind clears it in the
  same critical section it clears the binding.
- Fleetd.java now wires MemberRegistry.live(() -> config.get().fleet())
  instead of the frozen constructor.
- ConfigRef: corrected the fleet.leaders split-key message and the class
  doc's Hot bullet — architects is now hot for two independent consumers
  (CompositePeerLauncher for placement, MemberRegistry for identity), not
  only the one the message used to name. An architects-only edit still
  reports nothing beyond "config reloaded", which is now honest since the
  key really is fully hot for both consumers.

Added MemberRegistryLiveTest: real ConfigRef.reload() against a @TempDir
file, both directions (slot removed / slot added) for requireSlotFor and
reserve tested separately, plus a bound-architect-survives-removal test
that checks the binding AND the identity (roleForSlot/nameForSlot).

Mutation-tested: reverting requireSlotFor to a frozen snapshot fails
requireSlotForRefusesAProfileWhoseSlotWasRemovedByReload and its mirror;
reverting reserve the same way fails the two reserve tests; dropping the
boundEntries fallback fails the survives-removal test's roleForSlot
assertion. All three restored before commit.

mvn clean install: Tests run: 1510, Failures: 0, Errors: 0, Skipped: 0 —
BUILD SUCCESS.
ltms commented 2026-09-10 07:17:16 +02:00
Owner
Copy Link
Copy Source

Lead review — not merging yet, and the change I want is my own ticket's fault

Verified on ce74e16 myself: mvn -f <abs>/fleetd/pom.xml clean install → Tests run: 1510, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS, 0 compile errors.

What is right, and I am keeping it: MemberRegistry.live(() -> config.get().fleet()), slots() re-reading through the supplier on every call, the private-constructor-not-an-overload reasoning (a Fleet and a Supplier<Fleet> overload really are ambiguous for a literal null), all four reload-direction tests, and the ConfigRef javadoc and reload-message corrections. That last part matters as much as the code: the old message told the operator architects was "already applied" when it was not.

What has to change: boundEntries.

The PR keeps a live architect's full identity after its slot is removed from config — role and name both survive via the cache. So removing a slot and reloading does not revoke anything from the running session; it only stops future binds.

This ticket is titled "Revoking an architect slot does not revoke it". Shipping that leaves the headline defect half-closed.

This is my wording, not the implementer's. The ticket says "a rebuild must not drop or invalidate an existing architect's binding" and asks for "a test that an already-bound architect survives the rebuild". The implementer did exactly that. I had conflated two different things:

  • the binding — the slot-occupancy bookkeeping in terminalToSlot, which must survive or unbind breaks and slots leak;
  • the privilege — the ARCHITECT role that binding grants.

Only the first needs to survive.

Ruling: the binding survives, the privilege does not. I checked the alternative before ruling rather than assuming it was clean: with no cache, roleForSlot returns null and CallerResolver.resolve at CallerResolver.java:220 falls through to Principal.worker(c.terminal(), c.pid()). An immediate, clean demotion, and unbind still works because it never consults isSlot.

Why this direction. If an operator believes removal revokes and it does not, a privilege stays open silently — the exact defect class of this ticket. If an operator believes removal only affects new spawns and it actually revokes, the session is demoted and its next privileged call fails loudly. The charter says to fail toward the recoverable error, and a demoted member can still end its turn, because fleet_reply is allowed to any member as itself. I have asked the implementer to confirm that last point in Authz and to stop and ask if it is not true, since it would change the ruling.

Briefed: delete boundEntries and the entryFor cache fallback, revert isSlot to slots().containsKey(...), rewrite the class doc's binding rule, and check whether anything else cross-references slots() against terminalToSlot (snapshot() in particular). Tests: the demotion asserted through CallerResolver.resolve, not through roleForSlot — the seam is not the caller — plus two tests for what must not change (the binding still occupies the slot, and unbind still succeeds). Mutation proof per direction, including the mirror: demotion must not fire for a slot that is still in config.

## Lead review — not merging yet, and the change I want is my own ticket's fault Verified on `ce74e16` myself: `mvn -f <abs>/fleetd/pom.xml clean install` → `Tests run: 1510, Failures: 0, Errors: 0, Skipped: 0`, `BUILD SUCCESS`, 0 compile errors. **What is right, and I am keeping it:** `MemberRegistry.live(() -> config.get().fleet())`, `slots()` re-reading through the supplier on every call, the private-constructor-not-an-overload reasoning (a `Fleet` and a `Supplier<Fleet>` overload really are ambiguous for a literal `null`), all four reload-direction tests, and the `ConfigRef` javadoc and reload-message corrections. That last part matters as much as the code: the old message told the operator `architects` was "already applied" when it was not. **What has to change: `boundEntries`.** The PR keeps a live architect's full identity after its slot is removed from config — role and name both survive via the cache. So removing a slot and reloading does not revoke anything from the running session; it only stops future binds. This ticket is titled "Revoking an architect slot does not revoke it". Shipping that leaves the headline defect half-closed. **This is my wording, not the implementer's.** The ticket says "a rebuild must not drop or invalidate an existing architect's binding" and asks for "a test that an already-bound architect survives the rebuild". The implementer did exactly that. I had conflated two different things: - the **binding** — the slot-occupancy bookkeeping in `terminalToSlot`, which must survive or `unbind` breaks and slots leak; - the **privilege** — the ARCHITECT role that binding grants. Only the first needs to survive. **Ruling: the binding survives, the privilege does not.** I checked the alternative before ruling rather than assuming it was clean: with no cache, `roleForSlot` returns null and `CallerResolver.resolve` at `CallerResolver.java:220` falls through to `Principal.worker(c.terminal(), c.pid())`. An immediate, clean demotion, and `unbind` still works because it never consults `isSlot`. Why this direction. If an operator believes removal revokes and it does not, a privilege stays open silently — the exact defect class of this ticket. If an operator believes removal only affects new spawns and it actually revokes, the session is demoted and its next privileged call fails loudly. The charter says to fail toward the recoverable error, and a demoted member can still end its turn, because `fleet_reply` is allowed to any member as itself. I have asked the implementer to confirm that last point in `Authz` and to stop and ask if it is not true, since it would change the ruling. Briefed: delete `boundEntries` and the `entryFor` cache fallback, revert `isSlot` to `slots().containsKey(...)`, rewrite the class doc's binding rule, and check whether anything else cross-references `slots()` against `terminalToSlot` (`snapshot()` in particular). Tests: the demotion asserted **through `CallerResolver.resolve`, not through `roleForSlot`** — the seam is not the caller — plus two tests for what must not change (the binding still occupies the slot, and `unbind` still succeeds). Mutation proof per direction, including the mirror: demotion must not fire for a slot that is still in config.
agent added 1 commit 2026-09-10 07:29:41 +02:00
fleetd #424: revoke the ARCHITECT privilege on reload, not just future spawns
CI / contract (pull_request) Successful in 43s
Details
CI / build (pull_request) Successful in 2m3s
Details
7f672f0fb8 
Correction to the #424 fix in PR #428: the ticket asked to revoke a removed
architect slot, but the previous change (boundEntries) kept BOTH the binding
and the ARCHITECT privilege alive for an already-bound session after its slot
left config. That left the ticket's actual headline defect half-open.

The corrected rule: config governs both what may be bound next AND what a
bound slot still grants. Removing a slot now demotes its bound session to
worker on the very next request (roleForSlot/nameForSlot read slots() with no
cache, so CallerResolver.resolve falls through to Principal.worker(...)). The
terminalToSlot binding itself is untouched by a reload, on purpose: dropping
it would double-book the slot key and break unbind's compare-safe contract.

- Delete boundEntries and entryFor; profileForSlot/roleForSlot/nameForSlot/
  isSlot all read slots() directly, live, with no cache.
- Rewrite the class doc's binding rule for the corrected semantic.
- Replace the old "survives removal" test with anArchitectAlreadyBoundToASlotIsDemotedByReload,
  asserted through a real CallerResolver.resolve (not the roleForSlot seam),
  plus two tests for what must NOT change: the binding still occupies the
  slot after removal (a second terminal cannot claim it, even once the slot
  returns to config), and unbind still succeeds for the original terminal.

Verified snapshot()/CallerResolver.members() need no change: snapshot() only
ever reported raw terminalToSlot occupancy, and CallerResolver.members() has
no production caller.
ltms commented 2026-09-10 07:54:55 +02:00
Owner
Copy Link
Copy Source

Merged by hand as 7667727 on main. The forge could not do it: this branch and #429 both rewrote the same "Hot" bullet in ConfigRef.java, so the merge conflicted on content.

Verified on the merged tree: Tests run: 1535, Failures: 0, Errors: 0, Skipped: 0, 0 compile errors, BUILD SUCCESS.

Two stale claims fixed during the merge. The second commit 7f672f0 reversed the behaviour but never touched ConfigRef, whose whole job is to tell the operator what a reload does. It still said:

  • the Hot bullet: MemberRegistry's rule "keeps a live session's identity even after its slot is removed from config"
  • the reload-report comment: "only a NEW bind is refused"

Both were the pre-fix statement — that is, the bug this ticket exists to fix, written down as documentation. They now say removal revokes ARCHITECT on the bound pane's next request, and only the slot occupancy survives.

Mutation of three halves your own proof did not cover. Your three mutations plus the mirror all killed exactly the right test — that part is clean. So I mutated the sibling reads this PR also made live. Each pointed at a frozen snapshot taken at construction; each printed its method name and line, and the live-reader count went 5 -> 4:

Mutation Result at 1535 tests
profileForSlot frozen PASSED
nameForSlot frozen PASSED
isSlot frozen PASSED

All three passed, so none of them is pinned. profileForSlot is the one that matters: the spawn lifecycle reads it to choose the architect's backend, so if it were ever frozen again, a reload that repoints a slot to another profile would spawn on the old one and every test would stay green. isSlot gates bind/reserve, and it can lie without a single test noticing.

Not a merge blocker — the ticket's own fix is well pinned. Filed as a follow-up.

Merged by hand as `7667727` on `main`. The forge could not do it: this branch and #429 both rewrote the same "Hot" bullet in `ConfigRef.java`, so the merge conflicted on content. Verified on the merged tree: `Tests run: 1535, Failures: 0, Errors: 0, Skipped: 0`, 0 compile errors, BUILD SUCCESS. **Two stale claims fixed during the merge.** The second commit `7f672f0` reversed the behaviour but never touched `ConfigRef`, whose whole job is to tell the operator what a reload does. It still said: - the Hot bullet: MemberRegistry's rule "keeps a live session's identity even after its slot is removed from config" - the reload-report comment: "only a NEW bind is refused" Both were the pre-fix statement — that is, the bug this ticket exists to fix, written down as documentation. They now say removal revokes ARCHITECT on the bound pane's next request, and only the slot *occupancy* survives. **Mutation of three halves your own proof did not cover.** Your three mutations plus the mirror all killed exactly the right test — that part is clean. So I mutated the sibling reads this PR also made live. Each pointed at a frozen snapshot taken at construction; each printed its method name and line, and the live-reader count went 5 -> 4: | Mutation | Result at 1535 tests | |---|---| | `profileForSlot` frozen | **PASSED** | | `nameForSlot` frozen | **PASSED** | | `isSlot` frozen | **PASSED** | All three passed, so none of them is pinned. `profileForSlot` is the one that matters: the spawn lifecycle reads it to choose the architect's backend, so if it were ever frozen again, a reload that repoints a slot to another profile would spawn on the old one and every test would stay green. `isSlot` gates `bind`/`reserve`, and it can lie without a single test noticing. Not a merge blocker — the ticket's own fix is well pinned. Filed as a follow-up.
ltms closed this pull request 2026-09-10 07:55:00 +02:00
Some checks are pending
CI / contract (pull_request) Successful in 43s
Details
CI / build (pull_request) Successful in 2m3s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#428
Delete Branch "worker/424-architect-slot-hot-038b41-7"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?