diff --git a/.changeset/fix-archive-exit-code.md b/.changeset/fix-archive-exit-code.md new file mode 100644 index 0000000000..26c7865750 --- /dev/null +++ b/.changeset/fix-archive-exit-code.md @@ -0,0 +1,7 @@ +--- +"@fission-ai/openspec": patch +--- + +### Bug Fixes + +- **`archive` exits non-zero when blocked in human mode** — `openspec archive -y` (and any non-`--json` invocation) no longer returns exit code 0 when validation fails and nothing is archived. The three blocking paths in human mode — delta-spec validation failure, spec rebuild failure, and rebuilt-spec validation failure — now set `process.exitCode = 1`, matching the existing `--json` behavior. Previously the command printed "Validation failed" (or "Aborted. No files were changed.") and exited 0, letting scripts and CI believe the archive succeeded. Aligns `archive` with the same exit-code guarantee already approved for `apply` instructions (#1250). diff --git a/src/core/archive.ts b/src/core/archive.ts index a39d0756a4..3e9bf80025 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -306,6 +306,7 @@ export class ArchiveCommand { } console.log(chalk.red('\nValidation failed. Please fix the errors before archiving.')); console.log(chalk.yellow('To skip validation (not recommended), use --no-validate flag.')); + process.exitCode = 1; return null; } } else if (json) { @@ -428,6 +429,7 @@ export class ArchiveCommand { } console.log(String(err.message || err)); console.log('Aborted. No files were changed.'); + process.exitCode = 1; return null; } @@ -451,6 +453,7 @@ export class ArchiveCommand { else if (issue.level === 'WARNING') console.log(chalk.yellow(` ⚠ ${issue.message}`)); } console.log('Aborted. No files were changed.'); + process.exitCode = 1; return null; } } diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index ddd0658bec..d0d586862e 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -15,6 +15,7 @@ describe('ArchiveCommand', () => { let tempDir: string; let archiveCommand: ArchiveCommand; const originalConsoleLog = console.log; + const originalExitCode = process.exitCode; const originalXdgDataHome = process.env.XDG_DATA_HOME; beforeEach(async () => { @@ -38,6 +39,10 @@ describe('ArchiveCommand', () => { // Suppress console.log during tests console.log = vi.fn(); + // Isolate process.exitCode so a failing run can't leak into the next + // test or skew the vitest process exit status. + process.exitCode = undefined; + archiveCommand = new ArchiveCommand(); }); @@ -45,6 +50,9 @@ describe('ArchiveCommand', () => { // Restore console.log console.log = originalConsoleLog; + // Restore process.exitCode (clear anything a test set) + process.exitCode = originalExitCode; + if (originalXdgDataHome === undefined) { delete process.env.XDG_DATA_HOME; } else { @@ -826,6 +834,171 @@ E1 updated`); }); }); + describe('exit code on blocked archive (human mode)', () => { + // Regression for the silent-exit-0 bug: when archive is blocked in + // human mode it must set a non-zero exit code so scripts/CI can detect + // the failure, mirroring the JSON-mode behavior. + it('sets exit code 1 when delta spec validation fails', async () => { + const changeName = 'exit-delta-fail'; + const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); + const changeSpecDir = path.join(changeDir, 'specs', 'bad-capability'); + await fs.mkdir(changeSpecDir, { recursive: true }); + + // Delta spec missing required SHALL/MUST keyword -> validation error + const specContent = `# Bad Capability - Changes + +## ADDED Requirements + +### Requirement: Logging Feature + +The system will log all events. + +#### Scenario: Event recorded +- **WHEN** an event occurs +- **THEN** it is captured`; + await fs.writeFile(path.join(changeSpecDir, 'spec.md'), specContent); + await fs.writeFile(path.join(changeDir, 'tasks.md'), '- [x] Task 1\n'); + + await archiveCommand.execute(changeName, { yes: true, skipSpecs: true }); + + expect(process.exitCode).toBe(1); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('Validation failed') + ); + + // Change must NOT have been archived + const archiveDir = path.join(tempDir, 'openspec', 'changes', 'archive'); + const archives = await fs.readdir(archiveDir); + expect(archives.some(a => a.includes(changeName))).toBe(false); + }); + + it('sets exit code 1 when spec rebuild fails (MODIFIED on new spec)', async () => { + const changeName = 'exit-rebuild-fail'; + const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); + const changeSpecDir = path.join(changeDir, 'specs', 'new-capability'); + await fs.mkdir(changeSpecDir, { recursive: true }); + + // MODIFIED on a non-existent target spec aborts the rebuild + const specContent = `# New Capability - Changes + +## ADDED Requirements + +### Requirement: New Feature +New feature description. + +## MODIFIED Requirements + +### Requirement: Existing Feature +Modified content.`; + await fs.writeFile(path.join(changeSpecDir, 'spec.md'), specContent); + + await archiveCommand.execute(changeName, { yes: true, noValidate: true }); + + expect(process.exitCode).toBe(1); + expect(console.log).toHaveBeenCalledWith('Aborted. No files were changed.'); + + const mainSpecPath = path.join(tempDir, 'openspec', 'specs', 'new-capability', 'spec.md'); + await expect(fs.access(mainSpecPath)).rejects.toThrow(); + + const archiveDir = path.join(tempDir, 'openspec', 'changes', 'archive'); + const archives = await fs.readdir(archiveDir); + expect(archives.some(a => a.includes(changeName))).toBe(false); + }); + + it('sets exit code 1 when rebuilt spec fails validateSpecContent', async () => { + // Spot 3 is defensive: spot 1 (validateChangeDeltaSpecs) already + // enforces SHALL/MUST/scenario rules on the delta, and buildUpdatedSpec + // pre-validates target structure, so a real delta almost never reaches + // this branch. Spy on validateSpecContent (the existing --no-validate + // test uses the same spy pattern) to force the rebuilt spec invalid + // while buildUpdatedSpec runs for real — exercising the exit-code fix. + const changeName = 'exit-rebuilt-validate-fail'; + const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); + const changeSpecDir = path.join(changeDir, 'specs', 'rebuilt-capability'); + await fs.mkdir(changeSpecDir, { recursive: true }); + + // Existing main spec so MODIFIED targets a real spec and buildUpdatedSpec + // succeeds (does not throw). + const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'rebuilt-capability'); + await fs.mkdir(mainSpecDir, { recursive: true }); + const mainContent = `# rebuilt-capability Specification + +## Purpose +Rebuilt capability purpose. + +## Requirements + +### Requirement: Existing Feature +The system SHALL do the thing. + +#### Scenario: works +- **WHEN** x +- **THEN** y`; + await fs.writeFile(path.join(mainSpecDir, 'spec.md'), mainContent); + + // Valid MODIFIED delta (passes spot 1 delta validation). + const deltaContent = `# Rebuilt Capability - Changes + +## MODIFIED Requirements + +### Requirement: Existing Feature +The system SHALL do the thing differently. + +#### Scenario: works +- **WHEN** x +- **THEN** z`; + await fs.writeFile(path.join(changeSpecDir, 'spec.md'), deltaContent); + await fs.writeFile(path.join(changeDir, 'tasks.md'), '- [x] Task 1\n'); + + const specContentSpy = vi + .spyOn(Validator.prototype, 'validateSpecContent') + .mockResolvedValue({ + valid: false, + issues: [ + { level: 'ERROR', path: 'requirements[0]', message: 'mocked rebuilt-spec failure' }, + ], + summary: { errors: 1, warnings: 0, info: 0 }, + }); + + try { + await archiveCommand.execute(changeName, { yes: true }); + + expect(process.exitCode).toBe(1); + // buildUpdatedSpec ran for real and the spy made its output "invalid" + expect(specContentSpy).toHaveBeenCalled(); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('Validation errors in rebuilt spec for rebuilt-capability') + ); + expect(console.log).toHaveBeenCalledWith('Aborted. No files were changed.'); + + // Main spec must be unchanged (no writes happened) + const still = await fs.readFile(path.join(mainSpecDir, 'spec.md'), 'utf-8'); + expect(still).toBe(mainContent); + + // Change must NOT have been archived + const archiveDir = path.join(tempDir, 'openspec', 'changes', 'archive'); + const archives = await fs.readdir(archiveDir); + expect(archives.some(a => a.includes(changeName))).toBe(false); + } finally { + specContentSpy.mockRestore(); + } + }); + + it('leaves exit code 0 on successful archive (no leak from prior test)', async () => { + const changeName = 'exit-ok'; + const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); + await fs.mkdir(changeDir, { recursive: true }); + + await archiveCommand.execute(changeName, { yes: true }); + + expect(process.exitCode).toBeUndefined(); + + const archiveDir = path.join(tempDir, 'openspec', 'changes', 'archive'); + const archives = await fs.readdir(archiveDir); + expect(archives.some(a => a.includes(changeName))).toBe(true); + }); + }); + describe('error handling', () => { it('should throw error when openspec directory does not exist', async () => { // Remove openspec directory