Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/fix-archive-exit-code.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@fission-ai/openspec": patch
---

### Bug Fixes

- **`archive` exits non-zero when blocked in human mode** — `openspec archive <change> -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).
3 changes: 3 additions & 0 deletions src/core/archive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
}

Expand All @@ -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;
}
}
Expand Down
173 changes: 173 additions & 0 deletions test/core/archive.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand All @@ -38,13 +39,20 @@ 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();
});

afterEach(async () => {
// 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 {
Expand Down Expand Up @@ -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
Expand Down
Loading