Report what the app said and did, not just how the page moved - #129
Open
DavertMik wants to merge 2 commits into
Open
Report what the app said and did, not just how the page moved#129DavertMik wants to merge 2 commits into
DavertMik wants to merge 2 commits into
Conversation
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>
This was referenced Aug 21, 2026
DenysKuchma
requested changes
Aug 21, 2026
| 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]) { |
Collaborator
There was a problem hiding this comment.
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
|
|
||
| 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; |
Collaborator
There was a problem hiding this comment.
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A click that fires a request the server rejects looks like a success today.
pageDiffdescribes DOM and accessibility-tree movement only, so the toast carrying the server's error is either buried in raw markup —collapseHtmlPartsreplaces 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.ariaChangescannot carry it at all:nodeToEntrydrops 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/… → 400in<state>, with no page context and no link to the action that caused it. A bare 400 reads as a missing value. In traceecad7a69that 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 fromhtmlDiff.added, which already computed it and threw it away.requests— same-origin xhr/fetch made during the action, with status.captureMainDocumentResponsealready 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 withrequestStore.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, whichanalyzeProgressalready 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:Bug found on the way
Browser console messages were never recorded.
grabBrowserLogs()returns PlaywrightConsoleMessageobjects whosetypeandtextare methods, andcaptureBrowserLogsfiltered with['info','error','warning','warn'].includes(log.type)— comparing a function against a list of strings, always false. Every entry was discarded, which is whyconsole errorsreadnonefor a whole session while the page was logging its own failure. Both fields are normalized at capture;tests/unit/action-browser-logs.test.tslocks it down.Deliberately out of scope
htmlDiffruns 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.requestfailed, notresponse, so they carry no status and do not appear.XhrCapture, to keep third-party analytics out of every tool result.Testing
bun test tests/unit(1037 pass) andbun test tests/integration(81 pass), plusformat:checkandlint, all verified against this commit in a clean worktree. New coverage inpage-diff-evidence,pilot-action-evidenceandaction-browser-logs.This changes what the models see on every action, so it is worth a
regressionlabel before merging — I have not applied one.🤖 Generated with Claude Code