mirror of
https://github.com/simstudioai/sim.git
synced 2026-08-31 01:11:53 +08:00
fix(cli): correct three descriptions the CLI publishes, and restore --no-recursive (#7093)
Follow-ups from review of the v0.8.12 release PR, all on surfaces the CLI audit touched. - `credentialId` was one shared schema across PATCH and DELETE, so the disconnect reference offered "update or disconnect" for an operation that cannot update. Split into two, matching the two components OpenAPI already publishes for them. - A boolean query param documents the spellings an HTTP caller may send and closes by calling them the whole accepted set. The CLI renders those fields as bare flags that take no value, leaving the sentence pointing at a list neither `--help` nor the reference ever prints. Stripped for bare flags only; the REST prose and OpenAPI specs are unchanged. - `files list --recursive` became a bare switch in the audit, which removed the only way to send false. The API turns it on by itself as soon as `--search` is set, so a folder search always descended. The twin is back, declared per flag so the one-way toggles do not grow a meaningless negation.
This commit is contained in:
Waleed
@@ -21,7 +21,7 @@ sim credentials delete <credentialId> [options]
|
||||
|
||||
| Argument | Required | Description |
|
||||
| --- | --- | --- |
|
||||
| `credentialId` | Yes | Credential to update or disconnect. |
|
||||
| `credentialId` | Yes | Credential to disconnect. |
|
||||
|
||||
</CommandTable>
|
||||
|
||||
@@ -84,7 +84,7 @@ sim credentials update <credentialId> [options]
|
||||
|
||||
| Argument | Required | Description |
|
||||
| --- | --- | --- |
|
||||
| `credentialId` | Yes | Credential to update or disconnect. |
|
||||
| `credentialId` | Yes | Credential to update. |
|
||||
|
||||
</CommandTable>
|
||||
|
||||
|
||||
@@ -256,7 +256,8 @@ sim files list [options]
|
||||
| Option | Required | Description |
|
||||
| --- | --- | --- |
|
||||
| `--folder <value>` | No | Folder path as shown in the app; the leading / is optional. |
|
||||
| `--recursive` | No | Whether the folder filter includes files in subfolders. Defaults to true when a search is set, false otherwise, so listing a folder shows that folder while searching one looks through everything in it. Ignored when no folder filter is set, which already spans the workspace. The listed spellings are the whole accepted vocabulary and are case-sensitive; any other value is rejected. |
|
||||
| `--recursive` | No | Whether the folder filter includes files in subfolders. Defaults to true when a search is set, false otherwise, so listing a folder shows that folder while searching one looks through everything in it. Ignored when no folder filter is set, which already spans the workspace. |
|
||||
| `--no-recursive` | No | Send --recursive as false. |
|
||||
| `--scope <value>` | No | Which lifecycle set to list: `active` (default) for live files, `archived` for files a `DELETE` soft-deleted. `folderPath` resolves against active folders only, so pairing it with `scope=archived` returns an empty page when the containing folder was archived too. Accepted values: `active`, `archived`. |
|
||||
| `--search <value>` | No | Case-insensitive substring match against the file name. |
|
||||
| `--sort-by <value>` | No | Field used to sort the result. Sorting by `name` is case-sensitive and follows the storage collation, so do not rely on a case-insensitive order. Accepted values: `name`, `size`, `uploadedAt`, `updatedAt`. |
|
||||
|
||||
@@ -379,7 +379,7 @@ sim credentials delete <credentialId> [options]
|
||||
|
||||
| Argument | Required | Description |
|
||||
| --- | --- | --- |
|
||||
| `credentialId` | Yes | Credential to update or disconnect. |
|
||||
| `credentialId` | Yes | Credential to disconnect. |
|
||||
|
||||
</CommandTable>
|
||||
|
||||
@@ -448,7 +448,7 @@ sim credentials update <credentialId> [options]
|
||||
|
||||
| Argument | Required | Description |
|
||||
| --- | --- | --- |
|
||||
| `credentialId` | Yes | Credential to update or disconnect. |
|
||||
| `credentialId` | Yes | Credential to update. |
|
||||
|
||||
</CommandTable>
|
||||
|
||||
@@ -950,7 +950,8 @@ sim files list [options]
|
||||
| Option | Required | Description |
|
||||
| --- | --- | --- |
|
||||
| `--folder <value>` | No | Folder path as shown in the app; the leading / is optional. |
|
||||
| `--recursive` | No | Whether the folder filter includes files in subfolders. Defaults to true when a search is set, false otherwise, so listing a folder shows that folder while searching one looks through everything in it. Ignored when no folder filter is set, which already spans the workspace. The listed spellings are the whole accepted vocabulary and are case-sensitive; any other value is rejected. |
|
||||
| `--recursive` | No | Whether the folder filter includes files in subfolders. Defaults to true when a search is set, false otherwise, so listing a folder shows that folder while searching one looks through everything in it. Ignored when no folder filter is set, which already spans the workspace. |
|
||||
| `--no-recursive` | No | Send --recursive as false. |
|
||||
| `--scope <value>` | No | Which lifecycle set to list: `active` (default) for live files, `archived` for files a `DELETE` soft-deleted. `folderPath` resolves against active folders only, so pairing it with `scope=archived` returns an empty page when the containing folder was archived too. Accepted values: `active`, `archived`. |
|
||||
| `--search <value>` | No | Case-insensitive substring match against the file name. |
|
||||
| `--sort-by <value>` | No | Field used to sort the result. Sorting by `name` is case-sensitive and follows the storage collation, so do not rely on a case-insensitive order. Accepted values: `name`, `size`, `uploadedAt`, `updatedAt`. |
|
||||
|
||||
@@ -2411,12 +2411,12 @@
|
||||
"name": "credentialId",
|
||||
"in": "path",
|
||||
"required": true,
|
||||
"description": "Credential to update or disconnect.",
|
||||
"description": "Credential to disconnect.",
|
||||
"schema": {
|
||||
"type": "string",
|
||||
"minLength": 1,
|
||||
"maxLength": 255,
|
||||
"description": "Credential to update or disconnect."
|
||||
"description": "Credential to disconnect."
|
||||
}
|
||||
},
|
||||
{
|
||||
@@ -2487,12 +2487,12 @@
|
||||
"name": "credentialId",
|
||||
"in": "path",
|
||||
"required": true,
|
||||
"description": "Credential to update or disconnect.",
|
||||
"description": "Credential to update.",
|
||||
"schema": {
|
||||
"type": "string",
|
||||
"minLength": 1,
|
||||
"maxLength": 255,
|
||||
"description": "Credential to update or disconnect."
|
||||
"description": "Credential to update."
|
||||
}
|
||||
},
|
||||
{
|
||||
|
||||
@@ -467,9 +467,24 @@ export const v2CreateServiceAccountCredentialContract = defineRouteContract({
|
||||
},
|
||||
})
|
||||
|
||||
export const v2CredentialParamsSchema = z
|
||||
/**
|
||||
* The credential a path addresses, named for what the route does to it.
|
||||
*
|
||||
* `PATCH` and `DELETE` sit on the same path but are not the same operation, and
|
||||
* the OpenAPI document already publishes them as two components
|
||||
* (`UpdateCredentialParams`, `DeleteCredentialParams`). One shared `describe()`
|
||||
* forced both to read "update or disconnect", so the disconnect reference
|
||||
* offered an update the route cannot perform.
|
||||
*/
|
||||
export const v2UpdateCredentialParamsSchema = z
|
||||
.object({
|
||||
credentialId: nonEmptyIdSchema.max(255).describe('Credential to update or disconnect.'),
|
||||
credentialId: nonEmptyIdSchema.max(255).describe('Credential to update.'),
|
||||
})
|
||||
.strict()
|
||||
|
||||
export const v2DeleteCredentialParamsSchema = z
|
||||
.object({
|
||||
credentialId: nonEmptyIdSchema.max(255).describe('Credential to disconnect.'),
|
||||
})
|
||||
.strict()
|
||||
|
||||
@@ -618,7 +633,7 @@ export type V2UpdateCredentialBody = z.input<typeof v2UpdateCredentialBodySchema
|
||||
export const v2UpdateCredentialContract = defineRouteContract({
|
||||
method: 'PATCH',
|
||||
path: '/api/v2/credentials/[credentialId]',
|
||||
params: v2CredentialParamsSchema,
|
||||
params: v2UpdateCredentialParamsSchema,
|
||||
query: v2UpdateCredentialQuerySchema,
|
||||
body: v2UpdateCredentialBodySchema,
|
||||
response: {
|
||||
@@ -630,7 +645,7 @@ export const v2UpdateCredentialContract = defineRouteContract({
|
||||
export const v2DeleteCredentialContract = defineRouteContract({
|
||||
method: 'DELETE',
|
||||
path: '/api/v2/credentials/[credentialId]',
|
||||
params: v2CredentialParamsSchema,
|
||||
params: v2DeleteCredentialParamsSchema,
|
||||
query: v2DeleteCredentialQuerySchema,
|
||||
response: {
|
||||
mode: 'json',
|
||||
|
||||
@@ -681,7 +681,10 @@ export const CLI_CONTRACT: CliContract = {
|
||||
// one place the terminal override was missed: `--recursive` alone read
|
||||
// as "argument missing" and only `--recursive yes` worked, on the flag
|
||||
// spelled as a bare switch everywhere else in the CLI.
|
||||
recursive: { boolean: true },
|
||||
// Unlike the folder deletes, this one is on unless told otherwise: the
|
||||
// API turns it on as soon as `--search` is set, so the negation is the
|
||||
// only way to search a folder without descending into it.
|
||||
recursive: { boolean: true, negatable: true },
|
||||
},
|
||||
columns: [
|
||||
{ header: 'id' },
|
||||
|
||||
@@ -88,12 +88,27 @@ export interface FlagSpec {
|
||||
/**
|
||||
* Expose a string-backed API boolean as a conventional terminal toggle.
|
||||
*
|
||||
* A toggle declared here carries no generated `--no-<name>` twin, because the
|
||||
* string union behind it has no agreed false spelling to send. That also
|
||||
* makes it the way to withhold the negation from a field the API declares as
|
||||
* `z.literal(true)`, where a sent `false` is a request the route rejects.
|
||||
* A toggle declared here carries no generated `--no-<name>` twin by default,
|
||||
* because sending false is usually either meaningless — the server already
|
||||
* defaults the field to false — or rejected outright, as on a field the API
|
||||
* declares as `z.literal(true)`. {@link negatable} asks for the twin back on
|
||||
* the one kind of field where false is a real request.
|
||||
*/
|
||||
boolean?: true
|
||||
/**
|
||||
* Give a {@link boolean} toggle its `--no-<name>` twin after all.
|
||||
*
|
||||
* Withholding the twin is right for a one-way switch: most string-backed
|
||||
* toggles sit on a field the server already defaults to false, so a negation
|
||||
* would only restate the default, and on a `z.literal(true)` field it would
|
||||
* send a request the route rejects. `files list --recursive` is neither — the
|
||||
* API turns it on by itself as soon as a search is set, so without a spelling
|
||||
* for false there is no way to search one folder without descending into it.
|
||||
* Declared per flag rather than derived from the union's false spellings,
|
||||
* which every one of these toggles publishes whether or not sending one means
|
||||
* anything.
|
||||
*/
|
||||
negatable?: true
|
||||
/**
|
||||
* This field carries a folder path, so percent-encode each of its segments.
|
||||
*
|
||||
|
||||
@@ -10104,7 +10104,7 @@ export const V2_OPERATIONS = {
|
||||
method: 'DELETE',
|
||||
path: '/api/v2/credentials/[credentialId]',
|
||||
pathParams: ['credentialId'] as const,
|
||||
pathParamDocs: { credentialId: 'Credential to update or disconnect.' },
|
||||
pathParamDocs: { credentialId: 'Credential to disconnect.' },
|
||||
responseMode: 'json',
|
||||
summary: 'Disconnect Credential',
|
||||
query: {
|
||||
@@ -13411,7 +13411,7 @@ export const V2_OPERATIONS = {
|
||||
method: 'PATCH',
|
||||
path: '/api/v2/credentials/[credentialId]',
|
||||
pathParams: ['credentialId'] as const,
|
||||
pathParamDocs: { credentialId: 'Credential to update or disconnect.' },
|
||||
pathParamDocs: { credentialId: 'Credential to update.' },
|
||||
responseMode: 'json',
|
||||
summary: 'Update Credential',
|
||||
query: {
|
||||
|
||||
@@ -87,3 +87,97 @@ describe('a body field the contract documents as cleared by null', () => {
|
||||
expect(updateHelp().match(/sends the word/g)).toHaveLength(1)
|
||||
})
|
||||
})
|
||||
|
||||
const LIST_FILES: OperationSpec = {
|
||||
method: 'GET',
|
||||
path: '/api/v2/files',
|
||||
pathParams: [],
|
||||
query: {
|
||||
recursive: {
|
||||
kind: 'boolean',
|
||||
describe:
|
||||
'Whether the folder filter includes files in subfolders. The listed spellings are the whole accepted vocabulary and are case-sensitive; any other value is rejected.',
|
||||
},
|
||||
folder: { kind: 'string', describe: 'Folder path as shown in the app.' },
|
||||
},
|
||||
}
|
||||
|
||||
function listFilesHelp(): string {
|
||||
const command = new Command('list')
|
||||
addOperationOptions(command, 'listFiles', {}, LIST_FILES)
|
||||
return command.helpInformation()
|
||||
}
|
||||
|
||||
describe('a boolean query param that documents its wire spellings', () => {
|
||||
/**
|
||||
* The API accepts twelve spellings for a boolean query param and says so. The
|
||||
* CLI renders the field as a bare `--recursive` with a `--no-recursive` twin,
|
||||
* neither of which takes a value, so the sentence pointed at a list the help
|
||||
* never prints — in `--help` and in the generated reference alike.
|
||||
*/
|
||||
it('drops the vocabulary sentence a bare flag cannot honour', () => {
|
||||
const help = listFilesHelp()
|
||||
|
||||
expect(help).toMatch(/--recursive\s+Whether the folder filter includes files in subfolders\./)
|
||||
expect(help).not.toMatch(/listed spellings/)
|
||||
expect(help).not.toMatch(/case-sensitive/)
|
||||
})
|
||||
|
||||
/**
|
||||
* Stripping the clause must not eat the sentence that carries the meaning, and
|
||||
* must not reach a flag that does take a value — those still publish their
|
||||
* vocabulary, because there the reader can act on it.
|
||||
*/
|
||||
it('keeps the rest of the prose, and leaves valued flags untouched', () => {
|
||||
const help = listFilesHelp()
|
||||
|
||||
expect(help).toMatch(/--folder <value>\s+Folder path as shown in the app\./)
|
||||
expect(help).toMatch(/includes files in subfolders\./)
|
||||
})
|
||||
})
|
||||
|
||||
const LIST_FILES_NEGATABLE: OperationSpec = {
|
||||
method: 'GET',
|
||||
path: '/api/v2/files',
|
||||
pathParams: [],
|
||||
query: {
|
||||
recursive: {
|
||||
kind: 'enum',
|
||||
values: ['true', 'false'] as const,
|
||||
describe: 'Whether the folder filter includes files in subfolders.',
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
function negatableOpts(argv: string[]): Record<string, unknown> {
|
||||
const command = new Command('list').exitOverride()
|
||||
addOperationOptions(
|
||||
command,
|
||||
'listFiles',
|
||||
{ flags: { recursive: { boolean: true, negatable: true } } },
|
||||
LIST_FILES_NEGATABLE
|
||||
)
|
||||
command.parse(argv, { from: 'user' })
|
||||
return command.opts()
|
||||
}
|
||||
|
||||
describe('a string-backed toggle the API defaults to true', () => {
|
||||
/**
|
||||
* `--recursive` alone had no way to say false, so a folder search always
|
||||
* descended. The twin restores it.
|
||||
*/
|
||||
it('offers both spellings, and each sends what it says', () => {
|
||||
expect(negatableOpts(['--recursive']).recursive).toBe(true)
|
||||
expect(negatableOpts(['--no-recursive']).recursive).toBe(false)
|
||||
})
|
||||
|
||||
/**
|
||||
* Commander gives a lone `--no-x` an implicit `true` default, which would
|
||||
* make every unqualified list send `recursive=true` and override the API's
|
||||
* own conditional default. Declaring the positive flag first suppresses it —
|
||||
* an ordering this asserts rather than trusts.
|
||||
*/
|
||||
it('leaves the field absent when neither spelling is given', () => {
|
||||
expect(negatableOpts([])).not.toHaveProperty('recursive')
|
||||
})
|
||||
})
|
||||
|
||||
@@ -51,6 +51,24 @@ function literalNullHint(documented: string, name: string): string {
|
||||
return /\bnull\b/i.test(documented) ? ` (--${name} null sends the word, not JSON null)` : ''
|
||||
}
|
||||
|
||||
/**
|
||||
* The wire's boolean vocabulary, which a bare flag cannot offer.
|
||||
*
|
||||
* A boolean query param documents the spellings an HTTP caller may send
|
||||
* (`true`, `1`, `yes`, `on`, …) and closes by saying the listed ones are the
|
||||
* whole accepted set. That is correct for the API and publishes unchanged in
|
||||
* the OpenAPI specs, but this CLI renders those fields as a bare `--flag` and
|
||||
* its `--no-flag` twin, neither of which takes a value — so the sentence points
|
||||
* at a list the reader is never shown, in `--help` and in the generated
|
||||
* reference alike. Dropping it here keeps the terminal honest without weakening
|
||||
* the prose REST callers actually need.
|
||||
*/
|
||||
const WIRE_VOCABULARY_SENTENCE = /\s*The listed spellings[^.]*\.\s*/g
|
||||
|
||||
function withoutWireVocabulary(documented: string): string {
|
||||
return documented.replace(WIRE_VOCABULARY_SENTENCE, ' ').trim()
|
||||
}
|
||||
|
||||
function addFieldOption(
|
||||
command: Command,
|
||||
operation: V2OperationName,
|
||||
@@ -78,21 +96,24 @@ function addFieldOption(
|
||||
const documented = describeField(flag, descriptor, name, field)
|
||||
|
||||
if (descriptor.kind === 'boolean' || flag.boolean) {
|
||||
const booleanDoc = withoutWireVocabulary(documented)
|
||||
if (descriptor.required) {
|
||||
command.addOption(
|
||||
new Option(`${short}--${name} <true|false>`, `${documented} (required)`)
|
||||
new Option(`${short}--${name} <true|false>`, `${booleanDoc} (required)`)
|
||||
.choices(['true', 'false'])
|
||||
.makeOptionMandatory()
|
||||
)
|
||||
return
|
||||
}
|
||||
|
||||
command.option(`${short}--${name}`, documented)
|
||||
command.option(`${short}--${name}`, booleanDoc)
|
||||
// The twin exists to send an explicit `false`. Restating the positive
|
||||
// flag's prose here inverts its meaning ("Return only deployed workflows"
|
||||
// on the flag that stops doing exactly that), so it names its counterpart
|
||||
// instead and lets the reader look up one description, not two.
|
||||
if (!flag.boolean) command.option(`--no-${name}`, `Send --${name} as false`)
|
||||
if (!flag.boolean || flag.negatable) {
|
||||
command.option(`--no-${name}`, `Send --${name} as false`)
|
||||
}
|
||||
return
|
||||
}
|
||||
|
||||
|
||||
@@ -9,6 +9,27 @@ import { buildRequest, coerce, type FieldSpec } from './request'
|
||||
const WORKSPACE = 'ws_local'
|
||||
|
||||
describe('buildRequest', () => {
|
||||
/**
|
||||
* `recursive` is the one string-backed toggle the API turns on by itself —
|
||||
* it defaults to true as soon as a search is set. Its `--no-` twin has to
|
||||
* reach the wire as an explicit false, or searching a single folder without
|
||||
* descending into it is unsayable from the terminal.
|
||||
*/
|
||||
it('sends an explicit false for a negated string-backed toggle', () => {
|
||||
const built = buildRequest(
|
||||
'listFiles',
|
||||
[],
|
||||
{ folderPath: '/Reports', search: 'q3', recursive: false },
|
||||
WORKSPACE
|
||||
)
|
||||
expect(built.query.recursive).toBe(false)
|
||||
})
|
||||
|
||||
it('sends true when the same toggle is set positively', () => {
|
||||
const built = buildRequest('listFiles', [], { recursive: true }, WORKSPACE)
|
||||
expect(built.query.recursive).toBe(true)
|
||||
})
|
||||
|
||||
it('substitutes path params from positional args and injects the workspace', () => {
|
||||
expect(buildRequest('upsertTableRow', ['tbl_1'], { data: '{"a":1}' }, WORKSPACE)).toEqual({
|
||||
path: '/api/v2/tables/tbl_1/rows/upsert',
|
||||
|
||||
Reference in New Issue
Block a user