mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-28 04:02:18 +01:00
fix(github): recover review replay and preserve passive summaries (#17726)
<!-- entire-trail-link-start --> https://entire.com/gh/paperclipai/-/trails/8/changes/paperclip/8 <!-- entire-trail-link-end --> ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - GitHub tools save review assessments and publish their confirmed receipts. > - A timeout before publication staging can leave an invocation with no saved publication. > - Exact replay must repeat only the authorized preflight and converge on one publication. > - PR requests also share an App-owned summary comment. Progress and maintenance must preserve its final content. > - A native turn can intentionally wait for an author without sending another reply. > - This pull request repairs unstaged replay and prevents queued or passive-wait notices from replacing a published assessment. ## Linked Issues or Issue Description Related repairs: Refs #16883, Refs #16284, Refs #16275 recover saved publication receipts. Refs #17183 covers unavailable approved replay output. Refs #17714 fixes a separate publication lock-order problem. **What happened?** Exact review retries can return null `begin_review` output or `github_publication_unavailable` after an upstream read times out before publication staging. Separately, response maintenance can replace a completed PR assessment with “Review queued…” or a generic missing-response notice after an authorized no-new-work reply. The latter run can correctly commit a passive wait while its external summary is still overwritten. **Expected behavior** Exact retries repeat only the authorized preflight. A saved or uncertain publication keeps its existing recovery owner. Queued acknowledgements preserve final App-owned summary content. A committed native passive wait settles its current acknowledgement without writing a response or treating an old summary as a new-run reply. Success before presentation remains held until a durable decision exists; exact current-run confirmed receipts can retire their placeholders. Genuine missing/error responses retain fallback handling. **Steps to reproduce** 1. Hold a GitHub metadata read until an authorized gateway call times out before publication staging. Repeat the exact request. 2. Publish a valid PR assessment. Send an ordinary author reply that asks for no new work and no duplicate comment. 3. Commit the native `response_wake` with a no-comment server presentation decision. Run response maintenance twice. 4. The base returns the invalid replay result or replaces the assessment with progress/fallback text. **Paperclip version or commit** Replay regression: `41c42895fa862c586ff841f8c50200d93ffc83a4`. Maintenance regression: the previous PR head `c7553d2325df53d98d9febff7023ade4b1e704eb`. Both use real PostgreSQL and simulated GitHub responses. The maintenance failure was also observed in normal hosted use. **Deployment mode** Source checkout on macOS arm64 with isolated PostgreSQL 18, followed by a dedicated QA qualification. No production rollout is included. ## What Changed - Route exact `begin_review` replay through its typed owner. Repeat unstaged preflight only with the complete request digest and no successful invocation result. - Preserve current policy, immutable assessments, unique action keys, receipt recovery, and provider leases. - Preserve a verified App/marker-owned final shared PR comment during acknowledgements. New canonical assessments can still replace it at the same or a new head. - Project only a bounded server-committed native wait decision. Settle the current main-PR acknowledgement without provider writes or a fabricated current-run reply receipt. - Hold succeeded native runs while presentation is undecided, including the committed-before-presentation window. An exact current-run confirmed receipt can still retire its placeholder. - Keep missing/error fallback and uncertain publication checks. Update the bot guide, execution semantics, maps, and real-database regressions. ## Verification - **Causal RED:** exact begin replay returned null; exact unstaged submit replay returned unavailable. The new real-database maintenance test then reproduced the exact generic summary overwrite. Its first repair also exposed an earlier queued-acknowledgement overwrite. Both publication boundaries are repaired. Four additional ordering regressions reproduced the generic PATCH while native presentation was undecided; the corrected tests hold until the same row settles. - **Local GREEN:** 196 complete GitHub workflow cases pass on an empty migrated PostgreSQL fixture. This includes queued/running preservation, both success-before-presentation windows, null/scalar/invalid decisions, exact-current versus predecessor receipts, durable acknowledgement settlement, repeated no-op maintenance, eventual genuine missing/error fallback, pending writes and same/new-head canonical replacement. Workspace typecheck, exact-head build, maps and whitespace checks pass. The real database respects the actual selected-column projection. - **Command:** `PAPERCLIP_TEST_DATABASE_URL=<owned-postgres-18> pnpm --filter @paperclipai/server exec vitest run src/__tests__/chat-channels.integration.test.ts -t 'GitHub agent review workflow'`. - **CI/review:** historical heads `c7553d2325df53d98d9febff7023ade4b1e704eb` and `4d00d3af6f2ccbda8f38a98f81d392d82dd3c59c` had Greptile 5/5. Final readiness head `2d267f1e4aa0b835a11aede97798fbd800d9cd96` passed independent source review with no actionable finding and fresh Greptile 5/5 with zero unresolved threads; all 54 check runs pass with 2 intended skips; Snyk is successful and the merge state is CLEAN. The dedicated QA destination acceptance also passed. - **Full-suite limitation:** the earlier `pnpm test:run` attempt produced no final summary. Its retained log reports failures in unchanged workspace-runtime tests and 15-second gateway-audit and stale-question timeouts. Their cause remains unknown. Do not treat the targeted results as a full local-suite pass. - **Live evidence and limits:** normal native OpenCode/DeepSeek review delivery published a useful assessment, inline finding and App check at the exact reviewed head. A separate ordinary author reply delivered a useful answer and settled. Those are ordinary delivery proofs, not live injected-timeout proofs. A later no-new-work reply exposed the maintenance overwrite. The final combined source `070e9c4ead9341cd497b23f533a91bf194475e79` includes the exact publication repair and passed normal review → ordinary author reply → quiet destination acceptance. A normal current-head review restored the legitimate assessment; the ordinary no-new-work reply then committed a passive wait without new GitHub tools. The same 1,436-character summary stayed identical while queued/running, after terminal settlement, and more than 3 minutes 50 seconds after successful cleanup. Review rows and receipts stayed unchanged, with no owner, live run or queued input. The timeout fault fixtures prove replay and single-publication safety. The historical first upstream timeout producer remains unknown. The live board reads do not expose the raw acknowledgement action row: its terminal cancellation and next-pass no-op are real-PostgreSQL fixture proofs, while the hosted checks prove actual destination stability. The hosted journey did not inject a timeout or pause the terminal/presentation writer window. Private destinations, raw receipts and internal task identifiers remain outside this PR. ## Risks - Original and repeated metadata reads can overlap. The immutable assessment, action uniqueness and publication lease must remain authoritative. - Missing complete request identity or a deleted completed receipt remains held. The gateway does not reconstruct receipts or relax approval. - Passive suppression requires actual native mode/committed phase, current source ownership and the server's no-comment decision. It applies only to the main PR review surface. Uncertain current writes stay held; other surfaces keep normal placeholder retirement. - There is no migration, credential change, dependency or new retry scheduler. ## Model Used OpenAI GPT-6.1 Sol (`gpt-6.1-sol`) in Codex assisted with diagnosis, implementation, tests, and source review. It used reasoning, repository tools and code execution. The context window size is not exposed in this session. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
208a093d19
commit
db32bdf98b
7 files changed
+367
-17
No files matched your search
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<void>(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<void>(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([]);
|
||||
|
||||
@@ -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<boolean>`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<boolean>`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<Record<string, unknown>>`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("<!-- paperclip-response-state:final -->") === true;
|
||||
const body = `${projectSafeChatPublicationText(input.body)}\n\n${marker}${shared ? `\n${requestResponseMarker(source)}` : ""}\n<!-- paperclip-response-version:${versionAt.toISOString()} -->${input.final ? "\n<!-- paperclip-response-state:final -->" : ""}`;
|
||||
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("<!-- paperclip-response-state:final -->")))) || (alreadyFinal && !input.final) ||
|
||||
(input.preserveFinal && previous.body?.includes("<!-- paperclip-response-state:final -->"))) ? previous : null;
|
||||
const posted = preserved && (!shared || preserved.body?.includes(marker)) ? preserved : await api.request<Comment>(
|
||||
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("<!-- paperclip-response-state:final -->")),
|
||||
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);
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in new issue
Block a user