test(webview): ClineProvider parallelMode suite with viewStates pruning edges

[easonLiangWorldedtech]

Part of the vps2 durable per-view state series — tracked in easonLiangWorldedtech#41.

Unit F2c of the F2 split; supersedes #1555. Stacked on F2b. Content source of record: kind: commit, base 8554307ec -> head bff2a5ca8 (PR #1555).

Budget: 739 a+d standalone — over the soft 400 cap, under the hard 1000 cap. Rationale: one cohesive regression suite whose ~588 lines are shared mock setup for three describes; splitting the file would duplicate the setup rather than reduce what a reviewer reads.

Related GitHub Issue

Closes: easonLiangWorldedtech#41

Description

How this unit implements durable per-view state, and the choices reviewers should check:

• Per-view identity. Each ClineProvider starts with a host-generated temporary id (sidebar-3) and, on webviewDidLaunch, adopts the stable id the webview persists through acquireVsCodeApi().setState() (webview-ui/src/utils/vscode.ts). The id is sanitized to [A-Za-z0-9_-] and __proto__ is rejected so it stays a safe object key in the shared map.
• Durable viewStates map. mode and currentApiConfigName are stored per view id under the new viewStates global-state key (packages/types/src/global-settings.ts). Every write goes through one serialized write queue that re-reads the map fresh and merges into the existing entry, so concurrent views cannot clobber each other; entries with nothing persistable are deleted and stale entries are pruned.
• Re-keying. Writes captured while the provider still holds its temporary id persist under that id and are re-keyed to the stable id when the webview registers one, so a change made before the launch message stays durable. A failed registration restores the previous id so a later launch retries.
• Overlay composition in getState(). The view-local buffer is layered over the shared ContextProxy values; fields mutated while the async profile lookup is in flight are re-applied field by field, so a slow load cannot clobber a newer user selection.
• Cache/storage atomicity. ContextProxy#updateGlobalState fills the in-memory cache before awaiting globalState.update, so the three viewStates writes now go through writePersistedViewStates, which restores the previously cached map when the storage write rejects instead of leaving other views reading an uncommitted map.
• viewStates is excluded from settings import/export so a per-window view map is never transferred between machines.
• Title-bar command routing (src/activate/registerCommands.ts): title-bar commands target the click-origin instance, and overlapping openClineInNewTab calls are serialized.

Test Procedure

pnpm --dir src vitest run core/webview core/config core/task activate
pnpm --dir webview-ui vitest run src/utils/__tests__/vscode.spec.ts src/context/__tests__/ExtensionStateContext.spec.tsx

• ClineProvider.parallelMode.spec.ts — multi-tab isolation: two views with different ids keep separate mode, profile pin and apiConfiguration.
• ClineProvider.spec.ts → setViewStateId, persisted view state pruning, view state persistence edge cases — merge/clear/prune semantics, corrupted storage values treated as an empty map, re-keying of pre-launch writes, and the cache rollback when the storage write rejects.
• ClineProvider.sticky-mode.spec.ts / sticky-profile.spec.ts — a view's pinned profile survives a mode switch and a sibling view's change.
• vscode.spec.ts / ExtensionStateContext.spec.tsx — the webview reuses its persisted id or mints a new one, and posts it in webviewDidLaunch.
• Each fix in this branch was verified with a negative control: the production change was reverted in isolation and the corresponding test confirmed to fail.

Pre-Submission Checklist

• Issue Linked: linked to the approved tracking issue above.
• Scope: per-view durable state only (no settings-UI or task-lifecycle changes).
• Self-Review: performed.
• Testing: new suites plus per-fix regression tests.
• Visual Snapshot (UI changes only): N/A — no user-visible rendered state changes; the webview diff is the launch handshake only.
• Documentation Impact: considered; no user-facing documentation change is required (the setting is internal state, not a user control).
• Contribution Guidelines: read and agreed.

Documentation Updates

• No documentation updates are required.

Additional Notes

mutation-diff reports more than the 500 changed-executable-line cap for this series; the remedy is tracked on easonLiangWorldedtech#41 (maintainer-side cap decision), not another split.

[easonLiangWorldedtech]

Pushed 2c7eb07 — Security Boundaries + Persistence Integrity

