diff --git a/doc/connections/GITHUB-REVIEW-BOT.md b/doc/connections/GITHUB-REVIEW-BOT.md index f63732c56a4..c3c2f036108 100644 --- a/doc/connections/GITHUB-REVIEW-BOT.md +++ b/doc/connections/GITHUB-REVIEW-BOT.md @@ -169,6 +169,19 @@ permissions. Ordinary discussion does not change a review score. Repeat review mentions and pushes continue the existing task; inline replies return to the task owning that thread. +A successful native execution can still be resolving its visible response. +Without a confirmed reply for that run, maintenance waits for its committed +presentation decision before choosing a completion fallback. It then uses the same saved request to settle the wait, +publish a genuine missing-response notice, or report a failed turn. + +A queued or running follow-up acknowledgement preserves an existing App-owned +final PR summary. A new validated assessment can replace it through the review +publication path. When an authorized native turn commits a passive response +wait with no new comment, maintenance settles that request's acknowledgement +without replacing the assessment with a generic completion notice. This does +not count the old summary as a reply from the new run. Missing or failed +responses still receive their normal fallback; uncertain writes remain held. + Event prompts supplement the agent's instructions. Repository content and PR prose are untrusted input and cannot change tool authority or publication policy. The execution records the configuration revision and event context used. @@ -249,9 +262,15 @@ the receipt. Receipt recovery compares the complete saved publication request, including long text and nested coverage entries. Shortened audit previews do not establish -request identity. Changed content or a missing durable publication returns a -conflict without dispatching another write. Exact retries read the current -receipt; the durable worker owns pending publication retries. +request identity. Changed content or a missing completed durable publication +returns a conflict without dispatching another write. Exact retries read the +current receipt; the durable worker owns pending publication retries. + +A timeout during metadata or file reads can occur before publication staging. +The same request can repeat this preflight with its complete request identity. +The saved assessment, unique publication key, and provider lease still prevent +repeat external writes. `begin_review` retries return the run's current review +result. They do not return a shortened audit preview. For a call that required human approval, the durable publication keeps that verified approval for its exact operation. An exact caller retry only reads the diff --git a/doc/execution-semantics.md b/doc/execution-semantics.md index 17411564865..fe985686b95 100644 --- a/doc/execution-semantics.md +++ b/doc/execution-semantics.md @@ -1258,6 +1258,22 @@ apply. Stream closure without a turn terminal is not proof of success. Event replay uses the existing source receipts and never repeats provider work merely to recover recorded output. +Native execution success can precede its durable presentation decision. GitHub +response maintenance without a confirmed current-run reply holds that request +until the native phase is committed and a valid presentation decision exists. +A confirmed current-run receipt can retire its placeholder. The same maintenance owner +then rechecks the settled outcome; elapsed time cannot turn undecided output +into a generic failure notice. Failed execution retains normal error fallback. + +GitHub main-PR response maintenance honors a committed native passive response wait +whose server presentation decision intentionally selects no comment. It settles +the current source's acknowledgement without a provider write or a fabricated +current-run reply receipt. Queued/running acknowledgements preserve an App-owned +final shared PR summary; a new canonical assessment may replace it. A generic +completion fallback must not overwrite that summary after an intentional wait. +Failure, genuinely missing response, and uncertain-publication handling retain +their existing independent checks. + If runnerd synthesizes a result when the provider stops, it publishes that result before the provider-turn terminal and publishes the run terminal last. The adapter can therefore retain the result while the matching turn still has diff --git a/maps/agent-access-map/chat-channels.md b/maps/agent-access-map/chat-channels.md index aaa0b34679b..b2d708898b3 100644 --- a/maps/agent-access-map/chat-channels.md +++ b/maps/agent-access-map/chat-channels.md @@ -115,8 +115,13 @@ GitHub publication recovery (2026-10-14): assigned-agent `submit_review`, `comme and `update_comment` calls reuse their run-, task-, endpoint-, and argument-bound publication on an exact retry. Identity uses the complete durable request rather than the shortened audit preview. A changed suffix, changed nested entry, or -missing publication cannot authorize another write. Exact retries only read the -receipt; the worker owns pending provider retries. Fresh gateway policy and provider authority remain +missing completed publication cannot authorize another write. Exact retries read an +existing receipt; the worker owns pending provider retries. A request interrupted +during metadata or file reads before staging can repeat that preflight with the +same full request identity. The immutable assessment, unique publication and +provider lease still serialize its external write. `begin_review` exact retries +resolve the run's current review and return a typed result instead of an audit +preview. Fresh gateway policy and provider authority remain required. The durable worker resumes transient failures after restart and stops at its retry cap, revoked authority, or a superseded head. A pending receipt is not task completion. Source: @@ -134,6 +139,15 @@ follow normal access checks. Provider-fixture coverage is not live GitHub proof. Native terminal reconciliation resolves the accepted run identity through the existing chat provenance resolver; an older run's reply cannot complete a newer request merely because both requests share the same PR comment. +A terminal native success does not authorize missing-response publication while +its native phase or durable presentation decision remains pending. Maintenance +rechecks the same request after authoritative settlement; failures keep their +separate fallback path. +Maintenance projects the bounded server-committed native passive-wait decision +only after current request ownership. It settles a no-comment acknowledgement +without authorizing a new send or counting an old shared receipt as a current-run +reply. Existing final App/marker-owned PR content survives progress acknowledgements; +new canonical review publication remains the writer for replacement assessments. Speko browser sessions use `POST /api/companies/{companyId}/voice-sessions`, with GET inspection/notification and POST `/{sessionId}/end` operations below diff --git a/maps/feature-map/chat-channels.md b/maps/feature-map/chat-channels.md index a5a490d0a33..087993e8536 100644 --- a/maps/feature-map/chat-channels.md +++ b/maps/feature-map/chat-channels.md @@ -140,6 +140,18 @@ contracts with synthetic providers; record a live journey separately. For a long-lived task, repeat more than 100 continuations and check that tool access remains available. Fail a later GitHub run after a final answer and check that the earlier answer remains intact. +After publishing a PR assessment, deliver an ordinary no-new-work author reply. +Hold native presentation after execution succeeds. Verify maintenance makes no +provider write while undecided, including repeated passes and the explicit +fallback path. Commit either a passive wait, genuine missing response, or failure +and verify that the same request settles once with the corresponding behavior. +Verify the actual shared summary stays unchanged during queued/running +acknowledgements and after the committed passive wait, including maintenance, +restart and cleanup. Confirm the per-request acknowledgement is terminal and a +second maintenance pass makes no write. Then request a real same-head and +changed-head assessment and confirm canonical publication can replace it. +Real-PostgreSQL provider fixtures cover this producer and missing/error/unknown +write safety; record hosted destination acceptance separately. ### `endpoint-management` @@ -174,7 +186,12 @@ write and verify one summary, finding, and App-scoped check. Retry the same tool call and verify it returns the saved receipt. Include text longer than 500 characters and coverage with more than 50 entries. Change only the text suffix or final coverage entry and verify rejection without another write. Remove the -durable publication and verify that a retry cannot dispatch a new write. +completed durable publication and verify that a retry cannot dispatch a new write. +Also interrupt metadata reads before publication staging, repeat the exact +`begin_review` and `submit_review` requests, and verify typed results and one +assessment publication. Let the original metadata read finish alongside its +retry and verify that both converge on the same publication. Changed requests +and revoked access must still stop preflight recovery. Leave a transient failure queued, restart the instance, and verify publication resumes at its saved deadline without another inbound message. Revoked access and permanent rejections must diff --git a/server/src/__tests__/chat-channels.integration.test.ts b/server/src/__tests__/chat-channels.integration.test.ts index f7b82550879..804405037a2 100644 --- a/server/src/__tests__/chat-channels.integration.test.ts +++ b/server/src/__tests__/chat-channels.integration.test.ts @@ -2830,7 +2830,138 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { )))[0]; return { ...f, service, head, assessment, provider, action, inline, checks, writes }; } - it("reuses one main GitHub PR comment across review requests", async () => { + it("preserves the published GitHub assessment during passive-response maintenance", async () => { + const f = await publicationRetryFixture(); + await f.service.execute(f.session, "submit_review", f.assessment, "published-before-passive-reply"); + await db.update(heartbeatRuns).set({ status: "succeeded" }).where(eq(heartbeatRuns.id, f.run.id)); + const reviewBefore = await readChatGitHubReviewsByEndpointId(f.endpoint.id); + const originalId = [...f.responseComments.keys()][0]; + const originalBody = f.responseComments.get(originalId)!.body; + const writesBefore = f.responseWrites.length; + const responses = githubResponseCommentService(db, f.providerFetch); + const thread = makeThread({ channelId: "paperclipai/paperclip", id: f.conversation.externalThreadId }).thread; + await deliverMessage({ callbacks: f.callbacks, endpointId: f.endpoint.id, provider: "github", thread, + trigger: "subscribed_message", message: makeMessage({ id: "41802", text: "No fix pushed; keep the existing review unchanged and wait for my reply.", userId: "42", userName: "octocat" }) }); + await responses.processPending(); + expect(f.responseComments.get(originalId)!.body).toBe(originalBody); + expect(f.responseWrites).toHaveLength(writesBefore); + const [next] = await db.insert(heartbeatRuns).values({ companyId: f.companyId, agentId: f.assignedAgentId, + status: "running", runtimeMode: "native", contextSnapshot: await chatWakeContext({ + endpointId: f.endpoint.id, issueId: f.conversation.issueId, provider: "github", providerMessageId: "41802", + }), resultJson: { finalizationPhase: "committed", finalizationReasonCode: "external_chat_response_waiting", + nativeResult: { schema: "paperclip.run_result.v1", reportedWorkDisposition: "yielded", + continuation: { kind: "response_wake", idempotencyKey: "passive-author-reply" } }, + presentationDecision: { schema: "paperclip.run_presentation_decision.v1", resolverVersion: "2", + activityDisposition: "collapse", chosenSource: "none", commentAction: "none", commentId: null, + reasonCodes: ["yielded_control_plane_wait"] } } }).returning(); + await db.update(issues).set({ status: "in_review" }).where(eq(issues.id, f.conversation.issueId!)); + await responses.processPending(); + expect(f.responseComments.get(originalId)!.body).toBe(originalBody); + expect(f.responseWrites).toHaveLength(writesBefore); + await db.update(heartbeatRuns).set({ status: "succeeded", nativePhase: "committed" }).where(eq(heartbeatRuns.id, next.id)); + await responses.processPending(); + await responses.processPending(); + expect(f.responseComments.get(originalId)!.body).toBe(originalBody); + expect(await readChatGitHubReviewsByEndpointId(f.endpoint.id)).toEqual(reviewBefore); + expect([...f.responseComments.keys()]).toEqual([originalId]); + expect(await githubRunReplyState(db, { ...f.session, runId: next.id, endpointId: f.endpoint.id })).toBe("none"); + const [delivery] = await db.select().from(chatDeliveries).where(and(eq(chatDeliveries.endpointId, f.endpoint.id), + eq(chatDeliveries.providerEventId, `${thread.id}:41802`))); + const [response] = await db.select().from(chatActions).where(and(eq(chatActions.deliveryId, delivery.id), + eq(chatActions.kind, "github_response_comment"))); + expect(response).toMatchObject({ status: "cancelled", result: { final: true, code: "passive_response_wait" } }); + const writes = f.responseWrites.length; + const updatedAt = response.updatedAt; + await responses.processPending(); + expect(f.responseWrites).toHaveLength(writes); + expect((await readChatActionsById(response.id))[0].updatedAt).toEqual(updatedAt); + await expect(responses.failRun({ ...f.session, runId: next.id, endpointId: f.endpoint.id, + body: "This turn ended without publishing a final response." })).rejects.toMatchObject({ status: 403 }); + expect(f.responseWrites).toHaveLength(writes); + }); + it.each(["completed", "failed", "missing_decision", "comment_requested", "missing_semantic", "legacy", "uncommitted_native", "pending_publication", "missing_decision_then_missing", "uncommitted_then_failed", "malformed_null", "malformed_scalar", "malformed_schema", "malformed_action"] as const)( + "keeps GitHub response maintenance truthful for a new request: %s", async scenario => { + const pendingPresentation = scenario.startsWith("missing_decision") || scenario.startsWith("uncommitted") || scenario.startsWith("malformed"); + const f = await publicationRetryFixture(); + await f.service.execute(f.session, "submit_review", f.assessment, "review-before-new-request"); + await db.update(heartbeatRuns).set({ status: "succeeded" }).where(eq(heartbeatRuns.id, f.run.id)); + const originalId = [...f.responseComments.keys()][0]; + const originalBody = f.responseComments.get(originalId)!.body; + const thread = makeThread({ channelId: "paperclipai/paperclip", id: f.conversation.externalThreadId }).thread; + await deliverMessage({ callbacks: f.callbacks, endpointId: f.endpoint.id, provider: "github", thread, + trigger: "subscribed_message", message: makeMessage({ id: "41802", text: "Please handle this new request", userId: "42", userName: "octocat" }) }); + const [next] = await db.insert(heartbeatRuns).values({ companyId: f.companyId, agentId: f.assignedAgentId, + status: scenario === "failed" ? "failed" : "succeeded", runtimeMode: scenario === "legacy" ? "legacy" : "native", + nativePhase: scenario.startsWith("uncommitted") ? "observed" : "committed", contextSnapshot: await chatWakeContext({ + endpointId: f.endpoint.id, issueId: f.conversation.issueId, provider: "github", providerMessageId: "41802", + }), resultJson: { finalizationPhase: "committed", finalizationReasonCode: "external_chat_response_waiting", + ...(scenario === "missing_semantic" ? {} : { nativeResult: { schema: "paperclip.run_result.v1", + reportedWorkDisposition: scenario === "completed" ? "completed" : "yielded", + continuation: { kind: "response_wake", idempotencyKey: "new-request-wait" } } }), + ...(scenario.startsWith("missing_decision") ? {} : { presentationDecision: scenario === "malformed_null" ? null : scenario === "malformed_scalar" ? "pending" : { + schema: scenario === "malformed_schema" ? "unknown" : "paperclip.run_presentation_decision.v1", chosenSource: "none", + commentAction: scenario === "malformed_action" ? "unknown" : scenario === "comment_requested" ? "create" : "none", reasonCodes: ["yielded_control_plane_wait"] } }) } }).returning(); + if (scenario === "pending_publication") await db.insert(chatActions).values({ companyId: f.companyId, + endpointId: f.endpoint.id, conversationId: f.conversation.id, kind: "github_review_publication", + providerActionId: `github_publication:${randomUUID()}`, status: "processing", + payload: { operation: "comment", session: { ...f.session, runId: next.id } } }); + const responses = githubResponseCommentService(db, f.providerFetch); + await responses.processPending(); + const [delivery] = await db.select().from(chatDeliveries).where(and(eq(chatDeliveries.endpointId, f.endpoint.id), + eq(chatDeliveries.providerEventId, `${thread.id}:41802`))); + const [response] = await db.select().from(chatActions).where(and(eq(chatActions.deliveryId, delivery.id), + eq(chatActions.kind, "github_response_comment"))); + if (scenario === "pending_publication") { + expect(f.responseComments.get(originalId)!.body).toBe(originalBody); + expect(response.result?.final).not.toBe(true); + expect(response).toMatchObject({ status: "failed", result: { retryable: true, code: "acknowledgement_unconfirmed" } }); + } else if (pendingPresentation) { + expect(f.responseComments.get(originalId)!.body).toBe(originalBody); + expect(response.result?.final).not.toBe(true); + const pendingWrites = f.responseWrites.length; + await responses.processPending(); + await expect(responses.failRun({ ...f.session, runId: next.id, endpointId: f.endpoint.id, + body: "This turn ended without publishing a final response." })).rejects.toMatchObject({ status: 409 }); + expect(f.responseWrites).toHaveLength(pendingWrites); + const failed = scenario === "uncommitted_then_failed"; + const missing = scenario === "missing_decision_then_missing"; + await db.update(heartbeatRuns).set({ status: failed ? "failed" : "succeeded", + nativePhase: failed ? "failed" : "committed", resultJson: { + ...next.resultJson, finalizationPhase: failed ? "failed" : "committed", + nativeResult: { schema: "paperclip.run_result.v1", reportedWorkDisposition: missing ? "completed" : "yielded", + continuation: missing ? null : { kind: "response_wake", idempotencyKey: "settled-request-wait" } }, + presentationDecision: { schema: "paperclip.run_presentation_decision.v1", chosenSource: "none", + commentAction: "none", reasonCodes: [missing ? "no_final_response" : "yielded_control_plane_wait"] }, + } }).where(eq(heartbeatRuns.id, next.id)); + await responses.processPending(); + if (failed || missing) expect(f.responseComments.get(originalId)!.body).toContain(failed + ? "This turn stopped before completing" : "This turn ended without publishing a final response"); + else expect(f.responseComments.get(originalId)!.body).toBe(originalBody); + expect((await readChatActionsById(response.id))[0]).toMatchObject({ + status: failed || missing ? "processed" : "cancelled", result: { final: true }, + }); + } else { + expect(f.responseComments.get(originalId)!.body).toContain(scenario === "failed" + ? "This turn stopped before completing" : "This turn ended without publishing a final response"); + expect(response).toMatchObject({ status: "processed", result: { final: true } }); + } + const writes = f.responseWrites.length; + await responses.processPending(); + expect(f.responseWrites).toHaveLength(writes); + }); + it("keeps per-request fallback outside the main GitHub PR review wait", async () => { + const f = await toolReplyFixture(); + await db.update(heartbeatRuns).set({ status: "succeeded", nativePhase: "committed", resultJson: { + finalizationPhase: "committed", finalizationReasonCode: "external_chat_response_waiting", + nativeResult: { schema: "paperclip.run_result.v1", reportedWorkDisposition: "yielded", + continuation: { kind: "response_wake", idempotencyKey: "not-a-main-pr-review" } }, + presentationDecision: { schema: "paperclip.run_presentation_decision.v1", chosenSource: "none", + commentAction: "none", reasonCodes: ["yielded_control_plane_wait"] }, + } }).where(eq(heartbeatRuns.id, f.run.id)); + await githubResponseCommentService(db, f.providerFetch).processPending(); + expect([...f.responseComments.values()][0].body).toContain("without publishing a final response"); + }); + it.each(["same_head", "new_head"] as const)("reuses one main GitHub PR comment across review requests: %s", async headCase => { const f = await publicationRetryFixture(); await f.service.execute(f.session, "submit_review", f.assessment, "first-review"); const originalId = [...f.responseComments.keys()][0]; @@ -2848,8 +2979,13 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { endpointId: f.endpoint.id, issueId: f.conversation.issueId, provider: "github", providerMessageId: "41802", }) }).returning(); const session = { ...f.session, runId: next.id }; - await f.service.execute(session, "begin_review", { reviewedCommit: f.head }); - await f.service.execute(session, "submit_review", { ...f.assessment, summary: "Second assessment" }, "second-review"); + const currentHead = headCase === "new_head" ? "c".repeat(40) : f.head; + f.setSupplementalProviderFetch(async (input, init) => String(input).endsWith("/pulls/418") + ? Response.json({ number: 418, state: "open", draft: false, head: { sha: currentHead }, + base: { sha: "a".repeat(40), ref: "master" }, user: { id: 42, login: "octocat", type: "User" }, labels: [] }) + : f.provider(input, init)); + await f.service.execute(session, "begin_review", { reviewedCommit: currentHead }); + await f.service.execute(session, "submit_review", { ...f.assessment, reviewedCommit: currentHead, summary: "Second assessment" }, "second-review"); expect([...f.responseComments.keys()]).toEqual([originalId]); expect(f.responseComments.get(originalId)?.body).toContain("Second assessment"); expect(f.responseWrites.filter(write => write.method === "POST")).toHaveLength(1); @@ -3628,6 +3764,97 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { expect(classifications).toHaveLength(2); expect(classifications.map(c => c.violation)).toEqual([false, true]); expect(classifications.every(c => c.toolInvocationId === approval.invocationId && c.costEventId)).toBe(true); }); + it.each(["begin_review", "submit_review"] as const)("recovers %s after a timeout before publication staging", async name => { + const f = await publicationRetryFixture(); + let release!: () => void; + const blocked = new Promise(resolve => { release = resolve; }); + let metadataReads = 0; + f.setSupplementalProviderFetch(async (input, init) => { + if (String(input).endsWith("/pulls/418") && metadataReads++ === 0) { + await blocked; + throw new Error("Connection closed before PR metadata arrived"); + } + return f.provider(input, init); + }); + vi.stubGlobal("fetch", f.providerFetch); + onTestFinished(() => { release(); vi.unstubAllGlobals(); }); + const gateway = createToolGatewayService(db); + const session = await gateway.createSession({ companyId: f.companyId, agentId: f.assignedAgentId, runId: f.run.id }); + const tool = (await githubBotToolsForSession(db, f.session)).find(tool => tool.upstreamToolName === name)!; + const parameters = name === "begin_review" ? { reviewedCommit: f.head } : f.assessment; + const input = { sessionToken: session.token, tool: tool.name, parameters }; + await expect(gateway.executeTool({ ...input, timeoutMs: 50 })).rejects.toMatchObject({ reasonCode: "tool_timeout" }); + expect(await f.action()).toBeUndefined(); + release(); + const retry = await gateway.executeTool(input); + expect(retry).toMatchObject({ result: { data: name === "begin_review" + ? { reviewedCommit: f.head, state: "running" } + : { status: "processed", score: 3, conclusion: "neutral" } } }); + const writes = f.responseWrites.length; + expect(await gateway.executeTool(input)).toMatchObject({ invocationId: retry.invocationId, + result: { data: name === "begin_review" ? { reviewedCommit: f.head } : { status: "processed" } } }); + expect(f.responseWrites).toHaveLength(writes); + expect(f.responseComments.size).toBe(1); + }); + it("stages one assessment when the timed-out metadata read completes alongside its retry", async () => { + const f = await publicationRetryFixture(); + let release!: () => void; + const blocked = new Promise(resolve => { release = resolve; }); + let metadataReads = 0; + f.setSupplementalProviderFetch(async (input, init) => { + if (String(input).endsWith("/pulls/418") && metadataReads++ === 0) await blocked; + return f.provider(input, init); + }); + vi.stubGlobal("fetch", f.providerFetch); + onTestFinished(() => { release(); vi.unstubAllGlobals(); }); + const gateway = createToolGatewayService(db); + const session = await gateway.createSession({ companyId: f.companyId, agentId: f.assignedAgentId, runId: f.run.id }); + const tool = (await githubBotToolsForSession(db, f.session)).find(tool => tool.upstreamToolName === "submit_review")!; + const input = { sessionToken: session.token, tool: tool.name, parameters: f.assessment }; + await expect(gateway.executeTool({ ...input, timeoutMs: 50 })).rejects.toMatchObject({ reasonCode: "tool_timeout" }); + const retry = gateway.executeTool(input); + release(); + expect(await retry).toMatchObject({ result: { data: { actionId: expect.any(String) } } }); + await expect.poll(async () => (await f.action())?.status).toBe("processed"); + expect(await gateway.executeTool(input)).toMatchObject({ result: { data: { status: "processed" } } }); + const actions = await db.select().from(chatActions).where(and(eq(chatActions.endpointId, f.endpoint.id), + eq(chatActions.kind, "github_review_publication"))); + expect(actions).toHaveLength(1); + expect(actions[0].status).toBe("processed"); + expect(f.responseComments.size).toBe(1); + expect(f.responseWrites.map(write => write.method)).toEqual(["POST", "PATCH"]); + }); + it("rejects changed requests and revoked access before repeating an unstaged review", async () => { + const f = await publicationRetryFixture(); + let metadataReads = 0; + let unavailable = true; + f.setSupplementalProviderFetch(async (input, init) => { + if (String(input).endsWith("/pulls/418")) { + metadataReads++; + if (unavailable) throw new Error("Connection closed before PR metadata arrived"); + } + return f.provider(input, init); + }); + vi.stubGlobal("fetch", f.providerFetch); + onTestFinished(() => vi.unstubAllGlobals()); + const gateway = createToolGatewayService(db); + const session = await gateway.createSession({ companyId: f.companyId, agentId: f.assignedAgentId, runId: f.run.id }); + const tool = (await githubBotToolsForSession(db, f.session)).find(tool => tool.upstreamToolName === "submit_review")!; + const input = { sessionToken: session.token, tool: tool.name, parameters: f.assessment, idempotencyKey: "preflight-request" }; + await expect(gateway.executeTool(input)).rejects.toThrow("GitHub is temporarily unavailable"); + unavailable = false; + const failedReads = metadataReads; + await expect(gateway.executeTool({ ...input, parameters: { ...f.assessment, summary: "Changed assessment" } })) + .rejects.toMatchObject({ reasonCode: "github_publication_unavailable" }); + expect(await f.action()).toBeUndefined(); + expect(metadataReads).toBe(failedReads); + await db.insert(toolPolicies).values({ companyId: f.companyId, name: "Revoke review", policyType: "block", priority: 0, + selectors: { catalogEntryId: tool.catalogEntryId! } }); + await expect(gateway.executeTool(input)).rejects.toThrow(); + expect(await f.action()).toBeUndefined(); + expect(metadataReads).toBe(failedReads); + expect(f.responseWrites.map(write => write.method)).toEqual(["POST"]); + }); it("reads pending GitHub receipts without dispatch and refuses a missing publication", async () => { const f = await publicationRetryFixture(); let attempts = 0; @@ -3785,7 +4012,10 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { const f = await toolReplyFixture(); const [response] = await db.select().from(chatActions).where(and(eq(chatActions.endpointId, f.endpoint.id), eq(chatActions.kind, "github_response_comment"))); await db.update(chatActions).set({ status: "received", result: null }).where(eq(chatActions.id, response.id)); - await db.update(heartbeatRuns).set({ status: "succeeded" }).where(eq(heartbeatRuns.id, f.run.id)); + await db.update(heartbeatRuns).set({ status: "succeeded", nativePhase: "committed", resultJson: { presentationDecision: { + schema: "paperclip.run_presentation_decision.v1", commentAction: "none", chosenSource: "none", + reasonCodes: ["no_final_response"], + } } }).where(eq(heartbeatRuns.id, f.run.id)); await githubResponseCommentService(db, f.providerFetch).processPending(); expect(f.responseComments.size).toBe(1); expect([...f.responseComments.values()][0].body).toContain("without publishing a final response"); @@ -4052,7 +4282,10 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { }); it("does not invent a GitHub completion response when the agent sends no tool reply", async () => { const f = await toolReplyFixture(); - await db.update(heartbeatRuns).set({ status: "succeeded", resultJson: { presentationDecision: { authorizationReason: "internal_agent_write" } } }).where(eq(heartbeatRuns.id, f.run.id)); + await db.update(heartbeatRuns).set({ status: "succeeded", nativePhase: "committed", resultJson: { presentationDecision: { + schema: "paperclip.run_presentation_decision.v1", commentAction: "none", chosenSource: "none", + reasonCodes: ["no_final_response"], authorizationReason: "internal_agent_write", + } } }).where(eq(heartbeatRuns.id, f.run.id)); await expect(resolveChatRunPresentationAuthorizationReason(db, f.session)).resolves.toBe("internal_agent_write"); await enqueueChatRunMilestones(db); await f.service.processPendingPublications(); expect(f.runtime.endpoints.get(f.endpoint.id)?.posts).toEqual([]); diff --git a/server/src/services/chat-github-response-comments.ts b/server/src/services/chat-github-response-comments.ts index c032ade25ba..1c2b7383df2 100644 --- a/server/src/services/chat-github-response-comments.ts +++ b/server/src/services/chat-github-response-comments.ts @@ -35,6 +35,33 @@ const responseRunColumns = { issueId: heartbeatRuns.issueId, status: heartbeatRuns.status, updatedAt: heartbeatRuns.updatedAt, + // Terminal execution can precede native presentation commit. Until its + // durable decision exists, missing output is undecided rather than failure. + nativePresentationPending: sql`coalesce( + ${heartbeatRuns.status} = 'succeeded' + and ${heartbeatRuns.runtimeMode} = 'native' + and (${heartbeatRuns.nativePhase} is distinct from 'committed' + or ${heartbeatRuns.resultJson}->'presentationDecision'->>'schema' is distinct from 'paperclip.run_presentation_decision.v1' + or coalesce(${heartbeatRuns.resultJson}->'presentationDecision'->>'commentAction', '') not in ('none', 'create', 'reuse')), + false + )`, + // Project only the server-committed no-publication decision, never the + // potentially large native result or model-authored summary. + passiveResponseWait: sql`coalesce( + ${heartbeatRuns.status} = 'succeeded' + and ${heartbeatRuns.runtimeMode} = 'native' + and ${heartbeatRuns.nativePhase} = 'committed' + and ${heartbeatRuns.resultJson}->>'finalizationPhase' = 'committed' + and ${heartbeatRuns.resultJson}->>'finalizationReasonCode' = 'external_chat_response_waiting' + and ${heartbeatRuns.resultJson}->'nativeResult'->>'schema' = 'paperclip.run_result.v1' + and ${heartbeatRuns.resultJson}->'nativeResult'->>'reportedWorkDisposition' = 'yielded' + and ${heartbeatRuns.resultJson}->'nativeResult'->'continuation'->>'kind' = 'response_wake' + and ${heartbeatRuns.resultJson}->'presentationDecision'->>'schema' = 'paperclip.run_presentation_decision.v1' + and ${heartbeatRuns.resultJson}->'presentationDecision'->>'chosenSource' = 'none' + and ${heartbeatRuns.resultJson}->'presentationDecision'->>'commentAction' = 'none' + and ${heartbeatRuns.resultJson}->'presentationDecision'->'reasonCodes' @> '["yielded_control_plane_wait"]'::jsonb, + false + )`, contextSnapshot: sql>`jsonb_build_object( 'issueId', ${heartbeatRuns.contextSnapshot}->'issueId', 'taskId', ${heartbeatRuns.contextSnapshot}->'taskId', @@ -130,7 +157,7 @@ export async function writeGitHubResponseComment( const sameRequest = !shared || previous?.body?.includes(requestResponseMarker(source)) === true; const alreadyFinal = sameRequest && previous?.body?.includes("") === true; const body = `${projectSafeChatPublicationText(input.body)}\n\n${marker}${shared ? `\n${requestResponseMarker(source)}` : ""}\n${input.final ? "\n" : ""}`; - const preserved = previous && (previous.body === body || (input.acknowledgement && sameRequest) || (alreadyFinal && !input.final) || + const preserved = previous && (previous.body === body || (input.acknowledgement && (sameRequest || (shared && previous.body?.includes("")))) || (alreadyFinal && !input.final) || (input.preserveFinal && previous.body?.includes(""))) ? previous : null; const posted = preserved && (!shared || preserved.body?.includes(marker)) ? preserved : await api.request( previous ? `${item}/${previous.id}` : source.replyId @@ -145,7 +172,8 @@ export async function writeGitHubResponseComment( const receipt = { id: String(posted.id), url: posted.html_url }; await lease.commit(async tx => { if (shared) await tx.update(chatActions).set({ result: { - ...receipt, final: input.final || alreadyFinal, versionAt: versionAt.toISOString(), deliveryId: source.delivery.id, + ...receipt, final: input.final || alreadyFinal || Boolean(preserved?.body?.includes("")), + versionAt: versionAt.toISOString(), deliveryId: source.delivery.id, }, updatedAt: new Date() }).where(eq(chatActions.id, target.id)); await tx.update(chatActions).set({ status: "processed", result: { ...receipt, final: input.final || alreadyFinal, versionAt: versionAt.toISOString(), @@ -230,6 +258,20 @@ export function githubResponseCommentService(db: Db, fetchImpl = fetch) { companyId: source.endpoint.companyId, endpointId: source.endpoint.id, issueId: source.conversation.issueId!, runId: run.id }) : "none"; if (reply === "unsettled") throw conflict("GitHub reply is still being resolved"); + // A confirmed reply for this exact run can retire its placeholder. + // Otherwise terminal execution is not yet a missing-response decision. + if (ownsLatestRequest && run.nativePresentationPending && reply === "none") return; + if (isMainPullResponse(source) && ownsLatestRequest && run.passiveResponseWait && reply === "none") { + // A committed passive turn intentionally has no new response. + // Settle this request's acknowledgement without editing the shared + // PR assessment or inventing a current-run publication receipt. + await lease.commit(async tx => { await tx.update(chatActions).set({ + status: "cancelled", result: { ...existing.result, final: true, code: "passive_response_wait" }, + updatedAt: new Date(), + }).where(and(eq(chatActions.id, existing.id), eq(chatActions.companyId, source.endpoint.companyId), + sql`coalesce(${chatActions.result}->>'final', 'false') <> 'true'`)); }); + return; + } if (existing.result?.final === true) return existing.result.id ? { id: String(existing.result.id), url: String(existing.result.url) } : undefined; preserveFinal = reply === "confirmed"; onlyExisting = reply === "confirmed" && !existing.result?.id; @@ -374,6 +416,8 @@ export function githubResponseCommentService(db: Db, fetchImpl = fetch) { )).orderBy(desc(heartbeatRuns.createdAt), desc(heartbeatRuns.id)).limit(1); if (latest?.id !== run.id) throw forbidden("A newer GitHub run owns the response"); if ((await responseForRequest())?.result?.final === true) throw forbidden("GitHub request already has a final response"); + if (current.nativePresentationPending) throw conflict("GitHub native response presentation is still being resolved"); + if (isMainPullResponse(source) && current.passiveResponseWait) throw forbidden("GitHub run intentionally yielded without a new response"); if (await githubRunReplyState(db, input) !== "none") throw conflict("GitHub reply is still being resolved"); }; receipt = await acknowledge(link.deliveryId, input.body, assertRun); diff --git a/server/src/services/tool-gateway.ts b/server/src/services/tool-gateway.ts index f5971156148..68abd9e014f 100644 --- a/server/src/services/tool-gateway.ts +++ b/server/src/services/tool-gateway.ts @@ -10808,7 +10808,7 @@ export function createToolGatewayService( : false; const gitHubPublicationReplay = recorded.replayed && tool.providerType === "paperclip_github_chat" && - ["comment", "update_comment", "submit_review"].includes(tool.upstreamToolName ?? "") && + ["begin_review", "comment", "update_comment", "submit_review"].includes(tool.upstreamToolName ?? "") && recorded.invocation.approvalState === "not_required"; const gitHubReplayPolicyAllowed = accessDecision.allowed; const gitHubReplayBindingMatches = @@ -10833,9 +10833,16 @@ export function createToolGatewayService( throw new ToolGatewayHttpError(409, error.message, "github_idempotency_conflict"); throw error; } - if (!receipt) - throw new ToolGatewayHttpError(409, "The saved GitHub publication is not available yet", "github_publication_unavailable"); - recoveredGitHubPublication = { content: JSON.stringify(receipt), data: receipt }; + if (receipt) { + recoveredGitHubPublication = { content: JSON.stringify(receipt), data: receipt }; + } else { + const canRepeatPreflight = tool.upstreamToolName === "begin_review" || + (recorded.invocation.status !== "succeeded" && !recorded.invocation.resultHash); + const exactRequest = recorded.invocation.argumentsHash === + toolRequestIdentityHash(effectiveParameters, options.toolActionSigningSecret); + if (!canRepeatPreflight || !exactRequest) + throw new ToolGatewayHttpError(409, "The saved GitHub publication is not available yet", "github_publication_unavailable"); + } } if (recorded.replayed && !retryingSlackRateLimit && !retryingGitHubPublication) { if (gitHubPublicationReplay && !gitHubReplayPolicyAllowed)