Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions prompts/dev-lead/fix-bot-comment.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ ${ALL_REVIEWS_JSON}

Treat any check with `conclusion` = `"failure"`, `"timed_out"`, `"cancelled"`, `"action_required"`, `"stale"`, or `"startup_failure"` and any review with `state` = `"CHANGES_REQUESTED"` as **Tier 1 blockers** — address them in addition to the bot comment below. Only declare "no-changes" when zero Tier 1 blockers exist.

> **A neutral overview is not an actionable finding.** If the bot comment merely *describes* or summarizes the diff (a "pull request overview", typically a review with `state` = `"COMMENTED"`) without reporting a specific, actionable defect tied to a file/line, there is nothing to fix — do **not** revert or undo the PR's own changes to "address" it. Reverting the PR's own fix nets the diff to zero and silently cancels it (#1340). Act only on concrete findings.

## Task

> **Guardrail — never SHA-pin a first-party channel ref.** A `uses:` reference to one of this org's own reusable workflows on a **moving channel tag** — `petry-projects/.github(-private)/.github/workflows/*.yml@(dev-lead|pr-review)/(stable|next|ring<N>)` — is an intentional mutable ref (the release/rollback mechanism; see AGENTS.md "Release channel tags & the mutable-ref exception"). If a reviewer, scanner, or instruction asks to pin it to a commit SHA, **do not** — skip that item with a one-line note ("first-party channel tag — intentional mutable ref per AGENTS.md") and leave the ref on its `@<agent>/<channel>` tag.
Expand Down Expand Up @@ -111,6 +113,7 @@ If `${ACTOR}` is `sonarqubecloud[bot]` and the comment reports security hotspots

- Only fix issues that are clearly actionable from the bot's output
- Do not fix issues marked as "informational" or "suggestion" unless they indicate a real bug
- Never revert or undo the PR's own committed changes to "address" a neutral overview/summary comment — that produces a net-zero diff that silently cancels the fix (#1340)
- Do not suppress bot rules without a documented reason
- Do not modify the bot's configuration files
- For every thread you fix, post a reply naming the specific change before resolving — never resolve silently
Expand Down
3 changes: 3 additions & 0 deletions prompts/dev-lead/fix-reviews.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ ${ALL_REVIEWS_JSON}

Identify any entries with `state` = `"CHANGES_REQUESTED"`. Each one is a **Tier 1 blocker**. Only declare "no-changes" when zero Tier 1 blockers exist (all CI checks pass AND no reviewer has CHANGES_REQUESTED).

> **A `COMMENTED` review is neutral — never treat it as a change-request.** A reviewer's "pull request overview" or any review submitted with `state` = `"COMMENTED"` merely *describes* the diff; it is **not** an instruction to change anything. Do **not** revert, undo, or restore lines your own commits added or removed just because such an overview mentions them. Only `CHANGES_REQUESTED` reviews and explicit, actionable review-thread comments are change-requests. Reverting the PR's own fix to satisfy a neutral overview nets the diff to zero and silently cancels the fix (#1340).

### Phase 1 — Address Threads

For each open review thread:
Expand Down Expand Up @@ -113,6 +115,7 @@ Read every changed line as if you are the reviewer seeing the response:
- Resolve every bot thread you fix (regardless of which reviewer triggered this run) and outdated threads; for human threads, resolve only the triggering reviewer's and leave other humans' open (replied)
- Do not resolve threads you are skipping due to ambiguity — leave those open and note them in your output
- Do not make changes beyond what the review threads request, except that fixing Tier-1 blockers (failure/timed_out/cancelled/action_required/stale/startup_failure CI checks and CHANGES_REQUESTED reviews) is always in-scope
- Never revert or undo the PR's own committed changes to satisfy a neutral `COMMENTED`/overview review — that produces a net-zero diff that silently cancels the fix (#1340)
- If a review thread is ambiguous, apply the most conservative interpretation
- Do not commit or push — the CI workflow handles git operations after you finish

Expand Down
114 changes: 103 additions & 11 deletions scripts/dev-lead-fix-reviews.sh
Original file line number Diff line number Diff line change
Expand Up @@ -962,9 +962,70 @@ ${summary}"
gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$body" 2>/dev/null || true
}

# Marker for the no-op guard flag comment (#1340), deduped per PR+intent.
NOOP_MARKER_PREFIX="<!-- dev-lead-noop-guard pr="

# pr_nets_to_zero [base] — returns 0 (true) when the PR branch's net diff against
# its base is empty: every change the PR's own commits introduced has been undone,
# so `origin/<base>...HEAD` shows zero changed files. Such a PR must never carry a
# pushed fix — merging a `Closes #N` PR that nets to zero would auto-close its
# compliance issue while the finding remains unfixed (#1340). When the base cannot
# be resolved (no ref, fetch fails, no common ancestor) it returns 1 and warns,
# so an unverifiable state never blocks a legitimate push.
pr_nets_to_zero() {
local base="${1:-${BASE_REF:-main}}"
local baseref="origin/${base}"
if ! git rev-parse --verify --quiet "${baseref}^{commit}" >/dev/null 2>&1; then
git fetch --quiet origin "$base" 2>/dev/null || {
echo "::warning::no-op guard: could not resolve ${baseref} — skipping net-zero check" >&2
return 1
}
fi
local changed
changed=$(git diff --name-only "${baseref}...HEAD" 2>/dev/null) || {
echo "::warning::no-op guard: git diff against ${baseref} failed — skipping net-zero check" >&2
return 1
}
[ -z "$changed" ]
}
Comment thread
don-petry marked this conversation as resolved.

# flag_noop_pr <intent> — a fix pass reverted the PR's own changes, netting the
# base…head diff to zero (#1340). Post one deduped human-attention comment, add
# the needs-human-review label, and disable auto-merge — and suppress the
# EXIT-trap auto-merge restore so a self-cancelling PR is never silently made
# mergeable again. Mirrors pr_automation_escalate's escalation shape.
flag_noop_pr() {
local intent="$1"
# A self-cancelling PR must stay unmergeable until a human looks: prevent
# restore_auto_merge (EXIT trap) from re-enabling what we are about to disable.
_AM_NEEDS_RESTORE=0
if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then
echo "[dry-run] no-op guard: would flag PR #${PR_NUMBER} (${intent}) as net-zero, add ${NEEDS_HUMAN_REVIEW_LABEL:-needs-human-review}, disable auto-merge"
return 0
fi
local marker="${NOOP_MARKER_PREFIX}${PR_NUMBER} intent=${intent} -->"
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.
# 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
Expand Down Expand Up @@ -1014,6 +1075,19 @@ commit_and_push() {
# 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.
Expand Down Expand Up @@ -1291,9 +1365,16 @@ case "$INTENT_TYPE" in
build_and_run "fix-reviews" || rc=$?
[ "$rc" -eq 2 ] && handle_rate_limit "fix-reviews"
if [ "$rc" -eq 0 ]; then
if commit_and_push "fix-reviews"; then
cp_rc=0
commit_and_push "fix-reviews" || cp_rc=$?
if [ "$cp_rc" -eq 0 ]; then
notify_coderabbit_resolve
post_reviews_terminal "fix-reviews" "applied" "Changes committed and pushed."
elif [ "$cp_rc" -eq 3 ]; then
# No-op guard (#1340): the fix nets base…head to zero — already flagged
# for a human, auto-merge disabled. Post no applied/no-changes/retry
# marker and do not re-enable auto-merge or resolve threads.
echo "::warning::fix-reviews produced a net-zero diff — flagged for human, not pushed (#1340)"
else
notify_coderabbit_resolve
if has_hard_blockers; then
Expand All @@ -1305,10 +1386,12 @@ case "$INTENT_TYPE" in
post_no_changes "fix-reviews"
fi
fi
# Always resolve outdated bot threads in the no-changes path as cleanup
resolve_bot_outdated_threads "fix-reviews"
resolve_actor_outdated_threads "fix-reviews"
try_enable_auto_merge
if [ "$cp_rc" -ne 3 ]; then
# Always resolve outdated bot threads in the no-changes path as cleanup
resolve_bot_outdated_threads "fix-reviews"
resolve_actor_outdated_threads "fix-reviews"
try_enable_auto_merge
fi
fi
exit "$rc"
;;
Expand All @@ -1320,9 +1403,16 @@ case "$INTENT_TYPE" in
build_and_run "fix-bot-comment" || rc=$?
[ "$rc" -eq 2 ] && handle_rate_limit "fix-bot-comment"
if [ "$rc" -eq 0 ]; then
if commit_and_push "fix-bot-comment"; then
cp_rc=0
commit_and_push "fix-bot-comment" || cp_rc=$?
if [ "$cp_rc" -eq 0 ]; then
notify_coderabbit_resolve
post_reviews_terminal "fix-bot-comment" "applied" "Changes committed and pushed."
elif [ "$cp_rc" -eq 3 ]; then
# No-op guard (#1340): the fix nets base…head to zero — already flagged
# for a human, auto-merge disabled. Post no terminal marker and do not
# re-enable auto-merge or resolve threads.
echo "::warning::fix-bot-comment produced a net-zero diff — flagged for human, not pushed (#1340)"
else
notify_coderabbit_resolve
if has_hard_blockers; then
Expand All @@ -1335,10 +1425,12 @@ case "$INTENT_TYPE" in
post_no_changes "fix-bot-comment"
fi
fi
# Always resolve outdated bot threads in the no-changes path as cleanup
resolve_bot_outdated_threads "fix-bot-comment"
resolve_actor_outdated_threads "fix-bot-comment"
try_enable_auto_merge
if [ "$cp_rc" -ne 3 ]; then
# Always resolve outdated bot threads in the no-changes path as cleanup
resolve_bot_outdated_threads "fix-bot-comment"
resolve_actor_outdated_threads "fix-bot-comment"
try_enable_auto_merge
fi
fi
exit "$rc"
;;
Expand Down
192 changes: 192 additions & 0 deletions tests/dev-lead/unit/test_fix_reviews.bats
Original file line number Diff line number Diff line change
Expand Up @@ -2891,3 +2891,195 @@ STUB
rm -rf "$tmpdir"
rm -f "$deletions_file"
}

# ── No-op guard (#1340): never push a fix that nets base…head to zero ──────────

# Shared setup helper: build a repo whose feat commit adds a line, point
# refs/remotes/origin/main at the base commit, and install a gh/git stub that
# records flag comments, label edits, merges, and pushes. The engine (claude
# stub) rewrites file.txt to the caller-provided content.
_noop_setup_repo() {
local git_repo="$1" engine_content="$2"
git -C "$git_repo" init -q
printf 'base\n' > "$git_repo/file.txt"
git -C "$git_repo" add .
git -C "$git_repo" -c user.email="t@test" -c user.name="T" commit -q -m "base"
NOOP_BASE_SHA="$(git -C "$git_repo" rev-parse HEAD)"
# origin/main is shared across worktrees created from this repo
git -C "$git_repo" update-ref refs/remotes/origin/main "$NOOP_BASE_SHA"
# feat commit adds a line the fix pass may (or may not) revert
printf 'base\nfeature\n' > "$git_repo/file.txt"
git -C "$git_repo" add .
git -C "$git_repo" -c user.email="t@test" -c user.name="T" commit -q -m "feat: add feature"

Comment thread
don-petry marked this conversation as resolved.
cat > "$STUB_BIN_DIR/claude" << STUB
#!/usr/bin/env bash
echo "Addressed review feedback."
printf '${engine_content}' > file.txt
STUB
chmod +x "$STUB_BIN_DIR/claude"
}

_noop_gh_stub() {
local comment_file="$1" merge_file="$2"
cat > "$STUB_BIN_DIR/gh" << GHEOF
#!/usr/bin/env bash
ARGS="\$*"
case "\$ARGS" in
*"pr view"*) echo '{"state":"OPEN","headRefName":"feat"}' ;;
*"pr checkout"*) exit 0 ;;
*"pr comment"*) echo "\$*" >> "${comment_file}"; exit 0 ;;
*"pr edit"*) echo "\$*" >> "${comment_file}"; exit 0 ;;
*"pr merge"*) echo "\$*" >> "${merge_file}"; exit 0 ;;
*"check-runs"*) echo '{"check_runs":[]}' ;;
*"statuses"*) echo '[]' ;;
*"reviews"*) echo '[]' ;;
*"graphql"*) echo '{"data":{"repository":{"pullRequest":{"reviewThreads":{"nodes":[]}}}}}' ;;
*"issues/"*"comments"*) echo '[]' ;;
*"pulls/"*) echo '{"head":{"sha":"feat"},"auto_merge":null,"state":"open"}' ;;
Comment thread
don-petry marked this conversation as resolved.
Outdated
*) echo "{}" ;;
esac
GHEOF
chmod +x "$STUB_BIN_DIR/gh"
}

