From 23cd123ff8ea19faf72a304db128d99d5e3de937 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Wed, 4 Nov 2026 17:41:03 -0600 Subject: [PATCH] fix(e2e): retain the closed cause code of a journey cleanup failure (#19079) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The Product E2E journey harness runs the QA staging smoke plan and reports each case to TestOps > - Each case runs a journey, then releases its owned target resources (cleanup) > - Three recent cleanup failures had a passed journey and all checks passed, but each result only says `target_cleanup_failed` > - The adapter throws an error with a closed identifier code, and the harness discards that code, so no one can find the failing cleanup step > - This pull request keeps the identifier-shaped cause code as an extra cleanup phase > - The benefit is that the next cleanup failure is diagnosable from TestOps without private runner files ## Linked Issues or Issue Description **What happened?** QA staging smoke cleanup failures show only `target_cleanup_failed`. This occurred for `question-answer-resume` (launch 2390), and for `company-access-isolation` and `create-skill-studio` (launch 2693). In each case, the journey and all behavioral checks passed. The private runner report is deleted with the runner, and the adapter error code is not kept in the retained result. The cause is not known. **Expected behavior** A cleanup failure keeps a closed, identifier-shaped cause code (for example `fixture_cleanup_not_confirmed`). The result never keeps the raw error message. **Steps to reproduce** 1. Run a Product E2E journey with a target session whose `release()` throws an error that has a `code` property. 2. Read `normalized-results.json`. 3. Before this change, the cleanup phase only contains `target_cleanup_failed`. **Paperclip version or commit** master at c99bcd89fe. ## What Changed - `tests/runner-e2e/product-journeys/launch.ts`: when target release or stop-only recovery fails, also record `cause.` as a failed cleanup phase. The code must match `^[A-Za-z][A-Za-z0-9_]{1,79}$`. Other values are ignored. Timeouts keep their existing codes. `result.error` stays `target_cleanup_failed`. - `tests/runner-e2e/product-journeys/campaign.test.ts`: add a regression test. It checks that an identifier code is kept, that the raw message is not kept, and that a free-form code is not kept. ## Verification - `npx vitest run --config tests/runner-e2e/vitest.config.ts tests/runner-e2e/product-journeys`: 46 files, 1235 tests passed. - Negative control: with the old `launch.ts`, the new test fails. With the change, it passes. - `tsc -p tests/runner-e2e/tsconfig.json` shows no errors in the changed files. (This worktree has unbuilt workspace packages, so other unrelated errors appear.) - The Paperclip Cloud TestOps exporter already publishes phase codes that match its closed code pattern as step messages. A `cause.*` code matches that pattern. - Not verified live: QA staging smoke uses a pinned harness revision (`QA_SMOKE_HARNESS_SHA`). The new phase appears in TestOps only after the pin moves to a revision that contains this change. ## Risks - Low risk. The change only adds a phase entry when cleanup already failed. Pass and fail grading does not change. - An adapter could put private data into an error `code`. The strict identifier pattern limits this to short identifier strings. - Report consumers that count cleanup phases see one more failed cleanup phase. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, via Claude Code with tool use (shell, browser, code editing). Extended reasoning was used. ## 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 - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- .../product-journeys/campaign.test.ts | 26 ++++++++++++++++--- tests/runner-e2e/product-journeys/launch.ts | 10 ++++++- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/tests/runner-e2e/product-journeys/campaign.test.ts b/tests/runner-e2e/product-journeys/campaign.test.ts index 444d2155169..24395e5660d 100644 --- a/tests/runner-e2e/product-journeys/campaign.test.ts +++ b/tests/runner-e2e/product-journeys/campaign.test.ts @@ -38,7 +38,7 @@ import { regenerateRunnerDashboard } from "../dashboard-regenerate.js"; const roots: string[]=[]; afterEach(async()=>{vi.useRealTimers(); vi.restoreAllMocks(); persistenceFault.kind=null; persistenceFault.active=false; persistenceFault.latestReport=null; process.exitCode=0; await Promise.all(roots.splice(0).map(root=>rm(root,{recursive:true,force:true})));}); type Hook = "collect" | "release" | "stop" | "finalizeEvidence" | "checkpointEvidence"; -async function run(options:{search?:boolean;authFailure?:boolean;onboardingFailure?:boolean;preAcceptanceFailure?:boolean;missingArtifact?:boolean;drift?:boolean;cleanupFailure?:boolean;collectionFailure?:boolean;mini?:boolean;missingReceipt?:boolean;zeroRequests?:boolean;missingCheck?:boolean;finalizationFailure?:boolean;persistenceFailure?:"write"|"rename";persistenceAfter?:"collection"|"release";stallHook?:Hook;onHook?:(hook:Hook,signal?:AbortSignal)=>void}={}) { +async function run(options:{search?:boolean;authFailure?:boolean;onboardingFailure?:boolean;preAcceptanceFailure?:boolean;missingArtifact?:boolean;drift?:boolean;cleanupFailure?:boolean;cleanupCode?:string;stopCode?:string;collectionFailure?:boolean;mini?:boolean;missingReceipt?:boolean;zeroRequests?:boolean;missingCheck?:boolean;finalizationFailure?:boolean;persistenceFailure?:"write"|"rename";persistenceAfter?:"collection"|"release";stallHook?:Hook;onHook?:(hook:Hook,signal?:AbortSignal)=>void}={}) { const root=await mkdtemp(path.join(os.tmpdir(),"journey-campaign-test-")); roots.push(root); const module=path.join(root,"provider.mjs"); const proof = createConnectionProof(); @@ -52,7 +52,7 @@ async authenticate(){${options.authFailure ? "throw {outcome:'failed',code:'logi async onboard(evidence){${options.preAcceptanceFailure ? `evidence.onboardingPreAcceptance=${JSON.stringify(preAcceptanceEvidence)};throw {outcome:'failed',code:'onboarding_work_before_acceptance'};` : options.onboardingFailure ? "throw {outcome:'failed',code:'onboarding_failed'};" : `${options.missingArtifact ? "" : `evidence.artifact=${JSON.stringify(artifact)};`}evidence.checks.push(...['fresh-workspace','ui-connection-and-native-agent','first-task-complete','downloadable-artifact-verified'${options.missingCheck ? "" : ",'native-trace-verified'"}].map(id=>({id,passed:true})));onboarded=true;return context;`}}, async prepare(){throw Error('onboarding receipt was not reused')}, async identity(){return {...identity,commit:'${(options.drift?"b":"a").repeat(40)}'}}, -async collect(){${options.collectionFailure ? "throw Error('private collection error');" : options.missingReceipt ? "return [];" : `return onboarded?[{journeyId:'cloud-onboarding',runIds:['run-1'],usage:{runs:[{runId:'run-1',usage:{costUsd:0.01}}]},operations:{campaignId:input.campaignId,attemptId:input.campaignId+':cloud-onboarding:1',modelCostUsd:0.01,setupCostUsd:0.005,requestCount:${options.zeroRequests ? 0 : 2},feedback:{collection:'complete',complaints:0,suggestions:0},traceIds:['a'.repeat(32)]}}]:[];`}}, async own(){}, async release(){${options.cleanupFailure ? "throw Error('private raw error');" : "return 'released';"}}, async stop(){} +async collect(){${options.collectionFailure ? "throw Error('private collection error');" : options.missingReceipt ? "return [];" : `return onboarded?[{journeyId:'cloud-onboarding',runIds:['run-1'],usage:{runs:[{runId:'run-1',usage:{costUsd:0.01}}]},operations:{campaignId:input.campaignId,attemptId:input.campaignId+':cloud-onboarding:1',modelCostUsd:0.01,setupCostUsd:0.005,requestCount:${options.zeroRequests ? 0 : 2},feedback:{collection:'complete',complaints:0,suggestions:0},traceIds:['a'.repeat(32)]}}]:[];`}}, async own(){}, async release(){${options.cleanupFailure ? `throw Object.assign(Error('private raw error'),${JSON.stringify(options.cleanupCode ? { code: options.cleanupCode } : {})});` : "return 'released';"}}, async stop(){${options.stopCode ? `throw Object.assign(Error('private stop error'),${JSON.stringify({ code: options.stopCode })});` : ""}} , async finalizeEvidence(){ const report=JSON.parse(await readFile(input.outputDir+'/normalized-results.json','utf8')); if(report.results.some(r=>r.cleanup==='not_started'))throw Error('final evidence preceded cleanup'); @@ -75,7 +75,7 @@ await writeFile(input.outputDir+'/failure-checkpoint.json',JSON.stringify(report return { ...session, async collect(input?: {signal:AbortSignal}) { options.onHook?.("collect",input?.signal); if(options.stallHook==="collect")await new Promise(()=>{}); try { return await session.collect(); } finally { if (options.persistenceAfter !== "release" && options.persistenceFailure) persistenceFault.active=true; } }, async release(outcome: string,input?: {signal:AbortSignal}) { lifecycle.push(`release:${outcome}`); artifactBeforeRelease=JSON.parse(await readFile(path.join(output,"normalized-results.json"),"utf8")).results[0].productJourney.artifact; options.onHook?.("release",input?.signal); if(options.stallHook==="release")await new Promise(()=>{}); try { return await session.release(outcome); } finally { if (options.persistenceAfter === "release" && options.persistenceFailure) persistenceFault.active=true; } }, - async stop(input?: {signal:AbortSignal}) { lifecycle.push("stop"); options.onHook?.("stop",input?.signal); if(options.stallHook==="stop")await new Promise(()=>{}); }, + async stop(input?: {signal:AbortSignal}) { lifecycle.push("stop"); options.onHook?.("stop",input?.signal); if(options.stallHook==="stop")await new Promise(()=>{}); if(options.stopCode) await session.stop?.(); }, async finalizeEvidence(input?: {signal:AbortSignal}) { lifecycle.push("finalizeEvidence"); options.onHook?.("finalizeEvidence",input?.signal); if(options.stallHook==="finalizeEvidence")await new Promise(()=>{}); return session.finalizeEvidence(); }, async checkpointEvidence(input?: {signal:AbortSignal}) { lifecycle.push("checkpointEvidence"); options.onHook?.("checkpointEvidence",input?.signal); if(options.stallHook==="checkpointEvidence")await new Promise(()=>{}); return session.checkpointEvidence(); }, }; @@ -164,6 +164,26 @@ describe("product journey campaign boundary outcomes",()=>{ expect(campaign.results.every((r:any)=>r.cleanup==="failed"&&r.status==="failed")).toBe(true); expect(JSON.stringify(campaign)).not.toContain("private raw error"); }); + it("retains an identifier-shaped cleanup cause but never a free-form one",async()=>{ + const traced=await run({cleanupFailure:true,cleanupCode:"fixture_cleanup_not_confirmed"}); + for (const r of traced.results) { + expect(r.error).toBe("target_cleanup_failed"); + expect(r.productJourney.phases).toContainEqual({phase:"cleanup",outcome:"failed",code:"target_cleanup_failed"}); + expect(r.productJourney.phases).toContainEqual({phase:"cleanup",outcome:"failed",code:"cause.fixture_cleanup_not_confirmed"}); + } + expect(JSON.stringify(traced)).not.toContain("private raw error"); + const stopped=await run({cleanupFailure:true,cleanupCode:"fixture_cleanup_not_confirmed",stopCode:"owned_work_stop_unconfirmed"}); + for (const r of stopped.results) { + expect(r.error).toBe("target_cleanup_failed"); + expect(r.productJourney.phases).toContainEqual({phase:"cleanup",outcome:"failed",code:"target_stop_failed"}); + expect(r.productJourney.phases).toContainEqual({phase:"cleanup",outcome:"failed",code:"cause.fixture_cleanup_not_confirmed"}); + expect(r.productJourney.phases).toContainEqual({phase:"cleanup",outcome:"failed",code:"cause.owned_work_stop_unconfirmed"}); + } + expect(JSON.stringify(stopped)).not.toMatch(/private (raw|stop) error/); + const untraced=await run({cleanupFailure:true,cleanupCode:"private detail: tenant-42"}); + expect(JSON.stringify(untraced)).not.toContain("tenant-42"); + expect(untraced.results.every((r:any)=>!r.productJourney.phases.some((p:any)=>String(p.code).startsWith("cause.")))).toBe(true); + }); it("preserves failed authentication and blocked dependents when finalization also fails",async()=>{ const campaign=await run({authFailure:true,collectionFailure:true,cleanupFailure:true}); expect(campaign.results.map((r:any)=>r.productJourney.outcome)).toEqual(["failed","blocked"]); diff --git a/tests/runner-e2e/product-journeys/launch.ts b/tests/runner-e2e/product-journeys/launch.ts index 930112167c5..e96a990d03f 100644 --- a/tests/runner-e2e/product-journeys/launch.ts +++ b/tests/runner-e2e/product-journeys/launch.ts @@ -211,6 +211,13 @@ export async function launchProductJourneys(args: string[], repositoryRoot: stri } } }; + // Retain only an identifier-shaped adapter cause, never its message, so a + // generic cleanup failure can be traced to the target step that failed. + const recordCleanupCause = (error: unknown) => { + const cause = (error as { code?: unknown } | null)?.code; + if (typeof cause !== "string" || !/^[A-Za-z][A-Za-z0-9_]{1,79}$/.test(cause)) return; + failFinalization(`cause.${cause}`); + }; const persist = async () => { try { await writeFile(path.join(outputDir, "normalized-results.tmp"), JSON.stringify({ schema: "paperclip.runner-e2e.campaign/v2", campaignId, generatedAt: new Date().toISOString(), expected, source, results }, null, 2), { mode: 0o600 }); @@ -404,10 +411,11 @@ export async function launchProductJourneys(args: string[], repositoryRoot: stri for (const result of results) { if (result.cleanup !== "failed") result.cleanup = "passed"; result.productJourney!.resourceDisposition = disposition; result.productJourney!.phases.push({ phase: "cleanup", outcome: "passed" }); } } catch (error) { failFinalization(error instanceof JourneyError && error.code === "target_cleanup_timeout" ? error.code : "target_cleanup_failed"); + if (!(error instanceof JourneyError && error.code === "target_cleanup_timeout")) recordCleanupCause(error); // A timed-out release may still have an in-flight request. Stop-only recovery // gets its own bounded signal and cannot delete resources or free the lease. try { await withinLifecycleDeadline(Math.min(cleanupDeadlineAt - browserCloseReserveMs, Date.now() + stopReserveMs), "target_stop_timeout", input => session?.stop?.(input) ?? Promise.resolve()); } - catch { failFinalization("target_stop_failed"); } + catch (stopError) { failFinalization("target_stop_failed"); if (!(stopError instanceof JourneyError && stopError.code === "target_stop_timeout")) recordCleanupCause(stopError); } } try { await withinLifecycleDeadline(Math.min(cleanupDeadlineAt, Date.now() + browserCloseReserveMs), "browser_cleanup_timeout", () => browser?.close() ?? Promise.resolve()); } catch (error) { failFinalization(error instanceof JourneyError && error.code === "browser_cleanup_timeout" ? error.code : "browser_cleanup_failed"); }