From d28ac58ba967226d36d3ae85e66e002bb900c190 Mon Sep 17 00:00:00 2001 From: Danny Holloran Date: Thu, 27 Aug 2026 11:54:48 -0500 Subject: [PATCH 1/5] Add markpost push --dry-run to preview files before creating records --- src/commands/push.ts | 54 ++++++++++++++++-- tests/commands/push.test.ts | 106 ++++++++++++++++++++++++++++++++++++ 2 files changed, 155 insertions(+), 5 deletions(-) diff --git a/src/commands/push.ts b/src/commands/push.ts index 0b256e1..3736542 100644 --- a/src/commands/push.ts +++ b/src/commands/push.ts @@ -11,10 +11,17 @@ import { resolveMarkdownInputs } from '@/libs/files.js'; import { checkConfig } from '@/libs/config.js'; import { failWithUsage } from '@/libs/usage.js'; -export const USAGE = `Usage: markpost push +export const USAGE = `Usage: markpost push [--dry-run] - path One or more markdown files, directories (recursed for .md files), - or glob patterns to create records from`; + path One or more markdown files, directories (recursed for .md files), + or glob patterns to create records from + --dry-run Preview the resolved files (and any missing/unreadable inputs) + without creating any records`; + +// Preview flag: resolve the inputs and print the exact push plan without +// creating any server records. Named so the parse check and usage text share +// the one literal, mirroring sync's --dry-run. +const DRY_RUN_FLAG = '--dry-run'; // `systemicError` is set only when this file failed for a reason that will // recur for every other file (auth/5xx). Carrying it on the result rather than @@ -116,6 +123,25 @@ const abortLine = (abort: PushAbort): string => { return `${head} ${abort.unattempted} file(s) not attempted.`; }; +// The push dry run mirrors sync's --dry-run output style (a yellow preview +// header over a dim "would" list). It can't reuse sync's reportDryRunPlan: +// that previews server records being written to local files, whereas push +// previews local files being sent to the server — disjoint inputs, so only the +// visual style is shared, not the code. File paths are the user's own local +// inputs (not untrusted API data), so they're printed unsanitized like the rest +// of push's file output. +const reportPushDryRun = (filePaths: string[]): void => { + console.log( + chalk.yellow( + `Dry run — previewing ${filePaths.length} file(s); nothing will be pushed.`, + ), + ); + console.log(chalk.dim(`Would push ${filePaths.length} file(s):`)); + filePaths.forEach((filePath) => { + console.log(chalk.dim(` -> ${filePath}`)); + }); +}; + const reportSummary = (run: PushRun, unresolvedCount: number): void => { const { results, abort, total } = run; const succeeded = results.filter((result) => result.pushed).length; @@ -137,7 +163,8 @@ const reportSummary = (run: PushRun, unresolvedCount: number): void => { export const runPushCommand = async (args: string[]): Promise => { try { - const paths = args.filter((arg) => arg.length > 0); + const dryRun = args.includes(DRY_RUN_FLAG); + const paths = args.filter((arg) => arg.length > 0 && arg !== DRY_RUN_FLAG); if (paths.length === 0) { failWithUsage('No path given.', USAGE); @@ -164,8 +191,25 @@ export const runPushCommand = async (args: string[]): Promise => { return; } + const unresolvedCount = missing.length + skipped.length; + + // A dry run stops before any createRecord call: report the resolved plan, + // then mirror the real run's honesty by exiting non-zero when inputs went + // unresolved (a wrong glob, an unreadable path) — the exact mistake the + // preview exists to catch — minus the per-file push failures a dry run + // can't produce. + if (dryRun) { + reportPushDryRun(files); + + if (unresolvedCount > 0) { + process.exitCode = 1; + } + + return; + } + const run = await pushFiles(files); - reportSummary(run, missing.length + skipped.length); + reportSummary(run, unresolvedCount); } catch (error) { console.error(chalk.redBright(toMessage(error))); process.exitCode = 1; diff --git a/tests/commands/push.test.ts b/tests/commands/push.test.ts index 5b920f8..bb16caa 100644 --- a/tests/commands/push.test.ts +++ b/tests/commands/push.test.ts @@ -13,6 +13,7 @@ vi.mock('chalk', () => ({ redBright: vi.fn((value: unknown) => value), greenBright: vi.fn((value: unknown) => value), dim: vi.fn((value: unknown) => value), + yellow: vi.fn((value: unknown) => value), }, })); @@ -576,6 +577,111 @@ describe('runPushCommand', () => { expect(process.exitCode).toBe(1); }); + it('previews the resolved files without creating any records on --dry-run', async () => { + const { createRecord } = await import('@/libs/records.js'); + const { readMarkdown } = await import('@/libs/markdown.js'); + const { resolveMarkdownInputs } = await import('@/libs/files.js'); + vi.mocked(resolveMarkdownInputs).mockReturnValue({ + files: ['a.md', 'b.md'], + missing: [], + skipped: [], + }); + const { runPushCommand } = await import('@/commands/push.js'); + + await runPushCommand(['a.md', 'b.md', '--dry-run']); + + // The whole point: no record-creation path is touched on a dry run. + expect(createRecord).not.toHaveBeenCalled(); + expect(readMarkdown).not.toHaveBeenCalled(); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining( + 'Dry run — previewing 2 file(s); nothing will be pushed.', + ), + ); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('Would push 2 file(s):'), + ); + expect(console.log).toHaveBeenCalledWith(expect.stringContaining('a.md')); + expect(console.log).toHaveBeenCalledWith(expect.stringContaining('b.md')); + expect(process.exitCode).toBeUndefined(); + }); + + it('excludes the --dry-run flag from the resolved input paths', async () => { + const { resolveMarkdownInputs } = await import('@/libs/files.js'); + vi.mocked(resolveMarkdownInputs).mockReturnValue({ + files: ['a.md'], + missing: [], + skipped: [], + }); + const { runPushCommand } = await import('@/commands/push.js'); + + await runPushCommand(['--dry-run', 'a.md']); + + // The flag must never reach resolveMarkdownInputs as if it were a path. + expect(resolveMarkdownInputs).toHaveBeenCalledWith(['a.md']); + }); + + it('fails with usage when --dry-run is the only argument', async () => { + const { checkConfig } = await import('@/libs/config.js'); + const { resolveMarkdownInputs } = await import('@/libs/files.js'); + const { runPushCommand } = await import('@/commands/push.js'); + + await runPushCommand(['--dry-run']); + + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining('No path given.'), + ); + expect(checkConfig).not.toHaveBeenCalled(); + expect(resolveMarkdownInputs).not.toHaveBeenCalled(); + expect(process.exitCode).toBe(1); + }); + + it('reports missing inputs and exits 1 on a dry run without pushing', async () => { + const { createRecord } = await import('@/libs/records.js'); + const { resolveMarkdownInputs } = await import('@/libs/files.js'); + vi.mocked(resolveMarkdownInputs).mockReturnValue({ + files: ['real.md'], + missing: ['ghost.md'], + skipped: [], + }); + const { runPushCommand } = await import('@/commands/push.js'); + + await runPushCommand(['real.md', 'ghost.md', '--dry-run']); + + expect(createRecord).not.toHaveBeenCalled(); + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining('No markdown files found for "ghost.md".'), + ); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('Would push 1 file(s):'), + ); + // A wrong glob is exactly what --dry-run exists to catch, so it stays honest + // and exits non-zero like the real run would. + expect(process.exitCode).toBe(1); + }); + + it('exits 1 on a dry run when no inputs resolve to any file', async () => { + const { createRecord } = await import('@/libs/records.js'); + const { resolveMarkdownInputs } = await import('@/libs/files.js'); + vi.mocked(resolveMarkdownInputs).mockReturnValue({ + files: [], + missing: ['./missing/*.md'], + skipped: [], + }); + const { runPushCommand } = await import('@/commands/push.js'); + + await runPushCommand(['./missing/*.md', '--dry-run']); + + expect(createRecord).not.toHaveBeenCalled(); + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining('No markdown files to push.'), + ); + expect(console.log).not.toHaveBeenCalledWith( + expect.stringContaining('Would push'), + ); + expect(process.exitCode).toBe(1); + }); + it('catches and logs an unexpected error from config setup', async () => { const { checkConfig } = await import('@/libs/config.js'); vi.mocked(checkConfig).mockRejectedValue(Error('config blew up')); From e11ee892a77f88b45a6e134cdfd752afc24172b4 Mon Sep 17 00:00:00 2001 From: Danny Holloran Date: Thu, 27 Aug 2026 11:58:09 -0500 Subject: [PATCH 2/5] Address review: reject typo'd flags, skip config on dry run, README --- README.md | 2 +- src/commands/push.ts | 27 ++++++++++++++++++++++++- tests/commands/push.test.ts | 39 +++++++++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 341274d..dc9f420 100644 --- a/README.md +++ b/README.md @@ -20,7 +20,7 @@ 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 ` | Create records from one or more markdown files, directories, or glob patterns | +| `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]` | Manage sources; `sources list --json` prints machine-readable output | | `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 | diff --git a/src/commands/push.ts b/src/commands/push.ts index 3736542..e17517d 100644 --- a/src/commands/push.ts +++ b/src/commands/push.ts @@ -23,6 +23,10 @@ export const USAGE = `Usage: markpost push [--dry-run] // the one literal, mirroring sync's --dry-run. const DRY_RUN_FLAG = '--dry-run'; +// Any leading-dash token is a flag, not a path — used to reject a stray or +// mistyped flag before it's mistaken for a file to push. +const FLAG_PREFIX = '-'; + // `systemicError` is set only when this file failed for a reason that will // recur for every other file (auth/5xx). Carrying it on the result rather than // throwing lets `pushFiles` decide to abort with a flat check instead of a @@ -164,6 +168,24 @@ const reportSummary = (run: PushRun, unresolvedCount: number): void => { export const runPushCommand = async (args: string[]): Promise => { try { const dryRun = args.includes(DRY_RUN_FLAG); + + // Reject a stray or mistyped flag (`--dryrun`, `--dry-run=1`) rather than + // letting it fall through as a bogus path: push creates server records, so a + // fat-fingered --dry-run must fail loud, never silently run the real push + // when the user asked for a preview — the guard sync gives its one + // destructive command. + const unexpectedFlags = args.filter( + (arg) => arg.startsWith(FLAG_PREFIX) && arg !== DRY_RUN_FLAG, + ); + + if (unexpectedFlags.length > 0) { + failWithUsage( + `Unexpected arguments: ${unexpectedFlags.join(' ')}`, + USAGE, + ); + return; + } + const paths = args.filter((arg) => arg.length > 0 && arg !== DRY_RUN_FLAG); if (paths.length === 0) { @@ -171,7 +193,10 @@ export const runPushCommand = async (args: string[]): Promise => { return; } - if (!(await checkConfig())) { + // A dry run makes no network calls, so it doesn't need a configured token or + // output directory — let a user confirm which files a glob matches before + // setting up auth. The real push still requires config. + if (!dryRun && !(await checkConfig())) { return; } diff --git a/tests/commands/push.test.ts b/tests/commands/push.test.ts index bb16caa..1a90199 100644 --- a/tests/commands/push.test.ts +++ b/tests/commands/push.test.ts @@ -606,6 +606,45 @@ describe('runPushCommand', () => { expect(process.exitCode).toBeUndefined(); }); + // A mistyped --dry-run must fail loud, never fall through to the real push and + // create records the user only meant to preview. + it('rejects a mistyped dry-run flag without resolving inputs or pushing', async () => { + const { checkConfig } = await import('@/libs/config.js'); + const { createRecord } = await import('@/libs/records.js'); + const { resolveMarkdownInputs } = await import('@/libs/files.js'); + const { runPushCommand } = await import('@/commands/push.js'); + + await runPushCommand(['a.md', '--dryrun']); + + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining('Unexpected arguments: --dryrun'), + ); + expect(checkConfig).not.toHaveBeenCalled(); + expect(resolveMarkdownInputs).not.toHaveBeenCalled(); + expect(createRecord).not.toHaveBeenCalled(); + expect(process.exitCode).toBe(1); + }); + + // A dry run makes no network calls, so it must not gate on a configured token. + it('does not check config on a dry run', async () => { + const { checkConfig } = await import('@/libs/config.js'); + const { resolveMarkdownInputs } = await import('@/libs/files.js'); + vi.mocked(resolveMarkdownInputs).mockReturnValue({ + files: ['a.md'], + missing: [], + skipped: [], + }); + const { runPushCommand } = await import('@/commands/push.js'); + + await runPushCommand(['a.md', '--dry-run']); + + expect(checkConfig).not.toHaveBeenCalled(); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('Would push 1 file(s):'), + ); + expect(process.exitCode).toBeUndefined(); + }); + it('excludes the --dry-run flag from the resolved input paths', async () => { const { resolveMarkdownInputs } = await import('@/libs/files.js'); vi.mocked(resolveMarkdownInputs).mockReturnValue({ From 5c5d9b6d9462f97387d51eb8f63f1fedc8242667 Mon Sep 17 00:00:00 2001 From: Danny Holloran Date: Thu, 27 Aug 2026 12:04:08 -0500 Subject: [PATCH 3/5] Address review round 2: -- flag prefix, sanitize preview, exit-code in reporter, skipped test --- src/commands/push.ts | 53 ++++++++++++++++++++++--------------- tests/commands/push.test.ts | 22 +++++++++++++++ 2 files changed, 53 insertions(+), 22 deletions(-) diff --git a/src/commands/push.ts b/src/commands/push.ts index e17517d..79330fa 100644 --- a/src/commands/push.ts +++ b/src/commands/push.ts @@ -10,6 +10,7 @@ import { readMarkdown } from '@/libs/markdown.js'; import { resolveMarkdownInputs } from '@/libs/files.js'; import { checkConfig } from '@/libs/config.js'; import { failWithUsage } from '@/libs/usage.js'; +import { sanitizeForTerminal } from '@/libs/terminal.js'; export const USAGE = `Usage: markpost push [--dry-run] @@ -23,9 +24,10 @@ export const USAGE = `Usage: markpost push [--dry-run] // the one literal, mirroring sync's --dry-run. const DRY_RUN_FLAG = '--dry-run'; -// Any leading-dash token is a flag, not a path — used to reject a stray or -// mistyped flag before it's mistaken for a file to push. -const FLAG_PREFIX = '-'; +// A leading `--` marks a long flag, not a path — used to reject a stray or +// mistyped long flag before it's mistaken for a file to push. Kept to `--` (not +// a bare `-`) so a legitimate dash-leading filename from a glob still pushes. +const FLAG_PREFIX = '--'; // `systemicError` is set only when this file failed for a reason that will // recur for every other file (auth/5xx). Carrying it on the result rather than @@ -128,13 +130,19 @@ const abortLine = (abort: PushAbort): string => { }; // The push dry run mirrors sync's --dry-run output style (a yellow preview -// header over a dim "would" list). It can't reuse sync's reportDryRunPlan: -// that previews server records being written to local files, whereas push -// previews local files being sent to the server — disjoint inputs, so only the -// visual style is shared, not the code. File paths are the user's own local -// inputs (not untrusted API data), so they're printed unsanitized like the rest -// of push's file output. -const reportPushDryRun = (filePaths: string[]): void => { +// header over a dim "would" list) and, like reportSummary, owns the failure +// exit code the real run would set for unresolved inputs — so a script +// previewing a wrong glob still learns it matched nothing. It can't reuse sync's +// reportDryRunPlan: that previews server records written to local files, whereas +// push previews local files sent to the server — disjoint inputs, so only the +// visual style is shared. Paths are sanitized like sync's printWritePreview: a +// directory/glob walk can surface a filename the user never typed and doesn't +// control (a synced folder, a cloned repo), so an escape sequence in a name +// can't reach the terminal live. +const reportPushDryRun = ( + filePaths: string[], + unresolvedCount: number, +): void => { console.log( chalk.yellow( `Dry run — previewing ${filePaths.length} file(s); nothing will be pushed.`, @@ -142,8 +150,17 @@ const reportPushDryRun = (filePaths: string[]): void => { ); console.log(chalk.dim(`Would push ${filePaths.length} file(s):`)); filePaths.forEach((filePath) => { - console.log(chalk.dim(` -> ${filePath}`)); + console.log(chalk.dim(sanitizeForTerminal(` -> ${filePath}`))); }); + + // A wrong glob (missing/unreadable inputs) is exactly what --dry-run exists to + // catch, so it exits non-zero like the real run would — minus the per-file + // push failures a dry run can't produce. + if (unresolvedCount === 0) { + return; + } + + process.exitCode = 1; }; const reportSummary = (run: PushRun, unresolvedCount: number): void => { @@ -218,18 +235,10 @@ export const runPushCommand = async (args: string[]): Promise => { const unresolvedCount = missing.length + skipped.length; - // A dry run stops before any createRecord call: report the resolved plan, - // then mirror the real run's honesty by exiting non-zero when inputs went - // unresolved (a wrong glob, an unreadable path) — the exact mistake the - // preview exists to catch — minus the per-file push failures a dry run - // can't produce. + // A dry run stops before any createRecord call: report the resolved plan + // (which sets its own failure exit code for unresolved inputs) and return. if (dryRun) { - reportPushDryRun(files); - - if (unresolvedCount > 0) { - process.exitCode = 1; - } - + reportPushDryRun(files, unresolvedCount); return; } diff --git a/tests/commands/push.test.ts b/tests/commands/push.test.ts index 1a90199..c33c705 100644 --- a/tests/commands/push.test.ts +++ b/tests/commands/push.test.ts @@ -699,6 +699,28 @@ describe('runPushCommand', () => { expect(process.exitCode).toBe(1); }); + it('reports skipped unreadable inputs and exits 1 on a dry run without pushing', async () => { + const { createRecord } = await import('@/libs/records.js'); + const { resolveMarkdownInputs } = await import('@/libs/files.js'); + vi.mocked(resolveMarkdownInputs).mockReturnValue({ + files: ['real.md'], + missing: [], + skipped: ['./vault/locked'], + }); + const { runPushCommand } = await import('@/commands/push.js'); + + await runPushCommand(['real.md', './vault', '--dry-run']); + + expect(createRecord).not.toHaveBeenCalled(); + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining('Skipped unreadable path "./vault/locked".'), + ); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('Would push 1 file(s):'), + ); + expect(process.exitCode).toBe(1); + }); + it('exits 1 on a dry run when no inputs resolve to any file', async () => { const { createRecord } = await import('@/libs/records.js'); const { resolveMarkdownInputs } = await import('@/libs/files.js'); From 712aee2a2cd0542c2fd5a1d854d8c3c493cb3b26 Mon Sep 17 00:00:00 2001 From: Danny Holloran Date: Thu, 27 Aug 2026 12:07:40 -0500 Subject: [PATCH 4/5] Address review round 3: sanitize missing/skipped lines, add sanitization test --- src/commands/push.ts | 18 ++++++++++++++++-- tests/commands/push.test.ts | 21 +++++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/src/commands/push.ts b/src/commands/push.ts index 79330fa..83d28c9 100644 --- a/src/commands/push.ts +++ b/src/commands/push.ts @@ -219,12 +219,26 @@ export const runPushCommand = async (args: string[]): Promise => { const { files, missing, skipped } = resolveMarkdownInputs(paths); + // Sanitize both unresolved-input lines: a `skipped` path is discovered by + // walking a directory (resolveMarkdownInputs), so it can carry a filename + // the user never typed and doesn't control — an escape sequence in that name + // must not reach the terminal live. `missing` echoes a user-typed glob (lower + // risk), but keeping both branches identical stops a reader having to work + // out which one is trusted. for (const input of missing) { - console.error(chalk.redBright(`No markdown files found for "${input}".`)); + console.error( + chalk.redBright( + sanitizeForTerminal(`No markdown files found for "${input}".`), + ), + ); } for (const path of skipped) { - console.error(chalk.redBright(`Skipped unreadable path "${path}".`)); + console.error( + chalk.redBright( + sanitizeForTerminal(`Skipped unreadable path "${path}".`), + ), + ); } if (files.length === 0) { diff --git a/tests/commands/push.test.ts b/tests/commands/push.test.ts index c33c705..9c1a06d 100644 --- a/tests/commands/push.test.ts +++ b/tests/commands/push.test.ts @@ -645,6 +645,27 @@ describe('runPushCommand', () => { expect(process.exitCode).toBeUndefined(); }); + // A dry run walks directories, so a previewed path can carry a filename the + // user never typed; a control character in it must be stripped before print. + it('strips control characters from a previewed path', async () => { + const { resolveMarkdownInputs } = await import('@/libs/files.js'); + vi.mocked(resolveMarkdownInputs).mockReturnValue({ + files: ['notes/\u001b[2Jgotcha.md'], + missing: [], + skipped: [], + }); + const { runPushCommand } = await import('@/commands/push.js'); + + await runPushCommand(['notes', '--dry-run']); + + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('notes/ [2Jgotcha.md'), + ); + expect(console.log).not.toHaveBeenCalledWith( + expect.stringContaining('\u001b'), + ); + }); + it('excludes the --dry-run flag from the resolved input paths', async () => { const { resolveMarkdownInputs } = await import('@/libs/files.js'); vi.mocked(resolveMarkdownInputs).mockReturnValue({ From 9b4c6095b3dfb461a4447dfde50e26d4776936f4 Mon Sep 17 00:00:00 2001 From: Danny Holloran Date: Thu, 27 Aug 2026 12:12:28 -0500 Subject: [PATCH 5/5] Address review round 4: POSIX -- separator, sanitize push output lines --- src/commands/push.ts | 49 ++++++++++++++++++++++----- tests/commands/push.test.ts | 66 +++++++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 8 deletions(-) diff --git a/src/commands/push.ts b/src/commands/push.ts index 83d28c9..64a8247 100644 --- a/src/commands/push.ts +++ b/src/commands/push.ts @@ -25,10 +25,15 @@ export const USAGE = `Usage: markpost push [--dry-run] const DRY_RUN_FLAG = '--dry-run'; // A leading `--` marks a long flag, not a path — used to reject a stray or -// mistyped long flag before it's mistaken for a file to push. Kept to `--` (not -// a bare `-`) so a legitimate dash-leading filename from a glob still pushes. +// mistyped long flag before it's mistaken for a file to push. A genuine +// dash-leading filename is still pushable after the OPTIONS_END separator below. const FLAG_PREFIX = '--'; +// POSIX end-of-options separator: everything after the first bare `--` is a +// literal path, even if it starts with dashes — the standard escape hatch for a +// file whose name would otherwise look like a flag (e.g. `push -- --notes.md`). +const OPTIONS_END = '--'; + // `systemicError` is set only when this file failed for a reason that will // recur for every other file (auth/5xx). Carrying it on the result rather than // throwing lets `pushFiles` decide to abort with a flat check instead of a @@ -66,11 +71,21 @@ const pushFile = async (filePath: string): Promise => { const record = await createRecord(title, content); if (!record) { - console.error(chalk.redBright(`Failed to push "${filePath}".`)); + console.error( + chalk.redBright(sanitizeForTerminal(`Failed to push "${filePath}".`)), + ); return { filePath, pushed: false }; } - console.log(chalk.greenBright(`Pushed "${record.title}" (${record.uuid})`)); + // record.title/uuid come from the API response (untrusted, see terminal.ts), + // and filePath can be a directory-walk name the user never typed — sanitize + // every push output line so a crafted title or filename can't inject a live + // escape into the terminal. + console.log( + chalk.greenBright( + sanitizeForTerminal(`Pushed "${record.title}" (${record.uuid})`), + ), + ); return { filePath, pushed: true }; } catch (error) { // A timeout must abort the whole batch, not be logged per-file and @@ -87,7 +102,11 @@ const pushFile = async (filePath: string): Promise => { } console.error( - chalk.redBright(`Failed to push "${filePath}": ${toMessage(error)}`), + chalk.redBright( + sanitizeForTerminal( + `Failed to push "${filePath}": ${toMessage(error)}`, + ), + ), ); return { filePath, pushed: false }; } @@ -171,7 +190,9 @@ const reportSummary = (run: PushRun, unresolvedCount: number): void => { console.log(chalk.dim(`Pushed ${succeeded}/${total} file(s) successfully.`)); if (abort) { - console.error(chalk.redBright(abortLine(abort))); + // abort.filePath and abort.reason (a server-classified failure) are both + // untrusted, so sanitize the composed line like the per-file output above. + console.error(chalk.redBright(sanitizeForTerminal(abortLine(abort)))); } // `abort` always leaves a `pushed: false` result behind, so `failed` already @@ -186,12 +207,21 @@ export const runPushCommand = async (args: string[]): Promise => { try { const dryRun = args.includes(DRY_RUN_FLAG); + // Split on the first `--`: args before it are flag-checked, args after it + // are literal paths (so a dash-leading filename stays pushable). The + // separator itself belongs to neither slice. + const separatorIndex = args.indexOf(OPTIONS_END); + const optionArgs = + separatorIndex === -1 ? args : args.slice(0, separatorIndex); + const literalPaths = + separatorIndex === -1 ? [] : args.slice(separatorIndex + 1); + // Reject a stray or mistyped flag (`--dryrun`, `--dry-run=1`) rather than // letting it fall through as a bogus path: push creates server records, so a // fat-fingered --dry-run must fail loud, never silently run the real push // when the user asked for a preview — the guard sync gives its one // destructive command. - const unexpectedFlags = args.filter( + const unexpectedFlags = optionArgs.filter( (arg) => arg.startsWith(FLAG_PREFIX) && arg !== DRY_RUN_FLAG, ); @@ -203,7 +233,10 @@ export const runPushCommand = async (args: string[]): Promise => { return; } - const paths = args.filter((arg) => arg.length > 0 && arg !== DRY_RUN_FLAG); + const paths = [ + ...optionArgs.filter((arg) => arg.length > 0 && arg !== DRY_RUN_FLAG), + ...literalPaths.filter((arg) => arg.length > 0), + ]; if (paths.length === 0) { failWithUsage('No path given.', USAGE); diff --git a/tests/commands/push.test.ts b/tests/commands/push.test.ts index 9c1a06d..9e5f6d0 100644 --- a/tests/commands/push.test.ts +++ b/tests/commands/push.test.ts @@ -666,6 +666,72 @@ describe('runPushCommand', () => { ); }); + // A file whose name starts with dashes is pushable via the POSIX `--` + // end-of-options separator, so a glob that expands to one isn't rejected. + it('treats args after -- as literal paths even when dash-leading', async () => { + const { resolveMarkdownInputs } = await import('@/libs/files.js'); + vi.mocked(resolveMarkdownInputs).mockReturnValue({ + files: ['--notes.md'], + missing: [], + skipped: [], + }); + const { runPushCommand } = await import('@/commands/push.js'); + + await runPushCommand(['--dry-run', '--', '--notes.md']); + + expect(resolveMarkdownInputs).toHaveBeenCalledWith(['--notes.md']); + expect(console.error).not.toHaveBeenCalledWith( + expect.stringContaining('Unexpected arguments'), + ); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('Would push 1 file(s):'), + ); + }); + + // An unexpected flag is rejected even alongside --dry-run: the guard runs + // before the preview, so a typo can't ride in on a valid dry run. + it('rejects an unexpected flag even when --dry-run is present', async () => { + const { resolveMarkdownInputs } = await import('@/libs/files.js'); + const { createRecord } = await import('@/libs/records.js'); + const { runPushCommand } = await import('@/commands/push.js'); + + await runPushCommand(['a.md', '--dry-run', '--verbose']); + + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining('Unexpected arguments: --verbose'), + ); + expect(resolveMarkdownInputs).not.toHaveBeenCalled(); + expect(createRecord).not.toHaveBeenCalled(); + expect(process.exitCode).toBe(1); + }); + + // A record title is server-controlled and untrusted; a control character in + // it must be stripped before the success line reaches the terminal. + it('strips control characters from a pushed record title', async () => { + const { createRecord } = await import('@/libs/records.js'); + const { readMarkdown } = await import('@/libs/markdown.js'); + const { resolveMarkdownInputs } = await import('@/libs/files.js'); + vi.mocked(resolveMarkdownInputs).mockReturnValue({ + files: ['a.md'], + missing: [], + skipped: [], + }); + vi.mocked(readMarkdown).mockReturnValue({ title: 'A', content: 'Content' }); + vi.mocked(createRecord).mockResolvedValue( + recordFor('Sneaky\u001b[2KTitle', 'uuid-a'), + ); + const { runPushCommand } = await import('@/commands/push.js'); + + await runPushCommand(['a.md']); + + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining('Pushed "Sneaky [2KTitle" (uuid-a)'), + ); + expect(console.log).not.toHaveBeenCalledWith( + expect.stringContaining('\u001b'), + ); + }); + it('excludes the --dry-run flag from the resolved input paths', async () => { const { resolveMarkdownInputs } = await import('@/libs/files.js'); vi.mocked(resolveMarkdownInputs).mockReturnValue({