Confirm before deleting a source; --yes skips for scripts

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 <path...>` | Create records from one or more markdown files, directories, or glob patterns |
 | `markpost get <uuid> [--json]` | Fetch and display a single record; pass `--json` for machine-readable output |
-| `markpost sources <list\|create\|update\|delete\|rotate-secret> [uuid]` | Manage sources; `sources list --json` prints machine-readable output. `rotate-secret [uuid]` mints/replaces the signing secret of a provider source (github/zapier/shortcuts reveal a fresh secret once; stripe prompts for the new value) |
+| `markpost sources <list\|create\|update\|delete\|rotate-secret> [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 <uuid> --yes`) to skip the prompt. `rotate-secret [uuid]` mints/replaces the signing secret of a provider source (github/zapier/shortcuts reveal a fresh secret once; stripe prompts for the new value) |
 | `markpost records list [--source <type>] [--status <status>] [--search <text>] [--json]` | List records without deleting them, optionally filtered by source, status, or search text; pass `--json` for machine-readable output |
 | `markpost config <get\|set\|path> [key] [value]` | View or change the stored API token and output directory |
 | `markpost settings <get\|set> [key=value ...]` | View or change server-side sync settings (`autoSync`, `autoDelete`, `frontmatter`, `conflictStrategy`) |

src/commands/sources.ts

@@ -1,6 +1,6 @@
 import { parseArgs } from 'node:util';
 import chalk from 'chalk';
-import { input, password, select } from '@inquirer/prompts';
+import { confirm, input, password, select } from '@inquirer/prompts';
 import {
   createSource,
   deleteSource,
@@ -34,7 +34,7 @@ export const USAGE = `Usage: markpost sources <list|create|update|delete|rotate-
   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 a uuid with --yes to skip the prompt (for scripts)
   rotate-secret [uuid]  Rotate a provider source's signing secret; prompts to pick one if uuid is omitted`;
 
 export const buildEndpointUrl = (
@@ -55,18 +55,72 @@ 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<void>
+  (
+    uuid: string | undefined,
+    json: boolean,
+    skipConfirm: boolean,
+  ) => Promise<void>
 >([
   [LIST_SUBCOMMAND, (_uuid, json) => listSources(json)],
   ['create', () => createSourceCommand()],
   ['update', (uuid) => updateSourceCommand(uuid)],
-  ['delete', (uuid) => deleteSourceCommand(uuid)],
+  [
+    DELETE_SUBCOMMAND,
+    (uuid, _json, skipConfirm) => deleteSourceCommand(uuid, skipConfirm),
+  ],
   ['rotate-secret', (uuid) => rotateSecretCommand(uuid)],
 ]);
 
+// 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,
+  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
+  // 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} <uuid> --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. 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 a uuid with --yes (\`markpost sources ${DELETE_SUBCOMMAND} <uuid> --yes\`) to delete without a prompt.`;
+  }
+
+  return null;
+};
+
 export const runSourcesCommand = async (args: string[]): Promise<void> => {
   // Read `--json` straight from argv so every failure below is rendered in
   // whichever contract the caller asked for, even one thrown before parsing.
@@ -76,41 +130,49 @@ export const runSourcesCommand = async (args: string[]): Promise<void> => {
     // `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
-    // 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,
-      );
+    // 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);
       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.
@@ -300,17 +362,23 @@ const promptForSource = async (
   return sources.find((source) => source.uuid === selectedUuid) ?? null;
 };
 
-const findSourceByUuid = async (uuid: string): Promise<Source | null> => {
+// 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<Source | null> => {
   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<Source | null> => {
+  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.',
@@ -363,17 +431,97 @@ const updateSourceCommand = async (uuid?: string): Promise<void> => {
   await promptAndApplyRouteFolder(target);
 };
 
-const deleteSourceCommand = async (uuid?: string): Promise<void> => {
-  const targetUuid = uuid ?? (await promptForSource('delete'))?.uuid;
+// Deleting a source is irreversible: it drops the ingest config and the
+// 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 (label: string): Promise<boolean> =>
+  confirm({
+    message: `Delete source ${sanitizeForTerminal(
+      label,
+    )}? This drops its ingest config and one-time signing secret and cannot be undone.`,
+    default: false,
+  });
+
+// An empty list from lookupSourceByUuid can't tell a genuine non-match from a
+// swallowed load failure — fetchSources() folds transport errors (all but a
+// timeout) into []. So this can't claim the source is absent; it mirrors
+// findSourceByUuid's wording and leaves both possibilities open.
+const NO_MATCH_NOTE =
+  'no matching source found, or the list could not be loaded';
+// 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 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,
+): Promise<string> => {
+  if (picked) {
+    return `${picked.name} (${targetUuid})`;
+  }
+
+  const source = await lookupSourceByUuid(targetUuid).catch(() => undefined);
+
+  if (source === undefined) {
+    return `${targetUuid} (${LOOKUP_FAILED_NOTE})`;
+  }
+
+  return source
+    ? `${source.name} (${targetUuid})`
+    : `${targetUuid} (${NO_MATCH_NOTE})`;
+};
+
+// 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,
+): Promise<boolean> =>
+  confirmDeletion(await deleteConfirmationLabel(picked, targetUuid));
+
+const deleteSourceCommand = async (
+  uuid: string | undefined,
+  skipConfirm: boolean,
+): Promise<void> => {
+  const picked = uuid ? undefined : await promptForSource('delete');
+  // `||` (not `??`) so an empty-string uuid falls through to the picked source,
+  // matching the truthiness branch above — otherwise `delete ""` would open the
+  // picker, take a selection, then silently discard it on the `!targetUuid` guard.
+  const targetUuid = uuid || picked?.uuid;
 
   if (!targetUuid) {
     return;
   }
 
+  const confirmed =
+    skipConfirm || (await confirmSourceDeletion(picked, targetUuid));
+
+  if (!confirmed) {
+    console.log('Deletion cancelled.');
+    return;
+  }
+
   const meta = await deleteSource(targetUuid);
 
   if (!meta) {
-    console.error(chalk.redBright('Failed to delete source.'));
+    // Exit non-zero (not a bare console.error) so a scripted `delete <uuid>
+    // --yes || notify` catches a failed delete instead of reading it as done —
+    // delete now carries a documented --yes contract, like rotate-secret below.
+    failWithMessage('Failed to delete source.');
     return;
   }