fix(office): stop reports_to cycles from vanishing agents off the org chart (#2827)

Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com>
Co-authored-by: Carlos Florencio <carlosmflorencio@gmail.com>
This commit is contained in:
nova28
2026-08-20 17:20:11 +08:00
committed by GitHub
parent 41fe636e5f
commit aa9d9fd5f1
12 changed files with 359 additions and 20 deletions
@@ -158,6 +158,10 @@ func (h *Handler) createAgent(c *gin.Context) {
}
}
if err := h.svc.CreateAgentInstanceWithCaller(c.Request.Context(), agent, agentCallerFromCtx(c), req.Reason); err != nil {
if code := agentValidationErrorCode(err); code != "" {
c.JSON(http.StatusBadRequest, gin.H{"error": err.Error(), "code": code})
return
}
c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()})
return
}
@@ -210,6 +214,10 @@ func (h *Handler) updateAgent(c *gin.Context) {
}
applyAgentUpdates(agent, &req)
if err := h.svc.UpdateAgentInstance(ctx, agent); err != nil {
if code := agentValidationErrorCode(err); code != "" {
c.JSON(http.StatusBadRequest, gin.H{"error": err.Error(), "code": code})
return
}
c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()})
return
}
@@ -263,6 +271,23 @@ func respondRoutingValidation(c *gin.Context, err error) {
c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})
}
// agentValidationErrorCode maps validation sentinels to stable API codes.
// Clients can translate or handle these codes without matching backend text.
func agentValidationErrorCode(err error) string {
switch {
case errors.Is(err, ErrAgentCEOReportsTo):
return "agent_ceo_reports_to"
case errors.Is(err, ErrAgentReportsToInvalid):
return "agent_reports_to_invalid"
case errors.Is(err, ErrAgentReportsToSelf):
return "agent_reports_to_self"
case errors.Is(err, ErrAgentReportsToCycle):
return "agent_reports_to_cycle"
default:
return ""
}
}
func (h *Handler) deleteAgent(c *gin.Context) {
if caller := agentCallerFromCtx(c); caller != nil {
if !isAdminRole(caller.Role) {
@@ -0,0 +1,100 @@
package agents
import (
"encoding/json"
"net/http"
"testing"
"github.com/kandev/kandev/internal/office/models"
)
func TestUpdateAgent_ReportsToValidationReturnsClientErrorCode(t *testing.T) {
tests := []struct {
name string
setup func(t *testing.T, svc *AgentService) (targetID, reportsTo string)
wantCode string
wantStatus int
}{
{
name: "self",
setup: func(t *testing.T, svc *AgentService) (string, string) {
target := createAndGetAgent(t, svc, svc.repo, &models.AgentInstance{
WorkspaceID: "ws-1",
Name: "Worker",
Role: models.AgentRoleWorker,
})
return target.ID, target.ID
},
wantCode: "agent_reports_to_self",
wantStatus: http.StatusBadRequest,
},
{
name: "unknown manager",
setup: func(t *testing.T, svc *AgentService) (string, string) {
target := createAndGetAgent(t, svc, svc.repo, &models.AgentInstance{
WorkspaceID: "ws-1",
Name: "Worker",
Role: models.AgentRoleWorker,
})
return target.ID, "missing-manager"
},
wantCode: "agent_reports_to_invalid",
wantStatus: http.StatusBadRequest,
},
{
name: "cycle",
setup: func(t *testing.T, svc *AgentService) (string, string) {
ceo := createAndGetAgent(t, svc, svc.repo, &models.AgentInstance{
WorkspaceID: "ws-1",
Name: "CEO",
Role: models.AgentRoleCEO,
})
manager := createAndGetAgent(t, svc, svc.repo, &models.AgentInstance{
WorkspaceID: "ws-1",
Name: "Manager",
Role: models.AgentRoleWorker,
ReportsTo: ceo.ID,
})
worker := createAndGetAgent(t, svc, svc.repo, &models.AgentInstance{
WorkspaceID: "ws-1",
Name: "Worker",
Role: models.AgentRoleWorker,
ReportsTo: manager.ID,
})
return manager.ID, worker.ID
},
wantCode: "agent_reports_to_cycle",
wantStatus: http.StatusBadRequest,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
svc, _ := newTestAgentService(t)
targetID, reportsTo := tt.setup(t, svc)
rec := newPatchAgentRecorder(t, svc, targetID, mustJSON(t, map[string]string{
"reports_to": reportsTo,
}))
if rec.Code != tt.wantStatus {
t.Fatalf("status = %d, want %d; body=%s", rec.Code, tt.wantStatus, rec.Body.String())
}
var body map[string]any
if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil {
t.Fatalf("decode response: %v", err)
}
if body["code"] != tt.wantCode {
t.Errorf("code = %v, want %q; body=%s", body["code"], tt.wantCode, rec.Body.String())
}
})
}
}
func mustJSON(t *testing.T, value any) string {
t.Helper()
data, err := json.Marshal(value)
if err != nil {
t.Fatalf("marshal request: %v", err)
}
return string(data)
}
+34 -19
View File
@@ -240,6 +240,20 @@ func (s *AgentService) GetAgentFromConfig(ctx context.Context, idOrName string)
return agent, nil
}
// getAgentFromConfigInWorkspace resolves an ID or name without crossing a
// workspace boundary. IDs are globally unique, while names are workspace-scoped.
func (s *AgentService) getAgentFromConfigInWorkspace(
ctx context.Context, idOrName, workspaceID string,
) (*models.AgentInstance, error) {
if agent, err := s.repo.GetAgentInstance(ctx, idOrName); err == nil {
if agent.WorkspaceID != workspaceID {
return nil, ErrAgentReportsToInvalid
}
return agent, nil
}
return s.repo.GetAgentInstanceByName(ctx, workspaceID, idOrName)
}
// ListAgentsFromConfig returns all agent instances for a workspace.
// An empty workspaceID returns rows across all workspaces.
func (s *AgentService) ListAgentsFromConfig(ctx context.Context, workspaceID string) ([]*models.AgentInstance, error) {
@@ -769,7 +783,7 @@ func (s *AgentService) validateAgentCreate(ctx context.Context, agent *models.Ag
return err
}
if agent.ReportsTo != "" {
return s.validateReportsTo(ctx, agent.ReportsTo, "", agent.WorkspaceID)
return s.validateReportsTo(ctx, agent)
}
return nil
}
@@ -791,7 +805,7 @@ func (s *AgentService) validateAgentUpdate(ctx context.Context, agent *models.Ag
}
}
if agent.ReportsTo != "" {
return s.validateReportsTo(ctx, agent.ReportsTo, agent.ID, agent.WorkspaceID)
return s.validateReportsTo(ctx, agent)
}
return nil
}
@@ -821,40 +835,41 @@ func (s *AgentService) validateAgentNameUnique(ctx context.Context, name, worksp
}
// validateReportsTo ensures the target agent exists in the same workspace and
// that changing the edge does not create a cycle in the reporting tree.
func (s *AgentService) validateReportsTo(
ctx context.Context, reportsTo, selfID, workspaceID string,
) error {
if selfID != "" && reportsTo == selfID {
return ErrAgentReportsToSelf
}
target, err := s.GetAgentFromConfig(ctx, reportsTo)
// that changing the edge does not create a cycle in the reporting tree. On
// success it normalizes agent.ReportsTo to the target's ID, so a caller that
// names its manager rather than IDing it persists the canonical ID like every
// other reference — not the raw name, which would otherwise mismatch a
// same-named agent in another workspace on the next read.
func (s *AgentService) validateReportsTo(ctx context.Context, agent *models.AgentInstance) error {
selfID := agent.ID
target, err := s.getAgentFromConfigInWorkspace(ctx, agent.ReportsTo, agent.WorkspaceID)
if err != nil {
return ErrAgentReportsToInvalid
}
if target.WorkspaceID != workspaceID {
return ErrAgentReportsToInvalid
if selfID != "" && target.ID == selfID {
return ErrAgentReportsToSelf
}
agent.ReportsTo = target.ID
if selfID == "" {
return nil
}
agents, err := s.ListAgentsFromConfig(ctx, workspaceID)
agents, err := s.ListAgentsFromConfig(ctx, agent.WorkspaceID)
if err != nil {
return ErrAgentReportsToInvalid
}
parentByID := make(map[string]string, len(agents))
idByReference := make(map[string]string, len(agents)*2)
for _, agent := range agents {
parentByID[agent.ID] = agent.ReportsTo
idByReference[agent.ID] = agent.ID
if agent.Name != "" {
idByReference[agent.Name] = agent.ID
for _, a := range agents {
parentByID[a.ID] = a.ReportsTo
idByReference[a.ID] = a.ID
if a.Name != "" {
idByReference[a.Name] = a.ID
}
}
// Validate the proposed edge, not the stale value currently stored for the
// agent being updated.
parentByID[selfID] = reportsTo
parentByID[selfID] = target.ID
currentID := target.ID
visited := make(map[string]struct{}, len(agents))
@@ -0,0 +1,136 @@
package agents
import (
"context"
"errors"
"testing"
"github.com/kandev/kandev/internal/office/models"
)
// TestUpdateAgentInstance_ReportsToTwoAgentCycleRejected reproduces the
// scenario the card exists to close: bob already reports_to carol (set via
// a prior, valid update), then a PATCH sets carol reports_to bob. That
// closes a two-agent cycle and must be rejected with ErrAgentReportsToCycle,
// leaving carol's persisted reports_to unchanged.
func TestUpdateAgentInstance_ReportsToTwoAgentCycleRejected(t *testing.T) {
svc, repo := newTestAgentService(t)
ctx := context.Background()
bob := createAndGetAgent(t, svc, repo, &models.AgentInstance{
WorkspaceID: "ws-1", Name: "bob", Role: models.AgentRoleWorker,
})
carol := createAndGetAgent(t, svc, repo, &models.AgentInstance{
WorkspaceID: "ws-1", Name: "carol", Role: models.AgentRoleWorker,
})
bob.ReportsTo = carol.ID
if err := svc.UpdateAgentInstance(ctx, bob); err != nil {
t.Fatalf("bob reports_to carol: %v", err)
}
carol.ReportsTo = bob.ID
err := svc.UpdateAgentInstance(ctx, carol)
if !errors.Is(err, ErrAgentReportsToCycle) {
t.Fatalf("carol reports_to bob: got err %v, want ErrAgentReportsToCycle", err)
}
stored, getErr := repo.GetAgentInstance(ctx, carol.ID)
if getErr != nil {
t.Fatalf("GetAgentInstance: %v", getErr)
}
if stored.ReportsTo != "" {
t.Fatalf("carol reports_to persisted as %q despite rejected update", stored.ReportsTo)
}
}
// TestUpdateAgentInstance_ReportsToChainCycleRejected covers a longer chain:
// alice -> bob -> carol, then closing the loop with carol reports_to alice.
func TestUpdateAgentInstance_ReportsToChainCycleRejected(t *testing.T) {
svc, repo := newTestAgentService(t)
ctx := context.Background()
alice := createAndGetAgent(t, svc, repo, &models.AgentInstance{
WorkspaceID: "ws-1", Name: "alice", Role: models.AgentRoleWorker,
})
bob := createAndGetAgent(t, svc, repo, &models.AgentInstance{
WorkspaceID: "ws-1", Name: "bob", Role: models.AgentRoleWorker,
})
carol := createAndGetAgent(t, svc, repo, &models.AgentInstance{
WorkspaceID: "ws-1", Name: "carol", Role: models.AgentRoleWorker,
})
bob.ReportsTo = alice.ID
if err := svc.UpdateAgentInstance(ctx, bob); err != nil {
t.Fatalf("bob reports_to alice: %v", err)
}
carol.ReportsTo = bob.ID
if err := svc.UpdateAgentInstance(ctx, carol); err != nil {
t.Fatalf("carol reports_to bob: %v", err)
}
alice.ReportsTo = carol.ID
err := svc.UpdateAgentInstance(ctx, alice)
if !errors.Is(err, ErrAgentReportsToCycle) {
t.Fatalf("alice reports_to carol: got err %v, want ErrAgentReportsToCycle", err)
}
}
// TestUpdateAgentInstance_ReportsToDeepChainNoCycleResolves is the
// false-positive guard: a valid, non-cyclic chain of updates must all
// succeed.
func TestUpdateAgentInstance_ReportsToDeepChainNoCycleResolves(t *testing.T) {
svc, repo := newTestAgentService(t)
ctx := context.Background()
names := []string{"a", "b", "c", "d"}
created := make([]*models.AgentInstance, len(names))
for i, name := range names {
created[i] = createAndGetAgent(t, svc, repo, &models.AgentInstance{
WorkspaceID: "ws-1", Name: name, Role: models.AgentRoleWorker,
})
}
// b -> a, c -> b, d -> c: a valid deep chain, no cycle anywhere.
for i := 1; i < len(created); i++ {
created[i].ReportsTo = created[i-1].ID
if err := svc.UpdateAgentInstance(ctx, created[i]); err != nil {
t.Fatalf("%s reports_to %s: %v", names[i], names[i-1], err)
}
}
stored, err := repo.GetAgentInstance(ctx, created[len(created)-1].ID)
if err != nil {
t.Fatalf("GetAgentInstance: %v", err)
}
if stored.ReportsTo != created[len(created)-2].ID {
t.Fatalf("d reports_to = %q, want %q", stored.ReportsTo, created[len(created)-2].ID)
}
}
func TestCreateAgentInstance_ReportsToNameStaysInWorkspace(t *testing.T) {
svc, repo := newTestAgentService(t)
ctx := context.Background()
createAndGetAgent(t, svc, repo, &models.AgentInstance{
WorkspaceID: "ws-other", Name: "Manager", Role: models.AgentRoleWorker,
})
manager := createAndGetAgent(t, svc, repo, &models.AgentInstance{
WorkspaceID: "ws-1", Name: "Manager", Role: models.AgentRoleWorker,
})
worker := &models.AgentInstance{
WorkspaceID: "ws-1", Name: "Worker", Role: models.AgentRoleWorker,
ReportsTo: "Manager",
}
if err := svc.CreateAgentInstance(ctx, worker); err != nil {
t.Fatalf("create worker: %v", err)
}
stored, err := repo.GetAgentInstance(ctx, worker.ID)
if err != nil {
t.Fatalf("get worker: %v", err)
}
if stored.ReportsTo != manager.ID {
t.Fatalf("worker reports_to = %q, want local manager %q", stored.ReportsTo, manager.ID)
}
}
@@ -1,7 +1,9 @@
import { afterEach, describe, expect, it, vi } from "vitest";
import { cleanup, fireEvent, render, screen, waitFor, within } from "@testing-library/react";
import { toast } from "sonner";
import { StateProvider } from "@/components/state-provider";
import { updateAgentProfile } from "@/lib/api/domains/office-api";
import { ApiError } from "@/lib/api/client";
import type { AgentProfile } from "@/lib/state/slices/office/types";
import { agentProfileId as toAgentProfileId } from "@/lib/types/ids";
import { defaultOfficeState } from "@/lib/state/slices/office/office-slice";
@@ -213,6 +215,20 @@ describe("AgentConfigurationTab save errors", () => {
expect(screen.queryByRole("button", { name: SAVE_BUTTON })).not.toBeNull();
});
});
it("translates a reporting validation error code", async () => {
vi.mocked(updateAgentProfile).mockRejectedValueOnce(
new ApiError("raw backend message", 400, { code: "agent_reports_to_cycle" }),
);
renderConfigTab([baseAgent], baseAgent);
fireEvent.change(screen.getByLabelText("Name"), { target: { value: "Retry CEO" } });
fireEvent.click(screen.getByRole("button", { name: SAVE_BUTTON }));
await waitFor(() => {
expect(toast.error).toHaveBeenCalledWith("The reporting structure cannot contain a cycle.");
});
});
});
describe("AgentConfigurationTab draft navigation", () => {
@@ -11,6 +11,7 @@ import { toast } from "@/lib/toast/sonner";
import { useAppStore } from "@/components/state-provider";
import { selectOfficeAgentProfiles } from "@/lib/state/slices/office/selectors";
import { updateAgentProfile } from "@/lib/api/domains/office-api";
import { ApiError } from "@/lib/api/client";
import type { AgentProfile, AgentRole } from "@/lib/state/slices/office/types";
import { AgentRoutingCard } from "./agent-routing-card";
import { reportsToOptions } from "./reports-to-options";
@@ -20,6 +21,25 @@ type AgentConfigurationTabProps = {
agent: AgentProfile;
};
function agentValidationMessage(error: unknown, translate: (key: string) => string): string | null {
if (!(error instanceof ApiError) || !error.body || typeof error.body !== "object") return null;
const code = "code" in error.body ? error.body.code : null;
if (typeof code !== "string") return null;
// i18n-exempt: backend validation codes, not user-facing copy.
switch (code) {
case "agent_ceo_reports_to":
return translate("office:agentCeoReportsToError");
case "agent_reports_to_invalid":
return translate("office:agentReportsToInvalidError");
case "agent_reports_to_self":
return translate("office:agentReportsToSelfError");
case "agent_reports_to_cycle":
return translate("office:agentReportsToCycleError");
default:
return null;
}
}
// Catalog keys, not copy — module scope freezes a `t()` at the boot locale.
// The record keys are wire values and stay untranslated.
const FALLBACK_ROLES: Array<{ id: string; labelKey: string }> = [
@@ -239,7 +259,10 @@ function useAgentConfigurationForm(agent: AgentProfile) {
setDirty(false);
toast.success(t("office:agentConfigurationUpdated"));
} catch (err) {
toast.error(err instanceof Error ? err.message : t("office:failedToUpdateAgent"));
toast.error(
agentValidationMessage(err, t) ??
(err instanceof Error ? err.message : t("office:failedToUpdateAgent")),
);
} finally {
if (activeAgentIdRef.current === targetAgentId && saveRequestRef.current === requestId) {
setSaving(false);
+4
View File
@@ -34,6 +34,7 @@
"agentAutoPaused": "Agent auto-paused",
"agentAwaitingApproval": "Agent awaiting approval",
"agentConfigurationUpdated": "Agent configuration updated",
"agentCeoReportsToError": "A CEO agent must not have a manager.",
"agentCount_one": "{{count}} agent",
"agentCount_other": "{{count}} agents",
"agentCreated": "Agent created",
@@ -47,6 +48,9 @@
"agentRecentTasks": "Recent tasks",
"agentRunActivity": "Run activity",
"agentRunFailed": "Agent run failed",
"agentReportsToCycleError": "The reporting structure cannot contain a cycle.",
"agentReportsToInvalidError": "Select an agent from this workspace.",
"agentReportsToSelfError": "An agent cannot report to itself.",
"agents": "Agents",
"agentSections": "Agent sections",
"agentsEnabled": "Agents Enabled",
+4
View File
@@ -34,6 +34,7 @@
"agentAutoPaused": "Àĝēńţ àũţō-ƥàũśēď",
"agentAwaitingApproval": "Àĝēńţ àŵàĩţĩńĝ àƥƥŕōvàĺ",
"agentConfigurationUpdated": "Àĝēńţ ćōńƒĩĝũŕàţĩōń ũƥďàţēď",
"agentCeoReportsToError": "À ĆĒŐ àĝēńţ mũśţ ńōţ ħàvē à màńàĝēŕ.",
"agentCount_one": "{{count}} àĝēńţ",
"agentCount_other": "{{count}} àĝēńţś",
"agentCreated": "Àĝēńţ ćŕēàţēď",
@@ -47,6 +48,9 @@
"agentRecentTasks": "Ŕēćēńţ ţàśķś",
"agentRunActivity": "Ŕũń àćţĩvĩţŷ",
"agentRunFailed": "Àĝēńţ ŕũń ƒàĩĺēď",
"agentReportsToCycleError": "Ŧħē ŕēƥōŕţĩńĝ śţŕũćţũŕē ćàńńōţ ćōńţàĩń à ćŷćĺē.",
"agentReportsToInvalidError": "Ŝēĺēćţ àń àĝēńţ ƒŕōm ţħĩś ŵōŕķśƥàćē.",
"agentReportsToSelfError": "Àń àĝēńţ ćàńńōţ ŕēƥōŕţ ţō ĩţśēĺƒ.",
"agents": "Àĝēńţś",
"agentSections": "Àĝēńţ śēćţĩōńś",
"agentsEnabled": "Àĝēńţś Ēńàƀĺēď",
+4
View File
@@ -34,6 +34,7 @@
"agentAutoPaused": "Agente pausado automaticamente",
"agentAwaitingApproval": "Agente à espera de aprovação",
"agentConfigurationUpdated": "Configuração do agente atualizada",
"agentCeoReportsToError": "Um agente CEO não pode ter um gestor.",
"agentCount_one": "{{count}} agente",
"agentCount_other": "{{count}} agentes",
"agentCreated": "Agente criado",
@@ -47,6 +48,9 @@
"agentRecentTasks": "Tarefas recentes",
"agentRunActivity": "Atividade das execuções",
"agentRunFailed": "A execução do agente falhou",
"agentReportsToCycleError": "A estrutura de reporte não pode conter um ciclo.",
"agentReportsToInvalidError": "Selecione um agente deste espaço de trabalho.",
"agentReportsToSelfError": "Um agente não pode reportar a si próprio.",
"agents": "Agentes",
"agentSections": "Secções do agente",
"agentsEnabled": "Agentes ativados",
+4
View File
@@ -34,6 +34,7 @@
"agentAutoPaused": "智能体已自动暂停",
"agentAwaitingApproval": "智能体等待审批",
"agentConfigurationUpdated": "智能体配置已更新",
"agentCeoReportsToError": "CEO 智能体不能有上级。",
"agentCount_one": "{{count}} 个智能体",
"agentCount_other": "{{count}} 个智能体",
"agentCreated": "智能体已创建",
@@ -47,6 +48,9 @@
"agentRecentTasks": "最近任务",
"agentRunActivity": "运行活动",
"agentRunFailed": "智能体运行失败",
"agentReportsToCycleError": "汇报结构不能形成循环。",
"agentReportsToInvalidError": "请选择此工作区中的智能体。",
"agentReportsToSelfError": "智能体不能向自己汇报。",
"agents": "智能体",
"agentSections": "智能体分区",
"agentsEnabled": "已启用智能体",
+4
View File
@@ -34,6 +34,7 @@
"agentAutoPaused": "代理程式已自動暫停",
"agentAwaitingApproval": "代理程式等待審批",
"agentConfigurationUpdated": "代理程式設定檔已更新",
"agentCeoReportsToError": "CEO 代理程式不可有上級。",
"agentCount_one": "{{count}} 個代理程式",
"agentCount_other": "{{count}} 個代理程式",
"agentCreated": "代理程式已建立",
@@ -47,6 +48,9 @@
"agentRecentTasks": "最近任務",
"agentRunActivity": "執行活動",
"agentRunFailed": "代理程式執行失敗",
"agentReportsToCycleError": "匯報結構不可形成循環。",
"agentReportsToInvalidError": "請選擇此工作區內的代理程式。",
"agentReportsToSelfError": "代理程式不可向自己匯報。",
"agents": "代理程式",
"agentSections": "代理程式分區",
"agentsEnabled": "已啓用代理程式",
+4
View File
@@ -34,6 +34,7 @@
"agentAutoPaused": "代理程式已自動暫停",
"agentAwaitingApproval": "代理程式等待審批",
"agentConfigurationUpdated": "代理程式設定檔已更新",
"agentCeoReportsToError": "CEO 代理程式不可有上級。",
"agentCount_one": "{{count}} 個代理程式",
"agentCount_other": "{{count}} 個代理程式",
"agentCreated": "代理程式已建立",
@@ -47,6 +48,9 @@
"agentRecentTasks": "最近任務",
"agentRunActivity": "執行活動",
"agentRunFailed": "代理程式執行失敗",
"agentReportsToCycleError": "彙報結構不可形成循環。",
"agentReportsToInvalidError": "請選擇此工作區中的代理程式。",
"agentReportsToSelfError": "代理程式不可向自己彙報。",
"agents": "代理程式",
"agentSections": "代理程式分割槽",
"agentsEnabled": "已啟用代理程式",