fix(auth): autoselect recorded account at post-migration chooser - #764
fix(auth): autoselect recorded account at post-migration chooser#764stgmt wants to merge 9 commits into
Conversation
|
Thanks for the contribution. This PR has been marked as an external contribution and routed for maintainer review. Before merge, please make sure:
GitHub Copilot code review may be requested as an advisory first pass, but maintainer approval is still required. |
|
🤖 PR-Triage Autopilot Verdict: UNKNOWN A late-arriving sub-agent (spawned internally by one of the council's skill invocations) surfaced two genuinely new findings I hadn't already captured — worth appending to the verdict already delivered. Addendum to PR #764 verdict (2 new must-fix items — count now 12)
Both are corroborated by direct code citation ( Updated line: Consensus remains 🔴 RED — this doesn't change the verdict, just deepens the case against merging as-is. |
Council Review Verdict — 🔴 REDReviewed at Thanks for this — the problem is real, the issue analysis is right, and
Must-fixA. Broken documentation1. The exit-code table is corrupted in both USAGE mirrors. This destroys the Both mirrors are identically broken, which is why B. The feature does not behave as documented2. Substring matching can select the wrong account — with billing consequences. Nothing re-verifies identity after the click on the generation path, so every subsequent run silently generates and bills against the wrong Google account. Suggest matching exactly ( 3. The probe runs before the redirect settles, so it may never fire. 4. The click has no post-condition, and the documented exit-38 case cannot occur.
Relatedly: if 5. 6. A mismatch leaves a fully-authenticated wrong-account profile on disk. So 7. The remediation hint points at a flag that cannot help. 8. An apostrophe in the email throws a raw Playwright error. 9. Google's bot-rejection URL matches C. Evidence10. No e2e test, and the unit tests assert the mock rather than the behaviour. More importantly, the production callsite is untested. The only two tests reaching Your stated blocker for the click-through branch ("the recorded account was already signed in at proof time") is fair, specific, and exactly the kind of thing the contributor exemption is for. It doesn't cover the absence of any e2e file, though — including one your own signed-in account could run, asserting bootstrap still completes with the new callsite present. Suggested home: 11. 12. A selector-cascade miss now raises 38 with no drift detail, and no record of the carve-out. Related, and worth stating plainly: Nice-to-have
Checked and found fine — no action neededRecording these so you don't spend time on them:
Note on CIAll 14 other checks are green, but What would close this out
Items 1, 3, 4 and the nice-to-haves are quick. Items 2 and 5 are the substantive ones. Happy to re-run the council once you've pushed. |
|
🤖 PR-Triage Autopilot Verdict: UNKNOWN PR #764 — Council Review Verdict (Autonomous Mode)Reviewed at Methodology note: This PR already carries a complete, citation-backed council review posted to the thread at Consensus: 🔴 RED
Must-fix (11 remaining of the original 12 — confirmed still present at
|
|
All 11 open must-fix items addressed in Item-by-item:
Nice-to-haves, all done: dup ordering test deleted; Not changed per your "confirmed-good": MCP parity (untouched), CHANGELOG/AGENTS (already accurate), SonarCloud (repo-side). One honest scoping note: the live click-through branch itself is still test-only — the recorded account was signed in at proof time, so |
Council review — consensus 🔴 RED (reviewed at
|
|
I've pushed the blocker fix to this branch directly (maintainer edits are enabled) so it isn't sitting on you — two commits,
|
| result | |
|---|---|
| with the fix | 4 passed in 35.71s |
against the original client.py |
3 failed, 1 passed |
and the failure is the predicted one, verbatim:
E waiting for navigation to "**/project/**" until 'load'
playwright/_impl/_frame.py:264: TimeoutError
I also updated the two unit tests that encoded the false contract — the happy path now pins return_value=None and asserts the predicate accepts both cohorts while rejecting the chooser, and the failure path drives a real PlaywrightTimeoutError side effect and asserts the cause is chained rather than swallowed.
Local gates at 970d9aef: ruff check clean, ruff format clean, pyright src at baseline, tests/api/test_bootstrap_chooser.py + tests/auth/test_account_autoselect.py 9 passed, the new e2e 4 passed.
Still open from the review
Nothing here touches the other must-fixes — the substring host test at client.py:803, the duplicated GOOGLE_REJECTED_BROWSER_ROUTE literal, the docs/USAGE.md:1790 remediation that recommends a flag which cannot select a row, the remediation/detail contradiction on the two raise sites, or the email-in-logs redaction gap. The PR body still says exit 33 as well.
And the live gap is unchanged: this proves the mechanism, not that [data-email] is still the live chooser's anchor. That needs a signed-out account on a migrated profile, which the presentation-reels-google evidence isn't. I'm happy to run that side.
|
🤖 PR-Triage Autopilot Verdict: UNKNOWN PR #764 — Council Review Verdict (Autonomous Mode)Reviewed at Methodology note. This PR already carries three prior full/partial council rounds in-thread (RED at Sandbox constraint (read this before trusting D0/D4 below). This session's tool permissions allow Consensus: 🔴 RED
Must-fix (9)
Nice-to-have (2)
Confirmed-good
False positivesNone — every claim above was verified against Memory actions (D5) — reported only, not applied
Note on CI
Next step — live validation. Not reached. This verdict is RED on both mechanical (D0) and multiple LLM dimensions; live validation is not the next step until must-fix items 1–8 are resolved and CI is confirmed green from an actual log (not a rollup status), which requires tooling outside this sandbox. |
|
🤖 PR-Triage Autopilot Verdict: UNKNOWN PR #764 — Council Review Verdict (Autonomous Mode)Reviewed at Methodology note. This is round 5 of council review on this PR. Four prior rounds are posted in-thread (RED→RED→RED→RED across Consensus: 🔴 RED
Must-fix (6)
Nice-to-have (5)
Confirmed-good
False positivesNone — every claim above (mine and all 5 dispatched agents') was independently verified via direct file reads at Memory actions (D5) — reported only, not applied
Note on CI
Next step — live validation. Not reached. This verdict is RED on 3 LLM dimensions (D1, D8, D9); must-fix items 1–6 need resolving before another round. Items 2, 3, 4 are quick (a stale string in 3 places + a comment fix); items 1, 5, 6 are the substantive ones (an exact-host check, an email-redaction pattern, and a locale-read reorder or guard). |
Signed-off-by: stgmt <stigmat.rudnev@gmail.com>
33 is already claimed by the gflow doctor verdict (a successful diagnosis, not an error class) in EXIT_CODE_MAP documentation; the new typed chooser error must not collide. 38 is free and sits adjacent to the UI-cluster codes. Downstream presentation-reels client keys on class/38/retryable=false. Signed-off-by: stgmt <stigmat.rudnev@gmail.com>
The previous commit split the 37 row and dropped the 38 row from both docs copies; the table now carries the full 37 row followed by the new 38 row (FlowAccountChooserError). Signed-off-by: stgmt <stigmat.rudnev@gmail.com>
Exact data-email row match plus exact-text fallback (a substring superset can no longer win and bill the wrong account); chooser check moved after the locale settle; click-through verified by wait_for_url to the editor; bot-rejection hop excluded from the chooser heuristic; --account raises when the identity is unreadable and warns what the profile now holds on mismatch; remediation names only gflow auth login; chooser wired into diagnostics capture triggers; exit-38 carve-out recorded in the selector memory; e2e_auth bootstrap test added. Signed-off-by: stgmt <stigmat.rudnev@gmail.com>
…iable `Page.wait_for_url` is annotated `-> None`: it returns nothing and signals a miss by raising. So `(await page.wait_for_url(...) or "")` was always `""`, `"project" not in ""` was always True, and every SUCCESSFUL chooser click raised FlowAccountChooserError. The success path — the logger.info and `return True` beneath it — was unreachable in production, and a real stall raised an uncaught Playwright TimeoutError, surfacing as the same opaque exit 1 that ffroliva#763 is about. The landing predicate also changes. `**/project/**` cannot match the bootstrap URL `labs.google/fx/tools/flow`, and only the migrated origin serves /project/<id>, so the glob encoded one cohort's shape as universal. `flow_host_kind` is the codebase's exact-host classifier and answers for both cohorts; a substring test would match any URL merely carrying the host in a ?continue= parameter. The happy-path test asserted `AsyncMock(return_value=".../project/p1")` — a value Playwright cannot produce — which is why CI stayed green over an inert fix. It now pins the real contract: wait_for_url returns None, raises on a miss, and the predicate accepts both Flow cohorts while rejecting the chooser itself.
… page The defect the previous commit fixes shipped through a green unit suite because that suite mocked `page.wait_for_url` as returning a URL string — a contract Playwright does not have. A mock cannot falsify a belief about the mocked thing, so this adds tests that drive a real Chromium: real locator engine, real click, real navigation, real wait_for_url. Zero cost and no Google account: the chooser and both Flow landings are served by route interception, which also makes it deterministic — a real signed-out chooser cannot be staged on demand. Covers the branch production actually uses (the .gflow_account read, which every unit test bypasses by passing the address in), both host cohorts, the real-timeout failure path, and the wrong-account hazard that motivates the exact [data-email=] match. A/B: 4 passed against the fix; 3 failed against the original client.py, the click-through cases dying on an uncaught Playwright TimeoutError — `waiting for navigation to "**/project/**"` — which is the ffroliva#763 symptom. Also types `_handle_account_chooser(page: Page)`. It was `page: Any`, which is what let the inverted check past `pyright src` in the first place; with a real type the landing predicate's parameter is known and the gate has something to check.
`page: Any` erased `wait_for_url`'s signature, so the landing predicate's parameter was unknown and `pyright src` failed with reportUnknownLambdaType — the same looseness that let the original inverted check through the gate. This was claimed in 970d9ae and was not in it: an A/B control there restored client.py from HEAD, which at that moment predated the edit, so the change was silently reverted before the commit. CI caught it on all three Python versions.
Exact-host chooser gate via urlsplit (substring ?continue= can no longer misfire); rejected-browser route imported from its single source instead of an inline literal; USAGE row 38 names only gflow auth login; memory carve-out corrected to match the code (no bot-rejection inclusion, no URL-kind claim); account addresses get their own <redacted:email> pattern in the redaction module with call sites wired; session locale re-read from the editor after click-through; account_email test seam removed; --account success-path test added. Signed-off-by: stgmt <stigmat.rudnev@gmail.com>
b34952c to
fbbbaf3
Compare
|
All 6 must-fix items from round 5 addressed in
Nice-to-haves: Gates at HEAD: ruff check/format, pyright 0, hygiene, doc-links, website mirror in sync, PII clean; 134 focused tests green (incl. redaction + parity suites). Live e2e_auth from the prior round still stands (4 passed, $0); the signed-out chooser click remains yours to run whenever convenient. |
The exact-host urlsplit gate raised TypeError on mocked pages (url is a MagicMock), reddening every suite that enters the client context. Mirror flow_host_kind discipline: non-string or unparseable URLs return False so a probe error never displaces the real bootstrap failure. Regression test pins it. Signed-off-by: stgmt <stigmat.rudnev@gmail.com>
Summary
Fixes #763. When the post-migration hop lands on the Google account chooser
(
accounts.google.com/v3/signin/accountchooser) with the recorded account rowsigned out, the CLI now auto-selects the profile's recorded account from
.gflow_accountright after bootstrapgoto, before any mint/probe work.Unselectable cases (row absent, click-through does not return to
editor/session page,
--accountmismatch) raise a dedicated non-retryableFlowAccountChooserError(exit 38) instead of stalling into an opaqueUnexpectedError(exit 1) or selector-drift report.gflow auth logingains--account <email>to assert recorded == verified email.Sibling of #756 (the
/aboutlanding misreported asUiSelectorDriftError);this covers the accountchooser landing state.
Lifecycle
checkgreen, including the step 1b CLI-MCP mirror sweep (auth loginis exempt in
tests/mcp/test_cli_parity.pyas interactive sessionmanagement, so the new
--accountflag needs no MCP twin)auth status+ mintdry-run only; Generate never clicked); paid generation paths untouched
is stated below (the chooser auto-click branch itself: the recorded
account was already signed in at proof time, so the click-through is
covered by unit + bootstrap tests, not by a live chooser click)
Validation
[Unreleased] -> Added)uv run python scripts/ci/check_doc_links.pyuv run ruff check src testsuv run pyright srcuv run pytest -q tests/auth/ tests/api/test_bootstrap_chooser.py tests/api/test_client_migrated_mint.py(93 passed) +
tests/test_errors.py tests/mcp/test_cli_parity.py(63 passed)Live evidence ($0 — navigation and DOM reads only, Generate never clicked)
gflow auth status --profile presentation-reels-google-> Flow session verifiedf1ad61287861586d28fed31e6b3fce6a5f02833e1834a6213027a2e73990a940Contribution Checklist
developSigned-off-by:(git commit -s)