fix(e2e): harden reset_progress journey — Eval + 5s budget (bookshelf-ydjo9) #1342

Merged

zombor merged 1 commit from bd-bookshelf-ydjo9 into main 2026-08-05 12:49:13 +00:00

Conversation 2 Commits 1 Files changed 1 +25 -8

zombor commented 2026-08-05 12:30:10 +00:00

Owner

Copy link

Root cause

journey_reset_progress_test.go had two Eventually blocks (the kebab menu-open check, lines 99–104 and 115–120) that used page.MustEval() as the polling function with a 3-second budget.

Two independent problems:

1. MustEval inside Eventually: MustEval panics on a transient CDP error (e.g. context deadline exceeded under CI runner starvation). That panic propagates OUT of the Eventually callback instead of being caught and retried — the spec fails with a panic rather than a timeout assertion failure.

2. 3s budget too tight: The refresh-cover sibling uses 5s for the identical menu-open check. Under runner starvation, 3s is not enough margin for the Chromium click+Stimulus DOM update round-trip.

The file was skipped by the #1331 eu18m sweep, which targeted page.Element() calls INSIDE Eventually bodies. This file had page.MustEval() inside Eventually — the same class of panic-on-error problem but a different call site.

Fix

• Convert all three MustEval-inside-Eventually calls to the non-Must page.Eval variant (returning false on error so the loop retries), matching the pattern already used by the "confirming the dialog" step in the same file.
• Raise the two 3s budgets to 5s, matching the refresh-cover sibling pattern.

Test plan

• go build -tags e2e ./e2e/browser/... — compiles clean
• make e2e-policy-check — passes (all Describes are Ordered journey containers)
• Browser e2e cannot be run locally (no Docker+Chromium); relying on CI for the behavioral gate.

Closes bead bookshelf-ydjo9 on merge.

## Root cause `journey_reset_progress_test.go` had two `Eventually` blocks (the kebab menu-open check, lines 99–104 and 115–120) that used `page.MustEval()` as the polling function with a **3-second** budget. Two independent problems: 1. **`MustEval` inside `Eventually`**: `MustEval` panics on a transient CDP error (e.g. `context deadline exceeded` under CI runner starvation). That panic propagates OUT of the `Eventually` callback instead of being caught and retried — the spec fails with a panic rather than a timeout assertion failure. 2. **3s budget too tight**: The refresh-cover sibling uses 5s for the identical menu-open check. Under runner starvation, 3s is not enough margin for the Chromium click+Stimulus DOM update round-trip. The file was skipped by the #1331 eu18m sweep, which targeted `page.Element()` calls INSIDE `Eventually` bodies. This file had `page.MustEval()` inside `Eventually` — the same class of panic-on-error problem but a different call site. ## Fix - Convert all three `MustEval`-inside-`Eventually` calls to the non-Must `page.Eval` variant (returning `false` on error so the loop retries), matching the pattern already used by the "confirming the dialog" step in the same file. - Raise the two 3s budgets to 5s, matching the refresh-cover sibling pattern. ## Test plan - `go build -tags e2e ./e2e/browser/...` — compiles clean - `make e2e-policy-check` — passes (all Describes are Ordered journey containers) - Browser e2e cannot be run locally (no Docker+Chromium); relying on CI for the behavioral gate. Closes bead bookshelf-ydjo9 on merge.

zombor added 1 commit 2026-08-05 12:30:11 +00:00

fix(e2e): harden reset_progress journey — Eval + 5s budget (bookshelf-ydjo9) ... d604fdc9c9

Root cause: two Eventually blocks (menu-open check on lines 99–104 and 115–120)
used page.MustEval() as the polling function with a 3-second budget. MustEval
panics on a transient CDP error (context deadline exceeded under runner starvation);
the panic propagates OUT of the Eventually callback rather than being retried.
The 3s budget was also below the 5s used by the adjacent refresh-cover sibling
for the same menu-open check.

