diff --git a/agentteams-controller/internal/server/http.go b/agentteams-controller/internal/server/http.go index 9aa4c1f5a..8df3fca20 100644 --- a/agentteams-controller/internal/server/http.go +++ b/agentteams-controller/internal/server/http.go @@ -86,6 +86,7 @@ func NewHTTPServer(addr string, deps ServerDeps) *HTTPServer { mux.Handle("POST /api/v1/humans", mw.RequireAuthz(authpkg.ActionCreate, "human", nil)(http.HandlerFunc(rh.CreateHuman))) mux.Handle("GET /api/v1/humans", mw.RequireAuthz(authpkg.ActionList, "human", nil)(http.HandlerFunc(rh.ListHumans))) mux.Handle("GET /api/v1/humans/{name}", mw.RequireAuthz(authpkg.ActionGet, "human", nameFn)(http.HandlerFunc(rh.GetHuman))) + mux.Handle("PUT /api/v1/humans/{name}", mw.RequireAuthz(authpkg.ActionUpdate, "human", nameFn)(http.HandlerFunc(rh.UpdateHuman))) mux.Handle("DELETE /api/v1/humans/{name}", mw.RequireAuthz(authpkg.ActionDelete, "human", nameFn)(http.HandlerFunc(rh.DeleteHuman))) // Managers diff --git a/agentteams-controller/internal/server/resource_handler.go b/agentteams-controller/internal/server/resource_handler.go index c42f4e802..1a72f664e 100644 --- a/agentteams-controller/internal/server/resource_handler.go +++ b/agentteams-controller/internal/server/resource_handler.go @@ -267,6 +267,48 @@ func (h *ResourceHandler) UpdateWorker(w http.ResponseWriter, r *http.Request) { } } +func (h *ResourceHandler) UpdateHuman(w http.ResponseWriter, r *http.Request) { + name := r.PathValue("name") + if name == "" { + httputil.WriteError(w, http.StatusBadRequest, "human name is required") + return + } + + var req UpdateHumanRequest + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + httputil.WriteError(w, http.StatusBadRequest, "invalid JSON: "+err.Error()) + return + } + + ctx := r.Context() + for attempt := 0; attempt < k8sUpdateMaxRetries; attempt++ { + var human v1beta1.Human + if err := h.client.Get(ctx, client.ObjectKey{Name: name, Namespace: h.namespace}, &human); err != nil { + writeK8sError(w, "get human for update", err) + return + } + + if req.AccessibleTeams != nil { + human.Spec.AccessibleTeams = req.AccessibleTeams + } + if req.AccessibleWorkers != nil { + human.Spec.AccessibleWorkers = req.AccessibleWorkers + } + + if err := h.client.Update(ctx, &human); err != nil { + if apierrors.IsConflict(err) && attempt+1 < k8sUpdateMaxRetries { + time.Sleep(time.Duration(attempt+1) * 100 * time.Millisecond) + continue + } + writeK8sError(w, "update human", err) + return + } + + httputil.WriteJSON(w, http.StatusOK, humanToResponse(&human)) + return + } +} + func (h *ResourceHandler) DeleteWorker(w http.ResponseWriter, r *http.Request) { name := r.PathValue("name") if name == "" { diff --git a/agentteams-controller/internal/server/resource_handler_test.go b/agentteams-controller/internal/server/resource_handler_test.go index 33ba900f7..548fa4d90 100644 --- a/agentteams-controller/internal/server/resource_handler_test.go +++ b/agentteams-controller/internal/server/resource_handler_test.go @@ -883,3 +883,156 @@ func assertAgentResources(t *testing.T, got *v1beta1.AgentResourceRequirements, t.Fatalf("limits.memory = %q, want %q (resources=%+v)", got.Limits.Memory, memLimit, got) } } + +// TestUpdateHumanPartialSpecPreservesUnsentFields pins the nil-skip semantic +// that partial-spec PUTs rely on: a body carrying only accessibleWorkers must +// replace that list and leave every other spec field untouched. +func TestUpdateHumanPartialSpecPreservesUnsentFields(t *testing.T) { + scheme := newServerTestScheme(t) + human := &v1beta1.Human{ + ObjectMeta: metav1.ObjectMeta{Name: "alice", Namespace: "default"}, + Spec: v1beta1.HumanSpec{ + DisplayName: "Alice", + Email: "alice@example.com", + PermissionLevel: 2, + AccessibleTeams: []string{"team-one"}, + AccessibleWorkers: []string{"w-alpha"}, + Note: "keep me", + }, + } + k8sClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(human).Build() + handler := NewResourceHandler(k8sClient, "default", nil, "") + + body := []byte(`{"accessibleWorkers":["w-alpha","w-beta"]}`) + req := httptest.NewRequest(http.MethodPut, "/api/v1/humans/alice", bytes.NewReader(body)) + req.SetPathValue("name", "alice") + rec := httptest.NewRecorder() + handler.UpdateHuman(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("expected status %d, got %d: %s", http.StatusOK, rec.Code, rec.Body.String()) + } + var got v1beta1.Human + if err := k8sClient.Get(context.Background(), client.ObjectKey{Name: "alice", Namespace: "default"}, &got); err != nil { + t.Fatalf("get human: %v", err) + } + if !reflect.DeepEqual(got.Spec.AccessibleWorkers, []string{"w-alpha", "w-beta"}) { + t.Errorf("accessibleWorkers = %v, want [w-alpha w-beta]", got.Spec.AccessibleWorkers) + } + if got.Spec.DisplayName != "Alice" { + t.Errorf("displayName = %q, want %q (unsent field must be preserved)", got.Spec.DisplayName, "Alice") + } + if got.Spec.Email != "alice@example.com" { + t.Errorf("email = %q, want %q (unsent field must be preserved)", got.Spec.Email, "alice@example.com") + } + if got.Spec.PermissionLevel != 2 { + t.Errorf("permissionLevel = %d, want 2 (unsent field must be preserved)", got.Spec.PermissionLevel) + } + if !reflect.DeepEqual(got.Spec.AccessibleTeams, []string{"team-one"}) { + t.Errorf("accessibleTeams = %v, want [team-one] (unsent field must be preserved)", got.Spec.AccessibleTeams) + } + if got.Spec.Note != "keep me" { + t.Errorf("note = %q, want %q (unsent field must be preserved)", got.Spec.Note, "keep me") + } +} + +// TestUpdateHumanReplacesAccessibleTeams pins the other half of the partial +// update surface: a body carrying only accessibleTeams replaces that list. +func TestUpdateHumanReplacesAccessibleTeams(t *testing.T) { + scheme := newServerTestScheme(t) + human := &v1beta1.Human{ + ObjectMeta: metav1.ObjectMeta{Name: "bob", Namespace: "default"}, + Spec: v1beta1.HumanSpec{ + DisplayName: "Bob", + PermissionLevel: 2, + AccessibleTeams: []string{"team-one"}, + AccessibleWorkers: []string{"w-alpha"}, + }, + } + k8sClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(human).Build() + handler := NewResourceHandler(k8sClient, "default", nil, "") + + body := []byte(`{"accessibleTeams":["team-one","team-two"]}`) + req := httptest.NewRequest(http.MethodPut, "/api/v1/humans/bob", bytes.NewReader(body)) + req.SetPathValue("name", "bob") + rec := httptest.NewRecorder() + handler.UpdateHuman(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("expected status %d, got %d: %s", http.StatusOK, rec.Code, rec.Body.String()) + } + var got v1beta1.Human + if err := k8sClient.Get(context.Background(), client.ObjectKey{Name: "bob", Namespace: "default"}, &got); err != nil { + t.Fatalf("get human: %v", err) + } + if !reflect.DeepEqual(got.Spec.AccessibleTeams, []string{"team-one", "team-two"}) { + t.Errorf("accessibleTeams = %v, want [team-one team-two]", got.Spec.AccessibleTeams) + } + if !reflect.DeepEqual(got.Spec.AccessibleWorkers, []string{"w-alpha"}) { + t.Errorf("accessibleWorkers = %v, want [w-alpha] (unsent field must be preserved)", got.Spec.AccessibleWorkers) + } +} + +// TestHumanPutRouteIsRegistered pins the route table itself, not the handler. +// A registered handler is useless if no PUT pattern matches: net/http's mux +// answers 405 Method Not Allowed, which is exactly how the missing route +// surfaced against stock v1.2.0. +func TestHumanPutRouteIsRegistered(t *testing.T) { + srv := NewHTTPServer("127.0.0.1:0", ServerDeps{ + Namespace: "default", + AuthMw: authpkg.NewMiddleware(nil, nil, nil, nil, "default"), + }) + + req := httptest.NewRequest(http.MethodPut, "/api/v1/humans/alice", nil) + _, pattern := srv.Mux.Handler(req) + if pattern == "" { + t.Fatalf("no route matches PUT /api/v1/humans/{name} — mux would answer 405") + } + if pattern != "PUT /api/v1/humans/{name}" { + t.Errorf("matched pattern = %q, want %q", pattern, "PUT /api/v1/humans/{name}") + } +} + +// TestUpdateHumanClearsAccessibleSetsWithEmptyArrays pins the revoke path: +// an explicit empty array must clear the set, as distinct from omitting the +// field (which preserves it). Revoking the last team or worker grant sends +// [], so a regression here would silently leave access in place. +func TestUpdateHumanClearsAccessibleSetsWithEmptyArrays(t *testing.T) { + scheme := newServerTestScheme(t) + human := &v1beta1.Human{ + ObjectMeta: metav1.ObjectMeta{Name: "carol", Namespace: "default"}, + Spec: v1beta1.HumanSpec{ + DisplayName: "Carol", + PermissionLevel: 2, + Note: "keep me", + AccessibleTeams: []string{"team-one", "team-two"}, + AccessibleWorkers: []string{"w-alpha", "w-beta"}, + }, + } + k8sClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(human).Build() + handler := NewResourceHandler(k8sClient, "default", nil, "") + + body := []byte(`{"accessibleTeams":[],"accessibleWorkers":[]}`) + req := httptest.NewRequest(http.MethodPut, "/api/v1/humans/carol", bytes.NewReader(body)) + req.SetPathValue("name", "carol") + rec := httptest.NewRecorder() + handler.UpdateHuman(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("expected status %d, got %d: %s", http.StatusOK, rec.Code, rec.Body.String()) + } + var got v1beta1.Human + if err := k8sClient.Get(context.Background(), client.ObjectKey{Name: "carol", Namespace: "default"}, &got); err != nil { + t.Fatalf("get human: %v", err) + } + if len(got.Spec.AccessibleTeams) != 0 { + t.Errorf("accessibleTeams = %v, want empty (explicit [] must revoke)", got.Spec.AccessibleTeams) + } + if len(got.Spec.AccessibleWorkers) != 0 { + t.Errorf("accessibleWorkers = %v, want empty (explicit [] must revoke)", got.Spec.AccessibleWorkers) + } + if got.Spec.DisplayName != "Carol" || got.Spec.Note != "keep me" || got.Spec.PermissionLevel != 2 { + t.Errorf("unsent fields mutated: displayName=%q note=%q permissionLevel=%d", + got.Spec.DisplayName, got.Spec.Note, got.Spec.PermissionLevel) + } +} diff --git a/agentteams-controller/internal/server/types.go b/agentteams-controller/internal/server/types.go index a28c98a34..58d6178a4 100644 --- a/agentteams-controller/internal/server/types.go +++ b/agentteams-controller/internal/server/types.go @@ -151,6 +151,15 @@ type CreateHumanRequest struct { Note string `json:"note,omitempty"` } +// UpdateHumanRequest carries a partial Human spec. Every field is optional: +// a field left out of the JSON body is skipped, leaving the stored value +// untouched. Slice fields distinguish "omitted" (nil, skip) from "cleared" +// (empty non-nil slice, replace with empty). +type UpdateHumanRequest struct { + AccessibleTeams []string `json:"accessibleTeams,omitempty"` + AccessibleWorkers []string `json:"accessibleWorkers,omitempty"` +} + type HumanResponse struct { Name string `json:"name"` Phase string `json:"phase"`