From ad123ba23cded37f0e2cbe2effcf1d719fce9b16 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 14:04:01 -0400 Subject: [PATCH 1/2] fix(ci): node reports cannot set a hosted location or nself.* labels (D-1402) --- free/ci/internal/nodes/registry/registry.go | 42 ++++++++++++++++ .../internal/nodes/registry/registry_test.go | 50 +++++++++++++++++++ 2 files changed, 92 insertions(+) diff --git a/free/ci/internal/nodes/registry/registry.go b/free/ci/internal/nodes/registry/registry.go index b3be2395c..c04236387 100644 --- a/free/ci/internal/nodes/registry/registry.go +++ b/free/ci/internal/nodes/registry/registry.go @@ -7,6 +7,7 @@ import ( "github.com/nself-org/plugins/free/ci/internal/model" "github.com/nself-org/plugins/free/ci/internal/store" "reflect" + "strings" "time" ) @@ -151,6 +152,10 @@ func (r *Registry) UpdateCapability(ctx context.Context, id string, in model.Cap in.Availability.Persistence = old.Availability.Persistence in.Availability.OnBattery = old.Availability.OnBattery in.Availability.InteractivePolicy = old.Availability.InteractivePolicy + if !strings.HasPrefix(actor, PolicyActorPrefix) { + in.Location.Kind = keepHostedKind(old.Location.Kind, in.Location.Kind) + in.Identity.Labels = keepProviderLabels(old.Identity.Labels, in.Identity.Labels) + } in.SecretsEligible = eligible(in) if reportedIsolation.Value != nil && old.Trust.Isolation.Value != nil && isolationRank(*reportedIsolation.Value) < isolationRank(*old.Trust.Isolation.Value) { in.Trust.Isolation = reportedIsolation @@ -165,6 +170,43 @@ func (r *Registry) UpdateCapability(ctx context.Context, id string, in model.Cap }) return n, drift, err } + +// PolicyActorPrefix marks a coordinator-side policy actor (the burst provider is "policy:burst"). Only such an +// actor may set or clear a node's hosted location or its nself.* labels; a node report may not (D-1402), because +// a hosted runner skips the AuthorizedProjects check in sched eligibility. +const PolicyActorPrefix = "policy:" + +func isHosted(f model.Fact[string]) bool { return f.Value != nil && *f.Value == "hosted" } + +// keepHostedKind returns the reported location kind, except that a report cannot make a node hosted or stop it +// being hosted: then the stored kind stays. +func keepHostedKind(old, reported model.Fact[string]) model.Fact[string] { + if isHosted(old) != isHosted(reported) { + return old + } + return reported +} + +// keepProviderLabels returns the reported labels with the nself.* ones replaced by the stored nself.* ones. +func keepProviderLabels(old, reported []string) []string { + const prefix = "nself." + var out []string + for _, l := range reported { + if !strings.HasPrefix(l, prefix) { + out = append(out, l) + } + } + for _, l := range old { + if strings.HasPrefix(l, prefix) { + out = append(out, l) + } + } + if reported == nil && out == nil { + return nil + } + return out +} + func isolationRank(i model.Isolation) int { for n, v := range model.EnumValues("Isolation") { if v == string(i) { diff --git a/free/ci/internal/nodes/registry/registry_test.go b/free/ci/internal/nodes/registry/registry_test.go index f56422272..5b82b1e08 100644 --- a/free/ci/internal/nodes/registry/registry_test.go +++ b/free/ci/internal/nodes/registry/registry_test.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" "reflect" + "slices" "strings" "sync" "testing" @@ -287,3 +288,52 @@ func TestTrustRequiresAdminAuthority(t *testing.T) { t.Fatal("store accepted zero authority") } } + +// TestNodeReportCannotSetHostedOrProviderLabels: a node report may not make itself hosted (which skips the +// AuthorizedProjects check) or claim an nself.* label; the burst policy actor still can (D-1402). +func TestNodeReportCannotSetHostedOrProviderLabels(t *testing.T) { + r, c := setup(t) + ctx := context.Background() + id := c.Identity.ID + hosted := "hosted" + report := c + report.Location.Kind = model.Fact[string]{Value: &hosted, Source: "self", ObservedAt: time.Now().UTC(), Confidence: "known"} + report.Identity.Labels = append(append([]string(nil), c.Identity.Labels...), "nself.repo=acme/app", "nself.provider=github", "gpu=none") + for _, actor := range []string{"agent", "probe", "system-install"} { + n, _, err := r.UpdateCapability(ctx, id, report, actor) + if err != nil { + t.Fatalf("%s: %v", actor, err) + } + if k := n.Capability.Location.Kind.Value; k != nil && *k == "hosted" { + t.Fatalf("%s: a node report made the node hosted", actor) + } + for _, l := range n.Capability.Identity.Labels { + if strings.HasPrefix(l, "nself.") { + t.Fatalf("%s: a node report added label %q", actor, l) + } + } + if !slices.Contains(n.Capability.Identity.Labels, "gpu=none") { + t.Fatalf("%s: an ordinary label was dropped: %v", actor, n.Capability.Identity.Labels) + } + } + // The coordinator-side policy actor sets both, and a later node report can neither clear nor replace them. + n, _, err := r.UpdateCapability(ctx, id, report, PolicyActorPrefix+"burst") + if err != nil { + t.Fatal(err) + } + if k := n.Capability.Location.Kind.Value; k == nil || *k != "hosted" || !slices.Contains(n.Capability.Identity.Labels, "nself.repo=acme/app") { + t.Fatalf("policy actor could not set hosted and labels: %v %v", n.Capability.Location.Kind.Value, n.Capability.Identity.Labels) + } + back := c + back.Identity.Labels = []string{"nself.repo=evil/other"} + n, _, err = r.UpdateCapability(ctx, id, back, "agent") + if err != nil { + t.Fatal(err) + } + if k := n.Capability.Location.Kind.Value; k == nil || *k != "hosted" { + t.Fatal("a node report cleared the hosted location") + } + if !slices.Contains(n.Capability.Identity.Labels, "nself.repo=acme/app") || slices.Contains(n.Capability.Identity.Labels, "nself.repo=evil/other") { + t.Fatalf("a node report replaced a provider label: %v", n.Capability.Identity.Labels) + } +} From 9fbd742ec0dede8d039d797ee1ca3b8b2827ae54 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 14:04:02 -0400 Subject: [PATCH 2/2] fix(ci): list --coordinator in nodes set and nodes upgrade flag sets (D-1409) --- free/ci/cmd/nodes_remote.go | 8 +++----- free/ci/cmd/nodes_remote_test.go | 16 ++++++++++++++++ free/ci/cmd/nodes_set.go | 1 + free/ci/internal/agent/update/cli.go | 1 + free/ci/internal/nodes/lifecycle/hooks.go | 7 +++++++ 5 files changed, 28 insertions(+), 5 deletions(-) diff --git a/free/ci/cmd/nodes_remote.go b/free/ci/cmd/nodes_remote.go index 6eea58491..0a18f8cf1 100644 --- a/free/ci/cmd/nodes_remote.go +++ b/free/ci/cmd/nodes_remote.go @@ -17,7 +17,6 @@ import ( "bytes" "context" "errors" - "flag" "os" "slices" "strings" @@ -54,11 +53,10 @@ type nodeAdminState struct { var nodeAdminDeps atomic.Pointer[nodeAdminState] func init() { - // Add --coordinator to the flag sets the lifecycle owns (help and the generated reference list it). + // Add --coordinator to the flag sets the lifecycle owns (help and the generated reference list it); `nodes set` and + // `nodes upgrade` declare it in their own flag sets. for _, sub := range []string{"add", "list", "show", "test", "drain", "enable", "revoke", "remove", "rotate"} { - lifecycle.RegisterFlags(sub, func(fs *flag.FlagSet) { - fs.String("coordinator", "", "run this on the TEAM coordinator at https://host[:port] (an admin or member user credential; not for add, pair or test)") - }) + lifecycle.RegisterFlags(sub, lifecycle.AddCoordinatorFlag) } if err := clientapi.RegisterNodeRoutes(func() clientapi.NodeRunner { if nodeAdminDeps.Load() == nil { diff --git a/free/ci/cmd/nodes_remote_test.go b/free/ci/cmd/nodes_remote_test.go index 6716503fa..190a2ab1c 100644 --- a/free/ci/cmd/nodes_remote_test.go +++ b/free/ci/cmd/nodes_remote_test.go @@ -360,3 +360,19 @@ func TestNodesRemoteUpgradeClaimIsPerNodeAndRefusesNotQueues(t *testing.T) { rel() } } + +// TestRoutedNodesFlagSetsListCoordinator: help and the generated flag reference come from each subcommand's +// own flag set, so every routed `nodes` command must declare --coordinator there (D-1409). +func TestRoutedNodesFlagSetsListCoordinator(t *testing.T) { + for sub := range nodesRoutedSubs { + key := "nodes " + sub + e, ok := v15Subcommands[key] + if !ok || e.Flags == nil { + t.Errorf("%s: no V15 entry with a flag set", key) + continue + } + if e.Flags().Lookup("coordinator") == nil { + t.Errorf("%s: its flag set does not declare --coordinator", key) + } + } +} diff --git a/free/ci/cmd/nodes_set.go b/free/ci/cmd/nodes_set.go index d4af2b712..7dcfcfd7f 100644 --- a/free/ci/cmd/nodes_set.go +++ b/free/ci/cmd/nodes_set.go @@ -43,6 +43,7 @@ func nodesSetFlagSet(f *nodesSetFlags, errOut io.Writer) *flag.FlagSet { fs.StringVar(&f.interactive, "interactive", "", "protect (background work waits while the operator is active) or ignore") fs.StringVar(&f.onBattery, "on-battery", "", "pause (no new leases), drain (no new leases and drain the node) or allow") fs.StringVar(&f.persistence, "persistence", "", "persistent or intermittent (a laptop that sleeps)") + lifecycle.AddCoordinatorFlag(fs) return fs } diff --git a/free/ci/internal/agent/update/cli.go b/free/ci/internal/agent/update/cli.go index 7e523901c..679d28eec 100644 --- a/free/ci/internal/agent/update/cli.go +++ b/free/ci/internal/agent/update/cli.go @@ -67,6 +67,7 @@ func FlagSet(out io.Writer) *flag.FlagSet { fs.Duration("drain-wait", DefaultDrainWait, "how long to wait for running jobs before giving up") fs.Duration("hello-wait", DefaultHelloWait, "how long the upgraded agent has to say hello before the upgrade is rolled back") fs.Bool("json", false, "print one v1 JSON envelope") + lifecycle.AddCoordinatorFlag(fs) return fs } diff --git a/free/ci/internal/nodes/lifecycle/hooks.go b/free/ci/internal/nodes/lifecycle/hooks.go index 7f8ac9427..090192afe 100644 --- a/free/ci/internal/nodes/lifecycle/hooks.go +++ b/free/ci/internal/nodes/lifecycle/hooks.go @@ -132,6 +132,13 @@ func RegisterDefaultSource(fn DefaultSourceFunc) { ext.defSource = fn } +// CoordinatorUsage is the help text of --coordinator, the one flag every routed `nodes` subcommand takes. +const CoordinatorUsage = "run this on the TEAM coordinator at https://host[:port] (an admin or member user credential; not for add, pair or test)" + +// AddCoordinatorFlag declares --coordinator on a `nodes` flag set so help and the generated reference list it. +// The cmd dispatcher takes the flag out of argv before the subcommand parses, so the value is never read here. +func AddCoordinatorFlag(fs *flag.FlagSet) { fs.String("coordinator", "", CoordinatorUsage) } + // RegisterFlags lets a later Ticket add flags to a subcommand. func RegisterFlags(subcommand string, fn FlagsFunc) { if subcommand == "" || fn == nil {