diff --git a/server/src/services/tool-action-replay-intent.test.ts b/server/src/services/tool-action-replay-intent.test.ts new file mode 100644 index 00000000000..15d0e8860c1 --- /dev/null +++ b/server/src/services/tool-action-replay-intent.test.ts @@ -0,0 +1,82 @@ +import { afterEach, expect, it, vi } from "vitest"; +import { isNewHumanIntentAfterToolRejection } from "./tool-action-replay-intent.js"; +afterEach(() => vi.useRealTimers()); +function fixture() { + vi.useFakeTimers(); vi.setSystemTime(10_000); + return { + session: { companyId: "company", issueId: "task", agentId: "agent", runId: "new-run", responsibleUserId: "person" }, + rejection: { companyId: "company", issueId: "task", status: "rejected", requestedByAgentId: "agent", + resolvedByUserId: "person", decidedByUserId: "person", resolvedByAgentId: null, decidedByAgentId: null, + resolvedAt: new Date(2_000), decidedAt: new Date(2_000) }, + invocation: { companyId: "company", issueId: "task", agentId: "agent", runId: "original-run" }, + run: { id: "new-run", companyId: "company", agentId: "agent", status: "running", responsibleUserId: "person", startedAt: new Date(4_000), + issueId: "task", nativeIssueId: "task", contextSnapshot: { source: "issue.comment", issueId: "task", taskId: "task", commentId: "new-human" } }, + comment: { id: "new-human", companyId: "company", issueId: "task", authorType: "user", authorUserId: "person", authorAgentId: null, createdByRunId: null, + derivedAuthorAgentId: null, derivedCreatedByRunId: null, derivedAuthorSource: null, deletedAt: null, sourceTrust: null, presentation: null, + createdAt: new Date(3_000), body: "Please try that update again and let me review it." }, + } satisfies Parameters[0]; +} +it("distinguishes a new authenticated human request after rejection without approving it", () => { + const f = fixture(); expect(isNewHumanIntentAfterToolRejection(f)).toBe(true); + expect(f.rejection.status).toBe("rejected"); +}); +it("accepts the ordinary direct-human reopen source with the same persisted intent checks", () => { + const f = fixture(); f.run.contextSnapshot.source = "issue.comment.reopen"; + expect(isNewHumanIntentAfterToolRejection(f)).toBe(true); +}); +it.each(["automatic", "derived", "forged comment"])("does not treat %s reopen input as a new human intent", fault => { + const f = fixture(); f.run.contextSnapshot.source = "issue.comment.reopen"; + if (fault === "automatic") f.run.contextSnapshot.source = "automation.issue.comment.reopen"; + else if (fault === "derived") Object.assign(f.comment, { derivedCreatedByRunId: "agent-run" }); + else f.run.contextSnapshot.commentId = "unrelated-human-comment"; + expect(isNewHumanIntentAfterToolRejection(f)).toBe(false); +}); +const invalid: Array<[string, (f: ReturnType) => void]> = [ + ["same intent retry", f => { f.comment.createdAt = new Date(1_000); }], + ["same run retry", f => { f.invocation.runId = f.run.id; }], + ["rejection continuation", f => { f.run.contextSnapshot.source = "tool_action_review"; }], + ["automatic continuation", f => { f.run.contextSnapshot.source = "chat_task_completed"; }], + ["foreign session company", f => { f.session.companyId = "foreign"; }], + ["foreign request company", f => { f.rejection.companyId = "foreign"; }], + ["foreign request task", f => { f.rejection.issueId = "foreign"; }], + ["foreign invocation actor", f => { f.invocation.agentId = "foreign"; }], + ["foreign invocation task", f => { f.invocation.issueId = "foreign"; }], + ["foreign invocation company", f => { f.invocation.companyId = "foreign"; }], + ["foreign run company", f => { f.run.companyId = "foreign"; }], + ["foreign run actor", f => { f.run.agentId = "foreign"; }], + ["stopped run", f => { f.run.status = "cancelled"; }], + ["foreign context source", f => { f.run.contextSnapshot.issueId = "foreign"; }], + ["contradictory task binding", f => { f.run.contextSnapshot.taskId = "foreign"; }], + ["foreign native task", f => { f.run.nativeIssueId = "foreign"; }], + ["foreign stored run task", f => { f.run.issueId = "foreign"; }], + ["wrong canonical comment", f => { f.run.contextSnapshot.commentId = "old-human"; }], + ["foreign comment task", f => { f.comment.issueId = "foreign"; }], + ["foreign comment company", f => { f.comment.companyId = "foreign"; }], + ["another human", f => { f.comment.authorUserId = "another-person"; }], + ["wrong responsible user", f => { f.run.responsibleUserId = "another-person"; }], + ["another person's rejection", f => { f.rejection.decidedByUserId = "another-person"; }], + ["unknown rejection author", f => { f.rejection.decidedByUserId = ""; f.rejection.resolvedByUserId = ""; }], + ["agent-attributed comment", f => { Object.assign(f.comment, { authorAgentId: "agent" }); }], + ["agent-created comment", f => { Object.assign(f.comment, { createdByRunId: "old-run" }); }], + ["derived agent comment", f => { Object.assign(f.comment, { derivedAuthorAgentId: "agent" }); }], + ["derived run comment", f => { Object.assign(f.comment, { derivedCreatedByRunId: "run" }); }], + ["derived attribution", f => { Object.assign(f.comment, { derivedAuthorSource: "run_log" }); }], + ["deleted human comment", f => { Object.assign(f.comment, { deletedAt: new Date(5_000) }); }], + ["untrusted external source", f => { Object.assign(f.comment, { sourceTrust: { disposition: "quarantined" } }); }], + ["system notice", f => { Object.assign(f.comment, { presentation: { kind: "system_notice" } }); }], + ["non-user author", f => { f.comment.authorType = "agent"; }], + ["empty comment", f => { f.comment.body = " "; }], + ["new-session command", f => { f.comment.body = "/new"; }], + ["equal rejection time", f => { f.comment.createdAt = new Date(2_000); }], + ["comment after run started", f => { f.comment.createdAt = new Date(5_000); }], + ["future run", f => { f.run.startedAt = new Date(11_000); }], + ["invalid decision time", f => { f.rejection.decidedAt = new Date(NaN); }], + ["invalid comment time", f => { f.comment.createdAt = new Date(NaN); }], + ["invalid start time", f => { f.run.startedAt = new Date(NaN); }], +]; +it.each(invalid)("preserves the prior rejection for %s", (_name, mutate) => { + const f = fixture(); mutate(f); expect(isNewHumanIntentAfterToolRejection(f)).toBe(false); +}); +it.each(["pending", "approved", "executing", "executed", "failed", "cancelled"])("never bypasses %s action state", status => { + const f = fixture(); f.rejection.status = status; expect(isNewHumanIntentAfterToolRejection(f)).toBe(false); +}); diff --git a/server/src/services/tool-action-replay-intent.ts b/server/src/services/tool-action-replay-intent.ts new file mode 100644 index 00000000000..148826aba5e --- /dev/null +++ b/server/src/services/tool-action-replay-intent.ts @@ -0,0 +1,60 @@ +/** A fresh human request may ask for another review, never inherit approval. + * These values come only from persisted run/comment rows, not tool arguments. */ +interface ReplaySession { + companyId: string; agentId: string | null; issueId: string | null; runId: string | null; + responsibleUserId?: string | null; +} +interface ReplayRun { + id: string; companyId: string; agentId: string; status: string; responsibleUserId: string | null; + startedAt: Date | null; issueId?: string | null; nativeIssueId?: string | null; + contextSnapshot: Record | null; +} +interface HumanComment { + id: string; companyId: string; issueId: string; authorType: string | null; + authorUserId: string | null; authorAgentId: string | null; createdByRunId: string | null; + derivedAuthorAgentId: string | null; derivedCreatedByRunId: string | null; derivedAuthorSource: string | null; + deletedAt: Date | null; sourceTrust: unknown; presentation: { kind?: string } | null; + createdAt: Date; body: string; +} +interface RejectedAction { + companyId: string; issueId: string | null; status: string; requestedByAgentId: string | null; + resolvedByUserId: string | null; decidedByUserId: string | null; + resolvedByAgentId: string | null; decidedByAgentId: string | null; + resolvedAt: Date | null; decidedAt: Date | null; +} +interface Invocation { + companyId: string; issueId: string | null; agentId: string | null; runId: string | null; +} + +export function isNewHumanIntentAfterToolRejection(input: { + session: ReplaySession; rejection: RejectedAction; invocation: Invocation; + run: ReplayRun; comment: HumanComment; +}): boolean { + const { session, rejection, invocation, run, comment } = input; + if (rejection.status !== "rejected" || !session.runId || !session.issueId || !session.agentId || !session.responsibleUserId + || rejection.companyId !== session.companyId || rejection.issueId !== session.issueId || rejection.requestedByAgentId !== session.agentId + || invocation.companyId !== session.companyId || invocation.issueId !== session.issueId || invocation.agentId !== session.agentId + || !invocation.runId || invocation.runId === session.runId + || run.id !== session.runId || run.companyId !== session.companyId || run.agentId !== session.agentId + || run.status !== "running" || run.responsibleUserId !== session.responsibleUserId) return false; + const context = run.contextSnapshot; + if (!context || (context.source !== "issue.comment" && context.source !== "issue.comment.reopen") || context.commentId !== comment.id + || context.issueId !== session.issueId + || (context.taskId != null && context.taskId !== session.issueId) + || (run.issueId != null && run.issueId !== session.issueId) + || (run.nativeIssueId != null && run.nativeIssueId !== session.issueId)) return false; + if (comment.companyId !== session.companyId || comment.issueId !== session.issueId + || comment.authorType !== "user" || comment.authorUserId !== session.responsibleUserId + || comment.authorAgentId || comment.createdByRunId || comment.derivedAuthorAgentId || comment.derivedCreatedByRunId + || comment.derivedAuthorSource || comment.deletedAt || comment.sourceTrust != null + || comment.presentation?.kind === "system_notice" || !comment.body.trim() || comment.body.trim() === "/new") return false; + // A later request by a different person cannot override this person's decline. + const deciders = [rejection.decidedByUserId, rejection.resolvedByUserId].filter(value => value != null); + if (!deciders.length || deciders.some(value => value !== session.responsibleUserId) + || rejection.resolvedByAgentId || rejection.decidedByAgentId) return false; + const decisions = [rejection.decidedAt, rejection.resolvedAt].filter((value): value is Date => value != null); + const times = decisions.map(value => value.getTime()); + const requestedAt = comment.createdAt.getTime(), startedAt = run.startedAt?.getTime(); + return times.length > 0 && times.every(Number.isFinite) && Number.isFinite(requestedAt) && Number.isFinite(startedAt) + && Math.max(...times) < requestedAt && requestedAt <= startedAt! && startedAt! <= Date.now(); +} diff --git a/server/src/services/tool-gateway.ts b/server/src/services/tool-gateway.ts index 540668d69b0..5c19fe5302d 100644 --- a/server/src/services/tool-gateway.ts +++ b/server/src/services/tool-gateway.ts @@ -1,3 +1,4 @@ +import { isNewHumanIntentAfterToolRejection } from "./tool-action-replay-intent.js"; import { notifyChatRunMilestones } from "./chat-run-milestone-notifications.js"; import { writeToolConnectionHealthChange } from "./tool-connection-health-notifications.js"; import { writeApprovalChange } from "./approval-work-notifications.js"; @@ -71,6 +72,7 @@ import { documents, heartbeatRuns, issueApprovals, + issueComments, issueDocuments, issueThreadInteractions, issues, @@ -8597,6 +8599,35 @@ export function createToolGatewayService( } else if (match.actionRequest.canonicalArgumentsHash !== input.argumentsHash) { return null; } + // Rejection applies to that intent, not every later human request with the + // same arguments. A new trusted request still passes the normal policy and + // Ask first path below; it receives no execution or approval authority here. + if (match.actionRequest.status === "rejected" && input.session.runId && input.session.responsibleUserId) { + const [intent] = await db.select({ + run: { + id: heartbeatRuns.id, companyId: heartbeatRuns.companyId, agentId: heartbeatRuns.agentId, + status: heartbeatRuns.status, responsibleUserId: heartbeatRuns.responsibleUserId, + startedAt: heartbeatRuns.startedAt, issueId: heartbeatRuns.issueId, nativeIssueId: heartbeatRuns.nativeIssueId, + contextSnapshot: sql>`jsonb_build_object( + 'source', ${heartbeatRuns.contextSnapshot}->'source', + 'issueId', ${heartbeatRuns.contextSnapshot}->'issueId', + 'taskId', ${heartbeatRuns.contextSnapshot}->'taskId', + 'commentId', ${heartbeatRuns.contextSnapshot}->'commentId')`, + }, + comment: issueComments, + }).from(heartbeatRuns).innerJoin(issueComments, and( + eq(issueComments.companyId, heartbeatRuns.companyId), + eq(issueComments.issueId, input.session.issueId), + eq(issueComments.id, sql`case when ${heartbeatRuns.contextSnapshot}->>'commentId' + ~* '^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$' + then (${heartbeatRuns.contextSnapshot}->>'commentId')::uuid end`), + )).where(and( + eq(heartbeatRuns.id, input.session.runId), eq(heartbeatRuns.companyId, input.session.companyId), + eq(heartbeatRuns.agentId, input.session.agentId), + )).limit(1); + if (intent && isNewHumanIntentAfterToolRejection({ session: input.session, rejection: match.actionRequest, + invocation: match.invocation, ...intent })) return null; + } if (match.actionRequest.status === "failed" && match.invocation.errorCode !== "provider_interaction_required") return null; // The gateway builds an ask-first request in two steps inside one call: it // inserts the row with a null signature and a null expiry, then signs the diff --git a/tests/runner-e2e/chat-catalog-domains/external-app-review.ts b/tests/runner-e2e/chat-catalog-domains/external-app-review.ts index de5f71b71bb..182aad61113 100644 --- a/tests/runner-e2e/chat-catalog-domains/external-app-review.ts +++ b/tests/runner-e2e/chat-catalog-domains/external-app-review.ts @@ -395,6 +395,7 @@ export const appReviewExternalCases = [ "../../server/src/routes/issues.ts", "../../server/src/routes/tool-access.ts", "../../server/src/services/tool-gateway.ts", + "../../server/src/services/tool-action-replay-intent.ts", "../../server/src/services/tool-action-review.ts", "../../server/src/services/tool-action-delivery.ts", "../../server/src/services/tool-access-policy.ts",