Skip to content

fix: accept optional native login phase transitions - #91

Merged
flowcool merged 3 commits into
playwright-nextfrom
feature/playwright-native-login-85
Nov 1, 2026
Merged

flowcool merged 3 commits into
playwright-nextfrom
feature/playwright-native-login-85

Conversation

@flowcool

@flowcool flowcool commented Nov 1, 2026 •

Copy link
Copy Markdown
Owner

Behavior

Native login currently waits only for the configured next phase after a submission. A valid final session after password submission, or direct OTP entry that skips a device chooser, therefore fails.

Check final authentication and all later phase markers within one timeout after each submission. Final proof wins. Keep the durable transaction reservation, fresh OTP resolution and verification-only resume. No recipe schema, database, control or exit-code change. Owner: infra-zlth.85; recipe follow-up: infra-zlth.52.

Verification

  • Before: isolated Chromium reproduction on dd663c8 fails all four password-to-final/direct-OTP cases, including delayed responses; chooser and existing-session controls pass.
  • Command: PLAYWRIGHT_BROWSERS_PATH=/path/to/chromium python -m pytest tests/test_bc_login.py -q
  • Expected/observed: 71 passed. Includes C3 save/load and real dispatcher execution, optional chooser, fresh OTP, durable reservation, crashes and no replay.
  • Ruff (changed modules and repository F821), Bandit high severity and diff checks pass.
  • Full local pytest: 904 passed, 6 skipped, 104 subtests passed; one calendar case expected timeout but received the generic browser error. Its 1.5-second traversal budget ran during concurrent browser/image work. That unchanged case passes alone (1 passed), and the complete calendar suite passes (33 passed). Required full CI remains a merge gate.
  • Final image sha256:ad0f32e347e9c32966497066600184bd536c8d9eba507c54ca6728a7e4c7ee91, revision 000ffa1f5ed7676fd6d7f4ce1fb78992a5ddb1fc: 159 tests passed, packaged source hashes match, runtime checks and all seven legacy scenarios pass (8 synthetic PDFs). UID/GID 1003:1000, network none, proof container retained.
  • Image command: python tests/qualify_bourse_direct_image.py --image billcollector-login85:000ffa1 --engine "$PWD" --proof-dir /path/to/new-proof --container-name unique-proof-name. Network none; synthetic credentials and temporary DB/profile/output only. The qualifier now runs native login tests and supplies pytest-asyncio metadata/configuration.

Infra handoff

N/A: engine bug fix and recipe-agent handoff only. No release, production access or deployment. Schema and DB are unchanged. Revert the focused commits for code rollback.

Fixtures replace vault/notification transports, browser launch/teardown in dispatcher assertions, display services and consumers. These checks do not qualify a real broker, vault sidecar, recipe selectors, scheduling or Paperless delivery.

AI assistance

Codex assisted implementation, synthetic reproduction, source review and verification. Author checked reservation ownership, monotonic phase progression, final proof and the semantic diff.

Pullfrog review

Require a completed Pullfrog review of the final head and required CI. Final review: #91 (review) at 000ffa1f5ed7676fd6d7f4ce1fb78992a5ddb1fc: no new issues; both earlier suggestions addressed. All threads resolved. Final exact-image qualification passed. Required CI remains the final merge gate.

@coderabbitai

coderabbitai Bot commented Nov 1, 2026 •

Copy link
Copy Markdown

Important

  • Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

Run configuration
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — two minor suggestions inline.

Reviewed changes

I reviewed the native phased-login transition change, its docs and the qualifier update at 4a13b7e. tests/test_bc_login.py passes locally with Chromium (71 passed).

  • Phase transitions in _execute_login: after each submission, next_phase polls the final success locator first, then later intermediate markers from latest to current, all within one timeout_ms deadline. It only moves forward, and a timeout takes the same settle_attempt → login_failed pause path as before.
  • record_proof extraction: a foreign run still raises StepAbortError inside the reserved try, so settlement and no-replay are unchanged.
  • Unchanged guarantees: the reservation is still taken before the first submission, each phase still submits once, {{OTP}} still resolves at submit time, and resume still only verifies. Skipped phases also skip their preparation steps, which is the intended optional-chooser behavior.
  • Docs and changelog: doc/SAFE_LOGIN.md describes the new rule, and CHANGELOG.md has a Fixed entry.
  • Image qualifier: it now runs test_bc_login.py in the image, loads pytest_asyncio.plugin with asyncio_mode=auto, and copies the pytest-asyncio dist-info so the plugin can find its version.
  • Test coverage: the new test covers 4 paths × 2 delays (password → final, password → OTP, password → chooser → OTP, existing session) through save_recipe/load_recipe and the real _run_recipe dispatcher.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using claude-opus-5-5 | 𝕏

Comment thread apps/billcollector/runner.py Resolved
Comment thread doc/SAFE_LOGIN.md Resolved

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found. Both earlier suggestions are addressed.

Reviewed changes

I reviewed commit 000ffa1, which follows up on the review of 4a13b7e.

  • Shared phase markers: _execute_login now builds markers once, before the startup scan. The scan calls markers[index].wait_for(...) instead of building each locator again. Playwright locators are lazy, and markers was already built for every intermediate phase, so behavior does not change. Startup detection and post-submit next_phase now use the same locator for each phase.
  • Proof map: the requirement-to-proof map in doc/SAFE_LOGIN.md now lists test_optional_login_phases_follow_actual_session, including the guarantee that OTP is never filled after final proof.

Pullfrog  | View workflow run | Using claude-opus-5-5 | 𝕏

@flowcool
flowcool merged commit 4a6e981 into playwright-next Nov 1, 2026
12 checks passed
@flowcool
flowcool deleted the feature/playwright-native-login-85 branch November 1, 2026 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant