Skip to content

Commit 6a4b33b

Browse files
committed
fix(cli): gate destructive table imports and fix follow-mode rendering
A `tables import --mode replace` empties the table before its first batch, so the only warning was in the describe. It now confirms, and the wording tells the truth per mode: cancelling a replace leaves a prefix of the new file with the originals already gone, while an append re-adds its rows if the file is imported twice. `--yes` skips the gate, and the gate runs before the file is opened. Import and export cancellation carried no describe at all; the import one now confirms, the export one records why it deliberately does not. Follow-mode output truncated cells to whatever the first row happened to measure, so a longer status or workflow name arrived clipped with no signal. Cells now clamp at a shared ceiling and pad to the lock, and the log columns carry width floors so a short first page cannot pin a column narrower than its own values. Interrupting a staged download left the staging directory behind; it is now removed on SIGINT and SIGTERM before the signal is re-raised. `--select-output` without `--follow` selected from a response that does not carry outputs, and said nothing. It is refused client-side, with a separate message for `--async`. Its describe now names what the path addresses. `secrets set` always read a value, even when only metadata flags were passed. Off a TTY that was an immediate refusal, so a metadata-only edit exited 1 in CI for a value it was never asked for; on a TTY it stopped to prompt, and the prompt rejects an empty entry, so there was no way to say "leave the stored value alone" short of re-typing the secret. The read is now skipped and the field omitted, which is what lets a metadata-only edit run unattended. On a TTY, setting only a description no longer prompts. Passing both spellings of the reveal flag is refused rather than silently resolved. Four mandatory hand-authored flags now say so, `billing logs` names its key-type scope, and the dispatch list declares its columns.
1 parent b662356 commit 6a4b33b

17 files changed

Lines changed: 843 additions & 49 deletions

packages/sim-cli/src/commands/credentials.test.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,24 @@ describe('credential connection commands', () => {
156156
expect(help).not.toContain('--service-account-json')
157157
})
158158

