feat(build-tools): Add build pipeline workflow with 4 new tools#362
feat(build-tools): Add build pipeline workflow with 4 new tools#362paul-maas wants to merge 8 commits intogetsentry:mainfrom
Conversation
Add build-tools workflow group for macOS build pipeline automation: - xcodegen_generate: run xcodegen generate in project directory - create_dmg: create DMG from built app (with path traversal/symlink escape protection) - codesign_app: code sign + optional notarization pipeline (sign, verify, notarize, staple) - pfctl_anchor: read-only PF firewall anchor inspection (cannot modify firewall state) All tools use shared command-result structured output schema and createTypedTool pattern. 1 upstream file modified (domain-results.ts, +8 lines), 15 new files, 68 tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Wrapper script that launches supergateway with correct PATH (homebrew, MacPorts, user-local) and workflow config so build-tools and other homebrew-installed binaries are reachable from Docker containers via Streamable HTTP. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| const notarizeCommand: string[] = [ | ||
| 'xcrun', | ||
| 'notarytool', | ||
| 'submit', | ||
| params.targetPath, | ||
| '--team-id', | ||
| params.teamId!, | ||
| ]; | ||
| if (params.keychainProfile != null) { | ||
| notarizeCommand.push('--keychain-profile', params.keychainProfile); | ||
| } |
There was a problem hiding this comment.
Bug: When notarize is true but keychainProfile is omitted, the generated notarytool command lacks authentication flags, causing notarization to fail.
Severity: HIGH
Suggested Fix
Modify the command construction logic. When notarize is true and keychainProfile is not provided, ensure the command includes other valid authentication flags, such as --apple-id and --password, by reading them from the tool's parameters. Alternatively, enforce that keychainProfile is required when notarize is true through schema validation.
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/mcp/tools/build-tools/codesign_app.ts#L127-L138
Potential issue: When `notarize: true` is set without providing a `keychainProfile`, the
code constructs an `xcrun notarytool submit` command that lacks required authentication
flags. Apple's `notarytool` requires authentication via `--keychain-profile`, or
`--apple-id` and `--password`. The code's assumption that environment variables would be
automatically used for authentication is incorrect. Consequently, any notarization
attempt without a `keychainProfile` will fail at runtime with an authentication error,
rendering this feature path unusable for users relying on other authentication methods.
…entry - Fix TOCTOU race in create_dmg: validateScriptPath now returns the resolved realScriptPath instead of discarding it, eliminating the second unguarded realpathSync call - Fix positional arg misordering in create_dmg: outputPath is only passed when appPath is also present, preventing wrong-slot injection - Fix unused bundleId in codesign_app: removed from .refine() check since xcrun notarytool submit does not accept --bundle-id - Fix validation ordering in pfctl_anchor: rulesFile extension check now runs before buildCommand, keeping unvalidated values out of the command array - Fix duplicate --port in serve-mcp.sh: filter --port from passthrough args before forwarding to supergateway - Deduplicate setStructuredOutput into command-result-helpers.ts Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| if (!/\.(conf|rules)$/.test(params.rulesFile)) { | ||
| const result = createCommandFailure( | ||
| commandLabel, | ||
| `rulesFile must end with .conf or .rules: ${params.rulesFile}`, | ||
| ); | ||
| setStructuredOutput(ctx, result); | ||
| return; |
There was a problem hiding this comment.
Bug: The rulesFile parameter in pfctl_anchor.ts lacks proper path validation, allowing absolute paths and path traversal sequences (..), which creates a security gap.
Severity: LOW
Suggested Fix
Implement robust path validation for rulesFile, similar to the pattern in create_dmg.ts. Add checks to ensure the path is relative, does not contain path traversal (..), and resolves to a location within an expected directory using fs.realpathSync to prevent symlink escapes.
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/mcp/tools/build-tools/pfctl_anchor.ts#L52-L58
Potential issue: The `pfctl_anchor.ts` tool does not properly validate the `rulesFile`
input. It only checks for a `.conf` or `.rules` file extension but fails to prevent
absolute paths (e.g., `/etc/pf.conf`) or path traversal sequences (e.g.,
`../../sensitive.rules`). This is inconsistent with security patterns used elsewhere in
the codebase, such as in `create_dmg.ts`, which explicitly checks for and prevents these
vulnerabilities. While the current usage with `pfctl -n -f` is a dry-run syntax check
and limits the immediate security risk, this validation gap allows a user to point the
tool to any file on the system ending with the allowed extensions, which could be used
to probe for file existence or cause unintended behavior.
Bash set -u treats empty array expansion as unbound variable.
Use ${arr[@]+...} pattern for safe empty-array expansion.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ning - Remove Sentry MCP server from .mcp.json (not needed for fork) - Add --stateful flag to supergateway in serve-mcp.sh to prevent spawning a new child process per HTTP request - Disable Sentry telemetry via .xcodebuildmcp/config.yaml (not committed, local-only) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| "url": "https://mcp.sentry.dev/mcp" | ||
| } | ||
| } | ||
| "mcpServers": {} |
There was a problem hiding this comment.
Unrelated Sentry MCP config removal appears accidental
Low Severity
The Sentry MCP server configuration was removed from .mcp.json. This change seems accidentally included, as it's unrelated to the PR's purpose of adding build-tools and isn't mentioned in the PR description.
Reviewed by Cursor Bugbot for commit fdb060b. Configure here.
Spawned commands like xcodegen failed with ENOENT when the MCP server was launched outside a login shell (supergateway, Docker). Instead of relying on wrapper scripts to set PATH, the executor now prepends common tool directories (/opt/homebrew/bin, /usr/local/bin, /opt/local/bin) at module load time. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ie MCP processes ensure-mcp-server.sh now uses a per-port PID file to track the running server. On re-invocation it gracefully stops the old process before starting a new one, and refuses to start if the port is owned by an unknown process. serve-mcp.sh replaces exec with trap/wait so SIGTERM/SIGINT propagate to the entire process group (supergateway + node mcp), preventing orphaned child processes after the wrapper is killed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
supergateway v3.4.3 calls transport.send() without await inside a sync forEach/try-catch. When the HTTP connection closes before the child responds, the rejected promise crashes the process. This patch appends .catch() to all three affected gateway files. Upstream: supercorp-ai/supergateway#116 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| }; | ||
| export type CommandResultDomainResult = ToolDomainResultBase & { | ||
| kind: 'command-result'; | ||
| command: string; | ||
| summary: StatusSummary; | ||
| output: string; | ||
| diagnostics: BasicDiagnostics; | ||
| }; | ||
| export type WorkflowSelectionDomainResult = ToolDomainResultBase & { | ||
| kind: 'workflow-selection'; | ||
| enabledWorkflows: string[]; |
There was a problem hiding this comment.
Bug: The text renderer for the 'command-result' domain result is missing a case, causing it to omit the actual output and command fields from the text response.
Severity: MEDIUM
Suggested Fix
Add a case 'command-result': to the createSpecialCaseItems switch statement in src/utils/renderers/domain-result-text.ts. This new case should handle the CommandResultDomainResult type and render its command and output fields into the text response, ensuring the command's output is visible to the user.
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/types/domain-results.ts#L622-L634
Potential issue: The 'command-result' domain result kind, used by new tools like
`xcodegen_generate`, `create_dmg`, `codesign_app`, and `pfctl_anchor`, is not handled by
the `createSpecialCaseItems` switch statement in
`src/utils/renderers/domain-result-text.ts`. When rendering a result of this type for
text output, the system falls back to a generic rendering path. This generic path
correctly displays diagnostics and a summary status, but it omits the `output` and
`command` fields. As a consequence, the actual command output is silently dropped from
the text response, making it difficult for users relying on text output to see the
results of the command.
Also affects:
src/mcp/tools/build-tools/command-result-helpers.ts:1~48
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8d80ec7. Configure here.
| const spawnOpts: Parameters<typeof spawn>[2] = { | ||
| stdio: ['ignore', 'pipe', 'pipe'], | ||
| env: opts?.env ? { ...process.env, ...opts.env } : process.env, | ||
| env: { ...process.env, PATH: enrichedPath, ...opts?.env }, |
There was a problem hiding this comment.
Debug log prints stale PATH instead of enriched PATH
Low Severity
The debug log at line 74 prints process.env.PATH, but the spawned process actually uses enrichedPath (line 70), which may include additional directories like /opt/homebrew/bin. When debugging command-not-found issues, this log line will show the wrong PATH, making troubleshooting misleading.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8d80ec7. Configure here.
1 participant
Summary
build-toolsworkflow group with 4 new MCP tools for macOS build pipeline automationxcodegen_generate— runxcodegen generatein a project directorycreate_dmg— create DMG disk images with path traversal and symlink escape protectioncodesign_app— code sign and optionally notarize macOS apps/DMGs (sign -> verify -> notarize -> staple)pfctl_anchor— read-only PF firewall anchor inspection (cannot modify firewall state by construction)Design
createTypedToolpattern (same asdoctor.ts), not session-awarecommand-resultstructured output schema (xcodebuildmcp.output.command-result)src/types/domain-results.ts(+8 lines,CommandResultDomainResultadded to union)Security
pfctl_anchor: hardcoded read-only commands only (-sr,-sa,-n -f); no flush (-F), no load without dry-run; anchor name validated by regex;sudo -nprevents password prompt hangscreate_dmg: scriptPath must be relative, no..segments,realpathSyncpost-resolution check prevents symlink escapescodesign_app: targetPath restricted to.app/.dmg; entitlements restricted to.entitlements; credentials via Keychain profile or env vars only, never CLI argsTest plan
npm run typecheck— 0 errorsnpm run build— successnpm run lint:fix— cleannpm run format— cleannpm run docs:update— TOOLS.md / TOOLS-CLI.md regenerated🤖 Generated with Claude Code