Isolate app-host tests from runner user config - #9716
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCI now creates run-scoped app-host homes, records process receipts, forwards isolation variables through console and Xcode wrappers, validates configuration evidence, and performs authenticated cleanup with workflow and integration guards. ChangesApp-host CI isolation
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant Preparation
participant ConsoleSession
participant Xcodebuild
participant AppHost
participant Cleanup
CI->>Preparation: create run-scoped isolated home
Preparation-->>CI: publish isolation paths and confirmation
CI->>ConsoleSession: launch isolated command
ConsoleSession->>Xcodebuild: forward TEST_RUNNER isolation settings
Xcodebuild->>AppHost: start app-host process
AppHost->>AppHost: write process receipt
CI->>Cleanup: invoke cleanup after tests
Cleanup->>AppHost: authenticate and terminate verified processes
Cleanup-->>CI: remove isolated home and confirmation artifacts
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (5 errors, 1 warning, 1 inconclusive)
✅ Passed checks (18 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/test_ci_app_host_home_isolation.py`:
- Around line 20-37: Update the test assertions around the requirements loop to
parse WORKFLOW as YAML and scope validation to the
jobs.app-host-unit-tests.steps and workflow-guard-tests.steps structures. Verify
the home-preparation command and related environment settings within
app-host-unit-tests, and verify the guard invocation within
workflow-guard-tests, rather than searching the complete workflow text.
🪄 Autofix
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 Plus
Run ID: 98e6889c-1b36-4a0f-976e-07cbf7c249ac
📒 Files selected for processing (3)
.github/workflows/ci.ymlscripts/ci/run-in-console-session.shtests/test_ci_app_host_home_isolation.py
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/test_ci_app_host_home_isolation.py`:
- Around line 22-41: Update require_step to validate that WORKFLOW is a mapping
before calling .get(), and validate each step is a mapping before accessing its
name. Raise contextual SystemExit messages beginning with “FAIL:” for malformed
top-level workflows or steps, while preserving the existing job, steps-list, and
exactly-one-match checks.
🪄 Autofix
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 Plus
Run ID: da0de0dd-42aa-45ba-b845-296e507530e8
📒 Files selected for processing (1)
tests/test_ci_app_host_home_isolation.py
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_ci_app_host_home_isolation.py (1)
81-81: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not make app-host home writable by all local users.
chmod -R a+rwX "$APP_HOST_HOME"grants every local account write access to isolated CI app-host state. Keep access limited to the runner account by adjusting ownership/group permissions or another narrow access control. Update both the guard and the workflow setup together.🤖 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 `@tests/test_ci_app_host_home_isolation.py` at line 81, Replace the broad permissions in the app-host home setup and its corresponding guard in the test with runner-only ownership or group access. Update both the workflow setup and the expected command in tests/test_ci_app_host_home_isolation.py together, ensuring other local users cannot write to APP_HOST_HOME.
🤖 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 `@scripts/ci/run-app-host-xcodebuild.sh`:
- Line 40: Sanitize app-host isolation failures: in
scripts/ci/run-app-host-xcodebuild.sh lines 40-40, replace the
environment-variable-specific output with a generic product-level failure and
safe recovery guidance; at lines 83-84, remove the vendor name and raw echo
"$line", retaining raw details only in sanitized internal diagnostics. Update
tests/test_ci_app_host_xcodebuild_retry.sh lines 113-118 to assert the new
sanitized message.
- Around line 88-91: The log-scanning loop in the script must fail closed when
the attempt log cannot be read. Validate that log_path is readable before
scanning, and handle grep’s status explicitly: allow status 1 for no matches,
but propagate statuses greater than 1 as scan failures instead of masking them
with || true.
- Line 81: Update the path-prefix case pattern in the configuration-root
validation to require a trailing slash after expected_root, matching only
path=$expected_root/ as a literal prefix and rejecting sibling roots such as
app-host-home-other.
In `@tests/test_ci_app_host_xcodebuild_retry.sh`:
- Line 23: Replace the fixed sleep 10 delay in the mock with a completion-event
wait that remains blocked until the wrapper terminates it. Update the mock in
the retry test to use the existing termination/completion signaling mechanism,
preserving the timeout scenario without relying on wall-clock duration.
---
Outside diff comments:
In `@tests/test_ci_app_host_home_isolation.py`:
- Line 81: Replace the broad permissions in the app-host home setup and its
corresponding guard in the test with runner-only ownership or group access.
Update both the workflow setup and the expected command in
tests/test_ci_app_host_home_isolation.py together, ensuring other local users
cannot write to APP_HOST_HOME.
🪄 Autofix
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 Plus
Run ID: d1e973cc-b368-4563-9700-fa192a9c8461
📒 Files selected for processing (5)
cmux.xcodeproj/project.pbxprojcmux.xcodeproj/xcshareddata/xcschemes/cmux-unit.xcschemescripts/ci/run-app-host-xcodebuild.shtests/test_ci_app_host_home_isolation.pytests/test_ci_app_host_xcodebuild_retry.sh
Contributor
Author
|
@coderabbitai review |
|
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
tests/test_ci_app_host_xcodebuild_retry.sh (1)
31-54: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftRemove the real wall-clock dependency from this test.
The mock uses
sleep 10, and the assertion depends on a real0.1second idle timeout. Inject a controllable timeout source into the noninteractive wrapper, then advance it from the test.As per coding guidelines, “Test code must avoid real wall-clock dependencies.”
🤖 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 `@tests/test_ci_app_host_xcodebuild_retry.sh` around lines 31 - 54, Remove the real-time dependency from the retry test’s mock and invocation: update the noninteractive wrapper’s timeout source to be injectable, then control or advance that source within the test to trigger the idle-timeout behavior deterministically. Replace the mock’s sleep-based delay and CMUX_XCODEBUILD_NONINTERACTIVE_IDLE_TIMEOUT_SECONDS=0.1 usage while preserving the existing retry assertions.Source: Coding guidelines
scripts/ci/run-app-host-xcodebuild.sh (1)
40-41: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove implementation details from app-host failure output.
The output exposes internal environment-variable names, the Ghostty provider name, and a raw configuration log line. Emit a generic isolation failure with safe recovery guidance. Update the expected test text.
scripts/ci/run-app-host-xcodebuild.sh#L40-L41: replace the variable-specific failure text.scripts/ci/run-app-host-xcodebuild.sh#L102-L107: remove the provider name andecho "$line".tests/test_ci_app_host_xcodebuild_retry.sh#L129-L131: assert the sanitized outside-root failure.tests/test_ci_app_host_xcodebuild_retry.sh#L137-L170: assert the sanitized missing-log failure.🤖 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 `@scripts/ci/run-app-host-xcodebuild.sh` around lines 40 - 41, Sanitize app-host isolation failure output: in scripts/ci/run-app-host-xcodebuild.sh lines 40-41, replace the environment-variable-specific message with generic isolation failure and safe recovery guidance; in lines 102-107, remove the provider name and raw echo "$line" output. Update tests/test_ci_app_host_xcodebuild_retry.sh lines 129-131 and 137-170 to assert the sanitized outside-root and missing-log messages.Source: Coding guidelines
🤖 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 `@scripts/ci/run-in-console-session.sh`:
- Around line 37-66: Bind XDG_CONFIG_HOME to the isolated app-host configuration
directory: in scripts/ci/run-in-console-session.sh lines 37-66, resolve and
require it to equal $CFFIXED_USER_HOME/.config; in
scripts/ci/run-app-host-xcodebuild.sh lines 39-52, reject any different XDG root
before forwarding TEST_RUNNER_*; in tests/test_ci_app_host_xcodebuild_retry.sh
lines 112-171, add an external-XDG case asserting the wrapper fails before
launching xcodebuild.
In `@tests/test_ci_app_host_home_isolation.py`:
- Around line 124-134: Update the key lookup in the validation loop to search
for EnvironmentVariable entries using the TEST_RUNNER_ prefix, matching the
variables reported by the failure message and preventing scheme overrides of the
wrapper values.
In `@tests/test_ci_app_host_xcodebuild_retry.sh`:
- Around line 88-96: Update the test around the xcodebuild invocation to capture
the inherited HOME value before running the wrapper, then pass it into the awk
validation and require the logged original HOME field ($1) to equal that value.
Retain the existing checks rejecting app-host-specific redirects in $2 and $3.
---
Duplicate comments:
In `@scripts/ci/run-app-host-xcodebuild.sh`:
- Around line 40-41: Sanitize app-host isolation failure output: in
scripts/ci/run-app-host-xcodebuild.sh lines 40-41, replace the
environment-variable-specific message with generic isolation failure and safe
recovery guidance; in lines 102-107, remove the provider name and raw echo
"$line" output. Update tests/test_ci_app_host_xcodebuild_retry.sh lines 129-131
and 137-170 to assert the sanitized outside-root and missing-log messages.
In `@tests/test_ci_app_host_xcodebuild_retry.sh`:
- Around line 31-54: Remove the real-time dependency from the retry test’s mock
and invocation: update the noninteractive wrapper’s timeout source to be
injectable, then control or advance that source within the test to trigger the
idle-timeout behavior deterministically. Replace the mock’s sleep-based delay
and CMUX_XCODEBUILD_NONINTERACTIVE_IDLE_TIMEOUT_SECONDS=0.1 usage while
preserving the existing retry assertions.
🪄 Autofix
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 Plus
Run ID: 64ba7573-6797-443b-9824-02dfff2ab750
📒 Files selected for processing (5)
.github/workflows/ci.ymlscripts/ci/run-app-host-xcodebuild.shscripts/ci/run-in-console-session.shtests/test_ci_app_host_home_isolation.pytests/test_ci_app_host_xcodebuild_retry.sh
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4a290ce. Configure here.
5 of 6 tasks
This was referenced Aug 7, 2026
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
1 participant
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.
Summary
awaccess mode to agree before signalingRegression proof
4a290ce7a4adds failing repository-identity, immediate-recovery, and toolchain-home behavior tests;276101a2cbimplements the machine-ownership boundary82b5c67f12rejects untrusted newest confirmations;e2522e4979authenticates the global newest-scope scancb159ca708models lsof access fields;c978816822requires a separateawrecord4f94aadf9arequires atomic receipt publication;465d819476publishes by same-directory temporary inode and renameaw4a290ce7a4: repository identity collision, configured toolchain homes overwritten, and authenticated prior owner rejectedValidation
python3 tests/test_ci_app_host_home_isolation.pybash tests/test_ci_app_host_identity.shbash tests/test_ci_app_host_processes.shbash tests/test_ci_app_host_home_cleanup.shbash tests/test_ci_app_host_xcodebuild_retry.shbash tests/test_ci_app_host_xcodebuild_attempts.shgit diff --checkf9, thenaw, then the canonical path276101a2cbReview triage
The policy checker names two existing XCTest files.
CLIGenericHookPersistenceTests.swiftextends its existing hook-persistence behavior suite, andCLINotifyProcessTestSupport.swiftis that suite's shared process helper. Moving only the new assertions to Swift Testing would split one behavior suite, so this PR uses the repository's documented XCTest exception.