Skip to content

Commit ec54bce

Browse files
easonLiangWorldedtecheasonLiangWorldedtech
committed
fix(webview): roll back custom-mode registrations that fail partway through a batch
createTask captured the pre-batch snapshot of the mode slugs it was about to install and registered the batch's modes before the try block, so a registration that threw halfway (mode 2 of 3) escaped to the outer catch and none of the rollback ran: mode 1 stayed installed, either having overwritten a saved definition or added a slug the batch never finished using. The registration loop now runs inside the try, ahead of setValues, so the existing rollback - restore the captured definitions, remove the slugs this batch introduced, rethrow - covers it. (src/core/webview/ClineProvider.ts: the registration block was at 5013-5016 outside the try at 5019; it now sits at 5015-5021 inside it. 6253 -> 6255 lines.) Test: a new case in the buffer/rollback describe hands createTask two modes - one whose slug already exists under a different definition, one brand new - and rejects the second registration. It asserts the whole call sequence, so the restore cannot be confused with the registration: existing-mode/Overridden, new-mode/New, existing-mode/Existing, then that the new slug was removed through removeCustomModeEntry (never the saved slug) and that setValues never ran. Before the change the sequence stops at two entries. The pre-existing sibling case ("should remove a custom mode the batch registered when the batch fails") had the same blind spot: it re-registered the identical object it had snapshotted, so its restore assertion was satisfied by the registration call. It now hands in an overridden definition and asserts against the snapshot, which is the assertion it was meant to make. Strengthened, not weakened: emptying the rollback restore loop now fails it where it previously passed. Controls (ClineProvider.ts snapshotted at cafa6bc and restored by content hash after each): hoisting the registration back above the try fails the new case only (expected [ ...2 ] to deeply equal [ ...3 ]); moving it below setValues fails the new case through the setValuesSpy guard; emptying the restore loop fails both rollback cases; emptying the removal loop fails both through the removeCustomModeEntry guard. Gates: prettier --check clean; tsc --noEmit -p . clean; eslint --max-warnings=0 clean on both files; vitest core/webview + core/config 44 files / 1285 tests green (1284 before, +1 case). Scope unchanged at 751 extension-package executable lines (types 8): the change moves existing lines and adds coverage only.
1 parent 877ea0a commit ec54bce

2 files changed

Lines changed: 62 additions & 10 deletions

File tree

‎src/core/webview/ClineProvider.ts‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5008,15 +5008,17 @@ export class ClineProvider
50085008
)
50095009
: []
50105010

5011-
// Register the batch's custom modes before setValues validates `mode`: a mode
5012-
// selected in the same configuration is not known to the manager yet.
5013-
if (configuration.customModes?.length) {
5014-
for (const mode of configuration.customModes) {
5015-
await this.customModesManager.updateCustomMode(mode.slug, mode)
5016-
}
5017-
}
5011+
// Registration lives inside the try below: a mode whose write fails partway through
5012+
// the batch must trigger the same rollback as a failure after setValues.
50185013

