mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-01 06:42:06 +01:00
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 183c3e1d19
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 <noreply@paperclip.ing>
This commit is contained in:
1 parent
e795866d1e
commit
ae0d9d2898
4 files changed
+129
-18
No files matched your search
@@ -84,8 +84,12 @@ Unexpected browser/API errors retain only a closed `task_search_<stage>_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
|
||||
|
||||
@@ -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<R>(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<Fault>(["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);
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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) => `<a data-inbox-issue-link href="https://paperclip.example.test/QA/issues/${task.identifier}">${task.title}</a>`;
|
||||
|
||||
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(`<main id="main-content">${searchTasks.map(taskLink).join("")}</main>`);
|
||||
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<Locator["isVisible"]>) => {
|
||||
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<Locator["filter"]>) => withTransition(object.filter(...args));
|
||||
if (property === "evaluateAll") return async (...args: Parameters<Locator["evaluateAll"]>) => {
|
||||
batches++;
|
||||
if (batches === 1) {
|
||||
const held = await page.evaluateHandle(() => Array.from(document.querySelectorAll<HTMLAnchorElement>("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<Page["locator"]>) => 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(`<main id="main-content">${fixture.html}</main>`);
|
||||
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(`<main id="main-content">${searchTasks.slice(0, 2).map(taskLink).join("")}
|
||||
<div hidden>${taskLink(searchTasks[2]!)}</div></main>
|
||||
${taskLink({ ...searchTasks[2]!, identifier: "QA-99" })}`);
|
||||
expect(await taskSearchResultIds(page, searchTasks)).toEqual(["status-control", "target"]);
|
||||
});
|
||||
Reference in new issue
Block a user