Security Boundaries (the ❌ row on c6833ee)

setViewStateId took the identifier straight off the webview channel and used it as a key in the shared viewStates map. It is now validated as untrusted input:

gate	behaviour
type	a non-string value is logged and ignored — a crafted postMessage is JSON, so the declared viewStateId?: string is not enforced at the boundary
length	bounded by MAX_VIEW_STATE_ID_LENGTH (64) — the webview generates a UUID, so a far longer value is not a view id, and an unbounded key is unbounded growth in the shared map
ownership	an id another live ClineProvider already registered is refused; the view keeps its temporary id

Why ownership rather than host-side generation: the id has to survive a webview reload, which recreates the view on both sides, so the host cannot mint an id that persists across recreation — that is the F1c design in #41. A webview can already request any profile through the activateProviderProfile message path, so the id was never an authentication boundary. The boundary that is real is one live view adopting another live view's persisted key: it would load that view's pinned profile, nested overlay and mode into this provider, and this webview could then overwrite that view's entry. getAllInstances() claim check closes exactly that, and the view still works against the shared state on its temporary id.

Persistence Integrity (the second ❌ row)

#1921 is not cumulative with #1919, so the compensating deleteProviderProfile is ported here: pre-delete settings read (abort before the destructive commit unless it is the typed not-found), snapshots of profile list / shared selection / view pin / nested overlay, per-store rollback each awaited on its own, and a distinct Profile deletion left persisted state inconsistent: … error when the rollback itself fails. rePinViewLocalStateForDeletedProfile restores each sibling's own pin before rethrowing. The typed ProviderSettingsNotFoundError out of getProfile (the dependency the compensation branches on) is ported with it.

Tests + negative controls

New: refuses a viewStateId that another live view already owns, refuses an oversized viewStateId instead of using it as a persisted key, ignores a non-string viewStateId arriving from a crafted message, the three delete-profile compensation tests, and rejects a missing profile lookup with the typed not-found error. Three existing reload tests now dispose the writing provider before the new one re-registers the id — that is what a reload actually does, and it is the case the claim check must keep allowing.

mutation	result
claim predicate → false	foreign-claim test FAILS (viewStateId becomes owned-by-first-view)
length cap → false	oversized test FAILS (200-char key adopted)
typeof viewStateId !== "string" → false	non-string test FAILS (TypeError: viewStateId.trim is not a function)
manager typed-throw reverted	typed not-found test FAILS

Local: core/webview + core/config 972 passed; tsc --noEmit 100 errors (= local baseline, stale built @roo-code/types dist — CI compile is green); eslint --prune-suppressions --max-warnings=0 clean on all four files, no suppression delta.

No @coderabbitai full review requested — the content push triggers the at-head review.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@easonLiangWorldedtech Thanks for the detailed update and guidance.

You report that 2c7eb07 adds:

• Runtime type validation, a 64-character limit, and live-view ownership checks for setViewStateId.
• Compensating rollback for deleteProviderProfile, with a distinct error when rollback fails.
• Regression tests and negative controls for these changes.

The ownership check addresses live-view state isolation, not profile authorization. Reload tests must release the previous provider’s claim before registering the replacement provider.

I have not independently verified this head or the reported test results. I will not trigger a separate full review, as requested.

---

✏️ Learnings added

Learnt from: easonLiangWorldedtech
URL: https://github.com/Zoo-Code-Org/Zoo-Code/pull/1921

Timestamp: 2026-10-08T17:15:18.881Z
Learning: In Zoo-Code-Org/Zoo-Code, ClineProvider viewStateId ownership is a live-view state-isolation boundary, not a profile-authorization boundary: webviews can already request profiles through activateProviderProfile. The intended setViewStateId contract rejects non-string inputs, identifiers longer than MAX_VIEW_STATE_ID_LENGTH (64), and identifiers claimed by another live ClineProvider. Reload lifecycle tests dispose the previous provider before a replacement provider registers the same persisted identifier.

---

Learnt from: easonLiangWorldedtech
URL: https://github.com/Zoo-Code-Org/Zoo-Code/pull/1921

Timestamp: 2026-10-08T17:15:18.881Z
Learning: Zoo-Code-Org/Zoo-Code PR #1921 is not cumulative with PR #1919. Profile-deletion compensation implemented in #1919 must also be present in #1921; do not assume that #1921 inherits those changes.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

