Skip to content

Improve Depot test workflow: diagnostics, filter, timeout #720

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

Merged
lawrencecchen merged 1 commit into from
Mar 1, 2026
Merged

Improve Depot test workflow: diagnostics, filter, timeout #720

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
34 changes: 33 additions & 1 deletion .github/workflows/test-depot.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,14 @@ on:
required: false
default: false
type: boolean
test_filter:
description: "Run specific UI test class (e.g. UpdatePillUITests) or empty for all"
required: false
default: ""
test_timeout:
description: "Per-test timeout in seconds (default: 120)"
required: false
default: "120"
Comment on lines +24 to +27

@coderabbitai coderabbitai Bot Mar 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# Read the relevant sections of the test-depot.yml file
head -n 35 .github/workflows/test-depot.yml

Repository: manaflow-ai/cmux

Length of output: 977

🏁 Script executed:

# Check how TEST_TIMEOUT is used around line 24-27 and later invocations
cat -n .github/workflows/test-depot.yml | sed -n '20,40p'

Repository: manaflow-ai/cmux

Length of output: 802

🏁 Script executed:

# Look at lines around 167 and 183 to see if similar issues exist there
cat -n .github/workflows/test-depot.yml | sed -n '160,190p'

Repository: manaflow-ai/cmux

Length of output: 1135

🏁 Script executed:

# Find all references to test_timeout and TEST_TIMEOUT in the workflow
rg -n 'test_timeout|TEST_TIMEOUT' .github/workflows/test-depot.yml

Repository: manaflow-ai/cmux

Length of output: 219

🏁 Script executed:

# Get the full workflow file to identify all test invocations
wc -l .github/workflows/test-depot.yml

Repository: manaflow-ai/cmux

Length of output: 96

🏁 Script executed:

# Check if there are other test sections beyond what we've seen
cat -n .github/workflows/test-depot.yml | tail -100

Repository: manaflow-ai/cmux

Length of output: 4688

🏁 Script executed:

# Check xcodebuild documentation or search for any error handling patterns
# Let's also verify if there's any validation elsewhere in the codebase
rg -n 'maximum-test-execution-time-allowance|test.timeout|TEST_TIMEOUT' . --type yaml --type sh --type bash | head -20

Repository: manaflow-ai/cmux

Length of output: 88

🏁 Script executed:

# Search for any xcodebuild error handling or validation patterns
rg -n '\[0-9\]|^[0-9]|=~.*[0-9]' .github/workflows/ --type yaml | grep -i timeout

Repository: manaflow-ai/cmux

Length of output: 42

🏁 Script executed:

# Get full context around the UI tests section to understand the error handling
cat -n .github/workflows/test-depot.yml | sed -n '163,185p'

Repository: manaflow-ai/cmux

Length of output: 1085

Add validation for test_timeout before passing to xcodebuild.

test_timeout is a free-form string input with no validation. Non-numeric values pass directly to the -maximum-test-execution-time-allowance xcodebuild flag, causing xcodebuild to fail with a cryptic error message. Validate early in the "Run UI tests" step to fail clearly with an actionable message.

Suggested patch 
      - name: Run UI tests
        if: ${{ !inputs.skip_ui_tests }}
        env:
          TEST_FILTER: ${{ inputs.test_filter }}
          TEST_TIMEOUT: ${{ inputs.test_timeout || '120' }}
        run: |
          set -euo pipefail
          SOURCE_PACKAGES_DIR="$PWD/.ci-source-packages"

+         if ! [[ "$TEST_TIMEOUT" =~ ^[0-9]+$ ]] || [ "$TEST_TIMEOUT" -le 0 ]; then
+           echo "Invalid test_timeout: '$TEST_TIMEOUT'. Provide a positive integer (seconds)." >&2
+           exit 1
+         fi
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/test-depot.yml around lines 24 - 27, Validate the workflow
input `test_timeout` before it's passed to xcodebuild: in the "Run UI tests"
step add a check that `test_timeout` is a positive integer (or empty) and if it
isn't, fail the step with a clear error message explaining that `test_timeout`
must be a numeric value in seconds; only pass
`-maximum-test-execution-time-allowance` to xcodebuild when validation succeeds
and include the validated numeric value, otherwise omit the flag to avoid
xcodebuild cryptic failures.


jobs:
tests:
Expand Down Expand Up @@ -74,6 +82,18 @@ jobs:
rm GhosttyKit.xcframework.tar.gz
test -d GhosttyKit.xcframework

- name: Display diagnostics
run: |
echo "=== Screen resolution ==="
system_profiler SPDisplaysDataType 2>/dev/null || echo "No display info available"
echo ""
echo "=== Window server ==="
/usr/sbin/system_profiler SPSoftwareDataType | grep -i "boot mode" || true
echo ""
echo "=== Accessibility permissions ==="
tccutil reset Accessibility 2>/dev/null || true

@chatgpt-codex-connector chatgpt-codex-connector Bot Mar 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Remove accessibility reset from diagnostics step

In test-depot.yml, the diagnostics block calls tccutil reset Accessibility, which mutates runner state instead of just collecting diagnostics; on Depot hosts that rely on pre-granted Accessibility access for UI automation, this revokes permission immediately before the Run UI tests step and can cause event-synthesis failures or hangs. Because this runs unconditionally for every dispatch, it can introduce flaky or consistently failing UI test runs.

Useful? React with 👍 / 👎.

echo "Accessibility reset attempted"
Comment on lines +85 to +95

@coderabbitai coderabbitai Bot Mar 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n .github/workflows/test-depot.yml | head -100 | tail -20

Repository: manaflow-ai/cmux

Length of output: 951

🏁 Script executed:

# Get the exact section around lines 85-95
sed -n '75,100p' .github/workflows/test-depot.yml