Fix: convert all three MustEval-inside-Eventually calls to the non-Must page.Eval
variant (matching the already-correct "confirming the dialog" step), returning
false on error so the Eventually loop retries. Raise the two 3s budgets to 5s,
matching the refresh-cover sibling pattern.

Cannot run browser e2e locally (no Docker+Chromium in this environment); relying
on CI for the behavioral gate.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor commented 2026-08-05 12:46:44 +00:00

Author

Owner

Copy link

Code Review — bookshelf-ydjo9 / PR #1342

CI status: success (verified via /commits/{sha}/status rollup)
Mergeable: True

---

Phase 1 — Spec Compliance

Bead description: convert MustEval-inside-Eventually to non-Must page.Eval + error-check on 3 occurrences; raise two 3s budgets to 5s. The diff delivers exactly this — 3 conversions, two budget bumps — and nothing extra. Spec is fully satisfied.

---

Phase 2 — Code Quality

MustEval → Eval conversion correctness

All 3 converted blocks follow the same pattern:

result, err := page.Eval(`...`)
if err != nil {
    return false
}
return result.Value.Bool()

• On a transient CDP error page.Eval returns a non-nil err; the lambda returns false and Eventually retries — correct.
• On genuine success result.Value.Bool() propagates the JS return value — correct.
• Eventually(...).Should(BeTrue()) still fails loudly with a Gomega timeout message if the budget is exhausted without the condition becoming true — the assertion is not weakened.
• No error is swallowed: the err path converts to a retry signal, not silence; Eventually's own deadline is the outer bound that surfaces a real failure.

Budget bumps (3s → 5s)

Two menus-open Eventually blocks go 3s → 5s. The third (.apd-overlay visibility) was already 5s on origin/main and is unchanged. The bumps are explicitly justified by the comment citing the refresh-cover sibling (which uses 5s for the identical DOM check). These are secondary to the panic-elimination fix and do not mask a hang: the real fix is the non-panic retry path.

Consistency with eu18m non-blocking pattern

The converted pattern is identical to the approach used in the broader E2E browser sweep. The sibling journey_refresh_cover_test.go still uses MustEval inside Eventually (a pre-existing gap), but that is out of scope for this bead and is a pre-existing condition on origin/main.

Remaining MustEval-inside-Eventually in this file

There are none remaining in journey_reset_progress_test.go after this PR — all 3 occurrences are converted.

Black-box package

File declares package browser_test — correct.

No product code touched — confirmed, single test file changed.

No golangci exclusions added — confirmed.

---

No findings.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

**Code Review — bookshelf-ydjo9 / PR #1342** **CI status:** `success` (verified via `/commits/{sha}/status` rollup) **Mergeable:** `True` --- ### Phase 1 — Spec Compliance Bead description: convert `MustEval`-inside-`Eventually` to non-Must `page.Eval` + error-check on 3 occurrences; raise two 3s budgets to 5s. The diff delivers exactly this — 3 conversions, two budget bumps — and nothing extra. Spec is fully satisfied. --- ### Phase 2 — Code Quality **MustEval → Eval conversion correctness** All 3 converted blocks follow the same pattern: ```go result, err := page.Eval(`...`) if err != nil { return false } return result.Value.Bool() ``` - On a transient CDP error `page.Eval` returns a non-nil `err`; the lambda returns `false` and `Eventually` retries — correct. - On genuine success `result.Value.Bool()` propagates the JS return value — correct. - `Eventually(...).Should(BeTrue())` still fails loudly with a Gomega timeout message if the budget is exhausted without the condition becoming true — the assertion is not weakened. - No error is swallowed: the `err` path converts to a retry signal, not silence; `Eventually`'s own deadline is the outer bound that surfaces a real failure. **Budget bumps (3s → 5s)** Two menus-open Eventually blocks go 3s → 5s. The third (`.apd-overlay` visibility) was already 5s on `origin/main` and is unchanged. The bumps are explicitly justified by the comment citing the refresh-cover sibling (which uses 5s for the identical DOM check). These are secondary to the panic-elimination fix and do not mask a hang: the real fix is the non-panic retry path. **Consistency with eu18m non-blocking pattern** The converted pattern is identical to the approach used in the broader E2E browser sweep. The sibling `journey_refresh_cover_test.go` still uses `MustEval` inside `Eventually` (a pre-existing gap), but that is out of scope for this bead and is a pre-existing condition on `origin/main`. **Remaining MustEval-inside-Eventually in this file** There are none remaining in `journey_reset_progress_test.go` after this PR — all 3 occurrences are converted. **Black-box package** File declares `package browser_test` — correct. **No product code touched** — confirmed, single test file changed. **No golangci exclusions added** — confirmed. --- No findings. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-08-05 12:46:47 +00:00