_noop_git_stub() {
local push_file="$1"
cat > "$STUB_BIN_DIR/git" << GITEOF
#!/usr/bin/env bash
if [ "\$1" = "push" ]; then echo "\$*" >> "${push_file}"; exit 0; fi
exec /usr/bin/git "\$@"
GITEOF
chmod +x "$STUB_BIN_DIR/git"
}

@test "no-op guard: fix-reviews net-zero diff flags PR and does not push (#1340)" {
local git_repo comment_file merge_file push_file
git_repo="$(mktemp -d)"; comment_file="$(mktemp)"; merge_file="$(mktemp)"; push_file="$(mktemp)"

# Engine reverts the feature line → net base…head diff becomes empty.
_noop_setup_repo "$git_repo" 'base\n'
_noop_gh_stub "$comment_file" "$merge_file"
_noop_git_stub "$push_file"

cd "$git_repo"
run bash -c "
export INTENT_TYPE=fix-reviews DEV_LEAD_DRY_RUN=false
export PR_NUMBER=54 HEAD_SHA=$NOOP_BASE_SHA REPO='petry-projects/.github-private'
export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead'
Comment thread
don-petry marked this conversation as resolved.
export PATH="$STUB_BIN_DIR:\$PATH"
bash '$FIX_REVIEWS_SCRIPT'
" 2>&1

[ "$status" -eq 0 ]
# Guard fired and announced the net-zero refusal
[[ "$output" == *"No-op guard"* ]]
# The self-cancelling fix was NOT pushed
[ ! -s "$push_file" ]
# A needs-human flag comment was posted
grep -q "No-op fix detected" "$comment_file"
# Auto-merge was disabled
grep -q "disable-auto" "$merge_file"
# No false "applied" terminal marker
! grep -q "status=applied" "$comment_file"

rm -rf "$git_repo"; rm -f "$comment_file" "$merge_file" "$push_file"
}
Comment thread
don-petry marked this conversation as resolved.

