diff --git a/src/commands/amend/config.ts b/src/commands/amend/config.ts index 192e1b93..7cd65aec 100644 --- a/src/commands/amend/config.ts +++ b/src/commands/amend/config.ts @@ -55,5 +55,14 @@ export const options = { } as Record export const builder = (yargs: Argv) => { - return yargs.options(options).usage(getCommandUsageHeader(command)) + return yargs + .options(options) + .check((argv) => { + const a = argv as { dryRun?: boolean; apply?: boolean } + if (a.dryRun && a.apply) { + throw new Error('--dry-run and --apply cannot be combined — --dry-run previews, --apply amends.') + } + return true + }) + .usage(getCommandUsageHeader(command)) } diff --git a/src/commands/amend/configCheck.test.ts b/src/commands/amend/configCheck.test.ts new file mode 100644 index 00000000..dc86406b --- /dev/null +++ b/src/commands/amend/configCheck.test.ts @@ -0,0 +1,44 @@ +import { Argv } from 'yargs' +import { builder } from './config' + +// The builder's `.check()` callback enforces --dry-run/--apply mutual +// exclusion (#1889). Exercise it directly against a minimal yargs-chain +// stub instead of a full `.parseSync()`, since yargs' default failure +// handler calls `process.exit` on a thrown check error rather than +// propagating it to the test. +function extractCheckFn(): (argv: Record) => boolean { + let captured: ((argv: Record) => boolean) | undefined + const fakeYargs = { + options: () => fakeYargs, + check: (fn: (argv: Record) => boolean) => { + captured = fn + return fakeYargs + }, + usage: () => fakeYargs, + } + builder(fakeYargs as unknown as Argv) + if (!captured) throw new Error('builder did not register a .check() callback') + return captured +} + +describe('amend config validation (#1889)', () => { + const check = extractCheckFn() + + it('rejects --dry-run combined with --apply', () => { + expect(() => check({ dryRun: true, apply: true })).toThrow( + '--dry-run and --apply cannot be combined — --dry-run previews, --apply amends.' + ) + }) + + it('accepts --dry-run alone', () => { + expect(check({ dryRun: true })).toBe(true) + }) + + it('accepts --apply alone', () => { + expect(check({ apply: true })).toBe(true) + }) + + it('accepts neither flag', () => { + expect(check({})).toBe(true) + }) +}) diff --git a/src/commands/changelog/changelog.test.ts b/src/commands/changelog/changelog.test.ts index 79124b1a..a1741c1f 100644 --- a/src/commands/changelog/changelog.test.ts +++ b/src/commands/changelog/changelog.test.ts @@ -10,6 +10,7 @@ import { getRepo } from '../../lib/simple-git/getRepo' import { getCommitLogCurrentBranch } from '../../lib/simple-git/getCommitLogCurrentBranch' import { getCommitLogRangeDetails } from '../../lib/simple-git/getCommitLogRangeDetails' import { getCurrentBranchName } from '../../lib/simple-git/getCurrentBranchName' +import { getDiffForBranch } from '../../lib/simple-git/getDiffForBranch' import { executeChain } from '../../lib/langchain/utils/executeChain' import { loadConfig } from '../../lib/config/utils/loadConfig' import { getApiKeyForModel, getModelAndProviderFromConfig } from '../../lib/langchain/utils' @@ -59,6 +60,7 @@ const mockGetCommitLogCurrentBranch = getCommitLogCurrentBranch as jest.MockedFu typeof getCommitLogCurrentBranch > const mockGetCurrentBranchName = getCurrentBranchName as jest.MockedFunction +const mockGetDiffForBranch = getDiffForBranch as jest.MockedFunction const mockGetCommitLogRangeDetails = getCommitLogRangeDetails as jest.MockedFunction< typeof getCommitLogRangeDetails > @@ -449,4 +451,76 @@ describe('changelog command', () => { ) }) }) + + describe('--only-diff (#1889)', () => { + it('rejects --only-diff combined with --range instead of silently ignoring --range', async () => { + argv.onlyDiff = true + mockLoadConfig.mockReturnValue({ + service: { + authentication: { type: 'APIKey', credentials: { apiKey: 'mock-api-key' } }, + provider: 'openai', + model: 'gpt-4o', + tokenLimit: 4096, + temperature: 0.2, + maxConcurrent: 1, + }, + defaultBranch: 'main', + mode: 'stdout', + range: 'abc123:def456', + } as unknown as Config) + + await expect(handler(argv, logger)).rejects.toMatchObject({ name: 'CommandExitError', code: 1 }) + expect(logger.error).toHaveBeenCalledWith( + expect.stringContaining('--only-diff'), + expect.anything() + ) + }) + + it('rejects --only-diff combined with --tag instead of silently ignoring --tag', async () => { + argv.onlyDiff = true + argv.tag = 'v1.0.0' + + await expect(handler(argv, logger)).rejects.toMatchObject({ name: 'CommandExitError', code: 1 }) + expect(logger.error).toHaveBeenCalledWith( + expect.stringContaining('--only-diff'), + expect.anything() + ) + }) + + it('rejects --only-diff combined with --since-last-tag instead of silently ignoring it', async () => { + argv.onlyDiff = true + mockLoadConfig.mockReturnValue({ + service: { + authentication: { type: 'APIKey', credentials: { apiKey: 'mock-api-key' } }, + provider: 'openai', + model: 'gpt-4o', + tokenLimit: 4096, + temperature: 0.2, + maxConcurrent: 1, + }, + defaultBranch: 'main', + mode: 'stdout', + sinceLastTag: true, + } as unknown as Config) + + await expect(handler(argv, logger)).rejects.toMatchObject({ name: 'CommandExitError', code: 1 }) + expect(logger.error).toHaveBeenCalledWith( + expect.stringContaining('--only-diff'), + expect.anything() + ) + }) + + it('still allows --only-diff combined with --branch (branch is the diff base)', async () => { + argv.onlyDiff = true + argv.branch = 'develop' + mockGetDiffForBranch.mockResolvedValue({ staged: [], unstaged: [], untracked: [] }) + + await handler(argv, logger) + + expect(logger.error).not.toHaveBeenCalled() + expect(mockGetDiffForBranch).toHaveBeenCalledWith( + expect.objectContaining({ baseBranch: 'develop' }) + ) + }) + }) }) diff --git a/src/commands/changelog/handler.ts b/src/commands/changelog/handler.ts index c1692eb2..a991d7ea 100644 --- a/src/commands/changelog/handler.ts +++ b/src/commands/changelog/handler.ts @@ -111,6 +111,19 @@ export async function generateChangelogResult( commandExit(1) } + if (argv.onlyDiff) { + const ignoredByOnlyDiff = [ + config.range ? '--range' : null, + argv.tag ? '--tag' : null, + config.sinceLastTag ? '--since-last-tag' : null, + ].filter(Boolean) + + if (ignoredByOnlyDiff.length > 0) { + logger.error(`--only-diff cannot be combined with ${ignoredByOnlyDiff.join(', ')}.`, { color: 'red' }) + commandExit(1) + } + } + if (config.service.authentication.type !== 'None' && !key) { handleMissingApiKey(logger, config, { command: 'changelog' }) } diff --git a/src/commands/commit/config.ts b/src/commands/commit/config.ts index 00fbfd9e..57ee0550 100644 --- a/src/commands/commit/config.ts +++ b/src/commands/commit/config.ts @@ -164,5 +164,49 @@ export const options = { } as Record export const builder = (yargs: Argv) => { - return yargs.options(options).usage(getCommandUsageHeader(command)) + return yargs + .options(options) + .check((argv) => { + const a = argv as { + json?: boolean + split?: boolean + plan?: boolean + apply?: boolean + strictSplit?: boolean + printMessage?: boolean + _: Array + } + const positionalSplit = a._.includes('split') + const splitMode = Boolean(a.split || a.plan || positionalSplit) + + // handler.ts:58 already rejects `--json` combined with `--split`, + // `--plan`, or `--apply` — emitting a structured `emitJson({ error })` + // payload for machine consumers instead of exiting via yargs' plain-text + // `.fail()` handler. `.check()` runs during yargs parsing, strictly + // before the handler, so any of the rules below that only fire because + // one of those three flags is set must defer to that handler-level + // check instead of throwing here first. Rules that don't depend on + // split/plan/apply (e.g. `--strict-split` with none of them set) aren't + // covered by that handler guard, so they still validate here even + // under `--json`. + const jsonHandledByCommand = Boolean(a.json && (a.split || a.plan || a.apply)) + if (jsonHandledByCommand) { + return true + } + + if (a.plan && a.apply) { + throw new Error('--plan and --apply cannot be combined — --plan previews, --apply commits.') + } + if (a.printMessage && (a.split || a.plan || a.apply || a.strictSplit)) { + throw new Error('--print-message cannot be combined with --split, --plan, --apply, or --strict-split.') + } + if (a.apply && !splitMode) { + throw new Error('--apply requires --split (it applies a split plan).') + } + if (a.strictSplit && !splitMode) { + throw new Error('--strict-split requires --split or --plan.') + } + return true + }) + .usage(getCommandUsageHeader(command)) } diff --git a/src/commands/commit/configCheck.test.ts b/src/commands/commit/configCheck.test.ts new file mode 100644 index 00000000..15425845 --- /dev/null +++ b/src/commands/commit/configCheck.test.ts @@ -0,0 +1,120 @@ +import { Argv } from 'yargs' +import { builder } from './config' + +// The builder's `.check()` callback enforces the mutually-exclusive split +// flag rules (#1889). Exercise it directly against a minimal yargs-chain +// stub instead of a full `.parseSync()`, since yargs' default failure +// handler calls `process.exit` on a thrown check error rather than +// propagating it to the test. +function extractCheckFn(): (argv: Record) => boolean { + let captured: ((argv: Record) => boolean) | undefined + const fakeYargs = { + options: () => fakeYargs, + check: (fn: (argv: Record) => boolean) => { + captured = fn + return fakeYargs + }, + usage: () => fakeYargs, + } + builder(fakeYargs as unknown as Argv) + if (!captured) throw new Error('builder did not register a .check() callback') + return captured +} + +describe('commit config validation (#1889)', () => { + const check = extractCheckFn() + + it('rejects --plan combined with --apply', () => { + expect(() => check({ plan: true, apply: true, _: [] })).toThrow( + '--plan and --apply cannot be combined — --plan previews, --apply commits.' + ) + }) + + it('rejects --print-message combined with --split', () => { + expect(() => check({ printMessage: true, split: true, _: [] })).toThrow( + '--print-message cannot be combined with --split, --plan, --apply, or --strict-split.' + ) + }) + + it('rejects --print-message combined with --plan', () => { + expect(() => check({ printMessage: true, plan: true, _: [] })).toThrow( + '--print-message cannot be combined with --split, --plan, --apply, or --strict-split.' + ) + }) + + it('rejects --apply without --split', () => { + expect(() => check({ apply: true, _: [] })).toThrow( + '--apply requires --split (it applies a split plan).' + ) + }) + + it('rejects --strict-split without --split or --plan', () => { + expect(() => check({ strictSplit: true, _: [] })).toThrow( + '--strict-split requires --split or --plan.' + ) + }) + + it('accepts --split --apply', () => { + expect(check({ split: true, apply: true, _: [] })).toBe(true) + }) + + it('accepts --plan alone', () => { + expect(check({ plan: true, _: [] })).toBe(true) + }) + + it('accepts the `split` positional with --apply', () => { + expect(check({ apply: true, _: ['commit', 'split'] })).toBe(true) + }) + + it('accepts the `split` positional with --strict-split', () => { + expect(check({ strictSplit: true, _: ['commit', 'split'] })).toBe(true) + }) + + it('accepts --print-message alone', () => { + expect(check({ printMessage: true, _: [] })).toBe(true) + }) + + it('accepts no split-related flags', () => { + expect(check({ _: [] })).toBe(true) + }) + + // handler.ts:58 emits a structured `emitJson({ error })` payload when + // `--json` is combined with `--split`/`--plan`/`--apply`, for machine + // consumers. These combos must NOT throw here — check() runs before the + // handler, so throwing would replace that JSON contract with a plain-text + // yargs failure (#2039 review feedback). + it('defers to the handler for --json + --plan + --apply instead of throwing', () => { + expect(check({ json: true, plan: true, apply: true, _: [] })).toBe(true) + }) + + it('defers to the handler for --json + --apply without --split', () => { + expect(check({ json: true, apply: true, _: [] })).toBe(true) + }) + + it('defers to the handler for --json + --apply + --strict-split without --split', () => { + expect(check({ json: true, apply: true, strictSplit: true, _: [] })).toBe(true) + }) + + it('defers to the handler for --json + --print-message + --split', () => { + expect(check({ json: true, printMessage: true, split: true, _: [] })).toBe(true) + }) + + // `--strict-split` alone isn't covered by handler.ts's json guard (it only + // checks split/plan/apply), so it must still be validated here even under + // `--json` — otherwise `coco commit --json --strict-split` would silently + // fall into the draft-only path and drop `--strict-split` (the exact bug + // #1889 was opened to fix). + it('still rejects --json + --strict-split without --split or --plan', () => { + expect(() => check({ json: true, strictSplit: true, _: [] })).toThrow( + '--strict-split requires --split or --plan.' + ) + }) + + it('accepts --json alone', () => { + expect(check({ json: true, _: [] })).toBe(true) + }) + + it('accepts --json + --split (handler enforces the json/split conflict)', () => { + expect(check({ json: true, split: true, _: [] })).toBe(true) + }) +}) diff --git a/src/commands/doctor/config.ts b/src/commands/doctor/config.ts index 03abb033..b5eec3bc 100644 --- a/src/commands/doctor/config.ts +++ b/src/commands/doctor/config.ts @@ -33,5 +33,15 @@ export const options = { } as Record export const builder = (yargs: Argv) => { - return yargs.options(options).usage(getCommandUsageHeader(command)) + return yargs + .options(options) + .check((argv) => { + const a = argv as { clear?: boolean; cost?: boolean; fix?: boolean } + const picked = [a.clear && '--clear', a.cost && '--cost', a.fix && '--fix'].filter(Boolean) + if (picked.length > 1) { + throw new Error(`Options ${picked.join(', ')} cannot be used together.`) + } + return true + }) + .usage(getCommandUsageHeader(command)) } diff --git a/src/commands/doctor/configCheck.test.ts b/src/commands/doctor/configCheck.test.ts new file mode 100644 index 00000000..4caae346 --- /dev/null +++ b/src/commands/doctor/configCheck.test.ts @@ -0,0 +1,60 @@ +import { Argv } from 'yargs' +import { builder } from './config' + +// The builder's `.check()` callback enforces that --clear/--cost/--fix are +// mutually exclusive (#1889). Exercise it directly against a minimal +// yargs-chain stub instead of a full `.parseSync()`, since yargs' default +// failure handler calls `process.exit` on a thrown check error rather than +// propagating it to the test. +function extractCheckFn(): (argv: Record) => boolean { + let captured: ((argv: Record) => boolean) | undefined + const fakeYargs = { + options: () => fakeYargs, + check: (fn: (argv: Record) => boolean) => { + captured = fn + return fakeYargs + }, + usage: () => fakeYargs, + } + builder(fakeYargs as unknown as Argv) + if (!captured) throw new Error('builder did not register a .check() callback') + return captured +} + +describe('doctor config validation (#1889)', () => { + const check = extractCheckFn() + + it('rejects --clear combined with --cost', () => { + expect(() => check({ clear: true, cost: true })).toThrow( + 'Options --clear, --cost cannot be used together.' + ) + }) + + it('rejects --clear combined with --fix', () => { + expect(() => check({ clear: true, fix: true })).toThrow( + 'Options --clear, --fix cannot be used together.' + ) + }) + + it('rejects --cost combined with --fix', () => { + expect(() => check({ cost: true, fix: true })).toThrow( + 'Options --cost, --fix cannot be used together.' + ) + }) + + it('accepts --clear alone', () => { + expect(check({ clear: true })).toBe(true) + }) + + it('accepts --cost alone', () => { + expect(check({ cost: true })).toBe(true) + }) + + it('accepts --fix alone', () => { + expect(check({ fix: true })).toBe(true) + }) + + it('accepts no flags', () => { + expect(check({})).toBe(true) + }) +})