From 240a4a17ad83010f69c2c823ca9f02bf421acc52 Mon Sep 17 00:00:00 2001 From: YeonGyu-Kim Date: Sat, 16 May 2026 18:02:54 +0900 Subject: [PATCH] fix(workspace): harden omo migration review issues --- src/features/boulder-state/AGENTS.md | 4 +-- src/hooks/prometheus-md-only/index.test.ts | 36 +++++++++++++++++++ src/hooks/prometheus-md-only/path-policy.ts | 4 +-- .../git-worktree/format-file-changes.ts | 10 +++++- src/shared/git-worktree/git-worktree.test.ts | 17 +++++++++ src/shared/legacy-workspace-migration.test.ts | 19 +++++++++- src/shared/legacy-workspace-migration.ts | 11 ++++-- 7 files changed, 91 insertions(+), 10 deletions(-) diff --git a/src/features/boulder-state/AGENTS.md b/src/features/boulder-state/AGENTS.md index d403c7c59..e43583368 100644 --- a/src/features/boulder-state/AGENTS.md +++ b/src/features/boulder-state/AGENTS.md @@ -34,7 +34,7 @@ interface BoulderState { | File | Purpose | |------|---------| | `types.ts` | `BoulderState`, `BoulderWorkState`, `TaskSessionState`, status enums | -| `storage.ts` | Atomic CRUD on `.omo/boulder-state.json`. Writes via temp file + rename; file lock per work_id | +| `storage.ts` | Atomic CRUD on `.omo/boulder.json`. Writes via temp file + rename; file lock per work_id | | `constants.ts` | Path resolution + schema version constant | | `top-level-task.ts` | Helpers to identify the current top-level plan task and resolve its reusable subagent session | | `format-duration.ts` | `formatDurationHuman(ms)` — "1h 23m 5s" formatting for boulder duration | @@ -67,7 +67,7 @@ session.completed ## STORAGE ``` -/.omo/boulder-state.json # gitignored; one file per worktree +/.omo/boulder.json # gitignored; one file per worktree ``` Atomic writes: temp file → fsync (where supported) → rename. File lock prevents concurrent corruption. Schema migrations between versions handled inline in `storage.ts`. diff --git a/src/hooks/prometheus-md-only/index.test.ts b/src/hooks/prometheus-md-only/index.test.ts index 31ba48f0d..eeeff6f54 100644 --- a/src/hooks/prometheus-md-only/index.test.ts +++ b/src/hooks/prometheus-md-only/index.test.ts @@ -288,6 +288,42 @@ describe("prometheus-md-only", () => { ).rejects.toThrow("File operations restricted to .omo/*.md plan files only") }) + test("should block Prometheus from writing .md files when .omo is only part of a path segment", async () => { + // given + const hook = createPrometheusMdOnlyHook(createMockPluginInput()) + const input = { + tool: "Write", + sessionID: TEST_SESSION_ID, + callID: "call-1", + } + const output = { + args: { filePath: "/tmp/test/work.omo/plans/work-plan.md" }, + } + + // when / #then + await expect( + hook["tool.execute.before"](input, output) + ).rejects.toThrow("File operations restricted to .omo/*.md plan files only") + }) + + test("should block Prometheus from writing .md files under .omo-backup", async () => { + // given + const hook = createPrometheusMdOnlyHook(createMockPluginInput()) + const input = { + tool: "Write", + sessionID: TEST_SESSION_ID, + callID: "call-1", + } + const output = { + args: { filePath: "/tmp/test/.omo-backup/plans/work-plan.md" }, + } + + // when / #then + await expect( + hook["tool.execute.before"](input, output) + ).rejects.toThrow("File operations restricted to .omo/*.md plan files only") + }) + test("should block Edit tool for non-.md files", async () => { // given const hook = createPrometheusMdOnlyHook(createMockPluginInput()) diff --git a/src/hooks/prometheus-md-only/path-policy.ts b/src/hooks/prometheus-md-only/path-policy.ts index fb77abade..f541b03d9 100644 --- a/src/hooks/prometheus-md-only/path-policy.ts +++ b/src/hooks/prometheus-md-only/path-policy.ts @@ -23,9 +23,7 @@ export function isAllowedFile(filePath: string, workspaceRoot: string): boolean return false } - // 4. Check if .omo/ or .omo\ exists anywhere in the path (case-insensitive) - // This handles both direct paths (.omo/x.md) and nested paths (project/.omo/x.md) - if (!/\.omo[/\\]/i.test(rel)) { + if (!/(^|[/\\])\.omo([/\\]|$)/i.test(rel)) { return false } diff --git a/src/shared/git-worktree/format-file-changes.ts b/src/shared/git-worktree/format-file-changes.ts index 8abdb269e..8a23dabf8 100644 --- a/src/shared/git-worktree/format-file-changes.ts +++ b/src/shared/git-worktree/format-file-changes.ts @@ -1,5 +1,9 @@ import type { GitFileStat } from "./types" +function normalizePath(path: string): string { + return path.replaceAll("\\", "/") +} + export function formatFileChanges(stats: GitFileStat[], notepadPath?: string): string { if (stats.length === 0) return "[FILE CHANGES SUMMARY]\nNo file changes detected.\n" @@ -34,7 +38,11 @@ export function formatFileChanges(stats: GitFileStat[], notepadPath?: string): s } if (notepadPath) { - const notepadStat = stats.find((s) => s.path.includes("notepad") || s.path.includes(".omo")) + const normalizedNotepadPath = normalizePath(notepadPath) + const notepadStat = stats.find((s) => { + const normalizedPath = normalizePath(s.path) + return normalizedPath === normalizedNotepadPath || normalizedPath.includes(".omo/notepads/") + }) if (notepadStat) { lines.push("[NOTEPAD UPDATED]") lines.push(` ${notepadStat.path} (+${notepadStat.added})`) diff --git a/src/shared/git-worktree/git-worktree.test.ts b/src/shared/git-worktree/git-worktree.test.ts index 27183018b..2c903d1dc 100644 --- a/src/shared/git-worktree/git-worktree.test.ts +++ b/src/shared/git-worktree/git-worktree.test.ts @@ -48,4 +48,21 @@ describe("git-worktree", () => { expect(summary).toContain("src/b.ts") expect(summary).toContain("src/c.ts") }) + + test("#given notepad path #when formatting omo plan changes #then does not report notepad updated", () => { + const summary = formatFileChanges([ + { path: ".omo/plans/work.md", added: 1, removed: 0, status: "modified" }, + ], ".omo/notepads/work/notes.md") + + expect(summary).not.toContain("[NOTEPAD UPDATED]") + }) + + test("#given notepad path #when formatting omo notepad changes #then reports notepad updated", () => { + const summary = formatFileChanges([ + { path: ".omo/notepads/work/notes.md", added: 1, removed: 0, status: "modified" }, + ], ".omo/notepads/work/notes.md") + + expect(summary).toContain("[NOTEPAD UPDATED]") + expect(summary).toContain(".omo/notepads/work/notes.md") + }) }) diff --git a/src/shared/legacy-workspace-migration.test.ts b/src/shared/legacy-workspace-migration.test.ts index 2651de6c8..a293b7293 100644 --- a/src/shared/legacy-workspace-migration.test.ts +++ b/src/shared/legacy-workspace-migration.test.ts @@ -1,5 +1,5 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test" -import { existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from "node:fs" +import { existsSync, mkdirSync, readFileSync, rmSync, symlinkSync, writeFileSync } from "node:fs" import { tmpdir } from "node:os" import { join } from "node:path" import { migrateLegacyWorkspaceDirectory } from "./legacy-workspace-migration" @@ -72,6 +72,23 @@ describe("migrateLegacyWorkspaceDirectory", () => { expect(readFileSync(targetNotepadPath, "utf-8")).toBe("existing note") }) + test("#given legacy workspace contains symlinks #when migrating #then skips symlinks without copying target contents", () => { + // given + const externalFilePath = join(testDirectory, "external-secret.md") + const legacyLinkPath = join(testDirectory, ".sisyphus", "plans", "linked.md") + mkdirSync(join(testDirectory, ".sisyphus", "plans"), { recursive: true }) + writeFileSync(externalFilePath, "secret", "utf-8") + symlinkSync(externalFilePath, legacyLinkPath) + + // when + const result = migrateLegacyWorkspaceDirectory(testDirectory) + + // then + expect(result.migrated).toBe(false) + expect(result.skipped).toContain(join(".omo", "plans", "linked.md")) + expect(existsSync(join(testDirectory, ".omo", "plans", "linked.md"))).toBe(false) + }) + test("#given no legacy workspace #when migrating #then reports no migration", () => { // when const result = migrateLegacyWorkspaceDirectory(testDirectory) diff --git a/src/shared/legacy-workspace-migration.ts b/src/shared/legacy-workspace-migration.ts index 5dac990bb..f2ab98650 100644 --- a/src/shared/legacy-workspace-migration.ts +++ b/src/shared/legacy-workspace-migration.ts @@ -1,4 +1,4 @@ -import { copyFileSync, existsSync, mkdirSync, readdirSync, statSync } from "node:fs" +import { copyFileSync, existsSync, lstatSync, mkdirSync, readdirSync } from "node:fs" import { dirname, join, relative } from "node:path" import { log } from "./logger" @@ -12,10 +12,15 @@ export type LegacyWorkspaceMigrationResult = { } function copyMissingEntries(legacyPath: string, targetPath: string, targetRoot: string, skipped: string[]): boolean { - const legacyStat = statSync(legacyPath) + const legacyStat = lstatSync(legacyPath) + + if (legacyStat.isSymbolicLink()) { + skipped.push(join(WORKSPACE_DIR, relative(targetRoot, targetPath))) + return false + } if (existsSync(targetPath)) { - if (legacyStat.isDirectory() && statSync(targetPath).isDirectory()) { + if (legacyStat.isDirectory() && lstatSync(targetPath).isDirectory()) { let copiedChild = false for (const entry of readdirSync(legacyPath)) { copiedChild = copyMissingEntries(join(legacyPath, entry), join(targetPath, entry), targetRoot, skipped) || copiedChild