Skip to content

fix: treat research containers as scopes, recover them from children#98

Open
DavertMik wants to merge 1 commit into
mainfrom
fix/container-scope-validation
Open

fix: treat research containers as scopes, recover them from children#98
DavertMik wants to merge 1 commit into
mainfrom
fix/container-scope-validation

Conversation

@DavertMik

Copy link
Copy Markdown
Contributor

Implements plans/024-container-scope-validation.md. Follow-up to #97based on that branch, so this PR shows only its own commit. It retargets to main automatically once #97 merges.

Problem

In trace d9fd5d5a14aba27bc86cf925f57dc8a1 two of four containers were declared broken by testLocators:

  • 'nav' matched 2 elements, and testLocators required count === 1 — a perfectly usable scope marked broken
  • '.detail.detail-view-resizable' matched 0 at test time (it matched 1 in the live DOM later)

Both cascaded container broken onto every child locator without testing it, and only the AI (fixBrokenSections, 13s) repaired what was mostly a mechanical problem.

Design rule

A container is never clicked — it only scopes child lookups. Playwright searches inside the union of all matches (page.locator('nav').locator(child) looks in both navs), and children are still validated with strict count === 1.

  1. A container matching N >= 1 elements is valid. Children prove themselves individually against the union; a child duplicated across instances fails honestly with 2 elements and flows to fixBrokenSections as before.
  2. A container matching 0 elements is recovered deterministically from its children — section elements carry eidx, so resolve them to live nodes, take their lowest common ancestor, derive a stable CSS selector from its real attributes.
  3. Only if recovery fails does today's path apply: cascade + fixBrokenSections. The all-containers-broken retry now fires only when every container matches nothing and none is recoverable — genuine hallucination.

This also aligns stage 2 with validateContainers (stage 5), which already accepted count >= 1. One rule everywhere.

Explicitly out of scope: compound-selector decomposition, child-hit-count instance narrowing, any AI involvement.

Changes

  • src/utils/web-element.ts — new WebElement.commonAncestor(page, eidxList) beside fromEidxList, same evaluate/extractor pattern. Returns null when fewer than 2 distinct elements resolve, or when the walk up reaches body/html.
  • src/ai/researcher/locators.tstestLocators takes { scope?: boolean }; only the validity line changes. New public resolveContainers is the single entry point for stage-2 container handling; new private recoverContainerFromChildren builds candidates from the ancestor's real attributes (non-dynamic #id, then filteredClasses), prefers a unique match, and calls the existing updateSectionContainer so recovered sections skip the cascade automatically.
  • src/ai/researcher.ts — stage 2 calls resolveContainers; stage 4 passes { scope: true } so visual annotation uses the same rule.

Effect on the observed trace

  • 'nav' (2 matches) → valid scope; children tested against both navs, unique ones pass immediately — no cascade, no AI fix for that section
  • '.detail.detail-view-resizable' (0 matches) → children's eidx resolve to live nodes, LCA is the detail panel, recovered mechanically — the exact correction the regenerated AI report made
  • fixBrokenSections shrinks to genuinely dead locators or is skipped entirely

Test plan

  • New unit tests for WebElement.commonAncestor: nested case returns the wrapper, single-element and unresolvable cases return null, body-level case returns null
  • bun test tests/unit/ — 811 pass
  • bun test tests/integration/ — 63 pass
  • bun run check:fix — clean
  • Replay of the trace scenario against the live page (needs a real run; the mechanical path is unit-covered only)

Drive-by fix worth a look

tests/unit/web-element.test.ts now restores the window/document globals it replaces. It was already leaking its JSDOM fixture into tests/unit/explorer.test.ts, whose mocked page.evaluate runs functions against the global document — so that file was silently asserting against the leaked fixture instead of its own page, and only passed because of file ordering and which fixture happened to leak last. Adding fixtures here surfaced it. Both files now pass in either order.

🤖 Generated with Claude Code

Two of four containers in trace d9fd5d5a14aba27bc86cf925f57dc8a1 were
declared broken by testLocators, cascading `container broken` onto every
child locator without testing it and handing a mechanical problem to a
13s AI repair.

A container is never clicked -- it only scopes child lookups, and
Playwright searches inside the union of all its matches. So a container
matching N >= 1 elements is valid: children still prove themselves
individually with strict count === 1, and a child duplicated across
instances fails honestly. testLocators takes { scope: true } for
containers; element call sites stay strict. This also aligns stage 2
with validateContainers, which already accepted count >= 1.

A container matching 0 elements is now recovered without AI: the
section's elements carry eidx, so resolve them to live nodes, take their
lowest common ancestor, and derive a selector from its real attributes
(non-dynamic id, then filtered classes), preferring a unique match. Only
if that fails does the old cascade + fixBrokenSections path apply, and
the all-containers-broken retry now fires only when every container
matches nothing and none is recoverable.

tests/unit/web-element.test.ts also restores the window/document globals
it replaces. It was leaking its JSDOM fixture into tests/unit/explorer.test.ts,
whose mocked page.evaluate reads the global document -- it was silently
asserting against the leaked fixture rather than its own page.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
const containerLocs = result.containerLocators;
await this.testLocators(containerLocs, { scope: true });
for (const loc of containerLocs.filter((l) => l.valid === false)) {
if (await this.recoverContainerFromChildren(result, loc.locator)) loc.valid = true;

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.

Returns true when only one section sharing this CSS is recovered, marking the container valid for all sections. Track recovery per section or return true only when every matching section is recovered

Base automatically changed from fix/researcher-drop-broken-ratio-retry to main July 20, 2026 10:18
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