50195014
try {
5015+
// Register the batch's custom modes before setValues validates `mode`: a mode
5016+
// selected in the same configuration is not known to the manager yet.
5017+
if (configuration.customModes?.length) {
5018+
for (const mode of configuration.customModes) {
5019+
await this.customModesManager.updateCustomMode(mode.slug, mode)
5020+
}
5021+
}
50205022
await this.setValues(configuration)
50215023

50225024
if (configuration.allowedCommands) {

‎src/core/webview/__tests__/ClineProvider.spec.ts‎

Lines changed: 53 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6949,11 +6949,14 @@ describe("ClineProvider", () => {
69496949
new ContextProxy(mockContext),
69506950
new WebviewFocusTracker(),
69516951
)
6952-
const existingMode = { slug: "existing-mode", name: "Existing", roleDefinition: "r", groups: ["read" as const] }
6952+
const savedMode = { slug: "existing-mode", name: "Existing", roleDefinition: "r", groups: ["read" as const] }
6953+
// The batch hands in a different definition for the slug that already exists, so the restore
6954+
// call below cannot be satisfied by the registration call that preceded it.
6955+
const existingMode = { ...savedMode, name: "Overridden", roleDefinition: "changed" }
69536956
const newMode = { slug: "new-mode", name: "New", roleDefinition: "r", groups: ["read" as const] }
69546957
const manager = provider["customModesManager"]
69556958
Object.assign(manager, {
6956-
getCustomModes: vi.fn().mockResolvedValue([existingMode]),
6959+
getCustomModes: vi.fn().mockResolvedValue([savedMode]),
69576960
updateCustomMode: vi.fn().mockResolvedValue(undefined),
69586961
removeCustomModeEntry: vi.fn().mockResolvedValue(true),
69596962
})
@@ -6965,12 +6968,59 @@ describe("ClineProvider", () => {
69656968

69666969
// The slug that already existed is restored to its earlier definition; the slug this batch
69676970
// introduced is removed, and only through the rules-folder-safe path.
6968-
expect(manager.updateCustomMode).toHaveBeenCalledWith("existing-mode", existingMode)
6971+
expect(manager.updateCustomMode).toHaveBeenCalledWith("existing-mode", savedMode)
69696972
expect(manager.removeCustomModeEntry).toHaveBeenCalledWith("new-mode")
69706973
expect(manager.removeCustomModeEntry).not.toHaveBeenCalledWith("existing-mode")
69716974

69726975
await provider.dispose()
69736976
})
6977+
it("should roll back the modes a registration left half-installed", async () => {
6978+
const provider = new ClineProvider(
6979+
mockContext,
6980+
mockOutputChannel,
6981+
"sidebar",
6982+
new ContextProxy(mockContext),
6983+
new WebviewFocusTracker(),
6984+
)
6985+
const savedMode = { slug: "existing-mode", name: "Existing", roleDefinition: "r", groups: ["read" as const] }
6986+
// The batch re-registers the slug that already exists under a different definition, so a
6987+
// restore call is distinguishable from the registration call.
6988+
const overriddenMode = { ...savedMode, name: "Overridden", roleDefinition: "changed" }
6989+
const newMode = { slug: "new-mode", name: "New", roleDefinition: "r", groups: ["read" as const] }
6990+
const calls: Array<[string, string]> = []
6991+
const manager = provider["customModesManager"]
6992+
Object.assign(manager, {
6993+
getCustomModes: vi.fn().mockResolvedValue([savedMode]),
6994+
updateCustomMode: vi.fn((slug: string, mode: typeof savedMode) => {
6995+
calls.push([slug, mode.name])
6996+
// The batch's second registration is the one that throws.
6997+
if (slug === "new-mode") {
6998+
return Promise.reject(new Error("mode registration failed"))
6999+
}
7000+
return Promise.resolve(undefined)
7001+
}),
7002+
removeCustomModeEntry: vi.fn().mockResolvedValue(true),
7003+
})
7004+
const setValuesSpy = vi.spyOn(provider, "setValues").mockResolvedValue(undefined)
7005+
7006+
await expect(
7007+
provider.createTask("text", undefined, undefined, {}, { customModes: [overriddenMode, newMode] }),
7008+
).rejects.toThrow("mode registration failed")
7009+
7010+
// Registration order first, then the rollback: the slug that already existed is rewritten to
7011+
// the snapshot taken before the batch, and the slug this batch introduced is removed through
7012+
// the rules-folder-safe path. Nothing downstream of the failed registration runs.
7013+
expect(calls).toEqual([
7014+
["existing-mode", "Overridden"],
7015+
["new-mode", "New"],
7016+
["existing-mode", "Existing"],
7017+
])
7018+
expect(manager.removeCustomModeEntry).toHaveBeenCalledWith("new-mode")
7019+
expect(manager.removeCustomModeEntry).not.toHaveBeenCalledWith("existing-mode")
7020+
expect(setValuesSpy).not.toHaveBeenCalled()
7021+
7022+
await provider.dispose()
7023+
})
69747024

69757025
it("should keep a prototype key carried by a payload out of the rollback write", async () => {
69767026
const provider = new ClineProvider(

0 commit comments

Comments
 (0)