Skip to content

Commit 678bdc4

Browse files
fix(cli): review round 4 — repeated flags encode per field kind
`coerce` comma-joined every `list` flag, but that is only correct for the three fields whose wire type is a `string` the route splits (`workflowIds`, `folderIds`, `triggers`). The others genuinely want an array: - `rowIds` and `selectedOutputs` are `array`, so joining sent a string where the schema expects a list — `sim tables rows batch-delete --row a b` failed validation, and so did a single `--row a` - `knowledgeBaseIds` is a string-or-array union whose array branch is the right one; joining made `kb_1,kb_2` a single bogus id, so multi-`--kb` search silently searched nothing `list` now means only "accept the flag more than once" — the encoding follows the field's kind, which the generator already records. The two questions were conflated under one contract field and the `FlagSpec` doc now says so. Four tests, three of which fail against the previous code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EsThUPqZXjwuuyRjBbmVkj
1 parent 9c0317d commit 678bdc4

3 files changed

Lines changed: 53 additions & 6 deletions

File tree

packages/sim-cli/src/contract/types.ts

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,16 @@ export interface FlagSpec {
3030
/** Short alias, e.g. `w` for `--workspace`. */
3131
short?: string
3232
/**
33-
* Accept a repeated flag and send it comma-joined. For fields the schema
34-
* types as `string` but the route splits — invisible to any type-driven
35-
* generator, so it has to be stated.
33+
* Accept the flag more than once.
34+
*
35+
* Only says that several values are allowed — how they reach the wire is
36+
* decided by the field's kind, not here. A `string` field is one the route
37+
* splits on commas (`workflowIds`), so the values are joined; anything else
38+
* genuinely wants an array (`rowIds`, `knowledgeBaseIds`). Conflating the two
39+
* turned multi-value `--kb` and `--row` into a single bogus value.
40+
*
41+
* Still needed on the string case because "this string is really a list" is
42+
* invisible to any type-driven generator.
3643
*/
3744
list?: boolean
3845
/** Take a JSON string. Implied for object/array/unknown fields. */

packages/sim-cli/src/runtime/request.test.ts

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,3 +110,33 @@ describe('deriveCommandPath', () => {
110110
expect(deriveCommandPath('searchKnowledge')).toEqual(['knowledge', 'search'])
111111
})
112112
})
113+
114+
describe('repeated flags encode per the field kind, not uniformly', () => {
115+
it('joins a string field the route splits', () => {
116+
const built = buildRequest('listLogs', [], { workflow: ['wf_1', 'wf_2'] }, WORKSPACE)
117+
expect(built.query.workflowIds).toBe('wf_1,wf_2')
118+
})
119+
120+
it('keeps an array field as an array', () => {
121+
// Joining these produced a string where the wire wants an array, so
122+
// `--row a b` failed validation — and so did a single `--row a`.
123+
const built = buildRequest('deleteTableRows', ['tbl_1'], { row: ['r1', 'r2'] }, WORKSPACE)
124+
expect(built.body?.rowIds).toEqual(['r1', 'r2'])
125+
})
126+
127+
it('keeps a single repeated value as a one-element array, not a bare string', () => {
128+
const built = buildRequest('deleteTableRows', ['tbl_1'], { row: ['r1'] }, WORKSPACE)
129+
expect(built.body?.rowIds).toEqual(['r1'])
130+
})
131+
132+
it('sends the array branch of a string-or-array union', () => {
133+
// `knowledgeBaseIds` accepts either; joining made "kb_1,kb_2" a single id.
134+
const built = buildRequest(
135+
'searchKnowledge',
136+
[],
137+
{ kb: ['kb_1', 'kb_2'], query: 'refunds' },
138+
WORKSPACE
139+
)
140+
expect(built.body?.knowledgeBaseIds).toEqual(['kb_1', 'kb_2'])
141+
})
142+
})

packages/sim-cli/src/runtime/request.ts

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,11 +47,21 @@ export function takesJson(field: FieldSpec, flag: FlagSpec): boolean {
4747
export function coerce(raw: unknown, field: FieldSpec, flag: FlagSpec, flagName: string): unknown {
4848
if (raw === undefined) return undefined
4949

50-
// A repeated flag whose wire form is one comma-joined string. The schema
51-
// types these as `string`, so only the contract knows.
50+
/**
51+
* A repeated flag. `list` says the CLI accepts several values; the *wire*
52+
* encoding follows the field's own kind, because the two are not the same
53+
* question:
54+
*
55+
* - `string` — the route splits on commas (`workflowIds`, `folderIds`,
56+
* `triggers`), so the values are joined.
57+
* - anything else — the wire genuinely wants an array (`rowIds`,
58+
* `selectedOutputs`) or a string-or-array union whose array branch is the
59+
* right one (`knowledgeBaseIds`). Joining those produced a single bogus id
60+
* or failed validation outright.
61+
*/
5262
if (flag.list) {
5363
const values = Array.isArray(raw) ? raw : [raw]
54-
return values.join(',')
64+
return field.kind === 'string' ? values.join(',') : values
5565
}
5666

5767
if (takesJson(field, flag)) {

0 commit comments

Comments
 (0)