From 453f473dbec8a4e94d72959115420cc3654ace35 Mon Sep 17 00:00:00 2001 From: caballeto Date: Tue, 5 May 2026 22:10:08 +0200 Subject: [PATCH] fix(monitors): truncate incidentPolicy in create output + add delete confirm MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The single-record table renderer used by `monitors create` (and every other CRUD `create` / `get` command) JSON-stringified nested objects in full, so a `MonitorDto.incidentPolicy` blob blew the value column out to ~1500 characters and pushed the freshly-minted monitor id off-screen. Truncate object previews to 80 chars with a `(use --output json for full)` hint so the id and the rest of the record stay readable; JSON / YAML output is untouched. `monitors delete ` (and every other CRUD `delete`) now prompts for confirmation showing the resource's name + id (best-effort GET — typo'd ids surface a 404 before the prompt). `--yes` / `-y` skips the prompt for scripted use. In a non-TTY context (CI, piped stdin) we refuse with an EXIT_CODES.VALIDATION error rather than hanging on a prompt that nobody can answer. Round-2 DevEx friction P1 #8 + P1 #9. Co-authored-by: Cursor --- src/lib/crud-commands.ts | 99 +++++++++++++++++++++++++-- src/lib/output.ts | 19 ++++- test/lib/delete-confirm.test.ts | 98 ++++++++++++++++++++++++++ test/lib/output-single-record.test.ts | 56 +++++++++++++++ 4 files changed, 266 insertions(+), 6 deletions(-) create mode 100644 test/lib/delete-confirm.test.ts create mode 100644 test/lib/output-single-record.test.ts diff --git a/src/lib/crud-commands.ts b/src/lib/crud-commands.ts index 3145505..2f5cb27 100644 --- a/src/lib/crud-commands.ts +++ b/src/lib/crud-commands.ts @@ -2,7 +2,8 @@ import {Command, Args, Flags, type Interfaces} from '@oclif/core' import type {ZodType} from 'zod' import {globalFlags, buildClient, display} from './base-command.js' import {fetchPaginatedValidated} from './typed-api.js' -import {apiDelete, apiGetSingle, apiPostSingle, apiPutSingle} from './api-client.js' +import {apiDelete, apiGetSingle, apiPostSingle, apiPutSingle, type ApiClient} from './api-client.js' +import {DevhelmAuthError, DevhelmNotFoundError, EXIT_CODES} from './errors.js' import type {ColumnDef} from './output.js' import {parse as parseSchema} from './response-validation.js' import {uuidArg} from './validators.js' @@ -201,15 +202,46 @@ export function createDeleteCommand(config: ResourceConfig) { const idLabel = config.idField ?? 'id' class DeleteCmd extends Command { static description = `Delete a ${config.name}` - static examples = [`<%= config.bin %> ${config.plural} delete <${idLabel}>`] + static examples = [ + `<%= config.bin %> ${config.plural} delete <${idLabel}>`, + `<%= config.bin %> ${config.plural} delete <${idLabel}> --yes`, + ] static args = {[idLabel]: idArg(config)} - static flags = {...globalFlags} + static flags = { + ...globalFlags, + yes: Flags.boolean({ + char: 'y', + description: 'Skip the interactive confirmation prompt', + default: false, + }), + } async run() { const {args, flags} = await this.parse(DeleteCmd) const client = buildClient(flags) - const id = args[idLabel] - await apiDelete(client, `${config.apiPath}/${id}`) + // The id arg is declared `required: true` on the static args block + // above, so oclif guarantees it's a string at runtime. The Record + // index signature still types it as optional, hence the narrow. + const id = args[idLabel] as string + const path = `${config.apiPath}/${id}` + + if (!flags.yes) { + if (!process.stdin.isTTY) { + // In CI / piped invocations, prompting would hang and silent + // auto-confirm would defeat the safety check. Refuse loudly. + this.error( + `Refusing to delete ${config.name} '${id}' in non-interactive mode without --yes (or -y).`, + {exit: EXIT_CODES.VALIDATION}, + ) + } + const confirmed = await promptForDeletion(config, id, path, client) + if (!confirmed) { + this.log('Cancelled.') + return + } + } + + await apiDelete(client, path) this.log(`${config.name} '${id}' deleted.`) } } @@ -217,6 +249,63 @@ export function createDeleteCommand(config: ResourceConfig) { return DeleteCmd } +/** + * Best-effort label extractor used by the delete confirmation prompt so + * the user sees `'My Monitor' (uuid)` instead of just the bare id. The + * candidate keys cover the human-readable identifier on every CRUD + * resource we ship: `name` for most, `slug`/`key` for status-page slugs + * and secret keys, `summary`/`title` for incidents, `email` for users. + * Returns `undefined` when nothing usable is found — the caller falls + * back to the id alone, which is still safer than no prompt at all. + */ +export function extractEntityLabel(value: unknown): string | undefined { + if (!value || typeof value !== 'object') return undefined + const record = value as Record + for (const key of ['name', 'slug', 'key', 'title', 'summary', 'email']) { + const candidate = record[key] + if (typeof candidate === 'string' && candidate.length > 0) return candidate + } + return undefined +} + +/** + * Interactive confirmation prompt for the generated delete commands. + * + * GETs the resource first so the prompt can show its human-readable + * name. A 404 (or 401/403) is surfaced before the destructive action: + * the typo'd id never makes it to the prompt. Any other GET failure is + * swallowed — we still prompt with the bare id rather than blocking the + * user from a delete that would otherwise succeed. + * + * The non-TTY refusal is done by the caller (it has access to + * `this.error()` for a clean oclif exit) so this helper only runs when + * stdin is interactive. + */ +async function promptForDeletion( + config: ResourceConfig, + id: string, + path: string, + client: ApiClient, +): Promise { + let label = `'${id}'` + try { + const value = await apiGetSingle(client, path, config.responseSchema as ZodType) + const name = extractEntityLabel(value) + if (name) label = `'${name}' (${id})` + } catch (err) { + if (err instanceof DevhelmAuthError || err instanceof DevhelmNotFoundError) throw err + } + + const {createInterface} = await import('node:readline') + const rl = createInterface({input: process.stdin, output: process.stderr}) + const answer = await new Promise((resolve) => { + rl.question(`Delete ${config.name} ${label}? [y/N] `, resolve) + }) + rl.close() + const normalized = answer.trim().toLowerCase() + return normalized === 'y' || normalized === 'yes' +} + function extractResourceFlags(flags: Record, keys: string[]): Record { const body: Record = {} for (const key of keys) { diff --git a/src/lib/output.ts b/src/lib/output.ts index 6c0fee9..c829f64 100644 --- a/src/lib/output.ts +++ b/src/lib/output.ts @@ -59,12 +59,29 @@ function formatTable(data: unknown, columns?: ColumnDef[]): string { return table.toString() } +/** + * Default-table render of a single record (e.g. the response body of a + * `create` / `get` command). Nested objects are JSON-stringified and then + * truncated so one noisy field (e.g. `incidentPolicy`, `config`, `auth`) + * doesn't blow the value column out to several thousand characters and + * push the rest of the record off-screen. Users who actually need the + * full structure can re-run with `--output json` (or `yaml`). + */ +const SINGLE_RECORD_OBJECT_PREVIEW_CHARS = 80 +const SINGLE_RECORD_TRUNCATION_HINT = '… (use --output json for full)' + +function previewObjectValue(value: unknown): string { + const json = JSON.stringify(value) ?? '' + if (json.length <= SINGLE_RECORD_OBJECT_PREVIEW_CHARS) return json + return json.slice(0, SINGLE_RECORD_OBJECT_PREVIEW_CHARS) + SINGLE_RECORD_TRUNCATION_HINT +} + function formatSingleRecord(data: Record): string { const table = new Table({style: {head: ['cyan']}}) for (const [key, value] of Object.entries(data)) { if (value === undefined) continue - const display = typeof value === 'object' ? JSON.stringify(value) : String(value ?? '') + const display = typeof value === 'object' && value !== null ? previewObjectValue(value) : String(value ?? '') table.push({[key]: display}) } diff --git a/test/lib/delete-confirm.test.ts b/test/lib/delete-confirm.test.ts new file mode 100644 index 0000000..d1235b9 --- /dev/null +++ b/test/lib/delete-confirm.test.ts @@ -0,0 +1,98 @@ +import {describe, it, expect, beforeEach, afterEach, vi} from 'vitest' +import {z} from 'zod' +import {extractEntityLabel, createDeleteCommand, type ResourceConfig} from '../../src/lib/crud-commands.js' +import {EXIT_CODES} from '../../src/lib/errors.js' + +describe('extractEntityLabel', () => { + it('returns the name field when present', () => { + expect(extractEntityLabel({id: 'x', name: 'My Monitor'})).toBe('My Monitor') + }) + + it('falls back through the candidate list', () => { + expect(extractEntityLabel({id: 'x', slug: 'my-page'})).toBe('my-page') + expect(extractEntityLabel({id: 'x', key: 'API_KEY'})).toBe('API_KEY') + expect(extractEntityLabel({id: 'x', summary: 'Outage on us-east-1'})).toBe('Outage on us-east-1') + expect(extractEntityLabel({id: 'x', email: 'ops@example.com'})).toBe('ops@example.com') + }) + + it('prefers name over secondary fields', () => { + expect(extractEntityLabel({id: 'x', name: 'A', slug: 'b'})).toBe('A') + }) + + it('returns undefined when no candidate field matches', () => { + expect(extractEntityLabel({id: 'x', foo: 'bar'})).toBeUndefined() + }) + + it('ignores empty strings and non-string values', () => { + expect(extractEntityLabel({id: 'x', name: ''})).toBeUndefined() + expect(extractEntityLabel({id: 'x', name: 42})).toBeUndefined() + }) + + it('returns undefined for non-objects', () => { + expect(extractEntityLabel(null)).toBeUndefined() + expect(extractEntityLabel(undefined)).toBeUndefined() + expect(extractEntityLabel('a string')).toBeUndefined() + expect(extractEntityLabel(42)).toBeUndefined() + }) +}) + +const TEST_RESOURCE: ResourceConfig<{id: string; name: string}> = { + name: 'widget', + plural: 'widgets', + apiPath: '/api/v1/widgets', + responseSchema: z.object({id: z.string(), name: z.string()}), + columns: [ + {header: 'ID', get: (r) => r.id}, + {header: 'NAME', get: (r) => r.name}, + ], +} + +describe('createDeleteCommand --yes / TTY behavior', () => { + const SAVED_TOKEN = process.env.DEVHELM_API_TOKEN + const ORIGINAL_IS_TTY = process.stdin.isTTY + + beforeEach(() => { + // Ensure buildClient() doesn't throw on the auth check before we get + // to the prompt path the test actually exercises. + process.env.DEVHELM_API_TOKEN = 'dh_test_dummy' + }) + + afterEach(() => { + if (SAVED_TOKEN === undefined) delete process.env.DEVHELM_API_TOKEN + else process.env.DEVHELM_API_TOKEN = SAVED_TOKEN + Object.defineProperty(process.stdin, 'isTTY', {configurable: true, value: ORIGINAL_IS_TTY}) + vi.restoreAllMocks() + }) + + it('exposes the --yes / -y flag on the generated command', () => { + const Cmd = createDeleteCommand(TEST_RESOURCE) as unknown as {flags: {yes: {char?: string}}} + expect(Cmd.flags.yes).toBeDefined() + expect(Cmd.flags.yes.char).toBe('y') + }) + + it('refuses to run in non-TTY mode without --yes', async () => { + Object.defineProperty(process.stdin, 'isTTY', {configurable: true, value: false}) + const Cmd = createDeleteCommand(TEST_RESOURCE) + const id = '00000000-0000-0000-0000-000000000001' + // oclif's `this.error()` throws a typed Error tagged with the exit + // code; we assert on the message + exit code rather than the + // concrete class so we're not coupled to oclif's internal type. + await expect(Cmd.run([id])).rejects.toThrow(/non-interactive mode without --yes/) + await expect(Cmd.run([id])).rejects.toMatchObject({oclif: {exit: EXIT_CODES.VALIDATION}}) + }) + + it('skips the non-TTY refusal entirely when --yes is supplied', async () => { + Object.defineProperty(process.stdin, 'isTTY', {configurable: true, value: false}) + const Cmd = createDeleteCommand(TEST_RESOURCE) + const id = '00000000-0000-0000-0000-000000000002' + // With --yes we expect the command to bypass the prompt and reach + // the apiDelete network call. We don't have a live API so the call + // will fail downstream, but it must NOT fail with the + // "non-interactive mode" guard. + try { + await Cmd.run([id, '--yes', '--api-url', 'http://127.0.0.1:1']) + } catch (err) { + expect(err).not.toMatchObject({message: expect.stringMatching(/non-interactive mode/)}) + } + }) +}) diff --git a/test/lib/output-single-record.test.ts b/test/lib/output-single-record.test.ts new file mode 100644 index 0000000..639b153 --- /dev/null +++ b/test/lib/output-single-record.test.ts @@ -0,0 +1,56 @@ +import {describe, it, expect} from 'vitest' +import {formatOutput} from '../../src/lib/output.js' + +describe('formatOutput single-record table', () => { + it('JSON-stringifies short nested objects in full', () => { + const out = formatOutput({id: 'x', tags: ['a', 'b']}, 'table') + // Short arrays/objects render fully — no truncation hint. + expect(out).toContain('["a","b"]') + expect(out).not.toContain('use --output json') + }) + + it('truncates long nested object values with a hint to use --output json', () => { + const incidentPolicy = { + enabled: true, + escalations: Array.from({length: 12}, (_, i) => ({ + order: i, + delaySeconds: i * 60, + channelIds: ['00000000-0000-0000-0000-000000000000'], + })), + } + const out = formatOutput({id: 'mon-1', name: 'X', incidentPolicy}, 'table') + + // The truncated rendering carries the hint, and the resulting + // single-row width stays well under what the raw JSON would produce + // (the raw blob is >1000 chars). + expect(out).toContain('use --output json') + const rawJsonLength = JSON.stringify(incidentPolicy).length + expect(rawJsonLength).toBeGreaterThan(500) + const widestLine = Math.max(...out.split('\n').map((l) => l.length)) + expect(widestLine).toBeLessThan(200) + }) + + it('does not break when a nested object stringifies to exactly the cutoff', () => { + // 79-char JSON value — under the limit, no hint expected. + const value = {a: 'x'.repeat(70)} + expect(JSON.stringify(value).length).toBeLessThan(80) + const out = formatOutput({id: 'x', value}, 'table') + expect(out).not.toContain('use --output json') + }) + + it('renders null nested values as empty rather than {}', () => { + const out = formatOutput({id: 'x', auth: null}, 'table') + // null falls through to `String(value ?? '')` which gives '' — we + // never want to render the literal "null" or surface a misleading + // truncated "{}" for a missing value. + expect(out).not.toContain('null') + }) + + it('JSON output is unchanged by the table-only truncation logic', () => { + const incidentPolicy = {enabled: true, escalations: Array.from({length: 12}, () => ({delaySeconds: 60}))} + const out = formatOutput({id: 'mon-1', incidentPolicy}, 'json') + // The full structure must round-trip through JSON output even though + // the table renderer truncates it. + expect(JSON.parse(out)).toEqual({id: 'mon-1', incidentPolicy}) + }) +})