diff --git a/.agents/skills/create-agent-adapter/SKILL.md b/.agents/skills/create-agent-adapter/SKILL.md index 6d11362b099..8c87c349e57 100644 --- a/.agents/skills/create-agent-adapter/SKILL.md +++ b/.agents/skills/create-agent-adapter/SKILL.md @@ -156,7 +156,7 @@ Guidelines: - Use `info` for successful checks and context. Severity policy is product-critical: warnings are not save blockers. -Example: for `claude_local`, detected `ANTHROPIC_API_KEY` must be a `warn`, not an `error`, because Claude can still run (it just uses API-key auth instead of subscription auth). +Example: for `claude_local`, an explicitly configured `ANTHROPIC_API_KEY` or selected managed API connection is `info`: the user chose that authentication. An ambient server key overriding subscription login remains `warn`, not `error`. --- diff --git a/doc/execution-semantics.md b/doc/execution-semantics.md index 344bfeffd4d..00dc7459889 100644 --- a/doc/execution-semantics.md +++ b/doc/execution-semantics.md @@ -376,6 +376,17 @@ If the committed update assigns the issue to a user, clears the agent assignee, Plain text is not assignment. Writing an agent's name, role, or team label in a comment does not change ownership and does not create an agent wake. Agent routing from comment text requires a structured agent mention that resolves inside the company, an explicit `assigneeAgentId` mutation, or an existing current agent assignee receiving normal issue-thread feedback. +A delegation comment from the current assignee's run on this parent must not start competing parent work when the named worker already owns the referenced child. This applies to issue updates with a comment and standalone comments. Verify the source run's company, agent, and parent-task context. Then verify that the comment references the child's identifier, the child's `parentId` names this parent, and the child belongs to the same company and is assigned to the mentioned worker. Apply child-aware routing only in either of these states: + +- The parent is `blocked` and the child is `in_progress`. The child must have a blocker edge to the parent. The child's execution or checkout run must still be `running`, belong to that worker and company, and name that child in its run context. Verify comment and mutation access to the child, then retain the parent comment and append a linked copy on the child. Preserve the full comment, author, source run, responsible-user attribution, and source trust. Target the normal mention wake at the child and its new comment ID, with explicit resume and follow-up intent. This keeps new feedback available to the worker and lets the existing queue serialize a child continuation behind its current execution. +- The parent and child are both `done`. The assignee's closing comment must not start another worker run for the completed delegation. A blocker edge is not required after completion: a fast child can finish before the lead needs to record a wait. New agent work must use explicit `resume: true`, a status change, or a new assigned task. Explicit resume moves the parent out of `done` before this rule runs; the comment's prose alone does not restart completed work. + +The completed-delegation comment remains on the parent without a worker wake. Neither path changes ownership. Board-user comments and unrelated mentions retain their normal wake behavior. If multiple referenced children qualify for the same worker, the child or run no longer meets these conditions, child comment or mutation access is denied, or a lookup or copy fails, use the normal parent mention path. Do not parse mentions again while copying a comment, which would create another routing loop. Completion of the child still uses the existing blocker-resolution wake for the parent's assignee. + +The parent may receive a closing comment before its assignee changes the status to `done`. Recheck the completed-delegation rule when releasing that parent execution, before promoting a deferred mention. On the same transaction, verify the final parent state, finishing run, and every original queued or deferred comment ID. Each comment must belong to this parent and company, come from its assignee's finishing run, and reference exactly one completed direct child assigned to the mentioned worker. A link to the parent itself is allowed; any other extra issue reference keeps the normal mention path, including an unknown or foreign reference. Mixed human, other-run, unrelated, or ambiguous input retains its normal wake path. Explicit continuation and interaction requests also retain their normal path. + +Accepted agent feedback must survive a child changing to `done` before its active run exits. Deferred wake promotion may reopen that completed child only for its current assignee, with explicit agent resume intent and live tracked comments from another author. Claim promotion before reopening. Cancelled tasks, deleted comments, self-authored comments, empty continuations, and agent continuations without explicit intent keep their existing suppression rules. Normal pause, ownership, authorization, and budget gates still apply. + Pause and tree-control previews should make the same distinction visible. They should report whether the affected subtree contains live running work, queued wakes, agent-owned work, or only human-owned/static issues, so a pause after a handoff does not look like it interrupted agent execution when no agent execution path existed. ### Adapter-backed workspace coherence diff --git a/packages/adapters/claude-local/src/server/acp.auth.test.ts b/packages/adapters/claude-local/src/server/acp.auth.test.ts index 805ce4190f5..0e6b31f57b5 100644 --- a/packages/adapters/claude-local/src/server/acp.auth.test.ts +++ b/packages/adapters/claude-local/src/server/acp.auth.test.ts @@ -505,6 +505,25 @@ describe("Claude ACP hello probe on local and SSH targets", () => { expect(JSON.stringify(spawnedEnv)).not.toContain("caller-proxy"); }); + it("reports an explicitly selected API key as normal authentication on the ACP lane", async () => { + const result = await testClaudeAcpEnvironment({ + companyId: "company-1", + adapterType: "claude_local", + config: { engine: "acp", agentCommand: process.execPath, env: { ANTHROPIC_API_KEY: "selected-test-key" } }, + executionTarget: null, + environmentName: null, + }); + + expect(result.status).toBe("pass"); + expect(result.checks).toContainEqual(expect.objectContaining({ + code: "claude_acp_anthropic_api_key_detected", + level: "info", + message: "Using the selected Claude API connection.", + hint: undefined, + })); + expect(JSON.stringify(result.checks)).not.toContain("selected-test-key"); + }); + it("runs the host login probe with the host ANTHROPIC_API_KEY on a local target", async () => { // A local ACP run inherits the host environment, so a host ANTHROPIC_API_KEY // authenticates the real run. The Test lane runs the login probe with the @@ -530,7 +549,9 @@ describe("Claude ACP hello probe on local and SSH targets", () => { expect(result.checks.some((check) => check.code === "claude_hello_probe_auth_required")).toBe(false); expect(result.checks.some((check) => check.code === "claude_acp_login_probe_unavailable")).toBe(false); // The lane still reports that API-key auth is in use. - expect(result.checks.some((check) => check.code === "claude_acp_anthropic_api_key_detected")).toBe(true); + expect(result.checks).toContainEqual(expect.objectContaining({ + code: "claude_acp_anthropic_api_key_detected", level: "warn", + })); // The host key value never enters a check. expect(JSON.stringify(result.checks)).not.toContain("sk-ant-host-key"); }); diff --git a/packages/adapters/claude-local/src/server/acp.ts b/packages/adapters/claude-local/src/server/acp.ts index dcd2fbf20a8..cc7b4b207d3 100644 --- a/packages/adapters/claude-local/src/server/acp.ts +++ b/packages/adapters/claude-local/src/server/acp.ts @@ -785,12 +785,13 @@ export async function testClaudeAcpEnvironment( }); } else if (isNonEmpty(configApiKey) || isNonEmpty(hostApiKey)) { const source = isNonEmpty(configApiKey) ? "adapter config env" : "server environment"; + const selectedApiKey = Boolean(config.managedAiConnection) || isNonEmpty(configApiKey); checks.push({ code: "claude_acp_anthropic_api_key_detected", - level: config.managedAiConnection ? "info" : "warn", - message: config.managedAiConnection ? "Using the selected Claude API connection." : "ANTHROPIC_API_KEY is set. Claude ACP will use API-key auth instead of subscription credentials.", + level: selectedApiKey ? "info" : "warn", + message: selectedApiKey ? "Using the selected Claude API connection." : "ANTHROPIC_API_KEY is set. Claude ACP will use API-key auth instead of subscription credentials.", detail: `Detected in ${source}.`, - hint: config.managedAiConnection ? undefined : "Unset ANTHROPIC_API_KEY if you want subscription-based Claude login behavior.", + hint: selectedApiKey ? undefined : "Unset ANTHROPIC_API_KEY if you want subscription-based Claude login behavior.", }); } else if ( isNonEmpty(envConfig.CLAUDE_CODE_OAUTH_TOKEN) || diff --git a/packages/adapters/claude-local/src/server/test.ts b/packages/adapters/claude-local/src/server/test.ts index 507e645ef94..a13577730fc 100644 --- a/packages/adapters/claude-local/src/server/test.ts +++ b/packages/adapters/claude-local/src/server/test.ts @@ -205,13 +205,14 @@ export async function testEnvironment( }); } else if (isNonEmpty(configApiKey) || isNonEmpty(hostApiKey)) { const source = isNonEmpty(configApiKey) ? "adapter config env" : "server environment"; + const selectedApiKey = Boolean(config.managedAiConnection) || isNonEmpty(configApiKey); checks.push({ code: "claude_anthropic_api_key_overrides_subscription", - level: config.managedAiConnection ? "info" : "warn", + level: selectedApiKey ? "info" : "warn", message: - config.managedAiConnection ? "Using the selected Claude API connection." : "ANTHROPIC_API_KEY is set. Claude will use API-key auth instead of subscription credentials.", + selectedApiKey ? "Using the selected Claude API connection." : "ANTHROPIC_API_KEY is set. Claude will use API-key auth instead of subscription credentials.", detail: `Detected in ${source}.`, - hint: config.managedAiConnection ? undefined : "Unset ANTHROPIC_API_KEY if you want subscription-based Claude login behavior.", + hint: selectedApiKey ? undefined : "Unset ANTHROPIC_API_KEY if you want subscription-based Claude login behavior.", }); } else if ( isNonEmpty(env.CLAUDE_CODE_OAUTH_TOKEN) || diff --git a/packages/adapters/codex-local/src/server/test.ts b/packages/adapters/codex-local/src/server/test.ts index f6a5a496b33..0994994e948 100644 --- a/packages/adapters/codex-local/src/server/test.ts +++ b/packages/adapters/codex-local/src/server/test.ts @@ -73,6 +73,8 @@ function summarizeProbeDetail(stdout: string, stderr: string, parsedError: strin const CODEX_AUTH_REQUIRED_RE = /(?:not\s+logged\s+in|login\s+required|authentication\s+required|unauthorized|invalid(?:\s+or\s+missing)?\s+api(?:[_\s-]?key)?|openai[_\s-]?api[_\s-]?key|api[_\s-]?key.*required|please\s+run\s+`?codex\s+login`?)/i; +const PROBE_CLEANUP_WARNING = "[paperclip] Codex probe cleanup incomplete"; + async function prepareCodexHelloProbe(input: { runId: string; companyId: string; @@ -214,11 +216,13 @@ async function prepareCodexHelloProbe(input: { const probeHome = input.targetIsRemote ? path.posix.join(input.cwd, ".paperclip-runtime", "codex", `probe-home-${input.runId}`) : path.join(os.tmpdir(), `paperclip-codex-probe-${input.runId}`); + // The local finally path retries cleanup independently of the model result. + if (!input.targetIsRemote) probeHomeLocalDir = probeHome; return { command: "sh", args: [ "-c", - 'set -e; mkdir -p "$CODEX_HOME"; umask 077; printf "%s" "$_PAPERCLIP_CODEX_AUTH_JSON" > "$CODEX_HOME/auth.json"; unset _PAPERCLIP_CODEX_AUTH_JSON; trap \'rm -rf "$CODEX_HOME"\' EXIT INT TERM; "$0" "$@"', + `set -e; umask 077; mkdir -p "$CODEX_HOME"; printf "%s" "$_PAPERCLIP_CODEX_AUTH_JSON" > "$CODEX_HOME/auth.json"; unset _PAPERCLIP_CODEX_AUTH_JSON; cleanup() { result=$?; trap - EXIT; rm -f "$CODEX_HOME/auth.json" || true; rm -rf "$CODEX_HOME" || printf '%s\\n' '${PROBE_CLEANUP_WARNING}' >&2; exit "$result"; }; trap cleanup EXIT; trap 'exit 130' INT; trap 'exit 143' TERM; "$0" "$@"`, input.command, ...input.args, ], @@ -384,7 +388,16 @@ export async function testEnvironment( { ...config, fastMode: false }, { skipGitRepoCheck: targetIsSandbox }, ); - const args = execArgs.args; + // A connection test needs one small response, not plugin catalog sync, + // repository instructions, or a durable session. Keep provider/model + // configuration intact while removing unrelated startup work. + const args = [...execArgs.args]; + args.splice(args.length - 1, 0, + "-c", "features.plugins=false", + "-c", "features.remote_plugin=false", + "-c", "project_doc_max_bytes=0", + ...(args.includes("--ephemeral") ? [] : ["--ephemeral"]), + ); if (execArgs.fastModeIgnoredReason) { checks.push({ code: "codex_fast_mode_unsupported_model", @@ -440,8 +453,20 @@ export async function testEnvironment( }, ); const parsed = parseCodexJsonl(probe.stdout); - const detail = summarizeProbeDetail(probe.stdout, probe.stderr, parsed.errorMessage); - const authEvidence = `${parsed.errorMessage ?? ""}\n${probe.stdout}\n${probe.stderr}`.trim(); + // Plugin-catalog login is separate from model authentication. Its + // warnings must not explain an unrelated provider/process failure. + const providerStderr = probe.stderr.split(/\r?\n/) + .filter((line) => !/\bcodex_core_plugins(?:::|:)/.test(line) && !line.includes(PROBE_CLEANUP_WARNING)) + .join("\n"); + const detail = summarizeProbeDetail(probe.stdout, providerStderr, parsed.errorMessage); + const authEvidence = parsed.errorMessage?.trim() || providerStderr; + if (probe.stderr.includes(PROBE_CLEANUP_WARNING)) { + checks.push({ + code: "codex_probe_cleanup_incomplete", + level: "info", + message: "Temporary probe files could not be fully removed; this does not change the connection result.", + }); + } if (probe.timedOut) { checks.push({ diff --git a/server/src/__tests__/claude-local-adapter-environment.test.ts b/server/src/__tests__/claude-local-adapter-environment.test.ts index c83dc8d79fe..a68b77cfa3a 100644 --- a/server/src/__tests__/claude-local-adapter-environment.test.ts +++ b/server/src/__tests__/claude-local-adapter-environment.test.ts @@ -125,7 +125,7 @@ describe("claude_local environment diagnostics", () => { expect(result.checks.some((check) => check.level === "error")).toBe(false); }); - it("returns a warning (not an error) when ANTHROPIC_API_KEY is set in adapter env", async () => { + it("reports an explicitly configured API key as normal authentication", async () => { delete process.env.ANTHROPIC_API_KEY; delete process.env.CLAUDE_CODE_USE_BEDROCK; delete process.env.ANTHROPIC_BEDROCK_BASE_URL; @@ -143,12 +143,12 @@ describe("claude_local environment diagnostics", () => { }, }); - expect(result.status).toBe("warn"); + expect(result.status).toBe("pass"); expect( result.checks.some( (check) => check.code === "claude_anthropic_api_key_overrides_subscription" && - check.level === "warn", + check.level === "info", ), ).toBe(true); expect(result.checks.some((check) => check.level === "error")).toBe(false); diff --git a/server/src/__tests__/codex-local-adapter-environment.test.ts b/server/src/__tests__/codex-local-adapter-environment.test.ts index 46ad1c9f054..884e40950d7 100644 --- a/server/src/__tests__/codex-local-adapter-environment.test.ts +++ b/server/src/__tests__/codex-local-adapter-environment.test.ts @@ -5,6 +5,40 @@ import path from "node:path"; import { testEnvironment } from "@paperclipai/adapter-codex-local/server"; const itWindows = process.platform === "win32" ? it : it.skip; +const itPosix = process.platform === "win32" ? it.skip : it; + +async function runProbeFixture(options: { failCleanup?: boolean; error?: string } = {}) { + const root = await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-probe-result-")); + const capture = path.join(root, "capture.json"); + const command = path.join(root, "codex"); + await fs.writeFile(command, `#!${process.execPath} +const fs = require('node:fs'); +fs.writeFileSync(process.env.PROBE_CAPTURE, JSON.stringify({ args: process.argv.slice(2), home: process.env.CODEX_HOME })); +console.error('WARN codex_core_plugins::manager: remote installed plugin bundle sync failed error=chatgpt authentication required for remote plugin catalog'); +const error = process.env.PROBE_ERROR; +if (error) { console.error(error); process.exit(1); } +console.log(JSON.stringify({type:'item.completed',item:{type:'agent_message',text:'hello'}})); +console.log(JSON.stringify({type:'turn.completed',usage:{input_tokens:1,output_tokens:1}})); +`, { mode: 0o755 }); + if (options.failCleanup) { + // Deterministic equivalent of rm observing a concurrent plugin writer. + await fs.writeFile(path.join(root, "rm"), '#!/bin/sh\nif [ "$1" = "-rf" ]; then echo "rm: Directory not empty" >&2; exit 1; fi\nexec /bin/rm "$@"\n', { mode: 0o755 }); + } + try { + const result = await testEnvironment({ + companyId: "company-1", adapterType: "codex_local", + config: { engine: "cli", command, cwd: root, env: { + OPENAI_API_KEY: "fixture-key", PROBE_CAPTURE: capture, + PROBE_ERROR: options.error ?? "", PATH: `${root}${path.delimiter}${process.env.PATH ?? ""}`, + } }, + }); + return { result, capture: JSON.parse(await fs.readFile(capture, "utf8")) as { args: string[]; home: string } }; + } finally { + const recorded = await fs.readFile(capture, "utf8").then(JSON.parse).catch(() => null); + if (recorded?.home) await fs.rm(recorded.home, { recursive: true, force: true }); + await fs.rm(root, { recursive: true, force: true }); + } +} describe("codex_local environment diagnostics", () => { beforeEach(() => { @@ -13,6 +47,31 @@ describe("codex_local environment diagnostics", () => { afterEach(() => { vi.unstubAllEnvs(); }); + itPosix("preserves a successful hello when probe cleanup races a background writer", async () => { + const { result } = await runProbeFixture({ failCleanup: true }); + expect(result.status).toBe("pass"); + expect(result.checks).toContainEqual(expect.objectContaining({ code: "codex_hello_probe_passed" })); + expect(result.checks).not.toContainEqual(expect.objectContaining({ code: "codex_hello_probe_auth_required" })); + }); + + itPosix("does not diagnose an unrelated plugin login warning as model authentication failure", async () => { + const { result } = await runProbeFixture({ error: "Unable to start turn: disk is full" }); + expect(result.checks).toContainEqual(expect.objectContaining({ code: "codex_hello_probe_failed", detail: "Unable to start turn: disk is full" })); + expect(result.checks).not.toContainEqual(expect.objectContaining({ code: "codex_hello_probe_auth_required" })); + }); + + itPosix("still reports genuine provider authentication failures", async () => { + const { result } = await runProbeFixture({ error: "Invalid API key" }); + expect(result.checks).toContainEqual(expect.objectContaining({ code: "codex_hello_probe_auth_required" })); + }); + + itPosix("keeps hello probes free of plugin synchronization and repository instructions", async () => { + const { capture } = await runProbeFixture(); + expect(capture.args).toContain("features.plugins=false"); + expect(capture.args).toContain("features.remote_plugin=false"); + expect(capture.args).toContain("project_doc_max_bytes=0"); + expect(capture.args).toContain("--ephemeral"); + }); it("creates a missing working directory when cwd is absolute", async () => { const cwd = path.join( os.tmpdir(), diff --git a/server/src/__tests__/heartbeat-comment-wake-batching.test.ts b/server/src/__tests__/heartbeat-comment-wake-batching.test.ts index bf4bef72660..129249352e4 100644 --- a/server/src/__tests__/heartbeat-comment-wake-batching.test.ts +++ b/server/src/__tests__/heartbeat-comment-wake-batching.test.ts @@ -1199,10 +1199,11 @@ describeEmbeddedPostgres("heartbeat comment wake batching", () => { }, 120_000); it.each([ - { caseName: "allows a non-assignee mention on completed work", targetAssignee: false, terminalStatus: "done" }, - { caseName: "cancels an assignee continuation on completed work", targetAssignee: true, terminalStatus: "done" }, - { caseName: "cancels an assignee continuation on cancelled work", targetAssignee: true, terminalStatus: "cancelled" }, - ] as const)("$caseName without reopening an agent-commented task", async ({ targetAssignee, terminalStatus }) => { + { caseName: "allows a non-assignee mention on completed work", targetAssignee: false, terminalStatus: "done", explicitResume: false }, + { caseName: "delivers explicit agent feedback after completion", targetAssignee: true, terminalStatus: "done", explicitResume: true }, + { caseName: "cancels an assignee continuation without resume intent on completed work", targetAssignee: true, terminalStatus: "done", explicitResume: false }, + { caseName: "cancels an assignee continuation on cancelled work", targetAssignee: true, terminalStatus: "cancelled", explicitResume: true }, + ] as const)("$caseName", async ({ targetAssignee, terminalStatus, explicitResume }) => { const gateway = await createControlledGatewayServer(); const companyId = randomUUID(); const assigneeAgentId = randomUUID(); @@ -1211,6 +1212,7 @@ describeEmbeddedPostgres("heartbeat comment wake batching", () => { const issuePrefix = `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`; const heartbeat = heartbeatService(db); const targetAgentId = targetAssignee ? assigneeAgentId : mentionedAgentId; + const shouldReopen = targetAssignee && terminalStatus === "done" && explicitResume; const commentingAgentId = targetAssignee ? mentionedAgentId : assigneeAgentId; const wakeReason = targetAssignee ? "issue_commented" : "issue_comment_mentioned"; @@ -1269,7 +1271,7 @@ describeEmbeddedPostgres("heartbeat comment wake batching", () => { await db.insert(issues).values({ id: issueId, companyId, - title: "Do not reopen from agent mention", + title: "Agent feedback at completion boundary", status: "todo", priority: "medium", responsibleUserId: "responsible-user", @@ -1326,7 +1328,7 @@ describeEmbeddedPostgres("heartbeat comment wake batching", () => { commentId: comment.id, wakeCommentId: comment.id, wakeReason, - ...(targetAssignee ? { resumeIntent: true, followUpRequested: true } : {}), + ...(explicitResume ? { resumeIntent: true, followUpRequested: true } : {}), source: "comment.mention", }, requestedByActorType: "agent", @@ -1368,7 +1370,7 @@ describeEmbeddedPostgres("heartbeat comment wake batching", () => { gateway.releaseFirstWait(); - if (targetAssignee) { + if (targetAssignee && !shouldReopen) { await waitFor(async () => { const cancelled = await db.select().from(agentWakeupRequests).where(and( eq(agentWakeupRequests.companyId, companyId), @@ -1399,19 +1401,29 @@ describeEmbeddedPostgres("heartbeat comment wake batching", () => { ); }, 90_000); + const continuation = (await db.select().from(heartbeatRuns).where(and( + eq(heartbeatRuns.companyId, companyId), eq(heartbeatRuns.agentId, targetAgentId), + ))).find((run) => run.id !== firstRun!.id); + expect(continuation).toMatchObject({ + agentId: targetAgentId, + contextSnapshot: expect.objectContaining({ issueId }), + }); const issueAfterPromotion = await db .select({ status: issues.status, completedAt: issues.completedAt, + assigneeAgentId: issues.assigneeAgentId, }) .from(issues) .where(eq(issues.id, issueId)) .then((rows) => rows[0] ?? null); expect(issueAfterPromotion).toMatchObject({ - status: "done", + status: shouldReopen ? "in_progress" : "done", + assigneeAgentId, }); - expect(issueAfterPromotion?.completedAt).not.toBeNull(); + if (shouldReopen) expect(issueAfterPromotion?.completedAt).toBeNull(); + else expect(issueAfterPromotion?.completedAt).not.toBeNull(); const secondPayload = gateway.getAgentPayloads()[1] ?? {}; expect(secondPayload.paperclip).toBeUndefined(); @@ -1423,8 +1435,8 @@ describeEmbeddedPostgres("heartbeat comment wake batching", () => { issue: { id: issueId, identifier: `${issuePrefix}-1`, - title: "Do not reopen from agent mention", - status: "done", + title: "Agent feedback at completion boundary", + status: shouldReopen ? "in_progress" : "done", priority: "medium", }, }); diff --git a/server/src/__tests__/issue-update-comment-wakeup-routes.test.ts b/server/src/__tests__/issue-update-comment-wakeup-routes.test.ts index 775d3729c87..4ec0189d607 100644 --- a/server/src/__tests__/issue-update-comment-wakeup-routes.test.ts +++ b/server/src/__tests__/issue-update-comment-wakeup-routes.test.ts @@ -9,11 +9,13 @@ const SOURCE_RUN_ID = "44444444-4444-4444-8444-444444444444"; const mockIssueService = vi.hoisted(() => ({ getById: vi.fn(), + getByIdentifier: vi.fn(), getByIdForUpdate: vi.fn(), update: vi.fn(), addComment: vi.fn(), findMentionedAgents: vi.fn(), getRelationSummaries: vi.fn(), + getDependencyReadiness: vi.fn(), listWakeableBlockedDependents: vi.fn(), getWakeableParentAfterChildCompletion: vi.fn(), getCurrentScheduledRetry: vi.fn(), @@ -21,6 +23,12 @@ const mockIssueService = vi.hoisted(() => ({ })); const mockPauseGate = vi.hoisted(() => vi.fn(async (): Promise | null> => null)); +const mockAccessDecide = vi.hoisted(() => vi.fn(async (input: { action?: string; resource?: { issueId?: string } }) => ({ + allowed: true, + action: input.action, + reason: "allow_explicit_grant", + explanation: "Allowed by test grant.", +}))); const mockHeartbeatService = vi.hoisted(() => ({ wakeup: vi.fn(async () => undefined), @@ -57,12 +65,7 @@ vi.mock("../services/index.js", () => ({ }), accessService: () => ({ canUser: vi.fn(async () => true), - decide: vi.fn(async (input: { action?: string }) => ({ - allowed: true, - action: input.action, - reason: "allow_explicit_grant", - explanation: "Allowed by test grant.", - })), + decide: mockAccessDecide, hasPermission: vi.fn(async () => true), }), agentService: () => ({ @@ -133,12 +136,7 @@ function registerModuleMocks() { }), accessService: () => ({ canUser: vi.fn(async () => true), - decide: vi.fn(async (input: { action?: string }) => ({ - allowed: true, - action: input.action, - reason: "allow_explicit_grant", - explanation: "Allowed by test grant.", - })), + decide: mockAccessDecide, hasPermission: vi.fn(async () => true), }), agentService: () => ({ @@ -257,10 +255,13 @@ describe("issue update comment wakeups", () => { vi.doUnmock("../middleware/index.js"); registerModuleMocks(); vi.clearAllMocks(); + mockAccessDecide.mockImplementation(async (input) => ({ allowed: true, action: input.action, reason: "allow_explicit_grant", explanation: "Allowed by test grant." })); mockPauseGate.mockResolvedValue(null); mockIssueService.findMentionedAgents.mockResolvedValue([]); + mockIssueService.getByIdentifier.mockResolvedValue(null); mockIssueService.getByIdForUpdate.mockImplementation(async () => mockIssueService.getById()); mockIssueService.getRelationSummaries.mockResolvedValue({ blockedBy: [], blocks: [] }); + mockIssueService.getDependencyReadiness.mockResolvedValue({ unresolvedBlockerCount: 1 }); mockIssueService.listWakeableBlockedDependents.mockResolvedValue([]); mockIssueService.getWakeableParentAfterChildCompletion.mockResolvedValue(null); mockIssueService.getCurrentScheduledRetry.mockResolvedValue(null); @@ -677,6 +678,109 @@ describe("issue update comment wakeups", () => { ); }); + it.each((["post", "patch"] as const).flatMap((method) => [ + "active_delegation", "completed_delegation", "human_comment", "completed_human_comment", "unrelated_comment", "completed_child", + "active_feedback", "ambiguous_delegation", "child_access_denied", "child_mutation_denied", "forwarding_failure", + "source_run_other_issue", "completed_source_run_other_issue", + "completed_explicit_resume", "completed_parent_reference", "completed_mixed_reference", + "completed_delegation_without_blocker", "completed_foreign_company", "completed_unrelated_child", "completed_other_assignee", "completed_lookup_failure", + "unrelated_child", "foreign_company", "stopped_run", "foreign_run", "unrelated_run", "lookup_failure", + ].map((scenario) => ({ method, scenario }))))("routes $method mentions correctly for $scenario", async ({ method, scenario }) => { + const existing = makeIssue({ assigneeAgentId: ASSIGNEE_AGENT_ID, assigneeUserId: null, status: "blocked", executionRunId: SOURCE_RUN_ID }); + const child = makeIssue({ + id: "bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb", identifier: "PAP-1000", + parentId: existing.id, assigneeAgentId: MENTIONED_AGENT_ID, assigneeUserId: null, + status: "in_progress", executionRunId: "55555555-5555-4555-8555-555555555555", + }); + const secondChild = { ...child, id: "cccccccc-cccc-4ccc-8ccc-cccccccccccc", identifier: "PAP-1001", executionRunId: "66666666-6666-4666-8666-666666666666" }; + const delegationBody = scenario === "completed_parent_reference" + ? `${existing.identifier} is done thanks to [@QA](agent://${MENTIONED_AGENT_ID}) completing ${child.identifier}.` + : scenario === "completed_mixed_reference" + ? `[@QA](agent://${MENTIONED_AGENT_ID}) completed ${child.identifier}; now investigate PAP-9999.` + : scenario === "active_feedback" + ? `${child.identifier} is still failing; [@QA](agent://${MENTIONED_AGENT_ID}) please investigate the new error.` + : scenario === "unrelated_comment" + ? `[@QA](agent://${MENTIONED_AGENT_ID}) please review the parent task separately.` + : `Delegated to [@QA](agent://${MENTIONED_AGENT_ID}) via [${child.identifier}](/PAP/issues/${child.identifier}). Waiting for that task to finish.`; + // This unrelated mention must wake after the worker's routing decision. + // It gives the test an observable completion point for the async loop. + const body = `${delegationBody}${scenario === "ambiguous_delegation" ? ` Also check ${secondChild.identifier}.` : ""} [@Observer](agent://${PREVIOUS_AGENT_ID}) please review the parent separately.`; + const completedDelegation = scenario.startsWith("completed_") && scenario !== "completed_child"; + const humanComment = scenario === "human_comment" || scenario === "completed_human_comment"; + if (scenario === "completed_child" || completedDelegation) child.status = "done"; + if (completedDelegation) existing.status = "done"; + if (scenario === "unrelated_child" || scenario === "completed_unrelated_child") child.parentId = "other-parent"; + if (scenario === "foreign_company" || scenario === "completed_foreign_company") child.companyId = "other-company"; + if (scenario === "completed_other_assignee") child.assigneeAgentId = "other-agent"; + mockIssueService.getById.mockImplementation(async (id) => { + if (id === child.id && scenario === "lookup_failure") throw new Error("temporary read failure"); + if (id === secondChild.id) return secondChild; + return id === child.id ? child : existing; + }); + mockIssueService.getByIdentifier.mockImplementation(async (identifier) => { + if (scenario === "completed_lookup_failure") throw new Error("temporary identifier lookup failure"); + return identifier === child.identifier ? child : null; + }); + mockIssueService.update.mockImplementation(async (_id, patch) => scenario === "completed_explicit_resume" ? { ...existing, ...patch } : existing); + const originalComment = { + id: "delegation-note", issueId: existing.id, companyId: existing.companyId, body, + createdByRunId: humanComment ? null : SOURCE_RUN_ID, + authorAgentId: null, authorUserId: "local-board", authorType: "user", onBehalfOfUserId: "responsible-user", + sourceTrust: { preset: "low_trust_review", disposition: "quarantined", sourceIssueId: existing.id, sourceRunId: SOURCE_RUN_ID }, + }; + mockIssueService.addComment.mockImplementation(async (issueId, commentBody) => { + if (issueId === child.id && scenario === "forwarding_failure") throw new Error("temporary child comment failure"); + return issueId === child.id ? { ...originalComment, id: "forwarded-note", issueId, body: commentBody } : originalComment; + }); + mockIssueService.findMentionedAgents.mockResolvedValue([MENTIONED_AGENT_ID, PREVIOUS_AGENT_ID]); + mockIssueService.getRelationSummaries.mockResolvedValue({ blockedBy: completedDelegation && scenario !== "completed_delegation" ? [] : scenario === "ambiguous_delegation" ? [child, secondChild] : [child], blocks: [] }); + if (scenario === "child_access_denied") { + mockAccessDecide.mockImplementation(async (input) => ({ allowed: input.resource?.issueId !== child.id, action: input.action, reason: "test_access_decision", explanation: "Test child access decision." })); + } + if (scenario === "child_mutation_denied") { + mockAccessDecide.mockImplementation(async (input) => ({ allowed: input.resource?.issueId !== child.id || input.action !== "issue:mutate", action: input.action, reason: "test_access_decision", explanation: "Test child mutation decision." })); + } + mockHeartbeatService.getRun.mockImplementation(async (id) => ({ + id, companyId: id === child.executionRunId && scenario === "foreign_run" ? "other-company" : existing.companyId, + status: id === child.executionRunId && scenario === "stopped_run" ? "succeeded" : "running", + agentId: id === child.executionRunId || id === secondChild.executionRunId ? MENTIONED_AGENT_ID : ASSIGNEE_AGENT_ID, + contextSnapshot: { issueId: id === secondChild.executionRunId ? secondChild.id : id === child.executionRunId && scenario !== "unrelated_run" ? child.id : scenario.endsWith("source_run_other_issue") ? "other-source-issue" : existing.id }, + })); + const app = await createApp(); + const req = method === "post" + ? request(app).post(`/api/issues/${existing.id}/comments`) + : request(app).patch(`/api/issues/${existing.id}`); + if (!humanComment) req.set("X-Paperclip-Run-Id", SOURCE_RUN_ID); + const explicitResume = scenario === "completed_explicit_resume" ? { resume: true } : {}; + const res = await req.send(method === "post" ? { body, ...explicitResume } : { comment: body, ...explicitResume }); + expect(res.status).toBe(method === "post" ? 201 : 200); + await vi.waitFor(() => { + expect(mockHeartbeatService.wakeup).toHaveBeenCalledWith(PREVIOUS_AGENT_ID, expect.objectContaining({ reason: "issue_comment_mentioned" })); + }); + if (scenario === "active_delegation" || scenario === "active_feedback") { + expect(mockIssueService.addComment).toHaveBeenCalledWith( + child.id, + `Forwarded from [${existing.identifier}](/issues/${existing.identifier}#comment-${originalComment.id}):\n\n${body}`, + { agentId: undefined, userId: "local-board", runId: SOURCE_RUN_ID, onBehalfOfUserId: "responsible-user" }, + expect.objectContaining({ authorType: "user", sourceTrust: originalComment.sourceTrust }), + ); + expect(mockHeartbeatService.wakeup).toHaveBeenCalledWith(MENTIONED_AGENT_ID, expect.objectContaining({ + reason: "issue_comment_mentioned", + payload: expect.objectContaining({ issueId: child.id, commentId: "forwarded-note", resumeIntent: true, followUpRequested: true }), + contextSnapshot: expect.objectContaining({ issueId: child.id, taskId: child.id, commentId: "forwarded-note", wakeCommentId: "forwarded-note", source: "comment.mention.delegation", resumeIntent: true, followUpRequested: true }), + })); + expect(mockHeartbeatService.wakeup).not.toHaveBeenCalledWith(MENTIONED_AGENT_ID, expect.objectContaining({ payload: expect.objectContaining({ issueId: existing.id }) })); + } else if (["completed_delegation", "completed_delegation_without_blocker", "completed_parent_reference"].includes(scenario)) { + expect(mockHeartbeatService.wakeup).not.toHaveBeenCalledWith(MENTIONED_AGENT_ID, expect.objectContaining({ reason: "issue_comment_mentioned" })); + } else { + expect(mockHeartbeatService.wakeup).toHaveBeenCalledWith(MENTIONED_AGENT_ID, expect.objectContaining({ reason: "issue_comment_mentioned", payload: expect.objectContaining({ issueId: existing.id }) })); + if (scenario !== "forwarding_failure") { + expect(mockIssueService.addComment.mock.calls.some(([issueId]) => issueId === child.id)).toBe(false); + } + } + expect(mockIssueService.addComment).toHaveBeenCalled(); + }); + it("preserves an explicit resume on a run-authenticated top-level comment", async () => { const existing = makeIssue({ assigneeAgentId: ASSIGNEE_AGENT_ID, diff --git a/server/src/modules/wake-queue/adapters/postgres.test.ts b/server/src/modules/wake-queue/adapters/postgres.test.ts index 94dacfd82a7..09f7d11e3eb 100644 --- a/server/src/modules/wake-queue/adapters/postgres.test.ts +++ b/server/src/modules/wake-queue/adapters/postgres.test.ts @@ -106,6 +106,8 @@ describeEmbeddedPostgres("wake-queue postgres adapter", () => { assigneeAgentId?: string | null; executionRunId?: string | null; checkoutRunId?: string | null; + parentId?: string | null; + identifier?: string; }): Promise { const issueId = input.issueId ?? randomUUID(); await db.insert(issues).values({ @@ -117,6 +119,8 @@ describeEmbeddedPostgres("wake-queue postgres adapter", () => { assigneeAgentId: input.assigneeAgentId ?? null, executionRunId: input.executionRunId ?? null, checkoutRunId: input.checkoutRunId ?? null, + parentId: input.parentId ?? null, + identifier: input.identifier, }); return issueId; } @@ -165,6 +169,76 @@ describeEmbeddedPostgres("wake-queue postgres adapter", () => { return id; } + it.each([ + "completed", "multiple_comments", "repeated_reference", "parent_reference", "mixed_issue_references", "mixed_foreign_references", "mixed_unknown_references", "no_comments", "missing_comment", + "human_comment", "other_run_comment", "foreign_comment", "other_issue_comment", "deleted_comment", + "mixed_human_comments", "mixed_other_run_comments", "mixed_unrelated_comments", + "no_reference", "code_reference", "ambiguous_children", "child_open", "child_cancelled", + "foreign_child", "unrelated_child", "other_child_assignee", "parent_open", + "source_other_task", "source_other_agent", "wrong_company", "wrong_run", + ])("proves completed delegation from transactional comment and child rows (%s)", async (scenario) => { + const companyId = await seedCompany(); + const otherCompanyId = await seedCompany(); + const leadId = await seedAgent({ companyId, name: "Lead" }); + const workerId = await seedAgent({ companyId, name: "Worker" }); + const foreignAgentId = await seedAgent({ companyId: otherCompanyId }); + const issueId = await seedIssue({ companyId, identifier: "QA-1", assigneeAgentId: leadId, status: scenario === "parent_open" ? "in_progress" : "done" }); + const otherIssueId = await seedIssue({ companyId, identifier: "QA-99" }); + const foreignIssueId = await seedIssue({ companyId: otherCompanyId, identifier: "QA-98" }); + const runId = await seedRun({ companyId, agentId: scenario === "source_other_agent" ? workerId : leadId, + status: "succeeded", contextSnapshot: { issueId: scenario === "source_other_task" ? otherIssueId : issueId } }); + await db.update(issues).set({ executionRunId: runId }).where(eq(issues.id, issueId)); + const otherRunId = await seedRun({ companyId, agentId: leadId, status: "succeeded", contextSnapshot: { issueId } }); + await seedIssue({ + companyId: scenario === "foreign_child" ? otherCompanyId : companyId, + identifier: "QA-2", parentId: scenario === "unrelated_child" ? otherIssueId : issueId, + assigneeAgentId: scenario === "foreign_child" ? foreignAgentId : scenario === "other_child_assignee" ? leadId : workerId, + status: scenario === "child_open" ? "in_progress" : scenario === "child_cancelled" ? "cancelled" : "done", + }); + if (scenario === "ambiguous_children") { + await seedIssue({ companyId, identifier: "QA-3", parentId: issueId, assigneeAgentId: workerId, status: "done" }); + } + const body = scenario === "parent_reference" ? "QA-1 is done thanks to @Worker completing QA-2" + : scenario === "mixed_issue_references" ? "@Worker completed QA-2; now investigate QA-99" + : scenario === "mixed_foreign_references" ? "@Worker completed QA-2; also check QA-98" + : scenario === "mixed_unknown_references" ? "@Worker completed QA-2; also check QA-999" + : scenario === "no_reference" ? "Thanks @Worker" : scenario === "code_reference" ? "Example `QA-2`" + : scenario === "ambiguous_children" ? "@Worker completed QA-2 and QA-3" + : scenario === "repeated_reference" ? "@Worker completed [QA-2](/issues/QA-2); QA-2 is done" + : "@Worker completed QA-2"; + const [comment] = await db.insert(issueComments).values({ + companyId: scenario === "foreign_comment" ? otherCompanyId : companyId, + issueId: scenario === "foreign_comment" ? foreignIssueId : scenario === "other_issue_comment" ? otherIssueId : issueId, + authorAgentId: scenario === "human_comment" ? null : leadId, + authorUserId: scenario === "human_comment" ? "responsible-user" : null, + createdByRunId: scenario === "human_comment" ? null : scenario === "other_run_comment" ? otherRunId : runId, + body, deletedAt: scenario === "deleted_comment" ? new Date() : null, + }).returning(); + const commentIds = scenario === "no_comments" ? [] : [comment.id]; + if (scenario === "missing_comment") commentIds.push(randomUUID()); + if (["multiple_comments", "mixed_human_comments", "mixed_other_run_comments", "mixed_unrelated_comments"].includes(scenario)) { + const [second] = await db.insert(issueComments).values({ companyId, issueId, + authorAgentId: scenario === "mixed_human_comments" ? null : leadId, + authorUserId: scenario === "mixed_human_comments" ? "responsible-user" : null, + createdByRunId: scenario === "mixed_human_comments" ? null : scenario === "mixed_other_run_comments" ? otherRunId : runId, + body: scenario === "mixed_unrelated_comments" ? "@Worker, investigate a new error" : "QA-2 was delivered", + }).returning(); + commentIds.push(second.id); + } + let checked = false; + const adapter = createPostgresWakeQueueAdapter(db, stubDeps); + await adapter.withIssueExecutionLock({ companyId, runId, now: new Date() }, async (_locked, ports) => { + checked = true; + const completed = await ports.transaction.isCompletedDelegationMention({ + companyId: scenario === "wrong_company" ? otherCompanyId : companyId, + issueId, finishingRunId: scenario === "wrong_run" ? otherRunId : runId, wakeAgentId: workerId, commentIds, + }); + expect(completed).toBe(["completed", "multiple_comments", "repeated_reference", "parent_reference"].includes(scenario)); + return { outcome: { kind: "released" as const }, postCommitEffects: [] }; + }); + expect(checked).toBe(true); + }); + it.each([false, true])("rechecks disabled chat mode before interrupted queue promotion (conversation=%s)", async (conversation) => { const settings = instanceSettingsService(db); const original = (await settings.getExperimental()).enableAgentChat; diff --git a/server/src/modules/wake-queue/adapters/postgres.ts b/server/src/modules/wake-queue/adapters/postgres.ts index e52d385791c..1df9186a5f8 100644 --- a/server/src/modules/wake-queue/adapters/postgres.ts +++ b/server/src/modules/wake-queue/adapters/postgres.ts @@ -4,6 +4,7 @@ import { currentConversationCommentCondition } from "../../../services/agent-con import { getExecutionBlocker } from "../../../services/execution-blocker.js"; import { and, asc, eq, inArray, isNull, notInArray, or, sql } from "drizzle-orm"; import type { Db } from "@paperclipai/db"; +import { extractIssueReferenceIdentifiers } from "@paperclipai/shared"; import { agentWakeupRequests, agents, @@ -373,6 +374,44 @@ function buildTransaction(tx: Db, deps: WakeQueuePostgresAdapterDeps, db: Db, ru return { allSelfAuthored: rows.length > 0 && rows.every((row) => row.createdByRunId === finishingRunId) }; }, + async isCompletedDelegationMention({ companyId, issueId, finishingRunId, wakeAgentId, commentIds }) { + if ( + run.companyId !== companyId || run.id !== finishingRunId || + readNonEmptyString(parseObject(run.contextSnapshot).issueId) !== issueId + ) return false; + const uniqueCommentIds = [...new Set(commentIds)]; + if (uniqueCommentIds.length === 0) return false; + const parent = await tx.select({ status: issues.status, assigneeAgentId: issues.assigneeAgentId, identifier: issues.identifier }) + .from(issues).where(and(eq(issues.companyId, companyId), eq(issues.id, issueId))) + .then((rows) => rows[0]); + if (parent?.status !== "done" || parent.assigneeAgentId !== run.agentId) return false; + + const comments = await tx.select({ body: issueComments.body, createdByRunId: issueComments.createdByRunId, + authorAgentId: issueComments.authorAgentId }) + .from(issueComments).where(and( + eq(issueComments.companyId, companyId), eq(issueComments.issueId, issueId), + inArray(issueComments.id, uniqueCommentIds), isNull(issueComments.deletedAt), currentConversationCommentCondition(), + )); + if (comments.length !== uniqueCommentIds.length || comments.some((comment) => + comment.createdByRunId !== run.id || (comment.authorAgentId !== null && comment.authorAgentId !== run.agentId) + )) return false; + const referencesByComment = comments.map((comment) => extractIssueReferenceIdentifiers(comment.body) + .filter((identifier) => identifier !== parent.identifier)); + // A parent link is context; every other reference must identify the + // single completed child. Never discard unrelated follow-up work. + if (referencesByComment.some((references) => references.length !== 1)) return false; + const identifiers = [...new Set(referencesByComment.flat())]; + const children = await tx.select({ identifier: issues.identifier, status: issues.status }) + .from(issues).where(and( + eq(issues.companyId, companyId), eq(issues.parentId, issueId), + eq(issues.assigneeAgentId, wakeAgentId), inArray(issues.identifier, identifiers), + )); + return referencesByComment.every((references) => { + const referencedChildren = children.filter((child) => child.identifier !== null && references.includes(child.identifier)); + return referencedChildren.length === 1 && referencedChildren[0].status === "done"; + }); + }, + async reopenIssue({ companyId, issueId }) { const updated = await issuesSvc.updateForCompany(issueId, companyId, { status: "todo", executionState: null }, tx); return updated ? toIssueSnapshot(updated as unknown as IssueRow) : null; diff --git a/server/src/modules/wake-queue/application/ports.ts b/server/src/modules/wake-queue/application/ports.ts index 47fd3bd3745..63ecb59edca 100644 --- a/server/src/modules/wake-queue/application/ports.ts +++ b/server/src/modules/wake-queue/application/ports.ts @@ -158,6 +158,14 @@ export interface WakeQueueTransaction { finishingRunId: string; commentIds: string[]; }): Promise<{ allSelfAuthored: boolean }>; + /** Proves all candidate comments only report completed child work in the finishing parent's own run. */ + isCompletedDelegationMention(input: { + companyId: string; + issueId: string; + finishingRunId: string; + wakeAgentId: string; + commentIds: string[]; + }): Promise; reopenIssue(input: { companyId: string; issueId: string; runId: string }): Promise; /** * Atomically claims the wake for promotion, guarded on its current diff --git a/server/src/modules/wake-queue/application/use-cases.test.ts b/server/src/modules/wake-queue/application/use-cases.test.ts index 91ed298502c..07c3e49a584 100644 --- a/server/src/modules/wake-queue/application/use-cases.test.ts +++ b/server/src/modules/wake-queue/application/use-cases.test.ts @@ -114,6 +114,7 @@ function createFakeTransaction(overrides: Partial = {}): W releasePolicy: null, })), getCommentSelfAuthorship: vi.fn(async () => ({ allSelfAuthored: false })), + isCompletedDelegationMention: vi.fn(async () => false), reopenIssue: vi.fn(async () => null), claimDeferredWakeForPromotion: vi.fn(async () => true), finalizePromotedWake: vi.fn(async (input) => runSummary(input.wakeId)), @@ -127,11 +128,11 @@ function createFakeTransaction(overrides: Partial = {}): W }; } -function createFakeIssueLock(host: WakeQueueHost, transaction: WakeQueueTransaction, issue = ISSUE): IssueLockWriter { +function createFakeIssueLock(host: WakeQueueHost, transaction: WakeQueueTransaction, issue = ISSUE, run = RUN): IssueLockWriter { return { withIssueExecutionLock: vi.fn(async (_input, fn) => { - const result = await fn({ primaryIssue: issue, run: RUN }, { host, transaction }); - return { ...result, run: RUN }; + const result = await fn({ primaryIssue: issue, run }, { host, transaction }); + return { ...result, run }; }), }; } @@ -144,6 +145,49 @@ function createFakeRecovery(): RecoveryEscalationPort { } describe("releaseIssueExecution", () => { + it.each(["closing", "mixed_comments", "explicit_resume", "interaction", "other_source_task", "parent_open"])( + "checks late completed-delegation mentions without dropping independent input (%s)", async (scenario) => { + const originalCommentIds = ["closing-comment", "second-comment"]; + const candidate = wakeCandidate({ + reason: "issue_comment_mentioned", wakeReason: "issue_comment_mentioned", + requestedByActorType: "agent", requestedByActorId: RUN.agentId, + queuedCommentIds: [originalCommentIds[0]], deferredCommentIds: originalCommentIds, + preservesIndependentContinuation: scenario === "explicit_resume", + payload: scenario === "interaction" ? { mutation: "interaction" } : {}, + deferredContextSeed: scenario === "explicit_resume" ? { resumeIntent: true } : {}, + }); + const queue = [candidate]; + const transaction = createFakeTransaction({ + findNextDeferredWake: vi.fn(async () => queue.shift() ?? null), + isCompletedDelegationMention: vi.fn(async () => scenario !== "mixed_comments"), + getQueuedCommentLiveness: vi.fn(async () => ({ liveNonSelfCommentIds: [originalCommentIds[0]], containedSelfAuthoredComment: false })), + }); + const issue = { ...ISSUE, status: scenario === "parent_open" ? "in_progress" : "done" }; + const run = { ...RUN, status: "succeeded", contextSnapshot: { issueId: scenario === "other_source_task" ? "other-issue" : ISSUE.id } }; + const release = createReleaseIssueExecution({ + issueLock: createFakeIssueLock(createFakeHost(), transaction, issue, run), recovery: createFakeRecovery(), + }); + const result = await release({ companyId: RUN.companyId, runId: RUN.id, now: new Date() }); + if (scenario === "closing" || scenario === "mixed_comments") { + expect(transaction.isCompletedDelegationMention).toHaveBeenCalledWith({ + companyId: RUN.companyId, issueId: ISSUE.id, finishingRunId: RUN.id, + wakeAgentId: AGENT.id, commentIds: originalCommentIds, + }); + } else { + expect(transaction.isCompletedDelegationMention).not.toHaveBeenCalled(); + } + if (scenario === "closing") { + expect(result.outcome.kind).toBe("released"); + expect(transaction.cancelDeferredWake).toHaveBeenCalledTimes(1); + expect(transaction.claimDeferredWakeForPromotion).not.toHaveBeenCalled(); + expect(transaction.finalizePromotedWake).not.toHaveBeenCalled(); + } else { + expect(result.outcome.kind).toBe("promoted"); + expect(transaction.cancelDeferredWake).not.toHaveBeenCalled(); + } + }, + ); + it("preserves the former owner's queue for handoff adoption while draining the new owner's wake", async () => { const stale = wakeCandidate({ agentId: RUN.agentId, queuedCommentIds: ["saved-user-direction"] }); const current = wakeCandidate({ id: "wake-new-owner", agentId: "new-agent" }); @@ -489,6 +533,46 @@ describe("releaseIssueExecution", () => { expect(result.outcome.kind).toBe("promoted"); }); + it.each(["done_live", "cancelled_live", "done_missing", "done_self", "done_no_resume", "done_untracked_comment"])("handles explicit agent feedback after completion: %s", async (scenario) => { + const commentIds = ["accepted-agent-feedback"]; + const queue = [wakeCandidate({ + agentId: ISSUE.assigneeAgentId!, + requestedByActorType: "agent", + requestedByActorId: "delegating-agent", + queuedCommentIds: scenario === "done_untracked_comment" ? [] : commentIds, + deferredCommentIds: commentIds, + payload: { resumeIntent: scenario !== "done_no_resume", followUpRequested: true }, + deferredContextSeed: { resumeIntent: scenario !== "done_no_resume", followUpRequested: true, wakeCommentIds: commentIds }, + })]; + const transaction = createFakeTransaction({ + findNextDeferredWake: vi.fn(async () => queue.shift() ?? null), + getQueuedCommentLiveness: vi.fn(async () => ({ + liveNonSelfCommentIds: scenario === "done_missing" || scenario === "done_self" ? [] : commentIds, + containedSelfAuthoredComment: scenario === "done_self", + })), + getCommentSelfAuthorship: vi.fn(async () => ({ allSelfAuthored: scenario === "done_self" })), + reopenIssue: vi.fn(async () => ({ ...ISSUE, status: "todo" })), + }); + const release = createReleaseIssueExecution({ + issueLock: createFakeIssueLock(createFakeHost(), transaction, { ...ISSUE, status: scenario === "cancelled_live" ? "cancelled" : "done" }), + recovery: createFakeRecovery(), + }); + + const result = await release({ companyId: RUN.companyId, runId: RUN.id, now: new Date() }); + + if (scenario === "done_live") { + expect(transaction.cancelDeferredWake).not.toHaveBeenCalled(); + expect(transaction.reopenIssue).toHaveBeenCalledTimes(1); + expect(transaction.finalizePromotedWake).toHaveBeenCalledTimes(1); + expect(result.outcome.kind).toBe("promoted"); + } else { + expect(transaction.cancelDeferredWake).toHaveBeenCalledTimes(1); + expect(transaction.reopenIssue).not.toHaveBeenCalled(); + expect(transaction.finalizePromotedWake).not.toHaveBeenCalled(); + expect(result.outcome.kind).toBe("released"); + } + }); + it("never reopens the issue when the promotion claim loses the race, and moves on to the next wake", async () => { const doneIssue: IssueSnapshot = { ...ISSUE, status: "done" }; const queue = [ diff --git a/server/src/modules/wake-queue/application/use-cases.ts b/server/src/modules/wake-queue/application/use-cases.ts index b18377148d7..9341f3b5a15 100644 --- a/server/src/modules/wake-queue/application/use-cases.ts +++ b/server/src/modules/wake-queue/application/use-cases.ts @@ -167,6 +167,34 @@ async function runReleaseDrain( } processedWakeIds.add(candidate.id); + // A lead can post its closing comment before committing Done. Check + // again when the source run releases its queue, using every original + // comment ID so coalesced human or unrelated input is never hidden. + if ( + issue.status === "done" && + run.agentId === issue.assigneeAgentId && + run.contextSnapshot?.issueId === issue.id && + !candidate.authorizedFailedChatRetry && + !candidate.preservesIndependentContinuation && + candidate.payload.mutation !== "interaction" && + (candidate.wakeReason ?? candidate.reason) === "issue_comment_mentioned" && + await ports.transaction.isCompletedDelegationMention({ + companyId: run.companyId, + issueId: issue.id, + finishingRunId: run.id, + wakeAgentId: candidate.agentId, + commentIds: [...new Set([...candidate.queuedCommentIds, ...candidate.deferredCommentIds])], + }) + ) { + await ports.transaction.cancelDeferredWake({ + companyId: run.companyId, + wakeId: candidate.id, + reason: "Delegation closing note refers to completed child work", + now: input.now, + }); + continue; + } + const ordinaryTaskComment = !candidate.authorizedFailedChatRetry && candidate.payload.mutation !== "interaction" && !candidate.preservesIndependentContinuation && candidate.queuedCommentIds.length > 0 && ["issue_commented", "issue_reopened_via_comment"].includes(candidate.wakeReason ?? candidate.reason ?? ""); @@ -314,12 +342,18 @@ async function promoteDeferredWake( shouldReopen = !selfAuthorship.allSelfAuthored && (workingCandidate.requestedByActorType === "user" || - workingCandidate.wakeReason === "issue_reopened_via_comment"); + workingCandidate.wakeReason === "issue_reopened_via_comment" || + (currentIssue.status === "done" && + workingCandidate.agentId === currentIssue.assigneeAgentId && + workingCandidate.requestedByActorType === "agent" && + workingCandidate.deferredContextSeed.resumeIntent === true && + workingCandidate.queuedCommentIds.length > 0)); } - // Agent continuations can outlive the work they addressed. Only a human - // reopen can revive assignee execution; other agents may still receive - // notifications about the closed task. Cancel before claiming promotion so + // Agent continuations can outlive the work they addressed. Live, + // non-self feedback with an explicit resume intent must still be read + // after completion; it cannot revive a cancelled task. Other stale + // continuations cannot revive assignee execution. Cancel before claiming promotion so // the compare-and-set still sees the deferred wake. if ( !shouldReopen && diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 1535317cafe..716b334588a 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -1,7 +1,7 @@ import { deliverConversationComments, isConversation } from "../services/agent-conversations.js"; import { issueRecoveryActionReadModel } from "../services/issue-recovery-actions.js"; import { getExecutionBlocker } from "../services/execution-blocker.js"; -import { requiresExecutionReconciliation } from "@paperclipai/shared"; +import { extractIssueReferenceIdentifiers, requiresExecutionReconciliation } from "@paperclipai/shared"; import { validateExecutionReconciliation, markExecutionReconciliation, @@ -3677,6 +3677,132 @@ export function issueRoutes( return memoizeIssueRead(req, id, () => svc.getById(id)); } + async function routeDelegationMention( + req: Request, + parent: { id: string; identifier?: string | null; companyId: string; status: string }, + mentionedAgentId: string, + comment: IssueComment, + ): Promise<{ kind: "completed" } | { kind: "forwarded"; issueId: string; commentId: string } | null> { + if (parent.status !== "blocked" && parent.status !== "done") return null; + const references = new Set(extractIssueReferenceIdentifiers(comment.body)); + if (references.size === 0) return null; + let child: Awaited> = null; + let authorizationReason: string | undefined; + try { + const sourceRun = comment.createdByRunId + ? await heartbeat.getRun(comment.createdByRunId) + : null; + // The same lead can be working on another task. Its cross-task + // request is not a progress or closing note from this parent run. + if (sourceRun?.contextSnapshot?.issueId !== parent.id) return null; + // A fast child can finish before the lead records a blocking relation. + // Completion notes use the direct parent-child relationship instead. + const blockers = parent.status === "blocked" + ? (await svc.getRelationSummaries(parent.id)).blockedBy + : []; + for (const identifier of references) { + const blocker = blockers.find((candidate) => + candidate.assigneeAgentId === mentionedAgentId && candidate.identifier === identifier); + const candidate = parent.status === "done" + ? await svc.getByIdentifier(identifier) + : blocker ? await svc.getById(blocker.id) : null; + if ( + !candidate || candidate.companyId !== parent.companyId || + candidate.parentId !== parent.id || candidate.assigneeAgentId !== mentionedAgentId + ) continue; + // Do not choose an arbitrary task when the comment names several. + if (child && child.id !== candidate.id) return null; + child = candidate; + } + if (!child) return null; + if (parent.status === "done" && child.status === "done") { + // A closing note can name its parent, but another task reference + // can be a separate request. Preserve that mention's normal wake. + const childIdentifier = child.identifier; + if ([...references].some((identifier) => + identifier !== childIdentifier && identifier !== parent.identifier + )) return null; + logger.info({ issueId: parent.id, childIssueId: child.id, agentId: mentionedAgentId }, + "skipped completed delegation mention wake"); + return { kind: "completed" }; + } + if (parent.status !== "blocked" || child.status !== "in_progress") return null; + const runId = child.executionRunId ?? child.checkoutRunId; + if (!runId) return null; + const run = await heartbeat.getRun(runId); + if ( + !run || run.companyId !== parent.companyId || run.agentId !== mentionedAgentId || + run.status !== "running" || run.contextSnapshot?.issueId !== child.id + ) return null; + const decision = await decideIssueAccess(req, child, "issue:comment"); + if (!decision.allowed) return null; + // Accepted feedback can need another child turn after this one + // finishes, so the author must also be allowed to resume its task. + if (!(await decideIssueAccess(req, child, "issue:mutate")).allowed) return null; + authorizationReason = decision.reason; + } catch (err) { + // A failed optimization must not drop an otherwise valid mention. + logger.warn({ err, issueId: parent.id }, "could not check delegation for mention"); + return null; + } + + let forwarded: IssueComment; + try { + const parentRef = parent.identifier ?? parent.id; + forwarded = await svc.addComment( + child.id, + `Forwarded from [${parentRef}](/issues/${parentRef}#comment-${comment.id}):\n\n${comment.body}`, + { + agentId: comment.authorAgentId ?? undefined, + userId: comment.authorUserId ?? undefined, + runId: comment.createdByRunId, + onBehalfOfUserId: comment.onBehalfOfUserId, + }, + { authorType: comment.authorType, sourceTrust: comment.sourceTrust, authorizationReason }, + ); + } catch (err) { + logger.warn({ err, issueId: parent.id, childIssueId: child.id }, "could not forward delegation mention"); + return null; + } + // Heartbeat reads wake comments within the target issue. A linked copy + // preserves the full feedback and its trust/author provenance in that + // scope. Do not parse its mentions again: the child wake below is the + // only dispatch, and the existing issue queue serializes its delivery. + try { + await issueReferencesSvc.syncComment(forwarded.id); + } catch (err) { + logger.warn({ err, issueId: child.id, commentId: forwarded.id }, "could not index forwarded delegation comment"); + } + try { + const actor = getActorInfo(req); + await logActivity(db, { + companyId: parent.companyId, + actorType: actor.actorType, + actorId: actor.actorId, + agentId: actor.agentId, + runId: actor.runId, + agentApiKeyId: actor.agentApiKeyId, + action: "issue.comment_added", + entityType: "issue", + entityId: child.id, + details: { + commentId: forwarded.id, + identifier: child.identifier, + issueTitle: child.title, + source: "comment.mention.delegation", + sourceIssueId: parent.id, + sourceCommentId: comment.id, + authorizationReason, + }, + }); + } catch (err) { + // The copy is already durable. Keep its child-scoped wake even if + // reference indexing or the activity publication needs recovery. + logger.warn({ err, issueId: child.id, commentId: forwarded.id }, "could not publish forwarded delegation comment activity"); + } + return { kind: "forwarded", issueId: child.id, commentId: forwarded.id }; + } + const issueDetailEtag = privateJsonEtag(); router.use((req, res, next) => { if (/^\/issues\/[^/]+(?:\/|$)/.test(req.path)) { @@ -14549,20 +14675,29 @@ export function issueRoutes( (commentIsFromAssigneeRun && mentionedId === assigneeId) ) continue; + const delegation = commentIsFromAssigneeRun + ? await routeDelegationMention(req, issue, mentionedId, comment) + : null; + if (delegation?.kind === "completed") continue; + const wakeIssueId = delegation?.issueId ?? id; + const wakeCommentId = delegation?.commentId ?? comment.id; addWakeup(mentionedId, { source: "automation", triggerDetail: "system", reason: "issue_comment_mentioned", - payload: { issueId: id, commentId: comment.id }, + payload: { issueId: wakeIssueId, commentId: wakeCommentId, + ...(delegation ? { resumeIntent: true, followUpRequested: true } : {}), + }, requestedByActorType: actor.actorType, requestedByActorId: actor.actorId, contextSnapshot: { - issueId: id, - taskId: id, - commentId: comment.id, - wakeCommentId: comment.id, + issueId: wakeIssueId, + taskId: wakeIssueId, + commentId: wakeCommentId, + wakeCommentId, wakeReason: "issue_comment_mentioned", - source: "comment.mention", + source: delegation ? "comment.mention.delegation" : "comment.mention", + ...(delegation ? { resumeIntent: true, followUpRequested: true } : {}), }, }); } @@ -17953,20 +18088,29 @@ export function issueRoutes( (commentIsFromAssigneeRun && mentionedId === assigneeId) ) continue; + const delegation = commentIsFromAssigneeRun + ? await routeDelegationMention(req, currentIssue, mentionedId, comment) + : null; + if (delegation?.kind === "completed") continue; + const wakeIssueId = delegation?.issueId ?? id; + const wakeCommentId = delegation?.commentId ?? comment.id; addWakeup(mentionedId, { source: "automation", triggerDetail: "system", reason: "issue_comment_mentioned", - payload: { issueId: id, commentId: comment.id }, + payload: { issueId: wakeIssueId, commentId: wakeCommentId, + ...(delegation ? { resumeIntent: true, followUpRequested: true } : {}), + }, requestedByActorType: actor.actorType, requestedByActorId: actor.actorId, contextSnapshot: { - issueId: id, - taskId: id, - commentId: comment.id, - wakeCommentId: comment.id, + issueId: wakeIssueId, + taskId: wakeIssueId, + commentId: wakeCommentId, + wakeCommentId, wakeReason: "issue_comment_mentioned", - source: "comment.mention", + source: delegation ? "comment.mention.delegation" : "comment.mention", + ...(delegation ? { resumeIntent: true, followUpRequested: true } : {}), }, }); } diff --git a/ui/src/components/issue-properties/IssuePropertiesMarkdownWorkProduct.test.tsx b/ui/src/components/issue-properties/IssuePropertiesMarkdownWorkProduct.test.tsx index bbb59046882..83f02963ece 100644 --- a/ui/src/components/issue-properties/IssuePropertiesMarkdownWorkProduct.test.tsx +++ b/ui/src/components/issue-properties/IssuePropertiesMarkdownWorkProduct.test.tsx @@ -327,7 +327,7 @@ describe("markdown work product review row", () => { expect(looseLink?.getAttribute("href")).toBe("/api/attachments/22222222-2222-4222-8222-222222222222/content"); }); - it("keeps non-markdown work products on the raw link row", async () => { + it("keeps non-markdown work products on the download row", async () => { const contentPath = `/api/attachments/${ATTACHMENT_ID}/content`; mockIssuesApi.listWorkProducts.mockResolvedValue([ makeMarkdownWorkProduct({ @@ -348,7 +348,8 @@ describe("markdown work product review row", () => { const row = container.querySelector('[data-testid="task-chat-rich-work-product-artifact"]'); const link = row?.querySelector("a"); expect(row?.textContent).toContain("Verification report"); - expect(link?.getAttribute("href")).toBe(contentPath); + expect(link?.getAttribute("aria-label")).toBe("Download: Verification report"); + expect(link?.getAttribute("href")).toBe(`${contentPath}?download=1`); }); expect(container.querySelector("button[aria-expanded]")).toBeNull(); }); diff --git a/ui/src/components/new-agent/NewAgentSetup.tsx b/ui/src/components/new-agent/NewAgentSetup.tsx index 6ca7939772e..80a65dfdae7 100644 --- a/ui/src/components/new-agent/NewAgentSetup.tsx +++ b/ui/src/components/new-agent/NewAgentSetup.tsx @@ -808,8 +808,21 @@ function Setup({

