From e25cc961343c78485e9c37225b50d87f31fb54f2 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 3 Nov 2026 16:49:01 -0800 Subject: [PATCH] improvement(cleanup): tighten the agent-harness changes from the latest audit - CLI parse-error guidance branches on commander's error codes, reads hidden siblings through Help.visibleCommands, and names a misplaced positional only when unambiguous (no unknown option, one missing required flag, one extra operand); the hint uses the option's long flag, not '-w,' - body-variant shape errors use isRecordLike, compute the shape once, and show a generic example - workflow lint builds sections eagerly, derives hasWorkflowLintIssues from them, and phrases Run-button advice from the issue's fields - shared quotedBlockLabel; formatQuotedNameList takes a quote formatter - grep marks skipped non-text files instead of threading a callback - drop dead run-from-block loop branch and unreachable run-hook guard - shared empty lint report test factory; tighter assertions --- .../[workflowId]/deploy/route.test.ts | 12 +- .../[workflowId]/operations/route.test.ts | 16 +- .../[workflowId]/state/route.test.ts | 15 +- .../hooks/use-workflow-execution.ts | 16 +- .../sim/executor/utils/run-from-block.test.ts | 1 + apps/sim/executor/utils/run-from-block.ts | 2 +- .../api/contracts/v2/workflow-graph.test.ts | 16 +- .../agent-cli/engines/lint-scope.test.ts | 15 +- .../agent-cli/engines/universal-grep.ts | 30 ++- .../mothership/agent-cli/project-file-grep.ts | 6 +- .../apply-workflow-operations.test.ts | 10 +- .../workflows/application/deployments.test.ts | 15 +- .../run-workflow-from-copilot.test.ts | 4 +- .../application/run-workflow-from-copilot.ts | 14 +- .../lib/workflows/editing/lint-report.test.ts | 6 +- apps/sim/lib/workflows/editing/lint.test.ts | 7 +- apps/sim/lib/workflows/editing/lint.ts | 193 +++++++++--------- apps/sim/lib/workflows/labels.ts | 17 +- .../workspace-files/delivery.integration.ts | 13 +- packages/sim-cli/src/embed-isolation.test.ts | 2 +- packages/sim-cli/src/runtime/build.test.ts | 42 +++- packages/sim-cli/src/runtime/build.ts | 155 +++++++------- packages/sim-cli/src/runtime/request.ts | 53 ++--- packages/testing/src/factories/index.ts | 2 + .../src/factories/workflow-lint.factory.ts | 28 +++ packages/utils/src/string.ts | 14 +- 26 files changed, 360 insertions(+), 344 deletions(-) create mode 100644 packages/testing/src/factories/workflow-lint.factory.ts diff --git a/apps/sim/app/api/v2/workflows/[workflowId]/deploy/route.test.ts b/apps/sim/app/api/v2/workflows/[workflowId]/deploy/route.test.ts index 2f1491338e7..fab97dca15f 100644 --- a/apps/sim/app/api/v2/workflows/[workflowId]/deploy/route.test.ts +++ b/apps/sim/app/api/v2/workflows/[workflowId]/deploy/route.test.ts @@ -6,6 +6,7 @@ import { v2RouteMocks, } from '@sim/testing' import { createPersonalApiKeyPrincipal } from '@sim/testing/factories/principal.factory' +import { createEmptyWorkflowLintReport } from '@sim/testing/factories/workflow-lint.factory' import { createRouteContext } from '@sim/testing/helpers/http' import { auditMock } from '@sim/testing/mocks/audit.mock' import { createMockRequest } from '@sim/testing/mocks/request.mock' @@ -43,13 +44,7 @@ const sideEffectWarning = 'Deployment activation completed, and post-activation notifications are queued.' const unquotedRowJson = { - sources: [], - sinks: [], - orphanBlocks: [], - emptyOutgoingPorts: [], - invalidBranchPorts: [], - invalidConnectionTargets: [], - fieldIssues: [], + ...createEmptyWorkflowLintReport(), unresolvedReferences: [ { blockId: 'insert', @@ -60,9 +55,6 @@ const unquotedRowJson = { reason: 'unquoted-json-string: quote it', }, ], - tableFieldIssues: [], - manualRunIssue: null, - notes: [], } describe('POST /api/v2/workflows/[workflowId]/deploy', () => { diff --git a/apps/sim/app/api/v2/workflows/[workflowId]/operations/route.test.ts b/apps/sim/app/api/v2/workflows/[workflowId]/operations/route.test.ts index c406e0b6a96..f5c47cb69e9 100644 --- a/apps/sim/app/api/v2/workflows/[workflowId]/operations/route.test.ts +++ b/apps/sim/app/api/v2/workflows/[workflowId]/operations/route.test.ts @@ -5,6 +5,7 @@ import { v2RateLimiterModuleMock, v2RouteMocks, } from '@sim/testing' +import { createEmptyWorkflowLintReport } from '@sim/testing/factories/workflow-lint.factory' import { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' @@ -41,20 +42,7 @@ const DROPPED_INPUT = { error: 'Invalid credential ID', } -/** An empty report, with every field the contract publishes. */ -const LINT = { - sources: [], - sinks: [], - orphanBlocks: [], - emptyOutgoingPorts: [], - invalidBranchPorts: [], - invalidConnectionTargets: [], - fieldIssues: [], - unresolvedReferences: [], - tableFieldIssues: [], - manualRunIssue: null, - notes: [], -} +const LINT = createEmptyWorkflowLintReport() const auth = { principal: { kind: 'personal_api_key' as const, userId: 'user-1', keyId: 'personal-key-1' }, diff --git a/apps/sim/app/api/v2/workflows/[workflowId]/state/route.test.ts b/apps/sim/app/api/v2/workflows/[workflowId]/state/route.test.ts index b47cf8a1f32..d4c1f6b9a3d 100644 --- a/apps/sim/app/api/v2/workflows/[workflowId]/state/route.test.ts +++ b/apps/sim/app/api/v2/workflows/[workflowId]/state/route.test.ts @@ -5,6 +5,7 @@ import { v2RateLimiterModuleMock, v2RouteMocks, } from '@sim/testing' +import { createEmptyWorkflowLintReport } from '@sim/testing/factories/workflow-lint.factory' import { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' @@ -63,19 +64,7 @@ function putRequest(body: unknown) { }) } -const EMPTY_LINT = { - sources: [], - sinks: [], - orphanBlocks: [], - emptyOutgoingPorts: [], - invalidBranchPorts: [], - invalidConnectionTargets: [], - fieldIssues: [], - unresolvedReferences: [], - tableFieldIssues: [], - manualRunIssue: null, - notes: [], -} +const EMPTY_LINT = createEmptyWorkflowLintReport() describe('/api/v2/workflows/[workflowId]/state', () => { beforeEach(() => { diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-workflow-execution.ts b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-workflow-execution.ts index 963806756d6..d28c0c9354f 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-workflow-execution.ts +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-workflow-execution.ts @@ -1076,7 +1076,7 @@ export function useWorkflowExecution() { selectedOutputs = chatStore.getState().getSelectedWorkflowOutput(activeWorkflowId) } - let startBlockId: string | undefined + let startBlockId: string let finalWorkflowInput = workflowInput if (isExecutingFromChat) { @@ -1128,18 +1128,6 @@ export function useWorkflowExecution() { } } - if (!startBlockId) { - const error = new WorkflowValidationError( - 'No valid trigger block found to start execution', - 'validation', - 'validation', - 'Workflow Validation' - ) - logger.error('No startBlockId found after trigger search') - if (activeWorkflowId) finishOwnedExecution(activeWorkflowId, persistenceExecution) - throw error - } - logger.info('Final execution setup:', { startBlockId, isExecutingFromChat, @@ -1316,7 +1304,7 @@ export function useWorkflowExecution() { } // Add trigger block to executed blocks so downstream blocks can use run-from-block - if (data.success && startBlockId) { + if (data.success) { executedBlockIds.add(startBlockId) } diff --git a/apps/sim/executor/utils/run-from-block.test.ts b/apps/sim/executor/utils/run-from-block.test.ts index 3de2d7de8e3..cdabe5ec2ed 100644 --- a/apps/sim/executor/utils/run-from-block.test.ts +++ b/apps/sim/executor/utils/run-from-block.test.ts @@ -200,6 +200,7 @@ describe('validateRunFromBlock', () => { subflowType: 'loop', }), ]) + dag.loopConfigs.set('loop-1', { id: 'loop-1', nodes: ['A'], iterations: 2, loopType: 'for' }) const executedBlocks = new Set(['A']) const result = validateRunFromBlock('A', dag, executedBlocks) diff --git a/apps/sim/executor/utils/run-from-block.ts b/apps/sim/executor/utils/run-from-block.ts index 3fbfd20d998..1f7e18eb3c0 100644 --- a/apps/sim/executor/utils/run-from-block.ts +++ b/apps/sim/executor/utils/run-from-block.ts @@ -230,7 +230,7 @@ export function validateRunFromBlock( } if (node) { - if (node.metadata.isLoopNode) return insideSubflow(node.metadata.subflowId, 'loop') + // A branch-clone id (`block₍0₎`) is in no subflow config, so findOutermostContainer misses it. if (node.metadata.isParallelBranch) return insideSubflow(node.metadata.subflowId, 'parallel') if (node.metadata.isSentinel) { diff --git a/apps/sim/lib/api/contracts/v2/workflow-graph.test.ts b/apps/sim/lib/api/contracts/v2/workflow-graph.test.ts index 19cb2abef67..835567a0ca3 100644 --- a/apps/sim/lib/api/contracts/v2/workflow-graph.test.ts +++ b/apps/sim/lib/api/contracts/v2/workflow-graph.test.ts @@ -1,3 +1,4 @@ +import { createEmptyWorkflowLintReport } from '@sim/testing/factories/workflow-lint.factory' import { describe, expect, it } from 'vitest' import { v2ApplyWorkflowOperationsDataSchema, @@ -92,20 +93,7 @@ describe('v2WorkflowGraphSchema', () => { }) }) -/** An empty report carrying every field the lint schema requires. */ -const EMPTY_LINT = { - sources: [], - sinks: [], - orphanBlocks: [], - emptyOutgoingPorts: [], - invalidBranchPorts: [], - invalidConnectionTargets: [], - fieldIssues: [], - unresolvedReferences: [], - tableFieldIssues: [], - manualRunIssue: null, - notes: [], -} +const EMPTY_LINT = createEmptyWorkflowLintReport() /** A report exercising every finding kind the lint schema publishes. */ const FULL_LINT = { diff --git a/apps/sim/lib/mothership/agent-cli/engines/lint-scope.test.ts b/apps/sim/lib/mothership/agent-cli/engines/lint-scope.test.ts index 1635d443774..3e8b9387adc 100644 --- a/apps/sim/lib/mothership/agent-cli/engines/lint-scope.test.ts +++ b/apps/sim/lib/mothership/agent-cli/engines/lint-scope.test.ts @@ -3,6 +3,7 @@ import { createSessionPrincipal, createWorkspaceApiKeyPrincipal, } from '@sim/testing/factories/principal.factory' +import { createEmptyWorkflowLintReport } from '@sim/testing/factories/workflow-lint.factory' import { secretsUseCasesMock, secretsUseCasesMockFns, @@ -82,19 +83,7 @@ describe('Mothership workflow lint scope', () => { }) mocks.permission.mockResolvedValue('read') mocks.snapshot.mockResolvedValue({ workflowRecord: { id: 'wf-1' }, normalizedData: graph }) - mocks.report.mockResolvedValue({ - sources: [], - sinks: [], - orphanBlocks: [], - emptyOutgoingPorts: [], - invalidBranchPorts: [], - invalidConnectionTargets: [], - fieldIssues: [], - unresolvedReferences: [], - tableFieldIssues: [], - manualRunIssue: null, - notes: [], - }) + mocks.report.mockResolvedValue(createEmptyWorkflowLintReport()) mocks.secrets.mockResolvedValue({ secrets: [{ envKey: 'WORKFLOW_KEY' }], nextCursorKeys: null }) }) diff --git a/apps/sim/lib/mothership/agent-cli/engines/universal-grep.ts b/apps/sim/lib/mothership/agent-cli/engines/universal-grep.ts index 9fd8a5e35d8..a8b80340443 100644 --- a/apps/sim/lib/mothership/agent-cli/engines/universal-grep.ts +++ b/apps/sim/lib/mothership/agent-cli/engines/universal-grep.ts @@ -61,7 +61,6 @@ const PLATFORM_SCOPES: ReadonlySet = new Set(['blocks', 'tools']) interface GrepRuntime extends AgentCliRuntime { reportIncomplete(message: string): void - reportNonText(): void } /** @@ -121,6 +120,11 @@ export interface GrepMaterialized { label: string id: string text: string | null + /** + * Skipped unread because it has no text to extract: a note, not an incomplete search. + * Still returned, so `--in` naming it is not an unknown selector. + */ + nonText?: true } interface Page { @@ -389,13 +393,11 @@ async function fetchAll( if (scope !== 'files') { return mapConcurrent(entries, FETCH_CONCURRENCY, (item) => FETCHERS[scope](runtime, item)) } - // Skipped files keep their identity, so `--in` naming one is not an unknown selector. const skipped: GrepMaterialized[] = [] const readable: IndexEntry[] = [] for (const item of entries) { if (lacksText(item.raw)) { - runtime.reportNonText() - skipped.push({ scope, id: item.id, label: item.label, text: null }) + skipped.push({ scope, id: item.id, label: item.label, text: null, nonText: true }) } else readable.push(item) } if (readable.length > MAX_GREP_FILES) { @@ -589,15 +591,10 @@ export interface GrepSource { registry?: AgentCliRuntime['resolvedSecretTraceRegistry'] /** The only scopes this corpus holds, and the refusal for a search that names another. */ scopeLimit?: { scopes: readonly Scope[]; refusal: string } - /** - * `reportNonText` counts a file skipped unread because it has no text to extract: a note, - * not an incomplete search. The skipped file is still returned with null text. - */ materialize( scopes: Scope[], nameFilter: string | undefined, - reportIncomplete: (message: string) => void, - reportNonText: () => void + reportIncomplete: (message: string) => void ): Promise } @@ -652,10 +649,9 @@ export async function executeGrep(positionals: string[], flags: AgentCliFlags, s issueCount++ if (issues.length < 10) issues.push(clip(message)) } - let nonTextCount = 0 - const reportNonText = () => { - nonTextCount++ - } + const candidates = await source.materialize(searched, nameFilter, reportIncomplete) + source.signal?.throwIfAborted() + const nonTextCount = candidates.filter((candidate) => candidate.nonText).length const finish = (stdout: string) => { const skipped = nonTextCount ? `Skipped ${nonTextCount} file${nonTextCount === 1 ? '' : 's'} with no searchable text (images, media, archives, and other binary formats).` @@ -669,8 +665,6 @@ export async function executeGrep(positionals: string[], flags: AgentCliFlags, s stderr: [incomplete, skipped].filter(Boolean).join('\n'), } } - const candidates = await source.materialize(searched, nameFilter, reportIncomplete, reportNonText) - source.signal?.throwIfAborted() /** * A resource nothing in the searched worlds answers to is a wrong selector, not a * search with no hits — a silent "No matches" would hide the misspelling. @@ -758,8 +752,8 @@ export const universalGrepCommand: AgentCliEngine = { executeGrep(positionals, flags, { signal: runtime.signal, registry: runtime.resolvedSecretTraceRegistry, - async materialize(scopes, nameFilter, reportIncomplete, reportNonText) { - const bounded = gatedRuntime({ ...runtime, reportIncomplete, reportNonText }) + async materialize(scopes, nameFilter, reportIncomplete) { + const bounded = gatedRuntime({ ...runtime, reportIncomplete }) return nameFilter ? materializeWithin(bounded, scopes, nameFilter) : (await settleAll(scopes.map((scope) => materializeScope(bounded, scope)))).flat() diff --git a/apps/sim/lib/mothership/agent-cli/project-file-grep.ts b/apps/sim/lib/mothership/agent-cli/project-file-grep.ts index f1241512921..d839cce6913 100644 --- a/apps/sim/lib/mothership/agent-cli/project-file-grep.ts +++ b/apps/sim/lib/mothership/agent-cli/project-file-grep.ts @@ -35,8 +35,7 @@ export function grepProjectFiles( scopes: ['files'], refusal: 'Project grep supports only --scope files. Search workspace resources separately.', }, - async materialize(_scopes, nameFilter, reportIncomplete, reportNonText) { - // Kept so `--in` naming an unread non-text file is not an unknown selector. + async materialize(_scopes, nameFilter, reportIncomplete) { const skipped: GrepMaterialized[] = [] const selectedFiles: { item: Awaited>['files'][number] @@ -72,8 +71,7 @@ export function grepProjectFiles( type: observedMediaType(item.type, item.name), }) ) { - reportNonText() - skipped.push({ scope: 'files', id: item.id, label, text: null }) + skipped.push({ scope: 'files', id: item.id, label, text: null, nonText: true }) continue } if (selectedFiles.length >= MAX_GREP_FILES) { diff --git a/apps/sim/lib/workflows/application/apply-workflow-operations.test.ts b/apps/sim/lib/workflows/application/apply-workflow-operations.test.ts index 533f212bcbd..300a86de93a 100644 --- a/apps/sim/lib/workflows/application/apply-workflow-operations.test.ts +++ b/apps/sim/lib/workflows/application/apply-workflow-operations.test.ts @@ -266,8 +266,8 @@ describe('applyWorkflowOperations', () => { }) /** - * An agent piping the apply response through a projection of `warnings` - * never saw lint findings, which lived only under `lint`. + * A caller projecting the response down to `warnings` still learns lint + * findings exist; the findings stay in `lint`. */ it('points at lint findings from warnings, on commit and on dry run', async () => { const orphan = { blockId: 'block-1', blockName: 'Start', blockType: 'starter' } @@ -281,9 +281,9 @@ describe('applyWorkflowOperations', () => { principal: sessionPrincipal, input: { workflowId: 'workflow-1', operations, dryRun }, }) - expect(result.warnings).toEqual([ - 'Workflow lint found issues (Blocks with no incoming edge: 2). The findings are in `lint`.', - ]) + expect(result.warnings).toHaveLength(1) + expect(result.warnings[0]).toContain('Blocks with no incoming edge: 2') + expect(result.warnings[0]).toContain('in `lint`') } }) diff --git a/apps/sim/lib/workflows/application/deployments.test.ts b/apps/sim/lib/workflows/application/deployments.test.ts index b1c46c44bfc..4043bd43727 100644 --- a/apps/sim/lib/workflows/application/deployments.test.ts +++ b/apps/sim/lib/workflows/application/deployments.test.ts @@ -5,6 +5,7 @@ import { createSessionPrincipal, createWorkspaceApiKeyPrincipal, } from '@sim/testing/factories/principal.factory' +import { createEmptyWorkflowLintReport } from '@sim/testing/factories/workflow-lint.factory' import { auditMock, auditMockFns } from '@sim/testing/mocks/audit.mock' import { realtimeNotifyMock } from '@sim/testing/mocks/realtime-notify.mock' import { workflowAuthzMockFns } from '@sim/testing/mocks/workflow-authz.mock' @@ -83,19 +84,7 @@ const context = { billedAccountUserId: 'billing-owner-1', } -const cleanLint = { - sources: [], - sinks: [], - orphanBlocks: [], - emptyOutgoingPorts: [], - invalidBranchPorts: [], - invalidConnectionTargets: [], - fieldIssues: [], - unresolvedReferences: [], - tableFieldIssues: [], - manualRunIssue: null, - notes: [], -} +const cleanLint = createEmptyWorkflowLintReport() const adminPrincipals: Array<{ principal: Principal; actorUserId: string }> = [ { diff --git a/apps/sim/lib/workflows/application/run-workflow-from-copilot.test.ts b/apps/sim/lib/workflows/application/run-workflow-from-copilot.test.ts index cf3d8d8738a..632418ab223 100644 --- a/apps/sim/lib/workflows/application/run-workflow-from-copilot.test.ts +++ b/apps/sim/lib/workflows/application/run-workflow-from-copilot.test.ts @@ -362,7 +362,7 @@ describe('Copilot workflow run application commands', () => { expect(message).toContain('execution source-execution-1') expect(message).toContain('run_workflow_until_block') expect(message).toContain('stopAfterBlockId "agent-1"') - expect(message).toContain('executionId') + expect(message).toContain('or pass the executionId of a run that executed "Fetch" (fetch-1)') }) it('names the container a nested block sits in and points at it', async () => { @@ -378,7 +378,7 @@ describe('Copilot workflow run application commands', () => { expect(message).toContain('inside loop "Each row" (loop-1)') expect(message).toContain('block id "loop-1"') - expect(message).toContain('run_workflow') + expect(message).toContain('or run the whole workflow with run_workflow.') }) it('lists the blocks the workflow does have when the id is unknown', async () => { diff --git a/apps/sim/lib/workflows/application/run-workflow-from-copilot.ts b/apps/sim/lib/workflows/application/run-workflow-from-copilot.ts index ee466383500..e6dc24341a5 100644 --- a/apps/sim/lib/workflows/application/run-workflow-from-copilot.ts +++ b/apps/sim/lib/workflows/application/run-workflow-from-copilot.ts @@ -2,6 +2,7 @@ import { type Principal, requirePrincipalSubjectUserId } from '@sim/auth/princip import { createLogger } from '@sim/logger' import { toError } from '@sim/utils/errors' import { generateId } from '@sim/utils/id' +import { formatQuotedNameList } from '@sim/utils/string' import { mergeSubblockStateWithValues } from '@sim/workflow-persistence/subblocks' import type { BlockState } from '@sim/workflow-types/workflow' import { releaseExecutionSlot } from '@/lib/billing/calculations/usage-reservation' @@ -19,6 +20,7 @@ import { getExecutionStateForWorkflow, getLatestExecutionStateWithExecutionId, } from '@/lib/workflows/executor/execution-state' +import { quotedBlockLabel } from '@/lib/workflows/labels' import { loadDeployedWorkflowState, loadWorkflowFromNormalizedTables, @@ -261,10 +263,7 @@ function describeRunFromBlockRefusal( ): string { const refusal = error.refusal if (!refusal) return error.message - const label = (id: string) => { - const name = blocks[id]?.name - return name ? `"${name}" (${id})` : `"${id}"` - } + const label = (id: string) => quotedBlockLabel(id, blocks[id]?.name, { withId: true }) switch (refusal.kind) { case 'upstream_not_executed': { @@ -291,11 +290,8 @@ function describeRunFromBlockRefusal( if (existing) { return `Block ${label(refusal.blockId)} is not an executable block, so it cannot be run.` } - const all = Object.keys(blocks) - const listed = all.slice(0, MAX_LISTED_BLOCKS).map(label).join(', ') - const more = - all.length > MAX_LISTED_BLOCKS ? ` and ${all.length - MAX_LISTED_BLOCKS} more` : '' - return `Block "${refusal.blockId}" is not in the ${state} workflow. Its blocks are: ${listed}${more}.` + const listed = formatQuotedNameList(Object.keys(blocks), MAX_LISTED_BLOCKS, label) + return `Block "${refusal.blockId}" is not in the ${state} workflow. Its blocks are: ${listed}.` } } } diff --git a/apps/sim/lib/workflows/editing/lint-report.test.ts b/apps/sim/lib/workflows/editing/lint-report.test.ts index e090204634e..9f40f16f57c 100644 --- a/apps/sim/lib/workflows/editing/lint-report.test.ts +++ b/apps/sim/lib/workflows/editing/lint-report.test.ts @@ -143,11 +143,7 @@ describe('buildWorkflowLintReport notes', () => { }) }) -/** - * The editor's Run button refuses some graphs before they start. Lint said - * nothing about them, so an agent-built workflow looked clean until a user - * pressed Run. - */ +/** Graphs the editor's Run button refuses before they start surface as `manualRunIssue`. */ describe('buildWorkflowLintReport manual run', () => { async function manualRunIssue(blocks: Record, edges: unknown[]) { return (await buildWorkflowLintReport({ blocks, edges } as never, scope)).manualRunIssue diff --git a/apps/sim/lib/workflows/editing/lint.test.ts b/apps/sim/lib/workflows/editing/lint.test.ts index 084c59e068e..57daec10c39 100644 --- a/apps/sim/lib/workflows/editing/lint.test.ts +++ b/apps/sim/lib/workflows/editing/lint.test.ts @@ -540,8 +540,7 @@ describe('collectWorkflowTableIds', () => { /** * The agent's own runs name their trigger and skip the Run button's checks, so - * a Run-button refusal told under "Fix these before continuing" pushed agents - * to wire a stray Start into the graph instead of removing it. + * a Run-button refusal is advice, never listed under "Fix these before continuing". */ describe('formatWorkflowLintMessage', () => { const clean = { @@ -564,7 +563,7 @@ describe('formatWorkflowLintMessage', () => { const message = formatWorkflowLintMessage({ ...clean, manualRunIssue: strayStart }) expect(message).not.toContain('Fix these before continuing') expect(message).toBe( - 'The editor\'s Run button would fail: Start must be connected to other blocks to execute. Run starts from "Start" (start_trigger) ahead of any other trigger: connect it, or delete it if the workflow starts from another trigger. Runs that name their trigger are not affected.' + 'The editor\'s Run button would fail: Run starts from "Start" (start_trigger) ahead of any other trigger, and it is not connected to other blocks. Connect it, or delete it if the workflow starts from another trigger. Runs that name their trigger are not affected.' ) }) @@ -584,7 +583,7 @@ describe('formatWorkflowLintMessage', () => { }, }) expect(message).toBe( - 'Workflow lint found issues. Fix these before continuing: Blocks with no incoming edge: "Fn" (function). The editor\'s Run button would fail: Multiple API Trigger blocks found. Keep only one.' + 'Workflow lint found issues. Fix these before continuing: Blocks with no incoming edge: "Fn" (function). The editor\'s Run button would fail: the workflow has more than one API Trigger block. Keep only one.' ) }) }) diff --git a/apps/sim/lib/workflows/editing/lint.ts b/apps/sim/lib/workflows/editing/lint.ts index 0fec0dc00f4..0701c9b16c4 100644 --- a/apps/sim/lib/workflows/editing/lint.ts +++ b/apps/sim/lib/workflows/editing/lint.ts @@ -11,6 +11,7 @@ import { getMcpStructuredContentOutputs, getResponseFormatOutputs, } from '@/lib/workflows/blocks/outputs' +import { quotedBlockLabel } from '@/lib/workflows/labels' import type { CanonicalModeOverrides } from '@/lib/workflows/subblocks/visibility' import type { ManualRunEntryIssue } from '@/lib/workflows/triggers/manual-run' import { getBlock } from '@/blocks' @@ -431,119 +432,123 @@ type WorkflowLintIssueView = WorkflowLintResult & { } export function hasWorkflowLintIssues(lint: WorkflowLintIssueView) { - return ( - lint.orphanBlocks.length > 0 || - lint.emptyOutgoingPorts.length > 0 || - lint.invalidBranchPorts.length > 0 || - lint.invalidConnectionTargets.length > 0 || - (lint.fieldIssues?.length ?? 0) > 0 || - (lint.unresolvedReferences?.length ?? 0) > 0 || - (lint.tableFieldIssues?.length ?? 0) > 0 || - lint.manualRunIssue != null - ) + return collectBlockingSections(lint).length > 0 || lint.manualRunIssue != null } -/** One category of findings an agent must fix: its label, how many, and a renderer for them. */ +/** One category of findings an agent must fix: its label, how many, and the findings as text. */ interface WorkflowLintSection { label: string count: number - render: () => string + text: string } -function collectBlockingSections(lint: WorkflowLintIssueView): WorkflowLintSection[] { - const sections: WorkflowLintSection[] = [] - const add = (label: string, items: T[], render: (item: T) => string, separator = ', ') => { - if (items.length > 0) { - sections.push({ - label, - count: items.length, - render: () => items.map(render).join(separator), - }) - } - } +function lintSection(label: string, findings: string[], separator = ', '): WorkflowLintSection { + return { label, count: findings.length, text: findings.join(separator) } +} - add( - 'Blocks with no incoming edge', - lint.orphanBlocks, - (block) => `"${block.blockName || block.blockId}" (${block.blockType || 'unknown'})` - ) - add( - 'Unconnected required subflow start ports', - lint.emptyOutgoingPorts, - (port) => `"${port.blockName || port.blockId}".${port.label}` - ) - add( - 'Invalid condition/router branch handles', - lint.invalidBranchPorts, - (port) => `"${port.blockName || port.blockId}" uses "${port.sourceHandle}"` - ) - add( - 'Connections pointing at missing blocks', - lint.invalidConnectionTargets, - (edge) => `${edge.sourceBlockId} -> ${edge.targetBlockId}` - ) +function blockLabel(ref: WorkflowLintBlockRef): string { + return quotedBlockLabel(ref.blockId, ref.blockName) +} +function collectBlockingSections(lint: WorkflowLintIssueView): WorkflowLintSection[] { const fieldIssues = lint.fieldIssues ?? [] - add( - 'Blocks missing required fields', - fieldIssues.filter((issue) => issue.missingRequiredFields.length > 0), - (issue) => `"${issue.blockName || issue.blockId}" (${issue.missingRequiredFields.join(', ')})` - ) - add( - 'Values set on the inactive field mode (they will not resolve)', - fieldIssues.filter((issue) => issue.inactiveModeValues.length > 0), - (issue) => - `"${issue.blockName || issue.blockId}" (${issue.inactiveModeValues - .map( - (v) => `${v.inactiveMemberId}: move the value to "${v.activeMemberId ?? v.canonicalId}"` - ) - .join('; ')})` - ) - const unresolved = lint.unresolvedReferences ?? [] const renderRef = (ref: WorkflowLintUnresolvedReference) => - `"${ref.blockName || ref.blockId}".${ref.field} (${ref.reason})` - add( - 'Credential/resource references that do not resolve', - unresolved.filter((ref) => ref.kind === 'credential' || ref.kind === 'resource'), - renderRef - ) - add( - 'Agent tool/skill references that do not resolve (they will not attach at runtime)', - unresolved.filter( - (ref) => ref.kind === 'custom-tool' || ref.kind === 'mcp-tool' || ref.kind === 'skill' - ), - renderRef - ) - add( - 'Block output references that will not work as written', - unresolved.filter((ref) => ref.kind === 'block-output'), - (ref) => - `"${ref.blockName || ref.blockId}".${ref.field} ${ - Array.isArray(ref.value) ? ref.value.join(', ') : ref.value - } (${ref.reason})`, - '; ' - ) - add( - "Table filter/sort fields that are not columns of the referenced table (the run will fail in the block's error edge)", - lint.tableFieldIssues ?? [], - (issue) => `"${issue.blockName || issue.blockId}".${issue.field} (table "${issue.tableName}")` - ) + `${blockLabel(ref)}.${ref.field} (${ref.reason})` - return sections + return [ + lintSection( + 'Blocks with no incoming edge', + lint.orphanBlocks.map((block) => `${blockLabel(block)} (${block.blockType || 'unknown'})`) + ), + lintSection( + 'Unconnected required subflow start ports', + lint.emptyOutgoingPorts.map((port) => `${blockLabel(port)}.${port.label}`) + ), + lintSection( + 'Invalid condition/router branch handles', + lint.invalidBranchPorts.map((port) => `${blockLabel(port)} uses "${port.sourceHandle}"`) + ), + lintSection( + 'Connections pointing at missing blocks', + lint.invalidConnectionTargets.map((edge) => `${edge.sourceBlockId} -> ${edge.targetBlockId}`) + ), + lintSection( + 'Blocks missing required fields', + fieldIssues + .filter((issue) => issue.missingRequiredFields.length > 0) + .map((issue) => `${blockLabel(issue)} (${issue.missingRequiredFields.join(', ')})`) + ), + lintSection( + 'Values set on the inactive field mode (they will not resolve)', + fieldIssues + .filter((issue) => issue.inactiveModeValues.length > 0) + .map( + (issue) => + `${blockLabel(issue)} (${issue.inactiveModeValues + .map( + (v) => + `${v.inactiveMemberId}: move the value to "${v.activeMemberId ?? v.canonicalId}"` + ) + .join('; ')})` + ) + ), + lintSection( + 'Credential/resource references that do not resolve', + unresolved + .filter((ref) => ref.kind === 'credential' || ref.kind === 'resource') + .map(renderRef) + ), + lintSection( + 'Agent tool/skill references that do not resolve (they will not attach at runtime)', + unresolved + .filter( + (ref) => ref.kind === 'custom-tool' || ref.kind === 'mcp-tool' || ref.kind === 'skill' + ) + .map(renderRef) + ), + lintSection( + 'Block output references that will not work as written', + unresolved + .filter((ref) => ref.kind === 'block-output') + .map( + (ref) => + `${blockLabel(ref)}.${ref.field} ${ + Array.isArray(ref.value) ? ref.value.join(', ') : ref.value + } (${ref.reason})` + ), + '; ' + ), + lintSection( + "Table filter/sort fields that are not columns of the referenced table (the run will fail in the block's error edge)", + (lint.tableFieldIssues ?? []).map( + (issue) => `${blockLabel(issue)}.${issue.field} (table "${issue.tableName}")` + ) + ), + ].filter((found) => found.count > 0) } const RUN_BUTTON_REFUSAL = "The editor's Run button would fail" /** - * What the editor's Run button would refuse. Runs the agent or API start name - * their trigger and skip these checks, so this is advice for the person who - * presses Run, never a finding that blocks the caller. + * What the editor's Run button would refuse, phrased from the issue's fields + * rather than the canvas message. Runs the agent or API start name their + * trigger and skip these checks, so this is advice for the person who presses + * Run, never a finding that blocks the caller. */ function describeManualRunIssue(issue: ManualRunEntryIssue): string { - const refusal = `${RUN_BUTTON_REFUSAL}: ${issue.message.replace(/\.$/, '')}.` - if (issue.reason !== 'trigger-not-connected' || !issue.blockId) return refusal - return `${refusal} Run starts from "${issue.blockName || issue.blockId}" (${issue.blockType}) ahead of any other trigger: connect it, or delete it if the workflow starts from another trigger. Runs that name their trigger are not affected.` + switch (issue.reason) { + case 'no-trigger': + return `${RUN_BUTTON_REFUSAL}: the workflow has no trigger block to start from.` + case 'multiple-api-triggers': + return `${RUN_BUTTON_REFUSAL}: the workflow has more than one API Trigger block. Keep only one.` + case 'trigger-not-connected': { + const trigger = issue.blockId + ? `${quotedBlockLabel(issue.blockId, issue.blockName)} (${issue.blockType || 'unknown'})` + : 'its trigger' + return `${RUN_BUTTON_REFUSAL}: Run starts from ${trigger} ahead of any other trigger, and it is not connected to other blocks. Connect it, or delete it if the workflow starts from another trigger. Runs that name their trigger are not affected.` + } + } } /** @@ -556,7 +561,7 @@ export function formatWorkflowLintMessage(lint: WorkflowLintIssueView) { if (sections.length > 0) { sentences.push( `Workflow lint found issues. Fix these before continuing: ${sections - .map((section) => `${section.label}: ${section.render()}`) + .map((section) => `${section.label}: ${section.text}`) .join('; ')}` ) } diff --git a/apps/sim/lib/workflows/labels.ts b/apps/sim/lib/workflows/labels.ts index 350f80f48fc..c8b780e1f25 100644 --- a/apps/sim/lib/workflows/labels.ts +++ b/apps/sim/lib/workflows/labels.ts @@ -1,5 +1,6 @@ /** - * Display labels for workflow references that no longer resolve. + * Display labels for workflow references and blocks, shared by the surfaces + * that name them in prose. * * Lives in `lib/` rather than beside the logs table that first needed it because * `lib/workflows/subblocks/display.ts` reads it too, and importing it from @@ -8,3 +9,17 @@ * module for the sake of one string. */ export const DELETED_WORKFLOW_LABEL = 'Deleted Workflow' + +/** + * A block named in prose: `"Name"`, or `"id"` when it has no name. With + * `withId`, a named block also carries its id (`"Name" (id)`) for a reader whose + * next call addresses the block by id. + */ +export function quotedBlockLabel( + id: string, + name: string | null | undefined, + options: { withId?: boolean } = {} +): string { + if (!name) return `"${id}"` + return options.withId ? `"${name}" (${id})` : `"${name}"` +} diff --git a/apps/sim/lib/workspace-files/delivery.integration.ts b/apps/sim/lib/workspace-files/delivery.integration.ts index d7f59ea4bd1..1acd3ed20a6 100644 --- a/apps/sim/lib/workspace-files/delivery.integration.ts +++ b/apps/sim/lib/workspace-files/delivery.integration.ts @@ -5,6 +5,7 @@ import { dirname, join } from 'node:path' import { db } from '@sim/db' import { changelogRelease, + folder, permissions, user, userStats, @@ -20,7 +21,7 @@ import { import { setUploadDirServer, uploadsSetupMock } from '@sim/testing/mocks/uploads-setup.mock' import { getErrorMessage } from '@sim/utils/errors' import { generateId } from '@sim/utils/id' -import { and, eq, inArray, sql } from 'drizzle-orm' +import { and, eq, inArray } from 'drizzle-orm' import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest' vi.mock('@/lib/uploads/core/setup.server', () => uploadsSetupMock) @@ -308,9 +309,13 @@ describe('Downloads addressed by a files/ VFS path', () => { async function pathFixture() { const f = await fixture() const docsFolderId = generateId() - await db.execute(sql` - INSERT INTO folder (id, name, user_id, workspace_id, resource_type, parent_id) - VALUES (${docsFolderId}, 'Docs', ${f.editorId}, ${f.workspaceId}, 'file', NULL)`) + await db.insert(folder).values({ + id: docsFolderId, + name: 'Docs', + userId: f.editorId, + workspaceId: f.workspaceId, + resourceType: 'file', + }) const upload = (content: string, folderId: string | null) => uploadWorkspaceFile( f.workspaceId, diff --git a/packages/sim-cli/src/embed-isolation.test.ts b/packages/sim-cli/src/embed-isolation.test.ts index 9606b0b4342..64fc4a82810 100644 --- a/packages/sim-cli/src/embed-isolation.test.ts +++ b/packages/sim-cli/src/embed-isolation.test.ts @@ -176,7 +176,7 @@ describe('embedded CLI output ownership', () => { }), ]) expect(wrapped.exitCode).toBe(1) - expect(wrapped.stderr).toContain(`e.g. --rows '[{"column":"value"}]'; got a JSON object`) + expect(wrapped.stderr).toContain(`e.g. --rows '[{"key":"value"}]'; got a JSON object`) expect(wrapped.stderr).toContain('Pass the array under its "rows" key itself') expect(oneRow.stderr).toContain('For that shape use --data.') expect(rowList.stderr).toContain('--data must be a JSON object') diff --git a/packages/sim-cli/src/runtime/build.test.ts b/packages/sim-cli/src/runtime/build.test.ts index 01e8a36f167..9143c8e26af 100644 --- a/packages/sim-cli/src/runtime/build.test.ts +++ b/packages/sim-cli/src/runtime/build.test.ts @@ -3,7 +3,11 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import { V2_OPERATIONS } from '../generated/v2-api' import { SimApiError } from '../http/client' import { buildProgram } from '../program' -import { assertNoReservedProgramFlags, buildGeneratedCommands } from './build' +import { + addParseErrorGuidanceToTree, + assertNoReservedProgramFlags, + buildGeneratedCommands, +} from './build' import { kebab } from './derive' import { resetRenameWarnings } from './renamed' import type { OperationSpec } from './types' @@ -1028,3 +1032,39 @@ describe('a list that is not the whole answer', () => { expect(errors.join('')).toContain('tool names truncated') }) }) + +describe('parse-error guidance for a value typed where a flag belongs', () => { + function stderrOf(leaf: Command, argv: string[]): string { + const written: string[] = [] + const root = new Command('sim').addCommand(leaf.action(() => {})) + addParseErrorGuidanceToTree(root) + for (const command of [root, leaf]) { + command.exitOverride().configureOutput({ writeErr: (text) => written.push(text) }) + } + expect(() => root.parse([leaf.name(), ...argv], { from: 'user' })).toThrow() + return written.join('') + } + + const move = () => new Command('move').argument('').requiredOption('-w, --workspace ') + + it('names the long flag of an option that also has a short form', () => { + expect(stderrOf(move(), ['f_1', 'ws_1'])).toContain( + '"ws_1" was passed as a positional; pass it as --workspace "ws_1".' + ) + }) + + it('names no flag when an unknown option could have left the value behind', () => { + const stderr = stderrOf(move(), ['f_1', '--workspce', 'ws_1']) + expect(stderr).not.toContain('was passed as a positional') + expect(stderr).toContain('Example: sim move --workspace ') + }) + + it('names no flag when two required flags are missing', () => { + const create = new Command('create') + .requiredOption('--name ') + .requiredOption('--url ') + const stderr = stderrOf(create, ['my-server']) + expect(stderr).not.toContain('was passed as a positional') + expect(stderr).toContain('Example: sim create --name --url ') + }) +}) diff --git a/packages/sim-cli/src/runtime/build.ts b/packages/sim-cli/src/runtime/build.ts index 435dc20f036..1d3e4e9ff04 100644 --- a/packages/sim-cli/src/runtime/build.ts +++ b/packages/sim-cli/src/runtime/build.ts @@ -70,8 +70,6 @@ function commandPath(command: Command): string { return names.join(' ') } -const UNKNOWN_OPTION_TOKEN = /^error: unknown option '(.+?)'/ - /** * Whether an unknown-option token reads as a resource id rather than a flag. * @@ -93,12 +91,13 @@ function looksLikeAnId(token: string): boolean { ) } -const MISSING_OPTION_ERROR = /^error: required option '(.+?)' not specified/ -const EXCESS_ARGUMENTS_ERROR = /^error: too many arguments/ - -interface CommandInternals { - _actionHandler?: unknown - _hidden?: boolean +/** + * Whether the command runs an action of its own rather than only dispatching + * to subcommands. Commander keeps the handler off its typings, and the tree + * mixes generated commands with hand-written ones, so the contract cannot say. + */ +function hasOwnAction(command: Command): boolean { + return Boolean((command as Command & { _actionHandler?: unknown })._actionHandler) } function longFlagSyntax(option: Option): string { @@ -119,11 +118,31 @@ function optionList(command: Command): string { : `${commandPath(command)} takes no options.` } -/** Operands past the declared positionals: the values a caller typed where a flag was wanted. */ -function extraOperands(command: Command): string[] { - const declared = command.registeredArguments.length - if (command.registeredArguments.some((argument) => argument.variadic)) return [] - return command.args.filter((arg) => !arg.startsWith('-')).slice(declared) +/** + * The first unknown option commander saw. Commander files it, and every + * operand after it, at the end of `args`; an operand never opens with a dash + * unless it follows `--`, and then no option is unknown. + */ +function unknownOptionToken(command: Command): string | undefined { + return command.args.find((arg) => arg.length > 1 && arg.startsWith('-')) +} + +/** + * The value a caller typed as a positional where the one missing required flag + * was wanted (`files rename new.pdf`, `knowledge search `), when + * that reading is the only one. An unknown option puts its own value among the + * operands, and with two flags missing the value could belong to either, so + * both cases name nothing (as does a dash-led operand escaped with `--`, which + * reads the same as an unknown option here). + */ +function misplacedFlagValue(command: Command): { option: Option; value: string } | undefined { + const missing = command.options.filter( + (option) => option.mandatory && command.getOptionValue(option.attributeName()) === undefined + ) + if (missing.length !== 1 || unknownOptionToken(command) !== undefined) return undefined + if (command.registeredArguments.some((argument) => argument.variadic)) return undefined + const extra = command.args.slice(command.registeredArguments.length) + return extra.length === 1 ? { option: missing[0], value: extra[0] } : undefined } /** @@ -131,12 +150,14 @@ function extraOperands(command: Command): string[] { * with its description, so a read never picks a sibling that deletes. */ function siblingsDeclaring(command: Command, flag: string): string[] { - const siblings = command.parent?.commands ?? [] - return siblings + const parent = command.parent + if (!parent) return [] + return parent + .createHelp() + .visibleCommands(parent) .filter( (sibling) => sibling !== command && - !(sibling as Command & CommandInternals)._hidden && sibling.options.some( (option) => !option.hidden && (option.long === flag || option.short === flag) ) @@ -145,12 +166,12 @@ function siblingsDeclaring(command: Command, flag: string): string[] { } /** - * Appends recovery guidance to commander's parse errors, so the retry needs no - * `--help` round. + * Appends recovery guidance to commander's parse errors, keyed by the error + * code commander raises, so the retry needs no `--help` round. * * - A missing positional or required flag gets the full command line, and a - * value typed as a positional where the flag was wanted (`files rename - * new.pdf`, `knowledge search `) is named against that flag. + * value typed as a positional where the flag was wanted is named against + * that flag when nothing else could have put it there. * - Too many positionals get the same command line and the flag list. * - An unknown option gets the command's own flags, and the sibling commands * that do take it: `tables rows list --filter` belongs to `tables rows @@ -158,37 +179,37 @@ function siblingsDeclaring(command: Command, flag: string): string[] { * - An id swallowed as an option because it opens with a dash gets the `--` * escape, which commander never mentions. */ -function parseErrorGuidance(command: Command, message: string): string[] { - if (message.startsWith('error: missing required argument ')) { - return [`Example: ${usageExample(command)}`] - } - - const missingOption = MISSING_OPTION_ERROR.exec(message)?.[1] - if (missingOption) { - const extra = extraOperands(command) - const flag = missingOption.split(' ', 1)[0] - const moved = - extra.length === 1 - ? [`"${extra[0]}" was passed as a positional; pass it as ${flag} "${extra[0]}".`] +function parseErrorGuidance(command: Command, code: string | undefined): string[] { + switch (code) { + case 'commander.missingArgument': + return [`Example: ${usageExample(command)}`] + case 'commander.missingMandatoryOptionValue': { + const misplaced = misplacedFlagValue(command) + const moved = misplaced + ? [ + `"${misplaced.value}" was passed as a positional; pass it as ${misplaced.option.long ?? misplaced.option.short} "${misplaced.value}".`, + ] : [] - return [...moved, `Example: ${usageExample(command)}`] - } - - if (EXCESS_ARGUMENTS_ERROR.test(message)) { - return [`Example: ${usageExample(command)}`, optionList(command)] - } - - const token = UNKNOWN_OPTION_TOKEN.exec(message)?.[1] - if (!token) return [] - if (looksLikeAnId(token) && command.registeredArguments.length > 0) { - return [`Example: ${commandPath(command)} -- ${token}`] + return [...moved, `Example: ${usageExample(command)}`] + } + case 'commander.excessArguments': + return [`Example: ${usageExample(command)}`, optionList(command)] + case 'commander.unknownOption': { + const token = unknownOptionToken(command) + if (!token) return [] + if (looksLikeAnId(token) && command.registeredArguments.length > 0) { + return [`Example: ${commandPath(command)} -- ${token}`] + } + const flag = token.split('=', 1)[0] + const owners = siblingsDeclaring(command, flag) + return [ + optionList(command), + ...(owners.length > 0 ? [`${flag} is an option of:`, ...owners] : []), + ] + } + default: + return [] } - const flag = token.split('=', 1)[0] - const owners = siblingsDeclaring(command, flag) - return [ - optionList(command), - ...(owners.length > 0 ? [`${flag} is an option of:`, ...owners] : []), - ] } /** @@ -200,7 +221,7 @@ function parseErrorGuidance(command: Command, message: string): string[] { function addParseErrorGuidance(command: Command): void { const error = command.error.bind(command) command.error = (message, options) => { - const guidance = parseErrorGuidance(command, message) + const guidance = parseErrorGuidance(command, options?.code) return error(guidance.length > 0 ? `${message}\n${guidance.join('\n')}` : message, options) } } @@ -213,9 +234,7 @@ function addParseErrorGuidance(command: Command): void { */ export function addParseErrorGuidanceToTree(program: Command): void { const walk = (command: Command): void => { - const parsesOwnArguments = - command.commands.length === 0 || - Boolean((command as Command & CommandInternals)._actionHandler) + const parsesOwnArguments = command.commands.length === 0 || hasOwnAction(command) if (parsesOwnArguments || !command.parent) addParseErrorGuidance(command) for (const child of command.commands) walk(child) } @@ -298,17 +317,13 @@ export function assertNoReservedProgramFlags(program: Command): void { */ export function refuseHelpAfterUnknownCommand(program: Command): void { const walk = (command: Command): void => { - // Neither the action handler nor `unknownCommand` is in commander's - // typings, for the same reason `rawArgs` is not — reaching for them is what - // keeps this message, its "did you mean" suggestion, and its error code - // identical to the non-help path. - const internals = command as Command & { - _actionHandler?: unknown - unknownCommand: () => never - } + // `unknownCommand` is not in commander's typings, for the same reason + // `rawArgs` is not — reaching for it is what keeps this message, its "did + // you mean" suggestion, and its error code identical to the non-help path. + const internals = command as Command & { unknownCommand: () => never } const dispatchesOnly = command.commands.length > 0 && - !internals._actionHandler && + !hasOwnAction(command) && command.registeredArguments.length === 0 if (dispatchesOnly) { @@ -460,16 +475,12 @@ function configureOperation( return command } -function buildLeaf(operation: V2OperationName, spec: CommandSpec, leafName: string): Command { - return configureOperation(new Command(leafName), operation, spec) -} - /** * Registers a command at a path it used to have, hidden and warning on use. * - * The leaf is built by the same `buildLeaf` the current spelling uses, so a - * renamed command cannot drift from the one it forwards to — there is one - * definition and two ways to reach it. + * The leaf is configured by the same `configureOperation` the current spelling + * uses, so a renamed command cannot drift from the one it forwards to — there + * is one definition and two ways to reach it. * * Commander resolves a subcommand before it fills a positional, so a rename * that turned a group into a leaf (`files restore create` into `files restore`) @@ -494,7 +505,7 @@ function addRenamedCommand( parent = nestedGroup(parent, segment, { hidden: true }) } - const leaf = buildLeaf(operation, spec, rest[rest.length - 1]) + const leaf = configureOperation(new Command(rest[rest.length - 1]), operation, spec) leaf.hook('preAction', () => warnRenamedCommand(from, to)) addSubcommand(parent, leaf, { hidden: true }) } @@ -564,13 +575,13 @@ function addLeafCommand( for (const segment of rest.slice(0, -1)) { parent = nestedGroup(parent, segment) } - parent.addCommand(buildLeaf(operation, spec, rest[rest.length - 1]), { + parent.addCommand(configureOperation(new Command(rest[rest.length - 1]), operation, spec), { isDefault: spec.defaultSubcommand === true, }) return } - group.addCommand(buildLeaf(operation, spec, rest[0]), { + group.addCommand(configureOperation(new Command(rest[0]), operation, spec), { isDefault: spec.defaultSubcommand === true, }) } diff --git a/packages/sim-cli/src/runtime/request.ts b/packages/sim-cli/src/runtime/request.ts index 995a7cf23b7..a234cb8bc01 100644 --- a/packages/sim-cli/src/runtime/request.ts +++ b/packages/sim-cli/src/runtime/request.ts @@ -1,4 +1,5 @@ import { closeSync, existsSync, fstatSync, openSync, readSync } from 'node:fs' +import { isRecordLike } from '@sim/utils/object' import { CLI_CONTRACT } from '../contract/commands' import type { BodyVariantSpec, CommandSpec, FlagSpec } from '../contract/types' import { embedStore } from '../embed-context' @@ -8,38 +9,38 @@ import { embeddedFileContent } from '../transfer/local-file' import { camel, kebab } from './derive' import type { OperationSpec } from './types' -function jsonKind(value: unknown): string { - if (Array.isArray(value)) return 'a JSON array' - if (value === null) return 'null' - return typeof value === 'object' ? 'a JSON object' : `a JSON ${typeof value}` +const BODY_VARIANT_EXAMPLES: Readonly> = { + array: `'[{"key":"value"}]'`, + object: `'{"key":"value"}'`, +} + +function jsonShape(value: unknown): BodyVariantSpec['kind'] | undefined { + if (Array.isArray(value)) return 'array' + if (isRecordLike(value)) return 'object' + return undefined } /** * The refusal for a body variant given the wrong JSON shape, stating what came - * in and the spelling that takes it. `--rows '{"rows":[...]}'` and `--rows` - * with a single row were the recurring misses; neither value is reshaped, - * because guessing which part the caller meant is the server's write to make. + * in and the spelling that takes it. Neither `--rows '{"rows":[...]}'` nor a + * single-row `--rows` is reshaped, because guessing which part the caller + * meant is the server's write to make. */ function bodyVariantShapeError( variant: BodyVariantSpec, parsed: unknown, + shape: BodyVariantSpec['kind'] | undefined, variants: readonly BodyVariantSpec[] ): string { const flag = `--${variant.name}` - const example = variant.kind === 'array' ? `'[{"column":"value"}]'` : `'{"column":"value"}'` - const base = `${flag} must be a JSON ${variant.kind}, e.g. ${flag} ${example}; got ${jsonKind(parsed)}.` - if (variant.kind === 'array' && parsed && typeof parsed === 'object' && !Array.isArray(parsed)) { - if (Array.isArray((parsed as Record)[variant.property])) { - return `${base} Pass the array under its "${variant.property}" key itself, not the object wrapping it.` - } + const got = shape ? `a JSON ${shape}` : parsed === null ? 'null' : `a JSON ${typeof parsed}` + const base = `${flag} must be a JSON ${variant.kind}, e.g. ${flag} ${BODY_VARIANT_EXAMPLES[variant.kind]}; got ${got}.` + if (variant.kind === 'array' && isRecordLike(parsed) && Array.isArray(parsed[variant.property])) { + return `${base} Pass the array under its "${variant.property}" key itself, not the object wrapping it.` } - const other = variants.find( - (candidate) => - candidate !== variant && candidate.kind === (Array.isArray(parsed) ? 'array' : 'object') - ) - return other && parsed && typeof parsed === 'object' - ? `${base} For that shape use --${other.name}.` - : base + const other = + shape && variants.find((candidate) => candidate !== variant && candidate.kind === shape) + return other ? `${base} For that shape use --${other.name}.` : base } /** One request field, as the generator describes it. */ @@ -832,12 +833,12 @@ export async function buildRequest( const raw = flags[camel(variant.name)] if (typeof raw !== 'string') throw new SimApiError(`--${variant.name} is required`, 0) const parsed = await coerce(raw, { kind: variant.kind }, { json: true }, variant.name) - if ( - (variant.kind === 'object' && - (!parsed || typeof parsed !== 'object' || Array.isArray(parsed))) || - (variant.kind === 'array' && !Array.isArray(parsed)) - ) { - throw new SimApiError(bodyVariantShapeError(variant, parsed, commandSpec.bodyVariants), 0) + const shape = jsonShape(parsed) + if (shape !== variant.kind) { + throw new SimApiError( + bodyVariantShapeError(variant, parsed, shape, commandSpec.bodyVariants), + 0 + ) } return boundedRequest({ params, diff --git a/packages/testing/src/factories/index.ts b/packages/testing/src/factories/index.ts index 3321ba0432a..b8c584c70f0 100644 --- a/packages/testing/src/factories/index.ts +++ b/packages/testing/src/factories/index.ts @@ -71,3 +71,5 @@ export { createMinimalWorkflowState, createWorkflowState, } from './workflow.factory' +// Workflow lint factories +export { createEmptyWorkflowLintReport } from './workflow-lint.factory' diff --git a/packages/testing/src/factories/workflow-lint.factory.ts b/packages/testing/src/factories/workflow-lint.factory.ts new file mode 100644 index 00000000000..0135639cafa --- /dev/null +++ b/packages/testing/src/factories/workflow-lint.factory.ts @@ -0,0 +1,28 @@ +/** + * A workflow lint report with no findings, carrying every field the v2 lint + * contract publishes. A fresh object per call, so a test that spreads a finding + * in never leaks it into another. + * + * @example + * ```ts + * mocks.buildWorkflowLintReport.mockResolvedValue({ + * ...createEmptyWorkflowLintReport(), + * orphanBlocks: [{ blockId: 'fn', blockName: 'Fn', blockType: 'function' }], + * }) + * ``` + */ +export function createEmptyWorkflowLintReport() { + return { + sources: [], + sinks: [], + orphanBlocks: [], + emptyOutgoingPorts: [], + invalidBranchPorts: [], + invalidConnectionTargets: [], + fieldIssues: [], + unresolvedReferences: [], + tableFieldIssues: [], + manualRunIssue: null, + notes: [], + } +} diff --git a/packages/utils/src/string.ts b/packages/utils/src/string.ts index 98459f29458..402892f2fbb 100644 --- a/packages/utils/src/string.ts +++ b/packages/utils/src/string.ts @@ -211,18 +211,20 @@ export function sanitizeValueForJsonb(value: T): T { /** * Formats a list of names as quoted values with an overflow tail, listing at - * most `maxListed` names. + * most `maxListed` names. `quote` renders each listed name, for a caller whose + * label carries more than the quoted name. * * @example * formatQuotedNameList(['A', 'B'], 3) // '"A", "B"' * formatQuotedNameList(['A', 'B', 'C', 'D'], 3) // '"A", "B", "C" and 1 more' * formatQuotedNameList([], 3) // '' */ -export function formatQuotedNameList(names: string[], maxListed: number): string { - const listed = names - .slice(0, maxListed) - .map((name) => `"${name}"`) - .join(', ') +export function formatQuotedNameList( + names: string[], + maxListed: number, + quote: (name: string) => string = (name) => `"${name}"` +): string { + const listed = names.slice(0, maxListed).map(quote).join(', ') const overflow = names.length - maxListed return overflow > 0 ? `${listed} and ${overflow} more` : listed }