diff --git a/docs/chatgpt-coding-workflow.md b/docs/chatgpt-coding-workflow.md index f6826617..a90f3b85 100644 --- a/docs/chatgpt-coding-workflow.md +++ b/docs/chatgpt-coding-workflow.md @@ -137,8 +137,7 @@ advertised `SKILL.md` before following that skill. Skill paths may be outside the workspace. DevSpace only permits reading: -- advertised `SKILL.md` files -- files under a skill directory after that skill's `SKILL.md` has been read +- files within advertised skill directories Set `skills.enabled` to `false` to hide skills from workspace output. Enable Subagents and choose providers through `devspace init` or the persisted provider diff --git a/docs/gotchas.md b/docs/gotchas.md index 3bf7dd2d..842cf301 100644 --- a/docs/gotchas.md +++ b/docs/gotchas.md @@ -252,8 +252,9 @@ Copy or adapt them into one of the active profile directories before use. Legacy project paths such as `.pi/skills` can be added to `skills.paths` when needed. -If a skill appears in `open_workspace`, the model must read that skill's -`SKILL.md` before reading other files inside the skill directory. +If a skill appears in `open_workspace`, the model should read that skill's +`SKILL.md` before following it. DevSpace permits reads within advertised skill +directories without tracking whether `SKILL.md` was read first. ## Review Card Does Not Appear diff --git a/src/server.ts b/src/server.ts index 8945d303..03982a86 100644 --- a/src/server.ts +++ b/src/server.ts @@ -130,7 +130,7 @@ function serverInstructions( const showChangesInstruction = " If the turn successfully modifies files by creating, editing, overwriting, deleting, moving, or applying patches, call show_changes exactly once for that workspace after the final related file change and before your final response so the user can inspect the aggregate diff for that turn. Do not call it after every individual file change."; const skills = config.skillsEnabled - ? `When ${toolNames.openWorkspace} returns available skills and a task matches a skill, use ${toolNames.read} to read that skill's path before proceeding. Skill paths may be outside the workspace, but ${toolNames.read} only permits advertised SKILL.md files and files under already-loaded skill directories. ` + ? `When ${toolNames.openWorkspace} returns available skills and a task matches a skill, use ${toolNames.read} to read that skill's path before proceeding. Skill paths may be outside the workspace, and ${toolNames.read} permits files within advertised skill directories. ` : ""; const agents = `Follow instructions returned by ${toolNames.openWorkspace}. Before working under a path listed in availableAgentsFiles, use ${toolNames.read} to inspect that instruction file and follow it. `; const common = `Use DevSpace for coding work. Call ${toolNames.openWorkspace} once for each project folder or isolated worktree, then keep using its workspaceId. During continued work in the same project or worktree, do not call ${toolNames.openWorkspace} again. Open another workspace only when changing projects, switching checkout/worktree mode, creating another isolated worktree, or when the current workspaceId is rejected.`; @@ -597,7 +597,7 @@ function registerMcpSurface( "Read a file in a workspace. Use this for file inspection instead of shell commands like cat or sed.", "Use this tool to inspect relevant AGENTS.md or CLAUDE.md files listed by open_workspace before working in nested directories.", config.skillsEnabled - ? "If available skills were returned and a task matches one, read that skill's path before proceeding. Skill paths may be outside the workspace; only advertised SKILL.md files and files under already-loaded skill directories are readable." + ? "If available skills were returned and a task matches one, read that skill's path before proceeding. Skill paths may be outside the workspace; files within advertised skill directories are readable." : "", ] .filter(Boolean) @@ -650,7 +650,6 @@ function registerMcpSurface( }, response.content, startedAt); return response; } - workspaces.markReadPathLoaded(workspace, readPath); logToolCall(config, { tool: toolNames.read, diff --git a/src/skills.test.ts b/src/skills.test.ts index 41556b3a..d758dee8 100644 --- a/src/skills.test.ts +++ b/src/skills.test.ts @@ -232,18 +232,12 @@ try { assert.ok(projectSkill); assert.match(formatPathForPrompt(projectSkill.filePath), /SKILL\.md$/); - const skillFileRead = resolveSkillReadPath(loaded.skills, new Set(), projectSkill.filePath); - assert.equal(skillFileRead?.isSkillFile, true); + const skillFileRead = resolveSkillReadPath(loaded.skills, projectSkill.filePath); assert.equal(skillFileRead?.absolutePath, projectSkill.filePath); const resourcePath = join(projectSkill.baseDir, "references.md"); await writeFile(resourcePath, "reference\n"); - assert.equal(resolveSkillReadPath(loaded.skills, new Set(), resourcePath), undefined); - assert.equal( - resolveSkillReadPath(loaded.skills, new Set([projectSkill.baseDir]), resourcePath) - ?.isSkillFile, - false, - ); + assert.equal(resolveSkillReadPath(loaded.skills, resourcePath)?.absolutePath, resourcePath); } finally { if (originalHome === undefined) delete process.env.HOME; else process.env.HOME = originalHome; diff --git a/src/skills.ts b/src/skills.ts index cf4fa332..74c77344 100644 --- a/src/skills.ts +++ b/src/skills.ts @@ -18,7 +18,6 @@ export interface LoadedSkills { export interface SkillReadResolution { absolutePath: string; skill: Skill; - isSkillFile: boolean; } const SUBAGENTS_SKILL_NAME = "subagents"; @@ -84,7 +83,6 @@ export function loadWorkspaceSkills(config: ServerConfig, cwd: string): LoadedSk export function resolveSkillReadPath( skills: Skill[], - activatedSkillDirs: Set, inputPath: string, ): SkillReadResolution | undefined { const absolutePath = resolve(expandHomePath(inputPath)); @@ -92,28 +90,20 @@ export function resolveSkillReadPath( for (const skill of skills) { const skillFilePath = resolve(skill.filePath); if (absolutePath === skillFilePath) { - return { absolutePath, skill, isSkillFile: true }; + return { absolutePath, skill }; } } for (const skill of skills) { const baseDir = resolve(skill.baseDir); - if (!activatedSkillDirs.has(baseDir)) continue; if (!isPathInsideRoot(absolutePath, baseDir)) continue; - return { absolutePath, skill, isSkillFile: false }; + return { absolutePath, skill }; } return undefined; } -export function markSkillActivated( - activatedSkillDirs: Set, - skill: Skill, -): void { - activatedSkillDirs.add(resolve(skill.baseDir)); -} - export function formatPathForPrompt(path: string): string { const home = resolve(homedir()); const resolvedPath = resolve(path); diff --git a/src/workspaces.test.ts b/src/workspaces.test.ts index b31e108d..e3506937 100644 --- a/src/workspaces.test.ts +++ b/src/workspaces.test.ts @@ -156,6 +156,65 @@ test("persisted checkout and worktree sessions restore after recreating the regi } }); +test("workspace cache evicts old contexts without losing advertised skill reads", async (t) => { + const context = await fixture(t); + const stateDir = join(context.root, ".bounded-state"); + const agentDir = join(context.outsideRoot, "agent"); + const skillDir = join(agentDir, "skills", "cache-skill"); + const skillFile = join(skillDir, "SKILL.md"); + const resourceFile = join(skillDir, "reference.md"); + await mkdir(skillDir, { recursive: true }); + await writeFile( + skillFile, + [ + "---", + "name: cache-skill", + "description: Cache eviction regression skill.", + "---", + "", + "Read the reference when needed.", + "", + ].join("\n"), + ); + await writeFile(resourceFile, "reference\n"); + + const config = loadConfig(writeTestDevspaceConfig( + join(context.root, ".bounded-home"), + { + server: { port: 1 }, + workspaces: { + allowedRoots: [context.root], + worktreeRoot: join(context.root, ".devspace", "bounded-worktrees"), + }, + skills: { agentDir }, + subagents: { enabled: true, providers: [] }, + }, + )); + + const store = new SqliteWorkspaceStore(stateDir); + try { + const registry = new WorkspaceRegistry(config, store); + const first = await registry.openWorkspace(context.root); + assert.equal( + registry.resolveReadPath(first.workspace, resourceFile).absolutePath, + resourceFile, + ); + + for (let index = 0; index < 32; index += 1) { + await registry.openWorkspace(context.root); + } + + const restored = registry.getWorkspace(first.workspace.id); + assert.notEqual(restored, first.workspace); + assert.equal( + registry.resolveReadPath(restored, resourceFile).absolutePath, + resourceFile, + ); + } finally { + store.close(); + } +}); + test("workspace paths outside the allowed roots are rejected", async (t) => { const context = await fixture(t); diff --git a/src/workspaces.ts b/src/workspaces.ts index 3c83b343..6b339dd7 100644 --- a/src/workspaces.ts +++ b/src/workspaces.ts @@ -18,7 +18,6 @@ import { } from "./roots.js"; import { loadWorkspaceSkills, - markSkillActivated, resolveSkillReadPath, type LoadedSkills, type SkillReadResolution, @@ -55,7 +54,6 @@ export interface Workspace { skills: LoadedSkills["skills"]; skillDiagnostics: LoadedSkills["diagnostics"]; agentProfiles: LocalAgentProfile[]; - activatedSkillDirs: Set; } export interface WorkspaceContext { @@ -90,6 +88,8 @@ type DirectoryOps = { mkdir: (path: string, options: { recursive: true }) => Promise; }; +const MAX_CACHED_WORKSPACES = 32; + export class WorkspaceRegistry { private readonly workspaces = new Map(); private readonly pendingCheckoutOpens = new Map>(); @@ -247,6 +247,8 @@ export class WorkspaceRegistry { getWorkspace(workspaceId: string): Workspace { const workspace = this.workspaces.get(workspaceId); if (workspace) { + this.workspaces.delete(workspaceId); + this.workspaces.set(workspaceId, workspace); this.store?.touchSession(workspaceId); return workspace; } @@ -277,10 +279,9 @@ export class WorkspaceRegistry { : undefined, ...this.loadSkillsForWorkspace(root), agentProfiles: [], - activatedSkillDirs: new Set(), }; this.store?.touchSession(workspaceId); - this.workspaces.set(restoredWorkspace.id, restoredWorkspace); + this.rememberWorkspace(restoredWorkspace); return restoredWorkspace; } @@ -303,7 +304,6 @@ export class WorkspaceRegistry { } catch (workspaceError) { const skillRead = resolveSkillReadPath( workspace.skills, - workspace.activatedSkillDirs, inputPath, ); if (!skillRead) throw workspaceError; @@ -316,12 +316,6 @@ export class WorkspaceRegistry { } } - markReadPathLoaded(workspace: Workspace, readPath: WorkspaceReadPath): void { - if (readPath.skillRead?.isSkillFile) { - markSkillActivated(workspace.activatedSkillDirs, readPath.skillRead.skill); - } - } - resolveWorkingDirectory(workspace: Workspace, workingDirectory: string | undefined): string { const directory = workingDirectory ? this.resolvePath(workspace, workingDirectory) : workspace.root; return assertAllowedPath(directory, [workspace.root]); @@ -366,7 +360,6 @@ export class WorkspaceRegistry { worktree: input.worktree, ...this.loadSkillsForWorkspace(input.root), agentProfiles: await loadLocalAgentProfiles(this.config, input.root), - activatedSkillDirs: new Set(), }; this.store?.createSession({ @@ -378,7 +371,7 @@ export class WorkspaceRegistry { baseSha: workspace.worktree?.baseSha, managed: workspace.worktree?.managed, }); - this.workspaces.set(workspace.id, workspace); + this.rememberWorkspace(workspace); const agentsFiles = await this.loadInitialAgentsFiles(workspace.root); const availableAgentsFiles = await this.findAvailableAgentsFiles(workspace.root, agentsFiles); @@ -391,6 +384,18 @@ export class WorkspaceRegistry { }; } + private rememberWorkspace(workspace: Workspace): void { + this.workspaces.delete(workspace.id); + this.workspaces.set(workspace.id, workspace); + + if (!this.store) return; + while (this.workspaces.size > MAX_CACHED_WORKSPACES) { + const oldestWorkspaceId = this.workspaces.keys().next().value as string | undefined; + if (!oldestWorkspaceId) break; + this.workspaces.delete(oldestWorkspaceId); + } + } + private loadSkillsForWorkspace(root: string): Pick { const result = loadWorkspaceSkills(this.config, root); return {