Author

Owner

Copy link

Security Review — PR #1342 (bookshelf-ydjo9)

Scope: single file, e2e/browser/journey_reset_progress_test.go. No production code is touched.

Findings

(1) Production-code impact

Zero. The diff is entirely confined to the e2e browser test file. No handler, service, SQL query, middleware, or template is modified. No new routes are registered and no authorization or business logic changes.

(2) MustEval → Eval error-swallowing and security-relevant assertion coverage

The change converts three page.MustEval(...).Bool() calls inside Eventually loops to page.Eval(...) with if err != nil { return false }, and extends the corresponding poll budgets from 3 s to 5 s.

What the swallowed error represents: a CDP (Chrome DevTools Protocol) transport or protocol error — e.g. the tab crashed, the CDP session timed out, or the JS evaluation itself threw. On MustEval, any such error immediately panics the spec. On Eval with return false, the Eventually loop retries for up to 5 s before failing the spec normally. This is the standard flake-fix pattern for go-rod under CI runner starvation.

Security-relevant assertions and whether they are weakened:

The three converted Eval calls check:

• menu.classList.contains("is-open") — asserts the kebab menu opened after a click.
• !overlay.hasAttribute("hidden") — asserts the AppDialog confirm modal appeared.
• document.body.innerText.includes("Reading progress has been reset") — asserts the post-reset flash message is visible.

None of these are security assertions in themselves. The security-relevant property — that the DELETE request resets only the current user's progress and sessions — is established by the fixture setup in BeforeAll (rows seeded for user_id = 1) and is implicitly verified when the journey succeeds end-to-end: the test authenticates as that user via setAuthCookies, the Stimulus controller fires a real fetch DELETE, and the server processes it under the stub user's session. The flash message assertion proves the server returned a success response that the controller acted on.

The error-swallow path (return false) causes Eventually to keep polling until the budget is exhausted, then fail the It with a clear Gomega timeout error. It cannot produce a false-positive pass: Eventually(...).Should(BeTrue()) requires the function to return true, not merely to stop returning errors. A CDP error silently looping then timing out is a worse flake outcome than MustEval panicking immediately, not a security bypass.

There is no DB-level assertion after the reset confirming that only user_id = 1 rows were deleted and no cross-user rows were touched. However, that gap is pre-existing on main and is not introduced or worsened by this PR. The PR does not remove any existing assertion; it only changes the error-handling discipline on pre-existing DOM-state polling inside Eventually.

(3) Secrets / PII in fixtures

Fixtures insert user_id = 1 (the stub user), a synthetic book title ("Reset Progress Test Book"), epub_progress_percent = 0.42, and a reading session with no personal data fields. No tokens, passwords, real email addresses, or PII are present.

Summary

All three review questions are nil risk. The change is a mechanical flake fix (MustEval → Eval + 3 s → 5 s poll budget) that does not alter, weaken, or remove any security assertion. The no-cross-user-DB-assertion gap is pre-existing and out of scope for this PR.