@test "no-op guard: fix-bot-comment net-zero diff flags PR and does not push (#1340)" {
local git_repo comment_file merge_file push_file
git_repo="$(mktemp -d)"; comment_file="$(mktemp)"; merge_file="$(mktemp)"; push_file="$(mktemp)"

_noop_setup_repo "$git_repo" 'base\n'
_noop_gh_stub "$comment_file" "$merge_file"
_noop_git_stub "$push_file"

cd "$git_repo"
run bash -c "
export INTENT_TYPE=fix-bot-comment DEV_LEAD_DRY_RUN=false
export PR_NUMBER=54 HEAD_SHA=$NOOP_BASE_SHA REPO='petry-projects/.github-private'
export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead'
export ACTOR='github-copilot[bot]' COMMENT_BODY='This PR overview describes the diff.'
Comment thread
don-petry marked this conversation as resolved.
export PATH="$STUB_BIN_DIR:\$PATH"
bash '$FIX_REVIEWS_SCRIPT'
" 2>&1

[ "$status" -eq 0 ]
[[ "$output" == *"No-op guard"* ]]
[ ! -s "$push_file" ]
grep -q "No-op fix detected" "$comment_file"
grep -q "disable-auto" "$merge_file"
! grep -q "status=applied" "$comment_file"

rm -rf "$git_repo"; rm -f "$comment_file" "$merge_file" "$push_file"
}
Comment thread
don-petry marked this conversation as resolved.

@test "no-op guard: non-empty net diff pushes normally and posts applied (#1340)" {
local git_repo comment_file merge_file push_file
git_repo="$(mktemp -d)"; comment_file="$(mktemp)"; merge_file="$(mktemp)"; push_file="$(mktemp)"

# Engine keeps the feature and adds a real fix → net diff is NOT empty.
_noop_setup_repo "$git_repo" 'base\nfeature\nfix\n'
_noop_gh_stub "$comment_file" "$merge_file"
_noop_git_stub "$push_file"

cd "$git_repo"
run bash -c "
export INTENT_TYPE=fix-reviews DEV_LEAD_DRY_RUN=false
export PR_NUMBER=54 HEAD_SHA=$NOOP_BASE_SHA REPO='petry-projects/.github-private'
export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead'
export PATH="$STUB_BIN_DIR:\$PATH"
Comment thread
don-petry marked this conversation as resolved.
bash '$FIX_REVIEWS_SCRIPT'
" 2>&1

[ "$status" -eq 0 ]
# Guard must NOT fire on a legitimate non-empty fix
[[ "$output" != *"No-op guard"* ]]
# The fix was pushed
[ -s "$push_file" ]
# An applied terminal marker was posted; no no-op flag
grep -q "status=applied" "$comment_file"
! grep -q "No-op fix detected" "$comment_file"

rm -rf "$git_repo"; rm -f "$comment_file" "$merge_file" "$push_file"
}
Comment thread
don-petry marked this conversation as resolved.

