Repository navigation
fix(transports): name a known Flow landing instead of blaming the selector - #775
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (28)
📝 WalkthroughWalkthroughThe change adds shared Flow landing-page classification, typed errors for public and authentication landings, per-instance retryability overrides, browser-based diagnosis coverage, and updated bug-resolution and verification documentation. ChangesLanding-state diagnosis
Estimated code review effort: 4 (Complex) | ~45 minutes Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant Browser
participant Transport
participant LandingClassifier
participant ErrorEnvelope
Browser->>Transport: Navigate to requested Flow page
Transport->>Browser: Wait for readiness or selectors
Transport->>LandingClassifier: Classify landed URL after failure
LandingClassifier->>ErrorEnvelope: Create typed error and retryability
ErrorEnvelope-->>Transport: Return diagnostic failure
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ector Closes #756. Also fixes the 2026-09-10 RED nightly canary (#559), which is the same defect one URL apart. flow_host_kind() classifies the ORIGIN. /about, /project/<id> and /fx/api/auth/signin?error=Callback all share one, so when a readiness wait timed out it had nothing left to blame but its own anchor — sending the operator to "check for a newer release, then file a bug" over a session state no release changes. Reported at three sites and special-cased three times before (#721 credits, #749 agent mode, FlowAppError's own crash page), so it is fixed once, in the shared place, rather than a fourth time. - flow_landing_kind() joins flow_host_kind() in api/transports/_common.py and names a known non-app landing ("signin" / "public" / None). raise_for_known_landing() converts the diagnosis at the raise site. - Consulted ONLY inside an already-failed branch. Not ahead of the probe, which would delete the DOM evidence that corrects a wrong absence claim (#739 shipped one that way), and not as a bounded wait after goto, which reads page.url before Flow's client-side redirect lands (#639). - flow.google.com/about instead of the project -> FlowAppError (exit 31), naming the landing and the project it did not open, and deliberately not why: #756 measured the redirect and not its cause, and auth status reports the session verified while it happens. - labs .../auth/signin?error=Callback instead of the gallery -> AuthExpiredError (exit 3). This is the exact page behind the RED canary. - auth/internal_chromium.py drops its private _NEXTAUTH_ROUTE_PREFIX and reuses the shared classifier. The knowledge existed there since #767; no transport could reach it. Retryability was measured, not assumed. Routing /about to exit 31 would have flipped it from non-retryable to retryable purely as a side effect. The spike (scripts/dev/spike_about_redirect_stability.py, $0, ci-probe) came back 0/5 — the redirect stopped reproducing between 2026-09-08 and today — which is equally consistent with "transient" and with "a session state changed", and so settles nothing. The reading was pre-registered in the script before the run. Therefore GFlowError gains a per-instance `retryable` override (same class-default / instance-override shape as remediation_hint) and the /about raise site passes retryable=False to PRESERVE its previous exit-23 answer, not to claim a retry fails. is_retryable() pins the override with isinstance(..., bool): a MagicMock answers every attribute with a truthy child, so a truthiness test would report every mocked error as retryable with nothing noticing. Tests: tests/features/landing_state_diagnosis.feature bound from tests/e2e/test_landing_state_diagnosis_bdd.py — the first BDD-bound e2e. Three scenarios against a real browser over route interception, zero credits: both landings, plus an A/B control proving genuine drift still reports as drift. RED first (the canary's exact RuntimeError, reproduced deterministically), then green. Plus classifier unit tests and five retryability tests. tests/test_marker_registry.py now resolves a BDD module's cost tier through its Gherkin tags. It read Python source only, so it saw no pytest.mark and failed a correctly-tiered file; copying the tier into a pytestmark would have satisfied it and then drifted from the Feature.
12-dimension council on 63fb5bb: 3 GREEN (correctness, auth, BDD), 9 YELLOW, 0 RED, ~19 must-fix. Two dimensions found defects in the workflow hardening itself rather than in the fix. Security (D3): the landing URL is stripped to scheme+host+path before it reaches either the message or the log. The NextAuth family the "signin" arm matches includes /fx/api/auth/callback/google?state=...&code=..., and because AuthExpiredError is capture-exempt that message is the ONLY artifact — so it is exactly what a user pastes into an issue. redact_error_detail was already imported in this module and used 300 lines below; the new raise skipped it. Truthfulness (D2, D15, D9): the message said "its sign-in page" for a family that also contains the callback and /session. docs/MCP.md still told agents FlowAppError is unconditionally retryable and that RETRYABLE_ERRORS "is the whole" list; docs/USAGE.md's exit-3 row still described only the 401/403 trigger; and a docstring claimed the helper "never adds a failure", which is false at migrated_composer.ensure_editor, whose except branch has a recovery path. Reach (D6): a THIRD site. _enter_editor(project_id=...) has no readiness gate, so a landing page reaches _locate_prompt_box and raises a bare RuntimeError, which observability.py hashes because it is not a GFlowError — the operator saw "Unexpected error" with even the URL destroyed. Worse than the report #756 is about, and the guard was not consulted there. Scope (D14, D1): retryable moves off GFlowError onto FlowAppError, the one class that needs it — is_retryable reads it by getattr, so a base field bought no typing and no test-double visibility, only a field on every error in the project. The FlowApiError pass-through it required is deleted. flow_landing_kind's "public" arm is gated to the migrated host, the only one where /about was measured. Instrument (D13): the spike re-implemented the predicate it was measuring (endswith("/about") vs the production classifier), which would score /about?hl=en as a non-reproduction — the #743 shape its own docstring cites. It now calls flow_landing_kind. except BaseException became except Exception: the former swallowed Ctrl-C, recorded it as a non-/about sample and advanced the loop, so an interrupted run could print "DOES NOT REPRODUCE" assembled from interrupts. Guards (D4, D12): the binding guard could pass on an empty scan — every check is assert-not-<list>, so a mis-resolved _FEATURES_DIR made all of them vacuous while the fire-test still passed. It now asserts it scanned something. The "one feature, one binding module" rule stated in docs was the one rule nothing enforced; it is enforced now. Feature-name matching is anchored on the path separator. The "signin" branch had zero offline coverage (only the e2e reached it, and addopts excludes that), so the 4194-pass run never executed it. Memory (D5): docs/superpowers/memory/ui-selector-drift-error-exit-23.md keeps a carve-out ledger for exceptions to exit 23 — exits 36, 37 and 38 each recorded when introduced. Carve-out 4 is now recorded, with the transferable lesson the branch is actually about: flow_host_kind classifies the ORIGIN, not the page. Hardening defects the council found in the first commit (D9): the lane said "steps 3-5 have no gate" beside "step 3 = that Gherkin RED", which is unsatisfiable when step 2 is skipped — the gate is red-before-green, never Gherkin-before-green. And the continuation table routed 1 -> 1b -> 3, orphaning Phase 2 (predict) while the routing table still mandates it for exactly this kind of change. Declined, with reasons: excluding /about from incident capture (FlowAppError also covers the React crash page, where the DOM helps, and a class cannot exclude one shape), and moving the guard after _capture_debug_screenshot (D3 showed that placement removes an existing screenshot of a signed-in page; D9's answer, taken, is to document the capture change in DEBUGGING.md rather than revert it). Verified: 4197 passed, 92% coverage; e2e 3/3 in a real browser at $0; ruff, format, doc-links, website mirror, council-memory and hygiene gates all green.
Found by live-verifying the previous commit instead of trusting it. Same
profile, same test, real Google, $0:
BEFORE RuntimeError: Could not find 'New project' CTA on Flow gallery.
URL: https://accounts.google.com/v3/signin/accountchooser
?client_id=...&code_challenge=...&state=PKOA6qjxDh...
AFTER FlowAccountChooserError: Google's account chooser is displayed
(https://accounts.google.com/v3/signin/accountchooser) instead of
the Flow gallery - the session needs a person to pick an account.
Not selector drift.
`flow_landing_kind` returned None for accounts.google.com by design, and a unit
test asserted it, on the reasoning "the chooser has its own handler". That is
true at BOOTSTRAP - `client._handle_account_chooser` - and false for a hop that
happens after it, which is what profile `denon82` did on 2026-09-10: it left the
`?error=Callback` state the 02:00 canary reported and moved to Google's chooser,
where `_enter_editor` swept eighteen selectors for a "+ New project" CTA on
Google's sign-in page and blamed the anchor. So the previous commit's claim to
fix the RED canary was false for the state the profile is actually in.
The classifier now answers for Google's auth host too: "chooser" for a
`/v3/signin/accountchooser` path -> FlowAccountChooserError (38, the class #763
and #764 already established for this), "signin" for its other sign-in surfaces
-> AuthExpiredError (3). The bot-rejection hop (`/v3/signin/rejected`) keeps
returning None; it has its own error and must never read as a missing account.
Path-tested rather than importing GOOGLE_REJECTED_BROWSER_ROUTE, because
`_common` -> `auth` is a real import cycle - which is why `internal_chromium`
imports this module deferred.
`_is_safe_to_probe_session` is unaffected, verified: its `flow_host_kind(url) is
None` short-circuit returns False for accounts.google.com before this function
is consulted, so the cookie-rotation gate the council proved equivalent over 393
cases still behaves identically.
D3's redaction finding is no longer hypothetical - that live message carried the
OAuth `state`, `code_challenge` and `client_id`, and this is the artifact users
paste into issues. The stripping added in 7e29be4 removes them.
Not in scope, and recorded rather than smuggled in: the mid-run chooser now
DIAGNOSES but does not RECOVER. #764 taught the bootstrap path to autoselect the
recorded account; that lives on FlowApiClient and is not reachable from a
transport helper. Teaching this hop to autoselect would rescue the run instead of
naming the failure.
Verified: 4202 passed, 92% coverage; the live A/B above; ruff, format,
doc-links, website mirror and hygiene gates green.
Rebasing onto develop after #774 squash-merged stacked this branch's Added section above the one that arrived with the hardening, leaving two under [Unreleased]. Keep a Changelog wants one per type; content is unchanged.
ffroliva
force-pushed
the
bugfix/landing-state-not-drift
branch
from
September 10, 2026 13:40
aac0ef8 to
e8c4cd5
Compare
ffroliva
marked this pull request as ready for review
September 10, 2026 13:49
This was referenced Sep 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #756. Improves the 2026-09-10 RED nightly canary (#559) — see Live verification below for exactly how far.
Root cause
flow_host_kind()classifies the origin, never the page./about,/project/<id>and/fx/api/auth/signin?error=Callbackall satisfy the same host check — so a readiness wait that timed out had nothing left to blame but its own anchor:migrated_composer.ensure_editorflow.google.com/aboutUiSelectorDriftError(23) — #756ui_automation._enter_editorgallery sweeplabs.google/fx/api/auth/signin?error=CallbackCould not find 'New project' CTA— the RED canaryui_automation._locate_prompt_boxRuntimeError→ SHA-256 hashed → "Unexpected error", URL destroyedaccounts.google.com/v3/signin/accountchooserCould not find 'New project' CTA— found by live-verifying this PRclient._handle_account_chooseraccounts.google.comat bootstrapThe fourth instance of one pattern (after #721 credits, #749 agent mode, and
FlowAppError's crash page), so it is fixed once in the shared place.The fix
flow_landing_kind()joinsflow_host_kind()in_common.pyand answers the missing question ("chooser"/"signin"/"public"/None);raise_if_known_landing()converts the diagnosis at three raise sites.goto, which reads the URL before Flow's client-side redirect lands (Flow's migrated flow.google.com frontend: text-to-video is driven (v0.67.0); image, i2v/r2v, characters, scenes, extend, instructions, tools and project creation still pending #639).FlowAccountChooserError(38) · sign-in routes →AuthExpiredError(3) ·/abouton the migrated host →FlowAppError(31). The bot-rejection hop keeps its own error./aboutdeliberately does not say why: A /about redirect on flow.google.com is reported as UiSelectorDriftError (exit 23) #756 measured the redirect and not its cause, andauth statusreports the session verified while it happens./fx/api/auth/callback/google?state=…&code=…, and this message is what users paste into issues.auth/internal_chromium.pydrops its private_NEXTAUTH_ROUTE_PREFIXand reuses the shared classifier.Live verification — and a claim it disproved
An earlier revision of this PR said it fixed the canary. It did not, and the live run is what caught that. Same profile (
denon82), same test, real Google, $0:denon82had left the?error=Callbackstate the 02:00 canary reported and moved to Google's chooser — a landingflow_landing_kindexcluded by design, with a unit test encoding the reason "the chooser has its own handler". True at bootstrap; false for a hop after it. Fixed inaac0ef83.The test still fails, correctly: that profile's session is genuinely broken and no code change fixes it. What is verified is the diagnosis, and D3's redaction finding is now demonstrated rather than argued.
Retryability was measured, not assumed
Routing
/aboutto exit 31 would have flipped it from non-retryable to retryable purely as a side effect.scripts/dev/spike_about_redirect_stability.py($0,ci-probe, 5 attempts) came back 0/5 — the redirect stopped reproducing between 2026-09-08 and 2026-09-10.The reading was pre-registered in the script before the run:
0/Nsettles nothing, because a disappearance is equally consistent with "transient" and with "a session state changed". SoFlowAppErrorgains a per-instanceretryableoverride and the/aboutsite passesretryable=Falseto preserve its previous exit-23 answer — not to claim a retry fails. Write-up:docs/superpowers/spikes/2026-09-10-about-redirect-stability.md.Tests
tests/features/landing_state_diagnosis.featurebound fromtests/e2e/test_landing_state_diagnosis_bdd.py— the repo's first BDD-bound e2e. Real Chromium over route interception, zero credits: both landings plus an A/B control proving genuine drift still reports as drift. RED first, then green.Council review
12-dimension
/gflow:branch-reviewon63fb5bbd: 3 GREEN (correctness, auth, BDD), 9 YELLOW, 0 RED, ~19 must-fix — all applied in7e29be49. Highlights:code=/state=were being interpolated into the one artifact users paste into issues._is_safe_to_probe_sessionrewrite equivalent over 393 cases, 0 diffs. That function gates a cookie-rotation hazard; still equivalent afteraac0ef83(itsflow_host_kindshort-circuit fires before the new branch).retryablewas on the base class of every error to serve one raise site; moved toFlowAppError.docs/MCP.mdtold agentsFlowAppErroris unconditionally retryable whileUSAGE.mdsaid the opposite.Two findings declined with reasons (excluding
/aboutfrom incident capture; moving the guard after the debug screenshot) — see7e29be49.Test plan
4202 passed, 92% coverage3 passedin a real browser,-m e2e_auth, $0 — green after every council fixruff,format,check_doc_links,generate_website_docs --check,check_website_docs_pii,check_council_memory,check_repo_hygienedenon82— the chooser arm, real Google, $0ci-probe(real Chrome —channel=chromeconfirmed in the run log)/aboutarm. It stopped reproducing onci-probebefore it could be reached (0/5), so it rests on route-intercepted HTML. Recorded, not claimed.Follow-up, not in scope
The mid-run chooser now diagnoses but does not recover. #764 taught the bootstrap path to autoselect the recorded account; that lives on
FlowApiClientand is not reachable from a transport helper. Teaching this hop to autoselect would rescue the run instead of naming the failure — worth its own issue.Summary by CodeRabbit
Bug Fixes
/aboutredirects are identified as non-retryable, while authentication issues retain appropriate retry guidance.Documentation
Testing