fix(fleet): preserve pool identity and improve seat diagnostics

.github/workflows/fleet-portability.yml

@@ -0,0 +1,41 @@
+name: Fleet portability
+
+on:
+  pull_request:
+    paths:
+      - 'cmd/fleet/**'
+      - '.github/workflows/fleet-portability.yml'
+      - 'go.mod'
+      - 'go.sum'
+  push:
+    branches: [main]
+    paths:
+      - 'cmd/fleet/**'
+      - '.github/workflows/fleet-portability.yml'
+      - 'go.mod'
+      - 'go.sum'
+
+permissions:
+  contents: read
+
+concurrency:
+  group: fleet-portability-${{ github.ref }}
+  cancel-in-progress: true
+
+jobs:
+  seats:
+    strategy:
+      fail-fast: false
+      matrix:
+        os: [macos-latest, windows-latest]
+    runs-on: ${{ matrix.os }}
+    timeout-minutes: 10
+    steps:
+      - uses: actions/checkout@v4
+      - uses: actions/setup-go@v5
+        with:
+          go-version-file: go.mod
+      - name: Real Git seat identity and deny preservation
+        run: go test ./cmd/fleet/internal/verbs -run 'TestPool|TestWriteDenies|TestAssignReports|TestUndeclaredUsesHolderAssignment' -count=1
+      - name: Watcher tests (Windows includes process flags, not visual proof)
+        run: go test ./cmd/fleet/internal/watch -count=1

FOLLOWUPS.md

@@ -523,3 +523,22 @@ same bar workbench-mcp cleared. Evaluate then; not before.
   A future indexed reader could preserve longer windows within the same resource
   budget. Empty telemetry reason/out strings remain a compatibility deferral:
   current consumers normalize them identically to null/absent fields.
+
+## Fleet #289 — residual after the capped review rounds
+
+Code head: `b16c52387fcaadf35a63065c36845ba3aa74e6ab`. The Mac review used the
+initial panel plus two fix rounds; do not start a fourth panel cycle for these nits.
+
+- **Diagnostic only: duplicate extra denies.** Copilot identified, and Claude confirmed,
+  that manually duplicated entries in settings.local.json can appear twice in the
+  retained-extra NOTE. `writeDenies` deduplicates the actual written permissions through
+  `denySet`, so enforcement is unchanged and the resulting file is normalized. A future
+  small change can deduplicate `extra` too, with a duplicate-input regression. Deferred
+  under the review cap rather than changing enforcement or claiming the finding vanished.
+- **Repeated roles.map reads.** Tenant and label resolution independently scan bindings.
+  Consolidating one validated snapshot may simplify a later pool transaction change;
+  this PR does not claim atomicity against concurrent edits to all role bindings.
+
+No residual acceptance or merge authority is recorded here. The merge decision still
+belongs to the operator's governed path. Windows visible-window acceptance and effective
+hook migration remain separate from the green portability tests.

cmd/fleet/install.sh

@@ -40,8 +40,13 @@ plan() { say "  [$mode] $*"; }
 
 rollback() {
   for f in "$claude_settings" "$codex_hooks"; do
-    latest="$(ls -1t "$f".bak-* 2>/dev/null | head -1 || true)"
-    if [ -z "$latest" ]; then say "no backup for $f; nothing to restore"; continue; fi
+    # Only backups THIS script wrote, i.e. `.bak-` plus the $stamp shape. `.bak-*` also
+    # matches an unrelated sibling a person left beside the file — `settings.json.bak-env`
+    # is a real example — and restoring one of those over live harness config is a silent
+    # loss of whatever else it did not contain. A restore target should be a file the tool
+    # can prove it created.
+    latest="$(ls -1t "$f".bak-[0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9]-[0-9][0-9][0-9][0-9][0-9][0-9] 2>/dev/null | head -1 || true)"
+    if [ -z "$latest" ]; then say "no backup written by this script for $f; nothing to restore"; continue; fi
     say "restore $latest -> $f"
     cp "$latest" "$f"
   done

cmd/fleet/install_test.go

@@ -67,3 +67,30 @@ func TestInstallerRemovesOnlyFleetShadowHooks(t *testing.T) {
 		t.Fatal(first)
 	}
 }
+
+func TestInstallerRollbackIgnoresUnrelatedBackup(t *testing.T) {
+	bash, err := exec.LookPath("bash")
+	if err != nil {
+		t.Skip("bash unavailable")
+	}
+	root := t.TempDir()
+	dir := filepath.Join(root, ".claude")
+	if err := os.MkdirAll(dir, 0700); err != nil {
+		t.Fatal(err)
+	}
+	p := filepath.Join(dir, "settings.json")
+	for name, body := range map[string]string{p: "current", p + ".bak-20260908-120000": "expected", p + ".bak-env": "unrelated"} {
+		if err := os.WriteFile(name, []byte(body), 0600); err != nil {
+			t.Fatal(err)
+		}
+	}
+	c := exec.Command(bash, "install.sh", "--rollback")
+	c.Env = append(os.Environ(), "HOME="+root, "FLEET_HOME="+filepath.Join(root, "fleet"))
+	if out, err := c.CombinedOutput(); err != nil {
+		t.Fatalf("rollback: %v %s", err, out)
+	}
+	got, err := os.ReadFile(p)
+	if err != nil || string(got) != "expected" {
+		t.Fatalf("restored=%q err=%v", got, err)
+	}
+}

cmd/fleet/internal/verbs/pool_identity_test.go

@@ -0,0 +1,159 @@
+package verbs
+
+import (
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/itsHabib/workbench/cmd/fleet/internal/fleet"
+)
+
+func poolFixture(t *testing.T) (string, string, string) {
+	t.Helper()
+	root := t.TempDir()
+	old := fleet.OrgState
+	fleet.OrgState = root
+	t.Cleanup(func() { fleet.OrgState = old })
+	t.Setenv("ORG_TENANT", "")
+	repo := filepath.Join(root, "Mono")
+	run := func(args ...string) {
+		t.Helper()
+		c := exec.Command("git", args...)
+		if b, e := c.CombinedOutput(); e != nil {
+			t.Fatalf("git %v: %v %s", args, e, b)
+		}
+	}
+	run("init", repo)
+	run("-C", repo, "-c", "user.name=Test", "-c", "user.email=test@example.invalid", "commit", "--allow-empty", "-m", "fixture")
+	a, b := filepath.Join(root, "seat-a"), filepath.Join(root, "seat-b")
+	run("-C", repo, "worktree", "add", "--detach", a)
+	run("-C", repo, "worktree", "add", "--detach", b)
+	return repo, a, b
+}
+
+func poolMap(t *testing.T, lines ...string) {
+	t.Helper()
+	if err := os.WriteFile(fleet.RolesMap(), []byte(strings.Join(lines, "\n")+"\n"), 0600); err != nil {
+		t.Fatal(err)
+	}
+}
+
+func TestPoolTenantRejectsAmbiguousSiblings(t *testing.T) {
+	repo, a, b := poolFixture(t)
+	poolMap(t, a+" first worker:mono", b+" second worker:mono")
+	tenant, err := poolTenant("", repo, "Mono")
+	if err == nil || !strings.Contains(err.Error(), "ambiguous") {
+		t.Fatalf("wanted ambiguous tenant refusal; tenant=%q err=%v", tenant, err)
+	}
+	if got, err := poolTenant("second", repo, "Mono"); err != nil || got != "second" {
+		t.Fatalf("explicit tenant=%q err=%v", got, err)
+	}
+}
+
+func TestPoolLabelStaysWithinTenant(t *testing.T) {
+	repo, a, b := poolFixture(t)
+	poolMap(t, a+" first worker:foreign", b+" second worker:mono")
+	label, err := poolLabel(repo, "Mono", "second")
+	if err != nil || label != "mono" {
+		t.Fatalf("label=%q err=%v", label, err)
+	}
+}
+
+func TestPoolLabelRejectsAmbiguousSiblings(t *testing.T) {
+	repo, a, b := poolFixture(t)
+	poolMap(t, a+" work worker:Mono", b+" work worker:mono")
+	_, err := poolLabel(repo, "Mono", "work")
+	if err == nil || !strings.Contains(err.Error(), "ambiguous") {
+		t.Fatalf("wanted ambiguous label refusal: %v", err)
+	}
+}
+
+func TestPoolInheritsUnanimousLabelWithoutRolingMain(t *testing.T) {
+	repo, a, b := poolFixture(t)
+	poolMap(t, a+" work worker:mono", b+" work checker:mono")
+	tenant, err := poolTenant("", repo, "Mono")
+	if err != nil || tenant != "work" {
+		t.Fatalf("tenant=%q err=%v", tenant, err)
+	}
+	label, err := poolLabel(repo, "Mono", tenant)
+	if err != nil || label != "mono" {
+		t.Fatalf("label=%q err=%v", label, err)
+	}
+	if fleet.RoleOf(repo) != "" {
+		t.Fatal("main gained a role")
+	}
+}
+
+func TestPoolExactBindingWinsOverSiblingLabel(t *testing.T) {
+	repo, a, _ := poolFixture(t)
+	poolMap(t, repo+" work lead:preferred", a+" work worker:old")
+	label, err := poolLabel(repo, "Mono", "work")
+	if err != nil || label != "preferred" {
+		t.Fatalf("label=%q err=%v", label, err)
+	}
+}
+
+func TestWriteDeniesReportsAndPreservesExtras(t *testing.T) {
+	existing := map[string]any{"permissions": map[string]any{"deny": []any{"Bash(old)", "Bash(shared)"}}, "unrelated": true}
+	manifest := map[string]any{"denies": []any{"Bash(shared)", "Bash(new)"}}
+	target := filepath.Join(t.TempDir(), "settings.json")
+	deny, extra, err := writeDenies(existing, manifest, target)
+	if err != nil || strings.Join(extra, ",") != "Bash(old)" || len(deny) != 3 {
+		t.Fatalf("deny=%v extra=%v err=%v", deny, extra, err)
+	}
+	got := fleet.ReadJSON(target)
+	if !fleet.B(got, "unrelated") || len(fleet.Strs(fleet.M(got, "permissions"), "deny")) != 3 {
+		t.Fatalf("written settings=%v", got)
+	}
+}
+
+func TestPoolAmbiguityRefusesBeforeCreatingSeats(t *testing.T) {
+	repo, a, b := poolFixture(t)
+	oldState := fleet.State
+	fleet.State = t.TempDir()
+	t.Cleanup(func() { fleet.State = oldState })
+	lanes, err := filepath.Abs("../../testdata/lanes")
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Setenv("FLEET_LANES", lanes)
+	poolMap(t, a+" work worker:Mono", b+" work worker:mono")
+	before, err := os.ReadFile(fleet.RolesMap())
+	if err != nil {
+		t.Fatal(err)
+	}
+	err = cmdPool(repo, "author", "1", false, "work")
+	if err == nil || !strings.Contains(err.Error(), "ambiguous labels") {
+		t.Fatalf("expected label ambiguity, got %v", err)
+	}
+	after, err := os.ReadFile(fleet.RolesMap())
+	if err != nil || string(before) != string(after) {
+		t.Fatalf("roles changed on refusal: %v", err)
+	}
+	if _, err := os.Stat(filepath.Join(filepath.Dir(repo), "Mono-author-1")); !os.IsNotExist(err) {
+		t.Fatalf("seat created on refusal: %v", err)
+	}
+}
+
+func TestAssignReportsHoldingWorktree(t *testing.T) {
+	repo, a, _ := poolFixture(t)
+	branch := fleet.BranchOf(repo)
+	err := assignCheckout("seat-a", a, branch)
+	if err == nil || !strings.Contains(fleet.NormCase(strings.ReplaceAll(err.Error(), "\\", "/")), fleet.NormCase(fleet.LongPath(repo))) {
+		t.Fatalf("missing holding checkout %s: %v", repo, err)
+	}
+	if fleet.BranchOf(a) != "" {
+		t.Fatal("refused assignment changed seat branch")
+	}
+}
+
+func TestPoolAncestorDoesNotHideSiblingAmbiguity(t *testing.T) {
+	repo, a, b := poolFixture(t)
+	poolMap(t, filepath.Dir(repo)+" inherited lead:parent", a+" first worker:mono", b+" second worker:mono")
+	_, err := poolTenant("", repo, "Mono")
+	if err == nil || !strings.Contains(err.Error(), "ambiguous") {
+		t.Fatalf("ancestor hid tenant ambiguity: %v", err)
+	}
+}

cmd/fleet/internal/verbs/role.go

@@ -423,32 +423,49 @@ func writeMapLine(lines []string, same []fleet.MapRow, mapfile, checkout, tenant
 }
 
 // writeDenies merges the manifest's denies into the checkout's Claude settings and
-// writes them; returns the merged deny list. indent=2: a file a human maintains by
-// hand; do not collapse it.
-func writeDenies(existing, manifest map[string]any, settingsTarget string) ([]string, error) {
+// writes them; returns the merged deny list and the denies present in the file that the
+// manifest no longer declares. indent=2: a file a human maintains by hand; do not
+// collapse it.
+//
+// The merge is deliberate: it preserves denies a human added to settings.local.json.
+// The cost is that a deny can only ever be ADDED to a seat, so dropping one from a lane
+// manifest is a no-op against seats already roled, and a seat's permissions become a
+// high-water mark of every deny its lane ever declared. That silence produced two seats
+// of one kind, roled in one `fleet pool` run from one manifest, enforcing different
+// rules. Returning the extras does not change what is written; it lets the caller say
+// that a narrowing did not take.
+func writeDenies(existing, manifest map[string]any, settingsTarget string) (deny, extra []string, err error) {
 	perms, _ := existing["permissions"].(map[string]any)
 	if perms == nil {
 		perms = map[string]any{}
 		existing["permissions"] = perms
 	}
+	fromManifest := map[string]bool{}
+	for _, d := range fleet.Strs(manifest, "denies") {
+		fromManifest[d] = true
+	}
 	denySet := map[string]bool{}
 	for _, d := range fleet.Strs(perms, "deny") {
 		denySet[d] = true
+		if !fromManifest[d] {
+			extra = append(extra, d)
+		}
 	}
-	for _, d := range fleet.Strs(manifest, "denies") {
+	for d := range fromManifest {
 		denySet[d] = true
 	}
-	deny := sortedKeys(denySet)
+	sort.Strings(extra)
+	deny = sortedKeys(denySet)
 	denyAny := make([]any, len(deny))
 	for i, d := range deny {
 		denyAny[i] = d
 	}
 	perms["deny"] = denyAny
 	sb, _ := json.MarshalIndent(existing, "", "  ")
 	if err := os.WriteFile(settingsTarget, append(sb, '\n'), 0o644); err != nil {
-		return nil, err
+		return nil, nil, err
 	}
-	return deny, nil
+	return deny, extra, nil
 }
 
 func roleUnderLock(checkout, role string, force bool, tenant, slot, kind string, manifest map[string]any, card, cfgTarget, cfgText, hooksTarget string, hooksData map[string]any, rulesTarget, settingsTarget string, existing map[string]any, mapfile string) error {
@@ -485,7 +502,7 @@ func roleUnderLock(checkout, role string, force bool, tenant, slot, kind string,
 	if err := os.MkdirAll(filepath.Join(checkout, ".claude"), 0o755); err != nil {
 		return err
 	}
-	deny, err := writeDenies(existing, manifest, settingsTarget)
+	deny, extraDenies, err := writeDenies(existing, manifest, settingsTarget)
 	if err != nil {
 		return err
 	}
@@ -506,6 +523,12 @@ func roleUnderLock(checkout, role string, force bool, tenant, slot, kind string,
 	if excluded := excludeLocalArtifacts(checkout); excluded != "" {
 		note = " All generated files excluded via " + excluded + "."
 	}
+	// A deny the manifest dropped stays in the seat's settings, so say so: otherwise a
+	// narrowing looks applied and is not.
+	if len(extraDenies) > 0 {
+		note += fmt.Sprintf(" NOTE: %d deny(s) in this seat are not in the manifest and were kept: %s. Remove them from %s by hand to narrow this seat.",
+			len(extraDenies), strings.Join(extraDenies, ", "), fleet.LongPath(settingsTarget))
+	}
 	say("%s is now %s (manifest %s): Claude card + %d denies; Codex developer card + user hooks + %d exact command rules. Open a NEW Codex tab there, trust the project configuration, then trust the hook definitions.%s",
 		checkout, role, fleet.LongPath(filepath.Join(fleet.LanesDir(), kind, "manifest.json")), len(deny), strings.Count(rules, "prefix_rule("), note)
 	return nil