diff --git a/.changeset/cli-honest-deploy-waits.md b/.changeset/cli-honest-deploy-waits.md new file mode 100644 index 000000000..7b123cad3 --- /dev/null +++ b/.changeset/cli-honest-deploy-waits.md @@ -0,0 +1,10 @@ +--- +'@walkeros/cli': patch +--- + +`walkeros deploy` now waits long enough to cover a full server deploy by +default, so a slow but healthy deploy is no longer aborted early and reported as +a failure. Each run sends a fresh idempotency key, so retrying after a failure +starts a new deploy instead of replaying the previous result. Failures print a +stable, machine-readable error code (with a `Retry-After` hint on rate limits), +and `deploy create` no longer prints an empty token placeholder. diff --git a/.changeset/mcp-deploy-manage-contract.md b/.changeset/mcp-deploy-manage-contract.md new file mode 100644 index 000000000..f6acce385 --- /dev/null +++ b/.changeset/mcp-deploy-manage-contract.md @@ -0,0 +1,8 @@ +--- +'@walkeros/mcp': patch +--- + +The `deploy_manage` tool now matches its real behavior: `deploy` honors `wait`, +`delete` removes an active deployment, and `list` accepts `cursor` and `limit` +for pagination. A failed deployment surfaces its error reason so an assistant +can report why a deploy did not succeed. diff --git a/packages/cli/src/__tests__/unit/commands/deploy.test.ts b/packages/cli/src/__tests__/unit/commands/deploy.test.ts index dd60850f7..e40f99d58 100644 --- a/packages/cli/src/__tests__/unit/commands/deploy.test.ts +++ b/packages/cli/src/__tests__/unit/commands/deploy.test.ts @@ -1,7 +1,13 @@ import { requireProjectId } from '../../../core/auth.js'; import { apiFetch } from '../../../core/http.js'; import { getFlow } from '../../../commands/flows/index.js'; -import { deploy, getDeployment } from '../../../commands/deploy/index.js'; +import { + deploy, + getDeployment, + streamDeploymentStatus, + renderStatusLabel, + DEFAULT_DEPLOY_WAIT_MS, +} from '../../../commands/deploy/index.js'; import { setupMockApiClient } from '../../helpers/mock-api-client.js'; jest.mock('../../../core/api-client.js'); @@ -34,6 +40,23 @@ const multiFlowContent = { ], }; +/** Read the Idempotency-Key from a recorded request-init argument. */ +function idempotencyKeyFromHeaders(init: unknown): string { + if (init && typeof init === 'object' && 'headers' in init) { + const headers = (init as { headers?: Record }).headers; + return headers?.['Idempotency-Key'] ?? ''; + } + return ''; +} + +/** Read the AbortSignal from a recorded request-init argument. */ +function signalFromInit(init: unknown): AbortSignal | undefined { + if (init && typeof init === 'object' && 'signal' in init) { + return (init as { signal?: AbortSignal }).signal; + } + return undefined; +} + /** Helper to create an SSE stream response body */ function createSSEStreamBody(events: Array<{ event: string; data: object }>) { const text = events @@ -54,12 +77,15 @@ describe('deploy', () => { describe('deploy() without flowName', () => { it('calls legacy POST route', async () => { mockPost.mockResolvedValue({ - data: { status: 'bundling', deploymentId: 'dep_1' }, + data: { status: 'deploying', deploymentId: 'dep_1' }, }); await deploy({ flowId: 'cfg_1', wait: false }); expect(mockPost).toHaveBeenCalledWith( '/api/projects/{projectId}/flows/{flowId}/deploy', - { params: { path: { projectId: 'proj_default', flowId: 'cfg_1' } } }, + expect.objectContaining({ + params: { path: { projectId: 'proj_default', flowId: 'cfg_1' } }, + headers: { 'Idempotency-Key': expect.any(String) }, + }), ); }); @@ -78,7 +104,7 @@ describe('deploy', () => { it('streams deployment status via SSE when wait=true', async () => { mockPost.mockResolvedValue({ - data: { status: 'bundling', deploymentId: 'dep_1' }, + data: { status: 'deploying', deploymentId: 'dep_1' }, }); // SSE stream response @@ -87,7 +113,7 @@ describe('deploy', () => { createSSEStreamBody([ { event: 'status', - data: { status: 'bundling', substatus: 'building' }, + data: { status: 'deploying', substatus: 'building' }, }, { event: 'status', @@ -114,7 +140,7 @@ describe('deploy', () => { }); expect(result).toMatchObject({ status: 'published' }); - expect(statuses).toEqual(['bundling', 'published']); + expect(statuses).toEqual(['deploying', 'published']); expect(mockApiFetch).toHaveBeenCalledWith( '/api/projects/proj_default/deployments/dep_1/stream', expect.objectContaining({ @@ -129,7 +155,7 @@ describe('deploy', () => { mockGetFlow.mockResolvedValue(multiFlowContent as any); mockApiFetch.mockResolvedValue( new Response( - JSON.stringify({ status: 'bundling', deploymentId: 'dep_1' }), + JSON.stringify({ status: 'deploying', deploymentId: 'dep_1' }), { status: 200, }, @@ -144,7 +170,10 @@ describe('deploy', () => { }); expect(mockApiFetch).toHaveBeenCalledWith( '/api/projects/proj_default/flows/cfg_1/settings/cfg_web/deploy', - { method: 'POST' }, + expect.objectContaining({ + method: 'POST', + headers: { 'Idempotency-Key': expect.any(String) }, + }), ); }); @@ -162,7 +191,7 @@ describe('deploy', () => { // First call: trigger deploy mockApiFetch.mockResolvedValueOnce( new Response( - JSON.stringify({ status: 'bundling', deploymentId: 'dep_1' }), + JSON.stringify({ status: 'deploying', deploymentId: 'dep_1' }), { status: 200 }, ), ); @@ -172,7 +201,7 @@ describe('deploy', () => { createSSEStreamBody([ { event: 'status', - data: { status: 'bundling', substatus: 'publishing' }, + data: { status: 'deploying', substatus: 'publishing' }, }, { event: 'status', @@ -211,7 +240,7 @@ describe('deploy', () => { describe('deploy() SSE error handling', () => { it('throws on non-ok SSE stream response', async () => { mockPost.mockResolvedValue({ - data: { status: 'bundling', deploymentId: 'dep_1' }, + data: { status: 'deploying', deploymentId: 'dep_1' }, }); mockApiFetch.mockResolvedValueOnce( new Response('Not found', { status: 404 }), @@ -224,7 +253,7 @@ describe('deploy', () => { it('throws when stream ends without terminal status', async () => { mockPost.mockResolvedValue({ - data: { status: 'bundling', deploymentId: 'dep_1' }, + data: { status: 'deploying', deploymentId: 'dep_1' }, }); // Stream that closes without done event and no status events @@ -247,7 +276,7 @@ describe('deploy', () => { it('calls onStatus with substatus', async () => { mockPost.mockResolvedValue({ - data: { status: 'bundling', deploymentId: 'dep_1' }, + data: { status: 'deploying', deploymentId: 'dep_1' }, }); mockApiFetch.mockResolvedValueOnce( new Response( @@ -292,6 +321,161 @@ describe('deploy', () => { }); }); + describe('idempotency key per invocation', () => { + it('legacy POST sends a fresh unique Idempotency-Key per call', async () => { + mockPost.mockResolvedValue({ + data: { status: 'deploying', deploymentId: 'dep_1' }, + }); + + await deploy({ flowId: 'cfg_1', wait: false }); + await deploy({ flowId: 'cfg_1', wait: false }); + + const firstKey = idempotencyKeyFromHeaders(mockPost.mock.calls[0]?.[1]); + const secondKey = idempotencyKeyFromHeaders(mockPost.mock.calls[1]?.[1]); + + expect(firstKey).toEqual(expect.any(String)); + expect(firstKey.length).toBeGreaterThan(0); + expect(secondKey).not.toBe(firstKey); + }); + + it('per-settings POST sends a fresh unique Idempotency-Key per call', async () => { + mockGetFlow.mockResolvedValue(multiFlowContent as any); + // Fresh Response per call: a Response body can only be read once. + mockApiFetch + .mockResolvedValueOnce( + new Response( + JSON.stringify({ status: 'deploying', deploymentId: 'dep_1' }), + { status: 200 }, + ), + ) + .mockResolvedValueOnce( + new Response( + JSON.stringify({ status: 'deploying', deploymentId: 'dep_2' }), + { status: 200 }, + ), + ); + + await deploy({ flowId: 'cfg_1', flowName: 'web', wait: false }); + await deploy({ flowId: 'cfg_1', flowName: 'web', wait: false }); + + const firstKey = idempotencyKeyFromHeaders( + mockApiFetch.mock.calls[0]?.[1], + ); + const secondKey = idempotencyKeyFromHeaders( + mockApiFetch.mock.calls[1]?.[1], + ); + + expect(firstKey).toEqual(expect.any(String)); + expect(firstKey.length).toBeGreaterThan(0); + expect(secondKey).not.toBe(firstKey); + }); + }); + + describe('default wait budget', () => { + it('defaults to 12 minutes so a healthy long server deploy is not aborted', () => { + // Server deploy budget is 10 minutes; the client default must exceed it + // so a slow-but-healthy deploy is never aborted client-side and read as + // a CI failure. + expect(DEFAULT_DEPLOY_WAIT_MS).toBe(12 * 60 * 1000); + }); + + it('defaults the stream timeout to cover the full server deploy budget', async () => { + const body = new ReadableStream({ + start(controller) { + controller.close(); + }, + }); + mockApiFetch.mockResolvedValueOnce( + new Response(body, { + status: 200, + headers: { 'Content-Type': 'text/event-stream' }, + }), + ); + + // Stream ends without terminal status, so it rejects, but the signal + // passed to apiFetch must carry the 12-minute default budget. + await expect( + streamDeploymentStatus('proj_default', 'dep_1', {}), + ).rejects.toThrow('Stream ended without terminal status'); + + const signal = signalFromInit(mockApiFetch.mock.calls[0]?.[1]); + expect(signal).toBeInstanceOf(AbortSignal); + expect(signal?.aborted).toBe(false); + }); + + it('honors an explicit timeout override (the --wait flag)', async () => { + const body = new ReadableStream({ + start(controller) { + controller.close(); + }, + }); + mockApiFetch.mockResolvedValueOnce( + new Response(body, { + status: 200, + headers: { 'Content-Type': 'text/event-stream' }, + }), + ); + + const explicitSignal = AbortSignal.timeout(30_000); + await expect( + streamDeploymentStatus('proj_default', 'dep_1', { + signal: explicitSignal, + }), + ).rejects.toThrow('Stream ended without terminal status'); + + const signal = signalFromInit(mockApiFetch.mock.calls[0]?.[1]); + expect(signal).toBe(explicitSignal); + }); + }); + + describe('renderStatusLabel()', () => { + // The server's deployment status enum is + // ['idle', 'deploying', 'published', 'active', 'stopped', 'failed'] and the + // stream emits these (status, substatus) pairs (see the app's + // emitDeploymentUpdate calls and the snapshot route). `bundling` is never a + // status the stream emits; the build phase is `deploying`/`building`. + const emitted: Array<[string, string | null]> = [ + ['deploying', 'building'], + ['deploying', 'publishing'], + ['deploying', 'provisioning'], + ['deploying', 'starting'], + ['deploying', null], + ['published', null], + ['active', null], + ['failed', null], + ['stopped', null], + ]; + + it.each(emitted)( + 'maps emitted status %s/%s to a non-empty human label', + (status, substatus) => { + const label = renderStatusLabel(status, substatus); + expect(label.length).toBeGreaterThan(0); + // Must never echo the raw enum token as the whole label for a status we + // claim to recognize; it should be a readable phrase. + expect(label).not.toBe(status); + }, + ); + + it('falls back to the substatus-less label when only the substatus is unknown', () => { + expect(renderStatusLabel('deploying', 'unknown-substatus')).toBe( + renderStatusLabel('deploying', null), + ); + }); + + it('renders a safe fallback for an unknown status instead of nothing', () => { + const label = renderStatusLabel('totally-unknown', null); + expect(label.length).toBeGreaterThan(0); + expect(label).toContain('totally-unknown'); + }); + + it('renders a safe fallback for an unknown status with substatus', () => { + const label = renderStatusLabel('totally-unknown', 'weird'); + expect(label.length).toBeGreaterThan(0); + expect(label).toContain('totally-unknown'); + }); + }); + describe('getDeployment()', () => { it('without flowName calls typed GET route', async () => { mockGet.mockResolvedValue({ diff --git a/packages/cli/src/__tests__/unit/commands/deployments.test.ts b/packages/cli/src/__tests__/unit/commands/deployments.test.ts index ea8b06b10..01019f637 100644 --- a/packages/cli/src/__tests__/unit/commands/deployments.test.ts +++ b/packages/cli/src/__tests__/unit/commands/deployments.test.ts @@ -6,6 +6,7 @@ import { createDeployment, deleteDeployment, deleteDeploymentByFlowId, + createDeployCommand, DeploymentAmbiguityError, } from '../../../commands/deployments/index.js'; @@ -364,4 +365,54 @@ describe('deployments', () => { ).rejects.toThrow('No deployments found for flow flow_abc'); }); }); + + describe('createDeployCommand human-readable output', () => { + let logSpy: jest.SpyInstance; + + beforeEach(() => { + logSpy = jest.spyOn(console, 'log').mockImplementation(() => undefined); + }); + + afterEach(() => { + logSpy.mockRestore(); + }); + + const renderedLines = (): string => + logSpy.mock.calls.map(String).join('\n'); + + it('never prints undefined or a raw placeholder, and points at minting a real deploy token', async () => { + mockApiFetch + .mockResolvedValueOnce( + new Response( + JSON.stringify({ + config: { flows: { default: { server: {} } } }, + }), + { status: 200 }, + ), + ) + .mockResolvedValueOnce( + new Response( + JSON.stringify({ + id: 'dep_a1b2c3d4', + slug: 'k7m2x9p4q1w8', + type: 'server', + }), + { status: 201 }, + ), + ); + + await createDeployCommand('cfg_remoteflow', { + project: 'proj_123', + flow: 'default', + }); + + const output = renderedLines(); + // No undefined interpolation, and no bare `=` in the docker run. + expect(output).not.toContain('undefined'); + expect(output).not.toContain('WALKEROS_DEPLOY_TOKEN=<'); + // The instruction must point the user at minting a real deploy token. + expect(output.toLowerCase()).toContain('deploy token'); + expect(output).toContain('WALKEROS_DEPLOY_TOKEN'); + }); + }); }); diff --git a/packages/cli/src/cli.ts b/packages/cli/src/cli.ts index 563a92e62..48c4df77e 100644 --- a/packages/cli/src/cli.ts +++ b/packages/cli/src/cli.ts @@ -458,7 +458,7 @@ deployCmd .option('--no-wait', 'do not wait for deployment to complete') .option( '--timeout ', - 'timeout for deployment polling (default: 120)', + 'timeout in seconds for deployment polling (default: 720, the 12-minute server deploy budget)', ) .option('-o, --output ', 'output file path') .option('--json', 'output as JSON') diff --git a/packages/cli/src/commands/deploy/index.ts b/packages/cli/src/commands/deploy/index.ts index eac73504c..16b0b3062 100644 --- a/packages/cli/src/commands/deploy/index.ts +++ b/packages/cli/src/commands/deploy/index.ts @@ -1,3 +1,4 @@ +import { randomUUID } from 'node:crypto'; import { createApiClient } from '../../core/api-client.js'; import { requireProjectId } from '../../core/auth.js'; import { apiFetch } from '../../core/http.js'; @@ -5,13 +6,21 @@ import { ApiError, handleCliError, throwApiError, + throwApiResponseError, } from '../../core/api-error.js'; import { parseSSEEvents } from '../../core/sse.js'; import { createCLILogger } from '../../core/cli-logger.js'; import { writeResult } from '../../core/output.js'; import type { GlobalOptions } from '../../types/global.js'; +import type { components } from '../../types/api.gen.js'; import { getFlow } from '../flows/index.js'; +/** + * Response body of POST .../settings/{settingsId}/deploy, now that the + * per-settings deploy route is in the OpenAPI contract (api.gen.d.ts). + */ +type DeploySettingsResponse = components['schemas']['DeploySettingsResponse']; + // === Helpers === async function resolveSettingsId(options: { @@ -49,6 +58,15 @@ async function getAvailableFlowNames(options: { return settings?.map((c) => c.name) ?? []; } +/** + * Default client-side wait budget for a streamed deploy, in milliseconds. + * + * The server's deploy budget is 10 minutes; this default exceeds it so a + * slow-but-healthy long deploy is never aborted client-side and misread as a + * failure in CI. `--wait ` (the `timeout` option) overrides it. + */ +export const DEFAULT_DEPLOY_WAIT_MS = 12 * 60 * 1000; + // === SSE Streaming === interface DeploymentResult { @@ -70,7 +88,7 @@ export async function streamDeploymentStatus( onStatus?: (status: string, substatus: string | null) => void; }, ): Promise { - const timeoutMs = options.timeout ?? 120_000; + const timeoutMs = options.timeout ?? DEFAULT_DEPLOY_WAIT_MS; const response = await apiFetch( `/api/projects/${projectId}/deployments/${deploymentId}/stream`, @@ -148,10 +166,14 @@ export async function deploy(options: DeployOptions) { }); } - // Legacy path + // Legacy path. A fresh Idempotency-Key per invocation means a retry after a + // failed attempt is a new deploy, not a silent replay of `already_created`. const { data, error } = await client.POST( '/api/projects/{projectId}/flows/{flowId}/deploy', - { params: { path: { projectId, flowId: options.flowId } } }, + { + params: { path: { projectId, flowId: options.flowId } }, + headers: { 'Idempotency-Key': randomUUID() }, + }, ); if (error) { @@ -184,7 +206,12 @@ export async function deploy(options: DeployOptions) { return { ...data, ...result }; } -// TODO: Replace with typed client.POST() once api.gen.d.ts includes per-settings routes +// The per-settings deploy route now has an OpenAPI contract, so the success +// body is typed (`DeploySettingsResponse`). It still uses a raw `apiFetch` +// rather than the typed `client.POST()` on purpose: the bounded Retry-After +// retry below inspects `response.status` and the `Retry-After` header, and the +// `--wait` path then streams `text/event-stream`. openapi-fetch's `{ data, +// error }` client hides the raw `Response`, so it cannot drive either path. async function deploySettings(options: { flowId: string; projectId: string; @@ -196,17 +223,51 @@ async function deploySettings(options: { }) { const { flowId, projectId, settingsId } = options; - // 1. Trigger per-settings deploy - const response = await apiFetch( - `/api/projects/${projectId}/flows/${flowId}/settings/${settingsId}/deploy`, - { method: 'POST' }, - ); + // 1. Trigger per-settings deploy. A fresh Idempotency-Key per invocation + // means a retry after a failed attempt is a new deploy, not a silent replay + // of `already_created`. + const triggerDeploy = () => + apiFetch( + `/api/projects/${projectId}/flows/${flowId}/settings/${settingsId}/deploy`, + { method: 'POST', headers: { 'Idempotency-Key': randomUUID() } }, + ); + + let response = await triggerDeploy(); + + // For `--wait`, honor a single bounded Retry-After on a rate-limited trigger + // before giving up, so a brief deploy-rate ceiling does not fail an + // otherwise healthy automated deploy. + if (!response.ok && options.wait) { + let retryBody: unknown = {}; + try { + retryBody = await response.clone().json(); + } catch { + retryBody = {}; + } + try { + throwApiResponseError( + response, + retryBody, + `Deploy failed (${response.status})`, + ); + } catch (e) { + if (e instanceof ApiError && e.retryable) { + const waitSeconds = Math.min(e.retryAfterSeconds ?? 5, 60); + options.onStatus?.('rate_limited', `retrying in ${waitSeconds}s`); + await new Promise((resolve) => setTimeout(resolve, waitSeconds * 1000)); + response = await triggerDeploy(); + } else { + throw e; + } + } + } + if (!response.ok) { const body = await response.json().catch(() => ({})); - throwApiError(body, `Deploy failed (${response.status})`); + throwApiResponseError(response, body, `Deploy failed (${response.status})`); } - const data = await response.json(); + const data = (await response.json()) as DeploySettingsResponse; if (!options.wait) return data; // 2. Stream deployment status via SSE @@ -237,7 +298,7 @@ export async function getDeployment(options: { ); if (!response.ok) { const body = await response.json().catch(() => ({})); - throwApiError(body, 'Failed to get deployment'); + throwApiResponseError(response, body, 'Failed to get deployment'); } return response.json(); } @@ -263,18 +324,35 @@ interface DeployCommandOptions extends GlobalOptions { json?: boolean; } +// Progress labels keyed on the statuses the deploy stream ACTUALLY emits. The +// server status enum is idle | deploying | published | active | stopped | +// failed; the bundle phase is reported as `deploying`/`building`, not a +// separate `bundling` status. Keys are `status` or `status:substatus`. const statusLabels: Record = { - bundling: 'Building bundle...', - 'bundling:building': 'Building bundle...', - 'bundling:publishing': 'Publishing to web...', - deploying: 'Deploying container...', + 'deploying:building': 'Building bundle...', + 'deploying:publishing': 'Publishing to web...', 'deploying:provisioning': 'Provisioning container...', 'deploying:starting': 'Starting container...', + deploying: 'Deploying...', active: 'Container is live', published: 'Published', + stopped: 'Stopped', failed: 'Deployment failed', }; +/** + * Map a streamed `(status, substatus)` to a human progress line, falling back + * to the bare status label, then to a safe `Status: ` line for an unknown + * status so an unexpected value still renders something instead of nothing. + */ +export function renderStatusLabel( + status: string, + substatus: string | null, +): string { + const key = substatus ? `${status}:${substatus}` : status; + return statusLabels[key] || statusLabels[status] || `Status: ${status}`; +} + export async function deployCommand( flowId: string, options: DeployCommandOptions, @@ -295,10 +373,7 @@ export async function deployCommand( onStatus: options.json ? undefined : (status, substatus) => { - const key = substatus ? `${status}:${substatus}` : status; - log.info( - statusLabels[key] || statusLabels[status] || `Status: ${status}`, - ); + log.info(renderStatusLabel(status, substatus)); }, }); @@ -316,7 +391,9 @@ export async function deployCommand( } else if (r.status === 'failed') { log.error(`Failed: ${r.errorMessage || 'Unknown error'}`); process.exit(1); - } else if (r.status === 'bundling') { + } else if (r.status === 'deploying') { + // Non-wait path: the deploy-trigger response reports `deploying` (the + // server status enum has no `bundling`; the build phase is a substatus). log.info(`Deployment started: ${r.deploymentId} (${r.type})`); } else { log.info(`Status: ${r.status}`); diff --git a/packages/cli/src/commands/deployments/index.ts b/packages/cli/src/commands/deployments/index.ts index d367b1a2b..1f16be7f2 100644 --- a/packages/cli/src/commands/deployments/index.ts +++ b/packages/cli/src/commands/deployments/index.ts @@ -305,24 +305,23 @@ export async function createDeployCommand( return; } - // Human-readable output + // Human-readable output. The create-deployment response does not include a + // deploy token: tokens are minted separately (admin-only) and never stored + // in plaintext, so we point the user at creating one rather than printing a + // missing field. log.info(`Deployment created: ${result.id}`); log.info(` Slug: ${result.slug}`); log.info(` Type: ${result.type}`); - if (result.deployToken) { - log.info(` Token: ${result.deployToken}`); - log.warn(' Save this token — it will not be shown again.'); - } log.info(''); log.info('Run locally:'); log.info( ` walkeros run ${isRemoteFlow ? 'flow.json' : config} --deploy ${result.id}`, ); log.info(''); - log.info('Docker:'); - log.info( - ` docker run -e WALKEROS_DEPLOY_TOKEN=${result.deployToken ?? ''} \\`, - ); + log.info('Create a deploy token for this flow in the app'); + log.info('(Settings, Self-hosted deploy token) and set it as'); + log.info('WALKEROS_DEPLOY_TOKEN, then run with Docker:'); + log.info(' docker run -e WALKEROS_DEPLOY_TOKEN \\'); log.info(' -e WALKEROS_APP_URL=https://app.walkeros.io \\'); log.info(' walkeros/flow:latest'); } catch (err) { diff --git a/packages/cli/src/core/__tests__/api-error.test.ts b/packages/cli/src/core/__tests__/api-error.test.ts index 65ab82f98..2a09cfbf5 100644 --- a/packages/cli/src/core/__tests__/api-error.test.ts +++ b/packages/cli/src/core/__tests__/api-error.test.ts @@ -1,4 +1,10 @@ -import { ApiError, throwApiError } from '../api-error'; +import { + ApiError, + throwApiError, + throwApiResponseError, + handleCliError, + EXIT_RETRYABLE, +} from '../api-error'; describe('ApiError', () => { it('should carry code, message, and details', () => { @@ -20,6 +26,149 @@ describe('ApiError', () => { expect(err.code).toBeUndefined(); expect(err.details).toBeUndefined(); }); + + it('should carry status, retryable, and retryAfterSeconds', () => { + const err = new ApiError('Rate limited', { + code: 'RATE_LIMITED', + status: 429, + retryable: true, + retryAfterSeconds: 30, + }); + expect(err.status).toBe(429); + expect(err.retryable).toBe(true); + expect(err.retryAfterSeconds).toBe(30); + }); +}); + +describe('throwApiResponseError', () => { + function fakeResponse(status: number, retryAfter?: string): Response { + const headers = new Headers(); + if (retryAfter !== undefined) headers.set('Retry-After', retryAfter); + return { status, headers } as unknown as Response; + } + + it('marks a 429 with numeric Retry-After as retryable with a wait hint', () => { + const response = fakeResponse(429, '30'); + const body = { + error: { code: 'RATE_LIMITED', message: 'Too many deploys' }, + }; + + try { + throwApiResponseError(response, body, 'Deploy failed'); + throw new Error('expected throw'); + } catch (e) { + const err = e as ApiError; + expect(err).toBeInstanceOf(ApiError); + expect(err.status).toBe(429); + expect(err.retryable).toBe(true); + expect(err.retryAfterSeconds).toBe(30); + expect(err.code).toBe('RATE_LIMITED'); + } + }); + + it('treats a 503 with Retry-After as retryable', () => { + const response = fakeResponse(503, '5'); + try { + throwApiResponseError(response, {}, 'Service unavailable'); + throw new Error('expected throw'); + } catch (e) { + const err = e as ApiError; + expect(err.status).toBe(503); + expect(err.retryable).toBe(true); + expect(err.retryAfterSeconds).toBe(5); + } + }); + + it('marks a 409 (already in progress) as non-retryable', () => { + const response = fakeResponse(409); + const body = { + error: { code: 'DEPLOY_IN_PROGRESS', message: 'Already deploying' }, + }; + try { + throwApiResponseError(response, body, 'Deploy failed'); + throw new Error('expected throw'); + } catch (e) { + const err = e as ApiError; + expect(err.status).toBe(409); + expect(err.retryable).toBe(false); + expect(err.code).toBe('DEPLOY_IN_PROGRESS'); + } + }); + + it('handles an HTTP-date Retry-After value', () => { + const future = new Date(Date.now() + 12_000).toUTCString(); + const response = fakeResponse(429, future); + try { + throwApiResponseError(response, {}, 'Deploy failed'); + throw new Error('expected throw'); + } catch (e) { + const err = e as ApiError; + expect(err.retryable).toBe(true); + // Allow a little slack for clock/rounding. + expect(err.retryAfterSeconds).toBeGreaterThanOrEqual(10); + expect(err.retryAfterSeconds).toBeLessThanOrEqual(13); + } + }); +}); + +describe('handleCliError machine-readable output', () => { + let errorSpy: jest.SpyInstance; + let exitSpy: jest.SpyInstance; + + beforeEach(() => { + errorSpy = jest.spyOn(console, 'error').mockImplementation(() => undefined); + exitSpy = jest + .spyOn(process, 'exit') + .mockImplementation((code?: string | number | null) => { + throw new Error(`__exit__:${code}`); + }); + }); + + afterEach(() => { + errorSpy.mockRestore(); + exitSpy.mockRestore(); + }); + + function output(): string { + return errorSpy.mock.calls.map((c) => String(c[0])).join('\n'); + } + + it('prints a stable parseable code line CI can branch on', () => { + const err = new ApiError('Request validation failed', { + code: 'VALIDATION_ERROR', + }); + expect(() => handleCliError(err)).toThrow('__exit__:1'); + expect(output()).toMatch(/^error: code=VALIDATION_ERROR\b/m); + }); + + it('reports a retryable error with the wait hint and a distinct exit code', () => { + const err = new ApiError('Too many deploys', { + code: 'RATE_LIMITED', + status: 429, + retryable: true, + retryAfterSeconds: 30, + }); + expect(() => handleCliError(err)).toThrow(`__exit__:${EXIT_RETRYABLE}`); + const out = output(); + expect(out).toMatch(/^error: code=RATE_LIMITED\b/m); + expect(out).toMatch(/retryable=true/); + expect(out).toMatch(/retryAfter=30/); + }); + + it('keeps non-retryable failures clearly distinct (exit 1, retryable=false)', () => { + const err = new ApiError('Already deploying', { + code: 'DEPLOY_IN_PROGRESS', + status: 409, + retryable: false, + }); + expect(() => handleCliError(err)).toThrow('__exit__:1'); + expect(output()).toMatch(/retryable=false/); + }); + + it('falls back to code=UNKNOWN for a plain Error', () => { + expect(() => handleCliError(new Error('boom'))).toThrow('__exit__:1'); + expect(output()).toMatch(/^error: code=UNKNOWN\b/m); + }); }); describe('throwApiError', () => { diff --git a/packages/cli/src/core/api-error.ts b/packages/cli/src/core/api-error.ts index ff67deb5d..d3d567a8a 100644 --- a/packages/cli/src/core/api-error.ts +++ b/packages/cli/src/core/api-error.ts @@ -6,6 +6,12 @@ export interface ApiErrorDetail { export interface ApiErrorOptions { code?: string; details?: ApiErrorDetail[]; + // HTTP status of the failed response, when known. + status?: number; + // True when the failure is worth retrying (e.g. 429 / 503 with Retry-After). + retryable?: boolean; + // Seconds to wait before retrying, parsed from the Retry-After header. + retryAfterSeconds?: number; // Populated only for CLIENT_OUTDATED responses (HTTP 426). minVersion?: string; clientVersion?: string; @@ -14,9 +20,17 @@ export interface ApiErrorOptions { docs?: string; } +// Exit code for a retryable failure (e.g. rate limited). Distinct from the +// generic failure (1) and upgrade-required (2) so CI can branch on it. +// Mirrors the BSD sysexits.h EX_TEMPFAIL convention. +export const EXIT_RETRYABLE = 75; + export class ApiError extends Error { code?: string; details?: ApiErrorDetail[]; + status?: number; + retryable?: boolean; + retryAfterSeconds?: number; // Populated only for CLIENT_OUTDATED responses (HTTP 426). minVersion?: string; clientVersion?: string; @@ -29,6 +43,9 @@ export class ApiError extends Error { this.name = 'ApiError'; this.code = options?.code; this.details = options?.details; + this.status = options?.status; + this.retryable = options?.retryable; + this.retryAfterSeconds = options?.retryAfterSeconds; this.minVersion = options?.minVersion; this.clientVersion = options?.clientVersion; this.client = options?.client; @@ -38,43 +55,121 @@ export class ApiError extends Error { } /** - * Extract structured error from an openapi-fetch error response and throw. + * Build ApiError options from an openapi-style error body. * The error shape is: { error: { code, message, details: { errors: [] } } } + * Returns `null` when the body has no recognizable `error` object. + */ +function extractApiErrorOptions( + error: unknown, + fallbackMessage: string, +): { message: string; options: ApiErrorOptions } | null { + if ( + !error || + typeof error !== 'object' || + !('error' in error) || + typeof (error as Record).error !== 'object' + ) { + return null; + } + const inner = (error as { error: Record }).error; + const message = (inner.message as string) || fallbackMessage; + const code = inner.code as string | undefined; + const details = (inner.details as Record)?.errors as + | ApiErrorDetail[] + | undefined; + const options: ApiErrorOptions = { code, details }; + if (code === 'CLIENT_OUTDATED') { + options.minVersion = inner.minVersion as string | undefined; + options.clientVersion = inner.clientVersion as string | undefined; + options.client = inner.client as string | undefined; + options.upgrade = inner.upgrade as string | undefined; + options.docs = inner.docs as string | undefined; + } + return { message, options }; +} + +/** + * Extract structured error from an openapi-fetch error response and throw. * * For `code === 'CLIENT_OUTDATED'` (HTTP 426), also extracts the upgrade * metadata: `minVersion`, `clientVersion`, `client`, `upgrade`, `docs`. */ export function throwApiError(error: unknown, fallbackMessage: string): never { - if ( - error && - typeof error === 'object' && - 'error' in error && - typeof (error as Record).error === 'object' - ) { - const inner = (error as { error: Record }).error; - const message = (inner.message as string) || fallbackMessage; - const code = inner.code as string | undefined; - const details = (inner.details as Record)?.errors as - | ApiErrorDetail[] - | undefined; - const options: ApiErrorOptions = { code, details }; - if (code === 'CLIENT_OUTDATED') { - options.minVersion = inner.minVersion as string | undefined; - options.clientVersion = inner.clientVersion as string | undefined; - options.client = inner.client as string | undefined; - options.upgrade = inner.upgrade as string | undefined; - options.docs = inner.docs as string | undefined; - } - throw new ApiError(message, options); - } + const extracted = extractApiErrorOptions(error, fallbackMessage); + if (extracted) throw new ApiError(extracted.message, extracted.options); throw new ApiError(fallbackMessage); } /** - * Pretty-print an error and exit. Recognises `ApiError` with + * Parse a `Retry-After` header value into seconds. Accepts both the + * delta-seconds form (`"30"`) and the HTTP-date form. Returns `undefined` + * when the value is absent or unparseable. + */ +export function parseRetryAfter(value: string | null): number | undefined { + if (!value) return undefined; + const trimmed = value.trim(); + if (/^\d+$/.test(trimmed)) return parseInt(trimmed, 10); + const when = Date.parse(trimmed); + if (Number.isNaN(when)) return undefined; + return Math.max(0, Math.round((when - Date.now()) / 1000)); +} + +/** + * Like `throwApiError`, but classifies retryability from the HTTP response. + * A 429 is always retryable; a 503 is retryable when it carries a + * `Retry-After` hint. The parsed wait (seconds) is attached for `--wait` + * callers to honor and for the machine-readable error line. + */ +export function throwApiResponseError( + response: Pick, + body: unknown, + fallbackMessage: string, +): never { + const status = response.status; + const retryAfterSeconds = parseRetryAfter( + response.headers.get('retry-after'), + ); + const retryable = + status === 429 || (status === 503 && retryAfterSeconds !== undefined); + + const extracted = extractApiErrorOptions(body, fallbackMessage); + const message = extracted?.message ?? fallbackMessage; + const options: ApiErrorOptions = { + ...(extracted?.options ?? {}), + status, + retryable, + retryAfterSeconds, + }; + throw new ApiError(message, options); +} + +/** + * Emit a stable, parseable machine-readable error line so CI can branch on + * the failure without string-matching prose. Format: + * + * error: code= retryable=[ retryAfter=] message= + * + * `code` is `UNKNOWN` for non-ApiError failures. The line always starts with + * `error: code=` and the human-readable message comes last. + */ +function machineReadableErrorLine(err: unknown): string { + const code = err instanceof ApiError ? (err.code ?? 'UNKNOWN') : 'UNKNOWN'; + const retryable = err instanceof ApiError ? err.retryable === true : false; + const retryAfter = + err instanceof ApiError && err.retryAfterSeconds !== undefined + ? ` retryAfter=${err.retryAfterSeconds}` + : ''; + const message = err instanceof Error ? err.message : String(err); + return `error: code=${code} retryable=${retryable}${retryAfter} message=${message}`; +} + +/** + * Pretty-print an error and exit. Always prints a machine-readable code line + * first (see `machineReadableErrorLine`). Recognises `ApiError` with * `code === 'CLIENT_OUTDATED'` as a dedicated upgrade-required path - * (exit code 2), prints the upgrade and docs links, and otherwise - * falls back to a generic error print with exit code 1. + * (exit code 2) and prints the upgrade and docs links. Retryable failures + * (e.g. rate limited) exit with `EXIT_RETRYABLE` (75). Everything else + * exits 1. * * Designed to be used as a top-level handler (`process.on('uncaughtException', ...)`, * `program.parseAsync().catch(...)`) and as a drop-in replacement for @@ -82,14 +177,25 @@ export function throwApiError(error: unknown, fallbackMessage: string): never { */ export function handleCliError(err: unknown): never { /* eslint-disable no-console */ + console.error(machineReadableErrorLine(err)); + if (err instanceof ApiError && err.code === 'CLIENT_OUTDATED') { console.error(`\n${err.message}\n`); if (err.upgrade) console.error(` Upgrade: ${err.upgrade}`); if (err.docs) console.error(` Docs: ${err.docs}\n`); process.exit(2); } - const message = err instanceof Error ? err.message : String(err); - console.error(message); + + if (err instanceof ApiError && err.retryable) { + const hint = + err.retryAfterSeconds !== undefined + ? ` Retry after ${err.retryAfterSeconds}s.` + : ' This is temporary, retry shortly.'; + console.error(`${err.message}${hint}`); + process.exit(EXIT_RETRYABLE); + } + + console.error(err instanceof Error ? err.message : String(err)); process.exit(1); /* eslint-enable no-console */ } diff --git a/packages/mcps/mcp/src/__tests__/tools/deploy-manage.test.ts b/packages/mcps/mcp/src/__tests__/tools/deploy-manage.test.ts index 3ace423a1..65511393e 100644 --- a/packages/mcps/mcp/src/__tests__/tools/deploy-manage.test.ts +++ b/packages/mcps/mcp/src/__tests__/tools/deploy-manage.test.ts @@ -80,6 +80,31 @@ describe('deploy_manage tool', () => { }); }); + it('describes the now-real hosted behavior honestly', () => { + registerDeployTool(server as never, stubClient()); + const tool = server.getTool('deploy_manage')!; + const config = tool.config as { description: string }; + const description = config.description.toLowerCase(); + + // wait is honored, including the budget + expect(description).toContain('wait'); + expect(description).toContain('12-minute'); + + // delete works; no always-throws disclaimer + expect(description).toContain('delete'); + expect(description).not.toContain('throw'); + expect(description).not.toContain('not yet'); + expect(description).not.toContain('not implemented'); + expect(description).not.toContain('always'); + + // pagination args are passed through + expect(description).toContain('cursor'); + expect(description).toContain('limit'); + + // failure detail is reported by get/status + expect(description).toContain('errormessage'); + }); + describe('deploy', () => { it('requires flowId', async () => { registerDeployTool(server as never, stubClient()); diff --git a/packages/mcps/mcp/src/tools/deploy-manage.ts b/packages/mcps/mcp/src/tools/deploy-manage.ts index 5f8088226..3303f6bf6 100644 --- a/packages/mcps/mcp/src/tools/deploy-manage.ts +++ b/packages/mcps/mcp/src/tools/deploy-manage.ts @@ -20,7 +20,11 @@ import { const TITLE = 'Deploy Management'; const DESCRIPTION = 'Deploy walkerOS flows and manage deployments. ' + - 'For get/delete actions pass flowId (required) plus optional slug to disambiguate when a flow has multiple active deployments. ' + + 'deploy waits for the deployment to reach a terminal status by default (wait=true), with a 12-minute budget; pass wait=false to return immediately with the deployment id. ' + + 'A finished deployment carries its status and, on failure, an errorMessage with the user-facing reason; use the get action to re-read it. ' + + 'list supports cursor and limit for pagination. ' + + 'delete removes an active deployment. ' + + 'For get and delete pass flowId (required) plus optional slug to disambiguate when a flow has multiple active deployments. ' + "If a flow has >=2 active deployments and no slug is supplied, the tool returns a MULTIPLE_DEPLOYMENTS error with a details[] list showing each deployment's slug, type, status, and updatedAt."; const inputSchema = { @@ -47,7 +51,7 @@ const inputSchema = { .boolean() .optional() .describe( - 'Wait for deploy to complete (default true). Only used with deploy action.', + 'Wait for the deployment to reach a terminal status (default true), with a 12-minute budget. Set false to return the deployment id immediately. Only used with deploy action.', ), flowName: z .string() diff --git a/packages/web/destinations/snowplow/src/types/index.ts b/packages/web/destinations/snowplow/src/types/index.ts index 4fb9cd536..ae7cab0d7 100644 --- a/packages/web/destinations/snowplow/src/types/index.ts +++ b/packages/web/destinations/snowplow/src/types/index.ts @@ -15,23 +15,13 @@ import type { BrowserPlugin, ActivityTrackingConfiguration, } from '@snowplow/browser-tracker-core'; -import type { - Action, - Product, - Cart, - SPTransaction, - SPPromotion, - CheckoutStep, - Refund, - TransactionError, - User, - Page, -} from '@snowplow/browser-plugin-snowplow-ecommerce'; +// Action is used in a type position below (Action['type']), so it must stay +// imported. The other entity types are only re-exported. +import type { Action } from '@snowplow/browser-plugin-snowplow-ecommerce'; // Re-export official Snowplow entity types +export type { SelfDescribingJson, CommonEventProperties }; export type { - SelfDescribingJson, - CommonEventProperties, Action, Product, Cart, @@ -42,7 +32,7 @@ export type { TransactionError, User, Page, -}; +} from '@snowplow/browser-plugin-snowplow-ecommerce'; // Re-export Snowplow tracker core types export type { BrowserPlugin, ActivityTrackingConfiguration }; diff --git a/skills/walkeros-using-cli/commands-reference.md b/skills/walkeros-using-cli/commands-reference.md index 6dcb82f01..716f34abd 100644 --- a/skills/walkeros-using-cli/commands-reference.md +++ b/skills/walkeros-using-cli/commands-reference.md @@ -167,7 +167,7 @@ walkeros deploy start [options] | `--project ` | Project ID (defaults to WALKEROS_PROJECT_ID) | | `-f, --flow ` | Flow name for multi-config flows | | `--no-wait` | Return immediately without streaming | -| `--timeout ` | Timeout in seconds (default: 120) | +| `--timeout ` | Override the wait budget (default: 12 min) | | `--json` | JSON output | | `-v, --verbose` | Verbose logging | | `-s, --silent` | Silent mode | diff --git a/website/docs/apps/cli.mdx b/website/docs/apps/cli.mdx index 681ff725c..0706308d7 100644 --- a/website/docs/apps/cli.mdx +++ b/website/docs/apps/cli.mdx @@ -1305,7 +1305,7 @@ walkeros deploy create flow.json --flow production`} language="bash" /> -On success, the CLI displays the deployment ID, slug, type, and a one-time deploy token. It also shows example commands for running the deployment locally or via Docker. +On success, the CLI displays the deployment ID, slug, and type. It also shows example commands for running the deployment locally or via Docker. The create response does not include a deploy token: deploy tokens are minted separately, so the Docker instructions tell you to create one in the app (Settings, Self-hosted deploy token) and set it as `WALKEROS_DEPLOY_TOKEN`. | Option | Description | |--------|-------------| @@ -1353,12 +1353,23 @@ Starting container... | `--project ` | Project ID (defaults to `WALKEROS_PROJECT_ID`) | | `-f, --flow ` | Flow name for multi-config flows | | `--no-wait` | Return immediately after triggering (do not stream progress) | -| `--timeout ` | Timeout for deployment polling (default: 120) | +| `--timeout ` | Override the client-side wait budget (defaults to 12 minutes, which covers the server's full deploy budget) | | `-o, --output ` | Write result to file | | `--json` | Output as JSON | | `-v, --verbose` | Verbose output | | `-s, --silent` | Suppress output | +By default `deploy start` streams progress until the deploy completes. The +client-side wait budget defaults to 12 minutes so a slow but healthy long +deploy is never aborted early and misread as a failure in CI; `--timeout` +overrides it. Each invocation sends a fresh idempotency key, so retrying after +a failed attempt starts a new deploy instead of replaying the previous result. + +When a deploy fails, the CLI prints a stable, machine-readable `error.code` +alongside the message so CI can branch on the code without matching prose. A +rate-limited trigger (`429`) is reported as retryable with the `Retry-After` +hint; under `--wait` the CLI honors one bounded retry automatically. + ### deploy list List all deployments in a project. diff --git a/website/docs/apps/mcp.mdx b/website/docs/apps/mcp.mdx index b6d49b501..2cdd51899 100644 --- a/website/docs/apps/mcp.mdx +++ b/website/docs/apps/mcp.mdx @@ -326,8 +326,12 @@ Deploy walkerOS flows and manage deployments. | `projectId` | string | No | Project ID. Optional; falls back to the default project. | | `type` | `"web"` \| `"server"` | No | Deployment type filter for `list`. | | `status` | string | No | Status filter for `list`. | -| `wait` | boolean | No | Wait for deploy to complete (default: `true`). Only used with `deploy`. | +| `wait` | boolean | No | Wait for the deployment to reach a terminal status (default: `true`), with a 120-second budget. Set `false` to return the deployment id immediately. Only used with `deploy`. | | `flowName` | string | No | Flow name for multi-settings flows. Only used with `deploy`. | +| `cursor` | string | No | Pagination cursor from a previous `list` response. Only used with `list`. | +| `limit` | number | No | Max items per page (1-100). Only used with `list`. | + +A finished `deploy` carries its status and, on failure, an `errorMessage` with the user-facing reason; use the `get` action to re-read it. When a flow has more than one active deployment and no `slug` is supplied, `get` and `delete` return a `MULTIPLE_DEPLOYMENTS` error with a `details[]` list so the caller can pick a specific deployment. Soft-deleted deployments are always excluded.