Runtime

- {aiProviderForAdapter(brandType) && { setRuntimeAiBinding(binding); resetTest(); }} />} + {aiProviderForAdapter(brandType) && ( + connection && !aiBinding ? ( +
+

+ Using the connection selected in the Connect step. +

+ +
+ ) : ( + { setRuntimeAiBinding(binding); resetTest(); }} /> + ) + )} {models.error &&

Could not load models. Retry or enter a model ID manually.

} {((showModel && !usingKimiApi) || efforts.length > 0) && ( diff --git a/ui/src/components/task-chat/RichWorkProductCard.tsx b/ui/src/components/task-chat/RichWorkProductCard.tsx index 9780c214b6b..b435c2529bb 100644 --- a/ui/src/components/task-chat/RichWorkProductCard.tsx +++ b/ui/src/components/task-chat/RichWorkProductCard.tsx @@ -2,6 +2,7 @@ import { useContext, useState, type CSSProperties } from "react"; import { IssueGalleryContext } from "@/context/IssueGalleryContext"; import { ImageGalleryModal } from "@/components/ImageGalleryModal"; import { isImageContentType, isVideoLikeOutput } from "@/lib/issue-output"; +import { attachmentDownloadPath } from "@/lib/issue-attachments"; import type { IssueWorkProduct } from "@paperclipai/shared"; import { ExternalLink, @@ -162,7 +163,7 @@ export function RichWorkProductCard({ workProduct, href, variant = "card" }: Ric Icon = isImage ? Image : isVideo ? Film : File; const size = numberMeta(metadata, "byteSize", "size"); meta = [isImage ? "Image" : isVideo ? "Video" : stringMeta(metadata, "kind", "fileType") ?? "File", size === null ? null : formatBytes(size)]; - action = isImage || isVideo ? "Open gallery" : "Open preview"; + action = isImage || isVideo ? "Open gallery" : "Download"; break; } case "document": @@ -220,6 +221,13 @@ export function RichWorkProductCard({ workProduct, href, variant = "card" }: Ric const mediaPath = workProduct.type === "artifact" && (isImage || isVideo) ? stringMeta(metadata, "contentPath", "openPath") ?? href : null; + const artifactContentPath = stringMeta(metadata, "contentPath") + ?? (href && /^\/api\/attachments\/[^/?#]+\/content$/.test(href) ? href : null); + const actionHref = workProduct.type === "artifact" && !isImage && !isVideo + ? stringMeta(metadata, "downloadPath") ?? (artifactContentPath + ? attachmentDownloadPath({ contentPath: artifactContentPath }) + : href) + : href; const openGallery = () => { if (mediaPath && !openIssueGallery?.(mediaPath)) setGalleryOpen(true); }; @@ -254,8 +262,8 @@ export function RichWorkProductCard({ workProduct, href, variant = "card" }: Ric - ) : href ? ( - + ) : actionHref ? ( + {compact ? null : {action}} ) : null} diff --git a/ui/src/components/task-chat/TaskChatProtocolCard.test.tsx b/ui/src/components/task-chat/TaskChatProtocolCard.test.tsx index b268f09e475..9167dc05c48 100644 --- a/ui/src/components/task-chat/TaskChatProtocolCard.test.tsx +++ b/ui/src/components/task-chat/TaskChatProtocolCard.test.tsx @@ -201,6 +201,42 @@ describe("TaskChatProtocolCard", () => { expect(document.querySelector('[role="dialog"]')).toBeNull(); }); + it.each(["text/html", "application/zip"])("labels %s artifact links as downloads", (contentType) => { + const contentPath = "/api/attachments/file/content"; + flushSync(() => root.render( + , + )); + expect(container.textContent).toContain("Download"); + expect(container.textContent).not.toContain("Open preview"); + expect(container.querySelector("a")?.getAttribute("href")).toBe(`${contentPath}?download=1`); + }); + + it.each(["application/pdf", "text/plain", "text/markdown", "application/json", "text/csv"])("downloads %s artifacts through the download route", (contentType) => { + const contentPath = "/api/attachments/file/content"; + const downloadPath = `${contentPath}?download=1`; + flushSync(() => root.render( + , + )); + expect(container.querySelector('a[aria-label^="Download:"]')?.getAttribute("href")).toBe(downloadPath); + }); + + it("downloads a legacy attachment with only a content href", () => { + const contentPath = "/api/attachments/legacy-file/content"; + flushSync(() => root.render( + , + )); + expect(container.querySelector('a[aria-label^="Download:"]')?.getAttribute("href")).toBe(`${contentPath}?download=1`); + }); + it("opens standalone artifact media in a modal with a download", async () => { const contentPath = "/api/attachments/media/content"; flushSync(() => root.render(