Skip to content

Commit bcd9f2e

Browse files
committed
fix(cli): refuse what the help already says is invalid
Live testing of the published package found a set of flags the CLI documents as constrained and then transmits anyway. `--limit` on the two filtered row mutations says "0 is not accepted" and sent 0, -1 and 1.5; `--max-bytes` leaked its validator's own wording for a fraction or an oversized value; `--recipe` named no accepted values though the server takes four; a blank message or a malformed conversation id reached the wire, and an empty `-c` was dropped from the body so the turn silently started a new conversation. Selecting rows to delete by neither filter nor id reported the opposite mistake, because the only message came from the refinement that also catches passing both. `workflows activate create` chooses which existing version serves live traffic, exactly as `rollback` does, and `rollback` is gated where it was not. `deploy` is left alone: it publishes the draft the caller just typed, it is additive, and rollback undoes it. `--limit 0` with run state walked the whole table in pages the server accepts one at a time, which is what its own rule exists to prevent. `--workspace` is a root flag, so it is parsed for the thirty-nine operations that have nowhere to put it and then dropped. That silence is what made a correctly-scoped route look like it was ignoring scope, so it warns now rather than refusing — too many commands take it harmlessly for a refusal to be safe. Messages that named a request field the caller cannot type were only ever half rewritten, because a single-word field is indistinguishable from an English word. A message is now translated whole or left as the server sent it, so no sentence mixes the two vocabularies.
1 parent 76c7be5 commit bcd9f2e

23 files changed

Lines changed: 760 additions & 84 deletions

packages/sim-cli/src/commands/protocol/chat.test.ts

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,9 @@ function written(spy: WriteSpy): string {
100100
return spy.mock.calls.map((call) => String(call[0])).join('')
101101
}
102102

