test(dev-lead): E2E test suite for all trigger scenarios - #189
Conversation
…d on large PRs When the engine times out or fails on the same PR MAX_FAIL_ATTEMPTS times (default 2), post a PR-level exhaustion marker that blocks ALL subsequent SHAs on that PR — not just the current SHA. This prevents the agent from retrying every new commit on a PR where Claude consistently times out. - check_idempotency() now checks for exhaustion marker first (PR-wide block) then falls back to SHA-specific check - count_recent_failures() counts status=failed markers across all SHAs on the PR - After hitting threshold, post_exhaustion() posts a human-clearable block comment - Timeout (exit 124) is surfaced as a distinct reason in the failure comment - 3 new unit tests cover exhaustion posting, blocking, and below-threshold behavior - Use standalone jq pipe (not --jq flag) so test stubs work correctly
Adds tests/dev-lead/e2e/ with a master run-all.sh orchestrator, shared helpers library, and six scenario scripts covering the full dispatch matrix: fixture-based (01 bot-pr skip, 05 anti-loop guard, 06 exhaustion guard) and live GitHub API (02 human @mention, 03 ci-relay → fix-ci, 04 issue labeled). Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (12)
✨ 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 |
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an exhaustion guard mechanism to the fix-ci script to prevent excessive token consumption after repeated engine failures, along with a comprehensive E2E test suite. Feedback focused on improving the robustness of the scripts, specifically addressing dangerous JSON truncation in E2E helpers, potential race conditions when counting PR failures, and fragile parsing of GitHub CLI output. Additionally, recommendations were made to handle API pagination and avoid hardcoding the default branch name.
| local run_data | ||
| run_data=$(echo "$runs_json" | jq -r \ | ||
| ".workflow_runs[] | select(.name == \"${workflow_name}\")" \ | ||
| 2>/dev/null | head -c 4096 || true) |
There was a problem hiding this comment.
Using 'head -c 4096' to truncate the JSON response is dangerous. GitHub API responses for workflow runs can easily exceed 4KB, and truncating the stream will result in invalid JSON, causing the subsequent 'jq' calls to fail. Bash variables can handle much larger strings, so this truncation should be removed. If this large payload is subsequently passed to 'curl', ensure it is handled via a temporary file to avoid ARG_MAX limits.
| 2>/dev/null | head -c 4096 || true) | |
| 2>/dev/null || true) |
References
- When passing large JSON payloads (e.g., PR diffs) to curl in shell scripts, use a temporary file and the --data-binary @file flag to avoid exceeding the ARG_MAX command-line argument limit.
| if [ -n "$created_after" ]; then | ||
| run_data=$(echo "$runs_json" | jq -r \ | ||
| ".workflow_runs[] | select(.name == \"${workflow_name}\" and .created_at > \"${created_after}\")" \ | ||
| 2>/dev/null | head -c 4096 || true) |
There was a problem hiding this comment.
As noted above, truncating the JSON response to 4KB will likely lead to parsing errors in 'jq'. Please remove the 'head -c 4096' limit. For large payloads that might be passed to other tools like 'curl', remember to use temporary files to stay within ARG_MAX limits.
| 2>/dev/null | head -c 4096 || true) | |
| 2>/dev/null || true) |
References
- When passing large JSON payloads (e.g., PR diffs) to curl in shell scripts, use a temporary file and the --data-binary @file flag to avoid exceeding the ARG_MAX command-line argument limit.
| comments=$(gh api "repos/${REPO}/issues/${PR_NUMBER}/comments" 2>/dev/null \ | ||
| | jq -r '.[].body' 2>/dev/null || true) |
There was a problem hiding this comment.
The 'gh api' call for comments defaults to 30 items per page. If a PR has many comments, the exhaustion or idempotency markers might be missed. Adding '--per-page 100' or using '--paginate' would make this more robust. Keeping the error redirection inline ensures the intent to silence errors is clear for this specific tier.
| comments=$(gh api "repos/${REPO}/issues/${PR_NUMBER}/comments" 2>/dev/null \ | |
| | jq -r '.[].body' 2>/dev/null || true) | |
| comments=$(gh api "repos/${REPO}/issues/${PR_NUMBER}/comments" --per-page 100 2>/dev/null \ | |
| | jq -r '.[].body' 2>/dev/null || true) |
References
- Keep error routing logic inline when tiers have distinct requirements to avoid obscuring intent or introducing false positives.
| # count_recent_failures: count status=failed markers on this PR (any SHA) | ||
| count_recent_failures() { | ||
| local pattern="${MARKER_PREFIX}[a-f0-9A-F]* status=failed" | ||
| gh api "repos/${REPO}/issues/${PR_NUMBER}/comments" 2>/dev/null \ |
There was a problem hiding this comment.
Similar to the idempotency check, this API call should handle pagination or at least increase the items per page to ensure all failure markers are counted correctly. Maintaining the inline error routing is preferred here to keep the logic explicit.
| gh api "repos/${REPO}/issues/${PR_NUMBER}/comments" 2>/dev/null \ | |
| gh api "repos/${REPO}/issues/${PR_NUMBER}/comments" --per-page 100 2>/dev/null \ |
References
- Keep error routing logic inline when tiers have distinct requirements to avoid obscuring intent or introducing false positives.
| post_summary "failed" "$reason" | ||
|
|
||
| # Check if we've hit the consecutive-failure threshold for this PR | ||
| local fail_count | ||
| fail_count=$(count_recent_failures) | ||
| echo " [fix-ci] consecutive failures on this PR: $fail_count (threshold: $MAX_FAIL_ATTEMPTS)" | ||
| if [ "$fail_count" -ge "$MAX_FAIL_ATTEMPTS" ]; then | ||
| echo "::warning::Exhaustion threshold reached — posting PR-level block to prevent further token spend" | ||
| post_exhaustion "$reason" | ||
| fi |
There was a problem hiding this comment.
There is a potential race condition here. 'post_summary' calls the GitHub API to post a comment, and 'count_recent_failures' immediately calls the API to list comments. Due to eventual consistency in GitHub's APIs, the comment just posted might not be returned in the list immediately. It is safer to count the existing failures before posting the new one. Additionally, for LLM-based tasks, ensure the script detects rate limits by checking exit codes and scanning output streams, exiting with code 2 if a limit is hit to allow for engine fallback.
| post_summary "failed" "$reason" | |
| # Check if we've hit the consecutive-failure threshold for this PR | |
| local fail_count | |
| fail_count=$(count_recent_failures) | |
| echo " [fix-ci] consecutive failures on this PR: $fail_count (threshold: $MAX_FAIL_ATTEMPTS)" | |
| if [ "$fail_count" -ge "$MAX_FAIL_ATTEMPTS" ]; then | |
| echo "::warning::Exhaustion threshold reached — posting PR-level block to prevent further token spend" | |
| post_exhaustion "$reason" | |
| fi | |
| # Count existing failures before posting the new one to avoid API race conditions | |
| local prev_fail_count | |
| prev_fail_count=$(count_recent_failures) | |
| local total_fail_count=$((prev_fail_count + 1)) | |
| post_summary "failed" "$reason" | |
| echo " [fix-ci] consecutive failures on this PR: $total_fail_count (threshold: $MAX_FAIL_ATTEMPTS)" | |
| if [ "$total_fail_count" -ge "$MAX_FAIL_ATTEMPTS" ]; then | |
| echo "::warning::Exhaustion threshold reached — posting PR-level block to prevent further token spend" | |
| post_exhaustion "$reason" | |
| fi |
References
- In shell scripts implementing retry loops for LLM-based tasks, detect rate limits immediately by checking exit codes and scanning both stdout and stderr for rate-limit indicators before attempting a retry. If a rate limit is detected, exit with a specific code (e.g., 2) to enable engine fallback mechanisms.
| main_sha=$(gh api "repos/${E2E_TARGET_REPO}/git/ref/heads/main" \ | ||
| --jq '.object.sha' 2>/dev/null) |
There was a problem hiding this comment.
Hardcoding the branch name to 'main' can cause failures in repositories that use a different default branch (e.g., 'master'). It's better to query the repository's default branch dynamically.
| main_sha=$(gh api "repos/${E2E_TARGET_REPO}/git/ref/heads/main" \ | |
| --jq '.object.sha' 2>/dev/null) | |
| local default_branch | |
| default_branch=$(gh repo view "${E2E_TARGET_REPO}" --json defaultBranchRef --jq .defaultBranchRef.name) | |
| main_sha=$(gh api "repos/${E2E_TARGET_REPO}/git/ref/heads/${default_branch}" \ | |
| --jq '.object.sha' 2>/dev/null) |
| --base "main" \ | ||
| --title "${title}" \ | ||
| --body "${body}" \ | ||
| 2>&1 | grep -oE '[0-9]+$' | head -1) |
There was a problem hiding this comment.
Parsing the PR number from stdout using 'grep' is fragile. The 'gh pr create' command supports '--json' and '--jq' flags, which provide a much more reliable way to extract the PR number. Also, avoid hardcoding the base branch as 'main'.
| --base "main" \ | |
| --title "${title}" \ | |
| --body "${body}" \ | |
| 2>&1 | grep -oE '[0-9]+$' | head -1) | |
| local default_branch | |
| default_branch=$(gh repo view "${E2E_TARGET_REPO}" --json defaultBranchRef --jq .defaultBranchRef.name) | |
| pr_number=$(gh pr create \ | |
| --repo "${E2E_TARGET_REPO}" \ | |
| --head "${branch}" \ | |
| --base "${default_branch}" \ | |
| --title "${title}" \ | |
| --body "${body}" \ | |
| --json number --jq .number) |
There was a problem hiding this comment.
Pull request overview
Adds a new end-to-end test framework for the dev-lead agent under tests/dev-lead/e2e/, with an orchestrator and six scenarios covering bot-PR skip, human at-mention, CI-failure relay, issue-labeling, anti-loop guard, and exhaustion guard. The production change in scripts/dev-lead-fix-ci.sh introduces a PR-level exhaustion marker that blocks repeated engine-failure retries once MAX_FAIL_ATTEMPTS is hit, plus matching unit tests in test_fix_ci.bats.
Changes:
- Adds the
tests/dev-lead/e2e/suite:run-all.shorchestrator,lib/helpers.sh, six scenario scripts, README, and a gitignored results dir. - Adds an exhaustion guard to
scripts/dev-lead-fix-ci.sh(MAX_FAIL_ATTEMPTS,EXHAUSTION_MARKER,count_recent_failures,post_exhaustion) and refactorscheck_idempotencyto use jq on raw comment bodies. - Extends
test_fix_ci.batswith three exhaustion-related tests and updates the existing idempotency stub.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/dev-lead-fix-ci.sh | Adds PR-level exhaustion marker, failure counter, and reworks idempotency check |
| tests/dev-lead/unit/test_fix_ci.bats | Updates idempotency stub and adds three exhaustion-threshold tests |
| tests/dev-lead/e2e/run-all.sh | New orchestrator with --dry-run / --scenario flags and summary reporting |
| tests/dev-lead/e2e/lib/helpers.sh | Shared helpers: branch/PR/issue creation, workflow polling, assertions, cleanup |
| tests/dev-lead/e2e/scenarios/01-skip-bot-pr.sh | Fixture-based scenario asserting dependabot PR → skip(bot-pr) |
| tests/dev-lead/e2e/scenarios/02-human-at-mention.sh | Live scenario: @dev-lead comment triggers workflow |
| tests/dev-lead/e2e/scenarios/03-ci-failure-relay.sh | Live scenario: failing check → ci-relay → repository_dispatch → fix-ci |
| tests/dev-lead/e2e/scenarios/04-issue-labeled.sh | Live scenario: labeling issue dev-lead triggers the issue intent |
| tests/dev-lead/e2e/scenarios/05-skip-anti-loop.sh | Fixture-based anti-loop guard (BOT_USER → skip, human → human-pr) |
| tests/dev-lead/e2e/scenarios/06-exhaustion-guard.sh | Stubs gh and claude to validate the new exhaustion behaviour end-to-end |
| tests/dev-lead/e2e/README.md | Documents usage, scenarios, env vars, and CI integration |
| tests/dev-lead/e2e/results/.gitignore | Gitignores generated result files |
Comments suppressed due to low confidence (9)
tests/dev-lead/e2e/run-all.sh:128
--scenariomatching uses substring (*"${SINGLE_SCENARIO}"*) against the basename, so a short value like--scenario 0would silently match every scenario, and--scenario 06would only work because it happens to be unique. Consider matching by exact basename or by leading-digit prefix to make the CLI predictable, and erroring (or listing) when multiple scenarios match.
if [ -n "${SINGLE_SCENARIO}" ]; then
FILTERED=()
for s in "${ALL_SCENARIOS[@]}"; do
if [[ "$(basename "$s" .sh)" == *"${SINGLE_SCENARIO}"* ]]; then
FILTERED+=("$s")
fi
done
if [ "${#FILTERED[@]}" -eq 0 ]; then
log "ERROR: No scenario found matching '${SINGLE_SCENARIO}'"
log "Available scenarios:"
for s in "${ALL_SCENARIOS[@]}"; do
log " $(basename "$s" .sh)"
done
exit 1
fi
ALL_SCENARIOS=("${FILTERED[@]}")
fi
tests/dev-lead/e2e/run-all.sh:171
- The capability summary only reports
GH_TOKEN/GH_PATand tags live scenarios as "will be skipped" when neither is set, but the orchestrator does not actually skip those scenarios — it still runs them, and each scenario decides on its own whether to SKIP. More importantly, when neither token is set, each scenario will exit 0 (SKIP) butrun-all.shcounts that as PASS (line 164–166), since it has no way to distinguish PASS from internally-recorded SKIP. Consider readingresults.txtto count true SKIP outcomes, or have scenarios use a distinct exit code for SKIP.
# ── Execute scenario ────────────────────────────────────────────────────────
set +e
bash "${scenario_script}"
scenario_exit=$?
set -e
if [ "${scenario_exit}" -eq 0 ]; then
log "[PASS] ${scenario_name}"
PASS_COUNT=$(( PASS_COUNT + 1 ))
else
log "[FAIL] ${scenario_name} (exit code: ${scenario_exit})"
FAIL_COUNT=$(( FAIL_COUNT + 1 ))
fi
done
scripts/dev-lead-fix-ci.sh:161
- The exhaustion threshold logic counts
status=failedmarkers viacount_recent_failuresafterpost_summary "failed"has just been called — but that summary call posts the new failure marker through the real GitHub API. So in production, after the first failure on a fresh PR, the count immediately becomes 1; after the second, it becomes 2. With the defaultMAX_FAIL_ATTEMPTS=2, this works, but the comment around line 156 ("consecutive failures") is misleading: there is no recency window, so oldstatus=failedmarkers from prior SHAs (even after a successful intervening run) still count. Consider scoping the count to the current SHA or to comments newer than the most recentstatus=applied, or update the variable/comment naming to reflect that it is a lifetime PR-level count.
if [ "$engine_rc" -ne 0 ]; then
# Classify the failure: timeout (124) is a hard exhaustion candidate
local reason="Engine invocation failed (exit ${engine_rc})"
[ "$engine_rc" -eq 124 ] && reason="Engine timed out — PR may be too large for automated fixing"
post_summary "failed" "$reason"
# Check if we've hit the consecutive-failure threshold for this PR
local fail_count
fail_count=$(count_recent_failures)
echo " [fix-ci] consecutive failures on this PR: $fail_count (threshold: $MAX_FAIL_ATTEMPTS)"
if [ "$fail_count" -ge "$MAX_FAIL_ATTEMPTS" ]; then
echo "::warning::Exhaustion threshold reached — posting PR-level block to prevent further token spend"
post_exhaustion "$reason"
fi
exit 1
scripts/dev-lead-fix-ci.sh:51
- The regex
${MARKER_PREFIX}[a-f0-9A-F]* status=failedonly matches markers whose SHA segment is purely hexadecimal. Real commit SHAs always are, butMARKER_PREFIXends with<!-- dev-lead-fix-ci sha=and embeds a literal<!--. While<,!,-are not jq/Oniguruma metacharacters, future modifications toMARKER_PREFIXcould introduce regex specials and silently break the count. Consider building the regex from a regex-escaped marker prefix, or using a fixed-stringcontains("...")check combined with a separateendswithcheck onstatus=failed -->.
count_recent_failures() {
local pattern="${MARKER_PREFIX}[a-f0-9A-F]* status=failed"
gh api "repos/${REPO}/issues/${PR_NUMBER}/comments" 2>/dev/null \
| jq "[.[] | select(.body | test(\"${pattern}\"))] | length" 2>/dev/null \
|| echo "0"
}
scripts/dev-lead-fix-ci.sh:115
check_idempotencynow treats any exhaustion marker as a hard block until somebody manually deletes the comment. The user-facing instructions ("delete this comment or push a new commit with a substantially different change") inpost_exhaustionclaim a new commit will re-enable, but the guard explicitly says "blocks regardless of SHA" and the code only looks at the marker, not at the SHA. New pushes will still be blocked. Either reconcile the message with the behavior, or make the unblock work as advertised (e.g. invalidate the marker after N new SHAs, or only honour exhaustion markers whosesha=matches).
post_exhaustion() {
local reason="$1"
local body="${EXHAUSTION_MARKER}
## Dev-Lead Fix CI — exhausted
This PR has had **${MAX_FAIL_ATTEMPTS}** consecutive engine failures (timeouts or errors). Automated CI fixing has been paused to avoid consuming further tokens.
**Reason for last failure:** ${reason}
To re-enable, delete this comment or push a new commit with a substantially different change."
if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then
echo "[dry-run] would post exhaustion comment"
else
gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$body"
fi
}
tests/dev-lead/e2e/lib/helpers.sh:183
wait_for_workflow_by_eventechoes the conclusion to stdout (line 169) but also callsinfo/err(which write to stdout/stderr) from within the same function.infoprefixes go to stdout (line 22), so the caller's command substitutionconclusion=$(wait_for_workflow_by_event ...)will capture both the[hh:mm:ss] INFO …lines and the final conclusion concatenated together. The same issue affectswait_for_workflow. Callers like scenarios 03/04 then compare the multi-line result against"success"/"neutral", which will fail spuriously. Either routelog/infoto stderr (>&2) inside these helpers, or write the result to a file/named variable rather than stdout.
wait_for_workflow() {
local repo="${1:?repo required}"
local workflow_name="${2:?workflow_name required}"
local head_sha="${3:?head_sha required}"
local timeout_sec="${4:-300}"
local deadline=$(( $(date +%s) + timeout_sec ))
local poll_interval=15
info "Waiting for workflow '${workflow_name}' on SHA ${head_sha:0:8}... (timeout: ${timeout_sec}s)"
while [ "$(date +%s)" -lt "$deadline" ]; do
local run_data
run_data=$(gh api \
"repos/${repo}/actions/runs?head_sha=${head_sha}&per_page=20" \
--jq ".workflow_runs[] | select(.name == \"${workflow_name}\")" \
2>/dev/null || true)
if [ -n "$run_data" ]; then
local status conclusion
status=$(echo "$run_data" | jq -r '.status' 2>/dev/null | head -1)
conclusion=$(echo "$run_data" | jq -r '.conclusion // ""' 2>/dev/null | head -1)
if [ "$status" = "completed" ] && [ -n "$conclusion" ]; then
info "Workflow '${workflow_name}' completed: conclusion=${conclusion}"
echo "${conclusion}"
return 0
fi
info " status=${status} conclusion=${conclusion} — still running..."
else
info " no runs yet for '${workflow_name}' on ${head_sha:0:8}..."
fi
sleep "$poll_interval"
done
err "Timed out after ${timeout_sec}s waiting for workflow '${workflow_name}'"
echo "timeout"
return 1
}
# ── wait_for_workflow_by_event <repo> <workflow_name> <event> <timeout_sec> ───
# Polls until the most recent workflow run triggered by <event> completes.
# Used for events like repository_dispatch or issues where we don't have a SHA.
wait_for_workflow_by_event() {
local repo="${1:?repo required}"
local workflow_name="${2:?workflow_name required}"
local event="${3:?event required}"
local timeout_sec="${4:-300}"
local created_after="${5:-}" # ISO8601 string; filters to runs created after this time
local deadline=$(( $(date +%s) + timeout_sec ))
local poll_interval=15
info "Waiting for workflow '${workflow_name}' triggered by '${event}'... (timeout: ${timeout_sec}s)"
while [ "$(date +%s)" -lt "$deadline" ]; do
local runs_json
runs_json=$(gh api \
"repos/${repo}/actions/runs?event=${event}&per_page=10" \
2>/dev/null || echo '{"workflow_runs":[]}')
local run_data
run_data=$(echo "$runs_json" | jq -r \
".workflow_runs[] | select(.name == \"${workflow_name}\")" \
2>/dev/null | head -c 4096 || true)
if [ -n "$run_data" ]; then
# If created_after filter, select only runs newer than it
if [ -n "$created_after" ]; then
run_data=$(echo "$runs_json" | jq -r \
".workflow_runs[] | select(.name == \"${workflow_name}\" and .created_at > \"${created_after}\")" \
2>/dev/null | head -c 4096 || true)
fi
fi
if [ -n "$run_data" ]; then
local status conclusion
status=$(echo "$run_data" | jq -rs '.[0].status // ""')
conclusion=$(echo "$run_data" | jq -rs '.[0].conclusion // ""')
if [ "$status" = "completed" ] && [ -n "$conclusion" ] && [ "$conclusion" != "null" ]; then
info "Workflow '${workflow_name}' completed: conclusion=${conclusion}"
echo "${conclusion}"
return 0
fi
info " status=${status} conclusion=${conclusion} — still running..."
else
info " no matching runs yet for '${workflow_name}'..."
fi
sleep "$poll_interval"
done
err "Timed out after ${timeout_sec}s waiting for workflow '${workflow_name}'"
echo "timeout"
return 1
}
tests/dev-lead/e2e/lib/helpers.sh:17
- Scenarios 02–04 source
helpers.sh, which on line 14 emits a WARNING to stderr if neitherGH_TOKENnorGH_PATis set, before the scenario's ownGH_TOKENprecheck has a chance to gracefully SKIP. The warning is harmless but will be confusing in logs ("API calls will fail" then the test immediately reports SKIP). Consider moving the token presence check earlier in scenarios, or only emitting the warning at the point of first use.
GH_TOKEN="${GH_TOKEN:-${GH_PAT:-}}"
if [ -z "$GH_TOKEN" ]; then
echo "[helpers] WARNING: neither GH_TOKEN nor GH_PAT is set — API calls will fail" >&2
fi
export GH_TOKEN
tests/dev-lead/e2e/lib/helpers.sh:76
create_test_prextracts the PR number withgrep -oE '[0-9]+$' | head -1from the entire stdout+stderr output ofgh pr create.gh pr createprints the PR URL (e.g.https://github.com/owner/repo/pull/123), so the last numeric run captures the PR number — but on warning/error output, this can pick up an unrelated number (an HTTP status, a rate-limit count) and return the wrong value silently. Prefergh pr create … 2>/dev/null | tail -1and parse the URL explicitly, or usegh pr view --json number --jq .numberafter creation.
local pr_number
pr_number=$(gh pr create \
--repo "${E2E_TARGET_REPO}" \
--head "${branch}" \
--base "main" \
--title "${title}" \
--body "${body}" \
2>&1 | grep -oE '[0-9]+$' | head -1)
if [ -z "$pr_number" ]; then
err "Failed to create PR for branch ${branch}"
return 1
fi
tests/dev-lead/e2e/lib/helpers.sh:371
record_resultis called with status="SKIP" in scenarios 02–04 whenGH_TOKENis unset, but it is also called with "PASS"/"FAIL" elsewhere. Because the recorded line uses the literal status, the summary correctly distinguishes them — but the orchestrator counters inrun-all.shcount any exit-0 scenario as PASS (including SKIPs from scenarios 02–04), masking missing-token runs in the final tally. Either propagate SKIP via an exit code (e.g. 77, the autotools convention) and haverun-all.shinterpret it, or grepresults.txtfor the SKIP entries when computing the summary.
record_result() {
local scenario="${1:?scenario required}"
local status="${2:?status required}" # PASS or FAIL
local details="${3:-}"
mkdir -p "${E2E_RESULTS_DIR}"
local result_file="${E2E_RESULTS_DIR}/results.txt"
local ts
ts=$(date -u +%Y-%m-%dT%H:%M:%SZ)
echo "${ts} [${status}] ${scenario}: ${details}" >> "${result_file}"
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| local intent_output | ||
| intent_output=$( | ||
| GITHUB_ENV="${GITHUB_ENV_FILE}" \ | ||
| GITHUB_OUTPUT="${GITHUB_OUTPUT_FILE}" \ | ||
| GITHUB_EVENT_NAME="pull_request" \ | ||
| GITHUB_EVENT_PATH="${fixture}" \ | ||
| GITHUB_REPOSITORY="${E2E_TARGET_REPO}" \ | ||
| BOT_USER="donpetry-bot" \ | ||
| bash "${INTENT_SCRIPT}" 2>&1 | ||
| ) | ||
|
|
||
| local exit_code=$? |
| local intent_output_a | ||
| intent_output_a=$( | ||
| GITHUB_ENV="${GITHUB_ENV_FILE}" \ | ||
| GITHUB_OUTPUT="${GITHUB_OUTPUT_FILE}" \ | ||
| GITHUB_EVENT_NAME="pull_request" \ | ||
| GITHUB_EVENT_PATH="${fixture}" \ | ||
| GITHUB_REPOSITORY="${E2E_TARGET_REPO}" \ | ||
| BOT_USER="donpetry-bot" \ | ||
| bash "${INTENT_SCRIPT}" 2>&1 | ||
| ) | ||
|
|
||
| local exit_code_a=$? | ||
| log "Script exit code: ${exit_code_a}" | ||
| echo "${intent_output_a}" | sed 's/^/ /' | ||
|
|
||
| local intent_type_a intent_reason_a | ||
| intent_type_a=$(_get_env_var "INTENT_TYPE") | ||
| intent_reason_a=$(_get_env_var "INTENT_REASON") | ||
| log "INTENT_TYPE=${intent_type_a} INTENT_REASON=${intent_reason_a}" | ||
|
|
||
| # Reset for part B | ||
| > "$GITHUB_ENV_FILE" | ||
| > "$GITHUB_OUTPUT_FILE" | ||
|
|
||
| # ── Part B: human synchronize → must NOT emit anti-loop skip ────────────── | ||
| log "" | ||
| log "Part B: synchronize from human user (not BOT_USER) — using pr_opened_human.json" | ||
|
|
||
| local human_fixture="${FIXTURE_DIR}/pr_opened_human.json" | ||
| # pr_opened_human.json has action=opened, but we just need a sender that's not the bot | ||
| # We also verify the human synchronize path doesn't accidentally hit anti-loop. | ||
| # For a pure synchronize test from a human, we'd need a separate fixture. | ||
| # The existing pr_opened_human.json has action=opened which goes through different routing. | ||
| # We create an inline fixture for a human synchronize: | ||
| local human_sync_fixture | ||
| human_sync_fixture=$(mktemp --suffix=.json) | ||
| jq -n '{ | ||
| action: "synchronize", | ||
| number: 99, | ||
| pull_request: { | ||
| number: 99, | ||
| title: "Human sync test", | ||
| body: "", | ||
| state: "open", | ||
| author_association: "OWNER", | ||
| head: { | ||
| sha: "human999sha", | ||
| ref: "feat/human-branch", | ||
| repo: { full_name: "petry-projects/.github-private" } | ||
| }, | ||
| base: { | ||
| ref: "main", | ||
| repo: { full_name: "petry-projects/.github-private" } | ||
| } | ||
| }, | ||
| repository: { full_name: "petry-projects/.github-private" }, | ||
| sender: { login: "donpetry", type: "User" } | ||
| }' > "${human_sync_fixture}" | ||
|
|
||
| local intent_output_b | ||
| intent_output_b=$( | ||
| GITHUB_ENV="${GITHUB_ENV_FILE}" \ | ||
| GITHUB_OUTPUT="${GITHUB_OUTPUT_FILE}" \ | ||
| GITHUB_EVENT_NAME="pull_request" \ | ||
| GITHUB_EVENT_PATH="${human_sync_fixture}" \ | ||
| GITHUB_REPOSITORY="${E2E_TARGET_REPO}" \ | ||
| BOT_USER="donpetry-bot" \ | ||
| bash "${INTENT_SCRIPT}" 2>&1 | ||
| ) | ||
|
|
||
| local exit_code_b=$? | ||
| log "Script exit code (human sync): ${exit_code_b}" |
|
|
||
| [ "$status" -eq 1 ] | ||
| # Should post sha-level marker but NOT exhaustion comment | ||
| [[ "$output" != *"exhaustion threshold reached"* ]] || true |
| # The existing pr_opened_human.json has action=opened which goes through different routing. | ||
| # We create an inline fixture for a human synchronize: | ||
| local human_sync_fixture | ||
| human_sync_fixture=$(mktemp --suffix=.json) |
| LOG_MAX_LINES="${LOG_MAX_LINES:-200}" | ||
| MAX_FAIL_ATTEMPTS="${MAX_FAIL_ATTEMPTS:-2}" | ||
| MARKER_PREFIX="<!-- dev-lead-fix-ci sha=" | ||
| EXHAUSTION_MARKER="<!-- dev-lead-fix-ci pr=${PR_NUMBER} status=exhausted -->" |
| log() { echo "[$(date -u +%H:%M:%S)] $*"; } | ||
|
|
| #!/usr/bin/env bash | ||
| # tests/dev-lead/e2e/lib/helpers.sh — Shared helpers for dev-lead E2E tests. | ||
| # Sourced by each scenario script. | ||
| set -euo pipefail |
| ISSUE_NUMBER=$(gh issue create \ | ||
| --repo "${E2E_TARGET_REPO}" \ | ||
| --title "[E2E] Scenario 04: dev-lead label routing test" \ | ||
| --body "Automated E2E test issue for scenario 04. Safe to close. | ||
|
|
||
| This issue tests that the dev-lead agent correctly handles the \`issues labeled\` event with the \`dev-lead\` label. | ||
|
|
||
| Created: $(date -u)" \ | ||
| 2>&1 | grep -oE '[0-9]+$' | head -1) |
| local head_sha | ||
| head_sha=$(push_file_to_branch \ | ||
| "${BRANCH}" \ | ||
| "tests/dev-lead/e2e/.tmp-bad-script-03.sh" \ | ||
| "${content_b64}" \ | ||
| "test(e2e): scenario 03 — intentional CI failure file") | ||
|
|
||
| log "HEAD SHA: ${head_sha}" |
| if ! assert_conclusion "${conclusion}" "success" "${SCENARIO_NAME}: workflow completes successfully"; then | ||
| # Also accept "neutral" (if DEV_LEAD_DRY_RUN blocks real actions) for now | ||
| if [ "${conclusion}" = "neutral" ]; then | ||
| echo "[PASS] ${SCENARIO_NAME}: workflow conclusion=neutral (dry-run mode)" | ||
| else | ||
| all_pass=false | ||
| fi | ||
| fi |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0c087ef99
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| local ts | ||
| ts=$(date -u +%Y%m%d-%H%M%S) | ||
| local branch_name="${base_name}-${ts}" | ||
| info "Creating branch: ${branch_name} from main on ${E2E_TARGET_REPO}" |
There was a problem hiding this comment.
Live scenarios 02 and 03 capture this helper with BRANCH=$(create_test_branch ...), but info writes to stdout before the function echoes the branch name. That makes BRANCH contain both the log line and the branch, so the next push_file_to_branch "${BRANCH}" ... call uses an invalid ref and the live E2E scenarios fail as soon as a GitHub token is present. Send these logs to stderr or otherwise keep stdout reserved for the returned value.
Useful? React with 👍 / 👎.
| local pattern="${MARKER_PREFIX}[a-f0-9A-F]* status=failed" | ||
| gh api "repos/${REPO}/issues/${PR_NUMBER}/comments" 2>/dev/null \ | ||
| | jq "[.[] | select(.body | test(\"${pattern}\"))] | length" 2>/dev/null \ | ||
| || echo "0" |
There was a problem hiding this comment.
This counts every historical status=failed marker on the PR, even if a later status=applied, no-changes, or dry-run marker proves the failure streak was broken. In a PR with one old engine failure, then a successful auto-fix, then one new engine failure, MAX_FAIL_ATTEMPTS=2 will incorrectly post the PR-level exhaustion marker and block all future SHAs. The exhaustion check needs to consider only the latest consecutive failed attempts rather than all failed comments.
Useful? React with 👍 / 👎.
| conclusion=$(wait_for_workflow \ | ||
| "${E2E_TARGET_REPO}" \ | ||
| "Dev-Lead Agent" \ | ||
| "${head_sha}" \ | ||
| 300) || conclusion="timeout" |
There was a problem hiding this comment.
For the issue_comment trigger used in this scenario, the Dev-Lead Agent workflow run is associated with the workflow/default-branch commit rather than the PR branch commit created above, so filtering by the PR head_sha never finds the run even when the @mention correctly triggered it. This makes scenario 02 time out in live runs; use the event-based waiter with the recorded start time, as scenario 04 does for issues, instead of waiting on the PR SHA.
Useful? React with 👍 / 👎.
| # The failing file: a shell script with a syntax error (missing 'fi') | ||
| # shellcheck will catch this, or bash -n will reject it. | ||
| local bad_script | ||
| bad_script='#!/usr/bin/env bash | ||
| # E2E test: intentional syntax error for CI failure scenario | ||
| # This file is part of the automated E2E test suite — safe to delete. | ||
| if [ "x" = "y" ]; then | ||
| echo "syntax error below — missing fi" | ||
| # deliberately omitting fi to trigger shellcheck/bash -n failure | ||
| ' |
There was a problem hiding this comment.
Adding a syntactically invalid .sh file here does not fail any checked workflow in this repo: I checked .github/workflows/test.yml lines 18-22 and .github/workflows/test-dev-lead.yml lines 27-60, and they run two specific scripts, fixture validation, Bats unit tests, and prompt coverage—not shellcheck or bash -n over this new file. With no failing check_run, the relay path under test is never triggered and scenario 03 times out or validates the wrong thing; introduce a change that one of the configured workflows actually fails.
Useful? React with 👍 / 👎.
|
|
||
| **Reason for last failure:** ${reason} | ||
|
|
||
| To re-enable, delete this comment or push a new commit with a substantially different change." |
There was a problem hiding this comment.
The exhaustion marker is PR-level (check_idempotency skips whenever this comment exists, regardless of HEAD_SHA), so pushing a new commit does not re-enable automated fixing despite this message saying it does. In the exhaustion path users will follow the suggested remediation, push another commit, and still be blocked until the comment is deleted, so the comment should only advertise actions that the script actually honors.
Useful? React with 👍 / 👎.
| if [[ "${scenario_name}" == *"${needs_claude_scenario}"* ]] && [ "${HAS_CLAUDE_TOKEN}" = "false" ]; then | ||
| log " NOTE: ${scenario_name} will skip Claude response assertions (CLAUDE_CODE_OAUTH_TOKEN not set)" | ||
| fi |
There was a problem hiding this comment.
When GH_TOKEN is set but CLAUDE_CODE_OAUTH_TOKEN is not, this only logs a note and still executes scenarios 02-04. Those scenarios trigger the real Dev-Lead workflow, whose preflight requires CLAUDE_CODE_OAUTH_TOKEN, so the workflow concludes failure rather than merely skipping response-content assertions; the runner should skip these scenarios or provide a real dry-run path instead of running tests that are guaranteed to fail in that advertised configuration.
Useful? React with 👍 / 👎.
|
|
||
| [ "$status" -eq 1 ] | ||
| # Should post sha-level marker but NOT exhaustion comment | ||
| [[ "$output" != *"exhaustion threshold reached"* ]] || true |
There was a problem hiding this comment.
This assertion can never fail because of the trailing || true, so the below-threshold test will pass even if the script logs exhaustion threshold reached and posts the PR-level block after only one prior failure. That leaves the new exhaustion guard behavior unprotected against exactly the regression this test claims to catch.
Useful? React with 👍 / 👎.
Summary
tests/dev-lead/e2e/with a complete E2E test framework for the dev-lead agentrun-all.shorchestrator with--dry-runand--scenario <name>flagsScenarios
intent=skip(bot-pr)@dev-leadcomment →humanintent, workflow runsrepository_dispatch→fix-cidev-lead→issueintent, workflow runsskip(dev-lead-own-commit); human sync →human-prTest plan
bash -non all 8 shell files — all pass[PASS][PASS](both parts A and B)[PASS](both parts A and B)run-all.sh --dry-runlists all 6 scenarios and exits 0run-all.sh --scenario 05-skip-anti-loopruns and passesGH_TOKENand a live GitHub environment🤖 Generated with Claude Code