Repository navigation
chore(workflow): make the Bug Lane the documented route from symptom to fix - #774
Merged
Merged
Conversation
…to fix 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.
|
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 (9)
📝 WalkthroughWalkthroughThe change defines a surface-aware Bug Lane for issue resolution, updates repository guidance, documents BDD-bound E2E tests, and adds an offline guard for missing bindings, cost tiers, and E2E tags. ChangesBug Lane and BDD E2E Workflow
Estimated code review effort: 3 (Moderate) | ~20 minutes ✨ 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 |
5 of 7 tasks
ffroliva
marked this pull request as ready for review
September 10, 2026 13:38
ffroliva
added a commit
that referenced
this pull request
Sep 10, 2026
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
added a commit
that referenced
this pull request
Sep 10, 2026
…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.
Merged
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.
Summary
Formalises the chain a bug travels in this repo — spike → systematic-debugging → BDD → TDD → fix → e2e — canonically in
skills/issue-resolve/SKILL.md, cited (never restated) fromAGENTS.md,skills/spike,skills/scenario,docs/E2E_TESTING.mdanddocs/INDEX.md.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.
Why
Two real defects, both verified before the change:
issue-resolvestep 3 read "the test is the closest browser-free proxy" whileAGENTS.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 — an agent following either was compliant with the other's opposite. (memoryprose-conflicts-hide-in-disjoint-files).featurefiles, all offline-mocked; zero e2e tests used pytest-bdd;grep -i bdd docs/E2E_TESTING.mdreturned nothing.How BDD binds to e2e — no new machinery
pytest-bdd already converts Gherkin tags into pytest markers (verified on 8.1,
plugin.py:137), and feature-level tags propagate to every scenario (also measured, not assumed). So a Feature tagged@e2e @e2e_authis filtered by the existingaddoptsand selected by the existing-m <tier>.tests/features/; the step module lives intests/e2e/so it inherits that suite's profile-gating fixtures.tests/features/test_e2e_binding_guard.pyenforces it offline, in hosted CI, without a browser: orphan@e2eGherkin, a missing cost tier, an untagged live binding (which escapesaddoptsand makes CI drive Chrome), and double binding.The CI split
Hosted CI proves the e2e test exists and is wired (static text). Proving it passes stays the nightly canary's job on a machine with a warm profile — hosted CI cannot run the live tiers and never could.
Test plan
4172 passed, 92% coverage (full offline suite at this commit)ruff check/ruff format --checkonsrc testscheck_repo_hygiene,check_doc_links,generate_website_docs --check,check_website_docs_pii,check_council_memory— all greenNotes for review
Summary by CodeRabbit
Documentation
Tests