diff --git a/maps/agent-access-map/app-permissions.md b/maps/agent-access-map/app-permissions.md index 22675ec3973..ca3a23345df 100644 --- a/maps/agent-access-map/app-permissions.md +++ b/maps/agent-access-map/app-permissions.md @@ -313,7 +313,7 @@ Gaps: Agent invocation is denied by the stated policy. The authorized human or o API: `present` — `POST /api/tool-gateway/action-requests/{id}/decline`; JSON REST. Agent access: `restricted` — existing human setup or approval surface; agent can identify the saved setup and inspect the result. -Policy: Human consent, identity confirmation, action approval, or trusted-host administration. This endpoint does not delegate human identity or self-approval to an agent. +Policy: Human consent, identity confirmation, action approval, or trusted-host administration. This endpoint does not delegate human identity or self-approval to an agent. A distinct later direct-human comment, bound to the active source run and the same responsible person, can ask for a new review after that person's rejection. It grants no execution authority. Same-intent retries and rejection continuations retain the rejection; all current policy and approval checks still apply. Evidence: `source-only` — [route](../../server/src/routes/tool-gateway.ts); reviewed in the working tree based on the source commit above. Verification: `not-run` — individual operation not executed in this audit; shared authorization and representative family flows are exercised by the authenticated integration suite. Follow the route schema, inspect persistence/audit, and assert owner, scope and revoked-authority denial. Gaps: Agent invocation is denied by the stated policy. The authorized human or operator must perform this step; this entry does not claim delegated execution. diff --git a/maps/feature-map/app-permissions.md b/maps/feature-map/app-permissions.md index 921a493d5c9..5dc3a874396 100644 --- a/maps/feature-map/app-permissions.md +++ b/maps/feature-map/app-permissions.md @@ -20,6 +20,13 @@ Agent access: [companion](../agent-access-map/app-permissions.md). - `test-action`: `app-permissions/test-action` — choose an actor and inputs and inspect a real action result. - `account-maintenance`: `app-permissions/account-maintenance` — inspect accounts, reconnect/revoke, and review connection activity. +Declining an Ask first action declines that invocation. It does not change the +connection's policy. A later, distinct request from the same responsible person +can open a new review for the same action and arguments. The agent must obtain +that new approval before execution. Retries of the declined intent and its +rejection continuation remain declined. Missing or untrusted human provenance +does not open another review. + Changing the connection's agent list preserves profile assignments for agents that remain installed. Saving the list must not restore a removed app-profile assignment. New installations retain the current app-profile defaults. diff --git a/server/src/__tests__/tool-gateway-service.test.ts b/server/src/__tests__/tool-gateway-service.test.ts index 0d277ee4dbf..41f483dea02 100644 --- a/server/src/__tests__/tool-gateway-service.test.ts +++ b/server/src/__tests__/tool-gateway-service.test.ts @@ -2725,6 +2725,48 @@ describeEmbeddedPostgres("tool gateway service", () => { })).rejects.toMatchObject({ reasonCode: "action_declined" }); }); + it.each(["issue.comment", "issue.comment.reopen"])("opens a fresh review only for a later trusted human intent after a decline (%s)", async source => { + const { company, agent, issue, run } = await createRunFixture(db); + const human = "board-user"; + await db.insert(companyMemberships).values({ companyId: company.id, principalType: "user", principalId: human, membershipRole: "owner" }); + const [firstComment] = await db.insert(issueComments).values({ companyId: company.id, issueId: issue.id, + authorType: "user", authorUserId: human, body: "Update this note." }).returning(); + await db.update(heartbeatRuns).set({ responsibleUserId: human, startedAt: new Date(), + contextSnapshot: { issueId: issue.id, source: "issue.comment", commentId: firstComment!.id } }).where(eq(heartbeatRuns.id, run.id)); + await initializeRunIdentity(db, { companyId: company.id, runId: run.id, issueId: issue.id, + responsibleUserId: human, messageIds: [firstComment!.id], cause: "instruction" }); + await db.insert(toolPolicies).values(reviewNotePolicyRow(company)); + const gateway = createTestToolGatewayService(db); + const original = await gateway.createSession(runSessionInput(company, agent, run)); + const call = { tool: "mcp-remote-fixture:update_note", parameters: { noteId: "n1", body: "same proposed write" } }; + await expect(gateway.executeTool({ ...call, sessionToken: original.token })).rejects.toMatchObject({ reasonCode: "approval_required" }); + const [first] = await db.select().from(toolActionRequests).where(eq(toolActionRequests.companyId, company.id)); + await gateway.declineActionRequest({ companyId: company.id, actionRequestId: first!.id, actor: { userId: human } }); + await expect(gateway.executeTool({ ...call, sessionToken: original.token })).rejects.toMatchObject({ reasonCode: "action_declined" }); + await db.update(heartbeatRuns).set({ status: "succeeded" }).where(eq(heartbeatRuns.id, run.id)); + const [nextComment] = await db.insert(issueComments).values({ companyId: company.id, issueId: issue.id, + authorType: "user", authorUserId: human, body: "Please request my review for that update again." }).returning(); + // Make the relevant strict ordering explicit, without timer sleeps or a new clock. + const rejectedAt = new Date(nextComment!.createdAt.getTime() - 1); + await db.update(toolActionRequests).set({ decidedAt: rejectedAt, resolvedAt: rejectedAt }).where(eq(toolActionRequests.id, first!.id)); + const [nextRun] = await db.insert(heartbeatRuns).values({ companyId: company.id, agentId: agent.id, status: "running", + responsibleUserId: human, startedAt: new Date(), invocationSource: "on_demand", + contextSnapshot: { issueId: issue.id, source, commentId: nextComment!.id } }).returning(); + await initializeRunIdentity(db, { companyId: company.id, runId: nextRun!.id, issueId: issue.id, + responsibleUserId: human, messageIds: [nextComment!.id], cause: "instruction" }); + const next = await gateway.createSession(runSessionInput(company, agent, nextRun!)); + await expect(gateway.executeTool({ ...call, sessionToken: next.token })).rejects.toMatchObject({ reasonCode: "approval_required" }); + await expect(gateway.executeTool({ ...call, sessionToken: next.token })).rejects.toMatchObject({ reasonCode: "approval_required" }); + const requests = await db.select().from(toolActionRequests).where(eq(toolActionRequests.companyId, company.id)); + expect(requests).toHaveLength(2); + expect(requests.find(row => row.id === first!.id)?.status).toBe("rejected"); + expect(requests.find(row => row.id !== first!.id)).toMatchObject({ status: "pending", requestedByAgentId: agent.id }); + const invocations = await db.select().from(toolInvocations).where(eq(toolInvocations.companyId, company.id)); + expect(invocations.map(row => row.status).sort()).toEqual(["awaiting_approval", "denied"]); + expect(await db.select().from(toolAccessAuditEvents).where(and(eq(toolAccessAuditEvents.companyId, company.id), eq(toolAccessAuditEvents.action, "tool_gateway.call_completed")))).toHaveLength(0); + console.info(`Verified fresh tool review after decline: ${source}`); + }); + it("expires a stale identical request and creates a fresh approval", async () => { const { company, agent, run } = await createRunFixture(db); await db.insert(toolPolicies).values(reviewNotePolicyRow(company)); 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