From 932da826788c90e263378a53a2e9bab36c0315a7 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 23 Jul 2026 11:43:22 -0500 Subject: [PATCH 1/4] fix(show): resolve changes by directory instead of requiring proposal.md `openspec show ` and shell completion resolved a change only when `openspec/changes//proposal.md` existed. Every sibling command -- `list`, `status`, `instructions`, `validate` -- resolves a change by its directory (`getAvailableChanges`). The two rules disagree the moment a change is created: `openspec new change ` scaffolds only `.openspec.yaml`, so `list` showed the change while `show` reported `Unknown item`. A custom schema that defines no proposal artifact was never resolvable at all (#1161). Resolve by directory in `getActiveChangeIds`/`getArchivedChangeIds`, and report a change that exists without a proposal accurately -- pointing at `openspec status --change ` -- rather than as missing. The deprecated `openspec change list` keeps its own proposal-backed scan; its JSON output parses proposal.md per change. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../show-resolves-proposalless-changes.md | 5 ++ src/commands/change.ts | 17 +++- src/utils/item-discovery.ts | 51 +++++------ test/commands/show.test.ts | 24 ++++++ test/utils/item-discovery.test.ts | 85 +++++++++++++++++++ 5 files changed, 157 insertions(+), 25 deletions(-) create mode 100644 .changeset/show-resolves-proposalless-changes.md create mode 100644 test/utils/item-discovery.test.ts diff --git a/.changeset/show-resolves-proposalless-changes.md b/.changeset/show-resolves-proposalless-changes.md new file mode 100644 index 0000000000..c66bd534b7 --- /dev/null +++ b/.changeset/show-resolves-proposalless-changes.md @@ -0,0 +1,5 @@ +--- +'@fission-ai/openspec': patch +--- + +`openspec show ` and shell completion now resolve a change by its directory, matching `list`, `status`, `instructions`, and `validate`. Previously they required `proposal.md`, so a change created by `openspec new change` — which scaffolds only `.openspec.yaml` — was reported as `Unknown item` until the proposal was written, and a change from a schema with no proposal artifact was never resolvable. Showing a change that has no proposal yet now says so and points at `openspec status --change `. diff --git a/src/commands/change.ts b/src/commands/change.ts index 5df0f94140..62a31bd6ff 100644 --- a/src/commands/change.ts +++ b/src/commands/change.ts @@ -59,11 +59,26 @@ export class ChangeCommand { } } - const proposalPath = path.join(changesPath, changeName, 'proposal.md'); + const changeDir = path.join(changesPath, changeName); + const proposalPath = path.join(changeDir, 'proposal.md'); try { await fs.access(proposalPath); } catch { + // A change can exist without a proposal: `openspec new change` scaffolds + // only .openspec.yaml, and a custom schema need not define a proposal + // artifact. Say which of the two cases this is instead of reporting a + // change that does exist as missing. + const changeExists = await fs + .access(changeDir) + .then(() => true) + .catch(() => false); + if (changeExists) { + throw new Error( + `Change "${changeName}" has no proposal.md yet. ` + + `Run "openspec status --change ${changeName}" to see which artifact comes next.` + ); + } throw new Error(`Change "${changeName}" not found at ${proposalPath}`); } diff --git a/src/utils/item-discovery.ts b/src/utils/item-discovery.ts index 7c3d547d25..65d5b45fab 100644 --- a/src/utils/item-discovery.ts +++ b/src/utils/item-discovery.ts @@ -2,22 +2,25 @@ import { promises as fs } from 'fs'; import path from 'path'; import { discoverSpecFiles } from './spec-discovery.js'; +/** + * Returns the ids of active changes: every directory under openspec/changes/ + * except the archive and hidden directories. + * + * A change is resolved by its directory alone - the same rule `list`, + * `status`, `instructions` and `validate` use (`getAvailableChanges`). + * Requiring proposal.md here made `openspec show` and shell completion miss + * changes those commands resolve: `openspec new change ` scaffolds only + * `.openspec.yaml`, and a custom schema need not define a proposal artifact at + * all (#1161). + */ export async function getActiveChangeIds(root: string = process.cwd()): Promise { const changesPath = path.join(root, 'openspec', 'changes'); try { const entries = await fs.readdir(changesPath, { withFileTypes: true }); - const result: string[] = []; - for (const entry of entries) { - if (!entry.isDirectory() || entry.name.startsWith('.') || entry.name === 'archive') continue; - const proposalPath = path.join(changesPath, entry.name, 'proposal.md'); - try { - await fs.access(proposalPath); - result.push(entry.name); - } catch { - // skip directories without proposal.md - } - } - return result.sort(); + return entries + .filter((entry) => entry.isDirectory() && entry.name !== 'archive' && !entry.name.startsWith('.')) + .map((entry) => entry.name) + .sort(); } catch { return []; } @@ -29,22 +32,22 @@ export async function getSpecIds(root: string = process.cwd()): Promise spec.id); } +/** + * Returns the ids of archived changes: every directory under + * openspec/changes/archive/ except hidden directories. + * + * Resolved by directory for the same reason as `getActiveChangeIds`: a change + * archived from a schema without a proposal artifact has no proposal.md, and + * gating on it hid those entries from shell completion. + */ export async function getArchivedChangeIds(root: string = process.cwd()): Promise { const archivePath = path.join(root, 'openspec', 'changes', 'archive'); try { const entries = await fs.readdir(archivePath, { withFileTypes: true }); - const result: string[] = []; - for (const entry of entries) { - if (!entry.isDirectory() || entry.name.startsWith('.')) continue; - const proposalPath = path.join(archivePath, entry.name, 'proposal.md'); - try { - await fs.access(proposalPath); - result.push(entry.name); - } catch { - // skip directories without proposal.md - } - } - return result.sort(); + return entries + .filter((entry) => entry.isDirectory() && !entry.name.startsWith('.')) + .map((entry) => entry.name) + .sort(); } catch { return []; } diff --git a/test/commands/show.test.ts b/test/commands/show.test.ts index ee99a2f416..f9a9ea6ee3 100644 --- a/test/commands/show.test.ts +++ b/test/commands/show.test.ts @@ -101,6 +101,30 @@ describe('top-level show command', () => { } }); + it('resolves a scaffolded change that has no proposal.md yet', async () => { + // `openspec new change ` writes only .openspec.yaml, so `show` must + // resolve the change the same way `list` and `status` already do. + await fs.mkdir(path.join(changesDir, 'scaffolded'), { recursive: true }); + await fs.writeFile(path.join(changesDir, 'scaffolded', '.openspec.yaml'), 'schema: spec-driven\n', 'utf-8'); + + const originalCwd = process.cwd(); + try { + process.chdir(testDir); + let err: any; + try { + execFileSync('node', [openspecBin, 'show', 'scaffolded'], { encoding: 'utf-8' }); + } catch (e) { err = e; } + expect(err).toBeDefined(); + const stderr = err.stderr.toString(); + // Resolved as a change, not rejected as an unknown item. + expect(stderr).not.toContain('Unknown item'); + expect(stderr).toContain('has no proposal.md yet'); + expect(stderr).toContain('openspec status --change scaffolded'); + } finally { + process.chdir(originalCwd); + } + }); + it('prints nearest matches when not found', () => { const originalCwd = process.cwd(); try { diff --git a/test/utils/item-discovery.test.ts b/test/utils/item-discovery.test.ts new file mode 100644 index 0000000000..d831374a7a --- /dev/null +++ b/test/utils/item-discovery.test.ts @@ -0,0 +1,85 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { promises as fs } from 'fs'; +import os from 'os'; +import path from 'path'; +import { getActiveChangeIds, getArchivedChangeIds } from '../../src/utils/item-discovery.js'; + +describe('item discovery', () => { + let root: string; + let changesDir: string; + + beforeEach(async () => { + root = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-item-discovery-')); + changesDir = path.join(root, 'openspec', 'changes'); + await fs.mkdir(changesDir, { recursive: true }); + }); + + afterEach(async () => { + await fs.rm(root, { recursive: true, force: true }); + }); + + const makeChange = async (name: string, files: Record = {}) => { + const dir = path.join(changesDir, name); + await fs.mkdir(dir, { recursive: true }); + for (const [file, content] of Object.entries(files)) { + await fs.writeFile(path.join(dir, file), content, 'utf-8'); + } + }; + + describe('getActiveChangeIds', () => { + it('resolves a scaffolded change that has no proposal.md', async () => { + // What `openspec new change ` leaves on disk: metadata only. + await makeChange('scaffolded', { '.openspec.yaml': 'schema: spec-driven\n' }); + await makeChange('with-proposal', { 'proposal.md': '# With proposal' }); + + expect(await getActiveChangeIds(root)).toEqual(['scaffolded', 'with-proposal']); + }); + + it('resolves a change whose schema defines no proposal artifact', async () => { + await makeChange('no-proposal-schema', { + '.openspec.yaml': 'schema: custom\n', + 'tasks.md': '## 1. Work\n\n- [ ] 1.1 do it\n', + }); + + expect(await getActiveChangeIds(root)).toEqual(['no-proposal-schema']); + }); + + it('excludes the archive directory and hidden directories', async () => { + await makeChange('real-change'); + await fs.mkdir(path.join(changesDir, 'archive', '2026-01-01-old'), { recursive: true }); + await fs.mkdir(path.join(changesDir, '.scratch'), { recursive: true }); + await fs.writeFile(path.join(changesDir, 'stray-file.md'), 'not a change', 'utf-8'); + + expect(await getActiveChangeIds(root)).toEqual(['real-change']); + }); + + it('returns an empty list when the changes directory is missing', async () => { + await fs.rm(changesDir, { recursive: true, force: true }); + + expect(await getActiveChangeIds(root)).toEqual([]); + }); + }); + + describe('getArchivedChangeIds', () => { + it('resolves archived changes without requiring proposal.md', async () => { + const archiveDir = path.join(changesDir, 'archive'); + await fs.mkdir(path.join(archiveDir, '2026-01-02-no-proposal'), { recursive: true }); + await fs.mkdir(path.join(archiveDir, '2026-01-01-with-proposal'), { recursive: true }); + await fs.writeFile( + path.join(archiveDir, '2026-01-01-with-proposal', 'proposal.md'), + '# Archived', + 'utf-8' + ); + await fs.mkdir(path.join(archiveDir, '.tmp'), { recursive: true }); + + expect(await getArchivedChangeIds(root)).toEqual([ + '2026-01-01-with-proposal', + '2026-01-02-no-proposal', + ]); + }); + + it('returns an empty list when nothing has been archived', async () => { + expect(await getArchivedChangeIds(root)).toEqual([]); + }); + }); +}); From 67aca4ad0dd92bb5f61acbd347d9d60ce4fc4467 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 23 Jul 2026 11:50:54 -0500 Subject: [PATCH 2/4] fix(show): offer proposal-less changes in the no-name selector too `ChangeCommand.show` resolved a named proposal-less change but still built its no-name selector (and the non-interactive "Available IDs" hint) from the proposal-gated scan, so a scaffolded change could not be picked. Use directory-based discovery there as well. `list` keeps the local proposal-backed scan: its --json output parses proposal.md per change. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/commands/change.ts | 5 ++++- test/commands/show.test.ts | 23 +++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/src/commands/change.ts b/src/commands/change.ts index 62a31bd6ff..61266386bc 100644 --- a/src/commands/change.ts +++ b/src/commands/change.ts @@ -39,7 +39,10 @@ export class ChangeCommand { if (!changeName) { const canPrompt = isInteractive(options); - const changes = await this.getActiveChanges(changesPath); + // Offer the same changes `show ` can resolve. `list` keeps the + // proposal-backed scan below because its --json output parses + // proposal.md for every change it reports. + const changes = await getActiveChangeIds(this.rootPath ?? process.cwd()); if (canPrompt && changes.length > 0) { const { select } = await import('@inquirer/prompts'); const selected = await select({ diff --git a/test/commands/show.test.ts b/test/commands/show.test.ts index f9a9ea6ee3..a606b1fe53 100644 --- a/test/commands/show.test.ts +++ b/test/commands/show.test.ts @@ -125,6 +125,29 @@ describe('top-level show command', () => { } }); + it('offers a scaffolded change when "change show" is called without a name', async () => { + await fs.mkdir(path.join(changesDir, 'scaffolded'), { recursive: true }); + await fs.writeFile(path.join(changesDir, 'scaffolded', '.openspec.yaml'), 'schema: spec-driven\n', 'utf-8'); + + const originalCwd = process.cwd(); + const originalEnv = { ...process.env }; + try { + process.chdir(testDir); + process.env.OPEN_SPEC_INTERACTIVE = '0'; + let err: any; + try { + execFileSync('node', [openspecBin, 'change', 'show'], { encoding: 'utf-8' }); + } catch (e) { err = e; } + expect(err).toBeDefined(); + const stderr = err.stderr.toString(); + expect(stderr).toContain('Available IDs:'); + expect(stderr).toContain('scaffolded'); + } finally { + process.chdir(originalCwd); + process.env = originalEnv; + } + }); + it('prints nearest matches when not found', () => { const originalCwd = process.cwd(); try { From e48e4fd952d53f9b4ed3db23a07202601c9a7b27 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 23 Jul 2026 12:10:02 -0500 Subject: [PATCH 3/4] fix(change): unify list/show discovery and report a missing proposal honestly Adversarial review of the first two commits surfaced four defects. `change list` still used a private proposal-gated scan, so the deprecated alias reported a different set than `openspec list` -- the command its own deprecation warning tells you to use -- while the `show` selector beside it offered the wider set. Move it to `getActiveChangeIds` and drop the now unused helper and its ARCHIVE_DIR constant. Widening that list exposed three follow-on bugs, all fixed here: - Task counts were computed inside the proposal try block, so a change with tasks but no proposal.md reported 0/0. Task progress is independent of the proposal; resolve it first. - `--long` printed "(unable to read)" and `--json` "Unknown" for a change that is simply not written yet. Distinguish a missing proposal from an unreadable one by testing existence, not by sniffing error codes. - `show` reported a stray file under changes/, or a traversing name such as `../..`, as a change awaiting its proposal, pointing the user at a `status --change` call that cannot work. Require a directory that is a direct child of changes/. Also drops a duplicated proposal.md read in the --long path. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../show-resolves-proposalless-changes.md | 4 +- src/commands/change.ts | 102 +++++++++--------- .../core/commands/change-command.list.test.ts | 60 +++++++++++ .../change-command.show-validate.test.ts | 23 ++++ 4 files changed, 137 insertions(+), 52 deletions(-) diff --git a/.changeset/show-resolves-proposalless-changes.md b/.changeset/show-resolves-proposalless-changes.md index c66bd534b7..918c07f242 100644 --- a/.changeset/show-resolves-proposalless-changes.md +++ b/.changeset/show-resolves-proposalless-changes.md @@ -2,4 +2,6 @@ '@fission-ai/openspec': patch --- -`openspec show ` and shell completion now resolve a change by its directory, matching `list`, `status`, `instructions`, and `validate`. Previously they required `proposal.md`, so a change created by `openspec new change` — which scaffolds only `.openspec.yaml` — was reported as `Unknown item` until the proposal was written, and a change from a schema with no proposal artifact was never resolvable. Showing a change that has no proposal yet now says so and points at `openspec status --change `. +Change lookup no longer requires `proposal.md`. `openspec show`, `openspec change list/show/validate`, and shell completion now resolve a change by its directory, matching `openspec list`, `status`, `instructions`, and `validate`. + +Previously a change created by `openspec new change` — which scaffolds only `.openspec.yaml` — was reported as `Unknown item` by `openspec show` and was missing from completions and `openspec change list` until a proposal was written, and a change from a schema with no proposal artifact was never resolvable. `openspec change list` now reports the same set as `openspec list`, keeps task counts for a change that has no proposal yet, and labels it `(no proposal.md yet)` rather than `(unable to read)`. Showing such a change explains that the proposal is not written yet and points at `openspec status --change `. diff --git a/src/commands/change.ts b/src/commands/change.ts index 61266386bc..2de01e3a9b 100644 --- a/src/commands/change.ts +++ b/src/commands/change.ts @@ -10,8 +10,12 @@ import { isInteractive } from '../utils/interactive.js'; import { getActiveChangeIds } from '../utils/item-discovery.js'; import { getTaskProgressForChange } from '../utils/task-progress.js'; -// Constants for better maintainability -const ARCHIVE_DIR = 'archive'; +async function pathExists(target: string): Promise { + return fs + .access(target) + .then(() => true) + .catch(() => false); +} export class ChangeCommand { private converter: JsonConverter; @@ -39,9 +43,7 @@ export class ChangeCommand { if (!changeName) { const canPrompt = isInteractive(options); - // Offer the same changes `show ` can resolve. `list` keeps the - // proposal-backed scan below because its --json output parses - // proposal.md for every change it reports. + // Offer exactly the changes `show ` can resolve. const changes = await getActiveChangeIds(this.rootPath ?? process.cwd()); if (canPrompt && changes.length > 0) { const { select } = await import('@inquirer/prompts'); @@ -72,11 +74,17 @@ export class ChangeCommand { // only .openspec.yaml, and a custom schema need not define a proposal // artifact. Say which of the two cases this is instead of reporting a // change that does exist as missing. - const changeExists = await fs - .access(changeDir) - .then(() => true) - .catch(() => false); - if (changeExists) { + // + // Only a directory that is a direct child of changes/ qualifies. A stray + // file, or a traversing name like `../..`, is not a change, and naming it + // one would point the user at a `status --change` call that cannot work. + const isChangeDirectory = + path.dirname(path.resolve(changeDir)) === path.resolve(changesPath) && + (await fs + .stat(changeDir) + .then((stats) => stats.isDirectory()) + .catch(() => false)); + if (isChangeDirectory) { throw new Error( `Change "${changeName}" has no proposal.md yet. ` + `Run "openspec status --change ${changeName}" to see which artifact comes next.` @@ -120,36 +128,44 @@ export class ChangeCommand { async list(options?: { json?: boolean; long?: boolean }): Promise { const changesPath = path.join(process.cwd(), 'openspec', 'changes'); - const changes = await this.getActiveChanges(changesPath); - + // Same directory-based resolution as `openspec list`, the command this + // deprecated alias points users at. Every output path below already + // tolerates a change whose proposal.md is missing or unreadable. + const changes = await getActiveChangeIds(); + if (options?.json) { const changeDetails = await Promise.all( changes.map(async (changeName) => { - const proposalPath = path.join(changesPath, changeName, 'proposal.md'); + const changeDir = path.join(changesPath, changeName); + const proposalPath = path.join(changeDir, 'proposal.md'); + + // Resolve task progress through the shared tracked-tasks helper so + // this deprecated noun-form list cannot re-fork the resolution + // (#1202). Tasks are independent of the proposal: a change can carry + // tasks before, or without, a proposal.md. + const taskStatus = await getTaskProgressForChange(changesPath, changeName, process.cwd()); + + // No proposal yet is an ordinary state (scaffolded change, or a + // schema with no proposal artifact), so name the change rather than + // labelling it Unknown. Unknown stays for a proposal that exists but + // cannot be read or parsed. + if (!(await pathExists(proposalPath))) { + return { id: changeName, title: changeName, deltaCount: 0, taskStatus }; + } try { const content = await fs.readFile(proposalPath, 'utf-8'); - const changeDir = path.join(changesPath, changeName); const parser = new ChangeParser(content, changeDir); const change = await parser.parseChangeWithDeltas(changeName); - // Resolve task progress through the shared tracked-tasks helper so - // this deprecated noun-form list cannot re-fork the resolution (#1202). - const taskStatus = await getTaskProgressForChange(changesPath, changeName, process.cwd()); - return { id: changeName, title: this.extractTitle(content, changeName), deltaCount: change.deltas.length, taskStatus, }; - } catch (error) { - return { - id: changeName, - title: 'Unknown', - deltaCount: 0, - taskStatus: { total: 0, completed: 0 }, - }; + } catch { + return { id: changeName, title: 'Unknown', deltaCount: 0, taskStatus }; } }) ); @@ -170,19 +186,23 @@ export class ChangeCommand { // Long format: id: title and minimal counts for (const changeName of sorted) { - const proposalPath = path.join(changesPath, changeName, 'proposal.md'); + const changeDir = path.join(changesPath, changeName); + const proposalPath = path.join(changeDir, 'proposal.md'); + const { total, completed } = await getTaskProgressForChange(changesPath, changeName, process.cwd()); + const taskStatusText = total > 0 ? ` [tasks ${completed}/${total}]` : ''; + if (!(await pathExists(proposalPath))) { + console.log(`${changeName}: (no proposal.md yet)${taskStatusText}`); + continue; + } try { const content = await fs.readFile(proposalPath, 'utf-8'); const title = this.extractTitle(content, changeName); - const { total, completed } = await getTaskProgressForChange(changesPath, changeName, process.cwd()); - const taskStatusText = total > 0 ? ` [tasks ${completed}/${total}]` : ''; - const changeDir = path.join(changesPath, changeName); - const parser = new ChangeParser(await fs.readFile(proposalPath, 'utf-8'), changeDir); + const parser = new ChangeParser(content, changeDir); const change = await parser.parseChangeWithDeltas(changeName); const deltaCountText = ` [deltas ${change.deltas.length}]`; console.log(`${changeName}: ${title}${deltaCountText}${taskStatusText}`); } catch { - console.log(`${changeName}: (unable to read)`); + console.log(`${changeName}: (unable to read)${taskStatusText}`); } } } @@ -245,26 +265,6 @@ export class ChangeCommand { } } - private async getActiveChanges(changesPath: string): Promise { - try { - const entries = await fs.readdir(changesPath, { withFileTypes: true }); - const result: string[] = []; - for (const entry of entries) { - if (!entry.isDirectory() || entry.name.startsWith('.') || entry.name === ARCHIVE_DIR) continue; - const proposalPath = path.join(changesPath, entry.name, 'proposal.md'); - try { - await fs.access(proposalPath); - result.push(entry.name); - } catch { - // skip directories without proposal.md - } - } - return result.sort(); - } catch { - return []; - } - } - private extractTitle(content: string, changeName: string): string { const match = content.match(/^#\s+(?:Change:\s+)?(.+)$/im); return match ? match[1].trim() : changeName; diff --git a/test/core/commands/change-command.list.test.ts b/test/core/commands/change-command.list.test.ts index 9ec1df5a1d..fdd72c8f18 100644 --- a/test/core/commands/change-command.list.test.ts +++ b/test/core/commands/change-command.list.test.ts @@ -72,5 +72,65 @@ describe('ChangeCommand.list', () => { } finally { console.log = origLog; } + + }); +}); + +describe('ChangeCommand.list with a change that has no proposal.md', () => { + let cmd: ChangeCommand; + let tempRoot: string; + let originalCwd: string; + + const capture = async (run: () => Promise): Promise => { + const logs: string[] = []; + const origLog = console.log; + try { + console.log = (msg?: any, ...args: any[]) => { + logs.push([msg, ...args].filter(Boolean).join(' ')); + }; + await run(); + return logs.join('\n'); + } finally { + console.log = origLog; + } + }; + + beforeAll(async () => { + cmd = new ChangeCommand(); + originalCwd = process.cwd(); + tempRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-change-list-noproposal-')); + // What `openspec new change` leaves behind, plus tasks: no proposal.md. + const scaffolded = path.join(tempRoot, 'openspec', 'changes', 'scaffolded'); + await fs.mkdir(scaffolded, { recursive: true }); + await fs.writeFile(path.join(scaffolded, '.openspec.yaml'), 'schema: spec-driven\n', 'utf-8'); + await fs.writeFile(path.join(scaffolded, 'tasks.md'), '- [x] Task 1\n- [ ] Task 2\n', 'utf-8'); + process.chdir(tempRoot); + }); + + afterAll(async () => { + process.chdir(originalCwd); + await fs.rm(tempRoot, { recursive: true, force: true }); + }); + + it('lists it, matching what `openspec list` resolves', async () => { + expect(await capture(() => cmd.list({}))).toContain('scaffolded'); + }); + + it('--long reports the missing proposal and keeps task counts', async () => { + const out = await capture(() => cmd.list({ long: true })); + expect(out).toContain('scaffolded: (no proposal.md yet)'); + expect(out).toContain('[tasks 1/2]'); + expect(out).not.toContain('(unable to read)'); + }); + + it('--json names the change instead of "Unknown" and keeps task counts', async () => { + const parsed = JSON.parse(await capture(() => cmd.list({ json: true }))); + expect(parsed).toHaveLength(1); + expect(parsed[0]).toMatchObject({ + id: 'scaffolded', + title: 'scaffolded', + deltaCount: 0, + taskStatus: { total: 2, completed: 1 }, + }); }); }); diff --git a/test/core/commands/change-command.show-validate.test.ts b/test/core/commands/change-command.show-validate.test.ts index 5442a52cbf..2048603288 100644 --- a/test/core/commands/change-command.show-validate.test.ts +++ b/test/core/commands/change-command.show-validate.test.ts @@ -89,6 +89,29 @@ describe('ChangeCommand.show/validate', () => { } }); + describe('resolving a change that has no proposal.md', () => { + it('names the missing proposal and points at status', async () => { + await fs.mkdir(path.join(tempRoot, 'openspec', 'changes', 'scaffolded'), { recursive: true }); + + await expect(cmd.show('scaffolded', { json: false })).rejects.toThrow( + /Change "scaffolded" has no proposal\.md yet\..*openspec status --change scaffolded/s + ); + }); + + it('does not treat a stray file under changes/ as a change', async () => { + await fs.writeFile(path.join(tempRoot, 'openspec', 'changes', 'notes.md'), 'not a change', 'utf-8'); + + // Must stay the plain not-found error: `status --change notes.md` cannot work. + await expect(cmd.show('notes.md', { json: false })).rejects.toThrow(/not found at/); + await expect(cmd.show('notes.md', { json: false })).rejects.not.toThrow(/has no proposal\.md yet/); + }); + + it('does not treat a traversing name as a change', async () => { + await expect(cmd.show('../..', { json: false })).rejects.toThrow(/not found at/); + await expect(cmd.show('../..', { json: false })).rejects.not.toThrow(/has no proposal\.md yet/); + }); + }); + it('validate --strict --json returns a report with valid boolean', async () => { const logs: string[] = []; const origLog = console.log; From 66cd50c0acaa6133f5f9ca0d1b4f79839f6e8624 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Thu, 23 Jul 2026 12:19:34 -0500 Subject: [PATCH 4/4] fix(change): contain change lookup and stop guessing at unreadable proposals Second review round. `isDefinitelyMissing` replaces the plain existence check: fs.access can fail with EACCES or an I/O error, and treating that as "no proposal.md yet" hid a real read failure behind an ordinary-looking state. Only ENOENT counts as absent; anything else falls through to the existing unreadable handling. `show` now rejects a name that is not a direct child of changes/ before touching the filesystem. This closes a pre-existing traversal on main: `openspec change show ../..` resolved openspec/changes/../../proposal.md and printed a file from outside the changes directory. Both new tests fail without the guard. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/commands/change.ts | 46 ++++++++++++------- .../change-command.show-validate.test.ts | 19 ++++++-- 2 files changed, 46 insertions(+), 19 deletions(-) diff --git a/src/commands/change.ts b/src/commands/change.ts index 2de01e3a9b..f9a1995496 100644 --- a/src/commands/change.ts +++ b/src/commands/change.ts @@ -10,11 +10,25 @@ import { isInteractive } from '../utils/interactive.js'; import { getActiveChangeIds } from '../utils/item-discovery.js'; import { getTaskProgressForChange } from '../utils/task-progress.js'; -async function pathExists(target: string): Promise { +/** + * True only when `target` is definitively absent. An EACCES or I/O failure + * means existence cannot be determined, so callers fall through to their + * read-error path rather than claim the file was never written. + */ +async function isDefinitelyMissing(target: string): Promise { return fs .access(target) - .then(() => true) - .catch(() => false); + .then(() => false) + .catch((error: NodeJS.ErrnoException) => error?.code === 'ENOENT'); +} + +/** + * A change is a directory directly under changes/. Rejecting anything else up + * front keeps a traversing name (`../..`) from reading a proposal outside the + * changes directory, and keeps the missing-proposal message honest. + */ +function isChangeDirectoryName(changesPath: string, changeDir: string): boolean { + return path.dirname(path.resolve(changeDir)) === path.resolve(changesPath); } export class ChangeCommand { @@ -67,23 +81,23 @@ export class ChangeCommand { const changeDir = path.join(changesPath, changeName); const proposalPath = path.join(changeDir, 'proposal.md'); + if (!isChangeDirectoryName(changesPath, changeDir)) { + throw new Error(`Change "${changeName}" not found at ${proposalPath}`); + } + try { await fs.access(proposalPath); } catch { // A change can exist without a proposal: `openspec new change` scaffolds // only .openspec.yaml, and a custom schema need not define a proposal // artifact. Say which of the two cases this is instead of reporting a - // change that does exist as missing. - // - // Only a directory that is a direct child of changes/ qualifies. A stray - // file, or a traversing name like `../..`, is not a change, and naming it - // one would point the user at a `status --change` call that cannot work. - const isChangeDirectory = - path.dirname(path.resolve(changeDir)) === path.resolve(changesPath) && - (await fs - .stat(changeDir) - .then((stats) => stats.isDirectory()) - .catch(() => false)); + // change that does exist as missing. A stray file under changes/ is not a + // change, and naming it one would point the user at a `status --change` + // call that cannot work. + const isChangeDirectory = await fs + .stat(changeDir) + .then((stats) => stats.isDirectory()) + .catch(() => false); if (isChangeDirectory) { throw new Error( `Change "${changeName}" has no proposal.md yet. ` + @@ -149,7 +163,7 @@ export class ChangeCommand { // schema with no proposal artifact), so name the change rather than // labelling it Unknown. Unknown stays for a proposal that exists but // cannot be read or parsed. - if (!(await pathExists(proposalPath))) { + if (await isDefinitelyMissing(proposalPath)) { return { id: changeName, title: changeName, deltaCount: 0, taskStatus }; } @@ -190,7 +204,7 @@ export class ChangeCommand { const proposalPath = path.join(changeDir, 'proposal.md'); const { total, completed } = await getTaskProgressForChange(changesPath, changeName, process.cwd()); const taskStatusText = total > 0 ? ` [tasks ${completed}/${total}]` : ''; - if (!(await pathExists(proposalPath))) { + if (await isDefinitelyMissing(proposalPath)) { console.log(`${changeName}: (no proposal.md yet)${taskStatusText}`); continue; } diff --git a/test/core/commands/change-command.show-validate.test.ts b/test/core/commands/change-command.show-validate.test.ts index 2048603288..e0247ae4df 100644 --- a/test/core/commands/change-command.show-validate.test.ts +++ b/test/core/commands/change-command.show-validate.test.ts @@ -106,9 +106,22 @@ describe('ChangeCommand.show/validate', () => { await expect(cmd.show('notes.md', { json: false })).rejects.not.toThrow(/has no proposal\.md yet/); }); - it('does not treat a traversing name as a change', async () => { - await expect(cmd.show('../..', { json: false })).rejects.toThrow(/not found at/); - await expect(cmd.show('../..', { json: false })).rejects.not.toThrow(/has no proposal\.md yet/); + it('does not read a proposal outside changes/ via a traversing name', async () => { + // Reachable target: openspec/changes/../../proposal.md is tempRoot/proposal.md. + // Without containment this resolves and the file is printed verbatim. + await fs.writeFile(path.join(tempRoot, 'proposal.md'), '# Outside the changes directory', 'utf-8'); + const traversal = path.join('..', '..'); + + await expect(cmd.show(traversal, { json: false })).rejects.toThrow(/not found at/); + await expect(cmd.show(traversal, { json: false })).rejects.not.toThrow(/has no proposal\.md yet/); + }); + + it('does not treat a nested name as a change', async () => { + const nested = path.join('sample-change', 'specs'); + await fs.mkdir(path.join(tempRoot, 'openspec', 'changes', 'sample-change', 'specs'), { recursive: true }); + + await expect(cmd.show(nested, { json: false })).rejects.toThrow(/not found at/); + await expect(cmd.show(nested, { json: false })).rejects.not.toThrow(/has no proposal\.md yet/); }); });