diff --git a/.changeset/show-resolves-proposalless-changes.md b/.changeset/show-resolves-proposalless-changes.md new file mode 100644 index 0000000000..918c07f242 --- /dev/null +++ b/.changeset/show-resolves-proposalless-changes.md @@ -0,0 +1,7 @@ +--- +'@fission-ai/openspec': patch +--- + +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 5df0f94140..f9a1995496 100644 --- a/src/commands/change.ts +++ b/src/commands/change.ts @@ -10,8 +10,26 @@ 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'; +/** + * 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(() => 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 { private converter: JsonConverter; @@ -39,7 +57,8 @@ export class ChangeCommand { if (!changeName) { const canPrompt = isInteractive(options); - const changes = await this.getActiveChanges(changesPath); + // 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'); const selected = await select({ @@ -59,11 +78,32 @@ export class ChangeCommand { } } - const proposalPath = path.join(changesPath, changeName, 'proposal.md'); + 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. 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. ` + + `Run "openspec status --change ${changeName}" to see which artifact comes next.` + ); + } throw new Error(`Change "${changeName}" not found at ${proposalPath}`); } @@ -102,36 +142,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 isDefinitelyMissing(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 }; } }) ); @@ -152,19 +200,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 isDefinitelyMissing(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}`); } } } @@ -227,26 +279,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/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..a606b1fe53 100644 --- a/test/commands/show.test.ts +++ b/test/commands/show.test.ts @@ -101,6 +101,53 @@ 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('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 { 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..e0247ae4df 100644 --- a/test/core/commands/change-command.show-validate.test.ts +++ b/test/core/commands/change-command.show-validate.test.ts @@ -89,6 +89,42 @@ 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 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/); + }); + }); + it('validate --strict --json returns a report with valid boolean', async () => { const logs: string[] = []; const origLog = console.log; 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([]); + }); + }); +});