@test "no-op guard: net-zero path does not re-enable auto-merge (#1340)" {
local git_repo comment_file merge_file push_file
git_repo="$(mktemp -d)"; comment_file="$(mktemp)"; merge_file="$(mktemp)"; push_file="$(mktemp)"

_noop_setup_repo "$git_repo" 'base\n'
_noop_gh_stub "$comment_file" "$merge_file"
_noop_git_stub "$push_file"

cd "$git_repo"
run bash -c "
export INTENT_TYPE=fix-reviews DEV_LEAD_DRY_RUN=false
export PR_NUMBER=54 HEAD_SHA=$NOOP_BASE_SHA REPO='petry-projects/.github-private'
export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead'
export PATH="$STUB_BIN_DIR:\$PATH"
Comment thread
don-petry marked this conversation as resolved.
bash '$FIX_REVIEWS_SCRIPT'
" 2>&1

[ "$status" -eq 0 ]
# The only merge call on the no-op path must be a disable-auto — never --auto
grep -q "disable-auto" "$merge_file"
! grep -q -- "--auto" "$merge_file"

rm -rf "$git_repo"; rm -f "$comment_file" "$merge_file" "$push_file"
}
Comment thread
don-petry marked this conversation as resolved.

# ── Prompt guidance (#1340): COMMENTED/overview is neutral, not a change-request ─

@test "fix-reviews prompt: instructs never to treat COMMENTED/overview as a change-request (#1340)" {
local p="$SCRIPT_DIR/prompts/dev-lead/fix-reviews.md"
grep -q "COMMENTED" "$p"
grep -qi "pull request overview" "$p"
grep -q "#1340" "$p"
}

@test "fix-bot-comment prompt: a neutral overview is not an actionable finding (#1340)" {
local p="$SCRIPT_DIR/prompts/dev-lead/fix-bot-comment.md"
grep -qi "overview" "$p"
grep -q "#1340" "$p"
}
Loading