fix: stop returning snapshot filter errors to the model as page content - #174
Open
karaposu wants to merge 1 commit into
Open
fix: stop returning snapshot filter errors to the model as page content#174karaposu wants to merge 1 commit into
karaposu wants to merge 1 commit into
Conversation
When the filtered ARIA snapshot path failed to parse, filter_snapshot's catch returned "Error filtering snapshot: <message>\n<stack>" in the same slot as the page's interactive elements. The snapshot tool prints that string verbatim under an "Interactive Elements:" header, so the model received a JavaScript stack trace presented as the page, with no signal it was an error. The realistic trigger is Playwright's ariaSnapshot returning a non-string shape (the same API drift brightdata#158/brightdata#160 dealt with). Two changes, atomic by design: - filter_snapshot no longer catches: a parse failure throws. The "No interactive elements found" empty case stays -- that is a value (a parsed page with nothing interactive), not an error. - capture_snapshot wraps only the filter call. On a throw it logs the full stack to stderr and returns the unfiltered snapshot -- real content already in hand -- prefixed with a model-visible "[note: snapshot filtering failed (...)]" line, in the same result shape as filtered:false, so the tool renders it with no changes. The DOM pass is skipped on the degraded path; refs from the full snapshot still resolve through ref_locator's aria-ref branch, so the click/type workflow keeps working after a fallback. A non-string snapshot shows "(no snapshot text available)" instead of the word "undefined". This also makes flipping the tool's filtered default to true (as PR brightdata#163's bb37007 does on dev) safe: with the default on, every snapshot rides the filter path, and a filter bug would otherwise sit on the main road. Note for merge ordering: bb37007 edits the same capture_snapshot function (DOM-ref renumbering); the changes are logically independent. Adds test/aria-snapshot-filter.test.js: the parser/format matrix (verified against the real parser before the fix), the throw case (fails on the pre-fix code, which returned the error string), and a fallback wiring test using an injected fake page -- asserting note + full content, no DOM pass, and an intact result shape.
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.
Problem
When the filtered ARIA snapshot path fails to parse,
Aria_snapshot_filter.filter_snapshot's catchreturns
Error filtering snapshot: <message>\n<stack>in the same slot as the page's interactiveelements. The snapshot tool prints that string verbatim under its
Interactive Elements:header — sothe model receives a JavaScript stack trace presented as the page, with no signal it's an error, and
reasons about garbage. The realistic trigger is Playwright's
ariaSnapshotreturning a non-stringshape (the same API drift #158/#160 dealt with).
Fix (two changes, atomic by design)
filter_snapshotno longer catches: a parse failure throws. The'No interactive elements found'empty case stays — that is a value, not an error.capture_snapshotwraps only the filter call. On a throw it logs the full stack to stderr andreturns the unfiltered snapshot — real content already in hand — prefixed with a model-visible
[note: snapshot filtering failed (…)]line, in the same result shape asfiltered:false, so thetool renders it with no changes. Refs from the full snapshot still resolve through
ref_locator'saria-refbranch, so the click/type workflow keeps working after a fallback. A non-string snapshotshows
(no snapshot text available)instead of the wordundefined.Why this matters for the
filtered:truedefaultThis also makes flipping the snapshot tool's
filtereddefault totrue(as #163'sbb37007does)safe: with the default on, every snapshot rides the filter path, and a filter bug would otherwise sit
on the main road. Merge note:
bb37007edits the samecapture_snapshotfunction (DOM-refrenumbering) — the changes are logically independent.
Tests
test/aria-snapshot-filter.test.js: the parser/format matrix (verified against the real parserbefore being written), the throw case (fails on the pre-fix code, which returned the error string),
and a fallback wiring test using an injected fake page — asserting note + full content, no DOM pass,
and an intact result shape.