fix: address runtime unbind review nits

Co-authored-by: multica-agent <github@multica.ai>
This commit is contained in:
Eve
2026-07-31 15:28:24 +08:00
parent 348e72605b
commit ee354a7758
5 changed files with 183 additions and 40 deletions

View File

@@ -1,4 +1,4 @@
import { act, render, screen, waitFor } from "@testing-library/react";
import { act, fireEvent, render, screen, waitFor } from "@testing-library/react";
import { createRef, type ReactNode } from "react";
import { describe, it, expect, vi, beforeEach } from "vitest";
import { workspaceKeys } from "@multica/core/workspace/queries";
@@ -10,9 +10,16 @@ import enCommon from "../../locales/en/common.json";
import enAuth from "../../locales/en/auth.json";
import enSettings from "../../locales/en/settings.json";
import enEditor from "../../locales/en/editor.json";
import enIssues from "../../locales/en/issues.json";
const TEST_RESOURCES = {
en: { common: enCommon, auth: enAuth, settings: enSettings, editor: enEditor },
en: {
common: enCommon,
auth: enAuth,
settings: enSettings,
editor: enEditor,
issues: enIssues,
},
};
function I18nWrapper({ children }: { children: ReactNode }) {
@@ -49,6 +56,12 @@ vi.mock("@multica/core/auth", () => ({
useAuthStore: { getState: () => authState },
}));
vi.mock("../../common/actor-avatar", () => ({
ActorAvatar: ({ actorId }: { actorId: string }) => (
<span data-testid={`actor-${actorId}`} />
),
}));
import {
createMentionSuggestion,
MentionList,
@@ -186,8 +199,9 @@ describe("createMentionSuggestion", () => {
expect(items.some((i) => i.type === "agent" && i.label === "Aegis")).toBe(true);
});
it("does not offer an unbound agent as a mention target", () => {
it("keeps an unbound agent discoverable but marks it as unselectable", () => {
const qc = fakeQc({
members: [{ user_id: "u1", name: "Alice", role: "member" }],
agents: [
{
id: "a1",
@@ -204,11 +218,49 @@ describe("createMentionSuggestion", () => {
const config = createMentionSuggestion(qc);
const items = config.items!(itemArgs("a")) as MentionItem[];
expect(items.some((item) => item.type === "agent" && item.id === "a1")).toBe(
false,
expect(items).toContainEqual(
expect.objectContaining({
type: "agent",
id: "a1",
disabledReason: "agent_runtime_required",
}),
);
});
it("does not select a runtime-required mention row by click or keyboard", () => {
const command = vi.fn<(item: MentionItem) => void>();
const ref = createRef<MentionListRef>();
render(
<I18nWrapper>
<MentionList
ref={ref}
items={[
{
id: "a1",
label: "Aegis",
type: "agent",
disabledReason: "agent_runtime_required",
},
]}
query=""
command={command}
/>
</I18nWrapper>,
);
const row = screen.getByRole("button", {
name: "Aegis: This target has no runtime — bind one to run it",
});
expect(row).toHaveAttribute("aria-disabled", "true");
fireEvent.click(row);
expect(
ref.current?.onKeyDown({
event: new KeyboardEvent("keydown", { key: "Enter" }),
}),
).toBe(true);
expect(command).not.toHaveBeenCalled();
});
it("loads server issue matches into the popup when the list cache misses", async () => {
searchIssuesMock.mockResolvedValue({
issues: [
@@ -683,7 +735,7 @@ describe("createMentionSuggestion", () => {
expect(items.some((i) => i.type === "squad" && i.label === "Archived Squad")).toBe(false);
});
it("does not offer a squad whose leader is unbound", () => {
it("keeps a squad with an unbound leader discoverable but unselectable", () => {
const qc = fakeQc({
agents: [
{
@@ -709,8 +761,12 @@ describe("createMentionSuggestion", () => {
const config = createMentionSuggestion(qc);
const items = config.items!(itemArgs("")) as MentionItem[];
expect(items.some((item) => item.type === "squad" && item.id === "s1")).toBe(
false,
expect(items).toContainEqual(
expect.objectContaining({
type: "squad",
id: "s1",
disabledReason: "agent_runtime_required",
}),
);
});

View File

@@ -32,6 +32,11 @@ import { StatusIcon } from "../../issues/components/status-icon";
import { ProjectIcon } from "../../projects/components/project-icon";
import { useT } from "../../i18n";
import { Badge } from "@multica/ui/components/ui/badge";
import {
Tooltip,
TooltipContent,
TooltipTrigger,
} from "@multica/ui/components/ui/tooltip";
import { cn } from "@multica/ui/lib/utils";
import type { IssueStatus, ProjectStatus } from "@multica/core/types";
import { PROJECT_STATUS_CONFIG } from "@multica/core/projects/config";
@@ -49,6 +54,7 @@ import {
pickerNavigationDirection,
} from "./suggestion-popup";
import { isTriggerArmedAt } from "./suggestion-trigger-arming";
import { blockedReasonLabel } from "../../issues/blocked-trigger-copy";
// ---------------------------------------------------------------------------
// Types
@@ -68,6 +74,8 @@ export interface MentionItem {
icon?: string | null;
/** Project status snapshot for recent/current project rendering */
projectStatus?: ProjectStatus;
/** Present when the target should remain discoverable but cannot be selected. */
disabledReason?: "agent_runtime_required";
}
interface MentionListProps {
@@ -258,18 +266,23 @@ export const MentionList = forwardRef<MentionListRef, MentionListProps>(
// yet, fall back to the first row. This self-heals across reorders and
// async result arrival without ever force-resetting an active selection.
const selectedIndex = useMemo(() => {
if (selectedKey === null) return 0;
const firstSelectable = orderedItems.findIndex((item) => !item.disabledReason);
if (selectedKey === null) return firstSelectable;
const i = orderedItems.findIndex((it) => mentionItemKey(it) === selectedKey);
return i === -1 ? 0 : i;
return i === -1 || orderedItems[i]?.disabledReason
? firstSelectable
: i;
}, [orderedItems, selectedKey]);
useEffect(() => {
itemRefs.current[selectedIndex]?.scrollIntoView({ block: "nearest" });
if (selectedIndex >= 0) {
itemRefs.current[selectedIndex]?.scrollIntoView({ block: "nearest" });
}
}, [selectedIndex]);
const selectItem = useCallback(
(item: MentionItem | undefined) => {
if (!item) return;
if (!item || item.disabledReason) return;
const wsId = getCurrentWsId();
if (wsId) recordMentionUsage(wsId, item);
command(item);
@@ -286,16 +299,25 @@ export const MentionList = forwardRef<MentionListRef, MentionListProps>(
// see pickerNavigationDirection.
const direction = pickerNavigationDirection(event);
if (direction !== null) {
if (orderedItems.length === 0) return true;
const delta = direction === "next" ? 1 : orderedItems.length - 1;
const next = (selectedIndex + delta) % orderedItems.length;
const selectableIndexes = orderedItems.flatMap((item, index) =>
item.disabledReason ? [] : [index],
);
if (selectableIndexes.length === 0) return true;
const current = selectableIndexes.indexOf(selectedIndex);
const delta =
direction === "next" ? 1 : selectableIndexes.length - 1;
const next =
selectableIndexes[
((current === -1 ? 0 : current) + delta) %
selectableIndexes.length
]!;
setSelectedKey(mentionItemKey(orderedItems[next]!));
return true;
}
// Enter is the canonical accept; plain Tab is an additive alias (see
// isPickerAcceptKey). Shift/modifier+Tab fall through to focus nav.
if (isPickerAcceptKey(event)) {
if (orderedItems.length === 0) return true;
if (selectedIndex < 0) return true;
selectItem(orderedItems[selectedIndex]);
return true;
}
@@ -396,6 +418,7 @@ function MentionRow({
buttonRef: (el: HTMLButtonElement | null) => void;
}) {
const { t } = useT("editor");
const { t: issuesT } = useT("issues");
if (item.type === "issue") {
// Visually dim closed issues (done/cancelled) so they're distinguishable
// from active ones in the suggestion list — they're still selectable.
@@ -461,13 +484,20 @@ function MentionRow({
);
}
return (
const disabledMessage = item.disabledReason
? blockedReasonLabel(item.disabledReason, issuesT)
: null;
const button = (
<button
type="button"
ref={buttonRef}
aria-disabled={disabledMessage ? true : undefined}
aria-label={
disabledMessage ? `${item.label}: ${disabledMessage}` : undefined
}
className={`flex w-full items-center gap-2.5 px-3 py-1.5 text-left text-caption transition-colors ${
selected ? "bg-accent" : "hover:bg-accent/50"
}`}
selected ? "bg-accent" : disabledMessage ? "" : "hover:bg-accent/50"
} ${disabledMessage ? "cursor-not-allowed opacity-50" : ""}`}
onClick={onSelect}
>
<ActorAvatar
@@ -491,6 +521,17 @@ function MentionRow({
)}
</button>
);
if (!disabledMessage) return button;
return (
<Tooltip>
<TooltipTrigger render={button} />
<TooltipContent side="top" className="max-w-72 text-caption">
{disabledMessage}
</TooltipContent>
</Tooltip>
);
}
// ---------------------------------------------------------------------------
@@ -587,25 +628,38 @@ export function createMentionSuggestion(
.filter(
(a) =>
!a.archived_at &&
isAgentRuntimeBound(a) &&
(a.name.toLowerCase().includes(q) || matchesPinyin(a.name, q)) &&
canAssignAgentToIssue(a, { userId, role: myRole }).allowed,
)
.map((a) => ({ id: a.id, label: a.name, type: "agent" as const }));
const runnableAgentIds = new Set(
.map((a) => ({
id: a.id,
label: a.name,
type: "agent" as const,
disabledReason: isAgentRuntimeBound(a)
? undefined
: ("agent_runtime_required" as const),
}));
const activeAgentRuntimeBinding = new Map(
agents
.filter((agent) => !agent.archived_at && isAgentRuntimeBound(agent))
.map((agent) => agent.id),
.filter((agent) => !agent.archived_at)
.map((agent) => [agent.id, isAgentRuntimeBound(agent)]),
);
const squadItems: MentionItem[] = squads
.filter(
(s) =>
!s.archived_at &&
runnableAgentIds.has(s.leader_id) &&
activeAgentRuntimeBinding.has(s.leader_id) &&
(s.name.toLowerCase().includes(q) || matchesPinyin(s.name, q)),
)
.map((s) => ({ id: s.id, label: s.name, type: "squad" as const }));
.map((s) => ({
id: s.id,
label: s.name,
type: "squad" as const,
disabledReason: activeAgentRuntimeBinding.get(s.leader_id)
? undefined
: ("agent_runtime_required" as const),
}));
// Members and agents share a single ranked list — recently mentioned
// targets come first regardless of type, with an alphabetical fallback

View File

@@ -943,7 +943,7 @@ func (h *Handler) DeleteAgentRuntime(w http.ResponseWriter, r *http.Request) {
// KEY SHARE lock through their runtime FK, so no active agent can appear
// after this check and then be silently unbound by the teardown.
if _, err := qtx.LockAgentRuntime(r.Context(), rt.ID); err != nil {
writeError(w, http.StatusNotFound, "runtime not found")
writeError(w, http.StatusInternalServerError, "failed to lock runtime")
return
}
if _, err := qtx.ListUserAgentsByRuntimeForUpdate(r.Context(), rt.ID); err != nil {
@@ -1171,12 +1171,6 @@ func (h *Handler) UnbindAgentsAndDeleteRuntime(w http.ResponseWriter, r *http.Re
return
}
// Build the agent ID list once — it is the explicit allowlist the
// confirmed-set check is about. Nothing below keys off it: the teardown
// unbinds by runtime_id, and the locks above guarantee the set cannot grow
// between the check and the unbind.
_ = currentActive
// Single teardown, shared with the light DELETE path: unbind every user
// agent (active and archived) plus their task history, cancel what was
// running or queued, and hard-delete only the system agents. Nothing the

View File

@@ -410,9 +410,9 @@ func (h *Handler) UpdateSquad(w http.ResponseWriter, r *http.Request) {
if !ok {
return
}
// Stabilize runtime_id/archived_at through commit. Runtime teardown
// takes FOR UPDATE on this row and follows the same Agent→Autopilot lock
// order, so whichever operation starts first produces a complete result.
// Stabilize runtime_id through commit. Runtime teardown takes FOR UPDATE
// on this row and follows the same Agent→Autopilot lock order, so
// whichever operation starts first produces a complete result.
newLeader, err := qtx.LockAgentForAutopilotAssignment(r.Context(), db.LockAgentForAutopilotAssignmentParams{
ID: lid,
WorkspaceID: wsUUID,
@@ -421,10 +421,6 @@ func (h *Handler) UpdateSquad(w http.ResponseWriter, r *http.Request) {
writeError(w, http.StatusBadRequest, "leader must be a valid agent in this workspace")
return
}
if newLeader.ArchivedAt.Valid {
writeError(w, http.StatusUnprocessableEntity, "leader agent is archived; restore it or pick a different leader")
return
}
// A non-admin creator may only promote an agent they can @-trigger.
if !h.memberCanWireAgent(r.Context(), member, newLeader, workspaceID) {
writeError(w, http.StatusForbidden, "you can only use an agent you have access to as leader")

View File

@@ -10,6 +10,7 @@ import (
"net/http/httptest"
"sort"
"testing"
"time"
)
func TestCreateWorkspace_RejectsReservedSlug(t *testing.T) {
@@ -823,6 +824,48 @@ RETURNING id
}
}
// TestDeleteMember_CancelsDeferredTasks covers the second caller of
// CancelAgentTasksByRuntimeOrAgent. Member revocation must cancel a scheduled
// fallback just like queued/running work; otherwise it could become claimable
// after its owner and runtime access have been removed.
func TestDeleteMember_CancelsDeferredTasks(t *testing.T) {
fx := setupRevocationFixture(t, "handler-tests-revoke-deferred", "daemon-revoke-deferred")
ctx := context.Background()
var deferredTaskID string
if err := testPool.QueryRow(ctx, `
INSERT INTO agent_task_queue (agent_id, runtime_id, status, priority, fire_at)
VALUES ($1, $2, 'deferred', 0, now() + interval '1 hour')
RETURNING id
`, fx.AgentID, fx.RuntimeID).Scan(&deferredTaskID); err != nil {
t.Fatalf("insert deferred task: %v", err)
}
w := httptest.NewRecorder()
req := newRequest("DELETE", "/api/workspaces/"+fx.WorkspaceID+"/members/"+fx.MemberID, nil)
req.Header.Set("X-Workspace-ID", fx.WorkspaceID)
req = withURLParams(req, "id", fx.WorkspaceID, "memberId", fx.MemberID)
testHandler.DeleteMember(w, req)
if w.Code != http.StatusNoContent {
t.Fatalf("DeleteMember: expected 204, got %d: %s", w.Code, w.Body.String())
}
assertRevoked(t, fx)
var status string
var completedAt *time.Time
if err := testPool.QueryRow(ctx,
`SELECT status, completed_at FROM agent_task_queue WHERE id = $1`,
deferredTaskID,
).Scan(&status, &completedAt); err != nil {
t.Fatalf("query deferred task: %v", err)
}
if status != "cancelled" || completedAt == nil {
t.Fatalf("deferred task = (%q, %v), want cancelled with completed_at", status, completedAt)
}
}
// TestDeleteMember_NoRuntimes_DeletesMember covers the empty-revocation
// path: a member with no owned runtimes should still have their member row
// deleted by the same atomic transaction, with no spurious archive/cancel