From 53a00e5bc04c5cb9b01067c437f23119a004ac58 Mon Sep 17 00:00:00 2001 From: Muqsit Date: Sun, 14 Jun 2026 22:02:57 -0700 Subject: [PATCH] fix(view): report+write commands & memory on sync; decouple grok detection from config symlink (#187) --- src/lib/agents.ts | 40 +++++----- src/lib/command-skills.ts | 31 ++++++-- src/lib/staleness/writers/commands.test.ts | 86 ++++++++++++++++++++++ src/lib/versions.test.ts | 78 +++++++++++++++++++- src/lib/versions.ts | 27 ++++--- 5 files changed, 226 insertions(+), 36 deletions(-) create mode 100644 src/lib/staleness/writers/commands.test.ts diff --git a/src/lib/agents.ts b/src/lib/agents.ts index 817ea1bf..2292dcb4 100644 --- a/src/lib/agents.ts +++ b/src/lib/agents.ts @@ -105,39 +105,41 @@ function findInPath(command: string): string | null { } /** Grok-specific binary resolution. - * Grok does not live in node_modules/.bin. Its versioned binaries live in - * ~/.grok/downloads/ with names like `grok-0.1.218-macos-aarch64`. - * We still use the agents-cli version dir for *config isolation* via GROK_HOME. + * Grok does not live in node_modules/.bin. Its versioned binaries live in each + * managed version home under `.grok/downloads/`, so detection must not follow + * the host ~/.grok config symlink. */ function resolveGrokBinary(version?: string): string | null { - const grokDownloads = path.join(HOME, '.grok', 'downloads'); - if (!fs.existsSync(grokDownloads)) return null; - - const entries = fs.readdirSync(grokDownloads); - // Prefer exact version match in filename if (version && version !== 'latest') { - const match = entries.find((e) => e.includes(version) && e.startsWith('grok-')); - if (match) return path.join(grokDownloads, match); + const binaryPath = getBinaryPath('grok', version); + if (fs.existsSync(binaryPath)) return binaryPath; + return null; + } + + const resolvedVersion = resolveVersion('grok', process.cwd()); + if (resolvedVersion) { + const binaryPath = getBinaryPath('grok', resolvedVersion); + if (fs.existsSync(binaryPath)) return binaryPath; } - // Fallback: the "current" symlink or the plain `grok-*` without version in name - const current = entries.find((e) => e === 'grok' || e.startsWith('grok-') && !e.match(/grok-\d/)); - if (current) return path.join(grokDownloads, current); + const grokVersionsDir = path.join(getVersionsDir(), 'grok'); + if (!fs.existsSync(grokVersionsDir)) return null; - // Last resort: newest file by mtime let latest: string | null = null; let latestMtime = 0; - for (const e of entries) { - if (!e.startsWith('grok-')) continue; + for (const entry of fs.readdirSync(grokVersionsDir, { withFileTypes: true })) { + if (!entry.isDirectory()) continue; + const binaryPath = getBinaryPath('grok', entry.name); + if (!fs.existsSync(binaryPath)) continue; try { - const stat = fs.statSync(path.join(grokDownloads, e)); + const stat = fs.statSync(binaryPath); if (stat.mtimeMs > latestMtime) { latestMtime = stat.mtimeMs; - latest = e; + latest = binaryPath; } } catch {} } - return latest ? path.join(grokDownloads, latest) : null; + return latest; } function splitCommandLine(command: string): string[] { diff --git a/src/lib/command-skills.ts b/src/lib/command-skills.ts index 73278029..3bc9fc0b 100644 --- a/src/lib/command-skills.ts +++ b/src/lib/command-skills.ts @@ -83,12 +83,25 @@ export function buildCommandSkillContent(commandName: string, sourcePath: string ].join('\n'); } -export function skillSourceExists(skillName: string, skillSourceDirs: Array): boolean { - return skillSourceDirs.some((dir) => { - if (!dir) return false; +function findSkillSourceDir(skillName: string, skillSourceDirs: Array): string | null { + for (const dir of skillSourceDirs) { + if (!dir) continue; const candidate = path.join(dir, skillName); - return fs.existsSync(candidate) && fs.lstatSync(candidate).isDirectory(); - }); + if (fs.existsSync(candidate) && fs.lstatSync(candidate).isDirectory()) { + return candidate; + } + } + return null; +} + +export function skillSourceExists(skillName: string, skillSourceDirs: Array): boolean { + return findSkillSourceDir(skillName, skillSourceDirs) !== null; +} + +export function readSkillSourceCommandMarker(skillName: string, skillSourceDirs: Array): string | null { + const sourceDir = findSkillSourceDir(skillName, skillSourceDirs); + if (!sourceDir) return null; + return readSkillCommandMarker(path.join(sourceDir, 'SKILL.md')); } export function installCommandSkillToVersion( @@ -98,8 +111,12 @@ export function installCommandSkillToVersion( skillSourceDirs: Array = [] ): { success: boolean; skipped?: boolean; error?: string } { const skillName = commandSkillName(commandName); - if (skillSourceExists(skillName, skillSourceDirs)) { - return { success: false, skipped: true, error: `Skill '${skillName}' already exists` }; + const existingSkillSource = findSkillSourceDir(skillName, skillSourceDirs); + if (existingSkillSource) { + const sourceMarker = readSkillCommandMarker(path.join(existingSkillSource, 'SKILL.md')); + if (sourceMarker !== commandName) { + return { success: true, skipped: true, error: `Skill '${skillName}' already exists` }; + } } const skillsDir = safeJoin(agentDir, 'skills'); diff --git a/src/lib/staleness/writers/commands.test.ts b/src/lib/staleness/writers/commands.test.ts new file mode 100644 index 00000000..3f585837 --- /dev/null +++ b/src/lib/staleness/writers/commands.test.ts @@ -0,0 +1,86 @@ +import { describe, expect, it } from 'vitest'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { execFileSync } from 'child_process'; +import { fileURLToPath } from 'url'; + +const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../../../..'); + +function runCommandsWriterFixture(scriptBody: string): unknown { + const home = fs.mkdtempSync(path.join(os.tmpdir(), 'commands-writer-')); + try { + const script = ` + import * as fs from 'fs'; + import * as path from 'path'; + import { getWriter } from './src/lib/staleness/registry.ts'; + + const home = process.env.HOME; + if (!home) throw new Error('HOME missing'); + const userDir = path.join(home, '.agents'); + const projectRoot = path.join(home, 'project'); + const version = '0.2.33'; + const versionHome = path.join(home, '.agents', '.history', 'versions', 'grok', version, 'home'); + const agentDir = path.join(versionHome, '.grok'); + fs.mkdirSync(projectRoot, { recursive: true }); + fs.mkdirSync(agentDir, { recursive: true }); + const writeUser = (rel, content) => { + const p = path.join(userDir, rel); + fs.mkdirSync(path.dirname(p), { recursive: true }); + fs.writeFileSync(p, content, 'utf-8'); + return p; + }; + const writer = getWriter('commands', 'grok'); + if (!writer) throw new Error('grok commands writer missing'); + ${scriptBody} + `; + const out = execFileSync('bun', ['--eval', script], { + cwd: repoRoot, + env: { ...process.env, HOME: home }, + encoding: 'utf-8', + }); + return JSON.parse(out.trim()); + } finally { + fs.rmSync(home, { recursive: true, force: true }); + } +} + +describe('commands writer', () => { + it('rewrites marker-bearing command skills that collide with source skills', () => { + const result = runCommandsWriterFixture(` + writeUser('commands/debug.md', ['---', 'description: Fresh debug', '---', '', 'fresh body'].join('\\n')); + writeUser('skills/debug/SKILL.md', ['---', 'name: "debug"', 'description: "old"', 'agents_command: "debug"', '---', '', 'old body'].join('\\n')); + fs.mkdirSync(path.join(agentDir, 'skills', 'debug'), { recursive: true }); + fs.writeFileSync(path.join(agentDir, 'skills', 'debug', 'SKILL.md'), 'STALE', 'utf-8'); + + const writeResult = writer.write({ version, versionHome, selection: ['debug'], cwd: projectRoot }); + const skillPath = path.join(agentDir, 'skills', 'debug', 'SKILL.md'); + console.log(JSON.stringify({ + synced: writeResult.synced, + content: fs.readFileSync(skillPath, 'utf-8'), + })); + `) as { synced: string[]; content: string }; + + expect(result.synced).toEqual(['debug']); + expect(result.content).toContain('agents_command: "debug"'); + expect(result.content).toContain('fresh body'); + expect(result.content).not.toContain('STALE'); + }); + + it('reports genuine source-skill collisions as synced no-ops', () => { + const result = runCommandsWriterFixture(` + writeUser('commands/plan.md', ['---', 'description: Plan', '---', '', 'plan body'].join('\\n')); + writeUser('skills/plan/SKILL.md', ['---', 'name: "plan"', 'description: "real skill"', '---', '', 'real skill body'].join('\\n')); + + const writeResult = writer.write({ version, versionHome, selection: ['plan'], cwd: projectRoot }); + const skillPath = path.join(agentDir, 'skills', 'plan', 'SKILL.md'); + console.log(JSON.stringify({ + synced: writeResult.synced, + exists: fs.existsSync(skillPath), + })); + `) as { synced: string[]; exists: boolean }; + + expect(result.synced).toEqual(['plan']); + expect(result.exists).toBe(false); + }); +}); diff --git a/src/lib/versions.test.ts b/src/lib/versions.test.ts index 0e5bbc84..cd9a1ffe 100644 --- a/src/lib/versions.test.ts +++ b/src/lib/versions.test.ts @@ -25,7 +25,7 @@ function runVersionSync(home: string, expression: string): unknown { const moduleUrl = pathToFileURL(path.resolve('src/lib/versions.ts')).href; const tsxBin = path.resolve('node_modules/.bin/tsx'); const child = spawnSync(tsxBin, ['-e', ` - import { syncResourcesToVersion } from ${JSON.stringify(moduleUrl)}; + import { listInstalledVersions, syncResourcesToVersion } from ${JSON.stringify(moduleUrl)}; const home = ${JSON.stringify(home)}; const result = ${expression}; console.log(JSON.stringify(result)); @@ -91,6 +91,42 @@ describe('version resource sync path handling', () => { expect(skill).toContain('Recap the conversation so far.'); }); + it('keeps grok command-generated skills authoritative over marker-bearing source skills', async () => { + const home = makeTempHome(); + const commandPath = path.join(home, '.agents', 'commands', 'debug.md'); + const sourceSkillPath = path.join(home, '.agents', 'skills', 'debug', 'SKILL.md'); + const binaryPath = path.join(home, '.agents', '.history', 'versions', 'grok', '0.2.33', 'home', '.grok', 'downloads', 'grok-0.2.33-macos-aarch64'); + + fs.mkdirSync(path.dirname(commandPath), { recursive: true }); + fs.mkdirSync(path.dirname(sourceSkillPath), { recursive: true }); + fs.mkdirSync(path.dirname(binaryPath), { recursive: true }); + fs.writeFileSync( + commandPath, + ['---', 'description: Fresh debug command', '---', '', 'fresh command body'].join('\n'), + 'utf-8' + ); + fs.writeFileSync( + sourceSkillPath, + ['---', 'name: "debug"', 'description: "old generated command"', 'agents_command: "debug"', '---', '', 'old source skill body'].join('\n'), + 'utf-8' + ); + fs.writeFileSync(binaryPath, '#!/bin/sh\nexit 0\n', 'utf-8'); + fs.chmodSync(binaryPath, 0o755); + + const result = runVersionSync( + home, + "syncResourcesToVersion('grok', '0.2.33', { commands: ['debug'], skills: ['debug'] }, { cwd: home })" + ) as { commands: boolean; skills: boolean }; + + const syncedSkillPath = path.join(home, '.agents', '.history', 'versions', 'grok', '0.2.33', 'home', '.grok', 'skills', 'debug', 'SKILL.md'); + const syncedSkill = fs.readFileSync(syncedSkillPath, 'utf-8'); + expect(result.commands).toBe(true); + expect(result.skills).toBe(false); + expect(syncedSkill).toContain('fresh command body'); + expect(syncedSkill).not.toContain('old source skill body'); + expect(fs.existsSync(binaryPath)).toBe(true); + }); + it('does not follow symlinks inside copied skill resources', async () => { const home = makeTempHome(); @@ -162,4 +198,44 @@ describe('version resource sync path handling', () => { expect(settings.mcpServers?.safe).toBeDefined(); expect(settings.mcpServers?.evil).toBeUndefined(); }); + + it('writes missing grok AGENTS.md when syncing a partial selection without memory', async () => { + const home = makeTempHome(); + const rulesDir = path.join(home, '.agents', '.system', 'rules'); + + fs.mkdirSync(path.join(rulesDir, 'subrules'), { recursive: true }); + fs.writeFileSync( + path.join(rulesDir, 'rules.yaml'), + 'presets:\n default:\n subrules:\n - core\n', + 'utf-8' + ); + fs.writeFileSync(path.join(rulesDir, 'subrules', 'core.md'), 'Grok memory body\n', 'utf-8'); + + const result = runVersionSync( + home, + "syncResourcesToVersion('grok', '0.2.33', { skills: [] }, { cwd: home })" + ) as { memory: string[] }; + + const agentsPath = path.join(home, '.agents', '.history', 'versions', 'grok', '0.2.33', 'home', '.grok', 'AGENTS.md'); + expect(result.memory).toContain('AGENTS.md'); + expect(fs.existsSync(agentsPath)).toBe(true); + expect(fs.readFileSync(agentsPath, 'utf-8')).toContain('Grok memory body'); + }); + + it('detects grok binaries from the per-version home, not the host .grok symlink', async () => { + const home = makeTempHome(); + const installedDownloads = path.join(home, '.agents', '.history', 'versions', 'grok', '0.2.33', 'home', '.grok', 'downloads'); + const emptyConfigDir = path.join(home, '.agents', '.history', 'versions', 'grok', '0.2.32', 'home', '.grok'); + const hostGrok = path.join(home, '.grok'); + + fs.mkdirSync(installedDownloads, { recursive: true }); + fs.mkdirSync(path.join(emptyConfigDir, 'downloads'), { recursive: true }); + fs.writeFileSync(path.join(installedDownloads, 'grok-0.2.33-macos-aarch64'), '#!/bin/sh\nexit 0\n', 'utf-8'); + fs.chmodSync(path.join(installedDownloads, 'grok-0.2.33-macos-aarch64'), 0o755); + fs.symlinkSync(emptyConfigDir, hostGrok, 'dir'); + + const result = runVersionSync(home, "listInstalledVersions('grok')") as string[]; + + expect(result).toEqual(['0.2.33']); + }); }); diff --git a/src/lib/versions.ts b/src/lib/versions.ts index f1e2d3ef..2ffe22ce 100644 --- a/src/lib/versions.ts +++ b/src/lib/versions.ts @@ -42,7 +42,7 @@ import { composeRulesFromState } from './rules/compose.js'; import { loadManifest, saveManifest, buildManifest as buildSyncManifest, isStale } from './staleness/index.js'; import { emit } from './events.js'; import { safeJoin } from './paths.js'; -import { installCommandSkillToVersion, listCommandSkillsInVersion, shouldInstallCommandAsSkill } from './command-skills.js'; +import { installCommandSkillToVersion, listCommandSkillsInVersion, readSkillSourceCommandMarker, shouldInstallCommandAsSkill } from './command-skills.js'; import { getWriter, getDetector } from './staleness/registry.js'; /** Promisified exec for running shell commands. */ @@ -883,10 +883,7 @@ export function getVersionDir(agent: AgentId, version: string): string { export function getBinaryPath(agent: AgentId, version: string): string { const agentConfig = AGENTS[agent]; if (agent === 'grok') { - // Grok binaries live in the global ~/.grok/downloads, not per-version node_modules. - // We return a best-effort path (used for display / checks). Real resolution - // happens in agents.ts resolveGrokBinary + the generated shims. - const grokDownloads = path.join(os.homedir(), '.grok', 'downloads'); + const grokDownloads = path.join(getVersionHomePath(agent, version), '.grok', 'downloads'); // Best effort: first matching file for this version try { const entries = fs.readdirSync(grokDownloads); @@ -1805,11 +1802,11 @@ export function syncResourcesToVersion(agent: AgentId, version: string, selectio const commandsToSync = selection ? resolveSelection(selection.commands, available.commands) : available.commands; // No selection = sync all + const commandsAsSkills = shouldInstallCommandAsSkill(agent, version); if (commandsToSync.length > 0 && commandsWriter) { const commandsTarget = path.join(agentDir, agentConfig.commandsSubdir); - const commandsAsSkills = shouldInstallCommandAsSkill(agent, version); - if (commandsAsSkills) { + if (commandsAsSkills && agentConfig.commandsSubdir) { removePath(commandsTarget); } const r = commandsWriter.write({ version, versionHome, selection: commandsToSync, cwd }); @@ -1843,9 +1840,21 @@ export function syncResourcesToVersion(agent: AgentId, version: string, selectio // ~/.agents/skills/ (Gemini) are not registered; we clear the version-home // skills dir for them so a stale per-version copy never shadows central. const skillsWriter = getWriter('skills', agent); - const skillsToSync = selection + let skillsToSync = selection ? resolveSelection(selection.skills, available.skills) : available.skills; + if (commandsAsSkills && commandsToSync.length > 0 && skillsToSync.length > 0) { + const commandNames = new Set(commandsToSync); + const skillRoots = [ + path.join(getUserAgentsDir(), 'skills'), + getSkillsDir(), + ...getEnabledExtraRepos().map((e) => path.join(e.dir, 'skills')), + ]; + skillsToSync = skillsToSync.filter((skill) => { + if (!commandNames.has(skill)) return true; + return readSkillSourceCommandMarker(skill, skillRoots) !== skill; + }); + } if (agentConfig.nativeAgentsSkillsDir) { removePath(path.join(agentDir, 'skills')); @@ -1910,7 +1919,7 @@ export function syncResourcesToVersion(agent: AgentId, version: string, selectio // CAPABLE_AGENTS list and silently skipped it). Project rules are NOT // synced into the version home — they are composed into the workspace at // agents-run time (see compileRulesForProject). - const skipMemory = selection && (selection.memory === undefined || (Array.isArray(selection.memory) && selection.memory.length === 0)); + const skipMemory = selection && Array.isArray(selection.memory) && selection.memory.length === 0; const rulesWriter = getWriter('rules', agent); if (!skipMemory && rulesWriter) { try {