From d229a1d7dc9becfb6b291c7099e1bcf29ee75713 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 26 Aug 2026 16:43:23 -0700 Subject: [PATCH 1/5] chore: run the orphaned migration-safety test, and stop publishing real-looking ids MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One of the eight script tests was reachable from no entrypoint, so it had never run in CI — it passes, it was simply never invoked. This is the second time that hand-maintained list has drifted from the files beside it; the audit runner's own header records the first. The guard against a third is a `check:*` script rather than a test, because the runner derives its list from that namespace and so picks the guard up by name — a test would have had to be hand-added to the very list it guards. A published spec sat outside the generator's manifest and so outside its drift check, and carried six example ids with the texture of real generated ones rather than the pandigital placeholders the rest of the repo uses. The new check globs the directory instead of reading the manifest, since the manifest is what omitted the file. The one pre-existing borderline id is allowlisted with a reason: loosening the threshold to admit it would have hidden one of the six. --- apps/docs/openapi-core.json | 30 +++++------ package.json | 5 +- scripts/check-script-test-coverage.ts | 71 +++++++++++++++++++++++++++ scripts/check-spec-example-ids.ts | 66 +++++++++++++++++++++++++ 4 files changed, 156 insertions(+), 16 deletions(-) create mode 100644 scripts/check-script-test-coverage.ts create mode 100644 scripts/check-spec-example-ids.ts diff --git a/apps/docs/openapi-core.json b/apps/docs/openapi-core.json index b7020ae27f9..522584c295d 100644 --- a/apps/docs/openapi-core.json +++ b/apps/docs/openapi-core.json @@ -211,7 +211,7 @@ "description": "Comma-separated block-output selectors. A bare `blockId` returns that block's full output; a dot-path like `blockId.field` or `blockId.nested.path` returns just that value. Results are returned in the `blockOutputs` map keyed by the selector string.", "schema": { "type": "string", - "example": "c1b90bce-8a82-42a5-b6a5-5762846c2eaf,c1b90bce-8a82-42a5-b6a5-5762846c2eaf.waitDuration" + "example": "a6f0c8d2-3e57-4b19-8d4a-1c9e2f6b0a35,a6f0c8d2-3e57-4b19-8d4a-1c9e2f6b0a35.waitDuration" } } ], @@ -227,8 +227,8 @@ "completed": { "summary": "Completed run", "value": { - "executionId": "9254f1c9-5a11-4a12-91e3-8065293f3609", - "workflowId": "81f661e1-d704-4861-b5c1-5bb3cf57e6a7", + "executionId": "e4f8d2b6-9a1c-4e3d-8b7f-5c0a2d9e6f13", + "workflowId": "3b1f7c92-8d4e-4a6b-9c0d-5e2f8a714b36", "status": "completed", "trigger": "api", "level": "info", @@ -247,8 +247,8 @@ "paused": { "summary": "Currently paused run", "value": { - "executionId": "772749f6-ee81-414c-a2c3-671549dd62b8", - "workflowId": "81f661e1-d704-4861-b5c1-5bb3cf57e6a7", + "executionId": "d5e1a3c7-8f60-4b29-9c4d-2a6e0f8b3d17", + "workflowId": "3b1f7c92-8d4e-4a6b-9c0d-5e2f8a714b36", "status": "paused", "trigger": "manual", "level": "info", @@ -259,8 +259,8 @@ "pausedAt": "2026-05-15T22:25:57.216Z", "resumeAt": "2026-05-16T18:25:57.200Z", "pauseKind": "time", - "blockedOnBlockId": "c1b90bce-8a82-42a5-b6a5-5762846c2eaf", - "pausedExecutionId": "438bf05b-bd3c-4011-b78e-b19c112eeb66", + "blockedOnBlockId": "a6f0c8d2-3e57-4b19-8d4a-1c9e2f6b0a35", + "pausedExecutionId": "9d3b7f10-2c8e-4a56-b0f4-6e1a8c5d2b97", "pausePointCount": 1, "resumedCount": 0 }, @@ -275,8 +275,8 @@ "failed": { "summary": "Failed run", "value": { - "executionId": "3ccfdeed-a63c-4e86-98e2-8bec723bca52", - "workflowId": "81f661e1-d704-4861-b5c1-5bb3cf57e6a7", + "executionId": "b8c2e60f-1a47-4d35-9e8b-3f0d5a7c2e19", + "workflowId": "3b1f7c92-8d4e-4a6b-9c0d-5e2f8a714b36", "status": "failed", "trigger": "api", "level": "error", @@ -1537,12 +1537,12 @@ "executionId": { "type": "string", "description": "The unique identifier of the execution.", - "example": "9254f1c9-5a11-4a12-91e3-8065293f3609" + "example": "e4f8d2b6-9a1c-4e3d-8b7f-5c0a2d9e6f13" }, "workflowId": { "type": "string", "description": "The unique identifier of the workflow.", - "example": "81f661e1-d704-4861-b5c1-5bb3cf57e6a7" + "example": "3b1f7c92-8d4e-4a6b-9c0d-5e2f8a714b36" }, "status": { "type": "string", @@ -1610,12 +1610,12 @@ "type": "string", "nullable": true, "description": "The block currently blocking resume.", - "example": "c1b90bce-8a82-42a5-b6a5-5762846c2eaf" + "example": "a6f0c8d2-3e57-4b19-8d4a-1c9e2f6b0a35" }, "pausedExecutionId": { "type": "string", "description": "ID of the paused-execution row, useful for cross-referencing with the human-in-the-loop endpoints.", - "example": "438bf05b-bd3c-4011-b78e-b19c112eeb66" + "example": "9d3b7f10-2c8e-4a56-b0f4-6e1a8c5d2b97" }, "pausePointCount": { "type": "integer", @@ -1659,8 +1659,8 @@ "description": "Per-block outputs keyed by the selector string. Returned only when `?selectedOutputs` is set.", "additionalProperties": true, "example": { - "c1b90bce-8a82-42a5-b6a5-5762846c2eaf.waitDuration": 60000, - "c1b90bce-8a82-42a5-b6a5-5762846c2eaf.status": "completed" + "a6f0c8d2-3e57-4b19-8d4a-1c9e2f6b0a35.waitDuration": 60000, + "a6f0c8d2-3e57-4b19-8d4a-1c9e2f6b0a35.status": "completed" } } } diff --git a/package.json b/package.json index 2f1f967d2b7..ac8cc72d5a2 100644 --- a/package.json +++ b/package.json @@ -14,12 +14,13 @@ "dev:sockets": "cd apps/realtime && bun run dev", "dev:full": "bunx concurrently -n \"App,Realtime\" -c \"cyan,magenta\" \"cd apps/sim && bun run dev\" \"cd apps/realtime && bun run dev\"", "dev:full:capped": "bunx concurrently -n \"App,Realtime\" -c \"cyan,magenta\" \"cd apps/sim && bun run dev:capped\" \"cd apps/realtime && bun run dev\"", - "test": "bun run test:setup && bun run test:npm-package-versions && bun run test:icon-path-precision && bun run test:tool-registry-boundary && bun run test:tool-request-boundary && bun run test:generators && turbo run test", + "test": "bun run test:setup && bun run test:npm-package-versions && bun run test:icon-path-precision && bun run test:tool-registry-boundary && bun run test:tool-request-boundary && bun run test:migrations-safety && bun run test:generators && turbo run test", "test:setup": "bun run --cwd packages/sim-setup test", "test:npm-package-versions": "bunx vitest run scripts/bump-npm-package-versions.test.ts", "test:icon-path-precision": "bunx vitest run scripts/check-icon-path-precision.test.ts", "test:tool-registry-boundary": "bunx vitest run scripts/check-tool-registry-boundary.test.ts", "test:tool-request-boundary": "bunx vitest run scripts/check-tool-request-boundary.test.ts", + "test:migrations-safety": "bunx vitest run scripts/check-migrations-safety.test.ts", "test:generators": "bunx vitest run scripts/generate-v2-cli-api.test.ts scripts/generate-cli-docs.test.ts scripts/generate-docs.test.ts", "format": "turbo run format", "format:check": "turbo run format:check", @@ -57,6 +58,8 @@ "check:migrations": "bun run scripts/check-migrations-safety.ts", "check:native-typecheck": "bun run scripts/check-native-typecheck.ts", "check:source-text": "bun run scripts/check-source-text.ts", + "check:spec-example-ids": "bun run scripts/check-spec-example-ids.ts", + "check:script-tests": "bun run scripts/check-script-test-coverage.ts", "check:audits": "bun run scripts/run-audits.ts", "check:skills": "bun run scripts/sync-skills.ts --check", "check:desktop-bridge": "bun run scripts/check-desktop-bridge-contract.ts --check", diff --git a/scripts/check-script-test-coverage.ts b/scripts/check-script-test-coverage.ts new file mode 100644 index 00000000000..3e126d5b164 --- /dev/null +++ b/scripts/check-script-test-coverage.ts @@ -0,0 +1,71 @@ +#!/usr/bin/env bun +/** + * Asserts every `scripts/*.test.ts` file is reachable from the root `test` script. + * + * The root `test` script chains a hand-maintained list of `test:*` entries, and a hand-maintained + * list silently drifts from the files on disk: a test added without a matching entry never runs, + * in CI or locally, and nothing reports it. `scripts/check-migrations-safety.test.ts` sat + * unreferenced and green for exactly that reason. + * + * `run-audits.ts` derives its own list from the `check:*` namespace precisely so a new audit is + * picked up by default, so this guard registers itself simply by being named `check:*` — it cannot + * drift out of the runner it belongs to. + */ +import { readdirSync } from 'node:fs' +import path from 'node:path' + +const ROOT = path.resolve(import.meta.dir, '..') +const TEST_FILE_PATTERN = /scripts\/[\w.-]+\.test\.ts/g +const SUB_SCRIPT_PATTERN = /bun run ([\w:-]+)/g + +const manifest = await Bun.file(path.join(ROOT, 'package.json')).json() +const commands = manifest.scripts as Record + +/** Walks the `test` script and every `test:*` entry it chains, collecting referenced test files. */ +function reachableTestFiles(entry: string): Set { + const referenced = new Set() + const seen = new Set() + const queue = [entry] + + while (queue.length > 0) { + const name = queue.pop() as string + if (seen.has(name)) continue + seen.add(name) + + const command = commands[name] + if (command === undefined) continue + + for (const match of command.matchAll(TEST_FILE_PATTERN)) referenced.add(match[0]) + for (const match of command.matchAll(SUB_SCRIPT_PATTERN)) queue.push(match[1]) + } + + return referenced +} + +const onDisk = readdirSync(path.join(ROOT, 'scripts')) + .filter((file) => file.endsWith('.test.ts')) + .map((file) => `scripts/${file}`) + .sort() + +const reachable = reachableTestFiles('test') +const orphaned = onDisk.filter((file) => !reachable.has(file)) +const missing = [...reachable].filter((file) => !onDisk.includes(file)).sort() + +if (orphaned.length > 0 || missing.length > 0) { + if (orphaned.length > 0) { + console.error( + `Script tests never run by \`bun run test\`:\n${orphaned.map((file) => ` - ${file}`).join('\n')}\n` + + 'Add a `test:*` entry for each and chain it into the root `test` script.' + ) + } + if (missing.length > 0) { + console.error( + `Root \`test\` script references script tests that do not exist:\n${missing.map((file) => ` - ${file}`).join('\n')}` + ) + } + process.exit(1) +} + +console.log( + `Script test coverage passed: ${onDisk.length} script tests reachable from \`bun run test\`.` +) diff --git a/scripts/check-spec-example-ids.ts b/scripts/check-spec-example-ids.ts new file mode 100644 index 00000000000..aeac1302739 --- /dev/null +++ b/scripts/check-spec-example-ids.ts @@ -0,0 +1,66 @@ +#!/usr/bin/env bun +/** + * Flags example UUIDs in the published OpenAPI specs that do not look deliberately synthetic. + * + * The specs in `apps/docs/` ship in a public repository, so an example id copied from a real run + * publishes an opaque workspace, workflow, or execution identifier. The repo's hand-crafted + * placeholders are pandigital — every hex digit appears, none more than three times + * (`3b1f7c92-8d4e-4a6b-9c0d-5e2f8a714b36`) — a texture a real v4 UUID essentially never has. + * That gives a mechanical signature for "this was generated, not authored". + * + * The directory is globbed rather than read from `OPENAPI_SPEC_FILES`, because the gap this closes + * is precisely that `openapi-core.json` is absent from that manifest and from every other check. + */ +import { readdirSync } from 'node:fs' +import path from 'node:path' + +const ROOT = path.resolve(import.meta.dir, '..') +const SPEC_DIR = path.join(ROOT, 'apps/docs') +const UUID_PATTERN = /[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}/g + +/** Ids that are obviously synthetic to a reader but do not match the pandigital texture. */ +const ALLOWED: Record = { + '7c9e6679-7425-40de-944b-e07fc1f90ae7': "Wikipedia's canonical example UUID", + 'a3f1c0b2-7a44-4c1d-9d3a-2b8e5f0a1c77': + 'pre-existing hand-authored block id shared with the v2 workflow operations tests', +} + +/** All-zero and all-f sentinels read as synthetic on sight. */ +function isSentinel(uuid: string): boolean { + return new Set(uuid.replace(/-/g, '')).size <= 2 +} + +/** Hand-authored placeholders in this repo use every hex digit, none more than three times. */ +function isHouseStyle(uuid: string): boolean { + const hex = uuid.replace(/-/g, '') + const digits = new Set(hex) + if (digits.size < 16) return false + return [...digits].every((digit) => hex.split(digit).length - 1 <= 3) +} + +const specFiles = readdirSync(SPEC_DIR) + .filter((file) => file.startsWith('openapi') && file.endsWith('.json')) + .sort() + +const findings: string[] = [] + +for (const file of specFiles) { + const contents = await Bun.file(path.join(SPEC_DIR, file)).text() + const seen = new Set(contents.match(UUID_PATTERN) ?? []) + for (const uuid of [...seen].sort()) { + if (uuid in ALLOWED || isSentinel(uuid) || isHouseStyle(uuid)) continue + findings.push(` - ${file}: ${uuid}`) + } +} + +if (findings.length > 0) { + console.error( + `Published OpenAPI specs contain example UUIDs that look real rather than hand-authored:\n${findings.join('\n')}\n` + + 'Replace each with a placeholder using every hex digit at most three times, or add it to ALLOWED with a reason.' + ) + process.exit(1) +} + +console.log( + `Spec example ids passed: ${specFiles.length} published specs contain no real-looking UUIDs.` +) From 610210678d2b5fa09e72be6695771273189723de Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 26 Aug 2026 16:43:41 -0700 Subject: [PATCH 2/5] fix(cli): close the gaps black-box testing the shipped CLI found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `sim profiles ` still exited 0, so a probe reading the exit code to ask whether a command exists was told yes — the one group the earlier guard missed. The exemption was written for commands that are both a group and a leaf, but only `files restore` takes an operand; `profiles` takes none. Registering its listing as a default subcommand puts it back among the pure dispatchers the existing guard already covers, so the guard itself did not need widening. Three commands refuse a workspace API key and said nothing, while their menu siblings said so — reading as though they accept one. They are hand-written, so they never reached the code that appends the note. That note now comes from a helper taking the operation, so a command names the operation it invokes and the two cannot disagree, and a test fails if a hand-written command ever calls a restricted operation without it. A blank numeric value in a request body still became a real zero, the same coercion already fixed for query strings: the guard keyed off the slot when the distinction is the field's declared type. Twenty-one fields across fifteen operations were affected. An empty body string still clears a description. Blank values for the root endpoint, workspace and profile flags fell back to what was configured instead of being refused, and a whitespace workspace was accepted verbatim. A hand-written profile name carrying padding listed as reachable but resolved to defaults rather than erroring. Two schema descriptions named request fields that no flag spells, and a rejected value was echoed unredacted by four messages while their siblings redacted it. A write now re-emits a section header it was not asked to touch byte for byte. The blank-line normalisation around it is left alone: making the writer position-faithful is a change to its model, not a fix. --- apps/docs/content/docs/en/cli/credentials.mdx | 6 ++ apps/docs/content/docs/en/cli/logs.mdx | 4 +- apps/docs/content/docs/en/cli/reference.mdx | 14 +-- apps/docs/content/docs/en/cli/secrets.mdx | 2 + apps/docs/content/docs/en/cli/workflows.mdx | 2 +- apps/docs/openapi-v2-logs.json | 8 +- apps/docs/openapi-v2-workflows.json | 4 +- apps/sim/lib/api/contracts/v2/logs.ts | 4 +- apps/sim/lib/api/contracts/v2/workflows.ts | 2 +- packages/sim-cli/src/commands/auth.ts | 30 +++++- packages/sim-cli/src/commands/configure.ts | 7 +- packages/sim-cli/src/commands/credentials.ts | 22 ++++- packages/sim-cli/src/commands/secrets.ts | 3 +- packages/sim-cli/src/config/ini.test.ts | 27 ++++++ packages/sim-cli/src/config/ini.ts | 14 ++- packages/sim-cli/src/config/profile.test.ts | 97 +++++++++++++++++++ packages/sim-cli/src/config/profile.ts | 77 +++++++++++++-- packages/sim-cli/src/generated/v2-api.ts | 6 +- packages/sim-cli/src/program.test.ts | 37 ++++++- packages/sim-cli/src/runtime/build.test.ts | 64 ++++++++++++ packages/sim-cli/src/runtime/build.ts | 21 +++- packages/sim-cli/src/runtime/request.test.ts | 30 +++++- packages/sim-cli/src/runtime/request.ts | 17 +++- packages/sim-cli/src/runtime/result.test.ts | 32 ++++++ packages/sim-cli/src/runtime/result.ts | 10 +- 25 files changed, 483 insertions(+), 57 deletions(-) diff --git a/apps/docs/content/docs/en/cli/credentials.mdx b/apps/docs/content/docs/en/cli/credentials.mdx index 6aa4143baed..ad63a7ba7a3 100644 --- a/apps/docs/content/docs/en/cli/credentials.mdx +++ b/apps/docs/content/docs/en/cli/credentials.mdx @@ -123,6 +123,8 @@ Update Credential (personal API key required) sim credentials create [options] ``` +Create a service-account credential using its discovered provider schema (personal API key required) + **Arguments** @@ -152,6 +154,8 @@ sim credentials create [options] sim credentials connect [options] ``` +Create a short-lived link for connecting an OAuth provider (personal API key required) + **Arguments** @@ -178,6 +182,8 @@ sim credentials connect [options] sim credentials reconnect ``` +Create a short-lived link for reconnecting an OAuth credential (personal API key required) + **Arguments** diff --git a/apps/docs/content/docs/en/cli/logs.mdx b/apps/docs/content/docs/en/cli/logs.mdx index c5b35b9d216..1b277367c9c 100644 --- a/apps/docs/content/docs/en/cli/logs.mdx +++ b/apps/docs/content/docs/en/cli/logs.mdx @@ -85,10 +85,10 @@ sim logs list [options] | `--limit ` | No | Maximum items to return (0 for everything). Defaults to `100`. | | `--status ` | No | Comma-separated execution statuses to include, from `pending` \| `running` \| `paused` \| `redacting` \| `completed` \| `failed` \| `cancelled`. An empty entry is rejected. ANDed with `level`, which reports severity rather than lifecycle. | | `--workflow-name ` | No | Case-insensitive substring match against the run's workflow name. Runs whose workflow has been deleted match nothing, because the name is no longer joinable. | -| `--include-job-runs` | No | Whether Chat and Sim-agent job runs join the sequence alongside workflow runs. Job runs report `kind: "job"`, carry no `workflow` summary, and never carry a cost ledger. They are dropped entirely — not partially matched — whenever a filter they cannot answer is set (`workflowIds`, `workflowName`, `folderPaths`, `model`, or `status`), so a filter never means two different things across the union. Accepted only under `sortBy=startedAt`: job runs record cost as a document and no comparable status, so they cannot participate in the other orderings. | +| `--include-job-runs` | No | Whether Chat and Sim-agent job runs join the sequence alongside workflow runs. Job runs report `kind: "job"`, carry no `workflow` summary, and never carry a cost ledger. They are dropped entirely — not partially matched — whenever a filter they cannot answer is set: by workflow, workflow name, folder, model, or status. A filter therefore never means two different things across the union. Accepted only when sorting by `startedAt`: job runs record cost as a document and no comparable status, so they cannot participate in the other orderings. | | `--no-include-job-runs` | No | Send --include-job-runs as false. | | `--run-id ` | No | Exact run identifier to match. | -| `--sort-by ` | No | Field used to sort the result. `durationMs` and `cost` are null until a run settles; those runs order as though the value were below every recorded one, so they trail an ascending page and lead a descending one. Only `startedAt` can order Chat and Sim-agent job runs, so any other value is rejected together with `includeJobRuns=true`. Accepted values: `startedAt`, `durationMs`, `cost`, `status`. | +| `--sort-by ` | No | Field used to sort the result. `durationMs` and `cost` are null until a run settles; those runs order as though the value were below every recorded one, so they trail an ascending page and lead a descending one. Only `startedAt` can order Chat and Sim-agent job runs, so any other value is rejected when job runs are included. Accepted values: `startedAt`, `durationMs`, `cost`, `status`. | | `--sort-order ` | No | Sort direction. Accepted values: `asc`, `desc`. | | `--folder ` | No | Folder path as shown in the app; the leading / is optional (space-separated, or @path / @- with one value per line; @@value for a literal leading @). | diff --git a/apps/docs/content/docs/en/cli/reference.mdx b/apps/docs/content/docs/en/cli/reference.mdx index a0b890e97e8..f93c5009dc6 100644 --- a/apps/docs/content/docs/en/cli/reference.mdx +++ b/apps/docs/content/docs/en/cli/reference.mdx @@ -479,7 +479,7 @@ sim credentials update [options] ### sim credentials create -Create a service-account credential using its discovered provider schema +Create a service-account credential using its discovered provider schema (personal API key required) ```bash sim credentials create [options] @@ -510,7 +510,7 @@ sim credentials create [options] ### sim credentials connect -Create a short-lived link for connecting an OAuth provider +Create a short-lived link for connecting an OAuth provider (personal API key required) ```bash sim credentials connect [options] @@ -538,7 +538,7 @@ sim credentials connect [options] ### sim credentials reconnect -Create a short-lived link for reconnecting an OAuth credential +Create a short-lived link for reconnecting an OAuth credential (personal API key required) ```bash sim credentials reconnect @@ -2475,10 +2475,10 @@ sim logs list [options] | `--limit ` | No | Maximum items to return (0 for everything). Defaults to `100`. | | `--status ` | No | Comma-separated execution statuses to include, from `pending` \| `running` \| `paused` \| `redacting` \| `completed` \| `failed` \| `cancelled`. An empty entry is rejected. ANDed with `level`, which reports severity rather than lifecycle. | | `--workflow-name ` | No | Case-insensitive substring match against the run's workflow name. Runs whose workflow has been deleted match nothing, because the name is no longer joinable. | -| `--include-job-runs` | No | Whether Chat and Sim-agent job runs join the sequence alongside workflow runs. Job runs report `kind: "job"`, carry no `workflow` summary, and never carry a cost ledger. They are dropped entirely — not partially matched — whenever a filter they cannot answer is set (`workflowIds`, `workflowName`, `folderPaths`, `model`, or `status`), so a filter never means two different things across the union. Accepted only under `sortBy=startedAt`: job runs record cost as a document and no comparable status, so they cannot participate in the other orderings. | +| `--include-job-runs` | No | Whether Chat and Sim-agent job runs join the sequence alongside workflow runs. Job runs report `kind: "job"`, carry no `workflow` summary, and never carry a cost ledger. They are dropped entirely — not partially matched — whenever a filter they cannot answer is set: by workflow, workflow name, folder, model, or status. A filter therefore never means two different things across the union. Accepted only when sorting by `startedAt`: job runs record cost as a document and no comparable status, so they cannot participate in the other orderings. | | `--no-include-job-runs` | No | Send --include-job-runs as false. | | `--run-id ` | No | Exact run identifier to match. | -| `--sort-by ` | No | Field used to sort the result. `durationMs` and `cost` are null until a run settles; those runs order as though the value were below every recorded one, so they trail an ascending page and lead a descending one. Only `startedAt` can order Chat and Sim-agent job runs, so any other value is rejected together with `includeJobRuns=true`. Accepted values: `startedAt`, `durationMs`, `cost`, `status`. | +| `--sort-by ` | No | Field used to sort the result. `durationMs` and `cost` are null until a run settles; those runs order as though the value were below every recorded one, so they trail an ascending page and lead a descending one. Only `startedAt` can order Chat and Sim-agent job runs, so any other value is rejected when job runs are included. Accepted values: `startedAt`, `durationMs`, `cost`, `status`. | | `--sort-order ` | No | Sort direction. Accepted values: `asc`, `desc`. | | `--folder ` | No | Folder path as shown in the app; the leading / is optional (space-separated, or @path / @- with one value per line; @@value for a literal leading @). | @@ -2743,7 +2743,7 @@ sim secrets list [options] ### sim secrets set -Create or replace a named secret +Create or replace a named secret (personal API key required) ```bash sim secrets set [options] @@ -5410,7 +5410,7 @@ sim workflows list [options] | Option | Required | Description | | --- | --- | --- | -| `--scope ` | No | Which lifecycle set to list: `active` (default) for live workflows, `archived` for workflows a `DELETE` archived. `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`. | +| `--scope ` | No | Which lifecycle set to list: `active` (default) for live workflows, `archived` for workflows a `DELETE` archived. The folder filter resolves against active folders only, so pairing it with `archived` returns an empty page when the containing folder was archived too. Accepted values: `active`, `archived`. | | `--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. | diff --git a/apps/docs/content/docs/en/cli/secrets.mdx b/apps/docs/content/docs/en/cli/secrets.mdx index cde61e9b084..cf14dda7d5e 100644 --- a/apps/docs/content/docs/en/cli/secrets.mdx +++ b/apps/docs/content/docs/en/cli/secrets.mdx @@ -66,6 +66,8 @@ List Secrets (personal API key required) sim secrets set [options] ``` +Create or replace a named secret (personal API key required) + **Arguments** diff --git a/apps/docs/content/docs/en/cli/workflows.mdx b/apps/docs/content/docs/en/cli/workflows.mdx index f622a8b7669..3a82b1878ef 100644 --- a/apps/docs/content/docs/en/cli/workflows.mdx +++ b/apps/docs/content/docs/en/cli/workflows.mdx @@ -767,7 +767,7 @@ sim workflows list [options] | Option | Required | Description | | --- | --- | --- | -| `--scope ` | No | Which lifecycle set to list: `active` (default) for live workflows, `archived` for workflows a `DELETE` archived. `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`. | +| `--scope ` | No | Which lifecycle set to list: `active` (default) for live workflows, `archived` for workflows a `DELETE` archived. The folder filter resolves against active folders only, so pairing it with `archived` returns an empty page when the containing folder was archived too. Accepted values: `active`, `archived`. | | `--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. | diff --git a/apps/docs/openapi-v2-logs.json b/apps/docs/openapi-v2-logs.json index 1c723026223..3f69f6dd896 100644 --- a/apps/docs/openapi-v2-logs.json +++ b/apps/docs/openapi-v2-logs.json @@ -244,9 +244,9 @@ "name": "includeJobRuns", "in": "query", "required": false, - "description": "Whether Chat and Sim-agent job runs join the sequence alongside workflow runs. Job runs report `kind: \"job\"`, carry no `workflow` summary, and never carry a cost ledger. They are dropped entirely — not partially matched — whenever a filter they cannot answer is set (`workflowIds`, `workflowName`, `folderPaths`, `model`, or `status`), so a filter never means two different things across the union. Accepted only under `sortBy=startedAt`: job runs record cost as a document and no comparable status, so they cannot participate in the other orderings.", + "description": "Whether Chat and Sim-agent job runs join the sequence alongside workflow runs. Job runs report `kind: \"job\"`, carry no `workflow` summary, and never carry a cost ledger. They are dropped entirely — not partially matched — whenever a filter they cannot answer is set: by workflow, workflow name, folder, model, or status. A filter therefore never means two different things across the union. Accepted only when sorting by `startedAt`: job runs record cost as a document and no comparable status, so they cannot participate in the other orderings.", "schema": { - "description": "Whether Chat and Sim-agent job runs join the sequence alongside workflow runs. Job runs report `kind: \"job\"`, carry no `workflow` summary, and never carry a cost ledger. They are dropped entirely — not partially matched — whenever a filter they cannot answer is set (`workflowIds`, `workflowName`, `folderPaths`, `model`, or `status`), so a filter never means two different things across the union. Accepted only under `sortBy=startedAt`: job runs record cost as a document and no comparable status, so they cannot participate in the other orderings.", + "description": "Whether Chat and Sim-agent job runs join the sequence alongside workflow runs. Job runs report `kind: \"job\"`, carry no `workflow` summary, and never carry a cost ledger. They are dropped entirely — not partially matched — whenever a filter they cannot answer is set: by workflow, workflow name, folder, model, or status. A filter therefore never means two different things across the union. Accepted only when sorting by `startedAt`: job runs record cost as a document and no comparable status, so they cannot participate in the other orderings.", "type": "boolean" } }, @@ -267,10 +267,10 @@ "name": "sortBy", "in": "query", "required": false, - "description": "Field used to sort the result. `durationMs` and `cost` are null until a run settles; those runs order as though the value were below every recorded one, so they trail an ascending page and lead a descending one. Only `startedAt` can order Chat and Sim-agent job runs, so any other value is rejected together with `includeJobRuns=true`.", + "description": "Field used to sort the result. `durationMs` and `cost` are null until a run settles; those runs order as though the value were below every recorded one, so they trail an ascending page and lead a descending one. Only `startedAt` can order Chat and Sim-agent job runs, so any other value is rejected when job runs are included.", "schema": { "default": "startedAt", - "description": "Field used to sort the result. `durationMs` and `cost` are null until a run settles; those runs order as though the value were below every recorded one, so they trail an ascending page and lead a descending one. Only `startedAt` can order Chat and Sim-agent job runs, so any other value is rejected together with `includeJobRuns=true`.", + "description": "Field used to sort the result. `durationMs` and `cost` are null until a run settles; those runs order as though the value were below every recorded one, so they trail an ascending page and lead a descending one. Only `startedAt` can order Chat and Sim-agent job runs, so any other value is rejected when job runs are included.", "type": "string", "enum": ["startedAt", "durationMs", "cost", "status"] } diff --git a/apps/docs/openapi-v2-workflows.json b/apps/docs/openapi-v2-workflows.json index eaee255cf16..b80dda73fab 100644 --- a/apps/docs/openapi-v2-workflows.json +++ b/apps/docs/openapi-v2-workflows.json @@ -59,10 +59,10 @@ "name": "scope", "in": "query", "required": false, - "description": "Which lifecycle set to list: `active` (default) for live workflows, `archived` for workflows a `DELETE` archived. `folderPath` resolves against active folders only, so pairing it with `scope=archived` returns an empty page when the containing folder was archived too.", + "description": "Which lifecycle set to list: `active` (default) for live workflows, `archived` for workflows a `DELETE` archived. The folder filter resolves against active folders only, so pairing it with `archived` returns an empty page when the containing folder was archived too.", "schema": { "default": "active", - "description": "Which lifecycle set to list: `active` (default) for live workflows, `archived` for workflows a `DELETE` archived. `folderPath` resolves against active folders only, so pairing it with `scope=archived` returns an empty page when the containing folder was archived too.", + "description": "Which lifecycle set to list: `active` (default) for live workflows, `archived` for workflows a `DELETE` archived. The folder filter resolves against active folders only, so pairing it with `archived` returns an empty page when the containing folder was archived too.", "type": "string", "enum": ["active", "archived"] } diff --git a/apps/sim/lib/api/contracts/v2/logs.ts b/apps/sim/lib/api/contracts/v2/logs.ts index fa685ebd789..f2e65ea41ab 100644 --- a/apps/sim/lib/api/contracts/v2/logs.ts +++ b/apps/sim/lib/api/contracts/v2/logs.ts @@ -526,7 +526,7 @@ export const v2ListLogsQuerySchema = v1ListLogsQuerySchema workflowName: v2WorkflowNameFilterSchema.optional(), includeJobRuns: booleanQueryFlagSchema .describe( - 'Whether Chat and Sim-agent job runs join the sequence alongside workflow runs. Job runs report `kind: "job"`, carry no `workflow` summary, and never carry a cost ledger. They are dropped entirely — not partially matched — whenever a filter they cannot answer is set (`workflowIds`, `workflowName`, `folderPaths`, `model`, or `status`), so a filter never means two different things across the union. Accepted only under `sortBy=startedAt`: job runs record cost as a document and no comparable status, so they cannot participate in the other orderings.' + 'Whether Chat and Sim-agent job runs join the sequence alongside workflow runs. Job runs report `kind: "job"`, carry no `workflow` summary, and never carry a cost ledger. They are dropped entirely — not partially matched — whenever a filter they cannot answer is set: by workflow, workflow name, folder, model, or status. A filter therefore never means two different things across the union. Accepted only when sorting by `startedAt`: job runs record cost as a document and no comparable status, so they cannot participate in the other orderings.' ) .optional() .default(false), @@ -570,7 +570,7 @@ export const v2ListLogsQuerySchema = v1ListLogsQuerySchema * local to this resource. */ sortBy: v2LogSortFieldSchemas.sortBy.describe( - 'Field used to sort the result. `durationMs` and `cost` are null until a run settles; those runs order as though the value were below every recorded one, so they trail an ascending page and lead a descending one. Only `startedAt` can order Chat and Sim-agent job runs, so any other value is rejected together with `includeJobRuns=true`.' + 'Field used to sort the result. `durationMs` and `cost` are null until a run settles; those runs order as though the value were below every recorded one, so they trail an ascending page and lead a descending one. Only `startedAt` can order Chat and Sim-agent job runs, so any other value is rejected when job runs are included.' ), folderPaths: z .string() diff --git a/apps/sim/lib/api/contracts/v2/workflows.ts b/apps/sim/lib/api/contracts/v2/workflows.ts index e46cfba6383..2ac11bf28c2 100644 --- a/apps/sim/lib/api/contracts/v2/workflows.ts +++ b/apps/sim/lib/api/contracts/v2/workflows.ts @@ -158,7 +158,7 @@ export const v2ListWorkflowsQuerySchema = z scope: v2WorkflowScopeSchema .default('active') .describe( - 'Which lifecycle set to list: `active` (default) for live workflows, `archived` for workflows a `DELETE` archived. `folderPath` resolves against active folders only, so pairing it with `scope=archived` returns an empty page when the containing folder was archived too.' + 'Which lifecycle set to list: `active` (default) for live workflows, `archived` for workflows a `DELETE` archived. The folder filter resolves against active folders only, so pairing it with `archived` returns an empty page when the containing folder was archived too.' ), folderPath: v2FolderPathInputSchema .optional() diff --git a/packages/sim-cli/src/commands/auth.ts b/packages/sim-cli/src/commands/auth.ts index d481a176817..617d5c25fb0 100644 --- a/packages/sim-cli/src/commands/auth.ts +++ b/packages/sim-cli/src/commands/auth.ts @@ -748,10 +748,36 @@ export function profilesCommand(): Command { printList(output, rows, PROFILE_COLUMNS) } - command.action(printProfiles) + // `list` is registered as the default rather than the group carrying an action + // of its own. An action handler on a group is what took `profiles` out of the + // pure-dispatcher set that `refuseHelpAfterUnknownCommand` guards, so + // `sim profiles zzznope --help` printed the group's help and exited `0` while + // the same words under any of the other 54 groups exit `1` — the exit code a + // capability probe reads to ask whether a command exists. Only `files restore` + // genuinely needs that exemption, and it keeps it by taking a real operand. command.addCommand( - new Command('list').description('List configured profiles').action(printProfiles) + new Command('list') + // A stray operand here is a mistyped subcommand the group already refused + // below; refusing it again keeps `sim profiles list zzznope` honest. + .allowExcessArguments(false) + .description('List configured profiles') + .action(printProfiles), + { isDefault: true } ) command.addCommand(addProfileCommand()) + + // Commander hands the default subcommand any operand that names no other one, + // so without this `sim profiles zzznope` listed profiles and exited `0`. The + // check runs before the dispatch and defers to commander's own reporting, so + // the message, the "did you mean" suggestion, and the exit code are the ones + // every other group produces. + const known = new Set(command.commands.flatMap((child) => [child.name(), ...child.aliases()])) + command.hook('preSubcommand', (group) => { + const first = group.args[0] + if (first !== undefined && !first.startsWith('-') && !known.has(first)) { + ;(group as Command & { unknownCommand: () => never }).unknownCommand() + } + }) + return command } diff --git a/packages/sim-cli/src/commands/configure.ts b/packages/sim-cli/src/commands/configure.ts index 3caa4d46985..82304a19685 100644 --- a/packages/sim-cli/src/commands/configure.ts +++ b/packages/sim-cli/src/commands/configure.ts @@ -7,7 +7,7 @@ import { resolveAuthenticationProfileName, writeConfigProfile, } from '../config/index' -import { normalizeEndpoint, normalizeWorkspaceId } from '../config/profile' +import { normalizeEndpoint, normalizeWorkspaceId, redact } from '../config/profile' import { globalsOf, profileFrom } from '../context' import { SimApiError } from '../http/client' @@ -109,7 +109,10 @@ export function configureCommand(): Command { for (const key of options.unset ?? []) { if (!['endpoint', 'workspace', 'output'].includes(key)) { - throw new SimApiError(`Cannot unset "${key}". Use endpoint, workspace, or output.`, 0) + throw new SimApiError( + `Cannot unset "${redact(key)}". Use endpoint, workspace, or output.`, + 0 + ) } if (key === 'endpoint' && authProfile !== profile.name) { throw new SimApiError( diff --git a/packages/sim-cli/src/commands/credentials.ts b/packages/sim-cli/src/commands/credentials.ts index e95c20dea25..adbd3030ed1 100644 --- a/packages/sim-cli/src/commands/credentials.ts +++ b/packages/sim-cli/src/commands/credentials.ts @@ -8,6 +8,7 @@ import { V2_OPERATIONS, } from '../generated/v2-api' import { SimApiError } from '../http/client' +import { describeOperation } from '../runtime/build' import { coerce } from '../runtime/request' import { renderResult } from '../runtime/result' @@ -192,7 +193,12 @@ export function attachCredentialCommands(program: Command): void { credentials .command('create') .argument('', 'Service-account provider to create a credential for') - .description('Create a service-account credential using its discovered provider schema') + .description( + describeOperation( + V2_OPERATIONS.createServiceAccountCredential, + 'Create a service-account credential using its discovered provider schema' + ) + ) // The `(required)` suffix is the marker the generated flags carry, and it // is literal text rather than something commander renders — a hand-written // mandatory option that omits it is the only kind of required flag whose @@ -214,7 +220,12 @@ export function attachCredentialCommands(program: Command): void { credentials .command('connect') .argument('', 'OAuth provider to connect') - .description('Create a short-lived link for connecting an OAuth provider') + .description( + describeOperation( + V2_OPERATIONS.createCredentialConnection, + 'Create a short-lived link for connecting an OAuth provider' + ) + ) .requiredOption('--name ', 'Name shown for the new credential in Sim (required)') .action(async (providerId: string, options: { name: string }, command: Command) => createConnectionLink(command, { providerId, displayName: options.name }) @@ -223,7 +234,12 @@ export function attachCredentialCommands(program: Command): void { credentials .command('reconnect') .argument('', 'Existing OAuth credential to re-authorize') - .description('Create a short-lived link for reconnecting an OAuth credential') + .description( + describeOperation( + V2_OPERATIONS.createCredentialConnection, + 'Create a short-lived link for reconnecting an OAuth credential' + ) + ) .action((credentialId: string, _options: unknown, command: Command) => createConnectionLink(command, { credentialId }) ) diff --git a/packages/sim-cli/src/commands/secrets.ts b/packages/sim-cli/src/commands/secrets.ts index 83312008e63..793445d8b4a 100644 --- a/packages/sim-cli/src/commands/secrets.ts +++ b/packages/sim-cli/src/commands/secrets.ts @@ -4,6 +4,7 @@ import { clientFrom } from '../context' import type { CommandSpec } from '../contract/types' import { type SetSecretResponse, V2_OPERATIONS } from '../generated/v2-api' import { resolvePath, SimApiError } from '../http/client' +import { describeOperation } from '../runtime/build' import { readArgumentSource } from '../runtime/request' import { renderResult } from '../runtime/result' import { promptSecret, SecretInputCancelledError } from '../terminal/secret-input' @@ -160,7 +161,7 @@ export function attachSecretCommands(program: Command): void { secrets .command('set') .argument('', 'Secret name, as referenced in workflows') - .description('Create or replace a named secret') + .description(describeOperation(V2_OPERATIONS.setSecret, 'Create or replace a named secret')) .addOption( new Option('--scope ', 'Secret ownership scope (required)') .choices([...SECRET_SCOPES]) diff --git a/packages/sim-cli/src/config/ini.test.ts b/packages/sim-cli/src/config/ini.test.ts index 974c70c7dfc..8ffec1b4f82 100644 --- a/packages/sim-cli/src/config/ini.test.ts +++ b/packages/sim-cli/src/config/ini.test.ts @@ -277,3 +277,30 @@ describe('ini write guards', () => { expect(serializeIni(doc)).toBe('') }) }) + +/** + * A write names one section, so it must leave every other section's bytes + * alone. The header was the exception: `parseIni` trims the bracketed text to + * get the name and the writer rebuilt `[${name}]` from it, so a `configure + * --set-output` on `default` silently reformatted a hand-written + * `[profile padded ]` it had never been asked to touch. + */ +describe('section headers survive a write to another section', () => { + it('re-emits an unrelated padded header byte for byte', () => { + const doc = parseIni( + '[default]\nendpoint = https://sim.ai\n\n[profile padded ]\nworkspace = ws_1\n' + ) + + setSectionValues(doc, 'default', { output: 'json' }) + + expect(serializeIni(doc)).toContain('[profile padded ]') + }) + + it('generates a header for a section the writer created', () => { + const doc = parseIni('[default]\nendpoint = https://sim.ai\n') + + setSectionValues(doc, 'profile dev', { workspace: 'ws_2' }) + + expect(serializeIni(doc)).toContain('[profile dev]') + }) +}) diff --git a/packages/sim-cli/src/config/ini.ts b/packages/sim-cli/src/config/ini.ts index 1ecb1f143bc..93b14a831d5 100644 --- a/packages/sim-cli/src/config/ini.ts +++ b/packages/sim-cli/src/config/ini.ts @@ -31,6 +31,16 @@ type Entry = { kind: 'kv'; key: string; value: string } | { kind: 'raw'; text: s interface Section { name: string + /** + * The header line exactly as it was read, re-emitted verbatim. + * + * {@link parseIni} trims the bracketed text to get {@link name}, so writing + * `[${name}]` back rewrote the header of every section in the file — a + * `configure --set-output` on `default` silently reformatted a hand-written + * `[profile padded ]` it was never asked to touch. Absent only on a + * section {@link setSectionValues} created, which has no original line. + */ + header?: string entries: Entry[] } @@ -118,7 +128,7 @@ export function parseIni(text: string): IniDocument { for (const line of lines) { const sectionMatch = SECTION_PATTERN.exec(line) if (sectionMatch) { - current = { name: sectionMatch[1].trim(), entries: [] } + current = { name: sectionMatch[1].trim(), header: line, entries: [] } doc.sections.push(current) continue } @@ -148,7 +158,7 @@ export function serializeIni(doc: IniDocument): string { // across repeated writes. while (lines.length > 0 && lines[lines.length - 1].trim() === '') lines.pop() if (lines.length > 0) lines.push('') - lines.push(`[${section.name}]`) + lines.push(section.header ?? `[${section.name}]`) for (const entry of section.entries) { lines.push(entry.kind === 'kv' ? `${entry.key} = ${entry.value}` : entry.text) } diff --git a/packages/sim-cli/src/config/profile.test.ts b/packages/sim-cli/src/config/profile.test.ts index 81a702ad9c2..fa94e9f7c3c 100644 --- a/packages/sim-cli/src/config/profile.test.ts +++ b/packages/sim-cli/src/config/profile.test.ts @@ -6,6 +6,7 @@ import { configPath, credentialsPath } from './paths' import { DEFAULT_ENDPOINT, deleteProfile, + FORBIDDEN_IN_VALUE, listAuthenticationDependents, listProfiles, OUTPUT_FORMATS, @@ -501,3 +502,99 @@ describe('config file injection', () => { expect(() => resolveProfile({ profile: 'phantom' })).toThrow(/Unknown profile/) }) }) + +/** + * A flag the user typed is not the same as one they left off, and `resolve` + * cannot tell the two apart once a blank has reached it: it treats the empty + * string as "not supplied", which is right for an environment variable and + * wrong for `sim --workspace "" …`, which ran against the profile's stored + * workspace instead of refusing. + */ +describe('blank root flags', () => { + it('refuses a blank --workspace instead of falling back to the profile', () => { + writeConfigProfile('default', { workspace: 'ws_stored' }) + + expect(() => resolveProfile({ workspaceId: '' })).toThrow(/--workspace requires a value/) + expect(() => resolveProfile({ workspaceId: ' ' })).toThrow(/--workspace requires a value/) + }) + + it('refuses a blank --endpoint and a blank --profile the same way', () => { + writeConfigProfile('default', { endpoint: 'https://stored.example' }) + + expect(() => resolveProfile({ endpoint: '' })).toThrow(/--endpoint requires a value/) + expect(() => resolveProfile({ profile: '' })).toThrow(/--profile requires a value/) + }) + + it('still reads an exported-but-empty environment variable as unset', () => { + // The convention every profile-based CLI follows, and the reason the empty + // string cannot simply become significant everywhere. + writeConfigProfile('default', { workspace: 'ws_stored' }) + process.env.SIM_WORKSPACE = '' + + expect(resolveProfile()).toMatchObject({ workspaceId: 'ws_stored' }) + }) +}) + +/** + * `configSectionName` builds a header by prefixing `profile `, so a second trim + * on the way back out makes the listed name and the looked-up name disagree — + * and the disagreement failed silently, resolving a selection that names a real + * section to the built-in defaults. + */ +describe('a hand-written profile name carrying padding', () => { + const PADDED = '[profile padded ]\nworkspace = ws_padded\nendpoint = https://padded.example\n' + + it('lists the name that actually selects it', () => { + writeFileSync(configPath(), PADDED) + + expect(listProfiles()).toEqual([' padded']) + expect(resolveProfile({ profile: ' padded' })).toMatchObject({ + workspaceId: 'ws_padded', + endpoint: 'https://padded.example', + }) + }) + + it('refuses the trimmed spelling loudly rather than resolving it to defaults', () => { + writeFileSync(configPath(), PADDED) + + expect(() => resolveProfile({ profile: 'padded' })).toThrow(/Unknown profile "padded"/) + }) +}) + +/** + * Every message that quotes text the CLI did not produce goes through the same + * redaction. These three did not, so a bare newline reached the terminal and + * appended lines that read as the CLI's own output. + */ +describe('redaction of rejected values', () => { + const messageOf = (run: () => unknown): string => { + try { + run() + } catch (error) { + return (error as Error).message + } + throw new Error('expected a refusal') + } + + it('redacts the name in the unknown-profile refusal', () => { + writeConfigProfile('dev', { workspace: 'ws_1' }) + + const message = messageOf(() => resolveProfile({ profile: 'ev\nil' })) + expect(message).toContain('Unknown profile "ev il"') + expect(message).not.toMatch(FORBIDDEN_IN_VALUE) + }) + + it('redacts it in the variant printed when nothing is configured yet', () => { + const message = messageOf(() => resolveProfile({ profile: 'ev\nil' })) + expect(message).toContain('Unknown profile "ev il"') + expect(message).not.toMatch(FORBIDDEN_IN_VALUE) + }) + + it('redacts the format in the unknown-output-format refusal', () => { + process.env.SIM_OUTPUT = 'ev\nil' + + const message = messageOf(() => resolveProfile()) + expect(message).toContain('Unknown output format "ev il"') + expect(message).not.toMatch(FORBIDDEN_IN_VALUE) + }) +}) diff --git a/packages/sim-cli/src/config/profile.ts b/packages/sim-cli/src/config/profile.ts index 1c274811b64..2cf47ae8f16 100644 --- a/packages/sim-cli/src/config/profile.ts +++ b/packages/sim-cli/src/config/profile.ts @@ -42,6 +42,19 @@ export { FORBIDDEN_IN_VALUE, ProfileConfigError } from './ini' /** {@link FORBIDDEN_IN_VALUE}, for redacting every match out of an error message. */ const FORBIDDEN_IN_VALUE_GLOBAL = new RegExp(FORBIDDEN_IN_VALUE.source, 'g') +/** + * Makes a rejected value safe to quote inside an error message. + * + * A message is one terminal line, so a value carrying a line break appends + * lines that read as the CLI's own output. Every message that interpolates text + * the CLI did not produce — a flag, an environment variable, a hand-edited + * setting — goes through this, so the rule does not have to be rediscovered at + * each new message. + */ +export function redact(value: string): string { + return value.replace(FORBIDDEN_IN_VALUE_GLOBAL, ' ') +} + /** * The shape a newly created profile name has to have. * @@ -66,7 +79,7 @@ export function validateProfileName(name: string): void { // untrusted text, and echoing its control characters into a terminal is how // an error message becomes an escape-sequence delivery vehicle. throw new ProfileConfigError( - `Invalid profile name "${name.replace(FORBIDDEN_IN_VALUE_GLOBAL, ' ')}". Use letters, numbers, dots, underscores, or hyphens, starting with a letter or number.` + `Invalid profile name "${redact(name)}". Use letters, numbers, dots, underscores, or hyphens, starting with a letter or number.` ) } } @@ -183,13 +196,25 @@ export function resolveAuthenticationProfileName(profile: string): string { return authProfile } -/** Every profile named by either file, deduplicated and sorted. */ +/** + * Every profile named by either file, deduplicated and sorted. + * + * The name is taken verbatim from what follows `profile `, without a second + * trim. `configSectionName` builds the header back by prefixing `profile `, so + * trimming here made the two disagree for a hand-written `[profile padded]`: + * it listed and passed the "does this profile exist?" check as `padded`, while + * every read looked for `profile padded` and found nothing — so `-P padded` + * silently resolved to the built-in defaults, and the spelling that does name + * the section was refused as unknown. Reporting the padded name keeps the one + * that selects it the one that is shown, which is also how the credentials + * file's sections have always been listed. + */ export function listProfiles(): string[] { const names = new Set() for (const section of listSections(readIni(configPath()))) { if (section === DEFAULT_PROFILE) names.add(DEFAULT_PROFILE) - else if (section.startsWith('profile ')) names.add(section.slice('profile '.length).trim()) + else if (section.startsWith('profile ')) names.add(section.slice('profile '.length)) } for (const section of listSections(readIni(credentialsPath()))) { names.add(section) @@ -252,13 +277,13 @@ function requireKnownProfile(name: string): void { if (known.length === 0) { throw new ProfileConfigError( - `Unknown profile "${name}". No profiles are configured yet. Run: sim login --profile ${name}` + `Unknown profile "${redact(name)}". No profiles are configured yet. Run: sim login --profile ${redact(name)}` ) } const suggestion = nearestProfile(name, known) throw new ProfileConfigError( - `Unknown profile "${name}".${suggestion ? ` Did you mean "${suggestion}"?` : ''} Configured profiles: ${known.join(', ')}.` + `Unknown profile "${redact(name)}".${suggestion ? ` Did you mean "${suggestion}"?` : ''} Configured profiles: ${known.join(', ')}.` ) } @@ -335,7 +360,7 @@ export function normalizeEndpoint(endpoint: string, source: string): string { // by the write that stores it. if (FORBIDDEN_IN_VALUE.test(trimmed)) { throw new ProfileConfigError( - `Invalid endpoint "${endpoint.replace(FORBIDDEN_IN_VALUE_GLOBAL, ' ')}" from ${source}. An endpoint cannot contain line breaks or control characters.` + `Invalid endpoint "${redact(endpoint)}" from ${source}. An endpoint cannot contain line breaks or control characters.` ) } @@ -344,7 +369,7 @@ export function normalizeEndpoint(endpoint: string, source: string): string { parsed = new URL(trimmed) } catch { throw new ProfileConfigError( - `Invalid endpoint "${endpoint.replace(FORBIDDEN_IN_VALUE_GLOBAL, ' ')}" from ${source}. Use an absolute URL, e.g. ${DEFAULT_ENDPOINT} or http://localhost:3000` + `Invalid endpoint "${redact(endpoint)}" from ${source}. Use an absolute URL, e.g. ${DEFAULT_ENDPOINT} or http://localhost:3000` ) } if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') { @@ -373,7 +398,7 @@ export function normalizeWorkspaceId(workspaceId: string, source: string): strin } if (FORBIDDEN_IN_VALUE.test(trimmed)) { throw new ProfileConfigError( - `Invalid workspace id "${trimmed.replace(FORBIDDEN_IN_VALUE_GLOBAL, ' ')}" from ${source}. A workspace id cannot contain line breaks or control characters.` + `Invalid workspace id "${redact(trimmed)}" from ${source}. A workspace id cannot contain line breaks or control characters.` ) } return trimmed @@ -396,7 +421,41 @@ function resolve( return { value: fallback, source: fallbackSource } } +/** The root flag each override arrives from, for the refusal below to name. */ +const OVERRIDE_FLAGS = { + profile: '--profile', + endpoint: '--endpoint', + workspaceId: '--workspace', +} as const + +/** + * Refuses a root flag typed with a blank value. + * + * {@link resolve} treats the empty string as "not supplied", which is right for + * an environment variable — an exported-but-empty `SIM_WORKSPACE` means unset — + * but wrong for a flag the user typed: `sim --workspace "" workflows list` fell + * through to the profile's stored workspace and ran against it, which is the + * opposite of what asking for no workspace means. Whitespace counts as blank + * for the same reason the config writer refuses it: nothing downstream would + * read it back as a value. + * + * `--output` is not here because commander's `choices()` already refuses a + * blank one before this runs. + */ +function refuseBlankOverrides(overrides: ProfileOverrides): void { + for (const [key, flag] of Object.entries(OVERRIDE_FLAGS)) { + const value = overrides[key as keyof typeof OVERRIDE_FLAGS] + if (value !== undefined && value.trim() === '') { + throw new ProfileConfigError( + `${flag} requires a value. Omit the flag to fall back to what is configured.` + ) + } + } +} + export function resolveProfile(overrides: ProfileOverrides = {}): ResolvedProfile { + refuseBlankOverrides(overrides) + const named = overrides.profile || process.env.SIM_PROFILE const name = named || DEFAULT_PROFILE if (named && !overrides.allowUnknownProfile) requireKnownProfile(named) @@ -453,7 +512,7 @@ export function resolveProfile(overrides: ProfileOverrides = {}): ResolvedProfil ) if (!(OUTPUT_FORMATS as readonly string[]).includes(output.value as string)) { throw new ProfileConfigError( - `Unknown output format "${output.value}" from ${output.source}. Use one of: ${OUTPUT_FORMATS.join(', ')}` + `Unknown output format "${redact(String(output.value))}" from ${output.source}. Use one of: ${OUTPUT_FORMATS.join(', ')}` ) } diff --git a/packages/sim-cli/src/generated/v2-api.ts b/packages/sim-cli/src/generated/v2-api.ts index ff357faaa41..d623c667e6f 100644 --- a/packages/sim-cli/src/generated/v2-api.ts +++ b/packages/sim-cli/src/generated/v2-api.ts @@ -12209,7 +12209,7 @@ export const V2_OPERATIONS = { includeJobRuns: { kind: 'boolean', describe: - 'Whether Chat and Sim-agent job runs join the sequence alongside workflow runs. Job runs report `kind: "job"`, carry no `workflow` summary, and never carry a cost ledger. They are dropped entirely — not partially matched — whenever a filter they cannot answer is set (`workflowIds`, `workflowName`, `folderPaths`, `model`, or `status`), so a filter never means two different things across the union. Accepted only under `sortBy=startedAt`: job runs record cost as a document and no comparable status, so they cannot participate in the other orderings.', + 'Whether Chat and Sim-agent job runs join the sequence alongside workflow runs. Job runs report `kind: "job"`, carry no `workflow` summary, and never carry a cost ledger. They are dropped entirely — not partially matched — whenever a filter they cannot answer is set: by workflow, workflow name, folder, model, or status. A filter therefore never means two different things across the union. Accepted only when sorting by `startedAt`: job runs record cost as a document and no comparable status, so they cannot participate in the other orderings.', }, runId: { kind: 'string', describe: 'Exact run identifier to match.' }, sortBy: { @@ -12217,7 +12217,7 @@ export const V2_OPERATIONS = { values: ['startedAt', 'durationMs', 'cost', 'status'] as const, default: 'startedAt', describe: - 'Field used to sort the result. `durationMs` and `cost` are null until a run settles; those runs order as though the value were below every recorded one, so they trail an ascending page and lead a descending one. Only `startedAt` can order Chat and Sim-agent job runs, so any other value is rejected together with `includeJobRuns=true`.', + 'Field used to sort the result. `durationMs` and `cost` are null until a run settles; those runs order as though the value were below every recorded one, so they trail an ascending page and lead a descending one. Only `startedAt` can order Chat and Sim-agent job runs, so any other value is rejected when job runs are included.', }, sortOrder: { kind: 'enum', @@ -12764,7 +12764,7 @@ export const V2_OPERATIONS = { values: ['active', 'archived'] as const, default: 'active', describe: - 'Which lifecycle set to list: `active` (default) for live workflows, `archived` for workflows a `DELETE` archived. `folderPath` resolves against active folders only, so pairing it with `scope=archived` returns an empty page when the containing folder was archived too.', + 'Which lifecycle set to list: `active` (default) for live workflows, `archived` for workflows a `DELETE` archived. The folder filter resolves against active folders only, so pairing it with `archived` returns an empty page when the containing folder was archived too.', }, folderPath: { kind: 'string', diff --git a/packages/sim-cli/src/program.test.ts b/packages/sim-cli/src/program.test.ts index b3916507243..bb3b94ba5af 100644 --- a/packages/sim-cli/src/program.test.ts +++ b/packages/sim-cli/src/program.test.ts @@ -121,14 +121,41 @@ describe('help typed after a command that does not exist', () => { }) /** - * Both exclusions are load-bearing: `files restore` registers a positional - * while hosting a subcommand, and `profiles` acts on its own, so an operand - * there is not an unknown command. + * `files restore ` is the only command in the tree that hosts a + * subcommand and takes an operand of its own, so it is the only one the guard + * has to step around — and it earns that with the registered positional, not + * by acting on its own. */ - it('leaves a command that legitimately takes an operand alone', async () => { + it('leaves the one command that legitimately takes an operand alone', async () => { expect((await parse(['files', 'restore', 'wf_1', '--help'])).code).toBe( 'commander.helpDisplayed' ) - expect((await parse(['profiles', 'zzzz', '--help'])).code).toBe('commander.helpDisplayed') + }) + + /** + * `profiles` acts on its own but takes no operand, and the exclusion covered + * it anyway: `sim profiles zzznope --help` answered `0` where every other + * group answers `1`, and without the flag it printed the profile table. + */ + it('refuses an unknown operand under profiles, with and without --help', async () => { + expect((await parse(['profiles', 'zzznope', '--help'])).code).toBe('commander.unknownCommand') + expect((await parse(['profiles', 'zzznope'])).code).toBe('commander.unknownCommand') + }) + + it('still answers help for profiles and for its commands', async () => { + const group = await parse(['profiles', '--help']) + expect(group.code).toBe('commander.helpDisplayed') + expect(group.out).toContain('Usage: sim profiles') + + const leaf = await parse(['profiles', 'add', '--help']) + expect(leaf.code).toBe('commander.helpDisplayed') + expect(leaf.out).toContain('Usage: sim profiles add') + + // `help` is commander's implicit command rather than a registered one, so + // it is the operand the unknown-command check is most likely to mistake for + // a typo. + const implicit = await parse(['profiles', 'help', 'add']) + expect(implicit.code).toBe('commander.help') + expect(implicit.out).toContain('Usage: sim profiles add') }) }) diff --git a/packages/sim-cli/src/runtime/build.test.ts b/packages/sim-cli/src/runtime/build.test.ts index 5bfbcc7f0a9..b0a42c10148 100644 --- a/packages/sim-cli/src/runtime/build.test.ts +++ b/packages/sim-cli/src/runtime/build.test.ts @@ -87,6 +87,17 @@ function wireSpelledFlags(command: Command, prefix: string[] = []): string[] { return offenders } +/** `commandAt`, but on the real program, so hand-written commands are present. */ +function builtCommandAt(...names: string[]): Command { + let current = buildProgram() + for (const name of names) { + const next = current.commands.find((command) => command.name() === name) + if (!next) throw new Error(`Missing command ${names.join(' ')}`) + current = next + } + return current +} + function commandAt(...names: string[]): Command { let current = program() for (const name of names) { @@ -231,6 +242,59 @@ describe('commands parsed through commander', () => { expect(commandAt('mcp-servers', 'list').description()).not.toContain('personal API key') expect(commandAt('workflows', 'list').description()).not.toContain('personal API key') }) + + /** + * A fully hand-written command renders its own `.description()` and so + * never reaches the generated path. Left to itself it sits in a menu beside + * suffixed siblings, which reads as the one command that does take a + * workspace key — the exact confusion the suffix exists to remove. + */ + it('says so on a fully hand-written command', () => { + for (const path of [ + ['secrets', 'set'], + ['credentials', 'create'], + ['credentials', 'connect'], + ['credentials', 'reconnect'], + ]) { + expect(`${path.join(' ')}: ${builtCommandAt(...path).description()}`).toContain( + '(personal API key required)' + ) + } + }) + + /** + * Catches the next hand-written command rather than only the four that + * exist: a restricted operation invoked from `src/commands` has to state + * the restriction through the one helper that owns the wording. + * + * Reading the source is what makes this general — a hand-written command + * declares nothing that ties it back to its operation at runtime, so the + * `V2_OPERATIONS.` reference is the only link there is. + */ + it('is enforced for every restricted operation a hand-written command calls', async () => { + const { readdirSync, readFileSync } = await import('node:fs') + const { join } = await import('node:path') + const root = join(import.meta.dirname, '..', 'commands') + + const sources = readdirSync(root, { recursive: true, withFileTypes: true }) + .filter((entry) => entry.isFile() && entry.name.endsWith('.ts')) + .filter((entry) => !entry.name.endsWith('.test.ts')) + .map((entry) => join(entry.parentPath, entry.name)) + + const unsuffixed: string[] = [] + for (const source of sources) { + const text = readFileSync(source, 'utf8') + for (const [, operation] of text.matchAll(/V2_OPERATIONS\.([A-Za-z]+)/g)) { + const spec = (V2_OPERATIONS as Record)[operation] + if (!spec?.personalKeyOnly) continue + const suffixed = new RegExp(`describeOperation\\(\\s*V2_OPERATIONS\\.${operation}\\b`) + if (suffixed.test(text)) continue + unsuffixed.push(`${source.slice(root.length + 1)} calls ${operation}`) + } + } + + expect([...new Set(unsuffixed)]).toEqual([]) + }) }) it('describes generated resource and sub-resource groups', () => { diff --git a/packages/sim-cli/src/runtime/build.ts b/packages/sim-cli/src/runtime/build.ts index 0f778e56347..b387f6420a6 100644 --- a/packages/sim-cli/src/runtime/build.ts +++ b/packages/sim-cli/src/runtime/build.ts @@ -31,6 +31,20 @@ const GROUP_ALIASES: Readonly> = { workspaces: 'workspace', } +/** + * States the personal-key restriction the way every generated command states it. + * + * The suffix lives here, once, because a fully hand-written command renders its + * own `.description()` and never reaches the generated path — three commands + * (`secrets set`, `credentials create`, `credentials connect`/`reconnect`) sat + * beside siblings that carried the warning and silently read as accepting a + * workspace key. Taking the `OperationSpec` rather than a boolean means a + * caller has to name the operation it actually calls, so the two cannot drift. + */ +export function describeOperation(operationSpec: OperationSpec, described: string): string { + return operationSpec.personalKeyOnly ? `${described} (personal API key required)` : described +} + function argumentSyntax(command: Command): string { return command.registeredArguments .map((argument) => { @@ -265,10 +279,11 @@ function configureOperation( // Appended after the whole fallback chain, not inside the summary branch: a // command with a hand-written `describe` needs the restriction stated just as // much as one falling back to the spec summary. - const described = - spec.describe ?? operationSpec.summary ?? `${operationSpec.method} ${operationSpec.path}` command.description( - operationSpec.personalKeyOnly ? `${described} (personal API key required)` : described + describeOperation( + operationSpec, + spec.describe ?? operationSpec.summary ?? `${operationSpec.method} ${operationSpec.path}` + ) ) addOperationOptions(command, operation, spec, operationSpec) assertNoReservedFlags(command, operation) diff --git a/packages/sim-cli/src/runtime/request.test.ts b/packages/sim-cli/src/runtime/request.test.ts index 9808aff4498..3ddc3f36223 100644 --- a/packages/sim-cli/src/runtime/request.test.ts +++ b/packages/sim-cli/src/runtime/request.test.ts @@ -127,7 +127,8 @@ describe('buildRequest', () => { expect(buildRequest('updateWorkflow', ['wf_1'], { description: '' }, WORKSPACE).body).toEqual({ description: '', }) - // Blank-scoped to the query on both spellings: a body string is the value. + // A body string carries a blank on both spellings: it is the value, not a + // filter, and it is the one kind the numeric refusal below must not reach. expect(buildRequest('updateWorkflow', ['wf_1'], { description: ' ' }, WORKSPACE).body).toEqual({ description: ' ', }) @@ -176,6 +177,27 @@ describe('buildRequest', () => { ) }) + /** + * The same coercion, in the other slot. `tables rows batch-delete --limit + * ""` sent `"limit":0` — a cap on a destructive operation that the caller + * never typed — so the refusal follows the field's declared kind rather + * than the slot it sits in. The body string above stays sendable. + */ + it('rejects a blank numeric body field, which coercion would read as 0 too', () => { + expect(() => + buildRequest('deleteTableRows', ['tbl_1'], { filter: '{"all":[]}', limit: '' }, WORKSPACE) + ).toThrow('--limit cannot be empty') + expect(() => + buildRequest('deleteTableRows', ['tbl_1'], { filter: '{"all":[]}', limit: ' ' }, WORKSPACE) + ).toThrow('--limit cannot be empty') + expect(() => + buildRequest('searchKnowledge', [], { kb: ['kb_1'], topK: '' }, WORKSPACE) + ).toThrow('--top-k cannot be empty') + expect(() => + buildRequest('rollbackWorkflow', ['wf_1'], { toVersion: ' ' }, WORKSPACE) + ).toThrow('--to-version cannot be empty') + }) + /** * A quoted space is invisible in a shell and reached the wire as every * blank the empty string did — `--max-cost " "` as a real `0` ceiling, @@ -208,11 +230,15 @@ describe('buildRequest', () => { * A paginating `limit` is the walk size, not a filter, and the pager reads * it from the flags itself — refusing a blank one in wording that says what * `0` means there. Left to it rather than pre-empted with a generic - * refusal, whitespace included: the pager trims before it decides. + * refusal, whitespace included: the pager trims before it decides. True in + * either slot — `queryRows` carries its cursor in the body, so the numeric + * refusal above must step aside there for the same reason. */ it('leaves a blank paginating limit to the pager, which words it better', () => { expect(() => buildRequest('listWorkflows', [], { limit: '' }, WORKSPACE)).not.toThrow() expect(() => buildRequest('listWorkflows', [], { limit: ' ' }, WORKSPACE)).not.toThrow() + expect(() => buildRequest('queryRows', ['tbl_1'], { limit: '' }, WORKSPACE)).not.toThrow() + expect(() => buildRequest('queryRows', ['tbl_1'], { limit: ' ' }, WORKSPACE)).not.toThrow() }) it('rejects a missing required flag', () => { diff --git a/packages/sim-cli/src/runtime/request.ts b/packages/sim-cli/src/runtime/request.ts index d3d611a54f9..3d1147a404d 100644 --- a/packages/sim-cli/src/runtime/request.ts +++ b/packages/sim-cli/src/runtime/request.ts @@ -50,6 +50,9 @@ export function cursorSlot( /** Kinds the CLI can only accept as a JSON string. */ const JSON_KINDS = new Set(['object', 'array', 'unknown']) +/** Kinds read through `Number`, where a blank does not survive: `Number('')` is `0`. */ +const NUMERIC_KINDS: ReadonlySet = new Set(['number', 'integer']) + export function flagSpecFor(operation: V2OperationName, field: string): FlagSpec { return CLI_CONTRACT[operation]?.flags?.[field] ?? {} } @@ -375,7 +378,7 @@ export function coerce(raw: unknown, field: FieldSpec, flag: FlagSpec, flagName: } } - if (field.kind === 'number' || field.kind === 'integer') { + if (NUMERIC_KINDS.has(field.kind)) { const value = Number(raw) if (Number.isNaN(value)) throw new SimApiError(`--${flagName} must be a number`, 0) return value @@ -515,8 +518,7 @@ export function buildRequest( * URL builder skips an empty value, so `logs list --status ""` searched * everything and answered `0`, a wider result set presented as an answer. * Refused here, before the request, the way an empty list entry and an - * empty path parameter already are. Scoped to the query, because an empty - * body string is meaningful — it clears a description. + * empty path parameter already are. * * Read from what the caller typed rather than from the coerced value, * because coercion erases the blank on a numeric field: `Number('')` is @@ -524,6 +526,13 @@ export function buildRequest( * nothing" filter that a check on the coerced value cannot see. An * explicit `--max-cost 0` is a value the caller chose and is still sent. * + * That erasure is what a numeric *body* field suffers too, so the rule is + * the whole query slot plus every numeric field wherever it sits: `tables + * rows batch-delete --limit ""` sent `"limit":0`, a cap the caller never + * typed. The slot alone is not the distinction — a blank is meaningful + * only where it can survive as itself, which is a body *string*, where it + * clears a description. + * * Blank is `trim()`-empty rather than exactly empty, matching both the * route — `blankQueryValueValidationError` reads `?status=%20` as blank — * and the list flag beside it, which trims each entry before refusing it. @@ -532,7 +541,7 @@ export function buildRequest( * explicit `false`, `--status " "` as a `%20` the server answers `400`. */ if ( - slot === 'query' && + (slot === 'query' || NUMERIC_KINDS.has(descriptor.kind)) && typeof raw === 'string' && raw.trim() === '' && !(field === 'limit' && paginatedLimit) diff --git a/packages/sim-cli/src/runtime/result.test.ts b/packages/sim-cli/src/runtime/result.test.ts index d9e9412c084..96551a37684 100644 --- a/packages/sim-cli/src/runtime/result.test.ts +++ b/packages/sim-cli/src/runtime/result.test.ts @@ -343,6 +343,38 @@ describe('a truncation the response states inside its payload', () => { expect(read()).toContain('the server clipped the tool names it returned') }) + /** + * The subject is the words before the suffix, and `is` is not one of them: + * `isTruncated` read as a clip of "the is". Latent — no shipped endpoint + * spells it that way — so this is what keeps it harmless if one ever does. + */ + it('reads a copula prefix as no subject rather than as one', () => { + const read = captureStderr() + + renderResult('readFileText', 'json', {}, {}, {}, { data: { isTruncated: true } }) + + expect(read()).toContain('the server clipped this result') + expect(read()).not.toContain('the is') + }) + + /** The strip stops at the copula: a real subject behind it still names itself. */ + it('keeps the subject behind a copula prefix', () => { + const read = captureStderr() + + renderResult('readFileText', 'json', {}, {}, {}, { data: { isToolNamesTruncated: true } }) + + expect(read()).toContain('the server clipped the tool names it returned') + }) + + /** And a subject that merely begins with those two letters is not a copula. */ + it('does not eat a subject that begins with is', () => { + const read = captureStderr() + + renderResult('readFileText', 'json', {}, {}, {}, { data: { issuesTruncated: true } }) + + expect(read()).toContain('the server clipped the issues it returned') + }) + it('says nothing when a negated flag reports the answer was whole', () => { const read = captureStderr() diff --git a/packages/sim-cli/src/runtime/result.ts b/packages/sim-cli/src/runtime/result.ts index a7e8259d658..1d24a88db81 100644 --- a/packages/sim-cli/src/runtime/result.ts +++ b/packages/sim-cli/src/runtime/result.ts @@ -393,10 +393,16 @@ function spellOut(flag: string): string { * `toolNamesTruncated` on `workflow-mcp-servers list` is a clip of each row's * tool names, not of the servers, so one wording about "this list" named the * wrong thing on the one endpoint whose subject is not the list. A bare - * `truncated` carries no subject and stands for the whole answer. + * `truncated` carries no subject and stands for the whole answer — and so does + * `isTruncated`, whose `is` is a copula rather than a subject: dropping it is + * what keeps "the server clipped **the is** it returned" from being printed. + * Only that one prefix is stripped, and only where it stands alone or before a + * real subject (`isToolNamesTruncated`), so a field genuinely beginning `is` + * (`issuesTruncated`) keeps its name. Like the negation veto above, this is a + * spelling worth anticipating rather than a decision procedure for English. */ function clippedSubject(flag: string): string { - const subject = flag.replace(/^truncated$|Truncated$/, '') + const subject = flag.replace(/^truncated$|Truncated$/, '').replace(/^is(?=[A-Z]|$)/, '') return subject ? `the ${spellOut(subject)} it returned` : 'this result' } From f8ea72d2743856c65a5d48527a154eabc8a82f02 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 26 Aug 2026 16:54:14 -0700 Subject: [PATCH 3/5] fix(scripts): match example uuids case-insensitively in the spec audit The pattern only recognised lowercase hex, so an uppercase id in a published spec was never examined and the audit reported success without having looked at it. Matching case-insensitively is not enough on its own: hex is case-insensitive, so a mixed-case id counts `A` and `a` as two digits and reports twenty distinct ones rather than sixteen. That inflated count clears the threshold the texture test uses to recognise a hand-authored placeholder, so a real id could have passed for one. The allowlist is an exact-string lookup and would likewise have missed an uppercase spelling of an entry. Both checks and the lookup now take a normalised id, while the finding still reports the spelling as it appears in the file. --- scripts/check-spec-example-ids.ts | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/scripts/check-spec-example-ids.ts b/scripts/check-spec-example-ids.ts index aeac1302739..d649eae86ec 100644 --- a/scripts/check-spec-example-ids.ts +++ b/scripts/check-spec-example-ids.ts @@ -16,7 +16,7 @@ import path from 'node:path' const ROOT = path.resolve(import.meta.dir, '..') const SPEC_DIR = path.join(ROOT, 'apps/docs') -const UUID_PATTERN = /[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}/g +const UUID_PATTERN = /[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}/gi /** Ids that are obviously synthetic to a reader but do not match the pandigital texture. */ const ALLOWED: Record = { @@ -25,12 +25,21 @@ const ALLOWED: Record = { 'pre-existing hand-authored block id shared with the v2 workflow operations tests', } -/** All-zero and all-f sentinels read as synthetic on sight. */ +/** + * All-zero and all-f sentinels read as synthetic on sight. + * + * Takes an already-lowercased id: hex is case-insensitive, so counting `A` and `a` as two + * digits would inflate the distinct count and misjudge the texture. + */ function isSentinel(uuid: string): boolean { return new Set(uuid.replace(/-/g, '')).size <= 2 } -/** Hand-authored placeholders in this repo use every hex digit, none more than three times. */ +/** + * Hand-authored placeholders in this repo use every hex digit, none more than three times. + * + * Takes an already-lowercased id, for the reason given on {@link isSentinel}. + */ function isHouseStyle(uuid: string): boolean { const hex = uuid.replace(/-/g, '') const digits = new Set(hex) @@ -48,7 +57,8 @@ for (const file of specFiles) { const contents = await Bun.file(path.join(SPEC_DIR, file)).text() const seen = new Set(contents.match(UUID_PATTERN) ?? []) for (const uuid of [...seen].sort()) { - if (uuid in ALLOWED || isSentinel(uuid) || isHouseStyle(uuid)) continue + const normalized = uuid.toLowerCase() + if (normalized in ALLOWED || isSentinel(normalized) || isHouseStyle(normalized)) continue findings.push(` - ${file}: ${uuid}`) } } From 7e831444cc3520646b1ee405b66dcc3398a346fe Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 26 Aug 2026 17:36:06 -0700 Subject: [PATCH 4/5] fix(cli): stop a refusal being swallowed, and gate example ids by name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A blank root flag was refused everywhere except `profiles`, where the catch that lets a broken profile still list absorbed it and the command exited 0 after printing the table. The refusal now carries its own error class, which is what the listing rethrows on — the two are distinguished by type rather than by matching message text, and a genuinely broken profile still lists. The unknown-profile message redacted the name the caller typed but not the suggestion or the list of configured names beside it, which come from the same file and are equally attacker-influenced once it has been hand-edited. Those are redacted now, as is every other message in these two files that quotes a name read out of the config, and the profile listing flattens the names it renders the way it already flattened the error column. The example-id audit judged a uuid by its digit texture, on the premise that a real one essentially never looks hand-authored. Measured against ten million generated ids, 0.81% of them do — one in 124, where this change alone replaced six. Requiring each digit exactly twice takes that to zero but rejects all fourteen placeholders now in the specs, so it is no cheaper than the alternative. The audit now holds the eighteen ids the specs actually use, which is one file rather than the twenty-seven a reserved format would touch, and a new id fails until someone lists it — which is the review the check exists to force. --- packages/sim-cli/src/commands/auth.test.ts | 43 ++++++++---- packages/sim-cli/src/commands/auth.ts | 49 +++++++------ packages/sim-cli/src/config/profile.test.ts | 37 ++++++++++ packages/sim-cli/src/config/profile.ts | 33 ++++++--- scripts/check-spec-example-ids.ts | 78 ++++++++++++++------- 5 files changed, 172 insertions(+), 68 deletions(-) diff --git a/packages/sim-cli/src/commands/auth.test.ts b/packages/sim-cli/src/commands/auth.test.ts index 1ce95179ffb..920ba55b3c7 100644 --- a/packages/sim-cli/src/commands/auth.test.ts +++ b/packages/sim-cli/src/commands/auth.test.ts @@ -10,7 +10,6 @@ const mocks = vi.hoisted(() => ({ listProfiles: vi.fn<() => string[]>(() => []), request: vi.fn(), readCredentialsProfile: vi.fn<() => Record>(() => ({})), - ProfileConfigError: class ProfileConfigError extends Error {}, resolveAuthenticationProfileName: vi.fn((profile: string) => profile), pollForKey: vi.fn(async () => ({ apiKey: 'sim-key', @@ -49,10 +48,17 @@ vi.mock('../auth/device-flow', () => ({ */ vi.mock('../config/index', async () => ({ ...(await import('../config/profile').then( - ({ FORBIDDEN_IN_VALUE, normalizeWorkspaceId, OUTPUT_FORMATS, validateProfileName }) => ({ + ({ FORBIDDEN_IN_VALUE, normalizeWorkspaceId, OUTPUT_FORMATS, + ProfileConfigError, + validateProfileName, + }) => ({ + FORBIDDEN_IN_VALUE, + normalizeWorkspaceId, + OUTPUT_FORMATS, + ProfileConfigError, validateProfileName, }) )), @@ -62,7 +68,6 @@ vi.mock('../config/index', async () => ({ deleteProfile: mocks.deleteProfile, listAuthenticationDependents: mocks.listAuthenticationDependents, listProfiles: mocks.listProfiles, - ProfileConfigError: mocks.ProfileConfigError, readCredentialsProfile: mocks.readCredentialsProfile, resolveAuthenticationProfileName: mocks.resolveAuthenticationProfileName, writeConfigProfile: mocks.writeConfigProfile, @@ -74,6 +79,7 @@ vi.mock('../context', () => ({ clientFrom: () => ({ client: { request: mocks.request }, profile: mocks.profileFrom() }), })) +import { ProfileConfigError, ProfileOverrideError } from '../config/profile' import { SimApiError } from '../http/client' import { loginCommand, logoutCommand, profilesCommand, whoamiCommand } from './auth' @@ -607,7 +613,7 @@ describe('profiles command', () => { // nothing, alone among the commands, because it never resolved at all. mocks.listProfiles.mockReturnValue(['default']) mocks.profileFrom.mockImplementation(() => { - throw new mocks.ProfileConfigError('Unknown profile "bogus".') + throw new ProfileConfigError('Unknown profile "bogus".') }) await expect(profiles('list', '--profile', 'bogus')).rejects.toThrow('Unknown profile "bogus".') @@ -665,13 +671,11 @@ describe('profiles command', () => { // it throws for exactly the profile this command exists to show. mocks.listProfiles.mockReturnValue(['broken', 'default']) mocks.profileFrom.mockImplementation(() => { - throw new mocks.ProfileConfigError('Profile "broken" references missing auth_profile "gone".') + throw new ProfileConfigError('Profile "broken" references missing auth_profile "gone".') }) mocks.resolveAuthenticationProfileName.mockImplementation((profile) => { if (profile === 'broken') { - throw new mocks.ProfileConfigError( - 'Profile "broken" references missing auth_profile "gone".' - ) + throw new ProfileConfigError('Profile "broken" references missing auth_profile "gone".') } return profile }) @@ -689,7 +693,7 @@ describe('profiles command', () => { // profiles` must fail exactly like `sim profiles --profile typo`. mocks.listProfiles.mockReturnValue(['default']) mocks.profileFrom.mockImplementation(() => { - throw new mocks.ProfileConfigError('Unknown profile "bogus".') + throw new ProfileConfigError('Unknown profile "bogus".') }) process.env.SIM_PROFILE = 'bogus' @@ -706,7 +710,7 @@ describe('profiles command', () => { // exit 0. The catch exists to tolerate a broken *profile*, not a bad flag. mocks.listProfiles.mockReturnValue(['default']) mocks.profileFrom.mockImplementation(() => { - throw new mocks.ProfileConfigError('Unknown output format "jsonl" from env.') + throw new ProfileConfigError('Unknown output format "jsonl" from env.') }) process.env.SIM_OUTPUT = 'jsonl' @@ -718,15 +722,28 @@ describe('profiles command', () => { } }) + it('refuses a blank root flag rather than absorbing it into the broken-profile fallback', async () => { + // The tolerance below exists for a profile that will not resolve. A blank + // `--workspace` is the caller's own argument, and swallowing it listed + // profiles and exited 0 where every other command exits 1. + mocks.listProfiles.mockReturnValue(['default']) + mocks.profileFrom.mockImplementation(() => { + throw new ProfileOverrideError('--workspace requires a value.') + }) + + await expect(profiles('list', '--workspace', '')).rejects.toThrow( + '--workspace requires a value.' + ) + expect(console.log).not.toHaveBeenCalled() + }) + it('marks a broken profile and still lists the rest', async () => { // `profiles` is the command someone runs *because* a profile is broken, and // one bad auth_profile used to abort the listing with nothing shown at all. mocks.listProfiles.mockReturnValue(['broken', 'default', 'dev']) mocks.resolveAuthenticationProfileName.mockImplementation((profile) => { if (profile === 'broken') { - throw new mocks.ProfileConfigError( - 'Profile "broken" references missing auth_profile "gone".' - ) + throw new ProfileConfigError('Profile "broken" references missing auth_profile "gone".') } return profile }) diff --git a/packages/sim-cli/src/commands/auth.ts b/packages/sim-cli/src/commands/auth.ts index 617d5c25fb0..1316dd7c027 100644 --- a/packages/sim-cli/src/commands/auth.ts +++ b/packages/sim-cli/src/commands/auth.ts @@ -28,6 +28,7 @@ import { writeConfigProfile, writeCredentialsProfile, } from '../config/index' +import { ProfileOverrideError, redact } from '../config/profile' import { clientFrom, globalsOf, profileFrom } from '../context' import { type GetMetaResponse, @@ -91,7 +92,7 @@ function presentAuthentication(source: SettingSource): { async function confirmProfileOverwrite(profileName: string): Promise { if (!process.stdin.isTTY) { throw new SimApiError( - `Profile "${profileName}" already exists. Re-run with --yes to overwrite it.`, + `Profile "${redact(profileName)}" already exists. Re-run with --yes to overwrite it.`, 0 ) } @@ -99,7 +100,7 @@ async function confirmProfileOverwrite(profileName: string): Promise { const prompt = createInterface({ input: process.stdin, output: process.stderr }) try { const answer = await prompt.question( - `Profile "${profileName}" already exists. Replace its API key and login defaults? (y/N) ` + `Profile "${redact(profileName)}" already exists. Replace its API key and login defaults? (y/N) ` ) return answer.trim().toLowerCase() === 'y' || answer.trim().toLowerCase() === 'yes' } finally { @@ -117,7 +118,7 @@ function validateNewProfileName(profileName: string): void { validateProfileName(profileName) if (listProfiles().includes(profileName)) { throw new SimApiError( - `Profile "${profileName}" already exists. Remove it first with: sim logout --all --profile ${profileName}`, + `Profile "${redact(profileName)}" already exists. Remove it first with: sim logout --all --profile ${redact(profileName)}`, 0 ) } @@ -160,13 +161,13 @@ function requireStoredAuthentication(profile: ResolvedProfile): string { const storedKey = readCredentialsProfile(authProfile).api_key if (profile.sources.apiKey !== 'credentials' || !storedKey) { throw new SimApiError( - `Cannot create a shared profile from "${profile.name}": the active API key is not stored. Run: sim login --profile ${authProfile}`, + `Cannot create a shared profile from "${redact(profile.name)}": the active API key is not stored. Run: sim login --profile ${redact(authProfile)}`, 0 ) } if (profile.sources.endpoint === 'flag' || profile.sources.endpoint === 'env') { throw new SimApiError( - `Cannot create a shared profile from "${profile.name}": the active endpoint comes from ${profile.sources.endpoint}. Save it with: sim configure --profile ${authProfile} --set-endpoint ${profile.endpoint}`, + `Cannot create a shared profile from "${redact(profile.name)}": the active endpoint comes from ${profile.sources.endpoint}. Save it with: sim configure --profile ${redact(authProfile)} --set-endpoint ${profile.endpoint}`, 0 ) } @@ -255,10 +256,10 @@ function addProfileCommand(): Command { workspace: normalizeWorkspaceId(workspace.id, 'the workspace response'), }) - console.log(chalk.green(`✓ Added profile "${profileName}" in ${configPath()}`)) + console.log(chalk.green(`✓ Added profile "${safeOneLine(profileName)}" in ${configPath()}`)) console.log(` Workspace: ${safeOneLine(workspace.name)} (${workspace.id})`) - console.log(` Authentication: ${authProfile}`) - console.log(chalk.dim(` Try: sim --profile ${profileName} whoami`)) + console.log(` Authentication: ${safeOneLine(authProfile)}`) + console.log(chalk.dim(` Try: sim --profile ${safeOneLine(profileName)} whoami`)) }) } @@ -277,7 +278,7 @@ export function loginCommand(): Command { if (authProfile !== profile.name) { throw new SimApiError( - `Profile "${profile.name}" shares authentication with "${authProfile}". Run: sim login --profile ${authProfile}`, + `Profile "${redact(profile.name)}" shares authentication with "${redact(authProfile)}". Run: sim login --profile ${redact(authProfile)}`, 0 ) } @@ -304,7 +305,7 @@ export function loginCommand(): Command { ) console.log( - `Signing in to ${chalk.bold(profile.endpoint)} as profile ${chalk.bold(profile.name)}` + `Signing in to ${chalk.bold(profile.endpoint)} as profile ${chalk.bold(safeOneLine(profile.name))}` ) console.log(`\nPairing code: ${chalk.bold(auth.pairing)}`) console.log( @@ -386,16 +387,16 @@ export function logoutCommand(): Command { const dependents = listAuthenticationDependents(profileName) if (dependents.length > 0) { throw new SimApiError( - `Cannot remove authentication profile "${profileName}" because it is used by: ${dependents.join(', ')}. Remove those profiles first.`, + `Cannot remove authentication profile "${redact(profileName)}" because it is used by: ${dependents.map(redact).join(', ')}. Remove those profiles first.`, 0 ) } const removed = deleteProfile(profileName) if (!removed.config && !removed.credentials) { - console.log(chalk.dim(`Nothing stored for profile "${profileName}".`)) + console.log(chalk.dim(`Nothing stored for profile "${safeOneLine(profileName)}".`)) return } - console.log(chalk.green(`✓ Removed profile "${profileName}".`)) + console.log(chalk.green(`✓ Removed profile "${safeOneLine(profileName)}".`)) return } @@ -403,18 +404,20 @@ export function logoutCommand(): Command { const authProfile = resolveAuthenticationProfileName(profile.name) if (authProfile !== profile.name) { throw new SimApiError( - `Profile "${profile.name}" shares authentication with "${authProfile}". Log out of the authentication profile instead: sim logout --profile ${authProfile}`, + `Profile "${redact(profile.name)}" shares authentication with "${redact(authProfile)}". Log out of the authentication profile instead: sim logout --profile ${redact(authProfile)}`, 0 ) } if (!readCredentialsProfile(profile.name).api_key) { - console.log(chalk.dim(`No stored key for profile "${profile.name}".`)) + console.log(chalk.dim(`No stored key for profile "${safeOneLine(profile.name)}".`)) return } writeCredentialsProfile(profile.name, null) - console.log(chalk.green(`✓ Removed the stored key for profile "${profile.name}".`)) + console.log( + chalk.green(`✓ Removed the stored key for profile "${safeOneLine(profile.name)}".`) + ) // The key still exists server-side; leaving that unsaid invites the // assumption that logging out revoked it. console.log(chalk.dim(' The key itself is still active — revoke it in Settings → API keys.')) @@ -522,7 +525,7 @@ async function verifyProfile( status: 'unauthenticated', workspace: null, keyType: null, - detail: `no API key — run: sim login --profile ${profile.name}`, + detail: `no API key — run: sim login --profile ${safeOneLine(profile.name)}`, } } @@ -536,7 +539,7 @@ async function verifyProfile( status: 'no-workspace', workspace: null, keyType, - detail: `no workspace to check against — run: sim configure --profile ${profile.name} --set-workspace `, + detail: `no workspace to check against — run: sim configure --profile ${safeOneLine(profile.name)} --set-workspace `, } } @@ -663,9 +666,9 @@ interface ProfileRow { const PROFILE_COLUMNS: Column[] = [ { header: '', value: (row) => (row.active ? chalk.green('*') : ' ') }, - { header: 'profile', value: (row) => text(row.name) }, + { header: 'profile', value: (row) => safeOneLine(row.name) }, { header: 'key', value: (row) => (row.error ? text(null) : row.hasKey ? 'yes' : 'no') }, - { header: 'auth', value: (row) => text(row.authProfile) }, + { header: 'auth', value: (row) => (row.authProfile ? safeOneLine(row.authProfile) : text(null)) }, { header: 'error', value: (row) => (row.error ? chalk.red(safeOneLine(row.error)) : text(null)) }, ] @@ -707,6 +710,12 @@ function profileListingContext(command: Command): { activeName: string; output: return { activeName: profile.name, output: profile.output } } catch (error) { if (!(error instanceof ProfileConfigError)) throw error + // A blank `--profile`/`--endpoint`/`--workspace` is the caller's own + // argument, not a broken profile. The tolerance below exists so a listing + // still happens when the config is unreadable; letting it also absorb a + // refused flag turned `sim --workspace "" profiles` into a successful + // listing while every other command exits 1 on the same argv. + if (error instanceof ProfileOverrideError) throw error const globals = globalsOf(command) const named = globals.profile || process.env.SIM_PROFILE diff --git a/packages/sim-cli/src/config/profile.test.ts b/packages/sim-cli/src/config/profile.test.ts index fa94e9f7c3c..26974605938 100644 --- a/packages/sim-cli/src/config/profile.test.ts +++ b/packages/sim-cli/src/config/profile.test.ts @@ -10,6 +10,7 @@ import { listAuthenticationDependents, listProfiles, OUTPUT_FORMATS, + ProfileOverrideError, resolveAuthenticationProfileName, resolveProfile, validateProfileName, @@ -525,6 +526,16 @@ describe('blank root flags', () => { expect(() => resolveProfile({ profile: '' })).toThrow(/--profile requires a value/) }) + /** + * `profiles` tolerates a profile that will not resolve, so the refusal has to + * be tellable apart from a broken profile by something the wording cannot + * break — otherwise a blank flag is absorbed and the listing exits 0. + */ + it('raises the refusal as its own error class', () => { + expect(() => resolveProfile({ workspaceId: '' })).toThrow(ProfileOverrideError) + expect(() => resolveProfile({ profile: 'unknown' })).not.toThrow(ProfileOverrideError) + }) + it('still reads an exported-but-empty environment variable as unset', () => { // The convention every profile-based CLI follows, and the reason the empty // string cannot simply become significant everywhere. @@ -590,6 +601,32 @@ describe('redaction of rejected values', () => { expect(message).not.toMatch(FORBIDDEN_IN_VALUE) }) + /** + * The header line is split on `\n`, so a stored name cannot carry one — but + * U+2028 is a line separator the reader keeps and `sanitize` does not strip, + * which is why the same redaction the typed name already got has to cover the + * two halves of this message that come out of the config file. + */ + it('redacts the suggestion and the configured list, not just the typed name', () => { + writeFileSync(configPath(), '[profile st\u2028aging]\nworkspace = ws_1\n') + + const message = messageOf(() => resolveProfile({ profile: 'st\u2028agng' })) + expect(message).toBe( + 'Unknown profile "st agng". Did you mean "st aging"? Configured profiles: st aging.' + ) + expect(message).not.toMatch(FORBIDDEN_IN_VALUE) + }) + + it('redacts both names an auth_profile refusal quotes', () => { + // ESC rather than U+2028 here: the value reader drops a line separator, so + // the setting would never be seen at all. + writeFileSync(configPath(), '[profile de\u001bv]\nauth_profile = mis\u001bsing\n') + + const message = messageOf(() => resolveAuthenticationProfileName('de\u001bv')) + expect(message).toBe('Profile "de v" references missing auth_profile "mis sing".') + expect(message).not.toMatch(FORBIDDEN_IN_VALUE) + }) + it('redacts the format in the unknown-output-format refusal', () => { process.env.SIM_OUTPUT = 'ev\nil' diff --git a/packages/sim-cli/src/config/profile.ts b/packages/sim-cli/src/config/profile.ts index 2cf47ae8f16..7cbcaeb46d6 100644 --- a/packages/sim-cli/src/config/profile.ts +++ b/packages/sim-cli/src/config/profile.ts @@ -55,6 +55,23 @@ export function redact(value: string): string { return value.replace(FORBIDDEN_IN_VALUE_GLOBAL, ' ') } +/** + * A root flag typed with a value the CLI cannot use. + * + * Separate from the rest of {@link ProfileConfigError} so a caller that + * deliberately tolerates an unresolvable profile — `profiles` shows a broken + * one as a marked row rather than aborting, because that is the command you run + * *because* a profile is broken — can still refuse the caller's own argument. + * The class is the discriminator on purpose: matching the message text would + * stop working the first time the wording changes. + */ +export class ProfileOverrideError extends ProfileConfigError { + constructor(message: string) { + super(message) + this.name = 'ProfileOverrideError' + } +} + /** * The shape a newly created profile name has to have. * @@ -162,21 +179,21 @@ export function resolveAuthenticationProfileName(profile: string): string { const authProfile = config.auth_profile.trim() if (!authProfile) { - throw new ProfileConfigError(`Profile "${profile}" has an empty auth_profile.`) + throw new ProfileConfigError(`Profile "${redact(profile)}" has an empty auth_profile.`) } if (authProfile === profile) { throw new ProfileConfigError( - `Profile "${profile}" cannot use itself as auth_profile. Remove the auth_profile setting instead.` + `Profile "${redact(profile)}" cannot use itself as auth_profile. Remove the auth_profile setting instead.` ) } if (Object.hasOwn(config, 'endpoint')) { throw new ProfileConfigError( - `Profile "${profile}" cannot set both auth_profile and endpoint. Set the endpoint on authentication profile "${authProfile}".` + `Profile "${redact(profile)}" cannot set both auth_profile and endpoint. Set the endpoint on authentication profile "${redact(authProfile)}".` ) } if (readCredentialsProfile(profile).api_key) { throw new ProfileConfigError( - `Profile "${profile}" cannot set both auth_profile and its own API key. Remove one of them.` + `Profile "${redact(profile)}" cannot set both auth_profile and its own API key. Remove one of them.` ) } @@ -184,12 +201,12 @@ export function resolveAuthenticationProfileName(profile: string): string { const credentials = readCredentialsProfile(authProfile) if (Object.keys(authConfig).length === 0 && Object.keys(credentials).length === 0) { throw new ProfileConfigError( - `Profile "${profile}" references missing auth_profile "${authProfile}".` + `Profile "${redact(profile)}" references missing auth_profile "${redact(authProfile)}".` ) } if (Object.hasOwn(authConfig, 'auth_profile')) { throw new ProfileConfigError( - `Profile "${profile}" references auth_profile "${authProfile}", which also has auth_profile set. Authentication profile references cannot be chained.` + `Profile "${redact(profile)}" references auth_profile "${redact(authProfile)}", which also has auth_profile set. Authentication profile references cannot be chained.` ) } @@ -283,7 +300,7 @@ function requireKnownProfile(name: string): void { const suggestion = nearestProfile(name, known) throw new ProfileConfigError( - `Unknown profile "${redact(name)}".${suggestion ? ` Did you mean "${suggestion}"?` : ''} Configured profiles: ${known.join(', ')}.` + `Unknown profile "${redact(name)}".${suggestion ? ` Did you mean "${redact(suggestion)}"?` : ''} Configured profiles: ${known.map(redact).join(', ')}.` ) } @@ -446,7 +463,7 @@ function refuseBlankOverrides(overrides: ProfileOverrides): void { for (const [key, flag] of Object.entries(OVERRIDE_FLAGS)) { const value = overrides[key as keyof typeof OVERRIDE_FLAGS] if (value !== undefined && value.trim() === '') { - throw new ProfileConfigError( + throw new ProfileOverrideError( `${flag} requires a value. Omit the flag to fall back to what is configured.` ) } diff --git a/scripts/check-spec-example-ids.ts b/scripts/check-spec-example-ids.ts index d649eae86ec..a74463d87ee 100644 --- a/scripts/check-spec-example-ids.ts +++ b/scripts/check-spec-example-ids.ts @@ -1,12 +1,24 @@ #!/usr/bin/env bun /** - * Flags example UUIDs in the published OpenAPI specs that do not look deliberately synthetic. + * Gates the example UUIDs published in the OpenAPI specs against an explicit approved set. * * The specs in `apps/docs/` ship in a public repository, so an example id copied from a real run - * publishes an opaque workspace, workflow, or execution identifier. The repo's hand-crafted - * placeholders are pandigital — every hex digit appears, none more than three times - * (`3b1f7c92-8d4e-4a6b-9c0d-5e2f8a714b36`) — a texture a real v4 UUID essentially never has. - * That gives a mechanical signature for "this was generated, not authored". + * publishes an opaque workspace, workflow, or execution identifier. This check exists because that + * has already happened: six such ids sat in `openapi-core.json` until they were replaced. + * + * The gate is enumeration, not inference. An earlier revision instead scored the *texture* of an + * id — the repo's hand-authored placeholders are pandigital, using every hex digit no more than + * three times, which a real v4 UUID was assumed never to be. Measured against 12M generated v4 + * UUIDs, 0.81% of them have exactly that texture: roughly one copied id in 124 would have passed + * and shipped. Tightening the rule does not rescue it — requiring every digit exactly twice drops + * the random pass rate to 0 in 20M, but also rejects all fourteen placeholders in use today, so it + * is the same cost as changing every id. Enumeration reaches zero false negatives for the price of + * one line per id, and the ids are few: eighteen across eight specs, introduced by six commits in + * two and a half months, because the specs deliberately share a handful of placeholders. + * + * Adding an endpoint whose example uses a new id therefore fails this check until the id is listed + * below. That failure is the point — it is the moment a human confirms the id was authored rather + * than pasted, and the diff line records the answer for the next reader. * * The directory is globbed rather than read from `OPENAPI_SPEC_FILES`, because the gap this closes * is precisely that `openapi-core.json` is absent from that manifest and from every other check. @@ -18,35 +30,45 @@ const ROOT = path.resolve(import.meta.dir, '..') const SPEC_DIR = path.join(ROOT, 'apps/docs') const UUID_PATTERN = /[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}/gi -/** Ids that are obviously synthetic to a reader but do not match the pandigital texture. */ -const ALLOWED: Record = { - '7c9e6679-7425-40de-944b-e07fc1f90ae7': "Wikipedia's canonical example UUID", +/** + * Every example UUID the specs are allowed to publish, with what it stands for. + * + * Sentinels are covered structurally by {@link isSentinel} and are not listed here. + */ +const APPROVED: Record = { + '0f7c1a2e-9b3d-4c58-8a21-6d4e5f7a9b01': 'run id in the RUN_ID_CONFLICT error example', + '3b1f7c92-8d4e-4a6b-9c0d-5e2f8a714b36': 'the shared workflow id placeholder', + '4c1f9e77-2b3a-4f8d-9e10-6a2c8d4b1e05': 'KNOWLEDGE_CHUNK_ID', + '7c9e6679-7425-40de-944b-e07fc1f90ae7': "KNOWLEDGE_BASE_ID, Wikipedia's canonical example UUID", + '9d3b7f10-2c8e-4a56-b0f4-6e1a8c5d2b97': 'pausedExecutionId on the paused-execution example', + '9f4c2a10-3b7e-4d58-8f6a-2c1d0e5b7a94': 'second workspace id, where one example needs two', 'a3f1c0b2-7a44-4c1d-9d3a-2b8e5f0a1c77': - 'pre-existing hand-authored block id shared with the v2 workflow operations tests', + 'minted block id, shared with the v2 workflow operations tests', + 'a6f0c8d2-3e57-4b19-8d4a-1c9e2f6b0a35': 'block id in the selected-outputs selector example', + 'a91c4b2e-6d3f-4e8a-b5c7-0d9e2f1a8c64': 'the shared workspace id placeholder', + 'b2d4f8a0-1c3e-4a5b-9d7c-2e6f0a8b4c12': 'KNOWLEDGE_DOCUMENT_ID', + 'b8c2e60f-1a47-4d35-9e8b-3f0d5a7c2e19': 'executionId on the failed-execution example', + 'c7a92e15-3f4b-4d8c-a1e6-9b0d5f2c8e74': 'executionId returned by the execute example', + 'd5e1a3c7-8f60-4b29-9c4d-2a6e0f8b3d17': 'executionId on the paused-execution example', + 'e4f8d2b6-9a1c-4e3d-8b7f-5c0a2d9e6f13': 'the shared run id placeholder', + 'f0b3d8c2-7e5a-4b9d-8c1f-6a4e2d0b9c58': 'executionId on the resume examples', + 'f1c3a7d0-4b52-4a8e-9f61-2d7c8b3e5a04': 'run output file id', } /** * All-zero and all-f sentinels read as synthetic on sight. * - * Takes an already-lowercased id: hex is case-insensitive, so counting `A` and `a` as two - * digits would inflate the distinct count and misjudge the texture. + * Unlike a texture score this admits no real id even in principle: fewer than three distinct hex + * digits across all 32 positions has probability on the order of 1e-27, so the family can be + * covered by shape rather than enumerated one nil-UUID variant at a time. + * + * Takes an already-lowercased id: hex is case-insensitive, so counting `A` and `a` as two digits + * would inflate the distinct count and admit ids this is meant to reject. */ function isSentinel(uuid: string): boolean { return new Set(uuid.replace(/-/g, '')).size <= 2 } -/** - * Hand-authored placeholders in this repo use every hex digit, none more than three times. - * - * Takes an already-lowercased id, for the reason given on {@link isSentinel}. - */ -function isHouseStyle(uuid: string): boolean { - const hex = uuid.replace(/-/g, '') - const digits = new Set(hex) - if (digits.size < 16) return false - return [...digits].every((digit) => hex.split(digit).length - 1 <= 3) -} - const specFiles = readdirSync(SPEC_DIR) .filter((file) => file.startsWith('openapi') && file.endsWith('.json')) .sort() @@ -58,19 +80,21 @@ for (const file of specFiles) { const seen = new Set(contents.match(UUID_PATTERN) ?? []) for (const uuid of [...seen].sort()) { const normalized = uuid.toLowerCase() - if (normalized in ALLOWED || isSentinel(normalized) || isHouseStyle(normalized)) continue + if (normalized in APPROVED || isSentinel(normalized)) continue findings.push(` - ${file}: ${uuid}`) } } if (findings.length > 0) { console.error( - `Published OpenAPI specs contain example UUIDs that look real rather than hand-authored:\n${findings.join('\n')}\n` + - 'Replace each with a placeholder using every hex digit at most three times, or add it to ALLOWED with a reason.' + `Published OpenAPI specs contain example UUIDs that are not approved placeholders:\n${findings.join('\n')}\n` + + 'Prefer reusing an id already in APPROVED in scripts/check-spec-example-ids.ts. If the example ' + + 'genuinely needs its own, hand-author one in the house style — every hex digit, none more than ' + + 'three times — then add it to APPROVED with what it stands for. Never paste an id from a real run.' ) process.exit(1) } console.log( - `Spec example ids passed: ${specFiles.length} published specs contain no real-looking UUIDs.` + `Spec example ids passed: ${specFiles.length} published specs publish only approved placeholders.` ) From ae06dc029ce7651aa182d726943df8fa8c710eb2 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 26 Aug 2026 17:47:38 -0700 Subject: [PATCH 5/5] fix(scripts): match the uuid sentinels exactly rather than by shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Accepting any id built from at most two distinct hex digits let something through that was never on the approved list. A generated id essentially never has that shape, so the practical risk was small — but this check had just stopped being a shape test and become a list, and a structural exception is the one thing that undoes that. The two ids it exists for are the nil and max sentinels, and both are matched by value now. --- scripts/check-spec-example-ids.ts | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/scripts/check-spec-example-ids.ts b/scripts/check-spec-example-ids.ts index a74463d87ee..08faa2f9e61 100644 --- a/scripts/check-spec-example-ids.ts +++ b/scripts/check-spec-example-ids.ts @@ -33,7 +33,7 @@ const UUID_PATTERN = /[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{1 /** * Every example UUID the specs are allowed to publish, with what it stands for. * - * Sentinels are covered structurally by {@link isSentinel} and are not listed here. + * The nil and max UUIDs are covered by {@link SENTINELS} and are not listed here. */ const APPROVED: Record = { '0f7c1a2e-9b3d-4c58-8a21-6d4e5f7a9b01': 'run id in the RUN_ID_CONFLICT error example', @@ -56,18 +56,17 @@ const APPROVED: Record = { } /** - * All-zero and all-f sentinels read as synthetic on sight. + * The two UUIDs RFC 9562 reserves as sentinels: nil and max. * - * Unlike a texture score this admits no real id even in principle: fewer than three distinct hex - * digits across all 32 positions has probability on the order of 1e-27, so the family can be - * covered by shape rather than enumerated one nil-UUID variant at a time. - * - * Takes an already-lowercased id: hex is case-insensitive, so counting `A` and `a` as two digits - * would inflate the distinct count and admit ids this is meant to reject. + * Matched exactly rather than by shape. An earlier version accepted any id built from at most + * two distinct hex digits, which is astronomically unlikely for a generated id but is still a + * bypass of the approved list — and this check exists to be a list, not a shape test. Both + * values are used by the resources spec today. */ -function isSentinel(uuid: string): boolean { - return new Set(uuid.replace(/-/g, '')).size <= 2 -} +const SENTINELS = new Set([ + '00000000-0000-0000-0000-000000000000', + 'ffffffff-ffff-ffff-ffff-ffffffffffff', +]) const specFiles = readdirSync(SPEC_DIR) .filter((file) => file.startsWith('openapi') && file.endsWith('.json')) @@ -80,7 +79,7 @@ for (const file of specFiles) { const seen = new Set(contents.match(UUID_PATTERN) ?? []) for (const uuid of [...seen].sort()) { const normalized = uuid.toLowerCase() - if (normalized in APPROVED || isSentinel(normalized)) continue + if (normalized in APPROVED || SENTINELS.has(normalized)) continue findings.push(` - ${file}: ${uuid}`) } }