---

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — PR #1342 (bookshelf-ydjo9) **Scope:** single file, `e2e/browser/journey_reset_progress_test.go`. No production code is touched. ### Findings #### (1) Production-code impact Zero. The diff is entirely confined to the e2e browser test file. No handler, service, SQL query, middleware, or template is modified. No new routes are registered and no authorization or business logic changes. #### (2) MustEval → Eval error-swallowing and security-relevant assertion coverage The change converts three `page.MustEval(...).Bool()` calls inside `Eventually` loops to `page.Eval(...)` with `if err != nil { return false }`, and extends the corresponding poll budgets from 3 s to 5 s. **What the swallowed error represents:** a CDP (Chrome DevTools Protocol) transport or protocol error — e.g. the tab crashed, the CDP session timed out, or the JS evaluation itself threw. On `MustEval`, any such error immediately panics the spec. On `Eval` with `return false`, the `Eventually` loop retries for up to 5 s before failing the spec normally. This is the standard flake-fix pattern for go-rod under CI runner starvation. **Security-relevant assertions and whether they are weakened:** The three converted `Eval` calls check: - `menu.classList.contains("is-open")` — asserts the kebab menu opened after a click. - `!overlay.hasAttribute("hidden")` — asserts the AppDialog confirm modal appeared. - `document.body.innerText.includes("Reading progress has been reset")` — asserts the post-reset flash message is visible. None of these are security assertions in themselves. The security-relevant property — that the DELETE request resets *only the current user's* progress and sessions — is established by the fixture setup in `BeforeAll` (rows seeded for `user_id = 1`) and is implicitly verified when the journey succeeds end-to-end: the test authenticates as that user via `setAuthCookies`, the Stimulus controller fires a real fetch DELETE, and the server processes it under the stub user's session. The flash message assertion proves the server returned a success response that the controller acted on. The error-swallow path (`return false`) causes `Eventually` to keep polling until the budget is exhausted, then fail the `It` with a clear Gomega timeout error. It cannot produce a false-positive pass: `Eventually(...).Should(BeTrue())` requires the function to return `true`, not merely to stop returning errors. A CDP error silently looping then timing out is a worse flake outcome than `MustEval` panicking immediately, not a security bypass. There is no DB-level assertion after the reset confirming that only `user_id = 1` rows were deleted and no cross-user rows were touched. However, that gap is pre-existing on main and is not introduced or worsened by this PR. The PR does not remove any existing assertion; it only changes the error-handling discipline on pre-existing DOM-state polling inside `Eventually`. #### (3) Secrets / PII in fixtures Fixtures insert `user_id = 1` (the stub user), a synthetic book title ("Reset Progress Test Book"), `epub_progress_percent = 0.42`, and a reading session with no personal data fields. No tokens, passwords, real email addresses, or PII are present. ### Summary All three review questions are nil risk. The change is a mechanical flake fix (MustEval → Eval + 3 s → 5 s poll budget) that does not alter, weaken, or remove any security assertion. The no-cross-user-DB-assertion gap is pre-existing and out of scope for this PR. --- REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor merged commit cb92beceea into main 2026-08-05 12:49:13 +00:00

zombor referenced this pull request from a commit 2026-08-05 12:49:17 +00:00

Merge pull request 'fix(e2e): harden reset_progress journey — Eval + 5s budget (bookshelf-ydjo9)' (#1342) from bd-bookshelf-ydjo9 into main

zombor referenced this pull request from a commit 2026-08-06 00:20:42 +00:00

fix(e2e): sweep MustEval-inside-Eventually to Eval+error-check (bookshelf-b96py)

zombor referenced this pull request 2026-08-06 00:20:49 +00:00

fix(e2e): sweep MustEval-in-Eventually to Eval+error-check (bookshelf-b96py) #1349

zombor referenced this pull request 2026-08-06 00:42:15 +00:00

fix(e2e): sweep MustEval-in-Eventually to Eval+error-check (bookshelf-b96py) #1349

zombor referenced this pull request from a commit 2026-08-06 00:48:49 +00:00

fix(e2e): sweep MustEval-inside-Eventually to Eval+error-check (bookshelf-b96py)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!1342

No description provided.