diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c07ca20..572bd24b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,40 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 value of the message; only the credentials are gone. ### Fixed +- **A click that never lands now says what was true instead of nothing at all** + ([#776](https://github.com/ffroliva/gflow-cli/issues/776)). On `flow.google.com`, + `video r2v` reached `migrated.editor_ready` and died 5.039 s later as a bare + Playwright `TimeoutError` — exit 1, no locator, no cause, no MP4. By elimination that + is `migrated_composer.py`'s `trigger.click(timeout=5000)`: the `wait_for(visible)` one + line above it is guarded and would have raised exit 23, so the control was *visible* + and the *click* expired. [#752](https://github.com/ffroliva/gflow-cli/issues/752) + finding #7 predicted exactly this, at exactly this function, before #776 was filed — + its `count()`→visibility half was fixed and the click half was not, leaving a comment + that describes the failure the next line went on producing. + - **It reads; it does not diagnose.** Two causes were live and *neither could be + measured*: Flow's announcement overlay ([#593](https://github.com/ffroliva/gflow-cli/issues/593), + measured on labs.google, never on this host) and a mid-run agent-mode flip. A guard + built on either would answer confidently and be wrong half the time. So on a timeout + the driver reads Playwright's four actionability conditions back — agent chip, + `hidden`/`disabled`, body pointer-events, and a hit-test naming what is on top — and + reports the ones that fired. When every reading is healthy it **says so**, which + eliminates three conditions and leaves *stable*, rather than inventing a fourth. + - **Costs nothing when healthy** — the read runs only in the `except` branch, the rule + `raise_if_known_landing` already states: a guard ahead of the probe deletes the + evidence that would correct it. + - **MCP gains more than the CLI.** A non-`GFlowError` on the queued path shipped + `"detail": "sha256:…"` — a hash, not even the class name. The typed error routes it + to the Problem Details branch instead, so an agent now gets the locator and exit 23. + - Applied to four sites with a named reason each, not all nineteen: the reported one, + the composer click `_close_pane`'s own docstring records as failing this way, and + both credit-spending submits, where a bare timeout left "did it submit?" unanswerable. + - **The occluder report is a closed allowlist** — tag name plus at most three + framework-prefixed class tokens, never `aria-label`, `title`, `src` or `outerHTML`. + Typing the error moves the text from SHA-256-hashed telemetry to a message printed + raw, logged, and invited into a GitHub issue; a signed-in Flow page carries the + account email and signed media URLs on exactly the elements that occlude things. + - `retryable` is unchanged and **preserved, not measured** — the condition did not + reproduce, and a flag that moves as a side effect of retyping is a claim nobody made. - **A known Flow landing page is no longer reported as selector drift** ([#756](https://github.com/ffroliva/gflow-cli/issues/756), and the 2026-09-10 RED nightly canary). `flow_host_kind()` classifies the *origin*; `/about`, diff --git a/KNOWN_ISSUES.md b/KNOWN_ISSUES.md index f098e2b6..7e7aed0e 100644 --- a/KNOWN_ISSUES.md +++ b/KNOWN_ISSUES.md @@ -1237,6 +1237,29 @@ now names which of three things happened rather than blaming drift: **On 0.71.0 and earlier there is no recovery.** Open the project on `flow.google.com`, click the **Agent** chip off, and the account works again. +**Follow-up ([#776](https://github.com/ffroliva/gflow-cli/issues/776)) — the same +confusion survived one gate later, on the *click*.** The table above covers the readiness +*wait*. A control that passes that wait and then refuses the click used to expire as a bare +Playwright `TimeoutError`: exit 1, no locator, no cause. It now reports what was observed +at the moment it expired, because the cause could not be measured — Flow's announcement +overlay is a labs.google measurement that has never been reproduced on this host, and a +mid-run agent-mode flip is equally consistent with the evidence. + +| The message says | What it means | What to do | +|---|---|---| +| `… did not accept a click … the account is in Flow's agent mode` | the mode flipped after the editor was ready | turn the **Agent** chip off in a browser; re-run | +| `… it is covered by .` | something is stacked over the control — the class names it | dismiss it in a browser; re-run | +| `… the page is accepting no pointer events at all` | an overlay has the whole app blocked (#593's shape) | dismiss it in a browser; re-run | +| `… it carries a bare `hidden` attribute` / `it is disabled` | the control is present but not usable | usually agent mode or a cohort difference; check the Agent chip first | +| `… it is not rendered (display, visibility, or a zero-sized box)` | it is in the DOM but not on screen | as above — check the Agent chip, then file a bug with the log | +| `… it answers no hit test at its own centre` | nothing named itself as the cover, but the click still landed elsewhere | re-run once; if it repeats, file a bug — an overlay outside the document is the usual shape | +| `… it was visible, enabled and hit-testable … most likely still moving` | nothing readable was wrong | Playwright also needs a *stable* box; re-run once. If it repeats, file a bug — this message means we looked and found nothing, which is a real finding worth having | +| `… it could not be read back` | the page changed under the diagnosis | re-run; if it repeats, attach the log | + +The occluder is named by tag plus framework class only. That is deliberate — a signed-in +Flow page carries the account email and signed media URLs on exactly the elements that +tend to occlude things, and this message is printed, logged, and pasted into issues. + ### Auth verification depends on Google's NextAuth session endpoint - **Status:** Mitigated · **Severity:** Low (degrades fail-closed) · **Affects:** issue #15 fix onward · **Tracked:** issue #15 diff --git a/docs/E2E_TESTING.md b/docs/E2E_TESTING.md index 5f952ca1..e06e206b 100644 --- a/docs/E2E_TESTING.md +++ b/docs/E2E_TESTING.md @@ -132,6 +132,18 @@ directories, whose scenarios would run twice. > browser — that is the nightly canary's job (`scripts/canary/`), on a machine that has > one. Hosted CI cannot run the live tiers and never could. +**Two worked examples, deliberately different in kind:** + +| Feature | Binder | What only a browser could prove | +|---|---|---| +| `landing_state_diagnosis.feature` | `test_landing_state_diagnosis_bdd.py` | Flow's hop to `/about` is a **client-side** redirect, so `goto` returns before it runs (#639). A mocked page whose `url` the test assigns cannot fail that way | +| `click_attribution.feature` | `test_click_attribution_bdd.py` | Playwright's **actionability** gate — visible, stable, receives-events, enabled (#776). Each scenario breaks a different one *for real*: a stacked `div` that intercepts pointers, and a CSS animation that never lets the box settle while visibility and the hit test stay healthy | + +Both are route-intercepted and cost **$0** — real Chromium, `page.route(...).fulfill(...)`, +no Google, no profile, no credits. That combination is what makes a browser-only scenario +cheap enough to be non-negotiable: if a scenario needs a browser, the answer is an e2e +test, not a mocked proxy — the Bug Lane's step 5. + --- ## Environment variables diff --git a/docs/superpowers/plans/2026-09-10-776-attributable-click-timeouts/PREDICT.md b/docs/superpowers/plans/2026-09-10-776-attributable-click-timeouts/PREDICT.md new file mode 100644 index 00000000..aa6930ec --- /dev/null +++ b/docs/superpowers/plans/2026-09-10-776-attributable-click-timeouts/PREDICT.md @@ -0,0 +1,174 @@ +# Predict: attribute the migrated driver's click timeouts (#776) + +## Verdict on the proposal as submitted: **STOP** +**Confidence: 5.2/10** (mean 7.0, −2 Devil's Advocate found a simpler path the others missed; any STOP is a STOP) + +## Verdict on the revised proposal below: **CAUTION → proceed with mitigations** + +## Summary + +The proposal was: *port #593's overlay guard to the migrated driver, add a shared +`body{pointer-events:none}` + hit-test probe to `_common.py`, and call it pre-click.* + +Four personas returned GO/CAUTION on the mechanics. The Devil's Advocate returned STOP on +the **premise**, and it is right: the proposal picks a cause. The live spike run the same +hour independently agrees — the overlay mechanism is **unmeasured on this host** (0/3, +[`2026-09-10-migrated-click-blocked.md`](../../spikes/2026-09-10-migrated-click-blocked.md)). +A guard built on an unmeasured cause does not just fail to fire; it produces a +**confidently wrong** message, which #770 is already a live precedent for. + +## Persona findings + +### Architect — GO (8/10) +`_common.py` is the right home, and for a load-bearing reason nobody had stated: there is +an **existing import cycle** — `ui_automation.py:44` imports `migrated_composer`, and +`migrated_composer.py:1921` imports back with `# noqa: PLC0415 - cycle`. So the migrated +driver *cannot* top-level-import from `ui_automation.py`; `_common.py` has zero +intra-`transports` imports and is the only acyclic leaf both drivers already reach. +Recommends a `@staticmethod async def _click(...)` on `MigratedComposer` over a decorator, +matching `_dismiss_dialog`'s existing shape. Warns explicitly against harmonising the labs +driver's clicks in the same PR. + +### Security / reCAPTCHA — CAUTION (8/10) +The strongest finding of the five. **Converting a bare `TimeoutError` into a typed error +removes an accidental privacy net.** Verified in source: `_handle_unhandled_error` +(`_cli_helpers.py:324`) prints a generic message and SHA-256-hashes the telemetry, while +`_handle_gflow_error` (`_cli_helpers.py:304`) prints `exc.detail` **raw**, and +`json_output.py:55` ships it verbatim under `--json`. `redact_error_detail` is wired only +at the SQLite boundary — **not** on the console, structlog, or `--json` paths. + +So any DOM text this fix puts in `detail` is guaranteed to be printed, logged, and (by the +class's own remediation hint) invited into a GitHub issue. An occluding element can carry +an account email in `aria-label`/`title`, or a signed media URL in `src`. This is the exact +class of bug PR #777 fixed two hours ago. + +> **Mandatory:** the occluder report must be a **closed allowlist** — tag name plus a match +> against a fixed set of structural overlay markers. Never `outerHTML`, `textContent`, +> `aria-label`, `title`, `alt`, `src`, `href`, or an attribute dump. Any free-form DOM +> string must pass `redact_sensitive_text()` at the raise site. + +### Performance / Playwright — GO with a scope correction (8/10) +"Pre-click at modal-prone epochs" is ambiguous and the two readings differ by 4×: a real +r2v run makes **~16** clicks, while the labs guard it is modelled on runs at exactly **3** +sites. Worse, `_require_unblocked` has no de-duplication — on a genuinely blocked page each +call independently re-probes, waits ~1 s of jitter, re-attempts dismissal and re-probes, so +8+ pre-emptive sites would add 16–24 s of redundant latency before finally raising. No Page +pool or `__aexit__` risk: every `_checkout_page` is `try/finally`-paired. + +### CLI / MCP UX — CAUTION (8/10) +Three findings that change the implementation: + +**Exit 23 is right; do not mint a new code.** #593 already raises `UiSelectorDriftError` +for "an overlay is still covering the app" (`ui_automation.py:1345`). The project's own bar +for a new code is a *materially different caller action* (`errors.py:562`, `:594`), and +"dismiss the modal and re-run" is not different from 23's existing remediation. One +docstring line should acknowledge that the class covers *occluded*, not only *missing* — +#593 stretched it there already and the docs never caught up. + +**MCP is currently worse than the CLI, and this fix is the whole repair.** On the queued +path a non-`GFlowError` hits `worker/daemon.py:441-475`'s `else` branch, which ships +`"detail": f"sha256:{exception_message_hash(exc)}"` — a hash, not even the class name. Once +the raise site becomes a `GFlowError`, `daemon.py:449`'s `isinstance` branch fires instead +and the agent gets full problem details plus `exit_code=23`. Same transport, one fix, both +doors — but it must be *run* on the MCP path, not inferred. + +**The reporter may have seen nothing at all in `--json`.** `unexpected_payload()` +(`json_output.py:83`) emits no detail and no exception class without a debug flag, so the +`exception_class=TimeoutError` they quoted came from the **stderr structlog** event, not +stdout. An adapter reading only stdout got a bare failure. Worth telling them. + +It also flagged, independently of the Devil's Advocate, that a pre-click guard contradicts +a rule this codebase already learned: `_common.py:205-221` — *"Call this from inside a +failure branch … never before it … a guard placed ahead of the probe deletes the evidence +that would correct it."* + +### Devil's Advocate — STOP (3/10) +**Found the thing that changes the design.** [#752 finding #7](https://github.com/ffroliva/gflow-cli/issues/752), +a maintainer-authored review written *before* #776 was filed, predicts this exact symptom +at this exact function: + +> `_open_pane` still guards with `count()`, not visibility … a mode flip between +> `ensure_editor` and `apply_video_settings` escapes as a **bare Playwright TimeoutError +> with no exit-23 mapping and no mention of agent mode**. + +Half of that was fixed — `:870` became `wait_for(state="visible")`, and its comment at +`:866-869` spells the failure out. **The very next line, `:884`, is the click, still +unguarded.** The file documents the bug it still has, one line above it. + +Agent mode hides the trigger with a bare `hidden` attribute — it never touches +`body{pointer-events:none}`. So the proposed probe would return "not blocked" and the fix +would report the wrong cause. + +## High-confidence risks (2+ personas) + +1. **The proposal picks a cause it cannot see.** (Devil's Advocate STOP; Security Finding 4 + caveat; the spike's 0/3.) Playwright's actionability gate has four conditions — visible, + stable, receives-events, enabled. A body-`pointer-events` probe speaks to exactly one. +2. **A wrong typed message is worse than an honest bare one.** (Devil's Advocate; Security + Finding 2.) #770 is the live precedent. +3. **Blanket-converting 18 sites collides with open #759**, which was filed against this + very file for narrative duplication. (Devil's Advocate; Architect's scope-creep warning.) + +## Conflicts resolved + +- **Performance says "pre-emptive at epochs"; Devil's Advocate says "don't build the guard at all."** + Resolved in favour of the Devil's Advocate, on evidence Performance did not have: the spike + measured `body_pointer_events: auto` in **159/159** samples — including *while the settings + pane was open*. Angular CDK blocks with a `.cdk-overlay-backdrop` element, not by muting the + body, so on this host the **hit-test is the load-bearing detector and the body property is + the labs mechanism**. A pre-emptive body probe here would guard a mechanism this frontend + does not appear to use. +- **Architect says extract to `_common.py`; Devil's Advocate says that is a bigger structural + change than it looks.** Both hold: extraction is right *if* something shared is needed. Under + the revised proposal the read is migrated-host-specific and single-caller, so it stays local + until a second caller exists. The Architect's cycle finding remains the constraint if that + changes. + +## The revised proposal + +**Do not guess the cause. Read it, at the moment of failure, and report what was true.** + +1. One `_click` helper on `MigratedComposer`. On a Playwright timeout it performs a + post-mortem read and raises `UiSelectorDriftError` (exit 23) naming the locator and the + condition that actually failed: + - the agent-mode chip (`_agent_chip_pressed`, already exists at `:684`) — #752's cause + - `hidden` / `disabled` — the *visible* and *enabled* conditions + - `body{pointer-events}` + an allowlisted hit-test occluder — the *receives-events* condition + - none of the above ⇒ say exactly that; it rules out three and points at *stable* +2. **Zero cost on the happy path** — the read runs only in the `except` branch. +3. Applied to four sites with a named reason each, not eighteen: `:884` (#776's site), + `:1598` (named in `_close_pane`'s own docstring as historically failing this way), and + `:1725` / `:1858` (the credit-spending submits, where "did it submit?" is unanswerable today). + +### Required mitigations before EXECUTE + +1. **Allowlist the occluder report.** Tag name, plus only those classes matching a fixed + structural prefix set (`cdk-`, `mat-`, `mdc-`, `flow-`), each capped — mirroring the + existing `.slice(0, 200)` convention at `ui_automation.py:2472`. Never `outerHTML`, + `textContent`, `aria-label`, `title`, `alt`, `src`, `href`, or a generic attribute dump. + Pass the assembled detail through `redact_sensitive_text()` at the raise site. (Security + + CLI/MCP UX, reconciled: an allowlist *and* a bound.) +2. **Put the locator before the variable-length class blob in the message.** The queued MCP + path raw-slices `detail` to 500 chars (`data/redaction.py:117`) while the CLI path does + not; ordering keeps both surfaces showing the same essential text. (CLI/MCP UX) +3. **No pre-emptive guard, no shared `_common.py` probe** until a second caller or a measured + cause justifies one. Three personas and the spike converged here, and `_common.py:205-221` + already states the rule. (Devil's Advocate, CLI/MCP UX, Performance, spike) +4. **Exit 23, and add the missing docstring line** acknowledging *occluded* alongside + *missing*. No new exit code. (CLI/MCP UX) +5. **Preserve `retryable`.** Today's failure is non-retryable; the condition does not reproduce, + so per the Bug Lane's "A flag is a claim" middle row this is *preserved, not measured*. + `UiSelectorDriftError` is not in `RETRYABLE_ERRORS`, so the default already preserves it — + assert that in a test rather than leaving it to survive by luck. +6. **One helper, not eighteen message blocks** — #759. +7. **Run the MCP twin.** The fix flips `daemon.py:449`'s branch from the hashed `else` to the + `GFlowError` path; that is the larger half of the repair and the Iron Law applies to it + separately. (CLI/MCP UX) +8. **Verify via the raised error and the log line, not the incident bundle** — #722 blanks the + capture on this path. + +## Recommended next step + +Phase 3 — `/gflow:scenario`. The scenario is browser-only (a click that fails Playwright's +actionability gate cannot be expressed by a mocked page), so per the Bug Lane it binds to a +route-intercepted e2e in `tests/e2e/`, tagged `@e2e @e2e_auth`. diff --git a/docs/superpowers/plans/2026-09-10-776-attributable-click-timeouts/SCENARIO.md b/docs/superpowers/plans/2026-09-10-776-attributable-click-timeouts/SCENARIO.md new file mode 100644 index 00000000..7c81c442 --- /dev/null +++ b/docs/superpowers/plans/2026-09-10-776-attributable-click-timeouts/SCENARIO.md @@ -0,0 +1,111 @@ +# Scenario: attributable click timeouts on the migrated composer (#776) + +Feeds from [`PREDICT.md`](PREDICT.md) (STOP on the original proposal → CAUTION on the +revised one) and the spike +[`2026-09-10-migrated-click-blocked.md`](../../spikes/2026-09-10-migrated-click-blocked.md). + +## Coverage map + +| Dim | Active? | Why | +|---|---|---| +| **D3** Selector drift & locale invariance | **Yes — primary** | The whole change is what a failed click reports. The occluder must be named structurally; a translated label would violate AGENTS.md and be useless to a zh-CN reporter (#776 is one) | +| **D7** Error propagation & exit codes | **Yes — primary** | Bare `TimeoutError`/exit 1 → `UiSelectorDriftError`/exit 23. `retryable` must not move as a side effect | +| **D12** Observability | **Yes** | `detail` now reaches console raw, structlog, and `--json`. New event/field names are a contract | +| **D13** MCP parity | **Yes — the larger half** | The queued path currently hashes the detail away entirely (`daemon.py:441-475` `else`). The fix flips it to the `GFlowError` branch | +| **D10** Headless vs headed | **Yes** | The probe runs `page.evaluate` on a real page; must not break when the page is mid-teardown | +| **D8** Cross-platform | Partial | #776 is a Windows report, but the failure is upstream of any path handling. Only console encoding matters, already covered by `cli.py:76-82` | +| D1 auth · D2 WAF · D4 batch · D5 concurrency · D6 data · D9 transport · D11 input | No | The change is confined to one driver's failure branch. No auth, no wire call, no schema, no new input | + +## Scenario table + +| # | Dim | Scenario | Severity | Expected behaviour | Test category | +|---|---|---|---|---|---| +| 1 | D7/D3 | The settings trigger is visible but the click never lands; the **agent-mode chip is pressed** | **Critical** | `UiSelectorDriftError` (23) naming *agent mode*, not an overlay. This is #752 finding #7's predicted cause | E2E (BDD) | +| 2 | D7/D3 | The click never lands because an **element covers** the trigger | **Critical** | `UiSelectorDriftError` (23) naming the occluder by tag + structural class | E2E (BDD) | +| 3 | D7 | The click never lands and **every probe reads healthy** | **High** | The error says exactly that — visible, enabled, hit-testable — instead of inventing a cause. Rules out three of Playwright's four conditions and points at *stable* | E2E (BDD) | +| 4 | D7 | **A/B control** — an unobstructed trigger | **Critical** | The click lands, nothing is raised, no probe runs. Without this, scenarios 1–3 could pass against a helper that always raises | E2E (BDD) | +| 5 | D3/D12 | The occluding element carries an **account-identifying attribute** (`aria-label` with an email, `src` with a signed URL) | **Critical** | Neither appears anywhere in the message. Security persona's mandated regression test; PR #777 was this exact bug class | E2E (BDD) | +| 6 | D7 | `retryable` after the change | **High** | Still `False` — *preserved, not measured* (Bug Lane "A flag is a claim", middle row) | Unit | +| 7 | D12 | The occluder's class list is pathologically long | Medium | Capped client-side so the queued path's 500-char slice (`data/redaction.py:117`) cannot clip the remediation off | Unit | +| 8 | D13 | The same failure over **MCP** | **Critical** | Reaches the agent as RFC 9457 problem details with `exit_code: 23`, not `sha256:…` | E2E (MCP path) | +| 9 | D10 | The page is closed/navigating when the post-mortem read runs | High | The probe returns "unreadable" and the error still raises, naming the locator. A diagnostic must never replace the failure it is describing | E2E (BDD) | +| 10 | D7 | A click failing for a **non-timeout** reason | Medium | Not converted — only an actionability timeout is reinterpreted | Unit | + +## Must-cover before merge (Critical + High) + +1, 2, 3, 4, 5, 6, 8, 9 — i.e. every row above except 7 and 10, which are unit-level guards. + +## Deferred + +- The other 15 bare click sites (`_select`, `_select_model`, the frame picker). Per the + Devil's Advocate and #759, each waits for its own signature rather than a blanket + conversion. The helper exists, so adopting one later is a one-line change. +- Whether Flow's announcement modal reaches the migrated host at all — **unmeasured**, and + the fix is deliberately built not to depend on the answer. + +## Suggested BDD scenarios + +Browser-only by construction: Playwright's actionability gate (attached → visible → stable +→ receives-events → enabled) is what fails, and a mocked `Page` whose `.click()` is a stub +cannot express it. Per the Bug Lane step 5, that makes these e2e. + +```gherkin +@e2e @e2e_auth +Feature: A click that never lands says why + + Scenario: A pressed agent-mode chip is named as the cause + Given a Flow project page whose settings trigger is covered + And the agent-mode chip is pressed + When the driver opens the settings pane + Then it fails with exit 23 + And the message names Flow's agent mode + And the message does not blame an overlay + + Scenario: A covering element is named by its structure + Given a Flow project page whose settings trigger is covered + When the driver opens the settings pane + Then it fails with exit 23 + And the message names the covering element by tag and structural class + + Scenario: A healthy-looking failure is reported as unexplained + Given a Flow project page whose settings trigger accepts no click + When the driver opens the settings pane + Then it fails with exit 23 + And the message reports the control as visible, enabled and hit-testable + And the message does not name a cause it did not observe + + Scenario: An unobstructed trigger still opens the pane + Given a Flow project page whose settings trigger is clickable + When the driver opens the settings pane + Then the pane opens and nothing is raised + + Scenario: An account identifier on the covering element never reaches the message + Given a Flow project page whose settings trigger is covered + And the covering element carries an account email and a signed media URL + When the driver opens the settings pane + Then it fails with exit 23 + And the message contains neither the account email nor the signed URL + + Scenario: A page that cannot be read still reports the failed locator + Given a Flow project page whose settings trigger is covered + And the page stops answering probes + When the driver opens the settings pane + Then it fails with exit 23 + And the message names the settings trigger +``` + +Binding: `tests/e2e/test_click_attribution_bdd.py` via +`scenarios("../features/click_attribution.feature")`. One feature, one module — +`tests/features/test_e2e_binding_guard.py` enforces that offline. + +## Known-issues cross-reference + +| Entry | Relationship | +|---|---| +| [#752](https://github.com/ffroliva/gflow-cli/issues/752) finding #7 | **Predicted this symptom at this function.** The `wait_for` half was fixed; the click was not. Scenario 1 is that finding's regression test | +| [#749](https://github.com/ffroliva/gflow-cli/issues/749) / KNOWN_ISSUES "agent-mode chip hides the settings trigger" | Same mechanism, one gate later | +| [#593](https://github.com/ffroliva/gflow-cli/issues/593) / KNOWN_ISSUES "changelog modal wedges" | The labs precedent. Scenario 2 covers the shape **without** asserting it occurs on this host | +| [#722](https://github.com/ffroliva/gflow-cli/issues/722) | Blanks the incident bundle on this path — verification must lean on the raised error and the log line | +| [#759](https://github.com/ffroliva/gflow-cli/issues/759) | Comment bloat in this file. One helper, not eighteen message blocks | +| [#770](https://github.com/ffroliva/gflow-cli/issues/770) | Live precedent for a typed "most likely" message being wrong. Scenario 3 is the direct countermeasure | +| [#643](https://github.com/ffroliva/gflow-cli/issues/643) | The reporter's locale error. Measured irrelevant by the spike — 3/3 reproduced it while the click landed | diff --git a/docs/superpowers/spikes/2026-09-10-migrated-click-blocked.md b/docs/superpowers/spikes/2026-09-10-migrated-click-blocked.md new file mode 100644 index 00000000..fa793eb3 --- /dev/null +++ b/docs/superpowers/spikes/2026-09-10-migrated-click-blocked.md @@ -0,0 +1,121 @@ +# Is the migrated composer's settings trigger ever visible-but-unclickable? (#776) + +- **Date:** 2026-09-10 +- **Script:** [`scripts/dev/spike_migrated_click_blocked.py`](../../../scripts/dev/spike_migrated_click_blocked.py) +- **Profile / project:** `ci-probe` · `1e4efe0d-…` (migrated host, `flow.google.com`) +- **Cost:** $0 — navigation, DOM reads, one settings-pane click, Escape. No credits, no quota. +- **Raw:** `scripts/dev/_spike_out/spike_migrated_click_blocked_20260910_160220.json` (gitignored) + +## The question + +#776 dies 5.039 s after `migrated.editor_ready` with a bare Playwright `TimeoutError`. +By elimination that is `migrated_composer.py:884` — `await trigger.click(timeout=5000)`, +the only unguarded 5000 ms call in that window. Its sibling one line up +(`wait_for(state="visible")`, `:870`) *is* guarded, so the trigger was **visible** and the +**click** expired. + +#593 measured a mechanism with exactly that shape: an announcement overlay sets +`body { pointer-events: none }`, leaving controls visible **and enabled** but unclickable +(`ui_automation.py:1210`). The guard it produced, `_require_unblocked` +(`ui_automation.py:1313`), is called 4× on the labs path and **0×** on the migrated one. + +So: does that state occur on `flow.google.com`, and is `_dismiss_dialog` timed to miss it? + +## Pre-registered readings + +Written into the script's docstring before the run, so the result could not be respun: + +| Outcome | Reading | +|---|---| +| body blocked ≥1/N | #593's mechanism reaches the migrated host; port the guard | +| trigger `hit_testable:false` ≥1/N | occlusion without a body block; the guard needs the hit-test too | +| replayed click expires | #776 reproduced locally | +| 0/N, click always lands | **does not reproduce here; settles nothing** | + +## What was observed + +3 independent navigations, 159 readable DOM samples at 250 ms. + +| Run | trigger mounts | blocked | occluded | `wait_for(visible)` | click | +|---|---|---|---|---|---| +| 1 | 2194 ms | 0 | 0 | passed (64 ms) | **landed (130 ms)** | +| 2 | 3143 ms | 0 | 0 | passed (33 ms) | **landed (65 ms)** | +| 3 | 1479 ms | 0 | 0 | passed (39 ms) | **landed (70 ms)** | + +`body_pointer_events` was `auto` in **159 of 159** samples. `occluded_by` was `null` +whenever the trigger existed. Zero `.cdk-overlay-pane`, zero `[role='dialog']` at any +point before the click; exactly **1** overlay after it — Flow's own settings pane, which +is the pane `_open_pane` is trying to open. + +## Verdict on the overlay hypothesis: UNMEASURED + +**0/3. This settles nothing about #776**, and per the pre-registration it is not evidence +of transience — it is equally consistent with `ci-probe` never having been served the +announcement (which [`2026-09-05-migrated-frames-attach.md`](2026-09-05-migrated-frames-attach.md) +already noted: that account had dismissed it in a prior session, so the migrated-host +changelog modal has *still* never been captured live). + +The related rung-1 finding matters more than the 0/3: **#593's `pointer-events:none` was +measured on labs.google, not on the migrated host.** `migrated_composer.py:744`'s +description of the migrated dialog as "#593's twin" is **asserted, not measured** — it is +one of the few selector claims in that file with no dated spike behind it. + +**What would settle it:** the block probe running on a profile that has *not* yet +dismissed a Flow announcement, i.e. a first visit after a Flow deployment. That is a state +you cannot summon on demand — which is precisely why the fix must not depend on knowing +which overlay it is. + +## Three things this DID measure + +### 1. `_dismiss_dialog` provably runs before the app exists — 3/3 + +At the instant `_dismiss_dialog` fires (`migrated_composer.py:595`, immediately after +`goto(wait_until="domcontentloaded")`), every run read: + +``` +trigger=0 dialog=0 overlay=0 ready_state=interactive +``` + +The composer mounted **1479–3143 ms later**. So the driver's one and only overlay check +looks at a page Angular has not rendered yet, in every run. It cannot see a dialog that +mounts with the app, and no amount of retrying that call site changes it — the miss is +structural, not flaky. This confirms with numbers what `migrated_composer.py:607` asserts +in prose about the SPA race, and extends it: the race applies to `_dismiss_dialog`, not +just to the agent-chip probe that comment is about. + +### 2. A healthy click on this control costs 65–130 ms + +Two orders of magnitude under the 5000 ms budget. So #776's expiry is not a slow click or +a loaded machine — the element never became actionable at all. The `wait_for(visible)` +that precedes it returned in 33–64 ms, which is why it is `:884` and not `:870`. + +### 3. The locale-settle error is NOT sufficient to cause #776 — falsified + +The reporter asked whether `account_locale_lang_unchanged … reason=Error` is relevant. +All three runs reproduced it: + +``` +client.account_locale_lang_unchanged lang=en reason=TimeoutError waited_ms=4000.0 +``` + +…and the click landed every time. A failed locale settle therefore does not, on its own, +produce this failure. It is real and separately tracked as **#643**; it is not #776's +cause. (Consistent with the locale-invariance rule: `READY_ANCHOR = ".settings-trigger-button"` +is structural, so no locale can hide it.) + +## What this means for the fix + +The confirmed defect in #776 is **unattributability**, and that is independent of what +covers the trigger. A guard built only on `body{pointer-events:none}` would catch one of +Playwright's four actionability conditions (receives-events) and stay silent on the other +three — visible, stable, enabled. Building the fix *around the overlay* would be building +it around the one thing this spike could not measure. + +The durable move is to make the failure name itself: report the locator, and whatever the +page can tell us about why the click did not land, at the moment it did not land. + +## Related + +- [`2026-09-05-migrated-frames-attach.md`](2026-09-05-migrated-frames-attach.md) — the account had already dismissed the changelog; a non-blocking "high demand" banner was present +- [`2026-09-05-migrated-host-wire-protocol.md`](2026-09-05-migrated-host-wire-protocol.md) — `goto` 8.1–11.4 s, settled 11.1–14.7 s; `cdk-overlay-container` absent until the first overlay opens +- [`2026-09-10-about-redirect-stability.md`](2026-09-10-about-redirect-stability.md) — the other 0/N "unmeasured" result this week, same discipline diff --git a/scripts/dev/spike_migrated_click_blocked.py b/scripts/dev/spike_migrated_click_blocked.py new file mode 100644 index 00000000..07c50f88 --- /dev/null +++ b/scripts/dev/spike_migrated_click_blocked.py @@ -0,0 +1,376 @@ +r"""Is the migrated composer's settings trigger ever VISIBLE BUT UNCLICKABLE? ($0) + +Settles the mechanism question behind #776. + +A reporter on Windows / `flow.google.com` reaches `migrated.editor_ready` and then dies +5.039 s later with a bare Playwright `TimeoutError` and no exit code. By elimination that +is `migrated_composer.py:884` — `await trigger.click(timeout=5000)`, the one unguarded +5000 ms call between `editor_ready` and the next log line. Its sibling one line above +(`trigger.wait_for(state="visible", timeout=5000)`, `:870`) IS guarded and would have +raised `UiSelectorDriftError` (exit 23) instead, so the trigger was *visible* and the +*click* is what expired. + + A CLICK THAT DOES NOT LAND IS EVIDENCE ABOUT ACTIONABILITY. + IT IS NEVER EVIDENCE THAT THE CONTROL IS MISSING. + +`ui_automation.py:1210` (`_probe_page_block`, from #593) records the mechanism that +produces exactly this shape, measured live on 2026-08-27: + + while Flow's announcement modal is up the body carries `pointer-events: none` and is + neither `aria-hidden` nor `inert` — so every control reads visible and enabled yet + never receives a click. + +`_require_unblocked` guards against it in the labs driver at four call sites. The migrated +driver (`migrated_composer.py`, PR #664, later than #593's audit) calls it **zero** times, +and its only overlay handling — `_dismiss_dialog`, `:743` — matches `[role='dialog']` +alone and runs immediately after `goto(wait_until="domcontentloaded")`, which the module's +own comment at `:607` says is seconds before Angular mounts the composer. + +So there are two separate unknowns, and this probe measures both: + +1. **Does the block state occur on THIS host?** #593 measured labs.google. Whether + flow.google.com's Angular frontend blocks the body the same way has never been read. +2. **Is the driver's `_dismiss_dialog` timed to miss it?** Sampling from `goto` through + composer mount shows when overlays actually appear relative to when the driver looks. + +## Pre-registered readings — written before the run, so the result cannot be respun + +| Outcome | Reading | +|---|---| +| body `pointer-events:none` observed at/after mount in >=1 of N | the #593 mechanism reaches the migrated host; porting the guard is the fix | +| trigger present+visible but `hit_testable:false` in >=1 of N | occlusion WITHOUT a body block; the guard needs the hit-test too, not just the body probe | +| replayed click expires while the trigger reads visible | #776 reproduced locally — the strongest possible result | +| 0/N, click always lands | **does not reproduce on this profile; settles nothing** about the reporter's machine | + +That last row is the one worth pre-writing. A 0/N here does NOT clear +`migrated_composer.py:884`: the confirmed defect in #776 is that the failure arrives +**unattributable**, and that is true whatever covers the trigger. A disappearance is not +evidence of transience — it is equally consistent with this account never having been +served the announcement. Report it as *unmeasured*, and say what would settle it. + +## Cost + +Zero. Navigation, DOM reads, one click on the settings trigger (which opens Flow's own +settings pane and changes no setting), and Escape to close it. Nothing is typed, nothing +submitted, nothing created or deleted. No credits, no daily quota. + + python scripts/dev/spike_migrated_click_blocked.py \ + --profile ci-probe --project --samples 3 +""" + +from __future__ import annotations + +import argparse +import asyncio +import json +import sys +import time +from pathlib import Path +from typing import Any + +sys.path.insert(0, str(Path(__file__).resolve().parent)) + +from _spike_common import ( # noqa: E402, isort: skip + build_client, + default_out_path, + resolve_profile_dir, + step, +) + +# Copied from migrated_composer.py:82/123/83 so the probe and the driver cannot disagree +# about which controls are under discussion. +READY_ANCHOR = ".settings-trigger-button" +DIALOG = "[role='dialog']" +OVERLAY = ".cdk-overlay-pane" + +#: The driver's own click budget (migrated_composer.py:884). Replayed exactly: a probe +#: that waited longer would "succeed" on a page the real run would have abandoned. +DRIVER_CLICK_TIMEOUT_MS = 5000 + +#: `_probe_page_block`'s reading, plus the hit-test it does NOT do. The body property is +#: selector-free and locale-invariant — a property of BEING blocked rather than of any +#: particular announcement — which is why it survives whatever Flow ships next. +_BLOCK_JS = r""" +() => { + const body = getComputedStyle(document.body); + const html = getComputedStyle(document.documentElement); + const trig = document.querySelector('.settings-trigger-button'); + let t = null; + if (trig) { + const box = trig.getBoundingClientRect(); + const cx = box.x + box.width / 2, cy = box.y + box.height / 2; + const top = (box.width && box.height) ? document.elementFromPoint(cx, cy) : null; + const cs = getComputedStyle(trig); + // Present, visible, enabled and CLICKABLE are four different things. Playwright's + // actionability wait fails on the fourth while the first three all read fine, which + // is the entire reason #776 arrives with no message. + t = { + hidden_attr: trig.hasAttribute('hidden'), + disabled: trig.hasAttribute('disabled'), + display: cs.display, + visibility: cs.visibility, + pointer_events: cs.pointerEvents, + w: Math.round(box.width), h: Math.round(box.height), + hit_testable: !!top && (top === trig || trig.contains(top) || top.contains(trig)), + // What is actually on top, named structurally. This is the line that turns + // "a timeout" into "a was over it". + occluded_by: top && !(top === trig || trig.contains(top) || top.contains(trig)) + ? top.tagName.toLowerCase() + + (top.classList.length ? '.' + [...top.classList].slice(0, 3).join('.') : '') + : null, + }; + } + const overlays = [...document.querySelectorAll('.cdk-overlay-pane')].map(o => { + const box = o.getBoundingClientRect(); + return { + tag: o.tagName.toLowerCase(), + classes: [...o.classList].slice(0, 4), + // Component boundaries inside the overlay identify WHAT it is without reading + // a single translated label. + custom_tags: [...new Set([...o.querySelectorAll('*')] + .map(e => e.tagName.toLowerCase()).filter(x => x.includes('-')))].slice(0, 8), + has_changelog_link: !!o.querySelector("a[href*='changelog']"), + visible: box.width > 0 && box.height > 0, + }; + }); + return { + t_ms: Math.round(performance.now()), + url: location.href, + ready_state: document.readyState, + // THE #593 signal. + body_pointer_events: body.pointerEvents, + html_pointer_events: html.pointerEvents, + body_aria_hidden: document.body.getAttribute('aria-hidden'), + body_inert: document.body.hasAttribute('inert'), + counts: { + settings_trigger: document.querySelectorAll('.settings-trigger-button').length, + dialog: document.querySelectorAll("[role='dialog']").length, + overlay: document.querySelectorAll('.cdk-overlay-pane').length, + iframe: document.querySelectorAll('iframe').length, + contenteditable: document.querySelectorAll("[contenteditable='true']").length, + }, + // A changelog iframe is the #593 carrier on labs; recorded by href, not by text. + changelog_iframes: [...document.querySelectorAll('iframe')] + .map(f => f.getAttribute('src') || '') + .filter(s => s.includes('changelog')), + trigger: t, + overlays, + }; +} +""" + + +class ProbeFailedError(RuntimeError): + """A step this probe cannot complete. Never downgraded to a verdict.""" + + +def _blocked(sample: dict[str, Any]) -> bool: + """True when the app behind is unclickable — `_overlay_blocks_page`'s reading.""" + return sample.get("body_pointer_events") == "none" + + +def _occluded(sample: dict[str, Any]) -> bool: + """True when the trigger is rendered but something else answers a hit-test on it.""" + t = sample.get("trigger") + return bool(t and t.get("w") and not t.get("hit_testable")) + + +async def _timeline(page: Any, seconds: float, every_ms: int) -> list[dict[str, Any]]: + """Sample from now until `seconds`, so WHEN a block appears is visible, not just IF. + + The driver looks for a dialog once, immediately after `goto` returns. If overlays + mount later than that single read, the miss is structural and no amount of retrying + the same call site fixes it. + """ + out: list[dict[str, Any]] = [] + deadline = time.monotonic() + seconds + while time.monotonic() < deadline: + try: + out.append(await page.evaluate(_BLOCK_JS)) + except Exception as exc: # noqa: BLE001 - a mid-navigation read is not a failure + out.append({"error": str(exc)[:200], "t_ms": None}) + await page.wait_for_timeout(every_ms) + return out + + +def _summarise(tag: str, samples: list[dict[str, Any]]) -> dict[str, Any]: + good = [s for s in samples if "error" not in s] + blocked = [s for s in good if _blocked(s)] + occluded = [s for s in good if _occluded(s)] + first_trigger = next((s for s in good if (s["counts"]["settings_trigger"] or 0) > 0), None) + summary = { + "samples": len(samples), + "readable": len(good), + "blocked_samples": len(blocked), + "occluded_samples": len(occluded), + "first_trigger_t_ms": first_trigger["t_ms"] if first_trigger else None, + "max_overlays": max((s["counts"]["overlay"] for s in good), default=0), + "max_dialogs": max((s["counts"]["dialog"] for s in good), default=0), + "changelog_iframes": sorted({i for s in good for i in s["changelog_iframes"]}), + "occluders": sorted({s["trigger"]["occluded_by"] for s in occluded if s["trigger"]}), + } + step( + tag, + f"readable={summary['readable']}/{summary['samples']} " + f"blocked={summary['blocked_samples']} occluded={summary['occluded_samples']} " + f"trigger_at={summary['first_trigger_t_ms']}ms " + f"overlays<={summary['max_overlays']} dialogs<={summary['max_dialogs']}", + ) + if summary["occluders"]: + step(f"{tag}.occluders", str(summary["occluders"])) + return summary + + +async def _replay_driver_click(page: Any) -> dict[str, Any]: + """Do exactly what `_open_pane` does, and time it. + + This is the A/B that matters: the same two calls, same timeouts, same order. A click + that lands here on a page the probe just read as unblocked is a control result; one + that expires while `wait_for(visible)` passed IS #776, reproduced. + """ + trigger = page.locator(READY_ANCHOR).first + out: dict[str, Any] = {} + t0 = time.monotonic() + try: + await trigger.wait_for(state="visible", timeout=DRIVER_CLICK_TIMEOUT_MS) + out["wait_for_visible_ms"] = round((time.monotonic() - t0) * 1000) + out["wait_for_visible"] = "passed" + except Exception as exc: # noqa: BLE001 - this branch is exit 23 in the driver + out["wait_for_visible_ms"] = round((time.monotonic() - t0) * 1000) + out["wait_for_visible"] = "TIMED OUT" + out["wait_error"] = str(exc)[:300] + step("replay", "wait_for(visible) TIMED OUT — this run is the exit-23 branch, not #776") + return out + + # State captured BEFORE the click, so a block can be attributed to the click that + # follows rather than inferred from the wreckage afterwards. + out["before"] = await page.evaluate(_BLOCK_JS) + t1 = time.monotonic() + try: + await trigger.click(timeout=DRIVER_CLICK_TIMEOUT_MS) + out["click_ms"] = round((time.monotonic() - t1) * 1000) + out["click"] = "landed" + step("replay", f"click LANDED in {out['click_ms']}ms") + except Exception as exc: # noqa: BLE001 - the whole point of the probe + out["click_ms"] = round((time.monotonic() - t1) * 1000) + out["click"] = "TIMED OUT" + out["click_error"] = str(exc)[:400] + step("replay", f"click TIMED OUT after {out['click_ms']}ms — #776 REPRODUCED") + out["after"] = await page.evaluate(_BLOCK_JS) + # Leave the account exactly as found: the pane changes no setting, but an open + # overlay would poison a later sample in this same run. + try: + await page.keyboard.press("Escape") + await page.wait_for_timeout(400) + except Exception: # noqa: BLE001 - cleanup is best-effort + pass + return out + + +async def main() -> int: + ap = argparse.ArgumentParser(description=__doc__) + ap.add_argument("--profile", required=True) + ap.add_argument("--project", required=True) + ap.add_argument("--samples", type=int, default=3, help="independent navigations") + ap.add_argument("--watch-s", type=float, default=14.0, help="timeline length per sample") + ap.add_argument("--every-ms", type=int, default=250) + args = ap.parse_args() + + profile_dir = resolve_profile_dir(args.profile) + findings: dict[str, Any] = { + "profile": args.profile, + "project": args.project, + "question": ( + "is .settings-trigger-button ever visible-but-unclickable on flow.google.com, " + "and does body{pointer-events:none} (#593) occur on the migrated host? (#776)" + ), + "cost": "credit-free: navigation, DOM reads, one settings-pane click, Escape", + "driver_click_timeout_ms": DRIVER_CLICK_TIMEOUT_MS, + "runs": [], + } + url = f"https://flow.google.com/project/{args.project}" + + async with build_client(profile_dir) as client: + context = client._context # noqa: SLF001 - spike reads the live context + for i in range(args.samples): + page = await context.new_page() + run: dict[str, Any] = {"n": i + 1} + try: + step(f"run{i + 1}.goto", url) + await page.goto(url, wait_until="domcontentloaded", timeout=60_000) + + # The driver's ONE look for a dialog happens right here, before any + # settle. Recorded separately so "what _dismiss_dialog could have seen" + # is a measurement rather than an argument about the code. + run["at_dismiss_dialog_time"] = await page.evaluate(_BLOCK_JS) + step( + f"run{i + 1}.dismiss_window", + f"trigger={run['at_dismiss_dialog_time']['counts']['settings_trigger']} " + f"dialogs={run['at_dismiss_dialog_time']['counts']['dialog']} " + f"body_pe={run['at_dismiss_dialog_time']['body_pointer_events']}", + ) + + samples = await _timeline(page, args.watch_s, args.every_ms) + run["timeline_summary"] = _summarise(f"run{i + 1}.timeline", samples) + run["timeline"] = samples + + if not run["timeline_summary"]["readable"]: + raise ProbeFailedError( + f"run {i + 1}: every DOM read failed — this run says NOTHING" + ) + landed = page.url + run["landed_url"] = landed + if "flow.google.com/project/" not in landed: + # Same discipline as ensure_editor: a landing page cannot answer a + # question about the composer, and must not be read as one. + run["verdict"] = "NOT ON A PROJECT PAGE — says nothing about the click" + step(f"run{i + 1}.landed", f"{landed} — skipped") + continue + + run["replay"] = await _replay_driver_click(page) + shot = default_out_path(f"spike_click_blocked_run{i + 1}", ".png") + await page.screenshot(path=str(shot)) + run["screenshot"] = shot.name + finally: + findings["runs"].append(run) + await page.close() + + # ---- the question, answered from the read ------------------------------- + scored = [r for r in findings["runs"] if "replay" in r] + blocked_runs = [r for r in scored if r["timeline_summary"]["blocked_samples"]] + occluded_runs = [r for r in scored if r["timeline_summary"]["occluded_samples"]] + failed_clicks = [r for r in scored if r["replay"].get("click") == "TIMED OUT"] + late_trigger = [ + r + for r in scored + if not r["at_dismiss_dialog_time"]["counts"]["settings_trigger"] + and r["timeline_summary"]["first_trigger_t_ms"] is not None + ] + + findings["verdict"] = { + "scored_runs": len(scored), + "runs_with_body_block": len(blocked_runs), + "runs_with_occluded_trigger": len(occluded_runs), + "runs_where_the_click_expired": len(failed_clicks), + "runs_where_dismiss_dialog_ran_before_the_composer_existed": len(late_trigger), + # Stated as an observation, never as a conclusion about the reporter's machine. + "reading": ( + "#776 REPRODUCED — the driver's own click sequence expired here" + if failed_clicks + else "the block/occlusion state was observed, but the click still landed" + if blocked_runs or occluded_runs + else "NOT REPRODUCED on this profile — unmeasured, settles nothing about #776; " + "the confirmed defect (an unattributable timeout at migrated_composer.py:884) " + "is independent of which overlay causes it" + ), + } + + out = default_out_path("spike_migrated_click_blocked") + out.write_text(json.dumps(findings, indent=2, ensure_ascii=False), encoding="utf-8") + step("verdict", json.dumps(findings["verdict"], indent=2)) + step("out", str(out)) + return 0 + + +if __name__ == "__main__": + raise SystemExit(asyncio.run(main())) diff --git a/src/gflow_cli/api/transports/migrated_composer.py b/src/gflow_cli/api/transports/migrated_composer.py index fced7168..770dca57 100644 --- a/src/gflow_cli/api/transports/migrated_composer.py +++ b/src/gflow_cli/api/transports/migrated_composer.py @@ -39,6 +39,7 @@ from urllib.parse import unquote_plus, urlsplit import structlog +from playwright.async_api import TimeoutError as PlaywrightTimeoutError from gflow_cli.api.dto import GeneratedImage from gflow_cli.api.image import Aspect as ImageAspect @@ -69,6 +70,7 @@ UiSelectorDriftError, WireFormatError, ) +from gflow_cli.redaction import redact_sensitive_text if TYPE_CHECKING: from playwright.async_api import Page @@ -195,6 +197,48 @@ FRAME_UPLOAD_S = 60.0 FRAME_COMMIT_HIDDEN_S = 15.0 FRAME_THUMB_VISIBLE_S = 5.0 +#: What a click that expired may be asked about — Playwright's four actionability +#: conditions, read back after the fact. See :meth:`MigratedComposer._click`. +#: +#: **Everything returned here is a closed vocabulary.** Tag names, booleans, and class +#: tokens matching a fixed framework prefix — never ``outerHTML``, ``textContent``, +#: ``aria-label``, ``title``, ``alt``, ``src`` or ``href``. An occluding element on a +#: signed-in Flow page routinely carries the account email in ``aria-label`` and a signed +#: media URL in ``src``, and this string is printed raw to the console, shipped through +#: structlog, emitted under ``--json``, and invited into a GitHub issue by the error +#: class's own remediation hint. PR #777 fixed exactly this bug one surface over. +_CLICK_POSTMORTEM_JS = r""" +(el) => { + const cs = getComputedStyle(el); + const box = el.getBoundingClientRect(); + const cx = box.x + box.width / 2, cy = box.y + box.height / 2; + const top = (box.width && box.height) ? document.elementFromPoint(cx, cy) : null; + const hit = !!top && (top === el || el.contains(top) || top.contains(el)); + // Angular/CDK, Material and Flow's own components. A layout class on a bare
+ // is an accident; `cdk-overlay-backdrop` is a component boundary and says what the + // thing IS, in any locale. + const structural = (n) => + [...n.classList].filter(c => /^(cdk|mat|mdc|flow)-/.test(c)).slice(0, 3).join('.'); + return { + visible: cs.display !== 'none' && cs.visibility !== 'hidden' + && box.width > 0 && box.height > 0, + hidden_attr: el.hasAttribute('hidden'), + enabled: !el.hasAttribute('disabled') && el.getAttribute('aria-disabled') !== 'true', + hit_testable: hit, + // The #593 mechanism. Measured on labs.google, never on this host — kept because a + // reading that never fires costs nothing, and a missing one costs a wrong answer. + body_blocked: getComputedStyle(document.body).pointerEvents === 'none', + occluder: (top && !hit) + ? (top.tagName.toLowerCase() + (structural(top) ? '.' + structural(top) : '')) + : null, + }; +} +""" + +#: Names the submit control in a message. No CSS string can express the `arrow_forward` +#: ligature filter that builds it, so there is nothing here to rot into a selector. +SUBMIT_BUTTON = "the submit button (ligature 'arrow_forward')" + #: The submit reply arrived 4.0–4.6 s after the click in both measured runs. SUBMIT_REPLY_BUDGET_S = 60.0 # Angular enables the arrow_forward button ~100 ms after `insert_text` lands in the @@ -759,6 +803,104 @@ async def _dismiss_dialog(page: Page) -> None: except Exception as e: # noqa: BLE001 - best-effort, never the failure itself log.warning("migrated.dialog_not_dismissed", error=str(e)[:200]) + # --- clicking, and saying why a click did not land --------------------------- + + async def _click(self, page: Page, locator: Any, *, named: str, timeout: int) -> None: + """Click, and when it expires report what was actually TRUE — never why. + + Playwright's actionability gate has four conditions — visible, stable, receives + events, enabled — and a click that fails any of them expires as a bare + ``TimeoutError`` carrying no locator, no exit code and no cause. That is #776: + `editor_ready`, five seconds, exit 1, nothing to act on. + + **This reads; it does not diagnose.** Two candidate causes (#593, #752 finding #7) + were unmeasurable on this host as of the 2026-09-10 spike, and a guard built on + either would sometimes answer confidently and wrong (#770). So it states + observations; "every reading was healthy" is one of them. + + Costs nothing on a healthy run — the read happens only in the except branch, which + is also the rule :func:`_common.raise_if_known_landing` already states: a guard + placed ahead of the probe deletes the evidence that would correct it. + + Only a Playwright timeout is reinterpreted. A closed page or a detached frame is a + different failure and travels unchanged. + """ + try: + await locator.click(timeout=timeout) + except PlaywrightTimeoutError as e: + raise UiSelectorDriftError( + detail=await self._why_the_click_missed(page, locator, named, timeout) + ) from e + + async def _why_the_click_missed( + self, page: Page, locator: Any, named: str, timeout: int + ) -> str: + """The message for a click that expired: locator first, observations after. + + Locator first is not cosmetic. ``redact_sensitive_text`` truncates to 500 chars + at this raise site (``data/redaction.py``), so every surface sees the same cap and + anything variable-length — the occluder's class list — has to sit behind the one + part that must always survive it. + """ + head = f"migrated host: {named} did not accept a click within {timeout} ms" + try: + # Through the LOCATOR, not a selector string. Half this driver's controls are + # built by filtering on a ligature (`button` + `arrow_forward`), which no + # `document.querySelector` can express — and the submit button, where losing + # attribution costs the most, is one of them. + state: dict[str, Any] = await locator.evaluate(_CLICK_POSTMORTEM_JS) + except Exception as e: # noqa: BLE001 - a diagnostic never replaces the failure + # Detached, cross-origin, or the document replaced under us. All three are + # "we could not look", and none of them may swallow the failure itself. + return redact_sensitive_text( + f"{head} — it could not be read back ({str(e)[:120]}) (host=migrated)" + ) + + seen: list[str] = [] + # Agent mode first: it is the one cause here with a user action attached, and + # it hides the trigger with a bare `hidden` that touches neither the body's + # pointer-events nor the hit-test — so nothing else below would notice it. + if await self._agent_chip_pressed(page): + seen.append( + "the account is in Flow's agent mode, which hides it — turn the Agent " + "chip off in a browser and re-run" + ) + # One chain, because these are competing readings of the SAME question — can a + # pointer reach it — ordered most specific first. The JS only hit-tests + # `if (box.width && box.height)`, so an unrendered element always reports no hit + # test too; as independent `if`s that said "it is not rendered" and "it answers no + # hit test" about one fact. + if state.get("hidden_attr"): + seen.append("it carries a bare `hidden` attribute") + elif not state.get("visible"): + seen.append("it is not rendered (display, visibility, or a zero-sized box)") + elif state.get("occluder"): + seen.append(f"it is covered by {state['occluder']}") + elif not state.get("hit_testable"): + # Rendered, nothing named itself: whatever is on top is outside the document. + # Not "healthy" — letting it fall through would claim hit-testable of an + # element that had just failed the hit test. + seen.append("it answers no hit test at its own centre") + + # Separate axes: an element can be disabled, or the whole page blocked, whatever + # the chain above found. + if not state.get("enabled"): + seen.append("it is disabled") + if state.get("body_blocked"): + seen.append("the page is accepting no pointer events at all") + + if not seen: + # Every readable condition is healthy. Saying so eliminates three of + # Playwright's four checks instead of inventing one of them. + seen.append( + "it was visible, enabled and hit-testable at the moment the click " + "expired, so nothing readable on the page explains it — Playwright also " + "requires a stable bounding box, so the control was most likely still " + "moving or being re-rendered" + ) + # Belt and braces over the JS allowlist above. + return redact_sensitive_text(f"{head} — {'; '.join(seen)} (host=migrated)") + # --- settings --------------------------------------------------------------- async def apply_video_settings(self, page: Page, request: GenerateVideoRequest) -> None: @@ -881,7 +1023,9 @@ async def _open_pane(self, page: Page) -> Any: f"{why} (host=migrated)" ), ) from e - await trigger.click(timeout=5000) + # The other half of #752 finding #7: the guard above became a visibility wait, + # this click stayed bare, and the comment above describes what it went on doing. + await self._click(page, trigger, named=READY_ANCHOR, timeout=5000) # THE overlay that holds the option groups — not `.last`: once the model # menu (a second overlay) has opened and closed, a detached menu pane can # still be the last one in the DOM, and every axis after `--model` then @@ -1595,7 +1739,9 @@ async def send_prompt(self, page: Page, prompt: str, *, append: bool = False) -> detail=f"migrated host: composer ({COMPOSER}) not found (host=migrated)", ) if not append: - await composer.click(timeout=5000) + # _close_pane's docstring names THIS click as the one that surfaced a + # stuck settings pane as a bare 5 s TimeoutError naming only the composer. + await self._click(page, composer, named=COMPOSER, timeout=5000) # insert_text dispatches input events without key presses: a newline in the # prompt lands as text instead of an Enter that might submit early. await page.keyboard.insert_text(prompt) @@ -1722,7 +1868,9 @@ async def on_response(response: Any) -> None: ) await asyncio.sleep(SUBMIT_ENABLE_POLL_S) deadline = time.monotonic() + poll_timeout_s - await submit.click(timeout=5000) + # The credit-spending click: a bare timeout here leaves "did it submit?" + # unanswerable, which is the worst place in this driver to lose attribution. + await self._click(page, submit, named=SUBMIT_BUTTON, timeout=5000) log.info("migrated.submit_clicked") budget = min(SUBMIT_REPLY_BUDGET_S, poll_timeout_s) await asyncio.wait( @@ -1855,7 +2003,7 @@ async def on_response(response: Any) -> None: detail="migrated host: image submit stayed disabled (host=migrated)" ) await asyncio.sleep(SUBMIT_ENABLE_POLL_S) - await submit.click(timeout=5000) + await self._click(page, submit, named=SUBMIT_BUTTON, timeout=5000) done, _ = await asyncio.wait( {result, route_error}, timeout=IMAGE_REPLY_BUDGET_S, diff --git a/src/gflow_cli/errors.py b/src/gflow_cli/errors.py index 1512e7e2..6bd04b2b 100644 --- a/src/gflow_cli/errors.py +++ b/src/gflow_cli/errors.py @@ -609,10 +609,18 @@ class ExtendUnavailableError(GFlowError): class UiSelectorDriftError(GFlowError): - """Raised when a UI-automation selector cascade finds no matching element. + """Raised when a UI-automation selector cascade cannot reach the control it needs. + + Two shapes, and the second is easy to forget: the selector **finds nothing**, or it + finds the element and the element **will not take the interaction** — occluded, + disabled, or never holding still (#593's blocked overlay, #776's click that expired + while the control read visible and enabled). Both mean the same thing to a caller — + gflow cannot drive this control — which is why they share an exit code, and why the + ``detail`` has to say which one happened. Indicates that Flow's frontend has changed in a way that invalidates one - of the selector probes (mode-switch trigger, mode tab, sub-mode tab, etc.). + of the selector probes (mode-switch trigger, mode tab, sub-mode tab, etc.), + or that something on the page is in the way. The ``detail`` names the probe label and includes the debug screenshot or diagnostics JSON path when one was captured. diff --git a/tests/api/transports/test_click_attribution.py b/tests/api/transports/test_click_attribution.py new file mode 100644 index 00000000..fa128e4a --- /dev/null +++ b/tests/api/transports/test_click_attribution.py @@ -0,0 +1,274 @@ +"""Offline guards for the click post-mortem (#776). + +The behaviour itself is proven in the browser by +``tests/e2e/test_click_attribution_bdd.py`` — Playwright's actionability gate is what +fails, and no mock can express it. These two cases are the opposite: they are about what +the helper must *not* do, and both are cheap to pin without a browser. +""" + +from __future__ import annotations + +from typing import Any + +import pytest +from playwright.async_api import TimeoutError as PlaywrightTimeoutError + +from gflow_cli.api.transports.migrated_composer import MigratedComposer +from gflow_cli.errors import UiSelectorDriftError + + +class _Locator: + """A locator that fails a click the way we ask, and reads back what we say. + + Deliberately a plain class, not ``MagicMock``: a mock answers every attribute with a + truthy child, which is exactly how a guard silently stops guarding + (memory ``magicmock-truthy-getattr-silences-guards``). + """ + + def __init__(self, *, raises: BaseException, state: dict[str, Any] | None = None) -> None: + self._raises = raises + self._state = state or {} + + async def click(self, **_: object) -> None: + raise self._raises + + async def evaluate(self, _js: str) -> dict[str, Any]: + return self._state + + +class _Page: + """Just enough Page for the post-mortem: it is only asked for the agent chip.""" + + def locator(self, _sel: str) -> Any: # pragma: no cover - never reached here + raise AssertionError("the post-mortem must read through the LOCATOR, not the page") + + +_HEALTHY = { + "visible": True, + "hidden_attr": False, + "enabled": True, + "hit_testable": True, + "body_blocked": False, + "occluder": None, +} + + +@pytest.fixture +def composer(monkeypatch: pytest.MonkeyPatch) -> MigratedComposer: + """A composer whose agent-chip probe answers False, so it is never the cause.""" + monkeypatch.setattr( + MigratedComposer, "_agent_chip_pressed", staticmethod(lambda _page: _false()) + ) + return MigratedComposer() + + +async def _false() -> bool: + return False + + +@pytest.mark.asyncio +async def test_a_non_timeout_click_failure_is_not_reinterpreted( + composer: MigratedComposer, +) -> None: + """Only an actionability timeout becomes selector drift. + + A closed page, a detached frame or a navigation abort is a different failure with a + different remedy. Converting those too would relabel every browser mishap as "Flow + changed its frontend" and send users to file drift bugs about their own laptop. + """ + boom = RuntimeError("Target page, context or browser has been closed") + with pytest.raises(RuntimeError) as caught: + await composer._click( # noqa: SLF001 + _Page(), _Locator(raises=boom), named=".x", timeout=1000 + ) + assert caught.value is boom + + +@pytest.mark.asyncio +async def test_a_click_timeout_names_the_locator_first(composer: MigratedComposer) -> None: + """The locator leads the message, ahead of anything variable-length. + + The detail is truncated to 500 chars at the raise site (``data/redaction.py``), so + whatever must survive that cut has to come before the occluder's class list. + """ + error = await _drift(composer, state=_HEALTHY) + assert error.detail is not None + assert error.detail.index(".settings-trigger-button") < error.detail.index("did not accept") + + +@pytest.mark.asyncio +async def test_an_unreadable_element_still_reports_the_failure( + composer: MigratedComposer, +) -> None: + """A diagnostic may never replace the failure it was called to describe.""" + + class _Unreadable(_Locator): + async def evaluate(self, _js: str) -> dict[str, Any]: + raise RuntimeError("Execution context was destroyed") + + with pytest.raises(UiSelectorDriftError) as caught: + await composer._click( # noqa: SLF001 + _Page(), + _Unreadable(raises=PlaywrightTimeoutError("Timeout 5000ms exceeded")), + named=".settings-trigger-button", + timeout=5000, + ) + detail = caught.value.detail or "" + assert ".settings-trigger-button" in detail + assert "could not be read back" in detail + + +@pytest.mark.asyncio +async def test_nothing_readable_wrong_is_reported_as_such(composer: MigratedComposer) -> None: + """When every reading is healthy, say so — do not pick a cause. + + This is the countermeasure to #770, where a typed error named a "most likely" cause + that was wrong for the reporting account. Three of Playwright's four conditions are + eliminated here; naming the fourth as a *possibility* is honest, asserting it is not. + """ + detail = (await _drift(composer, state=_HEALTHY)).detail or "" + assert "visible, enabled and hit-testable" in detail + for invented in ("agent mode", "covered by", "announcement", "changelog"): + assert invented not in detail.lower(), f"invented {invented!r}: {detail}" + + +@pytest.mark.asyncio +async def test_the_occluder_report_is_bounded(composer: MigratedComposer) -> None: + """A named occluder must not be able to crowd the message out. + + The JS caps the class list at three framework-prefixed tokens, so even a pathological + CDK class soup leaves the 500-char MCP slice intact. Pinned here because the cap lives + in a JS string that no type checker and no linter can see. + """ + state = {**_HEALTHY, "hit_testable": False, "occluder": "div." + ".".join(["cdk-x"] * 3)} + detail = (await _drift(composer, state=state)).detail or "" + assert "div.cdk-x.cdk-x.cdk-x" in detail + assert len(detail) < 500, f"a single occluder should not fill the MCP budget: {detail}" + + +@pytest.mark.asyncio +async def test_a_failed_hit_test_without_an_occluder_is_not_called_healthy( + composer: MigratedComposer, +) -> None: + """`elementFromPoint` can miss and name nothing — a zero-box or an out-of-document + overlay. Falling through to the healthy branch would then claim hit-testable of an + element that had just failed the hit test.""" + state = {**_HEALTHY, "hit_testable": False, "occluder": None} + detail = (await _drift(composer, state=state)).detail or "" + assert "no hit test" in detail + assert "hit-testable at the moment" not in detail + + +async def _drift(composer: MigratedComposer, *, state: dict[str, Any]) -> UiSelectorDriftError: + with pytest.raises(UiSelectorDriftError) as caught: + await composer._click( # noqa: SLF001 + _Page(), + _Locator(raises=PlaywrightTimeoutError("Timeout 5000ms exceeded"), state=state), + named=".settings-trigger-button", + timeout=5000, + ) + return caught.value + + +# --------------------------------------------------------------------------- +# One case per reading the post-mortem can report. +# +# The e2e proves the BROWSER really produces these states; these prove the message +# for each one. They are here and not only there because CI's coverage run excludes +# `-m e2e`, so a branch exercised solely by the browser reads as dead code to +# SonarCloud's new-code gate — the exact way PR #777 went red at 70%. +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + ("override", "expected"), + [ + ({"hidden_attr": True, "hit_testable": False}, "bare `hidden` attribute"), + ({"visible": False, "hit_testable": False}, "not rendered"), + ({"enabled": False}, "it is disabled"), + ({"body_blocked": True}, "accepting no pointer events at all"), + ], + ids=["hidden", "not-rendered", "disabled", "body-blocked"], +) +@pytest.mark.asyncio +async def test_each_readable_condition_is_named( + composer: MigratedComposer, override: dict[str, Any], expected: str +) -> None: + detail = (await _drift(composer, state={**_HEALTHY, **override})).detail or "" + assert expected in detail, detail + # Whatever fired, the locator is still the first thing the reader sees. + assert ".settings-trigger-button" in detail + + +@pytest.mark.asyncio +async def test_agent_mode_leads_the_message_when_the_chip_is_pressed( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """#752 finding #7's cause, and the only one here with a user action attached. + + It is reported FIRST because it is the only reading a user can act on directly, and + because agent mode hides the trigger with a bare `hidden` that touches neither the + body's pointer-events nor the hit test — nothing else in the probe would notice it. + """ + + async def _true(_page: object) -> bool: + return True + + monkeypatch.setattr(MigratedComposer, "_agent_chip_pressed", staticmethod(_true)) + state = {**_HEALTHY, "hit_testable": False, "occluder": "div.cdk-overlay-backdrop"} + detail = (await _drift(MigratedComposer(), state=state)).detail or "" + assert "agent mode" in detail + # Both facts are reported — the occluder is real too — but the actionable one leads. + assert detail.index("agent mode") < detail.index("covered by") + + +@pytest.mark.asyncio +async def test_a_missing_reading_is_not_mistaken_for_health( + composer: MigratedComposer, +) -> None: + """An empty state dict must not read as "everything was fine". + + `locator.evaluate` returning a shape we did not expect (an older Chromium, a JS + error swallowed into a partial object) would make every `.get()` falsy. Falling + through to the healthy branch there would report "visible, enabled and hit-testable" + about an element nothing was ever read from. + """ + detail = (await _drift(composer, state={})).detail or "" + assert "visible, enabled and hit-testable" not in detail, detail + + +@pytest.mark.asyncio +async def test_an_unrendered_element_reports_one_fact_once(composer: MigratedComposer) -> None: + """ "Not rendered" and "answers no hit test" are the same fact, not two. + + `_CLICK_POSTMORTEM_JS` only calls `elementFromPoint` `if (box.width && box.height)`, + so a zero-box element ALWAYS comes back `hit_testable: false, occluder: null` as well. + Reported as independent readings that told the user the same thing twice in one + sentence — the shape of over-reporting that makes a diagnostic harder to act on than + a short one. + """ + state = {**_HEALTHY, "visible": False, "hit_testable": False, "occluder": None} + detail = (await _drift(composer, state=state)).detail or "" + assert "not rendered" in detail + assert "no hit test" not in detail + + +@pytest.mark.asyncio +async def test_disabled_and_blocked_are_reported_alongside_occlusion( + composer: MigratedComposer, +) -> None: + """Enabled-ness and a page-wide block are separate axes from occlusion. + + Chaining them onto the same `elif` ladder would hide a disabled control behind + whatever covered it — two different remedies collapsed into one message. + """ + state = { + **_HEALTHY, + "enabled": False, + "body_blocked": True, + "hit_testable": False, + "occluder": "div.cdk-overlay-backdrop", + } + detail = (await _drift(composer, state=state)).detail or "" + for fact in ("covered by", "it is disabled", "accepting no pointer events"): + assert fact in detail, f"missing {fact!r}: {detail}" diff --git a/tests/api/transports/test_migrated_composer.py b/tests/api/transports/test_migrated_composer.py index 76cecec2..642ed973 100644 --- a/tests/api/transports/test_migrated_composer.py +++ b/tests/api/transports/test_migrated_composer.py @@ -354,7 +354,15 @@ async def click(self, **_: Any) -> None: class PlaywrightTimeoutError(Exception): - pass + """A stand-in, NOT ``playwright.async_api.TimeoutError`` — and that matters now. + + Since #776, ``MigratedComposer._click`` catches the *real* Playwright class to + convert a click timeout into ``UiSelectorDriftError``. Raising this one from a fake + locator therefore does **not** match that ``except``, so the post-mortem never runs + and the test silently exercises nothing. If you are writing a click-timeout + regression test, import the real class — see + ``tests/api/transports/test_click_attribution.py``. + """ class FakeFileChooser: diff --git a/tests/e2e/test_click_attribution_bdd.py b/tests/e2e/test_click_attribution_bdd.py new file mode 100644 index 00000000..15a6fc7b --- /dev/null +++ b/tests/e2e/test_click_attribution_bdd.py @@ -0,0 +1,263 @@ +"""E2E for attributable click timeouts on the migrated composer (#776). + +Binds ``tests/features/click_attribution.feature``. The Gherkin's ``@e2e`` tags become +pytest markers via pytest-bdd, so ``-m e2e`` / ``-m e2e_auth`` select this file exactly +like a hand-written e2e — see ``docs/E2E_TESTING.md`` § BDD-bound e2e. + +**Why an e2e and not a unit test.** What fails in #776 is Playwright's *actionability* +gate — attached → visible → stable → receives-events → enabled. A mocked ``Page`` whose +``.click()`` is a stub cannot express "visible, enabled, and still not clickable"; it +would pass against a fix that reads the DOM at the wrong moment, which is the trap #639 +set one surface over. Only a real browser can falsify this. + +Each scenario therefore breaks a **different** actionability condition, in the browser, +for real: + +- covered by a stacked element → *receives-events* +- an infinite CSS transform → *stable* (visible, enabled and hit-testable throughout) +- a pressed agent-mode chip → #752 finding #7's predicted cause, which touches + neither ``body{pointer-events}`` nor the hit-test + +**Cost: zero.** Every Flow origin is served by Playwright route interception, so nothing +reaches Google, no credit is spent and no authenticated profile is needed — the same +harness as ``test_landing_state_diagnosis_bdd.py``. +""" + +from __future__ import annotations + +import asyncio +from typing import Any + +import pytest +from playwright.async_api import Route, async_playwright +from pytest_bdd import given, scenarios, then, when + +from gflow_cli.api.transports.migrated_composer import MigratedComposer +from gflow_cli.errors import UiSelectorDriftError, is_retryable + +scenarios("../features/click_attribution.feature") + +PROJECT_ID = "e2e-click-attribution" +PROJECT_URL = f"https://flow.google.com/project/{PROJECT_ID}" +ELSEWHERE_URL = "https://flow.google.com/project/somewhere-else" + +#: Planted on the covering element in the redaction scenario. Neither may ever appear in +#: a message that reaches a console, a log or a GitHub issue. +ACCOUNT_EMAIL = "e2e-victim@example.com" +SIGNED_URL = "https://lh3.googleusercontent.com/x?X-Goog-Signature=deadbeefcafe" + +_STYLE = """ + body { margin: 0; font-family: sans-serif; } + .settings-trigger-button { position: absolute; top: 100px; left: 100px; + width: 147px; height: 32px; } + #cover { position: absolute; top: 0; left: 0; width: 100vw; height: 100vh; + z-index: 9999; background: rgba(0,0,0,.01); } + /* Never stable, never still: Playwright's stability check can never pass, while + visibility, enabled-ness and the hit-test all read perfectly healthy. */ + @keyframes drift { from { transform: translateX(0); } to { transform: translateX(60px); } } + .jitter { animation: drift .18s linear infinite alternate; } +""" + +#: Clicking the trigger mounts the overlay `_open_pane` waits for, so the control +#: scenario exercises the whole happy path rather than just "no exception". +_OPENS_PANE_JS = """ + document.querySelector('.settings-trigger-button').addEventListener('click', () => { + const pane = document.createElement('div'); + pane.className = 'cdk-overlay-pane'; + pane.innerHTML = "
16:9
"; + document.body.appendChild(pane); + }); +""" + + +def _page( + *, + cover: str = "", + chip: bool = False, + jitter: bool = False, + script: str = "", +) -> str: + """A migrated-host editor stub: a real trigger, plus whatever is wrong with it.""" + chip_html = "" if chip else "" + cls = "settings-trigger-button jitter" if jitter else "settings-trigger-button" + return ( + f"" + f"{chip_html}" + f"" + f"{cover}" + f"" + "" + ) + + +#: A plain stacked overlay — the shape Angular CDK actually uses on this host (the +#: 2026-09-10 spike measured `body{pointer-events}` staying `auto` in 159/159 samples, +#: including while Flow's own pane was open, so the hit-test is the load-bearing +#: detector here and the body property is the labs mechanism). +_CDK_COVER = "
" + +#: The same cover, carrying exactly what must never be echoed back. +_LEAKY_COVER = ( + f"
{ACCOUNT_EMAIL}
" +) + + +@pytest.fixture +def world() -> dict[str, Any]: + return {} + + +async def _drive(html: str, run: Any) -> BaseException | None: + """Launch a real browser, serve `html` for every Flow URL, run `run(page)`.""" + async with async_playwright() as pw: + browser = await pw.chromium.launch(headless=True) + try: + page = await (await browser.new_context()).new_page() + + async def _handler(route: Route) -> None: + await route.fulfill(status=200, content_type="text/html", body=html) + + await page.route("https://flow.google.com/**", _handler) + await page.goto(PROJECT_URL, wait_until="domcontentloaded") + try: + await run(page) + except BaseException as exc: # noqa: BLE001 - the failure IS the assertion + return exc + return None + finally: + await browser.close() + + +def _open_pane(world: dict[str, Any]) -> None: + world["error"] = asyncio.run( + _drive( + world["html"], + lambda page: MigratedComposer()._open_pane(page), # noqa: SLF001 + ) + ) + + +def _text(world: dict[str, Any]) -> str: + return str(world["error"]) + + +# --------------------------------------------------------------------------- given + + +@given("a Flow project page whose settings trigger is covered") +def _covered(world: dict[str, Any]) -> None: + world["html"] = _page(cover=_CDK_COVER) + + +@given("a Flow project page whose settings trigger accepts no click") +def _never_stable(world: dict[str, Any]) -> None: + # Nothing covers it and nothing disables it — it simply never holds still. + world["html"] = _page(jitter=True) + + +@given("a Flow project page whose settings trigger is clickable") +def _clickable(world: dict[str, Any]) -> None: + world["html"] = _page() + + +@given("the agent-mode chip is pressed") +def _chip_pressed(world: dict[str, Any]) -> None: + world["html"] = _page(cover=_CDK_COVER, chip=True) + + +@given("the covering element carries an account email and a signed media URL") +def _leaky(world: dict[str, Any]) -> None: + world["html"] = _page(cover=_LEAKY_COVER) + + +@given("the page navigates away while the click is pending") +def _navigates_away(world: dict[str, Any]) -> None: + # #722's shape: by the time the failure is diagnosed the document the click was + # made against is gone, so the post-mortem read has nothing to answer with. + world["html"] = _page( + cover=_CDK_COVER, + script=f"setTimeout(() => location.replace({ELSEWHERE_URL!r}), 400);", + ) + + +# ---------------------------------------------------------------------------- when + + +@when("the driver opens the settings pane") +def _drive_open_pane(world: dict[str, Any]) -> None: + _open_pane(world) + + +# ---------------------------------------------------------------------------- then + + +@then("it fails with exit 23") +def _exit_23(world: dict[str, Any]) -> None: + from gflow_cli.errors import EXIT_CODE_MAP + + error = world["error"] + assert isinstance(error, UiSelectorDriftError), ( + f"expected UiSelectorDriftError, got {error!r} — a bare Playwright TimeoutError " + "is exactly the #776 defect: exit 1, no locator, no cause" + ) + assert EXIT_CODE_MAP[UiSelectorDriftError] == 23 + # A flag is a claim. Today's failure is non-retryable; the condition does not + # reproduce, so this is PRESERVED, not measured (Bug Lane, "A flag is a claim"). + assert is_retryable(error) is False, "retryable moved as a side effect of retyping" + + +@then("the message names the settings trigger") +def _names_trigger(world: dict[str, Any]) -> None: + assert ".settings-trigger-button" in _text(world), _text(world) + + +@then("the message names Flow's agent mode") +def _names_agent_mode(world: dict[str, Any]) -> None: + assert "agent mode" in _text(world).lower(), _text(world) + + +@then("the message does not blame an overlay") +def _not_an_overlay(world: dict[str, Any]) -> None: + # #752 finding #7: agent mode hides the trigger with a bare `hidden` and never + # touches body{pointer-events}. Reporting it as an announcement modal sends the + # user to dismiss something that was never there — #770's failure, repeated. + text = _text(world).lower() + for claim in ("announcement", "changelog", "dismiss the"): + assert claim not in text, f"blames an overlay it did not observe ({claim!r}): {text}" + + +@then("the message names the covering element by tag and structural class") +def _names_occluder(world: dict[str, Any]) -> None: + text = _text(world) + assert "div" in text, text + assert "cdk-overlay-backdrop" in text, text + + +@then("the message reports the control as visible, enabled and hit-testable") +def _reports_healthy(world: dict[str, Any]) -> None: + text = _text(world).lower() + for word in ("visible", "enabled", "hit-testable"): + assert word in text, f"missing {word!r} in: {text}" + + +@then("the message does not name a cause it did not observe") +def _no_invented_cause(world: dict[str, Any]) -> None: + text = _text(world).lower() + for claim in ("agent mode", "covered by", "announcement", "changelog"): + assert claim not in text, f"invented a cause ({claim!r}): {text}" + + +@then("the pane opens and nothing is raised") +def _control(world: dict[str, Any]) -> None: + # The A/B control. Without it every assertion above would also pass against a + # helper that raises unconditionally. + assert world["error"] is None, f"a healthy click was rejected: {world['error']!r}" + + +@then("the message contains neither the account email nor the signed URL") +def _no_pii(world: dict[str, Any]) -> None: + text = _text(world) + assert ACCOUNT_EMAIL not in text, f"leaked the account email: {text}" + assert "X-Goog-Signature" not in text, f"leaked a signed URL: {text}" + assert "googleusercontent" not in text, f"leaked a media host: {text}" diff --git a/tests/features/click_attribution.feature b/tests/features/click_attribution.feature new file mode 100644 index 00000000..8b8f88a4 --- /dev/null +++ b/tests/features/click_attribution.feature @@ -0,0 +1,71 @@ +@e2e @e2e_auth +Feature: A click that never lands says why + # #776. On flow.google.com the settings trigger reads visible, the click expires after + # 5000ms, and the run dies as a bare Playwright TimeoutError — exit 1, no locator, no + # cause. migrated_composer.py:866-869 describes that exact failure in a comment, and + # :884 is the line below the comment, still unguarded. + # + # These are e2e because Playwright's actionability gate is what fails. A mocked Page + # whose .click() is a stub cannot express "visible, enabled, and still not clickable" — + # it would pass against a fix that reads the DOM at the wrong moment, which is the #639 + # trap one surface over. Only a real browser can falsify this. + # + # Cost: zero. Every origin is served by Playwright route interception; nothing reaches + # Google and no profile is needed. + # + # The driver must never name a cause it did not observe (#770 is the live precedent for + # a confident wrong message), so "unexplained" is a first-class outcome here, not a gap. + + Scenario: A pressed agent-mode chip is named as the cause + # #752 finding #7 predicted this before #776 was filed: agent mode hides the trigger + # with a bare `hidden`, never with body{pointer-events:none}, so an overlay probe + # would read "not blocked" and report the wrong thing. + Given a Flow project page whose settings trigger is covered + And the agent-mode chip is pressed + When the driver opens the settings pane + Then it fails with exit 23 + And the message names Flow's agent mode + And the message does not blame an overlay + + Scenario: A covering element is named by its structure + Given a Flow project page whose settings trigger is covered + When the driver opens the settings pane + Then it fails with exit 23 + And the message names the covering element by tag and structural class + + Scenario: A healthy-looking failure is reported as unexplained + # Rules out three of Playwright's four conditions and points at the fourth (stable), + # which is the honest answer when nothing readable is wrong. + Given a Flow project page whose settings trigger accepts no click + When the driver opens the settings pane + Then it fails with exit 23 + And the message reports the control as visible, enabled and hit-testable + And the message does not name a cause it did not observe + + Scenario: An unobstructed trigger still opens the pane + # The A/B control. Without it every scenario above would also pass against a helper + # that raises unconditionally. + Given a Flow project page whose settings trigger is clickable + When the driver opens the settings pane + Then the pane opens and nothing is raised + + Scenario: An account identifier on the covering element never reaches the message + # PR #777 shipped two hours before this feature for the same bug class one surface + # over. A typed error prints `detail` raw to the console, to structlog and to --json, + # while the bare TimeoutError it replaces was SHA-256 hashed — so this fix REMOVES a + # privacy net and has to put back a deliberate one. + Given a Flow project page whose settings trigger is covered + And the covering element carries an account email and a signed media URL + When the driver opens the settings pane + Then it fails with exit 23 + And the message contains neither the account email nor the signed URL + + Scenario: A page that cannot be read still reports the failed locator + # #722's shape: by the time the failure is diagnosed, the document the click was made + # against is gone and the post-mortem read has nothing to answer with. A diagnostic + # must never replace the failure it is describing. + Given a Flow project page whose settings trigger is covered + And the page navigates away while the click is pending + When the driver opens the settings pane + Then it fails with exit 23 + And the message names the settings trigger diff --git a/tests/worker/test_daemon.py b/tests/worker/test_daemon.py index 449bd156..afc04b3c 100644 --- a/tests/worker/test_daemon.py +++ b/tests/worker/test_daemon.py @@ -10,7 +10,13 @@ from gflow_cli.api.video import VideoResult, VideoStatus from gflow_cli.data.store import DataStore -from gflow_cli.errors import DataIntegrityError, DataStoreError, FlowApiError, MediaAttributionError +from gflow_cli.errors import ( + DataIntegrityError, + DataStoreError, + FlowApiError, + MediaAttributionError, + UiSelectorDriftError, +) from gflow_cli.worker.daemon import FlowWorker from gflow_cli.worker.queue import QueueRepository @@ -785,3 +791,69 @@ async def test_migrated_host_error_crosses_the_queued_path(temp_db: DataStore) - assert updated.error["retryable"] is False assert updated.error["retryable"] is is_retryable(exc) worker.close() + + +# --------------------------------------------------------------------------- +# #776 — what an MCP caller actually receives when a click never lands +# +# The Iron Law applies to the MCP twin separately: the CLI and the queued path are +# two doors, and the adapter — not the shared transport — was always the risk. These +# two run the SAME `process_task` branch with the two exception shapes, so the +# difference between them is attributable to the retyping and nothing else. +# --------------------------------------------------------------------------- + + +async def _fail_t2v_with(temp_db: DataStore, exc: BaseException, task_id: str) -> dict: + repo = QueueRepository(temp_db) + task = repo.enqueue_task( + task_id=task_id, + profile_name="default", + task_type="t2v", + payload={"prompt": "a click that never lands"}, + ) + worker = FlowWorker("default", str(temp_db.path)) + fake_client = FakeFlowApiClient() + fake_client.generate_video.side_effect = exc + with patch("gflow_cli.worker.daemon.FlowApiClient", return_value=fake_client): + await worker.process_task(task) + updated = repo.get_task(task_id) + worker.close() + assert updated is not None and updated.error is not None + return updated.error + + +@pytest.mark.asyncio +async def test_a_bare_timeout_reaches_an_mcp_caller_as_a_hash(temp_db: DataStore) -> None: + """The control, and the reason #776 was unactionable over MCP. + + A non-``GFlowError`` takes `daemon.py`'s `else` branch, which ships a SHA-256 of the + message and nothing else — not the locator, not even the exception class. An agent + receiving this cannot tell a covered button from a dead network. + """ + error = await _fail_t2v_with(temp_db, TimeoutError("Timeout 5000ms exceeded"), "task-776-bare") + assert error["exit_code"] == 1 + assert error["detail"].startswith("sha256:") + assert "settings-trigger" not in error["detail"] + + +@pytest.mark.asyncio +async def test_the_typed_failure_reaches_an_mcp_caller_as_problem_details( + temp_db: DataStore, +) -> None: + """The fix, measured on the surface it changes most. + + Retyping the raise site is the whole MCP repair: the same failure now takes the + ``isinstance(exc, GFlowError)`` branch and arrives as RFC 9457 problem details with + exit 23 and the locator intact. + """ + detail = ( + "migrated host: .settings-trigger-button did not accept a click within 5000 ms " + "— it is covered by div.cdk-overlay-backdrop (host=migrated)" + ) + error = await _fail_t2v_with(temp_db, UiSelectorDriftError(detail=detail), "task-776-typed") + assert error["exit_code"] == 23 + assert not error["detail"].startswith("sha256:") + assert ".settings-trigger-button" in error["detail"] + assert "cdk-overlay-backdrop" in error["detail"] + # A flag is a claim: retyping must not have made this retryable by side effect. + assert error["retryable"] is False diff --git a/website/docs/E2E_TESTING.md b/website/docs/E2E_TESTING.md index 5f952ca1..e06e206b 100644 --- a/website/docs/E2E_TESTING.md +++ b/website/docs/E2E_TESTING.md @@ -132,6 +132,18 @@ directories, whose scenarios would run twice. > browser — that is the nightly canary's job (`scripts/canary/`), on a machine that has > one. Hosted CI cannot run the live tiers and never could. +**Two worked examples, deliberately different in kind:** + +| Feature | Binder | What only a browser could prove | +|---|---|---| +| `landing_state_diagnosis.feature` | `test_landing_state_diagnosis_bdd.py` | Flow's hop to `/about` is a **client-side** redirect, so `goto` returns before it runs (#639). A mocked page whose `url` the test assigns cannot fail that way | +| `click_attribution.feature` | `test_click_attribution_bdd.py` | Playwright's **actionability** gate — visible, stable, receives-events, enabled (#776). Each scenario breaks a different one *for real*: a stacked `div` that intercepts pointers, and a CSS animation that never lets the box settle while visibility and the hit test stay healthy | + +Both are route-intercepted and cost **$0** — real Chromium, `page.route(...).fulfill(...)`, +no Google, no profile, no credits. That combination is what makes a browser-only scenario +cheap enough to be non-negotiable: if a scenario needs a browser, the answer is an e2e +test, not a mocked proxy — the Bug Lane's step 5. + --- ## Environment variables diff --git a/website/docs/KNOWN_ISSUES.md b/website/docs/KNOWN_ISSUES.md index d5433ba6..007c91aa 100644 --- a/website/docs/KNOWN_ISSUES.md +++ b/website/docs/KNOWN_ISSUES.md @@ -1237,6 +1237,29 @@ now names which of three things happened rather than blaming drift: **On 0.71.0 and earlier there is no recovery.** Open the project on `flow.google.com`, click the **Agent** chip off, and the account works again. +**Follow-up ([#776](https://github.com/ffroliva/gflow-cli/issues/776)) — the same +confusion survived one gate later, on the *click*.** The table above covers the readiness +*wait*. A control that passes that wait and then refuses the click used to expire as a bare +Playwright `TimeoutError`: exit 1, no locator, no cause. It now reports what was observed +at the moment it expired, because the cause could not be measured — Flow's announcement +overlay is a labs.google measurement that has never been reproduced on this host, and a +mid-run agent-mode flip is equally consistent with the evidence. + +| The message says | What it means | What to do | +|---|---|---| +| `… did not accept a click … the account is in Flow's agent mode` | the mode flipped after the editor was ready | turn the **Agent** chip off in a browser; re-run | +| `… it is covered by .` | something is stacked over the control — the class names it | dismiss it in a browser; re-run | +| `… the page is accepting no pointer events at all` | an overlay has the whole app blocked (#593's shape) | dismiss it in a browser; re-run | +| `… it carries a bare `hidden` attribute` / `it is disabled` | the control is present but not usable | usually agent mode or a cohort difference; check the Agent chip first | +| `… it is not rendered (display, visibility, or a zero-sized box)` | it is in the DOM but not on screen | as above — check the Agent chip, then file a bug with the log | +| `… it answers no hit test at its own centre` | nothing named itself as the cover, but the click still landed elsewhere | re-run once; if it repeats, file a bug — an overlay outside the document is the usual shape | +| `… it was visible, enabled and hit-testable … most likely still moving` | nothing readable was wrong | Playwright also needs a *stable* box; re-run once. If it repeats, file a bug — this message means we looked and found nothing, which is a real finding worth having | +| `… it could not be read back` | the page changed under the diagnosis | re-run; if it repeats, attach the log | + +The occluder is named by tag plus framework class only. That is deliberate — a signed-in +Flow page carries the account email and signed media URLs on exactly the elements that +tend to occlude things, and this message is printed, logged, and pasted into issues. + ### Auth verification depends on Google's NextAuth session endpoint - **Status:** Mitigated · **Severity:** Low (degrades fail-closed) · **Affects:** issue #15 fix onward · **Tracked:** issue #15