From 7cdcf461d507e33443845331df8cc1481370c1ca Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 17 Aug 2026 15:32:40 -0700 Subject: [PATCH 1/3] fix(cli): resolve findings from a full command-surface audit Exercised all 147 commands against a live deployment. Fixes the defects that surfaced, plus the docs and generator drift they exposed. Transport - Stop following redirects. A bare domain that 301s to www silently converted POST to GET and dropped the body, so reads worked while every write failed with a misleading validation error and login returned 405. Both the client and the device flow now explain the redirect and name the endpoint to configure, rather than carrying credentials off-origin. - Report a non-JSON response as one instead of printing the HTML page. - Name the personal-API-key remedy on a workspace-key refusal, reading the machine-readable code the API actually sends. - Drop union-branch noise from validation errors that contradicted itself. - Show paging progress on stderr for multi-page fetches. Output - Clamp record values for table only. text is the format built for pipes, and it was truncating signed URLs and tool source mid-value. - Infer timestamp, duration, bytes and boolean formatting for API-owned keys so undeclared commands stop printing raw ISO and float ms. Skips user-defined table cells and leaves json/yaml on the raw payload. - Render a declared-but-absent field as an em dash; billing credits were vanishing silently. Paths, naming and validation - Percent-encode folder paths per segment and decode them for display, so a folder reads and types as the name shown in the app. - Reject a malformed endpoint where it is set and where it resolves, instead of crashing with a URL parse trace. - Request the detail level logs list's own columns need; its workflow column could never populate. - Rename three commands that described themselves wrongly and align two flags with their siblings. Old spellings still work: hidden, warned on stderr, and kept out of help and docs. - Verify whoami against the API, separating a bad key from an unreachable endpoint, and report the workspace by name. - Correct the --yes help text, which advertised skipping a prompt that does not exist. Docs - Teach the docs generator that a flag required by the runtime is required, and that hidden commands are not documented. --- .../content/docs/en/cli/authentication.mdx | 13 +- apps/docs/content/docs/en/cli/commands.mdx | 14 +- apps/docs/content/docs/en/cli/credentials.mdx | 2 +- .../docs/content/docs/en/cli/custom-tools.mdx | 2 +- apps/docs/content/docs/en/cli/files.mdx | 24 +- apps/docs/content/docs/en/cli/knowledge.mdx | 30 +- apps/docs/content/docs/en/cli/logs.mdx | 4 +- apps/docs/content/docs/en/cli/mcp-servers.mdx | 2 +- apps/docs/content/docs/en/cli/reference.mdx | 204 +++++----- apps/docs/content/docs/en/cli/scripting.mdx | 15 + apps/docs/content/docs/en/cli/secrets.mdx | 2 +- apps/docs/content/docs/en/cli/skills.mdx | 2 +- apps/docs/content/docs/en/cli/tables.mdx | 88 ++--- .../content/docs/en/cli/troubleshooting.mdx | 9 +- apps/docs/content/docs/en/cli/workflows.mdx | 26 +- packages/sim-cli/README.md | 13 +- packages/sim-cli/src/auth/device-flow.test.ts | 22 ++ packages/sim-cli/src/auth/device-flow.ts | 37 +- packages/sim-cli/src/commands/auth.test.ts | 160 ++++++-- packages/sim-cli/src/commands/auth.ts | 158 +++++++- .../sim-cli/src/commands/configure.test.ts | 52 +++ packages/sim-cli/src/commands/configure.ts | 5 +- .../commands/protocol/files-upload.test.ts | 25 ++ .../src/commands/protocol/files-upload.ts | 9 +- .../protocol/resource-directory.test.ts | 70 ++++ .../commands/protocol/resource-directory.ts | 22 +- .../commands/protocol/tables-import.test.ts | 21 ++ .../src/commands/protocol/tables-import.ts | 6 +- packages/sim-cli/src/config/profile.test.ts | 30 ++ packages/sim-cli/src/config/profile.ts | 34 +- .../sim-cli/src/contract/commands.test.ts | 178 +++++++++ packages/sim-cli/src/contract/commands.ts | 98 +++-- packages/sim-cli/src/contract/types.ts | 61 ++- packages/sim-cli/src/http/client.test.ts | 349 +++++++++++++++++- packages/sim-cli/src/http/client.ts | 264 ++++++++++++- packages/sim-cli/src/output/render.test.ts | 16 + packages/sim-cli/src/output/render.ts | 36 +- packages/sim-cli/src/runtime/build.test.ts | 159 +++++++- packages/sim-cli/src/runtime/build.ts | 64 +++- packages/sim-cli/src/runtime/execute.ts | 43 ++- packages/sim-cli/src/runtime/options.test.ts | 38 ++ packages/sim-cli/src/runtime/options.ts | 20 +- packages/sim-cli/src/runtime/renamed.ts | 41 ++ packages/sim-cli/src/runtime/request.test.ts | 91 ++++- packages/sim-cli/src/runtime/request.ts | 73 +++- packages/sim-cli/src/runtime/result.test.ts | 222 +++++++++++ packages/sim-cli/src/runtime/result.ts | 164 +++++++- scripts/generate-cli-docs.ts | 56 ++- 48 files changed, 2730 insertions(+), 344 deletions(-) create mode 100644 packages/sim-cli/src/commands/configure.test.ts create mode 100644 packages/sim-cli/src/contract/commands.test.ts create mode 100644 packages/sim-cli/src/runtime/options.test.ts create mode 100644 packages/sim-cli/src/runtime/renamed.ts create mode 100644 packages/sim-cli/src/runtime/result.test.ts diff --git a/apps/docs/content/docs/en/cli/authentication.mdx b/apps/docs/content/docs/en/cli/authentication.mdx index b913ddf7550..72ad91065b0 100644 --- a/apps/docs/content/docs/en/cli/authentication.mdx +++ b/apps/docs/content/docs/en/cli/authentication.mdx @@ -56,11 +56,18 @@ re-logging into an existing profile preselects the one already configured. ## Checking who you are ```bash -sim whoami +sim whoami # resolved settings, plus a live check that they work +sim whoami --no-verify # resolved settings only, no request ``` -Prints the resolved endpoint, workspace, output format, and account, and which -source each value came from. +Prints the resolved endpoint, workspace, and output format, and which source each +value came from, then reads the configured workspace to prove the key is accepted +and can reach it. + +It exits `0` when the check passes, `1` when the credentials are wrong, and `2` +when the check could not be made at all — no workspace to check against, or an +endpoint that did not answer. The split matters in CI: only `1` is fixed by +logging in again. ## Signing out diff --git a/apps/docs/content/docs/en/cli/commands.mdx b/apps/docs/content/docs/en/cli/commands.mdx index dab4a4295f2..cbe18f15bbf 100644 --- a/apps/docs/content/docs/en/cli/commands.mdx +++ b/apps/docs/content/docs/en/cli/commands.mdx @@ -78,12 +78,22 @@ sim logout [options] -## Show the resolved profile and where each setting came from +## Show the resolved profile, where each setting came from, and whether it works ```bash -sim whoami +sim whoami [options] ``` +**Options** + + + +| Option | Required | Description | +| --- | --- | --- | +| `--no-verify` | No | Skip the API check and only print the resolved settings. | + + + ## List the profiles defined in the config and credentials files ```bash diff --git a/apps/docs/content/docs/en/cli/credentials.mdx b/apps/docs/content/docs/en/cli/credentials.mdx index 534d284bf88..ff0e850e2c3 100644 --- a/apps/docs/content/docs/en/cli/credentials.mdx +++ b/apps/docs/content/docs/en/cli/credentials.mdx @@ -31,7 +31,7 @@ sim credentials delete [options] | Option | Required | Description | | --- | --- | --- | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | diff --git a/apps/docs/content/docs/en/cli/custom-tools.mdx b/apps/docs/content/docs/en/cli/custom-tools.mdx index 4830c239cc6..97e61c50269 100644 --- a/apps/docs/content/docs/en/cli/custom-tools.mdx +++ b/apps/docs/content/docs/en/cli/custom-tools.mdx @@ -49,7 +49,7 @@ sim custom-tools delete [options] | Option | Required | Description | | --- | --- | --- | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | diff --git a/apps/docs/content/docs/en/cli/files.mdx b/apps/docs/content/docs/en/cli/files.mdx index 43e106fac57..77cf0ca74c2 100644 --- a/apps/docs/content/docs/en/cli/files.mdx +++ b/apps/docs/content/docs/en/cli/files.mdx @@ -22,7 +22,7 @@ sim files batch-delete [options] | Option | Required | Description | | --- | --- | --- | | `--file-ids ` | Yes | File identifiers to update. (space-separated, or @path / @- with one value per line). | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -40,7 +40,7 @@ sim files create [options] | --- | --- | --- | | `--name ` | Yes | File name, including its extension. Path separators and dot segments are rejected. | | `--content-type ` | No | MIME type. When omitted, it is inferred from the file extension. | -| `--folder ` | No | Folder path; the leading / is optional. | +| `--folder ` | No | Folder path as shown in the app; the leading / is optional. | | `--content ` | No | Initial file content. Omit or send an empty string for a zero-byte file. The 70,000,000-character bound guards the JSON envelope; the decoded bytes must be at most 50 MiB, and a longer base64 payload is rejected with `413`. Use an upload session for anything larger. | | `--encoding ` | No | Encoding of the content field. Accepted values: `utf-8`, `base64`. | @@ -58,7 +58,7 @@ sim files folders create | Argument | Required | Description | | --- | --- | --- | -| `path` | Yes | Folder path; the leading / is optional | +| `path` | Yes | Folder path as shown in the app; the leading / is optional | @@ -74,7 +74,7 @@ sim files folders delete [options] | Argument | Required | Description | | --- | --- | --- | -| `path` | Yes | Folder path; the leading / is optional | +| `path` | Yes | Folder path as shown in the app; the leading / is optional | @@ -85,7 +85,7 @@ sim files folders delete [options] | Option | Required | Description | | --- | --- | --- | | `--recursive` | No | Delete the folder and its descendants. | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -124,8 +124,8 @@ Also available as `sim files folders mv`. | Argument | Required | Description | | --- | --- | --- | -| `path` | Yes | Folder path; the leading / is optional | -| `destination` | Yes | Folder path; the leading / is optional | +| `path` | Yes | Folder path as shown in the app; the leading / is optional | +| `destination` | Yes | Folder path as shown in the app; the leading / is optional | @@ -151,7 +151,7 @@ sim files delete [options] | Option | Required | Description | | --- | --- | --- | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -238,7 +238,7 @@ sim files list [options] | Option | Required | Description | | --- | --- | --- | -| `--folder ` | No | Folder path; the leading / is optional. | +| `--folder ` | No | Folder path as shown in the app; the leading / is optional. | | `--scope ` | 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 ` | No | Case-insensitive substring match against the file name. | | `--sort-by ` | 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`. | @@ -292,10 +292,10 @@ sim files rename [options] -## Restore file +## Restore an archived file ```bash -sim files restore create +sim files restore ``` **Arguments** @@ -357,7 +357,7 @@ sim files upload [options] | Option | Required | Description | | --- | --- | --- | -| `--folder ` | No | Destination folder path (defaults to /). | +| `--folder ` | No | Folder path as shown in the app; defaults to the root folder. | | `--name ` | No | Store it under a different name. | diff --git a/apps/docs/content/docs/en/cli/knowledge.mdx b/apps/docs/content/docs/en/cli/knowledge.mdx index 2333dc80edb..90f17671e7b 100644 --- a/apps/docs/content/docs/en/cli/knowledge.mdx +++ b/apps/docs/content/docs/en/cli/knowledge.mdx @@ -61,7 +61,7 @@ sim knowledge documents delete [options] | Option | Required | Description | | --- | --- | --- | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -116,7 +116,7 @@ sim knowledge documents list [options] ## Update document ```bash -sim knowledge documents update [options] +sim knowledge documents update [options] ``` **Arguments** @@ -125,7 +125,7 @@ sim knowledge documents update [options] | Argument | Required | Description | | --- | --- | --- | -| `id` | Yes | Unique knowledge base identifier. | +| `knowledgeBaseId` | Yes | Unique knowledge base identifier. | | `documentId` | Yes | Unique knowledge document identifier. | @@ -209,7 +209,7 @@ sim knowledge create [options] | `--name ` | Yes | Human-readable knowledge base name. | | `--description ` | No | Optional knowledge base description. | | `--chunking-config ` | No | Chunking configuration; defaults are applied when omitted. (JSON, or @path / @- to read a file or stdin). | -| `--folder ` | No | Folder path; the leading / is optional. | +| `--folder ` | No | Folder path as shown in the app; the leading / is optional. | @@ -225,7 +225,7 @@ sim knowledge folders create | Argument | Required | Description | | --- | --- | --- | -| `path` | Yes | Folder path; the leading / is optional | +| `path` | Yes | Folder path as shown in the app; the leading / is optional | @@ -241,7 +241,7 @@ sim knowledge folders delete [options] | Argument | Required | Description | | --- | --- | --- | -| `path` | Yes | Folder path; the leading / is optional | +| `path` | Yes | Folder path as shown in the app; the leading / is optional | @@ -252,7 +252,7 @@ sim knowledge folders delete [options] | Option | Required | Description | | --- | --- | --- | | `--recursive` | No | Delete the folder and its descendants. | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -291,8 +291,8 @@ Also available as `sim knowledge folders mv`. | Argument | Required | Description | | --- | --- | --- | -| `path` | Yes | Folder path; the leading / is optional | -| `destination` | Yes | Folder path; the leading / is optional | +| `path` | Yes | Folder path as shown in the app; the leading / is optional | +| `destination` | Yes | Folder path as shown in the app; the leading / is optional | @@ -318,7 +318,7 @@ sim knowledge delete [options] | Option | Required | Description | | --- | --- | --- | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -350,7 +350,7 @@ sim knowledge list [options] | Option | Required | Description | | --- | --- | --- | -| `--folder ` | No | Folder path; the leading / is optional. | +| `--folder ` | No | Folder path as shown in the app; the leading / is optional. | | `--search ` | No | Case-insensitive substring match against the resource name. | | `--sort-by ` | 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`, `createdAt`, `updatedAt`. | | `--sort-order ` | No | Sort direction. Accepted values: `asc`, `desc`. | @@ -361,7 +361,7 @@ sim knowledge list [options] ## List tags ```bash -sim knowledge tags list +sim knowledge tags list ``` **Arguments** @@ -370,7 +370,7 @@ sim knowledge tags list | Argument | Required | Description | | --- | --- | --- | -| `id` | Yes | Unique knowledge base identifier. | +| `knowledgeBaseId` | Yes | Unique knowledge base identifier. | @@ -423,7 +423,7 @@ sim knowledge update [options] | `--name ` | No | New knowledge base name. | | `--description ` | No | New knowledge base description. | | `--chunking-config ` | No | New document chunking configuration. (JSON, or @path / @- to read a file or stdin). | -| `--folder ` | No | Folder path; the leading / is optional. | +| `--folder ` | No | Folder path as shown in the app; the leading / is optional. | @@ -440,7 +440,7 @@ sim knowledge mv | Argument | Required | Description | | --- | --- | --- | | `id` | Yes | Unique knowledge base identifier. | -| `folder` | Yes | Folder path; the leading / is optional | +| `folder` | Yes | Folder path as shown in the app; the leading / is optional | diff --git a/apps/docs/content/docs/en/cli/logs.mdx b/apps/docs/content/docs/en/cli/logs.mdx index 1418a0226b0..d2c3420c550 100644 --- a/apps/docs/content/docs/en/cli/logs.mdx +++ b/apps/docs/content/docs/en/cli/logs.mdx @@ -57,12 +57,12 @@ sim logs list [options] | `--min-cost ` | No | Minimum execution cost in USD, from 0 to 1000000. A run is never charged a negative amount, so a negative bound is rejected rather than treated as a filter that matches every run. | | `--max-cost ` | No | Maximum execution cost in USD, from 0 to 1000000. A run is never charged a negative amount, so a negative bound is rejected rather than treated as a filter that matches every run. | | `--model ` | No | AI model used during execution. | -| `--details ` | No | Response detail level. Accepted values: `basic`, `full`. | +| `--details ` | No | Response detail level; full is requested by default to name each run’s workflow. Accepted values: `basic`, `full`. | | `--include-trace-spans` | No | Include trace spans in JSON or YAML output (implies full detail). | | `--include-final-output` | No | Include final output in JSON or YAML output (implies full detail). | | `--limit ` | No | Maximum items to return (0 for everything). Defaults to `100`. | | `--order ` | No | Sort direction by execution start time. This list is sortable only by execution start time, so it takes `order` in place of `sortBy`/`sortOrder`, which it rejects. Accepted values: `asc`, `desc`. | | `--run-id ` | No | Exact run identifier to match. | -| `--folder ` | No | Folder path; the leading / is optional (space-separated, or @path / @- with one value per line). | +| `--folder ` | No | Folder path as shown in the app; the leading / is optional (space-separated, or @path / @- with one value per line). | diff --git a/apps/docs/content/docs/en/cli/mcp-servers.mdx b/apps/docs/content/docs/en/cli/mcp-servers.mdx index ea6c97506f0..46afdcad279 100644 --- a/apps/docs/content/docs/en/cli/mcp-servers.mdx +++ b/apps/docs/content/docs/en/cli/mcp-servers.mdx @@ -58,7 +58,7 @@ sim mcp-servers delete [options] | Option | Required | Description | | --- | --- | --- | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | diff --git a/apps/docs/content/docs/en/cli/reference.mdx b/apps/docs/content/docs/en/cli/reference.mdx index cd24d1722fb..51250137abb 100644 --- a/apps/docs/content/docs/en/cli/reference.mdx +++ b/apps/docs/content/docs/en/cli/reference.mdx @@ -64,12 +64,22 @@ sim logout [options] ## sim whoami -Show the resolved profile and where each setting came from +Show the resolved profile, where each setting came from, and whether it works ```bash -sim whoami +sim whoami [options] ``` +**Options** + + + +| Option | Required | Description | +| --- | --- | --- | +| `--no-verify` | No | Skip the API check and only print the resolved settings. | + + + ## sim profiles List the profiles defined in the config and credentials files @@ -232,7 +242,7 @@ sim credentials delete [options] | Option | Required | Description | | --- | --- | --- | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -402,7 +412,7 @@ sim custom-tools delete [options] | Option | Required | Description | | --- | --- | --- | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -494,7 +504,7 @@ sim files batch-delete [options] | Option | Required | Description | | --- | --- | --- | | `--file-ids ` | Yes | File identifiers to update. (space-separated, or @path / @- with one value per line). | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -514,7 +524,7 @@ sim files create [options] | --- | --- | --- | | `--name ` | Yes | File name, including its extension. Path separators and dot segments are rejected. | | `--content-type ` | No | MIME type. When omitted, it is inferred from the file extension. | -| `--folder ` | No | Folder path; the leading / is optional. | +| `--folder ` | No | Folder path as shown in the app; the leading / is optional. | | `--content ` | No | Initial file content. Omit or send an empty string for a zero-byte file. The 70,000,000-character bound guards the JSON envelope; the decoded bytes must be at most 50 MiB, and a longer base64 payload is rejected with `413`. Use an upload session for anything larger. | | `--encoding ` | No | Encoding of the content field. Accepted values: `utf-8`, `base64`. | @@ -534,7 +544,7 @@ sim files folders create | Argument | Required | Description | | --- | --- | --- | -| `path` | Yes | Folder path; the leading / is optional | +| `path` | Yes | Folder path as shown in the app; the leading / is optional | @@ -552,7 +562,7 @@ sim files folders delete [options] | Argument | Required | Description | | --- | --- | --- | -| `path` | Yes | Folder path; the leading / is optional | +| `path` | Yes | Folder path as shown in the app; the leading / is optional | @@ -563,7 +573,7 @@ sim files folders delete [options] | Option | Required | Description | | --- | --- | --- | | `--recursive` | No | Delete the folder and its descendants. | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -606,8 +616,8 @@ Also available as `sim files folders mv`. | Argument | Required | Description | | --- | --- | --- | -| `path` | Yes | Folder path; the leading / is optional | -| `destination` | Yes | Folder path; the leading / is optional | +| `path` | Yes | Folder path as shown in the app; the leading / is optional | +| `destination` | Yes | Folder path as shown in the app; the leading / is optional | @@ -635,7 +645,7 @@ sim files delete [options] | Option | Required | Description | | --- | --- | --- | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -730,7 +740,7 @@ sim files list [options] | Option | Required | Description | | --- | --- | --- | -| `--folder ` | No | Folder path; the leading / is optional. | +| `--folder ` | No | Folder path as shown in the app; the leading / is optional. | | `--scope ` | 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 ` | No | Case-insensitive substring match against the file name. | | `--sort-by ` | 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`. | @@ -788,12 +798,12 @@ sim files rename [options] -### sim files restore create +### sim files restore -Restore File +Restore an archived file ```bash -sim files restore create +sim files restore ``` **Arguments** @@ -859,7 +869,7 @@ sim files upload [options] | Option | Required | Description | | --- | --- | --- | -| `--folder ` | No | Destination folder path (defaults to /). | +| `--folder ` | No | Folder path as shown in the app; defaults to the root folder. | | `--name ` | No | Store it under a different name. | @@ -1000,7 +1010,7 @@ sim knowledge documents delete [options] | Option | Required | Description | | --- | --- | --- | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -1061,7 +1071,7 @@ sim knowledge documents list [options] Update Document ```bash -sim knowledge documents update [options] +sim knowledge documents update [options] ``` **Arguments** @@ -1070,7 +1080,7 @@ sim knowledge documents update [options] | Argument | Required | Description | | --- | --- | --- | -| `id` | Yes | Unique knowledge base identifier. | +| `knowledgeBaseId` | Yes | Unique knowledge base identifier. | | `documentId` | Yes | Unique knowledge document identifier. | @@ -1158,7 +1168,7 @@ sim knowledge create [options] | `--name ` | Yes | Human-readable knowledge base name. | | `--description ` | No | Optional knowledge base description. | | `--chunking-config ` | No | Chunking configuration; defaults are applied when omitted. (JSON, or @path / @- to read a file or stdin). | -| `--folder ` | No | Folder path; the leading / is optional. | +| `--folder ` | No | Folder path as shown in the app; the leading / is optional. | @@ -1176,7 +1186,7 @@ sim knowledge folders create | Argument | Required | Description | | --- | --- | --- | -| `path` | Yes | Folder path; the leading / is optional | +| `path` | Yes | Folder path as shown in the app; the leading / is optional | @@ -1194,7 +1204,7 @@ sim knowledge folders delete [options] | Argument | Required | Description | | --- | --- | --- | -| `path` | Yes | Folder path; the leading / is optional | +| `path` | Yes | Folder path as shown in the app; the leading / is optional | @@ -1205,7 +1215,7 @@ sim knowledge folders delete [options] | Option | Required | Description | | --- | --- | --- | | `--recursive` | No | Delete the folder and its descendants. | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -1248,8 +1258,8 @@ Also available as `sim knowledge folders mv`. | Argument | Required | Description | | --- | --- | --- | -| `path` | Yes | Folder path; the leading / is optional | -| `destination` | Yes | Folder path; the leading / is optional | +| `path` | Yes | Folder path as shown in the app; the leading / is optional | +| `destination` | Yes | Folder path as shown in the app; the leading / is optional | @@ -1277,7 +1287,7 @@ sim knowledge delete [options] | Option | Required | Description | | --- | --- | --- | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -1313,7 +1323,7 @@ sim knowledge list [options] | Option | Required | Description | | --- | --- | --- | -| `--folder ` | No | Folder path; the leading / is optional. | +| `--folder ` | No | Folder path as shown in the app; the leading / is optional. | | `--search ` | No | Case-insensitive substring match against the resource name. | | `--sort-by ` | 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`, `createdAt`, `updatedAt`. | | `--sort-order ` | No | Sort direction. Accepted values: `asc`, `desc`. | @@ -1326,7 +1336,7 @@ sim knowledge list [options] List Tags ```bash -sim knowledge tags list +sim knowledge tags list ``` **Arguments** @@ -1335,7 +1345,7 @@ sim knowledge tags list | Argument | Required | Description | | --- | --- | --- | -| `id` | Yes | Unique knowledge base identifier. | +| `knowledgeBaseId` | Yes | Unique knowledge base identifier. | @@ -1392,7 +1402,7 @@ sim knowledge update [options] | `--name ` | No | New knowledge base name. | | `--description ` | No | New knowledge base description. | | `--chunking-config ` | No | New document chunking configuration. (JSON, or @path / @- to read a file or stdin). | -| `--folder ` | No | Folder path; the leading / is optional. | +| `--folder ` | No | Folder path as shown in the app; the leading / is optional. | @@ -1411,7 +1421,7 @@ sim knowledge mv | Argument | Required | Description | | --- | --- | --- | | `id` | Yes | Unique knowledge base identifier. | -| `folder` | Yes | Folder path; the leading / is optional | +| `folder` | Yes | Folder path as shown in the app; the leading / is optional | @@ -1518,13 +1528,13 @@ sim logs list [options] | `--min-cost ` | No | Minimum execution cost in USD, from 0 to 1000000. A run is never charged a negative amount, so a negative bound is rejected rather than treated as a filter that matches every run. | | `--max-cost ` | No | Maximum execution cost in USD, from 0 to 1000000. A run is never charged a negative amount, so a negative bound is rejected rather than treated as a filter that matches every run. | | `--model ` | No | AI model used during execution. | -| `--details ` | No | Response detail level. Accepted values: `basic`, `full`. | +| `--details ` | No | Response detail level; full is requested by default to name each run’s workflow. Accepted values: `basic`, `full`. | | `--include-trace-spans` | No | Include trace spans in JSON or YAML output (implies full detail). | | `--include-final-output` | No | Include final output in JSON or YAML output (implies full detail). | | `--limit ` | No | Maximum items to return (0 for everything). Defaults to `100`. | | `--order ` | No | Sort direction by execution start time. This list is sortable only by execution start time, so it takes `order` in place of `sortBy`/`sortOrder`, which it rejects. Accepted values: `asc`, `desc`. | | `--run-id ` | No | Exact run identifier to match. | -| `--folder ` | No | Folder path; the leading / is optional (space-separated, or @path / @- with one value per line). | +| `--folder ` | No | Folder path as shown in the app; the leading / is optional (space-separated, or @path / @- with one value per line). | @@ -1585,7 +1595,7 @@ sim mcp-servers delete [options] | Option | Required | Description | | --- | --- | --- | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -1725,7 +1735,7 @@ sim secrets delete [options] | Option | Required | Description | | --- | --- | --- | | `--scope ` | Yes | Whether the secret belongs to the workspace or to the caller. A personal secret belongs to the caller across every workspace, not to one workspace. Accepted values: `workspace`, `personal`. | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -1828,7 +1838,7 @@ sim skills delete [options] | Option | Required | Description | | --- | --- | --- | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -1958,7 +1968,7 @@ sim tables columns delete [options] | Option | Required | Description | | --- | --- | --- | | `--column-name ` | Yes | Name of the column to delete. | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -2080,7 +2090,7 @@ sim tables groups delete [options] | Option | Required | Description | | --- | --- | --- | | `--group-id ` | Yes | Workflow group to delete. | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -2307,7 +2317,7 @@ sim tables create [options] | `--name ` | Yes | Identifier: letters, numbers, and underscores; cannot start with a number. | | `--description ` | No | Optional table description. | | `--schema ` | Yes | Table schema: {"columns":[{"name":"email","type":"string"}]} (JSON, or @path / @- to read a file or stdin). | -| `--folder ` | No | Folder path; the leading / is optional. | +| `--folder ` | No | Folder path as shown in the app; the leading / is optional. | @@ -2325,7 +2335,7 @@ sim tables folders create | Argument | Required | Description | | --- | --- | --- | -| `path` | Yes | Folder path; the leading / is optional | +| `path` | Yes | Folder path as shown in the app; the leading / is optional | @@ -2343,7 +2353,7 @@ sim tables folders delete [options] | Argument | Required | Description | | --- | --- | --- | -| `path` | Yes | Folder path; the leading / is optional | +| `path` | Yes | Folder path as shown in the app; the leading / is optional | @@ -2354,7 +2364,7 @@ sim tables folders delete [options] | Option | Required | Description | | --- | --- | --- | | `--recursive` | No | Delete the folder and its descendants. | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -2397,8 +2407,8 @@ Also available as `sim tables folders mv`. | Argument | Required | Description | | --- | --- | --- | -| `path` | Yes | Folder path; the leading / is optional | -| `destination` | Yes | Folder path; the leading / is optional | +| `path` | Yes | Folder path as shown in the app; the leading / is optional | +| `destination` | Yes | Folder path as shown in the app; the leading / is optional | @@ -2456,7 +2466,7 @@ sim tables rows delete [options] | Option | Required | Description | | --- | --- | --- | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -2487,7 +2497,7 @@ sim tables rows batch-delete [options] | `--filter ` | No | Predicate: {"all":[{"field":"status","op":"eq","value":"active"}]}; groups use all/any. Operators: eq, ne, gt, gte, lt, lte, in, nin, contains, ncontains, startsWith, endsWith, like, ilike, nlike, nilike, isEmpty, isNotEmpty, isNull, isNotNull (JSON, or @path / @- to read a file or stdin). | | `--limit ` | No | Maximum items to return (0 for everything). Defaults to `100`. | | `--row ` | No | Explicit row identifiers to delete. (space-separated, or @path / @- with one value per line). | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -2515,7 +2525,7 @@ sim tables rows find [options] | Option | Required | Description | | --- | --- | --- | -| `--q ` | Yes | Value to find. | +| `--query ` | Yes | Value to find. | | `--filter ` | No | Predicate: {"all":[{"field":"status","op":"eq","value":"active"}]}; groups use all/any. Operators: eq, ne, gt, gte, lt, lte, in, nin, contains, ncontains, startsWith, endsWith, like, ilike, nlike, nilike, isEmpty, isNotEmpty, isNull, isNotNull (JSON, or @path / @- to read a file or stdin). | | `--sort ` | No | Ordered sort keys: [{"field":"createdAt","direction":"desc"}] (direction: asc or desc) (JSON, or @path / @- to read a file or stdin). | @@ -2598,6 +2608,34 @@ sim tables rows query [options] +### sim tables rows count + +Count rows matching a filter + +```bash +sim tables rows count [options] +``` + +**Arguments** + + + +| Argument | Required | Description | +| --- | --- | --- | +| `tableId` | Yes | Unique table identifier. | + + + +**Options** + + + +| Option | Required | Description | +| --- | --- | --- | +| `--filter ` | No | Predicate: {"all":[{"field":"status","op":"eq","value":"active"}]}; groups use all/any. Operators: eq, ne, gt, gte, lt, lte, in, nin, contains, ncontains, startsWith, endsWith, like, ilike, nlike, nilike, isEmpty, isNotEmpty, isNull, isNotNull (JSON, or @path / @- to read a file or stdin). | + + + ### sim tables rows enrich Run one row’s enrichment group @@ -2645,7 +2683,7 @@ sim tables rows batch-update [options] | `--filter ` | Yes | Predicate: {"all":[{"field":"status","op":"eq","value":"active"}]}; groups use all/any. Operators: eq, ne, gt, gte, lt, lte, in, nin, contains, ncontains, startsWith, endsWith, like, ilike, nlike, nilike, isEmpty, isNotEmpty, isNull, isNotNull (JSON, or @path / @- to read a file or stdin). | | `--data ` | Yes | Row-data patch applied to every matching row. (JSON, or @path / @- to read a file or stdin). | | `--limit ` | No | Maximum items to return (0 for everything). Defaults to `100`. | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -2732,7 +2770,7 @@ sim tables views delete [options] | Option | Required | Description | | --- | --- | --- | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -2830,7 +2868,7 @@ sim tables delete [options] | Option | Required | Description | | --- | --- | --- | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -2866,7 +2904,7 @@ sim tables list [options] | Option | Required | Description | | --- | --- | --- | -| `--folder ` | No | Folder path; the leading / is optional. | +| `--folder ` | No | Folder path as shown in the app; the leading / is optional. | | `--search ` | No | Case-insensitive substring match against the resource name. | | `--sort-by ` | 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`, `createdAt`, `updatedAt`. | | `--sort-order ` | No | Sort direction. Accepted values: `asc`, `desc`. | @@ -2874,34 +2912,6 @@ sim tables list [options] -### sim tables count create - -Count Rows - -```bash -sim tables count create [options] -``` - -**Arguments** - - - -| Argument | Required | Description | -| --- | --- | --- | -| `tableId` | Yes | Unique table identifier. | - - - -**Options** - - - -| Option | Required | Description | -| --- | --- | --- | -| `--predicate ` | No | Recursive predicate tree. Each group node is exactly one non-empty `all` or `any` array whose members are further groups or `{ field, op, value }` conditions; the root must be a group, not a bare condition. At most 100 members per group, 10 levels of nesting, and 500 nodes in total. The negating operators include nulls: `ne`, `nin`, `ncontains`, `nlike`, and `nilike` match rows whose column is null or absent, so "not X" is not the complement of "X" over a nullable column. That holds for every column type, multi-select included. To exclude nulls, `all`-combine the negation with `isNotEmpty` (multi-select) or `isNotNull`. Comparison: `eq`, `ne`, `gt`, `gte`, `lt`, `lte`. Membership: `in`, `nin` (array operand). Emptiness: `isEmpty`, `isNotEmpty`, `isNull`, `isNotNull` (no operand). Substring, always case-insensitive, operand matched literally: `contains`, `ncontains`, `startsWith`, `endsWith`. Pattern: `like`/`nlike` (case-sensitive), `ilike`/`nilike` (case-insensitive). **`*` is the only wildcard** and stands for any run of characters; `%`, `_`, and backslash match themselves. Use `like: "Hi*"`, not `like: "Hi%"`. A `select` column compares by option id and restricts its operators: single-select accepts `eq`, `ne`, `in`, `nin`; multi-select accepts `contains`, `ncontains`. Option names are accepted as operands and resolved to ids. (JSON, or @path / @- to read a file or stdin). | - - - ### sim tables update Update Table @@ -2928,7 +2938,7 @@ sim tables update [options] | --- | --- | --- | | `--name ` | No | Identifier: letters, numbers, and underscores; cannot start with a number. | | `--description ` | No | Replacement table description, or null to clear it. | -| `--folder ` | No | Folder path; the leading / is optional. | +| `--folder ` | No | Folder path as shown in the app; the leading / is optional. | @@ -2947,7 +2957,7 @@ sim tables mv | Argument | Required | Description | | --- | --- | --- | | `tableId` | Yes | Unique table identifier. | -| `folder` | Yes | Folder path; the leading / is optional | +| `folder` | Yes | Folder path as shown in the app; the leading / is optional | @@ -3007,7 +3017,7 @@ sim tables import [path] [options] | `--name ` | No | Identifier for the new table: letters, numbers, and underscores; defaults to the sanitized file name. | | `--table-id ` | No | Import into this existing table instead of creating one. | | `--mode ` | No | How to write into --table-id (default: append). Accepted values: `append`, `replace`. | -| `--folder ` | No | Folder path for the new table. | +| `--folder ` | No | Folder path for the new table, as shown in the app. | | `--file-id ` | No | Import a file already in the workspace instead of a local path. | | `--mapping ` | No | Column mapping (--table-id only). | | `--create-columns ` | No | Columns to create (--table-id only). | @@ -3195,7 +3205,7 @@ sim workflows create [options] | --- | --- | --- | | `--name ` | Yes | Workflow name. | | `--description ` | No | Optional workflow description. | -| `--folder ` | No | Folder path; the leading / is optional. | +| `--folder ` | No | Folder path as shown in the app; the leading / is optional. | @@ -3213,7 +3223,7 @@ sim workflows folders create | Argument | Required | Description | | --- | --- | --- | -| `path` | Yes | Folder path; the leading / is optional | +| `path` | Yes | Folder path as shown in the app; the leading / is optional | @@ -3231,7 +3241,7 @@ sim workflows folders delete [options] | Argument | Required | Description | | --- | --- | --- | -| `path` | Yes | Folder path; the leading / is optional | +| `path` | Yes | Folder path as shown in the app; the leading / is optional | @@ -3242,7 +3252,7 @@ sim workflows folders delete [options] | Option | Required | Description | | --- | --- | --- | | `--recursive` | No | Delete the folder and its descendants. | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -3285,8 +3295,8 @@ Also available as `sim workflows folders mv`. | Argument | Required | Description | | --- | --- | --- | -| `path` | Yes | Folder path; the leading / is optional | -| `destination` | Yes | Folder path; the leading / is optional | +| `path` | Yes | Folder path as shown in the app; the leading / is optional | +| `destination` | Yes | Folder path as shown in the app; the leading / is optional | @@ -3314,7 +3324,7 @@ sim workflows delete [options] | Option | Required | Description | | --- | --- | --- | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -3417,12 +3427,12 @@ sim workflows get -### sim workflows deployment list +### sim workflows deployment status -Get Workflow Deployment +Show a workflow’s current deployment ```bash -sim workflows deployment list +sim workflows deployment status ``` **Arguments** @@ -3497,7 +3507,7 @@ sim workflows import [options] | Option | Required | Description | | --- | --- | --- | | `--workflow ` | Yes | Workflow export object, bare workflow state, or JSON string containing either form. (JSON, or @path / @- to read a file or stdin). | -| `--folder ` | No | Folder path; the leading / is optional. | +| `--folder ` | No | Folder path as shown in the app; the leading / is optional. | | `--name ` | No | Override for the imported workflow name. | | `--description ` | No | Override for the imported workflow description. | @@ -3517,7 +3527,7 @@ sim workflows list [options] | Option | Required | Description | | --- | --- | --- | -| `--folder ` | No | Folder path; the leading / is optional. | +| `--folder ` | No | Folder path as shown in the app; the leading / is optional. | | `--deployed-only` | No | Return only workflows with an active deployment when true. | | `--no-deployed-only` | No | Send --deployed-only as false. | | `--limit ` | No | Maximum items to return (0 for everything). Defaults to `100`. | @@ -3599,7 +3609,7 @@ sim workflows update [options] | --- | --- | --- | | `--name ` | No | Replacement workflow name. | | `--description ` | No | Replacement workflow description; null clears it. | -| `--folder ` | No | Folder path; the leading / is optional. | +| `--folder ` | No | Folder path as shown in the app; the leading / is optional. | @@ -3618,7 +3628,7 @@ sim workflows mv | Argument | Required | Description | | --- | --- | --- | | `id` | Yes | Unique workflow identifier. | -| `folder` | Yes | Folder path; the leading / is optional | +| `folder` | Yes | Folder path as shown in the app; the leading / is optional | diff --git a/apps/docs/content/docs/en/cli/scripting.mdx b/apps/docs/content/docs/en/cli/scripting.mdx index 44627544323..32be39572b9 100644 --- a/apps/docs/content/docs/en/cli/scripting.mdx +++ b/apps/docs/content/docs/en/cli/scripting.mdx @@ -86,6 +86,7 @@ filter matching more rows than that silently affects only the first 100. Pass | --- | --- | | `0` | Success | | `1` | Anything else — API error, bad configuration, invalid arguments, or a missing `--yes` | +| `2` | `sim whoami` only: the check could not be made at all | Errors print one line to stderr, prefixed `Error:`, plus the API's error code and validation details when it supplies them. Failures are safe to branch on: @@ -100,6 +101,20 @@ fi An unexpected error prints a stack trace — that is a bug in the CLI, so please [open an issue](https://github.com/simstudioai/sim/issues). +`sim whoami` splits its failure in two because the fixes differ: `1` means the +credentials are wrong and a fresh `sim login` is the answer, while `2` means the +CLI never got a verdict — no workspace to check against, or an endpoint that did +not answer — and logging in again would not help. + +```bash +sim whoami > /dev/null +case $? in + 0) ;; # ready + 1) echo "run: sim login" >&2; exit 1 ;; + 2) echo "endpoint unreachable, retrying later" >&2; exit 75 ;; +esac +``` + ## Selecting workflow output `--select-output` takes `blockName.field` selectors. Fields that a run did not diff --git a/apps/docs/content/docs/en/cli/secrets.mdx b/apps/docs/content/docs/en/cli/secrets.mdx index 231a493adcd..b13dfb1751f 100644 --- a/apps/docs/content/docs/en/cli/secrets.mdx +++ b/apps/docs/content/docs/en/cli/secrets.mdx @@ -32,7 +32,7 @@ sim secrets delete [options] | Option | Required | Description | | --- | --- | --- | | `--scope ` | Yes | Whether the secret belongs to the workspace or to the caller. A personal secret belongs to the caller across every workspace, not to one workspace. Accepted values: `workspace`, `personal`. | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | diff --git a/apps/docs/content/docs/en/cli/skills.mdx b/apps/docs/content/docs/en/cli/skills.mdx index e5560dd276f..a965506ec1c 100644 --- a/apps/docs/content/docs/en/cli/skills.mdx +++ b/apps/docs/content/docs/en/cli/skills.mdx @@ -49,7 +49,7 @@ sim skills delete [options] | Option | Required | Description | | --- | --- | --- | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | diff --git a/apps/docs/content/docs/en/cli/tables.mdx b/apps/docs/content/docs/en/cli/tables.mdx index 281a03b44df..9f477ab9501 100644 --- a/apps/docs/content/docs/en/cli/tables.mdx +++ b/apps/docs/content/docs/en/cli/tables.mdx @@ -58,7 +58,7 @@ sim tables columns delete [options] | Option | Required | Description | | --- | --- | --- | | `--column-name ` | Yes | Name of the column to delete. | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -172,7 +172,7 @@ sim tables groups delete [options] | Option | Required | Description | | --- | --- | --- | | `--group-id ` | Yes | Workflow group to delete. | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -379,7 +379,7 @@ sim tables create [options] | `--name ` | Yes | Identifier: letters, numbers, and underscores; cannot start with a number. | | `--description ` | No | Optional table description. | | `--schema ` | Yes | Table schema: {"columns":[{"name":"email","type":"string"}]} (JSON, or @path / @- to read a file or stdin). | -| `--folder ` | No | Folder path; the leading / is optional. | +| `--folder ` | No | Folder path as shown in the app; the leading / is optional. | @@ -395,7 +395,7 @@ sim tables folders create | Argument | Required | Description | | --- | --- | --- | -| `path` | Yes | Folder path; the leading / is optional | +| `path` | Yes | Folder path as shown in the app; the leading / is optional | @@ -411,7 +411,7 @@ sim tables folders delete [options] | Argument | Required | Description | | --- | --- | --- | -| `path` | Yes | Folder path; the leading / is optional | +| `path` | Yes | Folder path as shown in the app; the leading / is optional | @@ -422,7 +422,7 @@ sim tables folders delete [options] | Option | Required | Description | | --- | --- | --- | | `--recursive` | No | Delete the folder and its descendants. | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -461,8 +461,8 @@ Also available as `sim tables folders mv`. | Argument | Required | Description | | --- | --- | --- | -| `path` | Yes | Folder path; the leading / is optional | -| `destination` | Yes | Folder path; the leading / is optional | +| `path` | Yes | Folder path as shown in the app; the leading / is optional | +| `destination` | Yes | Folder path as shown in the app; the leading / is optional | @@ -516,7 +516,7 @@ sim tables rows delete [options] | Option | Required | Description | | --- | --- | --- | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -545,7 +545,7 @@ sim tables rows batch-delete [options] | `--filter ` | No | Predicate: {"all":[{"field":"status","op":"eq","value":"active"}]}; groups use all/any. Operators: eq, ne, gt, gte, lt, lte, in, nin, contains, ncontains, startsWith, endsWith, like, ilike, nlike, nilike, isEmpty, isNotEmpty, isNull, isNotNull (JSON, or @path / @- to read a file or stdin). | | `--limit ` | No | Maximum items to return (0 for everything). Defaults to `100`. | | `--row ` | No | Explicit row identifiers to delete. (space-separated, or @path / @- with one value per line). | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -571,7 +571,7 @@ sim tables rows find [options] | Option | Required | Description | | --- | --- | --- | -| `--q ` | Yes | Value to find. | +| `--query ` | Yes | Value to find. | | `--filter ` | No | Predicate: {"all":[{"field":"status","op":"eq","value":"active"}]}; groups use all/any. Operators: eq, ne, gt, gte, lt, lte, in, nin, contains, ncontains, startsWith, endsWith, like, ilike, nlike, nilike, isEmpty, isNotEmpty, isNull, isNotNull (JSON, or @path / @- to read a file or stdin). | | `--sort ` | No | Ordered sort keys: [{"field":"createdAt","direction":"desc"}] (direction: asc or desc) (JSON, or @path / @- to read a file or stdin). | @@ -648,6 +648,32 @@ sim tables rows query [options] +## Count rows matching a filter + +```bash +sim tables rows count [options] +``` + +**Arguments** + + + +| Argument | Required | Description | +| --- | --- | --- | +| `tableId` | Yes | Unique table identifier. | + + + +**Options** + + + +| Option | Required | Description | +| --- | --- | --- | +| `--filter ` | No | Predicate: {"all":[{"field":"status","op":"eq","value":"active"}]}; groups use all/any. Operators: eq, ne, gt, gte, lt, lte, in, nin, contains, ncontains, startsWith, endsWith, like, ilike, nlike, nilike, isEmpty, isNotEmpty, isNull, isNotNull (JSON, or @path / @- to read a file or stdin). | + + + ## Run one row’s enrichment group ```bash @@ -691,7 +717,7 @@ sim tables rows batch-update [options] | `--filter ` | Yes | Predicate: {"all":[{"field":"status","op":"eq","value":"active"}]}; groups use all/any. Operators: eq, ne, gt, gte, lt, lte, in, nin, contains, ncontains, startsWith, endsWith, like, ilike, nlike, nilike, isEmpty, isNotEmpty, isNull, isNotNull (JSON, or @path / @- to read a file or stdin). | | `--data ` | Yes | Row-data patch applied to every matching row. (JSON, or @path / @- to read a file or stdin). | | `--limit ` | No | Maximum items to return (0 for everything). Defaults to `100`. | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -772,7 +798,7 @@ sim tables views delete [options] | Option | Required | Description | | --- | --- | --- | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -862,7 +888,7 @@ sim tables delete [options] | Option | Required | Description | | --- | --- | --- | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -894,7 +920,7 @@ sim tables list [options] | Option | Required | Description | | --- | --- | --- | -| `--folder ` | No | Folder path; the leading / is optional. | +| `--folder ` | No | Folder path as shown in the app; the leading / is optional. | | `--search ` | No | Case-insensitive substring match against the resource name. | | `--sort-by ` | 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`, `createdAt`, `updatedAt`. | | `--sort-order ` | No | Sort direction. Accepted values: `asc`, `desc`. | @@ -902,32 +928,6 @@ sim tables list [options] -## Count rows - -```bash -sim tables count create [options] -``` - -**Arguments** - - - -| Argument | Required | Description | -| --- | --- | --- | -| `tableId` | Yes | Unique table identifier. | - - - -**Options** - - - -| Option | Required | Description | -| --- | --- | --- | -| `--predicate ` | No | Recursive predicate tree. Each group node is exactly one non-empty `all` or `any` array whose members are further groups or `{ field, op, value }` conditions; the root must be a group, not a bare condition. At most 100 members per group, 10 levels of nesting, and 500 nodes in total. The negating operators include nulls: `ne`, `nin`, `ncontains`, `nlike`, and `nilike` match rows whose column is null or absent, so "not X" is not the complement of "X" over a nullable column. That holds for every column type, multi-select included. To exclude nulls, `all`-combine the negation with `isNotEmpty` (multi-select) or `isNotNull`. Comparison: `eq`, `ne`, `gt`, `gte`, `lt`, `lte`. Membership: `in`, `nin` (array operand). Emptiness: `isEmpty`, `isNotEmpty`, `isNull`, `isNotNull` (no operand). Substring, always case-insensitive, operand matched literally: `contains`, `ncontains`, `startsWith`, `endsWith`. Pattern: `like`/`nlike` (case-sensitive), `ilike`/`nilike` (case-insensitive). **`*` is the only wildcard** and stands for any run of characters; `%`, `_`, and backslash match themselves. Use `like: "Hi*"`, not `like: "Hi%"`. A `select` column compares by option id and restricts its operators: single-select accepts `eq`, `ne`, `in`, `nin`; multi-select accepts `contains`, `ncontains`. Option names are accepted as operands and resolved to ids. (JSON, or @path / @- to read a file or stdin). | - - - ## Update table ```bash @@ -952,7 +952,7 @@ sim tables update [options] | --- | --- | --- | | `--name ` | No | Identifier: letters, numbers, and underscores; cannot start with a number. | | `--description ` | No | Replacement table description, or null to clear it. | -| `--folder ` | No | Folder path; the leading / is optional. | +| `--folder ` | No | Folder path as shown in the app; the leading / is optional. | @@ -969,7 +969,7 @@ sim tables mv | Argument | Required | Description | | --- | --- | --- | | `tableId` | Yes | Unique table identifier. | -| `folder` | Yes | Folder path; the leading / is optional | +| `folder` | Yes | Folder path as shown in the app; the leading / is optional | @@ -1025,7 +1025,7 @@ sim tables import [path] [options] | `--name ` | No | Identifier for the new table: letters, numbers, and underscores; defaults to the sanitized file name. | | `--table-id ` | No | Import into this existing table instead of creating one. | | `--mode ` | No | How to write into --table-id (default: append). Accepted values: `append`, `replace`. | -| `--folder ` | No | Folder path for the new table. | +| `--folder ` | No | Folder path for the new table, as shown in the app. | | `--file-id ` | No | Import a file already in the workspace instead of a local path. | | `--mapping ` | No | Column mapping (--table-id only). | | `--create-columns ` | No | Columns to create (--table-id only). | diff --git a/apps/docs/content/docs/en/cli/troubleshooting.mdx b/apps/docs/content/docs/en/cli/troubleshooting.mdx index 7a8cd1441a5..4d3e2c59c73 100644 --- a/apps/docs/content/docs/en/cli/troubleshooting.mdx +++ b/apps/docs/content/docs/en/cli/troubleshooting.mdx @@ -3,11 +3,14 @@ title: Troubleshooting description: The failures whose cause is not obvious from the error message --- -Errors print one line to stderr, prefixed `Error:`, and exit `1`. Most say what -to do next; the cases below are the ones that do not. +Errors print one line to stderr, prefixed `Error:`, and exit `1` — except +`sim whoami`, which exits `2` when it could not reach the API to check at all. +Most say what to do next; the cases below are the ones that do not. Start with `sim whoami`. It prints the resolved endpoint, workspace, and output -format, **and where each came from** — which explains most surprises on its own. +format, **and where each came from** — which explains most surprises on its own — +then checks the resolved key against the API. Add `--no-verify` to skip the check +and stay offline. ## A command targets the wrong workspace or deployment diff --git a/apps/docs/content/docs/en/cli/workflows.mdx b/apps/docs/content/docs/en/cli/workflows.mdx index 23b387b8202..19ffd718bde 100644 --- a/apps/docs/content/docs/en/cli/workflows.mdx +++ b/apps/docs/content/docs/en/cli/workflows.mdx @@ -131,7 +131,7 @@ sim workflows create [options] | --- | --- | --- | | `--name ` | Yes | Workflow name. | | `--description ` | No | Optional workflow description. | -| `--folder ` | No | Folder path; the leading / is optional. | +| `--folder ` | No | Folder path as shown in the app; the leading / is optional. | @@ -147,7 +147,7 @@ sim workflows folders create | Argument | Required | Description | | --- | --- | --- | -| `path` | Yes | Folder path; the leading / is optional | +| `path` | Yes | Folder path as shown in the app; the leading / is optional | @@ -163,7 +163,7 @@ sim workflows folders delete [options] | Argument | Required | Description | | --- | --- | --- | -| `path` | Yes | Folder path; the leading / is optional | +| `path` | Yes | Folder path as shown in the app; the leading / is optional | @@ -174,7 +174,7 @@ sim workflows folders delete [options] | Option | Required | Description | | --- | --- | --- | | `--recursive` | No | Delete the folder and its descendants. | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -213,8 +213,8 @@ Also available as `sim workflows folders mv`. | Argument | Required | Description | | --- | --- | --- | -| `path` | Yes | Folder path; the leading / is optional | -| `destination` | Yes | Folder path; the leading / is optional | +| `path` | Yes | Folder path as shown in the app; the leading / is optional | +| `destination` | Yes | Folder path as shown in the app; the leading / is optional | @@ -240,7 +240,7 @@ sim workflows delete [options] | Option | Required | Description | | --- | --- | --- | -| `-y, --yes` | No | Skip the confirmation. | +| `-y, --yes` | Yes | Confirm this destructive operation. | @@ -335,10 +335,10 @@ sim workflows get -## Get workflow deployment +## Show a workflow’s current deployment ```bash -sim workflows deployment list +sim workflows deployment status ``` **Arguments** @@ -407,7 +407,7 @@ sim workflows import [options] | Option | Required | Description | | --- | --- | --- | | `--workflow ` | Yes | Workflow export object, bare workflow state, or JSON string containing either form. (JSON, or @path / @- to read a file or stdin). | -| `--folder ` | No | Folder path; the leading / is optional. | +| `--folder ` | No | Folder path as shown in the app; the leading / is optional. | | `--name ` | No | Override for the imported workflow name. | | `--description ` | No | Override for the imported workflow description. | @@ -425,7 +425,7 @@ sim workflows list [options] | Option | Required | Description | | --- | --- | --- | -| `--folder ` | No | Folder path; the leading / is optional. | +| `--folder ` | No | Folder path as shown in the app; the leading / is optional. | | `--deployed-only` | No | Return only workflows with an active deployment when true. | | `--no-deployed-only` | No | Send --deployed-only as false. | | `--limit ` | No | Maximum items to return (0 for everything). Defaults to `100`. | @@ -501,7 +501,7 @@ sim workflows update [options] | --- | --- | --- | | `--name ` | No | Replacement workflow name. | | `--description ` | No | Replacement workflow description; null clears it. | -| `--folder ` | No | Folder path; the leading / is optional. | +| `--folder ` | No | Folder path as shown in the app; the leading / is optional. | @@ -518,7 +518,7 @@ sim workflows mv | Argument | Required | Description | | --- | --- | --- | | `id` | Yes | Unique workflow identifier. | -| `folder` | Yes | Folder path; the leading / is optional | +| `folder` | Yes | Folder path as shown in the app; the leading / is optional | diff --git a/packages/sim-cli/README.md b/packages/sim-cli/README.md index 1b7682fdb15..dbbbbd1c9d8 100644 --- a/packages/sim-cli/README.md +++ b/packages/sim-cli/README.md @@ -46,7 +46,7 @@ The section-naming asymmetry — `[profile dev]` in config, `[dev]` in credentia sim configure --set-endpoint http://localhost:3000 --profile dev sim configure --set-workspace ws_local --profile dev sim profiles # list them; * marks the active one -sim whoami # resolved values, and where each came from +sim whoami # resolved values, where each came from, and whether they work ``` ## Where settings come from @@ -63,7 +63,16 @@ Each setting resolves independently, first match wins: Formats are listed under [Output formats](#output-formats). `sim whoami` prints the winning source per setting, which is usually the fastest -way to explain a surprising result. +way to explain a surprising result. It then reads the configured workspace to +prove the settings actually work; `--no-verify` skips that and stays offline. + +Its exit status is the answer, so CI can branch on it: + +| Code | Meaning | +| --- | --- | +| `0` | The key works and reached the configured workspace | +| `1` | The credentials are wrong — no key stored, or the API refused it | +| `2` | The check could not be made — nothing to check against, or the endpoint did not answer | For CI, skip `sim login` entirely and set `SIM_API_KEY` and `SIM_WORKSPACE` — nothing needs to touch the filesystem. `SIM_CONFIG_DIR` relocates both files if diff --git a/packages/sim-cli/src/auth/device-flow.test.ts b/packages/sim-cli/src/auth/device-flow.test.ts index 4b2747c53ce..6fa31cc3d80 100644 --- a/packages/sim-cli/src/auth/device-flow.test.ts +++ b/packages/sim-cli/src/auth/device-flow.test.ts @@ -96,6 +96,28 @@ describe('pollForKey', () => { it('gives up on a 403', async () => { await expect(poll([() => reply(403, { error: 'Forbidden' })])).rejects.toThrow('Forbidden') }) + + it('asks fetch not to follow a redirect', async () => { + // Following one rewrites this POST into a bodyless GET — which the route + // answers 405, a status nothing in the login chose — and hands `pollSecret` + // to whatever origin `Location` names. + const fetchMock = vi.spyOn(globalThis, 'fetch').mockResolvedValue(reply(200, COMPLETE)) + await pollForKey(ENDPOINT, createAuthRequest()) + + expect(fetchMock.mock.calls[0][1]).toMatchObject({ redirect: 'manual' }) + }) + + it('explains a redirected endpoint instead of failing on the method it became', async () => { + await expect( + poll([ + () => + new Response(null, { + status: 301, + headers: { location: 'https://www.sim.test/api/cli/auth/poll' }, + }), + ]) + ).rejects.toThrow(/redirected the login poll to https:\/\/www\.sim\.test/) + }) }) describe('createAuthRequest', () => { diff --git a/packages/sim-cli/src/auth/device-flow.ts b/packages/sim-cli/src/auth/device-flow.ts index 198f3610c30..5f2c5543f2c 100644 --- a/packages/sim-cli/src/auth/device-flow.ts +++ b/packages/sim-cli/src/auth/device-flow.ts @@ -1,6 +1,6 @@ import { createHash, randomBytes, randomInt } from 'node:crypto' import { sleep } from '../helpers' -import { SimApiError } from '../http/client' +import { REDIRECT_STATUSES, SimApiError } from '../http/client' /** * The terminal half of the CLI key handoff. @@ -105,6 +105,38 @@ interface PollResponse { workspaceBound?: boolean } +/** + * Explains a redirected poll rather than following it. + * + * The same policy `SimClient` applies, for the same two reasons and one more: + * a 301/302/303 rewrites this POST into a bodyless GET, which the route answers + * `405` — the login then fails naming a method nobody chose — and a redirect + * that IS followed hands `pollSecret`, the one redeemable value in the handoff, + * to whatever origin `Location` names. + */ +function toRedirectError(endpoint: string, response: Response): SimApiError { + const location = response.headers.get('location')?.trim() + let target: URL | null = null + if (location) { + try { + target = new URL(location, endpoint) + } catch { + target = null + } + } + + if (!target) { + return new SimApiError( + `${endpoint} answered the login poll with HTTP ${response.status} and no usable redirect target. Check the endpoint.`, + response.status + ) + } + return new SimApiError( + `${endpoint} redirected the login poll to ${target.origin}. The CLI does not follow redirects, because a redirect drops the request body and would carry the login secret to another origin. Re-run with --endpoint ${target.origin}, or run: sim configure --set-endpoint ${target.origin}`, + response.status + ) +} + /** * Polls until the user approves in the browser. * @@ -131,12 +163,15 @@ export async function pollForKey( headers: { 'content-type': 'application/json', accept: 'application/json' }, body: JSON.stringify({ request: auth.request, verifier: auth.pollSecret }), signal, + redirect: 'manual', }) } catch { response = null } if (response) { + if (REDIRECT_STATUSES.has(response.status)) throw toRedirectError(endpoint, response) + const raw = await response.text() if (!response.ok) { diff --git a/packages/sim-cli/src/commands/auth.test.ts b/packages/sim-cli/src/commands/auth.test.ts index a75fe6174ee..a7028c139f4 100644 --- a/packages/sim-cli/src/commands/auth.test.ts +++ b/packages/sim-cli/src/commands/auth.test.ts @@ -6,6 +6,7 @@ const mocks = vi.hoisted(() => ({ createAuthRequest: vi.fn(() => ({ pairing: 'ABCD', verifier: 'verifier' })), createInterface: vi.fn(), listProfiles: vi.fn<() => string[]>(() => []), + request: vi.fn(), readCredentialsProfile: vi.fn<() => Record>(() => ({})), pollForKey: vi.fn(async () => ({ apiKey: 'sim-key', @@ -44,8 +45,12 @@ vi.mock('../config/index', () => ({ writeConfigProfile: mocks.writeConfigProfile, writeCredentialsProfile: mocks.writeCredentialsProfile, })) -vi.mock('../context', () => ({ profileFrom: mocks.profileFrom })) +vi.mock('../context', () => ({ + profileFrom: mocks.profileFrom, + clientFrom: () => ({ client: { request: mocks.request }, profile: mocks.profileFrom() }), +})) +import { SimApiError } from '../http/client' import { loginCommand, profilesCommand, whoamiCommand } from './auth' const originalIsTTY = Object.getOwnPropertyDescriptor(process.stdin, 'isTTY') @@ -204,17 +209,14 @@ describe('profiles command', () => { }) describe('whoami command', () => { - beforeEach(() => { - vi.clearAllMocks() - vi.spyOn(console, 'log').mockImplementation(() => {}) - }) + const originalExitCode = process.exitCode - it('reports authentication without exposing any part of the API key', async () => { - mocks.profileFrom.mockReturnValue({ + function configured(overrides: Partial> = {}) { + return { name: 'default', endpoint: 'https://sim.ai', - apiKey: 'sim_super_secret_value', - workspaceId: 'ws_1', + apiKey: 'sim_super_secret_value' as string | null, + workspaceId: 'ws_1' as string | null, output: 'text', sources: { endpoint: 'default', @@ -222,8 +224,25 @@ describe('whoami command', () => { workspaceId: 'config', output: 'flag', }, + ...overrides, + } + } + + beforeEach(() => { + vi.clearAllMocks() + process.exitCode = undefined + mocks.profileFrom.mockReturnValue(configured()) + mocks.request.mockResolvedValue({ + data: { id: 'ws_1', name: "Waleed Latif's Workspace", memberCount: 3 }, }) + vi.spyOn(console, 'log').mockImplementation(() => {}) + }) + + afterEach(() => { + process.exitCode = originalExitCode + }) + it('reports authentication without exposing any part of the API key', async () => { await whoami() const output = vi.mocked(console.log).mock.calls.flat().join('\n') @@ -233,19 +252,7 @@ describe('whoami command', () => { }) it('uses non-secret-shaped authentication metadata in machine output', async () => { - mocks.profileFrom.mockReturnValue({ - name: 'default', - endpoint: 'https://sim.ai', - apiKey: 'sim_super_secret_value', - workspaceId: 'ws_1', - output: 'json', - sources: { - endpoint: 'default', - apiKey: 'credentials', - workspaceId: 'config', - output: 'flag', - }, - }) + mocks.profileFrom.mockReturnValue(configured({ output: 'json' })) await whoami() @@ -253,8 +260,117 @@ describe('whoami command', () => { expect(JSON.parse(output)).toMatchObject({ authenticated: true, sources: { authentication: 'credentials' }, + verification: { + status: 'verified', + workspace: { id: 'ws_1', name: "Waleed Latif's Workspace", memberCount: 3 }, + }, }) expect(output).not.toContain('apiKey') expect(output).not.toContain('sim_super_secret_value') }) + + it('checks the key against the API and names the workspace it reached', async () => { + await whoami() + + expect(mocks.request).toHaveBeenCalledWith('/api/v2/workspaces/ws_1', { method: 'GET' }) + const output = vi.mocked(console.log).mock.calls.flat().join('\n') + expect(output).toContain("Waleed Latif's Workspace") + expect(output).toContain('3 members') + expect(process.exitCode).toBeUndefined() + }) + + it('exits 1 when the API rejects the key, without hiding the resolved settings', async () => { + mocks.request.mockRejectedValue( + new SimApiError('Invalid API key — run: sim login --profile default', 401) + ) + + await whoami() + + const output = vi.mocked(console.log).mock.calls.flat().join('\n') + expect(output).toContain('Endpoint\thttps://sim.ai') + expect(output).toContain('Invalid API key') + expect(process.exitCode).toBe(1) + }) + + it('exits 2 rather than blaming the key when the endpoint cannot be reached', async () => { + mocks.request.mockRejectedValue( + new SimApiError('Could not reach https://sim.ai: fetch failed', 0) + ) + + await whoami() + + const output = vi.mocked(console.log).mock.calls.flat().join('\n') + expect(output).toContain('could not check — Could not reach https://sim.ai') + expect(process.exitCode).toBe(2) + }) + + it('exits 2 rather than blaming the key when the API itself is down', async () => { + // A 502 from a proxy mid-deploy said `✗ Bad Gateway` and exited 1, which + // tells a script to run `sim login` for something logging in cannot fix. + mocks.request.mockRejectedValue(new SimApiError('Bad Gateway', 502)) + + await whoami() + + const output = vi.mocked(console.log).mock.calls.flat().join('\n') + expect(output).toContain('could not check — Bad Gateway') + expect(process.exitCode).toBe(2) + }) + + it('exits 2 when the endpoint answers something other than the API', async () => { + // A wrong endpoint that serves a landing page comes back as a 200 the JSON + // client could not parse; the key was never judged. + mocks.request.mockRejectedValue( + new SimApiError('https://sim.ai/api/v2/workspaces/ws_1 returned HTML, not JSON', 200) + ) + + await whoami() + + expect(process.exitCode).toBe(2) + }) + + it('exits 1 when the key cannot reach the configured workspace', async () => { + mocks.request.mockRejectedValue(new SimApiError('Workspace not found', 404)) + + await whoami() + + const output = vi.mocked(console.log).mock.calls.flat().join('\n') + expect(output).toContain('Workspace not found') + expect(process.exitCode).toBe(1) + }) + + it('exits 2 when no workspace is configured, because the check reads one', async () => { + mocks.profileFrom.mockReturnValue( + configured({ workspaceId: null, sources: { ...configured().sources, workspaceId: 'unset' } }) + ) + + await whoami() + + expect(mocks.request).not.toHaveBeenCalled() + const output = vi.mocked(console.log).mock.calls.flat().join('\n') + expect(output).toContain('no workspace to check against') + expect(process.exitCode).toBe(2) + }) + + it('exits 1 when no key is configured', async () => { + mocks.profileFrom.mockReturnValue( + configured({ apiKey: null, sources: { ...configured().sources, apiKey: 'unset' } }) + ) + + await whoami() + + expect(mocks.request).not.toHaveBeenCalled() + expect(process.exitCode).toBe(1) + }) + + it('makes no request and stays offline under --no-verify', async () => { + mocks.profileFrom.mockReturnValue(configured({ output: 'json' })) + + await whoami('--no-verify') + + expect(mocks.request).not.toHaveBeenCalled() + expect(process.exitCode).toBeUndefined() + expect(JSON.parse(String(vi.mocked(console.log).mock.calls[0][0]))).toMatchObject({ + verification: { status: 'disabled', workspace: null }, + }) + }) }) diff --git a/packages/sim-cli/src/commands/auth.ts b/packages/sim-cli/src/commands/auth.ts index 242683ca644..71147ddf9b3 100644 --- a/packages/sim-cli/src/commands/auth.ts +++ b/packages/sim-cli/src/commands/auth.ts @@ -12,14 +12,16 @@ import { credentialsPath, deleteProfile, listProfiles, + type ResolvedProfile, readCredentialsProfile, type SettingSource, writeConfigProfile, writeCredentialsProfile, } from '../config/index' -import { profileFrom } from '../context' -import { SimApiError } from '../http/client' -import { printRecord } from '../output/render' +import { clientFrom, profileFrom } from '../context' +import { type GetWorkspaceResponse, V2_OPERATIONS } from '../generated/v2-api' +import { resolvePath, SimApiError, type SimClient } from '../http/client' +import { printRecord, safeOneLine } from '../output/render' /** * Best-effort browser launch. Failure is not an error: the URL is always printed @@ -203,14 +205,147 @@ export function logoutCommand(): Command { }) } +interface VerifiedWorkspace { + id: string + name: string + memberCount: number +} + +/** + * The outcome of checking the resolved settings against the API. + * + * Split by cause rather than into a boolean because each cause has a different + * fix, and `whoami` exists to name that fix: a rejected key needs a new login, a + * missing workspace needs `sim configure`, and an unreachable endpoint needs + * neither. + */ +type Verification = + | { status: 'verified'; workspace: VerifiedWorkspace; detail: null } + | { + status: 'rejected' | 'unreachable' | 'unauthenticated' | 'no-workspace' | 'disabled' + workspace: null + detail: string + } + +/** + * The only answers that are a verdict on the credentials themselves. + * + * 401 and 403 are the server judging the key; 404 means the configured + * workspace is not one this key can see. Everything else — a 502 from a proxy + * mid-deploy, a 429, a transport failure (status 0), an endpoint answering 200 + * with a login page — says nothing about the key, and calling it `rejected` + * told a user to run `sim login` for something logging in cannot fix. That is + * the flaky-VPN confusion the exit-code split exists to prevent. + */ +const CREDENTIAL_VERDICT_STATUSES = new Set([401, 403, 404]) + +/** + * `whoami` is the command people run to answer "am I set up correctly?", so the + * exit status has to carry that answer — reporting a junk key with exit 0 is the + * defect this mapping closes. + * + * 1 is the CLI's blanket "explained failure" code and means the credentials + * themselves are wrong. 2 is reserved for a check that could not be made at all: + * that is a different fix — retrying or setting a workspace helps, logging in + * again does not — and a script must be able to tell the two apart. + */ +const WHOAMI_EXIT_CODES = { + verified: 0, + disabled: 0, + unauthenticated: 1, + rejected: 1, + unreachable: 2, + 'no-workspace': 2, +} as const satisfies Record + +/** + * Confirms the resolved key really works, by reading the profile's own + * workspace. + * + * `getWorkspace` is the check because it is the cheapest read that proves all + * three settings at once — the endpoint answers, the key is accepted, and the + * key can reach the configured workspace — and because it comes back with the + * workspace's *name*, which is what tells a user the id they pasted is the + * workspace they meant. + * + * It is workspace-scoped, so a profile with no workspace has nothing to check + * against. That is reported rather than papered over with an account-scoped call + * a workspace-bound key would fail for reasons having nothing to do with its + * validity. + */ +async function verifyProfile( + client: Pick, + profile: ResolvedProfile +): Promise { + if (!profile.apiKey) { + return { + status: 'unauthenticated', + workspace: null, + detail: `no API key — run: sim login --profile ${profile.name}`, + } + } + if (!profile.workspaceId) { + return { + status: 'no-workspace', + workspace: null, + detail: `no workspace to check against — run: sim configure --profile ${profile.name} --set-workspace `, + } + } + + const operation = V2_OPERATIONS.getWorkspace + try { + const response = await client.request( + resolvePath(operation.path, { workspaceId: profile.workspaceId }), + { method: operation.method } + ) + const { id, name, memberCount } = response.data + // Projected field by field: the record carries display fields the machine + // output has no business inventing a contract for. + return { status: 'verified', workspace: { id, name, memberCount }, detail: null } + } catch (error) { + if (!(error instanceof SimApiError)) throw error + return { + status: CREDENTIAL_VERDICT_STATUSES.has(error.status) ? 'rejected' : 'unreachable', + workspace: null, + detail: error.message, + } + } +} + +function presentVerification(verification: Verification): string { + if (verification.status === 'verified') { + const { name, memberCount } = verification.workspace + const members = `${memberCount} ${memberCount === 1 ? 'member' : 'members'}` + // The name is server-supplied and lands in a terminal unescaped otherwise. + return `${chalk.green('✓')} ${safeOneLine(name)} · ${members}` + } + + const detail = safeOneLine(verification.detail) + switch (verification.status) { + case 'rejected': + return `${chalk.red('✗')} ${detail}` + case 'unauthenticated': + return chalk.yellow(`not logged in — ${detail}`) + case 'disabled': + return chalk.dim(detail) + default: + return chalk.yellow(`could not check — ${detail}`) + } +} + export function whoamiCommand(): Command { return new Command('whoami') - .description('Show the resolved profile and where each setting came from') - .action((_options: unknown, command: Command) => { - const profile = profileFrom(command) + .description('Show the resolved profile, where each setting came from, and whether it works') + .option('--no-verify', 'Skip the API check and only print the resolved settings') + .action(async (options: { verify: boolean }, command: Command) => { + const { client, profile } = clientFrom(command) const { sources } = profile const authentication = presentAuthentication(sources.apiKey) + const verification: Verification = options.verify + ? await verifyProfile(client, profile) + : { status: 'disabled', workspace: null, detail: 'not checked (--no-verify)' } + const annotate = (value: string, source: string) => source === 'unset' ? chalk.dim('not set') : `${value} ${chalk.dim(`(${source})`)}` @@ -227,6 +362,7 @@ export function whoamiCommand(): Command { ], ['Workspace', annotate(profile.workspaceId ?? '', sources.workspaceId)], ['Output', annotate(profile.output, sources.output)], + ['Verified', presentVerification(verification)], ], { profile: profile.name, @@ -240,8 +376,18 @@ export function whoamiCommand(): Command { workspaceId: sources.workspaceId, output: sources.output, }, + verification: { + status: verification.status, + workspace: verification.workspace, + detail: verification.detail, + }, } ) + + // Set rather than thrown: the resolved settings above are the answer the + // user came for, and a thrown error would replace them with one red line. + const exitCode = WHOAMI_EXIT_CODES[verification.status] + if (exitCode !== 0) process.exitCode = exitCode }) } diff --git a/packages/sim-cli/src/commands/configure.test.ts b/packages/sim-cli/src/commands/configure.test.ts new file mode 100644 index 00000000000..4a4bbb64080 --- /dev/null +++ b/packages/sim-cli/src/commands/configure.test.ts @@ -0,0 +1,52 @@ +import { mkdtempSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { Command } from 'commander' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { readConfigProfile } from '../config/index' +import { configureCommand } from './configure' + +vi.mock('../context', () => ({ + profileFrom: () => ({ name: 'default' }), +})) + +let dir: string + +function run(...args: string[]): Promise { + const root = new Command('sim').exitOverride() + root.addCommand(configureCommand()) + return root.parseAsync(['node', 'sim', 'configure', ...args]) +} + +beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'sim-cli-')) + process.env.SIM_CONFIG_DIR = dir + vi.spyOn(console, 'log').mockImplementation(() => {}) +}) + +afterEach(() => { + vi.restoreAllMocks() + rmSync(dir, { recursive: true, force: true }) + process.env.SIM_CONFIG_DIR = undefined +}) + +describe('configure --set-endpoint', () => { + it('refuses to store an endpoint that would later crash the URL parser', async () => { + await expect(run('--set-endpoint', 'not-a-url')).rejects.toThrow( + 'Invalid endpoint "not-a-url" from --set-endpoint. Use an absolute URL, e.g. https://sim.ai or http://localhost:3000' + ) + expect(readConfigProfile('default')).toEqual({}) + }) + + it('refuses a scheme the HTTP client cannot speak', async () => { + await expect(run('--set-endpoint', 'ftp://x.com')).rejects.toThrow( + 'Unsupported endpoint scheme "ftp" from --set-endpoint. Use http or https, e.g. https://sim.ai' + ) + expect(readConfigProfile('default')).toEqual({}) + }) + + it('stores a self-hosted endpoint with its trailing slashes stripped', async () => { + await run('--set-endpoint', 'http://localhost:3000//') + expect(readConfigProfile('default')).toMatchObject({ endpoint: 'http://localhost:3000' }) + }) +}) diff --git a/packages/sim-cli/src/commands/configure.ts b/packages/sim-cli/src/commands/configure.ts index 88879206b55..265cc0b4ecb 100644 --- a/packages/sim-cli/src/commands/configure.ts +++ b/packages/sim-cli/src/commands/configure.ts @@ -1,6 +1,7 @@ import chalk from 'chalk' import { Command } from 'commander' import { configPath, OUTPUT_FORMATS, readConfigProfile, writeConfigProfile } from '../config/index' +import { normalizeEndpoint } from '../config/profile' import { profileFrom } from '../context' import { SimApiError } from '../http/client' @@ -29,7 +30,9 @@ export function configureCommand(): Command { const profile = profileFrom(command) const updates: Record = {} - if (options.setEndpoint) updates.endpoint = options.setEndpoint.replace(/\/+$/, '') + if (options.setEndpoint) { + updates.endpoint = normalizeEndpoint(options.setEndpoint, '--set-endpoint') + } if (options.setWorkspace) updates.workspace = options.setWorkspace if (options.setOutput) { if (!(OUTPUT_FORMATS as readonly string[]).includes(options.setOutput)) { diff --git a/packages/sim-cli/src/commands/protocol/files-upload.test.ts b/packages/sim-cli/src/commands/protocol/files-upload.test.ts index c56ef989fd8..d11eeeb1c7f 100644 --- a/packages/sim-cli/src/commands/protocol/files-upload.test.ts +++ b/packages/sim-cli/src/commands/protocol/files-upload.test.ts @@ -139,4 +139,29 @@ describe('files upload', () => { }) expect(logged[0]).not.toContain('secret-token') }) + + it('encodes the destination folder, which the local path must never be', async () => { + // This command builds its own body, so it never reached the encoder every + // contract-driven `--folder` goes through: the same flag, the same value, + // accepted by `files list` and rejected here as non-canonical. + const path = join(dir, 'notes.txt') + writeFileSync(path, 'hello') + mockRequest + .mockResolvedValueOnce({ + data: { + session: { id: 'upload_1' }, + uploadToken: 'secret-token', + transfer: { method: 'put', url: 'https://storage.example/file', headers: {} }, + }, + }) + .mockResolvedValueOnce({ data: { file: { id: 'file_1' } } }) + vi.stubGlobal('fetch', vi.fn().mockResolvedValue(new Response(null, { status: 200 }))) + vi.spyOn(console, 'log').mockImplementation(() => {}) + + await program().parseAsync(['node', 'sim', 'file', 'upload', path, '--folder', '/Q1 (draft)']) + + expect(mockRequest.mock.calls[0][1].body).toMatchObject({ + folderPath: '/Q1%20%28draft%29', + }) + }) }) diff --git a/packages/sim-cli/src/commands/protocol/files-upload.ts b/packages/sim-cli/src/commands/protocol/files-upload.ts index c276b0b77d9..33a0a13209a 100644 --- a/packages/sim-cli/src/commands/protocol/files-upload.ts +++ b/packages/sim-cli/src/commands/protocol/files-upload.ts @@ -2,6 +2,7 @@ import type { Command } from 'commander' import { clientFrom } from '../../context' import type { CompleteFileUploadResponse, CreateFileUploadResponse } from '../../generated/v2-api' import { V2_OPERATIONS } from '../../generated/v2-api' +import { encodeFolderPath } from '../../runtime/request' import { contentTypeFor, localFile } from '../../transfer/local-file' import { finishUploadSession } from '../../transfer/upload-session' import { printProtocolResult } from './result' @@ -11,7 +12,7 @@ export function attachFileUpload(files: Command): void { .command('upload') .argument('', 'Local file to upload') .description('Upload a file to the workspace') - .option('--folder ', 'Destination folder path (defaults to /)') + .option('--folder ', 'Folder path as shown in the app; defaults to the root folder') .option('--name ', 'Store it under a different name') .action(async (path: string, options: { folder?: string; name?: string }, command: Command) => { const { client, profile } = clientFrom(command) @@ -27,7 +28,11 @@ export function attachFileUpload(files: Command): void { name, contentType: contentTypeFor(name), size, - ...(options.folder !== undefined ? { folderPath: options.folder } : {}), + // `` above is a LOCAL file and must stay untouched; only the + // destination folder is a wire-encoded API path. + ...(options.folder !== undefined + ? { folderPath: encodeFolderPath(options.folder) } + : {}), }, } ) diff --git a/packages/sim-cli/src/commands/protocol/resource-directory.test.ts b/packages/sim-cli/src/commands/protocol/resource-directory.test.ts index 1ef6d4b1d12..5f8558c2acb 100644 --- a/packages/sim-cli/src/commands/protocol/resource-directory.test.ts +++ b/packages/sim-cli/src/commands/protocol/resource-directory.test.ts @@ -144,6 +144,76 @@ describe('resource directory', () => { }) }) + it('encodes the path both commands take, as every contract-driven flag does', async () => { + // These two build their own request, so `buildRequest`'s encoding never ran + // for them: `--folder '/Folder 1'` worked while `ls '/Folder 1'` was + // rejected as non-canonical, and `mkdir` disagreed with the `folders + // create` the README calls its long form. + mockRequest.mockResolvedValue({ data: [], nextCursor: null }) + vi.spyOn(console, 'log').mockImplementation(() => {}) + + await program().parseAsync(['node', 'sim', 'table', 'ls', '/Q1 (draft)']) + expect(mockRequest).toHaveBeenCalledWith('/api/v2/tables/folders', { + query: expect.objectContaining({ parentPath: '/Q1%20%28draft%29' }), + }) + + mockRequest.mockReset() + mockRequest.mockResolvedValue({ data: { folder: {} } }) + await program().parseAsync(['node', 'sim', 'table', 'mkdir', '/Q1 (draft)']) + expect(mockRequest).toHaveBeenCalledWith('/api/v2/tables/folders', { + method: 'POST', + body: { workspaceId: 'ws_local', path: '/Q1%20%28draft%29' }, + }) + }) + + it('decodes folder paths for the human formats but leaves json on the wire form', async () => { + // `ls` builds its own columns, so the contract's `folder-path` display + // format never reached it: the sibling `folders list` printed `/Folder 2` + // while `ls` printed `/Folder%202` for the same folder, one column away + // from the decoded `name` it prints beside it. + mockRequest.mockImplementation(async (path: string) => { + if (path === '/api/v2/tables/folders') { + return { + data: [ + { + name: 'New folder', + path: '/Folder%202/New%20folder', + parentPath: '/Folder%202', + updatedAt: '2026-08-02T00:00:00.000Z', + }, + ], + nextCursor: null, + } + } + return { + data: [ + { + id: 'tbl_1', + name: 'Revenue', + folderPath: '/Folder%202', + updatedAt: '2026-08-03T00:00:00.000Z', + }, + ], + nextCursor: null, + } + }) + + const logged: string[] = [] + vi.spyOn(console, 'log').mockImplementation((line: string) => logged.push(line)) + + output.format = 'text' + await program().parseAsync(['node', 'sim', 'table', 'ls', '/Folder 2']) + expect(logged.join('\n')).toContain('/Folder 2/New folder') + expect(logged.join('\n')).not.toContain('%20') + + logged.length = 0 + output.format = 'json' + await program().parseAsync(['node', 'sim', 'table', 'ls', '/Folder 2']) + const entries = JSON.parse(logged[0]) as Array<{ kind: string; ref: string }> + expect(entries.find((entry) => entry.kind === 'folder')?.ref).toBe('/Folder%202/New%20folder') + expect(entries.find((entry) => entry.kind === 'table')?.ref).toBe('tbl_1') + }) + it('rejects extra directory arguments instead of silently ignoring them', async () => { await expect( program().parseAsync(['node', 'sim', 'file', 'ls', 'Reports', 'ignored']) diff --git a/packages/sim-cli/src/commands/protocol/resource-directory.ts b/packages/sim-cli/src/commands/protocol/resource-directory.ts index daf20cc6c88..58d93eeef20 100644 --- a/packages/sim-cli/src/commands/protocol/resource-directory.ts +++ b/packages/sim-cli/src/commands/protocol/resource-directory.ts @@ -15,7 +15,8 @@ import { import { requestAllPages, SimApiError, type SimClient, type V2Page } from '../../http/client' import { type Column, printList, text, timestamp } from '../../output/render' import { DEFAULT_LIMIT } from '../../runtime/options' -import { renderResult } from '../../runtime/result' +import { encodeFolderPath } from '../../runtime/request' +import { decodeFolderPath, renderResult } from '../../runtime/result' type FolderListOperation = | 'listFileFolders' @@ -74,11 +75,19 @@ interface ListOptions { limit: string } +/** + * A folder's `ref` is its path, so it decodes like one; a resource's `ref` is an + * opaque id and is shown as it arrived. Both stay pasteable into the next + * command because `encodeFolderPath` accepts either form. + */ const COLUMNS: Column[] = [ { header: 'kind', value: (entry) => text(entry.kind) }, { header: 'name', value: (entry) => text(entry.name) }, - { header: 'ref', value: (entry) => text(entry.ref) }, - { header: 'folder', value: (entry) => text(entry.folderPath) }, + { + header: 'ref', + value: (entry) => text(entry.kind === 'folder' ? decodeFolderPath(entry.ref) : entry.ref), + }, + { header: 'folder', value: (entry) => text(decodeFolderPath(entry.folderPath)) }, { header: 'updated', value: (entry) => timestamp(entry.updatedAt) }, ] @@ -170,7 +179,10 @@ export function attachResourceDirectoryCommands( } const limit = rawLimit === 0 ? Number.POSITIVE_INFINITY : rawLimit - const folderPath = path ?? '/' + // These commands build their own request, so the encoding `buildRequest` + // applies to every contract-driven folder flag has to be applied here too + // — otherwise `--folder '/Folder 1'` works and `ls '/Folder 1'` does not. + const folderPath = encodeFolderPath(path ?? '/') const { client, profile } = clientFrom(command) const workspaceId = client.requireWorkspace() const [folders, resources] = await Promise.all([ @@ -191,7 +203,7 @@ export function attachResourceDirectoryCommands( const operation = V2_OPERATIONS[config.createFolder] const result = await client.request<{ data?: unknown }>(operation.path, { method: operation.method, - body: { workspaceId: client.requireWorkspace(), path }, + body: { workspaceId: client.requireWorkspace(), path: encodeFolderPath(path) }, }) renderResult(config.createFolder, profile.output, result.data ?? result, {}) }) diff --git a/packages/sim-cli/src/commands/protocol/tables-import.test.ts b/packages/sim-cli/src/commands/protocol/tables-import.test.ts index b14cee1a0dd..9e35d380c11 100644 --- a/packages/sim-cli/src/commands/protocol/tables-import.test.ts +++ b/packages/sim-cli/src/commands/protocol/tables-import.test.ts @@ -127,4 +127,25 @@ describe('tables import output', () => { }) expect(logged[0]).not.toContain('uploadToken') }) + + it('encodes the destination folder the same way every other --folder is', async () => { + mockRequest.mockResolvedValue({ + data: { session: { id: 'import_1', status: 'queued' }, uploadToken: null, transfer: null }, + }) + vi.spyOn(console, 'log').mockImplementation(() => {}) + + await runImport([ + '--file-id', + 'f_1', + '--name', + 'Customers', + '--folder', + '/Q1 (draft)', + '--no-wait', + ]) + + expect(mockRequest.mock.calls[0][1].body.target).toMatchObject({ + folderPath: '/Q1%20%28draft%29', + }) + }) }) diff --git a/packages/sim-cli/src/commands/protocol/tables-import.ts b/packages/sim-cli/src/commands/protocol/tables-import.ts index 4d64f5b3ed2..b6d1ae47c41 100644 --- a/packages/sim-cli/src/commands/protocol/tables-import.ts +++ b/packages/sim-cli/src/commands/protocol/tables-import.ts @@ -9,7 +9,7 @@ import type { } from '../../generated/v2-api' import { V2_OPERATIONS } from '../../generated/v2-api' import { SimApiError, type SimClient } from '../../http/client' -import { coerce, type FieldSpec } from '../../runtime/request' +import { coerce, encodeFolderPath, type FieldSpec } from '../../runtime/request' import { contentTypeFor, localFile } from '../../transfer/local-file' import { finishUploadSession } from '../../transfer/upload-session' import { printProtocolResult } from './result' @@ -108,7 +108,7 @@ export function attachTableImport(tables: Command): void { 'How to write into --table-id (default: append)' ).choices(['append', 'replace']) ) - .option('--folder ', 'Folder path for the new table') + .option('--folder ', 'Folder path for the new table, as shown in the app') .option('--file-id ', 'Import a file already in the workspace instead of a local path') .option('--mapping ', 'Column mapping (--table-id only)') .option('--create-columns ', 'Columns to create (--table-id only)') @@ -144,7 +144,7 @@ export function attachTableImport(tables: Command): void { target = { type: 'new', name, - ...(options.folder !== undefined ? { folderPath: options.folder } : {}), + ...(options.folder !== undefined ? { folderPath: encodeFolderPath(options.folder) } : {}), } } diff --git a/packages/sim-cli/src/config/profile.test.ts b/packages/sim-cli/src/config/profile.test.ts index 225af15842b..0e9d13d3d87 100644 --- a/packages/sim-cli/src/config/profile.test.ts +++ b/packages/sim-cli/src/config/profile.test.ts @@ -96,6 +96,36 @@ describe('profile resolution', () => { expect(resolveProfile({ endpoint: 'https://sim.ai///' }).endpoint).toBe('https://sim.ai') }) + it('fails fast on an endpoint Node cannot parse, naming the source', () => { + expect(() => resolveProfile({ endpoint: 'not-a-url' })).toThrow( + 'Invalid endpoint "not-a-url" from flag. Use an absolute URL, e.g. https://sim.ai or http://localhost:3000' + ) + + process.env.SIM_ENDPOINT = 'not-a-url' + expect(() => resolveProfile()).toThrow('Invalid endpoint "not-a-url" from env.') + + Reflect.deleteProperty(process.env, 'SIM_ENDPOINT') + writeConfigProfile('default', { endpoint: 'not-a-url' }) + expect(() => resolveProfile()).toThrow('Invalid endpoint "not-a-url" from config.') + }) + + it('rejects a parseable endpoint the HTTP client could never call', () => { + expect(() => resolveProfile({ endpoint: 'ftp://x.com' })).toThrow( + 'Unsupported endpoint scheme "ftp" from flag. Use http or https, e.g. https://sim.ai' + ) + }) + + it('accepts every endpoint shape a self-hosted install needs', () => { + for (const endpoint of [ + 'http://localhost:3000', + 'https://10.0.0.7:8443', + 'https://sim.internal:8080/sim', + 'http://127.0.0.1:3000/', + ]) { + expect(resolveProfile({ endpoint }).endpoint).toBe(endpoint.replace(/\/+$/, '')) + } + }) + it('fails fast on an unrecognized active output format', () => { process.env.SIM_OUTPUT = 'xml' expect(() => resolveProfile()).toThrow( diff --git a/packages/sim-cli/src/config/profile.ts b/packages/sim-cli/src/config/profile.ts index c770cc2aae9..8de5402d580 100644 --- a/packages/sim-cli/src/config/profile.ts +++ b/packages/sim-cli/src/config/profile.ts @@ -130,10 +130,38 @@ export function deleteProfile(profile: string): { config: boolean; credentials: return { config, credentials } } -function normalizeEndpoint(endpoint: string): string { +/** + * Validates an endpoint and strips its trailing slashes. + * + * The check has to live here rather than at the call sites because an endpoint + * reaches the HTTP client from four directions — `--endpoint`, `SIM_ENDPOINT`, + * `configure --set-endpoint`, and a hand-edited `~/.sim/config` — and an + * unparseable one escapes as a raw `TypeError: Invalid URL` stack trace from + * inside Node's URL parser instead of a CLI error. + * + * `source` names where the value came from, so the message points at the thing + * the user has to edit. + */ +export function normalizeEndpoint(endpoint: string, source: string): string { // A trailing slash here produces `https://sim.ai//api/v2/...`, which some // proxies 404 rather than normalize. - return endpoint.replace(/\/+$/, '') + const trimmed = endpoint.replace(/\/+$/, '') + + let parsed: URL + try { + parsed = new URL(trimmed) + } catch { + throw new ProfileConfigError( + `Invalid endpoint "${endpoint}" from ${source}. Use an absolute URL, e.g. ${DEFAULT_ENDPOINT} or http://localhost:3000` + ) + } + if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') { + throw new ProfileConfigError( + `Unsupported endpoint scheme "${parsed.protocol.replace(/:$/, '')}" from ${source}. Use http or https, e.g. ${DEFAULT_ENDPOINT}` + ) + } + + return trimmed } /** @@ -205,7 +233,7 @@ export function resolveProfile(overrides: ProfileOverrides = {}): ResolvedProfil return { name, - endpoint: normalizeEndpoint(endpoint.value as string), + endpoint: normalizeEndpoint(endpoint.value as string, endpoint.source), apiKey: apiKey.value, workspaceId: workspaceId.value, output: output.value as OutputFormat, diff --git a/packages/sim-cli/src/contract/commands.test.ts b/packages/sim-cli/src/contract/commands.test.ts new file mode 100644 index 00000000000..c537ecbaee3 --- /dev/null +++ b/packages/sim-cli/src/contract/commands.test.ts @@ -0,0 +1,178 @@ +/** + * @vitest-environment node + */ +import type { Command } from 'commander' +import { describe, expect, it } from 'vitest' +import { V2_OPERATIONS, type V2OperationName } from '../generated/v2-api' +import { buildGeneratedCommands } from '../runtime/build' +import { flagNameFor, flagSpecFor } from '../runtime/request' +import type { OperationSpec } from '../runtime/types' +import { CLI_CONTRACT } from './commands' + +/** Every leaf command's full path, `tables rows count` style. */ +function leafPaths(options: { includeHidden?: boolean } = {}): string[] { + const paths: string[] = [] + const isHidden = (command: Command) => + (command as Command & { _hidden?: boolean })._hidden === true + const walk = (command: Command, prefix: string[]): void => { + const path = [...prefix, command.name()] + const children = options.includeHidden + ? command.commands + : command.commands.filter((child) => !isHidden(child)) + if (children.length === 0) { + paths.push(path.join(' ')) + return + } + for (const child of children) walk(child, path) + } + for (const group of buildGeneratedCommands()) walk(group, []) + return paths +} + +function commandAt(...names: string[]): Command { + let current: Command | undefined + let candidates: readonly Command[] = buildGeneratedCommands() + for (const name of names) { + current = candidates.find((command) => command.name() === name) + if (!current) throw new Error(`Missing command ${names.join(' ')}`) + candidates = current.commands + } + if (!current) throw new Error('No command requested') + return current +} + +describe('the command tree', () => { + it('registers every command name exactly once', () => { + // Commander resolves a duplicate name to the first registered match, so a + // collision does not fail loudly — the shadowed command's flags simply + // become unreachable, which is how the bulk document update once hid the + // single-document one. + const paths = leafPaths() + expect(paths.length).toBe(new Set(paths).size) + }) + + it('names each renamed command after what it does', () => { + // The retired path still resolves, so a script written before the rename + // keeps working; it is simply hidden, so nothing teaches it any more. Both + // halves matter: dropping it breaks callers, surfacing it undoes the rename. + const visible = leafPaths() + const all = leafPaths({ includeHidden: true }) + + for (const [current, retired] of [ + ['tables rows count', 'tables count create'], + ['files restore', 'files restore create'], + ['workflows deployment status', 'workflows deployment list'], + ]) { + expect(visible).toContain(current) + expect(visible).not.toContain(retired) + expect(all).toContain(retired) + } + }) + + it('spells one concept with one flag name across the contract', () => { + // `predicate` was `--filter` on two row commands and `--predicate` on the + // third, and the same idea was `--q` here and `--query` on knowledge search. + const flagsByField = new Map>() + for (const operation of Object.keys(V2_OPERATIONS) as V2OperationName[]) { + if (CLI_CONTRACT[operation]?.hidden) continue + const spec = V2_OPERATIONS[operation] as OperationSpec + for (const slot of ['query', 'body'] as const) { + for (const field of Object.keys(spec[slot] ?? {})) { + if (flagSpecFor(operation, field).omit) continue + const names = flagsByField.get(field) ?? new Set() + names.add(flagNameFor(operation, field)) + flagsByField.set(field, names) + } + } + } + + const divergent = [...flagsByField] + .filter(([, names]) => names.size > 1) + .map(([field, names]) => `${field}: ${[...names].sort().join(', ')}`) + + // `rowIds` is the one field still spelled two ways: `tables rows + // batch-delete` deliberately takes a singular repeated `--row`. + expect(divergent).toEqual(['rowIds: row, row-ids']) + }) +}) + +describe('renamed commands keep their surface', () => { + it('documents the filter operators on the row count', () => { + const help = commandAt('tables', 'rows', 'count').helpInformation() + expect(help).toContain('--filter ') + expect(help).toContain('{"all":[{"field":"status","op":"eq","value":"active"}]}') + expect(help).not.toContain('--predicate') + }) + + it('asks for a row search the same way knowledge search does', () => { + const help = commandAt('tables', 'rows', 'find').helpInformation() + expect(help).toContain('--query ') + expect(help).not.toMatch(/--q\b/) + }) + + it('names the parent knowledge base on every document command', () => { + for (const verb of ['get', 'update', 'delete', 'batch-update']) { + expect(commandAt('knowledge', 'documents', verb).helpInformation()).toContain( + '' + ) + } + expect(commandAt('knowledge', 'tags', 'list').helpInformation()).toContain('') + }) +}) + +/** + * Field names the v2 contract uses for a folder path. + * + * Only ever consulted here, to prove the contract marks all of them: the CLI + * itself drives off the explicit `folderPath` marker, because `path` on its + * own is also a LOCAL file on the upload commands. + */ +const FOLDER_PATH_FIELDS = new Set([ + 'folderPath', + 'folderPaths', + 'parentPath', + 'destinationPath', + 'targetFolderPath', + 'path', +]) + +describe('folder-path fields', () => { + it('marks every one of them for encoding', () => { + // One missed field is one command where the visible folder name is still + // rejected, and nothing about the failure would point back here. + const unmarked: string[] = [] + let checked = 0 + for (const operation of Object.keys(V2_OPERATIONS) as V2OperationName[]) { + const spec = V2_OPERATIONS[operation] as OperationSpec + // A hidden operation never reaches `buildRequest`; the bespoke command + // driving it (`files upload`) builds its own body and calls the encoder + // itself, so a marker here would claim an encoding this path never runs. + // That call is covered by the command's own test. + if (CLI_CONTRACT[operation]?.hidden) continue + for (const slot of ['query', 'body'] as const) { + for (const field of Object.keys(spec[slot] ?? {})) { + if (!FOLDER_PATH_FIELDS.has(field)) continue + checked += 1 + if (flagSpecFor(operation, field).folderPath !== true) { + unmarked.push(`${operation}.${field}`) + } + } + } + } + expect(unmarked).toEqual([]) + expect(checked).toBeGreaterThan(30) + }) + + it('decodes every one it also puts in a column', () => { + const undecoded: string[] = [] + for (const [operation, spec] of Object.entries(CLI_CONTRACT)) { + for (const column of [...(spec.columns ?? []), ...(spec.fields ?? [])]) { + const path = column.path ?? column.header + if (FOLDER_PATH_FIELDS.has(path) && column.format !== 'folder-path') { + undecoded.push(`${operation}.${path}`) + } + } + } + expect(undecoded).toEqual([]) + }) +}) diff --git a/packages/sim-cli/src/contract/commands.ts b/packages/sim-cli/src/contract/commands.ts index 4c7119c800f..8e522b3f37a 100644 --- a/packages/sim-cli/src/contract/commands.ts +++ b/packages/sim-cli/src/contract/commands.ts @@ -7,8 +7,17 @@ const TABLE_SORT_HELP = 'Ordered sort keys: [{"field":"createdAt","direction":"desc"}] (direction: asc or desc)' const CUSTOM_TOOL_SCHEMA_HELP = 'OpenAI function schema: {"type":"function","function":{"name":"...","parameters":{"type":"object","properties":{}}}}' +/** + * Every folder-path input the API accepts. + * + * `folderPath` is what marks the field for per-segment encoding, so a folder is + * typed by the name the app shows it under. It belongs on the shared constant + * rather than on each of the thirty-odd fields, because one that was missed + * would silently be the only place `/Folder 1` is still rejected. + */ const FOLDER_PATH_INPUT = { - describe: 'Folder path; the leading / is optional', + describe: 'Folder path as shown in the app; the leading / is optional', + folderPath: true, } as const const FOLDER_PATH_FLAG = { ...FOLDER_PATH_INPUT, @@ -18,7 +27,7 @@ const FOLDER_DELETE_FLAGS = { path: FOLDER_PATH_INPUT, recursive: { boolean: true, describe: 'Delete the folder and its descendants' }, } as const -const KNOWLEDGE_DOCUMENT_PATH_ARGUMENTS = { id: 'knowledgeBaseId' } as const +const KNOWLEDGE_BASE_PATH_ARGUMENT = { id: 'knowledgeBaseId' } as const const WORKFLOW_RUN_SCOPE = { id: { name: 'workflow', @@ -26,10 +35,11 @@ const WORKFLOW_RUN_SCOPE = { describe: 'Workflow ID', }, } as const +const FOLDER_COLUMN: ColumnSpec = { header: 'folder', path: 'folderPath', format: 'folder-path' } const FOLDER_LIST_COLUMNS: ColumnSpec[] = [ - { header: 'path' }, + { header: 'path', format: 'folder-path' }, { header: 'name' }, - { header: 'parent', path: 'parentPath' }, + { header: 'parent', path: 'parentPath', format: 'folder-path' }, { header: 'updated', path: 'updatedAt', format: 'timestamp' }, ] @@ -123,7 +133,7 @@ export const CLI_CONTRACT: CliContract = { bulkUpdateKnowledgeDocuments: { command: 'knowledge documents batch-update', describe: 'Enable or disable every matching document', - pathArgumentNames: KNOWLEDGE_DOCUMENT_PATH_ARGUMENTS, + pathArgumentNames: KNOWLEDGE_BASE_PATH_ARGUMENT, flags: { documentIds: { name: 'document', list: true }, selectAll: { boolean: true, describe: 'Apply to every document in the knowledge base' }, @@ -134,6 +144,15 @@ export const CLI_CONTRACT: CliContract = { command: 'workflows undeploy', describe: 'Take a workflow out of deployment', }, + // `GET /workflows/[id]/deployment` is a collection-shaped path holding one + // record, so the derived `list` promised a page of deployments there is no + // such thing as. `status` is what the singular group beside `versions` can be + // asked for. + getWorkflowDeployment: { + command: 'workflows deployment status', + renamedFrom: ['workflows deployment list'], + describe: 'Show a workflow’s current deployment', + }, setSecret: { hidden: true }, // ─── Destructive single-resource operations ─────────────────────────────── @@ -145,7 +164,7 @@ export const CLI_CONTRACT: CliContract = { }, deleteKnowledgeBase: { confirm: 'This deletes the knowledge base and every document in it.' }, deleteKnowledgeDocument: { - pathArgumentNames: KNOWLEDGE_DOCUMENT_PATH_ARGUMENTS, + pathArgumentNames: KNOWLEDGE_BASE_PATH_ARGUMENT, confirm: 'This deletes the document and its embeddings.', }, deleteFile: { confirm: 'This archives the file.' }, @@ -179,7 +198,14 @@ export const CLI_CONTRACT: CliContract = { workflowIds: { name: 'workflow', list: true }, folderPaths: { ...FOLDER_PATH_FLAG, list: true }, triggers: { name: 'trigger', list: true }, - details: { describe: 'Response detail level' }, + // The `workflow` column below reads `workflow.name`, which the API only + // sends at `full` — at its own `basic` default every row's workflow was an + // em-dash and a run had nothing naming what ran. Asked for by default so + // the declared columns can be filled; an explicit `--details basic` wins. + details: { + requestDefault: 'full', + describe: 'Response detail level; full is requested by default to name each run’s workflow', + }, includeTraceSpans: { boolean: true, describe: 'Include trace spans in JSON or YAML output (implies full detail)', @@ -313,7 +339,7 @@ export const CLI_CONTRACT: CliContract = { columns: [ { header: 'id' }, { header: 'name' }, - { header: 'folder', path: 'folderPath' }, + FOLDER_COLUMN, { header: 'rows', path: 'rowCount' }, { header: 'updated', path: 'updatedAt', format: 'timestamp' }, ], @@ -323,7 +349,7 @@ export const CLI_CONTRACT: CliContract = { columns: [ { header: 'id' }, { header: 'name' }, - { header: 'folder', path: 'folderPath' }, + FOLDER_COLUMN, { header: 'deployed', path: 'isDeployed', format: 'bool' }, { header: 'runs', path: 'runCount' }, { header: 'last run', path: 'lastRunAt', format: 'timestamp' }, @@ -336,7 +362,7 @@ export const CLI_CONTRACT: CliContract = { { header: 'name' }, // Now that files live in folders, which one is the difference between two // identically-named rows. - { header: 'folder', path: 'folderPath' }, + FOLDER_COLUMN, { header: 'size', format: 'bytes' }, { header: 'type' }, { header: 'uploaded by', path: 'uploadedByEmail' }, @@ -349,15 +375,21 @@ export const CLI_CONTRACT: CliContract = { columns: [ { header: 'id' }, { header: 'name' }, - { header: 'folder', path: 'folderPath' }, + FOLDER_COLUMN, { header: 'docs', path: 'docCount' }, { header: 'tokens', path: 'tokenCount' }, { header: 'model', path: 'embeddingModel' }, ], }, - getKnowledgeDocument: { pathArgumentNames: KNOWLEDGE_DOCUMENT_PATH_ARGUMENTS }, + // Every command whose `[id]` is the parent knowledge base rather than the + // thing being acted on names it in its own help and error messages. `update` + // and `tags list` were left out, so the same value was `` on one command + // and `` on its neighbours. + getKnowledgeDocument: { pathArgumentNames: KNOWLEDGE_BASE_PATH_ARGUMENT }, + updateKnowledgeDocument: { pathArgumentNames: KNOWLEDGE_BASE_PATH_ARGUMENT }, + listKnowledgeTags: { pathArgumentNames: KNOWLEDGE_BASE_PATH_ARGUMENT }, listKnowledgeDocuments: { - pathArgumentNames: KNOWLEDGE_DOCUMENT_PATH_ARGUMENTS, + pathArgumentNames: KNOWLEDGE_BASE_PATH_ARGUMENT, columns: [ { header: 'id' }, { header: 'filename' }, @@ -461,9 +493,9 @@ export const CLI_CONTRACT: CliContract = { }, // ─── The expanded files surface ─────────────────────────────────────────── - // Every one of these derives badly. `/files/move` and `/files/bulk-delete` - // are verbs sitting where the deriver expects a sub-resource, so it made them - // groups holding a lone `create`. + // Every one of these derives badly. `/files/move`, `/files/bulk-delete` and + // `/files/[fileId]/restore` are verbs sitting where the deriver expects a + // sub-resource, so it made them groups holding a lone `create`. bulkDeleteFiles: { // `batch-` for the bulk form, matching `tables rows batch-delete`. command: 'files batch-delete', @@ -481,7 +513,7 @@ export const CLI_CONTRACT: CliContract = { { header: 'name' }, { header: 'size', format: 'bytes' }, { header: 'type' }, - { header: 'folder', path: 'folderPath' }, + FOLDER_COLUMN, { header: 'uploaded by', path: 'uploadedByEmail' }, { header: 'uploaded', path: 'uploadedAt', format: 'timestamp' }, { header: 'updated', path: 'updatedAt', format: 'timestamp' }, @@ -511,6 +543,13 @@ export const CLI_CONTRACT: CliContract = { command: 'files rename', describe: 'Rename a file', }, + restoreFile: { + // `files restore create` created nothing; it is the inverse of the delete + // that archived the file. + command: 'files restore', + renamedFrom: ['files restore create'], + describe: 'Restore an archived file', + }, updateFileContent: { command: 'files set-content', describe: 'Replace a file’s contents', @@ -659,9 +698,9 @@ export const CLI_CONTRACT: CliContract = { }, // ─── The expanded tables surface ────────────────────────────────────────── - // `/cancel-runs`, `/rows/find`, `/columns/run` and the enrichment path all put - // a verb where the deriver expects a sub-resource, so each became - // a group holding a lone `create`. + // `/cancel-runs`, `/rows/find`, `/query/count`, `/columns/run` and the + // enrichment path all put a verb where the deriver expects a sub-resource, so + // each became a group holding a lone `create`. cancelTableRuns: { command: 'tables cancel-runs', describe: 'Stop every running column job', @@ -674,13 +713,30 @@ export const CLI_CONTRACT: CliContract = { command: 'tables rows find', describe: 'Find rows matching a predicate', flags: { - q: { describe: 'Value to find' }, + // `--q` was the wire field spelled out; the same idea is `--query` on + // `knowledge search`, and one concept should not have two flag names. + q: { name: 'query', renamedFrom: ['q'], describe: 'Value to find' }, predicate: { name: 'filter', json: true, describe: TABLE_FILTER_HELP }, sort: { json: true, describe: TABLE_SORT_HELP }, }, itemsPath: 'matches', columns: [{ header: 'ordinal' }, { header: 'row', path: 'rowId' }, { header: 'column' }], }, + queryRowsCount: { + // `tables count create` counted rows and created nothing. The count is a + // question about rows, so it belongs beside the other row commands. + command: 'tables rows count', + renamedFrom: ['tables count create'], + describe: 'Count rows matching a filter', + flags: { + predicate: { + name: 'filter', + renamedFrom: ['predicate'], + json: true, + describe: TABLE_FILTER_HELP, + }, + }, + }, runTableColumn: { command: 'tables columns run', describe: 'Run a column’s workflow', diff --git a/packages/sim-cli/src/contract/types.ts b/packages/sim-cli/src/contract/types.ts index 736c247ef24..5a8edbd1dd0 100644 --- a/packages/sim-cli/src/contract/types.ts +++ b/packages/sim-cli/src/contract/types.ts @@ -34,6 +34,15 @@ export interface FlagSpec { name?: string /** Short alias, e.g. `w` for `--workspace`. */ short?: string + /** + * Flag names this field used to answer to, such as `predicate` before the + * count command's filter was spelled the same as its six siblings'. + * + * Kept only so an existing script does not break: hidden from help and from + * the generated docs, warns on stderr, and refuses when combined with the + * current spelling rather than silently picking one. + */ + renamedFrom?: readonly string[] /** * Accept one or more space-separated values, or `@path` / `@-` with one * value per line. @@ -52,10 +61,32 @@ export interface FlagSpec { json?: boolean /** Overrides the help text otherwise taken from the OpenAPI description. */ describe?: string + /** + * Value sent when the caller passes nothing, in place of the server's default. + * + * For a command whose declared `columns` read a field the API only sends at a + * heavier setting: `logs list` shows `workflow.name`, which `details=basic` + * omits, so the primary debugging table had a permanently empty column. It is + * a request default, not a flag default — whatever the caller types wins, + * including a deliberate `--details basic`. + */ + requestDefault?: string /** Accepted values when the generated descriptor cannot recover an enum. */ choices?: readonly string[] /** Expose a string-backed API boolean as a conventional terminal toggle. */ boolean?: true + /** + * This field carries a folder path, so percent-encode each of its segments. + * + * The API's canonical folder path is percent-encoded per segment, which made + * the terminal the only place a folder had to be spelled `/Folder%201` + * instead of the `/Folder 1` shown everywhere else; typing what you see was + * rejected with a message that never mentioned encoding. Marked rather than + * inferred from the field's name: `files upload` and `knowledge documents + * upload` take a `path` that is a LOCAL file, and encoding one of those would + * break the read. + */ + folderPath?: true /** * Never expose this field as a flag, and never send it. * @@ -85,8 +116,24 @@ export interface ColumnSpec { header: string /** Dot path into the row. Defaults to `header`. */ path?: string - /** Rendering hint; `auto` inspects the value. */ - format?: 'auto' | 'timestamp' | 'bytes' | 'duration' | 'bool' | 'cost' | 'count' | 'trace-count' + /** + * Rendering hint; `auto` inspects the value. + * + * `folder-path` is the display half of `FlagSpec.folderPath`: it undoes the + * wire encoding for the human formats, so a folder no longer prints as + * `/cli-test-a/nested%20one` in the same row as the `nested one` the server + * put in the adjacent name column. + */ + format?: + | 'auto' + | 'timestamp' + | 'bytes' + | 'duration' + | 'bool' + | 'cost' + | 'count' + | 'trace-count' + | 'folder-path' } export interface BodyVariantSpec { @@ -121,6 +168,16 @@ export interface CommandSpec { groupDefault?: boolean /** Alternate leaf command names, such as `ls` for `list`. */ aliases?: readonly string[] + /** + * Full command paths this operation used to answer to, such as + * `tables count create` before it became `tables rows count`. + * + * Unlike {@link aliases}, these are kept only so an existing script does not + * break: each is hidden from help and from the generated docs, and warns on + * stderr with the current spelling. Give the whole path, because a rename can + * move a command between groups rather than just retitle its leaf. + */ + renamedFrom?: readonly string[] /** Route path parameters exposed as required named options instead of positionals. */ pathFlags?: Record /** Friendly placeholders for route path parameters that remain positional. */ diff --git a/packages/sim-cli/src/http/client.test.ts b/packages/sim-cli/src/http/client.test.ts index 39b5177e58e..53f9393b59b 100644 --- a/packages/sim-cli/src/http/client.test.ts +++ b/packages/sim-cli/src/http/client.test.ts @@ -13,6 +13,41 @@ afterEach(() => { vi.unstubAllGlobals() }) +function client(options: { apiKey?: string } = { apiKey: 'key' }): SimClient { + return new SimClient({ + name: 'default', + endpoint: 'https://sim.example', + apiKey: options.apiKey ?? null, + workspaceId: 'ws_1', + output: 'json', + sources: { + endpoint: 'default', + apiKey: 'env', + workspaceId: 'env', + output: 'default', + }, + }) +} + +function stubStderr(isTTY: boolean): { writes: string[]; restore: () => void } { + const writes: string[] = [] + const originalTTY = Object.getOwnPropertyDescriptor(process.stderr, 'isTTY') + const originalWrite = process.stderr.write + Object.defineProperty(process.stderr, 'isTTY', { configurable: true, value: isTTY }) + process.stderr.write = ((chunk: string) => { + writes.push(String(chunk)) + return true + }) as typeof process.stderr.write + return { + writes, + restore: () => { + process.stderr.write = originalWrite + if (originalTTY) Object.defineProperty(process.stderr, 'isTTY', originalTTY) + else Reflect.deleteProperty(process.stderr, 'isTTY') + }, + } +} + describe('cursor pagination', () => { it('follows v2 cursors through the requested item limit', async () => { const request = vi @@ -37,6 +72,213 @@ describe('cursor pagination', () => { auth: 'optional', }) }) + + it('reports progress on stderr once a second page is coming, then clears the line', async () => { + const request = vi + .fn() + .mockResolvedValueOnce({ data: ['a', 'b'], nextCursor: 'next' }) + .mockResolvedValueOnce({ data: ['c'], nextCursor: null }) + const stderr = stubStderr(true) + + try { + await requestAllPages({ request } as Pick, '/api/v2/items', { + pageSize: 2, + }) + } finally { + stderr.restore() + } + + expect(stderr.writes).toHaveLength(2) + expect(stderr.writes[0]).toContain('fetched 2') + expect(stderr.writes[1]).toBe('\r\u001b[K') + }) + + it('stays silent for a single page, and when stderr is not a terminal', async () => { + const single = vi.fn().mockResolvedValue({ data: ['a'], nextCursor: null }) + const paged = vi + .fn() + .mockResolvedValueOnce({ data: ['a'], nextCursor: 'next' }) + .mockResolvedValueOnce({ data: ['b'], nextCursor: null }) + + const tty = stubStderr(true) + try { + await requestAllPages({ request: single } as Pick, '/items', { + pageSize: 2, + }) + } finally { + tty.restore() + } + expect(tty.writes).toEqual([]) + + const piped = stubStderr(false) + try { + await requestAllPages({ request: paged } as Pick, '/items', { + pageSize: 1, + }) + } finally { + piped.restore() + } + expect(piped.writes).toEqual([]) + }) +}) + +describe('redirects', () => { + function redirect(location: string | null, status = 301): Response { + return new Response(null, { + status, + headers: location === null ? {} : { location }, + }) + } + + it('does not let fetch follow a redirect, which would drop the write body', async () => { + const fetch = vi.fn().mockResolvedValue(redirect('https://www.sim.example/api/v2/tables')) + vi.stubGlobal('fetch', fetch) + + await expect( + client().request('/api/v2/tables/folders', { method: 'POST', body: { path: '/a' } }) + ).rejects.toThrow(/redirected to https:\/\/www\.sim\.example/) + expect(fetch.mock.calls[0][1].redirect).toBe('manual') + }) + + it('names the endpoint to switch to, derived from the Location origin', async () => { + vi.stubGlobal( + 'fetch', + vi.fn().mockResolvedValue(redirect('https://www.sim.example:8443/api/v2/tables?x=1', 308)) + ) + + await expect(client().request('/api/v2/tables')).rejects.toMatchObject({ + message: + 'Endpoint redirected to https://www.sim.example:8443. Run: sim configure --profile default --set-endpoint https://www.sim.example:8443', + status: 308, + }) + }) + + it('resolves a relative Location rather than string-hacking the endpoint', async () => { + vi.stubGlobal('fetch', vi.fn().mockResolvedValue(redirect('/api/v2/tables/'))) + + await expect(client().request('/api/v2/tables')).rejects.toThrow( + /redirected to https:\/\/sim\.example\/api\/v2\/tables\// + ) + }) + + it('still explains itself when Location is missing or unparseable', async () => { + vi.stubGlobal('fetch', vi.fn().mockResolvedValue(redirect(null, 302))) + await expect(client().request('/api/v2/tables')).rejects.toThrow(/no usable redirect target/) + + vi.stubGlobal('fetch', vi.fn().mockResolvedValue(redirect('http://'))) + await expect(client().request('/api/v2/tables')).rejects.toThrow(/no usable redirect target/) + }) +}) + +describe('non-JSON responses', () => { + it('names the URL and the shape instead of dumping a page of HTML', async () => { + vi.stubGlobal( + 'fetch', + vi.fn().mockResolvedValue( + new Response('Example Domain', { + status: 404, + headers: { 'content-type': 'text/html; charset=UTF-8' }, + }) + ) + ) + + const failure = client().request('/api/v2/workflows') + + await expect(failure).rejects.toMatchObject({ + message: + 'https://sim.example/api/v2/workflows returned HTML, not JSON (HTTP 404) — check your endpoint.', + }) + await expect(failure).rejects.not.toThrow(/ { + vi.stubGlobal( + 'fetch', + vi.fn().mockResolvedValue( + new Response('hello', { + status: 200, + headers: { 'content-type': 'text/html' }, + }) + ) + ) + + await expect(client().request('/api/v2/workflows')).rejects.toMatchObject({ + name: 'SimApiError', + message: + 'https://sim.example/api/v2/workflows returned HTML, not JSON (HTTP 200) — check your endpoint.', + }) + }) + + it("keeps a short plain-text body, which is the proxy's own diagnosis", async () => { + vi.stubGlobal( + 'fetch', + vi.fn().mockResolvedValue( + new Response('upstream connect error', { + status: 502, + headers: { 'content-type': 'text/plain' }, + }) + ) + ) + + await expect(client().request('/api/v2/workflows')).rejects.toThrow( + /returned text\/plain, not JSON \(HTTP 502\) — check your endpoint\. Response: upstream connect error/ + ) + }) + + it('leaves an empty error body reported by status alone', async () => { + vi.stubGlobal('fetch', vi.fn().mockResolvedValue(new Response('', { status: 503 }))) + + await expect(client().request('/api/v2/workflows')).rejects.toMatchObject({ + message: 'Request failed with status 503', + }) + }) +}) + +describe('personal-key-only operations', () => { + it('appends the remedy, keyed off the code the API actually nests', async () => { + // The envelope this asserts is the one staging returns: `error.code` is the + // status class, and the actionable code rides in `error.details.code`. + // Fabricating it at the top level made a green test out of a dead branch. + vi.stubGlobal( + 'fetch', + vi.fn().mockResolvedValue( + new Response( + JSON.stringify({ + error: { + code: 'FORBIDDEN', + message: 'Workspace API key cannot perform this operation', + details: { code: 'WORKSPACE_KEY_OPERATION_NOT_PERMITTED' }, + }, + }), + { status: 403, headers: { 'content-type': 'application/json' } } + ) + ) + ) + + await expect(client().request('/api/v2/secrets')).rejects.toMatchObject({ + message: + 'Workspace API key cannot perform this operation — this operation needs a personal API key: sim login --profile default', + code: 'FORBIDDEN', + }) + }) + + it('invents no remedy for other forbidden codes', async () => { + vi.stubGlobal( + 'fetch', + vi + .fn() + .mockResolvedValue( + new Response( + JSON.stringify({ error: { code: 'FORBIDDEN', message: 'Insufficient permissions' } }), + { status: 403, headers: { 'content-type': 'application/json' } } + ) + ) + ) + + await expect(client().request('/api/v2/secrets')).rejects.toMatchObject({ + message: 'Insufficient permissions', + }) + }) }) describe('API errors', () => { @@ -109,28 +351,103 @@ describe('API errors', () => { expect(lines).toEqual([' details:', ' predicate.all.0.op: Expected one of eq, ne']) }) + it('drops the union branches the input did not take', () => { + // `keys` is part of the issue Zod emits and the route serializes verbatim, + // and it is the tell: a key rejected as unrecognized that another issue was + // found *inside* is a branch the input did not take, not a real complaint. + const lines = formatApiErrorDetails([ + { code: 'invalid_type', path: ['predicate', 'all', 0, 'all'], message: 'expected array' }, + { + code: 'unrecognized_keys', + keys: ['field', 'op', 'value'], + path: ['predicate', 'all', 0], + message: 'Unrecognized keys: "field", "op", "value"', + }, + { + code: 'invalid_value', + path: ['predicate', 'all', 0, 'op'], + message: 'Invalid option: expected one of "eq"|"ne"', + }, + { + code: 'unrecognized_keys', + keys: ['all'], + path: ['predicate'], + message: 'Unrecognized key: "all"', + }, + ]) + + expect(lines).toContain(' predicate.all.0.op: Invalid option: expected one of "eq"|"ne"') + expect(lines.join('\n')).not.toContain('Unrecognized key: "all"') + expect(lines.join('\n')).not.toContain('Unrecognized keys:') + }) + + it('keeps an unrecognized key nothing else was reported inside', () => { + // The suppression above once dropped every ancestor path, which swallowed + // this: `tll` is genuinely unknown, and the caller cannot see it anywhere + // else in the response. + const lines = formatApiErrorDetails([ + { + code: 'invalid_value', + path: ['config', 'model'], + message: 'Invalid option: expected one of "a"|"b"', + }, + { + code: 'unrecognized_keys', + keys: ['tll'], + path: ['config'], + message: 'Unrecognized key: "tll"', + }, + ]) + + expect(lines).toContain(' config: Unrecognized key: "tll"') + }) + + it('keeps a container-level cap reported alongside a bad element', () => { + // Both have to be fixed; showing only the element sends the caller back for + // a second identical 400. + const lines = formatApiErrorDetails([ + { path: ['rows'], message: 'Cannot insert more than 100 rows per batch' }, + { path: ['rows', 3, 'email'], message: 'Expected string, received number' }, + ]) + + expect(lines).toContain(' rows: Cannot insert more than 100 rows per batch') + expect(lines).toContain(' rows.3.email: Expected string, received number') + }) + + it('keeps a cross-field refusal, whose path is empty', () => { + // An empty path is an ancestor of every other path, so the blanket + // suppression erased exactly the message that names what to do. + const lines = formatApiErrorDetails([ + { path: [], message: 'Provide either filter or rowIds' }, + { path: ['workspaceId'], message: 'Required' }, + ]) + + expect(lines).toContain(' request: Provide either filter or rowIds') + expect(lines).toContain(' workspaceId: Required') + }) + + it('still shows every field of a genuine multi-field failure', () => { + const lines = formatApiErrorDetails([ + { path: ['name'], message: 'Required' }, + { path: ['workspaceId'], message: 'Required' }, + ]) + + expect(lines).toEqual([' details:', ' name: Required', ' workspaceId: Required']) + }) + + it('never suppresses the only issue there is', () => { + expect(formatApiErrorDetails([{ path: ['name'], message: 'Required' }])).toEqual([ + ' details:', + ' name: Required', + ]) + }) + it('keeps non-validation details as JSON', () => { expect(formatApiErrorDetails({ id: 'missing' })).toEqual([' details: {"id":"missing"}']) }) }) describe('raw requests', () => { - function client(options: { apiKey?: string } = { apiKey: 'key' }): SimClient { - return new SimClient({ - name: 'default', - endpoint: 'https://sim.example', - apiKey: options.apiKey ?? null, - workspaceId: 'ws_1', - output: 'json', - sources: { - endpoint: 'default', - apiKey: 'env', - workspaceId: 'env', - output: 'default', - }, - }) - } - it('returns an unconsumed response and forwards an abort signal', async () => { const fetch = vi.fn().mockResolvedValue(new Response('stream body')) vi.stubGlobal('fetch', fetch) diff --git a/packages/sim-cli/src/http/client.ts b/packages/sim-cli/src/http/client.ts index dbebfecc7d5..0e92021a519 100644 --- a/packages/sim-cli/src/http/client.ts +++ b/packages/sim-cli/src/http/client.ts @@ -1,3 +1,4 @@ +import chalk from 'chalk' import type { ResolvedProfile } from '../config/index' /** @@ -60,6 +61,56 @@ function buildUrl(endpoint: string, path: string, query?: Record 0 && text.length <= 200 + return new SimApiError( + `${url} returned ${kind}, not JSON (HTTP ${status}) — check your endpoint.${ + keepSnippet ? ` Response: ${truncate(text, 200)}` : '' + }`, + status + ) +} + /** * Pulls a human-readable message out of whatever the server returned. * @@ -68,16 +119,18 @@ function buildUrl(endpoint: string, path: string, query?: Record= other.length) return false + return path.every((segment, index) => segment === other[index]) +} + +/** + * True when `issue` rejects a key that another issue was found *underneath*. + * + * That combination only happens on a union: Zod reports every branch, so one bad + * operator arrives as both the real complaint (`predicate.all.0.op: invalid + * option`) and the branch with no `all` key at all (`predicate: unrecognized key + * "all"` — flatly untrue where the input landed). A genuinely unrecognized key + * is a key the schema knows nothing about, so no other issue can be reported + * inside it. + */ +function rejectsAKeyThatValidated(issue: DetailIssue, other: DetailIssue): boolean { + if (!issue.unrecognizedKeys || !isStrictPrefix(issue.path, other.path)) return false + return issue.unrecognizedKeys.includes(other.path[issue.path.length]) +} + +/** + * Drops the "unrecognized key" a union reports about the branch the input did + * not take. + * + * Scoped to that one shape on purpose. Suppressing every ancestor path instead + * swallowed complaints the caller has to act on and cannot see anywhere else: a + * container-level cap (`orderKeys: too big` alongside a bad element) and a + * cross-field refusal, whose path is empty and so is an ancestor of everything. + */ +function dropUnionBranchNoise(issues: DetailIssue[]): DetailIssue[] { + if (issues.length < 2) return issues + const kept = issues.filter( + (issue) => !issues.some((other) => rejectsAKeyThatValidated(issue, other)) + ) + return kept.length > 0 ? kept : issues +} + /** Formats nested validation issues as readable, path-aware lines. */ export function formatApiErrorDetails(details: unknown): string[] { - const issues = new Set() + const issues: DetailIssue[] = [] + const seen = new Set() const visit = (value: unknown, parentPath: string[] = []): void => { if (Array.isArray(value)) { @@ -124,15 +240,31 @@ export function formatApiErrorDetails(details: unknown): string[] { } if (typeof issue.message !== 'string' || issue.message === 'Invalid input') return - issues.add(`${path.length > 0 ? path.join('.') : 'request'}: ${issue.message}`) + const line = `${path.join('.')}: ${issue.message}` + if (seen.has(line)) return + seen.add(line) + issues.push({ + path, + message: issue.message, + unrecognizedKeys: + issue.code === 'unrecognized_keys' && Array.isArray(issue.keys) + ? issue.keys.map(String) + : null, + }) } visit(details) - if (issues.size === 0) return [` details: ${truncate(JSON.stringify(details), 1000)}`] - - const visible = [...issues].slice(0, 8) - const lines = [' details:', ...visible.map((issue) => ` ${issue}`)] - if (issues.size > visible.length) lines.push(` … ${issues.size - visible.length} more issues`) + if (issues.length === 0) return [` details: ${truncate(JSON.stringify(details), 1000)}`] + + const kept = dropUnionBranchNoise(issues) + const visible = kept.slice(0, 8) + const lines = [ + ' details:', + ...visible.map( + (issue) => ` ${issue.path.length > 0 ? issue.path.join('.') : 'request'}: ${issue.message}` + ), + ] + if (kept.length > visible.length) lines.push(` … ${kept.length - visible.length} more issues`) return lines } @@ -179,6 +311,25 @@ export class SimClient { * become the same structured `SimApiError` either way. */ async requestRaw(path: string, options: RequestOptions = {}): Promise { + return (await this.send(path, options)).response + } + + async request(path: string, options: RequestOptions = {}): Promise { + const { response, url } = await this.send(path, options) + const raw = await response.text() + + if (!raw) return undefined as T + try { + return JSON.parse(raw) as T + } catch { + throw toNonJsonError(url, response.status, response.headers.get('content-type'), raw) + } + } + + private async send( + path: string, + options: RequestOptions + ): Promise<{ response: Response; url: string }> { const apiKey = this.resolveApiKey(options.auth) const url = buildUrl(this.profile.endpoint, path, options.query) @@ -196,6 +347,7 @@ export class SimClient { }, body: hasBody ? JSON.stringify(options.body) : undefined, signal: options.signal, + redirect: 'manual', }) } catch (cause) { if (options.signal?.aborted) { @@ -207,24 +359,91 @@ export class SimClient { ) } + if (REDIRECT_STATUSES.has(response.status)) throw this.toRedirectError(url, response) + if (!response.ok) { const raw = await response.text() - const error = toApiError(response.status, raw) + const error = toApiError(url, response.status, response.headers.get('content-type'), raw) if (response.status === 401) { error.message = `${error.message} — run: sim login --profile ${this.profile.name}` } + if (namesWorkspaceKeyRefusal(error)) { + error.message = `${error.message} — this operation needs a personal API key: sim login --profile ${this.profile.name}` + } throw error } - return response + return { response, url } } - async request(path: string, options: RequestOptions = {}): Promise { - const response = await this.requestRaw(path, options) - const raw = await response.text() + /** + * Explains a redirect instead of following it, naming the endpoint to switch to. + * + * The destination comes from `Location` resolved against the request URL, so a + * relative target works and no string surgery is done on the configured + * endpoint. A `Location` that is missing or unparseable still has to produce a + * sentence — the redirect is the finding either way. + */ + private toRedirectError(url: string, response: Response): SimApiError { + const location = response.headers.get('location')?.trim() + let target: URL | null = null + if (location) { + try { + target = new URL(location, url) + } catch { + target = null + } + } - if (!raw) return undefined as T - return JSON.parse(raw) as T + if (!target) { + return new SimApiError( + `${url} answered HTTP ${response.status} with no usable redirect target. Check the endpoint for profile "${this.profile.name}".`, + response.status + ) + } + if (target.origin === new URL(url).origin) { + return new SimApiError( + `${url} redirected to ${target.href}. The CLI does not follow redirects, because a redirect can drop the request body and turn a write into a silent no-op.`, + response.status + ) + } + return new SimApiError( + `Endpoint redirected to ${target.origin}. Run: sim configure --profile ${this.profile.name} --set-endpoint ${target.origin}`, + response.status + ) + } +} + +export interface PageProgress { + /** Call once a further page is known to be coming, with the count so far. */ + advance: (fetched: number) => void + /** Erases the line, if anything was ever written to it. */ + finish: () => void +} + +/** + * Reports cursor progress on stderr while a list keeps paging. + * + * A long cursor is many sequential requests and reads as a hang, so say so — but + * only on a terminal, and only on stderr, because stdout is what gets piped to + * `jq`. + * + * Shared because the CLI pages in two places: {@link requestAllPages} for the + * `ls` commands, and the contract-driven loop in `runtime/execute`, which also + * has to carry a cursor in the body. Only one of them had the writer, and it was + * not the one nearly every `list --limit 0` goes through. + */ +export function pageProgress(): PageProgress { + let reported = false + return { + advance: (fetched) => { + if (!process.stderr.isTTY) return + reported = true + process.stderr.write(`\r${chalk.dim(`fetched ${fetched}…`)}\u001b[K`) + }, + finish: () => { + if (reported) process.stderr.write('\r\u001b[K') + }, } } @@ -239,6 +458,7 @@ export async function requestAllPages( if (limit <= 0) return [] const items: T[] = [] + const progress = pageProgress() let cursor: string | null = null do { const page: V2Page = await client.request>(path, { @@ -251,8 +471,12 @@ export async function requestAllPages( }) items.push(...page.data) cursor = page.nextCursor + + if (cursor && items.length < limit) progress.advance(items.length) } while (cursor && items.length < limit) + progress.finish() + return items.slice(0, limit) } diff --git a/packages/sim-cli/src/output/render.test.ts b/packages/sim-cli/src/output/render.test.ts index a72caa2946b..337f1ea7d25 100644 --- a/packages/sim-cli/src/output/render.test.ts +++ b/packages/sim-cli/src/output/render.test.ts @@ -180,6 +180,18 @@ describe('printRecord', () => { expect(logged[1]).toContain('alpha') }) + it('clamps a very wide value in table mode only', () => { + // A signed download URL is the whole point of the command that prints it; + // clamping it before the format branch corrupted `text`, silently. + const url = `https://example.com/${'a'.repeat(400)}` + printRecord('table', [['url', url]], {}) + printRecord('text', [['url', url]], {}) + + expect(logged[0]).toMatch(/…$/) + expect(logged[0].length).toBeLessThan(url.length) + expect(logged[1]).toBe(`url\t${url}`) + }) + it.each(['text', 'table'] as const)('sanitizes API-controlled labels in %s output', (format) => { printRecord(format, [[`${ESC}]0;pwned${BEL}safe\nlabel`, 'value']], {}) @@ -209,6 +221,10 @@ describe('formatters', () => { expect(duration(1500)).toBe('1.5s') expect(duration(90_000)).toBe('1m30s') }) + + it('rounds the high-resolution milliseconds the API reports', () => { + expect(duration(9.145596999907866)).toBe('9ms') + }) }) describe('sanitize', () => { diff --git a/packages/sim-cli/src/output/render.ts b/packages/sim-cli/src/output/render.ts index 3c748b79bca..916ad57e1b9 100644 --- a/packages/sim-cli/src/output/render.ts +++ b/packages/sim-cli/src/output/render.ts @@ -109,7 +109,10 @@ export function bytes(value: number | null | undefined): string { export function duration(ms: number | null | undefined): string { if (ms === null || ms === undefined) return EMPTY - if (ms < 1000) return `${ms}ms` + // The API measures runs with a high-resolution clock, so a duration arrives as + // `9.145596999907866`. Sub-millisecond precision is noise in a terminal and + // the raw float is wider than every other cell in the row. + if (ms < 1000) return `${Math.round(ms)}ms` if (ms < 60_000) return `${(ms / 1000).toFixed(1)}s` return `${Math.floor(ms / 60_000)}m${Math.round((ms % 60_000) / 1000)}s` } @@ -184,13 +187,25 @@ function oneLine(value: string): string { */ const MAX_CELL_WIDTH = 60 -function clampCell(value: string): string { +/** + * Widest a single record field may render in `table` mode. + * + * A record prints one value per line, so a long value costs nothing but its own + * line — far more room than a table column, where one wide cell sets the width + * for every row. The clamp lives in `printRecord` rather than in the caller that + * builds the fields, so `text`, `json` and `yaml` still carry the whole value: + * clamping before the format branch truncated commands whose entire output is + * one signed URL, silently, in the format built for pipes. + */ +const MAX_RECORD_WIDTH = 160 + +function clamp(value: string, width: number): string { // ANSI-bearing cells come from the short formatters (`yes`/`no`, the empty // glyph); slicing one mid-escape would corrupt it, and none are ever wide. - if (visibleWidth(value) <= MAX_CELL_WIDTH || value !== value.replace(ANSI_PATTERN, '')) { + if (visibleWidth(value) <= width || value !== value.replace(ANSI_PATTERN, '')) { return value } - return `${value.slice(0, MAX_CELL_WIDTH - 1)}…` + return `${value.slice(0, width - 1)}…` } function renderTable(rows: T[], columns: Column[]): string { @@ -200,7 +215,9 @@ function renderTable(rows: T[], columns: Column[]): string { // remote content and gets the same treatment as a cell. Doing it here rather // than only at each call site means a future column source cannot reopen this. const headers = columns.map((column) => sanitize(column.header)) - const cells = rows.map((row) => columns.map((column) => clampCell(oneLine(column.value(row))))) + const cells = rows.map((row) => + columns.map((column) => clamp(oneLine(column.value(row)), MAX_CELL_WIDTH)) + ) const widths = columns.map((_column, index) => Math.max(visibleWidth(headers[index]), ...cells.map((line) => visibleWidth(line[index]))) ) @@ -283,7 +300,10 @@ export function printDocument(format: OutputFormat, raw: unknown): void { ) } -/** Prints a single record: machine formats from the raw value, otherwise aligned lines. */ +/** + * Prints a single record: machine formats from the raw value, otherwise aligned + * lines. As in `printList`, only the `table` rendering is clamped. + */ export function printRecord(format: OutputFormat, fields: Array<[string, string]>, raw: unknown) { const machine = renderMachine(format, raw) if (machine !== null) { @@ -302,6 +322,8 @@ export function printRecord(format: OutputFormat, fields: Array<[string, string] const width = Math.max(...safeFields.map(([label]) => visibleWidth(label))) for (const [label, value] of safeFields) { - console.log(`${chalk.dim(pad(`${label}:`, width + 1))} ${oneLine(value)}`) + console.log( + `${chalk.dim(pad(`${label}:`, width + 1))} ${clamp(oneLine(value), MAX_RECORD_WIDTH)}` + ) } } diff --git a/packages/sim-cli/src/runtime/build.test.ts b/packages/sim-cli/src/runtime/build.test.ts index de9d2a939bb..e867d7c9d49 100644 --- a/packages/sim-cli/src/runtime/build.test.ts +++ b/packages/sim-cli/src/runtime/build.test.ts @@ -1,6 +1,7 @@ import { Command } from 'commander' import { beforeEach, describe, expect, it, vi } from 'vitest' import { buildGeneratedCommands } from './build' +import { resetRenameWarnings } from './renamed' /** * Drives commands through commander's own parsing rather than calling @@ -743,16 +744,20 @@ describe('single-resource rendering', () => { expect(printed.join('\n')).toMatch(/email/) }) - it('truncates a nested value rather than flooding the terminal', async () => { - const printed = await lines( - ['workflows', 'get', 'wf_1'], - { id: 'wf_1', state: { blocks: 'x'.repeat(5000) } }, - 'text' - ) + it('truncates a nested value in the table, and only there', async () => { + const payload = { id: 'wf_1', state: { blocks: 'x'.repeat(5000) } } + + const table = await lines(['workflows', 'get', 'wf_1'], payload, 'table') + const clamped = table.find((line) => line.startsWith('state')) ?? '' + expect(clamped.length).toBeLessThan(300) + expect(clamped).toMatch(/…$/) - const stateLine = printed.find((line) => line.startsWith('state')) ?? '' - expect(stateLine.length).toBeLessThan(300) - expect(stateLine).toMatch(/…$/) + // `text` is the format built for pipes, so it carries the whole value: the + // clamp is a legibility cap on the human table, and clamping before the + // format branch silently truncated commands whose output is one long value. + const piped = await lines(['workflows', 'get', 'wf_1'], payload, 'text') + const whole = piped.find((line) => line.startsWith('state')) ?? '' + expect(whole).toContain('x'.repeat(5000)) }) it('emits a document command as JSON whatever the display format is', async () => { @@ -880,7 +885,7 @@ describe('contract-selected list rendering', () => { }) it('renders row matches as rows', async () => { - const printed = await lines(['tables', 'rows', 'find', 'tbl_1', '--q', 'alice'], { + const printed = await lines(['tables', 'rows', 'find', 'tbl_1', '--query', 'alice'], { matches: [{ ordinal: 3, rowId: 'row_1', column: 'email' }], truncated: false, }) @@ -1006,6 +1011,30 @@ describe('pagination slot', () => { expect(mockRequest.mock.calls[1][1].query).toMatchObject({ cursor: 'c1' }) }) + it('says it is still fetching, on stderr, so a long cursor does not read as a hang', async () => { + // The progress writer only ever lived in `requestAllPages`, which just the + // `ls` commands use; every generated list pages through its own loop, so + // `--limit 0` sat silent through twenty sequential requests. stdout stays + // clean because that is what gets piped to `jq`. + mockRequest.mockReset() + mockRequest + .mockResolvedValueOnce({ data: [{ id: 'a' }], nextCursor: 'c1' }) + .mockResolvedValueOnce({ data: [{ id: 'b' }], nextCursor: null }) + vi.spyOn(console, 'log').mockImplementation(() => {}) + const terminal = Object.getOwnPropertyDescriptor(process.stderr, 'isTTY') + Object.defineProperty(process.stderr, 'isTTY', { configurable: true, value: true }) + const stderr = vi.spyOn(process.stderr, 'write').mockReturnValue(true) + + try { + await program().parseAsync(['node', 'sim', 'logs', 'list', '--limit', '0']) + } finally { + if (terminal) Object.defineProperty(process.stderr, 'isTTY', terminal) + else Reflect.deleteProperty(process.stderr, 'isTTY') + } + + expect(stderr.mock.calls.map(([chunk]) => String(chunk)).join('')).toContain('fetched 1') + }) + it('uses a valid per-page size for unlimited and large totals', async () => { for (const requested of ['0', '250']) { mockRequest.mockReset() @@ -1164,3 +1193,113 @@ describe('bodies and fields the generator cannot flatten', () => { expect(options.query).toMatchObject({ limit: 7 }) }) }) + +describe('spellings the CLI has retired', () => { + beforeEach(() => { + resetRenameWarnings() + }) + + function warnings(): string[] { + const written: string[] = [] + vi.spyOn(process.stderr, 'write').mockImplementation((chunk: string | Uint8Array) => { + written.push(String(chunk)) + return true + }) + return written + } + + it('still answers to a command path that moved between groups', async () => { + // `tables count create` counted rows and created nothing, so it became + // `tables rows count`. A script written against the old path predates the + // rename and has no way to know. + const written = warnings() + const [path, options] = await run( + ['tables', 'count', 'create', 'tbl_1', '--filter', '{"all":[]}'], + { data: { totalCount: 0 } } + ) + expect(path).toBe('/api/v2/tables/tbl_1/query/count') + expect(options.body).toMatchObject({ predicate: { all: [] } }) + expect(written.join('')).toContain('"sim tables count create" has been renamed') + }) + + it('still answers to a path whose group became the command itself', async () => { + // The hardest shape: `files restore create` retired in favour of `files + // restore`, so the old path needs a `create` *under* a command that now + // takes `` there. Commander matches the subcommand before the + // positional, which is what makes both spellings reachable. + const written = warnings() + const [path] = await run(['files', 'restore', 'create', 'wf_1'], { data: { id: 'wf_1' } }) + expect(path).toBe('/api/v2/files/wf_1/restore') + expect(written.join('')).toContain('"sim files restore create" has been renamed') + + const [current] = await run(['files', 'restore', 'wf_1'], { data: { id: 'wf_1' } }) + expect(current).toBe('/api/v2/files/wf_1/restore') + }) + + it('keeps retired spellings out of help', () => { + // A retired name exists for scripts, not for readers: surfacing it in help + // would teach the spelling being retired. Commander still lists a hidden + // command in `.commands`, so this asks what help itself would print. + const visible = (command: Command) => + command.commands + .filter((child) => (child as Command & { _hidden?: boolean })._hidden !== true) + .map((child) => child.name()) + + expect(visible(commandAt('tables'))).not.toContain('count') + expect(visible(commandAt('workflows', 'deployment'))).not.toContain('list') + expect(visible(commandAt('files', 'restore'))).toEqual([]) + expect( + commandAt('tables', 'rows', 'count') + .options.filter((option) => !option.hidden) + .map((option) => option.flags) + ).not.toContain('--predicate ') + }) + + it('folds a retired flag onto its current name', async () => { + const written = warnings() + const [, options] = await run(['tables', 'rows', 'find', 'tbl_1', '--q', 'needle'], { + data: { matches: [] }, + }) + expect(options.body).toMatchObject({ q: 'needle' }) + expect(written.join('')).toContain('"--q" has been renamed to "--query"') + }) + + it('refuses both spellings of one flag rather than picking a winner', async () => { + await expect( + run([ + 'tables', + 'rows', + 'count', + 'tbl_1', + '--predicate', + '{"all":[]}', + '--filter', + '{"any":[]}', + ]) + ).rejects.toThrow('--predicate is the former name of --filter; pass one, not both') + }) + + it('still requires a renamed-but-required field, naming its current spelling', async () => { + // The current flag cannot be commander-mandatory or the retired spelling + // would be rejected before it could be folded, so the requirement is raised + // downstream instead. It must still be raised. + await expect(run(['tables', 'rows', 'find', 'tbl_1'])).rejects.toThrow('--query is required') + }) + + it('never lets a retired path shadow a live command', () => { + const seen = new Map() + const walk = (command: Command, prefix: string[]) => { + for (const child of command.commands) { + const path = [...prefix, child.name()].join(' ') + const hidden = (child as Command & { _hidden?: boolean })._hidden === true + // Commander resolves a duplicate name to whichever was registered + // first, so a retired path sharing a live command's name would make the + // live one unreachable. + expect(seen.has(path) && !hidden).toBe(false) + seen.set(path, hidden) + walk(child, [...prefix, child.name()]) + } + } + walk(program(), []) + }) +}) diff --git a/packages/sim-cli/src/runtime/build.ts b/packages/sim-cli/src/runtime/build.ts index 0ffb597f79c..958ccce154e 100644 --- a/packages/sim-cli/src/runtime/build.ts +++ b/packages/sim-cli/src/runtime/build.ts @@ -5,6 +5,7 @@ import { V2_OPERATIONS, type V2OperationName } from '../generated/v2-api' import { deriveCommandPath } from './derive' import { executeOperation } from './execute' import { addOperationOptions } from './options' +import { warnRenamedCommand } from './renamed' import { flagNameFor, flagSpecFor, isProfileWorkspacePath, PROFILE_INJECTED_FIELD } from './request' import type { OperationSpec } from './types' @@ -155,6 +156,41 @@ function buildLeaf(operation: V2OperationName, spec: CommandSpec, leafName: stri return addMissingArgumentExample(configureOperation(new Command(leafName), operation, spec)) } +/** + * Registers a command at a path it used to have, hidden and warning on use. + * + * The leaf is built by the same `buildLeaf` the current spelling uses, so a + * renamed command cannot drift from the one it forwards to — there is one + * definition and two ways to reach it. + * + * Commander resolves a subcommand before it fills a positional, so a rename + * that turned a group into a leaf (`files restore create` into `files restore`) + * still parses: `create` is matched as the hidden subcommand rather than being + * read as the file id. The cost is that a resource whose id is literally + * `create` cannot be addressed through the current spelling, which no id + * generated by `@sim/utils/id` ever is. + */ +function addRenamedCommand( + groups: Map, + operation: V2OperationName, + spec: CommandSpec, + from: string, + to: string +): void { + const segments = from.split(' ') + const [groupName, ...rest] = segments + if (rest.length === 0) throw new Error(`${operation}.renamedFrom "${from}" must include a verb`) + + let parent = groupFor(groups, groupName) + for (const segment of rest.slice(0, -1)) { + parent = nestedGroup(parent, segment, { hidden: true }) + } + + const leaf = buildLeaf(operation, spec, rest[rest.length - 1]) + leaf.hook('preAction', () => warnRenamedCommand(from, to)) + parent.addCommand(leaf, { hidden: true }) +} + function groupFor(groups: Map, name: string): Command { const existing = groups.get(name) if (existing) return existing @@ -171,14 +207,20 @@ function resourceLabel(name: string): string { return label.replaceAll('-', ' ') } -function nestedGroup(parent: Command, name: string): Command { +/** + * `hidden` applies only when this call is what creates the group. A rename that + * reaches through a group the current surface also uses (`tables rows`) must + * leave it in help; only a group resurrected solely to host a renamed leaf + * (`tables count`) stays hidden. + */ +function nestedGroup(parent: Command, name: string, options: { hidden?: boolean } = {}): Command { const existing = parent.commands.find((candidate) => candidate.name() === name) if (existing) return existing const created = new Command(name).description( `Manage ${resourceLabel(parent.name())} ${name.replaceAll('-', ' ')}` ) - parent.addCommand(created) + parent.addCommand(created, { hidden: options.hidden }) return created } @@ -217,6 +259,12 @@ function variantCommandSpec(spec: CommandSpec, variant: CommandVariantSpec): Com /** Builds every JSON command described by the generated operation table. */ export function buildGeneratedCommands(): Command[] { const groups = new Map() + const renamed: Array<{ + operation: V2OperationName + spec: CommandSpec + from: string + to: string + }> = [] for (const operation of Object.keys(V2_OPERATIONS) as V2OperationName[]) { const spec = CLI_CONTRACT[operation] ?? {} @@ -247,6 +295,18 @@ export function buildGeneratedCommands(): Command[] { variant.command.split(' ') ) } + + for (const from of spec.renamedFrom ?? []) { + renamed.push({ operation, spec, from, to: segments.join(' ') }) + } + } + + // Second pass on purpose. Commander resolves a duplicate name to whichever + // was registered first, so registering every current spelling before any + // renamed one makes it impossible for a retired path to shadow a live command + // that happens to reuse its name. + for (const { operation, spec, from, to } of renamed) { + addRenamedCommand(groups, operation, spec, from, to) } return [...groups.values()].sort((a, b) => a.name().localeCompare(b.name())) diff --git a/packages/sim-cli/src/runtime/execute.ts b/packages/sim-cli/src/runtime/execute.ts index 5b3ebd83e70..d0ac08f7845 100644 --- a/packages/sim-cli/src/runtime/execute.ts +++ b/packages/sim-cli/src/runtime/execute.ts @@ -2,9 +2,10 @@ import type { Command } from 'commander' import { clientFrom } from '../context' import type { CommandSpec } from '../contract/types' import type { V2OperationName } from '../generated/v2-api' -import { SimApiError, type V2Page } from '../http/client' +import { pageProgress, SimApiError, type V2Page } from '../http/client' import { camel } from './derive' import { DEFAULT_LIMIT } from './options' +import { warnRenamedFlag } from './renamed' import { buildRequest, flagNameFor, @@ -20,6 +21,41 @@ function cursorSlot(operationSpec: OperationSpec): 'query' | 'body' | null { return null } +/** + * Moves a value supplied under a flag's former name onto its current one. + * + * Done here rather than in `buildRequest` because this is where the parsed + * flags are assembled and still keyed by what the caller typed; by the time the + * request is built, only the current spelling has meaning. + * + * Supplying both spellings is refused rather than resolved. They are the same + * field, so a caller who sets both has two different values in mind and no + * reading of "the new one wins" is more likely to be the intended one. + */ +function foldRenamedFlags( + operation: V2OperationName, + commandSpec: CommandSpec, + flags: Record +): void { + for (const [field, flag] of Object.entries(commandSpec.flags ?? {})) { + if (!flag.renamedFrom?.length) continue + + const current = flagNameFor(operation, field) + for (const previous of flag.renamedFrom) { + const supplied = flags[camel(previous)] + if (supplied === undefined) continue + if (flags[camel(current)] !== undefined) { + throw new SimApiError( + `--${previous} is the former name of --${current}; pass one, not both`, + 0 + ) + } + warnRenamedFlag(previous, current) + flags[camel(current)] = supplied + } + } +} + /** Executes a parsed generated command, including cursor pagination. */ export async function executeOperation( operation: V2OperationName, @@ -45,6 +81,8 @@ export async function executeOperation( requestFlags[camel(flagNameFor(operation, field))] = invocation[pathPositionalCount + index] } + foldRenamedFlags(operation, commandSpec, requestFlags) + if (commandSpec.confirm && !requestFlags.yes) { throw new SimApiError(`${commandSpec.confirm} Re-run with --yes to confirm.`, 0) } @@ -77,6 +115,7 @@ export async function executeOperation( const pageSize = Math.min(Number.isFinite(limit) ? limit : DEFAULT_LIMIT, DEFAULT_LIMIT) const pageLimit = 'limit' in (operationSpec[paging] ?? {}) ? { limit: pageSize } : {} const rows: unknown[] = [] + const progress = pageProgress() let cursor: string | null = null do { @@ -90,8 +129,10 @@ export async function executeOperation( }) rows.push(...page.data) cursor = page.nextCursor + if (cursor && rows.length < limit) progress.advance(rows.length) } while (cursor && rows.length < limit) + progress.finish() renderPage(profile.output, Number.isFinite(limit) ? rows.slice(0, limit) : rows, commandSpec) return } diff --git a/packages/sim-cli/src/runtime/options.test.ts b/packages/sim-cli/src/runtime/options.test.ts new file mode 100644 index 00000000000..83831c915ad --- /dev/null +++ b/packages/sim-cli/src/runtime/options.test.ts @@ -0,0 +1,38 @@ +/** + * @vitest-environment node + */ +import { Command } from 'commander' +import { describe, expect, it } from 'vitest' +import { addOperationOptions } from './options' +import type { OperationSpec } from './types' + +const DELETE_TABLE: OperationSpec = { + method: 'DELETE', + path: '/api/v2/tables/{tableId}', + pathParams: ['tableId'], +} + +function confirmHelp(): string { + const command = new Command('delete') + addOperationOptions( + command, + 'deleteTable', + { confirm: 'This deletes the table and all of its rows.' }, + DELETE_TABLE + ) + return command.helpInformation() +} + +describe('the --yes flag on a destructive command', () => { + /** + * `executeOperation` throws unless `--yes` is present, whether or not stdin is + * a terminal — nothing anywhere prompts. Advertising a confirmation to skip + * described a question the CLI never asks. + */ + it('describes itself as the confirmation, not as skipping one', () => { + const help = confirmHelp() + expect(help).toMatch(/-y, --yes\s+Confirm this destructive operation \(required\)/) + expect(help).not.toMatch(/skip/i) + expect(help).not.toMatch(/prompt/i) + }) +}) diff --git a/packages/sim-cli/src/runtime/options.ts b/packages/sim-cli/src/runtime/options.ts index 6fa18ea6ae7..54081cd0b4e 100644 --- a/packages/sim-cli/src/runtime/options.ts +++ b/packages/sim-cli/src/runtime/options.ts @@ -93,13 +93,25 @@ function addFieldOption( : '' }${descriptor.required ? ' (required)' : ''}` + const renamedFrom = flag.renamedFrom ?? [] const option = new Option(`${short}--${name} ${placeholder}`, describe) if (choices && !takesList) option.choices([...choices]) if (descriptor.default !== undefined && field !== 'limit') { option.default(undefined, String(descriptor.default)) } - if (descriptor.required) option.makeOptionMandatory() + // Commander's mandatory check runs before `executeOperation` can fold a + // renamed spelling onto the current one, so a required field that has been + // renamed would reject the very argv this exists to keep working. The + // requirement is not lost: `buildRequest` raises it against the current + // spelling once both have had their chance to supply the value. + if (descriptor.required && renamedFrom.length === 0) option.makeOptionMandatory() command.addOption(option) + + for (const previous of renamedFrom) { + const retired = new Option(`--${previous} ${placeholder}`).hideHelp() + if (choices && !takesList) retired.choices([...choices]) + command.addOption(retired) + } } /** Adds request-field and safety options for one generated operation. */ @@ -162,6 +174,10 @@ export function addOperationOptions( } if (commandSpec.confirm) { - command.option('-y, --yes', 'Skip the confirmation') + // There is no prompt to skip: a `confirm` command refuses outright when the + // flag is absent, in a TTY or not. Calling it "Skip the confirmation" sent + // readers looking for a question the CLI never asks, and hid that the flag + // is the only way the command ever runs. + command.option('-y, --yes', 'Confirm this destructive operation (required)') } } diff --git a/packages/sim-cli/src/runtime/renamed.ts b/packages/sim-cli/src/runtime/renamed.ts new file mode 100644 index 00000000000..6eabc7865b3 --- /dev/null +++ b/packages/sim-cli/src/runtime/renamed.ts @@ -0,0 +1,41 @@ +/** + * Support for spellings the CLI has moved on from. + * + * A rename is not an alias. {@link CommandSpec.aliases} are ergonomic shorthands + * — `ls`, `mv` — that the CLI wants people to use, so they appear in help. A + * renamed spelling is kept only so a script written against the old name keeps + * working: it stays out of help and out of the generated docs, and says once, + * on stderr, what to write instead. + * + * Warnings go to stderr rather than stdout because the old name is most likely + * to survive inside exactly the kind of script that pipes stdout into `jq`, and + * a deprecation notice in the middle of a JSON document is a worse bug than the + * one it reports. + */ + +/** Reported spellings, so a loop over many rows warns once rather than per row. */ +const warned = new Set() + +function warn(kind: string, from: string, to: string): void { + const key = `${kind}:${from}` + if (warned.has(key)) return + warned.add(key) + process.stderr.write( + `warning: ${kind} "${from}" has been renamed to "${to}". The old name still works.\n` + ) +} + +/** Announces a command path that has been renamed, naming its current spelling. */ +export function warnRenamedCommand(from: string, to: string): void { + warn('command', `sim ${from}`, `sim ${to}`) +} + +/** Announces a flag that has been renamed, naming its current spelling. */ +export function warnRenamedFlag(from: string, to: string): void { + warn('flag', `--${from}`, `--${to}`) +} + +/** Test seam: renames warn once per process, and each test needs a clean slate. */ +export function resetRenameWarnings(): void { + warned.clear() +} diff --git a/packages/sim-cli/src/runtime/request.test.ts b/packages/sim-cli/src/runtime/request.test.ts index dc8295e70d6..f419c078af6 100644 --- a/packages/sim-cli/src/runtime/request.test.ts +++ b/packages/sim-cli/src/runtime/request.test.ts @@ -47,11 +47,24 @@ describe('buildRequest', () => { }) it('omits absent optional fields so the server applies its own default', () => { + // Except where the contract asks for one, as `details` does below. const built = buildRequest('listLogs', [], {}, WORKSPACE) - expect(built.query).toEqual({ workspaceId: WORKSPACE }) + expect(built.query).toEqual({ workspaceId: WORKSPACE, details: 'full' }) expect(built.query).not.toHaveProperty('order') }) + it('asks for the detail level its own declared columns read from', () => { + // `logs list` renders `workflow.name`, which the API sends only at `full`, + // so the default request left the workflow column empty on every row. + const built = buildRequest('listLogs', [], {}, WORKSPACE) + expect(built.query.details).toBe('full') + }) + + it('lets an explicit detail level override the contract default', () => { + const built = buildRequest('listLogs', [], { details: 'basic' }, WORKSPACE) + expect(built.query.details).toBe('basic') + }) + it('never sends a field the contract marked omit', () => { // `stream` would switch the response to SSE, which the JSON client cannot read. const built = buildRequest('executeWorkflow', ['wf_1'], { stream: true }, WORKSPACE) @@ -253,3 +266,79 @@ describe('JSON flags that name a file', () => { expect(() => coerce('{"a":', field, {}, 'workflow')).not.toThrow(/@path/) }) }) + +describe('folder paths are typed by the name the app shows', () => { + it('encodes a space so the visible folder name is what the caller types', () => { + const built = buildRequest('listWorkflows', [], { folder: '/Folder 1' }, WORKSPACE) + expect(built.query.folderPath).toBe('/Folder%201') + }) + + it('leaves an already-encoded path alone, because that is the form it prints', () => { + // `workflows ls` prints the wire form in its `ref` column and the README + // uses it, so the value people paste back must not become `%2520`. + const built = buildRequest('listWorkflows', [], { folder: '/Folder%201' }, WORKSPACE) + expect(built.query.folderPath).toBe('/Folder%201') + }) + + it('encodes each segment and keeps the separators between them', () => { + const built = buildRequest('listTables', [], { folder: '/cli-test-a/nested one' }, WORKSPACE) + expect(built.query.folderPath).toBe('/cli-test-a/nested%20one') + }) + + it('still treats the leading slash as optional', () => { + const built = buildRequest('createTableFolder', [], { path: 'cli-test-noslash' }, WORKSPACE) + expect(built.body).toMatchObject({ path: 'cli-test-noslash' }) + }) + + it('escapes the characters encodeURIComponent leaves raw', () => { + // The route re-encodes each segment and demands a byte-for-byte match, and + // `encodeURIComponent` alone leaves `!'()*` alone — so `/Q1 (draft)` went + // out as `/Q1%20(draft)` and came back "Path must be a canonical folder + // path". Folder names like these are ordinary. + const built = buildRequest('createTableFolder', [], { path: "/Q1 (draft)/Sam's !*" }, WORKSPACE) + expect(built.body).toMatchObject({ path: '/Q1%20%28draft%29/Sam%27s%20%21%2A' }) + }) + + it('spells out a dot segment, which the API refuses to read as a relative path', () => { + const built = buildRequest('createTableFolder', [], { path: '/./..' }, WORKSPACE) + expect(built.body).toMatchObject({ path: '/%2E/%2E%2E' }) + }) + + it('leaves the canonical form it prints unchanged when pasted back', () => { + // Every one of these is what the CLI's own `ref` column shows, so it is what + // people paste into the next command; re-encoding it must be a no-op. + for (const name of ['Q1 (draft)', "Sam's stuff", 'wow!', 'a*b', '.', '..', '50% off']) { + const canonical = buildRequest('createTableFolder', [], { path: `/${name}` }, WORKSPACE).body + ?.path as string + const again = buildRequest('createTableFolder', [], { path: canonical }, WORKSPACE) + expect(again.body).toMatchObject({ path: canonical }) + } + }) + + it('encodes a literal percent that is not an escape', () => { + const built = buildRequest('createTableFolder', [], { path: '/50% off' }, WORKSPACE) + expect(built.body).toMatchObject({ path: '/50%25%20off' }) + }) + + it('encodes both ends of a folder move', () => { + const built = buildRequest( + 'relocateTableFolder', + [], + { path: '/old name', destination: '/new name' }, + WORKSPACE + ) + expect(built.body).toMatchObject({ path: '/old%20name', destinationPath: '/new%20name' }) + }) + + it('encodes every value of the repeatable folder filter before joining them', () => { + const built = buildRequest('listLogs', [], { folder: ['/a b', '/c'] }, WORKSPACE) + expect(built.query.folderPaths).toBe('/a%20b,/c') + }) + + it('leaves a field the contract has not marked untouched', () => { + // `files upload` and `knowledge documents upload` take a LOCAL path; the + // marker is what keeps the encoder away from one. + const local = './My Docs/report.pdf' + expect(coerce(local, { kind: 'string' }, {}, 'file')).toBe(local) + }) +}) diff --git a/packages/sim-cli/src/runtime/request.ts b/packages/sim-cli/src/runtime/request.ts index f30ff7e3f35..bb15831c9e0 100644 --- a/packages/sim-cli/src/runtime/request.ts +++ b/packages/sim-cli/src/runtime/request.ts @@ -156,6 +156,64 @@ function readListValues(raw: unknown, flagName: string): string[] { }) } +/** A percent-escape the caller has already applied, well-formed enough to decode. */ +const PERCENT_ESCAPE = /%[0-9A-Fa-f]{2}/ + +/** What `encodeURIComponent` leaves raw and the server's canonical encoder does not. */ +const SUB_DELIMITERS = /[!'()*]/g + +/** + * Encodes one segment exactly as `encodeFolderPathSegment` does server-side. + * + * The route does not merely decode a path, it re-encodes each segment and + * demands the result match byte for byte, so "close enough" is rejected outright + * with `Path must be a canonical folder path`. `encodeURIComponent` alone leaves + * `!'()*` raw — common in real folder names (`Q1 (draft)`, `Sam's stuff`) — and + * spells a lone `.` or `..` as itself, which the server refuses to let address a + * folder actually named that. + */ +function encodeFolderPathSegment(name: string): string { + if (name === '.') return '%2E' + if (name === '..') return '%2E%2E' + return encodeURIComponent(name).replace( + SUB_DELIMITERS, + (character) => `%${character.charCodeAt(0).toString(16).toUpperCase()}` + ) +} + +/** + * Rewrites one folder path into the API's canonical wire form. + * + * The wire form encodes each segment, so `/Folder 1` in the app is + * `/Folder%201` to the API — and typing the name you can see was rejected with + * a message that never said the word encoding. Splitting on `/` first is what + * keeps the separators: `encodeURIComponent` over the whole path would turn + * every one of them into `%2F` and address a single top-level folder whose name + * contains slashes. + * + * Decoding each segment before encoding it is what makes this idempotent, and + * it has to be: the encoded spelling is what the CLI prints today, what the + * README shows, and therefore what people will paste back. `/Folder 1` and + * `/Folder%201` must reach the same folder, and `%2520` is the failure to + * avoid. The limit of that rule is a folder whose name really contains a `%` + * followed by two hex digits — `100%20off` reads as `100 off`. A stray `%` is + * safe, because it fails to decode and is encoded literally, and the ambiguous + * name can always be typed in its encoded form (`100%2520off`). + */ +export function encodeFolderPath(value: string): string { + return value + .split('/') + .map((segment) => { + if (!PERCENT_ESCAPE.test(segment)) return encodeFolderPathSegment(segment) + try { + return encodeFolderPathSegment(decodeURIComponent(segment)) + } catch { + return encodeFolderPathSegment(segment) + } + }) + .join('/') +} + /** * Points at `@` when a value that failed to parse looks like a filename. * @@ -192,7 +250,12 @@ export function coerce(raw: unknown, field: FieldSpec, flag: FlagSpec, flagName: * or failed validation outright. */ if (flag.list) { - const values = readListValues(raw, flagName) + const values = readListValues(raw, flagName).map((value) => + flag.folderPath ? encodeFolderPath(value) : value + ) + // Encoding first is also what keeps the comma-joined form unambiguous: a + // folder name containing a comma leaves here as `%2C`, so the route's split + // cannot cut one path in half. return field.kind === 'string' ? values.join(',') : values } @@ -222,6 +285,8 @@ export function coerce(raw: unknown, field: FieldSpec, flag: FlagSpec, flagName: throw new SimApiError(`--${flagName} must be one of: ${choices.join(', ')}`, 0) } + if (flag.folderPath && typeof raw === 'string') return encodeFolderPath(raw) + return raw } @@ -311,12 +376,16 @@ export function buildRequest( // Commander stores `--min-duration-ms` as `minDurationMs`; reading by the // flag's own name silently finds nothing. const omitProfileWorkspace = commandSpec.allWorkspaces && flags.allWorkspaces === true - const raw = + const provided = field === PROFILE_INJECTED_FIELD ? omitProfileWorkspace ? undefined : workspaceId : flags[camel(flagName)] + // A contract default only applies to what the caller left unsaid, so + // typing the flag — including typing the server's own default back — still + // decides. It is validated like any other value, enum choices included. + const raw = provided ?? flag.requestDefault const value = coerce(raw ?? undefined, descriptor, flag, flagName) if (value === undefined) { diff --git a/packages/sim-cli/src/runtime/result.test.ts b/packages/sim-cli/src/runtime/result.test.ts new file mode 100644 index 00000000000..9bb79216577 --- /dev/null +++ b/packages/sim-cli/src/runtime/result.test.ts @@ -0,0 +1,222 @@ +/** + * @vitest-environment node + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { CLI_CONTRACT } from '../contract/commands' +import type { CommandSpec } from '../contract/types' +import { renderPage, renderResult } from './result' + +let logged: string[] + +beforeEach(() => { + logged = [] + vi.spyOn(console, 'log').mockImplementation((line: string) => { + logged.push(line) + }) +}) + +afterEach(() => { + vi.restoreAllMocks() +}) + +/** The table arrives as one string; its first line is the header. */ +function tableLines(): string[] { + return logged.join('\n').split('\n') +} + +describe('single-record output is only clamped for the human table', () => { + const url = `https://sim-storage.example.com/exports/probe.csv?X-Amz-Signature=${'a'.repeat(300)}` + + it('keeps the whole value in text, which exists to be piped', () => { + renderResult('tableExportDownload', 'text', { url }, {}) + expect(logged[0]).toBe(`url\t${url}`) + }) + + it('keeps the whole value in json', () => { + renderResult('tableExportDownload', 'json', { url }, {}) + expect(JSON.parse(logged[0])).toEqual({ url }) + }) + + it('clamps the value in table mode', () => { + renderResult('tableExportDownload', 'table', { url }, {}) + expect(logged[0]).toMatch(/…$/) + expect(logged[0].length).toBeLessThan(url.length) + }) +}) + +describe('inferred cells pick a format from the key shape', () => { + const row = { + createdAt: '2026-08-17T20:35:38.478Z', + durationMs: 9.145596999907866, + size: 3000000, + isActive: true, + deletedAt: null, + rowCount: 0, + displayName: 'probe', + } + + it('formats timestamps, durations, byte counts and booleans in a record', () => { + renderResult('getTable', 'text', row, {}) + expect(logged).toEqual([ + 'created at\t2026-08-17 20:35:38', + 'duration\t9ms', + 'size\t2.9 MB', + 'active\tyes', + 'deleted at\t', + 'row count\t0', + 'display name\tprobe', + ]) + }) + + it('de-camelCases inferred table headers', () => { + renderPage('table', [row], {}) + expect(tableLines()[0].split(/\s{2,}/)).toEqual([ + 'CREATED AT', + 'DURATION', + 'SIZE', + 'ACTIVE', + 'DELETED AT', + 'ROW COUNT', + 'DISPLAY NAME', + ]) + }) + + it('leaves json and yaml on the raw payload', () => { + renderResult('getTable', 'json', row, {}) + renderResult('getTable', 'yaml', row, {}) + expect(JSON.parse(logged[0])).toEqual(row) + expect(logged[1]).toContain('durationMs: 9.145596999907866') + }) + + it('infers nothing when the value type disagrees with the key', () => { + renderResult('getTable', 'text', { size: 'small', createdAt: 'whenever', isActive: 'yes' }, {}) + expect(logged).toEqual(['size\tsmall', 'created at\twhenever', 'is active\tyes']) + }) + + it('rounds a relevance score to a readable precision', () => { + renderResult('searchKnowledge', 'text', { similarity: 0.2818676545790171 }, {}) + expect(logged[0]).toBe('similarity\t0.2819') + }) + + it('leaves explicit column formats alone', () => { + const spec: CommandSpec = { + columns: [{ header: 'size', format: 'auto' }], + } + renderPage('text', [{ size: 3000000 }], spec) + expect(logged[0]).toBe('3000000') + }) +}) + +describe('cells the user named, not the API', () => { + // `tables rows list` and `tables rows query` expand `data`, whose keys are + // whatever the caller called their columns. A key shape is a promise about + // the value, and only the API's own field names carry one. + const rows = [{ id: 'row_1', data: { score: 3, size: 5, duration: 30, isBillable: true } }] + const spec: CommandSpec = { expand: 'data' } + + it('leaves a user column named like an API field alone', () => { + renderPage('text', rows, spec) + expect(logged[0]).toBe('row_1\t3\t5\t30\ttrue') + }) + + it('heads each one with the name the user has to type back into --filter', () => { + renderPage('table', rows, spec) + expect(tableLines()[0].split(/\s{2,}/)).toEqual([ + 'ID', + 'SCORE', + 'SIZE', + 'DURATION', + 'ISBILLABLE', + ]) + }) +}) + +describe('a folder path the operation declared no column for', () => { + it('is decoded in the record the create echoes back', () => { + // `tables folders create 'Reports/Q1 2026'` answered `/Reports/Q1%202026` + // while the `ls` right after it showed the same folder decoded. + renderResult( + 'createTableFolder', + 'text', + { name: 'Q1 2026', path: '/Reports/Q1%202026', parentPath: '/Reports' }, + {} + ) + expect(logged).toContain('path\t/Reports/Q1 2026') + }) + + it('stays in wire form in json, which is what gets fed back', () => { + renderResult('createTableFolder', 'json', { path: '/Reports/Q1%202026' }, {}) + expect(JSON.parse(logged[0])).toEqual({ path: '/Reports/Q1%202026' }) + }) +}) + +describe('a declared field that the API stops returning', () => { + const spec: CommandSpec = { + fields: [ + { header: 'plan' }, + { header: 'credits used', path: 'credits.used' }, + { header: 'credits limit', path: 'credits.limit' }, + ], + } + + it('is reported as absent rather than dropped', () => { + renderResult('getBillingStatus', 'table', { plan: 'team' }, spec) + expect(logged).toHaveLength(3) + expect(logged[1]).toContain('credits used') + expect(logged[2]).toContain('credits limit') + }) + + it('stays an empty field in text, so cut -f2 still lines up', () => { + renderResult('getBillingStatus', 'text', { plan: 'team' }, spec) + expect(logged).toEqual(['plan\tteam', 'credits used\t', 'credits limit\t']) + }) +}) + +describe('folder paths are shown by name, but piped in wire form', () => { + const folders = [ + { + path: '/cli-test-a/nested%20one', + name: 'nested one', + parentPath: '/cli-test-a', + updatedAt: '2026-08-17T20:35:38.478Z', + }, + ] + const spec = CLI_CONTRACT.listTableFolders as CommandSpec + + it('decodes the path in the table, which held it next to the decoded name', () => { + renderPage('table', folders, spec) + const [, row] = tableLines() + expect(row).toContain('/cli-test-a/nested one') + expect(row).not.toContain('%20') + }) + + it('decodes the path in text, the format shell plumbing reads', () => { + renderPage('text', folders, spec) + expect(logged[0].split('\t')[0]).toBe('/cli-test-a/nested one') + }) + + it('keeps the wire form in json, so a path fed back still resolves', () => { + renderPage('json', folders, spec) + expect(JSON.parse(logged[0])[0].path).toBe('/cli-test-a/nested%20one') + }) + + it('keeps the wire form in yaml for the same reason', () => { + renderPage('yaml', folders, spec) + expect(logged[0]).toContain('/cli-test-a/nested%20one') + }) + + it('decodes a declared record field too', () => { + renderResult( + 'getFile', + 'text', + { id: 'f_1', folderPath: '/cli-test-a/nested%20one' }, + CLI_CONTRACT.getFile as CommandSpec + ) + expect(logged).toContain('folder\t/cli-test-a/nested one') + }) + + it('shows an undecodable path as it arrived rather than dropping it', () => { + renderPage('text', [{ path: '/100%zz', name: 'x', parentPath: '/', updatedAt: null }], spec) + expect(logged[0].split('\t')[0]).toBe('/100%zz') + }) +}) diff --git a/packages/sim-cli/src/runtime/result.ts b/packages/sim-cli/src/runtime/result.ts index e803f45934d..664a06d1ba9 100644 --- a/packages/sim-cli/src/runtime/result.ts +++ b/packages/sim-cli/src/runtime/result.ts @@ -38,6 +38,32 @@ function at(row: unknown, path: string): unknown { ) } +/** + * Undoes the wire encoding of a folder path for the human formats. + * + * The inverse of `encodeFolderPath`, per segment for the same reason: `%2F` is + * a slash inside one folder's name, not a separator. A segment that fails to + * decode is shown as it arrived rather than dropped — the point is to show the + * name, and a malformed one is still the truth about what the server holds. + * + * Callers must reach this only from a `table` or `text` rendering path — the + * hand-written `ls` builds its own columns and so decodes through here directly. + * `json` and `yaml` render from the raw payload so that switching format never + * changes the data, and a script piping a path back needs the wire form. + */ +export function decodeFolderPath(value: string): string { + return value + .split('/') + .map((segment) => { + try { + return decodeURIComponent(segment) + } catch { + return segment + } + }) + .join('/') +} + function renderCell( value: unknown, format: ColumnSpec['format'], @@ -56,6 +82,8 @@ function renderCell( return typeof value === 'number' ? `$${value.toFixed(4)}` : text(null) case 'count': return Array.isArray(value) ? String(value.length) : text(null) + case 'folder-path': + return typeof value === 'string' ? text(decodeFolderPath(value)) : text(value) case 'trace-count': { const count = countTraceSpans(value) return `${count} ${count === 1 ? 'span' : 'spans'}${ @@ -68,11 +96,95 @@ function renderCell( } } -const NESTED_CELL_WIDTH = 160 +/** ISO timestamps: `createdAt`, `updatedAt`, `expiresAt`, `startDate`. */ +const TIMESTAMP_KEY = /(?:At|Date)$/ +/** Millisecond durations: `durationMs`, `totalDurationMs`, `duration`. */ +const DURATION_KEY = /Ms$|^duration/ +/** Byte counts: `size`, `fileSize`, `usageBytes`. */ +const BYTES_KEY = /^size$|(?:Size|Bytes)$/ +/** Yes/no facts: `isActive`, `hasServiceAccountKey`. */ +const BOOL_KEY = /^(?:is|has)[A-Z]/ +/** Relevance scores in 0–1: `similarity`, `score`, `matchScore`. */ +const RATIO_KEY = /^(?:similarity|score)$|(?:Similarity|Score)$/ +/** + * Wire-encoded folder paths, the only `*Path` keys the v2 responses carry. + * + * The folder create, move and delete operations declare no columns, so their + * echo of the path fell through to the raw wire form — `sim tables folders + * create 'Reports/Q1 2026'` answered `/Reports/Q1%202026` and the `ls` right + * after it showed the same folder decoded. + */ +const FOLDER_PATH_KEY = /^(?:path|parentPath|folderPath)$/ + +/** Enough of an ISO stamp to be sure a string is one before parsing it as a date. */ +const ISO_TIMESTAMP = /^\d{4}-\d{2}-\d{2}[T ]\d{2}:\d{2}/ + +/** Decimals kept for a ratio; `0.2818676545790171` is noise past the fourth. */ +const RATIO_PRECISION = 4 + +/** + * Picks a renderer for a value the contract says nothing about, from the shape + * of its key. + * + * Most operations declare no `columns`/`fields`, so their output fell through to + * `String(value)` and printed raw ISO stamps, raw byte counts and raw float + * milliseconds next to sibling commands that format all three. The runtime type + * has to agree with the key before anything is inferred — a `size` that is a + * string is not a byte count, a `deletedAt` of `null` is not a date — so a + * mismatch falls back to the plain stringification rather than to `NaN`. + * + * Only ever asked about a key the API itself named. A key shape is a promise + * about the value, and only the contract's own field names carry one. + */ +function inferFormat(key: string, value: unknown): ColumnSpec['format'] | null { + if (typeof value === 'boolean') return BOOL_KEY.test(key) ? 'bool' : null + if (typeof value === 'string') { + if (FOLDER_PATH_KEY.test(key)) return 'folder-path' + return TIMESTAMP_KEY.test(key) && ISO_TIMESTAMP.test(value) && !Number.isNaN(Date.parse(value)) + ? 'timestamp' + : null + } + if (typeof value !== 'number' || !Number.isFinite(value)) return null + if (DURATION_KEY.test(key)) return 'duration' + if (BYTES_KEY.test(key)) return 'bytes' + return null +} + +function inferredCell(key: string, value: unknown): string { + if (typeof value === 'number' && Number.isFinite(value) && RATIO_KEY.test(key)) { + return value.toFixed(RATIO_PRECISION) + } + return renderCell(value, inferFormat(key, value) ?? 'auto') +} + +/** `latestOperationStatus` → `latest operation status`, `row_count` → `row count`. */ +function humanizeKey(key: string): string { + return key + .replace(/([a-z0-9])([A-Z])/g, '$1 $2') + .replace(/([A-Z]+)([A-Z][a-z])/g, '$1 $2') + .replace(/[_-]+/g, ' ') + .replace(/\s+/g, ' ') + .trim() + .toLowerCase() +} -function recordCell(value: unknown): string { - const rendered = renderCell(value, 'auto') - return rendered.length > NESTED_CELL_WIDTH ? `${rendered.slice(0, NESTED_CELL_WIDTH)}…` : rendered +/** + * Header for an inferred column or field. + * + * The raw key reached the terminal as `DISPLAYNAME` and `LATESTOPERATIONSTATUS` + * once the table upper-cased it. A unit suffix goes too when the value's + * formatter already prints the unit (`durationMs` heading a `9ms`), and so does + * the `is` of a boolean, which the yes/no value makes redundant. `has` stays: + * `has key` says something that `key` alone does not. + */ +function inferHeader(key: string, format: ColumnSpec['format'] | null): string { + const trimmed = + format === 'duration' || format === 'bytes' + ? key.replace(/(?:Ms|Bytes)$/, '') + : format === 'bool' + ? key.replace(/^is(?=[A-Z])/, '') + : key + return humanizeKey(trimmed || key) } function columnsFrom(specs: ColumnSpec[]): Column[] { @@ -87,14 +199,29 @@ function fieldsFrom( specs: ColumnSpec[], options: RenderResultOptions = {} ): Array<[string, string]> { - return specs.flatMap((spec) => { + return specs.map<[string, string]>((spec) => { const value = at(data, spec.path ?? spec.header) - return value === undefined ? [] : [[spec.header, renderCell(value, spec.format, options)]] + // A declared field is editorial: someone decided this record is not fully + // described without it. Dropping it when the API stops returning it made + // `billing status` print no credits at all and say nothing about it, so an + // absent field shows the same glyph a null one does. + return [spec.header, value === undefined ? text(null) : renderCell(value, spec.format, options)] }) } +/** + * Builds columns for a list the contract declares none for. + * + * A row's own keys are the API's, so their shape may be read as a promise about + * the value. The keys inside `expand` are not: `tables rows list` and `tables + * rows query` expand `data`, whose keys are the column names the *user* chose. + * Inferring there renamed their columns (`isBillable` heading as `BILLABLE`, + * which is no longer the string `--filter` wants back) and reformatted their + * values (a `score` of 3 as `3.0000`, a `size` of 5 as `5 B`). So an expanded + * cell keeps its literal key and its plain stringification. + */ function inferColumns(rows: unknown[], expand?: string): Column[] { - const paths: Array<{ path: string; header: string }> = [] + const paths: Array<{ path: string; key: string; header: string; owned: boolean }> = [] const seen = new Set() for (const row of rows) { @@ -103,7 +230,7 @@ function inferColumns(rows: unknown[], expand?: string): Column[] { if (seen.has(key)) continue if (value !== null && typeof value === 'object') continue seen.add(key) - paths.push({ path: key, header: key }) + paths.push({ path: key, key, header: inferHeader(key, inferFormat(key, value)), owned: true }) } } @@ -115,14 +242,24 @@ function inferColumns(rows: unknown[], expand?: string): Column[] { for (const key of Object.keys(container)) { if (nested.has(key)) continue nested.add(key) - paths.push({ path: `${expand}.${key}`, header: seen.has(key) ? `${expand}.${key}` : key }) + // The header keeps the container prefix only where the bare key would + // collide with one of the row's own columns. + paths.push({ + path: `${expand}.${key}`, + key, + header: seen.has(key) ? `${expand}.${key}` : key, + owned: false, + }) } } } - return paths.map(({ path, header }) => ({ + return paths.map(({ path, key, header, owned }) => ({ header: sanitize(header), - value: (row: unknown) => renderCell(at(row, path), 'auto'), + // The format is re-inferred per row: the first row decided the header, but a + // later row may hold a different type under the same key. + value: (row: unknown) => + owned ? inferredCell(key, at(row, path)) : renderCell(at(row, path), 'auto'), })) } @@ -182,7 +319,10 @@ export function renderResult( const fields = spec.fields ? fieldsFrom(data, spec.fields, options) : data && typeof data === 'object' - ? Object.entries(data).map<[string, string]>(([key, value]) => [key, recordCell(value)]) + ? Object.entries(data).map<[string, string]>(([key, value]) => [ + inferHeader(key, inferFormat(key, value)), + inferredCell(key, value), + ]) : [] printRecord(format, fields, data) diff --git a/scripts/generate-cli-docs.ts b/scripts/generate-cli-docs.ts index 7eb87392d0c..2b2f5be0190 100644 --- a/scripts/generate-cli-docs.ts +++ b/scripts/generate-cli-docs.ts @@ -79,8 +79,28 @@ function titleFor(name: string): string { .join(' ') } +/** + * Commander records a hidden command on a private field and offers no getter, + * so this narrows structurally rather than widening the command to `any`. + */ +function isHiddenCommand(command: Command): boolean { + return (command as Command & { _hidden?: boolean })._hidden === true +} + +/** Every option a reader should be taught, in declaration order. */ +function documentedOptions(command: Command): Command['options'] { + return command.options.filter((option) => !option.hidden) +} + +/** + * Hidden entries are excluded for the same reason `--help` omits them: they are + * spellings the CLI has retired and keeps working only so an existing script + * does not break. Documenting one would teach the name being retired. + */ function subcommands(command: Command): Command[] { - return command.commands.filter((child) => child.name() !== HELP_COMMAND) + return command.commands.filter( + (child) => child.name() !== HELP_COMMAND && !isHiddenCommand(child) + ) } /** Depth-first walk yielding every leaf command, in the order commander lists them. */ @@ -144,17 +164,35 @@ function usageLine(entry: DocumentedCommand): string { const name = argument.variadic ? `${argument.name()}...` : argument.name() parts.push(argument.required ? `<${name}>` : `[${name}]`) } - if (entry.command.options.length > 0) parts.push('[options]') + if (documentedOptions(entry.command).length > 0) parts.push('[options]') return parts.join(' ') } +const REQUIRED_SUFFIX = /\s*\(required\)\s*$/i + /** * Commander help already spells required-ness inside the description of a * derived flag. The table states it in its own column, so the trailing marker * would read as "Yes | Workflow ID (required)". */ function stripRequiredSuffix(description: string): string { - return description.replace(/\s*\(required\)\s*$/i, '') + return description.replace(REQUIRED_SUFFIX, '') +} + +/** + * Whether the flag must be supplied for the command to run. + * + * `option.mandatory` alone under-reports it. A destructive command's `--yes` is + * enforced by the runtime rather than by Commander, deliberately: making it + * mandatory would replace the refusal that names the consequence ("This deletes + * the knowledge base and every document in it. Re-run with --yes to confirm.") + * with Commander's bare "required option '--yes' not specified". The flag is + * still required, and the description says so — which is the same marker + * {@link stripRequiredSuffix} removes, so reading it here keeps the column and + * the prose from contradicting each other. + */ +function isRequiredOption(option: Command['options'][number]): boolean { + return option.mandatory || REQUIRED_SUFFIX.test(option.description || '') } /** Help text is written without terminal punctuation; appended clauses need it. */ @@ -208,12 +246,12 @@ function renderArguments(entry: DocumentedCommand): string[] { } function renderOptions(entry: DocumentedCommand): string[] { - const options = entry.command.options + const options = documentedOptions(entry.command) if (options.length === 0) return [] const rows = options.map( (option) => - `| ${code(option.flags)} | ${option.mandatory ? 'Yes' : 'No'} | ${describeOption(option)} |` + `| ${code(option.flags)} | ${isRequiredOption(option) ? 'Yes' : 'No'} | ${describeOption(option)} |` ) return [ @@ -393,7 +431,9 @@ function renderReferencePage( '', '| Option | Description |', '| --- | --- |', - ...program.options.map((option) => `| ${code(option.flags)} | ${describeOption(option)} |`), + ...documentedOptions(program).map( + (option) => `| ${code(option.flags)} | ${describeOption(option)} |` + ), '', ] @@ -467,7 +507,9 @@ function renderIndexPage( '', '| Option | Description |', '| --- | --- |', - ...program.options.map((option) => `| ${code(option.flags)} | ${describeOption(option)} |`), + ...documentedOptions(program).map( + (option) => `| ${code(option.flags)} | ${describeOption(option)} |` + ), '', '## Command groups', '', From 00fca625c3fca9d4915f1d39d45022f1cc85c938 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 17 Aug 2026 15:38:19 -0700 Subject: [PATCH 2/3] fix(cli): clear the paging progress line when a page fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Progress is written without a trailing newline so it can be overwritten in place, and both paging loops cleaned it up only on success. A page that threw part-way through left `fetched 1200…` on the line the error was then printed onto, so the two ran together. --- packages/sim-cli/src/http/client.test.ts | 24 ++++++++++++++++ packages/sim-cli/src/http/client.ts | 35 ++++++++++++++---------- packages/sim-cli/src/runtime/execute.ts | 34 +++++++++++++---------- 3 files changed, 63 insertions(+), 30 deletions(-) diff --git a/packages/sim-cli/src/http/client.test.ts b/packages/sim-cli/src/http/client.test.ts index 53f9393b59b..639c999263a 100644 --- a/packages/sim-cli/src/http/client.test.ts +++ b/packages/sim-cli/src/http/client.test.ts @@ -93,6 +93,30 @@ describe('cursor pagination', () => { expect(stderr.writes[1]).toBe('\r\u001b[K') }) + it('clears the progress line when a later page fails', async () => { + // Progress is written without a trailing newline so it can be overwritten in + // place. Cleaning up only on success left `fetched 2…` on the line the error + // was then printed onto, so the two ran together. + const request = vi + .fn() + .mockResolvedValueOnce({ data: ['a', 'b'], nextCursor: 'next' }) + .mockRejectedValueOnce(new Error('page two failed')) + const stderr = stubStderr(true) + + try { + await expect( + requestAllPages({ request } as Pick, '/api/v2/items', { + pageSize: 2, + }) + ).rejects.toThrow('page two failed') + } finally { + stderr.restore() + } + + expect(stderr.writes[0]).toContain('fetched 2') + expect(stderr.writes.at(-1)).toBe('\r\u001b[K') + }) + it('stays silent for a single page, and when stderr is not a terminal', async () => { const single = vi.fn().mockResolvedValue({ data: ['a'], nextCursor: null }) const paged = vi diff --git a/packages/sim-cli/src/http/client.ts b/packages/sim-cli/src/http/client.ts index 0e92021a519..3997e67a767 100644 --- a/packages/sim-cli/src/http/client.ts +++ b/packages/sim-cli/src/http/client.ts @@ -460,22 +460,27 @@ export async function requestAllPages( const items: T[] = [] const progress = pageProgress() let cursor: string | null = null - do { - const page: V2Page = await client.request>(path, { - ...requestOptions, - query: { - ...query, - limit: Math.min(pageSize, limit - items.length), - cursor, - }, - }) - items.push(...page.data) - cursor = page.nextCursor - - if (cursor && items.length < limit) progress.advance(items.length) - } while (cursor && items.length < limit) + // `finally`, because a page that throws part-way through would otherwise skip + // the cleanup and leave `fetched 1200…` sitting on the line the error is then + // written onto. + try { + do { + const page: V2Page = await client.request>(path, { + ...requestOptions, + query: { + ...query, + limit: Math.min(pageSize, limit - items.length), + cursor, + }, + }) + items.push(...page.data) + cursor = page.nextCursor - progress.finish() + if (cursor && items.length < limit) progress.advance(items.length) + } while (cursor && items.length < limit) + } finally { + progress.finish() + } return items.slice(0, limit) } diff --git a/packages/sim-cli/src/runtime/execute.ts b/packages/sim-cli/src/runtime/execute.ts index d0ac08f7845..e265cc3028d 100644 --- a/packages/sim-cli/src/runtime/execute.ts +++ b/packages/sim-cli/src/runtime/execute.ts @@ -118,21 +118,25 @@ export async function executeOperation( const progress = pageProgress() let cursor: string | null = null - do { - const page: V2Page = await client.request(request.path, { - method: operationSpec.method, - query: paging === 'query' ? { ...request.query, ...pageLimit, cursor } : request.query, - body: - paging === 'body' - ? { ...(request.body ?? {}), ...pageLimit, ...(cursor ? { cursor } : {}) } - : request.body, - }) - rows.push(...page.data) - cursor = page.nextCursor - if (cursor && rows.length < limit) progress.advance(rows.length) - } while (cursor && rows.length < limit) - - progress.finish() + // `finally`, for the same reason as `requestAllPages`: a page that throws + // would otherwise leave the progress text on the line the error prints onto. + try { + do { + const page: V2Page = await client.request(request.path, { + method: operationSpec.method, + query: paging === 'query' ? { ...request.query, ...pageLimit, cursor } : request.query, + body: + paging === 'body' + ? { ...(request.body ?? {}), ...pageLimit, ...(cursor ? { cursor } : {}) } + : request.body, + }) + rows.push(...page.data) + cursor = page.nextCursor + if (cursor && rows.length < limit) progress.advance(rows.length) + } while (cursor && rows.length < limit) + } finally { + progress.finish() + } renderPage(profile.output, Number.isFinite(limit) ? rows.slice(0, limit) : rows, commandSpec) return } From ac958acd1b91b35e39aec8ea80498d23f6553491 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 17 Aug 2026 15:44:39 -0700 Subject: [PATCH 3/3] fix(cli): name a working API root when an endpoint redirects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The suggested endpoint was the redirect target's origin, which drops a path prefix. A self-hosted deployment reached at https://host/sim was told to set https://www.host — not an API root, so following the advice replaced one broken endpoint with another. Derive it by stripping the request's own path from the target instead, so a prefix survives, and say nothing about --set-endpoint when the target resolves to the endpoint already configured: a trailing-slash or path normalization redirect keeps the origin, and naming the value the caller already has explains nothing. The login poll shared both faults and now shares the helper. --- packages/sim-cli/src/auth/device-flow.ts | 13 ++++-- packages/sim-cli/src/http/client.test.ts | 55 ++++++++++++++++++++++++ packages/sim-cli/src/http/client.ts | 34 +++++++++++++-- 3 files changed, 95 insertions(+), 7 deletions(-) diff --git a/packages/sim-cli/src/auth/device-flow.ts b/packages/sim-cli/src/auth/device-flow.ts index 5f2c5543f2c..84dfaf3c2ba 100644 --- a/packages/sim-cli/src/auth/device-flow.ts +++ b/packages/sim-cli/src/auth/device-flow.ts @@ -1,6 +1,6 @@ import { createHash, randomBytes, randomInt } from 'node:crypto' import { sleep } from '../helpers' -import { REDIRECT_STATUSES, SimApiError } from '../http/client' +import { REDIRECT_STATUSES, redirectEndpoint, SimApiError } from '../http/client' /** * The terminal half of the CLI key handoff. @@ -105,6 +105,9 @@ interface PollResponse { workspaceBound?: boolean } +/** The route the login poll targets; also the suffix a redirect target is measured against. */ +const POLL_PATH = '/api/cli/auth/poll' + /** * Explains a redirected poll rather than following it. * @@ -131,8 +134,12 @@ function toRedirectError(endpoint: string, response: Response): SimApiError { response.status ) } + const refusal = `${endpoint} redirected the login poll to ${target.href}. The CLI does not follow redirects, because a redirect drops the request body and would carry the login secret to another origin.` + const suggested = redirectEndpoint(endpoint, POLL_PATH, target) return new SimApiError( - `${endpoint} redirected the login poll to ${target.origin}. The CLI does not follow redirects, because a redirect drops the request body and would carry the login secret to another origin. Re-run with --endpoint ${target.origin}, or run: sim configure --set-endpoint ${target.origin}`, + suggested + ? `${refusal} Re-run with --endpoint ${suggested}, or run: sim configure --set-endpoint ${suggested}` + : refusal, response.status ) } @@ -158,7 +165,7 @@ export async function pollForKey( let response: Response | null = null try { - response = await fetch(new URL('/api/cli/auth/poll', endpoint), { + response = await fetch(new URL(POLL_PATH, endpoint), { method: 'POST', headers: { 'content-type': 'application/json', accept: 'application/json' }, body: JSON.stringify({ request: auth.request, verifier: auth.pollSecret }), diff --git a/packages/sim-cli/src/http/client.test.ts b/packages/sim-cli/src/http/client.test.ts index 639c999263a..eb56855742b 100644 --- a/packages/sim-cli/src/http/client.test.ts +++ b/packages/sim-cli/src/http/client.test.ts @@ -3,6 +3,7 @@ import { CLI_CONTRACT } from '../contract/commands' import { V2_OPERATIONS, type V2OperationName } from '../generated/v2-api' import { formatApiErrorDetails, + redirectEndpoint, requestAllPages, resolvePath, SimApiError, @@ -93,6 +94,60 @@ describe('cursor pagination', () => { expect(stderr.writes[1]).toBe('\r\u001b[K') }) + describe('the endpoint a redirect implies', () => { + // Naming `target.origin` dropped a self-hosted endpoint's path prefix, so + // the suggested value was not an API root and following the advice broke a + // deployment that was one hostname away from working. + it('keeps a path prefix the endpoint carries', () => { + expect( + redirectEndpoint( + 'https://host/sim', + '/api/v2/workflows', + new URL('https://www.host/sim/api/v2/workflows') + ) + ).toBe('https://www.host/sim') + }) + + it('is just the origin when the endpoint has no prefix', () => { + expect( + redirectEndpoint( + 'https://sim.example', + '/api/v2/workflows', + new URL('https://www.sim.example/api/v2/workflows') + ) + ).toBe('https://www.sim.example') + }) + + it('implies no change when the target resolves to the endpoint already set', () => { + // A trailing-slash or path-normalization redirect keeps the origin; + // advising the value the caller already has explains nothing. + expect( + redirectEndpoint( + 'https://sim.example', + '/api/v2/workflows', + new URL('https://sim.example/api/v2/workflows/') + ) + ).toBeNull() + expect( + redirectEndpoint( + 'https://sim.example/', + '/api/v2/x', + new URL('https://sim.example/api/v2/x') + ) + ).toBeNull() + }) + + it('falls back to the origin when the target does not carry the request path', () => { + expect( + redirectEndpoint( + 'https://sim.example', + '/api/v2/workflows', + new URL('https://auth.example/login') + ) + ).toBe('https://auth.example') + }) + }) + it('clears the progress line when a later page fails', async () => { // Progress is written without a trailing newline so it can be overwritten in // place. Cleaning up only on success left `fetched 2…` on the line the error diff --git a/packages/sim-cli/src/http/client.ts b/packages/sim-cli/src/http/client.ts index 3997e67a767..65f0ed120f3 100644 --- a/packages/sim-cli/src/http/client.ts +++ b/packages/sim-cli/src/http/client.ts @@ -359,7 +359,7 @@ export class SimClient { ) } - if (REDIRECT_STATUSES.has(response.status)) throw this.toRedirectError(url, response) + if (REDIRECT_STATUSES.has(response.status)) throw this.toRedirectError(url, path, response) if (!response.ok) { const raw = await response.text() @@ -384,7 +384,7 @@ export class SimClient { * endpoint. A `Location` that is missing or unparseable still has to produce a * sentence — the redirect is the finding either way. */ - private toRedirectError(url: string, response: Response): SimApiError { + private toRedirectError(url: string, path: string, response: Response): SimApiError { const location = response.headers.get('location')?.trim() let target: URL | null = null if (location) { @@ -401,19 +401,45 @@ export class SimClient { response.status ) } - if (target.origin === new URL(url).origin) { + const suggested = redirectEndpoint(this.profile.endpoint, path, target) + if (!suggested) { return new SimApiError( `${url} redirected to ${target.href}. The CLI does not follow redirects, because a redirect can drop the request body and turn a write into a silent no-op.`, response.status ) } return new SimApiError( - `Endpoint redirected to ${target.origin}. Run: sim configure --profile ${this.profile.name} --set-endpoint ${target.origin}`, + `Endpoint redirected to ${suggested}. Run: sim configure --profile ${this.profile.name} --set-endpoint ${suggested}`, response.status ) } } +/** + * The endpoint a redirect implies, or null when it implies no change. + * + * Strips the request's own path from the target rather than taking + * `target.origin`, so a self-hosted endpoint carrying a path prefix + * (`https://host/sim`) keeps it. Naming the bare origin would hand back a value + * that is not an API root, and following that advice would break a deployment + * that was only ever one hostname away from working. + * + * Null when the target resolves to the endpoint already configured — a + * trailing-slash or path-normalization redirect keeps the origin, and telling + * someone to set the value they already have explains nothing. + */ +export function redirectEndpoint( + endpoint: string, + requestPath: string, + target: URL +): string | null { + const prefix = target.pathname.endsWith(requestPath) + ? target.pathname.slice(0, target.pathname.length - requestPath.length) + : '' + const suggested = `${target.origin}${prefix}`.replace(/\/+$/, '') + return suggested === endpoint.replace(/\/+$/, '') ? null : suggested +} + export interface PageProgress { /** Call once a further page is known to be coming, with the count so far. */ advance: (fetched: number) => void