From f0ff466aafab6b9635e9be2b4c5da3a07b6974c5 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Sat, 31 Oct 2026 22:54:28 -0500 Subject: [PATCH] fix(testops): wait for complete attachment evidence (#18509) ## Thinking Path > - Paperclip manages AI agents and their work. > - Product E2E evaluations check real user requests and their saved results. > - The TestOps reporter also verifies the uploaded result and attachment evidence. > - One failed attachment-row snapshot stopped the reader before its existing observation allowance. > - A later read with the same protected credential reference verified the original result. The original failed subfield remains unknown. > - This pull request handles attachment readiness within the existing bound while preserving the complete final evidence predicate. > - The reporter can wait for complete evidence without accepting missing, inherited, or incorrect files. ## Linked Issues or Issue Description Refs #18477 and #18470. **What happened?** The chat case passed its ten checks and cleanup in [run 41217990335](https://github.com/paperclipai/paperclip/actions/runs/41217990335), but the final reader failed at `attachment_page / attachment_rows`. The unchanged reader later verified that same retained result through the protected reporting identity in [read-only run 41268693639](https://github.com/paperclipai/paperclip/actions/runs/41268693639). This proves the observed snapshot differs. It does not identify which original row subfield failed or prove an upload race. The reader already permits twelve polls and 120 seconds for complete result and attachment metadata. It nevertheless rejects `missed === true` immediately. That flag cannot satisfy complete evidence, but it can be observed again within the existing allowance. **Expected behavior** Wait within the original limit for attachment evidence to become complete. Accept only the same full source, case, status, cleanup, attachment-name, media-type, byte-count, and non-missed predicate. Reject malformed IDs and inherited rows immediately. Permanent missing or incorrect evidence must still fail. **Steps to reproduce** 1. Call the real verifier with otherwise valid responses and one `missed: true` attachment. 2. Return the complete attachment on the second poll. 3. The old reader fails on the first snapshot. The regression now reaches the original complete acceptance predicate on the second snapshot. **Paperclip version/commit** Base `987e0932bf66e703750cd04698b3c7e8993580a7`. **Deployment mode** TestOps reporting observer after a Product E2E evaluation. This change starts no model or app and changes no feature oracle. ## What Changed - Move the strict `missed === true` condition from immediate structural rejection into bounded readiness. - Keep page shape, counts, unique positive IDs, inherited-row rejection, and every source and case guard hard. - Require the same complete final signature and status before success. Keep twelve polls, five-second intervals, the 120-second deadline, and request limits. - Retain closed per-poll counts and readiness booleans on verification and unverified failure. Clone failure observations so callers cannot change retained diagnostics. - Add seven focused regression and negative controls. Sync one existing consumer test with the precise new failure phase and predicate. ## Verification - Genuine parent RED: the real missed-then-complete verifier test failed at the original attachment-row guard. - API owner tests: 60 passed. Direct CLI, exporter, reporter, and retained-reader consumer tests: 31 passed. Node 26.4.0. - Permanent missing evidence reaches twelve polls and eleven waits, then fails. An elapsed 120-second deadline stops after the first page. Malformed IDs and inherited rows still fail immediately. Incorrect byte counts never pass. Closed diagnostics exclude private fields and return detached observation copies. - Three Node syntax checks and `git diff --check` passed. - Two review reproductions failed on the first PR head: a null row lost its controlled invalid-ID error, and a final-GET deadline crossing lost its exact readiness predicate. The corrected head preserves both diagnostics. Null rows still fail immediately; no deadline or acceptance condition changed. - No local app, database, provider, native build, or new evaluation ran. The MJS change needs no TypeScript emission. - [Normal CI 41269715187](https://github.com/paperclipai/paperclip/actions/runs/41269715187) passed on exact head `4e77c1220b0ba81e971591c1c7d28f9064e992cc`: 55 successes, three skipped checks, and one neutral review. Both required checks passed. Greptile is 5/5 and review threads are resolved. - The controlled normal TestOps-launched confirmation is pending merge. The family unit roster runs the local controls; normal PR CI uses separate runner suites, so CI green alone does not claim that roster ran there. ## Risks - A missed file may remain missing. The observer fails if complete evidence does not arrive inside the original allowance. - This is readiness handling, not a claim that `missed` caused the historical failure. Prior passed case grades and failed reporting workflows remain retained. - The previous fixed retained diagnostic intentionally pins the original helper. Its completed receipt remains valid; after this helper changes, that old diagnostic fails closed instead of silently replaying a different observer. ## Model Used OpenAI `gpt-6.1-sol` through Codex, with reasoning, code editing, shell tools, and independent source review. The host does not expose the context window size. ## 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 --- tests/runner-e2e/chat-feature-testops-api.mjs | 43 +++++-- .../chat-feature-testops-api.test.mjs | 112 ++++++++++++++++++ ...estops-retained-verification.node-test.mjs | 5 +- 3 files changed, 146 insertions(+), 14 deletions(-) diff --git a/tests/runner-e2e/chat-feature-testops-api.mjs b/tests/runner-e2e/chat-feature-testops-api.mjs index 8d9269ecea9..8be6bc63fc3 100644 --- a/tests/runner-e2e/chat-feature-testops-api.mjs +++ b/tests/runner-e2e/chat-feature-testops-api.mjs @@ -23,10 +23,11 @@ const uploadFailureChecks = { 'embedded_job', 'links_array', 'known_links'], result_status: ['status_supported', 'status_matches_export'], attachment_page: ['content_array', 'content_limit', 'total_integer', 'total_matches_content', 'pages_integer', 'pages_range', - 'unique_ids', 'attachment_rows'], + 'unique_ids', 'ids_valid', 'not_inherited', 'attachment_rows'], + attachment_readiness: ['attachment_page_observed', 'attachments_not_missed', 'attachment_signatures_match', 'result_status_matches', 'attachments_ready'], }; const uploadFailures = new WeakMap(); -function stopUpload(code, phase, readChecks) { +function stopUpload(code, phase, readChecks, attachmentReadiness) { const error = new SetupFailure(code); let values; try { values = readChecks(); } catch {} @@ -38,7 +39,8 @@ function stopUpload(code, phase, readChecks) { : ['string', 'number', 'boolean', 'object'].includes(type) ? type : 'other']; })); const failedPredicates = Object.keys(checks).filter(name => !checks[name]); - uploadFailures.set(error, { phase, firstFailedPredicate: failedPredicates[0] ?? 'checks_unavailable', failedPredicates, checks, types }); + uploadFailures.set(error, { phase, firstFailedPredicate: failedPredicates[0] ?? 'checks_unavailable', failedPredicates, checks, types, + ...(attachmentReadiness ? { attachmentReadiness: attachmentReadiness.map(row => ({ ...row })) } : {}) }); throw error; } const diagnosticCodes = new Set(['untrusted_chat_testops_context', 'testops_token_missing', 'testops_auth_http', @@ -59,7 +61,8 @@ export function failureProjection(error, stage) { code: error instanceof SetupFailure && diagnosticCodes.has(error.code) ? error.code : 'unknown_failure', httpStatus: error instanceof SetupFailure && Number.isInteger(error.status) && error.status >= 100 && error.status < 600 ? error.status : null, ...(verification ? { verification: { ...verification, failedPredicates: [...verification.failedPredicates], - checks: { ...verification.checks }, types: { ...verification.types } } } : {}), + checks: { ...verification.checks }, types: { ...verification.types }, + ...(verification.attachmentReadiness ? { attachmentReadiness: verification.attachmentReadiness.map(row => ({ ...row })) } : {}) } } : {}), ...(request && bootstrapRequestClasses.has(request.class) && id(request.resourceId) && ['STARTED', 'SUCCEEDED', 'FAILED'].includes(request.state) ? { request: { class: request.class, resourceId: request.resourceId, state: request.state, httpStatus: Number.isInteger(request.httpStatus) && request.httpStatus >= 100 && request.httpStatus < 600 ? request.httpStatus : null } } : {}) }; @@ -413,11 +416,19 @@ export async function verifyChatUpload(api, env, selected, exported, { wait = ms const runUrl = `https://github.com/paperclipai/paperclip/actions/runs/${context.runId}`; const sourceUrl = `https://github.com/paperclipai/paperclip/commit/${context.targetSha}`; const deadline = now() + 120_000; + const attachmentReadiness = []; + const unverified = () => stopUpload('chat_testops_upload_unverified', 'attachment_readiness', () => ({ + attachment_page_observed: [attachmentReadiness.length > 0], + attachments_not_missed: [attachmentReadiness.at(-1)?.missedCount === 0], + attachment_signatures_match: [attachmentReadiness.at(-1)?.signaturesMatch === true], + result_status_matches: [attachmentReadiness.at(-1)?.resultStatusMatches === true], + attachments_ready: [attachmentReadiness.at(-1)?.ready === true], + }), attachmentReadiness); const get = async route => { const remaining = deadline - now(); - if (remaining < 1) fail('chat_testops_upload_unverified'); + if (remaining < 1) unverified(); const value = await api(route, 'GET', undefined, Math.min(30_000, Math.floor(remaining))); - if (now() >= deadline) fail('chat_testops_upload_unverified'); + if (now() >= deadline) unverified(); return value; }; for (let poll = 0; poll < 12 && now() < deadline; poll++) { @@ -476,25 +487,33 @@ export async function verifyChatUpload(api, env, selected, exported, { wait = ms const attached = await get(`/api/testresult/attachment?${new URLSearchParams({ testResultId: String(row.id), page: '0', size: '100' })}`); if (!Array.isArray(attached?.content) || attached.content.length > 10 || !Number.isSafeInteger(attached.totalElements) || attached.totalElements !== attached.content.length || !Number.isSafeInteger(attached.totalPages) || attached.totalPages < 0 || attached.totalPages > 1 - || new Set(attached.content.map(value => value.id)).size !== attached.content.length - || attached.content.some(value => !id(value.id) || value.missed === true || value.fromTestCase === true)) stopUpload('chat_testops_upload_mismatch', 'attachment_page', () => ({ + || new Set(attached.content.map(value => value?.id)).size !== attached.content.length + || attached.content.some(value => !id(value?.id) || value?.fromTestCase === true)) stopUpload('chat_testops_upload_mismatch', 'attachment_page', () => ({ content_array: [Array.isArray(attached?.content), attached?.content], content_limit: [Array.isArray(attached?.content) && attached.content.length <= 10], total_integer: [Number.isSafeInteger(attached?.totalElements), attached?.totalElements], total_matches_content: [Array.isArray(attached?.content) && attached.totalElements === attached.content.length], pages_integer: [Number.isSafeInteger(attached?.totalPages), attached?.totalPages], pages_range: [Number.isSafeInteger(attached?.totalPages) && attached.totalPages >= 0 && attached.totalPages <= 1], unique_ids: [Array.isArray(attached?.content) && new Set(attached.content.map(value => value?.id)).size === attached.content.length], - attachment_rows: [Array.isArray(attached?.content) && !attached.content.some(value => !id(value?.id) || value?.missed === true || value?.fromTestCase === true)], + ids_valid: [Array.isArray(attached?.content) && attached.content.every(value => id(value?.id))], + not_inherited: [Array.isArray(attached?.content) && !attached.content.some(value => value?.fromTestCase === true)], + attachment_rows: [Array.isArray(attached?.content) && !attached.content.some(value => !id(value?.id) || value?.fromTestCase === true)], })); - if (now() < deadline && result.status === exported.status && same(signature(attached.content), signature(exported.attachmentMetadata))) { + const withinDeadline = now() < deadline; + const missedCount = attached.content.filter(value => value?.missed === true).length; + const signaturesMatch = same(signature(attached.content), signature(exported.attachmentMetadata)); + const resultStatusMatches = result.status === exported.status; + const ready = withinDeadline && resultStatusMatches && missedCount === 0 && signaturesMatch; + attachmentReadiness.push({ poll: poll + 1, attachmentCount: attached.content.length, missedCount, signaturesMatch, resultStatusMatches, withinDeadline, ready }); + if (ready) { return { schema: 'paperclip.chat-feature-testops-upload-verification/v1', ...context, projectId: 1, planId: selected.planId, jobId: selected.jobId, allureJobRunId: selected.allureJobRunId, launchId: selected.launchId, caseId: selected.caseId, resultId: result.id, executionId, status: result.status, cleanup: exported.cleanup, verification: 'VERIFIED', - attachments: exported.attachmentMetadata.map(value => ({ ...value })), + attachments: exported.attachmentMetadata.map(value => ({ ...value })), attachmentReadiness, launchUrl: `${endpoint}/launch/${selected.launchId}`, resultUrl: `${endpoint}/testresult/${result.id}` }; } } if (poll < 11 && now() + 5000 < deadline) await wait(5000); } - fail('chat_testops_upload_unverified'); + unverified(); } diff --git a/tests/runner-e2e/chat-feature-testops-api.test.mjs b/tests/runner-e2e/chat-feature-testops-api.test.mjs index ef86e1e2c58..292411c1d22 100644 --- a/tests/runner-e2e/chat-feature-testops-api.test.mjs +++ b/tests/runner-e2e/chat-feature-testops-api.test.mjs @@ -716,3 +716,115 @@ test('bootstrap unknown transport errors retain only source-owned request class assert.ok(!JSON.stringify(projected).includes('PRIVATE')); return true; }); }); + +test('attachment readiness waits for a missed row to become complete without changing the final evidence predicate', async () => { + const { verifyChatUpload } = await import('./chat-feature-testops-api.mjs'); + const base = uploadApi(); let pages = 0, waits = 0; + const result = await verifyChatUpload(async (...args) => { + const value = await base(...args); + if (args[0].startsWith('/api/testresult/attachment?')) { + pages++; + if (pages === 1) value.content[0].missed = true; + } + return value; + }, uploadContext, uploadSelection, uploadReceipt, { now: () => 0, wait: async ms => { assert.equal(ms, 5000); waits++; } }); + assert.equal(result.verification, 'VERIFIED'); + assert.equal(result.status, 'passed'); + assert.deepEqual(result.attachments, uploadReceipt.attachmentMetadata); + assert.equal(pages, 2); assert.equal(waits, 1); + assert.deepEqual(result.attachmentReadiness.map(row => row.missedCount), [1, 0]); +}); + +test('permanently missed attachments fail at the original poll bound with closed, detached observations', async () => { + const { verifyChatUpload } = await import('./chat-feature-testops-api.mjs'); + const base = uploadApi(); let pages = 0, waits = 0; + await assert.rejects(verifyChatUpload(async (...args) => { + const value = await base(...args); + if (args[0].startsWith('/api/testresult/attachment?')) { + pages++; value.content[0].missed = true; value.content[0].privateData = 'DO_NOT_RETAIN_ATTACHMENT_DATA'; + } + return value; + }, uploadContext, uploadSelection, uploadReceipt, { now: () => 0, wait: async ms => { assert.equal(ms, 5000); waits++; } }), error => { + assert.equal(error.code, 'chat_testops_upload_unverified'); + const projected = failureProjection(error, 'upload_verification'); + assert.equal(projected.verification.phase, 'attachment_readiness'); + assert.equal(projected.verification.firstFailedPredicate, 'attachments_not_missed'); + assert.equal(projected.verification.attachmentReadiness.length, 12); + assert.ok(projected.verification.attachmentReadiness.every(row => row.missedCount === 1 && row.ready === false)); + assert.ok(!JSON.stringify(projected).includes('DO_NOT_RETAIN')); + projected.verification.attachmentReadiness[0].missedCount = 999; + assert.equal(failureProjection(error, 'upload_verification').verification.attachmentReadiness[0].missedCount, 1); + return true; + }); + assert.equal(pages, 12); assert.equal(waits, 11); +}); + +test('attachment readiness cannot extend the original 120 second observation deadline', async () => { + const { verifyChatUpload } = await import('./chat-feature-testops-api.mjs'); + const base = uploadApi(); let clock = 0, calls = 0; + await assert.rejects(verifyChatUpload(async (...args) => { + calls++; const value = await base(...args); + if (args[0].startsWith('/api/testresult/attachment?')) value.content[0].missed = true; + return value; + }, uploadContext, uploadSelection, uploadReceipt, { now: () => clock, wait: async () => { clock = 120_000; } }), /upload_unverified/); + assert.equal(calls, 5); +}); + +test('malformed or inherited attachment rows still fail immediately even when an attachment is missed', async () => { + const { verifyChatUpload } = await import('./chat-feature-testops-api.mjs'); + for (const change of [{ id: '1' }, { id: 0 }, { fromTestCase: true }]) { + const base = uploadApi(); let pages = 0; + await assert.rejects(verifyChatUpload(async (...args) => { + const value = await base(...args); + if (args[0].startsWith('/api/testresult/attachment?')) { pages++; Object.assign(value.content[0], change, { missed: true }); } + return value; + }, uploadContext, uploadSelection, uploadReceipt, { now: () => 0, wait: async () => assert.fail('invalid identity must not wait') }), error => { + assert.equal(error.code, 'chat_testops_upload_mismatch'); + assert.equal(failureProjection(error, 'upload_verification').verification.phase, 'attachment_page'); return true; + }); + assert.equal(pages, 1); + } +}); + +test('a completed attachment with the wrong byte count never satisfies the final evidence signature', async () => { + const { verifyChatUpload } = await import('./chat-feature-testops-api.mjs'); + const base = uploadApi(); let pages = 0; + await assert.rejects(verifyChatUpload(async (...args) => { + const value = await base(...args); + if (args[0].startsWith('/api/testresult/attachment?')) { pages++; value.content[0].contentLength++; } + return value; + }, uploadContext, uploadSelection, uploadReceipt, { now: () => 0, wait: async () => {} }), error => { + assert.equal(error.code, 'chat_testops_upload_unverified'); + const result = failureProjection(error, 'upload_verification'); + assert.equal(result.verification.firstFailedPredicate, 'attachment_signatures_match'); return true; + }); + assert.equal(pages, 12); +}); + +test('a null attachment row retains the controlled invalid-ID rejection', async () => { + const { verifyChatUpload } = await import('./chat-feature-testops-api.mjs'); + const base = uploadApi(); + await assert.rejects(verifyChatUpload(async (...args) => { + if (args[0].startsWith('/api/testresult/attachment?')) return { content: [null], totalElements: 1, totalPages: 1 }; + return base(...args); + }, uploadContext, uploadSelection, uploadReceipt, { now: () => 0, wait: async () => assert.fail('malformed rows must not wait') }), error => { + assert.equal(error.code, 'chat_testops_upload_mismatch'); + assert.equal(failureProjection(error, 'upload_verification').verification.firstFailedPredicate, 'ids_valid'); return true; + }); +}); + +test('deadline crossing after a complete attachment GET keeps the exact readiness refusal', async () => { + const { verifyChatUpload } = await import('./chat-feature-testops-api.mjs'); + const base = uploadApi(); let lastGet = false, reads = 0; + await assert.rejects(verifyChatUpload(async (...args) => { + const value = await base(...args); + if (args[0].startsWith('/api/testresult/attachment?')) lastGet = true; + return value; + }, uploadContext, uploadSelection, uploadReceipt, { now: () => lastGet && ++reads >= 2 ? 120_000 : 0, + wait: async () => assert.fail('expired observation must not wait') }), error => { + assert.equal(error.code, 'chat_testops_upload_unverified'); + const projected = failureProjection(error, 'upload_verification'); + assert.equal(projected.verification.firstFailedPredicate, 'attachments_ready'); + assert.equal(projected.verification.attachmentReadiness[0].withinDeadline, false); return true; + }); +}); diff --git a/tests/runner-e2e/chat-feature-testops-retained-verification.node-test.mjs b/tests/runner-e2e/chat-feature-testops-retained-verification.node-test.mjs index e3798e2d9b4..c30b9dc72e8 100644 --- a/tests/runner-e2e/chat-feature-testops-retained-verification.node-test.mjs +++ b/tests/runner-e2e/chat-feature-testops-retained-verification.node-test.mjs @@ -91,8 +91,9 @@ test('actual three-way attachment row refusal retains the precise current protec const values = responses(); Object.assign(values[4].content[0], change, { secret: 'do-not-retain', source: 'unknown-private-url' }); const result = await snapshotAndReplay(async route => values[routes.indexOf(route)], selection, receipt); - assert.equal(result.failure.verification.phase, 'attachment_page'); - assert.equal(result.failure.verification.firstFailedPredicate, 'attachment_rows'); + assert.equal(result.failure.verification.phase, change.missed === true ? 'attachment_readiness' : 'attachment_page'); + assert.equal(result.failure.verification.firstFailedPredicate, + change.missed === true ? 'attachments_not_missed' : change.fromTestCase === true ? 'not_inherited' : 'ids_valid'); const safe = JSON.stringify(result); assert.ok(!safe.includes('do-not-retain') && !safe.includes('unknown-private-url')); assert.equal(result.verification, undefined);