diff --git a/src/commands/validate.ts b/src/commands/validate.ts index a5323a64..9e59a4d4 100644 --- a/src/commands/validate.ts +++ b/src/commands/validate.ts @@ -1,7 +1,7 @@ import ora from 'ora'; import path from 'path'; import { Validator } from '../core/validation/validator.js'; -import { isInteractive } from '../utils/interactive.js'; +import { isInteractive, resolveNoInteractive } from '../utils/interactive.js'; import { getActiveChangeIds, getSpecIds } from '../utils/item-discovery.js'; import { nearestMatches } from '../utils/match.js'; @@ -15,6 +15,7 @@ interface ExecuteOptions { strict?: boolean; json?: boolean; noInteractive?: boolean; + interactive?: boolean; // Commander sets this to false when --no-interactive is used concurrency?: string; } @@ -35,7 +36,7 @@ export class ValidateCommand { await this.runBulkValidation({ changes: !!options.all || !!options.changes, specs: !!options.all || !!options.specs, - }, { strict: !!options.strict, json: !!options.json, concurrency: options.concurrency }); + }, { strict: !!options.strict, json: !!options.json, concurrency: options.concurrency, noInteractive: resolveNoInteractive(options) }); return; } @@ -180,8 +181,8 @@ export class ValidateCommand { bullets.forEach(b => console.error(` ${b}`)); } - private async runBulkValidation(scope: { changes: boolean; specs: boolean }, opts: { strict: boolean; json: boolean; concurrency?: string }): Promise { - const spinner = !opts.json ? ora('Validating...').start() : undefined; + private async runBulkValidation(scope: { changes: boolean; specs: boolean }, opts: { strict: boolean; json: boolean; concurrency?: string; noInteractive?: boolean }): Promise { + const spinner = !opts.json && !opts.noInteractive ? ora('Validating...').start() : undefined; const [changeIds, specIds] = await Promise.all([ scope.changes ? getActiveChangeIds() : Promise.resolve([]), scope.specs ? getSpecIds() : Promise.resolve([]), diff --git a/src/utils/interactive.ts b/src/utils/interactive.ts index 3b73fa3b..aeb9fde9 100644 --- a/src/utils/interactive.ts +++ b/src/utils/interactive.ts @@ -1,4 +1,4 @@ -type InteractiveOptions = { +export type InteractiveOptions = { /** * Explicit "disable prompts" flag passed by internal callers. */ @@ -9,7 +9,12 @@ type InteractiveOptions = { interactive?: boolean; }; -function resolveNoInteractive(value?: boolean | InteractiveOptions): boolean { +/** + * Resolves whether non-interactive mode is requested. + * Handles both explicit `noInteractive: true` and Commander.js style `interactive: false`. + * Use this helper instead of manually checking options.noInteractive to avoid bugs. + */ +export function resolveNoInteractive(value?: boolean | InteractiveOptions): boolean { if (typeof value === 'boolean') return value; return value?.noInteractive === true || value?.interactive === false; } @@ -17,6 +22,8 @@ function resolveNoInteractive(value?: boolean | InteractiveOptions): boolean { export function isInteractive(value?: boolean | InteractiveOptions): boolean { if (resolveNoInteractive(value)) return false; if (process.env.OPEN_SPEC_INTERACTIVE === '0') return false; + // Respect the standard CI environment variable (set by GitHub Actions, GitLab CI, Travis, etc.) + if ('CI' in process.env) return false; return !!process.stdin.isTTY; } diff --git a/test/commands/validate.test.ts b/test/commands/validate.test.ts index 9e67a34b..b94f72d3 100644 --- a/test/commands/validate.test.ts +++ b/test/commands/validate.test.ts @@ -130,4 +130,18 @@ describe('top-level validate command', () => { const result = await runCLI(['validate', changeId], { cwd: testDir }); expect(result.exitCode).toBe(0); }); + + it('respects --no-interactive flag passed via CLI', async () => { + // This test ensures Commander.js --no-interactive flag is correctly parsed + // and passed to the validate command. The flag sets options.interactive = false + // (not options.noInteractive = true) due to Commander.js convention. + const result = await runCLI(['validate', '--specs', '--no-interactive'], { + cwd: testDir, + // Don't set OPEN_SPEC_INTERACTIVE to ensure we're testing the flag itself + env: { ...process.env, OPEN_SPEC_INTERACTIVE: undefined }, + }); + expect(result.exitCode).toBe(0); + // Should complete without hanging and without prompts + expect(result.stderr).not.toContain('What would you like to validate?'); + }); }); diff --git a/test/utils/interactive.test.ts b/test/utils/interactive.test.ts new file mode 100644 index 00000000..c1753d31 --- /dev/null +++ b/test/utils/interactive.test.ts @@ -0,0 +1,125 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { isInteractive, resolveNoInteractive, InteractiveOptions } from '../../src/utils/interactive.js'; + +describe('interactive utilities', () => { + let originalOpenSpecInteractive: string | undefined; + let originalCI: string | undefined; + let originalStdinIsTTY: boolean | undefined; + + beforeEach(() => { + // Save original environment + originalOpenSpecInteractive = process.env.OPEN_SPEC_INTERACTIVE; + originalCI = process.env.CI; + originalStdinIsTTY = process.stdin.isTTY; + + // Clear environment for clean testing + delete process.env.OPEN_SPEC_INTERACTIVE; + delete process.env.CI; + }); + + afterEach(() => { + // Restore original environment + if (originalOpenSpecInteractive !== undefined) { + process.env.OPEN_SPEC_INTERACTIVE = originalOpenSpecInteractive; + } else { + delete process.env.OPEN_SPEC_INTERACTIVE; + } + if (originalCI !== undefined) { + process.env.CI = originalCI; + } else { + delete process.env.CI; + } + // Restore stdin.isTTY + Object.defineProperty(process.stdin, 'isTTY', { + value: originalStdinIsTTY, + writable: true, + configurable: true, + }); + }); + + describe('resolveNoInteractive', () => { + it('should return true when noInteractive is true', () => { + expect(resolveNoInteractive({ noInteractive: true })).toBe(true); + }); + + it('should return true when interactive is false (Commander.js style)', () => { + // This is how Commander.js handles --no-interactive flag + expect(resolveNoInteractive({ interactive: false })).toBe(true); + }); + + it('should return false when noInteractive is false', () => { + expect(resolveNoInteractive({ noInteractive: false })).toBe(false); + }); + + it('should return false when interactive is true', () => { + expect(resolveNoInteractive({ interactive: true })).toBe(false); + }); + + it('should return false for empty options object', () => { + expect(resolveNoInteractive({})).toBe(false); + }); + + it('should return false for undefined', () => { + expect(resolveNoInteractive(undefined)).toBe(false); + }); + + it('should handle boolean value true', () => { + expect(resolveNoInteractive(true)).toBe(true); + }); + + it('should handle boolean value false', () => { + expect(resolveNoInteractive(false)).toBe(false); + }); + + it('should prioritize noInteractive over interactive when both set', () => { + // noInteractive: true should win + expect(resolveNoInteractive({ noInteractive: true, interactive: true })).toBe(true); + // If noInteractive is false, check interactive + expect(resolveNoInteractive({ noInteractive: false, interactive: false })).toBe(true); + }); + }); + + describe('isInteractive', () => { + it('should return false when noInteractive is true', () => { + expect(isInteractive({ noInteractive: true })).toBe(false); + }); + + it('should return false when interactive is false (Commander.js --no-interactive)', () => { + expect(isInteractive({ interactive: false })).toBe(false); + }); + + it('should return false when OPEN_SPEC_INTERACTIVE env var is 0', () => { + process.env.OPEN_SPEC_INTERACTIVE = '0'; + Object.defineProperty(process.stdin, 'isTTY', { value: true, writable: true, configurable: true }); + expect(isInteractive({})).toBe(false); + }); + + it('should return false when CI env var is set', () => { + process.env.CI = 'true'; + Object.defineProperty(process.stdin, 'isTTY', { value: true, writable: true, configurable: true }); + expect(isInteractive({})).toBe(false); + }); + + it('should return false when CI env var is set to any value', () => { + // CI can be set to any value, not just "true" + process.env.CI = '1'; + Object.defineProperty(process.stdin, 'isTTY', { value: true, writable: true, configurable: true }); + expect(isInteractive({})).toBe(false); + }); + + it('should return false when stdin is not a TTY', () => { + Object.defineProperty(process.stdin, 'isTTY', { value: false, writable: true, configurable: true }); + expect(isInteractive({})).toBe(false); + }); + + it('should return true when stdin is TTY and no flags disable it', () => { + Object.defineProperty(process.stdin, 'isTTY', { value: true, writable: true, configurable: true }); + expect(isInteractive({})).toBe(true); + }); + + it('should return true when stdin is TTY and options are undefined', () => { + Object.defineProperty(process.stdin, 'isTTY', { value: true, writable: true, configurable: true }); + expect(isInteractive(undefined)).toBe(true); + }); + }); +});