From 10bd1f69d33dbc23e54bf7a1fdcebf589b335225 Mon Sep 17 00:00:00 2001 From: Frank Steiler Date: Wed, 8 Jul 2026 03:24:56 +0200 Subject: [PATCH] =?UTF-8?q?fix(e2e):=20stabilize=20shard-3=20diary=20flake?= =?UTF-8?q?s=20=E2=80=94=20timeout=20budgeting,=20mock-response=20race,=20?= =?UTF-8?q?cleanup=20guarantees?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root-causes the recurring shard-3 (16-shard desktop matrix) diary E2E failures on main-targeted PRs: - diary-drafts.spec.ts Scenario 14: test.slow() only triples the OVERALL per-test timeout (15s->45s), not actionTimeout/expect.timeout. The test paired that 45s ceiling with four 30-45s per-step timeouts, any one of which could alone exhaust the budget under 8-worker CI load — starving the finally-block cleanup and surfacing as an "apiRequestContext.delete: Test timeout exceeded" (a symptom, not the cause). Fixed with an explicit test.setTimeout(60s), proportionate 15s step ceilings, and a guaranteed-cleanup-time pattern (testInfo.setTimeout(testInfo.timeout + 15s) + try/catch in finally) so teardown always gets its own budget regardless of how much of the test body's time was consumed. - diary-r2-uat.spec.ts Scenario 10: a route mock that always returns an empty page defeated waitForLoaded()'s visibility race, letting a stale in-flight response from the prior "All" click satisfy the next waitForResponse() predicate before the real "Manual" request/response pair occurred, intermittently leaving a shared requests[] capture array empty. Fixed by explicitly awaiting each transition's own response and reading the request straight off the resolved Response object. - document-linking.spec.ts Scenario 7a: was collateral cancellation from Scenario 14's maxFailures:1 trip, not an independent bug. Added the same defensive guaranteed-cleanup-time pattern as cheap insurance. - playwright.config.ts: comment-only documentation, verified against the Playwright 1.61.1 dispatcher source, that maxFailures already tolerates one retry-passing (flaky) test before fail-fast cancels a shard — no config change needed. - cornerstoneContainer.ts: incidental fix for a testcontainers 12.0.4 API rename (withCopyFileToContainer -> withCopyFilesToContainer) discovered while attempting local containerized verification. Fixes #1829 Co-Authored-By: Claude dev-team-lead (Sonnet 4.6) Co-Authored-By: Claude e2e-test-engineer (Sonnet 4.5) --- .../agent-memory/e2e-test-engineer/MEMORY.md | 3 +- .../bug-1829-shard3-flakes.md | 125 ++++++++++++++++++ .../e2e-test-engineer/general-e2e-patterns.md | 11 +- .../known-flakes-and-regressions.md | 5 +- .../agent-memory/security-engineer/MEMORY.md | 7 +- .../security-engineer/pr-review-table.md | 1 + e2e/containers/cornerstoneContainer.ts | 2 +- e2e/playwright.config.ts | 11 +- e2e/tests/diary/diary-drafts.spec.ts | 62 ++++++--- e2e/tests/diary/diary-r2-uat.spec.ts | 31 +++-- e2e/tests/documents/document-linking.spec.ts | 14 +- 11 files changed, 229 insertions(+), 43 deletions(-) create mode 100644 .claude/agent-memory/e2e-test-engineer/bug-1829-shard3-flakes.md diff --git a/.claude/agent-memory/e2e-test-engineer/MEMORY.md b/.claude/agent-memory/e2e-test-engineer/MEMORY.md index 37749d52f..d6dce18f9 100644 --- a/.claude/agent-memory/e2e-test-engineer/MEMORY.md +++ b/.claude/agent-memory/e2e-test-engineer/MEMORY.md @@ -19,8 +19,9 @@ - [story-933-dav-vendor-contacts.md](story-933-dav-vendor-contacts.md) — vendor contacts / CardDAV story notes. - [story-1248-mass-move.md](story-1248-mass-move.md) — mass-move budget line story notes. - [story-epic08-e2e.md](story-epic08-e2e.md) — EPIC-08 Paperless integration (no testcontainer yet — all document tests validate "not configured" state only). +- [bug-1829-shard3-flakes.md](bug-1829-shard3-flakes.md) — root cause + fix for the shard-3 diary flakes blocking main promotions; also proves `maxFailures` already tolerates one retry-passing test (Playwright source citation). ## Open follow-ups to flag to orchestrator -- Diary shard-3 E2E Gates flakiness recurred with NEW failure signatures as of 2026-07-07 despite fix PRs #1790/#1792/#1793 — see known-flakes-and-regressions.md. Needs dedicated investigation before more diary work. - No Paperless-ngx testcontainer exists yet (story-epic08-e2e.md) — all Paperless E2E coverage is `page.route()` mocked, not real integration. Add the container when Paperless work resumes. +- Full containerized E2E verification is not possible in sandboxes without `dhi.io` (Docker Hardened Images) registry credentials — building `cornerstone:e2e` fails with `401 Unauthorized`. This is an environment limitation, not a code issue; verification in such sandboxes must fall back to static checks (lint/prettier/tsc-diff/`playwright --list`) plus post-merge CI observation. diff --git a/.claude/agent-memory/e2e-test-engineer/bug-1829-shard3-flakes.md b/.claude/agent-memory/e2e-test-engineer/bug-1829-shard3-flakes.md new file mode 100644 index 000000000..0e927af14 --- /dev/null +++ b/.claude/agent-memory/e2e-test-engineer/bug-1829-shard3-flakes.md @@ -0,0 +1,125 @@ +--- +name: bug-1829-shard3-flakes +description: Root cause + fix for the recurring E2E shard-3 diary flakes blocking main promotions (issue #1829) — confirms maxFailures/retry accounting semantics. +metadata: + type: project +--- + +## Summary + +Issue #1829 reported 3 "failing" tests on shard 3/16 desktop on main-targeted PRs +(#1791, #1828). Investigation (job 85332065279 logs, `gh api repos/.../actions/jobs//logs`) +showed only **one** was a real, independent bug — the other two were consequences of +`maxFailures: 1` fail-fast, not independent defects: + +1. **`e2e/tests/diary/diary-drafts.spec.ts:854`** (Scenario 14) — REAL bug. Failed on + BOTH attempt 1 and retry #1 (48s each) → genuinely exhausted retries → correctly + tripped `maxFailures: 1`. +2. **`e2e/tests/documents/document-linking.spec.ts:369`** (Scenario 7a) — NOT a real + bug. Log shows it ran only 1.6s before being marked `✘` — it was cancelled + mid-flight as collateral damage the instant #1 tripped fail-fast, not a genuine + assertion/timeout failure of its own. +3. **`e2e/tests/diary/diary-r2-uat.spec.ts:599`** (Scenario 10) — REAL secondary bug + (a genuine race), but reported as "flaky" (failed attempt 1, passed retry) — this + did NOT trip `maxFailures` (see accounting proof below), so it did not cause the + shard cancellation; it's independently worth fixing but wasn't the trigger. + +## Root cause 1: diary-drafts.spec.ts:854 (the actual trigger) + +`test.slow()` triples the desktop project's **overall per-test timeout** (15s → 45s). +This is a single shared ceiling across every step in the test PLUS the `finally`-block +cleanup — it is NOT "3x budget per step," and it does NOT triple `actionTimeout` or +`expect.timeout` (a misconception baked into the original code's comments). The test +paired `test.slow()` (45s total) with FOUR separate explicit step timeouts of +30_000-45_000ms each (`waitForResponse`, `waitForURL`, two `expect(...).toBeVisible()` +calls) — any single one of which could alone consume the entire budget. Under heavy +8-worker CI load the cumulative real elapsed time across these steps regularly ate the +whole 45s, leaving nothing for the `finally` block's `deleteDiaryEntryViaApi()` call — +which is what actually threw ("apiRequestContext.delete: Test timeout of 45000ms +exceeded"), masking the true cause (slow steps, not a slow delete). + +**Fix** (commit on `fix/1829-e2e-shard3-flakes`): replaced `test.slow()` with +`test.setTimeout(60_000)` (explicit absolute budget, more generous and self-documenting +than a 3x multiplier), reduced the 4 oversized step timeouts to 15_000ms each +(proportionate "something is actually stuck" ceilings, not budget reservations), and — +the generalizable pattern — added `testInfo.setTimeout(testInfo.timeout + 15_000)` as +the FIRST line of the `finally` block before cleanup. This call is **additive to the +current remaining deadline**, not a reset to an absolute value, so it always grants +15s of guaranteed headroom for cleanup regardless of how much of the original budget +the test body already burned. Wrapped the delete in `try/catch` (best-effort cleanup; +an orphaned draft in the ephemeral CI DB is harmless, a masked test outcome is not). +Applied the same defensive pattern to `document-linking.spec.ts:369`'s two-item +cleanup even though it wasn't the root cause, since it's cheap insurance and directly +named in the issue. + +## Root cause 2: diary-r2-uat.spec.ts:599 (secondary, real race) + +Both this test and `Scenario 9` mock `**/api/diary-entries*` with an ALWAYS-EMPTY +paginated response (`makePaginatedEmpty()`). `DiaryPage.waitForLoaded()` races +`timeline`/`emptyState`/`errorBanner` visibility — but since the empty state is +already visible from a *prior* transition and never changes, `.waitFor({state: +'visible'})` on an already-visible element resolves **instantly**, without +synchronizing to the new request/response at all. Scenario 10 clicks "All" then +"Manual" (two transitions before the assertion); after the "All" click, +`waitForLoaded()` is a no-op, so the test proceeds to reset a `requests[]` capture +array and register a fresh `waitForResponse` for the "Manual" click's response — +but the "All" click's own response can still be in flight and arrive AFTER the +reset, matching the loose `waitForResponse` predicate (any 200 response containing +`/api/diary-entries`) and resolving it *before* the real "Manual" request/response +pair occurs. Result: `requests[]` is empty when read → `expect(lastRequest).toBeDefined()` +fails intermittently. Scenario 9 does NOT have this race because its `requests[]` +reset happens right after the very first (`goto`) transition, whose response is +provably not-yet-arrived when `waitForLoaded()` resolves (nothing was visible before +first navigation). + +**Fix**: explicitly `await page.waitForResponse(pred)` for the "All" transition's own +response (registered before the click) before resetting/registering for "Manual", and +derive `lastRequest`/`typeParam` directly from the resolved `Response` object's URL +instead of a side-channel mutable array — eliminates the race entirely rather than +narrowing the window. + +## `maxFailures` / retry accounting — proof it already tolerates one flaky test + +Verified against installed `testcontainers`... no wait, against `@playwright/test@1.61.1` +source (`packages/playwright/src/runner/dispatcher.ts`, `_reportTestEnd`): + +```ts +// Test is considered failing after the last retry. +if (test.outcome() === 'unexpected' && test.results.length > test.retries) + ++this._testRun.failedTestCount; +``` + +`maxFailures` only increments once a test has **exhausted all configured retries** +AND is still `'unexpected'`. A test that fails attempt 1 but passes on retry +("flaky") never counts. **AC #3 of issue #1829 ("should fail-fast tolerate one +retry-passing test before cancelling?") is already satisfied by Playwright's design — +no config change was needed.** `maxFailures: 1` in `e2e/playwright.config.ts` was left +as-is; only the comment was expanded to document this (with a source citation) so a +future engineer doesn't re-litigate it. Root-causing #1 above is the real fix — once +that test reliably passes on attempt 1 (or at worst passes on retry), fail-fast won't +trip and the other two tests stop showing collateral/independent failures. + +## Playwright config values (verified 2026-07-08, corrects a stale prior note) + +- Global `use`: `actionTimeout: 5_000`, `navigationTimeout: 10_000`. +- Desktop: test `timeout: 15_000` (top-level default), `expect: {timeout: 7_000}`. +- Tablet/Mobile: test `timeout: 30_000`, `expect: {timeout: 10_000}`, action/nav `10_000`. +- `retries: 1` on CI, `maxFailures: 1` only when `E2E_FAIL_FAST=1` (main-targeted PRs). + +## Incidental fix: testcontainers API drift (e2e/containers/cornerstoneContainer.ts) + +While attempting a real containerized verification run of this fix, discovered +`cornerstoneContainer.ts`'s `HTTP_PROXY`/`HTTPS_PROXY` branch called +`.withCopyFileToContainer(caPath, target)` (singular) — this method does not exist in +the installed `testcontainers@12.0.4` (renamed to `.withCopyFilesToContainer([{source, +target}])`, plural, array-based, confirmed by reading `node_modules/testcontainers/build/generic-container/generic-container.d.ts` +and a `node -e` runtime probe). This branch only executes when `HTTP_PROXY`/`HTTPS_PROXY` +env vars are set, which CI never sets — so the bug was dormant in CI and only surfaces +in a local sandbox that sits behind an HTTP(S) proxy requiring a trusted CA (like this +one). Fixed as a one-line incidental change. Full containerized verification still +wasn't possible in this sandbox after that fix: building the `cornerstone:e2e` image +requires `dhi.io` (Docker Hardened Images) registry credentials not available here +(`401 Unauthorized` pulling `dhi.io/node:24-alpine3.23`) — this is a credentials/ +environment limitation, not a code issue. **If you hit `withCopyFileToContainer is not +a function` in a future session, this is already fixed** — don't re-diagnose, just +check whether a newer `testcontainers` bump reintroduced /renamed the API again. diff --git a/.claude/agent-memory/e2e-test-engineer/general-e2e-patterns.md b/.claude/agent-memory/e2e-test-engineer/general-e2e-patterns.md index ec5bd1f9c..8308011e5 100644 --- a/.claude/agent-memory/e2e-test-engineer/general-e2e-patterns.md +++ b/.claude/agent-memory/e2e-test-engineer/general-e2e-patterns.md @@ -21,9 +21,16 @@ metadata: ## Viewport timeouts (Playwright projects) -- Desktop: `timeout: 10_000`, default action/expect timeouts (Playwright default 30s / no override). -- Tablet & Mobile: `timeout: 60_000`, `expect/action/navigationTimeout: 15_000`. +Verified against `e2e/playwright.config.ts` 2026-07-08 (previous note here was stale/wrong — corrected): + +- Global `use`: `actionTimeout: 5_000`, `navigationTimeout: 10_000`. +- Desktop: test `timeout: 15_000` (top-level default, no per-project override), `expect: {timeout: 7_000}`. +- Tablet & Mobile: test `timeout: 30_000` (2x desktop), `expect: {timeout: 10_000}`, `actionTimeout: 10_000`, `navigationTimeout: 10_000`. +- `maxFailures: 1` only on main-targeted PRs (`E2E_FAIL_FAST=1`); `retries: 1` on CI. See [bug-1829-shard3-flakes.md](bug-1829-shard3-flakes.md) for confirmed `maxFailures`/retry accounting semantics. - Never hardcode a lower `{timeout:N}` in a POM `waitFor()` call — it silently overrides the project-level tablet/mobile timeout. +- **`test.slow()` triples the OVERALL per-test timeout only** (desktop 15s→45s) — it does NOT triple `actionTimeout`/`expect.timeout`. That total is a single shared ceiling across every step in the test PLUS any `finally`-block cleanup; it is not a "budget per step." Never pair `test.slow()` with several individual step timeouts anywhere near or above the tripled total (e.g. three separate `{timeout: 45_000}` waits inside a test whose own timeout is 45_000) — under CI load the cumulative real time silently eats the whole budget and the failure surfaces at whatever call happens to be in flight when the deadline fires (often cleanup), misdirecting triage. See [bug-1829-shard3-flakes.md](bug-1829-shard3-flakes.md). +- **Guarantee teardown time regardless of test-body duration**: in a `finally` block (or `afterEach`), call `testInfo.setTimeout(testInfo.timeout + N)` right before cleanup — this is additive to the *current remaining* deadline (not a reset to an absolute value), so it always grants `N` ms of headroom for cleanup no matter how much of the original budget the test body already consumed. Requires adding `testInfo` as the test callback's second parameter: `async ({ page }, testInfo) => {...}`. Combine with `try/catch` around the cleanup call itself so a still-slow/failed delete doesn't throw past the (now-extended) deadline or mask the test's real outcome. +- **A route-mocked response that ALWAYS returns an empty page defeats `waitForLoaded()`-style "timeline/empty-state/error-banner visible" race helpers** — if the empty state was already visible from a *prior* transition, `.waitFor({state:'visible'})` on it resolves instantly without synchronizing to the new request. This breaks the common "click filter chip → `waitForLoaded()` → reset a `requests[]` capture array → register `waitForResponse` for the next click" pattern: a late-arriving response from the *previous* click can satisfy the next `waitForResponse` predicate before the real new request/response pair occurs, leaving the reset `requests[]` array empty when read. Fix: explicitly `await page.waitForResponse(pred)` for each transition's own response (don't rely on the UI-loaded signal to gate a network-timing-sensitive reset), and derive request/response data straight from the resolved `Response` object rather than a side-channel array. ## Strict-mode / selector anti-patterns diff --git a/.claude/agent-memory/e2e-test-engineer/known-flakes-and-regressions.md b/.claude/agent-memory/e2e-test-engineer/known-flakes-and-regressions.md index a4acb8e3a..f7e613a09 100644 --- a/.claude/agent-memory/e2e-test-engineer/known-flakes-and-regressions.md +++ b/.claude/agent-memory/e2e-test-engineer/known-flakes-and-regressions.md @@ -7,7 +7,7 @@ metadata: ## Currently open / unresolved -- **Diary shard-3 flakiness NOT fully resolved by fix PRs #1790/#1792/#1793** (triaged 2026-07-07): on beta PR #1803, `diary-r2-uat.spec.ts:599` "Manual mode API parameter (Scenario 10)" fails with `expect(lastRequest).toBeDefined()` (empty-requests-array race, a NEW failure mode); `diary-drafts.spec.ts:854` "Draft card click navigates to edit page (Scenario 14)" fails via `apiRequestContext.delete: Test timeout of 45000ms exceeded` in teardown (also a NEW failure mode vs. the original nav-timing flake). Same shard-3 pattern seen across PRs #1780/#1782/#1788/#1803 (2026-06-22 → 2026-07-06) — pre-existing, not caused by any single story. `gh run view --log`/`--log-failed` returns empty for these matrix jobs; use `gh api repos/.../actions/jobs//logs` instead. Non-blocking on beta (E2E Gates informational there) but needs dedicated follow-up — flag to orchestrator before picking up diary work. +(none — see Resolved section for the #1829 shard-3 fix) - `i18n/i18n.spec.ts` "German text does not overflow navigation sidebar on desktop" — pre-existing locale-init race, needs separate investigation. - `i18n.spec.ts` "Key page headings render in German" — intermittent ~10-20%: concurrent worker `afterEach(resetToEnglish)` races with another test's `setLanguage('de')`. Pre-existing. - `i18n-categories.spec.ts` "German locale: Manage trades tab shows 'Sanitär'..." — intermittent, locale doesn't initialize before English page renders. Pre-existing (seen before PR #1186 too). @@ -19,7 +19,8 @@ metadata: ## Resolved / fixed -- Diary Scenario 14 (`diary-drafts.spec.ts` draft-card-click-navigates) — was a persistent flake even after an earlier partial fix (PR #1671); root-caused and properly fixed via `fix/diary-scenario14-e2e-flake` (register `waitForResponse` before the click, not after). **Note (2026-07-07): recurred on shard 3 with a DIFFERENT failure signature** — see "Currently open" above; the original nav-timing bug is fixed but a new teardown-timeout flake has appeared since. +- **Issue #1829 (2026-07-08)** shard-3 diary flakes on main-targeted PRs: `diary-drafts.spec.ts:854` (Scenario 14) was genuinely failing on both attempt + retry — `test.slow()`'s 45s total test budget was being exhausted by 4 oversized nested step timeouts (30-45s each) before reaching cleanup, surfacing as `apiRequestContext.delete: Test timeout exceeded`. Fixed via `test.setTimeout(60_000)` + proportionate 15s step ceilings + `testInfo.setTimeout(testInfo.timeout + 15_000)` guaranteed-cleanup-time pattern in `finally`. `diary-r2-uat.spec.ts:599` (Scenario 10) had a genuine secondary race (always-empty mock defeats `waitForLoaded()`, letting a stale prior response satisfy the next `waitForResponse`) — fixed by awaiting each transition's own response explicitly and reading straight off the resolved `Response` object. `document-linking.spec.ts:369` was NOT an independent bug — confirmed via job logs it was cancelled mid-flight (1.6s runtime) as `maxFailures:1` collateral once drafts:854 exhausted its retries; no test-logic fix needed, just added the guaranteed-cleanup-time pattern defensively. Full root cause + `maxFailures`/retry-accounting proof (Playwright source citation) in [bug-1829-shard3-flakes.md](bug-1829-shard3-flakes.md). AC #3 (fail-fast retry tolerance) was already satisfied by Playwright's built-in behavior — no config change needed, only documented. +- Diary Scenario 14 (`diary-drafts.spec.ts` draft-card-click-navigates) — was a persistent flake even after an earlier partial fix (PR #1671); root-caused and properly fixed via `fix/diary-scenario14-e2e-flake` (register `waitForResponse` before the click, not after). Recurred 2026-07-07 with a DIFFERENT failure signature (teardown-timeout, not nav-timing) — see #1829 entry above for the follow-up fix. - `invoice-budget-line-create-and-link.spec.ts` Scenarios 1–4 — REAL production regression from PR #1566 (`eagerLinkInvoice:false`), filed as bug #1611. Tests were NOT weakened; they assert correct behavior. - Shard 3 promotion blocker (`budget-source-filter.spec.ts`, 2026-06-12): "Rapid debounce coalesces requests" flaked on strict request-count assertions vs. CI click serialization beyond the 50ms debounce (fixed PR #1665); "Perspective toggle changes Cost value" flaked on reading `textContent()` immediately after a radio click without waiting for React re-render on WebKit (fixed PR #1666). **General fix pattern**: after any click triggering a React state update, `await expect(locator).not.toHaveText(previousValue)` before `textContent()` — never read immediately post-click on WebKit. Remove `page.on('request', ...)` listeners you add (persist for the page's lifetime otherwise); prefer state assertions (`aria-pressed`, URL params) over raw request counting. - `dashboard.spec.ts:566` "Customize button appears when card dismissed" — fixed PR #1445 (`expect.poll` for preference state). Was issue #1431. diff --git a/.claude/agent-memory/security-engineer/MEMORY.md b/.claude/agent-memory/security-engineer/MEMORY.md index fe937877d..a35280489 100644 --- a/.claude/agent-memory/security-engineer/MEMORY.md +++ b/.claude/agent-memory/security-engineer/MEMORY.md @@ -9,6 +9,9 @@ - **Commit trailer**: `Co-Authored-By: Claude security-engineer (Sonnet 4.6) ` - **PR review**: Post as `--comment` (NOT `--approve` — same token can't approve own PRs) - **npm audit**: Run `npm audit --omit=dev` for production vuln check (dev audit includes npm's own bundled tools which have 39 vulns unrelated to app) +- **CI does NOT run a vuln scan**: `.github/workflows/ci.yml` Static Analysis job only runs `npm audit signatures` (provenance/signing check) — never `npm audit` for known CVEs. This security review is the only vulnerability-scan gate dependency PRs get; don't assume a green CI implies audit-clean. +- **Fast whole-lockfile audit without `npm install`**: parse `package-lock.json` `.packages` entries into unique `name -> [versions]`, POST as `{name:[versions...]}` to `https://registry.npmjs.org/-/npm/v1/security/advisories/bulk` (same data source `npm audit` uses). Works on a lockfile alone — no install needed, handles monorepos with 1700+ resolved packages in one call. Use this whenever reviewing a package-lock.json diff. +- **Verifying a scoped npm `overrides` entry**: nested syntax `"parentPkg": { "subDep": "version" }` resolves correctly if-and-only-if the lockfile shows `node_modules/parentPkg/node_modules/subDep` at the pinned version while root `node_modules/subDep` stays at the original (unscoped) version, and no *other* package also has its own nested copy of subDep (which would mean the override missed a consumer). Check via `grep -n '"node_modules/.*node_modules/"'` in the lockfile — cheaper than running `npm ls`. ## Established Baseline Security Controls @@ -30,9 +33,9 @@ Verified across EPIC-01/02/03/05 — all confirmed STRONG: - [Full PR review table](pr-review-table.md) — every PR reviewed since project inception, one line each - [Detailed review findings](review-history.md) — full write-ups for PRs with notable findings -Most recent: **#1853** repo-hygiene (dead BudgetPage removal, AutoItemizePdfPreview.test.tsx, wiki doc for merge-lines endpoint, errorHandler.ts doc-comment fix, checklist/CLAUDE.md exemption wording) — APPROVED, no findings. Confirmed `getDocumentPreviewUrl()` iframe src always resolves to the app's own `getBaseUrl()` proxy path (never a raw external URL) so no SSRF/open-redirect; jsdom client project has no `resources: 'usable'` so iframe `src` never triggers a real fetch in unit tests — safe default worth reusing when reviewing any future iframe/img-src test. +Most recent: **#1854** fix(deps): scope `js-yaml` override to `gray-matter@3.15.0` — APPROVED, no findings. Verified `3.15.0` is the genuine GHSA-h67p-54hq-rp68/CVE-2026-53550 patched release on the 3.x line (advisory has two independently-patched ranges: `4.0.0-4.1.1`→`4.2.0` and `<3.15.0`→`3.15.0`); confirmed via lockfile that only `node_modules/gray-matter/node_modules/js-yaml` resolves to 3.15.0 while root stays 4.2.0; full-lockfile bulk audit (1781 packages) returned 0 advisories; confirmed gray-matter's `lib/engines.js` calls `yaml.safeLoad`/`safeDump` (js-yaml 3.x's SAFE_SCHEMA — no `!!js/function` deserialization gadget) so no new attack surface. Also noted (informational, not blocking): a full `npm install` regeneration churns unrelated transitive deps incl. one prod dep (`@fastify/static > lru-cache` patch bump) — expected per repo's "always regenerate via full npm install" policy, disclosed in the PR body, audit-clean. -Previous: **#1852** trailer-enforcement tooling (CI job + script + squash-merge pattern docs) — APPROVED, 1 informational (`github.base_ref` interpolated into `run:` shell, low exploitability, see architecture-patterns.md for the base_ref-vs-head_ref triage method and the heredoc double-expansion analysis — both reusable for future CI/workflow reviews). +Previous: **#1853** repo-hygiene (dead BudgetPage removal, AutoItemizePdfPreview.test.tsx, wiki doc for merge-lines endpoint, errorHandler.ts doc-comment fix, checklist/CLAUDE.md exemption wording) — APPROVED, no findings. Confirmed `getDocumentPreviewUrl()` iframe src always resolves to the app's own `getBaseUrl()` proxy path (never a raw external URL) so no SSRF/open-redirect; jsdom client project has no `resources: 'usable'` so iframe `src` never triggers a real fetch in unit tests — safe default worth reusing when reviewing any future iframe/img-src test. ## Known Open Recommendations diff --git a/.claude/agent-memory/security-engineer/pr-review-table.md b/.claude/agent-memory/security-engineer/pr-review-table.md index b8035cd10..425252af1 100644 --- a/.claude/agent-memory/security-engineer/pr-review-table.md +++ b/.claude/agent-memory/security-engineer/pr-review-table.md @@ -81,3 +81,4 @@ Full review status per PR. See `review-history.md` for detailed findings on PRs | #1844 | Fix #1812 — i18n sweep (~69 strings, 27 components) + dead MilestonePanel cluster removal | APPROVED (all interpolation renders via JSX text/attribute sinks — auto-escaped; no dangerouslySetInnerHTML/Trans introduced) | 2026-07-07 | | #1852 | Fix #1820 — trailer enforcement: CI `trailer-check` job, `scripts/check-trailers.sh`, squash-merge trailer preservation pattern (CLAUDE.md + 5 skills), dev-team-lead self-derivation | APPROVED (1 informational: `${{ github.base_ref }}` interpolated into `run:` shell block — see architecture-patterns.md) | 2026-07-08 | | #1853 | Fix #1821 — repo hygiene: dead BudgetPage removal, AutoItemizePdfPreview.test.tsx, wiki doc for merge-lines endpoint, errorHandler.ts doc-comment fix, checklist/CLAUDE.md exemption wording | APPROVED (no findings — verified iframe src is app-internal proxy path, no SSRF; jsdom does no real fetch on iframe src) | 2026-07-08 | +| #1854 | Fix #1827 — scope `js-yaml` override to `gray-matter@3.15.0` (patched 3.x line) so Docs Deploy stops breaking on removed `safeLoad` API | APPROVED (verified 3.15.0 is the genuine GHSA-h67p-54hq-rp68 patched 3.x release, scoped override resolves gray-matter subtree only, full-lockfile bulk audit clean, gray-matter uses safeLoad = SAFE_SCHEMA) | 2026-07-08 | diff --git a/e2e/containers/cornerstoneContainer.ts b/e2e/containers/cornerstoneContainer.ts index bc1634270..00aa28afc 100644 --- a/e2e/containers/cornerstoneContainer.ts +++ b/e2e/containers/cornerstoneContainer.ts @@ -63,7 +63,7 @@ export async function startCornerstoneContainer( HTTPS_PROXY: httpsProxy || '', NODE_EXTRA_CA_CERTS: '/tmp/proxy-ca.crt', }) - .withCopyFileToContainer(caPath, '/tmp/proxy-ca.crt'); + .withCopyFilesToContainer([{ source: caPath, target: '/tmp/proxy-ca.crt' }]); } const container = await containerBuilder.start(); diff --git a/e2e/playwright.config.ts b/e2e/playwright.config.ts index d38fdabeb..c2d35da98 100644 --- a/e2e/playwright.config.ts +++ b/e2e/playwright.config.ts @@ -116,7 +116,16 @@ export default defineConfig({ timeout: 15_000, // 15 seconds per test (desktop default) /* On main-targeted PRs, stop the shard after the first non-recoverable failure - so fail-fast can cancel the remaining shards immediately. */ + so fail-fast can cancel the remaining shards immediately. + Verified (bug #1829, Playwright 1.61.1 runner/dispatcher.ts _reportTestEnd): + maxFailures only counts a test once it has exhausted ALL of its `retries` and + is still 'unexpected' — a test that fails on attempt 1 but passes on retry + ("flaky") never increments the failure count. This already gives one + retry-passing test a pass before cancelling the run; a genuinely flaky test + does not need special-casing here. Only a test that fails on every attempt + (a real bug, or a test with a real race) trips this. See + https://playwright.dev/docs/test-timeouts and the dispatcher source for the + exact accounting. */ maxFailures: process.env.E2E_FAIL_FAST ? 1 : undefined, /* Global timeout: cap the entire suite at 30 minutes on CI to prevent stuck runs */ diff --git a/e2e/tests/diary/diary-drafts.spec.ts b/e2e/tests/diary/diary-drafts.spec.ts index 502449793..2287e733c 100644 --- a/e2e/tests/diary/diary-drafts.spec.ts +++ b/e2e/tests/diary/diary-drafts.spec.ts @@ -853,10 +853,22 @@ test.describe('Drafts chip (Scenario 13)', () => { test.describe('Draft card click navigates to edit page (Scenario 14)', () => { test('Clicking a draft entry card navigates to /diary/:id/edit, not /diary/:id', async ({ page, - }) => { - // Triple default timeouts — the server under 8-worker CI load can respond slowly - // to getDiaryEntry(), causing the badge to appear >7s after navigation. - test.slow(); + }, testInfo) => { + // Give this test its own generous absolute budget instead of test.slow(). + // NOTE (bug #1829): test.slow() only triples the *overall* per-test timeout + // (15s -> 45s here) — it does NOT triple actionTimeout/expect.timeout, and it + // is a single shared ceiling for every step in the test PLUS the finally-block + // cleanup below. The previous version of this test paired test.slow() (45s + // total) with several individual step timeouts of 30_000-45_000ms each — + // any one of which could alone consume the entire budget. Under heavy 8-worker + // CI load the cumulative real time across steps regularly ate the whole 45s, + // starving the `deleteDiaryEntryViaApi` cleanup call and surfacing as + // "apiRequestContext.delete: Test timeout of 45000ms exceeded" — a cleanup + // symptom, not a cleanup cause. Use an explicit absolute timeout instead, and + // keep per-step ceilings proportionate (real durations are expected to be a + // few seconds; these are "something is actually stuck" ceilings, not budget + // reservations). + test.setTimeout(60_000); const diaryPage = new DiaryPage(page); const editPage = new DiaryEntryEditPage(page); @@ -883,33 +895,45 @@ test.describe('Draft card click navigates to edit page (Scenario 14)', () => { resp.url().endsWith(`/api/diary-entries/${draftId}`) && resp.request().method() === 'GET' && resp.status() === 200, - { timeout: 45_000 }, + { timeout: 15_000 }, ); await diaryPage.entryCard(draftId).click(); - // Should navigate to /diary/:id/edit. - // Pass an explicit timeout (3× the 10s navigationTimeout) to match test.slow(). - // test.slow() triples actionTimeout and expect.timeout but NOT navigationTimeout - // (which is set at the project config level). On loaded CI runners the SPA - // router + React re-render after the click can exceed the default 10s. - await page.waitForURL(new RegExp(`/diary/${draftId}/edit$`), { timeout: 30_000 }); + // Should navigate to /diary/:id/edit. On loaded CI runners the SPA router + + // React re-render after the click can exceed the default 10s navigationTimeout. + await page.waitForURL(new RegExp(`/diary/${draftId}/edit$`), { timeout: 15_000 }); expect(page.url()).toContain(`/diary/${draftId}/edit`); // Explicitly await the getDiaryEntry API response before asserting the heading/badge. - // Under heavy 8-worker CI load the server can take >15s to respond; waiting for the - // actual response ensures the heading assertion fires only after data is in-flight, - // preventing the hardcoded timeout from being the bottleneck. + // Under heavy CI load the server can take several seconds to respond; waiting for + // the actual response ensures the heading assertion fires only after data is + // in-flight, preventing the hardcoded timeout from being the bottleneck. await entryLoadPromise; - // The heading and draft badge render after getDiaryEntry() returns — both assertions - // now have the full test.slow() budget (45s) since the API response has arrived. - await expect(editPage.heading).toBeVisible({ timeout: 45_000 }); + // The heading and draft badge render after getDiaryEntry() returns. + await expect(editPage.heading).toBeVisible({ timeout: 15_000 }); // Edit page should show the draft badge (entry.status === 'draft') - await expect(editPage.draftBadge).toBeVisible({ timeout: 45_000 }); + await expect(editPage.draftBadge).toBeVisible({ timeout: 15_000 }); } finally { - if (draftId) await deleteDiaryEntryViaApi(page, draftId); + // Guarantee cleanup gets its own dedicated time budget on top of whatever + // remains, regardless of how much of the test's overall timeout the steps + // above consumed. testInfo.setTimeout(timeout) is additive to the current + // remaining deadline (not a reset to an absolute value), so this always adds + // 15s of headroom from "now" — see https://playwright.dev/docs/test-timeouts. + testInfo.setTimeout(testInfo.timeout + 15_000); + if (draftId) { + try { + await deleteDiaryEntryViaApi(page, draftId); + } catch (error) { + // Best-effort cleanup: a slow/failed delete must not mask this test's + // actual pass/fail outcome or itself blow the (already-extended) test + // timeout. The container is torn down after the run, so an orphaned + // draft here is harmless. + console.warn(`[diary-drafts] cleanup failed for draft ${draftId}:`, error); + } + } } }); }); diff --git a/e2e/tests/diary/diary-r2-uat.spec.ts b/e2e/tests/diary/diary-r2-uat.spec.ts index efd24fd2b..f1b5eeefd 100644 --- a/e2e/tests/diary/diary-r2-uat.spec.ts +++ b/e2e/tests/diary/diary-r2-uat.spec.ts @@ -598,10 +598,8 @@ test.describe('Automatic mode API parameter (Scenario 9)', () => { test.describe('Manual mode API parameter (Scenario 10)', () => { test('Selecting "Manual" mode sends only manual entry types to the API', async ({ page }) => { const diaryPage = new DiaryPage(page); - const requests: URL[] = []; await page.route('**/api/diary-entries*', async (route) => { - requests.push(new URL(route.request().url())); await route.fulfill({ status: 200, contentType: 'application/json', @@ -615,17 +613,27 @@ test.describe('Manual mode API parameter (Scenario 10)', () => { await diaryPage.openFiltersIfCollapsed(); // Default is now "Manual" — switch to "All" first so we can test clicking Manual. - // Use waitForLoaded() after the click instead of waitForResponse: it waits for - // the full load cycle (isLoading→false, empty-state visible), guaranteeing the - // All-click API response was received and captured in requests[]. + // + // NOTE (bug #1829): this mock always returns an empty page, so + // diaryPage.waitForLoaded() (which races timeline/empty-state/error-banner + // visibility) can resolve INSTANTLY here — the empty state is already visible + // from the initial load and never changes. It is not a reliable signal that + // the "All" click's own network round-trip has completed. Explicitly wait for + // that specific response before moving on, so a late-arriving "All" response + // can never be mistaken for the "Manual" click's response below (the race that + // previously caused an intermittently empty captured-requests list). + const allResponsePromise = page.waitForResponse( + (resp) => resp.url().includes('/api/diary-entries') && resp.status() === 200, + ); const allChip = page.getByTestId('mode-filter-all'); await allChip.waitFor({ state: 'visible' }); await allChip.click(); + await allResponsePromise; await diaryPage.waitForLoaded(); - // Reset captured requests from the "All" switch - requests.length = 0; - + // Register the listener BEFORE clicking Manual, and read the request straight + // off the resolved Response object — avoids any race with a shared mutable + // array that a stale in-flight response could otherwise still race against. const responsePromise = page.waitForResponse( (resp) => resp.url().includes('/api/diary-entries') && resp.status() === 200, ); @@ -633,11 +641,10 @@ test.describe('Manual mode API parameter (Scenario 10)', () => { const manualChip = page.getByTestId('mode-filter-manual'); await manualChip.waitFor({ state: 'visible' }); await manualChip.click(); - await responsePromise; + const manualResponse = await responsePromise; - const lastRequest = requests[requests.length - 1]; - expect(lastRequest).toBeDefined(); - const typeParam = lastRequest?.searchParams.get('type'); + const lastRequest = new URL(manualResponse.url()); + const typeParam = lastRequest.searchParams.get('type'); // type parameter must be present and contain at least one manual type expect(typeParam).toBeTruthy(); diff --git a/e2e/tests/documents/document-linking.spec.ts b/e2e/tests/documents/document-linking.spec.ts index 1a4c55aa2..bdf44359e 100644 --- a/e2e/tests/documents/document-linking.spec.ts +++ b/e2e/tests/documents/document-linking.spec.ts @@ -369,7 +369,7 @@ test.describe('Document Linking — System-wide Hide (Scenario 7)', { tag: '@res test('System-wide linked IDs filter the picker — toggle defaults ON and hides linked doc', async ({ page, testPrefix, - }) => { + }, testInfo) => { let workItemAId: string | null = null; let workItemBId: string | null = null; try { @@ -467,10 +467,18 @@ test.describe('Document Linking — System-wide Hide (Scenario 7)', { tag: '@res await expect(documentGrid.getByRole('listitem')).toHaveCount(1, { timeout: 10000 }); await expect(documentGrid).not.toContainText(MOCK_DOCUMENT.title); } finally { + // Guarantee cleanup gets its own dedicated time budget on top of whatever + // remains of the describe-level 60s timeout (bug #1829: don't let a slow + // test body starve teardown of its own two API deletes). + testInfo.setTimeout(testInfo.timeout + 10_000); await cleanupMocks(page); await page.unroute('**/api/document-links/linked-ids'); - if (workItemAId) await deleteWorkItemViaApi(page, workItemAId); - if (workItemBId) await deleteWorkItemViaApi(page, workItemBId); + try { + if (workItemAId) await deleteWorkItemViaApi(page, workItemAId); + if (workItemBId) await deleteWorkItemViaApi(page, workItemBId); + } catch (error) { + console.warn('[document-linking] cleanup failed for scenario 7a work items:', error); + } } });