From 23258922e6cc2aad175e959b120a4e354093fefe Mon Sep 17 00:00:00 2001 From: Dotta Date: Sun, 16 Aug 2026 19:02:19 +0000 Subject: [PATCH] fix(feedback): harden recovery authorization and guidance Redact raw recovery evidence, revoke terminal run tokens, and return actionable feedback when delivery cannot retry through a paused assignee. Co-Authored-By: Paperclip --- doc/SPEC-implementation.md | 7 + packages/shared/src/index.ts | 1 + packages/shared/src/types/index.ts | 1 + packages/shared/src/types/issue.ts | 14 ++ .../__tests__/agent-auth-middleware.test.ts | 75 +++++++- ...sue-feedback-delivery-retry-routes.test.ts | 168 ++++++++++++++++++ .../__tests__/issue-recovery-actions.test.ts | 99 ++++++++++- server/src/middleware/auth.ts | 70 +++++--- server/src/routes/issues.ts | 32 ++++ server/src/services/heartbeat.ts | 1 - server/src/services/issue-recovery-actions.ts | 17 +- .../components/FeedbackDeliveryState.test.tsx | 25 +++ ui/src/components/FeedbackDeliveryState.tsx | 26 ++- ui/src/components/IssueChatThread.tsx | 12 +- ui/src/components/TaskChatThread.tsx | 2 + .../components/task-chat/TaskChatBubble.tsx | 4 + .../task-chat/TaskChatThreadView.tsx | 5 + ui/src/pages/IssueDetail.tsx | 12 ++ 18 files changed, 534 insertions(+), 37 deletions(-) create mode 100644 server/src/__tests__/issue-feedback-delivery-retry-routes.test.ts diff --git a/doc/SPEC-implementation.md b/doc/SPEC-implementation.md index 5fd25612b51..7a748c6332d 100644 --- a/doc/SPEC-implementation.md +++ b/doc/SPEC-implementation.md @@ -599,6 +599,13 @@ Every issue PATCH emits an `issue.updated` activity receipt containing the actor, responsible user, run, authorization reason, and field-level before/after changes so both agent and board edits are visible in the issue activity stream. +Local-agent run JWTs are valid only while their exact persisted heartbeat run is +`running` and has no completion timestamp. The server revalidates that lifecycle +state on every authenticated request. Moving the run to any terminal status +revokes the run JWT immediately; there is no post-terminal grace window for +agent API reads or writes. Server-owned finalization and recovery bookkeeping +uses internal authority rather than the terminated run's credential. + ## 9.4 Permission Terminology and Default Visibility Rule Paperclip V1 keeps a company-scoped visibility model as the default because centralized authorization and scoped work-object controls are not yet a core V1 control surface. diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index 6d8bdae6760..c94ec05a8fa 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -1011,6 +1011,7 @@ export type { IssueExecutionDecision, IssueComment, IssueCommentFeedbackDelivery, + IssueFeedbackDeliveryRetryNoInvokableAssigneeReason, IssueFeedbackDeliveryRetryResponse, IssueCommentDerivedAuthorSource, IssueCommentMetadata, diff --git a/packages/shared/src/types/index.ts b/packages/shared/src/types/index.ts index 77149f2e85b..8e53b8232f3 100644 --- a/packages/shared/src/types/index.ts +++ b/packages/shared/src/types/index.ts @@ -650,6 +650,7 @@ export type { IssueExecutionDecision, IssueComment, IssueCommentFeedbackDelivery, + IssueFeedbackDeliveryRetryNoInvokableAssigneeReason, IssueFeedbackDeliveryRetryResponse, IssueCommentDerivedAuthorSource, IssueCommentMetadata, diff --git a/packages/shared/src/types/issue.ts b/packages/shared/src/types/issue.ts index f07542687d8..e263822ce79 100644 --- a/packages/shared/src/types/issue.ts +++ b/packages/shared/src/types/issue.ts @@ -986,8 +986,22 @@ export interface IssueCommentFeedbackDelivery { batchedCommentCount: number; } +export type IssueFeedbackDeliveryRetryNoInvokableAssigneeReason = + | "missing" + | "paused" + | "terminated" + | "pending_approval" + | "unknown_status" + | "manager_missing" + | "manager_company_mismatch" + | "manager_terminated" + | "reporting_cycle" + | "reporting_chain_too_deep"; + export interface IssueFeedbackDeliveryRetryResponse { outcome: "queued" | "already_queued" | "not_exhausted" | "no_invokable_assignee"; + /** Present only when `outcome` is `no_invokable_assignee`. */ + reason: IssueFeedbackDeliveryRetryNoInvokableAssigneeReason | null; message: string; feedbackDelivery: IssueCommentFeedbackDelivery | null; } diff --git a/server/src/__tests__/agent-auth-middleware.test.ts b/server/src/__tests__/agent-auth-middleware.test.ts index ff27e685b77..3394779109f 100644 --- a/server/src/__tests__/agent-auth-middleware.test.ts +++ b/server/src/__tests__/agent-auth-middleware.test.ts @@ -33,7 +33,14 @@ function createSelectChain(rowsForTable: (table: unknown) => unknown[]) { function createDbState(input: { agent: { id: string; companyId: string; status?: string }; agentKey?: { id: string; agentId: string; companyId: string; keyHash: string; responsibleUserId?: string | null }; - run?: { id: string; companyId: string; agentId: string; responsibleUserId?: string | null }; + run?: { + id: string; + companyId: string; + agentId: string; + status?: string; + finishedAt?: Date | null; + responsibleUserId?: string | null; + }; }) { const activity: Array> = []; const agentRow = { @@ -57,6 +64,8 @@ function createDbState(input: { id: input.run.id, companyId: input.run.companyId, agentId: input.run.agentId, + status: input.run.status ?? "running", + finishedAt: input.run.finishedAt ?? null, responsibleUserId: input.run.responsibleUserId ?? null, } : null; @@ -286,6 +295,70 @@ describe("agent auth middleware", () => { }); }); + it.each(["succeeded", "failed", "cancelled", "timed_out", "interrupted"])( + "revokes the run JWT before reads or writes when the run is %s", + async (status) => { + const companyId = randomUUID(); + const agentId = randomUUID(); + const runId = randomUUID(); + const issueId = randomUUID(); + const { db } = createDbState({ + agent: { id: agentId, companyId }, + run: { + id: runId, + companyId, + agentId, + status, + finishedAt: new Date(), + responsibleUserId: "user-claim", + }, + }); + const token = createLocalAgentJwt(agentId, companyId, "codex_local", runId, "user-claim"); + + const app = createApp(db); + const readRes = await request(app) + .get(`/companies/${companyId}/issues/${issueId}`) + .set("Authorization", `Bearer ${token}`) + .set("X-Paperclip-Run-Id", runId); + const writeRes = await request(app) + .patch(`/companies/${companyId}/issues/${issueId}`) + .set("Authorization", `Bearer ${token}`) + .set("X-Paperclip-Run-Id", runId) + .send({ title: "should not write" }); + + expect(readRes.status).toBe(401); + expect(readRes.body.error).toBe("Agent run is not active"); + expect(writeRes.status).toBe(401); + expect(writeRes.body.error).toBe("Agent run is not active"); + }, + ); + + it("fails closed when a running run already has a completion timestamp", async () => { + const companyId = randomUUID(); + const agentId = randomUUID(); + const runId = randomUUID(); + const { db } = createDbState({ + agent: { id: agentId, companyId }, + run: { + id: runId, + companyId, + agentId, + status: "running", + finishedAt: new Date(), + responsibleUserId: "user-claim", + }, + }); + const token = createLocalAgentJwt(agentId, companyId, "codex_local", runId, "user-claim"); + + const res = await request(createApp(db)) + .get(`/companies/${companyId}/protected`) + .set("Authorization", `Bearer ${token}`) + .set("X-Paperclip-Run-Id", runId); + + expect(res.status).toBe(401); + expect(res.body.error).toBe("Agent run is not active"); + }); + it("rejects fork-minted run JWTs before issue reads or writes reach live issue data", async () => { const companyId = randomUUID(); const agentId = randomUUID(); diff --git a/server/src/__tests__/issue-feedback-delivery-retry-routes.test.ts b/server/src/__tests__/issue-feedback-delivery-retry-routes.test.ts new file mode 100644 index 00000000000..3f6ee9951cb --- /dev/null +++ b/server/src/__tests__/issue-feedback-delivery-retry-routes.test.ts @@ -0,0 +1,168 @@ +import { randomUUID } from "node:crypto"; +import express from "express"; +import request from "supertest"; +import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; +import { + activityLog, + agents, + agentWakeupRequests, + companies, + createDb, + issueRecoveryActions, + issues, +} from "@paperclipai/db"; +import { + getEmbeddedPostgresTestSupport, + startEmbeddedPostgresTestDatabase, +} from "./helpers/embedded-postgres.js"; +import { errorHandler } from "../middleware/index.js"; +import { issueRoutes } from "../routes/issues.js"; +import { + FEEDBACK_DELIVERY_RECOVERY_ACTION_KIND, + FEEDBACK_DELIVERY_RECOVERY_CAUSE, +} from "../services/recovery/feedback-delivery.js"; + +const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport(); +const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip; + +if (!embeddedPostgresSupport.supported) { + console.warn( + `Skipping embedded Postgres feedback-delivery retry route tests on this host: ${ + embeddedPostgresSupport.reason ?? "unsupported environment" + }`, + ); +} + +describeEmbeddedPostgres("issue feedback-delivery retry routes", () => { + let db!: ReturnType; + let tempDb: Awaited> | null = null; + + beforeAll(async () => { + tempDb = await startEmbeddedPostgresTestDatabase("paperclip-feedback-delivery-retry-routes-"); + db = createDb(tempDb.connectionString); + }, 20_000); + + afterEach(async () => { + await db.delete(issueRecoveryActions); + await db.delete(activityLog); + await db.delete(issues); + await db.delete(agentWakeupRequests); + await db.delete(agents); + await db.delete(companies); + }); + + afterAll(async () => { + await tempDb?.cleanup(); + }); + + function createApp(companyId: string) { + const app = express(); + app.use(express.json()); + app.use((req, _res, next) => { + req.actor = { + type: "board", + userId: "board-user", + companyIds: [companyId], + memberships: [{ companyId, membershipRole: "admin", status: "active" }], + isInstanceAdmin: false, + source: "session", + }; + next(); + }); + app.use("/api", issueRoutes(db, {} as any)); + app.use(errorHandler); + return app; + } + + async function seedExhaustedDelivery(input: { agentStatus?: string; withAgent?: boolean } = {}) { + const companyId = randomUUID(); + const issueId = randomUUID(); + const agentId = input.withAgent === false ? null : randomUUID(); + const rootWakeupRequestId = randomUUID(); + const sourceCommentId = randomUUID(); + const issuePrefix = `F${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`; + + await db.insert(companies).values({ + id: companyId, + name: "Paperclip", + issuePrefix, + requireBoardApprovalForNewAgents: false, + }); + if (agentId) { + await db.insert(agents).values({ + id: agentId, + companyId, + name: "CodexCoder", + role: "engineer", + status: input.agentStatus ?? "paused", + adapterType: "codex_local", + adapterConfig: {}, + runtimeConfig: {}, + permissions: {}, + }); + } + await db.insert(issues).values({ + id: issueId, + companyId, + title: "Feedback delivery exhausted", + status: "in_review", + priority: "medium", + assigneeAgentId: agentId, + issueNumber: 1, + identifier: `${issuePrefix}-1`, + }); + await db.insert(issueRecoveryActions).values({ + companyId, + sourceIssueId: issueId, + kind: FEEDBACK_DELIVERY_RECOVERY_ACTION_KIND, + status: "active", + ownerType: agentId ? "agent" : "board", + ownerAgentId: agentId, + previousOwnerAgentId: agentId, + returnOwnerAgentId: agentId, + cause: FEEDBACK_DELIVERY_RECOVERY_CAUSE, + fingerprint: `feedback_delivery:${companyId}:${issueId}:${agentId ?? "missing"}:${rootWakeupRequestId}`, + evidence: { + rootWakeupRequestId, + outstandingCommentIds: [sourceCommentId], + }, + nextAction: "Deliver the outstanding feedback", + attemptCount: 1, + maxAttempts: 1, + }); + + return { companyId, issueId }; + } + + it("returns a paused reason instead of leaking the wakeup 409", async () => { + const { companyId, issueId } = await seedExhaustedDelivery({ agentStatus: "paused" }); + + const res = await request(createApp(companyId)) + .post(`/api/issues/${issueId}/feedback-delivery/retry`) + .send({}); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(res.body).toMatchObject({ + outcome: "no_invokable_assignee", + reason: "paused", + message: "This delivery's agent is paused. Unpause the agent before retrying.", + }); + expect(await db.select().from(agentWakeupRequests)).toHaveLength(0); + expect(await db.select().from(activityLog)).toHaveLength(0); + }); + + it("distinguishes a missing assignee from a paused one", async () => { + const { companyId, issueId } = await seedExhaustedDelivery({ withAgent: false }); + + const res = await request(createApp(companyId)) + .post(`/api/issues/${issueId}/feedback-delivery/retry`) + .send({}); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(res.body).toMatchObject({ + outcome: "no_invokable_assignee", + reason: "missing", + message: "This delivery has no agent to retry against. Reassign the task first.", + }); + }); +}); diff --git a/server/src/__tests__/issue-recovery-actions.test.ts b/server/src/__tests__/issue-recovery-actions.test.ts index f5a2d036ddf..f33f1867695 100644 --- a/server/src/__tests__/issue-recovery-actions.test.ts +++ b/server/src/__tests__/issue-recovery-actions.test.ts @@ -66,6 +66,79 @@ function makeRecoveryActionRow(overrides: Record = {}) { } describe("issueRecoveryActionService", () => { + it("redacts raw failure text from existing recovery-action rows", async () => { + const existingRow = makeRecoveryActionRow({ + evidence: { + sourceRunId: "run-1", + lastFailureReason: "Error: ECONNREFUSED /home/agent/.secret/token.json", + }, + }); + const fakeDb = { + select: vi.fn(() => ({ + from() { + return this; + }, + where() { + return this; + }, + orderBy() { + return this; + }, + limit: vi.fn(async () => [existingRow]), + })), + }; + + const result = await issueRecoveryActionService(fakeDb as never).getActiveForIssue( + "company-1", + "source-1", + ); + + expect(result?.evidence).toEqual({ sourceRunId: "run-1" }); + }); + + it("does not persist raw failure text in new recovery actions", async () => { + let insertedValues: Record | null = null; + const fakeDb = { + select: vi.fn(() => ({ + from() { + return this; + }, + where() { + return this; + }, + orderBy() { + return this; + }, + limit: vi.fn(async () => []), + })), + insert: vi.fn(() => ({ + values: vi.fn((values: Record) => { + insertedValues = values; + return { + returning: vi.fn(async () => [makeRecoveryActionRow({ evidence: values.evidence })]), + }; + }), + })), + }; + + const result = await issueRecoveryActionService(fakeDb as never).upsertSourceScoped({ + companyId: "company-1", + sourceIssueId: "source-1", + kind: "missing_disposition", + ownerAgentId: "agent-1", + cause: "successful_run_missing_issue_disposition", + fingerprint: "missing-disposition:fingerprint", + evidence: { + sourceRunId: "run-1", + lastFailureReason: "Process lost -- child pid 3026905 is no longer running", + }, + nextAction: "Choose a valid issue disposition.", + }); + + expect(insertedValues).toMatchObject({ evidence: { sourceRunId: "run-1" } }); + expect(result.evidence).toEqual({ sourceRunId: "run-1" }); + }); + it("does not reactivate an action resolved between the active read and update", async () => { const existingRow = makeRecoveryActionRow({ id: "existing-action", attemptCount: 1 }); const createdRow = makeRecoveryActionRow({ id: "new-action", attemptCount: 1 }); @@ -1287,10 +1360,30 @@ describeEmbeddedPostgres("issue recovery actions", () => { ownerAgentId: managerId, cause: "successful_run_missing_issue_disposition", fingerprint: "missing-disposition:fingerprint", - evidence: { sourceRunId: "run-1" }, + evidence: { + sourceRunId: "run-1", + lastFailureReason: "Error: ECONNREFUSED /home/agent/.secret/token.json", + }, nextAction: "Choose a valid issue disposition.", wakePolicy: { type: "wake_owner" }, }); + const [storedAction] = await db + .select({ evidence: issueRecoveryActions.evidence }) + .from(issueRecoveryActions) + .where(eq(issueRecoveryActions.id, action.id)); + expect(storedAction?.evidence).toEqual({ sourceRunId: "run-1" }); + + // Emulate an action persisted before storage sanitization shipped. Every + // recovery-action read model must still remove the raw failure text. + await db + .update(issueRecoveryActions) + .set({ + evidence: { + sourceRunId: "run-1", + lastFailureReason: "Process lost -- child pid 3026905 is no longer running; retrying once", + }, + }) + .where(eq(issueRecoveryActions.id, action.id)); const app = createApp(); const detail = await request(app).get(`/api/issues/${sourceIssueId}`).expect(200); @@ -1300,10 +1393,14 @@ describeEmbeddedPostgres("issue recovery actions", () => { kind: "missing_disposition", ownerAgentId: managerId, }); + expect(detail.body.activeRecoveryAction.evidence).toEqual({ sourceRunId: "run-1" }); + expect(JSON.stringify(detail.body)).not.toContain("3026905"); const list = await request(app).get(`/api/issues/${sourceIssueId}/recovery-actions`).expect(200); expect(list.body.active).toMatchObject({ id: action.id }); expect(list.body.actions).toHaveLength(1); + expect(list.body.active.evidence).toEqual({ sourceRunId: "run-1" }); + expect(JSON.stringify(list.body)).not.toContain("3026905"); }); it("projects recovery action metadata into the structured wake payload", async () => { diff --git a/server/src/middleware/auth.ts b/server/src/middleware/auth.ts index 01df6fadf50..7017c2c4d38 100644 --- a/server/src/middleware/auth.ts +++ b/server/src/middleware/auth.ts @@ -46,7 +46,7 @@ function pruneCloudTenantWriteDebounce( } import { instanceSettingsService } from "../services/instance-settings.js"; import { ensureHumanRoleDefaultGrants } from "../services/principal-access-compatibility.js"; -import { forbidden, unprocessable } from "../errors.js"; +import { forbidden, unauthorized, unprocessable } from "../errors.js"; export { isCloudManagedInstance } from "../services/cloud-instance.js"; @@ -58,13 +58,18 @@ function normalizeOptionalString(value: string | null | undefined) { return value?.trim() || null; } -async function resolveLegacyRunResponsibleUserId( +async function loadAgentJwtRun( db: Db, input: { companyId: string; agentId: string; runId: string }, ) { if (!isUuidLike(input.runId)) return null; - const run = await db - .select({ responsibleUserId: heartbeatRuns.responsibleUserId }) + return db + .select({ + id: heartbeatRuns.id, + status: heartbeatRuns.status, + finishedAt: heartbeatRuns.finishedAt, + responsibleUserId: heartbeatRuns.responsibleUserId, + }) .from(heartbeatRuns) .where( and( @@ -74,7 +79,6 @@ async function resolveLegacyRunResponsibleUserId( ), ) .then((rows) => rows[0] ?? null); - return normalizeOptionalString(run?.responsibleUserId); } async function loadResponsibleUserMemberships( @@ -301,22 +305,6 @@ export function actorMiddleware(db: Db, opts: ActorMiddlewareOptions): RequestHa return; } - const agentRecord = await db - .select() - .from(agents) - .where(eq(agents.id, claims.sub)) - .then((rows) => rows[0] ?? null); - - if (!agentRecord || agentRecord.companyId !== claims.company_id) { - next(); - return; - } - - if (agentRecord.status === "terminated" || agentRecord.status === "pending_approval") { - next(); - return; - } - const normalizedRunIdHeader = normalizeOptionalString(runIdHeader); if (normalizedRunIdHeader && normalizedRunIdHeader !== claims.run_id) { await auditAgentJwtRunHeaderMismatch(db, { @@ -337,13 +325,43 @@ export function actorMiddleware(db: Db, opts: ActorMiddlewareOptions): RequestHa return; } + const [agentRecord, jwtRun] = await Promise.all([ + db + .select() + .from(agents) + .where(eq(agents.id, claims.sub)) + .then((rows) => rows[0] ?? null), + loadAgentJwtRun(db, { + companyId: claims.company_id, + agentId: claims.sub, + runId: claims.run_id, + }), + ]); + + if (!agentRecord || agentRecord.companyId !== claims.company_id) { + next(); + return; + } + + if (agentRecord.status === "terminated" || agentRecord.status === "pending_approval") { + next(); + return; + } + + // A local-agent JWT is a capability for exactly one live heartbeat run, + // not a long-lived agent credential. Revalidate the persisted lifecycle + // on every request so terminalizing the run revokes the capability + // immediately instead of leaving it usable until the JWT's expiry. + // `finishedAt` is checked as a fail-closed guard for an inconsistent row + // whose status was not updated atomically with finalization. + if (!jwtRun || jwtRun.status !== "running" || jwtRun.finishedAt !== null) { + next(unauthorized("Agent run is not active")); + return; + } + const onBehalfOfUserId = claims.responsible_user_id !== undefined ? normalizeOptionalString(claims.responsible_user_id) - : await resolveLegacyRunResponsibleUserId(db, { - companyId: claims.company_id, - agentId: claims.sub, - runId: claims.run_id, - }); + : normalizeOptionalString(jwtRun.responsibleUserId); const onBehalfOfMemberships = await loadResponsibleUserMemberships(db, { companyId: claims.company_id, userId: onBehalfOfUserId, diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index f02ec664dc5..f839ad19ba9 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -113,6 +113,7 @@ import { FEEDBACK_DELIVERY_RETRY_WAKE_REASON, FEEDBACK_DELIVERY_WAKE_COMMENT_IDS_KEY, } from "../services/recovery/feedback-delivery.js"; +import { evaluateAgentInvokabilityFromDb } from "../services/agent-invokability.js"; import { getTelemetryClient } from "../telemetry.js"; import { isUniqueViolation } from "../db-errors.js"; import type { StorageService } from "../storage/types.js"; @@ -10826,6 +10827,7 @@ export function issueRoutes( const states = await readStates(); res.json({ outcome: "not_exhausted", + reason: null, message: "This task has no exhausted feedback delivery to retry.", feedbackDelivery: states.values().next().value ?? null, } satisfies IssueFeedbackDeliveryRetryResponse); @@ -10849,12 +10851,40 @@ export function issueRoutes( if (!targetAgentId || !rootWakeupRequestId) { res.json({ outcome: "no_invokable_assignee", + reason: "missing", message: "This delivery has no agent to retry against. Reassign the task first.", feedbackDelivery: (await readStates()).get(outstandingCommentIds[0] ?? "") ?? null, } satisfies IssueFeedbackDeliveryRetryResponse); return; } + const targetAgent = await db + .select({ + id: agents.id, + companyId: agents.companyId, + name: agents.name, + reportsTo: agents.reportsTo, + status: agents.status, + }) + .from(agents) + .where(and(eq(agents.id, targetAgentId), eq(agents.companyId, issue.companyId))) + .then((rows) => rows[0] ?? null); + const invokability = await evaluateAgentInvokabilityFromDb(db, targetAgent); + if (!invokability.invokable) { + const message = invokability.reason === "paused" + ? "This delivery's agent is paused. Unpause the agent before retrying." + : invokability.reason === "pending_approval" + ? "This delivery's agent is waiting for approval. Approve or reassign the agent before retrying." + : "This delivery has no invokable agent to retry against. Reassign the task first."; + res.json({ + outcome: "no_invokable_assignee", + reason: invokability.reason, + message, + feedbackDelivery: (await readStates()).get(outstandingCommentIds[0] ?? "") ?? null, + } satisfies IssueFeedbackDeliveryRetryResponse); + return; + } + const statesBefore = await readStates(); const alreadyInFlight = outstandingCommentIds.some( (commentId) => statesBefore.get(commentId)?.retryInFlight === true, @@ -10862,6 +10892,7 @@ export function issueRoutes( if (alreadyInFlight) { res.json({ outcome: "already_queued", + reason: null, message: "A delivery retry is already queued.", feedbackDelivery: statesBefore.get(outstandingCommentIds[0] ?? "") ?? null, } satisfies IssueFeedbackDeliveryRetryResponse); @@ -10919,6 +10950,7 @@ export function issueRoutes( const projected = latestCommentId ? statesAfter.get(latestCommentId) ?? null : null; res.json({ outcome: "queued", + reason: null, message: "Retrying feedback delivery.", // Deferred behind a live run still reads as in-flight to the operator, so // fall back to a synthetic in-flight projection rather than reporting a diff --git a/server/src/services/heartbeat.ts b/server/src/services/heartbeat.ts index 4150426ebb9..de81ea13203 100644 --- a/server/src/services/heartbeat.ts +++ b/server/src/services/heartbeat.ts @@ -10340,7 +10340,6 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) outstandingCommentIds: input.delivery.commentIds, retryGeneration: input.delivery.generation, lastFailureCode: input.run.errorCode, - lastFailureReason: input.run.error, gateErrorCode: input.gateErrorCode ?? null, gateReason: input.gateReason ?? null, }, diff --git a/server/src/services/issue-recovery-actions.ts b/server/src/services/issue-recovery-actions.ts index 3ed00bc863b..fc83d52fbe4 100644 --- a/server/src/services/issue-recovery-actions.ts +++ b/server/src/services/issue-recovery-actions.ts @@ -16,6 +16,17 @@ type IssueRecoveryActionRow = typeof issueRecoveryActions.$inferSelect; type DbTransaction = Parameters[0]>[0]; type DbOrTransaction = Db | DbTransaction; +/** + * Recovery evidence is returned on company-visible issue APIs, so raw adapter + * failure text must never cross this service boundary. Keep the exact denylist + * here as defense in depth for rows written before the producer was fixed. + */ +function sanitizeEvidence(evidence: Record): Record { + const sanitized = { ...evidence }; + delete sanitized.lastFailureReason; + return sanitized; +} + export type UpsertIssueRecoveryActionInput = { companyId: string; sourceIssueId: string; @@ -64,7 +75,7 @@ function toReadModel(row: IssueRecoveryActionRow): IssueRecoveryAction { returnOwnerAgentId: row.returnOwnerAgentId, cause: row.cause, fingerprint: row.fingerprint, - evidence: row.evidence, + evidence: sanitizeEvidence(row.evidence), nextAction: row.nextAction, wakePolicy: row.wakePolicy, monitorPolicy: row.monitorPolicy, @@ -195,7 +206,7 @@ export function issueRecoveryActionService(db: Db) { returnOwnerAgentId: input.returnOwnerAgentId ?? existing.returnOwnerAgentId, cause: input.cause, fingerprint: input.fingerprint, - evidence: input.evidence ?? existing.evidence, + evidence: sanitizeEvidence(input.evidence ?? existing.evidence), nextAction: input.nextAction, wakePolicy: input.wakePolicy ?? null, monitorPolicy: input.monitorPolicy ?? null, @@ -237,7 +248,7 @@ export function issueRecoveryActionService(db: Db) { returnOwnerAgentId: input.returnOwnerAgentId ?? null, cause: input.cause, fingerprint: input.fingerprint, - evidence: input.evidence ?? {}, + evidence: sanitizeEvidence(input.evidence ?? {}), nextAction: input.nextAction, wakePolicy: input.wakePolicy ?? null, monitorPolicy: input.monitorPolicy ?? null, diff --git a/ui/src/components/FeedbackDeliveryState.test.tsx b/ui/src/components/FeedbackDeliveryState.test.tsx index e63dcd6e7b6..3aa2a42fd66 100644 --- a/ui/src/components/FeedbackDeliveryState.test.tsx +++ b/ui/src/components/FeedbackDeliveryState.test.tsx @@ -13,6 +13,7 @@ import { FeedbackDeliveryFooterStatus, FeedbackDeliveryNotice, feedbackDeliveryAnchorId, + feedbackDeliveryRetryErrorMessage, isIssueCommentFeedbackDelivery, useFeedbackDeliveryTransientSuccess, } from "./FeedbackDeliveryState"; @@ -318,6 +319,30 @@ describe("FeedbackDeliveryNotice — exhausted", () => { ).toBe(false); }); + it("shows an actionable paused-agent retry failure inline", () => { + const message = feedbackDeliveryRetryErrorMessage({ + outcome: "no_invokable_assignee", + reason: "paused", + message: "server fallback", + feedbackDelivery: delivery(), + }); + const node = render( + {}} retryError={message} />, + ); + expect(node.querySelector("[data-testid='feedback-delivery-retry-error']")?.textContent).toBe( + "The assigned agent is paused. Unpause the agent before retrying.", + ); + }); + + it("keeps missing-assignee retry guidance distinct from paused guidance", () => { + expect(feedbackDeliveryRetryErrorMessage({ + outcome: "no_invokable_assignee", + reason: "missing", + message: "This delivery has no agent to retry against. Reassign the task first.", + feedbackDelivery: delivery(), + })).toBe("This delivery has no agent to retry against. Reassign the task first."); + }); + it("hides retry from a viewer without the authority to trigger it", () => { const node = render(); expect(node.querySelector("[data-testid='feedback-delivery-retry']")).toBeNull(); diff --git a/ui/src/components/FeedbackDeliveryState.tsx b/ui/src/components/FeedbackDeliveryState.tsx index 6e29cabd496..02a8ad68937 100644 --- a/ui/src/components/FeedbackDeliveryState.tsx +++ b/ui/src/components/FeedbackDeliveryState.tsx @@ -1,7 +1,10 @@ import { useEffect, useRef, useState } from "react"; import { Link } from "react-router-dom"; import { CircleCheck, RefreshCw } from "lucide-react"; -import type { IssueCommentFeedbackDelivery } from "@paperclipai/shared"; +import type { + IssueCommentFeedbackDelivery, + IssueFeedbackDeliveryRetryResponse, +} from "@paperclipai/shared"; import { InlineBanner } from "@/components/InlineBanner"; import { SystemNotice } from "@/components/SystemNotice"; @@ -19,6 +22,19 @@ export const FEEDBACK_DELIVERY_RETRYING_REVEAL_MS = 400; /** How long the ephemeral "Delivered after retry" confirmation stays visible. */ export const FEEDBACK_DELIVERY_TRANSIENT_SUCCESS_MS = 5000; +export function feedbackDeliveryRetryErrorMessage( + response: IssueFeedbackDeliveryRetryResponse, +) { + if (response.outcome !== "no_invokable_assignee") return response.message; + if (response.reason === "paused") { + return "The assigned agent is paused. Unpause the agent before retrying."; + } + if (response.reason === "pending_approval") { + return "The assigned agent is waiting for approval. Approve or reassign it before retrying."; + } + return response.message; +} + const FEEDBACK_DELIVERY_STATES = [ "retrying", "exhausted_needs_attention", @@ -232,13 +248,13 @@ export function FeedbackDeliveryNotice({ delivery, onRetry, retryPending = false, - retryError = false, + retryError = null, className, }: { delivery: IssueCommentFeedbackDelivery; onRetry?: (commentId: string) => void; retryPending?: boolean; - retryError?: boolean; + retryError?: string | boolean | null; className?: string; }) { const anchorId = feedbackDeliveryAnchorId(delivery.sourceCommentId); @@ -349,7 +365,9 @@ export function FeedbackDeliveryNotice({

