Repository navigation
Conversation
Collaborator
Generated by 🚫 Danger |
wpmobilebot
reviewed
Oct 28, 2026
wpmobilebot
reviewed
Oct 28, 2026
Contributor
🤖 Claude CI review: ✅ passCommit No proven bugsCommit What I checked:
What I did not check: this PR changes only CI scripts, not the app. I did not build the app, run unit tests or use the simulator. I also did not time a run against the previous setup. Other people's threads: AliSoftware's thread about Mac queue load is still open. It's a policy question for the maintainers. This commit partly addresses it by reusing the package cache. Earlier findings
|
Contributor
🤖 Build Failure AnalysisThis build has failures. Claude has analyzed them - check the build annotations for details. |
wpmobilebot
reviewed
Oct 28, 2026
wpmobilebot
reviewed
Oct 29, 2026
wpmobilebot
reviewed
Oct 29, 2026
wpmobilebot
reviewed
Oct 29, 2026
wpmobilebot
reviewed
Oct 29, 2026
Claude reviews each internal PR on a Mac CI agent with full shell access, so it can build, run tests and use simulators. The step fails only when Claude proves a bug caused by the PR. Claude runs with an allowlisted environment (the Anthropic key and the test site sign-in) and has no GitHub or Buildkite agent token. The script fetches the PR data for it and posts its result as a comment-only review.
Headless Xcode would ask before Claude can use it, and nobody can answer on CI, so the script enables it for every agent on the throwaway VM. Claude falls back to xcodebuild and simctl when that fails.
Claude builds the app itself when it needs to. Also pins the effort level to medium, the current default, so an update can't change it silently.
- Any unexpected failure now exits 2 (retried once), so exit 1 only ever means a fail verdict. Annotating the build is best-effort. - A review thread only counts as Claude's if wpmobilebot started it, not just if it carries the hidden marker, which anyone can type. - sudo is used to set up headless Xcode, then denied for the build user, so Claude doesn't get root. Claude doesn't run if sudo still works.
One comment per PR, updated in place on every run: the verdict, the summary, each new finding in full, and the status of earlier findings. Screenshots become links to the Buildkite build, since GitHub can't show its artifacts.
When a Spacefast key and Space are configured, the script publishes the review's screenshots additively under wordpress-ios/pr-<n>/build-<n>/, checks each URL loads, and embeds them in the PR comments. Otherwise, or if anything fails, it links to the Buildkite build as before. Claude never sees the key.
As the build user, Claude could read the job's secrets: the AWS keys and bot SSH key mounted into every Mac VM, files in the build user's home, and the Buildkite job token in the environment of the build user's processes. The script now creates a non-admin claude-reviewer user with its own group and a copy of the checkout, and runs Claude as that user over SSH to the VM (or with launchctl asuser). Before Claude starts, the reviewer checks that it can't read those files or process environments, signal the build user's processes, or use sudo. If any check fails, Claude doesn't run. It also reports whether simulators and Xcode MCP work for the second user.
The host mounts its AWS keys and the bot's SSH key into every Mac VM, and the mount is readable by every user, so the separate reviewer user could read them. Nothing in this step needs the mount after setup, so unmount it before the reviewer starts. The reviewer's checks still stop Claude if it's there.
The reviewer controls its evidence folder, so a symlink there made root's ditto, and then the artifact upload, copy files the reviewer can't read. The reviewer now archives what it can read, and the build user unpacks it into a fresh folder and deletes anything that isn't a plain file or folder. The step also tells the artifact upload not to follow symlinks. Also enables headless Xcode MCP for the reviewer user: the setting is per user, and mcp-server takes the user from the SUDO_* variables.
mcp-server takes the user from the SUDO_* variables but the settings folder from $HOME, so it tried to write into the build user's home and failed.
Setting HOME and the SUDO_* variables wasn't enough: mcp-server still wrote into the build user's settings. The reviewer now runs the two mcp-server commands itself, through a sudo rule that allows only those commands and is deleted again before Claude starts.
mcp-server enable stores an integrity hash in the user's keychain. The new reviewer user had none, so it failed with -25308 (user interaction is not allowed).
The permission check passed, but the bridge couldn't launch Xcode Service from the reviewer's SSH-only session, so Claude was told to use tools that never loaded. The reviewer script now sends an MCP initialize request and only offers the tools if it gets a reply.
- Drops Xcode MCP: it needs a desktop session, which the separate reviewer user doesn't have. Claude taps around with AXe instead (pinned, checksum verified, installed into the reviewer's home), not WebDriverAgent. - Adds .buildkite/claude/wp-test-site.sh, based on wordpress-rs's native test server (Automattic/wordpress-rs#1665): Homebrew PHP 8.4, MariaDB 11.4 and Apache, one fresh WordPress 7.1.4 site per call on http://localhost:<port>, with an admin user and an application password. The build user installs the Homebrew packages; the reviewer creates and runs the sites. - The reviewer checks AXe and creates one site before Claude starts, and reports both in isolation-checks.txt. - The reviewer no longer gets the shared test site's credentials.
- wp-test-site.sh: a start that fails partway now removes the site's folder, database and database users, so the name can be used again. - The reviewer's AXe check keeps AXe's errors in axe-check.log, retries while the simulator finishes starting, and no longer stops the review if the download fails. - The CI notes give Claude its time budget and the Jetpack build time, so it waits for a build before finishing. - The build failure analysis ignores the review step, which explains its own failures.
AliSoftware
reviewed
Oct 29, 2026
| ## Build and test | ||
|
|
||
| - The XCFrameworks are in place, but Swift packages may resolve again on your first build. App secrets aren't installed: builds use the example credentials, as for external contributors. | ||
| - You have up to about 90 minutes in total. A cold Jetpack build takes about 5 minutes on this VM, and a unit test run about 2 more. When the PR changes how the app behaves, build it and check the change in the simulator. Run builds and tests in the foreground: a single command may take up to an hour. Return your result only after every check you started has finished. |
Contributor
There was a problem hiding this comment.
Given the current state of our Mac queue recently and how it's been struggling to keep up with the demand especially due to more and more repos running very long jobs (and @mokagio recently trying to find workarounds to reduce the demand until we get more capacity)… I fear that doing an experiment that adds a 1h30 job to the queue will have quite a heavy impact our overall Mac CI fleet (across all repos) at this moment… 😓
Is there a particular reason to ask the agent to build the code itself, instead of reusing the Build Product artifacts of other jobs in the pipeline which also already build the app too (and also do it with cache mechanisms in place, like for SwiftPM resolution etc, while Claude won't use those speedup benefits when compiling again on its end)? Can't we instead share the build artifacts to reduce the time used by this Claude job drastically so it only focuses on testing the already-built app?
Anything that could help avoid clogging the queue for everyone else given the current situation, at least until we manage to add more agents and capacity to our Mac fleet 🙏
Contributor
Author
There was a problem hiding this comment.
Is there a particular reason to ask the agent to build the code itself
The agent will almost certainly have to rebuild the code, even if it receives a built binary because a lot of the work is fault injection, or writing tests to reproduce an issue, or injecting fake data/timeouts/???.
The point of this isn't to test the built binary, it's to prove issues in the code. For that it needs to be able to compile it – otherwise it's just assuming based on the code, which isn't very useful.
Contributor
There was a problem hiding this comment.
Right. That still means it will rebuild it from scratch (including SwiftPM dependencies and project framework) instead of relying on local cache of build products (which could help making it an incremental build)
Btw, as a side note, @twstokes noted that we also still need to tighten our security posture inside VMs on some edge cases and recent changes before being ready to let agents roam free in them (AINFRA-3517).
Contributor
Author
There was a problem hiding this comment.
I opened that issue :)
It explains that this PR has a workaround that avoids agents being able to read secrets by unmounting the secrets volume prior to running. In general though, it'd be nice if we didn't need the workaround and this stuff was scoped down to a single user without each project needing to handle it.
Contributor
There was a problem hiding this comment.
I opened that issue :)
Ah, nice, thanks for that 👍
It explains that this PR has a workaround that avoids agents being able to read secrets by unmounting the secrets volume prior to running. In general though, it'd be nice if we didn't need the workaround and this stuff was scoped down to a single user without each project needing to handle it.
Totally!
Each run posted a review that said "verdict: fail" or "pass", and those piled up, so a fixed PR still looked failing. A review can't be deleted once submitted, so new reviews no longer state a verdict, and each run rewrites the first line of the earlier runs' reviews to say they were superseded. Findings and their threads are kept. The summary comment, updated on every run, is the one place with the current verdict.
A GitHub Action on review_requested retries the Claude PR Review step on the PR's latest Buildkite build, using the same reusable workflow that retries Danger when labels change. The step allows retrying after a pass, and the review script removes the request when it finishes, so it can be requested again.
wpmobilebot
reviewed
Oct 29, 2026
wpmobilebot
removed their request for review
October 29, 2026 17:30
The reviewer user's activity is logged from places it can't change: - Its transcript streams back to the build user instead of a file it owns. - Managed-settings hooks send every tool call to a listener run by the build user. A tool call that can't be logged is blocked. - eslogger (as root) records the reviewer's process launches, file opens, changes and deletes. - A pf rule logs the reviewer's outgoing connections, with DNS traffic to name the addresses. A known command run as the reviewer checks that each log records it. The prompt, PR data and versions are uploaded, installer output is kept, and Claude Code and the Spacefast CLI are pinned to checksummed binaries.
The test site's database and web server inherited the file descriptor the reviewer script writes Claude's transcript to. They kept the SSH session open after Claude finished, so the job hung until it timed out. Only Claude's process now gets it.
The transcript only reached the log at the end, so a stuck or slow run looked the same from outside. Each tool call and the final result are now printed while the transcript is saved.
Streaming it back over SSH left the session open after Claude finished (build 35633), so the job hung until it timed out. Claude now appends to a root-owned file with the system append-only flag, which the reviewer can add to but not change or delete. The reviewer's checkout no longer includes the audit folder.
Build 35640 hung while collecting the audit logs. Each logger now gets a SIGINT and, after 10 seconds, a SIGKILL, and the wait for the Endpoint Security log is capped. The log shows how long stopping and summarizing took and how big each file is.
wpmobilebot
reviewed
Oct 30, 2026
eslogger's exec events include each process's whole environment, so the Endpoint Security log carried the reviewer's ANTHROPIC_API_KEY and was uploaded as an artifact (found by the Claude review in build 35643). The environment is now removed before the log is written, and any audit or evidence file that still contains one of the job's secrets is deleted before upload. The audit folder is no longer readable by the reviewer. Unchanged files' close events are dropped, halving the log.
A cancelled or timed-out job lost every audit log, because the VM shuts down before artifacts are uploaded. The logs are now uploaded in parts every two minutes to build/claude-audit-live/, each checked for the job's secrets first. The Endpoint Security log was 266 MB after six minutes, mostly reads of system files, Xcode and the reviewer's own folders. Those are now counted instead of logged; reads anywhere else are still listed. eslogger ignores SIGINT, so it now gets SIGTERM next, and the signal that stopped each logger is recorded.
Lists of paths compress well, and files-read.txt alone was 120 MB. Summaries over 1 MB are gzipped after the checks; the verdict is read from the original transcript.
wpmobilebot
reviewed
Oct 30, 2026
… log Claude was started with env -i ... ANTHROPIC_API_KEY=... claude, so the key was in a command's arguments, visible to ps and recorded by the Endpoint Security log (found by the Claude review in build 35644). Claude now gets an exported, filtered environment instead. The secret check before upload missed matches in large compressed parts: grep -q exits at the first match, gzip gets SIGPIPE, and under pipefail the pipeline failed, which read as no secret. The ES filter also now redacts the job's secrets anywhere in an event.
wpmobilebot
reviewed
Oct 30, 2026
The test sites, the simulator and anything Claude started outlive the reviewer's session, and the reviewer can read other users' command lines. Spacefast's token was then passed as an argument (raised by the Claude review in build 35645). The reviewer's launchd domains are now taken down and its processes killed before posting, and the run stops if any survive. sf reads the token from SPACEFAST_TOKEN instead of --token.
build/audit-log.tar.gz holds every audit result and the log parts uploaded during the run, so an audit needs one download. It's built last, from the EXIT trap, after the posting steps add their logs, and checked for the job's secrets first. The parts are still uploaded during the run, for jobs that are cancelled or time out.
The request was only removed after posting, so a run that exited early left wpmobilebot as a pending reviewer, and asking again couldn't start a new run. It's now cleared from the EXIT trap, and a failure to clear it is logged.
Its first Xcode build otherwise resolves them again, including 6.5 GB of binaries, which took 187 seconds locally. They're now resolved into DerivedData/SourcePackages before the checkout is copied. A copied folder needed no fetching in a build from a different path. The prompt's test command now uses the same DerivedData.
wpmobilebot
reviewed
Oct 30, 2026
This reverts commit b4dec67.
…kers-pr-prototype
wpmobilebot
reviewed
Oct 30, 2026
trunk now caches the packages' binaries, so restore them instead of resolving everything from the network (suggested by the Claude review in build 35673). Resolving stays, to fill in anything the cache is missing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds an experimental
Claude PR ReviewCI check. On every internal PR, Claude reviews the change on a Mac agent: it builds the app, runs tests, creates WordPress test sites and taps through the change in a simulator. The check fails only when Claude proves a bug caused by the PR, with a test or a screenshot. Everything Claude does on the machine is logged to one archive,audit-log.tar.gz.How it works
Runs on every PR that isn't from a fork. It doesn't wait for other steps.
Claude gets this prompt, plus CI notes in
.buildkite/claude/pr-review-ci-context.md:Exit status:
0= no proven bugs,1= a proven bug caused by the PR,2= anything else went wrong and the review didn't complete (retried once).Results go in three places:
Screenshots are published to the
jkmassel-screenshotsSpacefast Space so GitHub can show them. Test output is a Buildkite artifact, and the transcript is inaudit-log.tar.gz.On later runs, Claude reads replies to its own threads, checks the current code, replies, and resolves the thread when the issue is fixed.
claude-opus-5-5at--effort medium.What Claude can use
simctl, and AXe for tapping and reading the accessibility tree. Xcode MCP needs a desktop session, which the reviewer user doesn't have..buildkite/claude/wp-test-site.shcreates a fresh self-hosted WordPress onhttp://localhost:<port>in about 30 s, with an application password. It's based on wordpress-rs's native test server (Add a test server that runs without Docker, for macOS CI Automattic/wordpress-rs#1665). There is no WordPress.com account.xcodebuild. Swift packages are restored from the build jobs' cache before Claude starts, which adds about 1 minute to every run. A cold Jetpack build then takes about 4 minutes, and one unit test run about 3 more.Limits on what Claude can do
It can't approve or request changes. Claude has no GitHub token. The script fetches the PR data for it and posts its result, always as a
COMMENTreview. It only replies to and resolves threads thatwpmobilebotstarted with the review marker.It runs as a separate macOS user. The script creates
claude-reviewer: not an admin, with its own group. It gets a copy of the checkout and only the Anthropic key. Run as the build user, Claude could have read the job's secrets:ps -EThe shared credentials mount is unmounted before the reviewer starts, because every user on the VM can read it (AINFRA-3517).
Checked before Claude runs. As
claude-reviewer, the script checks that it can't read:.aws,.ssh,.configor keychainsIt also checks that it can't signal those processes or use
sudo. If any check fails, Claude doesn't run.Evidence comes back without root. The reviewer archives its evidence, and the build user unpacks it into a fresh folder and keeps only plain files. The artifact upload doesn't follow symlinks.
No app secrets.
a8c-secretsisn't installed, so builds use the example credentials.The reviewer stops before secrets are used. Its processes can outlive its session and it can read other users' command lines, so the script takes down its
launchddomains and kills everything it left running before any step that uses a secret. If anything survives, nothing is posted. The Anthropic key and Spacefast token are never passed as command arguments.Audit log
Everything
claude-reviewerdoes is logged from places it can't change.audit-log.tar.gzholds the results and the raw logs.sappndeslogger, as rootpfrule for the reviewer's user ID, andtcpdumpWhat's in the archive:
claude-audit/:audit-checks.txt,processes.txt,files-read.txt,files-written.txt.gz,network-summary.txt,privileged.txt, the transcript, the hook log, and the inputs (prompt, schema, PR data, versions and script checksums).claude-audit-live/: the raw event, network, hook and transcript logs, in parts.Keeping it usable:
esloggerenvironments are removed, and this job's secrets are redacted from every event. Every file is then checked for those secrets before upload.es-dropped.jsoninstead of listed. Reads anywhere else are listed. This tookfiles-read.txtfrom 120 MB to 30 KB.audit-checks.txt.Claude Code and the Spacefast CLI are pinned to checksummed binaries, and installer output is kept.
Known gaps
wpmobilebotreview request pending, which then has to be removed by hand.audit-checks.txt.Found along the way
esloggerexecevents include each process's whole environment, andesloggerignores SIGINT.BUILDKITE_*variables (hostmgrcopies only those), so a step'senv:settings never arrive. The review's options are command arguments for that reason.mac-ci-vmAWS keys andwpmobilebotSSH key.upload-secrets-to-s3.rbwrote multi-line secrets as heredocs. While a file doesn't parse, its values aren't redacted from build logs. The script now writes them single-quoted (inmobile-secrets), andwordpress-ios/envparses again. 4 Tumblr files still don't, because they contain shell code.Test plan
claude,gh,buildkite-agent,sudoandcurl: pass, fail, inline-comment fallback, no result, unexpected failures (exit 2), sudo that won't go away, fake review markers, the environment allowlist, and hostile evidence archivesclaude-revieweron a real VM (builds 35450 onwards)--validate-setuppasses end to end on a real VM (builds 35517 and 35521):trunk.CustomPostTabTests, made a test site with XML-RPC off, and checked the new sort order in the simulator, with a screenshot on Spacefast.audit-log.tar.gzuploaded (build 35650)--validate-setupwith the packages resolved up front: Jetpack build 240 s, signed in by tapping with AXe, unit test 168 s (build 35668)gh, because it needs a job to fail on CI. The normal path passed in build 35662.