fix(vscode): persist skill disable to SKILL.md frontmatter so the model honors it (ENG-1995) (#11294)

* fix(vscode): persist skill disable to SKILL.md frontmatter so the model honors it (ENG-1995)

The VS Code skill toggle only updated extension state (globalSkillsToggles /
localSkillsToggles), but the SDK builds the model's skill list and the `skills`
tool from each SKILL.md's frontmatter `disabled` flag. As a result, disabling a
skill in the sidebar left it fully available to the model, including in new
tasks.

toggleSkill now also writes the `disabled` flag to the skill's SKILL.md
frontmatter (no-op for remote skills, which have no backing file), via new
helpers updateSkillMarkdownDisabledState / setSkillDisabledInFrontmatter in
skills.ts. Adds unit tests for both helpers.

* fix(vscode): don't rewrite skills with malformed frontmatter (ENG-1995)

parseYamlFrontmatter fails open on invalid YAML, returning the full original
document as the body. updateSkillMarkdownDisabledState would then prepend a
second `---` block on a disable, corrupting the file. Bail out and leave the
file untouched when frontmatter fails to parse. Adds tests for the malformed
disable/enable cases.

Addresses Greptile review feedback on #11294.

* test(vscode): assert malformed-skill fixture is actually invalid YAML (ENG-1995)

Add a guard test that parseYamlFrontmatter reports hadFrontmatter and a
parseError for the shared malformed fixture, so the two "leave file untouched"
tests can't silently pass via a different code path if the fixture ever became
valid YAML.

Addresses Greptile review feedback on #11294.

* fix(vscode): resolve @cline/shared/storage subpath in mocha unit-test compile

The CommonJS mocha unit-test runner uses classic "node" moduleResolution,
which does not read the `exports` subpath maps in @cline/* package
manifests, so `@cline/shared/storage` (imported by
src/sdk/telemetry-settings-sync.ts) failed with TS2307 when test files
transitively reach the SDK adapter. Mirror the explicit paths mapping
already added to tsconfig.test.json for the integration-test compile.

* fix(vscode): restore E2E mock auth in SDK auth service so e2e tests can sign in

The SDK migration replaced classic AuthService (which swapped in
AuthServiceMock under E2E_TEST) with sdk/auth-service.ts, losing the
mock path. "Login to Cline" then invoked the real SDK OAuth flow and
opened a native browser dialog the Playwright tests cannot interact
with, so helper.signin() never authenticated and chat.test.ts +
diff.test.ts failed on every platform (the failures also reproduce on
the base branch).

- auth-service.ts: under E2E_TEST=true (and CLINE_ENVIRONMENT=local),
  exchange the well-known test code with the local mock API server and
  persist credentials to providers.json — no browser. Replaces classic
  AuthServiceMock (see origin/main src/services/auth/AuthServiceMock.ts).
- chat.test.ts/diff.test.ts: wait for the mock turn to complete before
  clicking New Task; SDK history is persisted at turn end, so navigating
  mid-turn races the write and Recent never shows.
- diff.test.ts: the footer Start New Task button only appears for
  attempt_completion turns under SDK TurnState; use the header New Task
  button like chat.test.ts.

apps/vscode/src/core/context/instructions/user-instructions/__tests__/skills.test.ts

@@ -12,7 +12,15 @@ import * as sinon from "sinon"
 import * as skillDirectories from "@/core/storage/skill-directories"
 import { Logger } from "@/shared/services/Logger"
 import * as fsUtils from "@/utils/fs"
-import { discoverSkills, getAvailableSkills, getSkillContent, parseRemoteSkillEntries } from "../skills"
+import { parseYamlFrontmatter } from "../frontmatter"
+import {
+	discoverSkills,
+	getAvailableSkills,
+	getSkillContent,
+	parseRemoteSkillEntries,
+	setSkillDisabledInFrontmatter,
+	updateSkillMarkdownDisabledState,
+} from "../skills"
 
 describe("Skills Utility Functions", () => {
 	let sandbox: sinon.SinonSandbox
@@ -656,3 +664,119 @@ description: Test
 		})
 	})
 })
+
+describe("updateSkillMarkdownDisabledState", () => {
+	it("adds disabled: true when disabling a skill with existing frontmatter", () => {
+		const input = ["---", "name: my-skill", "description: A skill", "---", "Body here"].join("\n")
+		const output = updateSkillMarkdownDisabledState(input, false)
+		expect(output).to.contain("disabled: true")
+		expect(output).to.contain("name: my-skill")
+		expect(output).to.contain("Body here")
+	})
+
+	it("removes disabled flag when enabling a previously-disabled skill", () => {
+		const input = ["---", "name: my-skill", "description: A skill", "disabled: true", "---", "Body"].join("\n")
+		const output = updateSkillMarkdownDisabledState(input, true)
+		expect(output).to.not.contain("disabled")
+		expect(output).to.contain("name: my-skill")
+		expect(output).to.contain("Body")
+	})
+
+	it("also clears a stale enabled: false when enabling", () => {
+		const input = ["---", "name: my-skill", "enabled: false", "---", "Body"].join("\n")
+		const output = updateSkillMarkdownDisabledState(input, true)
+		expect(output).to.not.contain("enabled: false")
+	})
+
+	it("drops the frontmatter block entirely when enabling leaves it empty", () => {
+		const input = ["---", "disabled: true", "---", "Body only"].join("\n")
+		const output = updateSkillMarkdownDisabledState(input, true)
+		expect(output).to.equal("Body only")
+	})
+
+	it("returns content unchanged when enabling a doc with no frontmatter", () => {
+		const input = "Just body, no frontmatter"
+		expect(updateSkillMarkdownDisabledState(input, true)).to.equal(input)
+	})
+
+	it("is idempotent: disabling an already-disabled skill keeps disabled: true once", () => {
+		const input = ["---", "name: s", "disabled: true", "---", "B"].join("\n")
+		const output = updateSkillMarkdownDisabledState(input, false)
+		expect(output.match(/disabled: true/g)).to.have.lengthOf(1)
+	})
+
+	// Frontmatter block whose YAML is genuinely invalid (asserted below). The
+	// `---` markers are well-formed so parseYamlFrontmatter detects frontmatter
+	// and then fails to parse it, exercising the parseError branch.
+	const MALFORMED_FRONTMATTER = ["---", "name: s", "description: : : bad", "  - nope", "---", "Body"].join("\n")
+
+	it("uses a fixture whose frontmatter YAML is actually invalid", () => {
+		// Guards the two tests below from rotting into false positives: if this
+		// fixture ever became valid YAML, updateSkillMarkdownDisabledState would
+		// take a different (rewriting) path and the "untouched" assertions could
+		// pass for the wrong reason.
+		const parsed = parseYamlFrontmatter(MALFORMED_FRONTMATTER)
+		expect(parsed.hadFrontmatter).to.be.true
+		expect(parsed.parseError, "fixture frontmatter should be invalid YAML").to.be.a("string")
+	})
+
+	it("leaves malformed-frontmatter files untouched when disabling (no double header)", () => {
+		// parseYamlFrontmatter fails open and returns the full original document
+		// as the body. Disabling must not prepend a second `---` block and corrupt
+		// the file.
+		const output = updateSkillMarkdownDisabledState(MALFORMED_FRONTMATTER, false)
+		expect(output).to.equal(MALFORMED_FRONTMATTER)
+		// Exactly one frontmatter opener/closer pair, not two.
+		expect(output.match(/^---$/gm)).to.have.lengthOf(2)
+	})
+
+	it("leaves malformed-frontmatter files untouched when enabling", () => {
+		expect(updateSkillMarkdownDisabledState(MALFORMED_FRONTMATTER, true)).to.equal(MALFORMED_FRONTMATTER)
+	})
+})
+
+describe("setSkillDisabledInFrontmatter", () => {
+	let sandbox: sinon.SinonSandbox
+	let readFileStub: sinon.SinonStub
+	let writeFileStub: sinon.SinonStub
+
+	beforeEach(() => {
+		sandbox = sinon.createSandbox()
+		sandbox.stub(Logger, "warn")
+		readFileStub = sandbox.stub(fs.promises, "readFile")
+		writeFileStub = sandbox.stub(fs.promises, "writeFile").resolves()
+	})
+
+	afterEach(() => sandbox.restore())
+
+	it("writes disabled: true to the SKILL.md when disabling a disk skill", async () => {
+		const skillPath = path.join("/home", "user", ".cline", "skills", "s", "SKILL.md")
+		readFileStub.withArgs(skillPath, "utf-8").resolves(["---", "name: s", "description: d", "---", "Body"].join("\n"))
+
+		const ok = await setSkillDisabledInFrontmatter(skillPath, false)
+
+		expect(ok).to.be.true
+		sinon.assert.calledOnce(writeFileStub)
+		const written = writeFileStub.getCall(0).args[1] as string
+		expect(written).to.contain("disabled: true")
+	})
+
+	it("does not write for remote skills (no backing file)", async () => {
+		const ok = await setSkillDisabledInFrontmatter("remote:Some Skill", false)
+		expect(ok).to.be.false
+		sinon.assert.notCalled(readFileStub)
+		sinon.assert.notCalled(writeFileStub)
+	})
+
+	it("skips the write when content is unchanged", async () => {
+		const skillPath = path.join("/home", "user", ".cline", "skills", "s", "SKILL.md")
+		// Already disabled; disabling again yields identical content.
+		readFileStub.withArgs(skillPath, "utf-8").resolves(["---", "name: s", "disabled: true", "---", "B"].join("\n"))
+
+		const ok = await setSkillDisabledInFrontmatter(skillPath, false)
+
+		expect(ok).to.be.true
+		sinon.assert.notCalled(writeFileStub)
+	})
+})
+

apps/vscode/src/core/context/instructions/user-instructions/skills.ts

@@ -3,10 +3,84 @@ import type { GlobalInstructionsFile } from "@shared/remote-config/schema"
 import type { SkillContent, SkillMetadata } from "@shared/skills"
 import { fileExistsAtPath, isDirectory } from "@utils/fs"
 import * as fs from "fs/promises"
+import * as yaml from "js-yaml"
 import * as path from "path"
 import { Logger } from "@/shared/services/Logger"
 import { parseYamlFrontmatter } from "./frontmatter"
 
+/**
+ * Update the `disabled` frontmatter flag of a SKILL.md document.
+ *
+ * The SDK (which builds the model's skill list and the `skills` tool) reads a
+ * skill's enabled state from the SKILL.md frontmatter `disabled` field, not from
+ * the extension's UI toggle state. Toggling a skill in the VS Code sidebar must
+ * therefore also write this flag so the change is reflected for the model
+ * (ENG-1995). This mirrors the SDK's updateSkillMarkdownEnabledState but lives in
+ * the extension and uses js-yaml (the extension's frontmatter parser).
+ *
+ * - enabled=false → sets `disabled: true`.
+ * - enabled=true  → removes `disabled` (and a stale `enabled: false`), dropping
+ *   the frontmatter block entirely if it becomes empty.
+ *
+ * Returns the original content unchanged when enabling a document that has no
+ * frontmatter (nothing to clear).
+ */
+export function updateSkillMarkdownDisabledState(content: string, enabled: boolean): string {
+	const { data, body, hadFrontmatter, parseError } = parseYamlFrontmatter(content)
+
+	if (!hadFrontmatter && enabled) {
+		return content
+	}
+
+	// parseYamlFrontmatter fails open on malformed YAML: it returns data={} and
+	// body=<full original content> (frontmatter block included). Serializing here
+	// would prepend a second `---` block and corrupt the file, so leave it
+	// untouched and let the user fix the frontmatter.
+	if (parseError) {
+		return content
+	}
+
+	if (enabled) {
+		delete data.disabled
+		if (data.enabled === false) {
+			delete data.enabled
+		}
+		if (Object.keys(data).length === 0) {
+			return body
+		}
+		return serializeSkillFrontmatter(data, body)
+	}
+
+	data.disabled = true
+	return serializeSkillFrontmatter(data, body)
+}
+
+function serializeSkillFrontmatter(data: Record<string, unknown>, body: string): string {
+	const yamlText = yaml.dump(data, { schema: yaml.JSON_SCHEMA }).trimEnd()
+	return `---\n${yamlText}\n---\n${body}`
+}
+
+/**
+ * Persist a skill's enabled/disabled state to its SKILL.md frontmatter on disk.
+ * No-op (returns false) for remote skills, which have no backing file.
+ */
+export async function setSkillDisabledInFrontmatter(skillMdPath: string, enabled: boolean): Promise<boolean> {
+	if (!skillMdPath || skillMdPath.startsWith("remote:")) {
+		return false
+	}
+	try {
+		const content = await fs.readFile(skillMdPath, "utf-8")
+		const updated = updateSkillMarkdownDisabledState(content, enabled)
+		if (updated !== content) {
+			await fs.writeFile(skillMdPath, updated)
+		}
+		return true
+	} catch (error) {
+		Logger.warn(`Failed to update skill frontmatter at ${skillMdPath}:`, error)
+		return false
+	}
+}
+
 /**
  * A remote skill entry after frontmatter validation.
  * name is always frontmatter.name (canonical). A warning is logged if entry.name drifts.

apps/vscode/src/core/controller/file/toggleSkill.ts

@@ -1,3 +1,4 @@
+import { setSkillDisabledInFrontmatter } from "@core/context/instructions/user-instructions/skills"
 import { SkillsToggles, ToggleSkillRequest } from "@shared/proto/cline/file"
 import { Logger } from "@/shared/services/Logger"
 import { Controller } from ".."
@@ -39,6 +40,13 @@ export async function toggleSkill(controller: Controller, request: ToggleSkillRe
 		controller.stateManager.setWorkspaceState("localSkillsToggles", localToggles)
 	}
 
+	// Persist the enabled state to the SKILL.md frontmatter as well. The SDK
+	// builds the model's skill list / `skills` tool from the frontmatter
+	// `disabled` flag, not from the extension's UI toggle state, so without this
+	// write a skill toggled off in the sidebar would still be offered to the
+	// model (ENG-1995). The helper is a no-op for remote skills (no backing file).
+	await setSkillDisabledInFrontmatter(skillPath, enabled)
+
 	await controller.postStateToWebview()
 
 	return SkillsToggles.create({

apps/vscode/src/sdk/auth-service.ts

@@ -460,6 +460,14 @@ export class AuthService {
 			return ProtoString.create({ value: "Already authenticated" })
 		}
 
+		// E2E test mode: authenticate against the local mock API server instead
+		// of loginClineOAuth(), which opens a real browser window the tests can't
+		// interact with. Replaces classic AuthServiceMock (see origin/main
+		// src/services/auth/AuthServiceMock.ts).
+		if (process.env.E2E_TEST === "true") {
+			return this.createMockAuthRequest()
+		}
+
 		let resolveAuthMessage!: (message: string) => void
 		let rejectAuthMessage!: (error: unknown) => void
 		const authMessagePromise = new Promise<string>((resolve, reject) => {
@@ -519,6 +527,72 @@ export class AuthService {
 		return ProtoString.create({ value: await authMessagePromise })
 	}
 
+	/**
+	 * E2E test login: exchange a well-known test code with the local mock API
+	 * server (src/test/e2e/fixtures/server) for tokens, with no browser
+	 * interaction. Replaces classic AuthServiceMock.createAuthRequest().
+	 */
+	private async createMockAuthRequest(): Promise<String> {
+		if (ClineEnv.config().environment !== "local") {
+			throw new Error("E2E mock auth is only available when CLINE_ENVIRONMENT=local")
+		}
+
+		const apiBaseUrl = ClineEnv.config().apiBaseUrl
+		const tokenUrl = new URL(CLINE_API_ENDPOINT.TOKEN_EXCHANGE, apiBaseUrl)
+		const response = await fetch(tokenUrl.toString(), {
+			method: "POST",
+			headers: {
+				"Content-Type": "application/json",
+				...(await buildBasicClineHeaders()),
+			},
+			body: JSON.stringify({
+				code: "test-personal-token",
+				grantType: "authorization_code",
+			}),
+		})
+		if (!response.ok) {
+			throw new Error(`Mock server authentication failed: ${response.status} ${response.statusText}`)
+		}
+		const responseJSON = await response.json()
+		const tokenData = responseJSON?.data
+		if (!responseJSON?.success || !tokenData?.accessToken) {
+			throw new Error("Invalid response from mock server")
+		}
+
+		this._clineAuthInfo = {
+			idToken: tokenData.accessToken,
+			refreshToken: tokenData.refreshToken,
+			expiresAt: new Date(tokenData.expiresAt).getTime() / 1000,
+			userInfo: {
+				id: tokenData.userInfo?.clineUserId || tokenData.userInfo?.subject || "",
+				email: tokenData.userInfo?.email || "",
+				displayName: tokenData.userInfo?.name || "",
+				createdAt: new Date().toISOString(),
+				organizations: tokenData.userInfo?.organizations ?? [],
+				appBaseUrl: ClineEnv.config().appBaseUrl,
+				subject: tokenData.userInfo?.subject,
+			},
+			provider: "cline",
+			startedAt: Date.now(),
+		}
+		this._authenticated = true
+
+		// Persist to providers.json so session config resolution
+		// (resolveApiKey) and provider-usability checks see the credentials.
+		writeClineCredentials({
+			accessToken: tokenData.accessToken,
+			refreshToken: tokenData.refreshToken,
+			expiresAt: new Date(tokenData.expiresAt).getTime(),
+			accountId: this._clineAuthInfo.userInfo.id,
+		})
+
+		await this.sendAuthStatusUpdate()
+		Logger.log(`[SdkAuthService] E2E mock login completed as ${this._clineAuthInfo.userInfo.email}`)
+
+		const { String: ProtoString } = await import("@shared/proto/cline/common")
+		return ProtoString.create({ value: apiBaseUrl })
+	}
+
 	/**
 	 * Initiate OCA OAuth login.
 	 */

apps/vscode/src/test/e2e/chat.test.ts

@@ -13,6 +13,11 @@ e2e("Chat - can send messages and switch between modes", async ({ helper, sideba
 	await sidebar.getByTestId("send-button").click()
 	await expect(inputbox).toHaveValue("")
 
+	// Wait for the (mock) agent turn to finish before navigating away — the task
+	// is persisted to SDK session history when the turn completes, so clicking
+	// "New Task" mid-turn races the history write and "Recent" may not show.
+	await expect(sidebar.getByText("mock Cline API response")).toBeVisible()
+
 	// Starting a new task should clear the current chat view and show the recent tasks
 	await sidebar.getByRole("button", { name: "New Task", exact: true }).first().click()
 	await expect(sidebar.getByText("Recent")).toBeVisible()

apps/vscode/src/test/e2e/diff.test.ts

@@ -16,8 +16,16 @@ e2e.describe("Diff Editor", () => {
 			await sidebar.getByTestId("send-button").click()
 			await expect(inputbox).toHaveValue("")
 
-			// Back to home page with history
-			await sidebar.getByRole("button", { name: "Start New Task" }).click()
+			// Wait for the (mock) agent turn to finish before navigating away —
+			// the task is persisted to SDK session history when the turn completes,
+			// and the mock server delays this response by 500ms.
+			await expect(sidebar.getByText("mock Cline API response")).toBeVisible()
+
+			// Back to home page with history. The turn ends in "awaiting_followup"
+			// (the mock response has no attempt_completion), so the footer shows no
+			// "Start New Task" button — use the header "New Task" button instead,
+			// same as chat.test.ts.
+			await sidebar.getByRole("button", { name: "New Task", exact: true }).first().click()
 			await expect(sidebar.getByText("Recent")).toBeVisible()
 			await expect(sidebar.getByText("Hello, Cline!")).toBeVisible() // History with the previous sent message
 

apps/vscode/tsconfig.unit-test.json

@@ -3,6 +3,50 @@
 	"compilerOptions": {
 		"module": "commonjs",
 		"moduleResolution": "node",
+		// Classic "node" moduleResolution (required for the CommonJS mocha unit-test
+		// runner) does not read the `exports` subpath maps in @cline/* package
+		// manifests, so bare subpath imports like `@cline/shared/storage` fail to
+		// resolve (TS2307) even though the built declarations exist. The base
+		// tsconfig uses `Bundler` resolution and is unaffected. We re-declare the
+		// inherited `paths` (TS replaces, not merges, `paths`) and add explicit
+		// mappings to the built dist so the unit-test compile can resolve these
+		// subpaths. Mirrors the same fix in tsconfig.test.json.
+		"baseUrl": ".",
+		"paths": {
+			"@/*": [
+				"./src/*"
+			],
+			"@api/*": [
+				"./src/core/api/*"
+			],
+			"@core/*": [
+				"./src/core/*"
+			],
+			"@generated/*": [
+				"./src/generated/*"
+			],
+			"@hosts/*": [
+				"./src/hosts/*"
+			],
+			"@integrations/*": [
+				"./src/integrations/*"
+			],
+			"@packages/*": [
+				"./src/packages/*"
+			],
+			"@services/*": [
+				"./src/services/*"
+			],
+			"@shared/*": [
+				"./src/shared/*"
+			],
+			"@utils/*": [
+				"./src/utils/*"
+			],
+			"@cline/shared/*": [
+				"./node_modules/@cline/shared/dist/*"
+			]
+		},
 		"typeRoots": [
 			"./node_modules/@types",
 			"./src/types"