{retryError ? (

- Couldn't start another retry. Try again. + {typeof retryError === "string" + ? retryError + : "Couldn't start another retry. Try again."}

) : null} diff --git a/ui/src/components/IssueChatThread.tsx b/ui/src/components/IssueChatThread.tsx index 7de1991593b..d007dea0ddb 100644 --- a/ui/src/components/IssueChatThread.tsx +++ b/ui/src/components/IssueChatThread.tsx @@ -222,6 +222,7 @@ interface IssueChatMessageContext { retryingFeedbackDeliveryCommentId?: string | null; /** Comment id whose last retry request failed, so the banner can say so inline. */ failedFeedbackDeliveryRetryCommentId?: string | null; + feedbackDeliveryRetryErrorMessage?: string | null; onImageClick?: (src: string) => void; onAcceptInteraction?: ( interaction: @@ -547,6 +548,7 @@ interface IssueChatThreadProps { onRetryFeedbackDelivery?: (commentId: string) => Promise | void; retryingFeedbackDeliveryCommentId?: string | null; failedFeedbackDeliveryRetryCommentId?: string | null; + feedbackDeliveryRetryErrorMessage?: string | null; onDeleteComment?: (commentId: string) => Promise | void; interruptingQueuedRunId?: string | null; stoppingRunId?: string | null; @@ -1479,6 +1481,7 @@ function IssueChatUserMessage({ onRetryFeedbackDelivery, retryingFeedbackDeliveryCommentId, failedFeedbackDeliveryRetryCommentId, + feedbackDeliveryRetryErrorMessage, currentUserId, userProfileMap, } = useContext(IssueChatCtx); @@ -1687,7 +1690,11 @@ function IssueChatUserMessage({ delivery={feedbackDelivery} onRetry={onRetryFeedbackDelivery} retryPending={retryingFeedbackDeliveryCommentId === feedbackDelivery.sourceCommentId} - retryError={failedFeedbackDeliveryRetryCommentId === feedbackDelivery.sourceCommentId} + retryError={ + failedFeedbackDeliveryRetryCommentId === feedbackDelivery.sourceCommentId + ? feedbackDeliveryRetryErrorMessage ?? true + : false + } /> ) : null} @@ -4440,6 +4447,7 @@ export function IssueChatThread({ onRetryFeedbackDelivery, retryingFeedbackDeliveryCommentId = null, failedFeedbackDeliveryRetryCommentId = null, + feedbackDeliveryRetryErrorMessage = null, interruptingQueuedRunId = null, stoppingRunId = null, onImageClick, @@ -5021,6 +5029,7 @@ export function IssueChatThread({ onRetryFeedbackDelivery: stableOnRetryFeedbackDelivery, retryingFeedbackDeliveryCommentId, failedFeedbackDeliveryRetryCommentId, + feedbackDeliveryRetryErrorMessage, onImageClick: stableOnImageClick, onAcceptInteraction: stableOnAcceptInteraction, onRejectInteraction: stableOnRejectInteraction, @@ -5053,6 +5062,7 @@ export function IssueChatThread({ stableOnRetryFeedbackDelivery, retryingFeedbackDeliveryCommentId, failedFeedbackDeliveryRetryCommentId, + feedbackDeliveryRetryErrorMessage, stableOnImageClick, stableOnAcceptInteraction, stableOnRejectInteraction, diff --git a/ui/src/components/TaskChatThread.tsx b/ui/src/components/TaskChatThread.tsx index 5bbfcc377d5..23621b409a9 100644 --- a/ui/src/components/TaskChatThread.tsx +++ b/ui/src/components/TaskChatThread.tsx @@ -139,6 +139,7 @@ export function TaskChatThread(props: TaskChatThreadProps) { onRetryFeedbackDelivery, retryingFeedbackDeliveryCommentId, failedFeedbackDeliveryRetryCommentId, + feedbackDeliveryRetryErrorMessage, } = props; const liveWorkLinks = useMemo( @@ -687,6 +688,7 @@ export function TaskChatThread(props: TaskChatThreadProps) { onRetryFeedbackDelivery={onRetryFeedbackDelivery} retryingFeedbackDeliveryCommentId={retryingFeedbackDeliveryCommentId} failedFeedbackDeliveryRetryCommentId={failedFeedbackDeliveryRetryCommentId} + feedbackDeliveryRetryErrorMessage={feedbackDeliveryRetryErrorMessage} tail={tailRunId || bottomBlockerLinks ? ( <> {tailRunId ? ( diff --git a/ui/src/components/task-chat/TaskChatBubble.tsx b/ui/src/components/task-chat/TaskChatBubble.tsx index 26af6d8e72c..b8df4764f06 100644 --- a/ui/src/components/task-chat/TaskChatBubble.tsx +++ b/ui/src/components/task-chat/TaskChatBubble.tsx @@ -44,6 +44,7 @@ interface TaskChatBubbleProps { onRetryFeedbackDelivery?: (commentId: string) => Promise | void; retryingFeedbackDeliveryCommentId?: string | null; failedFeedbackDeliveryRetryCommentId?: string | null; + feedbackDeliveryRetryErrorMessage?: string | null; } function initialsForName(name: string) { @@ -79,6 +80,7 @@ export function TaskChatBubble({ onRetryFeedbackDelivery, retryingFeedbackDeliveryCommentId, failedFeedbackDeliveryRetryCommentId, + feedbackDeliveryRetryErrorMessage, }: TaskChatBubbleProps) { // Clicking an embedded image opens the full-screen lightbox (with download); // arrow keys walk across the other images in the same bubble. @@ -235,6 +237,8 @@ export function TaskChatBubble({ } retryError={ failedFeedbackDeliveryRetryCommentId === item.feedbackDelivery.sourceCommentId + ? feedbackDeliveryRetryErrorMessage ?? true + : false } /> ) : null} diff --git a/ui/src/components/task-chat/TaskChatThreadView.tsx b/ui/src/components/task-chat/TaskChatThreadView.tsx index a1716ddfa82..9b6204fa918 100644 --- a/ui/src/components/task-chat/TaskChatThreadView.tsx +++ b/ui/src/components/task-chat/TaskChatThreadView.tsx @@ -46,6 +46,7 @@ interface TaskChatThreadViewProps { onRetryFeedbackDelivery?: (commentId: string) => Promise | void; retryingFeedbackDeliveryCommentId?: string | null; failedFeedbackDeliveryRetryCommentId?: string | null; + feedbackDeliveryRetryErrorMessage?: string | null; /** Content appended inside the transcript scroller after the settled thread. */ tail?: ReactNode; /** Optional streaming-aware key when `tail` changes without changing `items`. */ @@ -65,6 +66,7 @@ function renderItem( onRetryFeedbackDelivery?: (commentId: string) => Promise | void, retryingFeedbackDeliveryCommentId?: string | null, failedFeedbackDeliveryRetryCommentId?: string | null, + feedbackDeliveryRetryErrorMessage?: string | null, ) { switch (item.kind) { case "message": { @@ -82,6 +84,7 @@ function renderItem( onRetryFeedbackDelivery={onRetryFeedbackDelivery} retryingFeedbackDeliveryCommentId={retryingFeedbackDeliveryCommentId} failedFeedbackDeliveryRetryCommentId={failedFeedbackDeliveryRetryCommentId} + feedbackDeliveryRetryErrorMessage={feedbackDeliveryRetryErrorMessage} attachedTurn={ item.attachedTurn ? ( ))} diff --git a/ui/src/pages/IssueDetail.tsx b/ui/src/pages/IssueDetail.tsx index eeb30ad41bf..bcfbd9b9eec 100644 --- a/ui/src/pages/IssueDetail.tsx +++ b/ui/src/pages/IssueDetail.tsx @@ -100,6 +100,7 @@ import { type IssueChatRunFinalizationAction, } from "../components/IssueChatThread"; import { TaskChatThread } from "../components/TaskChatThread"; +import { feedbackDeliveryRetryErrorMessage } from "../components/FeedbackDeliveryState"; import type { TaskChatIssueBrief } from "../components/task-chat/TaskChatDescriptionBubble"; import { useClassicTaskInterfaceEnabled } from "../hooks/useClassicTaskInterfaceEnabled"; import { workModeMetaFor } from "../lib/work-mode-meta"; @@ -962,6 +963,7 @@ type IssueDetailChatTabProps = { onRetryFeedbackDelivery?: (commentId: string) => void; retryingFeedbackDeliveryCommentId?: string | null; failedFeedbackDeliveryRetryCommentId?: string | null; + feedbackDeliveryRetryErrorMessage?: string | null; onResolveRecoveryAction?: (outcome: import("../components/IssueRecoveryActionCard").RecoveryResolveOutcome) => void; onReissueIsolatedRecoveryAction?: (request: import("../components/IssueRecoveryActionCard").RecoveryReissueRequest) => void; reissueIsolatedRecoveryActionPending?: boolean; @@ -1070,6 +1072,7 @@ const IssueDetailChatTab = memo(function IssueDetailChatTab({ onRetryFeedbackDelivery, retryingFeedbackDeliveryCommentId, failedFeedbackDeliveryRetryCommentId, + feedbackDeliveryRetryErrorMessage, recoveryAction, onResolveRecoveryAction, onReissueIsolatedRecoveryAction, @@ -1349,6 +1352,7 @@ const IssueDetailChatTab = memo(function IssueDetailChatTab({ onRetryFeedbackDelivery={onRetryFeedbackDelivery} retryingFeedbackDeliveryCommentId={retryingFeedbackDeliveryCommentId ?? null} failedFeedbackDeliveryRetryCommentId={failedFeedbackDeliveryRetryCommentId ?? null} + feedbackDeliveryRetryErrorMessage={feedbackDeliveryRetryErrorMessage ?? null} onResolveRecoveryAction={onResolveRecoveryAction} onReissueIsolatedRecoveryAction={onReissueIsolatedRecoveryAction} reissueIsolatedRecoveryActionPending={reissueIsolatedRecoveryActionPending} @@ -4196,6 +4200,8 @@ export function IssueDetail() { useState(null); const [retryingFeedbackDeliveryCommentId, setRetryingFeedbackDeliveryCommentId] = useState(null); + const [feedbackDeliveryRetryFailureMessage, setFeedbackDeliveryRetryFailureMessage] = + useState(null); const retryFeedbackDelivery = useMutation({ // The commentId is carried so the banner that was pressed owns the pending // and error state; the request itself is scoped to the issue's one open @@ -4204,6 +4210,7 @@ export function IssueDetail() { onMutate: (input) => { setRetryingFeedbackDeliveryCommentId(input.commentId); setFailedFeedbackDeliveryRetryCommentId(null); + setFeedbackDeliveryRetryFailureMessage(null); }, onSuccess: (result, variables) => { // The delivery state lives on the comment, so the thread is the cache that @@ -4212,10 +4219,14 @@ export function IssueDetail() { invalidateIssueDetail(); if (result.outcome === "queued" || result.outcome === "already_queued") return; setFailedFeedbackDeliveryRetryCommentId(variables.commentId); + setFeedbackDeliveryRetryFailureMessage(feedbackDeliveryRetryErrorMessage(result)); pushToast({ title: "Couldn't retry delivery", body: result.message, tone: "error" }); }, onError: (err, variables) => { setFailedFeedbackDeliveryRetryCommentId(variables.commentId); + setFeedbackDeliveryRetryFailureMessage( + err instanceof Error ? err.message : "Unable to retry feedback delivery.", + ); pushToast({ title: "Couldn't retry delivery", body: err instanceof Error ? err.message : "Unable to retry feedback delivery.", @@ -5263,6 +5274,7 @@ export function IssueDetail() { onRetryFeedbackDelivery={handleRetryFeedbackDelivery} retryingFeedbackDeliveryCommentId={retryingFeedbackDeliveryCommentId} failedFeedbackDeliveryRetryCommentId={failedFeedbackDeliveryRetryCommentId} + feedbackDeliveryRetryErrorMessage={feedbackDeliveryRetryFailureMessage} onResolveRecoveryAction={handleResolveRecoveryAction} onReissueIsolatedRecoveryAction={handleReissueIsolatedRecoveryAction} reissueIsolatedRecoveryActionPending={reissueIsolatedRecoveryAction.isPending}