103+
/** A conversation id in the shape the route accepts and the command prints. */
104+
const CONVERSATION_ID = '3f2a1c4e-0000-4000-8000-000000000000'
105+
103106
const FINAL = {
104107
type: 'final',
105108
data: { content: 'Hello there', conversationId: 'conv-1', model: 'sim' },
@@ -127,17 +130,35 @@ describe('sim chat', () => {
127130
expect(written(stderr)).toContain('conversation: conv-1')
128131
})
129132

133+
/**
134+
* The route's own refusals name `message` and `conversationId`, and this
135+
* command builds its request by hand so nothing retypes them into what the
136+
* caller typed. A blank `-c` was worse than misnamed: it is falsy, so it was
137+
* dropped from the body and silently started a NEW conversation.
138+
*/
139+
it('refuses a blank message and a malformed -c before the request', async () => {
140+
await expect(run(' ')).rejects.toThrow('<message> cannot be empty')
141+
await expect(run('-c', '', 'hello')).rejects.toThrow('-c/--conversation must be a')
142+
await expect(run('-c', 'conv-1', 'hello')).rejects.toThrow('-c/--conversation must be a')
143+
expect(requestRaw).not.toHaveBeenCalled()
144+
})
145+
146+
/**
147+
* A conversation id as the command prints it: the route requires a UUID and
148+
* the CLI now says so before the request, so a stand-in like `conv-1` is
149+
* refused rather than sent.
150+
*/
130151
it('passes -c through as the conversation to continue', async () => {
131152
requestRaw.mockResolvedValue(ndjson([FINAL]))
132153

133-
await run('-c', 'conv-1', 'And which run on a schedule?')
154+
await run('-c', CONVERSATION_ID, 'And which run on a schedule?')
134155

135156
expect(requestRaw).toHaveBeenCalledWith('/api/v2/chat', {
136157
method: 'POST',
137158
body: {
138159
workspaceId: 'ws_local',
139160
message: 'And which run on a schedule?',
140-
conversationId: 'conv-1',
161+
conversationId: CONVERSATION_ID,
141162
},
142163
headers: { accept: 'application/x-ndjson' },
143164
})

packages/sim-cli/src/commands/protocol/chat.ts

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,12 @@ function ignoreBrokenPipe(stream: NodeJS.WriteStream): () => void {
142142
* proxies from idling the connection out. The generated `chat` operation is
143143
* hidden in the CLI contract in favour of this command.
144144
*/
145+
/**
146+
* The shape the route accepts for `conversationId`: a UUID, which is what the
147+
* command prints on stderr at the end of every turn.
148+
*/
149+
const CONVERSATION_ID = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i
150+
145151
export function attachChat(program: Command): void {
146152
program
147153
.command('chat')
@@ -164,6 +170,25 @@ Examples:
164170
`
165171
)
166172
.action(async (message: string, options: ChatOptions, command: Command) => {
173+
/**
174+
* Refused here so the refusal names what the caller typed. The route says
175+
* `message cannot be empty` and `conversationId must be a valid
176+
* conversation id` — its own field names, and this command builds its
177+
* request by hand so nothing retypes them into `<message>` and
178+
* `-c/--conversation`. A blank `-c` was worse than misnamed: it is falsy,
179+
* so it was dropped from the body and silently started a NEW conversation
180+
* instead of continuing one.
181+
*/
182+
if (message.trim() === '') {
183+
throw new SimApiError('<message> cannot be empty', 0)
184+
}
185+
if (options.conversation !== undefined && !CONVERSATION_ID.test(options.conversation)) {
186+
throw new SimApiError(
187+
'-c/--conversation must be a conversation id — the one printed on stderr after each turn',
188+
0
189+
)
190+
}
191+
167192
const { client, profile } = clientFrom(command)
168193
const workspaceId = client.requireWorkspace()
169194

packages/sim-cli/src/commands/protocol/knowledge-document-upload.test.ts

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -213,6 +213,41 @@ describe('knowledge documents upload', () => {
213213
expect(mockRequest).not.toHaveBeenCalled()
214214
})
215215

216+
/**
217+
* The route enforces both, and neither said so: the help read "Document
218+
* processing recipe" / "Document language code" and the CLI uploaded the file
219+
* before the server refused the value. Every other constrained flag in this
220+
* CLI uses commander `choices`.
221+
*/
222+
it('states what --recipe and --lang accept, and refuses a recipe before uploading', async () => {
223+
const path = join(dir, 'notes.txt')
224+
writeFileSync(path, 'hello')
225+
226+
const help = program()
227+
.commands.find((command) => command.name() === 'knowledge')
228+
?.commands.find((command) => command.name() === 'documents')
229+
?.commands.find((command) => command.name() === 'upload')
230+
?.helpInformation()
231+
.replace(/\s+/g, ' ')
232+
expect(help).toContain('choices: "default", "plain", "markdown", "code"')
233+
expect(help).toContain('hyphen-separated letter and digit subtags')
234+
235+
await expect(
236+
program().parseAsync([
237+
'node',
238+
'sim',
239+
'kb',
240+
'documents',
241+
'upload',
242+
'kb_1',
243+
path,
244+
'--recipe',
245+
'super-chunker-9000',
246+
])
247+
).rejects.toThrow(/Allowed choices are default, plain, markdown, code/)
248+
expect(mockRequest).not.toHaveBeenCalled()
249+
})
250+
216251
it('requires the knowledge-base argument before reading the file', async () => {
217252
const path = join(dir, 'notes.txt')
218253
writeFileSync(path, 'hello')

packages/sim-cli/src/commands/protocol/knowledge-document-upload.ts

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import type { Command } from 'commander'
1+
import { type Command, Option } from 'commander'
22
import { clientFrom } from '../../context'
33
import type {
44
CompleteKnowledgeDocumentUploadResponse,
@@ -35,6 +35,21 @@ function uploadMetadata(options: KnowledgeDocumentUploadOptions): Record<string,
3535
return metadata
3636
}
3737

38+
/**
39+
* The recipes the route accepts. Stated as `choices` like every other
40+
* constrained flag in this CLI, so `--help` lists them and a typo is refused
41+
* before the file is uploaded rather than after.
42+
*/
43+
const UPLOAD_RECIPES = ['default', 'plain', 'markdown', 'code'] as const
44+
45+
/**
46+
* The route enforces a shape, not BCP-47 conformance, so the help says the
47+
* shape and nothing more; a full parser is the route's own deliberate
48+
* non-goal and reimplementing one here would refuse tags the server accepts.
49+
*/
50+
const LANGUAGE_TAG_HELP =
51+
'Document language tag: hyphen-separated letter and digit subtags, for example en or en-US'
52+
3853
export function attachKnowledgeDocumentUpload(documents: Command): void {
3954
documents
4055
.command('upload')
@@ -44,8 +59,8 @@ export function attachKnowledgeDocumentUpload(documents: Command): void {
4459
.description('Upload a document to a knowledge base')
4560
.option('--name <name>', 'Store it under a different name')
4661
.option('--tag <value...>', 'Document tags, in tag1 through tag7 order')
47-
.option('--recipe <name>', 'Document processing recipe')
48-
.option('--lang <code>', 'Document language code')
62+
.addOption(new Option('--recipe <name>', 'Document processing recipe').choices(UPLOAD_RECIPES))
63+
.option('--lang <code>', LANGUAGE_TAG_HELP)
4964
.action(
5065
async (
5166
knowledgeBaseId: string,

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

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -454,7 +454,7 @@ describe('sim logs follow', () => {
454454
it('rejects a backlog count that is not a whole number of runs', async () => {
455455
respondWith([])
456456

457-
await expect(follow('-n', '-1')).rejects.toThrow('--lines must be a non-negative integer')
457+
await expect(follow('-n', '-1')).rejects.toThrow('--lines must be a whole number of 0 or more')
458458
expect(mockRequest).not.toHaveBeenCalled()
459459
})
460460

@@ -517,4 +517,27 @@ describe('sim logs follow', () => {
517517
expect(ms).toBeGreaterThan(0)
518518
}
519519
})
520+
/**
521+
* Root help states that a `wf_` prefix marks a FILE id and "never names a
522+
* workflow", so the example told the reader to pass a file id to
523+
* `--workflow`. Workflow ids are bare UUIDs.
524+
*/
525+
it('does not illustrate --workflow with a file-id prefix', () => {
526+
const root = new Command('sim').exitOverride()
527+
const logs = new Command('logs').exitOverride()
528+
root.addCommand(logs)
529+
attachLogsFollow(logs)
530+
// `helpInformation()` omits `addHelpText('after')`, which is where the
531+
// examples live, so the help is captured as the command would print it.
532+
let help = ''
533+
logs.commands[0].configureOutput({
534+
writeOut: (text) => {
535+
help += text
536+
},
537+
})
538+
logs.commands[0].outputHelp()
539+
540+
expect(help).not.toContain('wf_')
541+
expect(help).toMatch(/--workflow [0-9a-f]{8}-[0-9a-f]{4}-/)
542+
})
520543
})

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -467,7 +467,7 @@ function isTransient(error: unknown): boolean {
467467
function nonNegativeInteger(raw: string, flag: string): number {
468468
const value = Number(raw)
469469
if (!Number.isSafeInteger(value) || value < 0) {
470-
throw new SimApiError(`${flag} must be a non-negative integer`, 0)
470+
throw new SimApiError(`${flag} must be a whole number of 0 or more`, 0)
471471
}
472472
return value
473473
}
@@ -531,7 +531,7 @@ follow.
531531
532532
Examples:
533533
$ sim logs follow --level error
534-
$ sim logs follow --workflow wf_123 -n 0
534+
$ sim logs follow --workflow 00000000-0000-4000-8000-000000000000 -n 0
535535
$ sim --output json logs follow | jq -r '.runId'
536536
`
537537
)

packages/sim-cli/src/commands/protocol/resource-directory.test.ts

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,42 @@ describe('resource directory', () => {
216216
expect(entries.find((entry) => entry.kind === 'table')?.ref).toBe('tbl_1')
217217
})
218218

219+
/**
220+
* `files list` announces "showing the first N" off the surviving cursor and
221+
* `ls` printed the same capped answer with nothing on stderr, so one command
222+
* presented an incomplete listing as complete and its neighbour did not.
223+
*/
224+
it('says the combined listing was capped, as the contract-driven list does', async () => {
225+
mockRequest.mockImplementation(
226+
async (path: string, options: { query: { cursor?: string } }) => {
227+
if (path === '/api/v2/files/folders') return { data: [] }
228+
const cursor = Number(options.query.cursor ?? 0)
229+
return {
230+
data: Array.from({ length: 100 }, (_, index) => ({
231+
id: `file_${cursor}_${index}`,
232+
name: `file-${cursor}-${index}`,
233+
folderPath: '/',
234+
updatedAt: '2026-01-01T00:00:00.000Z',
235+
})),
236+
nextCursor: cursor < 2 ? String(cursor + 1) : null,
237+
}
238+
}
239+
)
240+
const written: string[] = []
241+
vi.spyOn(process.stderr, 'write').mockImplementation((chunk: string | Uint8Array) => {
242+
written.push(String(chunk))
243+
return true
244+
})
245+
vi.spyOn(console, 'log').mockImplementation(() => {})
246+
247+
await program().parseAsync(['node', 'sim', 'files', 'ls', '--limit', '5'])
248+
expect(written.join('')).toContain('showing the first 5')
249+
250+
written.length = 0
251+
await program().parseAsync(['node', 'sim', 'files', 'ls', '--limit', '0'])
252+
expect(written.join('')).not.toContain('showing the first')
253+
})
254+
219255
it('rejects extra directory arguments instead of silently ignoring them', async () => {
220256
await expect(
221257
program().parseAsync(['node', 'sim', 'file', 'ls', 'Reports', 'ignored'])

packages/sim-cli/src/commands/protocol/resource-directory.ts

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,11 @@ import {
1212
V2_OPERATIONS,
1313
type V2OperationName,
1414
} from '../../generated/v2-api'
15-
import { requestAllPages, SimApiError, type SimClient, type V2Page } from '../../http/client'
15+
import { requestPages, SimApiError, type SimClient, type V2Page } from '../../http/client'
1616
import { type Column, printList, text, timestamp } from '../../output/render'
1717
import { DEFAULT_LIMIT } from '../../runtime/options'
1818
import { encodeFolderPath } from '../../runtime/request'
19-
import { decodeFolderPath, renderResult } from '../../runtime/result'
19+
import { decodeFolderPath, renderResult, writeCursorTruncation } from '../../runtime/result'
2020

2121
type FolderListOperation =
2222
| 'listFileFolders'
@@ -103,17 +103,17 @@ async function listResources(
103103
folderPath: string,
104104
search: string | undefined,
105105
limit: number
106-
): Promise<DirectoryResource[]> {
106+
): Promise<{ items: DirectoryResource[]; truncated: boolean }> {
107107
const query = { workspaceId, folderPath, search, sortBy: 'name', sortOrder: 'asc' }
108108
const path = operationPath(config.resources)
109109
const paginated = 'cursor' in V2_OPERATIONS[config.resources].query
110110

111111
if (!paginated) {
112112
const page = await client.request<V2Page<DirectoryResource>>(path, { query })
113-
return page.data.slice(0, limit)
113+
return { items: page.data.slice(0, limit), truncated: page.data.length > limit }
114114
}
115115

116-
return requestAllPages<DirectoryResource>(client, path, {
116+
return requestPages<DirectoryResource>(client, path, {
117117
query,
118118
pageSize: DEFAULT_LIMIT,
119119
limit,
@@ -177,7 +177,7 @@ export function attachResourceDirectoryCommands(
177177
.action(async (path: string | undefined, options: ListOptions, command: Command) => {
178178
const rawLimit = Number(options.limit)
179179
if (!Number.isSafeInteger(rawLimit) || rawLimit < 0) {
180-
throw new SimApiError('--limit must be a non-negative integer', 0)
180+
throw new SimApiError('--limit must be a whole number of 0 or more (0 for everything)', 0)
181181
}
182182

183183
const limit = rawLimit === 0 ? Number.POSITIVE_INFINITY : rawLimit
@@ -191,8 +191,13 @@ export function attachResourceDirectoryCommands(
191191
listFolders(client, config.folders, workspaceId, folderPath, options.search),
192192
listResources(client, config, workspaceId, folderPath, options.search, limit),
193193
])
194-
const entries = entriesFor(config, folders, resources)
195-
printList(profile.output, entries.slice(0, limit), COLUMNS)
194+
const entries = entriesFor(config, folders, resources.items)
195+
const shown = entries.slice(0, limit)
196+
// Said here for the same reason the contract-driven `list` says it: the
197+
// combined listing is capped after the merge, so a full page of folders
198+
// can clip the resources even when the resource walk itself finished.
199+
writeCursorTruncation(shown.length, resources.truncated || entries.length > limit)
200+
printList(profile.output, shown, COLUMNS)
196201
})
197202

198203
group

0 commit comments

Comments
 (0)