Skip to content

Report what the app said and did, not just how the page moved - #129

Open
DavertMik wants to merge 2 commits into
mainfrom
feat/page-diff-messages-and-requests
Open

Report what the app said and did, not just how the page moved#129
DavertMik wants to merge 2 commits into
mainfrom
feat/page-diff-messages-and-requests

Conversation

@DavertMik

Copy link
Copy Markdown
Contributor

A click that fires a request the server rejects looks like a success today. pageDiff describes DOM and accessibility-tree movement only, so the toast carrying the server's error is either buried in raw markup — collapseHtmlParts replaces every part once the diff exceeds 8000 chars or 8 parts, which a real app re-render does routinely — or stripped outright by conversation compaction. ariaChanges cannot carry it at all: nodeToEntry drops every non-interactive role, so an alert never reaches the model.

Meanwhile the failed request reaches Pilot only as a session-wide network errors: POST /api/… → 400 in <state>, with no page context and no link to the action that caused it. A bare 400 reads as a missing value. In trace ecad7a69 that is exactly what happened: Pilot twice told the tester to fill in a Title that had been filled five minutes earlier, and the run spent 21 clicks and 8 minutes on a Save the server had already refused.

What changes

Three new fields on PageDiff, all scoped to the action that produced them:

  • messages — text the app put on the page in response. Live-region content first (role=alert|alertdialog|status|log, aria-live, <output>), then any other text that appeared. The second tier is what catches notifications built with no ARIA at all, which is the common case. Built from htmlDiff.added, which already computed it and threw it away.
  • requests — same-origin xhr/fetch made during the action, with status. captureMainDocumentResponse already watched the action's exact window for the main document status; it now records these too, so there is no second listener and no interaction with requestStore.clear().
  • consoleErrors — what the page logged while the action ran. grabBrowserLogs() drains its store, so this was already per-action; nothing surfaced it.

A 4xx/5xx now leads the tool's suggestion and points at the message the user was shown, instead of leaving the model to read the click as successful and try again.

Pilot reads the same evidence through formatActions, which analyzeProgress already sends — so it arrives attributed to the action, and narrowed to failure signal: rejected requests only, the first two messages, one console error. The review that misfired would have read:

[SUCCESS] ACTION click: Save
   requests: POST /api/zephyr/plans → 400
   messages: Operation against a key holding the wrong kind of value
   console: Ember Data Request POST /api/zephyr/plans returned a 400

Bug found on the way

Browser console messages were never recorded. grabBrowserLogs() returns Playwright ConsoleMessage objects whose type and text are methods, and captureBrowserLogs filtered with ['info','error','warning','warn'].includes(log.type) — comparing a function against a list of strings, always false. Every entry was discarded, which is why console errors read none for a whole session while the page was logging its own failure. Both fields are normalized at capture; tests/unit/action-browser-logs.test.ts locks it down.

Deliberately out of scope

  • htmlDiff runs only when the URL is unchanged, so a flash message shown after a redirect is not picked up — context re-injection already covers that case.
  • Requests that never get a response (connection refused, aborted) fire requestfailed, not response, so they carry no status and do not appear.
  • Cross-origin calls are filtered out, matching XhrCapture, to keep third-party analytics out of every tool result.

Testing

bun test tests/unit (1037 pass) and bun test tests/integration (81 pass), plus format:check and lint, all verified against this commit in a clean worktree. New coverage in page-diff-evidence, pilot-action-evidence and action-browser-logs.

This changes what the models see on every action, so it is worth a regression label before merging — I have not applied one.

🤖 Generated with Claude Code

DavertMik and others added 2 commits August 21, 2026 03:20
A click that fired a request the server rejected looked like a success. The
page diff described DOM and accessibility-tree movement only: a toast carrying
the server's error was either buried in raw markup that gets collapsed on a
re-render, or dropped by conversation compaction, and the rejected request
reached the Pilot much later as a bare session-wide status code.

Every action now carries three more pieces of evidence in its page diff:

- messages: text the app put on the page in response. Live-region content
  first (role=alert/status/log, aria-live, output), then any other text that
  appeared, which is what catches notifications built with no ARIA at all.
- requests: same-origin xhr/fetch made during the action, with status. The
  listener that already watched the action's window for the main document
  status now records these too.
- consoleErrors: what the page logged while the action ran.

A 4xx/5xx now leads the tool's suggestion and points at the message the user
was shown, instead of leaving the model to read the click as successful.

The Pilot reads the same evidence through recent_actions, attributed to the
action that caused it and narrowed to failure signal: rejected requests only,
first two messages, one console error.

Fixes browser logs never being recorded: grabBrowserLogs returns Playwright
ConsoleMessage objects whose type and text are methods, so the level filter
compared a function against a list of strings and discarded every entry. Both
are normalized at capture, which is why console errors always read as none.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every tool reads the diff off ActionResult.fromState(currentState), not off
the instance the capture produced. networkRequests survives that only through
the object spread, so a later change to fromState that lists fields explicitly
would silently empty pageDiff.requests with nothing failing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread src/utils/html-diff.ts
const appearedText = added.filter((line) => line.startsWith(TEXT_LINE_PREFIX)).map((line) => line.slice(TEXT_LINE_PREFIX.length));
const messages: string[] = [];

for (const candidate of [...collectLiveRegionTexts(originalMap, modifiedMap), ...appearedText]) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This puts any newly appeared page text into messages, not just actual app notifications. After normal navigation, headings and content from the new page may be reported to Pilot as feedback from the action. I’d only include generic appeared text when the URL hasn’t changed and handle live-region messages separately. We should also add a test for navigation between pages

Comment thread src/action.ts

const call: NetworkCall = { method: request.method(), path: url.pathname, status };
if (this.networkRequests.some((r) => r.method === call.method && r.path === call.path && r.status === call.status)) return;
if (status < 400 && this.networkRequests.length >= MAX_NETWORK_CALLS) return;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The limit currently applies only to successful requests, while failed requests can keep growing without a cap. Since this data is added to the model context, it would be safer to limit the total list while giving failed requests priority

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants