From 5416a4fc450dc37a57cd016a4428850a3bebdd55 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 21:29:30 -0700 Subject: [PATCH 1/2] test(approvals): pin credential-owner approval for a teammate's agent (TASK-512) Launch scope per the CTO's 10-25 ruling: a teammate's agent asks for a write under another person's connection, and only the credential owner can decide it. Real-Mongo service cases cover routing to the credential owner alone, refusal of the agent owner and the agent, a single pinned merge on approval with the decider in resolvedBy, and decline. These are the facts ADR-033 B3 relies on when it records decidedBy from resolvedBy with decidedByRole credential-owner. Delegated decision-owner approval stays deferred and reserved. Written by Rivet, a Commonly agent. Co-Authored-By: Claude Opus 5.5 Co-authored-by: rivet Agent-Model: claude/claude-opus-5-5/default --- .../service/teammate-approval.test.js | 143 ++++++++++++++++++ 1 file changed, 143 insertions(+) create mode 100644 backend/__tests__/service/teammate-approval.test.js diff --git a/backend/__tests__/service/teammate-approval.test.js b/backend/__tests__/service/teammate-approval.test.js new file mode 100644 index 0000000000..6986aeb1c2 --- /dev/null +++ b/backend/__tests__/service/teammate-approval.test.js @@ -0,0 +1,143 @@ +// TASK-512 launch scope: a teammate's agent asks for a write under another +// person's connection, and only the credential owner can approve it. +// ADR-033 B3 reads the decision record from `resolvedBy` with +// `decidedByRole: credential-owner`, so these cases pin the facts it relies on. +process.env.PG_HOST = ''; +process.env.JWT_SECRET = 'teammate-approval-test-only'; +const express = require('express'); +const request = require('supertest'); +const jwt = require('jsonwebtoken'); +jest.mock('../../services/githubAppService', () => ({ + getPullRequest: jest.fn(async () => ({ head: { sha: 'pinned-head' } })), + mergePullRequest: jest.fn(async () => ({ merged: true, sha: 'merged-head' })), +})); +jest.mock('../../models/ToolCall', () => ({ + create: jest.fn(async () => undefined), + digestArgs: (args) => require('crypto').createHash('sha256').update(JSON.stringify(args || {})).digest('hex'), + reserveBudgetLineage: jest.fn(async () => true), +})); +jest.mock('../../services/agentMessageService', () => ({ + postMessage: jest.fn(async () => ({ success: true, message: { _id: new (require('mongoose').Types.ObjectId)() } })), +})); +jest.mock('../../services/agentEventService', () => ({ enqueue: jest.fn(async () => ({})) })); +jest.mock('../../services/needsYouEmailService', () => ({ sendNeedsYouEmail: jest.fn(async () => undefined) })); +jest.mock('../../config/socket', () => ({ getIO: jest.fn(() => null) })); +const { setupMongoDb, closeMongoDb, clearMongoDb } = require('../utils/testUtils'); +const User = require('../../models/User'); +const Pod = require('../../models/Pod'); +const Integration = require('../../models/Integration'); +const ApprovalAction = require('../../models/ApprovalAction'); +const AttentionItem = require('../../models/AttentionItem'); +const { AgentInstallation } = require('../../models/AgentRegistry'); +const Installable = require('../../models/Installable'); +const { buildGithubToolInstallable } = require('../../services/installable/toolInstallables'); +const grants = require('../../services/roomGrantService'); +const broker = require('../../services/toolBrokerService'); +const approvals = require('../../services/approvalActionService'); +const github = require('../../services/githubAppService'); +const ToolCall = require('../../models/ToolCall'); + +const app = express(); +app.use(express.json()); +app.use('/api/approvals', require('../../routes/approvals')); + +// A owns the GitHub connection. B is A's teammate and owns the agent. +let a, b, agent, pod; +const human = (name) => User.create({ username: name, email: `${name}@example.test`, password: 'test-password' }); +const bearer = (person) => `Bearer ${jwt.sign({ id: String(person._id), tokenVersion: 0 }, process.env.JWT_SECRET)}`; + +beforeAll(setupMongoDb); +afterAll(closeMongoDb); +beforeEach(async () => { + jest.clearAllMocks(); + await clearMongoDb(); + a = await human('credential-a'); + b = await human('teammate-b'); + agent = await User.create({ + username: 'agent-b', email: 'agent-b@example.test', password: 'test-password', isBot: true, botType: 'agent', + botMetadata: { agentName: 'agent-b', instanceId: 'default', ownerUserId: String(b._id) }, + }); + pod = await Pod.create({ + name: 'Teammate approval', type: 'team', createdBy: a._id, + members: [a._id, b._id, agent._id], joinPolicy: 'invite-only', publicRead: false, + }); + await AgentInstallation.create({ + agentName: 'agent-b', instanceId: 'default', podId: pod._id, installedBy: b._id, version: '1.0.0', + status: 'active', config: { runtime: { runtimeType: 'hosted' }, hosted: { engine: 'native' } }, + }); + const connection = await Integration.create({ + type: 'github-app', scope: 'user', status: 'connected', createdBy: a._id, + config: { installationId: 'test-install', owner: 'test-owner', repo: 'test-repo' }, + }); + await Installable.create(buildGithubToolInstallable()); + const grant = await grants.mintGrant({ + connectionId: String(connection._id), installationId: 'test-install', brokerId: 'test-broker', + target: { kind: 'pod', id: String(pod._id) }, audience: [String(agent._id)], + tools: ['github.merge_pull_request'], writeMode: 'write-with-confirm', + expiresAt: new Date(Date.now() + 60000), + }); + await expect(broker.callTool({ + grantId: grant.grantId, agentUserId: String(agent._id), agentName: 'agent-b', instanceId: 'default', + hostedTurn: true, tool: 'github.merge_pull_request', args: { pullNumber: 7 }, + })).rejects.toMatchObject({ code: 'approval_required' }); +}); + +const parked = () => ApprovalAction.findOne({ actionType: 'tool_call' }); +const httpDecide = (row, person, decision = 'approved') => request(app) + .post(`/api/approvals/${row._id}/resolve`) + .set('Authorization', bearer(person)) + // Identity fields in the body must not choose who decided. + .send({ decision, resolvedBy: String(b._id), decidedBy: String(b._id), callerUserId: String(b._id) }); + +test('the teammate agent\'s write asks only the credential owner, never the agent owner', async () => { + const row = await parked(); + expect(String(row.ownerUserId)).toBe(String(a._id)); + // B3 records the decider as credential-owner; that holds only while the + // asked human and the connection owner are the same person. + expect(row.toolCall.credentialOwnerId).toBe(String(a._id)); + const queue = await AttentionItem.find({ + 'source.type': 'approval_action', 'source.id': String(row._id), status: 'open', + }).lean(); + expect(queue.map((item) => String(item.recipientUserId))).toEqual([String(a._id)]); + + expect((await httpDecide(row, b)).status).toBe(403); + const asAgent = await approvals.resolveApproval({ + approvalId: String(row._id), callerUserId: String(agent._id), decision: 'approved', + }); + expect(asAgent.status).toBe(403); + const pending = await ApprovalAction.findById(row._id); + expect(pending.status).toBe('flagged'); + expect(pending.resolvedBy).toBeUndefined(); + expect(github.mergePullRequest).not.toHaveBeenCalled(); +}); + +test('the credential owner approves the pinned merge once, and the row names them as the decider', async () => { + const row = await parked(); + const result = await httpDecide(row, a); + expect(result.status).toBe(200); + expect(result.body.approval).toMatchObject({ decision: 'approved', toolCallEnding: 'ran' }); + expect(github.mergePullRequest).toHaveBeenCalledTimes(1); + expect(github.mergePullRequest).toHaveBeenCalledWith(expect.objectContaining({ sha: 'pinned-head', pullNumber: 7 })); + + const saved = await ApprovalAction.findById(row._id); + expect(String(saved.resolvedBy)).toBe(String(a._id)); + expect(saved.toolCall.credentialOwnerId).toBe(String(a._id)); + expect(saved.toolCall.canonicalArgs).toBeUndefined(); + expect(ToolCall.create).toHaveBeenCalledWith(expect.objectContaining({ outcome: 'ok', credentialOwnerId: String(a._id) })); + expect(await AttentionItem.countDocuments({ 'source.id': String(row._id), status: 'open' })).toBe(0); + + expect((await httpDecide(row, a)).status).toBe(409); + expect(github.mergePullRequest).toHaveBeenCalledTimes(1); +}); + +test('the credential owner declining records them without a provider call', async () => { + const row = await parked(); + const result = await httpDecide(row, a, 'declined'); + expect(result.status).toBe(200); + expect(result.body.approval).toMatchObject({ decision: 'declined', toolCallEnding: 'declined' }); + const saved = await ApprovalAction.findById(row._id); + expect(String(saved.resolvedBy)).toBe(String(a._id)); + expect(saved.toolCall.canonicalArgs).toBeUndefined(); + expect(await AttentionItem.countDocuments({ 'source.id': String(row._id), status: 'open' })).toBe(0); + expect(github.mergePullRequest).not.toHaveBeenCalled(); +}); From 94acd8231d075773106df0e36fe28f59c50ce41c Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 22:59:26 -0700 Subject: [PATCH 2/2] feat(approvals): store who decided a parked tool call (TASK-512) The approval row now carries decidedBy and decidedByRole, written in the same atomic update as the decision, so the grant record reads the decider rather than inferring it (CTO note on TASK-512, 2026-10-26 05:20Z). - human approve or decline: the authenticated caller, credential-owner; the owner check already makes the caller the connection owner - expiry by sweep or late decision: policy / policy - legacy action cards and moot rows carry no decider The enum admits only credential-owner and policy: record-v1 reserves decision-owner and delegated approval stays deferred. Values match the ADR-033 B3/B4 mappings. A non-owner of a parked call now reads "Only the credential owner can decide this" instead of naming the workspace owner. Written by Rivet, a Commonly agent. Co-Authored-By: Claude Opus 5.5 Co-authored-by: rivet Agent-Model: claude/claude-opus-5-5/default --- .../service/teammate-approval.test.js | 91 +++++++++++++++---- backend/models/ApprovalAction.ts | 12 +++ backend/services/approvalActionService.ts | 9 +- 3 files changed, 94 insertions(+), 18 deletions(-) diff --git a/backend/__tests__/service/teammate-approval.test.js b/backend/__tests__/service/teammate-approval.test.js index 6986aeb1c2..cd69df8729 100644 --- a/backend/__tests__/service/teammate-approval.test.js +++ b/backend/__tests__/service/teammate-approval.test.js @@ -1,12 +1,13 @@ // TASK-512 launch scope: a teammate's agent asks for a write under another -// person's connection, and only the credential owner can approve it. -// ADR-033 B3 reads the decision record from `resolvedBy` with -// `decidedByRole: credential-owner`, so these cases pin the facts it relies on. +// person's connection, and only the credential owner can approve it. The row +// stores who decided (`decidedBy` + `decidedByRole`) when the decision +// commits, so the grant record (ADR-033 B3/B4) reads it rather than infers it. process.env.PG_HOST = ''; process.env.JWT_SECRET = 'teammate-approval-test-only'; const express = require('express'); const request = require('supertest'); const jwt = require('jsonwebtoken'); + jest.mock('../../services/githubAppService', () => ({ getPullRequest: jest.fn(async () => ({ head: { sha: 'pinned-head' } })), mergePullRequest: jest.fn(async () => ({ merged: true, sha: 'merged-head' })), @@ -42,7 +43,8 @@ app.use(express.json()); app.use('/api/approvals', require('../../routes/approvals')); // A owns the GitHub connection. B is A's teammate and owns the agent. -let a, b, agent, pod; +let a; let b; let agent; let + pod; const human = (name) => User.create({ username: name, email: `${name}@example.test`, password: 'test-password' }); const bearer = (person) => `Bearer ${jwt.sign({ id: String(person._id), tokenVersion: 0 }, process.env.JWT_SECRET)}`; @@ -54,31 +56,56 @@ beforeEach(async () => { a = await human('credential-a'); b = await human('teammate-b'); agent = await User.create({ - username: 'agent-b', email: 'agent-b@example.test', password: 'test-password', isBot: true, botType: 'agent', + username: 'agent-b', + email: 'agent-b@example.test', + password: 'test-password', + isBot: true, + botType: 'agent', botMetadata: { agentName: 'agent-b', instanceId: 'default', ownerUserId: String(b._id) }, }); pod = await Pod.create({ - name: 'Teammate approval', type: 'team', createdBy: a._id, - members: [a._id, b._id, agent._id], joinPolicy: 'invite-only', publicRead: false, + name: 'Teammate approval', + type: 'team', + createdBy: a._id, + members: [a._id, b._id, agent._id], + joinPolicy: 'invite-only', + publicRead: false, }); await AgentInstallation.create({ - agentName: 'agent-b', instanceId: 'default', podId: pod._id, installedBy: b._id, version: '1.0.0', - status: 'active', config: { runtime: { runtimeType: 'hosted' }, hosted: { engine: 'native' } }, + agentName: 'agent-b', + instanceId: 'default', + podId: pod._id, + installedBy: b._id, + version: '1.0.0', + status: 'active', + config: { runtime: { runtimeType: 'hosted' }, hosted: { engine: 'native' } }, }); const connection = await Integration.create({ - type: 'github-app', scope: 'user', status: 'connected', createdBy: a._id, + type: 'github-app', + scope: 'user', + status: 'connected', + createdBy: a._id, config: { installationId: 'test-install', owner: 'test-owner', repo: 'test-repo' }, }); await Installable.create(buildGithubToolInstallable()); const grant = await grants.mintGrant({ - connectionId: String(connection._id), installationId: 'test-install', brokerId: 'test-broker', - target: { kind: 'pod', id: String(pod._id) }, audience: [String(agent._id)], - tools: ['github.merge_pull_request'], writeMode: 'write-with-confirm', + connectionId: String(connection._id), + installationId: 'test-install', + brokerId: 'test-broker', + target: { kind: 'pod', id: String(pod._id) }, + audience: [String(agent._id)], + tools: ['github.merge_pull_request'], + writeMode: 'write-with-confirm', expiresAt: new Date(Date.now() + 60000), }); await expect(broker.callTool({ - grantId: grant.grantId, agentUserId: String(agent._id), agentName: 'agent-b', instanceId: 'default', - hostedTurn: true, tool: 'github.merge_pull_request', args: { pullNumber: 7 }, + grantId: grant.grantId, + agentUserId: String(agent._id), + agentName: 'agent-b', + instanceId: 'default', + hostedTurn: true, + tool: 'github.merge_pull_request', + args: { pullNumber: 7 }, })).rejects.toMatchObject({ code: 'approval_required' }); }); @@ -87,7 +114,13 @@ const httpDecide = (row, person, decision = 'approved') => request(app) .post(`/api/approvals/${row._id}/resolve`) .set('Authorization', bearer(person)) // Identity fields in the body must not choose who decided. - .send({ decision, resolvedBy: String(b._id), decidedBy: String(b._id), callerUserId: String(b._id) }); + .send({ + decision, + resolvedBy: String(b._id), + callerUserId: String(b._id), + decidedBy: String(b._id), + decidedByRole: 'decision-owner', + }); test('the teammate agent\'s write asks only the credential owner, never the agent owner', async () => { const row = await parked(); @@ -100,7 +133,9 @@ test('the teammate agent\'s write asks only the credential owner, never the agen }).lean(); expect(queue.map((item) => String(item.recipientUserId))).toEqual([String(a._id)]); - expect((await httpDecide(row, b)).status).toBe(403); + const asAgentOwner = await httpDecide(row, b); + expect(asAgentOwner.status).toBe(403); + expect(asAgentOwner.body.error).toBe('Only the credential owner can decide this'); const asAgent = await approvals.resolveApproval({ approvalId: String(row._id), callerUserId: String(agent._id), decision: 'approved', }); @@ -108,6 +143,8 @@ test('the teammate agent\'s write asks only the credential owner, never the agen const pending = await ApprovalAction.findById(row._id); expect(pending.status).toBe('flagged'); expect(pending.resolvedBy).toBeUndefined(); + expect(pending.decidedBy).toBeUndefined(); + expect(pending.decidedByRole).toBeUndefined(); expect(github.mergePullRequest).not.toHaveBeenCalled(); }); @@ -121,6 +158,8 @@ test('the credential owner approves the pinned merge once, and the row names the const saved = await ApprovalAction.findById(row._id); expect(String(saved.resolvedBy)).toBe(String(a._id)); + expect(saved.decidedBy).toBe(String(a._id)); + expect(saved.decidedByRole).toBe('credential-owner'); expect(saved.toolCall.credentialOwnerId).toBe(String(a._id)); expect(saved.toolCall.canonicalArgs).toBeUndefined(); expect(ToolCall.create).toHaveBeenCalledWith(expect.objectContaining({ outcome: 'ok', credentialOwnerId: String(a._id) })); @@ -137,7 +176,25 @@ test('the credential owner declining records them without a provider call', asyn expect(result.body.approval).toMatchObject({ decision: 'declined', toolCallEnding: 'declined' }); const saved = await ApprovalAction.findById(row._id); expect(String(saved.resolvedBy)).toBe(String(a._id)); + expect(saved.decidedBy).toBe(String(a._id)); + expect(saved.decidedByRole).toBe('credential-owner'); expect(saved.toolCall.canonicalArgs).toBeUndefined(); expect(await AttentionItem.countDocuments({ 'source.id': String(row._id), status: 'open' })).toBe(0); expect(github.mergePullRequest).not.toHaveBeenCalled(); }); + +test.each(['sweep', 'late decision'])('an expired parked call names policy as the decider (%s)', async (path) => { + const row = await parked(); + await ApprovalAction.updateOne({ _id: row._id }, { $set: { expiresAt: new Date(Date.now() - 1000) } }); + if (path === 'sweep') { + expect(await approvals.sweepExpiredToolCallApprovals()).toBe(1); + } else { + expect((await httpDecide(row, a)).status).toBe(409); + } + const saved = await ApprovalAction.findById(row._id); + expect(saved.status).toBe('expired'); + expect(saved.decidedBy).toBe('policy'); + expect(saved.decidedByRole).toBe('policy'); + expect(saved.resolvedBy).toBeUndefined(); + expect(github.mergePullRequest).not.toHaveBeenCalled(); +}); diff --git a/backend/models/ApprovalAction.ts b/backend/models/ApprovalAction.ts index 2bf7e090b5..c3a8deef0c 100644 --- a/backend/models/ApprovalAction.ts +++ b/backend/models/ApprovalAction.ts @@ -25,6 +25,9 @@ import mongoose, { Document, Model, Schema, Types } from 'mongoose'; export type ApprovalStatus = 'flagged' | 'resolved' | 'expired' | 'moot'; export type ApprovalDecision = 'approved' | 'declined'; export type ApprovalToolCallEnding = 'ran' | 'failed' | 'refused' | 'declined' | 'expired'; +/** record-v1 `decidedByRole` values a parked call can carry today. The spec + * reserves `decision-owner`; delegated approval is deferred (TASK-512). */ +export type ToolCallDecidedByRole = 'credential-owner' | 'policy'; // Registry of executable action types. Adding an action means adding its // executor in services/approvalExecutors.ts — the enum here only widens in @@ -71,6 +74,13 @@ export interface IApprovalAction extends Document { decision?: ApprovalDecision; resolvedBy?: Types.ObjectId; resolvedAt?: Date; + /** + * tool_call only: who decided the parked call, stored when the decision + * commits so the grant record reads it instead of inferring it (ADR-033 B3/B4). + * A user id with `credential-owner`, or `policy` for both when it expired. + */ + decidedBy?: string; + decidedByRole?: ToolCallDecidedByRole; // ADR-017:201 remains the legacy default. tool_call is terminal on expiry // because it authorizes disclosure to an external provider. decidedAfterExpiry?: boolean; @@ -112,6 +122,8 @@ const ApprovalActionSchema = new Schema( decision: { type: String, enum: ['approved', 'declined'] }, resolvedBy: { type: Schema.Types.ObjectId, ref: 'User' }, resolvedAt: { type: Date }, + decidedBy: { type: String }, + decidedByRole: { type: String, enum: ['credential-owner', 'policy'] }, decidedAfterExpiry: { type: Boolean }, executedAt: { type: Date }, executionResult: { type: Schema.Types.Mixed }, diff --git a/backend/services/approvalActionService.ts b/backend/services/approvalActionService.ts index 1b183db278..354132ee4e 100644 --- a/backend/services/approvalActionService.ts +++ b/backend/services/approvalActionService.ts @@ -805,7 +805,10 @@ export const resolveApproval = async (options: ResolveOptions): Promise