159+
it('marks its mandatory flags required in the help it renders', () => {
160+
// Commander enforces `requiredOption` but renders nothing to say so — the
161+
// marker is literal text the generated flags carry, so a hand-written
162+
// command is the one place a required flag can look optional.
163+
const flat = (...names: string[]) =>
164+
commandAt(...names)
165+
.helpInformation()
166+
.replace(/\s+/g, ' ')
167+
168+
expect(flat('credentials', 'create')).toContain(
169+
'Name shown for the credential in Sim (required)'
170+
)
171+
expect(flat('credentials', 'create')).toContain('read a file or stdin) (required)')
172+
expect(flat('credentials', 'connect')).toContain(
173+
'Name shown for the new credential in Sim (required)'
174+
)
175+
})
176+
159177
it('rejects missing and unsupported provider fields before creation', async () => {
160178
mockRequest.mockReset().mockResolvedValue({
161179
data: [

packages/sim-cli/src/commands/credentials.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -193,10 +193,14 @@ export function attachCredentialCommands(program: Command): void {
193193
.command('create')
194194
.argument('<providerId>', 'Service-account provider to create a credential for')
195195
.description('Create a service-account credential using its discovered provider schema')
196-
.requiredOption('--name <displayName>', 'Name shown for the credential in Sim')
196+
// The `(required)` suffix is the marker the generated flags carry, and it
197+
// is literal text rather than something commander renders — a hand-written
198+
// mandatory option that omits it is the only kind of required flag whose
199+
// help does not say so.
200+
.requiredOption('--name <displayName>', 'Name shown for the credential in Sim (required)')
197201
.requiredOption(
198202
'--credentials <json|@file>',
199-
'Provider credentials as JSON (or @path / @- to read a file or stdin)'
203+
'Provider credentials as JSON (or @path / @- to read a file or stdin) (required)'
200204
)
201205
.option('--description <description>', 'Optional credential description')
202206
.option(
@@ -211,7 +215,7 @@ export function attachCredentialCommands(program: Command): void {
211215
.command('connect')
212216
.argument('<providerId>', 'OAuth provider to connect')
213217
.description('Create a short-lived link for connecting an OAuth provider')
214-
.requiredOption('--name <displayName>', 'Name shown for the new credential in Sim')
218+
.requiredOption('--name <displayName>', 'Name shown for the new credential in Sim (required)')
215219
.action(async (providerId: string, options: { name: string }, command: Command) =>
216220
createConnectionLink(command, { providerId, displayName: options.name })
217221
)

packages/sim-cli/src/commands/protocol/files-get.test.ts

Lines changed: 63 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import {
33
existsSync,
44
lstatSync,
55
mkdtempSync,
6+
readdirSync,
67
readFileSync,
78
rmSync,
89
symlinkSync,
@@ -14,7 +15,12 @@ import { Writable } from 'node:stream'
1415
import { Command } from 'commander'
1516
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
1617
import { buildGeneratedCommands } from '../../runtime/build'
17-
import { isTerminalSafeContentType, saveToFile, streamToFile } from './files-get'
18+
import {
19+
isTerminalSafeContentType,
20+
removeStagingOnSignal,
21+
saveToFile,
22+
streamToFile,
23+
} from './files-get'
1824
import { attachProtocolCommands } from './index'
1925

2026
const { output, requestRaw } = vi.hoisted(() => ({
@@ -89,6 +95,62 @@ function program(): Command {
8995
return root
9096
}
9197

98+
describe('an interrupted download', () => {
99+
/** Staging directories left beside a destination, as `ls -a` shows them. */
100+
function stagingDirectories(): string[] {
101+
return readdirSync(dir).filter((entry) => entry.startsWith('.sim-download-'))
102+
}
103+
104+
it('removes the staging directory and re-raises when a signal arrives', () => {
105+
const staging = mkdtempSync(join(dir, '.sim-download-'))
106+
writeFileSync(join(staging, 'payload'), 'partial')
107+
// Injected: the real termination re-raises the signal, which would take the
108+
// test runner down with it.
109+
const terminate = vi.fn()
110+
const dispose = removeStagingOnSignal(() => staging, terminate)
111+
112+
process.emit('SIGINT')
113+
dispose()
114+
115+
expect(existsSync(staging)).toBe(false)
116+
expect(terminate).toHaveBeenCalledWith('SIGINT')
117+
})
118+
119+
it('watches for signals only while a download is staged', async () => {
120+
const before = { int: process.listenerCount('SIGINT'), term: process.listenerCount('SIGTERM') }
121+
let observed = 0
122+
const body = new ReadableStream<Uint8Array>({
123+
pull(controller) {
124+
observed = process.listenerCount('SIGINT')
125+
controller.enqueue(new TextEncoder().encode('data'))
126+
controller.close()
127+
},
128+
})
129+
130+
await saveToFile(body, join(dir, 'out.bin'), false)
131+
132+
expect(observed).toBe(before.int + 1)
133+
expect(process.listenerCount('SIGINT')).toBe(before.int)
134+
expect(process.listenerCount('SIGTERM')).toBe(before.term)
135+
})
136+
137+
it('disposes the watch when the publish itself fails', async () => {
138+
const target = join(dir, 'out.txt')
139+
writeFileSync(target, 'precious')
140+
const before = process.listenerCount('SIGINT')
141+
142+
await expect(saveToFile(bodyOf(['new']), target, false)).rejects.toThrow(/already exists/)
143+
144+
expect(process.listenerCount('SIGINT')).toBe(before)
145+
})
146+
147+
it('leaves no staging directory behind on a completed download', async () => {
148+
await saveToFile(bodyOf(['done']), join(dir, 'out.bin'), false)
149+
150+
expect(stagingDirectories()).toEqual([])
151+
})
152+
})
153+
92154
describe('streamToFile', () => {
93155
it('writes the body to disk', async () => {
94156
const target = join(dir, 'out.txt')

packages/sim-cli/src/commands/protocol/files-get.ts

Lines changed: 60 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { once } from 'node:events'
2-
import { createWriteStream, type WriteStream } from 'node:fs'
2+
import { createWriteStream, rmSync, type WriteStream } from 'node:fs'
33
import { link, lstat, mkdtemp, readlink, rename, rm } from 'node:fs/promises'
44
import { dirname, join, resolve } from 'node:path'
55
import { Readable, type Writable } from 'node:stream'
@@ -89,13 +89,68 @@ export async function streamToFile(
8989
}
9090
}
9191

92+
/** Signals that end the process while a download is staged beside its target. */
93+
const STAGE_SIGNALS: readonly NodeJS.Signals[] = ['SIGINT', 'SIGTERM']
94+
95+
/**
96+
* Ends the process by the signal that arrived, once our own handler has run.
97+
*
98+
* Installing a listener suppresses Node's default termination, so the handler
99+
* has to terminate itself. Re-raising rather than `process.exit(130)` keeps the
100+
* process dying *by signal*, so a wrapping shell still sees 130/143 and a
101+
* `trap` still fires — the behaviour an interrupted download has today.
102+
*/
103+
function reRaise(signal: NodeJS.Signals): void {
104+
process.removeAllListeners(signal)
105+
process.kill(process.pid, signal)
106+
}
107+
108+
/**
109+
* Removes the staging directory when a signal ends the process.
110+
*
111+
* `saveStagedFile` cleans up in normal control flow, which a signal never
112+
* reaches: the process is torn down mid-`pipeline`, so every Ctrl-C left
113+
* another `.sim-download-*` holding a partial payload beside the destination.
114+
* The removal is synchronous because the termination that follows gives an
115+
* async `rm` no turn to run.
116+
*
117+
* Exported for its own test: driving it through a real interrupt would take the
118+
* test runner down with it.
119+
*/
120+
export function removeStagingOnSignal(
121+
stagingDirectory: () => string | null,
122+
terminate: (signal: NodeJS.Signals) => void = reRaise
123+
): () => void {
124+
const installed = STAGE_SIGNALS.map((signal) => {
125+
const onSignal = () => {
126+
const directory = stagingDirectory()
127+
if (directory) {
128+
try {
129+
rmSync(directory, { recursive: true, force: true })
130+
} catch {
131+
// A staging directory we cannot remove is not worth masking the
132+
// interrupt the caller asked for.
133+
}
134+
}
135+
terminate(signal)
136+
}
137+
process.on(signal, onSignal)
138+
return [signal, onSignal] as const
139+
})
140+
141+
return () => {
142+
for (const [signal, onSignal] of installed) process.off(signal, onSignal)
143+
}
144+
}
145+
92146
async function saveStagedFile(
93147
body: ReadableStream<Uint8Array>,
94148
target: string,
95149
force: boolean
96150
): Promise<void> {
97151
let temporaryDirectory: string | null = null
98152
let failure: SimApiError | null = null
153+
const disposeSignalCleanup = removeStagingOnSignal(() => temporaryDirectory)
99154

100155
try {
101156
const publicationTarget = force ? await forcedPublicationTarget(target) : target
@@ -113,6 +168,10 @@ async function saveStagedFile(
113168
}
114169
} catch (error) {
115170
failure = normalizedWriteFailure(target, error)
171+
} finally {
172+
// Disposed on every path out, including the publish failure below: a
173+
// handler left installed would outlive the directory it removes.
174+
disposeSignalCleanup()
116175
}
117176

118177
if (temporaryDirectory) {

packages/sim-cli/src/commands/protocol/logs-follow.test.ts

Lines changed: 86 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,10 @@
33
*/
44
import { Command } from 'commander'
55
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
6+
import { CLI_CONTRACT } from '../../contract/commands'
67
import { type ListLogsResponse, V2_OPERATIONS } from '../../generated/v2-api'
78
import { SimApiError } from '../../http/client'
8-
import { attachLogsFollow, type LogRow } from './logs-follow'
9+
import { attachLogsFollow, type LogRow, MAX_CELL_WIDTH } from './logs-follow'
910

1011
const { mockRequest, mockSleep, profile } = vi.hoisted(() => ({
1112
mockRequest: vi.fn(),
@@ -236,6 +237,90 @@ describe('sim logs follow', () => {
236237
expect(stdout).toHaveLength(3)
237238
})
238239

240+
it('keeps a run id whole when the follow started with an empty backlog', async () => {
241+
// `-n 0` seeds the writer with no rows, so the widths used to lock to the
242+
// header labels — RUN is three characters, and a 36-character run id
243+
// printed as `9f…`, uncopyable.
244+
profile.output = 'table'
245+
const runId = '9f5e9856-1801-4028-a85f-6e335e65d974'
246+
const arrival = row(runId, '2026-08-17T10:00:01.000Z')
247+
arrival.workflow = {
248+
id: 'wf_1',
249+
name: 'clitest-nightly-sync',
250+
description: null,
251+
deleted: false,
252+
}
253+
respondWith([page([]), page([arrival])])
254+
255+
await follow('-n', '0')
256+
257+
const printed = stdout.join('\n')
258+
expect(printed).toContain(runId)
259+
expect(printed).toContain('clitest-nightly-sync')
260+
expect(printed).not.toContain('…')
261+
})
262+
263+
it('lines an arriving row up with the header it printed before any rows', async () => {
264+
// The floors are the half of this that keeps `-n 0` readable: without them
265+
// the columns lock to their header labels, and every cell of the first real
266+
// row overflows, so nothing below the header lines up for the life of the
267+
// follow.
268+
profile.output = 'table'
269+
const arrival = row('9f5e9856-1801-4028-a85f-6e335e65d974', '2026-08-17T10:00:01.000Z')
270+
respondWith([page([]), page([arrival])])
271+
272+
await follow('-n', '0')
273+
274+
const [header, printed] = stdout
275+
expect(printed.indexOf('completed')).toBe(header.indexOf('STATUS'))
276+
expect(printed.indexOf('Nightly sync')).toBe(header.indexOf('WORKFLOW'))
277+
expect(printed.indexOf('9f5e9856')).toBe(header.indexOf('RUN'))
278+
})
279+
280+
it('keeps a later row wider than the batch that locked the widths', async () => {
281+
profile.output = 'table'
282+
const first = row('run_1', '2026-08-17T10:00:01.000Z')
283+
first.workflow = { id: 'wf_1', name: 'short', description: null, deleted: false }
284+
const second = row('run_2', '2026-08-17T10:00:02.000Z')
285+
second.workflow = {
286+
id: 'wf_2',
287+
name: 'a-considerably-longer-workflow-name',
288+
description: null,
289+
deleted: false,
290+
}
291+
respondWith([page([first]), page([second, first])])
292+
293+
await follow('-n', '1')
294+
295+
expect(stdout.join('\n')).toContain('a-considerably-longer-workflow-name')
296+
})
297+
298+
it('still cuts a cell at the width one column may ever take', async () => {
299+
profile.output = 'table'
300+
const huge = 'x'.repeat(MAX_CELL_WIDTH + 40)
301+
const arrival = row('run_1', '2026-08-17T10:00:01.000Z')
302+
arrival.workflow = { id: 'wf_1', name: huge, description: null, deleted: false }
303+
respondWith([page([]), page([arrival])])
304+
305+
await follow('-n', '0')
306+
307+
const printed = stdout.join('\n')
308+
expect(printed).not.toContain(huge)
309+
expect(printed).toContain(`${'x'.repeat(MAX_CELL_WIDTH - 1)}…`)
310+
})
311+
312+
it('asks for no column floor wider than a column may render', () => {
313+
// A floor above the cap is silently clamped, so an oversized spec would
314+
// read as deliberate and do nothing.
315+
const oversized = Object.entries(CLI_CONTRACT).flatMap(([operation, spec]) =>
316+
[...(spec.columns ?? []), ...(spec.fields ?? [])]
317+
.filter((column) => (column.minWidth ?? 0) > MAX_CELL_WIDTH)
318+
.map((column) => `${operation}.${column.header}`)
319+
)
320+
321+
expect(oversized).toEqual([])
322+
})
323+
239324
it('keeps rows on stdout and retry notices on stderr', async () => {
240325
Object.defineProperty(process.stderr, 'isTTY', { value: true, configurable: true })
241326
const first = row('run_1', '2026-08-17T10:00:01.000Z')

0 commit comments

Comments
 (0)