dispatch-e2e validation + agent time discipline rules - #10024
dispatch-e2e validation + agent time discipline rules#10024azooz2003-bit wants to merge 7 commits into
Conversation
scripts/dispatch-e2e.sh validates each test_filter item against local test sources (class under cmuxUITests/ or cmuxTests/, method in that class's files) before dispatching test-e2e.yml, exiting 2 with nearest candidates on a miss so a typo no longer costs a full dispatch+watch cycle. Supports comma-separated filters (one run each), --dry-run, --watch, and passthrough for runner, record_video, test_timeout, and job_timeout. CLAUDE.md documents the wrapper as the E2E entrypoint (raw gh command as fallback) and adds an agent time discipline section: one blocking command per wait, one build dispatch per need, batched fixes per tagged rebuild, and probe-once-then-queue for a locked or offline iPhone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughThe change adds a validated hosted E2E dispatcher. It validates Swift test filters, supports dry runs and run monitoring, and updates testing and agent guidance for hosted verification. ChangesHosted E2E dispatch
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: 🟡 Moderate · up to The new E2E dispatcher can misread XCTest methods when comments, strings, or nested functions contain similar syntax, allowing invalid filters through or rejecting valid ones and potentially causing wasted hosted runs. The documented one-dispatch limit also remains inconsistent with the implementation, so these issues should be fixed or explicitly accepted before merge. Sequence Diagram(s)sequenceDiagram
participant Engineer
participant dispatch_e2e_sh
participant GitHubActions
Engineer->>dispatch_e2e_sh: Provide validated E2E filters and options
dispatch_e2e_sh->>GitHubActions: Dispatch one workflow per filter
GitHubActions-->>dispatch_e2e_sh: Return workflow run information
dispatch_e2e_sh->>GitHubActions: Watch runs when requested
GitHubActions-->>Engineer: Report run URLs and failures
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors)
✅ Passed checks (23 passed)
✨ Finishing Touches📝 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@CLAUDE.md`:
- Around line 65-74: Update the wrapper guidance in CLAUDE.md lines 65-74 and
beside the wrapper command in
skills/cmux-testing/references/local-vs-ci-validation.md line 20 to state that
the command must run from a worktree checked out at the same branch or SHA
passed via --ref; make no other changes.
In `@scripts/dispatch-e2e.sh`:
- Around line 272-288: Replace the displayTitle-based matching in resolve_run_id
with an authoritative dispatch-to-run correlation mechanism. Do not select runs
by title prefix or snapshot ordering; when no reliable run ID is returned, fail
closed and require an explicit run ID for --watch, ensuring the script never
watches an unrelated run.
- Around line 305-307: Update the dispatch validation flow around validate_item
and the CLEAN_ITEMS loop to build a per-target class and method index from
class_files once before iterating filters. Pass or otherwise reuse that index
for each validate_item call, replacing repeated recursive scans while preserving
the existing validation results and filtering behavior.
- Around line 275-291: Replace the fixed 15-attempt loop around the `gh run
list` lookup with an explicit completion or correlation signal that confirms the
target workflow run is available. Remove the two-second `sleep` and avoid
retrying `gh run list` on a wall-clock schedule; preserve filtering by `pre_ids`
and the `"$filter on "*` title match when resolving the run.
- Around line 207-239: Update the selector validation around method discovery to
build a class-qualified index of test methods, including class extensions,
rather than matching any func in a file containing the class. Ensure only test
methods belonging to $cls validate, while helper and lifecycle methods are
rejected; use this same index for candidate suggestions. Add a regression
fixture covering multiple classes or a helper method in one file.
🪄 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: 5f297077-809d-4b77-af01-dec70d5f587f
📒 Files selected for processing (3)
CLAUDE.mdscripts/dispatch-e2e.shskills/cmux-testing/references/local-vs-ci-validation.md
| Dispatch hosted UI/E2E runs through the validated wrapper. It checks every filter item against the local test sources and refuses to dispatch a filter that matches zero tests, which otherwise wastes a full dispatch+watch cycle before the run fails: | ||
|
|
||
| ```bash | ||
| ./scripts/dispatch-e2e.sh --ref <branch-or-sha> --filter "<Class or Class/method>[,more]" --watch | ||
| ``` | ||
|
|
||
| `--dry-run` validates and prints the `gh` command without dispatching; `--runner`, `--record-video`, `--timeout` (per-test seconds), and `--job-timeout` (minutes) pass through. Raw fallback: | ||
|
|
||
| ```bash | ||
| gh workflow run test-e2e.yml --repo manaflow-ai/cmux -f ref=<branch-or-sha> -f test_filter="<Class or Class/method>" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
State that the checkout must match the dispatched ref.
The wrapper validates sources from the current checkout, not from --ref. If these differ, validation can reject a selector that exists in the dispatched ref or approve one that does not.
CLAUDE.md#L65-L74: State that the command must run from a worktree checked out at<branch-or-sha>.skills/cmux-testing/references/local-vs-ci-validation.md#L20-L20: Add the same checkout/ref precondition beside the wrapper command.
📍 Affects 2 files
CLAUDE.md#L65-L74(this comment)skills/cmux-testing/references/local-vs-ci-validation.md#L20-L20
🤖 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 `@CLAUDE.md` around lines 65 - 74, Update the wrapper guidance in CLAUDE.md
lines 65-74 and beside the wrapper command in
skills/cmux-testing/references/local-vs-ci-validation.md line 20 to state that
the command must run from a worktree checked out at the same branch or SHA
passed via --ref; make no other changes.
| for item in "${CLEAN_ITEMS[@]}"; do | ||
| validate_item "$item" | ||
| done |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Build the test-source index once for the filter batch.
Each validate_item call performs recursive target scans through class_files. For F filters and S Swift files, this is O(F × S) file scanning. A batch of 100 filters across 1,000 test files can cause about 100,000 file inspections.
Build one per-target class and method index before this loop. Query that index for each filter. As per coding guidelines, avoid repeated batch rescans over scalable collections. As per path instructions, apply .github/review-bot-rules/algorithmic-complexity.md.
🤖 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/dispatch-e2e.sh` around lines 305 - 307, Update the dispatch
validation flow around validate_item and the CLEAN_ITEMS loop to build a
per-target class and method index from class_files once before iterating
filters. Pass or otherwise reuse that index for each validate_item call,
replacing repeated recursive scans while preserving the existing validation
results and filtering behavior.
Sources: Coding guidelines, Path instructions
The org removed required status checks from manaflow-ai/cmux; all PR checks are advisory review bots (verified: zero required_status_checks rules on main). The first-pass paragraph now says merge validation happens via the merge gate, not by watching PR checks. Hosted E2E dispatch gains a once-per-dogfood-round cap and a no dispatch-fix-redispatch rule (root-cause red runs locally or on a fleet simulator; only a new XCUITest needs one green hosted run), mirrored in the cmux-testing skill reference and as an agent time discipline rule. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@CLAUDE.md`:
- Line 77: Update the dispatch-cap guidance near “At most one dispatch per
dogfood round” to define the limit in terms of hosted runs, and explicitly
require a single comma-separated filter item for this workflow. Ensure the
wording accounts for scripts/dispatch-e2e.sh creating one hosted run per filter
item.
In `@skills/cmux-testing/references/local-vs-ci-validation.md`:
- Line 20: Update the raw GitHub Actions fallback command in the local-vs-ci
validation guidance to include both the target branch/ref and the selected
test_filter, matching the complete invocation documented in CLAUDE.md. Keep the
existing dispatch and filter guidance unchanged.
🪄 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: cbbc7d24-8c5a-4230-9182-3aa1f8ece946
📒 Files selected for processing (2)
CLAUDE.mdskills/cmux-testing/references/local-vs-ci-validation.md
| gh workflow run test-e2e.yml --repo manaflow-ai/cmux -f ref=<branch-or-sha> -f test_filter="<Class or Class/method>" | ||
| ``` | ||
|
|
||
| At most one dispatch per dogfood round. A red run means root-cause locally or on a fleet simulator first; never loop dispatch-fix-redispatch. A new XCUITest needs one green hosted run before the task is done; nothing else does. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Define the dispatch cap in terms of hosted runs.
scripts/dispatch-e2e.sh dispatches one hosted run for each comma-separated filter item. Therefore, --filter "ClassA,ClassB" creates two hosted runs although Line 77 says “At most one dispatch per dogfood round.” If the limit is one hosted run, reject multiple filter items for this workflow or document that only one filter item is allowed per round.
Proposed wording
-At most one dispatch per dogfood round.
+Use at most one filter item per dogfood round. The wrapper dispatches one hosted run per comma-separated filter item.📝 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.
| At most one dispatch per dogfood round. A red run means root-cause locally or on a fleet simulator first; never loop dispatch-fix-redispatch. A new XCUITest needs one green hosted run before the task is done; nothing else does. | |
| Use at most one filter item per dogfood round. The wrapper dispatches one hosted run per comma-separated filter item. A red run means root-cause locally or on a fleet simulator first; never loop dispatch-fix-redispatch. A new XCUITest needs one green hosted run before the task is done; nothing else does. |
🧰 Tools
🪛 LanguageTool
[grammar] ~77-~77: Ensure spelling is correct
Context: .../method>" ``` At most one dispatch per dogfood round. A red run means root-cause local...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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 `@CLAUDE.md` at line 77, Update the dispatch-cap guidance near “At most one
dispatch per dogfood round” to define the limit in terms of hosted runs, and
explicitly require a single comma-separated filter item for this workflow.
Ensure the wording accounts for scripts/dispatch-e2e.sh creating one hosted run
per filter item.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…un dispatch cap, executable raw fallback) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ved runs Method filters now validate inside class/extension blocks of the target class (brace-depth awk scope), so a same-file helper or sibling class cannot satisfy a selector XCTest would resolve to zero tests. --watch exits nonzero when a dispatched run id could not be resolved instead of silently omitting it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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 `@scripts/dispatch-e2e.sh`:
- Around line 141-160: Replace the raw awk brace-counting and grep logic in
class_scoped_lines and the XCTest indexing block with token-aware parsing that
ignores comments and string literals, tracks the target class body, and matches
only direct parameterless func test*() declarations. Ensure nested functions and
sibling classes are excluded, including when braces appear in comments or
strings, and add fixtures covering nested functions, comments, and
brace-containing strings.
🪄 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: 515a0e0f-9662-4493-999b-e98eb60342f8
📒 Files selected for processing (1)
scripts/dispatch-e2e.sh
| awk -v cls="$cls" ' | ||
| FNR == 1 { inside = 0; depth = 0; seen_open = 0 } | ||
| { | ||
| if (!inside) { | ||
| if ($0 ~ ("(^|[^A-Za-z0-9_])(class|extension)[[:space:]]+" cls "([^A-Za-z0-9_]|$)")) { | ||
| inside = 1; depth = 0; seen_open = 0 | ||
| } else { | ||
| next | ||
| } | ||
| } | ||
| line = $0 | ||
| o = gsub(/{/, "", line) | ||
| c = gsub(/}/, "", line) | ||
| depth += o - c | ||
| if (o > 0) seen_open = 1 | ||
| if (seen_open && depth <= 0) inside = 0 | ||
| } | ||
| ' "$@" | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Audit test-method declarations and their surrounding source before adding
# regression fixtures for nested functions and brace-containing strings.
fd -e swift . cmuxUITests cmuxTests -x \
rg -n -C 5 'func[[:space:]]+test[A-Za-z0-9_]*[[:space:]]*\(\)'Repository: manaflow-ai/cmux
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- scripts/dispatch-e2e.sh: relevant definitions and callers ---'
sed -n '100,175p;230,285p;340,420p' scripts/dispatch-e2e.sh
printf '%s\n' '--- selector-related references ---'
rg -n -C 4 'class_scoped_lines|test[A-Za-z0-9_]*|selector|awk' scripts/dispatch-e2e.sh
printf '%s\n' '--- Swift test declarations in likely test roots, without context expansion ---'
for root in cmuxUITests cmuxTests Packages; do
if [ -d "$root" ]; then
rg -n --glob '*.swift' '(^|[[:space:]])func[[:space:]]+test[A-Za-z0-9_]*[[:space:]]*\(' "$root" | head -n 120
fi
doneRepository: manaflow-ai/cmux
Length of output: 49876
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat >"$tmpdir/fixture.swift" <<'SWIFT'
final class OuterTests: XCTestCase {
func testDirect() {}
func helper() {
func testNested() {}
let text = "a { brace }"
}
}
extension OuterTests {
func testExtensionDirect() {}
}
final class NextTests: XCTestCase {
func testAdjacent() {}
}
SWIFT
# Extract the exact awk block from the reviewed script and apply it to the fixture.
awk '
/awk -v cls="\$cls"/ { capture = 1 }
capture { print }
capture && /^ '\'' "\$@"/ { exit }
' scripts/dispatch-e2e.sh >"$tmpdir/extractor.awk"
printf '%s\n' '--- extracted awk implementation ---'
cat "$tmpdir/extractor.awk"
printf '%s\n' '--- class-scoped output for OuterTests ---'
awk -v cls='OuterTests' '
FNR == 1 { inside = 0; depth = 0; seen_open = 0 }
{
if (!inside) {
if ($0 ~ ("(^|[^A-Za-z0-9_])(class|extension)[[:space:]]+" cls "([^A-Za-z0-9_]|$)")) {
inside = 1; depth = 0; seen_open = 0
} else {
next
}
}
print
line = $0
o = gsub(/{/, "", line)
c = gsub(/}/, "", line)
depth += o - c
if (o > 0) seen_open = 1
if (seen_open && depth <= 0) inside = 0
}
' "$tmpdir/fixture.swift"
printf '%s\n' '--- direct declaration candidates from extracted output ---'
awk -v cls='OuterTests' '
FNR == 1 { inside = 0; depth = 0; seen_open = 0 }
{
if (!inside) {
if ($0 ~ ("(^|[^A-Za-z0-9_])(class|extension)[[:space:]]+" cls "([^A-Za-z0-9_]|$)")) {
inside = 1; depth = 0; seen_open = 0
} else {
next
}
}
if ($0 ~ /func[[:space:]]+test[A-Za-z0-9_]*/) print FNR ":" $0
line = $0
o = gsub(/{/, "", line)
c = gsub(/}/, "", line)
depth += o - c
if (o > 0) seen_open = 1
if (seen_open && depth <= 0) inside = 0
}
' "$tmpdir/fixture.swift"Repository: manaflow-ai/cmux
Length of output: 1096
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat >"$tmpdir/fixture.swift" <<'SWIFT'
final class EarlyEndTests: XCTestCase {
let text = "}"
func testMissedAfterString() {}
}
final class LateEndTests: XCTestCase {
let text = "{"
func testIncludedBeforeSibling() {}
}
final class SiblingTests: XCTestCase {
func testSibling() {}
}
final class CommentEndTests: XCTestCase {
// }
func testMissedAfterComment() {}
}
final class FalsePositiveTests: XCTestCase {
// func testCommentedOut() {}
let text = "func testQuoted() {}"
func helper() {
func testNested() {}
}
}
SWIFT
extract='
FNR == 1 { inside = 0; depth = 0; seen_open = 0 }
{
if (!inside) {
if ($0 ~ ("(^|[^A-Za-z0-9_])(class|extension)[[:space:]]+" cls "([^A-Za-z0-9_]|$)")) {
inside = 1; depth = 0; seen_open = 0
} else {
next
}
}
print
line = $0
o = gsub(/{/, "", line)
c = gsub(/}/, "", line)
depth += o - c
if (o > 0) seen_open = 1
if (seen_open && depth <= 0) inside = 0
}
'
for cls in EarlyEndTests LateEndTests CommentEndTests FalsePositiveTests; do
printf '%s\n' "--- scoped lines for $cls ---"
awk -v cls="$cls" "$extract" "$tmpdir/fixture.swift"
done
printf '%s\n' '--- exact method-validation pattern for FalsePositiveTests ---'
awk -v cls='FalsePositiveTests' "$extract" "$tmpdir/fixture.swift" |
grep -E 'func[[:space:]]+test[A-Za-z0-9_]*[[:space:]]*\(\)' || trueRepository: manaflow-ai/cmux
Length of output: 1022
Index only direct XCTest method declarations.
class_scoped_lines and the grep at lines 260-271 accept nested functions, comments, and string literals as test methods. Raw brace counting also misses methods after } in a string or includes sibling classes after { in a string or comment. Use token-aware parsing to ignore comments and strings and match only direct parameterless func test*() members. Add fixtures for nested functions, comments, and brace-containing strings.
🤖 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/dispatch-e2e.sh` around lines 141 - 160, Replace the raw awk
brace-counting and grep logic in class_scoped_lines and the XCTest indexing
block with token-aware parsing that ignores comments and string literals, tracks
the target class body, and matches only direct parameterless func test*()
declarations. Ensure nested functions and sibling classes are excluded,
including when braces appear in comments or strings, and add fixtures covering
nested functions, comments, and brace-containing strings.
…ly discoverable test methods test-e2e.yml gains an optional dispatch_id input echoed into the run name; the dispatcher passes a per-dispatch nonce and resolves its exact run by it, falling back to the pre-existing-ids heuristic (with one no-nonce retry) while main's workflow predates the input. Method validation now only accepts depth-1 instance members of the class or its extensions, rejecting static, class, private, and fileprivate funcs plus nested functions and nested types' methods, matching XCTest discovery rules. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An audit of 23 agent sessions found hosted test-e2e dispatches whose test_filter matched zero tests, each wasting a full dispatch+watch cycle before re-dispatch, plus hours lost to poll-slice waiting, re-dispatched builds, per-one-line-fix rebuilds, and iPhone unlock watchers.
scripts/dispatch-e2e.shis a validated dispatcher fortest-e2e.yml. Before dispatching it requires each filter item's class to exist undercmuxUITests/(orcmuxTests/when target-qualified) and, forClass/method, the method to exist in that class's files. On a miss it refuses to dispatch, exits 2, and prints the nearest candidate names. Comma-separated filters dispatch one run each.--dry-runstops after validation and prints the gh command;--watchblocks ongh run watch --exit-status;--runner,--record-video,--timeout, and--job-timeoutpass through to the workflow inputs. Shellcheck clean.CLAUDE.md now documents the wrapper as the E2E entrypoint (raw
gh workflow runkept as fallback, also updated in the cmux-testing skill reference) and adds an agent time discipline section: park on one blocking command per wait instead of poll slices, one build dispatch per need, batch a dogfood round's fixes into one tagged rebuild, and for a locked or offline iPhone probe once, enqueue, notify, and end the turn.Docs and CI tooling only, no runtime change, so no tagged build.
Validation transcripts:
🤖 Generated with Claude Code
Summary by cubic
Validates hosted E2E dispatches and enforces time‑discipline to prevent zero‑test runs and silent watch passes. Previously a
test_filterwith no matches still dispatched and--watchcould succeed after omitting unresolved runs; now the wrapper validates selectors scoped to the class or its extensions, accepts only parameterless instancetest*methods, resolves runs exactly via a per-dispatchdispatch_id, and fails--watchif any run id cannot be resolved. No runtime changes../scripts/dispatch-e2e.sh --ref <branch-or-sha> --filter "<Class or Class/method>[,more]" [--watch] [--dry-run] [--runner] [--record-video] [--timeout] [--job-timeout]; run it from a checkout of the same ref you dispatch.test-e2e.ymladds optionaldispatch_id; the wrapper passes a nonce and, if the workflow on main rejects unknown inputs, retries once without it and falls back to a pre‑existing‑ids heuristic.gh workflow run test-e2e.yml --repo manaflow-ai/cmux -f ref=<branch-or-sha> -f test_filter="<Class or Class/method>"(required, non-empty).Written for commit b91b7ee. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation