mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-09 07:10:34 +01:00
fix(native): preserve warm continuity and deferred review replies (#18631)
## Thinking Path > - Paperclip manages AI agents and their saved work. > - Product E2E tests must check saved personal files across separate turns. > - The server records file collection in the durable `instruction_save` run-log event. > - The warm test read optional terminal result metadata instead and rejected a recorded successful save. > - This pull request reads the complete event stream for the current run and uses its latest save receipt. > - The prompt now specifies the exact nine bytes required by the temporary-file transfer count. > - Human review actions select the actual interaction card, which can share its interaction ID with a navigation button. > - A later real three-turn run exposed a missing reply after workspace finalization moved to its durable recovery owner. > - Reply recovery handled completed tasks but omitted ordinary tasks that awaited review. > - This pull request also publishes their recorded final reply once while preserving the pending review. ## Linked Issues or Issue Description **What happened?** The [retained live continuity attempt](https://github.com/paperclipai/paperclip/actions/runs/41477302876/job/125420001074) saved the correct memory file and unchanged 8 MiB binary. Its durable run log contained a successful `instruction_save` receipt. The oracle read `resultJson.instructionSave`, which was absent, and failed before turn two. The temporary file also had a trailing LF. Its prompt allowed that result, while the byte-count check required nine bytes. **Expected behavior** The oracle must use the documented authoritative save event. A missing receipt, a later failed save, incorrect transfer counts, or another run's events must fail. The prompt must specify the bytes that its independent check requires. **Steps to reproduce** 1. Run `daytona-warm-continuity.runner-codex.daytona.warm-three-turn` with the configured protected credentials and immutable image. 2. Complete a native turn whose terminal result has no `instructionSave` field. 3. Compare the oracle output with the same run's successful `instruction_save` event. A subsequent [three-turn attempt](https://github.com/paperclipai/paperclip/actions/runs/41495337334/job/125451304553) passed every personal-file assertion and reused the same warm process. Its first final reply was recorded as a completed agent-message event but was absent from comments. The log shows `native_workspace_finalization_busy`; the durable recovery path excluded `needs_review` replies. The final message matcher correctly failed. This failure remains recorded. The observed application revision was `aab87ce70744660a4a21c77278b0ede267c04720`. Related work: #14695 and #18494. The retained failed result keeps its original grade and definition. ## What Changed - Read all run-event pages through the existing bounded collector. - Require events to belong to the requested run and select the latest `instruction_save` event. - Retain strict byte counts and reject missing, failed, pending or superseded receipts. - Specify nine ASCII bytes and no trailing newline for the temporary file. - Scope human review actions to the interaction card and its exact interaction ID. - Advance the warm suite definition to version 5 and document its evidence source. - Recover recorded final replies for ordinary native tasks with an accepted `needs_review` result and terminal state. - Preserve pending reviews, existing reply selection, company/turn/digest checks and external governed-response authorization. - Update the task feature and agent access maps. ## Verification - Real PostgreSQL regression: deferred review reply discovery failed before the production repair (expected one candidate; found zero). The final-message event and pending review were present. - Recovery cases cover one publication, no repeated provider run, unchanged review, and rejection of absent/wrong-turn final messages, bad digest and suppressed comments. - Red: seven new boundary regressions failed against the old receipt lookup. - Green: 169 focused oracle, event pagination and catalog checks passed. Product E2E typecheck passed after rebuilding local package declarations. - Retained-evidence replay confirms that the new lookup finds the real successful save event. The original attempt remains failed because its temporary file included an LF. - Recursive typecheck passed before the final recovery change; server typecheck passed afterward. The recovery and control-plane suites passed all 112 tests. Full build and map checks passed. - The broad local test run reached one unrelated 30-second timeout in `attention-retention-work.test.ts` (maintained-review/confirmation batching). The same unchanged case then passed on one targeted retry (39.9 seconds including module load). The original failure remains retained. The broad run also reported one unchanged workspace-conflict case, which passed on its targeted retry (14.85 seconds including module load). Both original failures remain retained. The redundant broad local run was stopped after current-head CI completed all required gates: 55 successful checks and 3 selection skips. No all-green local broad-run claim is made. - The separate artifact-transfer cell already passed on the recorded application revision: independent source and delivered bytes matched at 407,485 bytes, with a completed task, succeeded native run and cleared execution locks. This does not establish a three-turn continuity pass. - [Real native three-turn acceptance](https://github.com/paperclipai/paperclip/actions/runs/41499102994/job/125465795210) **passed** at `3b207a42943e9eeb371d4f2d5623de72093eb6d5`, suite definition 5. Turn one exercised `committed_task_response_recovered`. All three completed final replies were persisted once and in order. All three strict personal-file receipts passed. The unchanged 8 MiB binary survived; later transfers copied only 36 and 54 bytes. The lease was created, resumed, and resumed with the same runner PID/start time, runner instance, native session and provider session. Final task was `done`, all three runs succeeded, and checkout/execution locks were null. - The aggregate startup summary has no separate provider-process PID proof. The raw run records independently prove the stable runner process and provider session stated above. This single native cell does not qualify the legacy cell or a full provider matrix. - The passing campaign reused the signed immutable runner image. It required no disk expansion. Earlier campaign image builds used the recorded temporary 120 GiB mitigation; default 60 GiB build capacity remains unqualified. All scoped completed probes were removed and independently returned 404; org CPU/disk usage returned to zero. This does not resolve unknown historical resources under a different credential identity. - Current-head Greptile is 5/5; review-thread pagination is complete with zero threads. All 55 current-head checks are successful; 3 selection checks are skipped. The paid campaign, result enforcement and report publication all completed successfully. ## Risks - Publishing a reply must not approve the work. Recovery only copies the accepted turn’s completed final message. It preserves the pending review and does not start an executor. - Conversation and external governed-response paths retain their existing separate authorization. The latest save event can record failure after an earlier success. The oracle rejects that case. Event pagination, missing receipts and cross-run evidence also fail closed. The exact temporary-file byte count remains required. The review selector still requires the intended interaction ID and enabled action on its actual card. The production change is limited to ordinary internal-task reply recovery. It still requires an accepted result, a matching successful terminal disposition, the completed final message, and existing publication authority. ## Model Used OpenAI Codex, based on GPT-6, with shell and code tools. The exact deployment variant and context-window size are 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 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
0fd82e386e
commit
53f480d1b3
11 files changed
+138
-18
No files matched your search
@@ -2,7 +2,7 @@ import { randomUUID } from "node:crypto";
|
||||
import { and, eq, sql } from "drizzle-orm";
|
||||
import { afterAll, beforeAll, describe, expect, it, vi } from "vitest";
|
||||
import { agents, authUsers, companies, completionContracts, createDb, heartbeatRunEvents, heartbeatRuns, issueComments, issues,
|
||||
nativeRunFinalizations, nativeRunResults, chatTaskHandoffs, chatCompletionDeliveries, agentWakeupRequests, type Db } from "@paperclipai/db";
|
||||
nativeRunFinalizations, nativeRunResults, issueThreadInteractions, chatTaskHandoffs, chatCompletionDeliveries, agentWakeupRequests, type Db } from "@paperclipai/db";
|
||||
import { getEmbeddedPostgresTestSupport, startEmbeddedPostgresTestDatabase } from "../../__tests__/helpers/embedded-postgres.js";
|
||||
import { resolveExternalTestDatabaseUrl } from "../../__tests__/helpers/external-test-database.js";
|
||||
import { NativeRunCoordinatorStore } from "./native-run-coordinator-store.js";
|
||||
@@ -26,7 +26,7 @@ describe.skipIf(!support.supported)("committed Agent Chat reply recovery", () =>
|
||||
}, 60_000);
|
||||
afterAll(async () => { await db?.$client.end(); await database?.cleanup(); });
|
||||
|
||||
async function fixture(conversation = true, options: { succeed?: boolean } = {}) {
|
||||
async function fixture(conversation = true, options: { succeed?: boolean; disposition?: "done" | "needs_review" } = {}) {
|
||||
const companyId = randomUUID(), agentId = randomUUID(), issueId = randomUUID(), runId = randomUUID();
|
||||
const sessionId = randomUUID(), runnerId = randomUUID(), userId = `fixture-${randomUUID()}`, wakeId = randomUUID();
|
||||
await db.insert(authUsers).values({ id: userId, name: "Conversation owner", email: `${userId}@example.test`,
|
||||
@@ -53,10 +53,12 @@ describe.skipIf(!support.supported)("committed Agent Chat reply recovery", () =>
|
||||
const prpEvent = { runId, normalizedSessionId: sessionId, turnId: "hire-turn", sourceEventId: "fixture-final-answer",
|
||||
payload: { kind: "agentMessage", channel: "final", providerPhase: "final", text: reply } };
|
||||
await db.insert(heartbeatRunEvents).values({ companyId, runId, agentId, seq: 170, eventType: "item.completed", payload: { prpEvent } });
|
||||
await port.completeRun({ result: { schema: "paperclip.run_result.v1", reportedWorkDisposition: "done",
|
||||
const disposition = options.disposition ?? "done";
|
||||
await port.completeRun({ result: { schema: "paperclip.run_result.v1", reportedWorkDisposition: disposition,
|
||||
summary: "A completed hire, with its final message preserved separately.", completionClaim: { contractRevision: "test",
|
||||
objectiveSatisfied: true, criteria: [], remainingWork: [] }, evidence: [], verification: [], attentionRequests: [], artifacts: [] },
|
||||
terminal: { schema: "paperclip.prp.terminal.v1", runTerminalState: "succeeded", turnTerminalState: "completed", reportedWorkDisposition: "done" }, turnId: "hire-turn" });
|
||||
objectiveSatisfied: true, criteria: [], remainingWork: [] }, evidence: [], verification: [],
|
||||
attentionRequests: disposition === "needs_review" ? [{ kind: "review", ownerClass: "human", summary: "Review the completed work." }] : [], artifacts: [] },
|
||||
terminal: { schema: "paperclip.prp.terminal.v1", runTerminalState: "succeeded", turnTerminalState: "completed", reportedWorkDisposition: disposition }, turnId: "hire-turn" });
|
||||
await finalizeNativeRun({ db, runId, workspaceFinalizeStatus: "succeeded", projectRunStatus: false });
|
||||
expect((await db.select().from(nativeRunFinalizations).where(eq(nativeRunFinalizations.runId, runId)))[0].phase).toBe("committed");
|
||||
if (options.succeed !== false) await db.update(heartbeatRuns).set({ status: "succeeded", finishedAt: new Date("2020-01-01T00:02:00Z") }).where(eq(heartbeatRuns.id, runId));
|
||||
@@ -641,6 +643,38 @@ describe.skipIf(!support.supported)("committed Agent Chat reply recovery", () =>
|
||||
expect(await comments(f.runId)).toEqual([expect.objectContaining({ body: f.reply })]);
|
||||
expect((await issue(f.issueId)).conversationAgentId).toBeNull();
|
||||
});
|
||||
it("recovers a deferred review reply once without resolving the review or rerunning work", async () => {
|
||||
const f = await fixture(false, { disposition: "needs_review" });
|
||||
const before = await issue(f.issueId);
|
||||
const reviews = await db.select().from(issueThreadInteractions).where(eq(issueThreadInteractions.issueId, f.issueId));
|
||||
expect(before.status).toBe("in_review");
|
||||
expect(reviews).toEqual([expect.objectContaining({ status: "pending" })]);
|
||||
expect(await comments(f.runId)).toEqual([]);
|
||||
const work = nativeResponsePresentationWork(db), entries = await work.load(f.input);
|
||||
expect(entries).toHaveLength(1);
|
||||
await expect(work.check(entries[0])).resolves.toBeNull();
|
||||
expect(await comments(f.runId)).toEqual([expect.objectContaining({ body: f.reply, createdByRunId: f.runId })]);
|
||||
expect(await issue(f.issueId)).toEqual({ ...before, updatedAt: expect.any(Date) });
|
||||
expect(await db.select().from(issueThreadInteractions).where(eq(issueThreadInteractions.issueId, f.issueId))).toEqual(reviews);
|
||||
expect(await work.load(f.input)).toEqual([]);
|
||||
await expect(recoverCommittedNativeChatResponse(db, f.input)).resolves.toBe(false);
|
||||
expect(await comments(f.runId)).toHaveLength(1);
|
||||
expect(await db.select().from(heartbeatRuns).where(eq(heartbeatRuns.nativeIssueId, f.issueId))).toHaveLength(1);
|
||||
});
|
||||
it.each(["missing-final", "wrong-turn", "digest", "skip-comment"])(
|
||||
"keeps deferred review response recovery closed for %s", async fault => {
|
||||
const f = await fixture(false, { disposition: "needs_review" });
|
||||
if (fault === "missing-final") await db.delete(heartbeatRunEvents).where(eq(heartbeatRunEvents.runId, f.runId));
|
||||
if (fault === "wrong-turn") await db.update(heartbeatRunEvents).set({ payload: { prpEvent: { ...f.prpEvent, turnId: "another-turn" } } })
|
||||
.where(and(eq(heartbeatRunEvents.runId, f.runId), eq(heartbeatRunEvents.seq, 170)));
|
||||
if (fault === "digest") await db.update(nativeRunResults).set({ canonicalSha256: "invalid-digest" }).where(eq(nativeRunResults.runId, f.runId));
|
||||
if (fault === "skip-comment") await db.update(heartbeatRuns).set({ contextSnapshot: { issueId: f.issueId, skipIssueComment: true } }).where(eq(heartbeatRuns.id, f.runId));
|
||||
const before = await issue(f.issueId);
|
||||
await expect(recoverCommittedNativeChatResponse(db, f.input)).resolves.toBe(false);
|
||||
expect(await comments(f.runId)).toEqual([]);
|
||||
expect(await issue(f.issueId)).toEqual(before);
|
||||
},
|
||||
);
|
||||
it("rolls back a partial reply write without changing the waiting state or consuming recovery", async () => {
|
||||
const f = await fixture(), abort = new Error("fixture-commit-aborted");
|
||||
const failing = new Proxy(db, { get(target, key) { return key === "transaction"
|
||||
|
||||
@@ -48,8 +48,13 @@ export function nativeResponsePresentationCandidates(scope?: NativeResponsePrese
|
||||
sql`${heartbeatRunEvents.payload} #>> '{prpEvent,payload,channel}' = 'final'`,
|
||||
sql`${heartbeatRunEvents.payload} #>> '{prpEvent,payload,text}' ~ '[^[:space:]]'`,
|
||||
))))),
|
||||
sql`${nativeRunResults.resultJson} #>> '{result,reportedWorkDisposition}' = 'done'`,
|
||||
sql`${nativeRunResults.resultJson} #>> '{terminal,reportedWorkDisposition}' = 'done'`),
|
||||
or(
|
||||
and(sql`${nativeRunResults.resultJson} #>> '{result,reportedWorkDisposition}' = 'done'`,
|
||||
sql`${nativeRunResults.resultJson} #>> '{terminal,reportedWorkDisposition}' = 'done'`),
|
||||
and(sql`(coalesce(${issues.conversationAgentId}::text, '') = '' or coalesce(${issues.conversationUserId}, '') = '')`,
|
||||
sql`${nativeRunResults.resultJson} #>> '{result,reportedWorkDisposition}' = 'needs_review'`,
|
||||
sql`${nativeRunResults.resultJson} #>> '{terminal,reportedWorkDisposition}' = 'needs_review'`),
|
||||
)),
|
||||
and(sql`${nativeRunResults.resultJson} #>> '{result,reportedWorkDisposition}' = 'yielded'`,
|
||||
sql`${nativeRunResults.resultJson} #>> '{terminal,reportedWorkDisposition}' = 'yielded'`,
|
||||
sql`jsonb_typeof(${nativeRunResults.resultJson} #> '{result,summary}') = 'string'
|
||||
|
||||
@@ -921,15 +921,17 @@ async function recoverCommittedNativeChatResponseInScope(
|
||||
&& run.contextSnapshot?.issueId === issue.id
|
||||
&& typeof run.contextSnapshot.conversationSessionGeneration === "number"
|
||||
&& run.contextSnapshot.conversationSessionGeneration === issue.conversationSessionGeneration;
|
||||
// A deferred ordinary-task review still needs its recorded final reply.
|
||||
// Publishing that reply must not resolve the interaction or the task.
|
||||
if (
|
||||
(!conversation || currentConversation) &&
|
||||
run.status === "succeeded" &&
|
||||
record(run.contextSnapshot).skipIssueComment !== true &&
|
||||
!isExternalChatPresentationContext(run.contextSnapshot) &&
|
||||
result.reportedWorkDisposition === "done" &&
|
||||
terminal.runTerminalState === "succeeded" &&
|
||||
terminal.turnTerminalState === "completed" &&
|
||||
terminal.reportedWorkDisposition === "done" &&
|
||||
((result.reportedWorkDisposition === "done" && terminal.reportedWorkDisposition === "done") ||
|
||||
(!conversation && result.reportedWorkDisposition === "needs_review" && terminal.reportedWorkDisposition === "needs_review")) &&
|
||||
(await resolveChatRunPresentationAuthorizationReason(tx, input)) === "internal_agent_write"
|
||||
) {
|
||||
const rows = await tx.select({ seq: heartbeatRunEvents.seq, payload: heartbeatRunEvents.payload })
|
||||
|
||||
Reference in new issue
Block a user