diff --git a/apps/vscode/src/core/context/instructions/user-instructions/__tests__/skills.test.ts b/apps/vscode/src/core/context/instructions/user-instructions/__tests__/skills.test.ts index 5169a79129..29796ec002 100644 --- a/apps/vscode/src/core/context/instructions/user-instructions/__tests__/skills.test.ts +++ b/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) + }) +}) + diff --git a/apps/vscode/src/core/context/instructions/user-instructions/skills.ts b/apps/vscode/src/core/context/instructions/user-instructions/skills.ts index b011482f2b..c10c65b0ec 100644 --- a/apps/vscode/src/core/context/instructions/user-instructions/skills.ts +++ b/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= (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, 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 { + 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. diff --git a/apps/vscode/src/core/controller/file/toggleSkill.ts b/apps/vscode/src/core/controller/file/toggleSkill.ts index e2ad440cd5..66477a2f99 100644 --- a/apps/vscode/src/core/controller/file/toggleSkill.ts +++ b/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({ diff --git a/apps/vscode/src/sdk/auth-service.ts b/apps/vscode/src/sdk/auth-service.ts index 4b5cb4637f..88cf7d1588 100644 --- a/apps/vscode/src/sdk/auth-service.ts +++ b/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((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 { + 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. */ diff --git a/apps/vscode/src/test/e2e/chat.test.ts b/apps/vscode/src/test/e2e/chat.test.ts index c15a24c6c1..64a0734541 100644 --- a/apps/vscode/src/test/e2e/chat.test.ts +++ b/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() diff --git a/apps/vscode/src/test/e2e/diff.test.ts b/apps/vscode/src/test/e2e/diff.test.ts index 99d3067742..ee8b83c70d 100644 --- a/apps/vscode/src/test/e2e/diff.test.ts +++ b/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 diff --git a/apps/vscode/tsconfig.unit-test.json b/apps/vscode/tsconfig.unit-test.json index 54cd70746b..b174ed4532 100644 --- a/apps/vscode/tsconfig.unit-test.json +++ b/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"