mirror of
https://github.com/simstudioai/sim.git
synced 2026-10-30 19:32:46 +08:00
fix(cli): resolve blockers and majors from a full command-surface audit (#7083)
* fix(cli): resolve blockers and majors from a full command-surface audit Audit of all 222 CLI leaves against a live deployment, plus fixes for every defect it confirmed. Blockers: - An unrecognized --profile resolved to built-in defaults, so a typo silently targeted production and transmitted the API key there. - sim logs follow sent an undeclared query key and failed on every invocation. - sim workflows run exited 0 on a failed run, so CI reported success. - knowledge connectors documents update matched rows already in the target state, making exclude and restore permanent no-ops. - PDF text layers below the OCR threshold are transcribed by a model and stored verbatim with no record that it happened. Majors include: rollback --version was swallowed by the program-level flag and silently did nothing; nullable string flags could not send null despite their help promising it; six protocol commands discarded excess arguments, dropping files on upload; sim chat crashed with EPIPE when piped to head; tables import dropped malformed CSV rows without reporting them; audit-logs required an organization id no API surface exposed; secrets could not opt out of redaction or read a value from a file; bulk deletes and moves exited 0 having done nothing; and MCP registrations were destroyed by undeploy rather than restored. Adds extraction_method to documents so OCR output is distinguishable from parsed text, and reports the applied scope on billing logs so the two ledger questions are no longer indistinguishable. * fix(mcp): bound MCP restore by server and re-check uniqueness under the lock Two gaps in the archive/restore lifecycle this branch introduced. The candidate query bounded archived rows and deduplicated to one per server afterwards, so several archived generations stacked on one server consumed the whole budget and every other server the workflow had been published on fell out of the result with no warning. Deduplication moves into SQL so the bound applies to servers, preserving most-recently-updated-per-server. The live-registration check ran before the server lock was acquired, so a concurrent tool create could land in between and the restore would un-archive a second live row for the same server and workflow, violating the partial unique index and rolling back the whole deployment. That check now runs under the lock alongside the tool-name, capacity, and metadata-budget checks it belongs with. * fix(review): address round-three review findings across CLI and server Restore now picks candidate servers by recency: DISTINCT ON forces its own key to lead the sort, so bounding on that statement kept the lexicographically lowest server ids and left a workflow's most recently used servers archived. Deduplication and bounding are now separate stages. CLI: a total miss on tables move reported only in notFound exited 0; unsetting a key or removing a profile mutated the first duplicate INI block while reads merged later ones, so the removal appeared to succeed and did nothing; an import that rejected cells but no rows showed a clean progress line; and the --run-id help implied idempotency it does not provide. Server: a run with no recorded output projection let block-name selectors past the new validation; the billing window comparison still fired on a bound that parsed but failed the shared schema; CSV rejection accounting reached only the streaming path, so buffered and synchronous imports still dropped records silently, through to the Copilot tool that reports them; case-insensitive tag name uniqueness now serializes on the knowledge-base row the delete paths already lock; and a failed sync claim reports the lifecycle reason rather than always claiming a sync is in progress. Reverts an over-scrub from the previous commit: workspace-file-imports is consumed only by Copilot, so naming save_upload and glob there is the correct remediation rather than a leak, and the sweep that guards against leaks now exempts it explicitly. Corrects two contract descriptions that promised a bulk tag save would rename or relocate an occupied slot, which it deliberately no longer does. * fix(cli): remove flags a caller cannot use, and correct three that misled Removes surface that should not have shipped: - `files uploads get` is hidden. Its `--upload-token` was required, and the token is minted and consumed inside a single `files upload`, which completes or aborts its session before returning. Nothing in the CLI could produce the value, so the command answered every invocation by asking for something unobtainable. The same flag is dropped from the two table-import commands, where a CLI-created import is already queryable without it. - The `--no-<flag>` companion that sent JSON null is gone. `--no-X` means "send boolean false" on thirty-seven other flags, and one spelling should not carry two meanings. `--description ''` already clears the displayed value, and the help now warns that the literal word null is stored as text rather than suggesting a substitute, because on the OAuth client fields null revokes a stored grant and an empty string does not. - The document extraction method column and its contract field are reverted. Nothing read them, they were null for every existing document, and the name collided with the parser metadata field that already exists. Corrects flags that misled: the workflow move destination is `--to`, matching its two siblings rather than meaning the opposite of `--folder` one command over; `files list --recursive` is a bare flag like the four folder deletes rather than a twelve-alias string; the dispatch row cap takes a count instead of its wire object; `--yes` no longer claims to be required on commands that accept `--dry-run`; cancelling every run on a table is confirm-gated; and the retry-processing negation, which the route rejects, is suppressed. Extends the guard that missed all of this: it swept only `--x-` prefixes over generated commands, so a header spelled without one was invisible to it. It now derives every header name from the operation table and sweeps the assembled program. * fix(mcp): budget MCP restore against the workflow's live server fanout Restore bounded its candidates at the per-workflow server limit counting archived rows only, never subtracting the memberships the workflow already holds live. The fanout validation a few lines later in the same deploy transaction counts live servers against that same limit, so a workflow with both live and archived registrations could restore past it and roll the whole deployment back. The candidate query now bounds on the remaining headroom, and the budget is re-checked under the server locks and spent once per accepted candidate, so a create that lands between the count and the unarchive cannot push it over. Candidates that do not fit are dropped by recency, matching how the set is already selected, and stay archived with a warning naming the workflow, the server, and the reason — restore still never throws inside the deploy transaction. One residual is left open deliberately: a create on a server outside the candidate set is serialized by neither the locks nor the recount. Closing it would need a workflow-level lock, which would change the ordering every other writer here depends on, and the create path runs its own limit check. * fix(mcp): stop restore spending its budget on servers it cannot restore A server can hold both a live registration and archived ones for the same workflow: the partial unique index constrains only the live row. Such a server was counted twice — once shrinking the restore budget, once consuming one of its slots — before the liveness check under the lock skipped it. While the bound was the full server limit that waste was invisible; once the bound became the remaining headroom, every slot spent that way cost a registration that could have been restored. The candidate query now excludes servers the workflow is already live on, so the budget and the candidate set agree. The exclusion sits on the inner stage, before deduplication and the bound, and is a pre-lock optimisation only: the check under the server lock stays authoritative, because the query can go stale between reading and unarchiving. Candidates rejected for a tool-name collision, the per-server cap, or the metadata budget are still not replaced. That case is only knowable under the lock, so replacing it would mean fetching past the bound and locking servers outside the candidate set, widening an ordering every writer here relies on.
This commit is contained in:
@@ -0,0 +1,53 @@
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import { z } from 'zod'
|
||||
import { CLI_MANAGED_HEADERS, renderSlotMap } from './generate-v2-cli-api'
|
||||
|
||||
describe('a field the contract types as nullable', () => {
|
||||
/**
|
||||
* The operation table describes what the CLI can build a flag from, and a flag
|
||||
* that sends JSON `null` is not one of them: `--no-<flag>` already means "send
|
||||
* this boolean as false" on 37 flags, and `--description ''` is how a string
|
||||
* is emptied. Emitting the nullability invited a second meaning for one
|
||||
* spelling, so it is no longer carried.
|
||||
*/
|
||||
it('describes it no differently from any other string', () => {
|
||||
const map = renderSlotMap(
|
||||
z.object({ description: z.string().nullable().optional().describe('Replacement.') }),
|
||||
' '
|
||||
)
|
||||
expect(map).toContain("kind: 'string'")
|
||||
expect(map).not.toContain('nullable')
|
||||
})
|
||||
})
|
||||
|
||||
describe('request headers reaching the CLI as flags', () => {
|
||||
/**
|
||||
* `getFileUpload` reads its session through an `upload-token` header, and the
|
||||
* operation table listed only its params and query — so the runtime had no
|
||||
* field to build a flag from and every call was rejected as invalid input
|
||||
* before it left the machine.
|
||||
*/
|
||||
it('describes a contract header the caller has to supply', () => {
|
||||
const map = renderSlotMap(
|
||||
z.object({ 'upload-token': z.string().describe('Signed upload control token.') }),
|
||||
' ',
|
||||
CLI_MANAGED_HEADERS
|
||||
)
|
||||
expect(map).toContain('"upload-token"')
|
||||
expect(map).toContain('required: true')
|
||||
})
|
||||
|
||||
/**
|
||||
* The client spreads contract headers last over its own block, so a flag for
|
||||
* one of these would let argv replace the profile's credential.
|
||||
*/
|
||||
it('leaves out a header the CLI sets for itself', () => {
|
||||
const map = renderSlotMap(
|
||||
z.object({ 'x-api-key': z.string(), 'upload-token': z.string() }),
|
||||
' ',
|
||||
CLI_MANAGED_HEADERS
|
||||
)
|
||||
expect(map).not.toContain('x-api-key')
|
||||
expect(map).toContain('upload-token')
|
||||
})
|
||||
})
|
||||
@@ -29,9 +29,10 @@
|
||||
import { spawnSync } from 'node:child_process'
|
||||
import { readdirSync, readFileSync, writeFileSync } from 'node:fs'
|
||||
import path from 'node:path'
|
||||
import { fileURLToPath } from 'node:url'
|
||||
import { z } from 'zod'
|
||||
|
||||
const ROOT = path.resolve(import.meta.dir, '..')
|
||||
const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..')
|
||||
const CONTRACTS_DIR = path.join(ROOT, 'apps/sim/lib/api/contracts/v2')
|
||||
const OUTPUT = path.join(ROOT, 'packages/sim-cli/src/generated/v2-api.ts')
|
||||
const DOCS_DIR = path.join(ROOT, 'apps/docs')
|
||||
@@ -378,7 +379,28 @@ function isUnionSlot(schema: z.ZodType): boolean {
|
||||
return Object.keys(json.properties ?? {}).length === 0 && Boolean(json.anyOf ?? json.oneOf)
|
||||
}
|
||||
|
||||
function renderSlotMap(schema: z.ZodType | undefined, indent: string): string | null {
|
||||
/**
|
||||
* Headers the CLI sets itself, which must never become flags.
|
||||
*
|
||||
* `options.headers` is spread last over the client's own header block, so a
|
||||
* flag spelled `--x-api-key` would let argv replace the profile's credential —
|
||||
* a footgun on every command that carries it, and an authentication decision
|
||||
* argv has no business making. No contract declares one of these today; the
|
||||
* exclusion exists so that adding one does not quietly grow a flag for it.
|
||||
*/
|
||||
export const CLI_MANAGED_HEADERS: ReadonlySet<string> = new Set([
|
||||
'x-api-key',
|
||||
'authorization',
|
||||
'accept',
|
||||
'content-type',
|
||||
'user-agent',
|
||||
])
|
||||
|
||||
export function renderSlotMap(
|
||||
schema: z.ZodType | undefined,
|
||||
indent: string,
|
||||
exclude?: ReadonlySet<string>
|
||||
): string | null {
|
||||
if (!schema) return null
|
||||
|
||||
const json = z.toJSONSchema(schema, { io: 'input', unrepresentable: 'any' }) as JsonSchema
|
||||
@@ -401,7 +423,7 @@ function renderSlotMap(schema: z.ZodType | undefined, indent: string): string |
|
||||
}
|
||||
}
|
||||
|
||||
const keys = Object.keys(properties)
|
||||
const keys = Object.keys(properties).filter((key) => !exclude?.has(key))
|
||||
|
||||
// A union body (e.g. single-row vs batch insert) has no flat field list. The
|
||||
// caller marks it `opaqueBody` so the runtime can offer the whole body as one
|
||||
@@ -495,11 +517,12 @@ function render(operations: Operation[]): string {
|
||||
out.push('/**')
|
||||
out.push(' * Every v2 operation, keyed by name.')
|
||||
out.push(' *')
|
||||
out.push(' * `query` and `body` describe each field well enough for the CLI to build a')
|
||||
out.push(' * flag for it and coerce the string argv gives back: its kind, whether it is')
|
||||
out.push(' * required, its enum values, and its server-side default. A slot the contract')
|
||||
out.push(' * does not declare — or one whose shape is a union with no flat field list —')
|
||||
out.push(' * is absent, and the runtime falls back to taking it as JSON.')
|
||||
out.push(' * `query`, `body`, and `headers` describe each field well enough for the CLI')
|
||||
out.push(' * to build a flag for it and coerce the string argv gives back: its kind,')
|
||||
out.push(' * whether it is required, its enum values, and its server-side default. A slot')
|
||||
out.push(' * the contract does not declare — or one whose shape is a union with no flat')
|
||||
out.push(' * field list — is absent, and the runtime falls back to taking it as JSON.')
|
||||
out.push(' * Headers the CLI sets itself, such as the API key, are never listed.')
|
||||
out.push(' *')
|
||||
out.push(" * `summary` is the operation's one-line description, lifted from the OpenAPI")
|
||||
out.push(' * specs so `--help` reuses prose that is already written and already checked.')
|
||||
@@ -536,6 +559,12 @@ function render(operations: Operation[]): string {
|
||||
out.push(` opaqueBody: true,`)
|
||||
}
|
||||
}
|
||||
// Contract headers are request input like any other slot: `upload-token`
|
||||
// is what addresses an upload session, and leaving it out of this table
|
||||
// meant the runtime could not build a flag for it, so `files uploads get`
|
||||
// was rejected as invalid input on every call it could ever make.
|
||||
const headers = renderSlotMap(op.contract.headers, ' ', CLI_MANAGED_HEADERS)
|
||||
if (headers) out.push(` headers: ${headers},`)
|
||||
out.push(' },')
|
||||
}
|
||||
out.push('} as const')
|
||||
@@ -606,4 +635,6 @@ async function main() {
|
||||
)
|
||||
}
|
||||
|
||||
main()
|
||||
// Guarded so the pure helpers above can be imported by tests without the
|
||||
// generator writing a file as a side effect of the import.
|
||||
if (import.meta.main) main()
|
||||
|
||||
Reference in New Issue
Block a user