test(e2e): route-affordance inventory and smoke walk for #1949 AC4 - #2673
Conversation
Hand-authored, import-free data module: 12 walked /workspace routes with 2-5 affordances each, the 22-name exclusion map with a stated reason per route, and the unnamed-redirect count. It imports nothing so the vitest coverage guard can consume it under tsconfig.vitest.json, which carries no node types.
Seven assertions read router.getRoutes() directly, with no generated JSON in between: shape, name existence, set equality against the named requiresShell surface in both directions, the unnamed /workspace redirect count, reason length, id uniqueness and source format, plus a canary proving the equality check can fail.
Four test blocks activate every activate:true row and assert the consequence the inventory declares. Nothing irreversible runs: the Today seal confirm is asserted enabled and declined, Execute stops at the apply dialog and cancels, the boardless accept-on-board control is asserted disabled, and openers are dismissed rather than submitted. Two measured corrections to the inventory ride along: the metrics response patterns no longer pin one board id because MetricsView auto-selects boards[0] on mount, and calendar.timeline-mode moves to out-of-slice-1 because the empty-state branch at CalendarView.vue:260 precedes both view modes while totalCards is 0.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Review record (alpha product-trust lane, review-and-ship round 1 at head fcfc2a3). Reviewer: one fresh-context independent reviewer subagent (read-only), input = merge-base..head diff, the worktree at the head and the ratified AC4 contract. Verdict: FIX-FIRST, one HIGH.
Confirmed clean by the reviewer: the router arithmetic (34 named shell routes = 12 inventory + 22 excluded, four unnamed redirects, not-found pinned as the only non-workspace member; the canary is load-bearing); the inventory imports nothing under a vitest tsconfig with no node types; the router is read the way the route-stability spec reads it; 18 selectors spot-checked against source including every substitution; openers whose consequence is an attribute are load-bearing; the state-flip assertions genuinely flip; all eight URL consequences are not pre-satisfied; the destructive-action policy holds everywhere (seal confirm and accept-on-board never clicked; Execute cancelled at the dialog; Approve, Archive, board-create and quick-capture on a throwaway user; no connector writes); every cited covering spec exists at the cited lines; the dev-tools exclusion is exact; no cross-worker hazard in the walk plan; no ESLint disables; the new file is picked up automatically by the E2E Smoke lane. Merge gate: round-2 push, one verification pass scoped to the fix diff, ci-required green at the round-2 head (including the E2E Smoke lane that executes the walk), aged three minutes, then merge commit. |
… rows HIGH: the four response rows armed page.waitForResponse just before the click, so a request the view already issued on mount could satisfy them and a dead control would still pass. Each route now consumes its mount read before arming (armed before goto, awaited), the metrics rows are pinned to the board the walk selects rather than the one MetricsView auto-selects, and rows carry an independent postCondition wherever the surface renders something that changes. Proven red-first by turning #board-select into a dead control. The row wait is bounded at 15 s so a dead control reports fast instead of burning the 90 s test timeout. Also: expectConsequence honours the affordance scope instead of taking a page-wide first(); today.write-note and today.seal-confirm declare their real consequences (focus, and the control's own enabled state) rather than nodes that render unconditionally; the automations-queue, settings-access and not-found exclusion reasons are corrected to what the source actually says; the guard pins the four unnamed redirect paths, not just their count; and the two coveredBy citations name the click lines.
|
Round-2 verification record (scoped to the fix diff fcfc2a3..53417f9; read-only pass). Verdict: SHIP for the fix diff, no CRITICAL or HIGH; the round-1 HIGH is closed for the four rows it named. Confirmed: the metrics mount issues exactly one request matching the from= anchor (the forecast and export URLs differ), so the pre-goto wait consumes it and nothing else is left in flight; the {metricsBoardId} pin is load-bearing (with a dead board select, neither the pinned metrics URL nor the export URL can be requested) and a board change always serialises from=; the range option's DOM value is the string 90; the notifications route issues exactly one mount GET (one caller in the tree, no bell fetch, no poller), the alternation excludes mark-all-read and preferences, and a dead Refresh fails at the 15 s bound; the bound goes through waitForResponse's timeout option; activate passes the same scope object to locate and expectConsequence, and the two scoped rows resolve inside their scopes; the write-note focus consequence genuinely reddens on a dead button; the guard's four redirect paths match the router table in sorted order; the inventory still imports nothing; nothing outside the three files. Three MEDIUMs tracked as #2678 rather than a third round (the two-round ceiling is reached; none is a merge blocker for a test-only PR): expectConsequence is a non-exhaustive if-chain and status.assertEnabled is enforced nowhere after round 2 (a future guarded row with a response consequence would assert nothing); the spec header claims universal mount consumption but the calendar rows still arm after navigation and are protected only by ad-hoc label assertions; the value post-condition on the two selects cannot fail for a dead select (the pin is the real defence) and the docs credit the wrong half. Two LOWs recorded there: a response promise armed before a blocking act rejects unawaited; 15 s headroom is about 5x the measured latency. Merge gate unchanged: ci-required green at 53417f9 (including the E2E Smoke lane that executes the walk), aged, merge commit. |
…als, review records) Answers the docs review of PR #2686: the SC-10 sentence returns to the unenumerated form (the enumeration dropped #2550 and #2610 and disagreed with the thirteenth block's count of ten); the #2675 bullet no longer says #2090 keeps only two residuals; the header names both PRs that carried a scoped verification pass (#2673, #2681) and the #2681 and #2679 bullets carry their review records; #2638 is tracked as both items.
Summary
Refs #1949 (AC4). Adds the route-walking affordance smoke pass as a closed, hand-authored inventory plus one Playwright walk plus one vitest coverage guard. There is no crawler and no generated JSON anywhere in the chain.
Three new files, zero production-source changes, zero doc changes.
frontend/taskdeck-web/tests/e2e/support/routeAffordanceInventory.tsis data only and imports nothing, because the vitest guard consumes it undertsconfig.vitest.json, whosetypesare["vite/client", "vite-plugin-pwa/client"]with no node types. It lists 12 walked/workspaceroutes with 2 to 5 affordances each (40 rows), a 22-name exclusion map with a stated reason per route, and the unnamed-redirect count.frontend/taskdeck-web/src/tests/guards/routeAffordanceCoverage.spec.tsreads the realrouter.getRoutes()table and fails until every named route carryingrequiresShellis either walked or excluded with a reason, in both directions.frontend/taskdeck-web/tests/e2e/route-affordances.spec.tswalks every row markedactivate: truein four test blocks and asserts the consequence the inventory declares.Nothing irreversible runs. Today's seal confirm is asserted enabled and never clicked. Execute is walked to the apply-to-board dialog, the accept button is asserted enabled, and the dialog is then cancelled, so
support/applyConfirm.tsis deliberately not used here because that helper's contract is to accept. The boardlessaccept-on-boardcontrol is asserted disabled, which is the #1944 shape, and never clicked. Openers for board settings, add column, add card, add connector and new view are opened and dismissed, never submitted.This is Refs, not Closes. AC4 is delivered as a closed inventory; the parser-precision LOWs and the keystroke descoping recorded on the issue stay as they are. Keystroke rows are deliberately absent because shortcut truth belongs to
shortcutLedgerTruth.spec.tsandshortcutNotation.spec.ts, and the inventory header says so.Round 2
One commit,
53417f9, answering the FIX-FIRST verdict. Every item below was verified by re-running the walk.HIGH — a mount read could satisfy a response row, so a dead control would pass
Correct and now fixed. The four
responserows armedpage.waitForResponseimmediately before the click, butMetricsViewauto-selectsboards[0]inonMountedand its watcher fetches in the same tick, andNotificationInboxViewfetches on mount while Refresh renders on first paint. Either mount request could settle the row's wait.Three defences, all applied:
Consume the mount read before arming. Both routes now arm a wait before
page.goto, await it, and only then letactivateregister the row's own wait. Playwright cannot match a response that arrived before its waiter existed, so only a genuinely new request can settle the row. This copies the pattern the Home block already used.Pin the patterns. Exact sources now in the inventory:
metrics.board-select—/api/metrics/boards/{metricsBoardId}\?from=metrics.range-select—/api/metrics/boards/{metricsBoardId}\?from=metrics.export-csv—/api/metrics/boards/{metricsBoardId}/export\?notifications.refreshandnotifications.unread-only—/api/notifications(\?|$)(unchanged; both notification rows rely on defence 1, see below){metricsBoardId}is a new context token bound at runtime to whichever of this block's two seeded boardsMetricsViewdid not auto-select, with an explicitexpect(...).not.toBe(preselected)guarding that choice. The mount read's URL therefore cannot match the rows' patterns even if defence 1 were removed. Anchoring on?from=also stops the old unanchored pattern from matching a bare/metrics/boards/<id>path.Independent post-conditions.
RouteAffordancegains an optionalpostCondition, asserted after the response, for rows where the surface renders something that changes:#board-selectholds{metricsBoardId}, and#range-selectholds90. These are what fail when the control is inert but the network happens to be busy.Both notifications rows deliberately carry no
postCondition, and the inventory says why on the rows:NotificationInboxView.vue:267renders the same "No notifications found." empty state filtered or not, and no helper can seed a notification to tell the two lists apart. They rest on defence 1 alone. That is a weaker row than the metrics ones and is stated as such rather than papered over.Red-first proof.
#board-select'sv-model="selectedBoardId"was changed to:value="selectedBoardId"in the worktree — the exact dead-control shape, where the DOM select changes but the ref never does, so the watcher never fires. The change was not committed andMetricsView.vuewas restored afterwards;git statusshows only the three test files.That 90 s failure exposed a second, smaller problem: an unbounded wait spends the whole test timeout before reporting. The row wait is now bounded at 15 s, so a dead control fails about six times faster and costs the shared E2E Smoke budget far less. Re-running after restoring
MetricsView.vueis green.MEDIUM —
expectConsequencediscarded the scopeCorrect. It built
nodeandattributelocators frompageand took.first(), so once a seed has two columns or two captures a consequence could be satisfied by a node the walk never opened. It now takes the same scope the affordance was found in and asserts there.urlstays page-level, because navigation is a property of the page and not of a scoped locator; that is commented in place.MEDIUM — two Today rows declared consequences that prove nothing
Correct on both.
today.write-notedeclared a nodeTodayLineForTomorrowrenders unconditionally, so the spec'stoBeFocusedwas carrying the whole proof while the inventory understated it;today.seal-confirmdeclared the group it lives inside rather than the control. Two consequence kinds were added so the inventory states the truth:focus(line-for-tomorrow-inputholds focus, which is whatPaperTodayView.vue:113-118actually does) andenabled(the[data-action="seal-confirm"]control itself). A third kind,value, backs the metrics post-conditions.assertReachableButNotActivatednow asserts the row's declared consequence instead of hardcoding an enabled check.MEDIUM — the automations-queue exclusion reason was false
Correct, and it was my error.
AutomationQueueViewis the raw queue-request surface (queueStorerows, a request-type and payload composer) and renders no review rail, so the old reason claimed a duplication that does not exist. The reason now states the real barrier: no E2E spec enters the route and no support helper seeds a queue request, so every control sits behind an empty list.LOWs
/workspace,/workspace/activity/user/:userId,/workspace/automations,/workspace/automations/proposals) and keeps the count as an arity check on that list. A count alone was satisfied by any four redirects.not-foundnow records that it does render two recovery links (NotFoundView.vue:18and:21) and stays excluded because the route holds no state to walk and both links duplicate shell navigation.workspace-settings-accessreason is corrected:registerUserSessioncan mint an account, so the barrier is the board-sharing step no helper performs.coveredBycitations now name the click lines:capture-loop.spec.ts:55(was 52, the testid lookup) andfirst-run.spec.ts:49(was 48, the visibility assertion).test.skip, agrepfilter,maxFailurescutting the run short — drops its rows from the proof silently. An unwalked new row is still always caught, because the plan-versus-inventory half runs in every surviving block; it is a whole missing block that goes unnoticed, and reading the summary for four passing blocks is what closes it. No marker file was added: it would put shared mutable state back into a file that just had it removed for exactly that reason.Changes
Four commits.
test(e2e): add the closed route-affordance inventory for #1949 AC4test(guards): pin the route-affordance inventory against the real routertest(e2e): walk the route-affordance inventory in four smoke blockstest(e2e): close the mount-read hole in the route-affordance response rows— round 2The compared surface in guard assertion 3 is "named routes whose meta carries
requiresShell: true" rather than a/workspacepath prefix. That set is the 33 named/workspaceroutes plusnot-found, which is/:pathMatch(.*)*but still renders inside the shell. A separate assertion pinsnot-foundas the only non-/workspacemember, so the framing cannot drift silently.Test plan
Verified (round 2 head, 53417f9)
npx vitest --run --maxWorkers=2 src/tests/guards/routeAffordanceCoverage.spec.ts src/tests/router/workspaceRouteStability.spec.ts— 2 files, 43 tests passed (7 new guard assertions plus the 36 existing router-stability tests).npm run typecheck(vue-tsc -b) — clean.npx eslint tests/e2e/route-affordances.spec.ts tests/e2e/support/routeAffordanceInventory.ts src/tests/guards/routeAffordanceCoverage.spec.ts— exit 0, no warnings. ESLint is the only static gate on the Playwright spec, which is in no tsconfig project.npx playwright test tests/e2e/route-affordances.spec.ts --project=chromium --workers=1 --reporter=listwith the four retiredLlm__Gemini__*variables unset — 4 passed in 21.8 s. Per test: board-seeded 5.3 s, home and inbox 1.3 s, today 873 ms, review 6.8 s. An intermediate round-2 run before the wait was bounded measured 30.0 s; the difference is run-to-run variance on this box, not the bound.git diff --check— exit 0.git status --porcelain— only the three test files;MetricsView.vuerestored after the red-first proof.Round 1 also verified
--listcompile and discovery (4 tests in 1 file) and a run at the local default of 2 workers (4 passed, 23.8 s), which confirmed the completeness check does not depend on worker layout.Red-first evidence
Three, each produced by breaking the checked thing, observing the failure, then restoring.
Coverage equality, by deleting the
workspace-archiveexclusion entry:The guard's own canary, by changing its synthetic inventory from
slice(1)(one route dropped) toslice(0)(complete):The HIGH, by turning
#board-selectinto a dead control — quoted in the Round 2 section above.NOT verified
npm run buildand the full vitest suite were not run. Nothing outside the three test files changed.file:line; those citations were read, not re-run.out-of-slice-1and are unproven at runtime.covered-elsewhererows are not clicked by this file.Boundaries and risks
Selector substitutions against the ratified contract
Every contract selector was checked against source before use. These moved.
PaperBoardColumn.vueis atsrc/views/paper/PaperBoardColumn.vue, notsrc/views/paper/board/. Thedata-action="toggle-add-card"hook is at line 298 as stated.[data-action="add-card-input"](real, atPaperCardComposer.vue:70). The inventory asserts the composer's stable testidpaper-card-composeron the same element tree, because the consequence vocabulary addresses nodes by testid.paper-board-dialog-namelives inPaperBoardSettingsDialog.vue:170, notPaperBoardView.vue. The affordance'ssourcepoints at the trigger,PaperBoardView.vue:779.expectApplyConfirmDialog(support/applyConfirm.ts:40) clicksapply-confirm-accept, so it could not be used for the Execute row, whose point is to stop before that.covered-elsewherecitations, corrected again in round 2 to the click lines:capture-loop.spec.ts:55,first-run.spec.ts:49. Capture-herereview-proposals.spec.ts:75, composer capture:88and Ask AI:97were confirmed as stated.smoke.spec.ts:293; chat isautomation-ops.spec.ts:56; ops cli isvalidation-ops-logs-health.spec.ts:14and ops logs:72.workspace-automations-queue,workspace-ops-endpointsandworkspace-metrics-cohortshave no E2E coverage at all, so their reasons state the seeding gap.paper-review-clear-scope(PaperReviewView.vue:2922) renders only inside the board-scoped EMPTY branch of the deep pane. It is walked in the board-seeded block, on a board with no proposals, rather than in the review block whose board deliberately has one. This is a deviation from the contract's block split.No production testid was added, and no production source is changed by this PR.
Rows recorded as out-of-slice-1
notifications.mark-all-read— renders only whenunreadCount > 0; no helper creates an unread notification.calendar.timeline-mode— found by running, not reading. The empty-state branch atCalendarView.vue:260(calendarData.totalCards === 0) precedes both view modes in thev-else-ifchain, so the timeline list cannot render for a throwaway user and no helper seeds a due-dated card. The button itself is present and clickable; only the declared consequence is unreachable.views-detail.open-result-card— no helper seeds a card matching the Blocked Work filter. This row also supplies the second affordance the 2-to-5 rule requires forworkspace-views-detail, which the contract listed with only one.Completeness check design
The obvious design, accumulating walked ids across the file and checking in
afterAll, is unsound here and was observed failing twice: Playwright restarts the worker process after a failing test, which silently resets the accumulator, andfullyParallel: truelets tests from one file land in different workers, which splits it. Completeness is proved instead by a declaredWALK_PLANthat must name exactly the inventory'sactivate: trueandguarded-not-activatedrows, checked on every test, plus a per-block check that the block walked exactly its share.E2E Smoke budget
Two independent ceilings, which matters for the #1872 slow-install concern.
timeout-minutes: 35on thee2e-smokejob (reusable-e2e-smoke.yml:27).timeout-minutes: 12on the "Run Playwright smoke tests" step (line 77), matched byglobalTimeout: 12 * 60_000inplaywright.config.ts:110.install-depshas its owntimeout-minutes: 10(line 63); the browser installtimeout-minutes: 5(line 68).Measured. Only two
ci-requiredruns onmaincompleted in the last 20; the rest were cancelled by superseding pushes, so three recent completed runs on PR branches are included.Headroom against the binding ceiling is about 5.5 min of the 12-min test step. This spec measures 21.8 s wall on a warm local box at
workers: 1; allowing 2 to 3x for a hosted runner, budget roughly 45 to 70 s, about 12 to 21 percent of that headroom.A slow install cannot push this spec past the job ceiling, because the two budgets are independent: worst case is 10 + 5 + 12 plus about 1 min of setup and upload, roughly 28 min against the 35-min job ceiling. The failure mode a slow install produces is its own step timeout, which this change does not affect.
If the lane tightens, shed rows to slice 2 in this order by marking them
out-of-slice-1and deleting theirWALK_PLANentries and calls: the three metrics rows first (heaviest per row: two selects and an export round trip), then the two integrations rows. That removes five of the twenty rows in the heaviest block. Do not raiseglobalTimeoutor the step timeout:.github/**is a control-plane change for the maintainer under SC-10 and is out of scope here.Other risks
boards.create-submit. No teardown helper exists in this repo's E2E support, which is the posture every other spec here takes.