Skip to content

Fix xcodemake log path generation #467

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
reallyseth wants to merge 1 commit into from
+93 −12
Closed

Fix xcodemake log path generation #467

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 37 additions & 1 deletion src/utils/__tests__/xcodemake.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
import { beforeEach, describe, expect, it, vi } from 'vitest';
import { createHash } from 'crypto';
import { mkdtempSync, rmSync, writeFileSync } from 'fs';
import { tmpdir } from 'os';
import { join } from 'path';

const { executorMock } = vi.hoisted(() => ({
executorMock: vi.fn(),
Expand All @@ -8,7 +12,7 @@ vi.mock('../command.ts', () => ({
getDefaultCommandExecutor: () => executorMock,
}));

import { executeXcodemakeCommand } from '../xcodemake.ts';
import { doesMakeLogFileExist, executeXcodemakeCommand } from '../xcodemake.ts';

describe('executeXcodemakeCommand', () => {
beforeEach(() => {
Expand Down Expand Up @@ -47,3 +51,35 @@ describe('executeXcodemakeCommand', () => {
expect(process.cwd()).toBe(originalCwd);
});
});

describe('doesMakeLogFileExist', () => {
it('checks for the sanitized xcodemake log name when arguments contain absolute paths', () => {
const projectDir = mkdtempSync(join(tmpdir(), 'xcodebuildmcp-xcodemake-'));
const commandArgs = [
'-workspace',
'SampleApp.xcworkspace',
'-scheme',
'SampleApp',
'-derivedDataPath',
'/Users/test/Library/Developer/XcodeBuildMCP/workspaces/SampleApp/DerivedData',
'build',
];
const logTag = ['xcodemake', ...commandArgs]
.join('_')
.replace(/[/:]+/g, '_')
.replace(/[^\p{L}\p{N}._+=,@ -]+/gu, '_')
.replace(/\s+/g, '_')
.replace(/_+/g, '_')
.replace(/^_+|_+$/g, '');
const logSuffix = `-${createHash('md5').update(commandArgs.join('\0')).digest('hex')}.log`;
const logFileName = `${logTag.slice(0, 150 - logSuffix.length).replace(/_+$/g, '')}${logSuffix}`;

try {
writeFileSync(join(projectDir, logFileName), '');

expect(doesMakeLogFileExist(projectDir, ['xcodebuild', ...commandArgs])).toBe(true);
} finally {
rmSync(projectDir, { recursive: true, force: true });
}
});
});
67 changes: 56 additions & 11 deletions src/utils/xcodemake.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { existsSync, readdirSync, statSync } from 'fs';
import * as path from 'path';
import * as os from 'os';
import * as fs from 'fs/promises';
import { createHash } from 'crypto';
import { getConfig } from './config-store.ts';

let overriddenXcodemakePath: string | null = null;
Expand Down Expand Up @@ -68,7 +69,7 @@ async function installXcodemake(): Promise<boolean> {
}

const scriptContent = await response.text();
await fs.writeFile(xcodemakePath, scriptContent, 'utf8');
await fs.writeFile(xcodemakePath, patchXcodemakeScript(scriptContent), 'utf8');

await fs.chmod(xcodemakePath, 0o755);
log('info', 'Made xcodemake executable');
Expand Down Expand Up @@ -125,12 +126,59 @@ export function doesMakefileExist(projectDir: string): boolean {
return existsSync(`${projectDir}/Makefile`);
}

export function doesMakeLogFileExist(projectDir: string, command: string[]): boolean {
const originalDir = process.cwd();
function sanitizeXcodemakeLogTag(args: string[]): string {
const sanitized = ['xcodemake', ...args]
.join('_')
.replace(/[/:]+/g, '_')
.replace(/[^\p{L}\p{N}._+=,@ -]+/gu, '_')
Comment on lines +132 to +133

@sentry sentry Bot Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The TypeScript code uses a Unicode-aware regex to sanitize log file names, while the patched Perl script uses [:alnum:], which may not handle Unicode correctly without explicit UTF-8 mode, causing a mismatch.
Severity: LOW

Suggested Fix

To ensure consistent behavior across different environments, add use utf8; or use feature 'unicode_strings'; to the beginning of the patched Perl script. This will guarantee that the [:alnum:] character class correctly handles Unicode characters, aligning its behavior with the TypeScript implementation.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: src/utils/xcodemake.ts#L132-L133

Potential issue: The `sanitizeXcodemakeLogTag` function in TypeScript uses a
Unicode-aware regular expression (`/[^\p{L}\p{N}._+=,@ -]+/gu`) to sanitize arguments
for a log file name. However, the corresponding logic in the patched Perl script uses a
POSIX character class (`[^[:alnum:]._+=,@ -]`). The Perl script does not explicitly
enable UTF-8 mode (e.g., with `use utf8;`), so its handling of non-ASCII characters is
dependent on the system's locale. This discrepancy can cause the log file name generated
by the Perl script to differ from the one expected by the TypeScript code, leading
`doesMakeLogFileExist` to incorrectly return `false` and trigger unnecessary rebuilds.

Also affects:

  • src/utils/xcodemake.ts:160~161

Did we get this right? 👍 / 👎 to inform future reviews.

.replace(/\s+/g, '_')
.replace(/_+/g, '_')
.replace(/^_+|_+$/g, '');

try {
process.chdir(projectDir);
return sanitized || 'xcodemake';
}

function getXcodemakeLogFileName(args: string[]): string {
const logSuffix = `-${createHash('md5').update(args.join('\0')).digest('hex')}.log`;
const maxLogNameLength = 150;
const maxLogTagLength = Math.max(0, maxLogNameLength - logSuffix.length);
const logTag = sanitizeXcodemakeLogTag(args).slice(0, maxLogTagLength).replace(/_+$/g, '');

return `${logTag || 'xcodemake'}${logSuffix}`;
}

function patchXcodemakeScript(scriptContent: string): string {
const patchedImports = scriptContent.includes('use Digest::MD5 qw(md5_hex);')
? scriptContent
: scriptContent.replace('use IO::File;\n', 'use IO::File;\nuse Digest::MD5 qw(md5_hex);\n');

return patchedImports
.replace(
'my $log = "xcodemake @original_ARGV.log";',
`my $logTag = join "_", ("xcodemake", @original_ARGV);
$logTag =~ s{[/:]+}{_}g;
$logTag =~ s{[^[:alnum:]._+=,@ -]+}{_}g;
$logTag =~ s{\\s+}{_}g;
$logTag =~ s{_+}{_}g;
$logTag =~ s{^_+|_+$}{}g;
$logTag ||= "xcodemake";
my $maxLogNameLength = 150;
my $logSuffix = "-" . md5_hex(join "\\0", @original_ARGV) . ".log";
$logTag = substr($logTag, 0, $maxLogNameLength - length($logSuffix));
$logTag =~ s{_+$}{};
my $log = ($logTag || "xcodemake") . $logSuffix;`,
)
.replaceAll(
`dollarEscape($make_dep_source);
unescape("'", $make_dep_source);`,
`dollarEscape($make_dep_source);
unescape("'", $make_dep_source);
escape(" ", $make_dep_source);`,
);

@cursor cursor Bot Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fragile patch leaves script unchanged

Medium Severity

patchXcodemakeScript relies on exact substring replacements with no check that they succeeded. If upstream xcodemake changes whitespace or wording, the written script may still use legacy log naming or omit Digest::MD5 while the patched block calls md5_hex, so log checks and xcodemake runs can fail silently or at runtime after download.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 5c0baf1. Configure here.

}

export function doesMakeLogFileExist(projectDir: string, command: string[]): boolean {
try {
const xcodemakeCommand = ['xcodemake', ...command.slice(1)];
const escapedCommand = xcodemakeCommand.map((arg) => {
// Remove projectDir from arguments if present at the start
Expand All @@ -140,11 +188,10 @@ export function doesMakeLogFileExist(projectDir: string, command: string[]): boo
}
return arg;
});
const commandString = escapedCommand.join(' ');
const logFileName = `${commandString}.log`;
log('debug', `Checking for Makefile log: ${logFileName} in directory: ${process.cwd()}`);
const logFileName = getXcodemakeLogFileName(escapedCommand.slice(1));
log('debug', `Checking for Makefile log: ${logFileName} in directory: ${projectDir}`);

const files = readdirSync('.');
const files = readdirSync(projectDir);

@cursor cursor Bot Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PATH xcodemake log name mismatch

Medium Severity

doesMakeLogFileExist now looks only for hashed, sanitized log filenames, but isXcodemakeAvailable still treats a PATH xcodemake from which as valid without applying patchXcodemakeScript. That binary keeps writing legacy log names, so the existence check stays false even when a Makefile and log are already present, and incremental make is skipped on every build.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 5c0baf1. Configure here.

const exists = files.includes(logFileName);
log('debug', `Makefile log ${exists ? 'exists' : 'does not exist'}: ${logFileName}`);
return exists;
Expand All @@ -154,8 +201,6 @@ export function doesMakeLogFileExist(projectDir: string, command: string[]): boo
`Error checking for Makefile log: ${error instanceof Error ? error.message : String(error)}`,
);
return false;
} finally {
process.chdir(originalDir);
}
}

Expand Down
Loading