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 0b256e1..64a8247 100644 --- a/src/commands/push.ts +++ b/src/commands/push.ts @@ -10,11 +10,29 @@ 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 +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'; + +// 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. 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 @@ -53,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 @@ -74,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 }; } @@ -116,6 +148,40 @@ 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) 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.`, + ), + ); + console.log(chalk.dim(`Would push ${filePaths.length} file(s):`)); + filePaths.forEach((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 => { const { results, abort, total } = run; const succeeded = results.filter((result) => result.pushed).length; @@ -124,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 @@ -137,25 +205,73 @@ 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); + + // 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 = optionArgs.filter( + (arg) => arg.startsWith(FLAG_PREFIX) && arg !== DRY_RUN_FLAG, + ); + + if (unexpectedFlags.length > 0) { + failWithUsage( + `Unexpected arguments: ${unexpectedFlags.join(' ')}`, + USAGE, + ); + return; + } + + 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); 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; } 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) { @@ -164,8 +280,17 @@ 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 + // (which sets its own failure exit code for unresolved inputs) and return. + if (dryRun) { + reportPushDryRun(files, unresolvedCount); + 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..9e5f6d0 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,259 @@ 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(); + }); + + // 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(); + }); + + // 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'), + ); + }); + + // 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({ + 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('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'); + 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'));