From 010b1125352557f0ad648676557f5f66adab028e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Wed, 15 Jul 2026 00:46:48 +0200 Subject: [PATCH 1/7] fix(mobile): serialize device ownership claims --- dev/local/mobile-android.test.ts | 119 ++++++++++++++++++++++ dev/local/mobile-android.ts | 85 +++++++++------- dev/local/mobile-simulator.test.ts | 155 +++++++++++++++++++++++++++++ dev/local/mobile-simulator.ts | 100 ++++++++++++++----- dev/local/process-lock.ts | 21 ++++ package.json | 2 + pnpm-lock.yaml | 15 ++- 7 files changed, 436 insertions(+), 61 deletions(-) create mode 100644 dev/local/mobile-android.test.ts create mode 100644 dev/local/process-lock.ts diff --git a/dev/local/mobile-android.test.ts b/dev/local/mobile-android.test.ts new file mode 100644 index 0000000000..5ba9c8daf1 --- /dev/null +++ b/dev/local/mobile-android.test.ts @@ -0,0 +1,119 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import test from 'node:test'; + +import { + claimAndroidDevice, + releaseAndroidDevice, + resolveAndroidEnvironment, +} from './mobile-android'; + +test('skips a partial Android SDK root when a later root has all required tools', () => { + const env = resolveAndroidEnvironment({ + home: '/Users/test', + path: '/usr/bin:/bin', + existingPaths: new Set([ + '/Users/test/Library/Android/sdk/platform-tools/adb', + '/opt/homebrew/share/android-commandlinetools/platform-tools/adb', + '/opt/homebrew/share/android-commandlinetools/emulator/emulator', + '/Library/Java/JavaVirtualMachines/temurin-17.jdk/Contents/Home/bin/java', + ]), + javaMajor: () => 17, + }); + + assert.equal(env.sdkRoot, '/opt/homebrew/share/android-commandlinetools'); + assert.equal(env.adb, '/opt/homebrew/share/android-commandlinetools/platform-tools/adb'); + assert.equal(env.emulator, '/opt/homebrew/share/android-commandlinetools/emulator/emulator'); +}); + +test('serializes stale Android claim replacement with concurrent claim attempts', () => { + const serial = `test-${process.pid}-${Date.now()}`; + const claimRoot = path.join(os.tmpdir(), 'kilo-mobile-android-claims'); + const filePath = path.join(claimRoot, `${serial}.json`); + const staleWorktree = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-stale-worktree-')); + const firstWorktree = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-one-')); + const secondWorktree = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-two-')); + fs.mkdirSync(claimRoot, { recursive: true }); + fs.writeFileSync( + filePath, + JSON.stringify({ serial, worktreeRoot: staleWorktree, claimedAt: new Date().toISOString() }) + ); + fs.rmSync(staleWorktree, { recursive: true }); + let attemptedConcurrentClaim = false; + + try { + const claim = claimAndroidDevice(serial, firstWorktree, { + fileOperations: { + readFileSync: (candidate, encoding) => { + const value = fs.readFileSync(candidate, encoding); + if (candidate === filePath && !attemptedConcurrentClaim) { + attemptedConcurrentClaim = true; + assert.throws( + () => claimAndroidDevice(serial, secondWorktree), + /claim is being updated concurrently/ + ); + } + return value; + }, + }, + }); + + assert.equal(attemptedConcurrentClaim, true); + assert.equal(claim.worktreeRoot, firstWorktree); + assert.equal(JSON.parse(fs.readFileSync(filePath, 'utf8')).worktreeRoot, firstWorktree); + } finally { + fs.rmSync(filePath, { force: true }); + fs.rmSync(`${filePath}.lock`, { force: true }); + fs.rmSync(firstWorktree, { recursive: true, force: true }); + fs.rmSync(secondWorktree, { recursive: true, force: true }); + } +}); + +test('preserves an active Android claim owned by another worktree', () => { + const serial = `test-active-${process.pid}-${Date.now()}`; + const firstWorktree = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-one-')); + const secondWorktree = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-two-')); + + try { + claimAndroidDevice(serial, firstWorktree); + + assert.throws( + () => claimAndroidDevice(serial, secondWorktree), + new RegExp(`claimed by ${firstWorktree}`) + ); + assert.throws( + () => releaseAndroidDevice(serial, secondWorktree), + new RegExp(`claimed by ${firstWorktree}`) + ); + + releaseAndroidDevice(serial, firstWorktree); + } finally { + const filePath = path.join(os.tmpdir(), 'kilo-mobile-android-claims', `${serial}.json`); + fs.rmSync(filePath, { force: true }); + fs.rmSync(`${filePath}.lock`, { force: true }); + fs.rmSync(firstWorktree, { recursive: true, force: true }); + fs.rmSync(secondWorktree, { recursive: true, force: true }); + } +}); + +test('recovers an orphaned Android claim mutation lock', () => { + const serial = `test-orphaned-${process.pid}-${Date.now()}`; + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + const filePath = path.join(os.tmpdir(), 'kilo-mobile-android-claims', `${serial}.json`); + const mutationLockPath = `${filePath}.lock`; + fs.mkdirSync(mutationLockPath, { recursive: true }); + const settledTime = new Date(Date.now() - 6000); + fs.utimesSync(mutationLockPath, settledTime, settledTime); + + try { + const claim = claimAndroidDevice(serial, worktreeRoot); + assert.equal(claim.worktreeRoot, worktreeRoot); + releaseAndroidDevice(serial, worktreeRoot); + } finally { + fs.rmSync(filePath, { force: true }); + fs.rmSync(mutationLockPath, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); diff --git a/dev/local/mobile-android.ts b/dev/local/mobile-android.ts index 0fe6851c9e..b645414520 100644 --- a/dev/local/mobile-android.ts +++ b/dev/local/mobile-android.ts @@ -3,6 +3,8 @@ import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; +import { withProcessLock } from './process-lock'; + type AndroidEnvironment = { adb: string; emulator: string; @@ -20,6 +22,11 @@ type ResolveArgs = { }; type DeviceClaim = { serial: string; worktreeRoot: string; claimedAt: string }; +type ClaimOptions = { + fileOperations?: { + readFileSync?: (filePath: string, encoding: 'utf8') => string; + }; +}; function firstExisting( candidates: string[], @@ -29,8 +36,9 @@ function firstExisting( } function resolveAndroidEnvironment(args: ResolveArgs): AndroidEnvironment { - const exists = args.existingPaths - ? (candidate: string) => args.existingPaths!.has(candidate) + const existingPaths = args.existingPaths; + const exists = existingPaths + ? (candidate: string) => existingPaths.has(candidate) : fs.existsSync; const sdkRoots = [ process.env.ANDROID_HOME, @@ -39,11 +47,9 @@ function resolveAndroidEnvironment(args: ResolveArgs): AndroidEnvironment { '/opt/homebrew/share/android-commandlinetools', '/usr/local/share/android-commandlinetools', ].filter((value): value is string => Boolean(value)); - const sdkRoot = sdkRoots.find(root => - firstExisting( - [path.join(root, 'platform-tools/adb'), path.join(root, 'emulator/emulator')], - exists - ) + const sdkRoot = sdkRoots.find( + root => + exists(path.join(root, 'platform-tools/adb')) && exists(path.join(root, 'emulator/emulator')) ); if (!sdkRoot) { throw new Error( @@ -51,8 +57,8 @@ function resolveAndroidEnvironment(args: ResolveArgs): AndroidEnvironment { ); } - const adb = firstExisting([path.join(sdkRoot, 'platform-tools/adb')], exists); - const emulator = firstExisting([path.join(sdkRoot, 'emulator/emulator')], exists); + const adb = path.join(sdkRoot, 'platform-tools/adb'); + const emulator = path.join(sdkRoot, 'emulator/emulator'); const sdkmanager = firstExisting( [ path.join(sdkRoot, 'cmdline-tools/latest/bin/sdkmanager'), @@ -79,10 +85,7 @@ function resolveAndroidEnvironment(args: ResolveArgs): AndroidEnvironment { candidate => exists(path.join(candidate, 'bin/java')) && javaMajor(candidate) === 17 ); - const missing = [!adb && 'adb', !emulator && 'emulator', !javaHome && 'JDK 17'].filter(Boolean); - if (missing.length > 0) { - throw new Error(`Android tooling incomplete: missing ${missing.join(', ')}`); - } + if (!javaHome) throw new Error('Android tooling incomplete: missing JDK 17'); const toolPaths = [ path.join(sdkRoot, 'platform-tools'), @@ -139,35 +142,49 @@ function claimPath(serial: string): string { ); } -function claimAndroidDevice(serial: string, worktreeRoot: string): DeviceClaim { +function withClaimMutationLock(filePath: string, mutate: () => T): T { + const lockFilePath = `${filePath}.lock`; + return withProcessLock(lockFilePath, `${path.basename(filePath, '.json')} claim`, mutate); +} + +function claimAndroidDevice( + serial: string, + worktreeRoot: string, + options?: ClaimOptions +): DeviceClaim { const filePath = claimPath(serial); fs.mkdirSync(path.dirname(filePath), { recursive: true }); - try { - const claim = JSON.parse(fs.readFileSync(filePath, 'utf8')) as DeviceClaim; - if (claim.worktreeRoot === worktreeRoot) return claim; - if (fs.existsSync(claim.worktreeRoot)) - throw new Error(`${serial} is claimed by ${claim.worktreeRoot}`); - fs.rmSync(filePath, { force: true }); - } catch (error) { - if (error instanceof SyntaxError) { + return withClaimMutationLock(filePath, () => { + try { + const readFileSync = options?.fileOperations?.readFileSync ?? fs.readFileSync; + const claim = JSON.parse(readFileSync(filePath, 'utf8')) as DeviceClaim; + if (claim.worktreeRoot === worktreeRoot) return claim; + if (fs.existsSync(claim.worktreeRoot)) + throw new Error(`${serial} is claimed by ${claim.worktreeRoot}`); fs.rmSync(filePath, { force: true }); - } else if (error instanceof Error && 'code' in error && error.code === 'ENOENT') { - // Missing claims can be created atomically below. - } else if (error instanceof Error && error.message.includes(' is claimed by ')) { - throw error; + } catch (error) { + if (error instanceof SyntaxError) { + fs.rmSync(filePath, { force: true }); + } else if (error instanceof Error && 'code' in error && error.code === 'ENOENT') { + // Missing claims can be created atomically below. + } else if (error instanceof Error && error.message.includes(' is claimed by ')) { + throw error; + } } - } - const claim = { serial, worktreeRoot, claimedAt: new Date().toISOString() }; - fs.writeFileSync(filePath, JSON.stringify(claim), { flag: 'wx' }); - return claim; + const claim = { serial, worktreeRoot, claimedAt: new Date().toISOString() }; + fs.writeFileSync(filePath, JSON.stringify(claim), { flag: 'wx' }); + return claim; + }); } function releaseAndroidDevice(serial: string, worktreeRoot: string): void { const filePath = claimPath(serial); - const claim = JSON.parse(fs.readFileSync(filePath, 'utf8')) as DeviceClaim; - if (claim.worktreeRoot !== worktreeRoot) - throw new Error(`${serial} is claimed by ${claim.worktreeRoot}`); - fs.rmSync(filePath); + withClaimMutationLock(filePath, () => { + const claim = JSON.parse(fs.readFileSync(filePath, 'utf8')) as DeviceClaim; + if (claim.worktreeRoot !== worktreeRoot) + throw new Error(`${serial} is claimed by ${claim.worktreeRoot}`); + fs.rmSync(filePath); + }); } function main(): void { diff --git a/dev/local/mobile-simulator.test.ts b/dev/local/mobile-simulator.test.ts index 83fe4537e3..affce92aba 100644 --- a/dev/local/mobile-simulator.test.ts +++ b/dev/local/mobile-simulator.test.ts @@ -44,3 +44,158 @@ test('uses exclusive claim creation to prevent concurrent simulator sharing', () const source = fs.readFileSync(new URL('./mobile-simulator.ts', import.meta.url), 'utf8'); assert.match(source, /flag: 'wx'/); }); + +for (const initialClaim of ['missing', 'invalid', 'stale'] as const) { + test(`serializes iOS claim cleanup after reading a ${initialClaim} claim`, () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const firstWorktree = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-one-')); + const secondWorktree = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-two-')); + const staleWorktree = path.join(lockRoot, 'removed-worktree'); + const filePath = path.join(lockRoot, 'A.json'); + if (initialClaim === 'invalid') fs.writeFileSync(filePath, '{'); + if (initialClaim === 'stale') { + fs.writeFileSync(filePath, JSON.stringify({ worktreeRoot: staleWorktree })); + } + let concurrentClaimError: unknown; + let concurrentClaimSucceeded = false; + let injected = false; + + try { + const claim = claimSimulator({ + devices, + lockRoot, + worktreeRoot: firstWorktree, + requestedId: 'A', + fileOperations: { + readFileSync: (candidate, encoding) => { + let value: string; + try { + value = fs.readFileSync(candidate, encoding); + } catch (error) { + if (!injected) { + injected = true; + try { + claimSimulator({ + devices, + lockRoot, + worktreeRoot: secondWorktree, + requestedId: 'A', + }); + concurrentClaimSucceeded = true; + } catch (claimError) { + concurrentClaimError = claimError; + } + } + throw error; + } + if (!injected) { + injected = true; + try { + claimSimulator({ + devices, + lockRoot, + worktreeRoot: secondWorktree, + requestedId: 'A', + }); + concurrentClaimSucceeded = true; + } catch (error) { + concurrentClaimError = error; + } + } + return value; + }, + }, + }); + + assert.equal(concurrentClaimSucceeded, false); + assert.match(String(concurrentClaimError), /claim is being updated concurrently/); + assert.equal(claim.device.id, 'A'); + assert.equal(JSON.parse(fs.readFileSync(filePath, 'utf8')).worktreeRoot, firstWorktree); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(firstWorktree, { recursive: true, force: true }); + fs.rmSync(secondWorktree, { recursive: true, force: true }); + } + }); +} + +for (const failedCommand of ['boot', 'bootstatus'] as const) { + test(`releases a newly acquired iOS claim when ${failedCommand} fails`, () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + const commands: string[] = []; + + try { + assert.throws( + () => + claimSimulator({ + devices, + lockRoot, + worktreeRoot, + requestedId: 'B', + prepare: () => { + for (const command of ['boot', 'bootstatus']) { + commands.push(command); + if (command === failedCommand) throw new Error(`${command} failed`); + } + }, + }), + new RegExp(`${failedCommand} failed`) + ); + + assert.deepEqual(commands, failedCommand === 'boot' ? ['boot'] : ['boot', 'bootstatus']); + assert.equal(fs.existsSync(path.join(lockRoot, 'B.json')), false); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } + }); +} + +test('preserves an existing iOS claim when simulator preparation fails', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + + try { + claimSimulator({ devices, lockRoot, worktreeRoot, requestedId: 'B' }); + + assert.throws( + () => + claimSimulator({ + devices, + lockRoot, + worktreeRoot, + requestedId: 'B', + prepare: () => { + throw new Error('bootstatus failed'); + }, + }), + /bootstatus failed/ + ); + assert.equal( + JSON.parse(fs.readFileSync(path.join(lockRoot, 'B.json'), 'utf8')).worktreeRoot, + worktreeRoot + ); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +test('recovers an orphaned iOS claim mutation lock', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + const mutationLockPath = path.join(lockRoot, 'A.json.lock'); + fs.mkdirSync(mutationLockPath); + const settledTime = new Date(Date.now() - 6000); + fs.utimesSync(mutationLockPath, settledTime, settledTime); + + try { + const claim = claimSimulator({ devices, lockRoot, worktreeRoot, requestedId: 'A' }); + assert.equal(claim.device.id, 'A'); + releaseSimulator({ deviceId: 'A', lockRoot, worktreeRoot }); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); diff --git a/dev/local/mobile-simulator.ts b/dev/local/mobile-simulator.ts index e604a2d50d..977a204ae4 100644 --- a/dev/local/mobile-simulator.ts +++ b/dev/local/mobile-simulator.ts @@ -3,32 +3,48 @@ import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; +import { withProcessLock } from './process-lock'; + type SimulatorDevice = { id: string; name: string; state: string }; type ClaimArgs = { devices: SimulatorDevice[]; lockRoot: string; worktreeRoot: string; requestedId?: string; + prepare?: (device: SimulatorDevice) => void; + fileOperations?: { + readFileSync?: (filePath: string, encoding: 'utf8') => string; + }; }; function lockPath(lockRoot: string, deviceId: string): string { return path.join(lockRoot, `${deviceId}.json`); } -function readOwner(lockRoot: string, deviceId: string): string | undefined { +function readOwner( + lockRoot: string, + deviceId: string, + readFileSync: (filePath: string, encoding: 'utf8') => string = fs.readFileSync +): string | undefined { try { - const claim = JSON.parse(fs.readFileSync(lockPath(lockRoot, deviceId), 'utf8')) as { + const claim = JSON.parse(readFileSync(lockPath(lockRoot, deviceId), 'utf8')) as { worktreeRoot?: string; }; if (claim.worktreeRoot && fs.existsSync(claim.worktreeRoot)) return claim.worktreeRoot; fs.rmSync(lockPath(lockRoot, deviceId), { force: true }); - } catch { + } catch (error) { + if (error instanceof Error && 'code' in error && error.code === 'ENOENT') return undefined; fs.rmSync(lockPath(lockRoot, deviceId), { force: true }); - // Missing or invalid claims are unowned. + // Invalid claims are unowned. } return undefined; } +function withClaimMutationLock(lockRoot: string, deviceId: string, mutate: () => T): T { + const mutationLockPath = `${lockPath(lockRoot, deviceId)}.lock`; + return withProcessLock(mutationLockPath, `Simulator ${deviceId} claim`, mutate); +} + function claimSimulator(args: ClaimArgs): { device: SimulatorDevice; alreadyOwned: boolean } { const { devices, lockRoot, worktreeRoot, requestedId } = args; fs.mkdirSync(lockRoot, { recursive: true }); @@ -39,22 +55,51 @@ function claimSimulator(args: ClaimArgs): { device: SimulatorDevice; alreadyOwne throw new Error(`Simulator ${requestedId ?? ''} is not available`.trim()); for (const device of candidates) { - const owner = readOwner(lockRoot, device.id); - if (owner === worktreeRoot) return { device, alreadyOwned: true }; - if (owner) { - if (requestedId) throw new Error(`Simulator ${device.id} is claimed by ${owner}`); - continue; - } try { - fs.writeFileSync( - lockPath(lockRoot, device.id), - JSON.stringify({ deviceId: device.id, worktreeRoot, claimedAt: new Date().toISOString() }), - { flag: 'wx' } - ); - return { device, alreadyOwned: false }; + const claim = withClaimMutationLock(lockRoot, device.id, () => { + const owner = readOwner( + lockRoot, + device.id, + args.fileOperations?.readFileSync ?? fs.readFileSync + ); + if (owner === worktreeRoot) return { device, alreadyOwned: true }; + if (owner) throw new Error(`Simulator ${device.id} is claimed by ${owner}`); + fs.writeFileSync( + lockPath(lockRoot, device.id), + JSON.stringify({ + deviceId: device.id, + worktreeRoot, + claimedAt: new Date().toISOString(), + }), + { flag: 'wx' } + ); + return { device, alreadyOwned: false }; + }); + try { + args.prepare?.(device); + return claim; + } catch (error) { + if (!claim.alreadyOwned) { + releaseSimulator({ deviceId: device.id, lockRoot, worktreeRoot }); + } + throw error; + } } catch (error) { if (error instanceof Error && 'code' in error && error.code === 'EEXIST') { - if (requestedId) throw new Error(`Simulator ${device.id} was claimed concurrently`); + if (requestedId) { + throw new Error(`Simulator ${device.id} was claimed concurrently`, { cause: error }); + } + continue; + } + if ( + error instanceof Error && + error.message.includes(' claim is being updated concurrently') + ) { + if (requestedId) throw error; + continue; + } + if (error instanceof Error && error.message.includes(' is claimed by ')) { + if (requestedId) throw error; continue; } throw error; @@ -68,11 +113,13 @@ function releaseSimulator(args: { lockRoot: string; worktreeRoot: string; }): void { - const owner = readOwner(args.lockRoot, args.deviceId); - if (owner && owner !== args.worktreeRoot) { - throw new Error(`Simulator ${args.deviceId} is claimed by ${owner}`); - } - fs.rmSync(lockPath(args.lockRoot, args.deviceId), { force: true }); + withClaimMutationLock(args.lockRoot, args.deviceId, () => { + const owner = readOwner(args.lockRoot, args.deviceId); + if (owner && owner !== args.worktreeRoot) { + throw new Error(`Simulator ${args.deviceId} is claimed by ${owner}`); + } + fs.rmSync(lockPath(args.lockRoot, args.deviceId), { force: true }); + }); } function listIosDevices(): SimulatorDevice[] { @@ -99,11 +146,12 @@ function main(): void { lockRoot, worktreeRoot, requestedId, + prepare: device => { + if (device.state === 'Booted') return; + execFileSync('xcrun', ['simctl', 'boot', device.id], { stdio: 'ignore' }); + execFileSync('xcrun', ['simctl', 'bootstatus', device.id, '-b'], { stdio: 'inherit' }); + }, }); - if (claim.device.state !== 'Booted') { - execFileSync('xcrun', ['simctl', 'boot', claim.device.id], { stdio: 'ignore' }); - execFileSync('xcrun', ['simctl', 'bootstatus', claim.device.id, '-b'], { stdio: 'inherit' }); - } console.log(JSON.stringify({ ...claim, worktreeRoot })); return; } diff --git a/dev/local/process-lock.ts b/dev/local/process-lock.ts new file mode 100644 index 0000000000..99cfacab46 --- /dev/null +++ b/dev/local/process-lock.ts @@ -0,0 +1,21 @@ +import lockfile from 'proper-lockfile'; + +export function withProcessLock(lockPath: string, label: string, mutate: () => T): T { + let release: (() => void) | undefined; + try { + release = lockfile.lockSync(lockPath, { + lockfilePath: lockPath, + realpath: false, + stale: 5000, + update: 1000, + }); + } catch (error) { + throw new Error(`${label} is being updated concurrently`, { cause: error }); + } + + try { + return mutate(); + } finally { + release(); + } +} diff --git a/package.json b/package.json index dff940f025..a87cc900c4 100644 --- a/package.json +++ b/package.json @@ -43,6 +43,7 @@ "packageManager": "pnpm@11.1.2", "devDependencies": { "@types/node": "catalog:", + "@types/proper-lockfile": "4.1.4", "@typescript/native-preview": "catalog:", "husky": "9.1.7", "ink": "6.8.0", @@ -50,6 +51,7 @@ "oxlint": "1.55.0", "oxlint-plugin-react-native": "0.2.17", "oxlint-tsgolint": "0.17.4", + "proper-lockfile": "4.1.2", "react": "19.2.6", "tsx": "catalog:", "typescript": "catalog:" diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 4ab8de0f67..796a3d5d83 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -139,6 +139,9 @@ importers: '@types/node': specifier: 'catalog:' version: 24.12.4 + '@types/proper-lockfile': + specifier: 4.1.4 + version: 4.1.4 '@typescript/native-preview': specifier: 'catalog:' version: 7.0.0-dev.20260514.1 @@ -160,6 +163,9 @@ importers: oxlint-tsgolint: specifier: 0.17.4 version: 0.17.4 + proper-lockfile: + specifier: 4.1.2 + version: 4.1.2 react: specifier: 19.2.6 version: 19.2.6 @@ -9365,6 +9371,9 @@ packages: '@types/pg@8.18.0': resolution: {integrity: sha512-gT+oueVQkqnj6ajGJXblFR4iavIXWsGAFCk3dP4Kki5+a9R4NMt0JARdk6s8cUKcfUoqP5dAtDSLU8xYUTFV+Q==} + '@types/proper-lockfile@4.1.4': + resolution: {integrity: sha512-uo2ABllncSqg9F1D4nugVl9v93RmjxF6LJzQLMLDdPaXCUIDPeOJ21Gbqi43xNKzBi/WQ0Q0dICqufzQbMjipQ==} + '@types/react-dom@19.2.3': resolution: {integrity: sha512-jp2L/eY6fn+KgVVQAOqYItbF0VY/YApe5Mz2F0aykSO8gx31bYCZyvSeYxCHKvzHG5eZjc+zyaS5BrBWya2+kQ==} peerDependencies: @@ -19655,7 +19664,7 @@ snapshots: cjs-module-lexer: 1.2.3 esbuild: 0.27.4 miniflare: 4.20260603.0(bufferutil@4.1.0)(utf-8-validate@6.0.6) - vitest: 4.1.6(@opentelemetry/api@1.9.1)(@types/node@24.12.4)(@vitest/coverage-v8@4.1.6)(@vitest/ui@4.1.6)(esbuild@0.27.4)(jiti@2.7.0)(terser@5.46.0)(tsx@4.21.0)(yaml@2.8.4) + vitest: 4.1.6(@opentelemetry/api@1.9.1)(@types/node@25.5.2)(@vitest/coverage-v8@4.1.6)(@vitest/ui@4.1.6)(esbuild@0.27.4)(jiti@2.7.0)(terser@5.46.0)(tsx@4.21.0)(yaml@2.8.4) wrangler: 4.98.0(@cloudflare/workers-types@4.20260605.1)(bufferutil@4.1.0)(utf-8-validate@6.0.6) zod: 3.25.76 transitivePeerDependencies: @@ -25853,6 +25862,10 @@ snapshots: pg-protocol: 1.13.0 pg-types: 2.2.0 + '@types/proper-lockfile@4.1.4': + dependencies: + '@types/retry': 0.12.0 + '@types/react-dom@19.2.3(@types/react@19.2.14)': dependencies: '@types/react': 19.2.14 From e667424395f2d31e7b0bb6fa1665c3e559221fab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Wed, 15 Jul 2026 00:46:57 +0200 Subject: [PATCH 2/7] fix(dev): verify shared mobile services --- dev/local/cli.ts | 4 +- dev/local/docker-api-probe.test.ts | 49 +++++++++++++++++++++ dev/local/docker-api-probe.ts | 14 ++++++ dev/local/runner.ts | 4 +- dev/local/services.test.ts | 11 +---- dev/local/tmux.test.ts | 71 ++++++++++++++++++++++++++++++ 6 files changed, 141 insertions(+), 12 deletions(-) create mode 100644 dev/local/docker-api-probe.test.ts create mode 100644 dev/local/docker-api-probe.ts diff --git a/dev/local/cli.ts b/dev/local/cli.ts index 7733537232..70b5e6ce05 100644 --- a/dev/local/cli.ts +++ b/dev/local/cli.ts @@ -38,6 +38,7 @@ import { captureServicePane, } from './tmux'; import { detectLanIp, prepareMobileEnvironment } from './mobile-env'; +import { probeDockerApi } from './docker-api-probe'; import { findRepoRoot, startServiceInTmux, @@ -189,7 +190,8 @@ async function cmdUp(args: string[], repoRoot: string): Promise { const service = getService(name); if (service.type !== 'infra' && service.port > 0 && (await probePort(service.port))) { if (name === 'kiloclaw-docker-tcp') { - reusedHostServices.add(name); + if (await probeDockerApi(service.port)) reusedHostServices.add(name); + else conflictingPorts.push(`${name}:${service.port}`); } else { conflictingPorts.push(`${name}:${service.port}`); } diff --git a/dev/local/docker-api-probe.test.ts b/dev/local/docker-api-probe.test.ts new file mode 100644 index 0000000000..2dbc67d5f3 --- /dev/null +++ b/dev/local/docker-api-probe.test.ts @@ -0,0 +1,49 @@ +import assert from 'node:assert/strict'; +import * as http from 'node:http'; +import test from 'node:test'; + +import { probeDockerApi } from './docker-api-probe'; + +async function listen(server: http.Server): Promise { + await new Promise((resolve, reject) => { + server.listen(0, '127.0.0.1', resolve); + server.once('error', reject); + }); + const address = server.address(); + assert.ok(address && typeof address !== 'string'); + return address.port; +} + +async function close(server: http.Server): Promise { + await new Promise((resolve, reject) => { + server.close(error => (error ? reject(error) : resolve())); + }); +} + +test('does not identify an arbitrary TCP listener as the Docker API proxy', async () => { + const server = http.createServer((_request, response) => { + response.end('not docker'); + }); + const port = await listen(server); + + try { + assert.equal(await probeDockerApi(port), false); + } finally { + await close(server); + } +}); + +test('identifies a ready Docker API listener by its ping response', async () => { + const server = http.createServer((request, response) => { + assert.equal(request.url, '/_ping'); + response.setHeader('Api-Version', '1.48'); + response.end('OK'); + }); + const port = await listen(server); + + try { + assert.equal(await probeDockerApi(port), true); + } finally { + await close(server); + } +}); diff --git a/dev/local/docker-api-probe.ts b/dev/local/docker-api-probe.ts new file mode 100644 index 0000000000..7101a67747 --- /dev/null +++ b/dev/local/docker-api-probe.ts @@ -0,0 +1,14 @@ +export async function probeDockerApi(port: number, timeoutMs = 500): Promise { + try { + const response = await fetch(`http://127.0.0.1:${port}/_ping`, { + signal: AbortSignal.timeout(timeoutMs), + }); + return ( + response.status === 200 && + response.headers.has('api-version') && + (await response.text()).trim() === 'OK' + ); + } catch { + return false; + } +} diff --git a/dev/local/runner.ts b/dev/local/runner.ts index b37fa7a3cd..24c9406e4b 100644 --- a/dev/local/runner.ts +++ b/dev/local/runner.ts @@ -369,13 +369,13 @@ export function restartServiceInTmux( return; } try { - const envPrefix = Object.entries(env ?? {}) + const envArgs = Object.entries(env ?? {}) .map(([key, value]) => `${key}=${shellQuote(value)}`) .join(' '); sendKeys( sessionName, currentPane.windowIndex, - envPrefix ? `${envPrefix} ${cmd}` : cmd, + envArgs ? `env ${envArgs} ${cmd}` : cmd, currentPane.paneIndex ); settle('relaunched'); diff --git a/dev/local/services.test.ts b/dev/local/services.test.ts index c473bd780a..b102636bba 100644 --- a/dev/local/services.test.ts +++ b/dev/local/services.test.ts @@ -6,6 +6,7 @@ import { computePortOffset, getAlwaysOnGroupIds, getService, + portOffset, resolveGroups, resolveSessionNextAuthUrl, } from './services'; @@ -71,15 +72,7 @@ test('keeps auto routing workers in their own opt-in group', () => { assert.equal(service.group, 'auto-routing'); assert.equal(service.type, 'worker'); assert.equal(service.dir, 'services/auto-routing'); - assert.equal( - service.port, - 8810 + - computePortOffset({ - explicit: process.env.KILO_PORT_OFFSET, - isPrimary: false, - slug: 'harden-mobile-agent-workflow', - }) - ); + assert.equal(service.port, 8810 + portOffset); assert.match(service.command.join(' '), /pnpm run dev/); const benchmark = getService('auto-routing-benchmark'); diff --git a/dev/local/tmux.test.ts b/dev/local/tmux.test.ts index 3b6648d56b..3cfd5ff99c 100644 --- a/dev/local/tmux.test.ts +++ b/dev/local/tmux.test.ts @@ -180,6 +180,77 @@ test( } ); +test( + 'restartServiceInTmux injects escaped env values into a non-POSIX shell', + { skip: !hasTmux || !fs.existsSync('/bin/tcsh') }, + async () => { + const sessionName = `kilo-tmux-test-${process.pid}-${Date.now()}`; + const serviceName = 'stripe'; + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-tmux-test-env-')); + const marker = path.join(tempDir, 'env-value'); + const envValue = "space ' quote $dollar; semicolon"; + const fakeTsx = path.join(tempDir, 'tsx'); + const foregroundProcess = path.join(tempDir, 'foreground-process'); + const tmux = (...args: string[]) => execFileSync('tmux', args, { stdio: 'ignore' }); + + fs.writeFileSync(fakeTsx, `#!/bin/sh\nprintf '%s' "$KILO_TEST_VALUE" > "$KILO_TEST_MARKER"\n`); + fs.writeFileSync( + foregroundProcess, + "#!/bin/sh\ntrap 'exit 0' INT TERM\nwhile :; do sleep 1; done\n" + ); + fs.chmodSync(fakeTsx, 0o755); + fs.chmodSync(foregroundProcess, 0o755); + + try { + tmux('new-session', '-d', '-s', sessionName, '-n', 'dashboard', 'sleep 120'); + tmux('new-window', '-d', '-t', sessionName, '-n', serviceName, '/bin/tcsh'); + + const serviceWindow = listWindows(sessionName).find(window => window.name === serviceName); + assert.ok(serviceWindow); + + tmux( + 'send-keys', + '-t', + `${sessionName}:${serviceWindow.index}.0`, + foregroundProcess, + 'Enter' + ); + let fixtureUp = false; + for (let i = 0; i < 20 && !fixtureUp; i++) { + const panePid = execFileSync( + 'tmux', + ['display-message', '-p', '-t', `${sessionName}:${serviceWindow.index}.0`, '#{pane_pid}'], + { encoding: 'utf-8' } + ).trim(); + try { + execFileSync('pgrep', ['-P', panePid], { stdio: 'ignore' }); + fixtureUp = true; + } catch { + await sleep(50); + } + } + assert.ok(fixtureUp, 'foreground process should start under tcsh'); + + const outcome = await restartServiceInTmux(sessionName, serviceName, { + KILO_TEST_MARKER: marker, + KILO_TEST_VALUE: envValue, + PATH: `${tempDir}:${process.env.PATH ?? ''}`, + }); + + assert.equal(outcome, 'relaunched'); + for (let i = 0; i < 20 && !fs.existsSync(marker); i++) await sleep(50); + assert.equal(fs.readFileSync(marker, 'utf-8'), envValue); + } finally { + fs.rmSync(tempDir, { force: true, recursive: true }); + try { + tmux('kill-session', '-t', sessionName); + } catch { + // Session may already be gone if tmux fails during setup. + } + } + } +); + test( 'restartServiceInTmux waits for a slow shutdown before sending the relaunch command', { skip: !hasTmux }, From 8f29bc0a80eb384fa0581cc6525c9b2b09ebaafb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Wed, 15 Jul 2026 00:47:04 +0200 Subject: [PATCH 3/7] fix(mobile): make agent E2E flows repeatable --- apps/mobile/.kilo/MOBILE_WORKFLOW.md | 11 ++++- apps/mobile/e2e/AGENTS.md | 15 ++----- apps/mobile/e2e/flows/logout.yaml | 7 ++- apps/mobile/e2e/flows/open-app.yaml | 14 +++++- apps/mobile/e2e/flows/settle-app.yaml | 9 +++- dev/local/mobile-workflow.test.ts | 65 +++++++++++++++++++++++++++ 6 files changed, 102 insertions(+), 19 deletions(-) diff --git a/apps/mobile/.kilo/MOBILE_WORKFLOW.md b/apps/mobile/.kilo/MOBILE_WORKFLOW.md index 32c13b47a9..261bf7f417 100644 --- a/apps/mobile/.kilo/MOBILE_WORKFLOW.md +++ b/apps/mobile/.kilo/MOBILE_WORKFLOW.md @@ -37,7 +37,11 @@ Reviewer and verifier invocations must be fresh sessions so earlier conclusions 6. Stop after three repair rounds if findings remain. The main session takes over or asks the user to resolve the underlying ambiguity; never loop indefinitely. 7. Once review has no valid findings, dispatch `mobile-e2e-verifier` with observable acceptance criteria and the intended worktree/service context. 8. Route product failures through implementer and reviewer again. Let the verifier attempt one recovery for environment failures. The main session classifies inconclusive results before deciding whether code should change. -9. The main session performs the final full-diff review and repository-appropriate verification, commits any final narrowly scoped repair, then pushes and creates or updates the PR. Do not squash the work into a catch-all commit unless the user explicitly requests it. +9. The main session performs the final full-diff review and repository-appropriate verification, commits any final narrowly scoped repair, then pushes and creates or updates the PR. Assign the PR to the requesting human. Do not squash the work into a catch-all commit unless the user explicitly requests it. +10. Wait until Kilobot has reviewed the latest head. Fetch every Kilobot review thread, including comments that arrive after earlier repairs, and triage each finding in the main session using the repository-root `AGENTS.md` review-remark workflow. +11. For each valid finding, plan the smallest coherent repair and send that bounded task to `mobile-implementer`. Run the required narrow checks, dispatch a fresh `mobile-reviewer`, and create the smallest coherent commit before pushing. Reply in the original review thread with the concrete fix, then resolve the thread. Reject invalid findings with technical evidence in the same thread instead of changing correct code. +12. Repeat the Kilobot triage, implementer, fresh reviewer, commit, push, reply, and resolution cycle until Kilobot has reviewed the latest head and there are no unresolved actionable Kilobot comments. Preserve the three-repair-round limit for any one finding; the main session takes over or asks the user if that limit is reached. +13. Run local mobile E2E again after Kilobot repairs that affect behavior, build/runtime configuration, or the E2E workflow. Documentation-only or test-only repairs may skip repeated device E2E when the orchestrator records why the previously verified behavior is unaffected. ## Handoff Requirements @@ -51,8 +55,9 @@ Every dispatch should include: - Exact checks or user flows expected for that stage - Prior findings being addressed, including rejected findings that must not be reopened without new evidence - The intended commit boundary for the assigned slice +- A prohibition on reading secret-bearing environment files: role agents must not read `.env`, `.env.*`, `.dev.vars`, or equivalent files. Use documented setup commands, sanitized status or manifest output, and sanitized explicit values supplied by the orchestrator instead. -Do not ask a role agent to infer context from the conversation it cannot see. Keep cross-repository changes on coordinated branches or working trees, and give the reviewer and verifier the location of every related diff. +Do not ask a role agent to infer context from the conversation it cannot see. Keep cross-repository changes on coordinated branches or working trees, and give the reviewer and verifier the location of every related diff. Never place secrets or raw environment-file contents in a handoff; provide only the minimum sanitized explicit values required for the task. ## Completion Gate @@ -66,3 +71,5 @@ The orchestrator may call the work complete only when: - E2E acceptance criteria pass, or a documented environment blocker is explicitly accepted by the user - Final automated checks pass in every changed repository - The main session has reviewed the complete diff and owns the Git/PR actions +- The PR is assigned to the requesting human +- Kilobot has reviewed the latest head and there are no unresolved actionable Kilobot comments diff --git a/apps/mobile/e2e/AGENTS.md b/apps/mobile/e2e/AGENTS.md index 2008fb03ab..1cdf9efbb4 100644 --- a/apps/mobile/e2e/AGENTS.md +++ b/apps/mobile/e2e/AGENTS.md @@ -85,7 +85,7 @@ xcrun simctl openurl \ "exp+kilo-app://expo-development-client/?url=http%3A%2F%2F%3A" ``` -Before testing, capture the `mobile` pane and verify `Starting project at /apps/mobile` plus a fresh `iOS Bundled` line. Seeing the Kilo login screen alone does not prove bundle provenance. The login preflight also reads Metro's development manifest and verifies `expoConfig.extra.apiBaseUrl` and `_internal.projectRoot` against this worktree. These endpoint extras come from the Metro manifest in a dev client; after env changes, regenerate env, restart Metro, reconnect the dev client to the exact Metro URL, and reload. Rebuild only when native config/plugins changed. The login helper dismisses the clean-install tracking alert and Expo dev-menu introduction when present. +Before testing, capture the `mobile` pane and verify `Starting project at /apps/mobile` plus a fresh `iOS Bundled` line. Seeing the Kilo login screen alone does not prove bundle provenance. The login preflight also reads Metro's development manifest and verifies `expoConfig.extra.apiBaseUrl` and `_internal.projectRoot` against this worktree. These endpoint extras come from the Metro manifest in a dev client; after env changes, regenerate env, restart Metro, reconnect the dev client to the exact Metro URL, and reload. Rebuild only when native config/plugins changed. The shared launch flows dismiss the clean-install tracking alert, accept the Expo dev-menu introduction with `Continue`, and then close the full Expo/React Native developer menu containing Fast Refresh and Element Inspector with its `Close` accessibility action. ## Sign In and Out @@ -148,23 +148,14 @@ Attach a screenshot of the changed flow to the PR when it helps review. For tran ## Remote CLI Session Flows -Use this only when testing session discovery, mirroring, or mobile-to-CLI messaging. Install the CLI in a disposable directory, never globally: +Use this only when testing session discovery, mirroring, or mobile-to-CLI messaging. The orchestrator must mint the user's local auth token and pass it as `KILO_E2E_AUTH_TOKEN`; role agents must not read environment files or receive `NEXTAUTH_SECRET`. Install the CLI in a disposable directory, never globally: ```bash CLI_SCRATCH=$(mktemp -d /tmp/kilo-cli.XXXXXX) npm install --prefix "$CLI_SCRATCH" @kilocode/cli E2E_EMAIL=${E2E_EMAIL:-e2e-mobile@example.com} USER_ID=$(pnpm -s dev:seed app:user-id "$E2E_EMAIL" --json | jq -r .userId) -TOKEN=$(NEXTAUTH_SECRET=$(grep '^NEXTAUTH_SECRET=' .env.local | cut -d= -f2- | tr -d '"') \ - USER_ID="$USER_ID" node -e ' -const crypto = require("crypto"); -const b64 = value => Buffer.from(JSON.stringify(value)).toString("base64url"); -const header = b64({ alg: "HS256", typ: "JWT" }); -const payload = b64({ kiloUserId: process.env.USER_ID, apiTokenPepper: null, version: 3 }); -const signature = crypto.createHmac("sha256", process.env.NEXTAUTH_SECRET) - .update(`${header}.${payload}`).digest("base64url"); -process.stdout.write(`${header}.${payload}.${signature}`); -') +TOKEN="${KILO_E2E_AUTH_TOKEN:?orchestrator must provide KILO_E2E_AUTH_TOKEN}" ``` Do not print or log the token. Read the actual Next.js and session-ingest ports from `pnpm dev:status --json`, then run the CLI in its own tmux session: diff --git a/apps/mobile/e2e/flows/logout.yaml b/apps/mobile/e2e/flows/logout.yaml index 7ad6213627..49ca2d23c5 100644 --- a/apps/mobile/e2e/flows/logout.yaml +++ b/apps/mobile/e2e/flows/logout.yaml @@ -7,6 +7,11 @@ # See e2e/AGENTS.md for the full workflow. appId: com.kilocode.kiloapp --- +- runFlow: + when: + visible: 'Verify code' + commands: + - tapOn: 'Back' - runFlow: when: notVisible: 'Welcome to Kilo Code' @@ -43,4 +48,4 @@ appId: com.kilocode.kiloapp - tapOn: text: 'Sign out' index: 0 -- assertVisible: 'Welcome to Kilo Code' +- assertVisible: 'you@example.com' diff --git a/apps/mobile/e2e/flows/open-app.yaml b/apps/mobile/e2e/flows/open-app.yaml index aa34c935fd..0d0c980354 100644 --- a/apps/mobile/e2e/flows/open-app.yaml +++ b/apps/mobile/e2e/flows/open-app.yaml @@ -10,6 +10,11 @@ appId: com.kilocode.kiloapp visible: 'This is the developer menu.*' commands: - tapOn: 'Continue' +- runFlow: + when: + visible: 'Fast Refresh|Element Inspector' + commands: + - tapOn: 'Close' - stopApp - runFlow: when: @@ -18,10 +23,10 @@ appId: com.kilocode.kiloapp - tapOn: text: 'Kilo' - extendedWaitUntil: - visible: 'Allow “Kilo” to track your activity across other companies’ apps and websites\?|Ask App Not to Track|This is the developer menu.*|“Kilo” Would Like to Send You Notifications|HOME|Home, tab, 1 of 4|Welcome to Kilo Code|Accept and continue' + visible: 'Allow “Kilo” to track your activity across other companies’ apps and websites\?|Ask App Not to Track|This is the developer menu.*|Fast Refresh|Element Inspector|“Kilo” Would Like to Send You Notifications|HOME|Home, tab, 1 of 4|Welcome to Kilo Code|Accept and continue' timeout: 30000 - extendedWaitUntil: - visible: 'Ask App Not to Track|This is the developer menu.*|“Kilo” Would Like to Send You Notifications' + visible: 'Ask App Not to Track|This is the developer menu.*|Fast Refresh|Element Inspector|“Kilo” Would Like to Send You Notifications' timeout: 3000 optional: true - runFlow: @@ -38,6 +43,11 @@ appId: com.kilocode.kiloapp visible: 'This is the developer menu.*' commands: - tapOn: 'Continue' +- runFlow: + when: + visible: 'Fast Refresh|Element Inspector' + commands: + - tapOn: 'Close' - runFlow: when: visible: '“Kilo” Would Like to Send You Notifications' diff --git a/apps/mobile/e2e/flows/settle-app.yaml b/apps/mobile/e2e/flows/settle-app.yaml index d03b043e72..d5f75b4207 100644 --- a/apps/mobile/e2e/flows/settle-app.yaml +++ b/apps/mobile/e2e/flows/settle-app.yaml @@ -1,10 +1,10 @@ appId: com.kilocode.kiloapp --- - extendedWaitUntil: - visible: 'Allow “Kilo” to track your activity across other companies’ apps and websites\?|Ask App Not to Track|This is the developer menu.*|“Kilo” Would Like to Send You Notifications|HOME|Home, tab, 1 of 4|Welcome to Kilo Code|Accept and continue' + visible: 'Allow “Kilo” to track your activity across other companies’ apps and websites\?|Ask App Not to Track|This is the developer menu.*|Fast Refresh|Element Inspector|“Kilo” Would Like to Send You Notifications|HOME|Home, tab, 1 of 4|Welcome to Kilo Code|Accept and continue' timeout: 15000 - extendedWaitUntil: - visible: 'Ask App Not to Track|This is the developer menu.*|“Kilo” Would Like to Send You Notifications' + visible: 'Ask App Not to Track|This is the developer menu.*|Fast Refresh|Element Inspector|“Kilo” Would Like to Send You Notifications' timeout: 3000 optional: true - runFlow: @@ -17,6 +17,11 @@ appId: com.kilocode.kiloapp visible: 'This is the developer menu.*' commands: - tapOn: 'Continue' +- runFlow: + when: + visible: 'Fast Refresh|Element Inspector' + commands: + - tapOn: 'Close' - runFlow: when: visible: '“Kilo” Would Like to Send You Notifications' diff --git a/dev/local/mobile-workflow.test.ts b/dev/local/mobile-workflow.test.ts index 7c9bfd1c35..42b861dcbc 100644 --- a/dev/local/mobile-workflow.test.ts +++ b/dev/local/mobile-workflow.test.ts @@ -19,6 +19,30 @@ test('login waits for the delayed Expo developer menu after launching Kilo', () assert.match(flow.slice(optionalWaitIndex, continueIndex), /optional: true/); }); +test('shared launch flows close the Expo developer menu after its introduction', () => { + const runbook = fs.readFileSync('apps/mobile/e2e/AGENTS.md', 'utf8'); + + for (const flowPath of [ + 'apps/mobile/e2e/flows/open-app.yaml', + 'apps/mobile/e2e/flows/settle-app.yaml', + ]) { + const flow = fs.readFileSync(flowPath, 'utf8'); + const continueIndex = flow.indexOf("tapOn: 'Continue'"); + const closeGuardIndex = flow.indexOf("visible: 'Fast Refresh|Element Inspector'", continueIndex); + const closeIndex = flow.indexOf("tapOn: 'Close'", closeGuardIndex); + + assert.ok(continueIndex >= 0, `${flowPath} should accept the developer-menu introduction`); + assert.ok(closeGuardIndex > continueIndex, `${flowPath} should detect the opened menu`); + assert.ok(closeIndex > closeGuardIndex, `${flowPath} should close the opened menu`); + assert.doesNotMatch(flow, /when:\n\s+visible: 'Close'/); + } + + assert.match( + runbook, + /developer menu containing Fast Refresh and Element Inspector with its `Close` accessibility action/ + ); +}); + test('login flows never use an unidentified generic Allow selector', () => { const request = fs.readFileSync('apps/mobile/e2e/flows/login-request-code.yaml', 'utf8'); const openApp = fs.readFileSync('apps/mobile/e2e/flows/open-app.yaml', 'utf8'); @@ -39,6 +63,21 @@ test('login request establishes a signed-out baseline before requesting a fresh assert.ok(request.indexOf('logout.yaml') < request.indexOf("tapOn: 'Send sign-in code'")); }); +test('login retry resets an already-open verification screen to the email form', () => { + const logout = fs.readFileSync('apps/mobile/e2e/flows/logout.yaml', 'utf8'); + const verificationIndex = logout.indexOf("visible: 'Verify code'"); + const backIndex = logout.indexOf("tapOn: 'Back'", verificationIndex); + const signedOutGuardIndex = logout.indexOf( + "notVisible: 'Welcome to Kilo Code'", + verificationIndex + ); + + assert.ok(verificationIndex >= 0); + assert.ok(backIndex > verificationIndex); + assert.ok(signedOutGuardIndex > backIndex); + assert.match(logout, /assertVisible: 'you@example\.com'/); +}); + test('login reuses the app state established by logout instead of relaunching each step', () => { const request = fs.readFileSync('apps/mobile/e2e/flows/login-request-code.yaml', 'utf8'); const verify = fs.readFileSync('apps/mobile/e2e/flows/login-verify-code.yaml', 'utf8'); @@ -207,3 +246,29 @@ test('workflow documents the shared Docker proxy exception without weakening bac assert.match(cli, /name === 'kiloclaw-docker-tcp'/); assert.match(cli, /Refusing to share occupied worktree service ports/); }); + +test('mobile workflow owns PR assignment and the post-PR Kilobot repair loop', () => { + const workflow = fs.readFileSync('apps/mobile/.kilo/MOBILE_WORKFLOW.md', 'utf8'); + + assert.match(workflow, /assign(?:s|ed)? the PR to the requesting human/i); + assert.match(workflow, /Kilobot has reviewed the latest head/i); + assert.match(workflow, /no unresolved actionable Kilobot comments/i); + assert.match(workflow, /plan the smallest coherent repair/i); + assert.match(workflow, /mobile-implementer/); + assert.match(workflow, /fresh `mobile-reviewer`/); + assert.match(workflow, /smallest coherent commit/i); + assert.match(workflow, /reply in the original review thread/i); + assert.match(workflow, /resolve the thread/i); + assert.match(workflow, /local mobile E2E/i); +}); + +test('mobile workflow keeps secret-bearing environment files out of subagent context', () => { + const workflow = fs.readFileSync('apps/mobile/.kilo/MOBILE_WORKFLOW.md', 'utf8'); + const runbook = fs.readFileSync('apps/mobile/e2e/AGENTS.md', 'utf8'); + + assert.match(workflow, /must not read .*\.env/i); + assert.match(workflow, /\.dev\.vars/); + assert.match(workflow, /sanitized explicit values/i); + assert.doesNotMatch(runbook, /grep .*\.env/); + assert.match(runbook, /KILO_E2E_AUTH_TOKEN/); +}); From 1e7de5e1724b0a0b73e9d417e608d7882f391dbd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Wed, 15 Jul 2026 00:49:17 +0200 Subject: [PATCH 4/7] style(mobile): format workflow regression test --- dev/local/mobile-workflow.test.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/dev/local/mobile-workflow.test.ts b/dev/local/mobile-workflow.test.ts index 42b861dcbc..fb1ae998e3 100644 --- a/dev/local/mobile-workflow.test.ts +++ b/dev/local/mobile-workflow.test.ts @@ -28,7 +28,10 @@ test('shared launch flows close the Expo developer menu after its introduction', ]) { const flow = fs.readFileSync(flowPath, 'utf8'); const continueIndex = flow.indexOf("tapOn: 'Continue'"); - const closeGuardIndex = flow.indexOf("visible: 'Fast Refresh|Element Inspector'", continueIndex); + const closeGuardIndex = flow.indexOf( + "visible: 'Fast Refresh|Element Inspector'", + continueIndex + ); const closeIndex = flow.indexOf("tapOn: 'Close'", closeGuardIndex); assert.ok(continueIndex >= 0, `${flowPath} should accept the developer-menu introduction`); From 40ca727bb85a0b816bb38b1ff39562ca0383ddee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Wed, 15 Jul 2026 02:25:21 +0200 Subject: [PATCH 5/7] fix(mobile): roll back failed simulator preparation --- apps/mobile/e2e/AGENTS.md | 30 +--- dev/local/mobile-simulator.test.ts | 237 ++++++++++++++++++++++++++++- dev/local/mobile-simulator.ts | 153 +++++++++++++++---- dev/local/mobile-workflow.test.ts | 29 +++- 4 files changed, 395 insertions(+), 54 deletions(-) diff --git a/apps/mobile/e2e/AGENTS.md b/apps/mobile/e2e/AGENTS.md index 1cdf9efbb4..e4734c842d 100644 --- a/apps/mobile/e2e/AGENTS.md +++ b/apps/mobile/e2e/AGENTS.md @@ -148,32 +148,16 @@ Attach a screenshot of the changed flow to the PR when it helps review. For tran ## Remote CLI Session Flows -Use this only when testing session discovery, mirroring, or mobile-to-CLI messaging. The orchestrator must mint the user's local auth token and pass it as `KILO_E2E_AUTH_TOKEN`; role agents must not read environment files or receive `NEXTAUTH_SECRET`. Install the CLI in a disposable directory, never globally: +Use this only when testing session discovery, mirroring, or mobile-to-CLI messaging. The orchestrator mints the user's local auth token, installs the CLI in a disposable directory, and starts it in a `kilo-e2e-cli-$(basename "$PWD")` tmux session with the required API URLs and bearer-token environment already set. Role agents must not read environment files, accept a bearer token, install the CLI, or run `wrangler` commands. Reuse the orchestrator-prepared session and verify session discovery and mirroring by inspecting its pane and the mobile list: ```bash -CLI_SCRATCH=$(mktemp -d /tmp/kilo-cli.XXXXXX) -npm install --prefix "$CLI_SCRATCH" @kilocode/cli -E2E_EMAIL=${E2E_EMAIL:-e2e-mobile@example.com} -USER_ID=$(pnpm -s dev:seed app:user-id "$E2E_EMAIL" --json | jq -r .userId) -TOKEN="${KILO_E2E_AUTH_TOKEN:?orchestrator must provide KILO_E2E_AUTH_TOKEN}" -``` - -Do not print or log the token. Read the actual Next.js and session-ingest ports from `pnpm dev:status --json`, then run the CLI in its own tmux session: - -```bash -CLI_SESSION="kilo-e2e-$(basename "$PWD")" -tmux new-session -d -s "$CLI_SESSION" -c "$PWD" -tmux set-environment -t "$CLI_SESSION" KILO_API_URL http://localhost: -tmux set-environment -t "$CLI_SESSION" KILO_SESSION_INGEST_URL http://localhost: -tmux set-environment -t "$CLI_SESSION" KILO_AUTH_CONTENT \ - "$(printf '{"kilo":{"type":"api","key":"%s"}}' "$TOKEN")" -tmux set-environment -t "$CLI_SESSION" KILO_REMOTE 1 -tmux set-environment -t "$CLI_SESSION" KILO_CLI_BIN "$CLI_SCRATCH/node_modules/.bin/kilo" -tmux new-window -t "$CLI_SESSION" -n cli -c "$PWD" '"$KILO_CLI_BIN"' +CLI_SESSION="kilo-e2e-cli-$(basename "$PWD")" +tmux ls +tmux list-windows -t "$CLI_SESSION" tmux capture-pane -p -t "$CLI_SESSION":cli -S -100 ``` -The mobile list updates after the CLI WebSocket connects and its first heartbeat (usually about 12 seconds). Use `tmux send-keys` for automation; slash commands need one Enter for autocomplete and another to submit. +Drive the orchestrator-prepared session with `tmux send-keys`; slash commands need one Enter for autocomplete and another to submit. The mobile list updates after the CLI WebSocket connects and its first heartbeat (usually about 12 seconds). If the orchestrator has not prepared a session for this worktree, stop and ask the orchestrator to install the CLI, mint a token, and start the session before exercising CLI flows. ## Android Emulator @@ -215,13 +199,11 @@ The existing login/logout helpers accept either an iOS simulator UDID or an Andr ## Cleanup -Clean up only resources you started: +Clean up only resources you started. The remote CLI session and its disposable install are owned by the orchestrator; do not kill `kilo-e2e-cli-*` sessions or remove CLI scratch directories you did not create: ```bash -tmux kill-session -t "$CLI_SESSION" # if created tmux kill-session -t "$IOS_BUILD_SESSION" # if created tmux kill-session -t "$ANDROID_SESSION" # if created -rm -rf "$CLI_SCRATCH" # if created rm -f "$LOGIN_LOG" # if created pnpm dev:stop # only if you started this worktree's stack xcrun simctl shutdown # only if you booted it diff --git a/dev/local/mobile-simulator.test.ts b/dev/local/mobile-simulator.test.ts index affce92aba..07ae67a630 100644 --- a/dev/local/mobile-simulator.test.ts +++ b/dev/local/mobile-simulator.test.ts @@ -4,7 +4,46 @@ import os from 'node:os'; import path from 'node:path'; import test from 'node:test'; -import { claimSimulator, releaseSimulator, type SimulatorDevice } from './mobile-simulator'; +import { + bootSimulator, + claimSimulator, + releaseSimulator, + type SimulatorDevice, +} from './mobile-simulator'; + +type ExecCall = { command: string; args: readonly string[]; options: unknown }; +type OutputCall = { command: string; args: readonly string[] }; +type CommandResult = { stdout: string; stderr: string; status: number | null }; + +function recordingExec( + behavior: (call: ExecCall) => Error | undefined +): (command: string, args: readonly string[], options: unknown) => Buffer { + const calls: ExecCall[] = []; + const exec = ((command: string, args: readonly string[], options: unknown) => { + const call: ExecCall = { command, args, options }; + calls.push(call); + const result = behavior(call); + if (result) throw result; + return Buffer.from(''); + }) as (command: string, args: readonly string[], options: unknown) => Buffer; + (exec as { calls?: ExecCall[] }).calls = calls; + return exec; +} + +function callsOf(exec: ReturnType): string[] { + return (exec as unknown as { calls: ExecCall[] }).calls.map(call => { + const action = call.args[1] ?? ''; + return `${action} ${call.args.slice(2).join(' ')}`.trim(); + }); +} + +type OutputBehavior = (call: OutputCall) => CommandResult; + +function recordingOutput( + behavior: OutputBehavior +): (command: string, args: readonly string[]) => CommandResult { + return (command: string, args: readonly string[]) => behavior({ command, args }); +} const devices: SimulatorDevice[] = [ { id: 'A', name: 'Kilo E2E-A', state: 'Booted' }, @@ -199,3 +238,199 @@ test('recovers an orphaned iOS claim mutation lock', () => { fs.rmSync(worktreeRoot, { recursive: true, force: true }); } }); + +test('blocks a same-worktree adoption during prepare so a failed first prepare cannot delete the adopted claim', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + let secondClaimError: unknown = null; + let secondClaimResult: ReturnType | null = null; + let firstClaimError: unknown = null; + + try { + claimSimulator({ + devices, + lockRoot, + worktreeRoot, + requestedId: 'B', + prepare: () => { + // While the first call's prepare is running, a second same-worktree + // claim attempt must be blocked by the device mutation lock instead of + // adopting the claim that the first call owns. + try { + secondClaimResult = claimSimulator({ + devices, + lockRoot, + worktreeRoot, + requestedId: 'B', + }); + } catch (error) { + secondClaimError = error; + } + throw new Error('bootstatus failed'); + }, + }); + } catch (error) { + firstClaimError = error; + } + + assert.match(String(firstClaimError), /bootstatus failed/); + assert.equal(secondClaimResult, null); + assert.match(String(secondClaimError), /claim is being updated concurrently/); +}); + +test('shuts down a simulator booted by this attempt when bootstatus fails', () => { + const device = { id: 'B', name: 'Kilo E2E-B', state: 'Shutdown' }; + const exec = recordingExec(() => undefined); + const runWithOutput = recordingOutput(() => { + throw new Error('bootstatus failed'); + }); + + assert.throws(() => bootSimulator(device, exec, runWithOutput), /bootstatus failed/); + assert.deepEqual(callsOf(exec), ['boot B', 'shutdown B']); +}); + +test('does not shut down when boot fails', () => { + const device = { id: 'B', name: 'Kilo E2E-B', state: 'Shutdown' }; + const exec = recordingExec(call => { + if (call.args[1] === 'boot') return new Error('boot failed'); + return undefined; + }); + const runWithOutput = recordingOutput(() => { + throw new Error('bootstatus should not be reached'); + }); + + assert.throws(() => bootSimulator(device, exec, runWithOutput), /boot failed/); + assert.deepEqual(callsOf(exec), ['boot B']); +}); + +test('does not boot or shut down an already-booted simulator', () => { + const device = { id: 'A', name: 'Kilo E2E-A', state: 'Booted' }; + const exec = recordingExec(() => undefined); + const runWithOutput = recordingOutput(() => { + throw new Error('runWithOutput should not be reached'); + }); + + bootSimulator(device, exec, runWithOutput); + assert.deepEqual(callsOf(exec), []); +}); + +test('does not shut down a simulator when boot and bootstatus both succeed', () => { + const device = { id: 'B', name: 'Kilo E2E-B', state: 'Shutdown' }; + const exec = recordingExec(() => undefined); + const runWithOutput = recordingOutput(() => ({ + stdout: 'Status=0, isTerminal=YES', + stderr: '', + status: 0, + })); + + bootSimulator(device, exec, runWithOutput); + assert.deepEqual(callsOf(exec), ['boot B']); +}); + +test('shut down after bootstatus failure swallows the shutdown error so the original cause surfaces', () => { + const device = { id: 'B', name: 'Kilo E2E-B', state: 'Shutdown' }; + const exec = recordingExec(call => { + if (call.args[1] === 'shutdown') return new Error('shutdown failed'); + return undefined; + }); + const runWithOutput = recordingOutput(() => { + throw new Error('bootstatus failed'); + }); + + assert.throws(() => bootSimulator(device, exec, runWithOutput), /bootstatus failed/); + assert.deepEqual(callsOf(exec), ['boot B', 'shutdown B']); +}); + +test('rejects bootstatus with terminal Data Migration Failed output even when exit is 0', () => { + const device = { id: 'B', name: 'Kilo E2E-B', state: 'Shutdown' }; + const exec = recordingExec(() => undefined); + const runWithOutput = recordingOutput(() => ({ + stdout: 'Status=3, isTerminal=YES\nData Migration Failed\n', + stderr: '', + status: 0, + })); + + assert.throws( + () => bootSimulator(device, exec, runWithOutput), + /bootstatus reported terminal failure[\s\S]*Data Migration Failed/ + ); + assert.deepEqual(callsOf(exec), ['boot B', 'shutdown B']); +}); + +test('rejects bootstatus when terminal failure appears on stderr only', () => { + const device = { id: 'B', name: 'Kilo E2E-B', state: 'Shutdown' }; + const exec = recordingExec(() => undefined); + const runWithOutput = recordingOutput(() => ({ + stdout: 'Status=0, isTerminal=NO\n', + stderr: 'Status=3, isTerminal=YES\nData Migration Failed\n', + status: 0, + })); + + assert.throws( + () => bootSimulator(device, exec, runWithOutput), + /bootstatus reported terminal failure/ + ); + assert.deepEqual(callsOf(exec), ['boot B', 'shutdown B']); +}); + +test('accepts bootstatus with successful terminal output and does not shut down', () => { + const device = { id: 'B', name: 'Kilo E2E-B', state: 'Shutdown' }; + const exec = recordingExec(() => undefined); + const runWithOutput = recordingOutput(() => ({ + stdout: 'Status=0, isTerminal=YES\nDevice booted.\n', + stderr: '', + status: 0, + })); + + bootSimulator(device, exec, runWithOutput); + assert.deepEqual(callsOf(exec), ['boot B']); +}); + +test('preserves non-zero bootstatus error precedence over a swallowed shutdown failure', () => { + const device = { id: 'B', name: 'Kilo E2E-B', state: 'Shutdown' }; + const exec = recordingExec(call => { + if (call.args[1] === 'shutdown') return new Error('shutdown failed'); + return undefined; + }); + const runWithOutput = recordingOutput(() => { + const error = new Error('xcrun simctl bootstatus B exited with status 1'); + (error as Error & { status?: number | null }).status = 1; + throw error; + }); + + assert.throws(() => bootSimulator(device, exec, runWithOutput), /exited with status 1/); + assert.deepEqual(callsOf(exec), ['boot B', 'shutdown B']); +}); + +test('rolls back the iOS claim when bootstatus reports a terminal failure', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + + try { + assert.throws( + () => + claimSimulator({ + devices, + lockRoot, + worktreeRoot, + requestedId: 'B', + prepare: () => { + // Simulate the real failure path: bootSimulator sees a terminal + // bootstatus output, throws, and rolls back the claim. + const exec = recordingExec(() => undefined); + const runWithOutput = recordingOutput(() => ({ + stdout: 'Status=3, isTerminal=YES\nData Migration Failed\n', + stderr: '', + status: 0, + })); + bootSimulator({ id: 'B', name: 'Kilo E2E-B', state: 'Shutdown' }, exec, runWithOutput); + }, + }), + /bootstatus reported terminal failure/ + ); + assert.equal(fs.existsSync(path.join(lockRoot, 'B.json')), false); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); diff --git a/dev/local/mobile-simulator.ts b/dev/local/mobile-simulator.ts index 977a204ae4..0aff9e6815 100644 --- a/dev/local/mobile-simulator.ts +++ b/dev/local/mobile-simulator.ts @@ -1,4 +1,8 @@ -import { execFileSync } from 'node:child_process'; +import { + execFileSync, + spawnSync, + type ExecFileSyncOptionsWithStringEncoding, +} from 'node:child_process'; import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; @@ -6,6 +10,17 @@ import path from 'node:path'; import { withProcessLock } from './process-lock'; type SimulatorDevice = { id: string; name: string; state: string }; +type ExecFn = ( + command: string, + args: readonly string[], + options: ExecFileSyncOptionsWithStringEncoding +) => string | Buffer; +type CommandResult = { + stdout: string; + stderr: string; + status: number | null; +}; +type ExecWithOutputFn = (command: string, args: readonly string[]) => CommandResult; type ClaimArgs = { devices: SimulatorDevice[]; lockRoot: string; @@ -45,6 +60,82 @@ function withClaimMutationLock(lockRoot: string, deviceId: string, mutate: () return withProcessLock(mutationLockPath, `Simulator ${deviceId} claim`, mutate); } +// Run a command and return its captured stdout, stderr, and exit status. Throws +// when the command exits non-zero so the existing thrown-error handling path +// still surfaces non-zero failures. The caller is responsible for parsing the +// captured output for terminal-failure indicators that can appear with exit 0. +function execWithOutput(command: string, args: readonly string[]): CommandResult { + const result = spawnSync(command, args, { encoding: 'utf8' }); + if (result.error) throw result.error; + if (result.signal) { + throw new Error(`${command} ${args.join(' ')} terminated with ${result.signal}`); + } + if (result.status !== 0) { + const stderr = result.stderr ? result.stderr.toString() : ''; + const message = + stderr.trim() || `${command} ${args.join(' ')} exited with status ${result.status}`; + const error = new Error(message); + (error as Error & { status?: number | null }).status = result.status; + (error as Error & { stdout?: string }).stdout = result.stdout?.toString() ?? ''; + (error as Error & { stderr?: string }).stderr = stderr; + throw error; + } + return { + stdout: result.stdout ? result.stdout.toString() : '', + stderr: result.stderr ? result.stderr.toString() : '', + status: result.status, + }; +} + +// Detect a terminal bootstatus failure in captured output. `xcrun simctl +// bootstatus -b` can return exit 0 with `Status=3, isTerminal=YES` and a +// `Data Migration Failed` line on a corrupted simulator; treating that as +// success leaves a half-booted device in our claim. We match the specific +// status code 3 (shutdown) reported as terminal, or the explicit failure +// message — a terminal Status=0 (booted) is a success. +function isBootstatusTerminalFailure(result: CommandResult): boolean { + const combined = `${result.stdout}\n${result.stderr}`; + return /Status=3,?\s*isTerminal=YES/.test(combined) || /Data Migration Failed/.test(combined); +} + +// Boot a shutdown simulator and wait for it to finish booting. If the boot +// succeeded but the subsequent `bootstatus` blocked boot failed (whether via +// non-zero exit or a terminal-failure output line), shut down the simulator +// that this attempt just booted so a follow-up claim does not observe a +// "Booted" device we started. Never shut down a simulator that was already +// booted by someone else, and never shut down a simulator whose `boot` failed +// (it never started). +function bootSimulator( + device: SimulatorDevice, + exec: ExecFn = execFileSync, + runWithOutput: ExecWithOutputFn = execWithOutput +): void { + if (device.state === 'Booted') return; + let booted = false; + try { + exec('xcrun', ['simctl', 'boot', device.id], { stdio: 'ignore' }); + booted = true; + const result = runWithOutput('xcrun', ['simctl', 'bootstatus', device.id, '-b']); + if (isBootstatusTerminalFailure(result)) { + const combined = `${result.stdout}\n${result.stderr}`.trim(); + // Echo a bounded tail of the captured output so the user can see why the + // boot was rejected without flooding logs. + const bounded = combined.split('\n').slice(-20).join('\n'); + throw new Error(`Simulator ${device.id} bootstatus reported terminal failure:\n${bounded}`); + } + } catch (error) { + if (booted) { + try { + exec('xcrun', ['simctl', 'shutdown', device.id], { stdio: 'ignore' }); + } catch { + // Swallow shutdown failures so the original boot/bootstatus error + // surfaces to the caller. + } + } + throw error; + } +} + function claimSimulator(args: ClaimArgs): { device: SimulatorDevice; alreadyOwned: boolean } { const { devices, lockRoot, worktreeRoot, requestedId } = args; fs.mkdirSync(lockRoot, { recursive: true }); @@ -62,28 +153,38 @@ function claimSimulator(args: ClaimArgs): { device: SimulatorDevice; alreadyOwne device.id, args.fileOperations?.readFileSync ?? fs.readFileSync ); - if (owner === worktreeRoot) return { device, alreadyOwned: true }; - if (owner) throw new Error(`Simulator ${device.id} is claimed by ${owner}`); - fs.writeFileSync( - lockPath(lockRoot, device.id), - JSON.stringify({ - deviceId: device.id, - worktreeRoot, - claimedAt: new Date().toISOString(), - }), - { flag: 'wx' } - ); - return { device, alreadyOwned: false }; - }); - try { - args.prepare?.(device); - return claim; - } catch (error) { - if (!claim.alreadyOwned) { - releaseSimulator({ deviceId: device.id, lockRoot, worktreeRoot }); + const alreadyOwned = owner === worktreeRoot; + if (owner && !alreadyOwned) { + throw new Error(`Simulator ${device.id} is claimed by ${owner}`); } - throw error; - } + if (!alreadyOwned) { + fs.writeFileSync( + lockPath(lockRoot, device.id), + JSON.stringify({ + deviceId: device.id, + worktreeRoot, + claimedAt: new Date().toISOString(), + }), + { flag: 'wx' } + ); + } + // Hold the device mutation lock through preparation so a same-worktree + // concurrent caller cannot adopt the claim we just created while + // preparation is in flight. If the prepare callback throws, the + // rollback below removes only the claim this exact call created; a + // pre-existing same-worktree claim is preserved. We inline the + // cleanup because releaseSimulator re-acquires this same lock. + try { + args.prepare?.(device); + } catch (error) { + if (!alreadyOwned) { + fs.rmSync(lockPath(lockRoot, device.id), { force: true }); + } + throw error; + } + return { device, alreadyOwned }; + }); + return claim; } catch (error) { if (error instanceof Error && 'code' in error && error.code === 'EEXIST') { if (requestedId) { @@ -146,11 +247,7 @@ function main(): void { lockRoot, worktreeRoot, requestedId, - prepare: device => { - if (device.state === 'Booted') return; - execFileSync('xcrun', ['simctl', 'boot', device.id], { stdio: 'ignore' }); - execFileSync('xcrun', ['simctl', 'bootstatus', device.id, '-b'], { stdio: 'inherit' }); - }, + prepare: device => bootSimulator(device), }); console.log(JSON.stringify({ ...claim, worktreeRoot })); return; @@ -174,5 +271,5 @@ if (isMain) { } } -export { claimSimulator, releaseSimulator }; +export { bootSimulator, claimSimulator, releaseSimulator }; export type { SimulatorDevice }; diff --git a/dev/local/mobile-workflow.test.ts b/dev/local/mobile-workflow.test.ts index fb1ae998e3..98c9ca0bad 100644 --- a/dev/local/mobile-workflow.test.ts +++ b/dev/local/mobile-workflow.test.ts @@ -273,5 +273,32 @@ test('mobile workflow keeps secret-bearing environment files out of subagent con assert.match(workflow, /\.dev\.vars/); assert.match(workflow, /sanitized explicit values/i); assert.doesNotMatch(runbook, /grep .*\.env/); - assert.match(runbook, /KILO_E2E_AUTH_TOKEN/); +}); + +test('remote CLI runbook is secret-free and defers credential-bearing setup to the orchestrator', () => { + const runbook = fs.readFileSync('apps/mobile/e2e/AGENTS.md', 'utf8'); + const remoteCliSection = runbook.slice( + runbook.indexOf('## Remote CLI Session Flows'), + runbook.indexOf('## Android Emulator') + ); + + // The role-agent runbook must not contain bearer tokens, signing secrets, + // or credential-bearing environment variables. + assert.doesNotMatch(remoteCliSection, /KILO_E2E_AUTH_TOKEN/); + assert.doesNotMatch(remoteCliSection, /KILO_AUTH_CONTENT/); + assert.doesNotMatch(remoteCliSection, /NEXTAUTH_SECRET/); + assert.doesNotMatch(remoteCliSection, /\$\{KILO_[A-Z_]+/); + + // The role-agent runbook must not install the CLI or set up the CLI session. + assert.doesNotMatch(remoteCliSection, /npm install.*@kilocode\/cli/); + assert.doesNotMatch(remoteCliSection, /CLI_SCRATCH=/); + assert.doesNotMatch(remoteCliSection, /tmux set-environment/); + assert.doesNotMatch(remoteCliSection, /wrangler secrets/); + + // The role-agent runbook must clearly delegate credential-bearing setup + // to the orchestrator and describe how the role agent reuses the prepared + // session to verify mobile session discovery and mirroring. + assert.match(remoteCliSection, /orchestrator/i); + assert.match(remoteCliSection, /kilo-e2e-cli-/); + assert.match(remoteCliSection, /session discovery|mirroring/); }); From 76ca8bff5401b82135fe20677a387f85c583d227 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Wed, 15 Jul 2026 03:50:06 +0200 Subject: [PATCH 6/7] fix(dev): recover abandoned simulator claims --- dev/local/mobile-simulator.test.ts | 1377 +++++++++++++++++++++++++++- dev/local/mobile-simulator.ts | 596 ++++++++++-- 2 files changed, 1888 insertions(+), 85 deletions(-) diff --git a/dev/local/mobile-simulator.test.ts b/dev/local/mobile-simulator.test.ts index 07ae67a630..775698c92a 100644 --- a/dev/local/mobile-simulator.test.ts +++ b/dev/local/mobile-simulator.test.ts @@ -191,36 +191,6 @@ for (const failedCommand of ['boot', 'bootstatus'] as const) { }); } -test('preserves an existing iOS claim when simulator preparation fails', () => { - const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); - const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); - - try { - claimSimulator({ devices, lockRoot, worktreeRoot, requestedId: 'B' }); - - assert.throws( - () => - claimSimulator({ - devices, - lockRoot, - worktreeRoot, - requestedId: 'B', - prepare: () => { - throw new Error('bootstatus failed'); - }, - }), - /bootstatus failed/ - ); - assert.equal( - JSON.parse(fs.readFileSync(path.join(lockRoot, 'B.json'), 'utf8')).worktreeRoot, - worktreeRoot - ); - } finally { - fs.rmSync(lockRoot, { recursive: true, force: true }); - fs.rmSync(worktreeRoot, { recursive: true, force: true }); - } -}); - test('recovers an orphaned iOS claim mutation lock', () => { const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); @@ -253,9 +223,10 @@ test('blocks a same-worktree adoption during prepare so a failed first prepare c worktreeRoot, requestedId: 'B', prepare: () => { - // While the first call's prepare is running, a second same-worktree - // claim attempt must be blocked by the device mutation lock instead of - // adopting the claim that the first call owns. + // The mutation lock is released before prepare runs. A second + // same-worktree claim attempt must observe the preparing claim and + // reject — it must not adopt the in-flight claim and it must not + // delete the adopted claim if the first prepare then fails. try { secondClaimResult = claimSimulator({ devices, @@ -275,7 +246,11 @@ test('blocks a same-worktree adoption during prepare so a failed first prepare c assert.match(String(firstClaimError), /bootstatus failed/); assert.equal(secondClaimResult, null); - assert.match(String(secondClaimError), /claim is being updated concurrently/); + assert.match(String(secondClaimError), /preparation in progress/); + // The preparing claim must have been rolled back by exact claimId. + assert.equal(fs.existsSync(path.join(lockRoot, 'B.json')), false); + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); }); test('shuts down a simulator booted by this attempt when bootstatus fails', () => { @@ -434,3 +409,1337 @@ test('rolls back the iOS claim when bootstatus reports a terminal failure', () = fs.rmSync(worktreeRoot, { recursive: true, force: true }); } }); + +test('rejects a different-worktree claim while a peer is preparing (no heartbeat)', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const firstWorktree = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-one-')); + const secondWorktree = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-two-')); + let secondClaimError: unknown = null; + + try { + claimSimulator({ + devices, + lockRoot, + worktreeRoot: firstWorktree, + requestedId: 'B', + prepare: () => { + // While the first worktree is preparing, a different worktree must + // see the preparing claim and reject without adopting it. + try { + claimSimulator({ + devices, + lockRoot, + worktreeRoot: secondWorktree, + requestedId: 'B', + }); + } catch (error) { + secondClaimError = error; + } + // Return normally so the first claim finalizes. + }, + }); + + assert.match(String(secondClaimError), /preparation in progress|claimed by/); + const persisted = JSON.parse(fs.readFileSync(path.join(lockRoot, 'B.json'), 'utf8')); + assert.equal(persisted.worktreeRoot, firstWorktree); + assert.equal(persisted.status, 'ready'); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(firstWorktree, { recursive: true, force: true }); + fs.rmSync(secondWorktree, { recursive: true, force: true }); + } +}); + +test('finalizes a successful prepare with a ready status and the same claimId', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + const snapshot: { value: { status?: string; claimId?: string } | null } = { value: null }; + + try { + const claim = claimSimulator({ + devices, + lockRoot, + worktreeRoot, + requestedId: 'B', + prepare: () => { + // While the mutation lock is released, the on-disk claim must be in + // the preparing state with a non-empty claimId. + snapshot.value = JSON.parse(fs.readFileSync(path.join(lockRoot, 'B.json'), 'utf8')) as { + status?: string; + claimId?: string; + }; + }, + }); + + const prepareClaimSnapshot = snapshot.value; + assert.equal(claim.alreadyOwned, false); + assert.equal(prepareClaimSnapshot?.status, 'preparing'); + assert.match(prepareClaimSnapshot?.claimId ?? '', /\S+/); + const finalClaim = JSON.parse(fs.readFileSync(path.join(lockRoot, 'B.json'), 'utf8')) as { + status: string; + claimId: string; + }; + assert.equal(finalClaim.status, 'ready'); + assert.equal(finalClaim.claimId, prepareClaimSnapshot?.claimId); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +test('rolls back only the exact preparing claimId when prepare fails', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + + try { + assert.throws( + () => + claimSimulator({ + devices, + lockRoot, + worktreeRoot, + requestedId: 'B', + prepare: () => { + throw new Error('bootstatus failed'); + }, + }), + /bootstatus failed/ + ); + assert.equal(fs.existsSync(path.join(lockRoot, 'B.json')), false); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +test('preserves a preparing claim when shutdown fails so a peer cannot adopt a running device', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const firstWorktree = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-one-')); + const secondWorktree = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-two-')); + const errorCapture: { value: unknown } = { value: null }; + + try { + try { + claimSimulator({ + devices, + lockRoot, + worktreeRoot: firstWorktree, + requestedId: 'B', + prepare: () => { + // Simulate bootSimulator: boot succeeds, bootstatus throws, then + // shutdown also throws. The claim must be preserved with the + // preparing status so a peer cannot adopt a possibly running + // device. The original prepare error must surface with a typed + // shutdownFailed signal. + const exec = recordingExec(call => { + if (call.args[1] === 'shutdown') return new Error('shutdown failed'); + return undefined; + }); + const runWithOutput = recordingOutput(() => { + throw new Error('bootstatus failed'); + }); + try { + bootSimulator({ id: 'B', name: 'Kilo E2E-B', state: 'Shutdown' }, exec, runWithOutput); + } catch (error) { + errorCapture.value = error; + throw error; + } + }, + }); + } catch (error) { + assert.match(String(error), /bootstatus failed/); + } + + const prepareError = errorCapture.value as Error & { shutdownFailed?: boolean }; + assert.equal(prepareError?.shutdownFailed, true); + const persisted = JSON.parse(fs.readFileSync(path.join(lockRoot, 'B.json'), 'utf8')) as { + status: string; + worktreeRoot: string; + }; + assert.equal(persisted.status, 'preparing'); + assert.equal(persisted.worktreeRoot, firstWorktree); + + // A peer must not be able to adopt the preserved preparing claim. + assert.throws( + () => + claimSimulator({ + devices, + lockRoot, + worktreeRoot: secondWorktree, + requestedId: 'B', + }), + /claimed by|preparation in progress/ + ); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(firstWorktree, { recursive: true, force: true }); + fs.rmSync(secondWorktree, { recursive: true, force: true }); + } +}); + +test('removes a preparing claim when shutdown succeeds after prepare failure', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + const errorCapture: { value: unknown } = { value: null }; + + try { + assert.throws( + () => + claimSimulator({ + devices, + lockRoot, + worktreeRoot, + requestedId: 'B', + prepare: () => { + const exec = recordingExec(() => undefined); + const runWithOutput = recordingOutput(() => { + throw new Error('bootstatus failed'); + }); + try { + bootSimulator( + { id: 'B', name: 'Kilo E2E-B', state: 'Shutdown' }, + exec, + runWithOutput + ); + } catch (error) { + errorCapture.value = error; + throw error; + } + }, + }), + /bootstatus failed/ + ); + const prepareError = errorCapture.value as Error & { shutdownFailed?: boolean }; + assert.equal(prepareError?.shutdownFailed, false); + assert.equal(fs.existsSync(path.join(lockRoot, 'B.json')), false); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +test('returns alreadyOwned for a same-worktree ready claim without re-preparing', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + let secondPrepareCalled = false; + + try { + const first = claimSimulator({ + devices, + lockRoot, + worktreeRoot, + requestedId: 'B', + }); + assert.equal(first.alreadyOwned, false); + + const second = claimSimulator({ + devices, + lockRoot, + worktreeRoot, + requestedId: 'B', + prepare: () => { + secondPrepareCalled = true; + }, + }); + assert.equal(second.alreadyOwned, true); + assert.equal(secondPrepareCalled, false); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +test('keeps the existing iOS claim when a same-worktree prepare throws after the claim is ready', () => { + // With the new state protocol, a same-worktree ready claim is returned + // as alreadyOwned without re-preparing, so the on-disk claim is naturally + // preserved even if the caller passes a failing prepare callback. + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + + try { + claimSimulator({ devices, lockRoot, worktreeRoot, requestedId: 'B' }); + const result = claimSimulator({ + devices, + lockRoot, + worktreeRoot, + requestedId: 'B', + prepare: () => { + throw new Error('bootstatus failed'); + }, + }); + assert.equal(result.alreadyOwned, true); + const persisted = JSON.parse(fs.readFileSync(path.join(lockRoot, 'B.json'), 'utf8')) as { + worktreeRoot: string; + status: string; + }; + assert.equal(persisted.worktreeRoot, worktreeRoot); + assert.equal(persisted.status, 'ready'); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +// --- Finding 1: releaseSimulator must never remove a preparing claim --- + +test('releaseSimulator rejects a preparing claim from the same worktree', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + + try { + claimSimulator({ + devices, + lockRoot, + worktreeRoot, + requestedId: 'B', + prepare: () => { + // While the claim is still preparing, the same worktree must not + // be able to release it — the device may be mid-boot. + assert.throws( + () => releaseSimulator({ deviceId: 'B', lockRoot, worktreeRoot }), + /preparation.*in progress|cannot release.*preparing/ + ); + // The claim must still be on disk. + assert.equal(fs.existsSync(path.join(lockRoot, 'B.json')), true); + }, + }); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +test('releaseSimulator rejects a preparing claim from a different worktree', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const firstWorktree = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-one-')); + const secondWorktree = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-two-')); + + try { + claimSimulator({ + devices, + lockRoot, + worktreeRoot: firstWorktree, + requestedId: 'B', + prepare: () => { + assert.throws( + () => releaseSimulator({ deviceId: 'B', lockRoot, worktreeRoot: secondWorktree }), + /preparation.*in progress|cannot release.*preparing|claimed by/ + ); + assert.equal(fs.existsSync(path.join(lockRoot, 'B.json')), true); + }, + }); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(firstWorktree, { recursive: true, force: true }); + fs.rmSync(secondWorktree, { recursive: true, force: true }); + } +}); + +test('releaseSimulator allows a normal ready release after prepare completes', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + + try { + claimSimulator({ devices, lockRoot, worktreeRoot, requestedId: 'B' }); + assert.equal(fs.existsSync(path.join(lockRoot, 'B.json')), true); + releaseSimulator({ deviceId: 'B', lockRoot, worktreeRoot }); + assert.equal(fs.existsSync(path.join(lockRoot, 'B.json')), false); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +// --- Finding 2: exact-claimId finalization/rollback must fail-closed on +// missing/corrupt/replaced records --- + +test('fails closed when the on-disk record is replaced mid-prepare during successful finalization', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + const replacementClaimId = 'peer-replacement-claim-id'; + + try { + assert.throws( + () => + claimSimulator({ + devices, + lockRoot, + worktreeRoot, + requestedId: 'B', + prepare: () => { + // Simulate a peer (or external write) replacing the record + // while our prepare is running and the mutation lock is + // released. The replacement uses a different claimId and + // must be preserved verbatim. + fs.writeFileSync( + path.join(lockRoot, 'B.json'), + JSON.stringify({ + deviceId: 'B', + worktreeRoot: 'other-worktree', + claimId: replacementClaimId, + status: 'preparing', + claimedAt: new Date().toISOString(), + }) + ); + // Prepare completes normally — the replacement happened + // between the initial write and our finalization. + }, + }), + /claim.*replaced|finalization failed|claimId mismatch/ + ); + + // The replacement must be preserved exactly. + const persisted = JSON.parse(fs.readFileSync(path.join(lockRoot, 'B.json'), 'utf8')) as { + claimId: string; + worktreeRoot: string; + status: string; + }; + assert.equal(persisted.claimId, replacementClaimId); + assert.equal(persisted.worktreeRoot, 'other-worktree'); + assert.equal(persisted.status, 'preparing'); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +test('fails closed when the on-disk record vanishes during successful finalization', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + + try { + assert.throws( + () => + claimSimulator({ + devices, + lockRoot, + worktreeRoot, + requestedId: 'B', + prepare: () => { + // The record disappears between the initial write and + // finalization (e.g., another process cleaned a stale entry). + fs.rmSync(path.join(lockRoot, 'B.json'), { force: true }); + }, + }), + /claim.*missing|finalization failed|record.*vanished|not found/ + ); + // The record is still gone — we did not recreate it with a false ready. + assert.equal(fs.existsSync(path.join(lockRoot, 'B.json')), false); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +test('fails closed when the on-disk record is corrupt during successful finalization', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + + try { + assert.throws( + () => + claimSimulator({ + devices, + lockRoot, + worktreeRoot, + requestedId: 'B', + prepare: () => { + // Corrupt the record mid-prepare. Finalization must not + // silently succeed; it must throw and must not delete the + // corrupt data. + fs.writeFileSync(path.join(lockRoot, 'B.json'), '{ not valid json'); + }, + }), + /claim.*corrupt|finalization failed|invalid/i + ); + // The corrupt data must be preserved (we do not delete unknown data). + const raw = fs.readFileSync(path.join(lockRoot, 'B.json'), 'utf8'); + assert.equal(raw, '{ not valid json'); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +test('does not delete a replacement record during rollback when prepare fails after replacement', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + const replacementClaimId = 'peer-replacement-claim-id'; + + try { + assert.throws( + () => + claimSimulator({ + devices, + lockRoot, + worktreeRoot, + requestedId: 'B', + prepare: () => { + fs.writeFileSync( + path.join(lockRoot, 'B.json'), + JSON.stringify({ + deviceId: 'B', + worktreeRoot: 'other-worktree', + claimId: replacementClaimId, + status: 'preparing', + claimedAt: new Date().toISOString(), + }) + ); + // Now the prepare itself fails. + throw new Error('bootstatus failed'); + }, + }), + /bootstatus failed/ + ); + // The replacement must be preserved exactly — rollback must not + // delete a record it does not own. + const persisted = JSON.parse(fs.readFileSync(path.join(lockRoot, 'B.json'), 'utf8')) as { + claimId: string; + worktreeRoot: string; + }; + assert.equal(persisted.claimId, replacementClaimId); + assert.equal(persisted.worktreeRoot, 'other-worktree'); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +// --- Finding 3: legacy-format claim compatibility --- + +test('treats a legacy claim (no status/claimId) as ready for the same worktree', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + + try { + // Write a legacy claim — only worktreeRoot, no status, no claimId. + fs.writeFileSync( + path.join(lockRoot, 'B.json'), + JSON.stringify({ worktreeRoot, claimedAt: new Date().toISOString() }) + ); + + const claim = claimSimulator({ devices, lockRoot, worktreeRoot, requestedId: 'B' }); + assert.equal(claim.alreadyOwned, true); + assert.equal(claim.device.id, 'B'); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +test('rejects a different worktree against a legacy claim (no status/claimId)', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const firstWorktree = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-one-')); + const secondWorktree = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-two-')); + + try { + fs.writeFileSync( + path.join(lockRoot, 'B.json'), + JSON.stringify({ worktreeRoot: firstWorktree, claimedAt: new Date().toISOString() }) + ); + + assert.throws( + () => claimSimulator({ devices, lockRoot, worktreeRoot: secondWorktree, requestedId: 'B' }), + new RegExp(`claimed by ${firstWorktree}`) + ); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(firstWorktree, { recursive: true, force: true }); + fs.rmSync(secondWorktree, { recursive: true, force: true }); + } +}); + +test('releaseSimulator can release a legacy claim from the owning worktree', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + + try { + fs.writeFileSync( + path.join(lockRoot, 'B.json'), + JSON.stringify({ worktreeRoot, claimedAt: new Date().toISOString() }) + ); + + releaseSimulator({ deviceId: 'B', lockRoot, worktreeRoot }); + assert.equal(fs.existsSync(path.join(lockRoot, 'B.json')), false); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +// --- Finding 5: releaseSimulator does not delete corrupt data --- + +test('releaseSimulator does not delete a corrupt claim and surfaces a clear error', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + + try { + fs.writeFileSync(path.join(lockRoot, 'B.json'), '{ not valid json'); + + assert.throws( + () => releaseSimulator({ deviceId: 'B', lockRoot, worktreeRoot }), + /corrupt|invalid|not.*valid/i + ); + // The corrupt data must be preserved — we do not silently delete + // unknown data. + const raw = fs.readFileSync(path.join(lockRoot, 'B.json'), 'utf8'); + assert.equal(raw, '{ not valid json'); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +// --- Finding: abandoned preparation recovery via preparer PID + identity --- +// A process SIGKILL/crash after writing a preparing claim must not leave +// the device permanently stuck. PID liveness AND matching process start +// identity (not wall-clock age) are the source of truth. PID reuse is +// detected by comparing the stored process identity against the current +// process identity queried at decision time. + +function writePreparingClaim( + lockRoot: string, + deviceId: string, + worktreeRoot: string, + options: { preparerPid?: number; preparerIdentity?: string; claimId?: string } = {} +): { claimId: string; preparerPid?: number; preparerIdentity?: string } { + const claimId = options.claimId ?? 'existing-claim-id'; + const record: Record = { + deviceId, + worktreeRoot, + claimId, + status: 'preparing', + claimedAt: new Date().toISOString(), + }; + if (options.preparerPid !== undefined) { + record.preparerPid = options.preparerPid; + } + if (options.preparerIdentity !== undefined) { + record.preparerIdentity = options.preparerIdentity; + } + fs.writeFileSync(path.join(lockRoot, `${deviceId}.json`), JSON.stringify(record)); + return { claimId, preparerPid: options.preparerPid, preparerIdentity: options.preparerIdentity }; +} + +// Test helper: a mutable map of deviceId -> state, used by tests to +// simulate `xcrun simctl list devices available --json` re-reads during +// the production recovery reset flow. +const testDeviceStates = new Map(); +function listIosDeviceStateForTest(deviceId: string): string | undefined { + return testDeviceStates.get(deviceId); +} + +// --- Active/abandoned classification: PID liveness + matching identity --- +// PID reuse (a dead PID reassigned to a different process) is detected by +// comparing the stored process start identity against the current process +// identity queried at decision time. If the identity cannot be queried +// or does not match, the claim is treated as abandoned. + +test('protects an active claim with alive PID and matching identity (same worktree)', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + const identity = 'current-process-identity'; + + try { + writePreparingClaim(lockRoot, 'B', worktreeRoot, { + preparerPid: process.pid, + preparerIdentity: identity, + }); + + assert.throws( + () => + claimSimulator({ + devices, + lockRoot, + worktreeRoot, + requestedId: 'B', + pidAlive: () => true, + processIdentity: () => identity, + }), + /preparation in progress/ + ); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +test('protects an active claim with alive PID and matching identity (different worktree)', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const firstWorktree = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-one-')); + const secondWorktree = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-two-')); + const identity = 'current-process-identity'; + + try { + writePreparingClaim(lockRoot, 'B', firstWorktree, { + preparerPid: process.pid, + preparerIdentity: identity, + }); + + assert.throws( + () => + claimSimulator({ + devices, + lockRoot, + worktreeRoot: secondWorktree, + requestedId: 'B', + pidAlive: () => true, + processIdentity: () => identity, + }), + /preparation in progress|claimed by/ + ); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(firstWorktree, { recursive: true, force: true }); + fs.rmSync(secondWorktree, { recursive: true, force: true }); + } +}); + +test('checks the recorded preparer identity instead of the current claimant identity', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const firstWorktree = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-one-')); + const secondWorktree = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-two-')); + const preparerPid = 4242; + const queriedPids: number[] = []; + + try { + writePreparingClaim(lockRoot, 'B', firstWorktree, { + preparerPid, + preparerIdentity: 'original-preparer', + }); + + assert.throws( + () => + claimSimulator({ + devices, + lockRoot, + worktreeRoot: secondWorktree, + requestedId: 'B', + pidAlive: () => true, + processIdentity: pid => { + queriedPids.push(pid); + return pid === preparerPid ? 'original-preparer' : 'new-claimant'; + }, + }), + new RegExp(`claimed by ${firstWorktree}`) + ); + assert.deepEqual(queriedPids, [preparerPid]); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(firstWorktree, { recursive: true, force: true }); + fs.rmSync(secondWorktree, { recursive: true, force: true }); + } +}); + +test('recovers when an alive PID has a different process identity (PID reuse)', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + let resetCalled = false; + + try { + writePreparingClaim(lockRoot, 'B', worktreeRoot, { + preparerPid: 424242, + preparerIdentity: 'stale-identity-from-old-process', + }); + + const claim = claimSimulator({ + devices, + lockRoot, + worktreeRoot, + requestedId: 'B', + pidAlive: () => true, + processIdentity: () => 'current-process-identity', + recoveryReset: deviceId => { + resetCalled = true; + return { id: deviceId, name: 'Kilo E2E-B', state: 'Shutdown' }; + }, + }); + + // PID is alive but the identity differs — the PID was reused by a + // different process. The claim is abandoned and must be recovered. + assert.equal(claim.alreadyOwned, false); + assert.equal(resetCalled, true); + const persisted = JSON.parse(fs.readFileSync(path.join(lockRoot, 'B.json'), 'utf8')) as { + preparerPid: number; + preparerIdentity: string; + }; + assert.equal(persisted.preparerPid, process.pid); + assert.equal(persisted.preparerIdentity, 'current-process-identity'); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +test('recovers when the process identity cannot be queried (fail closed)', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + let resetCalled = false; + + try { + writePreparingClaim(lockRoot, 'B', worktreeRoot, { + preparerPid: process.pid, + preparerIdentity: 'stored-identity', + }); + + const claim = claimSimulator({ + devices, + lockRoot, + worktreeRoot, + requestedId: 'B', + pidAlive: () => true, + processIdentity: () => undefined, + recoveryReset: deviceId => { + resetCalled = true; + return { id: deviceId, name: 'Kilo E2E-B', state: 'Shutdown' }; + }, + }); + + assert.equal(claim.alreadyOwned, false); + assert.equal(resetCalled, true); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +test('recovers when the PID is dead even if the stored identity matches', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + let resetCalled = false; + + try { + writePreparingClaim(lockRoot, 'B', worktreeRoot, { + preparerPid: 999999, + preparerIdentity: 'any-identity', + }); + + const claim = claimSimulator({ + devices, + lockRoot, + worktreeRoot, + requestedId: 'B', + pidAlive: () => false, + processIdentity: () => 'any-identity', + recoveryReset: deviceId => { + resetCalled = true; + return { id: deviceId, name: 'Kilo E2E-B', state: 'Shutdown' }; + }, + }); + + assert.equal(claim.alreadyOwned, false); + assert.equal(resetCalled, true); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +test('recovers when the process identity is missing from the stored claim (legacy)', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + let resetCalled = false; + + try { + // Legacy format: status=preparing but no preparerPid/preparerIdentity. + writePreparingClaim(lockRoot, 'B', worktreeRoot); + + const claim = claimSimulator({ + devices, + lockRoot, + worktreeRoot, + requestedId: 'B', + recoveryReset: deviceId => { + resetCalled = true; + return { id: deviceId, name: 'Kilo E2E-B', state: 'Shutdown' }; + }, + }); + + assert.equal(claim.alreadyOwned, false); + assert.equal(resetCalled, true); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +// --- Recovery reset contract: confirmed Shutdown device before normal prepare --- + +test('recovery reset: stale Booted snapshot but actual Shutdown skips shutdown and boots', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + let resetDeviceSeen: SimulatorDevice | undefined; + let prepareDeviceSeen: SimulatorDevice | undefined; + let shutdownCommandIssued = false; + + try { + writePreparingClaim(lockRoot, 'B', worktreeRoot, { + preparerPid: 999999, + preparerIdentity: 'stale', + }); + + // The recovery reset callback receives the deviceId and returns a + // confirmed Shutdown device. In the production code, the callback + // re-reads the actual state via `xcrun simctl list devices`, issues + // a shutdown if needed, and confirms Shutdown. Here we simulate + // the production behavior: the actual state is Shutdown, so no + // shutdown command is issued, and the callback returns Shutdown. + testDeviceStates.set('B', 'Shutdown'); + + const result = claimSimulator({ + // Stale list-time snapshot says Booted. + devices: [{ id: 'B', name: 'Kilo E2E-B', state: 'Booted' }], + lockRoot, + worktreeRoot, + requestedId: 'B', + pidAlive: () => false, + processIdentity: () => 'current', + recoveryReset: deviceId => { + // Simulate the production reset: re-read actual state. + const actualState = listIosDeviceStateForTest(deviceId); + if (actualState !== 'Shutdown') { + shutdownCommandIssued = true; + } + resetDeviceSeen = { id: deviceId, name: 'Kilo E2E-B', state: 'Shutdown' }; + return resetDeviceSeen; + }, + prepare: device => { + prepareDeviceSeen = device; + }, + }); + + assert.equal(result.alreadyOwned, false); + // No shutdown command was issued because actual state was Shutdown. + assert.equal(shutdownCommandIssued, false); + // Normal prepare received the confirmed Shutdown device and booted it. + assert.equal(prepareDeviceSeen?.state, 'Shutdown'); + assert.equal(prepareDeviceSeen?.id, 'B'); + } finally { + testDeviceStates.delete('B'); + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +test('recovery reset: stale Shutdown snapshot but actual Booted issues shutdown and confirms', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + const shutdownCommands: string[][] = []; + let prepareDeviceSeen: SimulatorDevice | undefined; + let actualStateAfterShutdown: string | undefined; + + try { + writePreparingClaim(lockRoot, 'B', worktreeRoot, { + preparerPid: 999999, + preparerIdentity: 'stale', + }); + // Simulate the actual device state being Booted (stale list-time + // snapshot says Shutdown). The recovery reset callback re-reads + // the actual state via this map. + testDeviceStates.set('B', 'Booted'); + + claimSimulator({ + // Stale list-time snapshot says Shutdown. + devices: [{ id: 'B', name: 'Kilo E2E-B', state: 'Shutdown' }], + lockRoot, + worktreeRoot, + requestedId: 'B', + pidAlive: () => false, + processIdentity: () => 'current', + recoveryReset: deviceId => { + // Simulate the production reset: re-read actual state, issue + // shutdown if needed, re-read and confirm. + const before = listIosDeviceStateForTest(deviceId); + if (before !== 'Shutdown') { + shutdownCommands.push(['xcrun', 'simctl', 'shutdown', deviceId]); + // Simulate the shutdown taking effect: update the test map + // so the next re-read returns Shutdown. + testDeviceStates.set(deviceId, 'Shutdown'); + } + const after = listIosDeviceStateForTest(deviceId); + if (after !== 'Shutdown') { + throw new Error(`Simulator ${deviceId} shutdown not confirmed`); + } + actualStateAfterShutdown = 'Shutdown'; + return { id: deviceId, name: 'Kilo E2E-B', state: 'Shutdown' }; + }, + prepare: device => { + prepareDeviceSeen = device; + }, + }); + + assert.deepEqual(shutdownCommands, [['xcrun', 'simctl', 'shutdown', 'B']]); + assert.equal(actualStateAfterShutdown, 'Shutdown'); + assert.equal(prepareDeviceSeen?.state, 'Shutdown'); + } finally { + testDeviceStates.delete('B'); + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +test('recovery reset: shutdown exit success but actual state remains Booted throws and preserves', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + let caught: Error | undefined; + + try { + writePreparingClaim(lockRoot, 'B', worktreeRoot, { + preparerPid: 999999, + preparerIdentity: 'stale', + }); + + try { + claimSimulator({ + devices: [{ id: 'B', name: 'Kilo E2E-B', state: 'Booted' }], + lockRoot, + worktreeRoot, + requestedId: 'B', + pidAlive: () => false, + processIdentity: () => 'current', + recoveryReset: deviceId => { + // The shutdown command "succeeds" (no throw) but the actual + // state remains Booted — the device is stuck. The reset + // callback must throw and the claim must be preserved. + const confirmed = listIosDeviceStateForTest(deviceId); + if (confirmed !== 'Shutdown') { + throw new Error(`Simulator ${deviceId} shutdown not confirmed`); + } + return { id: deviceId, name: 'Kilo E2E-B', state: 'Shutdown' }; + }, + }); + } catch (error) { + caught = error as Error; + } + + assert.ok(caught, 'claimSimulator must throw when shutdown is not confirmed'); + assert.match(String(caught), /shutdown not confirmed/); + + // The new preparing claim must be preserved. + const persisted = JSON.parse(fs.readFileSync(path.join(lockRoot, 'B.json'), 'utf8')) as { + status: string; + preparerPid: number; + }; + assert.equal(persisted.status, 'preparing'); + assert.equal(persisted.preparerPid, process.pid); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +test('recovery reset: absent callback throws and preserves the preparing claim', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + let caught: Error | undefined; + + try { + writePreparingClaim(lockRoot, 'B', worktreeRoot, { + preparerPid: 999999, + preparerIdentity: 'stale', + }); + + try { + claimSimulator({ + devices: [{ id: 'B', name: 'Kilo E2E-B', state: 'Booted' }], + lockRoot, + worktreeRoot, + requestedId: 'B', + pidAlive: () => false, + processIdentity: () => 'current', + // recoveryReset intentionally omitted. + }); + } catch (error) { + caught = error as Error; + } + + assert.ok(caught, 'claimSimulator must throw when recoveryReset is absent'); + assert.match(String(caught), /recovery reset/i); + + // The new preparing claim must be preserved. + const persisted = JSON.parse(fs.readFileSync(path.join(lockRoot, 'B.json'), 'utf8')) as { + status: string; + preparerPid: number; + }; + assert.equal(persisted.status, 'preparing'); + assert.equal(persisted.preparerPid, process.pid); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +test('recovery marks the claim ready only after the normal prepare succeeds', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + + try { + writePreparingClaim(lockRoot, 'B', worktreeRoot, { + preparerPid: 999999, + preparerIdentity: 'stale', + }); + + const claim = claimSimulator({ + devices: [{ id: 'B', name: 'Kilo E2E-B', state: 'Booted' }], + lockRoot, + worktreeRoot, + requestedId: 'B', + pidAlive: () => false, + processIdentity: () => 'current', + recoveryReset: deviceId => ({ id: deviceId, name: 'Kilo E2E-B', state: 'Shutdown' }), + prepare: () => { + // Normal prepare succeeds (no throw). The claim must be + // finalized as 'ready' only after this returns. + }, + }); + + assert.equal(claim.alreadyOwned, false); + const persisted = JSON.parse(fs.readFileSync(path.join(lockRoot, 'B.json'), 'utf8')) as { + status: string; + }; + assert.equal(persisted.status, 'ready'); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +test('recovery does not mark the claim ready when the normal prepare throws', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + let caught: Error | undefined; + + try { + writePreparingClaim(lockRoot, 'B', worktreeRoot, { + preparerPid: 999999, + preparerIdentity: 'stale', + }); + + try { + claimSimulator({ + devices: [{ id: 'B', name: 'Kilo E2E-B', state: 'Booted' }], + lockRoot, + worktreeRoot, + requestedId: 'B', + pidAlive: () => false, + processIdentity: () => 'current', + recoveryReset: deviceId => ({ id: deviceId, name: 'Kilo E2E-B', state: 'Shutdown' }), + prepare: () => { + throw new Error('bootstatus failed'); + }, + }); + } catch (error) { + caught = error as Error; + } + + assert.ok(caught); + assert.match(String(caught), /bootstatus failed/); + // The preparing claim must be removed (exact-own rollback) because + // the prepare failure did not leave the device in a bad state + // (bootSimulator's own shutdown succeeded — shutdownFailed=false). + const persistedPath = path.join(lockRoot, 'B.json'); + assert.equal(fs.existsSync(persistedPath), false); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +// --- Field validation: required ClaimRecord fields without String coercion --- + +test('treats a claim with missing deviceId as corrupt', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + + try { + fs.writeFileSync( + path.join(lockRoot, 'B.json'), + JSON.stringify({ + worktreeRoot, + claimId: 'x', + preparerPid: 1, + preparerIdentity: 'i', + status: 'preparing', + claimedAt: new Date().toISOString(), + }) + ); + + // Missing deviceId is corrupt — the initial phase must clean it up + // and create a fresh claim. + const claim = claimSimulator({ devices, lockRoot, worktreeRoot, requestedId: 'B' }); + assert.equal(claim.alreadyOwned, false); + const persisted = JSON.parse(fs.readFileSync(path.join(lockRoot, 'B.json'), 'utf8')) as { + deviceId: string; + }; + assert.equal(persisted.deviceId, 'B'); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +test('treats a claim with non-string worktreeRoot as corrupt', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + + try { + fs.writeFileSync( + path.join(lockRoot, 'B.json'), + JSON.stringify({ + deviceId: 'B', + worktreeRoot: 12345, + claimId: 'x', + preparerPid: 1, + preparerIdentity: 'i', + status: 'preparing', + claimedAt: new Date().toISOString(), + }) + ); + + const claim = claimSimulator({ devices, lockRoot, worktreeRoot, requestedId: 'B' }); + assert.equal(claim.alreadyOwned, false); + const persisted = JSON.parse(fs.readFileSync(path.join(lockRoot, 'B.json'), 'utf8')) as { + worktreeRoot: string; + }; + assert.equal(persisted.worktreeRoot, worktreeRoot); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +test('treats a claim with non-integer preparerPid as corrupt', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + + try { + fs.writeFileSync( + path.join(lockRoot, 'B.json'), + JSON.stringify({ + deviceId: 'B', + worktreeRoot, + claimId: 'x', + preparerPid: 'not-a-number', + preparerIdentity: 'i', + status: 'preparing', + claimedAt: new Date().toISOString(), + }) + ); + + const claim = claimSimulator({ devices, lockRoot, worktreeRoot, requestedId: 'B' }); + assert.equal(claim.alreadyOwned, false); + const persisted = JSON.parse(fs.readFileSync(path.join(lockRoot, 'B.json'), 'utf8')) as { + preparerPid: number; + }; + assert.equal(persisted.preparerPid, process.pid); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +test('treats a claim with an unparseable claimedAt as corrupt', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + + try { + fs.writeFileSync( + path.join(lockRoot, 'B.json'), + JSON.stringify({ + deviceId: 'B', + worktreeRoot, + claimId: 'x', + preparerPid: 1, + preparerIdentity: 'i', + status: 'preparing', + claimedAt: 'not-a-date', + }) + ); + + const claim = claimSimulator({ devices, lockRoot, worktreeRoot, requestedId: 'B' }); + assert.equal(claim.alreadyOwned, false); + const persisted = JSON.parse(fs.readFileSync(path.join(lockRoot, 'B.json'), 'utf8')) as { + claimedAt: string; + }; + assert.notEqual(persisted.claimedAt, 'not-a-date'); + assert.ok(!isNaN(Date.parse(persisted.claimedAt))); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +test('new preparing claims include both the current PID and process identity', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + const identity = 'injected-identity'; + + try { + claimSimulator({ + devices, + lockRoot, + worktreeRoot, + requestedId: 'B', + processIdentity: () => identity, + }); + const persisted = JSON.parse(fs.readFileSync(path.join(lockRoot, 'B.json'), 'utf8')) as { + preparerPid: number; + preparerIdentity: string; + }; + assert.equal(persisted.preparerPid, process.pid); + assert.equal(persisted.preparerIdentity, identity); + } finally { + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); + +test('attaches a rollback cleanup failure to the original prepare error', () => { + const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); + const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); + let caught: Error | undefined; + + try { + try { + claimSimulator({ + devices, + lockRoot, + worktreeRoot, + requestedId: 'B', + prepare: () => { + // Replace the claim file with a directory so the exact-own + // rollback rmSync fails deterministically. The error from + // fs.rmSync on a directory is platform-specific (EBUSY, + // EPERM, ENOTDIR); the test only asserts that the original + // prepare failure is the primary message and that the + // cleanup failure is attached for the operator. + const claimPath = path.join(lockRoot, 'B.json'); + fs.rmSync(claimPath, { force: true }); + fs.mkdirSync(claimPath); + throw new Error('bootstatus failed'); + }, + }); + } catch (error) { + caught = error as Error; + } + + assert.ok(caught, 'claimSimulator must throw when prepare fails'); + // The original prepare failure must be the primary message. + assert.match(String(caught), /bootstatus failed/); + // The cleanup failure must be attached so the operator can see it. + const withCleanup = caught as Error & { cleanupError?: unknown }; + assert.ok( + withCleanup.cleanupError !== undefined || caught.cause !== undefined, + 'cleanup failure must be attached to the prepare error' + ); + } finally { + // Clean up the swapped directory if it still exists. + try { + fs.rmSync(path.join(lockRoot, 'B.json'), { recursive: true, force: true }); + } catch { + // ignore + } + fs.rmSync(lockRoot, { recursive: true, force: true }); + fs.rmSync(worktreeRoot, { recursive: true, force: true }); + } +}); diff --git a/dev/local/mobile-simulator.ts b/dev/local/mobile-simulator.ts index 0aff9e6815..9fc9cda7d5 100644 --- a/dev/local/mobile-simulator.ts +++ b/dev/local/mobile-simulator.ts @@ -1,8 +1,5 @@ -import { - execFileSync, - spawnSync, - type ExecFileSyncOptionsWithStringEncoding, -} from 'node:child_process'; +import { execFileSync, spawnSync, type ExecFileSyncOptions } from 'node:child_process'; +import { randomUUID } from 'node:crypto'; import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; @@ -13,7 +10,7 @@ type SimulatorDevice = { id: string; name: string; state: string }; type ExecFn = ( command: string, args: readonly string[], - options: ExecFileSyncOptionsWithStringEncoding + options: ExecFileSyncOptions ) => string | Buffer; type CommandResult = { stdout: string; @@ -21,40 +18,299 @@ type CommandResult = { status: number | null; }; type ExecWithOutputFn = (command: string, args: readonly string[]) => CommandResult; +type ClaimStatus = 'preparing' | 'ready'; +// All fields are optional in the exported type. Current-format claims +// require deviceId, worktreeRoot, claimId, status, and claimedAt; +// legacy claims (no `status`) require only worktreeRoot. The +// `isValidCurrentClaim` and `isValidLegacyClaim` helpers enforce the +// required fields without String coercion. +type ClaimRecord = { + deviceId?: string; + worktreeRoot?: string; + claimId?: string; + // Preparer process PID and start identity. Both are required for + // current-format claims; legacy claims (no `status`) may omit them. + // Identity is the normalized output of `ps -o lstart= -p ` and + // is used to detect PID reuse: a dead PID reassigned to a different + // process will have a different identity. + preparerPid?: number; + preparerIdentity?: string; + status?: ClaimStatus; + claimedAt?: string; +}; type ClaimArgs = { devices: SimulatorDevice[]; lockRoot: string; worktreeRoot: string; requestedId?: string; prepare?: (device: SimulatorDevice) => void; + // Recovery reset contract: REQUIRED for abandoned-preparation + // recovery. The callback receives a deviceId and must return a + // confirmed Shutdown device. It is responsible for re-reading the + // actual simulator state, issuing a shutdown if needed, and + // confirming Shutdown before returning. If the callback is absent + // during recovery, claimSimulator throws and preserves the new + // preparing claim. No lock is held across the callback's commands. + recoveryReset?: (deviceId: string) => SimulatorDevice; + // Process identity probe: `ps -o lstart= -p ` (darwin/linux). + // Returns the normalized start time string, or undefined if it + // cannot be queried. Injectable for deterministic tests. + processIdentity?: (pid: number) => string | undefined; + // PID liveness probe. `process.kill(pid, 0)` returns true for alive + // PIDs and throws EPERM (alive but no permission) or ESRCH (dead). + // Injectable for deterministic tests. + pidAlive?: (pid: number) => boolean; fileOperations?: { readFileSync?: (filePath: string, encoding: 'utf8') => string; }; }; +// Typed error thrown by `bootSimulator` so the caller can distinguish a +// prepare failure (safe to roll back the claim) from a prepare failure whose +// follow-up shutdown also failed (unsafe to roll back — the device may still +// be running and must remain reserved). The original cause is preserved on +// `error.cause`. +export class PrepareError extends Error { + readonly shutdownFailed: boolean; + constructor(message: string, options: { shutdownFailed: boolean; cause?: unknown }) { + super(message); + this.name = 'PrepareError'; + this.shutdownFailed = options.shutdownFailed; + if (options.cause !== undefined) this.cause = options.cause; + } +} + function lockPath(lockRoot: string, deviceId: string): string { return path.join(lockRoot, `${deviceId}.json`); } -function readOwner( +// Probe a PID for liveness. `process.kill(pid, 0)` throws ESRCH when the +// process is dead and EPERM when it is alive but we cannot signal it; both +// are resolved here into a boolean. Process liveness — not wall-clock +// staleness — is the source of truth for abandoned-preparation recovery. +function defaultPidAlive(pid: number): boolean { + if (!Number.isInteger(pid) || pid <= 0) return false; + try { + process.kill(pid, 0); + return true; + } catch (error) { + if (error instanceof Error && 'code' in error) { + if (error.code === 'EPERM') return true; + if (error.code === 'ESRCH') return false; + } + return false; + } +} + +// Query a process start identity. `ps -o lstart= -p ` returns the +// human-readable start time (e.g. "Wed Jul 15 03:22:00 2026") on +// darwin and linux. The trimmed output is unique per process and +// survives PID reuse: a dead PID reassigned to a different process +// will produce a different identity. Returns undefined if the query +// fails or the process is gone. +function defaultProcessIdentity(pid: number): string | undefined { + if (!Number.isInteger(pid) || pid <= 0) return undefined; + try { + const result = spawnSync('ps', ['-o', 'lstart=', '-p', String(pid)], { encoding: 'utf8' }); + if (result.error) return undefined; + if (result.status !== 0) return undefined; + const stdout = result.stdout ? result.stdout.toString() : ''; + const identity = stdout.replace(/\s+/g, ' ').trim(); + return identity || undefined; + } catch { + return undefined; + } +} + +// Validate that a parsed claim has the required fields for a +// current-format record, without String coercion. Returns true if the +// claim is well-formed. A claim whose `status` is absent is treated as +// legacy and is validated separately (only `worktreeRoot` is required). +function isValidCurrentClaim(obj: Record): boolean { + if (typeof obj.deviceId !== 'string' || obj.deviceId.length === 0) return false; + if (typeof obj.worktreeRoot !== 'string' || obj.worktreeRoot.length === 0) return false; + if (typeof obj.claimId !== 'string' || obj.claimId.length === 0) return false; + if (typeof obj.status !== 'string') return false; + if (obj.status !== 'preparing' && obj.status !== 'ready') return false; + if (typeof obj.claimedAt !== 'string') return false; + if (isNaN(Date.parse(obj.claimedAt))) return false; + if ( + obj.preparerPid !== undefined && + (typeof obj.preparerPid !== 'number' || + !Number.isInteger(obj.preparerPid) || + obj.preparerPid <= 0) + ) { + return false; + } + if (obj.preparerIdentity !== undefined && typeof obj.preparerIdentity !== 'string') { + return false; + } + return true; +} + +function isValidLegacyClaim(obj: Record): boolean { + // Legacy claims have no `status` field and only require a worktreeRoot. + return typeof obj.worktreeRoot === 'string' && obj.worktreeRoot.length > 0; +} + +// Read the full on-disk claim, cleaning up invalid or stale entries. Used +// during the initial claim phase where stale-worktree cleanup is safe. +// Preparing claims are never deleted based on stale worktreeRoot — the +// caller uses PID liveness + identity to decide whether the preparation +// is active, abandoned, or legacy. Malformed claims (missing required +// fields, wrong types, unparseable dates) are removed in the initial +// phase so a fresh claim can be written. +function readClaim( lockRoot: string, deviceId: string, readFileSync: (filePath: string, encoding: 'utf8') => string = fs.readFileSync -): string | undefined { +): ClaimRecord | undefined { + let raw: string; try { - const claim = JSON.parse(readFileSync(lockPath(lockRoot, deviceId), 'utf8')) as { - worktreeRoot?: string; - }; - if (claim.worktreeRoot && fs.existsSync(claim.worktreeRoot)) return claim.worktreeRoot; - fs.rmSync(lockPath(lockRoot, deviceId), { force: true }); + raw = readFileSync(lockPath(lockRoot, deviceId), 'utf8'); } catch (error) { if (error instanceof Error && 'code' in error && error.code === 'ENOENT') return undefined; + try { + fs.rmSync(lockPath(lockRoot, deviceId), { force: true }); + } catch { + // ignore + } + return undefined; + } + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + try { + fs.rmSync(lockPath(lockRoot, deviceId), { force: true }); + } catch { + // ignore + } + return undefined; + } + if (typeof parsed !== 'object' || parsed === null) { + try { + fs.rmSync(lockPath(lockRoot, deviceId), { force: true }); + } catch { + // ignore + } + return undefined; + } + const obj = parsed as Record; + // Legacy: no `status` field, only worktreeRoot. + if (obj.status === undefined) { + if (!isValidLegacyClaim(obj)) { + try { + fs.rmSync(lockPath(lockRoot, deviceId), { force: true }); + } catch { + // ignore + } + return undefined; + } + const worktreeRoot = obj.worktreeRoot as string; + if (fs.existsSync(worktreeRoot)) return { worktreeRoot }; + try { + fs.rmSync(lockPath(lockRoot, deviceId), { force: true }); + } catch { + // ignore + } + return undefined; + } + // Current-format claim: validate required fields strictly. + if (!isValidCurrentClaim(obj)) { + try { + fs.rmSync(lockPath(lockRoot, deviceId), { force: true }); + } catch { + // ignore + } + return undefined; + } + const claim = obj as unknown as ClaimRecord; + if (claim.status === 'preparing') return claim; + // Ready: check worktreeRoot liveness. + if (claim.worktreeRoot && fs.existsSync(claim.worktreeRoot)) return claim; + try { fs.rmSync(lockPath(lockRoot, deviceId), { force: true }); - // Invalid claims are unowned. + } catch { + // ignore } return undefined; } +// Read the on-disk claim without stale-worktree cleanup. Used during the +// finalization phase where deleting a claim out from under the in-flight +// prepare would be unsafe. A legacy claim (missing status/claimId but a +// valid worktreeRoot) is treated as ready so older worktrees that predate +// the state protocol keep working. Status membership is validated +// exactly — any value other than 'preparing' or 'ready' is corrupt. +function readClaimRaw( + lockRoot: string, + deviceId: string +): + | { kind: 'missing' } + | { kind: 'corrupt' } + | { kind: 'current'; record: ClaimRecord } + | { kind: 'legacy'; worktreeRoot: string } { + let raw: string; + try { + raw = fs.readFileSync(lockPath(lockRoot, deviceId), 'utf8'); + } catch (error) { + if (error instanceof Error && 'code' in error && error.code === 'ENOENT') { + return { kind: 'missing' }; + } + // Unexpected I/O error: report as corrupt so the caller does not + // silently delete the record. + return { kind: 'corrupt' }; + } + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + return { kind: 'corrupt' }; + } + if (typeof parsed !== 'object' || parsed === null) return { kind: 'corrupt' }; + const obj = parsed as Record; + if (obj.status === undefined) { + if (!isValidLegacyClaim(obj)) return { kind: 'corrupt' }; + return { kind: 'legacy', worktreeRoot: obj.worktreeRoot as string }; + } + if (!isValidCurrentClaim(obj)) return { kind: 'corrupt' }; + return { + kind: 'current', + record: { + deviceId: obj.deviceId as string, + worktreeRoot: obj.worktreeRoot as string, + claimId: obj.claimId as string, + preparerPid: typeof obj.preparerPid === 'number' ? obj.preparerPid : undefined, + preparerIdentity: typeof obj.preparerIdentity === 'string' ? obj.preparerIdentity : undefined, + status: obj.status as ClaimStatus, + claimedAt: obj.claimedAt as string, + }, + }; +} + +// Classify a preparing claim as active or abandoned. A claim is active +// iff its preparer PID is alive AND its stored process identity matches +// the current process identity queried at decision time. PID reuse (a +// dead PID reassigned to a different process) is detected by the +// identity mismatch. If the identity cannot be queried (or is missing +// from the stored claim), the claim is treated as abandoned — fail +// closed. Arbitrary time-based staleness is not used. +function isClaimActive( + claim: ClaimRecord, + pidAlive: (pid: number) => boolean, + processIdentity: (pid: number) => string | undefined +): boolean { + if (typeof claim.preparerPid !== 'number' || claim.preparerPid <= 0) return false; + if (!pidAlive(claim.preparerPid)) return false; + // PID is alive — verify identity to detect PID reuse. Both the + // stored and current identities must be queryable and match. + if (claim.preparerIdentity === undefined) return false; + const preparerIdentity = processIdentity(claim.preparerPid); + if (preparerIdentity === undefined) return false; + return claim.preparerIdentity === preparerIdentity; +} + function withClaimMutationLock(lockRoot: string, deviceId: string, mutate: () => T): T { const mutationLockPath = `${lockPath(lockRoot, deviceId)}.lock`; return withProcessLock(mutationLockPath, `Simulator ${deviceId} claim`, mutate); @@ -104,7 +360,10 @@ function isBootstatusTerminalFailure(result: CommandResult): boolean { // that this attempt just booted so a follow-up claim does not observe a // "Booted" device we started. Never shut down a simulator that was already // booted by someone else, and never shut down a simulator whose `boot` failed -// (it never started). +// (it never started). Throws a `PrepareError` whose `shutdownFailed` flag +// tells the caller whether the follow-up shutdown also failed — when it did, +// the device may still be running and the caller must preserve the claim so +// a peer worktree cannot adopt it. function bootSimulator( device: SimulatorDevice, exec: ExecFn = execFileSync, @@ -112,6 +371,8 @@ function bootSimulator( ): void { if (device.state === 'Booted') return; let booted = false; + let shutdownFailed = false; + let cause: unknown; try { exec('xcrun', ['simctl', 'boot', device.id], { stdio: 'ignore' }); booted = true; @@ -121,21 +382,51 @@ function bootSimulator( // Echo a bounded tail of the captured output so the user can see why the // boot was rejected without flooding logs. const bounded = combined.split('\n').slice(-20).join('\n'); - throw new Error(`Simulator ${device.id} bootstatus reported terminal failure:\n${bounded}`); + cause = new Error(`Simulator ${device.id} bootstatus reported terminal failure:\n${bounded}`); + throw cause; } } catch (error) { + cause = error; if (booted) { try { exec('xcrun', ['simctl', 'shutdown', device.id], { stdio: 'ignore' }); } catch { - // Swallow shutdown failures so the original boot/bootstatus error - // surfaces to the caller. + // The follow-up shutdown failed; the device may still be running. + // Surface this via the typed `shutdownFailed` flag on PrepareError + // so the caller preserves the preparing claim instead of removing it. + shutdownFailed = true; } } - throw error; + const message = cause instanceof Error ? cause.message : String(cause); + throw new PrepareError(message, { shutdownFailed, cause }); } } +type ClaimOutcome = + | { device: SimulatorDevice; alreadyOwned: true } + | { device: SimulatorDevice; alreadyOwned: false; claimId: string; recovering: boolean }; + +function buildPreparingClaim( + deviceId: string, + worktreeRoot: string, + claimId: string, + processIdentity: (pid: number) => string | undefined +): Record { + const record: Record = { + deviceId, + worktreeRoot, + claimId, + preparerPid: process.pid, + status: 'preparing', + claimedAt: new Date().toISOString(), + }; + const identity = processIdentity(process.pid); + if (identity !== undefined) { + record.preparerIdentity = identity; + } + return record; +} + function claimSimulator(args: ClaimArgs): { device: SimulatorDevice; alreadyOwned: boolean } { const { devices, lockRoot, worktreeRoot, requestedId } = args; fs.mkdirSync(lockRoot, { recursive: true }); @@ -145,46 +436,197 @@ function claimSimulator(args: ClaimArgs): { device: SimulatorDevice; alreadyOwne if (candidates.length === 0) throw new Error(`Simulator ${requestedId ?? ''} is not available`.trim()); + const pidAlive = args.pidAlive ?? defaultPidAlive; + const processIdentity = args.processIdentity ?? defaultProcessIdentity; + for (const device of candidates) { try { - const claim = withClaimMutationLock(lockRoot, device.id, () => { - const owner = readOwner( + const outcome = withClaimMutationLock(lockRoot, device.id, (): ClaimOutcome => { + const existing = readClaim( lockRoot, device.id, args.fileOperations?.readFileSync ?? fs.readFileSync ); - const alreadyOwned = owner === worktreeRoot; - if (owner && !alreadyOwned) { - throw new Error(`Simulator ${device.id} is claimed by ${owner}`); + if (existing) { + if (existing.status === 'preparing') { + // A preparing claim is active iff its preparer PID is alive + // AND its stored process identity matches the current + // process identity queried at decision time. PID reuse (a + // dead PID reassigned to a different process) is detected + // by the identity mismatch. If the identity cannot be + // queried or is missing, the claim is treated as abandoned + // (fail closed). Recovery writes a fresh preparing claim + // under the current process so a follow-up prepare can + // safely reset the device state. + const active = isClaimActive(existing, pidAlive, processIdentity); + if (active) { + if (existing.worktreeRoot === worktreeRoot) { + throw new Error(`Simulator ${device.id} preparation in progress`); + } + throw new Error(`Simulator ${device.id} is claimed by ${existing.worktreeRoot}`); + } + // Abandoned preparing claim — adopt and replace. + const claimId = randomUUID(); + fs.writeFileSync( + lockPath(lockRoot, device.id), + JSON.stringify( + buildPreparingClaim(device.id, worktreeRoot, claimId, processIdentity) + ), + { flag: 'w' } + ); + return { device, alreadyOwned: false, claimId, recovering: true }; + } + if (existing.worktreeRoot === worktreeRoot) { + return { device, alreadyOwned: true }; + } + throw new Error(`Simulator ${device.id} is claimed by ${existing.worktreeRoot}`); } - if (!alreadyOwned) { - fs.writeFileSync( - lockPath(lockRoot, device.id), - JSON.stringify({ - deviceId: device.id, - worktreeRoot, - claimedAt: new Date().toISOString(), - }), - { flag: 'wx' } + // No claim (or a stale one was cleaned up by readClaim). Create a + // preparing claim with a unique claimId, the current PID, and the + // current process identity so a later finalization or rollback + // can verify it still owns the record. + const claimId = randomUUID(); + fs.writeFileSync( + lockPath(lockRoot, device.id), + JSON.stringify(buildPreparingClaim(device.id, worktreeRoot, claimId, processIdentity)), + { flag: 'wx' } + ); + return { device, alreadyOwned: false, claimId, recovering: false }; + }); + + if (outcome.alreadyOwned) { + return { device: outcome.device, alreadyOwned: true }; + } + + // The mutation lock is released before recovery and prepare run so + // a stalled bootstatus call cannot block peer claim attempts. The + // on-disk `preparing` claim plus the unique `claimId` are the + // source of truth; peer callers observe the preparing state and + // reject. + const claimId = outcome.claimId; + + // The device used for the normal prepare. For non-recovery claims + // this is the list-time device. For recovery claims, the recovery + // reset callback returns a confirmed Shutdown device — the + // list-time snapshot is stale and must not be passed to + // bootSimulator (which would skip the reboot and falsely finalize + // ready). + let prepareDevice: SimulatorDevice = device; + + if (outcome.recovering) { + if (!args.recoveryReset) { + // The recovery reset callback is required for abandoned + // preparation recovery. Without it we cannot confirm the + // device is in a Shutdown state, so we preserve the new + // preparing claim and throw — a peer must not be able to + // adopt a device that may still be running. + throw new Error( + `Simulator ${device.id} recovery reset is required to confirm the device is Shutdown before preparation can continue` ); } - // Hold the device mutation lock through preparation so a same-worktree - // concurrent caller cannot adopt the claim we just created while - // preparation is in flight. If the prepare callback throws, the - // rollback below removes only the claim this exact call created; a - // pre-existing same-worktree claim is preserved. We inline the - // cleanup because releaseSimulator re-acquires this same lock. + let confirmedDevice: SimulatorDevice; try { - args.prepare?.(device); - } catch (error) { - if (!alreadyOwned) { - fs.rmSync(lockPath(lockRoot, device.id), { force: true }); + confirmedDevice = args.recoveryReset(device.id); + } catch (resetError) { + // Preserve the new preparing claim (now owned by the current + // process) and surface the recovery reset failure. + throw new Error( + `Simulator ${device.id} recovery reset failed: ${ + resetError instanceof Error ? resetError.message : String(resetError) + }`, + { cause: resetError } + ); + } + // Defensive: force the state to Shutdown so bootSimulator + // cannot skip the reboot. The recovery reset is the source of + // truth for the device state. + prepareDevice = { ...confirmedDevice, state: 'Shutdown' as const }; + } + + let prepareError: PrepareError | undefined; + try { + args.prepare?.(prepareDevice); + } catch (error) { + prepareError = + error instanceof PrepareError + ? error + : new PrepareError(error instanceof Error ? error.message : String(error), { + shutdownFailed: false, + cause: error, + }); + } + + // Reacquire the mutation lock to finalize (mark ready) or roll back + // (remove the preparing claim). The on-disk record is the source of + // truth; we only touch it when our exact claimId is still current. + // On any mismatch (missing, corrupt, or replaced) the finalization + // path fails closed — it throws rather than silently returning + // success — while the rollback path leaves the record alone (we + // never delete data we do not own) and the original prepare error + // still surfaces to the caller. If the exact-own rollback deletion + // itself throws, the cleanup failure is attached to the prepare + // error so the operator can see both the original cause and the + // cleanup problem. + withClaimMutationLock(lockRoot, device.id, () => { + const result = readClaimRaw(lockRoot, device.id); + if (prepareError) { + if (prepareError.shutdownFailed) { + // Preserve the preparing claim so a peer cannot adopt a + // device that may still be running. The original prepare + // error still surfaces to the caller via the throw below. + return; + } + if (result.kind === 'current' && result.record.claimId === claimId) { + try { + fs.rmSync(lockPath(lockRoot, device.id), { force: true }); + } catch (cleanupError) { + // Attach the cleanup failure to the original prepare error + // so the operator sees both; the original prepare failure + // remains the primary reason the claim did not succeed. + prepareError = new PrepareError(prepareError.message, { + shutdownFailed: false, + cause: prepareError.cause ?? prepareError, + }); + (prepareError as PrepareError & { cleanupError?: unknown }).cleanupError = + cleanupError; + } } - throw error; + // For missing/corrupt/replaced records during rollback, do + // nothing — the original prepare error will be thrown. + return; } - return { device, alreadyOwned }; + if (result.kind === 'missing') { + throw new Error( + `Simulator ${device.id} finalization failed: claim record vanished during prepare` + ); + } + if (result.kind === 'corrupt') { + throw new Error( + `Simulator ${device.id} finalization failed: claim record is corrupt and was not modified` + ); + } + if (result.kind === 'legacy') { + // A legacy claim appeared mid-prepare (e.g., a peer on an + // older worktree wrote it). We do not own it; fail closed. + throw new Error( + `Simulator ${device.id} finalization failed: claim record was replaced by a legacy entry` + ); + } + // result.kind === 'current' + if (result.record.claimId !== claimId) { + throw new Error( + `Simulator ${device.id} finalization failed: claim record was replaced by another owner` + ); + } + fs.writeFileSync( + lockPath(lockRoot, device.id), + JSON.stringify({ ...result.record, status: 'ready' }), + { flag: 'w' } + ); }); - return claim; + + if (prepareError) throw prepareError; + return { device: outcome.device, alreadyOwned: false }; } catch (error) { if (error instanceof Error && 'code' in error && error.code === 'EEXIST') { if (requestedId) { @@ -203,6 +645,10 @@ function claimSimulator(args: ClaimArgs): { device: SimulatorDevice; alreadyOwne if (requestedId) throw error; continue; } + if (error instanceof Error && error.message.includes('preparation in progress')) { + if (requestedId) throw error; + continue; + } throw error; } } @@ -215,9 +661,30 @@ function releaseSimulator(args: { worktreeRoot: string; }): void { withClaimMutationLock(args.lockRoot, args.deviceId, () => { - const owner = readOwner(args.lockRoot, args.deviceId); - if (owner && owner !== args.worktreeRoot) { - throw new Error(`Simulator ${args.deviceId} is claimed by ${owner}`); + const result = readClaimRaw(args.lockRoot, args.deviceId); + if (result.kind === 'missing') { + // Nothing to release — idempotent. + return; + } + if (result.kind === 'corrupt') { + // Do not delete unknown data; surface a clear error so an operator + // can inspect the on-disk record manually. + throw new Error(`Simulator ${args.deviceId} claim is corrupt and cannot be released`); + } + if (result.kind === 'current') { + if (result.record.status === 'preparing') { + // A preparing claim may be mid-boot; releasing it would let a + // peer adopt a device that is still being prepared. + throw new Error( + `Simulator ${args.deviceId} cannot be released while preparation is in progress` + ); + } + if (result.record.worktreeRoot !== args.worktreeRoot) { + throw new Error(`Simulator ${args.deviceId} is claimed by ${result.record.worktreeRoot}`); + } + } else if (result.worktreeRoot !== args.worktreeRoot) { + // Legacy claim — treat as ready, but enforce worktree ownership. + throw new Error(`Simulator ${args.deviceId} is claimed by ${result.worktreeRoot}`); } fs.rmSync(lockPath(args.lockRoot, args.deviceId), { force: true }); }); @@ -248,6 +715,33 @@ function main(): void { worktreeRoot, requestedId, prepare: device => bootSimulator(device), + // Production recovery reset: re-read the actual simulator state + // at decision time (not the list-time snapshot). If the device + // is not Shutdown, issue a shutdown, re-read, and confirm + // Shutdown. Throw if the shutdown cannot be confirmed so the + // preparing claim is preserved and a peer cannot adopt a + // possibly running device. No mutation lock is held across + // these synchronous commands. + recoveryReset: deviceId => { + const reReadState = (): string | undefined => { + const devices = listIosDevices(); + return devices.find(d => d.id === deviceId)?.state; + }; + let state = reReadState(); + if (state === undefined) { + throw new Error(`Simulator ${deviceId} not found during recovery reset`); + } + if (state !== 'Shutdown') { + execFileSync('xcrun', ['simctl', 'shutdown', deviceId], { stdio: 'ignore' }); + state = reReadState(); + if (state !== 'Shutdown') { + throw new Error( + `Simulator ${deviceId} shutdown not confirmed (state=${state ?? 'unknown'})` + ); + } + } + return { id: deviceId, name: deviceId, state: 'Shutdown' as const }; + }, }); console.log(JSON.stringify({ ...claim, worktreeRoot })); return; @@ -257,7 +751,7 @@ function main(): void { console.log(`Released ${requestedId}`); return; } - throw new Error('Usage: pnpm dev:mobile:simulator >'); + throw new Error('Usage: pnpm dev:mobile:simulator '); } const isMain = @@ -272,4 +766,4 @@ if (isMain) { } export { bootSimulator, claimSimulator, releaseSimulator }; -export type { SimulatorDevice }; +export type { SimulatorDevice, ClaimRecord, ClaimStatus }; From bfd366acfc59e332a5d04a62e0f8f8ca0c77d650 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Wed, 15 Jul 2026 04:19:35 +0200 Subject: [PATCH 7/7] test(dev): verify simulator rollback cleanup --- dev/local/mobile-simulator.test.ts | 42 +++++++++++++++--------------- dev/local/mobile-simulator.ts | 10 +++++-- 2 files changed, 29 insertions(+), 23 deletions(-) diff --git a/dev/local/mobile-simulator.test.ts b/dev/local/mobile-simulator.test.ts index 775698c92a..bf7cebf6ae 100644 --- a/dev/local/mobile-simulator.test.ts +++ b/dev/local/mobile-simulator.test.ts @@ -1697,7 +1697,7 @@ test('new preparing claims include both the current PID and process identity', ( test('attaches a rollback cleanup failure to the original prepare error', () => { const lockRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-simulator-locks-')); const worktreeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'kilo-worktree-')); - let caught: Error | undefined; + let caught: (Error & { cleanupError?: unknown }) | undefined; try { try { @@ -1707,38 +1707,38 @@ test('attaches a rollback cleanup failure to the original prepare error', () => worktreeRoot, requestedId: 'B', prepare: () => { - // Replace the claim file with a directory so the exact-own - // rollback rmSync fails deterministically. The error from - // fs.rmSync on a directory is platform-specific (EBUSY, - // EPERM, ENOTDIR); the test only asserts that the original - // prepare failure is the primary message and that the - // cleanup failure is attached for the operator. - const claimPath = path.join(lockRoot, 'B.json'); - fs.rmSync(claimPath, { force: true }); - fs.mkdirSync(claimPath); throw new Error('bootstatus failed'); }, + fileOperations: { + // Inject an rmSync that throws only for the exact claim file + // so the rollback cleanup failure path is exercised + // deterministically across platforms. + rmSync: (filePath, options) => { + if (filePath === path.join(lockRoot, 'B.json')) { + throw new Error('injected cleanup deletion failure'); + } + fs.rmSync(filePath, options); + }, + }, }); } catch (error) { - caught = error as Error; + caught = error as Error & { cleanupError?: unknown }; } assert.ok(caught, 'claimSimulator must throw when prepare fails'); // The original prepare failure must be the primary message. assert.match(String(caught), /bootstatus failed/); - // The cleanup failure must be attached so the operator can see it. - const withCleanup = caught as Error & { cleanupError?: unknown }; + // The cleanup failure must be attached specifically as `cleanupError` + // so the operator can see it. We assert on `cleanupError` directly + // (not via `cause`, which is always set on every PrepareError and + // would mask a missing cleanupError). assert.ok( - withCleanup.cleanupError !== undefined || caught.cause !== undefined, - 'cleanup failure must be attached to the prepare error' + caught.cleanupError !== undefined, + 'cleanupError must be attached to the prepare error so the operator can see the rollback failure' ); + const cleanupError = caught.cleanupError as Error; + assert.match(String(cleanupError), /injected cleanup deletion failure/); } finally { - // Clean up the swapped directory if it still exists. - try { - fs.rmSync(path.join(lockRoot, 'B.json'), { recursive: true, force: true }); - } catch { - // ignore - } fs.rmSync(lockRoot, { recursive: true, force: true }); fs.rmSync(worktreeRoot, { recursive: true, force: true }); } diff --git a/dev/local/mobile-simulator.ts b/dev/local/mobile-simulator.ts index 9fc9cda7d5..cad22dbb7e 100644 --- a/dev/local/mobile-simulator.ts +++ b/dev/local/mobile-simulator.ts @@ -62,6 +62,11 @@ type ClaimArgs = { pidAlive?: (pid: number) => boolean; fileOperations?: { readFileSync?: (filePath: string, encoding: 'utf8') => string; + // Injectable rmSync used only for exact-own rollback deletion. + // Default is `fs.rmSync`. Tests can inject a throwing stub to + // verify cleanup-error attachment without platform-specific + // filesystem tricks. + rmSync?: (filePath: string, options: { force?: boolean }) => void; }; }; @@ -569,6 +574,7 @@ function claimSimulator(args: ClaimArgs): { device: SimulatorDevice; alreadyOwne // cleanup problem. withClaimMutationLock(lockRoot, device.id, () => { const result = readClaimRaw(lockRoot, device.id); + const rmSync = args.fileOperations?.rmSync ?? fs.rmSync; if (prepareError) { if (prepareError.shutdownFailed) { // Preserve the preparing claim so a peer cannot adopt a @@ -578,7 +584,7 @@ function claimSimulator(args: ClaimArgs): { device: SimulatorDevice; alreadyOwne } if (result.kind === 'current' && result.record.claimId === claimId) { try { - fs.rmSync(lockPath(lockRoot, device.id), { force: true }); + rmSync(lockPath(lockRoot, device.id), { force: true }); } catch (cleanupError) { // Attach the cleanup failure to the original prepare error // so the operator sees both; the original prepare failure @@ -751,7 +757,7 @@ function main(): void { console.log(`Released ${requestedId}`); return; } - throw new Error('Usage: pnpm dev:mobile:simulator '); + throw new Error('Usage: pnpm dev:mobile:simulator >'); } const isMain =