From aaf8ccf069558a912cf98d139b7d3fd290e4635f Mon Sep 17 00:00:00 2001 From: Danny Holloran Date: Sun, 6 Sep 2026 17:15:19 -0700 Subject: [PATCH 1/2] Guard sources create/update against non-interactive terminals Extends the both-streams TTY guard that sources delete already uses to sources create (always prompts) and sources update (always ends by prompting for the route folder, whether the target came from an explicit uuid or the interactive picker). Both now fail fast with a clear message under a non-interactive terminal instead of hanging. Closes #148 --- src/commands/sources.ts | 53 ++++++++--- tests/commands/sources.test.ts | 161 +++++++++++++++++++++++++-------- 2 files changed, 162 insertions(+), 52 deletions(-) diff --git a/src/commands/sources.ts b/src/commands/sources.ts index 80922fd..792a535 100644 --- a/src/commands/sources.ts +++ b/src/commands/sources.ts @@ -55,6 +55,8 @@ 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'; +const CREATE_SUBCOMMAND = 'create'; +const UPDATE_SUBCOMMAND = 'update'; // `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'; @@ -68,8 +70,8 @@ const SOURCES_HANDLERS = new Map< ) => Promise >([ [LIST_SUBCOMMAND, (_uuid, json) => listSources(json)], - ['create', () => createSourceCommand()], - ['update', (uuid) => updateSourceCommand(uuid)], + [CREATE_SUBCOMMAND, () => createSourceCommand()], + [UPDATE_SUBCOMMAND, (uuid) => updateSourceCommand(uuid)], [ DELETE_SUBCOMMAND, (uuid, _json, skipConfirm) => deleteSourceCommand(uuid, skipConfirm), @@ -77,6 +79,38 @@ const SOURCES_HANDLERS = new Map< ['rotate-secret', (uuid) => rotateSecretCommand(uuid)], ]); +// The message for the one subcommand (if any) that `subcommand` can't +// complete without an interactive terminal. Every one of these ends by +// rendering an inquirer prompt (a confirmation, a picker, or a text/select +// input), and inquirer needs both stdin and stdout to be a TTY to render and +// read one — a redirected/non-interactive run would otherwise hang, or (for +// delete) abort via the swallowed-Ctrl+C path below yet still exit 0. `delete` +// is only guarded here when `--yes` is absent (that flag is its documented +// escape hatch); `create` and `update` have no such flag — `create` always +// prompts, and `update` always ends by prompting for the route folder, +// whether the target came from an explicit uuid or the interactive picker — +// so both are guarded outright. Kept as one lookup (not three near-identical +// `!isInteractive` checks in `usageErrorFor`) so the guard condition itself +// stays in a single place. +const interactiveGuardMessageFor = ( + subcommand: string, + skipConfirm: boolean, +): string | null => { + if (subcommand === DELETE_SUBCOMMAND && !skipConfirm) { + return `\`sources delete\` needs an interactive terminal to confirm; pass a uuid with --yes (\`markpost sources ${DELETE_SUBCOMMAND} --yes\`) to delete without a prompt.`; + } + + if (subcommand === CREATE_SUBCOMMAND) { + return `\`sources create\` needs an interactive terminal — it always prompts for the source details.`; + } + + if (subcommand === UPDATE_SUBCOMMAND) { + return `\`sources update\` needs an interactive terminal — it prompts for the route folder, and to pick a source when no uuid is given.`; + } + + return null; +}; + // 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 @@ -107,15 +141,8 @@ const usageErrorFor = ( 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. 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} --yes\`) to delete without a prompt.`; + if (!isInteractive) { + return interactiveGuardMessageFor(subcommand, skipConfirm); } return null; @@ -152,8 +179,8 @@ export const runSourcesCommand = async (args: string[]): Promise => { } // 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. + // renders to stdout, so a redirect on either makes create/update/delete's + // prompts unanswerable. const isInteractive = Boolean(process.stdin.isTTY && process.stdout.isTTY); const usageError = usageErrorFor( subcommand, diff --git a/tests/commands/sources.test.ts b/tests/commands/sources.test.ts index addea93..0da8cc4 100644 --- a/tests/commands/sources.test.ts +++ b/tests/commands/sources.test.ts @@ -565,6 +565,44 @@ describe('runSourcesCommand', () => { expect.stringContaining('Failed to create source.'), ); }); + + // `create` always prompts (type, name, folder, provider) with no --yes-like + // escape hatch, so a non-TTY invocation must fail loud instead of hanging on + // the first unanswerable prompt — same guard shape as delete's non-TTY + // cases below. + it('fails loudly on a non-TTY stdin create instead of hanging on the first prompt', async () => { + process.stdin.isTTY = false; + const { select } = await import('@inquirer/prompts'); + const { createSource } = await import('@/libs/sources.js'); + const { runSourcesCommand } = await import('@/commands/sources.js'); + + await runSourcesCommand(['create']); + + expect(select).not.toHaveBeenCalled(); + expect(createSource).not.toHaveBeenCalled(); + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining('needs an interactive terminal'), + ); + expect(process.exitCode).toBe(1); + }); + + // Redirected stdout hides everything inquirer renders even with a TTY + // stdin, so it must be rejected the same way as a non-TTY stdin. + it('fails loudly on a redirected-stdout create instead of hanging on the first prompt', async () => { + process.stdout.isTTY = false; + const { select } = await import('@inquirer/prompts'); + const { createSource } = await import('@/libs/sources.js'); + const { runSourcesCommand } = await import('@/commands/sources.js'); + + await runSourcesCommand(['create']); + + expect(select).not.toHaveBeenCalled(); + expect(createSource).not.toHaveBeenCalled(); + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining('needs an interactive terminal'), + ); + expect(process.exitCode).toBe(1); + }); }); describe('update', () => { @@ -718,6 +756,65 @@ describe('runSourcesCommand', () => { expect(console.error).toHaveBeenCalledWith('Failed to update source.'); }); + + // Without a uuid, `update` opens the interactive picker; on a non-TTY + // stdin that picker can't render an answerable prompt, so it must fail + // loud instead of hanging — same guard shape as delete's non-TTY cases. + it('fails loudly on a non-TTY stdin update with no uuid instead of opening the picker', async () => { + process.stdin.isTTY = false; + const { fetchSources, updateSource } = await import('@/libs/sources.js'); + const { select } = await import('@inquirer/prompts'); + const { runSourcesCommand } = await import('@/commands/sources.js'); + + await runSourcesCommand(['update']); + + expect(fetchSources).not.toHaveBeenCalled(); + expect(select).not.toHaveBeenCalled(); + expect(updateSource).not.toHaveBeenCalled(); + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining('needs an interactive terminal'), + ); + expect(process.exitCode).toBe(1); + }); + + // Redirected stdout hides the picker even with a TTY stdin, so a bare + // `update` piped to a file must be rejected the same way. + it('fails loudly on a redirected-stdout update with no uuid instead of opening the picker', async () => { + process.stdout.isTTY = false; + const { fetchSources, updateSource } = await import('@/libs/sources.js'); + const { select } = await import('@inquirer/prompts'); + const { runSourcesCommand } = await import('@/commands/sources.js'); + + await runSourcesCommand(['update']); + + expect(fetchSources).not.toHaveBeenCalled(); + expect(select).not.toHaveBeenCalled(); + expect(updateSource).not.toHaveBeenCalled(); + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining('needs an interactive terminal'), + ); + expect(process.exitCode).toBe(1); + }); + + // Even with an explicit uuid, `update` still ends by prompting for the + // route folder — the same unanswerable-prompt hang, just reached via the + // direct-uuid path instead of the picker. + it('fails loudly on a non-TTY update with an explicit uuid instead of hanging on the route-folder prompt', async () => { + process.stdin.isTTY = false; + const { fetchSources, updateSource } = await import('@/libs/sources.js'); + const { input } = await import('@inquirer/prompts'); + const { runSourcesCommand } = await import('@/commands/sources.js'); + + await runSourcesCommand(['update', 'abc-123']); + + expect(fetchSources).not.toHaveBeenCalled(); + expect(input).not.toHaveBeenCalled(); + expect(updateSource).not.toHaveBeenCalled(); + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining('needs an interactive terminal'), + ); + expect(process.exitCode).toBe(1); + }); }); describe('delete', () => { @@ -1095,9 +1192,8 @@ describe('runSourcesCommand', () => { describe('rotate-secret', () => { it('rotates by uuid for a generated provider and reveals the new secret once', async () => { - const { fetchSources, rotateSourceSecret } = await import( - '@/libs/sources.js' - ); + const { fetchSources, rotateSourceSecret } = + await import('@/libs/sources.js'); vi.mocked(fetchSources).mockResolvedValue([githubSource]); vi.mocked(rotateSourceSecret).mockResolvedValue({ ...githubSource, @@ -1123,9 +1219,8 @@ describe('runSourcesCommand', () => { }); it('prompts (masked) for the new secret and sends it for a manual-secret provider', async () => { - const { fetchSources, rotateSourceSecret } = await import( - '@/libs/sources.js' - ); + const { fetchSources, rotateSourceSecret } = + await import('@/libs/sources.js'); const { password } = await import('@inquirer/prompts'); vi.mocked(fetchSources).mockResolvedValue([stripeSource]); vi.mocked(password).mockResolvedValueOnce('whsec_pasted_stripe'); @@ -1159,9 +1254,8 @@ describe('runSourcesCommand', () => { }); it('does not raise the missing-secret alarm for a manual provider (its response has none)', async () => { - const { fetchSources, rotateSourceSecret } = await import( - '@/libs/sources.js' - ); + const { fetchSources, rotateSourceSecret } = + await import('@/libs/sources.js'); const { password } = await import('@inquirer/prompts'); vi.mocked(fetchSources).mockResolvedValue([stripeSource]); vi.mocked(password).mockResolvedValueOnce('whsec_pasted_stripe'); @@ -1183,9 +1277,8 @@ describe('runSourcesCommand', () => { }); it('aborts without calling the API when a manual secret is left blank', async () => { - const { fetchSources, rotateSourceSecret } = await import( - '@/libs/sources.js' - ); + const { fetchSources, rotateSourceSecret } = + await import('@/libs/sources.js'); const { password } = await import('@inquirer/prompts'); vi.mocked(fetchSources).mockResolvedValue([stripeSource]); vi.mocked(password).mockResolvedValueOnce(' '); @@ -1200,9 +1293,8 @@ describe('runSourcesCommand', () => { }); it('refuses a source with no rotatable secret and skips the API call', async () => { - const { fetchSources, rotateSourceSecret } = await import( - '@/libs/sources.js' - ); + const { fetchSources, rotateSourceSecret } = + await import('@/libs/sources.js'); vi.mocked(fetchSources).mockResolvedValue([webhookSource]); const { runSourcesCommand } = await import('@/commands/sources.js'); @@ -1215,9 +1307,8 @@ describe('runSourcesCommand', () => { }); it('reports not-found when the uuid does not match any source', async () => { - const { fetchSources, rotateSourceSecret } = await import( - '@/libs/sources.js' - ); + const { fetchSources, rotateSourceSecret } = + await import('@/libs/sources.js'); vi.mocked(fetchSources).mockResolvedValue([githubSource]); const { runSourcesCommand } = await import('@/commands/sources.js'); @@ -1230,9 +1321,8 @@ describe('runSourcesCommand', () => { }); it('offers only rotatable sources in the interactive picker', async () => { - const { fetchSources, rotateSourceSecret } = await import( - '@/libs/sources.js' - ); + const { fetchSources, rotateSourceSecret } = + await import('@/libs/sources.js'); const { select } = await import('@inquirer/prompts'); vi.mocked(fetchSources).mockResolvedValue([ webhookSource, @@ -1250,18 +1340,15 @@ describe('runSourcesCommand', () => { expect(select).toHaveBeenCalledWith( expect.objectContaining({ - choices: [ - expect.objectContaining({ value: 'ghi-789' }), - ], + choices: [expect.objectContaining({ value: 'ghi-789' })], }), ); expect(rotateSourceSecret).toHaveBeenCalledWith('ghi-789', {}); }); it('explains rotate-secret needs a provider source when only non-rotatable sources exist', async () => { - const { fetchSources, rotateSourceSecret } = await import( - '@/libs/sources.js' - ); + const { fetchSources, rotateSourceSecret } = + await import('@/libs/sources.js'); vi.mocked(fetchSources).mockResolvedValue([webhookSource, emailSource]); const { runSourcesCommand } = await import('@/commands/sources.js'); @@ -1288,9 +1375,8 @@ describe('runSourcesCommand', () => { }); it('warns when a generated rotation succeeds but the response omits the secret', async () => { - const { fetchSources, rotateSourceSecret } = await import( - '@/libs/sources.js' - ); + const { fetchSources, rotateSourceSecret } = + await import('@/libs/sources.js'); vi.mocked(fetchSources).mockResolvedValue([githubSource]); // Server rotated the secret (old one now dead) but returned no plaintext. vi.mocked(rotateSourceSecret).mockResolvedValue({ @@ -1308,9 +1394,8 @@ describe('runSourcesCommand', () => { }); it('reports an error when the rotation fails', async () => { - const { fetchSources, rotateSourceSecret } = await import( - '@/libs/sources.js' - ); + const { fetchSources, rotateSourceSecret } = + await import('@/libs/sources.js'); vi.mocked(fetchSources).mockResolvedValue([githubSource]); vi.mocked(rotateSourceSecret).mockResolvedValue(null); const { runSourcesCommand } = await import('@/commands/sources.js'); @@ -1327,9 +1412,8 @@ describe('runSourcesCommand', () => { it('strips control characters from a hostile rotated secret before printing', async () => { const control = String.fromCharCode(0x1b); - const { fetchSources, rotateSourceSecret } = await import( - '@/libs/sources.js' - ); + const { fetchSources, rotateSourceSecret } = + await import('@/libs/sources.js'); vi.mocked(fetchSources).mockResolvedValue([githubSource]); vi.mocked(rotateSourceSecret).mockResolvedValue({ ...githubSource, @@ -1354,9 +1438,8 @@ describe('runSourcesCommand', () => { // --json before doing anything — a `| jq` pipeline would lose the secret. it('rejects --json on rotate-secret before prompting or calling the API', async () => { const { checkConfig } = await import('@/libs/config.js'); - const { fetchSources, rotateSourceSecret } = await import( - '@/libs/sources.js' - ); + const { fetchSources, rotateSourceSecret } = + await import('@/libs/sources.js'); const { runSourcesCommand } = await import('@/commands/sources.js'); await runSourcesCommand(['rotate-secret', '--json']); From 042ee9e30740d27c9f3bc87ce1ac83044bc08abd Mon Sep 17 00:00:00 2001 From: Danny Holloran Date: Sun, 6 Sep 2026 17:24:31 -0700 Subject: [PATCH 2/2] Address code review: pin distinguishing guard messages, cover rotate-secret gap in tests - Assert the distinguishing half of each new TTY-guard message (create/update) instead of the shared generic substring, so a reordered branch can't slip past the tests with misleading advice. - Interpolate CREATE_SUBCOMMAND/UPDATE_SUBCOMMAND in their own messages, matching the delete message's existing pattern. - Add a regression test pinning that 'list' still works over a redirected/non-TTY stream (the primary scripted use case). - Add a regression test documenting that rotate-secret is deliberately left unguarded for now (tracked via @todo + follow-up suggestion), so a future change can't silently widen or narrow that gap without a test noticing. - Note the rotate-secret gap in the README so scripters don't assume it's covered by this change. --- README.md | 62 +++++++++++++++++----------------- src/commands/sources.ts | 34 +++++++++++-------- tests/commands/sources.test.ts | 54 +++++++++++++++++++++++++---- 3 files changed, 97 insertions(+), 53 deletions(-) diff --git a/README.md b/README.md index 620f19c..23c5fb0 100644 --- a/README.md +++ b/README.md @@ -17,16 +17,16 @@ every command. A bare `markpost` with no arguments prints that help and exits non-zero — it does **not** sync, so an accidental invocation can't delete server-side records. -| Command | Description | -|---|---| -| `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 [--dry-run] ` | Create records from one or more markdown files, directories, or glob patterns. `--dry-run` reports which files would be pushed, plus any missing or unreadable inputs, without creating any records | -| `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) and needs an interactive terminal; in scripts pass a uuid with `--yes` (`sources delete --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 ] [--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`) | -| `markpost help` | Show aggregated usage | +| Command | Description | +| ---------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `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 [--dry-run] ` | Create records from one or more markdown files, directories, or glob patterns. `--dry-run` reports which files would be pushed, plus any missing or unreadable inputs, without creating any records | +| `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) and needs an interactive terminal; in scripts pass a uuid with `--yes` (`sources delete --yes`) to skip the prompt. `sources create` and `sources update` also need an interactive terminal — they always prompt (for source details, or the route folder) and have no `--yes` equivalent, so they exit with an error rather than hang under a pipe or cron job. `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); it isn't guarded yet — it can still hang waiting on a prompt (a picker with no uuid, or the stripe secret prompt), so run it interactively | +| `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`) | +| `markpost help` | Show aggregated usage | The destructive fetch/write/delete sync runs only under the explicit `markpost sync` command. @@ -45,11 +45,11 @@ object to **stderr**: `error` is one of a small, stable set of machine-readable codes: -| `error` code | When it happens | -|-------------------|---------------------------------------------------------------------------------| +| `error` code | When it happens | +| ----------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------ | | `config_required` | A required value (API token or output directory) is not configured and `--json` mode will not prompt. Also includes a `missing` field naming the config key. | -| `usage` | A bad or missing argument/subcommand, or `--json` passed where it is not supported. | -| `fetch_failed` | The requested operation could not be completed (a failed or empty fetch, or an error thrown while carrying it out — e.g. an auth/5xx failure). | +| `usage` | A bad or missing argument/subcommand, or `--json` passed where it is not supported. | +| `fetch_failed` | The requested operation could not be completed (a failed or empty fetch, or an error thrown while carrying it out — e.g. an auth/5xx failure). | Any string in `message` that is server-derived is sanitized so it cannot inject a live terminal escape sequence. Additional fields (such as `missing`) may @@ -148,24 +148,24 @@ npm install Copy [`.envrc`](.envrc) and populate your values. If you use [direnv](https://direnv.net/), run `direnv allow` to load them automatically. -| Variable | Description | -|---|---| -| `API_TOKEN` | API token for sync.danholloran.me | -| `BASE_URL` | Base URL of the sync API (e.g. `http://localhost:8888` for local dev) | +| Variable | Description | +| ------------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `API_TOKEN` | API token for sync.danholloran.me | +| `BASE_URL` | Base URL of the sync API (e.g. `http://localhost:8888` for local dev) | | `OUTPUT_DIRECTORY` | Path to the directory where synced files are written; a leading `~`, `$HOME`, or `${HOME}` is expanded to your home directory. A relative path is resolved against the current working directory, so prefer an absolute path or a `~` prefix for scheduled runs | ### Scripts -| Command | Description | -| ------------------ | -------------------------------------- | -| `npm run build` | Compile TypeScript to `dist/` | -| `npm run watch` | Watch and recompile on changes | -| `npm test` | Run tests with Vitest | -| `npm run test:ci` | Run tests once (CI mode) | -| `npm run test:ui` | Run tests with Vitest UI | -| `npm run lint` | Check formatting and linting | -| `npm run lint:fix` | Auto-fix formatting and linting issues | -| `npm run sync:contract` | Refresh the vendored markpost API contract (see below) | +| Command | Description | +| ------------------------------------- | ------------------------------------------------------------- | +| `npm run build` | Compile TypeScript to `dist/` | +| `npm run watch` | Watch and recompile on changes | +| `npm test` | Run tests with Vitest | +| `npm run test:ci` | Run tests once (CI mode) | +| `npm run test:ui` | Run tests with Vitest UI | +| `npm run lint` | Check formatting and linting | +| `npm run lint:fix` | Auto-fix formatting and linting issues | +| `npm run sync:contract` | Refresh the vendored markpost API contract (see below) | | `npm run sync:markdown-serialization` | Refresh the vendored markpost serialization slice (see below) | ### Contract sync @@ -201,7 +201,7 @@ re-exports the generic envelope types (`ApiError`, `ApiRequest`, run: npx vitest run tests/types/contract-drift.test.ts ``` after your existing install step. -- **What this does *not* do:** it does not detect when markpost's *real* +- **What this does _not_ do:** it does not detect when markpost's _real_ upstream contract has changed and the vendored copy has fallen behind — that would require network access at test time (flaky, and fails offline CI). Re-run `npm run sync:contract` periodically or whenever a markpost API @@ -230,14 +230,14 @@ test failing. This closes that gap the same way the contract sync does. it never ships in the published `dist/`. Review the diff, run `npm test`, then commit. - **Catching drift:** `tests/libs/frontmatter-drift.test.ts` runs on every - `npm test` / `npm run test:ci`. It executes markpost's *real* (vendored) + `npm test` / `npm run test:ci`. It executes markpost's _real_ (vendored) serialization functions and the CLI's mirrored ones over a shared battery of inputs — plain values, empty and multi-tag lists, every YAML metacharacter, whitespace, and escape sequences — and fails if any input serializes differently. No network access needed. When markpost's serialization changes, re-run the sync: the vendored slice updates, and if the CLI mirror has not been updated to match, this test goes red. -- **What this does *not* do:** it does not detect when markpost's upstream +- **What this does _not_ do:** it does not detect when markpost's upstream serialization has changed and the vendored slice has fallen behind — that would require network access at test time. Re-run `npm run sync:markdown-serialization` whenever a markpost markdown change is diff --git a/src/commands/sources.ts b/src/commands/sources.ts index 792a535..be854eb 100644 --- a/src/commands/sources.ts +++ b/src/commands/sources.ts @@ -60,6 +60,7 @@ const UPDATE_SUBCOMMAND = 'update'; // `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 ROTATE_SECRET_SUBCOMMAND = 'rotate-secret'; const SOURCES_HANDLERS = new Map< string, @@ -76,22 +77,25 @@ const SOURCES_HANDLERS = new Map< DELETE_SUBCOMMAND, (uuid, _json, skipConfirm) => deleteSourceCommand(uuid, skipConfirm), ], - ['rotate-secret', (uuid) => rotateSecretCommand(uuid)], + [ROTATE_SECRET_SUBCOMMAND, (uuid) => rotateSecretCommand(uuid)], ]); -// The message for the one subcommand (if any) that `subcommand` can't -// complete without an interactive terminal. Every one of these ends by -// rendering an inquirer prompt (a confirmation, a picker, or a text/select -// input), and inquirer needs both stdin and stdout to be a TTY to render and -// read one — a redirected/non-interactive run would otherwise hang, or (for -// delete) abort via the swallowed-Ctrl+C path below yet still exit 0. `delete` -// is only guarded here when `--yes` is absent (that flag is its documented -// escape hatch); `create` and `update` have no such flag — `create` always -// prompts, and `update` always ends by prompting for the route folder, +// The message for the subcommand (if any) that can't complete without an +// interactive terminal — inquirer needs both stdin and stdout to be a TTY to +// render and read a prompt, so a redirected/non-interactive run would +// otherwise hang, or (for delete) abort via the swallowed-Ctrl+C path below +// yet still exit 0. `delete` is only guarded when `--yes` is absent (its +// documented escape hatch); `create` and `update` have no such flag — `create` +// always prompts, and `update` always ends by prompting for the route folder, // whether the target came from an explicit uuid or the interactive picker — -// so both are guarded outright. Kept as one lookup (not three near-identical -// `!isInteractive` checks in `usageErrorFor`) so the guard condition itself -// stays in a single place. +// so both are guarded outright. `rotate-secret` also prompts (a picker with no +// uuid, or a password input for a manual-secret provider) but is deliberately +// left out here: it's out of scope for this change, same as create/update +// were out of scope for delete's original guard. See the `rotate-secret` +// non-TTY test below for what this currently leaves unguarded. +// @todo Guard `rotate-secret` the same way (picker needs a uuid; a +// manual-secret provider's password prompt needs a TTY check inside +// collectRotateInput, since the provider isn't known until after fetchSources). const interactiveGuardMessageFor = ( subcommand: string, skipConfirm: boolean, @@ -101,11 +105,11 @@ const interactiveGuardMessageFor = ( } if (subcommand === CREATE_SUBCOMMAND) { - return `\`sources create\` needs an interactive terminal — it always prompts for the source details.`; + return `\`sources ${CREATE_SUBCOMMAND}\` needs an interactive terminal — it always prompts for the source details.`; } if (subcommand === UPDATE_SUBCOMMAND) { - return `\`sources update\` needs an interactive terminal — it prompts for the route folder, and to pick a source when no uuid is given.`; + return `\`sources ${UPDATE_SUBCOMMAND}\` needs an interactive terminal — it prompts for the route folder, and to pick a source when no uuid is given.`; } return null; diff --git a/tests/commands/sources.test.ts b/tests/commands/sources.test.ts index 0da8cc4..8815a35 100644 --- a/tests/commands/sources.test.ts +++ b/tests/commands/sources.test.ts @@ -427,6 +427,22 @@ describe('runSourcesCommand', () => { expect.stringContaining('never hit'), ); }); + + // `list` never prompts, so it must stay usable on a non-TTY — this is the + // primary scripted path (`sources list --json > file`) the new create/ + // update guard must not sweep in alongside them. + it('still lists on a non-TTY (neither stdin nor stdout is a terminal)', async () => { + process.stdin.isTTY = false; + process.stdout.isTTY = false; + const { fetchSources } = await import('@/libs/sources.js'); + vi.mocked(fetchSources).mockResolvedValue([webhookSource]); + const { runSourcesCommand } = await import('@/commands/sources.js'); + + await runSourcesCommand(['list', '--json']); + + expect(fetchSources).toHaveBeenCalled(); + expect(process.exitCode).toBeUndefined(); + }); }); describe('create', () => { @@ -581,7 +597,7 @@ describe('runSourcesCommand', () => { expect(select).not.toHaveBeenCalled(); expect(createSource).not.toHaveBeenCalled(); expect(console.error).toHaveBeenCalledWith( - expect.stringContaining('needs an interactive terminal'), + expect.stringContaining('always prompts for the source details'), ); expect(process.exitCode).toBe(1); }); @@ -599,7 +615,7 @@ describe('runSourcesCommand', () => { expect(select).not.toHaveBeenCalled(); expect(createSource).not.toHaveBeenCalled(); expect(console.error).toHaveBeenCalledWith( - expect.stringContaining('needs an interactive terminal'), + expect.stringContaining('always prompts for the source details'), ); expect(process.exitCode).toBe(1); }); @@ -772,7 +788,7 @@ describe('runSourcesCommand', () => { expect(select).not.toHaveBeenCalled(); expect(updateSource).not.toHaveBeenCalled(); expect(console.error).toHaveBeenCalledWith( - expect.stringContaining('needs an interactive terminal'), + expect.stringContaining('prompts for the route folder'), ); expect(process.exitCode).toBe(1); }); @@ -791,7 +807,7 @@ describe('runSourcesCommand', () => { expect(select).not.toHaveBeenCalled(); expect(updateSource).not.toHaveBeenCalled(); expect(console.error).toHaveBeenCalledWith( - expect.stringContaining('needs an interactive terminal'), + expect.stringContaining('prompts for the route folder'), ); expect(process.exitCode).toBe(1); }); @@ -811,7 +827,7 @@ describe('runSourcesCommand', () => { expect(input).not.toHaveBeenCalled(); expect(updateSource).not.toHaveBeenCalled(); expect(console.error).toHaveBeenCalledWith( - expect.stringContaining('needs an interactive terminal'), + expect.stringContaining('prompts for the route folder'), ); expect(process.exitCode).toBe(1); }); @@ -1122,7 +1138,7 @@ describe('runSourcesCommand', () => { expect(confirm).not.toHaveBeenCalled(); expect(deleteSource).not.toHaveBeenCalled(); expect(console.error).toHaveBeenCalledWith( - expect.stringContaining('needs an interactive terminal'), + expect.stringContaining('--yes'), ); expect(process.exitCode).toBe(1); }); @@ -1141,7 +1157,7 @@ describe('runSourcesCommand', () => { expect(confirm).not.toHaveBeenCalled(); expect(deleteSource).not.toHaveBeenCalled(); expect(console.error).toHaveBeenCalledWith( - expect.stringContaining('needs an interactive terminal'), + expect.stringContaining('--yes'), ); expect(process.exitCode).toBe(1); }); @@ -1218,6 +1234,30 @@ describe('runSourcesCommand', () => { expect(secretMentions).toHaveLength(1); }); + // `rotate-secret` is deliberately left out of the create/update/delete TTY + // guard added for #148 (see the `@todo` in sources.ts) — a uuid'd rotation + // of a generated provider needs no prompt at all, so it must keep working + // on a non-TTY. This pins that the broader `!isInteractive` check doesn't + // sweep rotate-secret in by accident; it is not an endorsement of piping + // this (the secret still lands in whatever stdout is redirected to). + it('still rotates by uuid on a non-TTY (deliberately unguarded for now)', async () => { + process.stdin.isTTY = false; + process.stdout.isTTY = false; + const { fetchSources, rotateSourceSecret } = + await import('@/libs/sources.js'); + vi.mocked(fetchSources).mockResolvedValue([githubSource]); + vi.mocked(rotateSourceSecret).mockResolvedValue({ + ...githubSource, + providerSecret: 'whsec_rotated_value', + }); + const { runSourcesCommand } = await import('@/commands/sources.js'); + + await runSourcesCommand(['rotate-secret', 'ghi-789']); + + expect(rotateSourceSecret).toHaveBeenCalledWith('ghi-789', {}); + expect(process.exitCode).toBeUndefined(); + }); + it('prompts (masked) for the new secret and sends it for a manual-secret provider', async () => { const { fetchSources, rotateSourceSecret } = await import('@/libs/sources.js');