Repository: manaflow-ai/cmux

Length of output: 1059

🏁 Script executed:

# Check if there are other steps in the same job that might use UI automation
rg -n "ui|UI|accessibility|Accessibility|automation|screenshot|xcode" .github/workflows/test-depot.yml -i

Repository: manaflow-ai/cmux

Length of output: 1927

Keep diagnostics read-only; remove Accessibility reset.

The Display diagnostics step runs tccutil reset Accessibility (line 94), which mutates runner state by clearing accessibility permissions in the TCC database. The subsequent Run UI tests step requires those permissions for XCUITest framework to interact with UI elements. Resetting accessibility before UI tests invalidates the permissions needed for test automation.

Suggested patch 
       - name: Display diagnostics
         run: |
           echo "=== Screen resolution ==="
           system_profiler SPDisplaysDataType 2>/dev/null || echo "No display info available"
           echo ""
           echo "=== Window server ==="
           /usr/sbin/system_profiler SPSoftwareDataType | grep -i "boot mode" || true
           echo ""
-          echo "=== Accessibility permissions ==="
-          tccutil reset Accessibility 2>/dev/null || true
-          echo "Accessibility reset attempted"
+          echo "=== Accessibility diagnostics ==="
+          echo "Skipping TCC reset in diagnostics step to avoid mutating test environment"
📝 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.

Suggested change
- name: Display diagnostics
run: |
echo "=== Screen resolution ==="
system_profiler SPDisplaysDataType 2>/dev/null || echo "No display info available"
echo ""
echo "=== Window server ==="
/usr/sbin/system_profiler SPSoftwareDataType | grep -i "boot mode" || true
echo ""
echo "=== Accessibility permissions ==="
tccutil reset Accessibility 2>/dev/null || true
echo "Accessibility reset attempted"
- name: Display diagnostics
run: |
echo "=== Screen resolution ==="
system_profiler SPDisplaysDataType 2>/dev/null || echo "No display info available"
echo ""
echo "=== Window server ==="
/usr/sbin/system_profiler SPSoftwareDataType | grep -i "boot mode" || true
echo ""
echo "=== Accessibility diagnostics ==="
echo "Skipping TCC reset in diagnostics step to avoid mutating test environment"
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/test-depot.yml around lines 85 - 95, The "Display
diagnostics" workflow step mutates runner state by calling tccutil reset
Accessibility; remove the mutation to keep diagnostics read-only by deleting or
commenting out the tccutil reset Accessibility (and the "Accessibility reset
attempted" echo) in the "Display diagnostics" step so only non-mutating commands
like system_profiler and greps remain, ensuring XCUITest accessibility
permissions are preserved for the subsequent "Run UI tests" step.


- name: Clean DerivedData
run: |
rm -rf ~/Library/Developer/Xcode/DerivedData/GhosttyTabs-*
Expand Down Expand Up @@ -142,11 +162,23 @@ jobs:

- name: Run UI tests
if: ${{ !inputs.skip_ui_tests }}
env:
TEST_FILTER: ${{ inputs.test_filter }}
TEST_TIMEOUT: ${{ inputs.test_timeout || '120' }}
run: |
set -euo pipefail
SOURCE_PACKAGES_DIR="$PWD/.ci-source-packages"

# Build the -only-testing argument
if [ -n "$TEST_FILTER" ]; then
ONLY_TESTING="-only-testing:cmuxUITests/$TEST_FILTER"
else
ONLY_TESTING="-only-testing:cmuxUITests"
fi

xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux -configuration Debug \
-clonedSourcePackagesDirPath "$SOURCE_PACKAGES_DIR" \
-disableAutomaticPackageResolution \
-destination "platform=macOS" \
-only-testing:cmuxUITests test
-maximum-test-execution-time-allowance "$TEST_TIMEOUT" \
$ONLY_TESTING test
Comment on lines +173 to +184

@coderabbitai coderabbitai Bot Mar 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

cat -n .github/workflows/test-depot.yml | sed -n '170,190p'

Repository: manaflow-ai/cmux

Length of output: 803

🏁 Script executed:

rg -n 'TEST_FILTER' .github/workflows/test-depot.yml

Repository: manaflow-ai/cmux

Length of output: 227

🏁 Script executed:

head -100 .github/workflows/test-depot.yml | rg -n 'inputs|test_filter|on:'

Repository: manaflow-ai/cmux

Length of output: 528

🏁 Script executed:

head -30 .github/workflows/test-depot.yml

Repository: manaflow-ai/cmux

Length of output: 820

Quote ONLY_TESTING when passing it to xcodebuild.

The test_filter input on line 20 accepts arbitrary user-provided text with no validation. Line 184 expands $ONLY_TESTING unquoted, which contains this user input via line 174. If test_filter contains spaces or shell metacharacters, unquoted expansion will cause word splitting and produce malformed CLI arguments.

Suggested patch 
          xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux -configuration Debug \
            -clonedSourcePackagesDirPath "$SOURCE_PACKAGES_DIR" \
            -disableAutomaticPackageResolution \
            -destination "platform=macOS" \
            -maximum-test-execution-time-allowance "$TEST_TIMEOUT" \
-            $ONLY_TESTING test
+            "$ONLY_TESTING" test
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/test-depot.yml around lines 173 - 184, The xcodebuild
command currently expands $ONLY_TESTING unquoted which allows user-supplied
TEST_FILTER to cause word-splitting or injection; update the xcodebuild
invocation to quote the variable (use "$ONLY_TESTING") so the entire value is
passed as a single argument, keeping the existing logic that sets ONLY_TESTING
from TEST_FILTER intact and preventing malformed CLI args when TEST_FILTER
contains spaces or metacharacters.

Loading