diff --git a/.github/workflows/fleet-portability.yml b/.github/workflows/fleet-portability.yml new file mode 100644 index 00000000..2f1c84cb --- /dev/null +++ b/.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 diff --git a/FOLLOWUPS.md b/FOLLOWUPS.md index d7db2769..859e395d 100644 --- a/FOLLOWUPS.md +++ b/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. diff --git a/cmd/fleet/install.sh b/cmd/fleet/install.sh index 9b65256d..de5667f6 100755 --- a/cmd/fleet/install.sh +++ b/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 diff --git a/cmd/fleet/install_test.go b/cmd/fleet/install_test.go index 618b9de5..ea5f80ca 100644 --- a/cmd/fleet/install_test.go +++ b/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) + } +} diff --git a/cmd/fleet/internal/verbs/pool_identity_test.go b/cmd/fleet/internal/verbs/pool_identity_test.go new file mode 100644 index 00000000..0e1a6dc2 --- /dev/null +++ b/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) + } +} diff --git a/cmd/fleet/internal/verbs/role.go b/cmd/fleet/internal/verbs/role.go index aba76dfb..f701e076 100644 --- a/cmd/fleet/internal/verbs/role.go +++ b/cmd/fleet/internal/verbs/role.go @@ -423,22 +423,39 @@ 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 @@ -446,9 +463,9 @@ func writeDenies(existing, manifest map[string]any, settingsTarget string) ([]st 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 diff --git a/cmd/fleet/internal/verbs/views.go b/cmd/fleet/internal/verbs/views.go index af0244ac..e7248946 100644 --- a/cmd/fleet/internal/verbs/views.go +++ b/cmd/fleet/internal/verbs/views.go @@ -583,7 +583,11 @@ func cmdPool(checkout, kind, nArg string, rewarm bool, tenant string) error { if err != nil { return err } - p := newPooler(checkout, base, parent, tenant, rewarm, cfg) + label, err := poolLabel(checkout, base, tenant) + if err != nil { + return err + } + p := newPooler(checkout, base, parent, tenant, label, rewarm, cfg) for _, k := range order { for i := 1; i <= wanted[k]; i++ { if err := p.one(k, i); err != nil { @@ -595,8 +599,8 @@ func cmdPool(checkout, kind, nArg string, rewarm bool, tenant string) error { return nil } -func newPooler(checkout, base, parent, tenant string, rewarm bool, cfg map[string]any) *pooler { - p := &pooler{checkout: checkout, base: base, parent: parent, tenant: tenant, rewarm: rewarm, cmds: warmCommands(cfg), +func newPooler(checkout, base, parent, tenant, label string, rewarm bool, cfg map[string]any) *pooler { + p := &pooler{checkout: checkout, base: base, parent: parent, tenant: tenant, label: label, rewarm: rewarm, cmds: warmCommands(cfg), named: map[string]string{}, worktrees: registeredWorktrees(checkout)} _, rows := fleet.MapRows(fleet.RolesMap()) for _, r := range rows { @@ -665,30 +669,91 @@ func anyKeys(m map[string]any) map[string]bool { return out } -// poolTenant settles the tenant BEFORE any worktree exists: given, inherited from -// the checkout's own map line, or ORG_TENANT. +// sameRepoRows finds bindings that share the checkout's git common directory. +// A repository may have several tenants or labels; file order is not authority. +func sameRepoRows(checkout string) []fleet.MapRow { + want := fleet.RepoID(checkout) + if want == "" { + return nil + } + _, rows := fleet.MapRows(fleet.RolesMap()) + var found []fleet.MapRow + for _, r := range rows { + if fleet.RepoID(r.Path) == want { + found = append(found, r) + } + } + return found +} + +// poolTenant inherits a sibling tenant only when all candidates agree. Resolve +// this before creating any seat; an explicit tenant can disambiguate the pool. func poolTenant(tenant, checkout, base string) (string, error) { - if tenant == "" { - tenant = fleet.TenantOf(checkout) + if tenant != "" { + return tenant, nil + } + role, inherited, _ := fleet.MapRowsFor(checkout) + if role != "" && inherited != "" { + return inherited, nil + } + seen := map[string]bool{} + for _, r := range sameRepoRows(checkout) { + seen[r.Tenant] = true + } + if len(seen) > 1 { + return "", refuse("fleet pool: ambiguous tenants for %s in %s: %s; select --tenant before creating seats", base, fleet.RolesMap(), strings.Join(sortedKeys(seen), ", ")) } - if tenant == "" { - tenant = os.Getenv("ORG_TENANT") + for t := range seen { + return t, nil } - if tenant == "" { - return "", refuse("fleet pool: no tenant for slots of %s: %s has no roles.map line to inherit from and ORG_TENANT is unset. Next action: fleet pool %s ... --tenant ", base, checkout, checkout) + if inherited != "" { + return inherited, nil } - return tenant, nil + if tenant = os.Getenv("ORG_TENANT"); tenant != "" { + return tenant, nil + } + return "", refuse("fleet pool: no tenant for slots of %s: %s has no roles.map line to inherit from, no sibling seat of that repo has one, and ORG_TENANT is unset. Next action: fleet pool %s ... --tenant ", base, checkout, checkout) +} + +// poolLabel inherits only within the selected tenant. Conflicting labels require +// reconciliation, not silently rewriting kept seats to the first row's spelling. +func poolLabel(checkout, base, tenant string) (string, error) { + if fleet.TenantOf(checkout) == tenant { + if _, label, ok := strings.Cut(fleet.RoleOf(checkout), ":"); ok && label != "" { + return label, nil + } + } + labels := map[string]bool{} + for _, r := range sameRepoRows(checkout) { + if r.Tenant != tenant { + continue + } + if _, label, ok := strings.Cut(r.Role, ":"); ok && label != "" { + labels[label] = true + } + } + if len(labels) > 1 { + return "", refuse("fleet pool: ambiguous labels for tenant %s in %s: %s; reconcile the bindings before creating seats", tenant, fleet.RolesMap(), strings.Join(sortedKeys(labels), ", ")) + } + for label := range labels { + return label, nil + } + return base, nil } // pooler is one `fleet pool` run: what it knows before the loop, what it did. type pooler struct { checkout, base, parent, tenant string - rewarm bool - cmds []string - named map[string]string - worktrees map[string]bool - live []fleet.Rec - made, kept, warmed []string + // label is the repo label in a seat's role. Distinct from base, which names the + // seat directory: the directory can be called anything, the label must match the + // rest of the repo's roles.map lines. + label string + rewarm bool + cmds []string + named map[string]string + worktrees map[string]bool + live []fleet.Rec + made, kept, warmed []string } // one creates or keeps one seat: never disturbs an occupied one, refuses a name @@ -712,7 +777,7 @@ func (p *pooler) one(k string, i int) error { return refuse("fleet pool: git worktree add %s failed: %s", path, out) } } - if err := cmdRole(path, k+":"+p.base, false, p.tenant, slot); err != nil { + if err := cmdRole(path, k+":"+p.label, false, p.tenant, slot); err != nil { return err } if fresh { @@ -843,12 +908,18 @@ func assignGuards(slot, path, branch string) error { // assignCheckout puts the seat's tree on the branch, creating it from origin when // it is not local, and confirms where the tree landed. func assignCheckout(slot, path, branch string) error { - rc, txt := gitTry(path, gitTimeout, "checkout", "--quiet", branch) + rc, first := gitTry(path, gitTimeout, "checkout", "--quiet", branch) if rc != 0 { - rc, txt = gitTry(path, gitTimeout, "checkout", "--quiet", "-b", branch, "origin/"+branch) - } - if rc != 0 { - return refuse("fleet assign: could not check out %s in %s: %s; nothing was assigned", branch, slot, txt) + // The -b fallback is only for a branch that is not local yet, so when it fails + // too, the first attempt's message is the one worth printing: it names the + // worktree already holding the branch. The fallback can only say the branch + // exists, which is what the first attempt just established, and reporting that + // instead sends the reader back to the checkout that already failed. This is the + // common case when a seat takes over an existing branch, because a branch already + // worked on this machine is local. + if rc2, _ := gitTry(path, gitTimeout, "checkout", "--quiet", "-b", branch, "origin/"+branch); rc2 != 0 { + return refuse("fleet assign: could not check out %s in %s: %s; nothing was assigned", branch, slot, first) + } } _, landed := gitTry(path, gitTimeout, "rev-parse", "--abbrev-ref", "HEAD") if landed != branch { @@ -1072,7 +1143,7 @@ func Unowned(repo string) map[string]any { } pairs = filtered } - assigns := undeliveredAssigns() + assigns := assignsByChange() host, _ := os.Hostname() if host == "" { host = "?" @@ -1117,8 +1188,13 @@ func mapKeys[V any](m map[string]V) map[string]bool { return out } -// undeliveredAssigns is every assignment not yet read by a session, by (repo, branch). -func undeliveredAssigns() map[[2]string]fleet.Rec { +// assignsByChange is EVERY assignment record, keyed by (repo, branch). +// +// Named for what it returns. It was called undeliveredAssigns, whose comment claimed "not +// yet read by a session" — but nothing here filters `delivered_to`; that filter lives in +// the board's row builder. A caller trusting the old name would have silently dropped +// every assignment a session had already picked up. +func assignsByChange() map[[2]string]fleet.Rec { assigns := map[[2]string]fleet.Rec{} d := fleet.Path("assign") ents, _ := os.ReadDir(d) diff --git a/cmd/fleet/internal/verbs/work.go b/cmd/fleet/internal/verbs/work.go index 8ee1760e..a51fbe6d 100644 --- a/cmd/fleet/internal/verbs/work.go +++ b/cmd/fleet/internal/verbs/work.go @@ -337,6 +337,14 @@ func evidenceState(row WorkRow, rid, branch, rel, state string) string { // undeclaredRows is every branch a session holds that no row declares: undeclared // while the holder lives, dead once it does not. +// +// `undeclared` means no ownership ROW exists, which is not the same as nobody being +// accountable. `fleet assign --for ` records the accountable role, the dispatcher, +// the seat and the brief in the assignment; reading none of that made the board print +// "for no one accountable" about a change whose own assignment named the role — two +// records fleet wrote, disagreeing. The STATE stays `undeclared`, which was right: a seat +// assignment is not an ownership declaration. The columns now come from the assignment +// when there is one. func undeclaredRows(declared map[string]bool) []WorkRow { var rows []WorkRow for _, l := range leaseRows() { @@ -350,12 +358,38 @@ func undeclaredRows(declared map[string]bool) []WorkRow { state = "dead" // a dead holder nobody declared is still a dead holder } parts := fleet.KeyParts(key) - rows = append(rows, WorkRow{"change": fleet.S(parts, "branch"), "repo": fleet.S(parts, "repo"), "relationship": nil, "for": nil, "by": nil, - "at": l["at"], "due": nil, "slot": nil, "brief": nil, "key": key, "hands": sid, "state": state, "head": nil, "done_at": nil}) + repo, branch := fleet.S(parts, "repo"), fleet.S(parts, "branch") + row := WorkRow{"change": branch, "repo": repo, "relationship": nil, "for": nil, "by": nil, + "at": l["at"], "due": nil, "slot": nil, "brief": nil, "key": key, "hands": sid, "state": state, "head": nil, "done_at": nil} + if a := holderAssignment(repo, branch, sid); a != nil { + row["for"] = nilIfEmpty(fleet.S(a, "for")) + row["by"] = nilIfEmpty(fleet.S(a, "by")) + row["slot"] = nilIfEmpty(fleet.S(a, "slot")) + row["brief"] = nilIfEmpty(fleet.S(a, "brief")) + } + rows = append(rows, row) } return rows } +// holderAssignment reads the holder's seat, never a branch-wide collapse of +// assignments left behind in other seats. Another session's delivery is stale +// context even when the seat and branch have since been reused. +func holderAssignment(repo, branch, sid string) fleet.Rec { + slot := fleet.S(fleet.SessionRecord(sid), "slot") + if slot == "" { + return nil + } + a := fleet.ReadJSON(fleet.Path("assign", fleet.Safe(slot)+".json")) + if fleet.S(a, "repo") != repo || fleet.S(a, "branch") != branch || fleet.S(a, "slot") != slot { + return nil + } + if recipient := fleet.S(a, "delivered_to"); recipient != "" && recipient != sid { + return nil + } + return a +} + // WorkAttention is the set of work states a hub must decide something about. var WorkAttention = map[string]bool{"dead": true, "late": true, "undeclared": true, "abandoned": true, "failed": true, "unknown": true} diff --git a/cmd/fleet/internal/verbs/work_assignment_test.go b/cmd/fleet/internal/verbs/work_assignment_test.go new file mode 100644 index 00000000..160f2e37 --- /dev/null +++ b/cmd/fleet/internal/verbs/work_assignment_test.go @@ -0,0 +1,33 @@ +package verbs + +import ( + "testing" + + "github.com/itsHabib/workbench/cmd/fleet/internal/fleet" +) + +func TestUndeclaredUsesHolderAssignment(t *testing.T) { + old := fleet.State + fleet.State = t.TempDir() + t.Cleanup(func() { fleet.State = old }) + write := func(p string, r fleet.Rec) { + t.Helper() + if err := fleet.WriteJSON(p, r); err != nil { + t.Fatal(err) + } + } + write(fleet.Path("sessions", "current.json"), fleet.Rec{"session": "current", "slot": "a-current", "last": fleet.Now()}) + write(fleet.KeyFile("leases", "repo:fixture:topic"), fleet.Rec{"key": "repo:fixture:topic", "session": "current", "at": fleet.Now()}) + write(fleet.Path("assign", "a-current.json"), fleet.Rec{"repo": "fixture", "branch": "topic", "slot": "a-current", "delivered_to": "current", "for": "lead:current"}) + write(fleet.Path("assign", "z-old.json"), fleet.Rec{"repo": "fixture", "branch": "topic", "slot": "z-old", "delivered_to": "old", "for": "lead:old"}) + rows := undeclaredRows(map[string]bool{}) + if len(rows) != 1 || fleet.S(rows[0], "for") != "lead:current" || fleet.S(rows[0], "slot") != "a-current" { + t.Fatalf("wrong assignment attached: %v", rows) + } + // A reused seat's record delivered to another session is not the holder's work. + write(fleet.Path("assign", "a-current.json"), fleet.Rec{"repo": "fixture", "branch": "topic", "slot": "a-current", "delivered_to": "other", "for": "lead:other"}) + rows = undeclaredRows(map[string]bool{}) + if len(rows) != 1 || rows[0]["for"] != nil { + t.Fatalf("foreign delivery attached: %v", rows) + } +} diff --git a/cmd/fleet/internal/watch/detach_windows.go b/cmd/fleet/internal/watch/detach_windows.go index 30d6a217..76c5c61d 100644 --- a/cmd/fleet/internal/watch/detach_windows.go +++ b/cmd/fleet/internal/watch/detach_windows.go @@ -7,10 +7,17 @@ import ( "syscall" ) -// detach starts the watcher in a new process group with no console, so the hook's -// exit and the harness's console do not take it. +// detach starts the watcher in a new process group with no console window, so the +// hook's exit and the harness's console do not take it. +// +// CREATE_NO_WINDOW rather than DETACHED_PROCESS. Both keep the child off the parent's +// console, but `go build` produces a console-subsystem binary, and under +// DETACHED_PROCESS Windows gives such a child its own console - which is a window that +// pops on screen for every spawn. Since any session revives a dead watcher, a watcher +// that fails to stay up flashes a window per SessionStart. CREATE_NO_WINDOW is the flag +// that means "console application, no window"; it is documented as invalid combined +// with DETACHED_PROCESS, so this replaces it rather than adding to it. func detach(cmd *exec.Cmd) { - const createNewProcessGroup = 0x00000200 - const detachedProcess = 0x00000008 - cmd.SysProcAttr = &syscall.SysProcAttr{CreationFlags: createNewProcessGroup | detachedProcess} + const createNoWindow = 0x08000000 + cmd.SysProcAttr = &syscall.SysProcAttr{CreationFlags: syscall.CREATE_NEW_PROCESS_GROUP | createNoWindow} } diff --git a/cmd/fleet/internal/watch/detach_windows_test.go b/cmd/fleet/internal/watch/detach_windows_test.go new file mode 100644 index 00000000..de9dabb0 --- /dev/null +++ b/cmd/fleet/internal/watch/detach_windows_test.go @@ -0,0 +1,23 @@ +//go:build windows + +package watch + +import ( + "os/exec" + "testing" +) + +func TestDetachHasNoConsoleWindow(t *testing.T) { + cmd := exec.Command("unused") + detach(cmd) + const noWindow = 0x08000000 + const detachedProcess = 0x00000008 + const newGroup = 0x00000200 + if cmd.SysProcAttr == nil { + t.Fatal("missing process attributes") + } + flags := cmd.SysProcAttr.CreationFlags + if flags&noWindow == 0 || flags&newGroup == 0 || flags&detachedProcess != 0 { + t.Fatalf("unexpected creation flags: %#x", flags) + } +} diff --git a/friction-log.md b/friction-log.md index 3eb271a4..b094525e 100644 --- a/friction-log.md +++ b/friction-log.md @@ -394,3 +394,26 @@ diagnostic fix; do not recommend it as recovery from a possibly active watcher. empty chains, explicit/environment/default tenant selection, and broken chains. - **Boundary:** no live directory cleanup or custody changes; this does not resolve Fleet work with unknown accountable roles or create roles or slots. + +### 2026-09-08 — Pool inherited the first sibling's tenant and label + +- What I tried: review work-machine Fleet #289 on macOS using temporary Git worktrees. +- What happened: two bindings of one repository in different tenants made poolTenant + silently select the first row. The same first-row lookup could supply a label from a + different tenant, or conceal conflicting labels before a pool top-up re-roled seats. +- Class: wrong-default. +- Smallest fix: inherit only an unambiguous tenant, filter sibling labels by the selected + tenant, and refuse conflicting labels before creating seats or rewriting roles.map. +- Status: fixed with real Git regression tests; the new tenant test failed on 0af6100. + Mac Fleet race tests pass. Windows execution is delegated to the portability CI job; + a visible-window check still needs the work machine's next real session start. + +### 2026-09-08 — Board accountability came from an older seat assignment + +- What I tried: fold Codex's review of #289 with a two-seat regression fixture. +- What happened: a branch-wide map collapsed both assignments by filename order, + showing lead:old and z-old against the current holder in a-current. +- Class: misleading-status. +- Smallest fix: read the holder's recorded slot, verify repository/branch/slot, and + reject an assignment delivered to another session. Unknown holder context stays empty. +- Status: fixed; regression failed on 925b785 before the change.