From 1acc4516eca822c0089fbb841588e5d39716db98 Mon Sep 17 00:00:00 2001 From: J Date: Tue, 14 Jul 2026 17:01:45 +0800 Subject: [PATCH] fix(admission): typed reason codes + run-now whitelist + real security tests (MUL-4525) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Elon's review of the P0 first increment (must-fix 1–3). 1. Run now no longer has a false-success branch. handleRunNow now classifies on a whitelist via a pure runNowToastKind(): only issue_created/running → success; skipped → warning; failed and any unknown/future status → error. Add run-now-toast unit test over all five status classes plus reason-code → key mapping. 2. reason_code is a typed value decided at the admission source, not reverse-engineered from English failure text. New leaf package internal/dispatch holds the canonical ReasonCode enum (shared by handler + service so they can never drift). shouldSkipDispatch / errDispatchSkipped / the fail path now carry a typed code through DispatchAutopilotManual straight into the response; the substring classifier is deleted. Fixes the two missed branches: attribution fail-closed → attribution_blocked (typed errors.Is), "agent has no runtime bound" → runtime_offline. Regression tests for both. 3. Security acceptance tests exercise the REAL handlers, not injected callbacks: - Chat: create session while invokable → revoke invoke (flip to private, keep owner-view) → send returns 403 + reason_code with zero chat_message / task writes. - Rerun: private historical agent through RerunIssue + canInvokeAgent — a non-invoking workspace owner is refused 403 + reason_code and mutates nothing (fail-before-mutation); the agent owner is allowed 202. No migration; reason_code is a decision-time value only the manual "run now" response carries. Additive on the wire. Backend build/vet/handler+service suites and FE typecheck/lint/vitest green. (Migration-prefix collision with main remains the deferred pre-merge renumber.) Co-authored-by: multica-agent --- .../components/autopilot-detail-page.tsx | 41 ++-- .../components/run-now-toast.test.ts | 42 ++++ .../autopilots/components/run-now-toast.ts | 55 +++++ packages/views/locales/en/autopilots.json | 1 + packages/views/locales/ja/autopilots.json | 1 + packages/views/locales/ko/autopilots.json | 1 + .../views/locales/zh-Hans/autopilots.json | 1 + server/internal/dispatch/reason.go | 41 ++++ server/internal/handler/admission.go | 49 ++-- .../admission_security_mul4525_test.go | 226 ++++++++++++++++++ server/internal/handler/admission_test.go | 67 ++---- server/internal/handler/autopilot.go | 75 ++---- .../service/admission_mul4525_test.go | 11 +- server/internal/service/autopilot.go | 108 ++++++--- server/internal/service/reason_code_test.go | 53 ++++ 15 files changed, 584 insertions(+), 188 deletions(-) create mode 100644 packages/views/autopilots/components/run-now-toast.test.ts create mode 100644 packages/views/autopilots/components/run-now-toast.ts create mode 100644 server/internal/dispatch/reason.go create mode 100644 server/internal/handler/admission_security_mul4525_test.go create mode 100644 server/internal/service/reason_code_test.go diff --git a/packages/views/autopilots/components/autopilot-detail-page.tsx b/packages/views/autopilots/components/autopilot-detail-page.tsx index 42ef601523..ce06b12b57 100644 --- a/packages/views/autopilots/components/autopilot-detail-page.tsx +++ b/packages/views/autopilots/components/autopilot-detail-page.tsx @@ -62,6 +62,7 @@ import type { AgentTask } from "@multica/core/types/agent"; import { ReadonlyContent } from "../../editor"; import { TranscriptButton } from "../../common/task-transcript"; import { AutopilotDialog } from "./autopilot-dialog"; +import { runNowToastKind, runNowBlockedKey } from "./run-now-toast"; import { WebhookPayloadPreview } from "./webhook-payload-preview"; import { WebhookDeliveriesSection } from "./webhook-deliveries-section"; import { ProjectIcon } from "../../projects/components/project-icon"; @@ -695,36 +696,26 @@ export function AutopilotDetailPage({ autopilotId }: { autopilotId: string }) { // doesn't send the field (older backend). const canManageAccess = autopilot.can_manage_access ?? canWrite; - // Maps a blocked/failed run's stable reason_code to a localized "not - // triggered" message. A server-driven code always has a default branch so an - // unknown/newer code degrades to a generic message instead of nothing. - const runNowBlockedMessage = (reasonCode?: string): string => { - switch (reasonCode) { - case "invocation_not_allowed": - return t(($) => $.detail.run_blocked_invocation_not_allowed); - case "runtime_offline": - return t(($) => $.detail.run_blocked_runtime_offline); - case "target_unavailable": - return t(($) => $.detail.run_blocked_target_unavailable); - case "attribution_blocked": - return t(($) => $.detail.run_blocked_attribution); - default: - return t(($) => $.detail.run_blocked_generic); - } - }; - const handleRunNow = async () => { try { const run = await triggerAutopilot.mutateAsync(autopilotId); - // A blocked or not-ready dispatch returns 200 with a non-success run - // status (MUL-4525): never show success for skipped/failed. reason_code is - // the stable, localized cause; an unknown code falls back to a generic - // "not triggered" message rather than echoing the raw server reason. - if (run?.status === "skipped" || run?.status === "failed") { - toast.warning(runNowBlockedMessage(run?.reason_code)); + // Manual "run now" returns 200 even when admission blocks the run, so the + // toast is driven by the run's domain status, not the HTTP 2xx (MUL-4525). + // Success is a whitelist (issue_created/running) — a skipped run warns, a + // failed or unknown/future status errors — never a false "triggered". + const kind = runNowToastKind(run?.status); + if (kind === "success") { + toast.success(t(($) => $.detail.toast_triggered)); return; } - toast.success(t(($) => $.detail.toast_triggered)); + // reason_code is the stable, typed cause the server decided at admission + // time; an unknown/absent code degrades to a generic "not triggered". + const message = t(($) => $.detail[runNowBlockedKey(run?.reason_code)]); + if (kind === "warning") { + toast.warning(message); + } else { + toast.error(message); + } } catch (e: any) { toast.error(e?.message || t(($) => $.detail.toast_trigger_failed)); } diff --git a/packages/views/autopilots/components/run-now-toast.test.ts b/packages/views/autopilots/components/run-now-toast.test.ts new file mode 100644 index 0000000000..58f0728485 --- /dev/null +++ b/packages/views/autopilots/components/run-now-toast.test.ts @@ -0,0 +1,42 @@ +import { describe, expect, it } from "vitest"; +import { runNowToastKind, runNowBlockedKey } from "./run-now-toast"; + +// Elon must-fix 1 (MUL-4525): the "run now" toast must be a whitelist — only +// explicit start statuses are success; every other class (including an unknown +// future status) must NOT show a false "triggered". +describe("runNowToastKind", () => { + it("shows success only for explicit start statuses", () => { + expect(runNowToastKind("issue_created")).toBe("success"); + expect(runNowToastKind("running")).toBe("success"); + }); + + it("warns on a skipped (admission-blocked) run", () => { + expect(runNowToastKind("skipped")).toBe("warning"); + }); + + it("errors on a failed run", () => { + expect(runNowToastKind("failed")).toBe("error"); + }); + + it("errors on an unknown/future status instead of claiming success", () => { + expect(runNowToastKind("blocked")).toBe("error"); + expect(runNowToastKind("deferred")).toBe("error"); + expect(runNowToastKind("")).toBe("error"); + expect(runNowToastKind(undefined)).toBe("error"); + }); +}); + +describe("runNowBlockedKey", () => { + it("maps each known reason_code to its localized message key", () => { + expect(runNowBlockedKey("invocation_not_allowed")).toBe("run_blocked_invocation_not_allowed"); + expect(runNowBlockedKey("runtime_offline")).toBe("run_blocked_runtime_offline"); + expect(runNowBlockedKey("target_unavailable")).toBe("run_blocked_target_unavailable"); + expect(runNowBlockedKey("attribution_blocked")).toBe("run_blocked_attribution"); + expect(runNowBlockedKey("already_active")).toBe("run_blocked_already_active"); + }); + + it("degrades an unknown or absent code to the generic message", () => { + expect(runNowBlockedKey("some_future_code")).toBe("run_blocked_generic"); + expect(runNowBlockedKey(undefined)).toBe("run_blocked_generic"); + }); +}); diff --git a/packages/views/autopilots/components/run-now-toast.ts b/packages/views/autopilots/components/run-now-toast.ts new file mode 100644 index 0000000000..ebe9103a39 --- /dev/null +++ b/packages/views/autopilots/components/run-now-toast.ts @@ -0,0 +1,55 @@ +// Classifies a manual "run now" outcome into a toast (MUL-4525). Pure so it can +// be unit-tested across every run-status class without mounting the page. +// +// The response schema deliberately accepts any status string for forward +// compatibility, so success must be a WHITELIST — never "anything that isn't +// skipped/failed". A future or anomalous-but-parseable status (e.g. "blocked", +// "deferred") must degrade to an error toast, never a false "triggered". + +export type RunNowToastKind = "success" | "warning" | "error"; + +// Only explicit start statuses count as success. Everything else is a +// non-success outcome the user must be warned/errored about. +export function runNowToastKind(status: string | undefined): RunNowToastKind { + switch (status) { + case "issue_created": + case "running": + return "success"; + case "skipped": + // Admission blocked / target not ready — recoverable, informational. + return "warning"; + case "failed": + return "error"; + default: + // Unknown / future status: never claim success. + return "error"; + } +} + +// The i18n key (under the autopilots `detail` namespace) describing why a +// non-success run did not trigger, keyed on the stable server reason_code. An +// unknown/absent code degrades to a generic message. +export type RunNowBlockedKey = + | "run_blocked_invocation_not_allowed" + | "run_blocked_runtime_offline" + | "run_blocked_target_unavailable" + | "run_blocked_attribution" + | "run_blocked_already_active" + | "run_blocked_generic"; + +export function runNowBlockedKey(reasonCode: string | undefined): RunNowBlockedKey { + switch (reasonCode) { + case "invocation_not_allowed": + return "run_blocked_invocation_not_allowed"; + case "runtime_offline": + return "run_blocked_runtime_offline"; + case "target_unavailable": + return "run_blocked_target_unavailable"; + case "attribution_blocked": + return "run_blocked_attribution"; + case "already_active": + return "run_blocked_already_active"; + default: + return "run_blocked_generic"; + } +} diff --git a/packages/views/locales/en/autopilots.json b/packages/views/locales/en/autopilots.json index 39718609f0..7785fae662 100644 --- a/packages/views/locales/en/autopilots.json +++ b/packages/views/locales/en/autopilots.json @@ -78,6 +78,7 @@ "run_blocked_runtime_offline": "Not triggered — the assignee's runtime is offline", "run_blocked_target_unavailable": "Not triggered — the assignee is unavailable", "run_blocked_attribution": "Not triggered — the run could not be attributed to a responsible member", + "run_blocked_already_active": "Not triggered — a recent run already covers this", "run_blocked_generic": "Not triggered — blocked by an admission policy", "section_properties": "Properties", "section_triggers": "Triggers", diff --git a/packages/views/locales/ja/autopilots.json b/packages/views/locales/ja/autopilots.json index 024b8f87ae..a49f79c2ed 100644 --- a/packages/views/locales/ja/autopilots.json +++ b/packages/views/locales/ja/autopilots.json @@ -78,6 +78,7 @@ "run_blocked_runtime_offline": "実行されませんでした — 担当のランタイムがオフラインです", "run_blocked_target_unavailable": "実行されませんでした — 担当が利用できません", "run_blocked_attribution": "実行されませんでした — 実行を責任者に帰属できませんでした", + "run_blocked_already_active": "実行されませんでした — 直近の実行が本件をカバーしています", "run_blocked_generic": "実行されませんでした — 受付ポリシーによりブロックされました", "section_properties": "プロパティ", "section_triggers": "トリガー", diff --git a/packages/views/locales/ko/autopilots.json b/packages/views/locales/ko/autopilots.json index 8c8eb70460..820126c907 100644 --- a/packages/views/locales/ko/autopilots.json +++ b/packages/views/locales/ko/autopilots.json @@ -78,6 +78,7 @@ "run_blocked_runtime_offline": "실행되지 않음 — 담당의 런타임이 오프라인입니다", "run_blocked_target_unavailable": "실행되지 않음 — 담당을 사용할 수 없습니다", "run_blocked_attribution": "실행되지 않음 — 실행을 책임 구성원에게 귀속할 수 없습니다", + "run_blocked_already_active": "실행되지 않음 — 최근 실행이 이미 이 건을 처리했습니다", "run_blocked_generic": "실행되지 않음 — 승인 정책에 의해 차단되었습니다", "section_properties": "속성", "section_triggers": "트리거", diff --git a/packages/views/locales/zh-Hans/autopilots.json b/packages/views/locales/zh-Hans/autopilots.json index 7b46250b48..365ccb2694 100644 --- a/packages/views/locales/zh-Hans/autopilots.json +++ b/packages/views/locales/zh-Hans/autopilots.json @@ -78,6 +78,7 @@ "run_blocked_runtime_offline": "未触发——执行对象的运行时已离线", "run_blocked_target_unavailable": "未触发——执行对象不可用", "run_blocked_attribution": "未触发——无法将本次运行归因到某个负责成员", + "run_blocked_already_active": "未触发——已有最近的运行覆盖了本次", "run_blocked_generic": "未触发——被准入策略拦截", "section_properties": "属性", "section_triggers": "触发器", diff --git a/server/internal/dispatch/reason.go b/server/internal/dispatch/reason.go new file mode 100644 index 0000000000..a2f9878058 --- /dev/null +++ b/server/internal/dispatch/reason.go @@ -0,0 +1,41 @@ +// Package dispatch holds the canonical, cross-layer vocabulary for execution +// admission outcomes (MUL-4525). It is a leaf package (no internal deps) so both +// the service layer — which MAKES the admission/skip decision and therefore owns +// the reason at its source — and the handler layer — which serializes it to the +// wire — share one enum and can never drift. +// +// A ReasonCode is decided at the branch that blocks/skips a run and carried +// through to the response verbatim; it is never reverse-engineered from a +// human-readable failure string. Codes are stable, localizable by clients, and +// enumeration-safe: a code never reveals whether a private agent exists, its +// name, or its owner. +package dispatch + +// ReasonCode is a stable, client-localizable admission/dispatch reason. +type ReasonCode string + +const ( + // ReasonQueued / ReasonCoalesced / ReasonDeferred are the success-path codes. + ReasonQueued ReasonCode = "queued" + ReasonCoalesced ReasonCode = "coalesced" + ReasonDeferred ReasonCode = "deferred" + + // ReasonInvocationNotAllowed: the acting principal may not trigger this + // target under the invocation-permission model. Deliberately generic — it + // does not distinguish "target is private" from "target does not exist". + ReasonInvocationNotAllowed ReasonCode = "invocation_not_allowed" + // ReasonTargetUnavailable: the target cannot run (archived agent, deleted / + // archived squad, unresolvable leader, or no assignee). + ReasonTargetUnavailable ReasonCode = "target_unavailable" + // ReasonRuntimeOffline: the target is permitted but its runtime is not bound + // / not online at dispatch time. + ReasonRuntimeOffline ReasonCode = "runtime_offline" + // ReasonAttributionBlocked: a fail-closed workspace could not resolve a + // responsible human for the run, so it was refused. + ReasonAttributionBlocked ReasonCode = "attribution_blocked" + // ReasonAlreadyActive: a run is already active/pending for this target and + // this trigger did not coalesce. + ReasonAlreadyActive ReasonCode = "already_active" + // ReasonInternalError: an unexpected server error prevented a clean decision. + ReasonInternalError ReasonCode = "internal_error" +) diff --git a/server/internal/handler/admission.go b/server/internal/handler/admission.go index 50298add60..b59e13dc67 100644 --- a/server/internal/handler/admission.go +++ b/server/internal/handler/admission.go @@ -1,6 +1,10 @@ package handler -import "net/http" +import ( + "net/http" + + "github.com/multica-ai/multica/server/internal/dispatch" +) // Unified execution-admission contract (MUL-4525). // @@ -37,37 +41,24 @@ const ( DispatchBlocked DispatchStatus = "blocked" ) -// DispatchReasonCode is a stable, client-localizable, enumeration-safe reason. -// New codes may be added; clients must treat an unknown code as a generic -// failure (they switch with a default branch). A code NEVER encodes the +// DispatchReasonCode is the wire-facing admission reason. It aliases the +// canonical, cross-layer enum in the dispatch package so the service (which +// decides the reason at its source) and the handler (which serializes it) can +// never drift. New codes may be added; clients must treat an unknown code as a +// generic failure (they switch with a default branch). A code NEVER encodes the // existence, name, or owner of a target the caller is not allowed to see. -type DispatchReasonCode string +type DispatchReasonCode = dispatch.ReasonCode const ( - // ReasonQueued / ReasonCoalesced / ReasonDeferred are the success-path - // codes paired with the matching DispatchStatus. - ReasonQueued DispatchReasonCode = "queued" - ReasonCoalesced DispatchReasonCode = "coalesced" - ReasonDeferred DispatchReasonCode = "deferred" - - // ReasonInvocationNotAllowed: the acting user is not permitted to trigger - // this target under the invocation-permission model. Deliberately generic: - // it does not distinguish "target is private" from "target does not exist". - ReasonInvocationNotAllowed DispatchReasonCode = "invocation_not_allowed" - // ReasonTargetUnavailable: the target cannot run (archived agent, deleted / - // archived squad, unresolvable leader, no assignee). - ReasonTargetUnavailable DispatchReasonCode = "target_unavailable" - // ReasonRuntimeOffline: the target exists and is permitted, but its runtime - // is offline at dispatch time. - ReasonRuntimeOffline DispatchReasonCode = "runtime_offline" - // ReasonAttributionBlocked: a fail-closed workspace could not resolve a - // responsible human for the run, so it was refused. - ReasonAttributionBlocked DispatchReasonCode = "attribution_blocked" - // ReasonAlreadyActive: a run is already active/pending for this target and - // this trigger did not coalesce. - ReasonAlreadyActive DispatchReasonCode = "already_active" - // ReasonInternalError: an unexpected server error prevented a decision. - ReasonInternalError DispatchReasonCode = "internal_error" + ReasonQueued = dispatch.ReasonQueued + ReasonCoalesced = dispatch.ReasonCoalesced + ReasonDeferred = dispatch.ReasonDeferred + ReasonInvocationNotAllowed = dispatch.ReasonInvocationNotAllowed + ReasonTargetUnavailable = dispatch.ReasonTargetUnavailable + ReasonRuntimeOffline = dispatch.ReasonRuntimeOffline + ReasonAttributionBlocked = dispatch.ReasonAttributionBlocked + ReasonAlreadyActive = dispatch.ReasonAlreadyActive + ReasonInternalError = dispatch.ReasonInternalError ) // DispatchTarget is the caller-visible reference to an execution target. Name diff --git a/server/internal/handler/admission_security_mul4525_test.go b/server/internal/handler/admission_security_mul4525_test.go new file mode 100644 index 0000000000..e0c24f5704 --- /dev/null +++ b/server/internal/handler/admission_security_mul4525_test.go @@ -0,0 +1,226 @@ +package handler + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/jackc/pgx/v5/pgtype" + "github.com/multica-ai/multica/server/internal/dispatch" + "github.com/multica-ai/multica/server/internal/util" + db "github.com/multica-ai/multica/server/pkg/db/generated" +) + +// seedSecurityTestOwner creates a throwaway workspace member to own an agent, so +// the agent is NOT owned by testUserID (who must be able to VIEW but not INVOKE). +func seedSecurityTestOwner(t *testing.T, label string) string { + t.Helper() + ctx := context.Background() + var ownerID string + if err := testPool.QueryRow(ctx, `INSERT INTO "user" (name, email) VALUES ($1, $2) RETURNING id`, + label, fmt.Sprintf("%s-%d@multica.test", label, time.Now().UnixNano())).Scan(&ownerID); err != nil { + t.Fatalf("seed owner user: %v", err) + } + t.Cleanup(func() { testPool.Exec(context.Background(), `DELETE FROM "user" WHERE id = $1`, ownerID) }) + if _, err := testPool.Exec(ctx, `INSERT INTO member (workspace_id, user_id, role) VALUES ($1, $2, 'member')`, + testWorkspaceID, ownerID); err != nil { + t.Fatalf("seed owner member: %v", err) + } + return ownerID +} + +// readReasonCode pulls the stable reason_code out of a structured blocked-admission +// response body (MUL-4525). +func readReasonCode(t *testing.T, body []byte) string { + t.Helper() + var resp struct { + Error string `json:"error"` + ReasonCode string `json:"reason_code"` + } + if err := json.Unmarshal(body, &resp); err != nil { + t.Fatalf("decode blocked response: %v (body=%s)", err, string(body)) + } + return resp.ReasonCode +} + +// TestSendChatMessage_InvokeRevokedAfterSessionCreate is the MUL-4525 must-fix 3 +// chat acceptance test: a session created while the user could invoke the agent +// must stop sending the instant that invoke permission is revoked — even though +// the user (a workspace owner) can still VIEW the transcript. The refusal is a +// structured 403 with a stable reason_code, and NOTHING is persisted: no chat +// message, no task. +func TestSendChatMessage_InvokeRevokedAfterSessionCreate(t *testing.T) { + if testHandler == nil || testPool == nil { + t.Skip("database not available") + } + ctx := context.Background() + ownerID := seedSecurityTestOwner(t, "chat-agent-owner") + + // public_to agent owned by someone else, with testUserID on its member + // allow-list — so testUserID may invoke it while it is public_to. + var agentID string + if err := testPool.QueryRow(ctx, ` + INSERT INTO agent (workspace_id, name, description, runtime_mode, runtime_config, + runtime_id, visibility, permission_mode, max_concurrent_tasks, owner_id, + instructions, custom_env, custom_args) + VALUES ($1, 'chat-revoke-agent', '', 'cloud', '{}'::jsonb, $2, 'private', 'public_to', 1, $3, + '', '{}'::jsonb, '[]'::jsonb) + RETURNING id`, testWorkspaceID, handlerTestRuntimeID(t), ownerID).Scan(&agentID); err != nil { + t.Fatalf("seed agent: %v", err) + } + t.Cleanup(func() { testPool.Exec(context.Background(), `DELETE FROM agent WHERE id = $1`, agentID) }) + if _, err := testPool.Exec(ctx, ` + INSERT INTO agent_invocation_target (agent_id, target_type, target_id) + VALUES ($1, 'member', $2)`, agentID, testUserID); err != nil { + t.Fatalf("seed member invocation target: %v", err) + } + + // Create the session as testUserID while they can still invoke the agent. + createW := httptest.NewRecorder() + createReq := withChatTestWorkspaceCtx(t, newRequest("POST", "/api/chat/sessions", map[string]any{ + "agent_id": agentID, + "title": "revoke test", + })) + testHandler.CreateChatSession(createW, createReq) + if createW.Code != http.StatusCreated { + t.Fatalf("CreateChatSession while invokable: expected 201, got %d: %s", createW.Code, createW.Body.String()) + } + var session struct { + ID string `json:"id"` + } + if err := json.Unmarshal(createW.Body.Bytes(), &session); err != nil { + t.Fatalf("decode session: %v", err) + } + t.Cleanup(func() { testPool.Exec(context.Background(), `DELETE FROM chat_session WHERE id = $1`, session.ID) }) + + // Revoke invoke permission: flip the agent to private. testUserID keeps VIEW + // access (workspace owner) but loses INVOKE access (not the agent owner). + if _, err := testPool.Exec(ctx, `UPDATE agent SET permission_mode = 'private' WHERE id = $1`, agentID); err != nil { + t.Fatalf("revoke invoke: %v", err) + } + + countMessages := func() int { + var n int + if err := testPool.QueryRow(ctx, `SELECT count(*) FROM chat_message WHERE chat_session_id = $1`, session.ID).Scan(&n); err != nil { + t.Fatalf("count messages: %v", err) + } + return n + } + countTasks := func() int { + var n int + if err := testPool.QueryRow(ctx, `SELECT count(*) FROM agent_task_queue WHERE chat_session_id = $1`, session.ID).Scan(&n); err != nil { + t.Fatalf("count tasks: %v", err) + } + return n + } + if countMessages() != 0 || countTasks() != 0 { + t.Fatalf("precondition: session should have no messages/tasks yet") + } + + // The send must be refused before anything is written. + sendW := httptest.NewRecorder() + sendReq := withChatTestWorkspaceCtx(t, newRequest("POST", "/api/chat/sessions/"+session.ID+"/messages", map[string]any{ + "content": "are you still there?", + })) + sendReq = withURLParam(sendReq, "sessionId", session.ID) + testHandler.SendChatMessage(sendW, sendReq) + + if sendW.Code != http.StatusForbidden { + t.Fatalf("SendChatMessage after revoke: expected 403, got %d: %s", sendW.Code, sendW.Body.String()) + } + if code := readReasonCode(t, sendW.Body.Bytes()); code != string(dispatch.ReasonInvocationNotAllowed) { + t.Errorf("reason_code = %q, want invocation_not_allowed", code) + } + if got := countMessages(); got != 0 { + t.Errorf("blocked send persisted %d chat messages, want 0", got) + } + if got := countTasks(); got != 0 { + t.Errorf("blocked send persisted %d tasks, want 0", got) + } +} + +// TestRerunIssue_PrivateHistoricalAgent is the MUL-4525 must-fix 3 rerun +// acceptance test, driven through the REAL handler + canInvokeAgent (not an +// injected callback): a user who can see the issue but cannot invoke its private +// agent is refused with a structured 403 and mutates nothing; the agent's owner +// is allowed. +func TestRerunIssue_PrivateHistoricalAgent(t *testing.T) { + if testHandler == nil || testPool == nil { + t.Skip("database not available") + } + ctx := context.Background() + agentID, ownerID, _ := privateAgentTestFixture(t) // private agent owned by ownerID + + var issueID string + if err := testPool.QueryRow(ctx, ` + INSERT INTO issue (workspace_id, title, creator_type, creator_id, assignee_type, assignee_id, priority) + VALUES ($1, 'rerun private agent', 'member', $2, 'agent', $3, 'medium') + RETURNING id`, testWorkspaceID, ownerID, agentID).Scan(&issueID); err != nil { + t.Fatalf("seed issue: %v", err) + } + t.Cleanup(func() { + testPool.Exec(context.Background(), `DELETE FROM agent_task_queue WHERE issue_id = $1`, issueID) + testPool.Exec(context.Background(), `DELETE FROM issue WHERE id = $1`, issueID) + }) + + // A historical task run by the private agent. + orig, err := testHandler.TaskService.EnqueueTaskForIssue(ctx, db.Issue{ + ID: util.MustParseUUID(issueID), + AssigneeID: util.MustParseUUID(agentID), + Priority: "medium", + CreatorType: "member", + CreatorID: util.MustParseUUID(ownerID), + WorkspaceID: util.MustParseUUID(testWorkspaceID), + AssigneeType: pgtype.Text{String: "agent", Valid: true}, + }) + if err != nil { + t.Fatalf("enqueue original task: %v", err) + } + origID := util.UUIDToString(orig.ID) + + taskCount := func() int { + var n int + if err := testPool.QueryRow(ctx, `SELECT count(*) FROM agent_task_queue WHERE issue_id = $1`, issueID).Scan(&n); err != nil { + t.Fatalf("count tasks: %v", err) + } + return n + } + origStatus := func() string { + var s string + if err := testPool.QueryRow(ctx, `SELECT status FROM agent_task_queue WHERE id = $1`, origID).Scan(&s); err != nil { + t.Fatalf("read orig status: %v", err) + } + return s + } + beforeCount, beforeStatus := taskCount(), origStatus() + + // DENY: testUserID (workspace owner) can view the issue but cannot invoke the + // private agent → structured 403, and nothing is cancelled or created. + denyW := httptest.NewRecorder() + denyReq := withURLParam(newRequest("POST", "/api/issues/"+issueID+"/rerun", map[string]any{"task_id": origID}), "id", issueID) + testHandler.RerunIssue(denyW, denyReq) + if denyW.Code != http.StatusForbidden { + t.Fatalf("RerunIssue as non-invoker: expected 403, got %d: %s", denyW.Code, denyW.Body.String()) + } + if code := readReasonCode(t, denyW.Body.Bytes()); code != string(dispatch.ReasonInvocationNotAllowed) { + t.Errorf("reason_code = %q, want invocation_not_allowed", code) + } + if got := taskCount(); got != beforeCount { + t.Errorf("blocked rerun changed task count: got %d, want %d", got, beforeCount) + } + if got := origStatus(); got != beforeStatus { + t.Errorf("blocked rerun changed original task status: got %q, want %q", got, beforeStatus) + } + + // ALLOW: the agent owner may rerun their own private agent. + allowW := httptest.NewRecorder() + allowReq := withURLParam(newRequestAs(ownerID, "POST", "/api/issues/"+issueID+"/rerun", map[string]any{"task_id": origID}), "id", issueID) + testHandler.RerunIssue(allowW, allowReq) + if allowW.Code != http.StatusAccepted { + t.Fatalf("RerunIssue as agent owner: expected 202, got %d: %s", allowW.Code, allowW.Body.String()) + } +} diff --git a/server/internal/handler/admission_test.go b/server/internal/handler/admission_test.go index 58f01781e9..ccb7b8370e 100644 --- a/server/internal/handler/admission_test.go +++ b/server/internal/handler/admission_test.go @@ -1,50 +1,31 @@ package handler -import "testing" +import ( + "testing" -// TestAutopilotRunReasonCode pins the mapping from a run's status/failure_reason -// to the stable, enumeration-safe DispatchReasonCode the "run now" UI localizes -// (MUL-4525). The raw English reason must never reach the wire; every skip/fail -// phrasing the dispatch layer produces must classify to a known code, and an -// unrecognized reason must fall back to internal_error rather than leak text. -func TestAutopilotRunReasonCode(t *testing.T) { - ptr := func(s string) *string { return &s } - cases := []struct { - name string - status string - reason *string - wantNil bool - want DispatchReasonCode - }{ - {"success has no code", "completed", nil, true, ""}, - {"issue_created has no code", "issue_created", nil, true, ""}, - {"running has no code", "running", nil, true, ""}, - {"creator lacks access", "skipped", ptr("autopilot creator lacks access to private assignee agent"), false, ReasonInvocationNotAllowed}, - {"clicker not allowed to trigger", "skipped", ptr("you are not allowed to trigger this autopilot's assignee agent"), false, ReasonInvocationNotAllowed}, - {"squad leader not allowed to invoke", "failed", ptr("not allowed to invoke private squad leader"), false, ReasonInvocationNotAllowed}, - {"runtime offline", "skipped", ptr("assignee agent runtime is offline at dispatch time"), false, ReasonRuntimeOffline}, - {"no assignee", "skipped", ptr("autopilot has no assignee"), false, ReasonTargetUnavailable}, - {"squad archived", "skipped", ptr("assignee squad is archived"), false, ReasonTargetUnavailable}, - {"agent gone", "skipped", ptr("assignee agent no longer exists"), false, ReasonTargetUnavailable}, - {"unknown reason falls back", "failed", ptr("dispatch create_issue: boom"), false, ReasonInternalError}, - {"failed with nil reason", "failed", nil, false, ReasonInternalError}, + "github.com/jackc/pgx/v5/pgtype" + db "github.com/multica-ai/multica/server/pkg/db/generated" +) + +// TestRunToResponseDoesNotReverseEngineerReasonCode pins Elon must-fix 2: the +// serializer must NOT derive reason_code from a persisted run's English +// failure_reason. The typed code is a decision-time value the manual "run now" +// handler injects from the dispatch outcome; a run read back from the DB (list / +// history) carries the human failure_reason but no guessed code. +func TestRunToResponseDoesNotReverseEngineerReasonCode(t *testing.T) { + run := db.AutopilotRun{ + Status: "skipped", + FailureReason: pgtype.Text{ + String: "assignee agent lacks access to private assignee agent", + Valid: true, + }, } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - got := autopilotRunReasonCode(tc.status, tc.reason) - if tc.wantNil { - if got != nil { - t.Fatalf("want nil, got %q", *got) - } - return - } - if got == nil { - t.Fatalf("want %q, got nil", tc.want) - } - if *got != string(tc.want) { - t.Errorf("got %q, want %q", *got, tc.want) - } - }) + resp := runToResponse(run) + if resp.ReasonCode != nil { + t.Fatalf("runToResponse should not synthesize a reason_code from failure_reason, got %q", *resp.ReasonCode) + } + if resp.FailureReason == nil || *resp.FailureReason == "" { + t.Errorf("failure_reason should still be surfaced for history rows") } } diff --git a/server/internal/handler/autopilot.go b/server/internal/handler/autopilot.go index 39aed0528d..a157bcfa04 100644 --- a/server/internal/handler/autopilot.go +++ b/server/internal/handler/autopilot.go @@ -275,61 +275,26 @@ func runToResponse(r db.AutopilotRun) AutopilotRunResponse { json.Unmarshal(r.Result, &result) } return AutopilotRunResponse{ - ID: uuidToString(r.ID), - AutopilotID: uuidToString(r.AutopilotID), - TriggerID: uuidToPtr(r.TriggerID), - Source: r.Source, - Status: r.Status, - IssueID: uuidToPtr(r.IssueID), - TaskID: uuidToPtr(r.TaskID), - TriggeredAt: timestampToString(r.TriggeredAt), - CompletedAt: timestampToPtr(r.CompletedAt), - FailureReason: textToPtr(r.FailureReason), - ReasonCode: autopilotRunReasonCode(r.Status, textToPtr(r.FailureReason)), + ID: uuidToString(r.ID), + AutopilotID: uuidToString(r.AutopilotID), + TriggerID: uuidToPtr(r.TriggerID), + Source: r.Source, + Status: r.Status, + IssueID: uuidToPtr(r.IssueID), + TaskID: uuidToPtr(r.TaskID), + TriggeredAt: timestampToString(r.TriggeredAt), + CompletedAt: timestampToPtr(r.CompletedAt), + FailureReason: textToPtr(r.FailureReason), + // ReasonCode is left unset here: it is a decision-time value the manual + // "run now" handler injects from the typed dispatch outcome (MUL-4525). + // Persisted rows (list/history) surface the human failure_reason instead + // of a code reverse-engineered from that text. TriggerPayload: payload, Result: result, CreatedAt: timestampToString(r.CreatedAt), } } -// autopilotRunReasonCode maps a non-success autopilot run to a stable, wire-safe -// DispatchReasonCode the UI can localize (MUL-4525), so the "run now" affordance -// can warn without echoing the raw English failure_reason — which may name a -// private assignee agent and is not enumeration-safe. Returns nil for -// success-path runs. Classification is substring-based over the fixed set of -// reasons shouldSkipDispatch / formatAdmissionReason / failRun produce; an -// unmatched reason falls back to internal_error rather than leaking the text. -func autopilotRunReasonCode(status string, failureReason *string) *string { - if status != "skipped" && status != "failed" { - return nil - } - reason := "" - if failureReason != nil { - reason = strings.ToLower(*failureReason) - } - var code DispatchReasonCode - switch { - case strings.Contains(reason, "not allowed to trigger"), - strings.Contains(reason, "not allowed to invoke"), - strings.Contains(reason, "lacks access"), - strings.Contains(reason, "cannot access private"): - code = ReasonInvocationNotAllowed - case strings.Contains(reason, "runtime is "): - // AgentReadiness phrasings: "... runtime is offline/starting/... at - // dispatch time" — the target exists and is permitted but not ready. - code = ReasonRuntimeOffline - case strings.Contains(reason, "no assignee"), - strings.Contains(reason, "archived"), - strings.Contains(reason, "no longer exists"), - strings.Contains(reason, "cannot be resolved"): - code = ReasonTargetUnavailable - default: - code = ReasonInternalError - } - s := string(code) - return &s -} - // runToResponseSlim mirrors runToResponse but omits TriggerPayload, intended // for list endpoints where echoing the full webhook envelope (up to // 256 KiB × N rows) would dominate response size. Clients fetch the full @@ -2053,11 +2018,19 @@ func (h *Handler) TriggerAutopilot(w http.ResponseWriter, r *http.Request) { } actorType, actorID := h.resolveActor(r, userID, workspaceID) - run, err := h.AutopilotService.DispatchAutopilotManual(r.Context(), autopilot, pgtype.UUID{}, nil, memberActorUserID(actorType, actorID)) + run, reasonCode, err := h.AutopilotService.DispatchAutopilotManual(r.Context(), autopilot, pgtype.UUID{}, nil, memberActorUserID(actorType, actorID)) if err != nil { writeError(w, http.StatusInternalServerError, "failed to trigger autopilot: "+err.Error()) return } - writeJSON(w, http.StatusOK, runToResponse(*run)) + // Carry the typed admission reason (decided at its source, MUL-4525) straight + // into the response — no reverse-engineering from failure_reason text. The + // UI branches on run status + this code for the "run now" toast. + resp := runToResponse(*run) + if reasonCode != "" { + c := string(reasonCode) + resp.ReasonCode = &c + } + writeJSON(w, http.StatusOK, resp) } diff --git a/server/internal/service/admission_mul4525_test.go b/server/internal/service/admission_mul4525_test.go index 039316c5ac..ddc1abbe6b 100644 --- a/server/internal/service/admission_mul4525_test.go +++ b/server/internal/service/admission_mul4525_test.go @@ -9,6 +9,7 @@ import ( "time" "github.com/jackc/pgx/v5/pgtype" + "github.com/multica-ai/multica/server/internal/dispatch" "github.com/multica-ai/multica/server/internal/events" "github.com/multica-ai/multica/server/internal/util" db "github.com/multica-ai/multica/server/pkg/db/generated" @@ -127,17 +128,21 @@ func TestAutopilotDispatchAdmitsClickerNotCreator(t *testing.T) { svc := &AutopilotService{Queries: q} // Manual dispatch by the agent owner (the clicker) is admitted. - if reason, skip := svc.shouldSkipDispatch(ctx, ap, util.MustParseUUID(ownerID)); skip { + if reason, _, skip := svc.shouldSkipDispatch(ctx, ap, util.MustParseUUID(ownerID)); skip { t.Fatalf("manual dispatch by the agent owner should be admitted, got skip: %q", reason) } // Automation (no human actor) falls back to the creator gate, which denies - // the admin-but-non-owner creator on a private agent. - reason, skip := svc.shouldSkipDispatch(ctx, ap, pgtype.UUID{}) + // the admin-but-non-owner creator on a private agent — and the typed reason + // code is decided at that branch, not guessed from text. + reason, code, skip := svc.shouldSkipDispatch(ctx, ap, pgtype.UUID{}) if !skip { t.Fatalf("automation dispatch should be blocked by the creator gate") } if !strings.Contains(strings.ToLower(reason), "creator") { t.Errorf("automation skip reason = %q, want creator-gate phrasing", reason) } + if code != dispatch.ReasonInvocationNotAllowed { + t.Errorf("skip reason_code = %q, want invocation_not_allowed", code) + } } diff --git a/server/internal/service/autopilot.go b/server/internal/service/autopilot.go index bc9623f0e3..eebb2facb6 100644 --- a/server/internal/service/autopilot.go +++ b/server/internal/service/autopilot.go @@ -14,6 +14,7 @@ import ( "github.com/jackc/pgx/v5/pgtype" "github.com/multica-ai/multica/server/internal/analytics" "github.com/multica-ai/multica/server/internal/attribution" + "github.com/multica-ai/multica/server/internal/dispatch" "github.com/multica-ai/multica/server/internal/events" "github.com/multica-ai/multica/server/internal/issueguard" "github.com/multica-ai/multica/server/internal/issueposition" @@ -117,8 +118,10 @@ func (s *AutopilotService) DispatchAutopilot( payload []byte, ) (*db.AutopilotRun, error) { // No member actor on this entry point (schedule / webhook / api, or a manual - // trigger without a resolved member): attribution resolves rule_owner. - return s.dispatchAutopilot(ctx, autopilot, triggerID, source, payload, pgtype.Timestamptz{}, pgtype.UUID{}) + // trigger without a resolved member): attribution resolves rule_owner. These + // callers don't surface a per-run reason code to a human, so it is dropped. + run, _, err := s.dispatchAutopilot(ctx, autopilot, triggerID, source, payload, pgtype.Timestamptz{}, pgtype.UUID{}) + return run, err } // DispatchAutopilotManual is the "run now" entry point for a member manually @@ -133,7 +136,9 @@ func (s *AutopilotService) DispatchAutopilotManual( triggerID pgtype.UUID, payload []byte, actorUserID pgtype.UUID, -) (*db.AutopilotRun, error) { +) (*db.AutopilotRun, dispatch.ReasonCode, error) { + // The manual path is the one surface that shows a per-run outcome to a + // human, so it returns the typed reason code decided at the admission source. return s.dispatchAutopilot(ctx, autopilot, triggerID, "manual", payload, pgtype.Timestamptz{}, actorUserID) } @@ -218,8 +223,10 @@ func (s *AutopilotService) DispatchAutopilotForPlan( return nil, fmt.Errorf("dispatch for plan: lookup existing run: %w", err) } - // Scheduled dispatch has no member actor → rule_owner attribution. - return s.dispatchAutopilot(ctx, autopilot, triggerID, source, payload, plannedTS, pgtype.UUID{}) + // Scheduled dispatch has no member actor → rule_owner attribution, and no + // human surface for a per-run reason code, so it is dropped. + run, _, err := s.dispatchAutopilot(ctx, autopilot, triggerID, source, payload, plannedTS, pgtype.UUID{}) + return run, err } // isAutopilotRunComplete decides whether an existing autopilot_run row @@ -269,9 +276,10 @@ func (s *AutopilotService) dispatchAutopilot( payload []byte, plannedAt pgtype.Timestamptz, actorUserID pgtype.UUID, -) (*db.AutopilotRun, error) { - if reason, skip := s.shouldSkipDispatch(ctx, autopilot, actorUserID); skip { - return s.recordSkippedRun(ctx, autopilot, triggerID, source, payload, plannedAt, reason) +) (*db.AutopilotRun, dispatch.ReasonCode, error) { + if reason, code, skip := s.shouldSkipDispatch(ctx, autopilot, actorUserID); skip { + run, err := s.recordSkippedRun(ctx, autopilot, triggerID, source, payload, plannedAt, reason) + return run, code, err } // Determine initial status based on execution mode. @@ -290,7 +298,7 @@ func (s *AutopilotService) dispatchAutopilot( PlannedAt: plannedAt, }) if err != nil { - return nil, fmt.Errorf("create run: %w", err) + return nil, dispatch.ReasonInternalError, fmt.Errorf("create run: %w", err) } s.captureAutopilotRunStarted(autopilot, run, source) @@ -298,26 +306,26 @@ func (s *AutopilotService) dispatchAutopilot( case "create_issue": triggerTimezone := s.resolveAutopilotTriggerTimezone(ctx, triggerID) if err := s.dispatchCreateIssue(ctx, autopilot, &run, triggerTimezone, actorUserID); err != nil { - if skipped := s.handleDispatchSkip(ctx, autopilot, &run, err); skipped != nil { - return skipped, nil + if skipped, code := s.handleDispatchSkip(ctx, autopilot, &run, err); skipped != nil { + return skipped, code, nil } s.failRun(ctx, run.ID, err.Error()) s.captureAutopilotRunFailed(autopilot, run, source, err.Error()) - return &run, fmt.Errorf("dispatch create_issue: %w", err) + return &run, dispatchFailReasonCode(err), fmt.Errorf("dispatch create_issue: %w", err) } case "run_only": if err := s.dispatchRunOnly(ctx, autopilot, &run, actorUserID); err != nil { - if skipped := s.handleDispatchSkip(ctx, autopilot, &run, err); skipped != nil { - return skipped, nil + if skipped, code := s.handleDispatchSkip(ctx, autopilot, &run, err); skipped != nil { + return skipped, code, nil } s.failRun(ctx, run.ID, err.Error()) s.captureAutopilotRunFailed(autopilot, run, source, err.Error()) - return &run, fmt.Errorf("dispatch run_only: %w", err) + return &run, dispatchFailReasonCode(err), fmt.Errorf("dispatch run_only: %w", err) } default: s.failRun(ctx, run.ID, "unknown execution_mode: "+autopilot.ExecutionMode) s.captureAutopilotRunFailed(autopilot, run, source, "unknown execution_mode: "+autopilot.ExecutionMode) - return &run, fmt.Errorf("unknown execution_mode: %s", autopilot.ExecutionMode) + return &run, dispatch.ReasonInternalError, fmt.Errorf("unknown execution_mode: %s", autopilot.ExecutionMode) } // Update last_run_at on the autopilot. @@ -336,7 +344,18 @@ func (s *AutopilotService) dispatchAutopilot( }, }) - return &run, nil + return &run, "", nil +} + +// dispatchFailReasonCode types a dispatch error that fell through to failRun. +// It inspects the error with typed checks (never substring matching): a +// fail-closed attribution refusal is attribution_blocked; everything else is an +// unclassified internal error. +func dispatchFailReasonCode(err error) dispatch.ReasonCode { + if errors.Is(err, ErrAttributionFailClosed) { + return dispatch.ReasonAttributionBlocked + } + return dispatch.ReasonInternalError } // dispatchCreateIssue creates an issue and enqueues a task for the agent. @@ -372,7 +391,7 @@ func (s *AutopilotService) dispatchCreateIssue(ctx context.Context, ap db.Autopi ); err != nil { return fmt.Errorf("recent duplicate guard: %w", err) } else if found { - return &errDispatchSkipped{reason: "recent duplicate autopilot issue: " + util.UUIDToString(duplicate.ID)} + return &errDispatchSkipped{reason: "recent duplicate autopilot issue: " + util.UUIDToString(duplicate.ID), code: dispatch.ReasonAlreadyActive} } issueNumber, err := qtx.IncrementIssueCounter(ctx, ap.WorkspaceID) @@ -610,6 +629,10 @@ func (s *AutopilotService) notifyAutopilotSubscribersOnCreate( // monitor would auto-pause autopilots whose only crime was a flaky runtime). type errDispatchSkipped struct { reason string + // code is the stable, typed admission reason decided at THIS branch and + // carried through to the response (MUL-4525) — never reverse-engineered from + // the human-readable reason string above. + code dispatch.ReasonCode } func (e *errDispatchSkipped) Error() string { return e.reason } @@ -629,7 +652,7 @@ func (s *AutopilotService) dispatchRunOnly(ctx context.Context, ap db.Autopilot, // if the row disappeared or the squad was archived between // admission and dispatch, that is a skip, not a failure. if errors.Is(err, pgx.ErrNoRows) || errors.Is(err, errSquadArchived) { - return &errDispatchSkipped{reason: formatAdmissionReason(ap, "assignee no longer resolvable")} + return &errDispatchSkipped{reason: formatAdmissionReason(ap, "assignee no longer resolvable"), code: dispatch.ReasonTargetUnavailable} } return fmt.Errorf("resolve leader: %w", err) } @@ -638,13 +661,13 @@ func (s *AutopilotService) dispatchRunOnly(ctx context.Context, ap db.Autopilot, return fmt.Errorf("check agent readiness: %w", err) } if !ready { - return &errDispatchSkipped{reason: formatAdmissionReason(ap, reason)} + return &errDispatchSkipped{reason: formatAdmissionReason(ap, reason), code: agentReadinessReasonCode(agent)} } // Fail-closed invocation gate for squad autopilots (admission principal = // manual clicker, else creator — see autopilotAdmitInvoke). if ap.AssigneeType == "squad" && !s.autopilotAdmitInvoke(ctx, ap, agent, actorUserID) { - return &errDispatchSkipped{reason: formatAdmissionReason(ap, "not allowed to invoke private squad leader")} + return &errDispatchSkipped{reason: formatAdmissionReason(ap, "not allowed to invoke private squad leader"), code: dispatch.ReasonInvocationNotAllowed} } // Attribution splits on the trigger. A MANUAL trigger is a direct human action: @@ -668,7 +691,7 @@ func (s *AutopilotService) dispatchRunOnly(ctx context.Context, ap db.Autopilot, // workspace is fail-closed (MUL-4302 §3.5). autopilotAttr, err = s.TaskSvc.applyAttributionFallback(ctx, autopilotAttr, agent) if err != nil { - return &errDispatchSkipped{reason: formatAdmissionReason(ap, "workspace fail-closed: no accountable human for autopilot run")} + return &errDispatchSkipped{reason: formatAdmissionReason(ap, "workspace fail-closed: no accountable human for autopilot run"), code: dispatch.ReasonAttributionBlocked} } apSource, _, apEvidenceKind, apEvidenceRef := attributionCreateParams(autopilotAttr) task, err := s.Queries.CreateAutopilotTask(ctx, db.CreateAutopilotTaskParams{ @@ -892,10 +915,10 @@ func taskFailureReasonForAutopilotRun(task db.AgentTaskQueue) string { // DispatchAutopilot up the stack and the failure-vs-skip distinction is // owned by the dispatcher entry point. Keeps dispatchRunOnly free of // state-mutation helpers. -func (s *AutopilotService) handleDispatchSkip(ctx context.Context, ap db.Autopilot, run *db.AutopilotRun, err error) *db.AutopilotRun { +func (s *AutopilotService) handleDispatchSkip(ctx context.Context, ap db.Autopilot, run *db.AutopilotRun, err error) (*db.AutopilotRun, dispatch.ReasonCode) { var skipErr *errDispatchSkipped if !errors.As(err, &skipErr) { - return nil + return nil, "" } updated, uerr := s.Queries.UpdateAutopilotRunSkipped(ctx, db.UpdateAutopilotRunSkippedParams{ ID: run.ID, @@ -907,7 +930,7 @@ func (s *AutopilotService) handleDispatchSkip(ctx context.Context, ap db.Autopil // Leave the run in its current (running/issue_created) state if // the update failed; the failure monitor will eventually fail it // out, but at least we didn't pretend it succeeded. - return nil + return nil, "" } *run = updated slog.Info("autopilot dispatch skipped post-admission", @@ -921,7 +944,7 @@ func (s *AutopilotService) handleDispatchSkip(ctx context.Context, ap db.Autopil // caught a late readiness regression. s.Queries.UpdateAutopilotLastRunAt(ctx, ap.ID) s.publishRunDone(util.UUIDToString(ap.WorkspaceID), updated, "skipped") - return run + return run, skipErr.code } func (s *AutopilotService) failRun(ctx context.Context, runID pgtype.UUID, reason string) { @@ -948,9 +971,9 @@ func (s *AutopilotService) failRun(ctx context.Context, runID pgtype.UUID, reaso // scheduled run. Migration 096 removed the agent FK on autopilot, so an // agent assignee being missing is now a real condition the gate must // handle (previously cascade-deleted). -func (s *AutopilotService) shouldSkipDispatch(ctx context.Context, ap db.Autopilot, actorUserID pgtype.UUID) (string, bool) { +func (s *AutopilotService) shouldSkipDispatch(ctx context.Context, ap db.Autopilot, actorUserID pgtype.UUID) (string, dispatch.ReasonCode, bool) { if !ap.AssigneeID.Valid { - return "autopilot has no assignee", true + return "autopilot has no assignee", dispatch.ReasonTargetUnavailable, true } agent, squadResolved, err := s.resolveAutopilotLeader(ctx, ap) if err != nil { @@ -973,18 +996,18 @@ func (s *AutopilotService) shouldSkipDispatch(ctx context.Context, ap db.Autopil // should have rewritten this autopilot's assignee to the leader // already; surfacing the case explicitly keeps the failure // reason useful when something slipped past the transfer. - return "assignee squad is archived", true + return "assignee squad is archived", dispatch.ReasonTargetUnavailable, true case missing && squadResolved: - return "assignee squad cannot be resolved", true + return "assignee squad cannot be resolved", dispatch.ReasonTargetUnavailable, true case missing && !squadResolved: // Agent row gone. With migration 096 the FK is gone too, so // this is the new "agent was hard-deleted under us" case. Skip // rather than fail-open: we know retrying will not help. - return "assignee agent no longer exists", true + return "assignee agent no longer exists", dispatch.ReasonTargetUnavailable, true } // Transient DB error — fail-open so the next scheduler tick gets a // chance to succeed. - return "", false + return "", "", false } ready, reason, err := AgentReadiness(ctx, s.Queries, agent) if err != nil { @@ -993,7 +1016,7 @@ func (s *AutopilotService) shouldSkipDispatch(ctx context.Context, ap db.Autopil "runtime_id", util.UUIDToString(agent.RuntimeID), "error", err, ) - return "", false + return "", "", false } if !ready { if ap.ExecutionMode == "create_issue" && strings.HasPrefix(reason, "agent runtime is ") { @@ -1003,7 +1026,7 @@ func (s *AutopilotService) shouldSkipDispatch(ctx context.Context, ap db.Autopil "reason", reason, ) } else { - return formatAdmissionReason(ap, reason), true + return formatAdmissionReason(ap, reason), agentReadinessReasonCode(agent), true } } // Invocation gate at the autopilot layer (MUL-3963 / MUL-4525). The @@ -1017,11 +1040,22 @@ func (s *AutopilotService) shouldSkipDispatch(ctx context.Context, ap db.Autopil // autopilots the gate runs against the resolved leader. if !s.autopilotAdmitInvoke(ctx, ap, agent, actorUserID) { if actorUserID.Valid { - return "you are not allowed to trigger this autopilot's assignee agent", true + return "you are not allowed to trigger this autopilot's assignee agent", dispatch.ReasonInvocationNotAllowed, true } - return "autopilot creator lacks access to private assignee agent", true + return "autopilot creator lacks access to private assignee agent", dispatch.ReasonInvocationNotAllowed, true } - return "", false + return "", "", false +} + +// agentReadinessReasonCode types the reason an AgentReadiness check failed from +// the agent's own state rather than the human-readable reason string (MUL-4525). +// An archived agent cannot run at all; anything else (no runtime bound, or a +// bound runtime that is not online) is a runtime-availability problem. +func agentReadinessReasonCode(agent db.Agent) dispatch.ReasonCode { + if agent.ArchivedAt.Valid { + return dispatch.ReasonTargetUnavailable + } + return dispatch.ReasonRuntimeOffline } // formatAdmissionReason rewrites the generic AgentReadiness reason into the diff --git a/server/internal/service/reason_code_test.go b/server/internal/service/reason_code_test.go new file mode 100644 index 0000000000..05b0c3afdf --- /dev/null +++ b/server/internal/service/reason_code_test.go @@ -0,0 +1,53 @@ +package service + +import ( + "errors" + "fmt" + "testing" + + "github.com/jackc/pgx/v5/pgtype" + "github.com/multica-ai/multica/server/internal/dispatch" + db "github.com/multica-ai/multica/server/pkg/db/generated" +) + +// TestDispatchFailReasonCode is the regression for Elon must-fix 2, case 1: a +// dispatch that fails fail-closed on attribution must be classified +// attribution_blocked, not internal_error — decided by a TYPED errors.Is check, +// not by substring-matching an English message. +func TestDispatchFailReasonCode(t *testing.T) { + if got := dispatchFailReasonCode(ErrAttributionFailClosed); got != dispatch.ReasonAttributionBlocked { + t.Errorf("bare fail-closed: got %q, want attribution_blocked", got) + } + // The real dispatch path wraps the sentinel (create_issue enqueue → %w); + // errors.Is must still see through the wrap. + wrapped := fmt.Errorf("dispatch create_issue: enqueue task for issue: %w", ErrAttributionFailClosed) + if got := dispatchFailReasonCode(wrapped); got != dispatch.ReasonAttributionBlocked { + t.Errorf("wrapped fail-closed: got %q, want attribution_blocked", got) + } + if got := dispatchFailReasonCode(errors.New("some other failure")); got != dispatch.ReasonInternalError { + t.Errorf("generic error: got %q, want internal_error", got) + } +} + +// TestAgentReadinessReasonCode is the regression for Elon must-fix 2, case 2: a +// runtime-availability failure (no runtime bound, or a bound-but-offline +// runtime) must map to runtime_offline, and an archived agent to +// target_unavailable — decided from the agent's own state, not the reason text. +func TestAgentReadinessReasonCode(t *testing.T) { + validRuntime := pgtype.UUID{Bytes: [16]byte{1}, Valid: true} + archivedAt := pgtype.Timestamptz{Valid: true} + + // No runtime bound (the "agent has no runtime bound" case that previously + // fell through the substring classifier to generic). + if got := agentReadinessReasonCode(db.Agent{}); got != dispatch.ReasonRuntimeOffline { + t.Errorf("no runtime bound: got %q, want runtime_offline", got) + } + // Bound runtime that is offline at dispatch time. + if got := agentReadinessReasonCode(db.Agent{RuntimeID: validRuntime}); got != dispatch.ReasonRuntimeOffline { + t.Errorf("bound offline runtime: got %q, want runtime_offline", got) + } + // Archived agent cannot run at all. + if got := agentReadinessReasonCode(db.Agent{ArchivedAt: archivedAt, RuntimeID: validRuntime}); got != dispatch.ReasonTargetUnavailable { + t.Errorf("archived agent: got %q, want target_unavailable", got) + } +}