diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 25d904611..c411cbb87 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -137,7 +137,8 @@ jobs: tests/test_validate_workflow_schedules.bats \ tests/caller_stub_freeze.bats \ tests/pr_review_canary.bats \ - tests/dev-lead/unit/test_maintainer_review_thread_gate.bats + tests/dev-lead/unit/test_maintainer_review_thread_gate.bats \ + tests/dev-lead/unit/test_conflict_integrity.bats actionlint: # General workflow linting (#1256, epic #1052 Part D): actionlint validates diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index 08737f985..8e392abe5 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -10,6 +10,7 @@ source "$(dirname "$0")/lib/auto-merge.sh" source "$(dirname "$0")/lib/git-push-guard.sh" source "$(dirname "$0")/lib/pr-automation-budget.sh" source "$(dirname "$0")/lib/maintainer-review-thread-gate.sh" +source "$(dirname "$0")/lib/conflict-integrity.sh" INTENT_TYPE="${INTENT_TYPE:-fix-reviews}" PR_NUMBER="${PR_NUMBER:-}" @@ -24,6 +25,7 @@ PROMPTS_DIR="$(resolve_abs "$PROMPTS_DIR")" export PROMPTS_DIR REVIEWS_MARKER_PREFIX="" + local body="${marker} +> [!WARNING] +> **Post-conflict-resolution integrity check flagged this resolution.** +> The automated rebase introduced **duplicate top-level declarations** — the +> #1449 corruption pattern, where a botched merge silently doubled functions and +> the damage was only found hours later by an unrelated failing test. This is +> **advisory**: the resolution is not blocked or reverted. Please verify the +> file(s) below before trusting this branch. + +${report} +_Detected by \`scripts/lib/conflict-integrity.sh\` (#1482). If this repetition is intentional, disregard._" + + if [ "$DEV_LEAD_DRY_RUN" = "true" ]; then + echo "[dry-run] would post conflict-integrity warning for PR ${PR_NUMBER}" + echo "$body" + return 0 + fi + # Idempotent: skip if an integrity marker for this PR already exists. + if gh pr view "$PR_NUMBER" --repo "$REPO" --json comments \ + --jq '.comments[].body' 2>/dev/null | grep -F "$marker" > /dev/null; then + echo "conflict-integrity marker already present on PR ${PR_NUMBER} — skipping" + return 0 + fi + # Best-effort: don't fail the overall rebase if the advisory post fails. + gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$body" 2>/dev/null || true +} + +# run_post_resolution_integrity_check: after an automated conflict resolution +# lands, scan the shell files it touched for duplicate top-level declarations +# the resolution *introduced* (#1449 corruption class). Parents are the base ref +# (origin/) and the branch tip before the resolution (); a symbol +# whose declaration count exceeds both parents is flagged. Runs on the +# conflict-resolution path itself (#1482 AC #2) rather than waiting for an +# unrelated downstream test to trip over the corruption. +run_post_resolution_integrity_check() { + local base_ref="$1" pre_ref="$2" + [ -n "$pre_ref" ] || return 0 + + local changed + # Compare pre_ref to the working tree (not just HEAD) so that uncommitted + # worktree edits left by the agent are also included in the scan (#1496). + changed="$(git diff --name-only "$pre_ref" -- '*.sh' 2>/dev/null || true)" + [ -n "$changed" ] || return 0 + + local report="" file findings pa pb + while IFS= read -r file; do + [ -n "$file" ] || continue + [ -f "$file" ] || continue # deleted by the resolution — nothing to check + pa="$(mktemp)"; pb="$(mktemp)" + git show "origin/${base_ref}:${file}" >"$pa" 2>/dev/null || : >"$pa" + git show "${pre_ref}:${file}" >"$pb" 2>/dev/null || : >"$pb" + findings="$(new_duplicate_symbols "$file" "$pa" "$pb")" + rm -f "$pa" "$pb" + if [ -n "$findings" ]; then + report="${report}$(format_integrity_findings "$file" "$findings") +" + fi + done </dev/null \ + | jq "[.[] | select(.body | test(\"${pattern}\"))] | length" 2>/dev/null \ + || echo "0") + [ "${count:-0}" -gt 0 ] +} # post_reviews_rate_limited: posts a rate-limited marker for fix-reviews intents. # For retryable intents (fix-reviews, review-changes, rebase), the cron will re-dispatch. @@ -1081,1223 +1123,6 @@ commit_and_push() { return 0 } -# post_reviews_terminal: writes a terminal status marker after a retryable -# intent completes. This prevents the retry cron from re-dispatching the same -# intent on subsequent runs when the SHA hasn't changed. -post_reviews_terminal() { - local intent="$1" status="${2:-applied}" summary="${3:-}" - local sha_part="" - [ -n "${HEAD_SHA:-}" ] && sha_part=" sha=${HEAD_SHA}" - local marker="${REVIEWS_MARKER_PREFIX}${PR_NUMBER}${sha_part} intent=${intent} status=${status} -->" - - local body="${marker}" - if [ -n "$summary" ]; then - body="${body} -## Dev-Lead — ${intent} (${status}) -${summary}" - fi - - if [ "$DEV_LEAD_DRY_RUN" = "true" ]; then - echo "[dry-run] would post reviews terminal marker: intent=${intent} status=${status}" - [ -n "$summary" ] && echo "$body" - return 0 - fi - # Best-effort: don't fail the overall script if the marker post fails - gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$body" 2>/dev/null || true -} - -# detect_conflicting_paths — list paths that conflict when merging -# origin/ into the current HEAD, one per line. Uses a trial merge -# (immediately aborted) because it is robust across git versions: the former -# `git merge-tree HEAD ` 3-arg form prints a "changed in both" -# section header whose last field is the literal word "both" (the filename is on -# the indented our/their lines), so `awk '{print $NF}'` produced a bogus "both" -# path that was fed to the rebase prompt. Leaves the worktree clean. -detect_conflicting_paths() { - local base="$1" - [[ -z "$base" ]] && return 0 - git merge --no-commit --no-ff "origin/${base}" >/dev/null 2>&1 || true - git diff --name-only --diff-filter=U || true - git merge --abort >/dev/null 2>&1 || true -} - -# expire_stale_terminal_markers: deletes any existing terminal comments (applied, -# no-changes, or failed) for this SHA+intent before a hard-blocker retry marker is -# posted. Without this, the retry cron sees a stale terminal and skips re-dispatch -# even though a new hard blocker (e.g. a CHANGES_REQUESTED review added after the -# prior run) now requires retry. -expire_stale_terminal_markers() { - local intent="$1" - local sha="${HEAD_SHA:-}" - [ -z "$sha" ] && return 0 - if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then - echo "[dry-run] would expire stale terminal markers for intent=${intent} sha=${sha}" - return 0 - fi - local pattern="${REVIEWS_MARKER_PREFIX}${PR_NUMBER} sha=${sha} intent=${intent} status=(applied|no-changes|failed)" - local stale_ids - stale_ids=$(gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments?per_page=100" 2>/dev/null \ - | jq -r --arg pat "$pattern" '[.[] | select(.body | test($pat))] | .[].id' 2>/dev/null || true) - for comment_id in $stale_ids; do - echo "::notice::expire_stale_terminal_markers: deleting stale terminal comment ${comment_id} for intent=${intent} SHA=${sha}" - gh api -X DELETE "repos/${REPO}/issues/comments/${comment_id}" 2>/dev/null || \ - echo "::warning::expire_stale_terminal_markers: failed to delete comment ${comment_id}" >&2 - done -} - -# expire_stale_rate_limited_marker: deletes any existing rate-limited marker for this -# SHA+intent before a new one is posted. Without this, when a hard blocker persists -# past the initial backoff window the dedup check in post_reviews_rate_limited skips -# posting, leaving a marker whose reset_time is already in the past. The retry cron -# then dispatches on every scan indefinitely instead of extending the backoff. -expire_stale_rate_limited_marker() { - local intent="$1" - local sha="${HEAD_SHA:-}" - [ -z "$sha" ] && return 0 - if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then - echo "[dry-run] would expire stale rate-limited marker for intent=${intent} sha=${sha}" - return 0 - fi - local pattern="${REVIEWS_MARKER_PREFIX}${PR_NUMBER} sha=${sha} intent=${intent} status=rate-limited" - local stale_ids - stale_ids=$(gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments?per_page=100" 2>/dev/null \ - | jq -r --arg pat "$pattern" '[.[] | select(.body | test($pat))] | .[].id' 2>/dev/null || true) - for comment_id in $stale_ids; do - echo "::notice::expire_stale_rate_limited_marker: deleting stale rate-limited comment ${comment_id} for intent=${intent} SHA=${sha}" - gh api -X DELETE "repos/${REPO}/issues/comments/${comment_id}" 2>/dev/null || \ - echo "::warning::expire_stale_rate_limited_marker: failed to delete comment ${comment_id}" >&2 - done -} - -# has_reviews_rate_limited_marker: returns 0 if a rate-limited marker for this -# intent+SHA already exists on the PR (dedup check). -has_reviews_rate_limited_marker() { - local intent="$1" - local sha="${HEAD_SHA:-}" - [ -z "$sha" ] && return 1 # no SHA means no dedup possible - local pattern="${REVIEWS_MARKER_PREFIX}${PR_NUMBER} sha=${sha} intent=${intent} status=rate-limited" - local count - count=$(gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments?per_page=100" 2>/dev/null \ - | jq "[.[] | select(.body | test(\"${pattern}\"))] | length" 2>/dev/null \ - || echo "0") - [ "${count:-0}" -gt 0 ] -} - -# post_reviews_rate_limited: posts a rate-limited marker for fix-reviews intents. -# For retryable intents (fix-reviews, review-changes, rebase), the cron will re-dispatch. -# For non-retryable intents (on-mention, fix-bot-comment), asks the user to re-trigger -# since USER_INSTRUCTION/COMMENT_BODY cannot be reconstructed at retry time. -# -# $2 (reason) selects the user-facing wording: -# rate-limit (default) — all AI engines genuinely rate-limited (engine exit 2) -# blocked — engine ran fine but the PR still has hard blockers -# (failing/cancelled checks or CHANGES_REQUESTED reviews); -# schedules a 30-minute backoff retry (issue #461) -# Both reasons post the same machine-readable `status=rate-limited` marker token — -# dev-lead-retry.sh keys its re-dispatch scan on that string — only the visible -# text differs, so users are no longer told "rate-limited" when the real cause -# is PR blockers. -post_reviews_rate_limited() { - local intent="$1" - local reason="${2:-rate-limit}" - - # The blocked path owns its backoff: a fixed 30-minute reset so the retry cron - # backs off instead of re-dispatching immediately. The rate-limit path's reset - # is parsed from engine output (parse_reset_time) before this function is called. - if [ "$reason" = "blocked" ]; then - printf '%s' "$(date -u -d '+30 minutes' +%Y-%m-%dT%H:%M:%SZ 2>/dev/null || true)" > /tmp/dev-lead-rate-limit-reset - fi - - # Expire any stale terminal markers (applied/no-changes/failed) for this SHA+intent - # so the retry cron is not masked by a prior terminal that predates the current blocker. - # Without this, a no-changes terminal from before a reviewer's CHANGES_REQUESTED would - # cause the cron to skip dispatch even though a new rate-limited marker was just posted. - expire_stale_terminal_markers "$intent" - - # Detect whether a prior rate-limited marker exists BEFORE posting the new one. - # Used to suppress duplicate visible ack comments when a persistent blocker keeps - # triggering retries — the user-facing ack is only shown on the first cycle. - local had_prior_rl_marker=false - if [ "${DEV_LEAD_DRY_RUN:-false}" = "false" ]; then - has_reviews_rate_limited_marker "$intent" && had_prior_rl_marker=true - fi - - # Collect IDs of existing rate-limited markers BEFORE posting the new one. The new - # marker is posted first so the old one remains as a safety net if the post fails - # transiently; old markers are only removed after the replacement is confirmed posted. - local stale_rl_ids="" - if [ -n "${HEAD_SHA:-}" ]; then - if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then - echo "[dry-run] would expire stale rate-limited marker for intent=${intent} sha=${HEAD_SHA}" - else - local rl_pattern="${REVIEWS_MARKER_PREFIX}${PR_NUMBER} sha=${HEAD_SHA} intent=${intent} status=rate-limited" - stale_rl_ids=$(gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments?per_page=100" 2>/dev/null \ - | jq -r --arg pat "$rl_pattern" '[.[] | select(.body | test($pat))] | .[].id' 2>/dev/null || true) - fi - fi - - local reset_time - reset_time=$(cat /tmp/dev-lead-rate-limit-reset 2>/dev/null || true) - local reset_detail="" - if [ -n "$reset_time" ]; then - reset_detail=" reset=${reset_time}" - fi - - local sha_detail="" - if [ -n "${HEAD_SHA:-}" ]; then - sha_detail=" sha=${HEAD_SHA}" - fi - - # `reason=` is informational (visible-text selection + marker forensics); the - # retry cron and marker dedup patterns match on `status=rate-limited` and are - # unaffected by the extra field. - local marker="${REVIEWS_MARKER_PREFIX}${PR_NUMBER}${sha_detail} intent=${intent} status=rate-limited reason=${reason}${reset_detail} -->" - - # Retry message depends on the reason and on whether the intent can be - # re-dispatched automatically. - local heading retry_msg - if [ "$reason" = "blocked" ]; then - heading="## Dev-Lead — waiting on PR blockers (intent: ${intent})" - retry_msg="No changes were committed, but the PR still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews). The retry cron will re-attempt automatically." - if [ -n "$reset_time" ]; then - retry_msg="${retry_msg} Next attempt after: \`${reset_time}\`" - fi - else - heading="## Dev-Lead — rate-limited (intent: ${intent})" - case "$intent" in - fix-reviews|review-changes|rebase) - retry_msg="The retry cron will re-attempt automatically." - ;; - on-mention|fix-bot-comment) - retry_msg="Please re-trigger manually (re-mention \`@dev-lead\`) when the rate limit clears — the original request cannot be reconstructed automatically." - ;; - *) - retry_msg="Manual re-trigger may be required." - ;; - esac - if [ -n "$reset_time" ]; then - retry_msg="${retry_msg} Rate limit resets at: \`${reset_time}\`" - fi - fi - - local marker_body="${marker} -${heading} -**PR:** #${PR_NUMBER} -${retry_msg}" - - if [ "$DEV_LEAD_DRY_RUN" = "true" ]; then - echo "[dry-run] would post rate-limited marker for intent=${intent} reason=${reason}" - echo "$marker_body" - else - # Post the new marker FIRST, then remove old marker(s) only after the replacement - # is confirmed. If the post fails transiently, the old marker remains as a safety net - # so the retry cron does not lose track of this SHA+intent. - if gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$marker_body"; then - for _stale_id in $stale_rl_ids; do - echo "::notice::post_reviews_rate_limited: deleting superseded rate-limited marker ${_stale_id} for intent=${intent}" - gh api -X DELETE "repos/${REPO}/issues/comments/${_stale_id}" 2>/dev/null || \ - echo "::warning::post_reviews_rate_limited: failed to delete old rate-limited marker ${_stale_id}" >&2 - done - fi - fi - - # For user-triggered intents, post a separate visible acknowledgment on the first - # rate-limit cycle only. Suppress repeat acks when a persistent blocker keeps the - # backoff interval cycling — the old ack is still visible and a repeat is misleading. - if [ "$had_prior_rl_marker" = "false" ]; then - case "$intent" in - review-changes) - local actor_mention="" - [ -n "${ACTOR:-}" ] && actor_mention="@${ACTOR} " - local reset_display="${reset_time:-unknown}" - local ack_body - if [ "$reason" = "blocked" ]; then - ack_body="> [!NOTE] -> ${actor_mention}I reviewed this PR and no code changes were needed, but it still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews), so I cannot mark it done yet. I'll re-check automatically. -> Next attempt after: \`${reset_display}\`" - else - ack_body=" -> [!NOTE] -> ${actor_mention}I received your request but all AI engines are currently rate-limited. I'll retry automatically once the rate limit clears. -> Rate limit resets at: \`${reset_display}\`" - fi - if [ "$DEV_LEAD_DRY_RUN" = "true" ]; then - echo "[dry-run] would post user-visible ${reason} acknowledgment" - echo "$ack_body" - else - gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$ack_body" - fi - ;; - on-mention) - local actor_mention="" - [ -n "${ACTOR:-}" ] && actor_mention="@${ACTOR} " - local reset_display="${reset_time:-unknown}" - local ack_body=" -> [!NOTE] -> ${actor_mention}I received your request but all AI engines are currently rate-limited. Please re-mention \`@dev-lead\` when the rate limit clears (estimated: \`${reset_display}\`) — I cannot reconstruct the original instruction automatically." - if [ "$DEV_LEAD_DRY_RUN" = "true" ]; then - echo "[dry-run] would post user-visible rate-limit acknowledgment" - echo "$ack_body" - else - gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$ack_body" - fi - ;; - esac - fi -} - -handle_rate_limit() { - local intent="$1" - echo "::warning::All engines rate-limited for intent=${intent} — posting rate-limited marker" - post_reviews_rate_limited "$intent" - [[ -n "${PR_NUMBER:-}" ]] && try_enable_auto_merge - exit 2 -} - -# post_reviews_terminal: writes a terminal status marker after a retryable -# intent completes. This prevents the retry cron from re-dispatching the same -# intent on subsequent runs when the SHA hasn't changed. - - -# post_reviews_terminal: writes a terminal status marker after a retryable -# intent completes. This prevents the retry cron from re-dispatching the same -# intent on subsequent runs when the SHA hasn't changed. - -# commit_and_push: adds all changes, commits with an intent-specific message, -# and pushes to the PR branch. Returns 0 if changes were made and pushed, -# 1 if no changes were found. -detect_conflicting_paths() { - local base="$1" - [[ -z "$base" ]] && return 0 - git merge --no-commit --no-ff "origin/${base}" >/dev/null 2>&1 || true - git diff --name-only --diff-filter=U || true - git merge --abort >/dev/null 2>&1 || true -} - -# expire_stale_terminal_markers: deletes any existing terminal comments (applied, -# no-changes, or failed) for this SHA+intent before a hard-blocker retry marker is -# posted. Without this, the retry cron sees a stale terminal and skips re-dispatch -# even though a new hard blocker (e.g. a CHANGES_REQUESTED review added after the -# prior run) now requires retry. -expire_stale_terminal_markers() { - local intent="$1" - local sha="${HEAD_SHA:-}" - [ -z "$sha" ] && return 0 - if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then - echo "[dry-run] would expire stale terminal markers for intent=${intent} sha=${sha}" - return 0 - fi - local pattern="${REVIEWS_MARKER_PREFIX}${PR_NUMBER} sha=${sha} intent=${intent} status=(applied|no-changes|failed)" - local stale_ids - stale_ids=$(gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments?per_page=100" 2>/dev/null \ - | jq -r --arg pat "$pattern" '[.[] | select(.body | test($pat))] | .[].id' 2>/dev/null || true) - for comment_id in $stale_ids; do - echo "::notice::expire_stale_terminal_markers: deleting stale terminal comment ${comment_id} for intent=${intent} SHA=${sha}" - gh api -X DELETE "repos/${REPO}/issues/comments/${comment_id}" 2>/dev/null || \ - echo "::warning::expire_stale_terminal_markers: failed to delete comment ${comment_id}" >&2 - done -} - -# expire_stale_rate_limited_marker: deletes any existing rate-limited marker for this -# SHA+intent before a new one is posted. Without this, when a hard blocker persists -# past the initial backoff window the dedup check in post_reviews_rate_limited skips -# posting, leaving a marker whose reset_time is already in the past. The retry cron -# then dispatches on every scan indefinitely instead of extending the backoff. -expire_stale_rate_limited_marker() { - local intent="$1" - local sha="${HEAD_SHA:-}" - [ -z "$sha" ] && return 0 - if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then - echo "[dry-run] would expire stale rate-limited marker for intent=${intent} sha=${sha}" - return 0 - fi - local pattern="${REVIEWS_MARKER_PREFIX}${PR_NUMBER} sha=${sha} intent=${intent} status=rate-limited" - local stale_ids - stale_ids=$(gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments?per_page=100" 2>/dev/null \ - | jq -r --arg pat "$pattern" '[.[] | select(.body | test($pat))] | .[].id' 2>/dev/null || true) - for comment_id in $stale_ids; do - echo "::notice::expire_stale_rate_limited_marker: deleting stale rate-limited comment ${comment_id} for intent=${intent} SHA=${sha}" - gh api -X DELETE "repos/${REPO}/issues/comments/${comment_id}" 2>/dev/null || \ - echo "::warning::expire_stale_rate_limited_marker: failed to delete comment ${comment_id}" >&2 - done -} - -# has_reviews_rate_limited_marker: returns 0 if a rate-limited marker for this -# intent+SHA already exists on the PR (dedup check). - -# post_reviews_rate_limited: posts a rate-limited marker for fix-reviews intents. -# For retryable intents (fix-reviews, review-changes, rebase), the cron will re-dispatch. -# For non-retryable intents (on-mention, fix-bot-comment), asks the user to re-trigger -# since USER_INSTRUCTION/COMMENT_BODY cannot be reconstructed at retry time. -# -# $2 (reason) selects the user-facing wording: -# rate-limit (default) — all AI engines genuinely rate-limited (engine exit 2) -# blocked — engine ran fine but the PR still has hard blockers -# (failing/cancelled checks or CHANGES_REQUESTED reviews); -# schedules a 30-minute backoff retry (issue #461) -# Both reasons post the same machine-readable `status=rate-limited` marker token — -# dev-lead-retry.sh keys its re-dispatch scan on that string — only the visible -# text differs, so users are no longer told "rate-limited" when the real cause -# is PR blockers. -post_reviews_rate_limited() { - local intent="$1" - local reason="${2:-rate-limit}" - - # The blocked path owns its backoff: a fixed 30-minute reset so the retry cron - # backs off instead of re-dispatching immediately. The rate-limit path's reset - # is parsed from engine output (parse_reset_time) before this function is called. - if [ "$reason" = "blocked" ]; then - printf '%s' "$(date -u -d '+30 minutes' +%Y-%m-%dT%H:%M:%SZ 2>/dev/null || true)" > /tmp/dev-lead-rate-limit-reset - fi - - # Expire any stale terminal markers (applied/no-changes/failed) for this SHA+intent - # so the retry cron is not masked by a prior terminal that predates the current blocker. - # Without this, a no-changes terminal from before a reviewer's CHANGES_REQUESTED would - # cause the cron to skip dispatch even though a new rate-limited marker was just posted. - expire_stale_terminal_markers "$intent" - - # Detect whether a prior rate-limited marker exists BEFORE posting the new one. - # Used to suppress duplicate visible ack comments when a persistent blocker keeps - # triggering retries — the user-facing ack is only shown on the first cycle. - local had_prior_rl_marker=false - if [ "${DEV_LEAD_DRY_RUN:-false}" = "false" ]; then - has_reviews_rate_limited_marker "$intent" && had_prior_rl_marker=true - fi - - # Collect IDs of existing rate-limited markers BEFORE posting the new one. The new - # marker is posted first so the old one remains as a safety net if the post fails - # transiently; old markers are only removed after the replacement is confirmed posted. - local stale_rl_ids="" - if [ -n "${HEAD_SHA:-}" ]; then - if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then - echo "[dry-run] would expire stale rate-limited marker for intent=${intent} sha=${HEAD_SHA}" - else - local rl_pattern="${REVIEWS_MARKER_PREFIX}${PR_NUMBER} sha=${HEAD_SHA} intent=${intent} status=rate-limited" - stale_rl_ids=$(gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments?per_page=100" 2>/dev/null \ - | jq -r --arg pat "$rl_pattern" '[.[] | select(.body | test($pat))] | .[].id' 2>/dev/null || true) - fi - fi - - local reset_time - reset_time=$(cat /tmp/dev-lead-rate-limit-reset 2>/dev/null || true) - local reset_detail="" - if [ -n "$reset_time" ]; then - reset_detail=" reset=${reset_time}" - fi - - local sha_detail="" - if [ -n "${HEAD_SHA:-}" ]; then - sha_detail=" sha=${HEAD_SHA}" - fi - - # `reason=` is informational (visible-text selection + marker forensics); the - # retry cron and marker dedup patterns match on `status=rate-limited` and are - # unaffected by the extra field. - local marker="${REVIEWS_MARKER_PREFIX}${PR_NUMBER}${sha_detail} intent=${intent} status=rate-limited reason=${reason}${reset_detail} -->" - - # Retry message depends on the reason and on whether the intent can be - # re-dispatched automatically. - local heading retry_msg - if [ "$reason" = "blocked" ]; then - heading="## Dev-Lead — waiting on PR blockers (intent: ${intent})" - retry_msg="No changes were committed, but the PR still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews). The retry cron will re-attempt automatically." - if [ -n "$reset_time" ]; then - retry_msg="${retry_msg} Next attempt after: \`${reset_time}\`" - fi - else - heading="## Dev-Lead — rate-limited (intent: ${intent})" - case "$intent" in - fix-reviews|review-changes|rebase) - retry_msg="The retry cron will re-attempt automatically." - ;; - on-mention|fix-bot-comment) - retry_msg="Please re-trigger manually (re-mention \`@dev-lead\`) when the rate limit clears — the original request cannot be reconstructed automatically." - ;; - *) - retry_msg="Manual re-trigger may be required." - ;; - esac - if [ -n "$reset_time" ]; then - retry_msg="${retry_msg} Rate limit resets at: \`${reset_time}\`" - fi - fi - - local marker_body="${marker} -${heading} -**PR:** #${PR_NUMBER} -${retry_msg}" - - if [ "$DEV_LEAD_DRY_RUN" = "true" ]; then - echo "[dry-run] would post rate-limited marker for intent=${intent} reason=${reason}" - echo "$marker_body" - else - # Post the new marker FIRST, then remove old marker(s) only after the replacement - # is confirmed. If the post fails transiently, the old marker remains as a safety net - # so the retry cron does not lose track of this SHA+intent. - if gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$marker_body"; then - for _stale_id in $stale_rl_ids; do - echo "::notice::post_reviews_rate_limited: deleting superseded rate-limited marker ${_stale_id} for intent=${intent}" - gh api -X DELETE "repos/${REPO}/issues/comments/${_stale_id}" 2>/dev/null || \ - echo "::warning::post_reviews_rate_limited: failed to delete old rate-limited marker ${_stale_id}" >&2 - done - fi - fi - - # For user-triggered intents, post a separate visible acknowledgment on the first - # rate-limit cycle only. Suppress repeat acks when a persistent blocker keeps the - # backoff interval cycling — the old ack is still visible and a repeat is misleading. - if [ "$had_prior_rl_marker" = "false" ]; then - case "$intent" in - review-changes) - local actor_mention="" - [ -n "${ACTOR:-}" ] && actor_mention="@${ACTOR} " - local reset_display="${reset_time:-unknown}" - local ack_body - if [ "$reason" = "blocked" ]; then - ack_body=" -> [!NOTE] -> ${actor_mention}I reviewed this PR and no code changes were needed, but it still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews), so I cannot mark it done yet. I'll re-check automatically. -> Next attempt after: \`${reset_display}\`" - else - ack_body=" -> [!NOTE] -> ${actor_mention}I received your request but all AI engines are currently rate-limited. I'll retry automatically once the rate limit clears. -> Rate limit resets at: \`${reset_display}\`" - fi - if [ "$DEV_LEAD_DRY_RUN" = "true" ]; then - echo "[dry-run] would post user-visible ${reason} acknowledgment" - echo "$ack_body" - else - gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$ack_body" - fi - ;; - on-mention) - local actor_mention="" - [ -n "${ACTOR:-}" ] && actor_mention="@${ACTOR} " - local reset_display="${reset_time:-unknown}" - local ack_body=" -> [!NOTE] -> ${actor_mention}I received your request but all AI engines are currently rate-limited. Please re-mention \`@dev-lead\` when the rate limit clears (estimated: \`${reset_display}\`) — I cannot reconstruct the original instruction automatically." - if [ "$DEV_LEAD_DRY_RUN" = "true" ]; then - echo "[dry-run] would post user-visible rate-limit acknowledgment" - echo "$ack_body" - else - gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$ack_body" - fi - ;; - esac - fi -} - -handle_rate_limit() { - local intent="$1" - echo "::warning::All engines rate-limited for intent=${intent} — posting rate-limited marker" - post_reviews_rate_limited "$intent" - [[ -n "${PR_NUMBER:-}" ]] && try_enable_auto_merge - exit 2 -} - -# post_reviews_terminal: writes a terminal status marker after a retryable -# intent completes. This prevents the retry cron from re-dispatching the same -# intent on subsequent runs when the SHA hasn't changed. - -# Marker for the no-op guard flag comment (#1340), deduped per PR+intent. -NOOP_MARKER_PREFIX="" - if gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments?per_page=100" 2>/dev/null \ - | jq -r '.[].body // ""' 2>/dev/null | grep -qF "$marker"; then - echo "::notice::PR #${PR_NUMBER} already flagged as net-zero for intent=${intent} — not reposting" - else - gh pr comment "$PR_NUMBER" --repo "$REPO" --body "${marker} -## No-op fix detected — human attention needed - -The \`${intent}\` pass reverted this PR's own changes, so its net diff against \`${BASE_REF:-main}\` is now **empty** (zero changed files). Merging a PR that nets to zero would auto-close its \`Closes #N\` compliance issue while the underlying finding remains unfixed (#1340), and the idempotent audit would immediately re-open it. - -Auto-merge has been disabled and no commit was pushed. A human should restore the correct fix or close this PR." \ - || echo "::warning::could not post no-op flag comment on PR #${PR_NUMBER}" - fi - gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label "${NEEDS_HUMAN_REVIEW_LABEL:-needs-human-review}" 2>/dev/null \ - || echo "::warning::could not add ${NEEDS_HUMAN_REVIEW_LABEL:-needs-human-review} label on PR #${PR_NUMBER}" - gh pr merge "$PR_NUMBER" --repo "$REPO" --disable-auto 2>/dev/null \ - || echo "::notice::auto-merge was not enabled on PR #${PR_NUMBER} (nothing to disable)" - return 0 -} - -# commit_and_push: adds all changes, commits with an intent-specific message, -# and pushes to the PR branch. Returns 0 if changes were made and pushed, -# 1 if no changes were found, 3 if the no-op guard aborted the push (#1340). - -# expire_stale_terminal_markers: deletes any existing terminal comments (applied, -# no-changes, or failed) for this SHA+intent before a hard-blocker retry marker is -# posted. Without this, the retry cron sees a stale terminal and skips re-dispatch -# even though a new hard blocker (e.g. a CHANGES_REQUESTED review added after the -# prior run) now requires retry. -expire_stale_terminal_markers() { - local intent="$1" - local sha="${HEAD_SHA:-}" - [ -z "$sha" ] && return 0 - if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then - echo "[dry-run] would expire stale terminal markers for intent=${intent} sha=${sha}" - return 0 - fi - local pattern="${REVIEWS_MARKER_PREFIX}${PR_NUMBER} sha=${sha} intent=${intent} status=(applied|no-changes|failed)" - local stale_ids - stale_ids=$(gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments?per_page=100" 2>/dev/null \ - | jq -r --arg pat "$pattern" '[.[] | select(.body | test($pat))] | .[].id' 2>/dev/null || true) - for comment_id in $stale_ids; do - echo "::notice::expire_stale_terminal_markers: deleting stale terminal comment ${comment_id} for intent=${intent} SHA=${sha}" - gh api -X DELETE "repos/${REPO}/issues/comments/${comment_id}" 2>/dev/null || \ - echo "::warning::expire_stale_terminal_markers: failed to delete comment ${comment_id}" >&2 - done -} - -# expire_stale_rate_limited_marker: deletes any existing rate-limited marker for this -# SHA+intent before a new one is posted. Without this, when a hard blocker persists -# past the initial backoff window the dedup check in post_reviews_rate_limited skips -# posting, leaving a marker whose reset_time is already in the past. The retry cron -# then dispatches on every scan indefinitely instead of extending the backoff. -expire_stale_rate_limited_marker() { - local intent="$1" - local sha="${HEAD_SHA:-}" - [ -z "$sha" ] && return 0 - if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then - echo "[dry-run] would expire stale rate-limited marker for intent=${intent} sha=${sha}" - return 0 - fi - local pattern="${REVIEWS_MARKER_PREFIX}${PR_NUMBER} sha=${sha} intent=${intent} status=rate-limited" - local stale_ids - stale_ids=$(gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments?per_page=100" 2>/dev/null \ - | jq -r --arg pat "$pattern" '[.[] | select(.body | test($pat))] | .[].id' 2>/dev/null || true) - for comment_id in $stale_ids; do - echo "::notice::expire_stale_rate_limited_marker: deleting stale rate-limited comment ${comment_id} for intent=${intent} SHA=${sha}" - gh api -X DELETE "repos/${REPO}/issues/comments/${comment_id}" 2>/dev/null || \ - echo "::warning::expire_stale_rate_limited_marker: failed to delete comment ${comment_id}" >&2 - done -} - -# has_reviews_rate_limited_marker: returns 0 if a rate-limited marker for this -# intent+SHA already exists on the PR (dedup check). - -# post_reviews_rate_limited: posts a rate-limited marker for fix-reviews intents. -# For retryable intents (fix-reviews, review-changes, rebase), the cron will re-dispatch. -# For non-retryable intents (on-mention, fix-bot-comment), asks the user to re-trigger -# since USER_INSTRUCTION/COMMENT_BODY cannot be reconstructed at retry time. -# -# $2 (reason) selects the user-facing wording: -# rate-limit (default) — all AI engines genuinely rate-limited (engine exit 2) -# blocked — engine ran fine but the PR still has hard blockers -# (failing/cancelled checks or CHANGES_REQUESTED reviews); -# schedules a 30-minute backoff retry (issue #461) -# Both reasons post the same machine-readable `status=rate-limited` marker token — -# dev-lead-retry.sh keys its re-dispatch scan on that string — only the visible -# text differs, so users are no longer told "rate-limited" when the real cause -# is PR blockers. -post_reviews_rate_limited() { - local intent="$1" - local reason="${2:-rate-limit}" - - # The blocked path owns its backoff: a fixed 30-minute reset so the retry cron - # backs off instead of re-dispatching immediately. The rate-limit path's reset - # is parsed from engine output (parse_reset_time) before this function is called. - if [ "$reason" = "blocked" ]; then - printf '%s' "$(date -u -d '+30 minutes' +%Y-%m-%dT%H:%M:%SZ 2>/dev/null || true)" > /tmp/dev-lead-rate-limit-reset - fi - - # Expire any stale terminal markers (applied/no-changes/failed) for this SHA+intent - # so the retry cron is not masked by a prior terminal that predates the current blocker. - # Without this, a no-changes terminal from before a reviewer's CHANGES_REQUESTED would - # cause the cron to skip dispatch even though a new rate-limited marker was just posted. - expire_stale_terminal_markers "$intent" - - # Detect whether a prior rate-limited marker exists BEFORE posting the new one. - # Used to suppress duplicate visible ack comments when a persistent blocker keeps - # triggering retries — the user-facing ack is only shown on the first cycle. - local had_prior_rl_marker=false - if [ "${DEV_LEAD_DRY_RUN:-false}" = "false" ]; then - has_reviews_rate_limited_marker "$intent" && had_prior_rl_marker=true - fi - - # Collect IDs of existing rate-limited markers BEFORE posting the new one. The new - # marker is posted first so the old one remains as a safety net if the post fails - # transiently; old markers are only removed after the replacement is confirmed posted. - local stale_rl_ids="" - if [ -n "${HEAD_SHA:-}" ]; then - if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then - echo "[dry-run] would expire stale rate-limited marker for intent=${intent} sha=${HEAD_SHA}" - else - local rl_pattern="${REVIEWS_MARKER_PREFIX}${PR_NUMBER} sha=${HEAD_SHA} intent=${intent} status=rate-limited" - stale_rl_ids=$(gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments?per_page=100" 2>/dev/null \ - | jq -r --arg pat "$rl_pattern" '[.[] | select(.body | test($pat))] | .[].id' 2>/dev/null || true) - fi - fi - - local reset_time - reset_time=$(cat /tmp/dev-lead-rate-limit-reset 2>/dev/null || true) - local reset_detail="" - if [ -n "$reset_time" ]; then - reset_detail=" reset=${reset_time}" - fi - - local sha_detail="" - if [ -n "${HEAD_SHA:-}" ]; then - sha_detail=" sha=${HEAD_SHA}" - fi - - # `reason=` is informational (visible-text selection + marker forensics); the - # retry cron and marker dedup patterns match on `status=rate-limited` and are - # unaffected by the extra field. - local marker="${REVIEWS_MARKER_PREFIX}${PR_NUMBER}${sha_detail} intent=${intent} status=rate-limited reason=${reason}${reset_detail} -->" - - # Retry message depends on the reason and on whether the intent can be - # re-dispatched automatically. - local heading retry_msg - if [ "$reason" = "blocked" ]; then - heading="## Dev-Lead — waiting on PR blockers (intent: ${intent})" - retry_msg="No changes were committed, but the PR still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews). The retry cron will re-attempt automatically." - if [ -n "$reset_time" ]; then - retry_msg="${retry_msg} Next attempt after: \`${reset_time}\`" - fi - else - heading="## Dev-Lead — rate-limited (intent: ${intent})" - case "$intent" in - fix-reviews|review-changes|rebase) - retry_msg="The retry cron will re-attempt automatically." - ;; - on-mention|fix-bot-comment) - retry_msg="Please re-trigger manually (re-mention \`@dev-lead\`) when the rate limit clears — the original request cannot be reconstructed automatically." - ;; - *) - retry_msg="Manual re-trigger may be required." - ;; - esac - if [ -n "$reset_time" ]; then - retry_msg="${retry_msg} Rate limit resets at: \`${reset_time}\`" - fi - fi - - local marker_body="${marker} -${heading} -**PR:** #${PR_NUMBER} -${retry_msg}" - - if [ "$DEV_LEAD_DRY_RUN" = "true" ]; then - echo "[dry-run] would post rate-limited marker for intent=${intent} reason=${reason}" - echo "$marker_body" - else - # Post the new marker FIRST, then remove old marker(s) only after the replacement - # is confirmed. If the post fails transiently, the old marker remains as a safety net - # so the retry cron does not lose track of this SHA+intent. - if gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$marker_body"; then - for _stale_id in $stale_rl_ids; do - echo "::notice::post_reviews_rate_limited: deleting superseded rate-limited marker ${_stale_id} for intent=${intent}" - gh api -X DELETE "repos/${REPO}/issues/comments/${_stale_id}" 2>/dev/null || \ - echo "::warning::post_reviews_rate_limited: failed to delete old rate-limited marker ${_stale_id}" >&2 - done - fi - fi - - # For user-triggered intents, post a separate visible acknowledgment on the first - # rate-limit cycle only. Suppress repeat acks when a persistent blocker keeps the - # backoff interval cycling — the old ack is still visible and a repeat is misleading. - if [ "$had_prior_rl_marker" = "false" ]; then - case "$intent" in - review-changes) - local actor_mention="" - [ -n "${ACTOR:-}" ] && actor_mention="@${ACTOR} " - local reset_display="${reset_time:-unknown}" - local ack_body - if [ "$reason" = "blocked" ]; then - ack_body=" -> [!NOTE] -> ${actor_mention}I reviewed this PR and no code changes were needed, but it still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews), so I cannot mark it done yet. I'll re-check automatically. -> Next attempt after: \`${reset_display}\`" - else - ack_body=" -> [!NOTE] -> ${actor_mention}I received your request but all AI engines are currently rate-limited. I'll retry automatically once the rate limit clears. -> Rate limit resets at: \`${reset_display}\`" - fi - if [ "$DEV_LEAD_DRY_RUN" = "true" ]; then - echo "[dry-run] would post user-visible ${reason} acknowledgment" - echo "$ack_body" - else - gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$ack_body" - fi - ;; - on-mention) - local actor_mention="" - [ -n "${ACTOR:-}" ] && actor_mention="@${ACTOR} " - local reset_display="${reset_time:-unknown}" - local ack_body=" -> [!NOTE] -> ${actor_mention}I received your request but all AI engines are currently rate-limited. Please re-mention \`@dev-lead\` when the rate limit clears (estimated: \`${reset_display}\`) — I cannot reconstruct the original instruction automatically." - if [ "$DEV_LEAD_DRY_RUN" = "true" ]; then - echo "[dry-run] would post user-visible rate-limit acknowledgment" - echo "$ack_body" - else - gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$ack_body" - fi - ;; - esac - fi -} - -handle_rate_limit() { - local intent="$1" - echo "::warning::All engines rate-limited for intent=${intent} — posting rate-limited marker" - post_reviews_rate_limited "$intent" - [[ -n "${PR_NUMBER:-}" ]] && try_enable_auto_merge - exit 2 -} - -# post_reviews_terminal: writes a terminal status marker after a retryable -# intent completes. This prevents the retry cron from re-dispatching the same -# intent on subsequent runs when the SHA hasn't changed. - -# Marker for the no-op guard flag comment (#1340), deduped per PR+intent. -NOOP_MARKER_PREFIX="" - if gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments?per_page=100" 2>/dev/null \ - | jq -r '.[].body // ""' 2>/dev/null | grep -qF "$marker"; then - echo "::notice::PR #${PR_NUMBER} already flagged as net-zero for intent=${intent} — not reposting" - else - gh pr comment "$PR_NUMBER" --repo "$REPO" --body "${marker} -## No-op fix detected — human attention needed - -The \`${intent}\` pass reverted this PR's own changes, so its net diff against \`${BASE_REF:-main}\` is now **empty** (zero changed files). Merging a PR that nets to zero would auto-close its \`Closes #N\` compliance issue while the underlying finding remains unfixed (#1340), and the idempotent audit would immediately re-open it. - -Auto-merge has been disabled and no commit was pushed. A human should restore the correct fix or close this PR." \ - || echo "::warning::could not post no-op flag comment on PR #${PR_NUMBER}" - fi - gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label "${NEEDS_HUMAN_REVIEW_LABEL:-needs-human-review}" 2>/dev/null \ - || echo "::warning::could not add ${NEEDS_HUMAN_REVIEW_LABEL:-needs-human-review} label on PR #${PR_NUMBER}" - gh pr merge "$PR_NUMBER" --repo "$REPO" --disable-auto 2>/dev/null \ - || echo "::notice::auto-merge was not enabled on PR #${PR_NUMBER} (nothing to disable)" - return 0 -} - -# commit_and_push: adds all changes, commits with an intent-specific message, -# and pushes to the PR branch. Returns 0 if changes were made and pushed, -# 1 if no changes were found, 3 if the no-op guard aborted the push (#1340). -commit_and_push() { - local intent="$1" - local has_uncommitted=false has_unpushed=false - - # git status --porcelain covers untracked files that git diff misses - [ -n "$(git status --porcelain 2>/dev/null)" ] && has_uncommitted=true - - # Detect engine-committed but not pushed: prefer @{u} if upstream is configured, - # fall back to HEAD_SHA (resolved from PR API at script startup) for fork checkouts. - local upstream - upstream=$(git rev-parse --abbrev-ref --symbolic-full-name "@{u}" 2>/dev/null || true) - if [ -n "$upstream" ]; then - git log "${upstream}..HEAD" --oneline 2>/dev/null | grep -q . && has_unpushed=true - elif [ -n "${HEAD_SHA:-}" ]; then - git log "${HEAD_SHA}..HEAD" --oneline 2>/dev/null | grep -q . && has_unpushed=true - fi - - if ! $has_uncommitted && ! $has_unpushed; then - echo "::notice::No changes to commit for intent=${intent}" - return 1 - fi - - local commit_msg - case "$intent" in - fix-reviews) commit_msg="fix(reviews): address review comments [skip ci-relay]" ;; - fix-bot-comment) commit_msg="fix(bot): address bot feedback [skip ci-relay]" ;; - human|human-pr) commit_msg="chore: apply manual instructions [skip ci-relay]" ;; - rebase) commit_msg="chore: resolve rebase conflicts [skip ci-relay]" ;; - *) commit_msg="chore: dev-lead update (${intent}) [skip ci-relay]" ;; - esac - - if [ "$DEV_LEAD_DRY_RUN" = "true" ]; then - if $has_uncommitted; then - echo "[dry-run] would git add -A, commit '${commit_msg}', and push" - else - echo "[dry-run] engine already committed — would push existing commit(s) without re-committing" - fi - else - if $has_uncommitted; then - git add -A - # Ensure git identity is set — actions/checkout only sets local config for the - # repo it checks out (.github-private), not for target repos cloned separately. - setup_git_identity - # Explicit exit on failure: set -e is suspended when commit_and_push is called from - # an if-statement condition, so git commit failures would be silently swallowed - # otherwise. Using exit (not return) ensures CI fails visibly instead of posting a - # false "Changes committed and pushed" comment. - git commit -m "$commit_msg" || { echo "::error::git commit failed — check git identity configuration on the runner" >&2; exit 1; } - fi - # No-op guard (#1340): a fix pass that reverts the PR's own changes nets the - # base…head diff to zero. Pushing it would let a `Closes #N` PR auto-close its - # compliance issue while the finding remains unfixed. Abort the push and flag - # for a human instead of self-cancelling the fix. - case "$intent" in - fix-reviews|fix-bot-comment) - if pr_nets_to_zero "${BASE_REF:-main}"; then - echo "::error::No-op guard: PR #${PR_NUMBER} nets to zero changed files against ${BASE_REF:-main} after ${intent} — refusing to push a self-cancelling fix (#1340)" - flag_noop_pr "$intent" - return 3 - fi - ;; - esac - # No-clobber push (#1311): never discard a concurrent writer's unseen commit. - # push_no_clobber fast-forwards normally and only ever force-with-leases a - # rewritten branch, aborting if the remote moved beyond what we fetched. - push_no_clobber || { - echo "::error::git push failed — check remote access and branch permissions" >&2 - exit 1 - } - fi - return 0 -} - -# expire_stale_terminal_markers: deletes any existing terminal comments (applied, -# no-changes, or failed) for this SHA+intent before a hard-blocker retry marker is -# posted. Without this, the retry cron sees a stale terminal and skips re-dispatch -# even though a new hard blocker (e.g. a CHANGES_REQUESTED review added after the -# prior run) now requires retry. -expire_stale_terminal_markers() { - local intent="$1" - local sha="${HEAD_SHA:-}" - [ -z "$sha" ] && return 0 - if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then - echo "[dry-run] would expire stale terminal markers for intent=${intent} sha=${sha}" - return 0 - fi - local pattern="${REVIEWS_MARKER_PREFIX}${PR_NUMBER} sha=${sha} intent=${intent} status=(applied|no-changes|failed)" - local stale_ids - stale_ids=$(gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments?per_page=100" 2>/dev/null \ - | jq -r --arg pat "$pattern" '[.[] | select(.body | test($pat))] | .[].id' 2>/dev/null || true) - for comment_id in $stale_ids; do - echo "::notice::expire_stale_terminal_markers: deleting stale terminal comment ${comment_id} for intent=${intent} SHA=${sha}" - gh api -X DELETE "repos/${REPO}/issues/comments/${comment_id}" 2>/dev/null || \ - echo "::warning::expire_stale_terminal_markers: failed to delete comment ${comment_id}" >&2 - done -} - -# expire_stale_rate_limited_marker: deletes any existing rate-limited marker for this -# SHA+intent before a new one is posted. Without this, when a hard blocker persists -# past the initial backoff window the dedup check in post_reviews_rate_limited skips -# posting, leaving a marker whose reset_time is already in the past. The retry cron -# then dispatches on every scan indefinitely instead of extending the backoff. -expire_stale_rate_limited_marker() { - local intent="$1" - local sha="${HEAD_SHA:-}" - [ -z "$sha" ] && return 0 - if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then - echo "[dry-run] would expire stale rate-limited marker for intent=${intent} sha=${sha}" - return 0 - fi - local pattern="${REVIEWS_MARKER_PREFIX}${PR_NUMBER} sha=${sha} intent=${intent} status=rate-limited" - local stale_ids - stale_ids=$(gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments?per_page=100" 2>/dev/null \ - | jq -r --arg pat "$pattern" '[.[] | select(.body | test($pat))] | .[].id' 2>/dev/null || true) - for comment_id in $stale_ids; do - echo "::notice::expire_stale_rate_limited_marker: deleting stale rate-limited comment ${comment_id} for intent=${intent} SHA=${sha}" - gh api -X DELETE "repos/${REPO}/issues/comments/${comment_id}" 2>/dev/null || \ - echo "::warning::expire_stale_rate_limited_marker: failed to delete comment ${comment_id}" >&2 - done -} - -# has_reviews_rate_limited_marker: returns 0 if a rate-limited marker for this -# intent+SHA already exists on the PR (dedup check). -has_reviews_rate_limited_marker() { - local intent="$1" - local sha="${HEAD_SHA:-}" - [ -z "$sha" ] && return 1 # no SHA means no dedup possible - local pattern="${REVIEWS_MARKER_PREFIX}${PR_NUMBER} sha=${sha} intent=${intent} status=rate-limited" - local count - count=$(gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments?per_page=100" 2>/dev/null \ - | jq --arg pattern "$pattern" "[.[] | select(.body | test(\$pattern))] | length" 2>/dev/null \ - || echo "0") - [ "${count:-0}" -gt 0 ] -} - -# post_reviews_rate_limited: posts a rate-limited marker for fix-reviews intents. -# For retryable intents (fix-reviews, review-changes, rebase), the cron will re-dispatch. -# For non-retryable intents (on-mention, fix-bot-comment), asks the user to re-trigger -# since USER_INSTRUCTION/COMMENT_BODY cannot be reconstructed at retry time. -# -# $2 (reason) selects the user-facing wording: -# rate-limit (default) — all AI engines genuinely rate-limited (engine exit 2) -# blocked — engine ran fine but the PR still has hard blockers -# (failing/cancelled checks or CHANGES_REQUESTED reviews); -# schedules a 30-minute backoff retry (issue #461) -# Both reasons post the same machine-readable `status=rate-limited` marker token — -# dev-lead-retry.sh keys its re-dispatch scan on that string — only the visible -# text differs, so users are no longer told "rate-limited" when the real cause -# is PR blockers. -post_reviews_rate_limited() { - local intent="$1" - local reason="${2:-rate-limit}" - - # The blocked path owns its backoff: a fixed 30-minute reset so the retry cron - # backs off instead of re-dispatching immediately. The rate-limit path's reset - # is parsed from engine output (parse_reset_time) before this function is called. - if [ "$reason" = "blocked" ]; then - printf '%s' "$(date -u -d '+30 minutes' +%Y-%m-%dT%H:%M:%SZ 2>/dev/null || true)" > /tmp/dev-lead-rate-limit-reset - fi - - # Expire any stale terminal markers (applied/no-changes/failed) for this SHA+intent - # so the retry cron is not masked by a prior terminal that predates the current blocker. - # Without this, a no-changes terminal from before a reviewer's CHANGES_REQUESTED would - # cause the cron to skip dispatch even though a new rate-limited marker was just posted. - expire_stale_terminal_markers "$intent" - - # Detect whether a prior rate-limited marker exists BEFORE posting the new one. - # Used to suppress duplicate visible ack comments when a persistent blocker keeps - # triggering retries — the user-facing ack is only shown on the first cycle. - local had_prior_rl_marker=false - if [ "${DEV_LEAD_DRY_RUN:-false}" = "false" ]; then - has_reviews_rate_limited_marker "$intent" && had_prior_rl_marker=true - fi - - # Collect IDs of existing rate-limited markers BEFORE posting the new one. The new - # marker is posted first so the old one remains as a safety net if the post fails - # transiently; old markers are only removed after the replacement is confirmed posted. - local stale_rl_ids="" - if [ -n "${HEAD_SHA:-}" ]; then - if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then - echo "[dry-run] would expire stale rate-limited marker for intent=${intent} sha=${HEAD_SHA}" - else - local rl_pattern="${REVIEWS_MARKER_PREFIX}${PR_NUMBER} sha=${HEAD_SHA} intent=${intent} status=rate-limited" - stale_rl_ids=$(gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments?per_page=100" 2>/dev/null \ - | jq -r --arg pat "$rl_pattern" '[.[] | select(.body | test($pat))] | .[].id' 2>/dev/null || true) - fi - fi - - local reset_time - reset_time=$(cat /tmp/dev-lead-rate-limit-reset 2>/dev/null || true) - local reset_detail="" - if [ -n "$reset_time" ]; then - reset_detail=" reset=${reset_time}" - fi - - local sha_detail="" - if [ -n "${HEAD_SHA:-}" ]; then - sha_detail=" sha=${HEAD_SHA}" - fi - - # `reason=` is informational (visible-text selection + marker forensics); the - # retry cron and marker dedup patterns match on `status=rate-limited` and are - # unaffected by the extra field. - local marker="${REVIEWS_MARKER_PREFIX}${PR_NUMBER}${sha_detail} intent=${intent} status=rate-limited reason=${reason}${reset_detail} -->" - - # Retry message depends on the reason and on whether the intent can be - # re-dispatched automatically. - local heading retry_msg - if [ "$reason" = "blocked" ]; then - heading="## Dev-Lead — waiting on PR blockers (intent: ${intent})" - retry_msg="No changes were committed, but the PR still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews). The retry cron will re-attempt automatically." - if [ -n "$reset_time" ]; then - retry_msg="${retry_msg} Next attempt after: \`${reset_time}\`" - fi - else - heading="## Dev-Lead — rate-limited (intent: ${intent})" - case "$intent" in - fix-reviews|review-changes|rebase) - retry_msg="The retry cron will re-attempt automatically." - ;; - on-mention|fix-bot-comment) - retry_msg="Please re-trigger manually (re-mention \`@dev-lead\`) when the rate limit clears — the original request cannot be reconstructed automatically." - ;; - *) - retry_msg="Manual re-trigger may be required." - ;; - esac - if [ -n "$reset_time" ]; then - retry_msg="${retry_msg} Rate limit resets at: \`${reset_time}\`" - fi - fi - - local marker_body="${marker} -${heading} -**PR:** #${PR_NUMBER} -${retry_msg}" - - if [ "$DEV_LEAD_DRY_RUN" = "true" ]; then - echo "[dry-run] would post rate-limited marker for intent=${intent} reason=${reason}" - echo "$marker_body" - else - # Post the new marker FIRST, then remove old marker(s) only after the replacement - # is confirmed. If the post fails transiently, the old marker remains as a safety net - # so the retry cron does not lose track of this SHA+intent. - if gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$marker_body"; then - for _stale_id in $stale_rl_ids; do - echo "::notice::post_reviews_rate_limited: deleting superseded rate-limited marker ${_stale_id} for intent=${intent}" - gh api -X DELETE "repos/${REPO}/issues/comments/${_stale_id}" 2>/dev/null || \ - echo "::warning::post_reviews_rate_limited: failed to delete old rate-limited marker ${_stale_id}" >&2 - done - fi - fi - - # For user-triggered intents, post a separate visible acknowledgment on the first - # rate-limit cycle only. Suppress repeat acks when a persistent blocker keeps the - # backoff interval cycling — the old ack is still visible and a repeat is misleading. - if [ "$had_prior_rl_marker" = "false" ]; then - case "$intent" in - review-changes) - local actor_mention="" - [ -n "${ACTOR:-}" ] && actor_mention="@${ACTOR} " - local reset_display="${reset_time:-unknown}" - local ack_body - if [ "$reason" = "blocked" ]; then - ack_body=" -> [!NOTE] -> ${actor_mention}I reviewed this PR and no code changes were needed, but it still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews), so I cannot mark it done yet. I'll re-check automatically. -> Next attempt after: \`${reset_display}\`" - else - ack_body=" -> [!NOTE] -> ${actor_mention}I received your request but all AI engines are currently rate-limited. I'll retry automatically once the rate limit clears. -> Rate limit resets at: \`${reset_display}\`" - fi - if [ "$DEV_LEAD_DRY_RUN" = "true" ]; then - echo "[dry-run] would post user-visible ${reason} acknowledgment" - echo "$ack_body" - else - gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$ack_body" - fi - ;; - on-mention) - local actor_mention="" - [ -n "${ACTOR:-}" ] && actor_mention="@${ACTOR} " - local reset_display="${reset_time:-unknown}" - local ack_body=" -> [!NOTE] -> ${actor_mention}I received your request but all AI engines are currently rate-limited. Please re-mention \`@dev-lead\` when the rate limit clears (estimated: \`${reset_display}\`) — I cannot reconstruct the original instruction automatically." - if [ "$DEV_LEAD_DRY_RUN" = "true" ]; then - echo "[dry-run] would post user-visible rate-limit acknowledgment" - echo "$ack_body" - else - gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$ack_body" - fi - ;; - esac - fi -} - -handle_rate_limit() { - local intent="$1" - echo "::warning::All engines rate-limited for intent=${intent} — posting rate-limited marker" - post_reviews_rate_limited "$intent" - [[ -n "${PR_NUMBER:-}" ]] && try_enable_auto_merge - exit 2 -} - case "$INTENT_TYPE" in fix-reviews) # Get open review threads @@ -2485,10 +1310,17 @@ case "$INTENT_TYPE" in git fetch origin "$BASE_REF" CONFLICTING_FILES=$(detect_conflicting_paths "$BASE_REF") export CONFLICTING_FILES + # Capture the branch tip *before* the resolution so the integrity check can + # use it as one parent (the base ref is the other) when scanning the resolved + # tree for introduced duplicate declarations (#1482). + PRE_RESOLVE_SHA="$(git rev-parse HEAD 2>/dev/null || true)" rc=0 build_and_run "rebase" || rc=$? [ "$rc" -eq 2 ] && handle_rate_limit "rebase" if [ "$rc" -eq 0 ]; then + # Advisory post-resolution integrity check (#1482): flag, do not block. + # `|| true` guarantees a detector fault can never fail a real resolution. + run_post_resolution_integrity_check "$BASE_REF" "${PRE_RESOLVE_SHA:-}" || true if commit_and_push "rebase"; then # Engine left commits/changes for the script to push — resolution applied. post_reviews_terminal "rebase" "applied" "Rebase completed and pushed." diff --git a/scripts/lib/conflict-integrity.sh b/scripts/lib/conflict-integrity.sh new file mode 100644 index 000000000..3164f68bd --- /dev/null +++ b/scripts/lib/conflict-integrity.sh @@ -0,0 +1,115 @@ +#!/usr/bin/env bash +# conflict-integrity.sh — post-conflict-resolution integrity check (#1482). +# +# A narrow, mechanical detector for the corruption class from #1449: a botched +# automated conflict resolution that duplicates whole blocks of a shell script +# (grew scripts/engine.sh from 2688 to 4011 lines, doubling run_writer / +# parse_reset_time / extract_verdict_json). Such a resolution "completes" and +# reports success, so nothing on the conflict-resolution path checks it — it only +# surfaces hours later when an unrelated test trips over the corrupted function. +# +# These helpers are pure (no network/git side effects) so they are unit-testable +# in isolation; the git/gh plumbing that feeds them lives in the caller +# (dev-lead-fix-reviews.sh's rebase intent). The detector is deliberately +# mechanical — duplicate top-level declarations — not a semantic "is this diff +# correct" analysis, which is neither tractable nor in scope. + +# extract_top_level_symbols +# Emit one line per top-level declaration in file order, tagged by kind: +# fn:NAME — a function declaration (`NAME() {` / `NAME()` / `function NAME`) +# var:NAME — a top-level variable assignment (optionally export/readonly/declare) +# "Top-level" means the declaration begins at column 0 (no leading whitespace), +# so nested functions and in-function `local` assignments are ignored. A symbol +# declared N times appears N times. +extract_top_level_symbols() { + local file="$1" + [ -f "$file" ] || return 0 + awk ' + # NAME() { / NAME() (POSIX + ksh function forms), column 0 only. + /^[A-Za-z_][A-Za-z0-9_]*[[:space:]]*\(\)[[:space:]]*\{/ { + name = $0 + sub(/[[:space:]]*\(\).*$/, "", name) + print "fn:" name + next + } + # function NAME (with or without trailing parens). + /^function[[:space:]]+[A-Za-z_][A-Za-z0-9_]*/ { + name = $2 + sub(/\(.*$/, "", name) + print "fn:" name + next + } + # [export|readonly|declare -x] NAME=... top-level assignment, column 0. + /^(export[[:space:]]+|readonly[[:space:]]+|declare[[:space:]]+(-[A-Za-z]+[[:space:]]+)*)?[A-Za-z_][A-Za-z0-9_]*=/ { + line = $0 + sub(/^(export[[:space:]]+|readonly[[:space:]]+|declare[[:space:]]+(-[A-Za-z]+[[:space:]]+)*)/, "", line) + name = line + sub(/=.*$/, "", name) + print "var:" name + next + } + ' "$file" +} + +# symbol_counts +# Emit "SYMBOLCOUNT" for every top-level symbol, sorted. +symbol_counts() { + extract_top_level_symbols "$1" | LC_ALL=C sort | uniq -c \ + | awk '{ print $2 "\t" $1 }' +} + +# _count_of +# Look up a symbol's count in a "SYMBOLCOUNT" block; 0 if absent. +_count_of() { + local counts="$1" sym="$2" c + c="$(printf '%s\n' "$counts" | awk -F'\t' -v s="$sym" '$1 == s { print $2; exit }')" + printf '%s' "${c:-0}" +} + +# new_duplicate_symbols +# Emit "SYMBOLCOUNT" for each top-level symbol whose declaration count in +# the resolved file EXCEEDS its count in *both* parents — i.e., the resolution +# introduced extra copies. A symbol that already appeared N times in a parent and +# still appears N times is NOT flagged, so a legitimate large upstream merge (or +# an intentional pre-existing repetition) does not trip the check (AC #1, #6). +# A missing parent file counts as zero declarations (a brand-new file that itself +# ships duplicate declarations is still flagged). +new_duplicate_symbols() { + local resolved="$1" parent_a="$2" parent_b="$3" + { + extract_top_level_symbols "$parent_a" | awk '{print "a\t" $0}' + extract_top_level_symbols "$parent_b" | awk '{print "b\t" $0}' + extract_top_level_symbols "$resolved" | awk '{print "r\t" $0}' + } | awk -F'\t' ' + $1 == "a" { count_a[$2]++ } + $1 == "b" { count_b[$2]++ } + $1 == "r" { count_r[$2]++ } + END { + for (sym in count_r) { + rcount = count_r[sym] + if (rcount > 1) { + acount = count_a[sym] + 0 + bcount = count_b[sym] + 0 + maxp = (acount > bcount) ? acount : bcount + if (rcount > maxp) { + print sym "\t" rcount + } + } + } + } + ' | LC_ALL=C sort +} + +# format_integrity_findings +# Render a Markdown bullet naming the file and each duplicated symbol, where +# is the "SYMBOLCOUNT" output of new_duplicate_symbols. Emits +# nothing when there are no findings. +format_integrity_findings() { + local file="$1" findings="$2" + [ -n "$findings" ] || return 0 + printf -- '- `%s` — duplicated top-level declarations after resolution:\n' "$file" + printf '%s\n' "$findings" | while IFS="$(printf '\t')" read -r sym count; do + [ -n "$sym" ] || continue + printf -- ' - `%s` (declared %s times)\n' "$sym" "$count" + done +} diff --git a/tests/dev-lead/fixtures/conflict-integrity/parent_base.sh b/tests/dev-lead/fixtures/conflict-integrity/parent_base.sh new file mode 100644 index 000000000..bcd3e1a96 --- /dev/null +++ b/tests/dev-lead/fixtures/conflict-integrity/parent_base.sh @@ -0,0 +1,25 @@ +#!/usr/bin/env bash +# Fixture: the BASE (main) side of the #1449 conflict — each top-level symbol +# declared exactly once. Represents scripts/engine.sh on `main`. +set -euo pipefail + +MAX_RETRIES=3 + +run_writer() { + local prompt="$1" + echo "writing $prompt" +} + +extract_verdict_json() { + local log="$1" + grep -oE '\{.*\}' "$log" +} + +parse_reset_time() { + local header="$1" + date -d "$header" +%s +} + +build_prompt() { + echo "prompt" +} diff --git a/tests/dev-lead/fixtures/conflict-integrity/parent_branch.sh b/tests/dev-lead/fixtures/conflict-integrity/parent_branch.sh new file mode 100644 index 000000000..e0a610539 --- /dev/null +++ b/tests/dev-lead/fixtures/conflict-integrity/parent_branch.sh @@ -0,0 +1,31 @@ +#!/usr/bin/env bash +# Fixture: the BRANCH (PR head) side of the #1449 conflict — each top-level +# symbol declared exactly once, with the branch's own edits (run_writer gains a +# model arg, plus a branch-only run_reviewer). +set -euo pipefail + +MAX_RETRIES=3 + +run_writer() { + local prompt="$1" + local model="$2" + echo "writing $prompt with $model" +} + +extract_verdict_json() { + local log="$1" + grep -oE '\{.*\}' "$log" +} + +parse_reset_time() { + local header="$1" + date -d "$header" +%s +} + +build_prompt() { + echo "prompt" +} + +run_reviewer() { + echo "review" +} diff --git a/tests/dev-lead/fixtures/conflict-integrity/parent_predup.sh b/tests/dev-lead/fixtures/conflict-integrity/parent_predup.sh new file mode 100644 index 000000000..74eb36527 --- /dev/null +++ b/tests/dev-lead/fixtures/conflict-integrity/parent_predup.sh @@ -0,0 +1,17 @@ +#!/usr/bin/env bash +# Fixture: a parent that ALREADY declares `legacy_shim` twice (an intentional, +# pre-existing repetition upstream). The detector must not flag a symbol that was +# already duplicated in a parent — only duplication *introduced* by the merge. +set -euo pipefail + +legacy_shim() { + echo "shim v1" +} + +legacy_shim() { + echo "shim v2" +} + +core_a() { + echo a +} diff --git a/tests/dev-lead/fixtures/conflict-integrity/resolved_clean.sh b/tests/dev-lead/fixtures/conflict-integrity/resolved_clean.sh new file mode 100644 index 000000000..ce363f98b --- /dev/null +++ b/tests/dev-lead/fixtures/conflict-integrity/resolved_clean.sh @@ -0,0 +1,30 @@ +#!/usr/bin/env bash +# Fixture: a CORRECT resolution of the #1449 conflict — the union of both sides, +# each top-level symbol declared exactly once. Must produce NO integrity finding. +set -euo pipefail + +MAX_RETRIES=3 + +run_writer() { + local prompt="$1" + local model="$2" + echo "writing $prompt with $model" +} + +extract_verdict_json() { + local log="$1" + grep -oE '\{.*\}' "$log" +} + +parse_reset_time() { + local header="$1" + date -d "$header" +%s +} + +build_prompt() { + echo "prompt" +} + +run_reviewer() { + echo "review" +} diff --git a/tests/dev-lead/fixtures/conflict-integrity/resolved_corrupted.sh b/tests/dev-lead/fixtures/conflict-integrity/resolved_corrupted.sh new file mode 100644 index 000000000..a5975b852 --- /dev/null +++ b/tests/dev-lead/fixtures/conflict-integrity/resolved_corrupted.sh @@ -0,0 +1,48 @@ +#!/usr/bin/env bash +# Fixture: the BOTCHED resolution of the #1449 conflict — the merge duplicated +# whole blocks, so run_writer / extract_verdict_json / parse_reset_time (and the +# MAX_RETRIES constant) each appear TWICE, roughly doubling the file. This is the +# exact corruption signature that grew scripts/engine.sh from 2688 to 4011 lines. +set -euo pipefail + +MAX_RETRIES=3 +MAX_RETRIES=3 + +run_writer() { + local prompt="$1" + echo "writing $prompt" +} + +extract_verdict_json() { + local log="$1" + grep -oE '\{.*\}' "$log" +} + +parse_reset_time() { + local header="$1" + date -d "$header" +%s +} + +build_prompt() { + echo "prompt" +} + +run_reviewer() { + echo "review" +} + +run_writer() { + local prompt="$1" + local model="$2" + echo "writing $prompt with $model" +} + +extract_verdict_json() { + local log="$1" + grep -oE '\{.*\}' "$log" +} + +parse_reset_time() { + local header="$1" + date -d "$header" +%s +} diff --git a/tests/dev-lead/fixtures/conflict-integrity/resolved_large_legit.sh b/tests/dev-lead/fixtures/conflict-integrity/resolved_large_legit.sh new file mode 100644 index 000000000..c5d277505 --- /dev/null +++ b/tests/dev-lead/fixtures/conflict-integrity/resolved_large_legit.sh @@ -0,0 +1,39 @@ +#!/usr/bin/env bash +# Fixture: a LEGITIMATELY large resolution — it carries the pre-existing +# `legacy_shim` double declaration through unchanged (count still 2, same as the +# parent) and adds many brand-new single-declaration functions (a real upstream +# feature merge). Must produce NO integrity finding: the raw size grew, but no +# symbol's declaration count exceeds both parents. (AC #6) +set -euo pipefail + +legacy_shim() { + echo "shim v1" +} + +legacy_shim() { + echo "shim v2" +} + +core_a() { + echo a +} + +feature_b() { + echo b +} + +feature_c() { + echo c +} + +feature_d() { + echo d +} + +feature_e() { + echo e +} + +feature_f() { + echo f +} diff --git a/tests/dev-lead/unit/test_conflict_integrity.bats b/tests/dev-lead/unit/test_conflict_integrity.bats new file mode 100644 index 000000000..380e0bd2d --- /dev/null +++ b/tests/dev-lead/unit/test_conflict_integrity.bats @@ -0,0 +1,111 @@ +#!/usr/bin/env bats +# Unit + regression tests for scripts/lib/conflict-integrity.sh (#1482). +# +# The regression fixture reproduces the #1449 corruption: a botched conflict +# resolution duplicated whole blocks of scripts/engine.sh (2688 → 4011 lines), +# doubling run_writer / parse_reset_time / extract_verdict_json. The detector must +# catch that at the point of resolution — not wait for an unrelated unit test to +# trip over the corrupted function hours later. + +SCRIPT_DIR="$(cd "$(dirname "$BATS_TEST_FILENAME")/../../.." && pwd)" +LIB="$SCRIPT_DIR/scripts/lib/conflict-integrity.sh" +FIX="$SCRIPT_DIR/tests/dev-lead/fixtures/conflict-integrity" + +setup() { + # shellcheck source=scripts/lib/conflict-integrity.sh + source "$LIB" +} + +# --------------------------------------------------------------------------- +# extract_top_level_symbols +# --------------------------------------------------------------------------- + +@test "extract_top_level_symbols: emits top-level functions and vars" { + run extract_top_level_symbols "$FIX/parent_base.sh" + [ "$status" -eq 0 ] + [[ "$output" == *"fn:run_writer"* ]] + [[ "$output" == *"fn:extract_verdict_json"* ]] + [[ "$output" == *"fn:parse_reset_time"* ]] + [[ "$output" == *"var:MAX_RETRIES"* ]] +} + +@test "extract_top_level_symbols: ignores indented / nested declarations" { + local f="$BATS_TEST_TMPDIR/test_file.sh" + cat > "$f" <<'EOF' +top() { + local nested=1 + inner() { echo nested; } +} +EOF + run extract_top_level_symbols "$f" + [ "$status" -eq 0 ] + [[ "$output" == *"fn:top"* ]] + # A function defined *inside* another (indented) is not a top-level decl. + [[ "$output" != *"fn:inner"* ]] + # An indented `local` assignment is not a top-level variable decl. + [[ "$output" != *"var:nested"* ]] +} + +# --------------------------------------------------------------------------- +# new_duplicate_symbols — the #1449 regression +# --------------------------------------------------------------------------- + +@test "new_duplicate_symbols: flags the #1449 doubled engine.sh symbols" { + run new_duplicate_symbols "$FIX/resolved_corrupted.sh" "$FIX/parent_base.sh" "$FIX/parent_branch.sh" + [ "$status" -eq 0 ] + [[ "$output" == *"fn:run_writer"* ]] + [[ "$output" == *"fn:extract_verdict_json"* ]] + [[ "$output" == *"fn:parse_reset_time"* ]] + # The duplicated top-level constant is caught too. + [[ "$output" == *"var:MAX_RETRIES"* ]] + # Symbols that stayed single (build_prompt, run_reviewer) are NOT flagged. + [[ "$output" != *"build_prompt"* ]] + [[ "$output" != *"run_reviewer"* ]] +} + +@test "new_duplicate_symbols: reports the resolved declaration count" { + run new_duplicate_symbols "$FIX/resolved_corrupted.sh" "$FIX/parent_base.sh" "$FIX/parent_branch.sh" + [ "$status" -eq 0 ] + # Each doubled symbol appears twice in the resolved file. + [[ "$output" == *$'fn:run_writer\t2'* ]] +} + +@test "new_duplicate_symbols: a correct union resolution is clean" { + run new_duplicate_symbols "$FIX/resolved_clean.sh" "$FIX/parent_base.sh" "$FIX/parent_branch.sh" + [ "$status" -eq 0 ] + [ -z "$output" ] +} + +# --------------------------------------------------------------------------- +# AC #6 — no false positive on legitimately large diffs / pre-existing dups +# --------------------------------------------------------------------------- + +@test "new_duplicate_symbols: does not flag a symbol already duplicated in a parent" { + run new_duplicate_symbols "$FIX/resolved_large_legit.sh" "$FIX/parent_predup.sh" "$FIX/parent_base.sh" + [ "$status" -eq 0 ] + # legacy_shim was already declared twice in the parent — carrying it through + # unchanged is legitimate, not corruption. + [[ "$output" != *"legacy_shim"* ]] + # Many new single-declaration functions in a large diff are not flagged. + [ -z "$output" ] +} + +@test "new_duplicate_symbols: missing parent blobs are treated as empty (new file)" { + run new_duplicate_symbols "$FIX/resolved_corrupted.sh" "/nonexistent/base.sh" "/nonexistent/branch.sh" + [ "$status" -eq 0 ] + [[ "$output" == *$'fn:run_writer\t2'* ]] +} + +# --------------------------------------------------------------------------- +# format_integrity_findings — the actionable comment body +# --------------------------------------------------------------------------- + +@test "format_integrity_findings: names the file and the duplicated symbols" { + local findings + findings="$(new_duplicate_symbols "$FIX/resolved_corrupted.sh" "$FIX/parent_base.sh" "$FIX/parent_branch.sh")" + run format_integrity_findings "scripts/engine.sh" "$findings" + [ "$status" -eq 0 ] + [[ "$output" == *"scripts/engine.sh"* ]] + [[ "$output" == *"run_writer"* ]] + [[ "$output" == *"parse_reset_time"* ]] +}