diff --git a/.claude/rules/07-env-and-urls.md b/.claude/rules/07-env-and-urls.md index db51f0f8c..3e2f7f3d7 100644 --- a/.claude/rules/07-env-and-urls.md +++ b/.claude/rules/07-env-and-urls.md @@ -85,6 +85,10 @@ The CI quality check (`pnpm quality:wrangler-bindings`) verifies: 1. No `[env.*]` sections exist in checked-in `wrangler.toml` files 2. All required binding types are present at the top level +### Required action when adding a Worker var consumed by sync-wrangler-config + +If `scripts/deploy/sync-wrangler-config.ts` reads a GitHub Environment variable from `process.env` to generate Worker `[vars]`, add it to the centralized `wrangler_sync_env` mapping in `.github/workflows/deploy-reusable.yml`. Do not add ad hoc per-step sync env blocks. First deploys run the sync script twice (initial sync, then tail-consumer re-sync), and both invocations must receive identical optional Worker env inputs so the second sync cannot silently drop operator overrides or restore script defaults. + ### Why this architecture Wrangler does NOT inherit bindings (D1, KV, R2, DO, AI, tail_consumers) from top-level into `[env.*]` sections. Previously, this required manually duplicating every binding 3x (top-level + staging + production). Now the sync script generates complete env sections, eliminating duplication and making the config fork-friendly. diff --git a/.github/workflows/deploy-reusable.yml b/.github/workflows/deploy-reusable.yml index ead24611c..f272d0eb6 100644 --- a/.github/workflows/deploy-reusable.yml +++ b/.github/workflows/deploy-reusable.yml @@ -357,6 +357,7 @@ jobs: BASE_DOMAIN: ${{ vars.BASE_DOMAIN }} RESOURCE_PREFIX: ${{ steps.prefix.outputs.value }} REQUIRE_APPROVAL: ${{ vars.REQUIRE_APPROVAL }} + SETUP_FORCE: ${{ vars.SETUP_FORCE }} HETZNER_BASE_IMAGE: ${{ vars.HETZNER_BASE_IMAGE }} PLATFORM_FEEDBACK_PROJECT_ID: ${{ vars.PLATFORM_FEEDBACK_PROJECT_ID }} REPORT_ISSUE_TITLE_MAX_LENGTH: ${{ vars.REPORT_ISSUE_TITLE_MAX_LENGTH }} @@ -373,11 +374,16 @@ jobs: DO_MIGRATION_STATE_PROBE_RETRY_DELAY_MS: ${{ vars.DO_MIGRATION_STATE_PROBE_RETRY_DELAY_MS }} CF_CONTAINER_ENABLED: ${{ vars.CF_CONTAINER_ENABLED }} CF_CONTAINER_SLEEP_AFTER: ${{ vars.CF_CONTAINER_SLEEP_AFTER }} + CF_CONTAINER_ACTIVE_WORK_MAX_MS: ${{ vars.CF_CONTAINER_ACTIVE_WORK_MAX_MS }} + CF_CONTAINER_KEEPALIVE_RENEW_INTERVAL_MS: ${{ vars.CF_CONTAINER_KEEPALIVE_RENEW_INTERVAL_MS }} CF_CONTAINER_PORT_READY_TIMEOUT_MS: ${{ vars.CF_CONTAINER_PORT_READY_TIMEOUT_MS }} CF_CONTAINER_WAKE_TIMEOUT_MS: ${{ vars.CF_CONTAINER_WAKE_TIMEOUT_MS }} + CF_CONTAINER_RECOVERY_MAX_ATTEMPTS: ${{ vars.CF_CONTAINER_RECOVERY_MAX_ATTEMPTS }} CF_CONTAINER_CREATE_WORKSPACE_TIMEOUT_MS: ${{ vars.CF_CONTAINER_CREATE_WORKSPACE_TIMEOUT_MS }} CF_CONTAINER_CLONE_FILTER: ${{ vars.CF_CONTAINER_CLONE_FILTER }} CF_CONTAINER_VM_AGENT_PORT: ${{ vars.CF_CONTAINER_VM_AGENT_PORT }} + SANDBOX_CONTAINER_MAX_INSTANCES: ${{ vars.SANDBOX_CONTAINER_MAX_INSTANCES }} + VM_AGENT_CONTAINER_MAX_INSTANCES: ${{ vars.VM_AGENT_CONTAINER_MAX_INSTANCES }} SANDBOX_ENABLED: ${{ vars.SANDBOX_ENABLED }} MAX_CONCURRENT_SETUP_SESSIONS: ${{ vars.MAX_CONCURRENT_SETUP_SESSIONS }} SETUP_SESSION_TTL_MS: ${{ vars.SETUP_SESSION_TTL_MS }} @@ -447,8 +453,10 @@ jobs: elif [ "$HTTP_CODE" -eq 409 ]; then echo "workers.dev subdomain already configured (OK)" else - echo "::warning::Failed to set workers.dev subdomain (HTTP ${HTTP_CODE}): ${BODY}" - echo "Cron triggers may not work. Initialize manually: CF Dashboard > Workers & Pages > Settings > Domains & Routes" + echo "::error::Failed to set workers.dev subdomain (HTTP ${HTTP_CODE}): ${BODY}" + echo "Deployment cannot continue because Cloudflare cron triggers require the workers.dev subdomain prerequisite." + echo "Initialize manually: CF Dashboard > Workers & Pages > Settings > Domains & Routes" + exit 1 fi # Verify Pages custom domain is active (must run BEFORE Worker deploys wildcard routes) @@ -686,10 +694,42 @@ jobs: BASE_DOMAIN: ${{ vars.BASE_DOMAIN }} RESOURCE_PREFIX: ${{ steps.prefix.outputs.value }} REQUIRE_APPROVAL: ${{ vars.REQUIRE_APPROVAL }} + SETUP_FORCE: ${{ vars.SETUP_FORCE }} HETZNER_BASE_IMAGE: ${{ vars.HETZNER_BASE_IMAGE }} + PLATFORM_FEEDBACK_PROJECT_ID: ${{ vars.PLATFORM_FEEDBACK_PROJECT_ID }} + REPORT_ISSUE_TITLE_MAX_LENGTH: ${{ vars.REPORT_ISSUE_TITLE_MAX_LENGTH }} + REPORT_ISSUE_DESCRIPTION_MAX_LENGTH: ${{ vars.REPORT_ISSUE_DESCRIPTION_MAX_LENGTH }} + REPORT_ISSUE_CONTENT_MAX_LENGTH: ${{ vars.REPORT_ISSUE_CONTENT_MAX_LENGTH }} + RATE_LIMIT_REPORT_ISSUE_POST: ${{ vars.RATE_LIMIT_REPORT_ISSUE_POST }} + PLATFORM_FEEDBACK_TRIAGE_WINDOW_MINUTES: ${{ vars.PLATFORM_FEEDBACK_TRIAGE_WINDOW_MINUTES }} + PLATFORM_FEEDBACK_TRIAGE_ERROR_LIMIT: ${{ vars.PLATFORM_FEEDBACK_TRIAGE_ERROR_LIMIT }} + PLATFORM_FEEDBACK_TRIAGE_GROUP_LIMIT: ${{ vars.PLATFORM_FEEDBACK_TRIAGE_GROUP_LIMIT }} + PLATFORM_FEEDBACK_TRIAGE_EVIDENCE_LIMIT: ${{ vars.PLATFORM_FEEDBACK_TRIAGE_EVIDENCE_LIMIT }} + PLATFORM_FEEDBACK_TRIAGE_CLAIM_TTL_MS: ${{ vars.PLATFORM_FEEDBACK_TRIAGE_CLAIM_TTL_MS }} ARTIFACTS_BINDING_ENABLED: ${{ vars.ARTIFACTS_BINDING_ENABLED }} DO_MIGRATION_STATE_PROBE_ATTEMPTS: ${{ vars.DO_MIGRATION_STATE_PROBE_ATTEMPTS }} DO_MIGRATION_STATE_PROBE_RETRY_DELAY_MS: ${{ vars.DO_MIGRATION_STATE_PROBE_RETRY_DELAY_MS }} + CF_CONTAINER_ENABLED: ${{ vars.CF_CONTAINER_ENABLED }} + CF_CONTAINER_SLEEP_AFTER: ${{ vars.CF_CONTAINER_SLEEP_AFTER }} + CF_CONTAINER_ACTIVE_WORK_MAX_MS: ${{ vars.CF_CONTAINER_ACTIVE_WORK_MAX_MS }} + CF_CONTAINER_KEEPALIVE_RENEW_INTERVAL_MS: ${{ vars.CF_CONTAINER_KEEPALIVE_RENEW_INTERVAL_MS }} + CF_CONTAINER_PORT_READY_TIMEOUT_MS: ${{ vars.CF_CONTAINER_PORT_READY_TIMEOUT_MS }} + CF_CONTAINER_WAKE_TIMEOUT_MS: ${{ vars.CF_CONTAINER_WAKE_TIMEOUT_MS }} + CF_CONTAINER_RECOVERY_MAX_ATTEMPTS: ${{ vars.CF_CONTAINER_RECOVERY_MAX_ATTEMPTS }} + CF_CONTAINER_CREATE_WORKSPACE_TIMEOUT_MS: ${{ vars.CF_CONTAINER_CREATE_WORKSPACE_TIMEOUT_MS }} + CF_CONTAINER_CLONE_FILTER: ${{ vars.CF_CONTAINER_CLONE_FILTER }} + CF_CONTAINER_VM_AGENT_PORT: ${{ vars.CF_CONTAINER_VM_AGENT_PORT }} + SANDBOX_CONTAINER_MAX_INSTANCES: ${{ vars.SANDBOX_CONTAINER_MAX_INSTANCES }} + VM_AGENT_CONTAINER_MAX_INSTANCES: ${{ vars.VM_AGENT_CONTAINER_MAX_INSTANCES }} + SANDBOX_ENABLED: ${{ vars.SANDBOX_ENABLED }} + MAX_CONCURRENT_SETUP_SESSIONS: ${{ vars.MAX_CONCURRENT_SETUP_SESSIONS }} + SETUP_SESSION_TTL_MS: ${{ vars.SETUP_SESSION_TTL_MS }} + SETUP_SESSION_CAPTURE_POLL_MS: ${{ vars.SETUP_SESSION_CAPTURE_POLL_MS }} + CODEX_DEVICE_AUTH_REQUEST_TIMEOUT_MS: ${{ vars.CODEX_DEVICE_AUTH_REQUEST_TIMEOUT_MS }} + SETUP_SESSION_SWEEP_MAX_CANDIDATES: ${{ vars.SETUP_SESSION_SWEEP_MAX_CANDIDATES }} + POOL_LEASE_BUFFER_MS: ${{ vars.POOL_LEASE_BUFFER_MS }} + SANDBOX_EXEC_TIMEOUT_MS: ${{ vars.SANDBOX_EXEC_TIMEOUT_MS }} + SANDBOX_VM_AGENT_PORT: ${{ vars.SANDBOX_VM_AGENT_PORT }} - name: Re-deploy API Worker (with tail_consumers) if: ${{ inputs.dry_run != true && steps.first_deploy.outputs.is_first == 'true' }} diff --git a/apps/api/src/db/migrations/0105_bootstrap_token_consumes.sql b/apps/api/src/db/migrations/0105_bootstrap_token_consumes.sql new file mode 100644 index 000000000..b0b551783 --- /dev/null +++ b/apps/api/src/db/migrations/0105_bootstrap_token_consumes.sql @@ -0,0 +1,8 @@ +CREATE TABLE IF NOT EXISTS bootstrap_token_consumes ( + token_hash TEXT PRIMARY KEY NOT NULL, + created_at INTEGER NOT NULL, + expires_at INTEGER NOT NULL, + consumed_at INTEGER +); +CREATE INDEX IF NOT EXISTS idx_bootstrap_token_consumes_expiry ON bootstrap_token_consumes(expires_at); +CREATE INDEX IF NOT EXISTS idx_bootstrap_token_consumes_consumed ON bootstrap_token_consumes(consumed_at); diff --git a/apps/api/src/durable-objects/project-data/messages.ts b/apps/api/src/durable-objects/project-data/messages.ts index de92f08e7..96ce0e8af 100644 --- a/apps/api/src/durable-objects/project-data/messages.ts +++ b/apps/api/src/durable-objects/project-data/messages.ts @@ -406,10 +406,39 @@ export function getMessages( const trimmedRows = candidateRows.slice(0, safeCount); const orderedRows = order === 'desc' ? trimmedRows.reverse() : trimmedRows; + const messages: Record[] = []; + let skipped = 0; + + for (const row of orderedRows) { + try { + messages.push( + compact ? parseChatMessageRowCompact(row, compactOptions) : parseChatMessageRow(row) + ); + } catch (e) { + skipped++; + log.warn('messages.list_row_skipped', { + rowId: typeof row.id === 'string' ? row.id : null, + rowSessionId: typeof row.session_id === 'string' ? row.session_id : null, + requestedSessionId: sessionId, + compact, + error: String(e), + }); + } + } + + if (skipped > 0) { + log.warn('messages.list_degraded', { + sessionId, + requestedLimit: limit, + fetched: rows.length, + returned: messages.length, + skipped, + compact, + }); + } + return { - messages: orderedRows.map((row) => - compact ? parseChatMessageRowCompact(row, compactOptions) : parseChatMessageRow(row) - ), + messages, hasMore, }; } diff --git a/apps/api/src/durable-objects/task-runner/workspace-steps.ts b/apps/api/src/durable-objects/task-runner/workspace-steps.ts index 7edbf2645..4ebb43a4c 100644 --- a/apps/api/src/durable-objects/task-runner/workspace-steps.ts +++ b/apps/api/src/durable-objects/task-runner/workspace-steps.ts @@ -404,15 +404,15 @@ async function createWorkspaceOnVmAgent( id: state.projectId, repoProvider: projectRepo?.repoProvider ?? 'github', }); + const checkoutBranch = state.config.outputBranch || state.config.branch; + const baseBranch = + checkoutBranch === state.config.branch ? state.config.defaultBranch : state.config.branch; const response = await createWorkspaceOnNode(nodeId, rc.env, state.userId, { workspaceId, repository: state.config.repository, - branch: state.config.branch, - baseBranch: - state.config.branch === state.config.outputBranch - ? state.config.defaultBranch - : state.config.branch, + branch: checkoutBranch, + baseBranch, defaultBranch: state.config.defaultBranch || 'main', repoProvider: gitSource.repoProvider, cloneUrl: gitSource.cloneUrl, diff --git a/apps/api/src/routes/workspaces/runtime.ts b/apps/api/src/routes/workspaces/runtime.ts index 020ece641..fa73d6bb8 100644 --- a/apps/api/src/routes/workspaces/runtime.ts +++ b/apps/api/src/routes/workspaces/runtime.ts @@ -33,6 +33,7 @@ import { MessageBatchSchema, } from '../../schemas'; import { appendBootLog } from '../../services/boot-log'; +import { registerBootstrapTokenConsume } from '../../services/bootstrap'; import { syncActiveAgentCredentialSecret } from '../../services/composable-credentials/agent-sync'; import { decrypt, encrypt } from '../../services/encryption'; import { getInstallationToken, getUserInstallationRepositories } from '../../services/github-app'; @@ -1568,6 +1569,12 @@ runtimeRoutes.post('/:id/bootstrap-token', requireAuth(), requireApproved(), asy createdAt: now, }; + await registerBootstrapTokenConsume( + c.env.DATABASE, + bootstrapToken, + new Date(Date.now() + 60 * 1000).toISOString() + ); + await c.env.KV.put(`bootstrap:${bootstrapToken}`, JSON.stringify(data), { expirationTtl: 60, }); diff --git a/apps/api/src/services/bootstrap.ts b/apps/api/src/services/bootstrap.ts index 835173b10..02251c375 100644 --- a/apps/api/src/services/bootstrap.ts +++ b/apps/api/src/services/bootstrap.ts @@ -2,7 +2,7 @@ * Bootstrap Token Service * * Manages one-time bootstrap tokens for secure credential delivery to VMs. - * Tokens are stored in KV with a 15-minute TTL and are deleted after single use. + * Tokens are stored in KV with a configurable TTL and are deleted after single use. */ import type { BootstrapTokenData } from '@simple-agent-manager/shared'; @@ -13,9 +13,8 @@ import { decrypt, encrypt } from './encryption'; /** KV key prefix for bootstrap tokens */ const BOOTSTRAP_PREFIX = 'bootstrap:'; -const inFlightRedemptions = new Map>(); -/** Default bootstrap token TTL in seconds (15 minutes) */ +/** Default bootstrap token TTL in seconds. */ const DEFAULT_BOOTSTRAP_TTL = 900; const LEGACY_CLOCK_SKEW_SECONDS = 60; @@ -23,6 +22,7 @@ interface BootstrapEnv { BOOTSTRAP_TOKEN_TTL_SECONDS?: string; ENCRYPTION_KEY: string; CREDENTIAL_ENCRYPTION_KEY?: string; + DATABASE: D1Database; } /** Get bootstrap TTL from env or use default (per constitution principle XI) */ @@ -74,6 +74,8 @@ export async function storeBootstrapToken( callbackTokenIv: encryptedCallbackToken.iv, }; + await registerBootstrapTokenConsume(env.DATABASE, token, nowPlusSeconds(ttl)); + await kv.put(`${BOOTSTRAP_PREFIX}${token}`, JSON.stringify(storedData), { expirationTtl: ttl, }); @@ -94,61 +96,155 @@ export async function redeemBootstrapToken( env: BootstrapEnv ): Promise { const key = `${BOOTSTRAP_PREFIX}${token}`; - const existing = inFlightRedemptions.get(key); - if (existing) { + const consumeState = await reserveBootstrapTokenConsume(env.DATABASE, token); + if (consumeState === 'rejected') { return null; } - const redemption = (async () => { - const data = await kv.get(key, { type: 'json' }); + const data = await kv.get(key, { type: 'json' }); + if (!data) { + return null; + } - if (!data) { + if (consumeState === 'legacy-claim-required') { + const claimed = await claimLegacyBootstrapTokenConsume(env.DATABASE, token, env); + if (!claimed) { return null; } + } - // Delete immediately to enforce single-use before decrypting or returning credentials. - await kv.delete(key); + // Delete after the D1 consume decision. If payload handling later fails, the D1 + // row remains consumed so ambiguous/failed redemption is fail-closed. + await kv.delete(key); - if (data.encryptedCallbackToken && data.callbackTokenIv) { - const callbackToken = await decrypt( - data.encryptedCallbackToken, - data.callbackTokenIv, - getCredentialEncryptionKey(env) - ); + if (data.encryptedCallbackToken && data.callbackTokenIv) { + const callbackToken = await decrypt( + data.encryptedCallbackToken, + data.callbackTokenIv, + getCredentialEncryptionKey(env) + ); - return { - ...data, - callbackToken, - }; - } + return { + ...data, + callbackToken, + }; + } - // Backward compatibility for bootstrap entries written before callback token encryption. - // This is intentionally bounded to entries still inside the configured bootstrap TTL - // plus a small clock-skew allowance; older plaintext records fail closed. - if (data.callbackToken && isLegacyPlaintextCallbackTokenStillRedeemable(data, env)) { - log.warn('bootstrap.legacy_plaintext_callback_token_redeemed', { - workspaceId: data.workspaceId, - createdAt: data.createdAt, - }); - return data; - } + // Backward compatibility for bootstrap entries written before callback token encryption. + // This is intentionally bounded to entries still inside the configured bootstrap TTL + // plus a small clock-skew allowance; older plaintext records fail closed. + if (data.callbackToken && isLegacyPlaintextCallbackTokenStillRedeemable(data, env)) { + log.warn('bootstrap.legacy_plaintext_callback_token_redeemed', { + workspaceId: data.workspaceId, + createdAt: data.createdAt, + }); + return data; + } - if (data.callbackToken) { - log.warn('bootstrap.legacy_plaintext_callback_token_rejected', { - workspaceId: data.workspaceId, - createdAt: data.createdAt, - }); - } + if (data.callbackToken) { + log.warn('bootstrap.legacy_plaintext_callback_token_rejected', { + workspaceId: data.workspaceId, + createdAt: data.createdAt, + }); + } + + throw new Error('Bootstrap token data is missing callback token material'); +} + +export async function registerBootstrapTokenConsume( + db: D1Database, + token: string, + expiresAt: string +): Promise { + await db + .prepare( + `INSERT INTO bootstrap_token_consumes (token_hash, created_at, expires_at) + VALUES (?, ?, ?)` + ) + .bind(await hashBootstrapToken(token), Date.now(), parseBootstrapExpiry(expiresAt)) + .run(); +} + +type BootstrapConsumeState = 'consumed' | 'legacy-claim-required' | 'rejected'; + +async function reserveBootstrapTokenConsume( + db: D1Database, + token: string +): Promise { + const now = Date.now(); + const tokenHash = await hashBootstrapToken(token); + + const updated = await db + .prepare( + `UPDATE bootstrap_token_consumes + SET consumed_at = ? + WHERE token_hash = ? + AND consumed_at IS NULL + AND expires_at > ?` + ) + .bind(now, tokenHash, now) + .run(); - throw new Error('Bootstrap token data is missing callback token material'); - })(); + if (d1Changes(updated) === 1) { + return 'consumed'; + } + + const existing = await db + .prepare('SELECT token_hash FROM bootstrap_token_consumes WHERE token_hash = ?') + .bind(tokenHash) + .first('token_hash'); + + return existing ? 'rejected' : 'legacy-claim-required'; +} + +async function claimLegacyBootstrapTokenConsume( + db: D1Database, + token: string, + env: Pick +): Promise { + const now = Date.now(); + + // Migration-safe compatibility for unexpired tokens that were written to KV + // before the D1 ledger existed, or direct legacy producers that only wrote KV. + // The unique token_hash insert is the atomic cross-isolate claim: one request + // can create the consumed row, all duplicates fail closed. + const legacyClaim = await db + .prepare( + `INSERT OR IGNORE INTO bootstrap_token_consumes (token_hash, created_at, expires_at, consumed_at) + VALUES (?, ?, ?, ?)` + ) + .bind(await hashBootstrapToken(token), now, nowPlusSecondsMs(getBootstrapTTL(env)), now) + .run(); + + return d1Changes(legacyClaim) === 1; +} + +function d1Changes(result: D1Result): number { + return typeof result.meta?.changes === 'number' ? result.meta.changes : 0; +} + +async function hashBootstrapToken(token: string): Promise { + const bytes = new TextEncoder().encode(token); + const digest = await crypto.subtle.digest('SHA-256', bytes); + return [...new Uint8Array(digest)] + .map((byte) => byte.toString(16).padStart(2, '0')) + .join(''); +} + +function nowPlusSeconds(seconds: number): string { + return new Date(nowPlusSecondsMs(seconds)).toISOString(); +} + +function nowPlusSecondsMs(seconds: number): number { + return Date.now() + seconds * 1000; +} - inFlightRedemptions.set(key, redemption); - try { - return await redemption; - } finally { - inFlightRedemptions.delete(key); +function parseBootstrapExpiry(expiresAt: string): number { + const parsed = Date.parse(expiresAt); + if (!Number.isFinite(parsed)) { + throw new Error('Invalid bootstrap token expiry'); } + return parsed; } function isLegacyPlaintextCallbackTokenStillRedeemable( diff --git a/apps/api/tests/unit/durable-objects/project-data-messages.test.ts b/apps/api/tests/unit/durable-objects/project-data-messages.test.ts index 3137d6a88..b29697991 100644 --- a/apps/api/tests/unit/durable-objects/project-data-messages.test.ts +++ b/apps/api/tests/unit/durable-objects/project-data-messages.test.ts @@ -1,6 +1,7 @@ -import { describe, expect, it, vi } from 'vitest'; +import { afterEach, describe, expect, it, vi } from 'vitest'; import { getMessages } from '../../../src/durable-objects/project-data/messages'; +import { log } from '../../../src/lib/logger'; type QueryRow = Record; @@ -26,6 +27,10 @@ function makeSql(rows: QueryRow[]) { } describe('ProjectData messages getMessages', () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + it('keeps newest-page default behavior ordered chronologically for rendering', () => { const newest = makeRow({ id: 'newest', content: 'Newest', created_at: 3000, sequence: 3 }); const older = makeRow({ id: 'older', content: 'Older', created_at: 2000, sequence: 2 }); @@ -38,6 +43,70 @@ describe('ProjectData messages getMessages', () => { expect(result.hasMore).toBe(false); }); + it('skips a malformed message row among valid rows instead of throwing', () => { + const warn = vi.spyOn(log, 'warn').mockImplementation(() => {}); + const newest = makeRow({ id: 'newest', content: 'Newest', created_at: 3000, sequence: 3 }); + const bad = makeRow({ id: 'bad', content: null, created_at: 2000, sequence: 2 }); + const oldest = makeRow({ id: 'oldest', content: 'Oldest', created_at: 1000, sequence: 1 }); + const sql = makeSql([newest, bad, oldest]); + + expect(() => getMessages(sql, 'session-1', 3)).not.toThrow(); + + const result = getMessages(sql, 'session-1', 3); + expect(result.messages.map((message) => message.id)).toEqual(['oldest', 'newest']); + expect(result.hasMore).toBe(false); + expect(warn).toHaveBeenCalledWith( + 'messages.list_row_skipped', + expect.objectContaining({ + rowId: 'bad', + rowSessionId: 'session-1', + requestedSessionId: 'session-1', + compact: false, + error: expect.stringContaining('content'), + }) + ); + expect(warn).toHaveBeenCalledWith( + 'messages.list_degraded', + expect.objectContaining({ returned: 2, skipped: 1 }) + ); + }); + + it('skips malformed compact message rows without failing the compact list', () => { + const warn = vi.spyOn(log, 'warn').mockImplementation(() => {}); + const good = makeRow({ id: 'good', content: 'Good compact content' }); + const bad = makeRow({ id: 'bad-compact', content: null }); + const sql = makeSql([good, bad]); + + const result = getMessages(sql, 'session-1', 2, null, undefined, true); + + expect(result.messages.map((message) => message.id)).toEqual(['good']); + expect(warn).toHaveBeenCalledWith( + 'messages.list_row_skipped', + expect.objectContaining({ + rowId: 'bad-compact', + compact: true, + error: expect.stringContaining('content'), + }) + ); + }); + + it('returns an empty non-throwing list when every message row is malformed', () => { + const warn = vi.spyOn(log, 'warn').mockImplementation(() => {}); + const sql = makeSql([ + makeRow({ id: 'bad-1', content: null }), + makeRow({ id: 'bad-2', role: null }), + ]); + + const result = getMessages(sql, 'session-1', 2); + + expect(result.messages).toEqual([]); + expect(result.hasMore).toBe(false); + expect(warn).toHaveBeenCalledWith( + 'messages.list_degraded', + expect.objectContaining({ returned: 0, skipped: 2 }) + ); + }); + it('supports oldest-first lookups for the initial user prompt', () => { const initialPrompt = makeRow({ id: 'initial', content: 'Initial prompt', created_at: 1000, sequence: 1 }); const followUp = makeRow({ id: 'follow-up', content: 'Follow-up prompt', created_at: 3000, sequence: 3 }); diff --git a/apps/api/tests/unit/durable-objects/task-runner-workspace-branch-dispatch.test.ts b/apps/api/tests/unit/durable-objects/task-runner-workspace-branch-dispatch.test.ts index 948d6d162..6f68cfbeb 100644 --- a/apps/api/tests/unit/durable-objects/task-runner-workspace-branch-dispatch.test.ts +++ b/apps/api/tests/unit/durable-objects/task-runner-workspace-branch-dispatch.test.ts @@ -89,22 +89,31 @@ describe('TaskRunner workspace branch dispatch', () => { }); it.each([ - [ - 'generated output branch', - 'sam/generated-output-abc123', - 'sam/generated-output-abc123', - 'main', - ], - [ - 'explicit continuation branch', - 'feature/existing-work', - 'sam/continuation-output-def456', - 'feature/existing-work', - ], + { + scenario: 'generated output branch', + configuredBranch: 'sam/generated-output-abc123', + outputBranch: 'sam/generated-output-abc123', + checkoutBranch: 'sam/generated-output-abc123', + baseBranch: 'main', + }, + { + scenario: 'explicit non-default branch with separate output branch', + configuredBranch: 'feature/existing-work', + outputBranch: 'sam/continuation-output-def456', + checkoutBranch: 'sam/continuation-output-def456', + baseBranch: 'feature/existing-work', + }, + { + scenario: 'explicit default branch with separate output branch', + configuredBranch: 'main', + outputBranch: 'sam/default-safe-output-ghi789', + checkoutBranch: 'sam/default-safe-output-ghi789', + baseBranch: 'main', + }, ])( - 'sends the %s in the outbound create-workspace payload', - async (_scenario, branch, outputBranch, baseBranch) => { - await handleWorkspaceDispatch(makeState(branch, outputBranch), makeContext()); + 'checks out the task output branch for $scenario', + async ({ configuredBranch, outputBranch, checkoutBranch, baseBranch }) => { + await handleWorkspaceDispatch(makeState(configuredBranch, outputBranch), makeContext()); expect(mocks.createWorkspaceOnNode).toHaveBeenCalledOnce(); expect(mocks.createWorkspaceOnNode).toHaveBeenCalledWith( 'node-1', @@ -112,7 +121,7 @@ describe('TaskRunner workspace branch dispatch', () => { 'user-1', expect.objectContaining({ workspaceId: 'workspace-1', - branch, + branch: checkoutBranch, baseBranch, defaultBranch: 'main', }) diff --git a/apps/api/tests/unit/routes/bootstrap.test.ts b/apps/api/tests/unit/routes/bootstrap.test.ts index ebe03a776..09f70f202 100644 --- a/apps/api/tests/unit/routes/bootstrap.test.ts +++ b/apps/api/tests/unit/routes/bootstrap.test.ts @@ -1,6 +1,9 @@ import type { BootstrapResponse, BootstrapTokenData } from '@simple-agent-manager/shared'; +import Database from 'better-sqlite3'; import { Hono } from 'hono'; -import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { createSqliteD1 } from '../../helpers/sqlite-d1'; // Mock rate-limit middleware to be a passthrough (tested separately) vi.mock('../../../src/middleware/rate-limit', () => ({ @@ -18,14 +21,34 @@ const mockKV = { // Mock environment const mockEnv = { KV: mockKV, - DATABASE: {}, + DATABASE: undefined as unknown as D1Database, ENCRYPTION_KEY: 'iZEI8rg5FHtTo2yvt6Qw3m4z6aTfqj5MdLEGqOvdqw0=', // Valid 32-byte base64 key BASE_DOMAIN: 'workspaces.example.com', }; +let sqlite: Database.Database; + +function installBootstrapLedger(db: Database.Database): void { + db.exec(` + CREATE TABLE bootstrap_token_consumes ( + token_hash TEXT PRIMARY KEY NOT NULL, + created_at INTEGER NOT NULL, + expires_at INTEGER NOT NULL, + consumed_at INTEGER + ); + `); +} + describe('Bootstrap Routes', () => { beforeEach(() => { - vi.clearAllMocks(); + vi.resetAllMocks(); + sqlite = new Database(':memory:'); + installBootstrapLedger(sqlite); + mockEnv.DATABASE = createSqliteD1(sqlite); + }); + + afterEach(() => { + sqlite.close(); }); describe('POST /api/bootstrap/:token', () => { @@ -193,7 +216,7 @@ describe('Bootstrap Routes', () => { expect(body.gitUserEmail).toBeNull(); }); - it('rejects concurrent replay while first redemption is in flight', async () => { + it('rejects concurrent replay across requests using the D1 consume ledger', async () => { const { bootstrapRoutes } = await import('../../../src/routes/bootstrap'); const { encrypt } = await import('../../../src/services/encryption'); @@ -213,14 +236,10 @@ describe('Bootstrap Routes', () => { createdAt: new Date().toISOString(), }; - let releaseGet!: () => void; - mockKV.get.mockReturnValueOnce(new Promise((resolve) => { - releaseGet = () => resolve(tokenData); - })); + mockKV.get.mockResolvedValue(tokenData); const first = app.request('/api/bootstrap/concurrent-token', { method: 'POST' }, mockEnv); const second = app.request('/api/bootstrap/concurrent-token', { method: 'POST' }, mockEnv); - releaseGet(); const [res1, res2] = await Promise.all([first, second]); expect([res1.status, res2.status].sort()).toEqual([200, 401]); diff --git a/apps/api/tests/unit/services/bootstrap-callback-encryption.test.ts b/apps/api/tests/unit/services/bootstrap-callback-encryption.test.ts index 17260dffa..da4d94468 100644 --- a/apps/api/tests/unit/services/bootstrap-callback-encryption.test.ts +++ b/apps/api/tests/unit/services/bootstrap-callback-encryption.test.ts @@ -5,8 +5,11 @@ * to verify encrypted callbackToken decryption works end-to-end. */ import type { BootstrapResponse, BootstrapTokenData } from '@simple-agent-manager/shared'; +import Database from 'better-sqlite3'; import { Hono } from 'hono'; -import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { createSqliteD1 } from '../../helpers/sqlite-d1'; vi.mock('../../../src/middleware/rate-limit', () => { const continueRequest = async (_c: unknown, next: () => Promise) => next(); @@ -25,14 +28,28 @@ type KvMock = { const TEST_ENCRYPTION_KEY = 'iZEI8rg5FHtTo2yvt6Qw3m4z6aTfqj5MdLEGqOvdqw0='; let kv: KvMock; +let sqlite: Database.Database; let env: { KV: KvMock; - DATABASE: Record; + DATABASE: D1Database; ENCRYPTION_KEY: string; BASE_DOMAIN: string; }; +function installBootstrapLedger(db: Database.Database): void { + db.exec(` + CREATE TABLE bootstrap_token_consumes ( + token_hash TEXT PRIMARY KEY NOT NULL, + created_at INTEGER NOT NULL, + expires_at INTEGER NOT NULL, + consumed_at INTEGER + ); + `); +} + function resetBootstrapHarness() { + sqlite = new Database(':memory:'); + installBootstrapLedger(sqlite); kv = { put: vi.fn(), get: vi.fn(), @@ -40,7 +57,7 @@ function resetBootstrapHarness() { }; env = { KV: kv, - DATABASE: {}, + DATABASE: createSqliteD1(sqlite), ENCRYPTION_KEY: TEST_ENCRYPTION_KEY, BASE_DOMAIN: 'workspaces.example.com', }; @@ -55,10 +72,14 @@ async function requestBootstrapToken(token: string) { describe('Bootstrap Callback Token Encryption (F-004)', () => { beforeEach(() => { - vi.clearAllMocks(); + vi.resetAllMocks(); resetBootstrapHarness(); }); + afterEach(() => { + sqlite.close(); + }); + it('decrypts encryptedCallbackToken via the bootstrap route', async () => { const { encrypt } = await import('../../../src/services/encryption'); diff --git a/apps/api/tests/unit/services/bootstrap.test.ts b/apps/api/tests/unit/services/bootstrap.test.ts index 6e8a014bf..8fd552cd5 100644 --- a/apps/api/tests/unit/services/bootstrap.test.ts +++ b/apps/api/tests/unit/services/bootstrap.test.ts @@ -1,5 +1,8 @@ import type { BootstrapTokenData } from '@simple-agent-manager/shared'; -import { beforeEach, describe, expect, it, vi } from 'vitest'; +import Database from 'better-sqlite3'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { createSqliteD1 } from '../../helpers/sqlite-d1'; // Mock KV namespace const mockKV = { @@ -10,11 +13,37 @@ const mockKV = { const mockEnv = { ENCRYPTION_KEY: 'iZEI8rg5FHtTo2yvt6Qw3m4z6aTfqj5MdLEGqOvdqw0=', + DATABASE: undefined as unknown as D1Database, }; +let sqlite: Database.Database; + +function installBootstrapLedger(db: Database.Database): void { + db.exec(` + CREATE TABLE bootstrap_token_consumes ( + token_hash TEXT PRIMARY KEY NOT NULL, + created_at INTEGER NOT NULL, + expires_at INTEGER NOT NULL, + consumed_at INTEGER + ); + `); +} + +async function tokenHash(token: string): Promise { + const digest = await crypto.subtle.digest('SHA-256', new TextEncoder().encode(token)); + return [...new Uint8Array(digest)].map((byte) => byte.toString(16).padStart(2, '0')).join(''); +} + describe('Bootstrap Service', () => { beforeEach(() => { - vi.clearAllMocks(); + vi.resetAllMocks(); + sqlite = new Database(':memory:'); + installBootstrapLedger(sqlite); + mockEnv.DATABASE = createSqliteD1(sqlite); + }); + + afterEach(() => { + sqlite.close(); }); describe('generateBootstrapToken', () => { @@ -45,7 +74,7 @@ describe('Bootstrap Service', () => { }); describe('storeBootstrapToken', () => { - it('should store token data in KV with 15-minute TTL', async () => { + it('should store token data in KV with the default TTL', async () => { const { storeBootstrapToken } = await import( '../../../src/services/bootstrap' ); @@ -79,7 +108,7 @@ describe('Bootstrap Service', () => { }); }); - describe('redeemBootstrapToken (get + delete for single-use)', () => { + describe('redeemBootstrapToken (D1 atomic consume + KV payload)', () => { it('should return null for non-existent token', async () => { const { redeemBootstrapToken } = await import( '../../../src/services/bootstrap' @@ -139,6 +168,134 @@ describe('Bootstrap Service', () => { // Token should be deleted after redemption (single-use) expect(mockKV.delete).toHaveBeenCalledWith('bootstrap:valid-token'); }); + + it('allows exactly one concurrent redemption across requests', async () => { + const { redeemBootstrapToken, registerBootstrapTokenConsume } = await import( + '../../../src/services/bootstrap' + ); + const { encrypt } = await import('../../../src/services/encryption'); + + const encryptedCallbackToken = await encrypt('jwt-callback-token', mockEnv.ENCRYPTION_KEY); + const data: BootstrapTokenData = { + workspaceId: 'ws-atomic', + encryptedHetznerToken: 'encrypted-hetzner', + hetznerTokenIv: 'hetzner-iv', + encryptedCallbackToken: encryptedCallbackToken.ciphertext, + callbackTokenIv: encryptedCallbackToken.iv, + encryptedGithubToken: null, + githubTokenIv: null, + createdAt: new Date().toISOString(), + }; + + await registerBootstrapTokenConsume( + mockEnv.DATABASE, + 'atomic-token', + new Date(Date.now() + 60_000).toISOString() + ); + mockKV.get.mockResolvedValue(data); + + const results = await Promise.all([ + redeemBootstrapToken(mockKV as unknown as KVNamespace, 'atomic-token', mockEnv), + redeemBootstrapToken(mockKV as unknown as KVNamespace, 'atomic-token', mockEnv), + ]); + + expect(results.filter(Boolean)).toHaveLength(1); + expect(results.filter((result) => result === null)).toHaveLength(1); + expect(mockKV.delete).toHaveBeenCalledTimes(1); + }); + + it('rejects replay after a successful registered-token consume', async () => { + const { redeemBootstrapToken, registerBootstrapTokenConsume } = await import( + '../../../src/services/bootstrap' + ); + const { encrypt } = await import('../../../src/services/encryption'); + + const encryptedCallbackToken = await encrypt('jwt-callback-token', mockEnv.ENCRYPTION_KEY); + const data: BootstrapTokenData = { + workspaceId: 'ws-once', + encryptedHetznerToken: 'encrypted-hetzner', + hetznerTokenIv: 'hetzner-iv', + encryptedCallbackToken: encryptedCallbackToken.ciphertext, + callbackTokenIv: encryptedCallbackToken.iv, + encryptedGithubToken: null, + githubTokenIv: null, + createdAt: new Date().toISOString(), + }; + + await registerBootstrapTokenConsume( + mockEnv.DATABASE, + 'single-use-registered', + new Date(Date.now() + 60_000).toISOString() + ); + mockKV.get.mockResolvedValueOnce(data); + + const first = await redeemBootstrapToken( + mockKV as unknown as KVNamespace, + 'single-use-registered', + mockEnv + ); + const second = await redeemBootstrapToken( + mockKV as unknown as KVNamespace, + 'single-use-registered', + mockEnv + ); + + expect(first?.workspaceId).toBe('ws-once'); + expect(second).toBeNull(); + expect(mockKV.get).toHaveBeenCalledTimes(1); + }); + + it('fails closed for expired registered tokens without reading KV', async () => { + const { redeemBootstrapToken, registerBootstrapTokenConsume } = await import( + '../../../src/services/bootstrap' + ); + + await registerBootstrapTokenConsume( + mockEnv.DATABASE, + 'expired-registered', + new Date(Date.now() - 1_000).toISOString() + ); + + const result = await redeemBootstrapToken( + mockKV as unknown as KVNamespace, + 'expired-registered', + mockEnv + ); + + expect(result).toBeNull(); + expect(mockKV.get).not.toHaveBeenCalled(); + expect(mockKV.delete).not.toHaveBeenCalled(); + }); + + it('allows one KV-only legacy token redemption through atomic insert-wins claim', async () => { + const { redeemBootstrapToken } = await import('../../../src/services/bootstrap'); + + const data: BootstrapTokenData = { + workspaceId: 'ws-legacy', + encryptedHetznerToken: 'encrypted-hetzner', + hetznerTokenIv: 'hetzner-iv', + callbackToken: 'legacy-callback-token', + encryptedGithubToken: null, + githubTokenIv: null, + createdAt: new Date().toISOString(), + }; + mockKV.get.mockResolvedValue(data); + + const results = await Promise.all([ + redeemBootstrapToken(mockKV as unknown as KVNamespace, 'legacy-kv-only', mockEnv), + redeemBootstrapToken(mockKV as unknown as KVNamespace, 'legacy-kv-only', mockEnv), + ]); + + expect(results.filter(Boolean)).toHaveLength(1); + expect(results.find(Boolean)?.callbackToken).toBe('legacy-callback-token'); + expect(mockKV.delete).toHaveBeenCalledTimes(1); + + const rows = sqlite.prepare('SELECT * FROM bootstrap_token_consumes WHERE token_hash = ?').all( + await tokenHash('legacy-kv-only') + ); + expect(rows).toHaveLength(1); + expect(rows[0]).toMatchObject({ consumed_at: expect.any(Number) }); + }); }); describe('Token expiry (KV TTL)', () => { @@ -167,7 +324,7 @@ describe('Bootstrap Service', () => { mockEnv ); - // Verify TTL is set to 900 seconds (15 minutes) + // Verify the default TTL is used when no override is configured expect(mockKV.put).toHaveBeenCalledWith( expect.any(String), expect.any(String), diff --git a/apps/api/wrangler.toml b/apps/api/wrangler.toml index 154935c7f..e60b61562 100644 --- a/apps/api/wrangler.toml +++ b/apps/api/wrangler.toml @@ -356,6 +356,7 @@ crons = ["*/5 * * * *", "30 * * * *", "0 3 * * *", "0 4 * * *", "0 5 1 * *"] # - GITHUB_CLIENT_SECRET # - GITHUB_APP_ID # - GITHUB_APP_PRIVATE_KEY +# - GITHUB_APP_SLUG # - CF_API_TOKEN # - CF_ZONE_ID # - CF_ACCOUNT_ID (required for admin observability log viewer) @@ -365,8 +366,11 @@ crons = ["*/5 * * * *", "30 * * * *", "0 3 * * *", "0 4 * * *", "0 5 1 * *"] # - BETTER_AUTH_SECRET (optional — overrides ENCRYPTION_KEY for BetterAuth sessions) # - CREDENTIAL_ENCRYPTION_KEY (optional — overrides ENCRYPTION_KEY for AES-GCM credential encryption) # - GITHUB_WEBHOOK_SECRET (optional — overrides ENCRYPTION_KEY for GitHub webhook HMAC) -# - ORIGIN_CA_CERT (Origin CA certificate for VM agent TLS) -# - ORIGIN_CA_KEY (Origin CA private key for VM agent TLS) +# - DEPLOY_SIGNING_PRIVATE_KEY (Ed25519 signing key for deployment apply payloads) +# - DEPLOY_SIGNING_PUBLIC_KEY (Ed25519 verification key for deployment apply payloads) +# - PREVIEW_SIGNING_KEY (HMAC signing key for interactive preview URLs) +# - DEVCONTAINER_CACHE_CLOUDFLARE_API_TOKEN (optional — narrower token for managed devcontainer registry credentials) +# - DEVCONTAINER_CACHE_CLOUDFLARE_ACCOUNT_ID (optional — account override for managed devcontainer registry credentials) # - GOOGLE_CLIENT_ID (optional — Google Cloud Console OAuth client ID for infra/GCP OIDC integration) # - GOOGLE_CLIENT_SECRET (optional — Google Cloud Console OAuth client secret for infra/GCP) # - GOOGLE_LOGIN_CLIENT_ID (optional — Google login OAuth client ID for "Sign in with Google"; separate client, or set via /setup) @@ -376,7 +380,10 @@ crons = ["*/5 * * * *", "30 * * * *", "0 3 * * *", "0 4 * * *", "0 5 1 * *"] # - GITLAB_CLIENT_SECRET (optional — GitLab OAuth secret; or set via /setup) # - SEGMENT_WRITE_KEY (optional — Segment.io write key, enables Segment event forwarding) # - GA4_API_SECRET (optional — Google Analytics 4 API secret, enables GA4 forwarding; NOTE: GA4 Measurement Protocol requires api_secret as a query parameter — ensure outbound request URLs are not logged) +# - GA4_MEASUREMENT_ID (optional — Google Analytics 4 measurement ID, paired with GA4_API_SECRET) # - R2_ACCESS_KEY_ID (optional — R2 S3-compatible API token key ID, enables task attachment presigned uploads) # - R2_SECRET_ACCESS_KEY (optional — R2 S3-compatible API token secret, enables task attachment presigned uploads) # - TRIAL_CLAIM_TOKEN_SECRET (required when trials are enabled — HMAC secret for sam_trial_claim / sam_trial_fingerprint cookies. 32+ bytes base64.) # - CF_AIG_TOKEN (optional — Cloudflare AI Gateway Unified Billing token; enables OpenAI/Anthropic models without separate API keys) +# - ANTHROPIC_API_KEY_TRIAL (optional — Anthropic API key for trial traffic when not using Workers AI) +# - SMOKE_TEST_AUTH_ENABLED (optional — enables smoke-test token auth for test environments) diff --git a/apps/www/src/content/docs/docs/architecture/security.md b/apps/www/src/content/docs/docs/architecture/security.md index 245b7bc48..8771ed467 100644 --- a/apps/www/src/content/docs/docs/architecture/security.md +++ b/apps/www/src/content/docs/docs/architecture/security.md @@ -15,7 +15,7 @@ However, SAM's own hosted deployment also has an **enabled platform-level cloud ### Platform Secrets -These are Cloudflare Worker secrets set during deployment: +These Cloudflare Worker secrets are generated or copied during deployment and are required for a fully functional install: | Secret | Purpose | | ---------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------- | @@ -28,8 +28,12 @@ These are Cloudflare Worker secrets set during deployment: | `DEPLOY_SIGNING_PUBLIC_KEY` | Ed25519 key for deployment-node payload verification (auto-generated) | | `TRIAL_CLAIM_TOKEN_SECRET` | HMAC secret for trial onboarding claim tokens (auto-generated) | | `CF_API_TOKEN` | Cloudflare deploy, DNS, Origin CA certificate issuance, observability, and AI Gateway operations (requires Account → SSL and Certificates → Edit) | +| `CF_ACCOUNT_ID` | Cloudflare account identifier used by account-scoped Cloudflare APIs | +| `CF_ZONE_ID` | Cloudflare zone identifier used for DNS and Origin CA operations | -Security keys are automatically generated and persisted by Pulumi on first deployment. Cloudflare secrets remain Worker secrets because they are deployment trust roots. GitHub App/OAuth, GitHub webhook, Google OAuth, and GitLab OAuth credentials can be supplied either as optional environment fallbacks or through the first-run/superadmin platform config UI; runtime values are stored encrypted in D1 and override environment fallbacks. They never appear in source control. +Security keys are automatically generated and persisted by Pulumi on first deployment. Cloudflare secrets remain Worker secrets because they are deployment trust roots. GitHub App/OAuth, GitHub webhook, Google OAuth, GitLab OAuth, analytics forwarding, R2 attachment-upload credentials, devcontainer cache credentials, trial provider keys, and smoke-test auth flags can be supplied as optional Worker secret fallbacks when an installation needs them. Runtime platform values saved through first-run setup or the superadmin platform config UI are stored encrypted in D1 and override environment fallbacks. They never appear in source control. + +New VM nodes do not require static `ORIGIN_CA_CERT` or `ORIGIN_CA_KEY` Worker secrets. If those legacy secrets exist from an older deployment, remove them after draining old nodes and confirming the per-node CSR model is deployed. ### Platform Integration Credentials diff --git a/apps/www/src/content/docs/docs/guides/self-hosting.mdx b/apps/www/src/content/docs/docs/guides/self-hosting.mdx index 5efa4420e..344578e47 100644 --- a/apps/www/src/content/docs/docs/guides/self-hosting.mdx +++ b/apps/www/src/content/docs/docs/guides/self-hosting.mdx @@ -176,7 +176,7 @@ without their own cloud credential, on [Instant sessions](/docs/guides/instant-s Setting it to `false` means every session provisions a cloud VM, so **each user must connect their own cloud provider credential before they can do anything**. -**Environment secrets:** +**GitHub Environment secrets:** | Secret | Description | | ------------------------------------------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | diff --git a/packages/vm-agent/internal/auth/session.go b/packages/vm-agent/internal/auth/session.go index af3752bfb..a2cd69833 100644 --- a/packages/vm-agent/internal/auth/session.go +++ b/packages/vm-agent/internal/auth/session.go @@ -32,6 +32,8 @@ type SessionManager struct { maxSessions int cookieDomain string // Domain for cookie sharing across subdomains (e.g., ".example.com") stopCleanup chan struct{} + cleanupDone chan struct{} + stopOnce sync.Once } // SessionManagerConfig holds configuration for the session manager. @@ -67,6 +69,7 @@ func NewSessionManagerWithConfig(cfg SessionManagerConfig) *SessionManager { maxSessions: cfg.MaxSessions, cookieDomain: cfg.CookieDomain, stopCleanup: make(chan struct{}), + cleanupDone: make(chan struct{}), } // Start cleanup goroutine @@ -262,6 +265,7 @@ func (sm *SessionManager) ClearCookie(w http.ResponseWriter) { func (sm *SessionManager) cleanup() { ticker := time.NewTicker(sm.cleanupInterval) defer ticker.Stop() + defer close(sm.cleanupDone) for { select { @@ -288,7 +292,10 @@ func (sm *SessionManager) cleanup() { // Stop stops the cleanup goroutine. func (sm *SessionManager) Stop() { - close(sm.stopCleanup) + sm.stopOnce.Do(func() { + close(sm.stopCleanup) + }) + <-sm.cleanupDone } // generateSessionID generates a random session ID. diff --git a/packages/vm-agent/internal/auth/session_test.go b/packages/vm-agent/internal/auth/session_test.go index f175fe681..be5e70fa1 100644 --- a/packages/vm-agent/internal/auth/session_test.go +++ b/packages/vm-agent/internal/auth/session_test.go @@ -183,3 +183,43 @@ func TestGetSessionForWorkspace_ExpiredScopedCookie(t *testing.T) { t.Error("expected to fall back to legacy cookie when scoped session is expired") } } + +func TestSessionManagerStopIsIdempotent(t *testing.T) { + sm := newTestSessionManager() + + sm.Stop() + sm.Stop() +} + +func TestSessionManagerStopIsSafeUnderConcurrentCalls(t *testing.T) { + sm := newTestSessionManager() + + const callers = 16 + done := make(chan struct{}, callers) + for i := 0; i < callers; i++ { + go func() { + sm.Stop() + done <- struct{}{} + }() + } + + for i := 0; i < callers; i++ { + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("concurrent Stop calls did not return") + } + } +} + +func TestSessionManagerStopWaitsForCleanupExit(t *testing.T) { + sm := newTestSessionManager() + + sm.Stop() + + select { + case <-sm.cleanupDone: + default: + t.Fatal("Stop returned before cleanup goroutine exited") + } +} diff --git a/packages/vm-agent/internal/server/server.go b/packages/vm-agent/internal/server/server.go index fcda821ca..246c432da 100644 --- a/packages/vm-agent/internal/server/server.go +++ b/packages/vm-agent/internal/server/server.go @@ -108,6 +108,9 @@ type Server struct { callbackToken string httpClient *http.Client // shared HTTP client with timeout for control-plane callbacks done chan struct{} + stopOnce sync.Once + stopErrMu sync.Mutex + stopErr error publishJobsMu sync.Mutex publishJobs map[string]publishJobState buildPublishRunner func(context.Context, *preparedBuildPublish, publish.EventSink) (*publish.ReleaseResult, error) @@ -970,46 +973,57 @@ func (s *Server) StopAllWorkspacesAndSessions() { // Stop gracefully stops the server. func (s *Server) Stop(ctx context.Context) error { - // Signal background goroutines to stop. - close(s.done) + s.stopOnce.Do(func() { + // Signal background goroutines to stop. + close(s.done) - // Stop all port scanners - s.stopAllPortScanners() + // Stop all port scanners + s.stopAllPortScanners() - // Close JWT validator - s.jwtValidator.Close() + // Stop browser auth session cleanup. + s.sessionManager.Stop() - s.sessionHostMu.Lock() - for key, host := range s.sessionHosts { - if host != nil { - host.Stop() + // Close JWT validator + s.jwtValidator.Close() + + s.sessionHostMu.Lock() + for key, host := range s.sessionHosts { + if host != nil { + host.Stop() + } + delete(s.sessionHosts, key) } - delete(s.sessionHosts, key) - } - s.sessionHostMu.Unlock() + s.sessionHostMu.Unlock() - // Close all workspace PTY sessions. - s.workspaceMu.Lock() - for _, runtime := range s.workspaces { - runtime.PTY.CloseAllSessions() - } - s.workspaceMu.Unlock() + // Close all workspace PTY sessions. + s.workspaceMu.Lock() + for _, runtime := range s.workspaces { + runtime.PTY.CloseAllSessions() + } + s.workspaceMu.Unlock() - // Flush and stop error reporter - s.errorReporter.Shutdown() + // Flush and stop error reporter + s.errorReporter.Shutdown() - // Flush and stop all per-workspace message reporters - s.shutdownAllReporters() + // Flush and stop all per-workspace message reporters + s.shutdownAllReporters() - // Close persistence store - if s.store != nil { - if err := s.store.Close(); err != nil { - slog.Warn("Failed to close persistence store", "error", err) + // Close persistence store + if s.store != nil { + if err := s.store.Close(); err != nil { + slog.Warn("Failed to close persistence store", "error", err) + } } - } - // Shutdown HTTP server - return s.httpServer.Shutdown(ctx) + // Shutdown HTTP server + stopErr := s.httpServer.Shutdown(ctx) + s.stopErrMu.Lock() + s.stopErr = stopErr + s.stopErrMu.Unlock() + }) + s.stopErrMu.Lock() + defer s.stopErrMu.Unlock() + return s.stopErr } // setupRoutes configures the HTTP routes. diff --git a/packages/vm-agent/internal/server/shutdown_test.go b/packages/vm-agent/internal/server/shutdown_test.go new file mode 100644 index 000000000..f0fdf3267 --- /dev/null +++ b/packages/vm-agent/internal/server/shutdown_test.go @@ -0,0 +1,116 @@ +package server + +import ( + "bytes" + "context" + "net/http" + "runtime/pprof" + "strings" + "sync" + "testing" + "time" + + "github.com/workspace/vm-agent/internal/acp" + "github.com/workspace/vm-agent/internal/auth" + "github.com/workspace/vm-agent/internal/config" + "github.com/workspace/vm-agent/internal/container" + "github.com/workspace/vm-agent/internal/errorreport" + "github.com/workspace/vm-agent/internal/messagereport" + "github.com/workspace/vm-agent/internal/ports" +) + +func newShutdownTestServer(t *testing.T) *Server { + t.Helper() + + sessionManager := auth.NewSessionManagerWithConfig(auth.SessionManagerConfig{ + CookieName: "vm_session", + Secure: false, + TTL: time.Hour, + CleanupInterval: time.Hour, + MaxSessions: 100, + }) + + errorReporter := errorreport.New("", "node-1", "", errorreport.Config{FlushInterval: time.Hour}) + errorReporter.Start() + + return &Server{ + config: &config.Config{}, + httpServer: &http.Server{}, + jwtValidator: &auth.JWTValidator{}, + sessionManager: sessionManager, + workspaces: make(map[string]*WorkspaceRuntime), + sessionHosts: make(map[string]*acp.SessionHost), + messageReporters: make(map[string]*messagereport.Reporter), + portScanners: make(map[string]*ports.Scanner), + portDiscoveries: make(map[string]*container.Discovery), + errorReporter: errorReporter, + done: make(chan struct{}), + } +} + +func authCleanupGoroutines(t *testing.T) int { + t.Helper() + + var buf bytes.Buffer + if err := pprof.Lookup("goroutine").WriteTo(&buf, 2); err != nil { + t.Fatalf("write goroutine profile: %v", err) + } + return strings.Count(buf.String(), "github.com/workspace/vm-agent/internal/auth.(*SessionManager).cleanup") +} + +func waitForAuthCleanupCount(t *testing.T, want int) { + t.Helper() + + deadline := time.After(2 * time.Second) + tick := time.NewTicker(10 * time.Millisecond) + defer tick.Stop() + + for { + if got := authCleanupGoroutines(t); got == want { + return + } + select { + case <-tick.C: + case <-deadline: + t.Fatalf("auth cleanup goroutine count = %d, want %d", authCleanupGoroutines(t), want) + } + } +} + +func TestServerStopIsIdempotentAndStopsAuthCleanup(t *testing.T) { + baseline := authCleanupGoroutines(t) + s := newShutdownTestServer(t) + waitForAuthCleanupCount(t, baseline+1) + + if err := s.Stop(context.Background()); err != nil { + t.Fatalf("first Stop returned error: %v", err) + } + if err := s.Stop(context.Background()); err != nil { + t.Fatalf("second Stop returned error: %v", err) + } + + waitForAuthCleanupCount(t, baseline) +} + +func TestServerStopIsSafeUnderConcurrentCalls(t *testing.T) { + s := newShutdownTestServer(t) + + const callers = 16 + var wg sync.WaitGroup + errs := make(chan error, callers) + wg.Add(callers) + for i := 0; i < callers; i++ { + go func() { + defer wg.Done() + errs <- s.Stop(context.Background()) + }() + } + wg.Wait() + close(errs) + + for err := range errs { + if err != nil { + t.Fatalf("concurrent Stop returned error: %v", err) + } + } +} diff --git a/scripts/deploy/sync-wrangler-config.ts b/scripts/deploy/sync-wrangler-config.ts index 4c4b94519..6dbf1cb79 100644 --- a/scripts/deploy/sync-wrangler-config.ts +++ b/scripts/deploy/sync-wrangler-config.ts @@ -48,6 +48,19 @@ const TAIL_WORKER_WRANGLER_TOML_PATH = resolve( const DEPLOY_STATE_DIR = resolve(import.meta.dirname, '../../.wrangler'); const FIRST_DEPLOY_MARKER = resolve(DEPLOY_STATE_DIR, 'tail-worker-first-deploy'); const SETUP_TOKEN_BYTES = 24; +const DEFAULT_SANDBOX_CONTAINER_MAX_INSTANCES = 6; +const DEFAULT_VM_AGENT_CONTAINER_MAX_INSTANCES = 3; + +const CONTAINER_MAX_INSTANCE_CONFIG = { + SandboxDO: { + envVar: 'SANDBOX_CONTAINER_MAX_INSTANCES', + defaultValue: DEFAULT_SANDBOX_CONTAINER_MAX_INSTANCES, + }, + VmAgentContainer: { + envVar: 'VM_AGENT_CONTAINER_MAX_INSTANCES', + defaultValue: DEFAULT_VM_AGENT_CONTAINER_MAX_INSTANCES, + }, +} as const satisfies Record; // Re-exported so tests and callers keep a single import site while the // migration-state logic lives in its own module (file size rule 18). @@ -57,6 +70,12 @@ const recordSchema = v.custom>( (value) => typeof value === 'object' && value !== null && !Array.isArray(value), 'Expected an object' ); +const positiveSafeIntegerSchema = v.pipe( + v.number(), + v.integer('must be an integer'), + v.minValue(1, 'must be greater than or equal to 1'), + v.safeInteger('must be a safe integer') +); function requireRecord(value: unknown, path: string): Record { const result = v.safeParse(recordSchema, value); @@ -93,6 +112,49 @@ function cloudflareWorkerVariablesUrl( return `https://dash.cloudflare.com/${accountId}/workers/services/view/${workerName}/${environment}/settings/variables`; } +function parseContainerMaxInstances(envVar: string, fallback: number): number { + const rawValue = process.env[envVar]; + if (!rawValue) { + return fallback; + } + + const trimmed = rawValue.trim(); + if (!/^\d+$/.test(trimmed)) { + throw new Error(`${envVar} must be a positive safe integer`); + } + + const parsed = Number(trimmed); + const result = v.safeParse(positiveSafeIntegerSchema, parsed); + if (!result.success) { + throw new Error(`${envVar} ${result.issues[0]?.message ?? 'must be a positive safe integer'}`); + } + return result.output; +} + +function getConfiguredContainerMaxInstances( + className: string, + currentValue: number | undefined +): number | undefined { + const config = + CONTAINER_MAX_INSTANCE_CONFIG[className as keyof typeof CONTAINER_MAX_INSTANCE_CONFIG]; + if (!config) { + return currentValue; + } + return parseContainerMaxInstances(config.envVar, config.defaultValue); +} + +function generateContainerBindings( + containers: ContainerBinding[] | undefined +): ContainerBinding[] | undefined { + return containers?.map((container) => ({ + ...container, + max_instances: getConfiguredContainerMaxInstances( + container.class_name, + container.max_instances + ), + })); +} + // ============================================================================ // Pulumi // ============================================================================ @@ -422,12 +484,13 @@ function getStaticApiWorkerBindings( includeArtifactsBinding: boolean, durableObjectMigrations: MigrationEntry[] | undefined ): Partial { + const containers = generateContainerBindings(staticBindings.containers); return { ...(staticBindings.durable_objects ? { durable_objects: staticBindings.durable_objects } : {}), ...(staticBindings.ai ? { ai: staticBindings.ai } : {}), ...(analyticsEngineDatasets ? { analytics_engine_datasets: analyticsEngineDatasets } : {}), ...(durableObjectMigrations ? { migrations: durableObjectMigrations } : {}), - ...(staticBindings.containers ? { containers: staticBindings.containers } : {}), + ...(containers ? { containers } : {}), ...(includeArtifactsBinding ? { artifacts: staticBindings.artifacts } : {}), }; } diff --git a/scripts/quality/check-wrangler-bindings.ts b/scripts/quality/check-wrangler-bindings.ts index e07d7075e..63c80769e 100644 --- a/scripts/quality/check-wrangler-bindings.ts +++ b/scripts/quality/check-wrangler-bindings.ts @@ -10,6 +10,9 @@ * static bindings and resolves migrations from the top-level declarations. * If they're missing at the top level, they'll be missing at runtime. * + * 3. Worker secret inventory comments stay aligned with configure-secrets.sh + * so operator-facing docs/config comments do not silently drift. + * * This check runs in CI to prevent misconfigurations. */ import { readFileSync } from 'node:fs'; @@ -21,6 +24,9 @@ const TAIL_WORKER_WRANGLER_PATH = resolve( import.meta.dirname, '../../apps/tail-worker/wrangler.toml' ); +const CONFIGURE_SECRETS_PATH = resolve(import.meta.dirname, '../deploy/configure-secrets.sh'); +const SECRET_COMMENT_PATTERN = /^# - `?([A-Z0-9_]+)`?/; +const sortAlphabetically = (left: string, right: string): number => left.localeCompare(right); interface Binding { name?: string; @@ -57,78 +63,127 @@ function fail(errors: string[]): never { process.exit(1); } -function main(): void { - const errors: string[] = []; - - // ======================================== - // Check 1: No env sections committed - // ======================================== - - const apiContent = readFileSync(API_WRANGLER_PATH, 'utf-8'); - const apiConfig = TOML.parse(apiContent) as unknown as WranglerConfig; +function extractConfiguredWorkerSecrets(scriptContent: string): string[] { + return Array.from( + new Set( + Array.from(scriptContent.matchAll(/set_worker_secret\s+"([A-Z0-9_]+)"/g), (match) => match[1]) + ) + ).sort(sortAlphabetically); +} - if (apiConfig.env && Object.keys(apiConfig.env).length > 0) { - const envNames = Object.keys(apiConfig.env).join(', '); - errors.push( - `apps/api/wrangler.toml contains [env.*] sections (${envNames}). ` + - `These are generated at deploy time by sync-wrangler-config.ts. ` + - `Remove them from the checked-in file.` - ); +function extractWranglerCommentedSecrets(wranglerContent: string): string[] { + const secretsHeader = 'Secrets (set via wrangler secret put):'; + const start = wranglerContent.indexOf(secretsHeader); + if (start === -1) { + return []; } - const tailContent = readFileSync(TAIL_WORKER_WRANGLER_PATH, 'utf-8'); - const tailConfig = TOML.parse(tailContent) as unknown as WranglerConfig; + const headerLineEnd = wranglerContent.indexOf('\n', start); + if (headerLineEnd === -1) { + return []; + } - if (tailConfig.env && Object.keys(tailConfig.env).length > 0) { - const envNames = Object.keys(tailConfig.env).join(', '); - errors.push( - `apps/tail-worker/wrangler.toml contains [env.*] sections (${envNames}). ` + - `These are generated at deploy time. Remove them from the checked-in file.` - ); + const afterHeader = wranglerContent.slice(headerLineEnd + 1); + const lines = afterHeader.split('\n'); + const secrets: string[] = []; + + for (const line of lines) { + if (!line.startsWith('#')) { + break; + } + const match = SECRET_COMMENT_PATTERN.exec(line); + if (match) { + secrets.push(match[1]); + } } - // ======================================== - // Check 2: Top-level has required bindings - // ======================================== + return Array.from(new Set(secrets)).sort(sortAlphabetically); +} - if (!apiConfig.durable_objects?.bindings?.length) { - errors.push( - 'apps/api/wrangler.toml: top-level missing durable_objects.bindings (sync script copies these to env sections)' - ); - } +function diffSecrets(expected: string[], actual: string[]): { missing: string[]; extra: string[] } { + const actualSet = new Set(actual); + const expectedSet = new Set(expected); + return { + missing: expected.filter((name) => !actualSet.has(name)), + extra: actual.filter((name) => !expectedSet.has(name)), + }; +} - if (!apiConfig.ai?.binding) { - errors.push( - 'apps/api/wrangler.toml: top-level missing [ai] binding (sync script copies this to env sections)' - ); +function checkNoCommittedEnvSections(errors: string[], config: WranglerConfig, path: string): void { + if (!config.env || Object.keys(config.env).length === 0) { + return; } - if (!apiConfig.d1_databases?.length) { - errors.push('apps/api/wrangler.toml: top-level missing d1_databases'); - } + const envNames = Object.keys(config.env).join(', '); + errors.push( + `${path} contains [env.*] sections (${envNames}). These are generated at deploy time. Remove them from the checked-in file.` + ); +} - if (!apiConfig.kv_namespaces?.length) { - errors.push('apps/api/wrangler.toml: top-level missing kv_namespaces'); +function checkRequiredApiBindings(errors: string[], apiConfig: WranglerConfig): void { + const requiredBindingChecks: Array<{ isPresent: boolean; message: string }> = [ + { + isPresent: Boolean(apiConfig.durable_objects?.bindings?.length), + message: + 'apps/api/wrangler.toml: top-level missing durable_objects.bindings (sync script copies these to env sections)', + }, + { + isPresent: Boolean(apiConfig.ai?.binding), + message: + 'apps/api/wrangler.toml: top-level missing [ai] binding (sync script copies this to env sections)', + }, + { + isPresent: Boolean(apiConfig.d1_databases?.length), + message: 'apps/api/wrangler.toml: top-level missing d1_databases', + }, + { + isPresent: Boolean(apiConfig.kv_namespaces?.length), + message: 'apps/api/wrangler.toml: top-level missing kv_namespaces', + }, + { + isPresent: Boolean(apiConfig.r2_buckets?.length), + message: 'apps/api/wrangler.toml: top-level missing r2_buckets', + }, + { + isPresent: Boolean(apiConfig.migrations?.length), + message: + 'apps/api/wrangler.toml: top-level missing [[migrations]] (sync script resolves these into env sections)', + }, + ]; + + for (const check of requiredBindingChecks) { + if (!check.isPresent) { + errors.push(check.message); + } } +} - if (!apiConfig.r2_buckets?.length) { - errors.push('apps/api/wrangler.toml: top-level missing r2_buckets'); +function checkSecretInventory( + errors: string[], + configuredSecrets: string[], + commentedSecrets: string[] +): void { + if (commentedSecrets.length === 0) { + errors.push( + 'apps/api/wrangler.toml: missing "# Secrets (set via wrangler secret put):" inventory comment' + ); + return; } - if (!apiConfig.migrations?.length) { + const { missing, extra } = diffSecrets(configuredSecrets, commentedSecrets); + if (missing.length > 0) { errors.push( - 'apps/api/wrangler.toml: top-level missing [[migrations]] (sync script resolves these into env sections)' + `apps/api/wrangler.toml secret inventory is missing configured secrets from configure-secrets.sh: ${missing.join(', ')}` ); } - - // ======================================== - // Result - // ======================================== - - if (errors.length > 0) { - fail(errors); + if (extra.length > 0) { + errors.push( + `apps/api/wrangler.toml secret inventory lists secrets not configured by configure-secrets.sh: ${extra.join(', ')}` + ); } +} +function logSuccess(apiConfig: WranglerConfig, configuredSecrets: string[]): void { const doCount = apiConfig.durable_objects?.bindings?.length ?? 0; const d1Count = apiConfig.d1_databases?.length ?? 0; const kvCount = apiConfig.kv_namespaces?.length ?? 0; @@ -140,6 +195,31 @@ function main(): void { console.log( ` Top-level: ${doCount} DOs, ${d1Count} D1, ${kvCount} KV, ${r2Count} R2, AI, ${migrationCount} migrations` ); + console.log( + ` Worker secret inventory: ${configuredSecrets.length} configured secrets documented.` + ); +} + +function main(): void { + const errors: string[] = []; + const apiContent = readFileSync(API_WRANGLER_PATH, 'utf-8'); + const apiConfig = TOML.parse(apiContent) as unknown as WranglerConfig; + const configureSecretsContent = readFileSync(CONFIGURE_SECRETS_PATH, 'utf-8'); + const tailContent = readFileSync(TAIL_WORKER_WRANGLER_PATH, 'utf-8'); + const tailConfig = TOML.parse(tailContent) as unknown as WranglerConfig; + const configuredSecrets = extractConfiguredWorkerSecrets(configureSecretsContent); + const commentedSecrets = extractWranglerCommentedSecrets(apiContent); + + checkNoCommittedEnvSections(errors, apiConfig, 'apps/api/wrangler.toml'); + checkNoCommittedEnvSections(errors, tailConfig, 'apps/tail-worker/wrangler.toml'); + checkRequiredApiBindings(errors, apiConfig); + checkSecretInventory(errors, configuredSecrets, commentedSecrets); + + if (errors.length > 0) { + fail(errors); + } + + logSuccess(apiConfig, configuredSecrets); } main(); diff --git a/scripts/quality/deploy-reusable-workflow.test.ts b/scripts/quality/deploy-reusable-workflow.test.ts index e696431d0..b95d0e95d 100644 --- a/scripts/quality/deploy-reusable-workflow.test.ts +++ b/scripts/quality/deploy-reusable-workflow.test.ts @@ -1,4 +1,7 @@ -import { readFileSync } from 'node:fs'; +import { execFileSync } from 'node:child_process'; +import { chmodSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; import { describe, expect, it } from 'vitest'; @@ -6,15 +9,99 @@ const workflow = readFileSync( new URL('../../.github/workflows/deploy-reusable.yml', import.meta.url), 'utf8' ); +const syncWranglerConfig = readFileSync( + new URL('../deploy/sync-wrangler-config.ts', import.meta.url), + 'utf8' +); function stepBlock(stepName: string): string { const pattern = new RegExp( String.raw` - name: ${stepName}[\s\S]*?(?=\n - name:|\n #|$)` ); - const match = workflow.match(pattern); + const block = workflow.match(pattern)?.[0]; + + expect(block).toBeDefined(); + if (!block) { + throw new Error(`Unable to find workflow step: ${stepName}`); + } + return block; +} + +function extractOptionalWorkerEnvVars(): string[] { + const match = syncWranglerConfig.match(/getOptionalProcessEnvVars\(\[\s*([\s\S]*?)\s*\]\)/); + const optionalEnvBlock = match?.[1]; + + expect(optionalEnvBlock).toBeDefined(); + if (!optionalEnvBlock) { + throw new Error('Unable to find sync-wrangler optional Worker env var list'); + } + + const vars = Array.from(optionalEnvBlock.matchAll(/'([A-Z0-9_]+)'/g), (varMatch) => varMatch[1]); + expect(vars).toContain('CF_CONTAINER_ENABLED'); + expect(vars).toContain('SANDBOX_ENABLED'); + expect(vars).toContain('MAX_CONCURRENT_SETUP_SESSIONS'); - expect(match?.[0]).toBeDefined(); - return match![0]; + return vars; +} + +const DIRECT_SYNC_ENV_MAPPINGS = { + PULUMI_STACK: 'PULUMI_STACK: ${{ steps.pulumi-select.outputs.stack_name }}', + CF_API_TOKEN: 'CF_API_TOKEN: ${{ secrets.CF_API_TOKEN }}', + CLOUDFLARE_API_TOKEN: 'CLOUDFLARE_API_TOKEN: ${{ secrets.CF_API_TOKEN }}', + ARTIFACTS_BINDING_ENABLED: 'ARTIFACTS_BINDING_ENABLED: ${{ vars.ARTIFACTS_BINDING_ENABLED }}', + SETUP_FORCE: 'SETUP_FORCE: ${{ vars.SETUP_FORCE }}', + BASE_DOMAIN: 'BASE_DOMAIN: ${{ vars.BASE_DOMAIN }}', + RESOURCE_PREFIX: 'RESOURCE_PREFIX: ${{ steps.prefix.outputs.value }}', +} as const; +function stepRunScript(stepName: string): string { + const block = stepBlock(stepName); + const runIndex = block.indexOf(' run: |\n'); + + expect(runIndex).toBeGreaterThan(-1); + + return block + .slice(runIndex + ' run: |\n'.length) + .split('\n') + .filter((line) => line.startsWith(' ') || line.trim() === '') + .map((line) => (line.startsWith(' ') ? line.slice(' '.length) : line)) + .join('\n'); +} + +function runWorkersDevSubdomainStep(httpCode: number): { output: string; status: number } { + const tmp = mkdtempSync(join(tmpdir(), 'sam-workers-dev-test-')); + const curlPath = join(tmp, 'curl'); + + writeFileSync( + curlPath, + `#!/usr/bin/env bash\nprintf 'fake-body\\n%s\\n' "$SAM_FAKE_HTTP_CODE"\n` + ); + chmodSync(curlPath, 0o755); + + try { + const output = execFileSync('bash', ['-c', stepRunScript('Ensure workers.dev Subdomain')], { + cwd: new URL('../..', import.meta.url), + env: { + ...process.env, + PATH: `${tmp}:${process.env.PATH ?? ''}`, + CF_ACCOUNT_ID: 'account-test', + CF_API_TOKEN: 'token-test', + RESOURCE_PREFIX: 'sam-test', + SAM_FAKE_HTTP_CODE: String(httpCode), + }, + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'pipe'], + }); + + return { output, status: 0 }; + } catch (error) { + const execError = error as { stdout?: Buffer | string; stderr?: Buffer | string; status?: number }; + return { + output: `${execError.stdout?.toString() ?? ''}${execError.stderr?.toString() ?? ''}`, + status: execError.status ?? 1, + }; + } finally { + rmSync(tmp, { recursive: true, force: true }); + } } describe('deploy reusable workflow', () => { @@ -40,17 +127,41 @@ describe('deploy reusable workflow', () => { expect(redeployAfterSecretsIndex).toBeGreaterThan(deployApiIndex); }); - it('passes derived deployment identity into every Wrangler config sync phase', () => { - for (const name of [ - 'Sync Wrangler Config \\(API \\+ Tail Worker\\)', - 'Re-sync Wrangler Config \\(add tail_consumers\\)', - ]) { - const block = stepBlock(name); + it('passes derived deployment identity through the shared Wrangler config sync env', () => { + const initialSync = stepBlock('Sync Wrangler Config \\(API \\+ Tail Worker\\)'); + const firstDeployResync = stepBlock('Re-sync Wrangler Config \\(add tail_consumers\\)'); - expect(block).toContain('pnpm tsx scripts/deploy/sync-wrangler-config.ts'); - expect(block).toContain('BASE_DOMAIN: ${{ vars.BASE_DOMAIN }}'); - expect(block).toContain('RESOURCE_PREFIX: ${{ steps.prefix.outputs.value }}'); - expect(block).toContain('ARTIFACTS_BINDING_ENABLED: ${{ vars.ARTIFACTS_BINDING_ENABLED }}'); + expect(initialSync).toContain('pnpm tsx scripts/deploy/sync-wrangler-config.ts'); + expect(initialSync).toContain('BASE_DOMAIN: ${{ vars.BASE_DOMAIN }}'); + expect(initialSync).toContain('RESOURCE_PREFIX: ${{ steps.prefix.outputs.value }}'); + expect(initialSync).toContain( + 'ARTIFACTS_BINDING_ENABLED: ${{ vars.ARTIFACTS_BINDING_ENABLED }}' + ); + + expect(firstDeployResync).toContain('pnpm tsx scripts/deploy/sync-wrangler-config.ts'); + expect(firstDeployResync).toContain('BASE_DOMAIN: ${{ vars.BASE_DOMAIN }}'); + expect(firstDeployResync).toContain('RESOURCE_PREFIX: ${{ steps.prefix.outputs.value }}'); + expect(firstDeployResync).toContain( + 'ARTIFACTS_BINDING_ENABLED: ${{ vars.ARTIFACTS_BINDING_ENABLED }}' + ); + }); + + it('uses one complete env mapping for every Wrangler config sync invocation', () => { + const initialSync = stepBlock('Sync Wrangler Config \\(API \\+ Tail Worker\\)'); + const firstDeployResync = stepBlock('Re-sync Wrangler Config \\(add tail_consumers\\)'); + + expect(initialSync).toContain('env:'); + expect(firstDeployResync).toContain('env:'); + + for (const mapping of Object.values(DIRECT_SYNC_ENV_MAPPINGS)) { + expect(initialSync).toContain(mapping); + expect(firstDeployResync).toContain(mapping); + } + + for (const envVar of extractOptionalWorkerEnvVars()) { + const mapping = `${envVar}: \${{ vars.${envVar} }}`; + expect(initialSync).toContain(mapping); + expect(firstDeployResync).toContain(mapping); } }); @@ -144,6 +255,17 @@ describe('deploy reusable workflow', () => { } }); + it('forwards Cloudflare container max-instance overrides into the wrangler config sync env', () => { + const sync = stepBlock('Sync Wrangler Config \\(API \\+ Tail Worker\\)'); + + expect(sync).toContain( + 'SANDBOX_CONTAINER_MAX_INSTANCES: ${{ vars.SANDBOX_CONTAINER_MAX_INSTANCES }}' + ); + expect(sync).toContain( + 'VM_AGENT_CONTAINER_MAX_INSTANCES: ${{ vars.VM_AGENT_CONTAINER_MAX_INSTANCES }}' + ); + }); + it('forwards the codex credential-setup tunables into the wrangler config sync env', () => { const sync = stepBlock('Sync Wrangler Config \\(API \\+ Tail Worker\\)'); @@ -172,4 +294,30 @@ describe('deploy reusable workflow', () => { expect(build).toContain('make -C packages/vm-agent build-all'); expect(build).toContain('VERSION="$GITHUB_SHA"'); }); + + it('continues deployment when workers.dev subdomain setup succeeds', () => { + const result = runWorkersDevSubdomainStep(200); + + expect(result.status).toBe(0); + expect(result.output).toContain('workers.dev subdomain ready: sam-test.workers.dev'); + }); + + it('continues deployment when workers.dev subdomain is already enabled', () => { + const result = runWorkersDevSubdomainStep(409); + + expect(result.status).toBe(0); + expect(result.output).toContain('workers.dev subdomain already configured (OK)'); + }); + + it('fails closed when workers.dev subdomain setup fails', () => { + const result = runWorkersDevSubdomainStep(403); + + expect(result.status).toBe(1); + expect(result.output).toContain( + '::error::Failed to set workers.dev subdomain (HTTP 403): fake-body' + ); + expect(result.output).toContain( + 'Deployment cannot continue because Cloudflare cron triggers require the workers.dev subdomain prerequisite.' + ); + }); }); diff --git a/scripts/quality/sync-wrangler-config.test.ts b/scripts/quality/sync-wrangler-config.test.ts index 8b0f4766f..819fe002e 100644 --- a/scripts/quality/sync-wrangler-config.test.ts +++ b/scripts/quality/sync-wrangler-config.test.ts @@ -171,6 +171,117 @@ describe('sync wrangler config', () => { }); }); + it('generates Cloudflare container max_instances with unchanged safe defaults', () => { + vi.stubEnv('RESOURCE_PREFIX', 's123abc'); + + const containers = [ + { + class_name: 'SandboxDO', + image: './Dockerfile.sandbox', + instance_type: 'standard-1', + max_instances: 999, + }, + { + class_name: 'VmAgentContainer', + image: './Dockerfile.vm-agent-container', + instance_type: 'standard-1', + max_instances: 999, + }, + ]; + + const envConfig = generateApiWorkerEnv({ containers }, outputs, 'prod', false, false, null); + + expect(envConfig.containers).toEqual([ + { + class_name: 'SandboxDO', + image: './Dockerfile.sandbox', + instance_type: 'standard-1', + max_instances: 6, + }, + { + class_name: 'VmAgentContainer', + image: './Dockerfile.vm-agent-container', + instance_type: 'standard-1', + max_instances: 3, + }, + ]); + }); + + it('respects deployment overrides for Cloudflare container max_instances', () => { + vi.stubEnv('RESOURCE_PREFIX', 's123abc'); + vi.stubEnv('SANDBOX_CONTAINER_MAX_INSTANCES', '8'); + vi.stubEnv('VM_AGENT_CONTAINER_MAX_INSTANCES', '5'); + + const containers = [ + { + class_name: 'SandboxDO', + image: './Dockerfile.sandbox', + instance_type: 'standard-1', + max_instances: 6, + }, + { + class_name: 'VmAgentContainer', + image: './Dockerfile.vm-agent-container', + instance_type: 'standard-1', + max_instances: 3, + }, + { + class_name: 'OtherContainer', + image: './Dockerfile.other', + instance_type: 'standard-1', + max_instances: 2, + }, + ]; + + const envConfig = generateApiWorkerEnv({ containers }, outputs, 'prod', false, false, null); + + expect(envConfig.containers).toEqual([ + { + class_name: 'SandboxDO', + image: './Dockerfile.sandbox', + instance_type: 'standard-1', + max_instances: 8, + }, + { + class_name: 'VmAgentContainer', + image: './Dockerfile.vm-agent-container', + instance_type: 'standard-1', + max_instances: 5, + }, + { + class_name: 'OtherContainer', + image: './Dockerfile.other', + instance_type: 'standard-1', + max_instances: 2, + }, + ]); + }); + + it('fails closed for invalid Cloudflare container max_instances overrides', () => { + vi.stubEnv('RESOURCE_PREFIX', 's123abc'); + vi.stubEnv('SANDBOX_CONTAINER_MAX_INSTANCES', '0'); + + const topLevel: WranglerToml = { + containers: [ + { + class_name: 'SandboxDO', + image: './Dockerfile.sandbox', + instance_type: 'standard-1', + max_instances: 6, + }, + ], + }; + + expect(() => generateApiWorkerEnv(topLevel, outputs, 'prod', false, false, null)).toThrow( + 'SANDBOX_CONTAINER_MAX_INSTANCES must be greater than or equal to 1' + ); + + vi.stubEnv('SANDBOX_CONTAINER_MAX_INSTANCES', '1.5'); + expect(() => generateApiWorkerEnv(topLevel, outputs, 'prod', false, false, null)).toThrow( + 'SANDBOX_CONTAINER_MAX_INSTANCES must be a positive safe integer' + ); + }); + it('omits Artifacts binding and disables runtime flag when Artifacts is not enabled', () => { vi.stubEnv('RESOURCE_PREFIX', 's123abc'); diff --git a/tasks/active/2026-07-29-strict-cto-remediation-mega-pr.md b/tasks/active/2026-07-29-strict-cto-remediation-mega-pr.md new file mode 100644 index 000000000..b6bdad4a2 --- /dev/null +++ b/tasks/active/2026-07-29-strict-cto-remediation-mega-pr.md @@ -0,0 +1,50 @@ +# Strict CTO remediation mega PR + +## Problem + +Eight remediation PRs have been completed independently and need to be integrated into one safe mega PR. The integration must preserve each remediation's tests and guarantees, validate the combined diff locally and on staging, and merge to production only after all gates pass. + +## Input PRs + +- #1689 default-branch/output-branch safety +- #1690 deploy-reusable Wrangler sync env parity +- #1691 fail-closed workers.dev/cron setup +- #1692 deployment docs and Worker secret inventory drift +- #1693 VM-agent shutdown idempotency +- #1694 configurable Cloudflare container max_instances +- #1695 ProjectData row fault isolation + bootstrap TTL wording +- #1696 atomic bootstrap token redemption + +## Research findings + +- SAM instructions require progress updates, use of the output branch `sam/execute-task-using-skill-ggdn3n`, and no merge until validation is complete. +- Existing local main worktree has an unrelated `.codex/config.toml` modification that must be preserved and excluded. +- All eight PRs are open against `main` and initially report clean merge state from GitHub. +- Affected areas are expected to include task dispatch/branch handling, deployment scripts/docs/env inventory, Cloudflare Worker setup, VM agent Go shutdown behavior, ProjectData Durable Object message listing, and bootstrap token redemption. + +## Checklist + +- [x] Create integration branch from current `origin/main`. +- [x] Merge/cherry-pick PRs #1689 through #1696. +- [x] Resolve conflicts without dropping tests or docs from any remediation. +- [x] Run targeted tests for all affected areas. +- [x] Run full feasible local validation: lint, typecheck, tests, build. +- [x] Run local specialist reviews for correctness, security, Cloudflare/env consistency, Go quality, task completion, and test quality. +- [ ] Open mega PR with specialist review evidence. +- [ ] Wait for CI to be completely green. +- [ ] Check staging state before deploy and avoid clobbering active validation. +- [ ] Deploy the mega PR to staging. +- [ ] Validate affected surfaces on staging: task dispatch/output branches, bootstrap token flow, ProjectData message listing, deploy workflow config behavior, docs/build/quality checks, and VM-agent shutdown as safely testable. +- [ ] Validate core agent workflows on staging for both Claude and Codex using Playwright token-login and platform credential fallback. +- [ ] Merge mega PR only after all gates pass. +- [ ] Monitor production Deploy Production workflow by merge `headSha` and require a successful deploy run. +- [ ] Report final PR URL, merge commit, staging evidence, core Claude/Codex evidence, production deploy evidence, and source PR disposition. + +## Acceptance criteria + +- One integration PR contains all eight remediation PRs' intended code, docs, and tests. +- CI is green on the integration PR. +- Staging deploy succeeds and all listed affected surfaces are verified. +- Both Claude and Codex core staging workflows return valid responses. +- Production deploy succeeds after merge. +- No individual remediation PR is merged as a substitute for the mega PR. diff --git a/tasks/archive/2026-07-29-atomic-bootstrap-token-redemption.md b/tasks/archive/2026-07-29-atomic-bootstrap-token-redemption.md new file mode 100644 index 000000000..d050c860a --- /dev/null +++ b/tasks/archive/2026-07-29-atomic-bootstrap-token-redemption.md @@ -0,0 +1,37 @@ +# Atomic bootstrap token redemption + +## Problem + +Bootstrap token redemption currently depends on KV `get` then `delete`, plus an isolate-local in-flight map. That prevents duplicate redemption inside one isolate but does not make the single-use consume atomic across Cloudflare Worker isolates or concurrent requests. + +## Research findings + +- `apps/api/src/services/bootstrap.ts` stores token payloads in KV and redeems with KV `get/delete`. +- `apps/api/src/routes/bootstrap.ts` exposes `POST /api/bootstrap/:token` and preserves the current response shape for VM agents. +- `apps/api/src/routes/workspaces/runtime.ts` has a legacy bootstrap-token endpoint that writes KV directly. +- Existing tests cover normal valid/invalid redemption and an isolate-local in-flight replay, but not cross-isolate atomic consume. +- Cloudflare KV cannot provide compare-and-delete semantics; D1 can provide an atomic conditional write/update for the consume decision. +- Relevant prior findings: + - `tasks/archive/2026-03-12-shannon-security-assessment.md` documents the TOCTOU race on bootstrap KV get/delete. + - `tasks/archive/2026-07-18-harden-callback-bootstrap-token-lifecycle.md` requires fail-closed callback/bootstrap token lifecycle behavior while keeping bounded legacy compatibility. + +## Checklist + +- [x] Add a D1 migration for a bootstrap token consume ledger keyed by a non-secret token hash. +- [x] Register newly created bootstrap tokens in the ledger while preserving the existing KV payload format. +- [x] Redeem tokens only after an atomic D1 consume succeeds. +- [x] Add migration-safe handling for in-flight legacy KV-only tokens, using atomic insert-wins semantics. +- [x] Fail closed if the consume state is ambiguous or failed. +- [x] Update direct legacy bootstrap-token creation to register the ledger. +- [x] Add tests for concurrent redemption, single-use semantics, expired/missing tokens, and existing valid-token compatibility. +- [x] Run relevant validation and local specialist reviews. +- [x] Open a narrow PR and do not merge. + +## Acceptance criteria + +- Exactly one concurrent redemption can receive credentials for a known valid token. +- Replays are rejected after one successful consume. +- Missing and expired tokens are rejected without exposing credentials. +- Existing valid token response semantics and token format remain unchanged. +- In-flight KV-only tokens remain redeemable at most once through an atomic legacy claim path. +- CI is green and the PR documents no-breaking-change rationale. diff --git a/tasks/archive/2026-07-29-configurable-container-max-instances.md b/tasks/archive/2026-07-29-configurable-container-max-instances.md new file mode 100644 index 000000000..e29beb2b5 --- /dev/null +++ b/tasks/archive/2026-07-29-configurable-container-max-instances.md @@ -0,0 +1,47 @@ +# Make Cloudflare container max_instances configurable + +## Problem statement + +Cloudflare container `max_instances` is checked into `apps/api/wrangler.toml` as static values for the sandbox and VM-agent container bindings. A prior startup task failed before agent startup with a Hetzner 422 unrelated to this code path, and the requested retry is a tightly scoped remediation PR: keep the current safe defaults exactly unchanged while making the Cloudflare container capacity limits configurable through generic deployment configuration. + +## Research findings + +- `apps/api/wrangler.toml` defines two top-level `[[containers]]` blocks: + - `SandboxDO` with `max_instances = 6` + - `VmAgentContainer` with `max_instances = 3` +- `scripts/deploy/sync-wrangler-config.ts` copies top-level `containers` into generated `[env.*]` blocks through `extractStaticBindings()` and `getStaticApiWorkerBindings()`. +- `scripts/quality/sync-wrangler-config.test.ts` already tests generated Worker vars and deployment-time override pass-through for cf-container tunables. +- `.github/workflows/deploy-reusable.yml` forwards optional GitHub Environment vars into the Wrangler sync step; new max-instance overrides must be forwarded there too. +- `.claude/rules/43-long-running-mcp-tools.md` and `tasks/archive/2026-07-19-fix-instant-container-clone-timeout.md` document a previous cf-container deploy plumbing issue where deployment tunables needed explicit workflow and sync-script coverage. +- `packages/shared/AGENTS.md` notes configurable defaults should follow env-var resolution patterns and validation should use Valibot. + +## Implementation checklist + +- [x] Add centralized container max-instance config in the Wrangler sync script with defaults exactly `SandboxDO=6` and `VmAgentContainer=3`. +- [x] Add generic deployment environment variable names for overriding those limits. +- [x] Validate overrides as positive safe integers before generating Wrangler config. +- [x] Generate/sync container blocks from centralized config instead of copying static `max_instances` through unchanged. +- [x] Forward the new optional variables from `.github/workflows/deploy-reusable.yml`. +- [x] Add quality tests proving defaults remain `6` and `3`. +- [x] Add quality tests proving overrides are respected. +- [x] Add quality tests proving invalid overrides fail closed. +- [x] Run local Cloudflare/config/test specialist reviews and address findings. +- [x] Open a PR against `main`, wait for CI, and do not merge. + +## Acceptance criteria + +- Generated `containers` bindings preserve the existing defaults exactly: `SandboxDO.max_instances = 6`, `VmAgentContainer.max_instances = 3`. +- Operators can override each container limit through generic deployment configuration without editing checked-in wrangler files. +- Invalid override values stop config generation with a clear error. +- Deployment workflow forwards the new variables to the sync script. +- Tests cover defaults, valid overrides, invalid overrides, and workflow forwarding. +- PR is open, CI is green, and the PR is not merged. + +## References + +- `apps/api/wrangler.toml` +- `scripts/deploy/sync-wrangler-config.ts` +- `scripts/quality/sync-wrangler-config.test.ts` +- `.github/workflows/deploy-reusable.yml` +- `.claude/rules/43-long-running-mcp-tools.md` +- `tasks/archive/2026-07-19-fix-instant-container-clone-timeout.md` diff --git a/tasks/archive/2026-07-29-project-data-list-row-isolation.md b/tasks/archive/2026-07-29-project-data-list-row-isolation.md new file mode 100644 index 000000000..252aff36a --- /dev/null +++ b/tasks/archive/2026-07-29-project-data-list-row-isolation.md @@ -0,0 +1,36 @@ +# ProjectData list row isolation hardening + +## Problem + +ProjectData Durable Object list reads must not fail an entire API/list response when one stored row is malformed or no longer matches the current row schema. The sessions list path already has row-level isolation, but message list reads still map fetched rows through throwing parsers. A malformed `chat_messages` row can therefore break message list retrieval for otherwise valid session history. + +Also fix the stale bootstrap TTL comment/test wording so it remains correct when the TTL is configurable. + +## Research findings + +- `.claude/rules/50-list-read-row-fault-isolation.md` requires per-row parse/enrichment isolation for multi-row D1/DO SQLite reads. +- `apps/api/src/durable-objects/project-data/sessions.ts` already uses `enrichSessionRows()` to skip and warn-log malformed session rows without changing the response contract. +- `apps/api/src/durable-objects/project-data/messages.ts:getMessages()` still does `orderedRows.map(parseChatMessageRow...)`, so one malformed row throws the whole read. +- `apps/api/tests/unit/durable-objects/project-data-messages.test.ts` has focused unit coverage for `getMessages()` ordering and pagination. +- `apps/api/src/services/bootstrap.ts` and `apps/api/tests/unit/services/bootstrap.test.ts` describe the default TTL as 15 minutes/900 seconds, but comments should not imply a fixed TTL when `BOOTSTRAP_TOKEN_TTL_SECONDS` overrides it. + +## Checklist + +- [x] Add row-level parse isolation to `getMessages()` for normal and compact message rows. +- [x] Warn-log skipped message rows with context, best-effort row id/session id, compact mode, and parser error. +- [x] Preserve the existing `{ messages, hasMore }` response contract and ordering behavior. +- [x] Add a good/bad/good regression test for malformed message rows. +- [x] Add an all-bad regression test returning an empty non-throwing list. +- [x] Update stale bootstrap TTL comment/test wording without changing runtime behavior. +- [x] Run targeted tests and broader validation. +- [x] Run local reviewer/subagent checks for tests and code review. +- [x] Open PR, wait for CI, and do not merge. + +## Acceptance criteria + +- Message list reads skip malformed rows and return valid rows instead of throwing. +- Skip behavior is diagnosable via structured warn logging. +- Existing API/response shape is unchanged. +- Targeted tests cover malformed rows among valid rows and the all-bad case. +- Bootstrap TTL comment/test wording reflects configurability. +- PR is open with CI green and remains unmerged. diff --git a/tasks/archive/2026-07-29-safe-output-branch-dispatch.md b/tasks/archive/2026-07-29-safe-output-branch-dispatch.md new file mode 100644 index 000000000..21cb70016 --- /dev/null +++ b/tasks/archive/2026-07-29-safe-output-branch-dispatch.md @@ -0,0 +1,26 @@ +# Safe output branch dispatch + +## Problem + +SAM-dispatched task work can run in a workspace checked out on the repository default branch while `tasks.output_branch` records a separate branch. VM-agent auto-commit-on-completion already has a fail-closed guard that blocks pushing when HEAD is still on the project default branch, but the safer behavior is to create task workspaces on the output branch in the first place. + +## Research findings + +- `apps/api/src/durable-objects/task-runner/workspace-steps.ts` sends the branch payload to the VM agent during workspace dispatch. Before this change it used `state.config.branch` as the checkout branch even when `state.config.outputBranch` was different. +- `packages/vm-agent/internal/server/server.go` contains an auto-commit guard that blocks default-branch pushes when HEAD equals the project default branch. Existing tests cover default-branch block and output-branch success. +- `apps/api/tests/unit/durable-objects/task-runner-workspace-branch-dispatch.test.ts` already targeted this branch payload contract, including the risky explicit-branch case. + +## Implementation checklist + +- [x] Change task-runner workspace dispatch to check out `outputBranch` when present. +- [x] Preserve explicit configured branch as the clone base when it differs from `outputBranch`. +- [x] Preserve existing non-default output branch behavior by basing generated output branches on the project default branch. +- [x] Add targeted tests covering generated output branch, explicit non-default branch with separate output branch, and explicit default branch with separate output branch. +- [x] Re-run VM-agent default-branch push guard tests to prove fail-closed protection remains intact. + +## Acceptance criteria + +- [x] Default branch is not the VM-agent checkout target when task `outputBranch` exists. +- [x] Explicit branch dispatch remains safe: explicit branch is used as the base branch, not the branch that receives task work. +- [x] Existing successful non-default branch behavior still works. +- [x] Public API contracts are unchanged; only internal task-runner-to-VM-agent payload values change. diff --git a/tasks/archive/2026-07-29-vm-agent-shutdown-idempotency.md b/tasks/archive/2026-07-29-vm-agent-shutdown-idempotency.md new file mode 100644 index 000000000..b3e1053dd --- /dev/null +++ b/tasks/archive/2026-07-29-vm-agent-shutdown-idempotency.md @@ -0,0 +1,63 @@ +# VM-agent shutdown idempotency + +## Problem + +VM-agent shutdown paths are not fully idempotent. `auth.SessionManager.Stop()` closes its cleanup channel directly, so repeated calls panic. `server.Server.Stop()` also closes its `done` channel directly and does not stop the owned auth session cleanup goroutine. Shutdown may therefore panic during repeated lifecycle cleanup and may leave an owned goroutine running. + +This task is a retry of the failed startup task `01KYQJ1P3FPQBDPCWFQ0SWPAYJ`; duplicate check found no active queued duplicate for VM-agent shutdown idempotency. + +## Research findings + +- `packages/vm-agent/internal/auth/session.go` + - `NewSessionManagerWithConfig()` starts a cleanup goroutine. + - `SessionManager.Stop()` currently calls `close(sm.stopCleanup)` directly, making repeated calls panic. + - Tests in `session_test.go` defer `sm.Stop()` in many cases, so the public cleanup API is already expected to be safe to call from tests and shutdown paths. +- `packages/vm-agent/internal/server/server.go` + - `Server.Start()` starts node health, ACP heartbeat, and error reporter background work. + - `Server.Stop()` closes `s.done` directly, so repeated calls can panic. + - `Server.Stop()` stops port scanners, JWT validator, ACP session hosts, PTY sessions, reporters, persistence, and HTTP server, but does not stop `s.sessionManager`. +- Relevant rules: + - `.claude/rules/46-vm-agent-diagnostic-getter-sync.md`: vm-agent background goroutine state must be synchronized and race-tested where feasible. + - `.claude/rules/02-quality-gates.md`: lifecycle bug fixes need regression tests that would have caught the violated invariant. + - `.claude/rules/14-do-workflow-persistence.md` and `.claude/rules/25-review-merge-gate.md`: maintain `.do-state.md` and complete specialist reviews before PR completion. + +## Implementation checklist + +- [x] Make `auth.SessionManager.Stop()` idempotent and safe under repeated/concurrent calls without changing public API shape. +- [x] Ensure `auth.SessionManager` cleanup goroutine exits when `Stop()` is called. +- [x] Make `server.Server.Stop()` idempotent for repeated/concurrent calls without changing public API shape. +- [x] Ensure `Server.Stop()` stops the owned auth session cleanup goroutine. +- [x] Preserve current shutdown ordering for external behavior unless a narrow ordering change is required for complete cleanup. +- [x] Add focused Go tests for repeated and concurrent `SessionManager.Stop()`. +- [x] Add focused Go tests proving `Server.Stop()` calls auth session cleanup and repeated `Server.Stop()` does not panic. +- [x] Run relevant Go tests, including `-race` if feasible. +- [x] Run local `go-specialist`, `test-engineer`, and task-completion validation reviews and address findings. +- [ ] Open a PR against `main`, wait for CI, and do not merge. + +## Acceptance criteria + +- Repeated `SessionManager.Stop()` calls do not panic. +- Repeated `Server.Stop()` calls do not panic. +- `Server.Stop()` cleanly stops owned background goroutines, including auth session cleanup. +- No external/public API changes are introduced. +- Strong Go regression tests cover the shutdown idempotency contract and pass locally. +- PR includes specialist review evidence, tests run, CI status, and a no-breaking-change rationale. + + +## Validation evidence + +- `go test ./internal/auth ./internal/server` — passed. +- `go test -race ./internal/auth ./internal/server` — passed. +- `go test ./...` from `packages/vm-agent` — passed. + +## Specialist review evidence + +| Reviewer | Status | Outcome | +| --- | --- | --- | +| task-completion-validator | PASS | Research findings, checked checklist items, and acceptance criteria are represented in the diff/tests; no UI or multi-resource paths apply. | +| go-specialist | PASS | Concurrency/resource lifecycle is bounded by `sync.Once`; no mutex is held during I/O; auth cleanup and server background shutdown are explicit. | +| test-engineer | PASS | Regression tests cover repeated and concurrent Stop calls, cleanup goroutine exit, and race-targeted execution. | + +## No-breaking-change rationale + +The change adds only unexported synchronization fields and internal shutdown coordination. Existing constructors, methods, HTTP routes, config fields, and response shapes are unchanged. First-call shutdown behavior is preserved while repeated calls become safe no-ops that return the first shutdown result. diff --git a/tasks/archive/2026-07-29-worker-secret-inventory-drift.md b/tasks/archive/2026-07-29-worker-secret-inventory-drift.md new file mode 100644 index 000000000..da1001e7a --- /dev/null +++ b/tasks/archive/2026-07-29-worker-secret-inventory-drift.md @@ -0,0 +1,30 @@ +# Reconcile Worker secret inventory docs + +## Problem + +The public deployment/self-hosting docs and checked-in Worker secret inventory comments have drifted from the deploy script that actually writes Worker secrets. In particular, `apps/api/wrangler.toml` still listed legacy Origin CA Worker secrets while omitting newer deployment signing and optional fallback secrets. + +## Research findings + +- Public operator docs live under `apps/www/src/content/docs/docs/`; non-public Markdown should not be used as user-facing documentation. +- `scripts/deploy/configure-secrets.sh` is the operational source for Worker secrets written during deployment. +- `scripts/deploy/types.ts` declares required and optional deploy secret categories but is not itself a complete list of every optional Worker secret written by the shell script. +- `apps/api/wrangler.toml` contains the visible checked-in Worker secret inventory comment. +- `scripts/quality/check-wrangler-bindings.ts` already runs in CI via `pnpm quality:wrangler-bindings`, making it the lowest-risk place to add an inventory drift check. +- New VM nodes use per-node CSR/Origin CA issuance and do not require static `ORIGIN_CA_CERT` or `ORIGIN_CA_KEY` Worker secrets. + +## Checklist + +- [x] Update the `wrangler.toml` Worker secret inventory comment to match configured secrets. +- [x] Update public self-hosting/security docs to clarify generated Worker secrets, optional fallback secrets, and legacy Origin CA cleanup. +- [x] Add a quality check that compares `configure-secrets.sh` `set_worker_secret` calls with the `wrangler.toml` inventory comment. +- [x] Run targeted checks. +- [x] Run doc-sync and env-validator reviews. +- [ ] Open PR and wait for CI without merging. + +## Acceptance criteria + +- Public docs no longer imply legacy Origin CA Worker secrets are required. +- Checked-in Worker secret inventory cannot drift silently from `configure-secrets.sh`. +- No runtime behavior changes. +- PR is open, CI is green, and remains unmerged. diff --git a/tasks/archive/2026-07-29-workersdev-cron-fail-closed.md b/tasks/archive/2026-07-29-workersdev-cron-fail-closed.md new file mode 100644 index 000000000..04929c2a8 --- /dev/null +++ b/tasks/archive/2026-07-29-workersdev-cron-fail-closed.md @@ -0,0 +1,34 @@ +# workers.dev cron setup fail-closed remediation + +## Problem + +The reusable staging/production deploy workflow currently attempts to initialize the account workers.dev subdomain before Worker deployment, but hard failures only emit a warning and allow the deploy to continue. Cloudflare cron triggers may not work unless the workers.dev subdomain prerequisite is configured, so deployment must fail closed by default or use a clearly named explicit degraded-mode override. + +## Research findings + +- `.github/workflows/deploy.yml` and `.github/workflows/deploy-staging.yml` both call `.github/workflows/deploy-reusable.yml`; the reusable workflow is the single target. +- `.github/workflows/deploy-reusable.yml` step `Ensure workers.dev Subdomain` treats 2xx and 409 as success, but warns/continues for every other HTTP result. +- Existing workflow static tests live in `scripts/quality/deploy-reusable-workflow.test.ts`. +- Related deployment hardening task `tasks/archive/2026-07-18-safe-d1-migration-deploy-order.md` uses the same static workflow-test pattern for deployment gates. +- Public docs mention self-hosting/deployment configuration, but this fix preserves default success behavior and only changes failed prerequisite handling; public docs are only needed if an explicit operator override is added. + +## Implementation checklist + +- [x] Change `Ensure workers.dev Subdomain` to fail closed by default for non-2xx/non-409 responses. +- [x] Add a clearly named degraded-mode override only if needed. +- [x] Preserve successful 2xx deploy behavior. +- [x] Preserve 409 already-configured deploy behavior. +- [x] Add tests for success, 409 already-enabled, hard failure, and explicit override if implemented. +- [x] Run targeted workflow quality tests. +- [x] Run broader relevant validation. +- [x] Complete local Cloudflare/security/test/task-completion reviews. +- [ ] Open PR and do not merge. + +## Acceptance criteria + +- Deployment exits non-zero by default when workers.dev subdomain setup cannot be configured/verified. +- 2xx and 409 responses continue to pass. +- Any degraded-mode path is explicitly named and opt-in. +- Static tests prove the shell behavior branches. +- PR includes test evidence, CI status, and no-breaking-change/deploy-risk rationale. + diff --git a/tasks/archive/2026-07-29-wrangler-sync-env-parity.md b/tasks/archive/2026-07-29-wrangler-sync-env-parity.md new file mode 100644 index 000000000..cfbeb95f2 --- /dev/null +++ b/tasks/archive/2026-07-29-wrangler-sync-env-parity.md @@ -0,0 +1,55 @@ +# Wrangler sync env parity for first deploy + +## Problem statement + +`.github/workflows/deploy-reusable.yml` runs `scripts/deploy/sync-wrangler-config.ts` twice on first deploys: the initial config sync and a re-sync after the tail worker exists. The initial sync passes cf-container, sandbox, and setup tuning environment variables, but the first-deploy re-sync omits them. Because `sync-wrangler-config.ts` reads those values from `process.env` and defaults `CF_CONTAINER_ENABLED` to `true`, a first deploy can silently re-enable Cloudflare Containers or drop operator-provided timeout/capacity tunables. + +This PR must be tightly scoped: make the mapping deterministic and test it. + +## Research findings + +- `scripts/deploy/sync-wrangler-config.ts` builds Worker vars in `getApiWorkerVars()`. +- Optional Worker vars consumed by the script are listed in the `getOptionalProcessEnvVars([...])` call. +- The workflow has two `pnpm tsx scripts/deploy/sync-wrangler-config.ts` invocations: + - `Sync Wrangler Config (API + Tail Worker)` + - `Re-sync Wrangler Config (add tail_consumers)` +- Existing quality tests in `scripts/quality/deploy-reusable-workflow.test.ts` already inspect workflow step blocks and are the right place for a deterministic regression test. +- `.claude/rules/07-env-and-urls.md` records that wrangler env sections are generated at deploy time and that Miniflare tests do not catch `wrangler.toml` generation mistakes. +- Prior incident lesson: `tasks/archive/2026-07-19-fix-instant-container-clone-timeout.md` notes env-validator findings where new cf-container env vars were not wired through deploy pipeline allowlists. + +## Checklist + +- [x] Centralize the sync env mapping for all `sync-wrangler-config.ts` workflow invocations. +- [x] Ensure first sync and first-deploy re-sync receive identical optional Worker env inputs. +- [x] Include all env vars consumed by `getOptionalProcessEnvVars()`, including currently missing `CF_CONTAINER_ACTIVE_WORK_MAX_MS`, `CF_CONTAINER_KEEPALIVE_RENEW_INTERVAL_MS`, and `CF_CONTAINER_RECOVERY_MAX_ATTEMPTS`. +- [x] Preserve existing behavior when GitHub vars/secrets are absent. +- [x] Add a workflow quality test that derives consumed optional env vars from `sync-wrangler-config.ts` and fails if any sync invocation does not use the centralized mapping. +- [x] Add a process note to prevent future duplicated deploy sync env blocks. +- [x] Run relevant local tests and full quality gates. +- [x] Run local env-validator and test-engineer reviews before PR completion. +- [ ] Open a narrow PR and do not merge. + +## Acceptance criteria + +- Both deploy-reusable sync invocations use the same env mapping for `sync-wrangler-config.ts`. +- A test fails if a sync invocation omits any direct sync env or optional Worker var consumed by `sync-wrangler-config.ts`. +- Defaults and behavior remain unchanged when vars are absent. +- CI is green on the PR. +- PR is left open/unmerged. + +## Post-mortem + +- **What broke**: First deploys could re-run wrangler config generation without the same operator overrides used by the initial sync. That could produce a different API Worker env section during the tail-consumer re-sync. +- **Root cause**: The workflow duplicated the env mapping inline for each sync invocation, and the second block drifted behind the first. +- **Timeline**: The drift was found by strict CTO infra review task `01KYQHJJM4W83JKKDKTTPA9CNF` and remediated in this task. +- **Why it wasn't caught**: Existing workflow quality tests checked selected vars on the initial sync only and did not derive expected coverage from the env vars consumed by `sync-wrangler-config.ts`. +- **Class of bug**: Duplicated deployment env mappings across multi-phase deploy workflows. +- **Process fix**: Add a project rule note and a regression test that requires sync invocations to share a centralized mapping and keeps it aligned with `sync-wrangler-config.ts`. + +## References + +- `.github/workflows/deploy-reusable.yml` +- `scripts/deploy/sync-wrangler-config.ts` +- `scripts/quality/deploy-reusable-workflow.test.ts` +- `.claude/rules/07-env-and-urls.md` +- `tasks/archive/2026-07-19-fix-instant-container-clone-timeout.md` diff --git a/tasks/backlog/2026-08-06-wrangler-sync-env-parity-test-coverage-gap.md b/tasks/backlog/2026-08-06-wrangler-sync-env-parity-test-coverage-gap.md new file mode 100644 index 000000000..628cac9b9 --- /dev/null +++ b/tasks/backlog/2026-08-06-wrangler-sync-env-parity-test-coverage-gap.md @@ -0,0 +1,64 @@ +# Wrangler sync env-parity test misses DO_MIGRATION_STATE_PROBE_* vars + +## Problem + +`scripts/quality/deploy-reusable-workflow.test.ts` enforces that both Wrangler config sync +invocations in `.github/workflows/deploy-reusable.yml` forward an identical env mapping: + +- `Sync Wrangler Config (API + Tail Worker)` +- `Re-sync Wrangler Config (add tail_consumers)` (runs on first deploy, after the tail worker exists) + +The test builds its required list two ways: + +1. `DIRECT_SYNC_ENV_MAPPINGS` — a hand-maintained constant. +2. `extractOptionalWorkerEnvVars()` — dynamically scraped from the `getOptionalProcessEnvVars([...])` + array in `scripts/deploy/sync-wrangler-config.ts`. + +Neither covers `DO_MIGRATION_STATE_PROBE_ATTEMPTS` or `DO_MIGRATION_STATE_PROBE_RETRY_DELAY_MS`, +because those reach the script through a separate `readBoundedIntEnv` call in +`scripts/deploy/durable-object-migrations.ts` rather than through `getOptionalProcessEnvVars`. + +So if a future change drops either var from one of the two sync steps, **no test fails.** + +## Why it matters + +`generateApiWorkerEnv` regenerates `[env.*]` from scratch on every invocation — it does not merge with +prior state. Anything missing from the re-sync step's `process.env` is silently dropped from the +regenerated config on the second `wrangler deploy` of a first-time install. + +Severity is bounded but real: omitting these two falls back to the documented defaults +(`DO_MIGRATION_STATE_PROBE_ATTEMPTS` = 3 attempts, `DO_MIGRATION_STATE_PROBE_RETRY_DELAY_MS` = 2000ms) +rather than bypassing the fail-closed migration-tag check. So it degrades retry tuning, it does not +disable the Durable Object migration safety guard. That is why this is a follow-up and not a blocker. + +This is the same *class* of drift that PR #1697's merge already had to repair by hand: before that +merge, main's re-sync step was missing 26 vars relative to its own primary step, and the PR branch's +re-sync step had independently drifted 10 `PLATFORM_FEEDBACK_*` vars behind its own primary step. +Both drifts existed precisely because the automated parity check did not cover them. The recurring +lesson is that a partially-derived allowlist invites exactly this drift. + +## Context + +Discovered by the `cloudflare-specialist` review during PR #1697 (merge of `origin/main` into the +strict-CTO remediation bundle, 2026-08-06). At that time both vars **are** correctly present in both +steps — verified by direct grep — so there is no live bug today, only a missing guardrail. + +## Acceptance Criteria + +- [ ] The parity test covers `DO_MIGRATION_STATE_PROBE_ATTEMPTS` and + `DO_MIGRATION_STATE_PROBE_RETRY_DELAY_MS`, either by extending `DIRECT_SYNC_ENV_MAPPINGS` or by + also scraping `readBoundedIntEnv` call sites in `scripts/deploy/durable-object-migrations.ts`. +- [ ] Prefer a derivation that cannot silently miss a *future* var read through a third mechanism — + e.g. scrape every `process.env.X` read reachable from `sync-wrangler-config.ts` and assert each + appears in both steps, rather than maintaining another hand-written allowlist. +- [ ] The test is proven discriminating: temporarily delete one of the two vars from the re-sync step + and confirm the test goes red before relying on it. +- [ ] No production behavior change — this is test-coverage hardening only. + +## References + +- `scripts/quality/deploy-reusable-workflow.test.ts` (`uses one complete env mapping for every + Wrangler config sync invocation`) +- `scripts/deploy/durable-object-migrations.ts` (`readBoundedIntEnv`) +- `.claude/rules/07-env-and-urls.md` — Wrangler binding + DO migration safety rules +- PR #1697; main's DO-migration-compat work in #1649