From e490f78928cea7dca93373c0fbbb2daefe505195 Mon Sep 17 00:00:00 2001 From: Grimicorn Agent Date: Thu, 27 Aug 2026 06:17:15 -0500 Subject: [PATCH 1/5] Confirm before deleting a source; --yes skips for scripts Closes #135 --- README.md | 2 +- src/commands/sources.ts | 60 +++++++++++++++--- tests/commands/sources.test.ts | 109 +++++++++++++++++++++++++++++++-- 3 files changed, 158 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 341274d..68de6c2 100644 --- a/README.md +++ b/README.md @@ -22,7 +22,7 @@ server-side records. | `markpost sync [--dry-run]` | Fetch all pending records, write each to a markdown file, and (when `autoDelete` is enabled) delete the written records from the server. `--dry-run` reports the exact write/delete plan without writing or mutating anything | | `markpost push ` | Create records from one or more markdown files, directories, or glob patterns | | `markpost get [--json]` | Fetch and display a single record; pass `--json` for machine-readable output | -| `markpost sources [uuid]` | Manage sources; `sources list --json` prints machine-readable output | +| `markpost sources [uuid] [--yes]` | Manage sources; `sources list --json` prints machine-readable output. `sources delete` asks to confirm first (deleting a source is irreversible — it drops the ingest config and one-time signing secret); pass `--yes` to skip the prompt in scripts | | `markpost records list [--source ] [--status ] [--search ] [--json]` | List records without deleting them, optionally filtered by source, status, or search text; pass `--json` for machine-readable output | | `markpost config [key] [value]` | View or change the stored API token and output directory | | `markpost settings [key=value ...]` | View or change server-side sync settings (`autoSync`, `autoDelete`, `frontmatter`, `conflictStrategy`) | diff --git a/src/commands/sources.ts b/src/commands/sources.ts index 8b74f21..c0abe3d 100644 --- a/src/commands/sources.ts +++ b/src/commands/sources.ts @@ -1,6 +1,6 @@ import { parseArgs } from 'node:util'; import chalk from 'chalk'; -import { input, select } from '@inquirer/prompts'; +import { confirm, input, select } from '@inquirer/prompts'; import { createSource, deleteSource, @@ -25,7 +25,7 @@ export const USAGE = `Usage: markpost sources [uuid] list List all sources (pass --json for machine-readable output) create Create a new source (prompts for details) update [uuid] Update a source's route folder; prompts to pick one if uuid is omitted - delete [uuid] Delete a source; prompts to pick one if uuid is omitted`; + delete [uuid] Delete a source; prompts to pick one if uuid is omitted. Asks to confirm first; pass --yes to skip the prompt (for scripts)`; export const buildEndpointUrl = ( sourceType: SourceType, @@ -45,15 +45,25 @@ export const buildEndpointUrl = ( // Only `list` renders JSON; the other subcommands are interactive or emit a // one-off result, so --json means nothing to them. const LIST_SUBCOMMAND = 'list'; +// `delete` is the only subcommand `--yes` applies to, so it's named for the +// guard that rejects the flag elsewhere as well as its handler-map key. +const DELETE_SUBCOMMAND = 'delete'; const SOURCES_HANDLERS = new Map< string, - (uuid: string | undefined, json: boolean) => Promise + ( + uuid: string | undefined, + json: boolean, + skipConfirm: boolean, + ) => Promise >([ [LIST_SUBCOMMAND, (_uuid, json) => listSources(json)], ['create', () => createSourceCommand()], ['update', (uuid) => updateSourceCommand(uuid)], - ['delete', (uuid) => deleteSourceCommand(uuid)], + [ + DELETE_SUBCOMMAND, + (uuid, _json, skipConfirm) => deleteSourceCommand(uuid, skipConfirm), + ], ]); export const runSourcesCommand = async (args: string[]): Promise => { @@ -65,14 +75,16 @@ export const runSourcesCommand = async (args: string[]): Promise => { // `parseArgs` keeps --json out of the uuid slot (so `sources delete --json` // still prompts rather than trying to delete a source named "--json") and // rejects an unknown/mistyped flag. Only `list` reads json. - const { positionals } = parseArgs({ + const { positionals, values } = parseArgs({ args, allowPositionals: true, options: { json: { type: 'boolean' }, + yes: { type: 'boolean' }, }, }); const [subcommand, uuid] = positionals; + const skipConfirm = Boolean(values.yes); const handler = SOURCES_HANDLERS.get(subcommand); // Validate before the config check so a bad subcommand fails on usage @@ -95,11 +107,23 @@ export const runSourcesCommand = async (args: string[]): Promise => { return; } + // `--yes` only skips the delete confirmation; reject it anywhere else + // rather than silently ignoring it, mirroring the --json guard above so a + // misplaced flag fails loudly instead of appearing to take effect. + if (skipConfirm && subcommand !== DELETE_SUBCOMMAND) { + failWithUsage( + `--yes is only supported by \`sources ${DELETE_SUBCOMMAND}\`.`, + USAGE, + json, + ); + return; + } + if (!(await checkConfig(json))) { return; } - await handler(uuid, json); + await handler(uuid, json, skipConfirm); } catch (error) { // A deliberate Ctrl+C at a prompt throws @inquirer's `ExitPromptError`; // that's a user abort, not a command failure, so don't flag it non-zero. @@ -336,13 +360,35 @@ const updateSourceCommand = async (uuid?: string): Promise => { await promptAndApplyRouteFolder(target); }; -const deleteSourceCommand = async (uuid?: string): Promise => { +// Deleting a source is irreversible: it drops the ingest config and the +// one-time signing secret, which can never be retrieved again. The uuid is +// sanitized because it may have come from an untrusted API response via the +// interactive picker. Defaults to "no" so a bare Enter cancels rather than +// deletes. Isolated here so the delete flow stays unit-testable by mocking the +// prompt. +const confirmDeletion = async (targetUuid: string): Promise => + confirm({ + message: `Delete source ${sanitizeForTerminal( + targetUuid, + )}? This drops its ingest config and one-time signing secret and cannot be undone.`, + default: false, + }); + +const deleteSourceCommand = async ( + uuid: string | undefined, + skipConfirm: boolean, +): Promise => { const targetUuid = uuid ?? (await promptForSource('delete'))?.uuid; if (!targetUuid) { return; } + if (!skipConfirm && !(await confirmDeletion(targetUuid))) { + console.log('Deletion cancelled.'); + return; + } + const meta = await deleteSource(targetUuid); if (!meta) { diff --git a/tests/commands/sources.test.ts b/tests/commands/sources.test.ts index 2028316..8d38ca8 100644 --- a/tests/commands/sources.test.ts +++ b/tests/commands/sources.test.ts @@ -11,7 +11,11 @@ vi.mock('@/libs/sources.js', () => ({ updateSource: vi.fn(), deleteSource: vi.fn(), })); -vi.mock('@inquirer/prompts', () => ({ input: vi.fn(), select: vi.fn() })); +vi.mock('@inquirer/prompts', () => ({ + input: vi.fn(), + select: vi.fn(), + confirm: vi.fn(), +})); vi.mock('chalk', () => ({ default: { redBright: vi.fn((value: unknown) => value), @@ -691,45 +695,140 @@ describe('runSourcesCommand', () => { }); describe('delete', () => { - it('deletes directly by uuid when one is provided', async () => { + it('deletes directly by uuid when one is provided and confirmed', async () => { const { deleteSource } = await import('@/libs/sources.js'); vi.mocked(deleteSource).mockResolvedValue({ deleted: 1 }); - const { select } = await import('@inquirer/prompts'); + const { select, confirm } = await import('@inquirer/prompts'); + vi.mocked(confirm).mockResolvedValue(true); const { runSourcesCommand } = await import('@/commands/sources.js'); await runSourcesCommand(['delete', 'abc-123']); + expect(confirm).toHaveBeenCalledTimes(1); expect(deleteSource).toHaveBeenCalledWith('abc-123'); expect(select).not.toHaveBeenCalled(); }); - it('prompts to pick a source when no uuid is given', async () => { + it('prompts to pick a source when no uuid is given, then confirms', async () => { const { fetchSources, deleteSource } = await import('@/libs/sources.js'); vi.mocked(fetchSources).mockResolvedValue([webhookSource]); vi.mocked(deleteSource).mockResolvedValue({ deleted: 1 }); - const { select } = await import('@inquirer/prompts'); + const { select, confirm } = await import('@inquirer/prompts'); vi.mocked(select).mockResolvedValue('abc-123'); + vi.mocked(confirm).mockResolvedValue(true); + const { runSourcesCommand } = await import('@/commands/sources.js'); + + await runSourcesCommand(['delete']); + + expect(deleteSource).toHaveBeenCalledWith('abc-123'); + }); + + // The confirmation is the whole point of the feature: a "no" answer must + // abort before any DELETE reaches the server. + it('aborts without deleting when the confirmation is declined', async () => { + const { deleteSource } = await import('@/libs/sources.js'); + const { confirm } = await import('@inquirer/prompts'); + vi.mocked(confirm).mockResolvedValue(false); + const { runSourcesCommand } = await import('@/commands/sources.js'); + + await runSourcesCommand(['delete', 'abc-123']); + + expect(deleteSource).not.toHaveBeenCalled(); + expect(console.log).toHaveBeenCalledWith('Deletion cancelled.'); + }); + + // The confirm message must name the source being deleted so the user knows + // what they're destroying, and warn that it's irreversible. + it('shows the uuid and an irreversible-warning in the confirmation prompt', async () => { + const { deleteSource } = await import('@/libs/sources.js'); + vi.mocked(deleteSource).mockResolvedValue({ deleted: 1 }); + const { confirm } = await import('@inquirer/prompts'); + vi.mocked(confirm).mockResolvedValue(true); + const { runSourcesCommand } = await import('@/commands/sources.js'); + + await runSourcesCommand(['delete', 'abc-123']); + + expect(confirm).toHaveBeenCalledWith( + expect.objectContaining({ + message: expect.stringContaining('abc-123'), + default: false, + }), + ); + expect(vi.mocked(confirm).mock.calls[0][0].message).toContain( + 'cannot be undone', + ); + }); + + // A hostile source name/uuid surfaced through the picker must be stripped + // before it reaches the confirm prompt, same as every other printed field. + it('sanitizes the uuid in the confirmation prompt', async () => { + const control = String.fromCharCode(0x1b); + const { fetchSources, deleteSource } = await import('@/libs/sources.js'); + vi.mocked(fetchSources).mockResolvedValue([ + { ...webhookSource, uuid: `abc${control}123` }, + ]); + vi.mocked(deleteSource).mockResolvedValue({ deleted: 1 }); + const { select, confirm } = await import('@inquirer/prompts'); + vi.mocked(select).mockResolvedValue(`abc${control}123`); + vi.mocked(confirm).mockResolvedValue(true); const { runSourcesCommand } = await import('@/commands/sources.js'); await runSourcesCommand(['delete']); + expect(vi.mocked(confirm).mock.calls[0][0].message).not.toContain( + control, + ); + expect(vi.mocked(confirm).mock.calls[0][0].message).toContain('abc 123'); + }); + + // The scripting escape hatch: --yes deletes straight away with no prompt. + it('skips the confirmation and deletes when --yes is passed', async () => { + const { deleteSource } = await import('@/libs/sources.js'); + vi.mocked(deleteSource).mockResolvedValue({ deleted: 1 }); + const { confirm } = await import('@inquirer/prompts'); + const { runSourcesCommand } = await import('@/commands/sources.js'); + + await runSourcesCommand(['delete', 'abc-123', '--yes']); + + expect(confirm).not.toHaveBeenCalled(); expect(deleteSource).toHaveBeenCalledWith('abc-123'); }); + // --yes is meaningless outside delete; it must fail loudly like a misplaced + // --json rather than appearing to take effect. + it('rejects --yes on a non-delete subcommand', async () => { + const { checkConfig } = await import('@/libs/config.js'); + const { fetchSources } = await import('@/libs/sources.js'); + const { runSourcesCommand } = await import('@/commands/sources.js'); + + await runSourcesCommand(['list', '--yes']); + + expect(checkConfig).not.toHaveBeenCalled(); + expect(fetchSources).not.toHaveBeenCalled(); + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining('--yes is only supported by `sources delete`.'), + ); + expect(process.exitCode).toBe(1); + }); + it('does nothing when there are no sources to pick from', async () => { const { fetchSources, deleteSource } = await import('@/libs/sources.js'); vi.mocked(fetchSources).mockResolvedValue([]); + const { confirm } = await import('@inquirer/prompts'); const { runSourcesCommand } = await import('@/commands/sources.js'); await runSourcesCommand(['delete']); expect(deleteSource).not.toHaveBeenCalled(); + expect(confirm).not.toHaveBeenCalled(); expect(console.log).toHaveBeenCalledWith('No sources to delete.'); }); it('reports an error when deletion fails', async () => { const { deleteSource } = await import('@/libs/sources.js'); vi.mocked(deleteSource).mockResolvedValue(null); + const { confirm } = await import('@inquirer/prompts'); + vi.mocked(confirm).mockResolvedValue(true); const { runSourcesCommand } = await import('@/commands/sources.js'); await runSourcesCommand(['delete', 'abc-123']); From 343ba6af9f39ad4b7cf2155cf94b588250283586 Mon Sep 17 00:00:00 2001 From: Grimicorn Agent Date: Thu, 27 Aug 2026 06:23:38 -0500 Subject: [PATCH 2/5] Address review: TTY guard, --yes requires uuid, name in confirm, tests --- README.md | 2 +- src/commands/sources.ts | 47 +++++++++++++-- tests/commands/sources.test.ts | 106 +++++++++++++++++++++++++++++++++ 3 files changed, 148 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 68de6c2..e85950a 100644 --- a/README.md +++ b/README.md @@ -22,7 +22,7 @@ server-side records. | `markpost sync [--dry-run]` | Fetch all pending records, write each to a markdown file, and (when `autoDelete` is enabled) delete the written records from the server. `--dry-run` reports the exact write/delete plan without writing or mutating anything | | `markpost push ` | Create records from one or more markdown files, directories, or glob patterns | | `markpost get [--json]` | Fetch and display a single record; pass `--json` for machine-readable output | -| `markpost sources [uuid] [--yes]` | Manage sources; `sources list --json` prints machine-readable output. `sources delete` asks to confirm first (deleting a source is irreversible — it drops the ingest config and one-time signing secret); pass `--yes` to skip the prompt in scripts | +| `markpost sources [uuid] [--yes]` | Manage sources; `sources list --json` prints machine-readable output. `sources delete` asks to confirm first (deleting a source is irreversible — it drops the ingest config and one-time signing secret) and needs an interactive terminal; in scripts pass a uuid with `--yes` (`sources delete --yes`) to skip the prompt | | `markpost records list [--source ] [--status ] [--search ] [--json]` | List records without deleting them, optionally filtered by source, status, or search text; pass `--json` for machine-readable output | | `markpost config [key] [value]` | View or change the stored API token and output directory | | `markpost settings [key=value ...]` | View or change server-side sync settings (`autoSync`, `autoDelete`, `frontmatter`, `conflictStrategy`) | diff --git a/src/commands/sources.ts b/src/commands/sources.ts index c0abe3d..e57507e 100644 --- a/src/commands/sources.ts +++ b/src/commands/sources.ts @@ -25,7 +25,7 @@ export const USAGE = `Usage: markpost sources [uuid] list List all sources (pass --json for machine-readable output) create Create a new source (prompts for details) update [uuid] Update a source's route folder; prompts to pick one if uuid is omitted - delete [uuid] Delete a source; prompts to pick one if uuid is omitted. Asks to confirm first; pass --yes to skip the prompt (for scripts)`; + delete [uuid] Delete a source; prompts to pick one if uuid is omitted. Asks to confirm first; pass a uuid with --yes to skip the prompt (for scripts)`; export const buildEndpointUrl = ( sourceType: SourceType, @@ -119,6 +119,34 @@ export const runSourcesCommand = async (args: string[]): Promise => { return; } + // `--yes` promises a non-interactive delete, so it needs an explicit uuid — + // without one the picker still opens and a script blocks on it forever. + if (skipConfirm && !uuid) { + failWithUsage( + `--yes requires a uuid: \`markpost sources ${DELETE_SUBCOMMAND} --yes\`.`, + USAGE, + json, + ); + return; + } + + // The confirmation prompt can't be answered without a TTY: inquirer aborts + // on stdin EOF and that abort is swallowed as a Ctrl+C below, so a + // non-interactive `sources delete` would delete nothing yet still exit 0. + // Fail loud instead and point scripts at --yes. + if ( + subcommand === DELETE_SUBCOMMAND && + !skipConfirm && + !process.stdin.isTTY + ) { + failWithUsage( + '`sources delete` needs an interactive terminal to confirm; pass --yes to delete without a prompt.', + USAGE, + json, + ); + return; + } + if (!(await checkConfig(json))) { return; } @@ -361,15 +389,15 @@ const updateSourceCommand = async (uuid?: string): Promise => { }; // Deleting a source is irreversible: it drops the ingest config and the -// one-time signing secret, which can never be retrieved again. The uuid is +// one-time signing secret, which can never be retrieved again. The label is // sanitized because it may have come from an untrusted API response via the // interactive picker. Defaults to "no" so a bare Enter cancels rather than // deletes. Isolated here so the delete flow stays unit-testable by mocking the // prompt. -const confirmDeletion = async (targetUuid: string): Promise => +const confirmDeletion = async (label: string): Promise => confirm({ message: `Delete source ${sanitizeForTerminal( - targetUuid, + label, )}? This drops its ingest config and one-time signing secret and cannot be undone.`, default: false, }); @@ -378,13 +406,20 @@ const deleteSourceCommand = async ( uuid: string | undefined, skipConfirm: boolean, ): Promise => { - const targetUuid = uuid ?? (await promptForSource('delete'))?.uuid; + // Only the interactive path yields a full Source; the direct-uuid path + // deletes by uuid without a lookup, so it can only name the uuid. + const picked = uuid ? undefined : await promptForSource('delete'); + const targetUuid = uuid ?? picked?.uuid; if (!targetUuid) { return; } - if (!skipConfirm && !(await confirmDeletion(targetUuid))) { + // Name the source the user picked ("name (uuid)") so the confirmation is + // recognizable; a bare-uuid delete only has the uuid to show. + const label = picked ? `${picked.name} (${targetUuid})` : targetUuid; + + if (!skipConfirm && !(await confirmDeletion(label))) { console.log('Deletion cancelled.'); return; } diff --git a/tests/commands/sources.test.ts b/tests/commands/sources.test.ts index 8d38ca8..d1c1141 100644 --- a/tests/commands/sources.test.ts +++ b/tests/commands/sources.test.ts @@ -91,16 +91,22 @@ describe('buildEndpointUrl', () => { }); describe('runSourcesCommand', () => { + // `sources delete` now refuses to prompt without a TTY, so simulate an + // interactive terminal by default; the non-TTY guard has its own case below. + const originalIsTTY = process.stdin.isTTY; + beforeEach(() => { vi.resetModules(); vi.clearAllMocks(); process.exitCode = undefined; + process.stdin.isTTY = true; vi.spyOn(console, 'log').mockImplementation(() => {}); vi.spyOn(console, 'error').mockImplementation(() => {}); }); afterEach(() => { process.exitCode = undefined; + process.stdin.isTTY = originalIsTTY; }); it('always checks config before dispatching', async () => { @@ -794,6 +800,42 @@ describe('runSourcesCommand', () => { expect(deleteSource).toHaveBeenCalledWith('abc-123'); }); + // A Ctrl+C at the confirmation is a deliberate abort: it must delete + // nothing, exit 0, and stay quiet — never fall through to the DELETE. + it('aborts cleanly without deleting when the confirmation is Ctrl+C-ed', async () => { + const { deleteSource } = await import('@/libs/sources.js'); + const { confirm } = await import('@inquirer/prompts'); + const exitPromptError = Object.assign(new Error('User force closed'), { + name: 'ExitPromptError', + }); + vi.mocked(confirm).mockRejectedValue(exitPromptError); + const { runSourcesCommand } = await import('@/commands/sources.js'); + + await runSourcesCommand(['delete', 'abc-123']); + + expect(deleteSource).not.toHaveBeenCalled(); + expect(process.exitCode).toBeUndefined(); + expect(console.error).not.toHaveBeenCalled(); + }); + + // The interactive confirmation names the picked source so the user can + // recognize it, not just the opaque uuid they never typed. + it('names the picked source in the confirmation prompt', async () => { + const { fetchSources, deleteSource } = await import('@/libs/sources.js'); + vi.mocked(fetchSources).mockResolvedValue([webhookSource]); + vi.mocked(deleteSource).mockResolvedValue({ deleted: 1 }); + const { select, confirm } = await import('@inquirer/prompts'); + vi.mocked(select).mockResolvedValue('abc-123'); + vi.mocked(confirm).mockResolvedValue(true); + const { runSourcesCommand } = await import('@/commands/sources.js'); + + await runSourcesCommand(['delete']); + + const message = vi.mocked(confirm).mock.calls[0][0].message; + expect(message).toContain('Webhook Source'); + expect(message).toContain('abc-123'); + }); + // --yes is meaningless outside delete; it must fail loudly like a misplaced // --json rather than appearing to take effect. it('rejects --yes on a non-delete subcommand', async () => { @@ -811,6 +853,70 @@ describe('runSourcesCommand', () => { expect(process.exitCode).toBe(1); }); + // The guard is shared, but assert create and update hit it too so a future + // reorder that moves it below dispatch can't pass unnoticed. + it('rejects --yes on create and update, not just list', async () => { + const { runSourcesCommand } = await import('@/commands/sources.js'); + + await runSourcesCommand(['create', '--yes']); + expect(process.exitCode).toBe(1); + + process.exitCode = undefined; + await runSourcesCommand(['update', '--yes']); + expect(process.exitCode).toBe(1); + }); + + // --yes promises a non-interactive delete, so with no uuid it must fail + // rather than open the picker a script can't answer. + it('rejects --yes without a uuid instead of opening the picker', async () => { + const { fetchSources, deleteSource } = await import('@/libs/sources.js'); + const { select } = await import('@inquirer/prompts'); + const { runSourcesCommand } = await import('@/commands/sources.js'); + + await runSourcesCommand(['delete', '--yes']); + + expect(select).not.toHaveBeenCalled(); + expect(fetchSources).not.toHaveBeenCalled(); + expect(deleteSource).not.toHaveBeenCalled(); + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining('--yes requires a uuid'), + ); + expect(process.exitCode).toBe(1); + }); + + // Without a TTY the prompt can't be answered and inquirer's abort is + // swallowed as Ctrl+C, so a bare non-interactive delete must fail loud + // rather than silently delete nothing and exit 0. + it('fails loudly on a non-TTY delete when --yes is absent', async () => { + process.stdin.isTTY = false; + const { deleteSource } = await import('@/libs/sources.js'); + const { confirm } = await import('@inquirer/prompts'); + const { runSourcesCommand } = await import('@/commands/sources.js'); + + await runSourcesCommand(['delete', 'abc-123']); + + expect(confirm).not.toHaveBeenCalled(); + expect(deleteSource).not.toHaveBeenCalled(); + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining('needs an interactive terminal'), + ); + expect(process.exitCode).toBe(1); + }); + + // The scripting path: --yes with a uuid deletes on a non-TTY, no prompt. + it('deletes on a non-TTY when a uuid and --yes are given', async () => { + process.stdin.isTTY = false; + const { deleteSource } = await import('@/libs/sources.js'); + vi.mocked(deleteSource).mockResolvedValue({ deleted: 1 }); + const { confirm } = await import('@inquirer/prompts'); + const { runSourcesCommand } = await import('@/commands/sources.js'); + + await runSourcesCommand(['delete', 'abc-123', '--yes']); + + expect(confirm).not.toHaveBeenCalled(); + expect(deleteSource).toHaveBeenCalledWith('abc-123'); + }); + it('does nothing when there are no sources to pick from', async () => { const { fetchSources, deleteSource } = await import('@/libs/sources.js'); vi.mocked(fetchSources).mockResolvedValue([]); From cbcf0c007a7c18f2b37989b12c282e52f4eee13c Mon Sep 17 00:00:00 2001 From: Grimicorn Agent Date: Thu, 27 Aug 2026 06:32:13 -0500 Subject: [PATCH 3/5] Address review round 2: label lookup, stdout guard, validator extract, tests --- src/commands/sources.ts | 133 +++++++++++++++++++-------------- tests/commands/sources.test.ts | 108 ++++++++++++++++++++++---- 2 files changed, 170 insertions(+), 71 deletions(-) diff --git a/src/commands/sources.ts b/src/commands/sources.ts index e57507e..08ad26a 100644 --- a/src/commands/sources.ts +++ b/src/commands/sources.ts @@ -66,6 +66,51 @@ const SOURCES_HANDLERS = new Map< ], ]); +// The invocation-level usage checks that all fail the same way (one usage +// message, non-zero exit). Returns the message to show, or null when the +// invocation is valid. Kept in one place so their ordering is a single unit +// rather than four near-identical guard blocks in the runner. +const usageErrorFor = ( + subcommand: string, + uuid: string | undefined, + json: boolean, + skipConfirm: boolean, +): string | null => { + // Reject --json where it does nothing rather than silently ignoring it: + // `sources create --json | jq` would otherwise "succeed" with human text on + // stdout, losing the one-time signing secret it was trying to capture. + if (json && subcommand !== LIST_SUBCOMMAND) { + return `--json is only supported by \`sources ${LIST_SUBCOMMAND}\`.`; + } + + // --yes only skips the delete confirmation; reject it elsewhere so a + // misplaced flag fails loudly instead of appearing to take effect. + if (skipConfirm && subcommand !== DELETE_SUBCOMMAND) { + return `--yes is only supported by \`sources ${DELETE_SUBCOMMAND}\`.`; + } + + // --yes promises a non-interactive delete, so it needs an explicit uuid — + // without one the picker still opens and a script blocks on it forever. + if (skipConfirm && !uuid) { + return `--yes requires a uuid: \`markpost sources ${DELETE_SUBCOMMAND} --yes\`.`; + } + + // The confirmation prompt can't be answered without an interactive terminal: + // inquirer renders to stdout and reads stdin, and its EOF abort is swallowed + // as a Ctrl+C below — so a redirected/non-interactive `sources delete` would + // hang or delete nothing yet still exit 0. Fail loud and point scripts at + // --yes. + if ( + subcommand === DELETE_SUBCOMMAND && + !skipConfirm && + (!process.stdin.isTTY || !process.stdout.isTTY) + ) { + return '`sources delete` needs an interactive terminal to confirm; pass --yes to delete without a prompt.'; + } + + return null; +}; + export const runSourcesCommand = async (args: string[]): Promise => { // Read `--json` straight from argv so every failure below is rendered in // whichever contract the caller asked for, even one thrown before parsing. @@ -88,62 +133,18 @@ export const runSourcesCommand = async (args: string[]): Promise => { const handler = SOURCES_HANDLERS.get(subcommand); // Validate before the config check so a bad subcommand fails on usage - // alone, without needing a configured account. + // alone, without needing a configured account. The bad-subcommand case + // fails differently (it prints the subcommand), so it stays here; the rest + // share one usage-error shape and live in `usageErrorFor`. if (!handler) { failWithSubcommandUsage(subcommand, USAGE, json); return; } - // Reject --json where it does nothing rather than silently ignoring it: - // `sources create --json | jq` would otherwise "succeed" with human text on - // stdout, and the one-time signing secret it was trying to capture would be - // lost (see createSourceCommand's unrecoverable-secret warning). - if (json && subcommand !== LIST_SUBCOMMAND) { - failWithUsage( - `--json is only supported by \`sources ${LIST_SUBCOMMAND}\`.`, - USAGE, - json, - ); - return; - } - - // `--yes` only skips the delete confirmation; reject it anywhere else - // rather than silently ignoring it, mirroring the --json guard above so a - // misplaced flag fails loudly instead of appearing to take effect. - if (skipConfirm && subcommand !== DELETE_SUBCOMMAND) { - failWithUsage( - `--yes is only supported by \`sources ${DELETE_SUBCOMMAND}\`.`, - USAGE, - json, - ); - return; - } - - // `--yes` promises a non-interactive delete, so it needs an explicit uuid — - // without one the picker still opens and a script blocks on it forever. - if (skipConfirm && !uuid) { - failWithUsage( - `--yes requires a uuid: \`markpost sources ${DELETE_SUBCOMMAND} --yes\`.`, - USAGE, - json, - ); - return; - } + const usageError = usageErrorFor(subcommand, uuid, json, skipConfirm); - // The confirmation prompt can't be answered without a TTY: inquirer aborts - // on stdin EOF and that abort is swallowed as a Ctrl+C below, so a - // non-interactive `sources delete` would delete nothing yet still exit 0. - // Fail loud instead and point scripts at --yes. - if ( - subcommand === DELETE_SUBCOMMAND && - !skipConfirm && - !process.stdin.isTTY - ) { - failWithUsage( - '`sources delete` needs an interactive terminal to confirm; pass --yes to delete without a prompt.', - USAGE, - json, - ); + if (usageError) { + failWithUsage(usageError, USAGE, json); return; } @@ -402,12 +403,31 @@ const confirmDeletion = async (label: string): Promise => default: false, }); +// Build the confirmation label. The interactive pick already carries the +// Source; a bare-uuid delete looks the source up (read-only, best-effort) so +// the prompt names it — surfacing a wrong-but-valid copy-pasted uuid before it +// destroys the source rather than echoing back the exact string the user +// typed. Falls back to the raw uuid when the list can't be loaded +// (fetchSources swallows transport errors into []), so a lookup miss never +// blocks a delete that would otherwise succeed. +const deleteConfirmationLabel = async ( + picked: Source | null | undefined, + targetUuid: string, +): Promise => { + if (picked) { + return `${picked.name} (${targetUuid})`; + } + + const sources = await fetchSources(); + const source = sources.find((candidate) => candidate.uuid === targetUuid); + + return source ? `${source.name} (${targetUuid})` : targetUuid; +}; + const deleteSourceCommand = async ( uuid: string | undefined, skipConfirm: boolean, ): Promise => { - // Only the interactive path yields a full Source; the direct-uuid path - // deletes by uuid without a lookup, so it can only name the uuid. const picked = uuid ? undefined : await promptForSource('delete'); const targetUuid = uuid ?? picked?.uuid; @@ -415,11 +435,10 @@ const deleteSourceCommand = async ( return; } - // Name the source the user picked ("name (uuid)") so the confirmation is - // recognizable; a bare-uuid delete only has the uuid to show. - const label = picked ? `${picked.name} (${targetUuid})` : targetUuid; - - if (!skipConfirm && !(await confirmDeletion(label))) { + if ( + !skipConfirm && + !(await confirmDeletion(await deleteConfirmationLabel(picked, targetUuid))) + ) { console.log('Deletion cancelled.'); return; } diff --git a/tests/commands/sources.test.ts b/tests/commands/sources.test.ts index d1c1141..3a776e0 100644 --- a/tests/commands/sources.test.ts +++ b/tests/commands/sources.test.ts @@ -91,22 +91,26 @@ describe('buildEndpointUrl', () => { }); describe('runSourcesCommand', () => { - // `sources delete` now refuses to prompt without a TTY, so simulate an - // interactive terminal by default; the non-TTY guard has its own case below. - const originalIsTTY = process.stdin.isTTY; + // `sources delete` now refuses to prompt without an interactive terminal + // (both stdin and stdout must be TTYs), so simulate one by default; the + // non-TTY guards have their own cases below. + const originalStdinIsTTY = process.stdin.isTTY; + const originalStdoutIsTTY = process.stdout.isTTY; beforeEach(() => { vi.resetModules(); vi.clearAllMocks(); process.exitCode = undefined; process.stdin.isTTY = true; + process.stdout.isTTY = true; vi.spyOn(console, 'log').mockImplementation(() => {}); vi.spyOn(console, 'error').mockImplementation(() => {}); }); afterEach(() => { process.exitCode = undefined; - process.stdin.isTTY = originalIsTTY; + process.stdin.isTTY = originalStdinIsTTY; + process.stdout.isTTY = originalStdoutIsTTY; }); it('always checks config before dispatching', async () => { @@ -702,7 +706,8 @@ describe('runSourcesCommand', () => { describe('delete', () => { it('deletes directly by uuid when one is provided and confirmed', async () => { - const { deleteSource } = await import('@/libs/sources.js'); + const { fetchSources, deleteSource } = await import('@/libs/sources.js'); + vi.mocked(fetchSources).mockResolvedValue([webhookSource]); vi.mocked(deleteSource).mockResolvedValue({ deleted: 1 }); const { select, confirm } = await import('@inquirer/prompts'); vi.mocked(confirm).mockResolvedValue(true); @@ -732,7 +737,8 @@ describe('runSourcesCommand', () => { // The confirmation is the whole point of the feature: a "no" answer must // abort before any DELETE reaches the server. it('aborts without deleting when the confirmation is declined', async () => { - const { deleteSource } = await import('@/libs/sources.js'); + const { fetchSources, deleteSource } = await import('@/libs/sources.js'); + vi.mocked(fetchSources).mockResolvedValue([webhookSource]); const { confirm } = await import('@inquirer/prompts'); vi.mocked(confirm).mockResolvedValue(false); const { runSourcesCommand } = await import('@/commands/sources.js'); @@ -746,7 +752,8 @@ describe('runSourcesCommand', () => { // The confirm message must name the source being deleted so the user knows // what they're destroying, and warn that it's irreversible. it('shows the uuid and an irreversible-warning in the confirmation prompt', async () => { - const { deleteSource } = await import('@/libs/sources.js'); + const { fetchSources, deleteSource } = await import('@/libs/sources.js'); + vi.mocked(fetchSources).mockResolvedValue([webhookSource]); vi.mocked(deleteSource).mockResolvedValue({ deleted: 1 }); const { confirm } = await import('@inquirer/prompts'); vi.mocked(confirm).mockResolvedValue(true); @@ -765,6 +772,41 @@ describe('runSourcesCommand', () => { ); }); + // The direct-uuid path looks the source up (like update does) so a valid + // but wrong copy-pasted uuid shows its real name for the user to catch, + // rather than echoing back the string they typed. + it('names the looked-up source on the direct-uuid path', async () => { + const { fetchSources, deleteSource } = await import('@/libs/sources.js'); + vi.mocked(fetchSources).mockResolvedValue([webhookSource]); + vi.mocked(deleteSource).mockResolvedValue({ deleted: 1 }); + const { confirm } = await import('@inquirer/prompts'); + vi.mocked(confirm).mockResolvedValue(true); + const { runSourcesCommand } = await import('@/commands/sources.js'); + + await runSourcesCommand(['delete', 'abc-123']); + + expect(vi.mocked(confirm).mock.calls[0][0].message).toContain( + 'Webhook Source', + ); + }); + + // A lookup that can't resolve the uuid (e.g. fetchSources swallowed a + // transport error into []) must still confirm on the bare uuid, never block + // the delete. + it('falls back to the bare uuid when the source lookup misses', async () => { + const { fetchSources, deleteSource } = await import('@/libs/sources.js'); + vi.mocked(fetchSources).mockResolvedValue([]); + vi.mocked(deleteSource).mockResolvedValue({ deleted: 1 }); + const { confirm } = await import('@inquirer/prompts'); + vi.mocked(confirm).mockResolvedValue(true); + const { runSourcesCommand } = await import('@/commands/sources.js'); + + await runSourcesCommand(['delete', 'abc-123']); + + expect(vi.mocked(confirm).mock.calls[0][0].message).toContain('abc-123'); + expect(deleteSource).toHaveBeenCalledWith('abc-123'); + }); + // A hostile source name/uuid surfaced through the picker must be stripped // before it reaches the confirm prompt, same as every other printed field. it('sanitizes the uuid in the confirmation prompt', async () => { @@ -803,7 +845,8 @@ describe('runSourcesCommand', () => { // A Ctrl+C at the confirmation is a deliberate abort: it must delete // nothing, exit 0, and stay quiet — never fall through to the DELETE. it('aborts cleanly without deleting when the confirmation is Ctrl+C-ed', async () => { - const { deleteSource } = await import('@/libs/sources.js'); + const { fetchSources, deleteSource } = await import('@/libs/sources.js'); + vi.mocked(fetchSources).mockResolvedValue([webhookSource]); const { confirm } = await import('@inquirer/prompts'); const exitPromptError = Object.assign(new Error('User force closed'), { name: 'ExitPromptError', @@ -853,16 +896,33 @@ describe('runSourcesCommand', () => { expect(process.exitCode).toBe(1); }); - // The guard is shared, but assert create and update hit it too so a future - // reorder that moves it below dispatch can't pass unnoticed. - it('rejects --yes on create and update, not just list', async () => { + // The guard is shared, but assert create and update hit it *before* + // dispatch (no prompts, no API calls) so a future reorder that moves it + // below dispatch can't slip past on the exit code alone. + it('rejects --yes on create before dispatching', async () => { + const { checkConfig } = await import('@/libs/config.js'); + const { createSource } = await import('@/libs/sources.js'); + const { select } = await import('@inquirer/prompts'); const { runSourcesCommand } = await import('@/commands/sources.js'); await runSourcesCommand(['create', '--yes']); + + expect(checkConfig).not.toHaveBeenCalled(); + expect(select).not.toHaveBeenCalled(); + expect(createSource).not.toHaveBeenCalled(); expect(process.exitCode).toBe(1); + }); + + it('rejects --yes on update before dispatching', async () => { + const { checkConfig } = await import('@/libs/config.js'); + const { fetchSources, updateSource } = await import('@/libs/sources.js'); + const { runSourcesCommand } = await import('@/commands/sources.js'); - process.exitCode = undefined; await runSourcesCommand(['update', '--yes']); + + expect(checkConfig).not.toHaveBeenCalled(); + expect(fetchSources).not.toHaveBeenCalled(); + expect(updateSource).not.toHaveBeenCalled(); expect(process.exitCode).toBe(1); }); @@ -887,7 +947,7 @@ describe('runSourcesCommand', () => { // Without a TTY the prompt can't be answered and inquirer's abort is // swallowed as Ctrl+C, so a bare non-interactive delete must fail loud // rather than silently delete nothing and exit 0. - it('fails loudly on a non-TTY delete when --yes is absent', async () => { + it('fails loudly on a non-TTY stdin delete when --yes is absent', async () => { process.stdin.isTTY = false; const { deleteSource } = await import('@/libs/sources.js'); const { confirm } = await import('@inquirer/prompts'); @@ -903,6 +963,25 @@ describe('runSourcesCommand', () => { expect(process.exitCode).toBe(1); }); + // Redirected stdout (`sources delete > log`) leaves stdin a TTY but + // hides the prompt inquirer renders to stdout — same hang, so it must fail + // the same way. + it('fails loudly on a redirected-stdout delete when --yes is absent', async () => { + process.stdout.isTTY = false; + const { deleteSource } = await import('@/libs/sources.js'); + const { confirm } = await import('@inquirer/prompts'); + const { runSourcesCommand } = await import('@/commands/sources.js'); + + await runSourcesCommand(['delete', 'abc-123']); + + expect(confirm).not.toHaveBeenCalled(); + expect(deleteSource).not.toHaveBeenCalled(); + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining('needs an interactive terminal'), + ); + expect(process.exitCode).toBe(1); + }); + // The scripting path: --yes with a uuid deletes on a non-TTY, no prompt. it('deletes on a non-TTY when a uuid and --yes are given', async () => { process.stdin.isTTY = false; @@ -931,7 +1010,8 @@ describe('runSourcesCommand', () => { }); it('reports an error when deletion fails', async () => { - const { deleteSource } = await import('@/libs/sources.js'); + const { fetchSources, deleteSource } = await import('@/libs/sources.js'); + vi.mocked(fetchSources).mockResolvedValue([webhookSource]); vi.mocked(deleteSource).mockResolvedValue(null); const { confirm } = await import('@inquirer/prompts'); vi.mocked(confirm).mockResolvedValue(true); From 5d1b31f4be2bb15d4c7649f80b7919f3788fce62 Mon Sep 17 00:00:00 2001 From: Grimicorn Agent Date: Thu, 27 Aug 2026 06:36:44 -0500 Subject: [PATCH 4/5] Address review round 3: best-effort label lookup, visible miss, dedupe, pure validator --- src/commands/sources.ts | 77 +++++++++++++++++++++++----------- tests/commands/sources.test.ts | 38 ++++++++++++++--- 2 files changed, 84 insertions(+), 31 deletions(-) diff --git a/src/commands/sources.ts b/src/commands/sources.ts index 08ad26a..ead3bd6 100644 --- a/src/commands/sources.ts +++ b/src/commands/sources.ts @@ -75,6 +75,7 @@ const usageErrorFor = ( uuid: string | undefined, json: boolean, skipConfirm: boolean, + isInteractive: boolean, ): string | null => { // Reject --json where it does nothing rather than silently ignoring it: // `sources create --json | jq` would otherwise "succeed" with human text on @@ -99,12 +100,10 @@ const usageErrorFor = ( // inquirer renders to stdout and reads stdin, and its EOF abort is swallowed // as a Ctrl+C below — so a redirected/non-interactive `sources delete` would // hang or delete nothing yet still exit 0. Fail loud and point scripts at - // --yes. - if ( - subcommand === DELETE_SUBCOMMAND && - !skipConfirm && - (!process.stdin.isTTY || !process.stdout.isTTY) - ) { + // --yes. Only delete is guarded here because it's the irreversible one; + // `create`/`update` also prompt, but that predates this change and their + // non-TTY behavior is out of scope for the delete-confirmation work. + if (subcommand === DELETE_SUBCOMMAND && !skipConfirm && !isInteractive) { return '`sources delete` needs an interactive terminal to confirm; pass --yes to delete without a prompt.'; } @@ -141,7 +140,17 @@ export const runSourcesCommand = async (args: string[]): Promise => { return; } - const usageError = usageErrorFor(subcommand, uuid, json, skipConfirm); + // A prompt needs both streams to be a terminal: inquirer reads stdin and + // renders to stdout, so a redirect on either makes the confirmation + // unanswerable. + const isInteractive = Boolean(process.stdin.isTTY && process.stdout.isTTY); + const usageError = usageErrorFor( + subcommand, + uuid, + json, + skipConfirm, + isInteractive, + ); if (usageError) { failWithUsage(usageError, USAGE, json); @@ -326,17 +335,23 @@ const promptForSource = async (action: string): Promise => { return sources.find((source) => source.uuid === selectedUuid) ?? null; }; -const findSourceByUuid = async (uuid: string): Promise => { +// Fetch the source list and pick one out by uuid, or null if none matches. +// fetchSources() swallows transport errors (except a timeout, which +// propagates) into [], so a missing uuid is indistinguishable here from a +// failed load. Shared by the reporting `findSourceByUuid` and the best-effort +// delete label so the fetch+find isn't written three ways. +const lookupSourceByUuid = async (uuid: string): Promise => { const sources = await fetchSources(); - const source = sources.find((candidate) => candidate.uuid === uuid); + return sources.find((candidate) => candidate.uuid === uuid) ?? null; +}; + +const findSourceByUuid = async (uuid: string): Promise => { + const source = await lookupSourceByUuid(uuid); if (source) { return source; } - // fetchSources() swallows transport errors (except a timeout, which - // propagates) and returns [], so a uuid that doesn't match is - // indistinguishable here from a failed lookup. console.error( chalk.redBright( 'Source not found, or the source list could not be loaded.', @@ -403,13 +418,16 @@ const confirmDeletion = async (label: string): Promise => default: false, }); +const NO_SOURCE_FOUND_NOTE = 'no source found with this uuid'; + // Build the confirmation label. The interactive pick already carries the -// Source; a bare-uuid delete looks the source up (read-only, best-effort) so -// the prompt names it — surfacing a wrong-but-valid copy-pasted uuid before it -// destroys the source rather than echoing back the exact string the user -// typed. Falls back to the raw uuid when the list can't be loaded -// (fetchSources swallows transport errors into []), so a lookup miss never -// blocks a delete that would otherwise succeed. +// Source; a bare-uuid delete looks the source up so the prompt names it — +// surfacing a wrong-but-valid (or non-existent) copy-pasted uuid before it +// destroys anything, rather than echoing back the exact string the user typed. +// The lookup is purely cosmetic, so it's best-effort: any failure (including a +// timeout, which fetchSources re-throws) falls back to the bare uuid rather +// than blocking a delete that would otherwise succeed. A miss is called out in +// the label so it isn't mistaken for a successful match. const deleteConfirmationLabel = async ( picked: Source | null | undefined, targetUuid: string, @@ -418,12 +436,21 @@ const deleteConfirmationLabel = async ( return `${picked.name} (${targetUuid})`; } - const sources = await fetchSources(); - const source = sources.find((candidate) => candidate.uuid === targetUuid); + const source = await lookupSourceByUuid(targetUuid).catch(() => null); - return source ? `${source.name} (${targetUuid})` : targetUuid; + return source + ? `${source.name} (${targetUuid})` + : `${targetUuid} (${NO_SOURCE_FOUND_NOTE})`; }; +// Build the label, then prompt. Kept separate from the runner so the `--yes` +// short-circuit skips the label lookup (and its fetch) entirely. +const confirmSourceDeletion = async ( + picked: Source | null | undefined, + targetUuid: string, +): Promise => + confirmDeletion(await deleteConfirmationLabel(picked, targetUuid)); + const deleteSourceCommand = async ( uuid: string | undefined, skipConfirm: boolean, @@ -435,10 +462,10 @@ const deleteSourceCommand = async ( return; } - if ( - !skipConfirm && - !(await confirmDeletion(await deleteConfirmationLabel(picked, targetUuid))) - ) { + const confirmed = + skipConfirm || (await confirmSourceDeletion(picked, targetUuid)); + + if (!confirmed) { console.log('Deletion cancelled.'); return; } diff --git a/tests/commands/sources.test.ts b/tests/commands/sources.test.ts index 3a776e0..b4476f7 100644 --- a/tests/commands/sources.test.ts +++ b/tests/commands/sources.test.ts @@ -791,9 +791,9 @@ describe('runSourcesCommand', () => { }); // A lookup that can't resolve the uuid (e.g. fetchSources swallowed a - // transport error into []) must still confirm on the bare uuid, never block - // the delete. - it('falls back to the bare uuid when the source lookup misses', async () => { + // transport error into []) must still confirm on the bare uuid — and flag + // the miss so it isn't mistaken for a matched source — never block delete. + it('flags the miss in the label when the source lookup finds nothing', async () => { const { fetchSources, deleteSource } = await import('@/libs/sources.js'); vi.mocked(fetchSources).mockResolvedValue([]); vi.mocked(deleteSource).mockResolvedValue({ deleted: 1 }); @@ -803,8 +803,32 @@ describe('runSourcesCommand', () => { await runSourcesCommand(['delete', 'abc-123']); + const message = vi.mocked(confirm).mock.calls[0][0].message; + expect(message).toContain('abc-123'); + expect(message).toContain('no source found with this uuid'); + expect(deleteSource).toHaveBeenCalledWith('abc-123'); + }); + + // The label lookup is cosmetic: a timeout (which fetchSources re-throws, not + // swallows) must not abort a delete that issued no such read before this + // change. It falls back to the bare uuid and still confirms + deletes. + it('still confirms and deletes when the label lookup times out', async () => { + const { fetchSources, deleteSource } = await import('@/libs/sources.js'); + const timeoutError = Object.assign(new Error('timed out'), { + name: 'ApiTimeoutError', + }); + vi.mocked(fetchSources).mockRejectedValue(timeoutError); + vi.mocked(deleteSource).mockResolvedValue({ deleted: 1 }); + const { confirm } = await import('@inquirer/prompts'); + vi.mocked(confirm).mockResolvedValue(true); + const { runSourcesCommand } = await import('@/commands/sources.js'); + + await runSourcesCommand(['delete', 'abc-123']); + + expect(confirm).toHaveBeenCalledTimes(1); expect(vi.mocked(confirm).mock.calls[0][0].message).toContain('abc-123'); expect(deleteSource).toHaveBeenCalledWith('abc-123'); + expect(process.exitCode).toBeUndefined(); }); // A hostile source name/uuid surfaced through the picker must be stripped @@ -829,9 +853,10 @@ describe('runSourcesCommand', () => { expect(vi.mocked(confirm).mock.calls[0][0].message).toContain('abc 123'); }); - // The scripting escape hatch: --yes deletes straight away with no prompt. - it('skips the confirmation and deletes when --yes is passed', async () => { - const { deleteSource } = await import('@/libs/sources.js'); + // The scripting escape hatch: --yes deletes straight away with no prompt + // and no label lookup (the short-circuit must skip fetchSources entirely). + it('skips the confirmation and the label lookup when --yes is passed', async () => { + const { fetchSources, deleteSource } = await import('@/libs/sources.js'); vi.mocked(deleteSource).mockResolvedValue({ deleted: 1 }); const { confirm } = await import('@inquirer/prompts'); const { runSourcesCommand } = await import('@/commands/sources.js'); @@ -839,6 +864,7 @@ describe('runSourcesCommand', () => { await runSourcesCommand(['delete', 'abc-123', '--yes']); expect(confirm).not.toHaveBeenCalled(); + expect(fetchSources).not.toHaveBeenCalled(); expect(deleteSource).toHaveBeenCalledWith('abc-123'); }); From 594543381216c385a764057f4a6899cc3ddf4645 Mon Sep 17 00:00:00 2001 From: Grimicorn Agent Date: Thu, 27 Aug 2026 06:40:48 -0500 Subject: [PATCH 5/5] Address review round 4: distinguish failed lookup from non-match, test hardening --- src/commands/sources.ts | 30 +++++++++++++++++++++--------- tests/commands/sources.test.ts | 13 ++++++++++--- 2 files changed, 31 insertions(+), 12 deletions(-) diff --git a/src/commands/sources.ts b/src/commands/sources.ts index ead3bd6..ec65f94 100644 --- a/src/commands/sources.ts +++ b/src/commands/sources.ts @@ -418,16 +418,23 @@ const confirmDeletion = async (label: string): Promise => default: false, }); -const NO_SOURCE_FOUND_NOTE = 'no source found with this uuid'; +// The list loaded but held no source with this uuid. True whether the uuid is +// wrong or the list was legitimately empty — never asserts more than that. +const NO_MATCH_NOTE = 'no matching source in the source list'; +// The lookup itself failed (e.g. a timeout, which fetchSources re-throws), so +// the name is simply unknown — distinct from a confirmed non-match, and never +// claiming the source doesn't exist. +const LOOKUP_FAILED_NOTE = 'source name unavailable — could not load the list'; // Build the confirmation label. The interactive pick already carries the // Source; a bare-uuid delete looks the source up so the prompt names it — // surfacing a wrong-but-valid (or non-existent) copy-pasted uuid before it // destroys anything, rather than echoing back the exact string the user typed. -// The lookup is purely cosmetic, so it's best-effort: any failure (including a -// timeout, which fetchSources re-throws) falls back to the bare uuid rather -// than blocking a delete that would otherwise succeed. A miss is called out in -// the label so it isn't mistaken for a successful match. +// The lookup is purely cosmetic, so it's best-effort: any failure falls back to +// the bare uuid rather than blocking a delete that would otherwise succeed. The +// three outcomes stay distinct in the label so a failed load is never +// mis-reported as a confirmed non-match. `undefined` marks a thrown lookup, +// `null` a loaded-but-missing one. const deleteConfirmationLabel = async ( picked: Source | null | undefined, targetUuid: string, @@ -436,15 +443,20 @@ const deleteConfirmationLabel = async ( return `${picked.name} (${targetUuid})`; } - const source = await lookupSourceByUuid(targetUuid).catch(() => null); + const source = await lookupSourceByUuid(targetUuid).catch(() => undefined); + + if (source === undefined) { + return `${targetUuid} (${LOOKUP_FAILED_NOTE})`; + } return source ? `${source.name} (${targetUuid})` - : `${targetUuid} (${NO_SOURCE_FOUND_NOTE})`; + : `${targetUuid} (${NO_MATCH_NOTE})`; }; -// Build the label, then prompt. Kept separate from the runner so the `--yes` -// short-circuit skips the label lookup (and its fetch) entirely. +// Compose label-building with the prompt into one named step so the call site +// reads as a sentence; the `||` at the call site is what short-circuits this +// away (label lookup included) under `--yes`. const confirmSourceDeletion = async ( picked: Source | null | undefined, targetUuid: string, diff --git a/tests/commands/sources.test.ts b/tests/commands/sources.test.ts index b4476f7..12e2dd3 100644 --- a/tests/commands/sources.test.ts +++ b/tests/commands/sources.test.ts @@ -747,6 +747,8 @@ describe('runSourcesCommand', () => { expect(deleteSource).not.toHaveBeenCalled(); expect(console.log).toHaveBeenCalledWith('Deletion cancelled.'); + // A user "no" is a clean abort, not a failure — must not exit non-zero. + expect(process.exitCode).toBeUndefined(); }); // The confirm message must name the source being deleted so the user knows @@ -805,13 +807,14 @@ describe('runSourcesCommand', () => { const message = vi.mocked(confirm).mock.calls[0][0].message; expect(message).toContain('abc-123'); - expect(message).toContain('no source found with this uuid'); + expect(message).toContain('no matching source in the source list'); expect(deleteSource).toHaveBeenCalledWith('abc-123'); }); // The label lookup is cosmetic: a timeout (which fetchSources re-throws, not // swallows) must not abort a delete that issued no such read before this - // change. It falls back to the bare uuid and still confirms + deletes. + // change. It falls back to the bare uuid, says the name is unavailable (not + // that the source is missing), and still confirms + deletes. it('still confirms and deletes when the label lookup times out', async () => { const { fetchSources, deleteSource } = await import('@/libs/sources.js'); const timeoutError = Object.assign(new Error('timed out'), { @@ -826,7 +829,11 @@ describe('runSourcesCommand', () => { await runSourcesCommand(['delete', 'abc-123']); expect(confirm).toHaveBeenCalledTimes(1); - expect(vi.mocked(confirm).mock.calls[0][0].message).toContain('abc-123'); + const message = vi.mocked(confirm).mock.calls[0][0].message; + expect(message).toContain('abc-123'); + // A failed load must not be reported as a confirmed non-match. + expect(message).not.toContain('no matching source in the source list'); + expect(message).toContain('could not load the list'); expect(deleteSource).toHaveBeenCalledWith('abc-123'); expect(process.exitCode).toBeUndefined(); });