---

🧠 Learnings used

Learnt from: easonLiangWorldedtech
Repo: Zoo-Code-Org/Zoo-Code

Timestamp: 2026-10-08T11:15:24.502Z
Learning: In src/core/webview/ClineProvider.ts, viewStates rollback must preserve an absent storage key as undefined. Read the rollback target directly with context.globalState.get<GlobalState["viewStates"]>("viewStates"); getPersistedViewStates({ fresh: true }) normalizes an absent key to {}, which violates the persistence-failure contract. ContextProxy.setValue updates the cache before awaiting storage, so rollback must use the storage value rather than a potentially stale cached value.

---

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

You are interacting with an AI system.

[easonLiangWorldedtech]

Pushed 657c109 (non-cumulative port of #1919 420a831) — ⚠️ Lifecycle Resource Cleanup (createTabPanelUnlocked)

Confirmed: the body constructs ClineProvider and then runs eight more steps (newGroupRight, createWebviewPanel, setPanel, iconPath, resolveWebviewView, the two panel listeners, delay(100) + lockEditorGroup) with no cleanup. Any of those awaits can reject, and the retry path added earlier makes the failure reachable repeatedly. What survived:

• the constructed provider stays in ClineProvider.activeInstances with its listeners attached (duplicate provider work, and it counts as a visible instance for getVisibleInstance heuristics);
• setPanel(newPanel, "tab") may already have run, so tabPanel points at a panel whose provider never resolved — the next "Open in editor" finds no live provider for it, drops the ref and rebuilds, i.e. the leak is paid again on every retry.

Fix as requested: the post-construction body now runs inside a try, and disposeFailedTabCreation disposes the provider, clears the tracked ref only while it still names this panel, and disposes the panel. Each step is awaited in its own try/catch so one failing cleanup cannot strand the other, and an incomplete cleanup is logged as its own line instead of hiding behind the original error, which is still what the caller sees.

Tests: "disposes the provider and the panel when a creation step rejects" (the last step, lockEditorGroup, rejects — provider constructed, panel created and tracked, view resolved) and "reports incomplete cleanup when the tab-creation rollback itself fails" (provider dispose rejects and panel dispose throws -> the incomplete-cleanup line, both steps still attempted).

Negative control: removing the disposeFailedTabCreation call fails both tests (dispose never called); removing only the tracked-ref clear fails the first on getPanel() still returning the orphan panel. Reverted byte-for-byte and re-passed.

git diff -w shows the real delta (+52/-1 in registerCommands.ts); the rest of the diff is the one-tab re-indentation the try requires.

Local: core/webview + core/config + activate = 1084 passed; tsc --noEmit 100 errors (= local baseline); eslint --prune-suppressions --max-warnings=0 clean on both files. src/activate/registerCommands.ts is byte-identical to the #1919 branch version; the hunk review against c4e38b9 shows only the helper, the try/catch and the re-indentation — nothing foreign.

[easonLiangWorldedtech]

Pushed c788f52 (port of #1919 027c502) — ❌ Persistence Integrity (cancelled profile deletion vs. the next queued mutation)

Half confirmed, half argued.

Confirmed: the overlap is real, but the fix is not where the check put it. deleteProviderProfileUnlocked now checks the AbortSignal once more, immediately before the selection/settings rewrite (the shared selection, the shared provider settings, and other views' pins), and the catch skips compensation entirely once the signal is aborted. A timed-out operation has already handed the queue turn to the next mutation, so replaying its rollback would overwrite that mutation's writes - which is the corruption the row describes, just reached through the rollback rather than through the forward writes. Both paths log what state was left behind, so a partial deletion is reported instead of silently repaired.

Argued - do not advance the queue from the unbounded run promise. this.providerProfileMutationQueue = run.then(...) makes one hung durable write (deleteConfig never settling) block every later profile mutation for the lifetime of the extension host: no caller can delete, upsert, activate, or switch mode, and no caller is waiting for the hung one. Advancing on the timeout-bounded callerResult is deliberate; the invariant that makes it safe is that the cancelled operation stops touching durable state at the cancellation point, which is exactly what the added guards enforce. The row's own resolution keeps the caller timeout; the difference is only who owns the stores afterwards.

Test: "stops writing durable state once the deletion's abort signal fires" - the cancellation lands on the survivor lookup, i.e. after the settings commit and the profile-list write. Asserts the list write landed, the shared selection and provider settings were not rewritten, and no compensation write happened (saveConfig never called).

Negative controls: removing the pre-rewrite guard lets the deletion run to completion (the rejection assertion fails); removing the compensation skip replays the profile list (the list no longer equals [keeper]). Both reverted byte-for-byte and re-passed.

This branch was further behind than expected: deleteProviderProfile here still ran unserialized against upsert/activation/mode-switch, so the port also brings the queue wrapper and the two queue-contract abort checks. git diff is 5 hunks / +51 lines, all additions - reviewed hunk-by-hunk, nothing foreign. Local: 1085 passed; tsc --noEmit 100 errors (= baseline); eslint clean.

[easonLiangWorldedtech]

Pushed 55aba85 (Security Boundaries) and 60546d0 (Lifecycle) — non-cumulative ports of #1919 59f4492 / 9e47e3e — ❌ Security Boundaries + ⚠️ Lifecycle Resource Cleanup

❌ Security Boundaries — confirmed, fixed

The generic settings loop is the problem, not the profile paths: it iterates Object.entries(message.updatedSettings) and calls provider.setValue(key, value) for every key, and since the per-view buffer landed that write also updates this view's local overlay - which getState() prefers over the shared values. Nothing validated the runtime shape, so the compile-time type was the only thing standing between a webview payload and apiConfiguration. (Worth noting for the record: apiConfiguration is not even a RooCodeSettings key, so the crafted payload the row describes is exactly the case the type cannot express.)

HOST_OWNED_SETTINGS = apiConfiguration, listApiConfigMeta, viewStates is now rejected at the top of the loop with its own log line. Those keys are produced by validated profile-management paths - ProviderSettingsManager resolves and validates a configuration before it can become active - and ClineProvider owns the per-view durable map, so the generic settings route must not carry them into either sink.

Test: "refuses to apply profile- and provider-owned keys supplied by the webview" - one payload with a legitimate setting plus all three host-owned keys; the ordinary setting still goes through provider.setValue, none of the three reach setValue or contextProxy.setValue, and the refusal is logged. Negative control: removing the guard fails the apiConfiguration assertion (the injected configuration is applied). Reverted byte-for-byte and re-passed.

⚠️ Lifecycle Resource Cleanup — confirmed, fixed

Exactly as described: dispose() set _disposed, ran a chain of awaited teardown steps, and removed the provider from activeInstances only on its last line. A rejecting mcpHub.unregisterClient() / skillsManager.dispose() / marketplaceManager.cleanup() skipped the unregistration, removeAllListeners() and McpServerManager.unregisterProvider(this), leaving a provider discoverable through getVisibleInstance / getAllInstances / getInstanceForView with its view and listeners attached - and _disposed meant nothing would ever retry. The tab-creation rollback added earlier disposes providers, so this is on a reachable path, not only on shutdown.

Each teardown step now runs through attemptCleanup, which records its own failure and lets the rest proceed; the unregistration is unconditional afterwards, and an incomplete teardown is reported as its own line (Disposal was incomplete (mcpHub: ...; marketplace manager: ...); the provider was unregistered anyway.).

Test: "unregisters a provider whose teardown step rejects" - mcpHub.unregisterClient rejects and marketplaceManager.cleanup throws; asserts dispose() does not crash, the provider is gone from getAllInstances() and no longer resolvable for its own view, and the incomplete-teardown line names both failures. Negative control: making attemptCleanup propagate makes dispose() throw at the first failing step and the provider stays registered. Reverted byte-for-byte and re-passed.

On the helper half of the resolution: disposeFailedTabCreation already clears the tracked ref between two independent try/catch blocks, so the ref clear is unconditional and the original creation error is what the caller sees; the incomplete-cleanup line is separate. No change needed there.

Local: core/webview + core/config + activate = 1087 passed; tsc --noEmit 100 errors (= baseline); eslint clean. Port diffs are identical to #1919 (+62 and +67/-8), reviewed hunk-by-hunk.