diff --git a/harnesses/pi/extension-source/hivemind.ts b/harnesses/pi/extension-source/hivemind.ts index 007ee0a2..5ab22ee2 100644 --- a/harnesses/pi/extension-source/hivemind.ts +++ b/harnesses/pi/extension-source/hivemind.ts @@ -1288,6 +1288,7 @@ const PI_SKILLIFY_COMMANDS: { cmd: string; desc: string }[] = [ { cmd: "hivemind skillify push ", desc: "upload a local skill to the org table (inverse of pull)" }, { cmd: "hivemind skillify push --from ", desc: "which local skills dir to read (default: project)" }, { cmd: "hivemind skillify push --dry-run", desc: "preview without writing to the org table" }, + { cmd: "hivemind skillify push --review", desc: "show the exact candidate and proposed version without publishing" }, { cmd: "hivemind skillify unpull", desc: "remove every skill previously installed by pull" }, { cmd: "hivemind skillify unpull --user ", desc: "remove only that author's pulls" }, { cmd: "hivemind skillify unpull --not-mine", desc: "remove all pulls except your own" }, diff --git a/src/cli/skillify-spec.ts b/src/cli/skillify-spec.ts index e16fa0c8..f9516829 100644 --- a/src/cli/skillify-spec.ts +++ b/src/cli/skillify-spec.ts @@ -49,6 +49,7 @@ export const SKILLIFY_COMMANDS: SkillifyCommand[] = [ { cmd: "hivemind skillify push ", desc: "upload a local skill to the org table (inverse of pull)" }, { cmd: "hivemind skillify push --from ", desc: "which local skills dir to read (default: project)" }, { cmd: "hivemind skillify push --dry-run", desc: "preview without writing to the org table" }, + { cmd: "hivemind skillify push --review", desc: "show the exact candidate and proposed version without publishing" }, { cmd: "hivemind skillify unpull", desc: "remove every skill previously installed by pull" }, { cmd: "hivemind skillify unpull --user ", desc: "remove only that author's pulls" }, { cmd: "hivemind skillify unpull --not-mine", desc: "remove all pulls except your own" }, @@ -114,6 +115,7 @@ export const SKILLIFY_SPEC: SkillifySubcommand[] = [ options: [ { flag: "--from ", desc: "which local skills dir to read (default: project)" }, { flag: "--dry-run", desc: "preview without writing to the org table" }, + { flag: "--review", desc: "show the exact candidate and proposed version without publishing" }, ], note: "the manual counterpart to mining — push a skill Claude wrote locally straight to the org table (append-only: re-pushing an existing skill lands a new version). Authorship/lineage is preserved; you're added as a contributor.", }, diff --git a/src/commands/skillify.ts b/src/commands/skillify.ts index e7f5fc9f..f68e285a 100644 --- a/src/commands/skillify.ts +++ b/src/commands/skillify.ts @@ -12,6 +12,7 @@ * hivemind skillify team remove — remove a username from the team list * hivemind skillify team list — list current team members * hivemind skillify pull [skill-name] [opts] — fetch skills from Deeplake to local FS + * hivemind skillify push --review — inspect a candidate without publishing * hivemind skillify status — show counter + per-project state * * The team list is consumed by the worker when scope=team: SQL filter @@ -199,6 +200,45 @@ function takeBooleanFlag(args: string[], flag: string): boolean { return true; } +/** Identifies Unicode formatting controls that can visually reorder terminal output. */ +function isBidirectionalControl(code: number): boolean { + return code === 0x061c || + code === 0x200e || + code === 0x200f || + (code >= 0x202a && code <= 0x202e) || + (code >= 0x2066 && code <= 0x2069); +} + +/** Rejects skill content that could alter or disguise the review shown in a terminal. */ +function assertTerminalSafeReview(name: string, text: string): void { + for (let offset = 0; offset < text.length; offset++) { + const code = text.charCodeAt(offset); + const allowedWhitespace = code === 0x09 || code === 0x0a || code === 0x0d; + if ( + (!allowedWhitespace && code < 0x20) || + (code >= 0x7f && code <= 0x9f) || + isBidirectionalControl(code) + ) { + const point = `U+${code.toString(16).toUpperCase().padStart(4, "0")}`; + throw new Error(`cannot review '${name}': SKILL.md contains terminal control character ${point} at offset ${offset}`); + } + } +} + +/** Renders untrusted metadata visibly without changing its underlying value. */ +function escapeTerminalMetadata(text: string): string { + let escaped = ""; + for (let offset = 0; offset < text.length; offset++) { + const code = text.charCodeAt(offset); + if (code < 0x20 || (code >= 0x7f && code <= 0x9f) || isBidirectionalControl(code)) { + escaped += `\\u${code.toString(16).toUpperCase().padStart(4, "0")}`; + } else { + escaped += text[offset]; + } + } + return escaped; +} + async function pullSkills(args: string[]): Promise { // Parse flags first so the remaining positional is the optional skill name const work = [...args]; @@ -274,6 +314,7 @@ async function pushSkills(args: string[]): Promise { // Parse flags first so the remaining positional is the required skill name. const work = [...args]; const fromRaw = takeFlagValue(work, "--from") ?? "project"; + const review = takeBooleanFlag(work, "--review"); const dryRun = takeBooleanFlag(work, "--dry-run"); const skillName = work[0]; @@ -283,7 +324,7 @@ async function pushSkills(args: string[]): Promise { throw new Error(`Invalid --from '${fromRaw}'. Use 'project' or 'global'.`); } if (!skillName) { - throw new Error("Usage: hivemind skillify push [--from project|global] [--dry-run]"); + throw new Error("Usage: hivemind skillify push [--from project|global] [--dry-run] [--review]"); } const config = loadRoutedConfig(); @@ -294,7 +335,7 @@ async function pushSkills(args: string[]): Promise { config.token, config.apiUrl, config.orgId, config.workspaceId, config.skillsTableName, ); const query = (sql: string) => api.query(sql) as Promise[]>; - const scopeCfg = loadScopeConfig(); + const scopeCfg = loadScopeConfig({ migrateLegacy: !review }); const summary = await runPush({ query, @@ -306,7 +347,7 @@ async function pushSkills(args: string[]): Promise { pusher: config.userName, scope: scopeCfg.scope, agent: "cli", - dryRun, + dryRun: dryRun || review, }); const src = fromRaw === "global" @@ -315,6 +356,20 @@ async function pushSkills(args: string[]): Promise { const verDesc = summary.previousVersion === null ? `v${summary.version} (new)` : `v${summary.previousVersion} → v${summary.version}`; + + if (review) { + assertTerminalSafeReview(summary.name, summary.sourceText); + console.log(`Review candidate: ${summary.name} (proposed v${summary.version})`); + console.log(`File: ${escapeTerminalMetadata(summary.localPath)}`); + console.log(`Author: ${escapeTerminalMetadata(summary.author)}`); + console.log(`Scope: ${summary.scope}`); + console.log("--- BEGIN SKILL.md ---"); + console.log(summary.sourceText); + console.log("--- END SKILL.md ---"); + console.log("Review mode — nothing published. Rerun without --review to publish."); + return; + } + const tag = summary.action === "pushed" ? "✓ pushed" : "→ would push"; console.log(`Source: ${src}`); console.log(` ${tag.padEnd(15)} ${summary.name.padEnd(40)} ${verDesc.padEnd(18)} (${summary.author}, scope=${summary.scope})`); diff --git a/src/skillify/push.ts b/src/skillify/push.ts index ddc633f2..504516ec 100644 --- a/src/skillify/push.ts +++ b/src/skillify/push.ts @@ -66,6 +66,8 @@ export interface PushSummary { project: string; projectKey: string; scope: Scope; + /** Exact local SKILL.md snapshot used to derive this push proposal. */ + sourceText: string; } export interface ParsedLocalSkill { @@ -78,6 +80,8 @@ export interface ParsedLocalSkill { version: number; agent?: string; createdAt?: string; + /** Exact local SKILL.md bytes decoded as UTF-8, before parsing or trimming. */ + sourceText: string; } /** @@ -92,7 +96,8 @@ export function readLocalSkill(skillsRoot: string, name: string): ParsedLocalSki if (!existsSync(path)) { throw new Error(`skill '${name}' not found at ${path}`); } - const parsed = parseFrontmatter(readFileSync(path, "utf-8")); + const sourceText = readFileSync(path, "utf-8"); + const parsed = parseFrontmatter(sourceText); if (!parsed) { throw new Error(`skill '${name}' at ${path} has no valid frontmatter — cannot push`); } @@ -107,6 +112,7 @@ export function readLocalSkill(skillsRoot: string, name: string): ParsedLocalSki version: typeof fm.version === "number" && fm.version > 0 ? fm.version : 1, agent: typeof fm.created_by_agent === "string" ? fm.created_by_agent : undefined, createdAt: typeof fm.created_at === "string" ? fm.created_at : undefined, + sourceText, }; } @@ -202,5 +208,6 @@ export async function runPush(args: PushArgs): Promise { project, projectKey, scope: args.scope, + sourceText: local.sourceText, }; } diff --git a/src/skillify/scope-config.ts b/src/skillify/scope-config.ts index 77922113..0d719dfa 100644 --- a/src/skillify/scope-config.ts +++ b/src/skillify/scope-config.ts @@ -16,7 +16,7 @@ */ import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; -import { join } from "node:path"; +import { dirname, join } from "node:path"; import { migrateLegacyStateDir } from "./legacy-migration.js"; import { getStateDir } from "./state-dir.js"; @@ -43,9 +43,26 @@ function configPath(): string { const DEFAULT: ScopeConfig = { scope: "me", team: [], install: "project" }; -export function loadScopeConfig(): ScopeConfig { - migrateLegacyStateDir(); - const CONFIG_PATH = configPath(); +export interface LoadScopeConfigOptions { + /** Set false for read-only commands that must not rename the legacy state directory. */ + migrateLegacy?: boolean; +} + +export function loadScopeConfig(options: LoadScopeConfigOptions = {}): ScopeConfig { + const shouldMigrate = options.migrateLegacy ?? true; + if (shouldMigrate) migrateLegacyStateDir(); + + const currentStateDir = getStateDir(); + let CONFIG_PATH = configPath(); + if ( + !shouldMigrate && + !existsSync(CONFIG_PATH) && + !existsSync(currentStateDir) && + !process.env.HIVEMIND_STATE_DIR?.trim() + ) { + const legacyPath = join(dirname(currentStateDir), "skilify", "config.json"); + if (existsSync(legacyPath)) CONFIG_PATH = legacyPath; + } if (!existsSync(CONFIG_PATH)) return DEFAULT; try { const raw = JSON.parse(readFileSync(CONFIG_PATH, "utf-8")); diff --git a/tests/claude-code/skillify-cli.test.ts b/tests/claude-code/skillify-cli.test.ts index 6ffa86b7..fd30b2c0 100644 --- a/tests/claude-code/skillify-cli.test.ts +++ b/tests/claude-code/skillify-cli.test.ts @@ -3,6 +3,8 @@ import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync import { tmpdir, homedir } from "node:os"; import { join } from "node:path"; +const { apiQueries } = vi.hoisted(() => ({ apiQueries: [] as string[] })); + // Logged paths use the native separator (product builds them with path.join). // Compare against join(...) substrings rather than "/"-literal regexes so the // assertions hold on Windows too. @@ -16,7 +18,8 @@ vi.mock("../../src/config.js", () => ({ })); vi.mock("../../src/deeplake-api.js", () => ({ DeeplakeApi: class { - async query(_sql: string) { + async query(sql: string) { + apiQueries.push(sql); return [{ name: "fake-skill", project: "p", project_key: "pk", body: "body", version: 1, source_agent: "claude_code", @@ -57,6 +60,7 @@ beforeEach(() => { else configBackup = null; try { rmSync(CONFIG_PATH); } catch { /* nothing */ } logged = []; erred = []; + apiQueries.length = 0; // Default: logged in. Individual tests can `loadConfigMock.mockReturnValueOnce(null)` // to exercise the unauthenticated path of unpull (no login needed) vs --not-mine // (which still requires myUsername). @@ -299,11 +303,12 @@ describe("push", () => { // The DeeplakeApi mock returns a fake row (version 1) for the version SELECT, // so a real push bumps to v2; the INSERT response is ignored. let pushDir: string; - function writeProjectSkill(name: string): void { + function writeProjectSkill(name: string): string { const dir = join(process.cwd(), ".claude", "skills", name); mkdirSync(dir, { recursive: true }); + const path = join(dir, "SKILL.md"); writeFileSync( - join(dir, "SKILL.md"), + path, [ "---", `name: ${name}`, @@ -317,6 +322,7 @@ describe("push", () => { "## Body", ].join("\n"), ); + return path; } beforeEach(() => { pushDir = mkdtempSync(join(tmpdir(), "skillify-cli-push-")); @@ -338,6 +344,68 @@ describe("push", () => { expect(out).toContain("Dry run — nothing written to the org skills table."); }); + it("--review prints the exact candidate and proposed version without writing", async () => { + const path = writeProjectSkill("demo-skill"); + const candidate = `${readFileSync(path, "utf-8")} \n\n`; + writeFileSync(path, candidate); + runSkillifyCommand(["push", "demo-skill", "--review"]); + await new Promise(r => setImmediate(r)); + const out = logged.join("\n"); + + expect(out).toContain("Review candidate: demo-skill (proposed v2)"); + expect(out).toContain("--- BEGIN SKILL.md ---"); + expect(out).toContain("## Body"); + expect(out).toContain("--- END SKILL.md ---"); + expect(logged).toContain(candidate); + expect(out).toContain("Review mode — nothing published"); + expect(apiQueries.some(sql => sql.includes("INSERT INTO"))).toBe(false); + }); + + it("--review rejects terminal control sequences without publishing", async () => { + const path = writeProjectSkill("unsafe-skill"); + const candidate = `${readFileSync(path, "utf-8")}\n\u001b]8;;https://example.com\u0007spoof\u001b]8;;\u0007`; + writeFileSync(path, candidate); + + runSkillifyCommand(["push", "unsafe-skill", "--review"]); + await new Promise(r => setImmediate(r)); + + expect(erred).toEqual([ + `push error: cannot review 'unsafe-skill': SKILL.md contains terminal control character U+001B at offset ${candidate.indexOf("\u001b")}`, + ]); + expect(logged.join("\n")).not.toContain("spoof"); + expect(apiQueries.some(sql => sql.includes("INSERT INTO"))).toBe(false); + }); + + it("--review rejects bidirectional formatting controls", async () => { + const path = writeProjectSkill("bidi-skill"); + const candidate = `${readFileSync(path, "utf-8")}\nvisible \u202Ehidden`; + writeFileSync(path, candidate); + + runSkillifyCommand(["push", "bidi-skill", "--review"]); + await new Promise(r => setImmediate(r)); + + expect(erred).toEqual([ + `push error: cannot review 'bidi-skill': SKILL.md contains terminal control character U+202E at offset ${candidate.indexOf("\u202E")}`, + ]); + expect(logged.join("\n")).not.toContain("hidden"); + expect(apiQueries.some(sql => sql.includes("INSERT INTO"))).toBe(false); + }); + + it("--review escapes formatting controls in the displayed source path", async () => { + const unsafeDir = join(pushDir, "path\u202Espoof"); + mkdirSync(unsafeDir, { recursive: true }); + process.chdir(unsafeDir); + writeProjectSkill("path-skill"); + + runSkillifyCommand(["push", "path-skill", "--review"]); + await new Promise(r => setImmediate(r)); + const out = logged.join("\n"); + + expect(out).not.toContain("\u202E"); + expect(out).toContain("\\u202E"); + expect(apiQueries.some(sql => sql.includes("INSERT INTO"))).toBe(false); + }); + it("real push reports the published version (remote v1 → v2)", async () => { writeProjectSkill("demo-skill"); runSkillifyCommand(["push", "demo-skill"]); @@ -364,7 +432,7 @@ describe("push", () => { it("missing skill name is rejected with the exact usage line", async () => { runSkillifyCommand(["push"]); await new Promise(r => setImmediate(r)); - expect(erred.join("\n")).toContain("Usage: hivemind skillify push [--from project|global] [--dry-run]"); + expect(erred.join("\n")).toContain("Usage: hivemind skillify push [--from project|global] [--dry-run] [--review]"); }); it("requires login with the exact message", async () => { diff --git a/tests/claude-code/skillify-scope-config.test.ts b/tests/claude-code/skillify-scope-config.test.ts index 2e26e3af..2b9e6ead 100644 --- a/tests/claude-code/skillify-scope-config.test.ts +++ b/tests/claude-code/skillify-scope-config.test.ts @@ -1,6 +1,6 @@ -import { afterEach, beforeEach, describe, expect, it } from "vitest"; -import { existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; -import { homedir } from "node:os"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { homedir, tmpdir } from "node:os"; import { join } from "node:path"; import { loadScopeConfig, saveScopeConfig } from "../../src/skillify/scope-config.js"; @@ -30,6 +30,71 @@ describe("loadScopeConfig", () => { expect(cfg).toEqual({ scope: "me", team: [], install: "project" }); }); + it("can read a legacy config without migrating either state directory", async () => { + const fakeHome = mkdtempSync(join(tmpdir(), "skillify-scope-review-")); + const previousHome = process.env.HOME; + const previousOverride = process.env.HIVEMIND_STATE_DIR; + process.env.HOME = fakeHome; + delete process.env.HIVEMIND_STATE_DIR; + + const legacyDir = join(fakeHome, ".deeplake", "state", "skilify"); + const currentDir = join(fakeHome, ".deeplake", "state", "skillify"); + mkdirSync(legacyDir, { recursive: true }); + writeFileSync(join(legacyDir, "config.json"), JSON.stringify({ + scope: "team", team: ["alice"], install: "global", + })); + + try { + vi.resetModules(); + const { loadScopeConfig: loadFresh } = await import("../../src/skillify/scope-config.js"); + expect(loadFresh({ migrateLegacy: false })).toEqual({ + scope: "team", team: ["alice"], install: "global", + }); + expect(existsSync(legacyDir)).toBe(true); + expect(existsSync(currentDir)).toBe(false); + } finally { + if (previousHome === undefined) delete process.env.HOME; + else process.env.HOME = previousHome; + if (previousOverride === undefined) delete process.env.HIVEMIND_STATE_DIR; + else process.env.HIVEMIND_STATE_DIR = previousOverride; + rmSync(fakeHome, { recursive: true, force: true }); + vi.resetModules(); + } + }); + + it("ignores legacy config when the current state directory already exists", async () => { + const fakeHome = mkdtempSync(join(tmpdir(), "skillify-scope-review-current-")); + const previousHome = process.env.HOME; + const previousOverride = process.env.HIVEMIND_STATE_DIR; + process.env.HOME = fakeHome; + delete process.env.HIVEMIND_STATE_DIR; + + const legacyDir = join(fakeHome, ".deeplake", "state", "skilify"); + const currentDir = join(fakeHome, ".deeplake", "state", "skillify"); + mkdirSync(legacyDir, { recursive: true }); + mkdirSync(currentDir, { recursive: true }); + writeFileSync(join(legacyDir, "config.json"), JSON.stringify({ + scope: "team", team: ["alice"], install: "global", + })); + + try { + vi.resetModules(); + const { loadScopeConfig: loadFresh } = await import("../../src/skillify/scope-config.js"); + expect(loadFresh({ migrateLegacy: false })).toEqual({ + scope: "me", team: [], install: "project", + }); + expect(existsSync(legacyDir)).toBe(true); + expect(existsSync(currentDir)).toBe(true); + } finally { + if (previousHome === undefined) delete process.env.HOME; + else process.env.HOME = previousHome; + if (previousOverride === undefined) delete process.env.HIVEMIND_STATE_DIR; + else process.env.HIVEMIND_STATE_DIR = previousOverride; + rmSync(fakeHome, { recursive: true, force: true }); + vi.resetModules(); + } + }); + it("returns the default when config file is malformed JSON", () => { mkdirSync(STATE_DIR, { recursive: true }); writeFileSync(CONFIG_PATH, "{this isn't json");