Repository navigation
EXPERIMENT: unit tests job on Blacksmith macOS (do not merge) - #6418
lawrencecchen wants to merge 2 commits into
Conversation
Live test-e2e proof shows testmanagerd brokers the control session on Blacksmith managed macOS (XCUITest ran 61s, only failed on foreground app activation which unit key-window tests self-skip). Testing whether the full unit tests job is green on Blacksmith so we can move it off Warp.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe ChangesBlacksmith macOS Runner Migration
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~4 minutes Possibly related issues
Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (21 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis is an explicitly non-mergeable experiment that re-routes the
Confidence Score: 4/5The PR is explicitly marked do not merge and touches only CI workflow files with no production code changes. Both changed files are CI-only. The ci.yml change is a straightforward runner label swap with a clear experimental comment. The only concern is the throwaway diagnostic workflow being committed to source control — if accidentally merged, it leaves a permanently dispatchable orphan workflow. The change is otherwise low-risk. .github/workflows/exp-blacksmith-gui-probe.yml — should be deleted rather than merged into main once the experiment concludes. Important Files Changed
Reviews (1): Last reviewed commit: "EXPERIMENT: run unit tests job on blacks..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| name: exp-blacksmith-gui-probe | ||
|
|
||
| # Throwaway diagnostic: does a Blacksmith managed macOS runner have a GUI / | ||
| # WindowServer / testmanagerd-capable session, or is it headless? Decides | ||
| # whether the XCTest `tests` job could ever run there. Dispatch-only. |
There was a problem hiding this comment.
The file header explicitly calls this a "Throwaway diagnostic" and the PR is tagged "do not merge." Per the source-control-artifacts rule, scratch/temp tooling without a durable product or test-system reason should not enter source control. If this PR is accidentally merged (or cherry-picked later), this orphaned workflow will remain in .github/workflows/ and will be dispatch-runnable by anyone with repository write access indefinitely.
File Used: .github/review-bot-rules/source-control-artifacts.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 256-261: The CI workflow file has been updated to run the test job
on the blacksmith-6vcpu-macos-15 runner, but the documentation in
docs/ci-runners.md still describes this job as intentionally pinned to Warp.
Update the documentation in docs/ci-runners.md to reflect the current runner
configuration for the tests job, either by updating it to reference the new
Blacksmith runner or by explicitly marking this change as a temporary experiment
with an associated date so CI triage guidance remains accurate.
In @.github/workflows/exp-blacksmith-gui-probe.yml:
- Around line 39-40: The exit status captured by $? on line 40 reflects the exit
code of the tail command (head) in the pipeline, not the automationmodetool
command itself. To fix this, explicitly capture the automationmodetool exit
status before piping to head. You can do this by either: (1) storing the command
output in a variable and capturing $? immediately after, then piping that
variable to head for display, or (2) using set -o pipefail to make the pipeline
return the status of the first command (automationmodetool) instead of the last
command (head). Choose the approach that maintains readability while ensuring
the reported exit status reflects the actual automationmodetool execution
result.
- Around line 9-17: The runner input in the workflow is being used directly in
the runs-on field without any validation, which allows arbitrary runner labels
to be specified. Add an allowed values constraint to the runner input definition
to restrict it to only approved runner labels. Modify the runner input
definition to include an allowed list of acceptable values (such as just the
default blacksmith-6vcpu-macos-15 or a curated set of approved runners) to
prevent unintended or sensitive runner scheduling.
- Around line 1-17: The exp-blacksmith-gui-probe.yml workflow lacks explicit
permission declarations, which means it inherits the default broad permissions.
Add a `permissions` block at the top level of the workflow (after the `on:`
section and before the `jobs:` section) with minimal required permissions. Since
this is a read-only diagnostic workflow, set permissions to their most
restrictive values, such as `contents: read` if repository access is needed, or
leave it empty if no permissions are required at all.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b3f7a1a8-a1e9-4709-8a84-d2b91951c432
📒 Files selected for processing (2)
.github/workflows/ci.yml.github/workflows/exp-blacksmith-gui-probe.yml
| # EXPERIMENT (2026-06-19): try the unit tests on Blacksmith managed macOS. | ||
| # A live test-e2e run on blacksmith-6vcpu-macos-15 showed testmanagerd DOES | ||
| # broker the XCTest control session there (the XCUITest ran 61s); it only | ||
| # failed on app foreground activation, which the unit suite's key-window | ||
| # tests already self-skip. Validating whether the full unit job goes green. | ||
| runs-on: blacksmith-6vcpu-macos-15 |
There was a problem hiding this comment.
Sync runner-policy docs with this experiment switch.
Line 261 now runs tests on Blacksmith, but docs/ci-runners.md still documents this job as intentionally pinned to Warp. Please update the doc (or explicitly mark this as temporary in the doc) to keep CI triage guidance accurate.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 256 - 261, The CI workflow file has
been updated to run the test job on the blacksmith-6vcpu-macos-15 runner, but
the documentation in docs/ci-runners.md still describes this job as
intentionally pinned to Warp. Update the documentation in docs/ci-runners.md to
reflect the current runner configuration for the tests job, either by updating
it to reference the new Blacksmith runner or by explicitly marking this change
as a temporary experiment with an associated date so CI triage guidance remains
accurate.
| name: exp-blacksmith-gui-probe | ||
|
|
||
| # Throwaway diagnostic: does a Blacksmith managed macOS runner have a GUI / | ||
| # WindowServer / testmanagerd-capable session, or is it headless? Decides | ||
| # whether the XCTest `tests` job could ever run there. Dispatch-only. | ||
| on: | ||
| workflow_dispatch: | ||
| inputs: | ||
| runner: | ||
| description: macOS runner label to probe | ||
| type: string | ||
| default: blacksmith-6vcpu-macos-15 | ||
|
|
||
| jobs: | ||
| probe: | ||
| runs-on: ${{ inputs.runner }} | ||
| timeout-minutes: 10 |
There was a problem hiding this comment.
Set least-privilege token permissions for this diagnostic workflow.
This workflow is read/diagnostic only, but no permissions block is declared, so it inherits broader defaults. Add explicit minimal permissions to reduce blast radius.
Suggested hardening
name: exp-blacksmith-gui-probe
+permissions:
+ contents: read📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name: exp-blacksmith-gui-probe | |
| # Throwaway diagnostic: does a Blacksmith managed macOS runner have a GUI / | |
| # WindowServer / testmanagerd-capable session, or is it headless? Decides | |
| # whether the XCTest `tests` job could ever run there. Dispatch-only. | |
| on: | |
| workflow_dispatch: | |
| inputs: | |
| runner: | |
| description: macOS runner label to probe | |
| type: string | |
| default: blacksmith-6vcpu-macos-15 | |
| jobs: | |
| probe: | |
| runs-on: ${{ inputs.runner }} | |
| timeout-minutes: 10 | |
| name: exp-blacksmith-gui-probe | |
| permissions: | |
| contents: read | |
| # Throwaway diagnostic: does a Blacksmith managed macOS runner have a GUI / | |
| # WindowServer / testmanagerd-capable session, or is it headless? Decides | |
| # whether the XCTest `tests` job could ever run there. Dispatch-only. | |
| on: | |
| workflow_dispatch: | |
| inputs: | |
| runner: | |
| description: macOS runner label to probe | |
| type: string | |
| default: blacksmith-6vcpu-macos-15 | |
| jobs: | |
| probe: | |
| runs-on: ${{ inputs.runner }} | |
| timeout-minutes: 10 |
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 1-53: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[info] 15-15: workflow or action definition without a name (anonymous-definition): this job
(anonymous-definition)
[warning] 6-12: insufficient job-level concurrency limits (concurrency-limits): workflow is missing concurrency setting
(concurrency-limits)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/exp-blacksmith-gui-probe.yml around lines 1 - 17, The
exp-blacksmith-gui-probe.yml workflow lacks explicit permission declarations,
which means it inherits the default broad permissions. Add a `permissions` block
at the top level of the workflow (after the `on:` section and before the `jobs:`
section) with minimal required permissions. Since this is a read-only diagnostic
workflow, set permissions to their most restrictive values, such as `contents:
read` if repository access is needed, or leave it empty if no permissions are
required at all.
Source: Linters/SAST tools
| runner: | ||
| description: macOS runner label to probe | ||
| type: string | ||
| default: blacksmith-6vcpu-macos-15 | ||
|
|
||
| jobs: | ||
| probe: | ||
| runs-on: ${{ inputs.runner }} | ||
| timeout-minutes: 10 |
There was a problem hiding this comment.
Constrain runner input to an allowlist before using it in runs-on.
Line 16 directly trusts a dispatch input for runner selection. That allows arbitrary label targeting across your self-hosted fleet. Restrict this to explicit choices (or hardcode) to prevent accidental/sensitive runner scheduling.
Suggested hardening
on:
workflow_dispatch:
inputs:
runner:
description: macOS runner label to probe
- type: string
+ type: choice
default: blacksmith-6vcpu-macos-15
+ options:
+ - blacksmith-6vcpu-macos-15
+ - blacksmith-6vcpu-macos-26📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| runner: | |
| description: macOS runner label to probe | |
| type: string | |
| default: blacksmith-6vcpu-macos-15 | |
| jobs: | |
| probe: | |
| runs-on: ${{ inputs.runner }} | |
| timeout-minutes: 10 | |
| runner: | |
| description: macOS runner label to probe | |
| type: choice | |
| default: blacksmith-6vcpu-macos-15 | |
| options: | |
| - blacksmith-6vcpu-macos-15 | |
| - blacksmith-6vcpu-macos-26 | |
| jobs: | |
| probe: | |
| runs-on: ${{ inputs.runner }} | |
| timeout-minutes: 10 |
🧰 Tools
🪛 zizmor (1.25.2)
[info] 15-15: workflow or action definition without a name (anonymous-definition): this job
(anonymous-definition)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/exp-blacksmith-gui-probe.yml around lines 9 - 17, The
runner input in the workflow is being used directly in the runs-on field without
any validation, which allows arbitrary runner labels to be specified. Add an
allowed values constraint to the runner input definition to restrict it to only
approved runner labels. Modify the runner input definition to include an allowed
list of acceptable values (such as just the default blacksmith-6vcpu-macos-15 or
a curated set of approved runners) to prevent unintended or sensitive runner
scheduling.
| sudo -n automationmodetool enable-automationmode-without-authentication 2>&1 | head -5 | ||
| echo "automationmodetool exit: $?" |
There was a problem hiding this comment.
automationmodetool exit is currently reporting head’s status, not the tool’s.
Because Line 39 is piped to head, $? on Line 40 reflects the pipeline tail command. Capture the command status explicitly so probe output is trustworthy.
Suggested fix
- sudo -n automationmodetool enable-automationmode-without-authentication 2>&1 | head -5
- echo "automationmodetool exit: $?"
+ am_output="$(sudo -n automationmodetool enable-automationmode-without-authentication 2>&1)"
+ am_status=$?
+ printf '%s\n' "$am_output" | head -5
+ echo "automationmodetool exit: $am_status"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sudo -n automationmodetool enable-automationmode-without-authentication 2>&1 | head -5 | |
| echo "automationmodetool exit: $?" | |
| am_output="$(sudo -n automationmodetool enable-automationmode-without-authentication 2>&1)" | |
| am_status=$? | |
| printf '%s\n' "$am_output" | head -5 | |
| echo "automationmodetool exit: $am_status" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/exp-blacksmith-gui-probe.yml around lines 39 - 40, The
exit status captured by $? on line 40 reflects the exit code of the tail command
(head) in the pipeline, not the automationmodetool command itself. To fix this,
explicitly capture the automationmodetool exit status before piping to head. You
can do this by either: (1) storing the command output in a variable and
capturing $? immediately after, then piping that variable to head for display,
or (2) using set -o pipefail to make the pipeline return the status of the first
command (automationmodetool) instead of the last command (head). Choose the
approach that maintains readability while ensuring the reported exit status
reflects the actual automationmodetool execution result.
This branch was successfully deployed
1 participant
Throwaway experiment to answer: can the slow
testsjob run on Blacksmith managed macOS instead of Warp?A live
test-e2edispatch onblacksmith-6vcpu-macos-15proved testmanagerd brokers the XCTest control session on Blacksmith (the XCUITest ran for 61s, no 120s control-session timeout). It only failed onFailed to activate application ... (Running Background)— foreground activation, which the unit suite's key-window tests already self-skip.This PR points the
testsjob atblacksmith-6vcpu-macos-15to see if the full unit job goes green. Do not merge; informational only.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Low Risk
Workflow-only changes for a labeled experiment; no app or test code changes, though merging would shift where the required macOS unit gate runs.
Overview
Throwaway CI experiment (not intended to merge): the required macOS
testsjob now runs onblacksmith-6vcpu-macos-15instead ofwarp-macos-15-arm64-6x, after prior self-hosted Austin runners were timing out on testmanagerd control-session setup. Comments document that a live test-e2e run on Blacksmith already brokered XCTest successfully; this change checks whether the full unit job goes green (foreground-activation failures are expected to be less relevant because key-window tests self-skip).Adds
exp-blacksmith-gui-probe, a workflow_dispatch-only diagnostic that prints console user, WindowServer, launchctl GUI domain, automationmodetool, SIP, and display probes on a configurable Blacksmith macOS label—meant to decide if XCTest app-host jobs can run there at all.Reviewed by Cursor Bugbot for commit 50bcd5a. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Experiment: run the unit
testsjob on Blacksmith managed macOS to validate stable XCTest control sessions and reduce dependence on Warp. A priortest-e2eonblacksmith-6vcpu-macos-15showedtestmanagerdworks (61s run), with only foreground activation failures that the unit key-window tests self-skip.Refactors
testsjob in.github/workflows/ci.ymltoblacksmith-6vcpu-macos-15.New Features
exp-blacksmith-gui-probeworkflow to verify GUI/WindowServer,testmanagerdreadiness, automation mode, SIP, and display presence for a given runner label.Written for commit 50bcd5a. Summary will update on new commits.
Summary by CodeRabbit