chore(release): v0.73.0 - #779
Merged
Merged
Conversation
…ser (#764) When Google hands the session to flow.google.com and redirects to an account chooser, FlowApiClient now auto-selects the profile's recorded account from .gflow_account instead of stalling into an opaque RecaptchaError/exit 1. - Exact, case-insensitive row match on both tiers; anchored so the chooser's "Remove <email>" / "Sign out of <email>" rows can never be clicked. - A chooser is identified positively (chooser path or account rows), so an ordinary expired session still classifies as AuthExpiredError (exit 3). - Unselectable cases raise a typed, non-retryable FlowAccountChooserError (exit 38) naming the URL the session actually landed on. - gflow auth login --account <email> asserts login authenticated as the required account, failing closed on mismatch. - .gflow_account is treated as untrusted input, fixing an untyped failure in the selector and a crash in gflow auth list on a damaged file. Closes #763. Follow-up hardening tracked in #773.
…to fix (#774) Formalises the chain a bug travels here — spike -> systematic-debugging -> BDD -> TDD -> fix -> e2e — canonically in skills/issue-resolve/SKILL.md, cited from AGENTS.md, skills/spike, skills/scenario, docs/E2E_TESTING.md and docs/INDEX.md. Written once; nothing restates it, because a duplicated checklist drifts. Gated by surface, not severity: steps 0-2 are skippable when the cause is already proven and the blast radius is one line, but a skip is a claim and has to be said out loud. Steps 3-5 have no gate. Removes a contradiction that no gate could see. issue-resolve step 3 read "the test is the closest browser-free proxy" while AGENTS.md's Iron Law read "if no e2e test covers the change, write one" and listed "covered by unit tests" among the excuses that are NOT blockers. Disjoint files, clean merge, and an agent following either one was compliant with the other's opposite. "Browser-free" is no longer a blocker; only a named external blocker is. Wires BDD to e2e with no new machinery: pytest-bdd converts Gherkin tags into pytest markers (verified on 8.1 — plugin.py:137), so a Feature tagged @e2e @e2e_auth is filtered by the existing addopts and selected by the existing -m <tier>. Feature files stay in tests/features/; the step module lives in tests/e2e/ so it inherits that suite's profile-gating fixtures. tests/features/test_e2e_binding_guard.py enforces it offline, in hosted CI, without a browser — orphan @e2e Gherkin, a missing cost tier, and the inverse hazard of a feature bound from tests/e2e/ but left untagged, which escapes addopts and makes hosted CI try to drive Chrome. It carries its own fire-test, so green means "no orphans" rather than "never looked". Proving these tests PASS stays the nightly canary's job, on a machine with a warm profile.
…ector (#775) * fix(transports): name a known Flow landing instead of blaming the selector 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. * fix(review): apply the branch-council findings 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. * fix(transports): recognise Google's auth host as a known landing 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. * docs(changelog): fold the duplicate Added heading the rebase created 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.
…777) * fix(security): strip OAuth query params from chooser error messages Found while live-verifying v0.73.0's release gate, in code this release did not touch. `client._handle_account_chooser` interpolates `page.url` verbatim at three raise sites, and Google's auth URLs carry `state`, `code_challenge`, `client_id` and challenge tokens. That text is the artifact users are asked to paste into a GitHub issue. Measured on the real path, not theorised. `gflow image t2i --profile <name>` against a profile Google had put behind a password challenge: BEFORE exit 38, 5 secret matches - accounts.google.com/v3/signin/challenge/pwd?TL=ACv9tzFkh8ZJ... plus the OAuth state and client_id AFTER exit 38, 0 secret matches, same landing still named `safe_page_url()` joins `flow_host_kind` / `flow_landing_kind` in `_common.py` and keeps scheme+host+path. All FOUR raise sites route through it - the three in client.py and `raise_if_known_landing`, which had been stripping inline. One helper rather than four copies: the council flagged this class of leak in the new code (D3) and it applied identically to the raise site next door, which is the tell that it belonged in a shared function. The landing stays in the message. Knowing WHERE the session stopped is the whole value of the diagnosis; only the credentials are gone. Regression test asserts both halves - the URL is still named AND none of client_id / code_challenge / state= / TL= survive - so a future edit that re-inlines the URL goes red instead of quietly leaking again. Verified: ruff, format, pyright 0 errors; 8 chooser tests, 86 in the affected modules; the live A/B above. * test: cover the new lines SonarCloud flagged (new_coverage 70% < 80%) Zero issues; the gate failed purely on coverage of lines this PR added or changed. Two real gaps, both legitimate: - `safe_page_url` edge branches. I had verified None / "" / non-URL / "https://[bad" by running them in a terminal and reading the output. That is evidence for one person once; it is not a test, so nothing re-checks it and the lines were exactly as untested as the gate said. Every branch is pinned now. - `client.py`'s "no account is recorded in this profile" raise had NO test at all — every chooser test writes an ACCOUNT_FILE first, which is how the line stayed uncovered while the two raise sites beside it were exercised. It is a real path: a profile authenticated before .gflow_account existed, or one whose file was removed, has nothing to auto-select with. No coverage exclusions widened, no assertions weakened — the skill forbids both, and either would turn the gate green while leaving the lines untested.
* spike(776): measure whether the migrated settings trigger is ever visible-but-unclickable
0/3 on the overlay hypothesis — body{pointer-events:none} never occurred and the
click landed in 65-130ms every run, so this settles nothing about #776 and is
recorded as unmeasured, not as transience.
Three things it did measure:
- _dismiss_dialog runs before Angular mounts the composer, 3/3 (trigger appears
1479-3143ms after domcontentloaded, the check fires at ~0ms). The miss is
structural, not flaky.
- a healthy click on this control costs 65-130ms against a 5000ms budget, so
#776's expiry is a control that never became actionable.
- the locale-settle error the reporter asked about reproduced on all 3 runs while
the click still landed - it is #643, and it is not sufficient to cause #776.
Rung 1 also found #593's pointer-events measurement was taken on labs.google;
migrated_composer.py:744 calling the migrated dialog '#593's twin' is asserted,
not measured.
Refs #776
* fix(migrated): say what was true when a click never lands (#776)
video r2v on flow.google.com reached migrated.editor_ready and died 5.039s later
as a bare Playwright TimeoutError - exit 1, no locator, no cause, no MP4. By
elimination that is _open_pane's trigger.click(timeout=5000): the wait_for(visible)
one line above IS guarded and would have raised exit 23, so the control was visible
and the click expired.
#752 finding #7 predicted this at this exact function before #776 was filed. Its
count()->visibility half was fixed; 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, measured on labs.google, never on this host -
0/3 in today's spike) and a mid-run agent-mode flip. A guard built on either would
answer confidently and be wrong half the time (#770 is the precedent). So on a
timeout the driver reads Playwright's four actionability conditions back and reports
the ones that fired; when every reading is healthy it says so, which eliminates
three 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
- MCP gains more than the CLI: a non-GFlowError on the queued path shipped
'detail: sha256:...'; the typed error routes it to the Problem Details branch
- four sites with a named reason each, not all nineteen
- the occluder report is a closed allowlist (tag + <=3 framework class tokens);
typing the error moves the text from hashed telemetry to a message printed raw,
and a signed-in Flow page carries the account email on exactly those elements
- retryable unchanged: preserved, not measured
Closes #776
* test+docs(776): cover every reading offline, and fix the docstring the council caught
Council D9 found a Blocker I had put on my own predict list and then dropped:
UiSelectorDriftError's docstring still said 'finds no matching element', which the
new raise site makes false - it now also fires when the selector DOES match and the
element will not take the interaction.
- errors.py: the class covers two shapes, and the detail has to say which
- KNOWN_ISSUES: the lookup table documented 6 of the 8 messages the code can emit;
the missing two were 'not rendered' and 'answers no hit test'
- E2E_TESTING: a second worked example, chosen because it fails DIFFERENTLY from the
first - a client-side redirect vs Playwright's actionability gate
- tests: one offline case per reading. CI's coverage run excludes -m e2e, so a branch
proven only in the browser reads as dead code to Sonar's new-code gate - the exact
way PR #777 went red at 70%
- tests/worker: the MCP twin A/B - a bare TimeoutError reaches an agent as
'detail: sha256:...', the typed one as problem details with exit 23
Refs #776
* refactor(776): cut the prose the council measured, and merge one fact told twice
Council D14 measured 64.5% of this change's added lines as non-code prose, against
the 35% issue #759 already named as a problem in this same file. Fair hit - three of
the passages were the CHANGELOG's content retold at the call site, and one was an
exact duplicate of the comment 80 lines above it.
- _click's docstring: the #593/#752/#770 archaeology is the CHANGELOG's job
- the redaction comment above the redact_sensitive_text return restated the
module-level one verbatim
- the _open_pane call site told the #752 story a third time
- SUBMIT_BUTTON's three-line comment for a one-line constant
D1 also found a real over-report: the JS only hit-tests when the box has a nonzero
width and height, so an unrendered element ALWAYS comes back hit_testable=false and
occluder=null. Two independent ifs then said "it is not rendered" and "it answers no
hit test" about one fact. Now one chain for "can a pointer reach it", ordered most
specific first, with enabled-ness and a page-wide block kept as the separate axes
they are - chaining those would have hidden a disabled control behind whatever
covered it.
Two tests added for the invariants the real JS enforces, which the earlier
parametrized cases violated by holding hit_testable=True on an unrendered element.
Refs #776
* docs(776): correct a truncation claim that was wrong, and flag a fake that no longer works
Council D4 caught a false statement in my own docstring: it said the queued MCP path
raw-slices detail to 500 chars "while the CLI path does not". It does. redact_sensitive_text
IS redact_error_detail, which truncates at the raise site, so every surface sees the same
cap. Locator-first ordering is still right, for the simpler reason that the cut happens at
the source - fixed in both the method docstring and the test that repeated it.
Also:
- tests/worker: _fail_r2v_with enqueues a t2v task. The name described the reporter's
command, not the fixture.
- tests/api/transports/test_migrated_composer.py defines a local PlaywrightTimeoutError
that shadows the real class for that whole 2000-line file. Since #776 the driver
catches the REAL one, so a future click-timeout test written with that file's dominant
fake would silently exercise nothing and say so nowhere. Documented at the definition,
pointing at the file that does it correctly.
Refs #776
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
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
2 participants
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.
Release PR for v0.73.0. The signed tag
v0.73.0is already pushed and the release workflow ran from it; this PR brings the bump commit and the full integration history ontomain.Merge with
--merge, never--squash— the branch was cut fromdevelopand carries the whole batch of unreleased integration commits.What ships
Four error paths stopped lying about what went wrong. None of these change what the tool can do; they change what it says when it cannot, which is the difference between a user filing a useful bug and re-running blind.
TimeoutErrorat exit 1state/code_challenge/client_idbefore reaching an error messageGates
pyright src0 errorscheck_release_artifacts.pygreendocs/LIVE_VERIFICATION_v0.73.0.md— 7 arms: 4 verified live, 1 in a real browser against a synthetic page, 3 recorded as not verified with named reasonsTwo things the doc-review gate caught
A release blocker.
## [0.72.0]still carried content mismerged from PR #764 after the v0.72.0 tag —git show v0.72.0:CHANGELOG.mdproves it never shipped there, and the same feature was about to appear under two different release headings. The section is now byte-exact with the tag and the content sits under[0.73.0]where it belongs. This is thechangelog-backmerge-silent-mismergefailure mode, caught by the gate rather than by a reader.A false claim I introduced. The changelog described both migrated-composer submit sites as "credit-spending". Only the video one is; the image one costs daily quota. Corrected.
Also added from that review: a KNOWN_ISSUES entry for #773 item 3 (an
--accountmismatch raises exit 38 but leaves the wrong-account profile in place — on a billed product, the wrong account paying), and adocs/SECURITY.mdnote recording that a typed error'sdetailis not hashed the way an unhandled one is.