diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 1e9b4f437..566c9ed23 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -25,6 +25,10 @@ - Fixed `createAgentSession()` dropping the hidden `resolve` tool from the registry when no active tool sets `deferrable: true`, even though plan mode dispatches the plan-approval `resolve { action: "apply", ... }` call through a standing handler. Read-only plan-mode toolsets (e.g. `read`, `search`, `find`, `web_search`) silently activated plan mode without `resolve`, leaving the agent unable to submit the finalized plan and forcing the user to exit plan mode manually. `resolve` is now kept whenever `plan.enabled` is true, so the standing handler always has a callable tool ([#1428](https://github.com/can1357/oh-my-pi/issues/1428)) +### Fixed + +- Fixed `omp` startup and `/changelog` reading the host project's `CHANGELOG.md` as omp's — `getPackageDir()` no longer falls back to the user's `cwd` when no owning `package.json` is locatable, preventing spurious `lastChangelogVersion` writes ([#1423](https://github.com/can1357/oh-my-pi/issues/1423)) + ## [15.5.3] - 2026-05-27 ### Breaking Changes diff --git a/packages/coding-agent/src/config.ts b/packages/coding-agent/src/config.ts index 0637cc817..fc2b34332 100644 --- a/packages/coding-agent/src/config.ts +++ b/packages/coding-agent/src/config.ts @@ -18,30 +18,57 @@ const priorityList = [ // ============================================================================= /** - * Get the base directory for resolving optional package assets (docs, examples). - * Walk up from import.meta.dir until we find package.json, or fall back to cwd. + * Walk up from `startDir` looking for a `package.json`. Returns the directory + * containing the marker, or `undefined` when the walk hits the filesystem root + * without finding one. + * + * Exported for unit-testing the resolution contract from arbitrary start + * directories (notably the `bun --compile` case where `import.meta.dir` + * resolves to `/$bunfs/root` and no owning package is locatable — issue + * #1423). Production callers should use {@link getPackageDir} instead. */ -export function getPackageDir(): string { - // Allow override via environment variable (useful for Nix/Guix where store paths tokenize poorly) - const envDir = process.env.PI_PACKAGE_DIR; - if (envDir) { - return expandTilde(envDir); - } - - let dir = import.meta.dir; +export function walkUpForPackageDir(startDir: string): string | undefined { + let dir = startDir; while (dir !== path.dirname(dir)) { if (fs.existsSync(path.join(dir, "package.json"))) { return dir; } dir = path.dirname(dir); } - // Fallback to project dir (docs/examples won't be found, but that's fine) - return getProjectDir(); + return undefined; } -/** Get path to CHANGELOG.md (optional, may not exist in binary) */ -export function getChangelogPath(): string { - return path.resolve(path.join(getPackageDir(), "CHANGELOG.md")); +/** + * Get the base directory for resolving optional package assets (docs, examples, CHANGELOG.md). + * + * Honors the `PI_PACKAGE_DIR` override (useful for Nix/Guix store paths); + * otherwise walks up from `import.meta.dir` looking for a `package.json`. + * Returns `undefined` when no owning package is locatable — notably inside + * `bun --compile` binaries where `import.meta.dir` resolves to `/$bunfs/root` + * and the walk hits the filesystem root with nothing found. + * + * Callers MUST treat `undefined` as "no package assets available" and skip the + * lookup. NEVER fall back to the user's `cwd` here: that conflates the host + * project with omp's own assets and was the source of issue #1423 (the host + * project's `CHANGELOG.md` rendered as omp's startup changelog). + */ +export function getPackageDir(): string | undefined { + const envDir = process.env.PI_PACKAGE_DIR; + if (envDir) { + return expandTilde(envDir); + } + return walkUpForPackageDir(import.meta.dir); +} + +/** + * Path to omp's own `CHANGELOG.md`, or `undefined` when the package directory + * cannot be resolved (e.g. inside `bun --compile` binaries that don't bundle + * package assets). Callers MUST skip changelog parsing when this is undefined; + * see issue #1423. + */ +export function getChangelogPath(): string | undefined { + const packageDir = getPackageDir(); + return packageDir ? path.resolve(packageDir, "CHANGELOG.md") : undefined; } // ============================================================================= diff --git a/packages/coding-agent/src/utils/changelog.ts b/packages/coding-agent/src/utils/changelog.ts index c50eb3fac..e931a792a 100644 --- a/packages/coding-agent/src/utils/changelog.ts +++ b/packages/coding-agent/src/utils/changelog.ts @@ -8,10 +8,18 @@ export interface ChangelogEntry { } /** - * Parse changelog entries from CHANGELOG.md - * Scans for ## lines and collects content until next ## or EOF + * Parse changelog entries from the file at `changelogPath`. Scans for `## [x.y.z]` + * headings and collects each block until the next heading or EOF. + * + * Returns `[]` when `changelogPath` is `undefined` (package directory not + * resolvable — see `getChangelogPath`) or the file is missing. Callers MUST NOT + * synthesize a fallback path from the host project's cwd; doing so caused issue + * #1423 (the host project's `CHANGELOG.md` was rendered as omp's). */ -export async function parseChangelog(changelogPath: string): Promise { +export async function parseChangelog(changelogPath: string | undefined): Promise { + if (!changelogPath) { + return []; + } try { const content = await Bun.file(changelogPath).text(); const lines = content.split("\n"); diff --git a/packages/coding-agent/test/issue-1423-repro.test.ts b/packages/coding-agent/test/issue-1423-repro.test.ts new file mode 100644 index 000000000..2e6513a89 --- /dev/null +++ b/packages/coding-agent/test/issue-1423-repro.test.ts @@ -0,0 +1,120 @@ +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it } from "bun:test"; +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; +import { getChangelogPath, getPackageDir, walkUpForPackageDir } from "../src/config"; +import { parseChangelog } from "../src/utils/changelog"; + +/** + * Regression: omp startup parsed the host project's `CHANGELOG.md` as if it + * were omp's, then persisted `lastChangelogVersion` to the global config. Root + * cause was `getPackageDir()` falling back to `getProjectDir()` (the user's + * cwd) when the walk-up from `import.meta.dir` couldn't find a `package.json` + * — which happens in `bun --compile` binaries where `import.meta.dir` + * resolves to `/$bunfs/root`. + * + * Contract this test defends: + * 1. `walkUpForPackageDir()` returns `undefined` when no + * ancestor contains `package.json`. NEVER falls back to `process.cwd()`. + * 2. `parseChangelog(undefined)` returns `[]` so the compiled-binary path + * skips startup display and never mutates `lastChangelogVersion`. + * 3. `getChangelogPath()` (when defined) never points under the host + * project's `cwd`. + * 4. `PI_PACKAGE_DIR` overrides anchor exactly to the override directory and + * do not fall back to `cwd`. + */ +describe("issue #1423 — package-dir lookup must not fall back to cwd", () => { + let projectDir: string; + let originalCwd: string; + let originalEnv: string | undefined; + + beforeAll(() => { + originalEnv = process.env.PI_PACKAGE_DIR; + delete process.env.PI_PACKAGE_DIR; + }); + + afterAll(() => { + if (originalEnv === undefined) { + delete process.env.PI_PACKAGE_DIR; + } else { + process.env.PI_PACKAGE_DIR = originalEnv; + } + }); + + beforeEach(() => { + originalCwd = process.cwd(); + projectDir = fs.mkdtempSync(path.join(os.tmpdir(), "omp-issue-1423-")); + fs.writeFileSync( + path.join(projectDir, "CHANGELOG.md"), + "# Changelog\n\n## [99.0.0] - 2099-01-01\n\n### Added\n\n- Host project entry, NOT omp's.\n", + ); + // Critical: no package.json in projectDir — only CHANGELOG.md. + process.chdir(projectDir); + }); + + afterEach(() => { + process.chdir(originalCwd); + fs.rmSync(projectDir, { recursive: true, force: true }); + }); + + it("walkUpForPackageDir returns undefined when no ancestor owns a package.json", () => { + // Find a tmpdir whose ancestors (up to /) have no package.json. Assuming + // /tmp/{deeply,nested} on Linux/macOS, this is the realistic bunfs case. + const isolated = fs.mkdtempSync(path.join(os.tmpdir(), "omp-issue-1423-isolated-")); + const deep = path.join(isolated, "a", "b", "c"); + fs.mkdirSync(deep, { recursive: true }); + try { + // Sanity: confirm no ancestor of `deep` has a package.json. If a + // host machine happens to ship `/package.json`, this assertion + // surfaces it instead of letting the test silently pass. + for (let dir = deep; dir !== path.dirname(dir); dir = path.dirname(dir)) { + expect(fs.existsSync(path.join(dir, "package.json"))).toBe(false); + } + expect(walkUpForPackageDir(deep)).toBeUndefined(); + } finally { + fs.rmSync(isolated, { recursive: true, force: true }); + } + }); + + it("walkUpForPackageDir from cwd-with-CHANGELOG-only returns undefined, not cwd", () => { + // Before the fix, `getPackageDir()` would have returned `getProjectDir()` + // (≈ `process.cwd()` = `projectDir`) once the walk-up failed. The pure + // helper exercises the exact resolution shape from an arbitrary start + // directory, proving the fallback was removed at the source. + expect(walkUpForPackageDir(projectDir)).toBeUndefined(); + }); + + it("parseChangelog(undefined) yields no entries", async () => { + const entries = await parseChangelog(undefined); + expect(entries).toEqual([]); + }); + + it("getChangelogPath()'s real-tree result never points under host cwd", () => { + const changelogPath = getChangelogPath(); + // In dev/test runs from the workspace, walk-up succeeds and resolves + // to omp's own CHANGELOG.md. The host project's `cwd` MUST not bleed + // into the resolution either way. + if (changelogPath !== undefined) { + expect(changelogPath.startsWith(projectDir)).toBe(false); + } + }); + + it("host project's CHANGELOG.md is never parsed as omp's", async () => { + const changelogPath = getChangelogPath(); + const entries = await parseChangelog(changelogPath); + const hostEntry = entries.find(e => e.major === 99 && e.minor === 0 && e.patch === 0); + expect(hostEntry).toBeUndefined(); + }); + + it("PI_PACKAGE_DIR override stays put and does not fall back to cwd", () => { + const bogusDir = fs.mkdtempSync(path.join(os.tmpdir(), "omp-issue-1423-override-")); + try { + process.env.PI_PACKAGE_DIR = bogusDir; + expect(getPackageDir()).toBe(bogusDir); + expect(getChangelogPath()).toBe(path.join(bogusDir, "CHANGELOG.md")); + } finally { + delete process.env.PI_PACKAGE_DIR; + fs.rmSync(bogusDir, { recursive: true, force: true }); + } + }); +});