From ae0d9d289831c770a413658d3520fb1dc8ad60e3 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Fri, 30 Oct 2026 15:51:28 -0500 Subject: [PATCH] fix(evals): batch task search result readback across renders (#18370) ## Thinking Path > - Paperclip lets people manage AI agents and saved work. > - Product journeys check user actions against saved state. > - The task search journey compares the exact visible task identities. > - A search can replace rows while the harness reads an old indexed locator. > - The attribute read can then time out after the correct results appear. > - This pull request reads the visible links together and preserves strict result checks. ## Linked Issues or Issue Description Refs #18355. **What happened?** An attached Cloud attempt passed the task search journey. A later attempt on the same harness and app revision failed at search-result readback. Its owned screenshot showed the correct target and completed control. Authentication, collection, and cleanup passed. The failure remains recorded and the journey has not been admitted to the daily plan. The reader called `all()`, then awaited `isVisible()` and `getAttribute()` for each indexed locator. When a search replaced the old collection between those reads, the next attribute read waited for an index that no longer existed. **Expected behavior** Read the matching visible links together. Compare the complete identity set against the owned target and controls. A render transition must not consume the whole poll deadline on a stale row index. **Steps to reproduce** 1. Render three task links in the task collection. 2. Let the reader observe the third link as visible. 3. Replace the collection with the two correct search matches before its attribute read. 4. The old reader times out waiting for `nth(2)`, even though the two correct rows are present. ## What Changed - Read visible link attributes in one batch. Discard a detached snapshot and retry within the existing poll deadline. - Keep the exact identity comparison, duplicate detection, control exclusion, saved-content checks, and zero-run checks. - Add a real Chromium render-transition regression and invalid-result cases. Extend the stateful flow test for a detached snapshot. - Document the readback boundary. No app UI, target configuration, or cleanup behavior changes. ## Verification - Review follow-up: the existing journey wait helper is now exercised with the real Playwright poll. Its first read evaluates the actual reader callback on detached browser DOM nodes and returns no evidence. The second read contains an extra owned control. Only the third, exact pair succeeds. The test observes all three reads. The 30-second deadline and grading behavior are unchanged. - Rebased onto current master, including the merged project journey. Credential-free discovery confirms exactly one `project-task-context` and one `task-search-filter` entry. - Latest rebased checks passed: all nine Chromium search regressions, all 381 Product journey unit tests, runner typecheck, discovery, maps, and diff checks. Exact head 183c3e1d19cf77c933632bc3774a5e2ec1547377 passed all 58 CI checks, external Snyk status, and fresh Greptile 5/5 with no unresolved findings. - Red: the real Chromium transition test failed on the old reader with `locator.getAttribute: Timeout 150ms exceeded`, waiting for `nth(2)`. The short timeout bounds only this regression fixture. - Green: all nine credential-free Chromium search tests passed. They cover the transition, duplicate/extra/unknown/wrong destinations, visible-link scope, and status-label behavior. - All 131 focused search/oracle/contract/campaign tests passed. - Runner typecheck passed: `node node_modules/typescript/bin/tsc -p tests/runner-e2e/tsconfig.json`. - `node scripts/check-maps.mjs` and `git diff --check` passed. - Fresh QA staging acceptance passed at this exact head: all five checks, authentication/setup/journey/collection/cleanup phases, and zero agent runs. The [measured QA report](https://pages.paperclip.ing/evals-qa-smoke-41124961876-1/) preserves current source and measurement provenance. Daily admission follows merge and a normal TestOps job verification; the earlier failure remains recorded. ## Risks - The fix uses the existing Playwright visible-element predicate and task-link selector. A future UI selector change can still fail the journey. - A detached snapshot returns no evidence. It cannot satisfy a nonempty expected result set. The original poll deadline stays in place; there is no added journey retry. - Browser fixtures prove the readback mechanism. They do not replace the required live Cloud pass. ## Model Used OpenAI Codex, GPT-6 family. The host does not expose an exact model version or context limit. Used repository tools, TypeScript checks, and fresh local Chromium contexts. ## 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/PRODUCT-JOURNEYS.md | 8 +- .../task-search-filter.test.ts | 16 ++- .../product-journeys/task-search-filter.ts | 21 ++-- tests/runner-e2e/task-search-filter.spec.ts | 102 +++++++++++++++++- 4 files changed, 129 insertions(+), 18 deletions(-) diff --git a/tests/runner-e2e/PRODUCT-JOURNEYS.md b/tests/runner-e2e/PRODUCT-JOURNEYS.md index ef7abc55ee9..895005cd15a 100644 --- a/tests/runner-e2e/PRODUCT-JOURNEYS.md +++ b/tests/runner-e2e/PRODUCT-JOURNEYS.md @@ -84,8 +84,12 @@ Unexpected browser/API errors retain only a closed `task_search__failed` code. Stage names come from the journey source; raw exception text, URLs, form values, and response bodies are not added to reporting. Existing owned evidence failures keep their specific codes and the screenshot privacy guard stays in place. -It does not depend on row order or an otherwise empty company. This selection is -manual until a separate target owner records a fresh pass and admits it to a daily +It does not depend on row order or an otherwise empty company. The browser reads +the visible links together. Per-row indexed locators can disappear +while a search replaces the old collection; a detached snapshot is retried within +the existing poll deadline. Duplicates, unknown destinations, and extra controls +remain evidence failures. This does not add a journey retry or extend its timeout. +The selection stays manual until a separate target owner records a fresh pass and admits it to a daily plan; adding the shared journey does not qualify any deployed target. Supported acquisitions are managed self-hosted (`local`), registered existing diff --git a/tests/runner-e2e/product-journeys/task-search-filter.test.ts b/tests/runner-e2e/product-journeys/task-search-filter.test.ts index d4646725f38..5753f30d727 100644 --- a/tests/runner-e2e/product-journeys/task-search-filter.test.ts +++ b/tests/runner-e2e/product-journeys/task-search-filter.test.ts @@ -16,9 +16,9 @@ vi.mock("@playwright/test", () => ({ expect: Object.assign( type Fault = "ignore-search" | "ignore-status" | "duplicate-result" | "unknown-result" | "reload-query" | "reload-status" | "back-query" | "back-status" | "wrong-route" | "reload-content" | "saved-company" | "saved-description" | "target-run" | "control-run" | "missing-run-evidence" - | "intent" | "receipt" | "missing-identifier" | "ignore-named-view"; + | "intent" | "receipt" | "missing-identifier" | "ignore-named-view" | "detached-snapshot"; type UiNode = { key: string; role?: string; name?: string; title?: string; text?: string; href?: string; - visible?: boolean; children?: UiNode[]; click?(): void; fill?(value: string): void; check?(): void; + visible?: boolean; connected?: boolean; children?: UiNode[]; click?(): void; fill?(value: string): void; check?(): void; value?(): string; checked?(): boolean }; type CollectionState = { query: string; backlog: boolean }; @@ -59,6 +59,7 @@ function fixture(fault?: Fault, shell: "coordinator" | "streamlined" | "legacy" }; const links = (): UiNode[] => displayedId || !allTasks ? [] : visibleTasks().map((task, index) => ({ key: `task-${task.id}-${index}`, role: "link", name: task.title, text: task.title, + connected: fault !== "detached-snapshot" || task.id !== "task-2", href: `/QA/issues/${task.identifier}`, click: () => openTask(task), })); const nodes = (): UiNode[] => { @@ -107,7 +108,10 @@ function fixture(fault?: Fault, shell: "coordinator" | "streamlined" | "legacy" resolve, or(other: { resolve(): UiNode[] }) { return locator(() => [...new Map([...resolve(), ...other.resolve()].map(node => [node.key, node])).values()]); }, first() { return locator(() => resolve().slice(0, 1)); }, - filter({ hasText }: { hasText: string }) { return locator(() => resolve().filter(node => node.text?.includes(hasText))); }, + filter({ hasText, visible }: { hasText?: string; visible?: boolean }) { + return locator(() => resolve().filter(node => (hasText === undefined || node.text?.includes(hasText)) + && (visible !== true || node.visible !== false))); + }, locator(selector: string) { if (selector !== "..") throw Error(`Unsupported control-relative selector: ${selector}`); return locator(() => { @@ -122,6 +126,10 @@ function fixture(fault?: Fault, shell: "coordinator" | "streamlined" | "legacy" return locator(() => flatten(resolve().flatMap(node => node.children ?? [])).filter(node => node.role === role && matches(node.name, name))); }, async all() { return resolve().map(node => locator(() => [node])); }, + async evaluateAll(read: (nodes: Array<{ isConnected: boolean; getAttribute(name: string): string | null }>) => R) { + return read(resolve().map(node => ({ isConnected: node.connected !== false, + getAttribute(name: string) { expect(name).toBe("href"); return node.href ?? null; } }))); + }, async isVisible() { return resolve().length > 0; }, async getAttribute(name: string) { expect(name).toBe("href"); return unique().href ?? null; }, async inputValue() { const read = unique().value; if (!read) throw Error("Not an input"); return read(); }, @@ -229,7 +237,7 @@ describe("taskSearchFilter browser journey", () => { expect(test.evidence.checks).toEqual([]); }); it.each(["ignore-search", "ignore-status", "duplicate-result", "unknown-result", "reload-query", "reload-status", - "back-query", "back-status", "reload-content"])("does not report success when the UI has %s", async fault => { + "back-query", "back-status", "reload-content", "detached-snapshot"])("does not report success when the UI has %s", async fault => { const test = fixture(fault); await expect(test.run()).rejects.toThrow(); expect(test.evidence.checks.some(check => check.passed)).toBe(false); diff --git a/tests/runner-e2e/product-journeys/task-search-filter.ts b/tests/runner-e2e/product-journeys/task-search-filter.ts index 2bdabcb0061..245e031f1ee 100644 --- a/tests/runner-e2e/product-journeys/task-search-filter.ts +++ b/tests/runner-e2e/product-journeys/task-search-filter.ts @@ -12,20 +12,21 @@ export function taskSearchBacklogCheckbox(filters: Locator) { .getByRole("checkbox", { name: /^Backlog(?:\s*Backlog)?$/ }); } -async function resultIds(page: Page, tasks: readonly SearchTask[]) { - const ids: string[] = []; - for (const link of await page.locator("main#main-content a[data-inbox-issue-link]").all()) { - if (!await link.isVisible()) continue; - const href = await link.getAttribute("href"); +export async function taskSearchResultIds(page: Page, tasks: readonly SearchTask[]) { + // Read the matched nodes together. Indexed locators from all() can disappear + // between isVisible() and getAttribute() while the search replaces its rows. + const hrefs = await page.locator("main#main-content a[data-inbox-issue-link]").filter({ visible: true }) + .evaluateAll(links => links.some(link => !link.isConnected) ? null : links.map(link => link.getAttribute("href"))); + if (hrefs === null) return []; + return hrefs.map(href => { const identifier = href ? decodeURIComponent(new URL(href, page.url()).pathname.split("/").at(-1)!) : ""; - ids.push(tasks.find(task => task.identifier === identifier || task.id === identifier)?.id ?? `unknown:${identifier}`); - } - return ids.sort(); + return tasks.find(task => task.identifier === identifier || task.id === identifier)?.id ?? `unknown:${identifier}`; + }).sort(); } -async function expectResults(page: Page, tasks: readonly SearchTask[], expectedIds: readonly string[]) { +export async function expectResults(page: Page, tasks: readonly SearchTask[], expectedIds: readonly string[]) { let observed: string[] = []; - await expect.poll(async () => { observed = await resultIds(page, tasks); return observed; }, { timeout: 30_000 }) + await expect.poll(async () => { observed = await taskSearchResultIds(page, tasks); return observed; }, { timeout: 30_000 }) .toEqual([...expectedIds].sort()); return observed; } diff --git a/tests/runner-e2e/task-search-filter.spec.ts b/tests/runner-e2e/task-search-filter.spec.ts index 3dc8a2172f9..ce0df6860a9 100644 --- a/tests/runner-e2e/task-search-filter.spec.ts +++ b/tests/runner-e2e/task-search-filter.spec.ts @@ -1,5 +1,6 @@ -import { expect, test } from "@playwright/test"; -import { taskSearchBacklogCheckbox } from "./product-journeys/task-search-filter.js"; +import { expect, test, type Locator, type Page } from "@playwright/test"; +import { expectResults, taskSearchBacklogCheckbox, taskSearchResultIds } from "./product-journeys/task-search-filter.js"; +import type { SearchTask } from "./product-journeys/task-search-evidence.js"; // Reproduce the observed Radix checkbox + StatusIcon + visible text label. // No app server, provider, browser profile, or target credentials are involved. @@ -31,3 +32,100 @@ for (const presentation of ["legacy", "spaced-legacy", "refreshed"] as const) { await expect(page.locator("#assignee-backlog")).not.toBeChecked(); }); } + +const searchTasks: SearchTask[] = ["target", "status-control", "search-control"].map((name, index) => ({ + id: name, identifier: `QA-${index + 1}`, companyId: "company", title: name, description: `${name} context`, + status: index === 1 ? "done" : "backlog", assigneeUserId: "board", assigneeAgentId: null, +})); +const taskLink = (task: SearchTask) => `${task.title}`; + +test("task search readback survives a search rerender that removes an observed row", async ({ page }) => { + page.setDefaultTimeout(150); // Bound only the reproduced stale-index wait. + await page.setContent(`
${searchTasks.map(taskLink).join("")}
`); + let transitions = 0; + let batches = 0; + let detachedReads = 0; + let invalidReads = 0; + const transition = async () => { + if (transitions++) return; + await page.locator("main#main-content").evaluate((main, html) => { main.innerHTML = html; }, searchTasks.slice(0, 2).map(taskLink).join("")); + }; + // Keep browser operations and the journey's poll real. Hold actual DOM nodes + // across the first render so the reader must recover from a detached batch. + const withTransition = (source: Locator): Locator => new Proxy(source, { + get(object, property, receiver) { + if (property === "all") return async () => (await object.all()).map((row, index) => index !== 2 ? row : new Proxy(row, { + get(element, operation, elementReceiver) { + if (operation === "isVisible") return async (...args: Parameters) => { + const visible = await element.isVisible(...args); + if (visible) await transition(); + return visible; + }; + const value = Reflect.get(element, operation, elementReceiver); + return typeof value === "function" ? value.bind(element) : value; + }, + })); + if (property === "filter") return (...args: Parameters) => withTransition(object.filter(...args)); + if (property === "evaluateAll") return async (...args: Parameters) => { + batches++; + if (batches === 1) { + const held = await page.evaluateHandle(() => Array.from(document.querySelectorAll("main#main-content a[data-inbox-issue-link]"))); + try { + await transition(); + const detached = await held.evaluate(...args); + expect(detached).toBeNull(); + detachedReads++; + // A later connected snapshot can still be wrong. The same poll + // must reject its extra control before accepting the final pair. + await page.locator("main#main-content").evaluate((main, html) => { main.innerHTML = html; }, searchTasks.map(taskLink).join("")); + return detached; + } finally { await held.dispose(); } + } + const result = await object.evaluateAll(...args); + if (batches === 2) { + expect(result).toHaveLength(3); + invalidReads++; + await page.locator("main#main-content").evaluate((main, html) => { main.innerHTML = html; }, searchTasks.slice(0, 2).map(taskLink).join("")); + } + return result; + }; + const value = Reflect.get(object, property, receiver); + return typeof value === "function" ? value.bind(object) : value; + }, + }); + const observedPage = new Proxy(page, { + get(object, property, receiver) { + if (property === "locator") return (...args: Parameters) => withTransition(object.locator(...args)); + const value = Reflect.get(object, property, receiver); + return typeof value === "function" ? value.bind(object) : value; + }, + }); + await expect(expectResults(observedPage, searchTasks, ["target", "status-control"])) + .resolves.toEqual(["status-control", "target"]); + expect(transitions).toBe(1); + expect(detachedReads).toBe(1); + expect(invalidReads).toBe(1); + expect(batches).toBe(3); + await expect(page.locator("a[data-inbox-issue-link]")).toHaveCount(2); +}); + +for (const fixture of [ + { name: "extra owned control", html: searchTasks.map(taskLink).join(""), ids: ["search-control", "status-control", "target"] }, + { name: "duplicate target", html: [...searchTasks.slice(0, 2), searchTasks[0]!].map(taskLink).join(""), ids: ["status-control", "target", "target"] }, + { name: "unknown destination", html: searchTasks.slice(0, 2).map(taskLink).join("") + taskLink({ ...searchTasks[2]!, identifier: "QA-99" }), ids: ["status-control", "target", "unknown:QA-99"] }, + { name: "wrong target destination", html: taskLink(searchTasks[1]!) + taskLink({ ...searchTasks[0]!, identifier: "QA-2" }), ids: ["status-control", "status-control"] }, +]) { + test(`task search snapshot preserves invalid evidence: ${fixture.name}`, async ({ page }) => { + await page.setContent(`
${fixture.html}
`); + const ids = await taskSearchResultIds(page, searchTasks); + expect(ids).toEqual(fixture.ids); + expect(ids).not.toEqual(["status-control", "target"]); + }); +} + +test("task search snapshot retains the existing visible-link scope", async ({ page }) => { + await page.setContent(`
${searchTasks.slice(0, 2).map(taskLink).join("")} +
+ ${taskLink({ ...searchTasks[2]!, identifier: "QA-99" })}`); + expect(await taskSearchResultIds(page, searchTasks)).toEqual(["status-control", "target"]); +});