Repository navigation
test(e2e): port view-state suite with reload and rehydration coverage (vps2 F6) - #1565
Open
easonLiangWorldedtech wants to merge 439 commits into
Open
easonLiangWorldedtech wants to merge 439 commits into
easonLiangWorldedtech wants to merge 439 commits into
Conversation
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.
Draft PR — vps2 unit F6 (e2e view-state suite + fresh-tab API surface).
Tracking: fork issue 41 (vps2 series ledger). Upstream issue: #1564 (this series' gap record; the review lineage is #929, A5 re-review minor 2). Port source: the CS worktree apps/vscode-e2e (e9a44b2, the A5 re-review state) — 3 files byte-identical to CS; the reload test is F6-authored.
What this unit does at head 56264d1
Budget
Unit delta vs the stack base stays within budget (the e2e port is 4 files, +543/-0; the review-fix layer is production-hardening carried from the pre-merge review). The PR diff vs upstream/main is larger because the branch is stacked on the unmerged F1-F5 series; the series merges bottom-up, at which point each PR's CI gate measures its own unit. The CI mutation-diff changed-line advisory is a by-construction artifact of the stacked diff.
How to test
pnpm --dir apps/vscode-e2e test(mocked e2e) — the "Roo Code View State" suite incl. the sidebar reload/rehydration test.vitest run src/core/webview/__tests__/ClineProvider.parallelMode.spec.ts— pruning, retention, profile-mutation and multi-instance describes.vitest run src/activate/__tests__/registerCommands.spec.ts— fresh-tab creation, tracked-tab reuse, init-failure cleanup, panel title test.vitest run src/extension/__tests__/api-task-control.spec.ts— startNewTask fresh-tab wiring without evicting a tracked tab's task.Verification numbers (head 56264d1)
Updated this round (b2333b1 -> d5a5895)
Mutation gate (carried)
Series mechanics
Pre-merge ERROR-row pre-check (head 599844c)
setViewStateIdnow rejects non-string ids (crafted postMessage), bounds the key length (MAX_VIEW_STATE_ID_LENGTH), and refuses ids already owned by another live instance; a refused view stays on its temporary id against shared state. Regression tests: non-string, oversized, claimed-by-another-view. Negative controls: disabling the ownership check reddens exactly the claim test; widening the length bound reddens exactly the oversized test; restores byte-exact (hash match).viewStateReadiness, so API-driven task creation never falls through to its timeout when a registration is refused._saveViewLocalStateFromMutationpersists the non-secret subset before updating the buffer; the re-key is a single queued write (no delete-then-write window; authorship clears only after the write lands); the mutation hook keeps the buffer synced across setValue/setValues and profile upsert/activation/deletion.--max-warnings=0clean; tsc clean except the two known local telemetry-dist noise lines; prettier merge-ref gate ALL-MATCHED.upsertProviderProfilewithactivate=false, and the zoo-gateway callback's non-active path) rewrote the profile on disk but left pinned views serving the pre-save snapshot; the non-activating branch now refreshes pinned sibling views (and this view when it holds the pin), and the callback routes throughupsertProviderProfileso the refresh cannot be forgotten.getState()no longer spreads the pinned overlay over the shared provider settings (a key the profile never defined leaked in from the shared profile: model id, region, tuning). The overlay is a complete snapshot: used in place, with onlyapiProviderfalling back to shared state.--max-warnings=0clean, tsc clean except the two known local telemetry-dist noise lines, prettier merge-ref gate ALL-MATCHED.Follow-up: overlay read from the view buffer (required-lane fix)
The previous round's overlay-completeness change read the overlay from the merged state values, which also carry the raw
apiConfigurationstate key. That key is a partial patch written by shared-store paths, not a view snapshot: with no overlay present and the raw key partial,getState()served only the partial object and dropped the shared provider settings the previous merge produced (the parallel-mode activation tests caught exactly this - the activated profile's model id vanished; required lane red on the units carrying that spec). The overlay is now read from the view buffer itself, where a present value is by contract a complete profile and an absent one means the shared store serves the settings.Proofs: ClineProvider.parallelMode.spec green where present (the six synchronization tests that regressed), ClineProvider.spec green including the overlay-completeness tests, sticky-mode and webviewMessageHandler green; negative control: reverting the overlay to a shallow merge over the shared settings still reddens exactly the leak test; restores byte-exact (hash match); eslint
--max-warnings=0clean; tsc clean except the two known local telemetry-dist noise lines; prettier merge-ref gate ALL-MATCHED.Follow-up: active task handlers rebuilt on non-activating saves (head eb27fac)
A review round on the view-identity unit found that the non-activating-save refresh updated pinned views' buffers but not their active tasks' API handlers, and the posted state prefers the task's own
apiConfiguration: a conversation running in a view pinned to the saved profile kept sending the old token (e.g. after the Zoo Gateway callback rotated it) even though the view's buffer had caught up. The refresh now rebuilds the affected tasks' handlers in the pinned views, and the non-activating branch rebuilds this view's own handler when it holds the pin.Proofs: ClineProvider.spec green (the pinned-sibling refresh test now asserts the active task's handler is rebuilt with the saved settings, not just the private buffer); negative control: dropping the rebuild from the refresh reddens exactly that assertion, restores byte-exact (hash match); parallelMode/sticky-mode/webviewMessageHandler green; eslint
--max-warnings=0clean; tsc clean except the two known local telemetry-dist noise lines; prettier merge-ref gate ALL-MATCHED.Follow-up: test-quality round from the review (head 320fd43)
The review round on the api-wiring unit named four test-quality issues; the ones present in this unit are fixed here: the
viewStatesdescribe block moved out of thealwaysDenyUnapprovedCommandsblock (test names carried the wrong subject), the prune test'stoBeDefined()survival checks replaced with exact-shapetoEqualassertions (they pass even if pruning strips fields), the stale-temporary-entry test's fixed 10 ms wait replaced with awaiting the load itself (the negative assertion could pass vacuously on a slow load), and the readiness-cancel test's fake timers restored in afinallyblock (a failed assertion left fake timers active and hung a later real-timer test).Proofs: global-settings 9/9 where present, ClineProvider.spec green, sticky-mode green where present, api-task-control green where present; negative controls: pruning that strips
updatedAtreddens exactly the prune test, aloadViewStatethat adopts the unowned temporary-id entry reddens exactly the stale-entry test, restores byte-exact (hash match); eslint--max-warnings=0clean (src and types); prettier merge-ref gate ALL-MATCHED.Follow-up: review-round fixes from the series (head b6341af)
A review round on this unit produced three fixes. The view-state readiness bound no longer rejects headless callers whose webview never launched (it only rejects when the webview has launched and readiness failed), and the IPC start-task listener catches rejections since the IPC server does not await async listeners (new headless-fallback and launched-stuck tests; negative control: an unconditional timeout reddens exactly the fallback test). The readiness-cancel test now pins the bound's own timer via fake timers and a zero timer count instead of a global
clearTimeoutspy, and the mode-ordering test asserts the switch has not been called before the pending initialization is released (negative controls: dropping the timer clear / the initialization await reddens exactly the respective assertion).Proofs: Task.spec 190/190, api-task-control 26/26; negative controls: dropping the initialization await reddens exactly the ordering test's in-flight assertion, dropping the readiness timer's clear reddens exactly the timer-count assertion, an unconditional timeout reddens exactly the headless-fallback test, restores byte-exact (hash match); eslint
--max-warnings=0clean; tsc clean except the two known local telemetry-dist noise lines; prettier merge-ref gate ALL-MATCHED.Updated this round (CodeRabbit follow-up, head e27816f)
handleModeSwitchUnlockednow captures the pre-switch task mode with a task-write rollback and reconciles every write it owns (shared mode, durable view pin, task mode, task history) when a failure lands after the task-mode write - including the mode-linked profile activation - sosubmitUserMessagedelivers on the pre-switch mode and profile instead of a half-applied switch; regression test included.taskApiConfigName); regression tests cover both sites.afterEachdisposes the per-test provider before resetting the instance registry; the webview state-payload test renamed.Updated this round (CodeRabbit follow-up 3, head ed668c4)
Updated this round (CodeRabbit follow-up 4, head 03913bf)
Updated this round (CodeRabbit follow-up 5, head 1759c8e)
Updated this round (CodeRabbit follow-up 6, head 9e20cc9)
Updated this round (CodeRabbit follow-up 7, head 786a1ce)
Updated this round (CodeRabbit follow-up 8, head 0e9337f)
Stryker suppression comments removed
Per the AGENTS.md policy (never add Stryker directives; remove them when a file is touched), all
// Stryker disabledirectives were dropped from this PR. Equivalence rationale for the removed suppressions:CR round 2026-10-15 (b)
setValuesmode validation (identical flagged block; regression test added) so the units stay byte-identical at merge.CR round 2026-10-15 (c)
getConfigurationkeeps the sharedapiProvideras a fallback base when a pinned view-local profile carries no provider (matchinggetState()); all other shared provider-setting keys stay stripped. Regression test added; the identical block in fix(api): wire task controls and sidebar-targeted configuration (vps2 F5) #1557/fix(webview): show modes skeleton until view state loads (vps2 F7) #1567 received the same fix.finallypath indispose()and the tab-init failure cleanup nests the dispose await so a cleanup rejection cannot replace the initialization error (same fix as fix(webview): send stable view-state id on launch and re-pin per-view state #1552; regression tests added).CR round 2026-10-15 (d)
apiProvider(identical flagged block; regression test added).CR round 2026-10-15 (e)
ClineProvider.dispose()now covers the whole disposal operation with the deregistrationfinally, so a throw in any cleanup step can no longer leave the provider registered while_disposedblocks a retry; regression test added. Same fix in fix(api): wire task controls and sidebar-targeted configuration (vps2 F5) #1557/fix(webview): show modes skeleton until view state loads (vps2 F7) #1567 (identicaldispose()body).createTaskrestores the previous mode selection when custom-mode registration rejects (identical flagged block; regression test added).CR round 2026-10-15 (f)
Row-9 update (head 44d7a56)
setGlobalStatepersistsparsed.datainstead of the raw payload: non-strict schemas strip unknown fields only in the parsed value, so credential-shaped extras cannot reach durable global state (CR Security Boundaries).ContextProxy#updateGlobalStatelayer on this unit; the new tests pin that guard.Verification: new tests are focused negative controls (reverting each guard reddens exactly the matching test); ClineProvider.spec, webviewMessageHandler.spec spot-checks, api specs, spec-inclusive tsc --noEmit, eslint --max-warnings=0 (suppression counts unchanged) and prettier all green on this head.
Upstream refresh (head 44d7a56)
Merged the current
upstream/main(merge commit; the row-9 fix commit is unchanged and is the first parent) to clear the review gate's mergeability hold. Upstream refactoredupsertProviderProfileinto a newpersistProviderProfilemethod and added arunToCompletionqueue option; this unit's transactional body was ported into the new method unchanged and upstream'sPromise.allSettledactivation fix was adopted in it. ClineProvider.spec, webviewMessageHandler.spec,tsc --noEmit, eslint (suppression counts unchanged) and prettier re-verified green at this head. The first refreshed head's windows unit-test lane caught that upstream's newupdateProfileModeltests inClineProvider.apiHandlerRebuild.spec.tsmock the settings manager with a staticgetProfile, which starves the post-save read-back; the mock now repointsgetProfileat whatsaveConfigwrote (the real manager's behavior), and the file's 38 tests pass locally alongside the rest of the verification set.