Skip to content

Commit 80b55dc

Browse files
cameroncookecodex
andcommitted
fix(daemon): Address workspace cleanup review feedback
Harden daemon socket directory validation, make shutdown cleanup use the expanded timeout budget, and remove stale registry-owned sockets by their recorded path. Keep workspace-scoped log paths in snapshot normalization and preserve Swift Testing display names that end with a period. Co-Authored-By: OpenAI Codex <codex@openai.com>
1 parent bfda741 commit 80b55dc

123 files changed

Lines changed: 540 additions & 377 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

src/daemon.ts

Lines changed: 241 additions & 229 deletions
Large diffs are not rendered by default.

src/daemon/__tests__/daemon-registry.test.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,18 @@ describe('daemon registry', () => {
232232
expect(existsSync(replacementEntry.socketPath)).toBe(true);
233233
});
234234

235+
it('cleans up the registry-owned socket when no socket path is provided', () => {
236+
const entry = createEntry({ socketPath: path.join(daemonRunDir, 'custom.sock') });
237+
writeDaemonRegistryEntry(entry);
238+
mkdirSync(path.dirname(entry.socketPath), { recursive: true, mode: 0o700 });
239+
writeFileSync(entry.socketPath, 'socket placeholder');
240+
241+
cleanupWorkspaceDaemonFiles(entry.workspaceKey);
242+
243+
expect(existsSync(registryPathForWorkspaceKey(entry.workspaceKey))).toBe(false);
244+
expect(existsSync(entry.socketPath)).toBe(false);
245+
});
246+
235247
it('cleans up stale matching workspace metadata and socket', () => {
236248
const entry = createEntry();
237249
writeDaemonRegistryEntry(entry);
Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
import { afterEach, beforeEach, describe, expect, it } from 'vitest';
2+
import {
3+
chmodSync,
4+
existsSync,
5+
mkdtempSync,
6+
rmSync,
7+
statSync,
8+
symlinkSync,
9+
writeFileSync,
10+
} from 'node:fs';
11+
import { tmpdir } from 'node:os';
12+
import * as path from 'node:path';
13+
import { ensureSocketDir } from '../socket-path.ts';
14+
15+
let tempDir: string;
16+
17+
describe('ensureSocketDir', () => {
18+
beforeEach(() => {
19+
tempDir = mkdtempSync(path.join(tmpdir(), 'xcodebuildmcp-socket-path-'));
20+
});
21+
22+
afterEach(() => {
23+
rmSync(tempDir, { recursive: true, force: true });
24+
});
25+
26+
it('creates a private socket directory', () => {
27+
const socketPath = path.join(tempDir, 'daemon', 'd.sock');
28+
29+
ensureSocketDir(socketPath);
30+
31+
expect(existsSync(path.dirname(socketPath))).toBe(true);
32+
expect(statSync(path.dirname(socketPath)).mode & 0o777).toBe(0o700);
33+
});
34+
35+
it('tightens permissions on an existing socket directory owned by the current user', () => {
36+
const dir = path.join(tempDir, 'daemon');
37+
const socketPath = path.join(dir, 'd.sock');
38+
ensureSocketDir(socketPath);
39+
chmodSync(dir, 0o755);
40+
41+
ensureSocketDir(socketPath);
42+
43+
expect(statSync(dir).mode & 0o777).toBe(0o700);
44+
});
45+
46+
it('rejects symlink socket directories', () => {
47+
const targetDir = path.join(tempDir, 'target');
48+
const linkDir = path.join(tempDir, 'daemon');
49+
ensureSocketDir(path.join(targetDir, 'placeholder.sock'));
50+
symlinkSync(targetDir, linkDir);
51+
52+
expect(() => ensureSocketDir(path.join(linkDir, 'd.sock'))).toThrow(/cannot be a symlink/u);
53+
});
54+
55+
it('rejects non-directory socket path parents', () => {
56+
const filePath = path.join(tempDir, 'daemon');
57+
writeFileSync(filePath, 'not a directory');
58+
59+
expect(() => ensureSocketDir(path.join(filePath, 'd.sock'))).toThrow(/not a directory/u);
60+
});
61+
});

src/daemon/daemon-registry.ts

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import {
99
writeFileSync,
1010
} from 'node:fs';
1111
import { dirname, join } from 'node:path';
12-
import { daemonDirForWorkspaceKey, registryPathForWorkspaceKey } from './socket-path.ts';
12+
import { registryPathForWorkspaceKey } from './socket-path.ts';
1313
import { getWorkspacesDir, getWorkspaceFilesystemLayout } from '../utils/log-paths.ts';
1414
import { tryAcquireFsLockSync } from '../utils/fs-lock-sync.ts';
1515
import { isPidAlive } from '../utils/process-liveness.ts';
@@ -319,16 +319,14 @@ export function cleanupWorkspaceDaemonFiles(
319319
options?: DaemonFileCleanupOptions,
320320
): void {
321321
withDaemonRegistryMutationLock(workspaceKey, () => {
322-
const socketPath =
323-
options?.socketPath ?? join(daemonDirForWorkspaceKey(workspaceKey), 'd.sock');
324322
const registryPath = registryPathForWorkspaceKey(workspaceKey);
325323
const removed = removeRegistryAtPathIfOwned(registryPath, workspaceKey, options);
326-
if (!removed || removed.socketPath !== socketPath) {
324+
if (!removed) {
327325
return;
328326
}
329327

330328
try {
331-
unlinkSync(socketPath);
329+
unlinkSync(removed.socketPath);
332330
} catch {
333331
// ignore
334332
}

src/daemon/socket-path.ts

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { mkdirSync, existsSync, unlinkSync } from 'node:fs';
1+
import { chmodSync, existsSync, lstatSync, mkdirSync, statSync, unlinkSync } from 'node:fs';
22
import { join, dirname } from 'node:path';
33
import { tmpdir } from 'node:os';
44
import {
@@ -65,11 +65,33 @@ export function getSocketPath(opts?: GetSocketPathOptions): string {
6565
return socketPathForWorkspaceRoot(workspaceRoot);
6666
}
6767

68+
function validateSocketDir(dir: string): void {
69+
const linkStat = lstatSync(dir);
70+
if (linkStat.isSymbolicLink()) {
71+
throw new Error(`Daemon socket directory cannot be a symlink: ${dir}`);
72+
}
73+
74+
const stat = statSync(dir);
75+
if (!stat.isDirectory()) {
76+
throw new Error(`Daemon socket path parent is not a directory: ${dir}`);
77+
}
78+
79+
const uid = process.getuid?.();
80+
if (uid !== undefined && stat.uid !== uid) {
81+
throw new Error(`Daemon socket directory is not owned by the current user: ${dir}`);
82+
}
83+
84+
if ((stat.mode & 0o077) !== 0) {
85+
chmodSync(dir, 0o700);
86+
}
87+
}
88+
6889
export function ensureSocketDir(socketPath: string): void {
6990
const dir = dirname(socketPath);
7091
if (!existsSync(dir)) {
7192
mkdirSync(dir, { recursive: true, mode: 0o700 });
7293
}
94+
validateSocketDir(dir);
7395
}
7496

7597
export function removeStaleSocket(socketPath: string): void {

src/server/__tests__/mcp-shutdown.test.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,9 @@ describe('runMcpShutdown', () => {
203203
(step) => step.name === 'workspace-filesystem.cleanup-owned',
204204
);
205205
expect(filesystemStep?.status).toBe('completed');
206+
expect(mocks.cleanupOwnedWorkspaceFilesystemArtifacts).toHaveBeenCalledWith({
207+
timeoutMs: 2100,
208+
});
206209
});
207210

208211
it('uses a larger timeout budget for debugger dispose-all', async () => {

src/server/mcp-shutdown.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,10 @@ export async function runMcpShutdown(input: {
174174
);
175175
};
176176

177+
const workspaceFilesystemCleanupTimeoutMs = bulkStepTimeoutMs(
178+
input.snapshot.ownedSimulatorLaunchOsLogSessionCount,
179+
);
180+
177181
const cleanupSteps: Array<{
178182
name: string;
179183
timeoutMs: number;
@@ -192,8 +196,11 @@ export async function runMcpShutdown(input: {
192196
},
193197
{
194198
name: 'workspace-filesystem.cleanup-owned',
195-
timeoutMs: bulkStepTimeoutMs(input.snapshot.ownedSimulatorLaunchOsLogSessionCount),
196-
operation: () => cleanupOwnedWorkspaceFilesystemArtifacts({ timeoutMs: STEP_TIMEOUT_MS }),
199+
timeoutMs: workspaceFilesystemCleanupTimeoutMs,
200+
operation: () =>
201+
cleanupOwnedWorkspaceFilesystemArtifacts({
202+
timeoutMs: workspaceFilesystemCleanupTimeoutMs,
203+
}),
197204
},
198205
{
199206
name: 'video-capture.stop-all',

src/snapshot-tests/__fixtures__/cli/device/build--error-compiler.txt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,4 +13,4 @@ Compiler Errors (1):
1313
example_projects/iOS_Calculator/CalculatorApp/CalculatorApp.swift:33:42
1414

1515
❌ Build failed. (⏱️ <DURATION>)
16-
└ Build Logs: <HOME>/Library/Developer/XcodeBuildMCP/logs/build_device_<TIMESTAMP>_pid<PID>.log
16+
└ Build Logs: <HOME>/Library/Developer/XcodeBuildMCP/workspaces/XcodeBuildMCP-<HASH>/logs/build_device_<TIMESTAMP>_pid<PID>.log

src/snapshot-tests/__fixtures__/cli/device/build--error-wrong-scheme.txt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,4 +12,4 @@ Errors (1):
1212
✗ The workspace named "CalculatorApp" does not contain a scheme named "NONEXISTENT". The "-list" option can be used to find the names of the schemes in the workspace.
1313

1414
❌ Build failed. (⏱️ <DURATION>)
15-
└ Build Logs: <HOME>/Library/Developer/XcodeBuildMCP/logs/build_device_<TIMESTAMP>_pid<PID>.log
15+
└ Build Logs: <HOME>/Library/Developer/XcodeBuildMCP/workspaces/XcodeBuildMCP-<HASH>/logs/build_device_<TIMESTAMP>_pid<PID>.log

src/snapshot-tests/__fixtures__/cli/device/build--success.txt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
Derived Data: <HOME>/Library/Developer/XcodeBuildMCP/workspaces/XcodeBuildMCP-<HASH>/DerivedData/CalculatorApp-<HASH>
99

1010
✅ Build succeeded. (⏱️ <DURATION>)
11-
└ Build Logs: <HOME>/Library/Developer/XcodeBuildMCP/logs/build_device_<TIMESTAMP>_pid<PID>.log
11+
└ Build Logs: <HOME>/Library/Developer/XcodeBuildMCP/workspaces/XcodeBuildMCP-<HASH>/logs/build_device_<TIMESTAMP>_pid<PID>.log
1212

1313
Next steps:
1414
1. Get built device app path: xcodebuildmcp device get-app-path --scheme "CalculatorApp"

0 commit comments

Comments
 (0)