From af5374f67bbc54209ffbb2b622c42ad9218fd7f7 Mon Sep 17 00:00:00 2001 From: Carlos Almeida Date: Thu, 13 Aug 2026 11:28:55 -0600 Subject: [PATCH 1/3] fix(sweep): replace the reply-reaction shortcut with the four-condition trigger gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Step 2g decided "is Greptile satisfied?" by checking for a positive reaction on THE MOST RECENT NON-GREPTILE COMMENT - i.e. on one of your own replies. A πŸ‘ on a reply is not a re-review of your code. Under that rule the re-trigger is skipped while the fix sits un-reviewed, which is how a sweep reports a PR as converged when Greptile has never seen the change. Transplant the four-condition gate the fleet standardised on. Satisfied now requires ALL of: an @greptileai TRIGGER comment exists and really mentions Greptile (a literal inside a code span, fenced block, indented block or HTML comment notifies nobody and is not a trigger); Greptile reacted positively TO THAT TRIGGER; Greptile posted nothing since; and Greptile has reviewed the CURRENT head, established from its own `Last reviewed commit` marker rather than a commit timestamp - Greptile re-reviews by EDITING its summary IN PLACE, so timestamps cannot observe a completed re-review. Every fetch failure fails safe by posting. Add Step 2i (final mandatory re-trigger) and renumber Return result to 2j, matching the fleet layout. The gate is meant to run twice and its own header names Step 2i; that header doubles as the sentinel the shared test suite locates it by, so aligning this repo's numbering keeps one identical suite proving every copy - the alternative, rewording the sentinel per repo, would fork the contract. The 50-trigger cap is preserved and now stated where it matters: the gate is not exempt from it, and Step 2g's prose says so. That prose previously taught the exact rule the gate replaces ("skip the actual trigger only if Greptile already reacted to your most recent reply"), contradicting the code beneath it. Verified with test_sweep_greptile_gate.py, ported alongside: it extracts the real fenced block from SKILL.md and runs it behind a stub gh under both bash and zsh. Three of its six cases are mutation tests that re-introduce the pre-fix behaviour and require the regression assertion to fail. 6/6. Wired via sweep-gate.yml, since a gate that lives in Markdown is otherwise never executed by anything. Refs: optave/data-retrieval-storage-svc#1021, optave/data-retrieval-storage-svc#930 --- .claude/skills/sweep/SKILL.md | 287 ++++++++++- .github/scripts/test_sweep_greptile_gate.py | 538 ++++++++++++++++++++ .github/workflows/sweep-gate.yml | 59 +++ 3 files changed, 864 insertions(+), 20 deletions(-) create mode 100644 .github/scripts/test_sweep_greptile_gate.py create mode 100644 .github/workflows/sweep-gate.yml diff --git a/.claude/skills/sweep/SKILL.md b/.claude/skills/sweep/SKILL.md index 9cea10ea7..2426b1daf 100644 --- a/.claude/skills/sweep/SKILL.md +++ b/.claude/skills/sweep/SKILL.md @@ -317,11 +317,13 @@ trigger_count=$(gh api repos//issues//comments --paginate \ echo "Greptile has been re-triggered $trigger_count time(s) so far by this sweep." ``` -If `trigger_count` is already **50 or more**: do NOT trigger again, no matter how many real findings you just fixed. Reply to any outstanding comment (per Step 2e) so nothing is left unacknowledged, then run the mandatory final live re-check (Step 2h.1) β€” hitting the cap does not exempt you from it, and comments can still have arrived since your last check β€” reply to anything that check turns up, and only then proceed to Step 2i and report `Status: needs-human-review`, noting in Notes how many rounds occurred and what the last item was. Fixing a real bug on round 51+ does not extend the cap β€” it's a budget on wall-clock and review noise, not a correctness gate; a human reviews the rest. +If `trigger_count` is already **50 or more**: do NOT trigger again, no matter how many real findings you just fixed. Reply to any outstanding comment (per Step 2e) so nothing is left unacknowledged, then run the mandatory final live re-check (Step 2h.1) β€” hitting the cap does not exempt you from it, and comments can still have arrived since your last check β€” reply to anything that check turns up, and only then proceed to Step 2j and report `Status: needs-human-review`, noting in Notes how many rounds occurred and what the last item was. Fixing a real bug on round 51+ does not extend the cap β€” it's a budget on wall-clock and review noise, not a correctness gate; a human reviews the rest. If `trigger_count` is under 50, proceed: -**Greptile:** Always re-trigger after replying to Greptile comments β€” whether the comment was actionable or not. First, run the verification script below to confirm all Greptile comments have replies. Then, skip the actual trigger only if Greptile already reacted to your most recent reply with a positive emoji (thumbs up, check, etc.), which means it is already satisfied. +**Greptile:** Re-trigger after replying to Greptile comments β€” whether the comment was actionable or not β€” so Greptile re-reviews the *updated* PR. First, run the verification script below to confirm all Greptile comments have replies. Then run the **Greptile re-trigger gate** (defined just below and reused verbatim by Step 2i): it posts `@greptileai` unless Greptile is *verifiably satisfied with the current PR head*. + +> A positive reaction on one of **your replies** is **not** satisfaction and never was β€” only a reaction on an `@greptileai` **trigger comment** counts. Skipping the trigger on a reply-reaction leaves your fix un-reviewed, and it is the specific mistake this gate replaced. Let the gate decide; do not second-guess it. (The 50-trigger cap above still applies β€” if it is spent, do not run the gate; follow the cap's instructions instead.) **CRITICAL β€” verify all Greptile comments have replies BEFORE triggering.** Posting `@greptileai` without replying to every comment is worse than not triggering at all β€” it starts a new review cycle while the old one still has unanswered feedback. Run this check first: @@ -357,22 +359,251 @@ echo "All Greptile comments have replies β€” safe to re-trigger." **Do NOT proceed to the re-trigger step below until the check above passes.** If any comments are unanswered, go back to Step 2e, reply to each one, then re-run this check. ```bash -# Step 1: Check if greptileai left a positive reaction on your most recent reply -last_reply_id=$(gh api repos//issues//comments --paginate \ - --jq '[.[] | select(.user.login != "greptile-apps[bot]")] | last | .id') - -positive_count=$(gh api repos//issues/comments/$last_reply_id/reactions \ - --jq '[.[] | select(.user.login == "greptile-apps[bot]" and (.content == "+1" or .content == "hooray" or .content == "heart" or .content == "rocket"))] | length') - -# Step 2: If positive reaction exists β†’ skip. Otherwise β†’ re-trigger, capturing this -# trigger's own comment ID directly from the post response β€” Step 2h's reaction check -# reuses this exact value rather than searching for "the last @greptileai comment", -# which would drift to a different trigger if this PR is also being worked by another -# session or agent (Parallel Sessions) before this round's poll runs. -if [ "$positive_count" -gt 0 ]; then - echo "Greptile already reacted positively β€” skipping re-trigger." +# === Greptile re-trigger gate (shared by Step 2g and Step 2i) === +# Re-trigger Greptile UNLESS it is verifiably satisfied with the CURRENT PR head. +# "Satisfied" requires ALL of: +# (1) an `@greptileai` TRIGGER comment exists β€” a non-Greptile comment that REALLY MENTIONS +# @greptileai, i.e. GitHub linkified it. A body that merely CONTAINS the literal inside a +# code span, a fenced block, an indented block, or an HTML comment notifies nobody and is +# NOT a trigger (see the classifier's own header below, and #964), +# (2) Greptile reacted to THAT TRIGGER with a positive emoji (+1/hooray/heart/rocket), +# (3) Greptile posted no issue-style or inline comment after that trigger, AND +# (4) Greptile has reviewed the CURRENT head β€” established from its own `Last reviewed commit` +# marker, falling back to a commit-timestamp proxy only when that marker can't be parsed. +# A positive reaction on one of YOUR REPLIES is NOT a satisfied signal β€” only a reaction on a +# TRIGGER comment counts. If ANY condition fails, post `@greptileai`. Same idempotent gate is +# safe to re-run, so Step 2i calls it verbatim as the final mandatory check. + +# Candidate issue-stream comments, oldest first: ``. +# The body rides through jq's `@json` so every record stays ONE line β€” a raw multi-line body would +# break the line-oriented classifier below. `@json` is core jq needing no regex, so this does not +# depend on which jq flavour gh embeds, and it uses gh's BUILT-IN --jq (never a standalone `jq` +# binary β€” same rationale as Step 0's guard). +trigger_candidates=$(gh api repos//issues//comments --paginate \ + --jq '.[] | select(.user.login != "greptile-apps[bot]") | "\(.id)\t\(.created_at)\t\((.body // "") | @json)"') \ + || { echo "FATAL: could not fetch trigger comments β€” aborting gate"; exit 1; } + +# +# ── Decision half: pure text, NO network. Reads ONLY $trigger_candidates (newline-separated, +# possibly empty) and sets $trigger_id / $trigger_ts (both empty β‡’ no real trigger exists). +# `scripts/greptile-trigger-guard-check.sh` extracts these exact lines between the two markers and +# runs them under bash AND zsh against frozen fixtures (CI job `greptile-trigger-guard`), so this +# block is tested, executable code β€” not prose. Keep it hermetic: no gh/curl/git in command +# position, and no input but that one variable. +# +# A TRIGGER is a comment that really MENTIONS @greptileai β€” one GitHub linkified, so Greptile was +# notified. A body that merely CONTAINS the literal is NOT a trigger: GitHub does not linkify a +# mention inside an inline code span, a fenced block, an indented block, or an HTML comment. The +# gate used to test the raw body with `test("@greptileai")`, which counted all of those β€” and that +# defeats the gate with its own detector. The warning above is explicit that a πŸ‘ on one of OUR +# replies must never substitute for a reaction on a real trigger, yet a reply whose prose merely +# said `@greptileai` in backticks was classified AS the trigger, so conditions (1) and (2) were +# both satisfied by exactly that reply-reaction. A false trigger also moved $trigger_ts later, +# relaxing condition (3)'s baseline. Observed on PR #964: the `Digest` stage's "Concepts to know" +# comment reads "…blocking a re-trigger of `@greptileai` while feedback is unanswered", and being +# the newest match it won `| last` over the real bare trigger β€” the one actually carrying +# Greptile's πŸ‘. No wrong SKIP was ever observed (that comment happened to carry no reaction, so +# condition (2) failed), but the detector demonstrably picked it, and the `Digest` stage emits +# comments of that shape systematically. +# +# The stripper below is deliberately OVER-aggressive (first-to-last backtick per line, greedy HTML +# comments, an unterminated fence swallowing to EOF). Every gap therefore DROPS a candidate, and a +# dropped candidate costs at most one redundant `@greptileai` β€” it can never invent a trigger and +# skip a review. Bias the direction; do not make the parser clever. +# +# NO positional variables ($0/$1/…) anywhere in this block. Skill argument substitution rewrites +# them before the text reaches the agent, so `/sweep 947` turned `awk 'NF{last=$0}'` into +# `awk 'NF{last=947}'` (#951) β€” inside this very gate. The awk program therefore reads records with +# `getline` into a NAMED variable and splits fields into an array. Do not reintroduce $0/$1 here; +# `scripts/greptile-trigger-guard-check.sh` lints for them. +trigger_pair=$(printf '%s\n' "$trigger_candidates" | awk ' + BEGIN { + # Dynamic regex so the handle stays a single source of truth; tolower() on both sides makes the + # match case-insensitive the way GitHub mentions are. Kept as a variable (not inlined) so every + # fleet copy of this block is textually identical apart from the handle β€” the drift #962 exists + # to stop. + mention = tolower("@greptileai") + # A fence opener, built rather than written: a literal triple-backtick INSIDE this fenced + # block truncates any extractor that scans for the closing fence non-greedily β€” including + # a harness that executes this gate end-to-end. CommonMark itself would not close the fence + # on an 8-space-indented line, but "correct per spec" is no help when the tools that read + # this file disagree. Do not inline it back. + fence = sprintf("%c%c%c", 96, 96, 96) + while ((getline rec) > 0) { + if (rec == "") continue + nf = split(rec, fld, "\t") + if (nf < 3) continue # malformed record β€” drop (fail safe) + body = fld[3] + for (i = 4; i <= nf; i++) body = body "\t" fld[i] + if (substr(body, 1, 1) != "\"") continue # not a JSON string literal β€” drop + if (substr(body, length(body), 1) != "\"") continue + body = substr(body, 2, length(body) - 2) + gsub(//, " ", body) # HTML comments never notify anyone + sub(/ + +if [ -z "$trigger_id" ]; then + # (1) fails β€” no trigger has ever reflected this PR. This is the case the old reply-reaction + # shortcut got wrong: a πŸ‘ on a reply is not a trigger, so we MUST post one. + echo "No @greptileai trigger comment exists β€” posting trigger." + gh api repos//issues//comments -f body="@greptileai" else - trigger_id=$(gh api repos//issues//comments -f body="@greptileai" --jq '.id') + # Trigger timestamp (ISO-8601 β†’ lexicographically comparable as a string) came from the same + # record as the id, so it needs no second fetch. Keep the guard anyway: an empty created_at + # would leave conditions (3)/(4) unable to run. + if [ -z "$trigger_ts" ]; then + # Fail safe: we found a trigger id but could not resolve its timestamp, so the + # after-trigger inline-comment check (3) and the push-staleness check (4) can't + # run. Never assume the conditions they control are met β€” post the trigger. + echo "Could not resolve trigger timestamp β€” posting @greptileai (fail safe)." + gh api repos//issues//comments -f body="@greptileai" + exit 0 + fi + + # (2) positive reaction ON THE TRIGGER (never on a reply). + positive=$(gh api "repos//issues/comments/$trigger_id/reactions" \ + --jq '[.[] | select(.user.login == "greptile-apps[bot]" and (.content == "+1" or .content == "hooray" or .content == "heart" or .content == "rocket"))] | length' 2>/dev/null || echo 0) + + # (3) any Greptile comment after the trigger: issue stream (id-ordered) + inline stream (time-ordered). + # Capture the RAW per-page `length` lines (one per page under --paginate) so a failed fetch is + # detectable: `--jq '... | length'` emits "0" on a successful fetch with no matches but NOTHING on + # a failed one. Don't pipe straight into a summing awk (`print s+0` emits "0" on empty stdin, making + # a failed fetch indistinguishable from zero comments and silently satisfying this condition). + # trigger_ts is guaranteed non-empty here (guarded above). + issue_raw=$(gh api repos//issues//comments --paginate \ + --jq "[.[] | select(.user.login == \"greptile-apps[bot]\" and .id > $trigger_id)] | length" || echo "") + inline_raw=$(gh api repos//pulls//comments --paginate \ + --jq "[.[] | select(.user.login == \"greptile-apps[bot]\" and .created_at > \"$trigger_ts\")] | length" || echo "") + if [ -z "$issue_raw" ] || [ -z "$inline_raw" ]; then + # Fail safe (same reasoning as the trigger_ts/head_ts guards): a failed fetch can't prove + # "no new comments" β€” post the trigger rather than assume condition (3) is met. + echo "Could not fetch after-trigger comments β€” posting @greptileai (fail safe)." + gh api repos//issues//comments -f body="@greptileai" + exit 0 + fi + # Sum the per-page counts from each stream. Shell arithmetic, NOT `awk '{s+=$1}'`: a positional + # awk variable in skill text is rewritten by argument substitution before the agent ever sees it + # (#951), so under `/sweep 947` that awk became `{s+=947}` β€” adding 947 per page and leaving this + # condition unable to discriminate. A non-numeric count means the fetch returned something + # unexpected, which cannot prove "no new comments" β€” post rather than assume. + # + # The loop is fed by process substitution, NOT a here-doc: a here-doc terminator must sit at + # column 0, so the block breaks the moment a fleet copy INDENTS this gate β€” and one already does, + # wrapping it in a function so its early-outs can `return` rather than `exit`. `< <(…)` also keeps + # the loop in THIS shell, so the accumulator survives; `printf | while` would lose it to a + # subshell. Both shells in the guard's matrix support it, as the `comm -23 <(…)` above does. + comments_after=0 + while IFS= read -r page_count; do + [ -n "$page_count" ] || continue + case "$page_count" in + *[!0-9]*) + echo "Non-numeric after-trigger count ('$page_count') β€” posting @greptileai (fail safe)." + gh api repos//issues//comments -f body="@greptileai" + exit 0 + ;; + esac + comments_after=$((comments_after + page_count)) + done < <(printf '%s\n%s\n' "$issue_raw" "$inline_raw") + + # (4) has Greptile reviewed the CURRENT head? Prefer Greptile's OWN direct evidence over any + # timestamp proxy: its sticky summary comment ends with a footer naming the exact commit it + # reviewed β€” + # Reviews (N): Last reviewed commit: [""](https://github.com///commit/<40-hex>) + # β€” so when that SHA equals the PR head, Greptile HAS reviewed the head and (4) holds regardless + # of commit/push timestamps. This is load-bearing because Greptile re-reviews by EDITING that + # summary IN PLACE (same comment id, no new comment), so timestamps alone cannot observe a + # completed re-review. Observed on PR #930: the marker named the exact head at Confidence + # Score 5/5, yet the timestamp proxy read "commit newer than trigger" and posted `@greptileai` + # twice more β€” review #9 of a commit already approved, and a converged sweep that looked + # unconverged. Timestamps stay as the FALLBACK only (see the else branch). + head_sha=$(gh pr view --repo \ + --json headRefOid --jq '.headRefOid // empty' 2>/dev/null | tr -d '[:space:]' | tr 'A-Z' 'a-z') + if [ -z "$head_sha" ]; then + # Fail safe (same reasoning as the trigger_ts guard above): with no head SHA, neither the + # marker comparison nor the timestamp fallback can prove the trigger reflects the current + # head. Post rather than assume β€” a failed fetch must never count as satisfied. + echo "Could not resolve PR head SHA β€” posting @greptileai (fail safe)." + gh api repos//issues//comments -f body="@greptileai" + exit 0 + fi + + # Every SHA Greptile has published as "last reviewed" (normally one β€” the sticky summary). + # `--jq` only SELECTS the bodies; grep does the extraction, so this needs neither a standalone + # `jq` binary nor jq-flavour named-capture regex. grep is line-oriented and the footer is a + # single line, so `.*` cannot run past it into another comment's link. + # inline-sweep GREPTILE-LAST-REVIEWED extractor [#930] + reviewed_shas=$(gh api repos//issues//comments --paginate \ + --jq '.[] | select(.user.login == "greptile-apps[bot]") | .body' 2>/dev/null \ + | grep -o 'Last reviewed commit:.*/commit/[0-9a-fA-F]\{40\}' \ + | grep -o '[0-9a-fA-F]\{40\}$' | tr 'A-Z' 'a-z') + + if [ -n "$reviewed_shas" ] && printf '%s\n' "$reviewed_shas" | grep -qx "$head_sha"; then + # Direct evidence: a published Greptile review names the current head. Not stale. + pushed_after=0; head_basis="marker" + elif [ -n "$reviewed_shas" ]; then + # Direct evidence the other way: every review Greptile published names some OTHER commit, + # so it has not reviewed this head. Stale β€” post. + pushed_after=1; head_basis="marker" + else + # FALLBACK ONLY β€” no parsable marker (Greptile has not summarised yet, the footer format + # changed, or the fetch failed). Empty output cannot distinguish those three, and all of them + # land on the pre-existing behaviour below, so this path is never weaker than it was. + # Latest commit's committedDate is a proxy for push time β€” if it post-dates the trigger, the + # trigger predates your fix and is stale. + # Caveat: committedDate is the local commit/amend time, not the true push time, so a cherry-pick, + # a timestamp-preserving rebase, or `git commit --date` can leave it BEFORE a later push and + # yield a false "not stale" (pushed_after=0). This errs toward skipping, but Step 2i's mandatory + # final run of this gate is the backstop. For an exact push time, use the GraphQL `pushedDate`. + head_ts=$(gh pr view --repo \ + --json commits --jq '.commits[-1].committedDate // empty' 2>/dev/null || echo "") + if [ -z "$head_ts" ]; then + # Fail safe (same reasoning as the trigger_ts guard above): if the head commit time + # can't be fetched, the staleness check (4) can't run. Don't let it short-circuit to + # "not stale" β€” post the trigger so a failed fetch never silently counts as satisfied. + echo "Could not resolve head commit time β€” posting @greptileai (fail safe)." + gh api repos//issues//comments -f body="@greptileai" + exit 0 + fi + # awk does the string compare (portable across bash/zsh; avoids `[ \> ]`, which zsh rejects). + # ISO-8601 values are non-numeric, so awk compares them lexically = chronologically. + # Both h and t are guaranteed non-empty here (the fail-safes above exit otherwise). + pushed_after=$(awk -v h="$head_ts" -v t="$trigger_ts" 'BEGIN{print (h>t) ? 1 : 0}') + head_basis="timestamp-proxy" + fi + + if [ "$positive" -gt 0 ] && [ "$comments_after" -eq 0 ] && [ "$pushed_after" -eq 0 ]; then + echo "Greptile satisfied with current head $head_sha (reacted to trigger $trigger_id; no new comments; head reviewed per $head_basis) β€” skipping re-trigger." + else + echo "Not satisfied (positive=$positive comments_after=$comments_after pushed_after=$pushed_after basis=$head_basis) β€” posting @greptileai." + gh api repos//issues//comments -f body="@greptileai" + fi fi ``` @@ -401,9 +632,25 @@ After re-triggering: ### 2h.1. Final fresh check β€” do this immediately before reporting, every time -Right before you write your Step 2i result, re-run Step 2d + 2d.1 **one more time, live** β€” regardless of how confident you are that things are settled. Comments arrive asynchronously; a check from even 10–15 minutes ago can already be stale, and reporting `Status: ready` on stale data is worse than reporting late or as `needs-human-review`. If this final check turns up anything new, handle it (reply, and re-trigger only if the Step 2g cap allows it) before finalizing. Only write your Step 2i block once this last check is clean, or you've hit a hard stop (the 50-trigger cap, or 3 rounds of CI fixes). +Right before you write your Step 2j result, re-run Step 2d + 2d.1 **one more time, live** β€” regardless of how confident you are that things are settled. Comments arrive asynchronously; a check from even 10–15 minutes ago can already be stale, and reporting `Status: ready` on stale data is worse than reporting late or as `needs-human-review`. If this final check turns up anything new, handle it (reply, and re-trigger only if the Step 2g cap allows it) before finalizing. Only write your Step 2i block once this last check is clean, or you've hit a hard stop (the 50-trigger cap, or 3 rounds of CI fixes). + +### 2i. Final Greptile re-trigger (mandatory) + +After all work on the PR is complete β€” CI green, every comment addressed, every reply posted β€” **run the Greptile re-trigger gate from Step 2g one final time** (verify reply coverage first, then run the gate). This step is **mandatory and must never be skipped**: the gate itself decides whether to post `@greptileai`, and it skips the post *only* when Greptile is verifiably satisfied with the current head. + +Greptile is **satisfied** only when **all four** of the gate's conditions hold: +1. An `@greptileai` *trigger comment* exists (posted by a non-Greptile user) β€” and it **really + mentions** Greptile. A comment that merely contains the literal `@greptileai` inside a code + span, a fenced block, an indented block, or an HTML comment notifies nobody, so it is **not** a + trigger; the gate strips those before testing. Left untested, the detector itself would hand + conditions (1) and (2) to exactly the reply-reaction the warning above rejects. +2. Greptile reacted to **that trigger** with a positive emoji (πŸ‘, πŸŽ‰, ❀️, or πŸš€). +3. Greptile posted **no** new comment (issue-style or inline) after that trigger. +4. **Greptile has reviewed the current head.** Take this from Greptile's own summary footer β€” `Reviews (N): Last reviewed commit: [...](.../commit/)` β€” and require that `` to equal `gh pr view --json headRefOid`. Greptile re-reviews by editing that summary **in place**, so a "was anything pushed after the trigger?" timestamp check cannot see a re-review that already happened and will re-trigger a commit Greptile has already approved. The commit-timestamp comparison remains only as the fallback for when the marker can't be parsed. + +Do **not** skip the trigger on any other basis. In particular, a positive reaction on one of *your replies* is **not** satisfaction β€” that mistake leaves your fix un-reviewed and the mandatory final trigger unsent. When in doubt, the gate errs toward posting; let it run rather than second-guessing it. -### 2i. Return result +### 2j. Return result At the end of processing, the subagent MUST return a structured result with these fields so the main agent can build the summary table: @@ -456,4 +703,4 @@ If any subagent failed or returned an error, note it in the Status column as `ag - **No fake "something will notify me" framing.** A background job, watcher, or "monitor" you start yourself does not resume your turn when it fires β€” only your own next tool call does. Do not write or act on "is running and will notify me," "remains armed," or "I'll pick this back up when re-prompted." If you're about to write something like that, make another tool call instead. - **Respect the shared GitHub API rate limit.** All subagents in a sweep (and any concurrent session) share one identity's quota. Poll no tighter than every 60–120s and batch checks rather than looping per-item. `gh api rate_limit` is free to check. If you hit `API rate limit exceeded`, bridge the wait yourself with bounded sleep-then-recheck cycles against the `reset` epoch (see "Mind the GitHub API rate limit") β€” never assume it will clear on its own without you checking, and never spam retries while it's still exhausted. - **The 50-Greptile-trigger cap is counted from live data (actual `@greptileai` comments posted), not from memory of "how many rounds I've done."** Check it mechanically (Step 2g) before every trigger. Once hit, stop triggering even if you just fixed a real bug β€” reply, don't trigger, and report `needs-human-review`. -- **Never declare `Status: ready` from a stale check.** Re-verify comments and CI live, immediately before writing your Step 2i result (Step 2h.1) β€” not from a check earlier in the session, and not just because you feel confident nothing more is coming. The orchestrator relaying a manual spot-check to you is not a substitute for this either β€” always do your own final live check. +- **Never declare `Status: ready` from a stale check.** Re-verify comments and CI live, immediately before writing your Step 2j result (Step 2h.1) β€” not from a check earlier in the session, and not just because you feel confident nothing more is coming. The orchestrator relaying a manual spot-check to you is not a substitute for this either β€” always do your own final live check. diff --git a/.github/scripts/test_sweep_greptile_gate.py b/.github/scripts/test_sweep_greptile_gate.py new file mode 100644 index 000000000..cd5aa4ddf --- /dev/null +++ b/.github/scripts/test_sweep_greptile_gate.py @@ -0,0 +1,538 @@ +"""Shell-tier CI test for the /sweep Greptile re-trigger gate (PR #930 false negative). + +Runs THE ACTUAL gate embedded in `.claude/skills/sweep/SKILL.md` (single-source; no +copy) against a corpus of scenarios, driving it with a stub `gh` on PATH. If the gate +in SKILL.md changes, this test runs the changed code. + +## What it pins + +The gate skips the mandatory `@greptileai` re-trigger only when Greptile is verifiably +satisfied with the CURRENT head. Condition (4) β€” "has Greptile reviewed this head?" β€” +used to be answered with a timestamp proxy (`commits[-1].committedDate > trigger +created_at`). That proxy has a reproducible FALSE NEGATIVE, because Greptile re-reviews +by editing its sticky summary comment IN PLACE: no new comment, no new `created_at`, so +a completed re-review is invisible to timestamps. + +Observed on PR #930 (2026-08-10), reproduced verbatim in SCENARIOS below: + + trigger 5237591525 2026-08-10T08:11:42Z (+1 from greptile-apps[bot]) + commit 51ae6c23 2026-08-10T08:14:51Z <- committedDate AFTER the trigger + summary "Reviews (8): Last reviewed commit: .../commit/51ae6c23..." <- == head + +The proxy read "commit newer than trigger" -> stale -> posted `@greptileai` again, which +produced review #9 of a commit Greptile had already scored 5/5. The fix prefers Greptile's +own `Last reviewed commit` marker and falls back to the proxy only when it can't parse it. + +`test_mutation_*` are the load-bearing tests: they mutate the gate back to the pre-fix +behaviour and assert the #930 scenario FLIPS to posting. Without them, the regression +assertion could pass with and against the bug it names. + +Run: python -m pytest .github/scripts/test_sweep_greptile_gate.py + or: cd .github/scripts && python3 test_sweep_greptile_gate.py (stdlib runner below) + +Requires `jq` (the stub `gh` evaluates the gate's real `--jq` filters with it, so a broken +filter fails the test instead of being mocked away). Pre-installed on GitHub-hosted runners. +""" + +from __future__ import annotations + +import json +import os +import re +import shutil +import subprocess +import sys +import tempfile +from pathlib import Path + +REPO = Path(__file__).resolve().parents[2] +# Where this file sits decides where the skill sits: +# a seeded repo .github/scripts/ -> /.claude/skills/sweep/SKILL.md +# the kit itself repo-foundry/templates/github/scripts/ -> repo-foundry/templates/claude/skills/sweep/SKILL.md +# The kit ships this test as a template AND runs it in place, for the same reason +# test_section_ref_lint.py is run in place: these tests are the kit's only regression guard +# on the template it ships, so a template that rots would otherwise fail silently. +_SKILL_CANDIDATES = ( + REPO / ".claude" / "skills" / "sweep" / "SKILL.md", + REPO / "claude" / "skills" / "sweep" / "SKILL.md", +) +SKILL = next((p for p in _SKILL_CANDIDATES if p.is_file()), _SKILL_CANDIDATES[0]) + +# Sentinel identifying the gate's fenced bash block inside SKILL.md. +GATE_SENTINEL = "# === Greptile re-trigger gate (shared by Step 2g and Step 2i) ===" +# Load-bearing marker on the SHA extractor the fix introduces. The mutation tests locate +# the extractor by this marker, so moving/renaming it fails loudly instead of silently +# turning the mutation into a no-op (which would make the mutation "pass" for free). +EXTRACTOR_MARKER = "# inline-sweep GREPTILE-LAST-REVIEWED extractor [#930]" + +PR = "930" +# `` and `` are the two placeholders SKILL.md tells the agent to substitute, so +# substituting them here is exactly what a real run does. `` must be filled even though the +# stub `gh` ignores it: left in place, bash reads `--repo ` as a redirect from a file named +# `repo` and the gate dies before reaching condition (4). Copies that already carry a real slug +# (this one does) are unaffected β€” the replace is then a no-op. Kept so every fleet copy of this +# test is identical, and so the repo-foundry template (which does carry ``) can run it. +REPO_SLUG = "optave/data-retrieval-storage-svc" +# The kit parameterises the reviewer handle; installed copies carry the literal. Substituting +# it here is, like `` above, exactly what an install does β€” and a no-op in any copy that +# already carries the literal, so every fleet copy of this test stays identical. +REVIEWER_BOT_TOKEN = "{{REVIEWER_BOT}}" +REVIEWER_BOT = "@greptileai" +HEAD = "51ae6c237998c0472a7cfeec1be75cb020e3c4a5" +OTHER = "7a2c4067b1c9e4d2a8f60513be7cc1a94d2e8f01" +TRIGGER_ID = 5237591525 +TRIGGER_TS = "2026-08-10T08:11:42Z" +SUMMARY_ID = 5230439302 # < TRIGGER_ID, so it is not an "after the trigger" comment +BOT = "greptile-apps[bot]" + +# A push that lands AFTER the trigger β€” what makes the old timestamp proxy cry "stale". +COMMIT_AFTER_TRIGGER = "2026-08-10T08:14:51Z" +COMMIT_BEFORE_TRIGGER = "2026-08-10T08:00:00Z" + + +def _summary_body(sha: str | None, reviews: int = 8) -> str: + """Greptile's sticky summary. `sha=None` omits the footer marker entirely.""" + body = ( + "### Greptile Summary\n\n" + "Confidence Score: 5/5\n\n" + "No blocking failure remains; the change is scoped and documented.\n\n" + "\n\n" + ) + if sha is None: + return ( + body + + "[Re-trigger Greptile](https://app.greptile.com/api/retrigger?id=51550856)" + ) + # Byte-for-byte the shape observed on #930, including the HTML-escaped subject and the + # second, NON-commit link after the marker (which a greedy `.*` must not swallow past). + return body + ( + f"Reviews ({reviews}): Last reviewed commit: " + f"["docs(skills): cut fake edges..."]" + f"(https://github.com/optave/data-retrieval-storage-svc/commit/{sha}) | " + f"[Re-trigger Greptile](https://app.greptile.com/api/retrigger?id=51550856)" + ) + + +def _fixture( + *, + marker_sha: str | None = HEAD, + head: str = HEAD, + commit_ts: str = COMMIT_AFTER_TRIGGER, + trigger_reaction: bool = True, + reply_reaction: bool = False, + inline_after_trigger: bool = False, + include_trigger: bool = True, + fail: list[str] | None = None, +) -> dict: + issue_comments = [ + { + "id": SUMMARY_ID, + "user": {"login": BOT}, + "created_at": "2026-08-09T07:49:31Z", + "body": _summary_body(marker_sha), + } + ] + if include_trigger: + issue_comments.append( + { + "id": TRIGGER_ID, + "user": {"login": "carlos-alm"}, + "created_at": TRIGGER_TS, + "body": "@greptileai", + } + ) + inline_comments = [ + # A Greptile inline finding from BEFORE the trigger β€” must not count for condition (3). + { + "id": 3743075822, + "user": {"login": BOT}, + "created_at": "2026-08-09T07:49:35Z", + "in_reply_to_id": None, + "body": "nit: stale wording", + } + ] + if inline_after_trigger: + inline_comments.append( + { + "id": 3799000001, + "user": {"login": BOT}, + "created_at": "2026-08-10T08:20:00Z", + "in_reply_to_id": None, + "body": "new finding on the current head", + } + ) + reactions: dict[str, list] = {str(TRIGGER_ID): [], "3743075822": []} + if trigger_reaction: + reactions[str(TRIGGER_ID)] = [{"user": {"login": BOT}, "content": "+1"}] + if reply_reaction: + # A πŸ‘ on one of OUR replies. Never a satisfied signal β€” the rule this fix must not weaken. + issue_comments.append( + { + "id": 5237600000, + "user": {"login": "carlos-alm"}, + "created_at": "2026-08-10T08:12:00Z", + "body": "Addressed in 51ae6c2.", + } + ) + reactions["5237600000"] = [{"user": {"login": BOT}, "content": "+1"}] + return { + "headRefOid": head, + "commits": [{"oid": head, "committedDate": commit_ts}], + "issue_comments": issue_comments, + "inline_comments": inline_comments, + "reactions": reactions, + "fail": fail or [], + } + + +# The stub `gh`. Serves fixture JSON through the REAL `jq`, so the gate's own `--jq` +# filters are exercised rather than mocked. Records every comment POST it is asked to make. +GH_STUB = r"""#!/usr/bin/env python3 +import json, os, re, subprocess, sys + +fx = json.load(open(os.environ["GH_STUB_FIXTURE"])) +argv = sys.argv[1:] + +def jq(filt, payload): + if filt is None: + print(json.dumps(payload)) + return 0 + p = subprocess.run(["jq", "-r", filt], input=json.dumps(payload), + capture_output=True, text=True) + sys.stdout.write(p.stdout) + sys.stderr.write(p.stderr) + return p.returncode + +def opt(names): + for n in names: + if n in argv: + i = argv.index(n) + if i + 1 < len(argv): + return argv[i + 1] + return None + +filt = opt(["--jq", "-q"]) + +def fail_if(key): + if key in fx["fail"]: + sys.exit(1) + +if argv[0] == "pr" and argv[1] == "view": + which = opt(["--json"]) + if which == "headRefOid": + fail_if("headRefOid") + sys.exit(jq(filt, {"headRefOid": fx["headRefOid"]})) + if which == "commits": + fail_if("commits") + sys.exit(jq(filt, {"commits": fx["commits"]})) + sys.exit(1) + +if argv[0] == "api" and len(argv) > 1 and argv[1] == "graphql": + # `gh api graphql -f query='...'` β€” the GraphQL pushedDate lookup some fleet copies use as + # their timestamp FALLBACK instead of `gh pr view --json commits`. Must be matched BEFORE the + # `-f` POST branch below: the query rides in on `-f query=`, so the POST branch would swallow + # it and record the query text as a posted comment body. Serve it from the same fixture the + # `commits` path uses, with a null pushedDate so the gate's `(.pushedDate // .committedDate)` + # resolves to the scenario's commit timestamp. Inert in copies that never call graphql, which + # keeps this file byte-identical across the fleet. + fail_if("commits") + sys.exit(jq(filt, {"data": {"repository": {"pullRequest": {"commits": {"nodes": [ + {"commit": {"committedDate": fx["commits"][-1]["committedDate"], "pushedDate": None}} + ]}}}}})) + +if argv[0] == "api": + url = next(a for a in argv[1:] if not a.startswith("-")) + # A POST: `gh api -f body=@greptileai` + if "-f" in argv: + body = opt(["-f"]) + with open(os.environ["GH_STUB_POSTS"], "a") as fh: + fh.write(body + "\n") + print('{"id": 1}') + sys.exit(0) + if re.search(r"/issues/comments/(\d+)/reactions$", url): + fail_if("reactions") + cid = re.search(r"/issues/comments/(\d+)/reactions$", url).group(1) + sys.exit(jq(filt, fx["reactions"].get(cid, []))) + if re.search(r"/issues/\d+/comments$", url): + fail_if("issue_comments") + sys.exit(jq(filt, fx["issue_comments"])) + if re.search(r"/pulls/\d+/comments$", url): + fail_if("inline_comments") + sys.exit(jq(filt, fx["inline_comments"])) + sys.exit(1) + +sys.exit(1) +""" + + +def _gate_source() -> str: + """Pull the ACTUAL gate out of .claude/skills/sweep/SKILL.md. + + Single-source: if the gate in SKILL.md changes, this test runs the changed code. + The gate is the one ```bash fence carrying GATE_SENTINEL; `` is the + placeholder SKILL.md tells the agent to substitute, so substituting it here is + exactly what a real run does. + """ + text = SKILL.read_text("utf-8") + blocks = re.findall(r"```bash\n(.*?)```", text, re.S) + gates = [b for b in blocks if GATE_SENTINEL in b] + assert len(gates) == 1, ( + f"expected exactly one bash fence carrying the gate sentinel, got {len(gates)}" + ) + return ( + gates[0] + .replace("", PR) + .replace("", REPO_SLUG) + .replace(REVIEWER_BOT_TOKEN, REVIEWER_BOT) + ) + + +def _shells() -> list[str]: + """Shells to run the gate under. + + bash is CI's shell; zsh is the maintainers' interactive session shell, and a /sweep guard + in a sibling repo once SILENTLY PASSED under zsh while failing its job (it iterated an + unquoted parameter, which zsh does not word-split). A gate that is only ever proven under + bash is not proven for the shell it actually runs in, so both are required when present. + Set REQUIRE_SHELLS=1 to turn a missing shell into a failure instead of a skip. + """ + found = [s for s in ("bash", "zsh") if shutil.which(s)] + missing = [s for s in ("bash", "zsh") if not shutil.which(s)] + if missing and os.environ.get("REQUIRE_SHELLS") == "1": + raise AssertionError(f"REQUIRE_SHELLS=1 but these shells are absent: {missing}") + assert "bash" in found, "bash is required to run the gate" + return found + + +def _run_gate( + fixture: dict, source: str | None = None, shell: str = "bash" +) -> tuple[str, list[str]]: + """Run the gate under `shell` with a stub `gh` on PATH. Returns (stdout, posted bodies).""" + if shutil.which("jq") is None: + raise AssertionError("this test needs `jq` to evaluate the gate's real --jq filters") + src = _gate_source() if source is None else source + with tempfile.TemporaryDirectory() as td: + td_p = Path(td) + bindir = td_p / "bin" + bindir.mkdir() + gh = bindir / "gh" + gh.write_text(GH_STUB, "utf-8") + gh.chmod(0o755) + (td_p / "fixture.json").write_text(json.dumps(fixture), "utf-8") + posts = td_p / "posts.txt" + posts.write_text("", "utf-8") + script = td_p / "gate.sh" + script.write_text(src, "utf-8") + env = { + **os.environ, + "PATH": f"{bindir}:{os.environ['PATH']}", + "GH_STUB_FIXTURE": str(td_p / "fixture.json"), + "GH_STUB_POSTS": str(posts), + } + proc = subprocess.run([shell, str(script)], capture_output=True, text=True, env=env) + assert proc.returncode == 0, ( + f"gate exited {proc.returncode} under {shell}\n" + f"stdout:\n{proc.stdout}\nstderr:\n{proc.stderr}" + ) + posted = [ln for ln in posts.read_text("utf-8").splitlines() if ln.strip()] + return proc.stdout, posted + + +# (name, fixture kwargs, expect_post, why) +SCENARIOS = [ + # ── THE #930 REGRESSION ──────────────────────────────────────────────────────────── + # Greptile's marker names the exact head, it reacted +1 to the trigger, and it posted + # nothing since β€” yet a commit's committedDate post-dates the trigger. The old proxy + # posted here. It must now skip. + ( + "marker_matches_head_regression", + {}, + False, + "marker names the current head β€” satisfied despite a newer committedDate", + ), + # Case-insensitivity: a SHA is a SHA. A case mismatch must not resurface the bug. + ( + "marker_matches_head_uppercase", + {"marker_sha": HEAD.upper()}, + False, + "marker matches the head case-insensitively", + ), + # ── DIRECT EVIDENCE THE OTHER WAY ────────────────────────────────────────────────── + ( + "marker_names_other_commit", + {"marker_sha": OTHER}, + True, + "Greptile's last review names a different commit β€” it has not seen this head", + ), + # ── FALLBACK PRESERVED (marker unparsable) ───────────────────────────────────────── + ( + "no_marker_commit_after_trigger", + {"marker_sha": None}, + True, + "no marker -> timestamp proxy -> commit newer than trigger -> stale", + ), + ( + "no_marker_commit_before_trigger", + {"marker_sha": None, "commit_ts": COMMIT_BEFORE_TRIGGER}, + False, + "no marker -> timestamp proxy -> nothing pushed since -> satisfied (pre-existing)", + ), + # ── FAIL-SAFE ────────────────────────────────────────────────────────────────────── + ( + "head_sha_fetch_fails", + {"fail": ["headRefOid"]}, + True, + "cannot fetch the head -> post rather than assume satisfied", + ), + ( + "marker_fetch_fails_and_commits_fetch_fails", + {"fail": ["commits"], "marker_sha": None}, + True, + "no marker and no commit time -> post (fail safe)", + ), + # ── THE OTHER THREE CONDITIONS MUST STILL BITE ───────────────────────────────────── + ( + "no_trigger_comment_at_all", + {"include_trigger": False}, + True, + "condition (1): no trigger has ever reflected this PR", + ), + ( + "reaction_only_on_our_reply", + {"trigger_reaction": False, "reply_reaction": True}, + True, + "condition (2): a πŸ‘ on OUR reply is never satisfaction β€” only one on the trigger counts", + ), + ( + "greptile_commented_after_trigger", + {"inline_after_trigger": True}, + True, + "condition (3): a new inline finding outranks a matching marker", + ), +] + + +def test_gate_scenarios(): + for shell in _shells(): + for name, kwargs, expect_post, why in SCENARIOS: + out, posted = _run_gate(_fixture(**kwargs), shell=shell) + got_post = len(posted) > 0 + assert got_post == expect_post, ( + f"[{shell}/{name}] expected {'POST' if expect_post else 'SKIP'}, got " + f"{'POST' if got_post else 'SKIP'} β€” {why}\ngate said: {out.strip()}" + ) + if got_post: + assert posted == ["body=@greptileai"], ( + f"[{shell}/{name}] unexpected post payload {posted!r}" + ) + + +def test_regression_skip_cites_the_marker_not_the_proxy(): + """The #930 case must be decided BY THE MARKER. A skip reached via the timestamp + proxy would be the right answer for the wrong reason and would not survive a real + push, so pin the basis the gate reports.""" + out, posted = _run_gate(_fixture()) + assert not posted + assert "basis" not in out, f"a satisfied gate should not report a not-satisfied basis: {out}" + assert "head reviewed per marker" in out, ( + f"expected the marker to be the deciding basis, got: {out}" + ) + assert HEAD in out, f"expected the satisfied head SHA in the message, got: {out}" + + +def test_fallback_skip_cites_the_proxy(): + out, posted = _run_gate(_fixture(marker_sha=None, commit_ts=COMMIT_BEFORE_TRIGGER)) + assert not posted + assert "head reviewed per timestamp-proxy" in out, f"expected the proxy basis, got: {out}" + + +# ── Mutation tests ──────────────────────────────────────────────────────────────────── +# The scenario table above is only meaningful if `marker_matches_head_regression` actually +# depends on the fix. These mutate the gate back to pre-fix behaviour and require it to fail. + + +def _mutate_extractor(replacement: str) -> str: + """Replace the marker-SHA extractor (located by its load-bearing marker) in the real gate.""" + src = _gate_source() + assert EXTRACTOR_MARKER in src, f"extractor marker missing from the gate: {EXTRACTOR_MARKER!r}" + # The extractor runs from its marker line to the end of the `reviewed_shas=$(...)` pipeline. + pattern = re.compile( + re.escape(EXTRACTOR_MARKER) + r"\n\s*reviewed_shas=\$\(.*?tr 'A-Z' 'a-z'\)\n", + re.S, + ) + mutated, n = pattern.subn(replacement, src) + assert n == 1, f"expected to mutate exactly one extractor, matched {n}" + return mutated + + +def test_mutation_removing_the_marker_lookup_reintroduces_the_930_bug(): + """Pre-fix behaviour: with no marker SHA the gate falls back to the committedDate proxy, + which is byte-identical to the code that shipped the bug. The #930 scenario must POST.""" + mutated = _mutate_extractor(' reviewed_shas=""\n') + out, posted = _run_gate(_fixture(), source=mutated) + assert posted == ["body=@greptileai"], ( + "MUTATION NOT DETECTED: with the marker lookup removed, the #930 scenario still " + f"skipped the re-trigger β€” the regression assertion is not load-bearing. " + f"Gate said: {out.strip()}" + ) + assert "basis=timestamp-proxy" in out, ( + f"expected the mutant to fall back to the proxy, got: {out}" + ) + + +def test_mutation_breaking_the_sha_regex_reintroduces_the_930_bug(): + """A subtler mutant: keep the extractor but make its SHA pattern wrong. It must not + silently still pass β€” the extractor's precision is part of the fix.""" + broken = _gate_source() + assert broken.count(r"[0-9a-fA-F]\{40\}") == 2, "expected two 40-hex patterns in the extractor" + mutated = broken.replace(r"[0-9a-fA-F]\{40\}", r"[0-9a-fA-F]\{41\}") + out, posted = _run_gate(_fixture(), source=mutated) + assert posted == ["body=@greptileai"], ( + "MUTATION NOT DETECTED: a broken SHA pattern still produced a satisfied gate β€” the " + f"extractor is not actually being exercised. Gate said: {out.strip()}" + ) + + +def test_mutation_ignoring_a_marker_mismatch_is_caught(): + """The inverse guard: a mutant that treats ANY marker as satisfaction (dropping the + equality check) must be caught by the `marker_names_other_commit` scenario.""" + src = _gate_source() + old = ( + ' if [ -n "$reviewed_shas" ] && printf \'%s\\n\' "$reviewed_shas"' + ' | grep -qx "$head_sha"; then' + ) + assert old in src, "could not locate the marker/head equality check to mutate" + mutated = src.replace(old, ' if [ -n "$reviewed_shas" ]; then') + # The mutant accepts ANY marker as satisfaction, so it SKIPS where the real gate POSTs. + # Seeing the mutant skip is what proves `marker_names_other_commit`'s POST is produced by + # the equality check and not by some other condition incidentally failing. + out, posted = _run_gate(_fixture(marker_sha=OTHER), source=mutated) + assert posted == [], ( + "MUTATION NOT DETECTED: dropping the marker==head equality check still produced a " + f"re-trigger, so `marker_names_other_commit` passes for some other reason and is not " + f"load-bearing. Gate said: {out.strip()}" + ) + + +TESTS = [ + test_gate_scenarios, + test_regression_skip_cites_the_marker_not_the_proxy, + test_fallback_skip_cites_the_proxy, + test_mutation_removing_the_marker_lookup_reintroduces_the_930_bug, + test_mutation_breaking_the_sha_regex_reintroduces_the_930_bug, + test_mutation_ignoring_a_marker_mismatch_is_caught, +] + + +if __name__ == "__main__": + failures = 0 + for t in TESTS: + try: + t() + except AssertionError as exc: # report every failure, don't stop at the first + failures += 1 + print(f"FAIL {t.__name__}: {exc}", file=sys.stderr) + else: + print(f"ok {t.__name__}") + print(f"\n{len(TESTS) - failures}/{len(TESTS)} passed") + sys.exit(1 if failures else 0) diff --git a/.github/workflows/sweep-gate.yml b/.github/workflows/sweep-gate.yml new file mode 100644 index 000000000..9899ced2f --- /dev/null +++ b/.github/workflows/sweep-gate.yml @@ -0,0 +1,59 @@ +# sweep-gate β€” execute the /sweep Greptile re-trigger gate against fixture PR state. +# +# The gate lives inside .claude/skills/sweep/SKILL.md as a fenced bash block, so nothing +# else in CI ever runs it: it is shipped, load-bearing, and otherwise unproven. A wrong +# verdict either DROPS reviewer feedback (skipped when it should post) or spams redundant +# re-reviews and makes a converged sweep look unconverged (posted when it should skip). +# +# The suite extracts the real fenced block from SKILL.md β€” no copy β€” and drives it with a +# stub `gh`, so a change to the gate is a change to the tested code. Three of its cases are +# mutation tests that revert the gate to the pre-fix timestamp proxy and REQUIRE the +# regression assertion to fail, so that assertion cannot rot into one that passes with and +# without the bug it names. +# +# zsh is installed and REQUIRED rather than skipped: it is the maintainers' interactive +# shell, and a /sweep guard in a sibling repo once silently PASSED under zsh while failing +# its job, because zsh does not word-split an unquoted parameter. jq is pre-installed on +# ubuntu-latest. Pure stdlib otherwise β€” no pip install. + +name: sweep-gate + +on: + pull_request: + paths: + - '.claude/skills/sweep/SKILL.md' + - '.github/scripts/test_sweep_greptile_gate.py' + - '.github/workflows/sweep-gate.yml' + workflow_dispatch: + +permissions: + contents: read + +concurrency: + group: sweep-gate-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + +jobs: + gate: + name: Greptile re-trigger gate (#930 regression) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-python@v5 + with: + python-version: '3.12' + + - name: Ensure zsh is present (the cross-shell proof must never silently degrade) + run: | + set -euo pipefail + if ! command -v zsh > /dev/null 2>&1; then + sudo apt-get update -qq && sudo apt-get install -y --no-install-recommends zsh + fi + zsh --version + + - name: Run the gate suite + working-directory: .github/scripts + env: + REQUIRE_SHELLS: "1" + run: python3 test_sweep_greptile_gate.py From 16b67ae07813bb77da78debb1402fb45de0c0994 Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Fri, 14 Aug 2026 18:16:25 -0600 Subject: [PATCH 2/3] fix(sweep): make the Greptile trigger gate fail safe on two comment-fetch failures (#2486) The trigger-comments fetch aborted the whole gate with exit 1 instead of posting @greptileai, so a transient API error during Step 2g or the mandatory Step 2i could stop the sweep short of the required final trigger. The reviewed-commit marker fetch piped gh api straight into grep, so a failed fetch was indistinguishable from a legitimate 'no marker yet' and could fall through to the timestamp-proxy fallback, silently permitting a skip of a head Greptile never actually reviewed. Both now check their own fetch's exit status directly and fail safe by posting rather than aborting or falling through. Adds two scenarios and two mutation tests to test_sweep_greptile_gate.py proving both fixes are load-bearing, and updates the doc comments that referenced the old mutation-test count. --- .claude/skills/sweep/SKILL.md | 30 ++++- .github/scripts/test_sweep_greptile_gate.py | 124 +++++++++++++++++++- .github/workflows/sweep-gate.yml | 7 +- 3 files changed, 151 insertions(+), 10 deletions(-) diff --git a/.claude/skills/sweep/SKILL.md b/.claude/skills/sweep/SKILL.md index 2426b1daf..d6c6adcbd 100644 --- a/.claude/skills/sweep/SKILL.md +++ b/.claude/skills/sweep/SKILL.md @@ -381,7 +381,15 @@ echo "All Greptile comments have replies β€” safe to re-trigger." # binary β€” same rationale as Step 0's guard). trigger_candidates=$(gh api repos//issues//comments --paginate \ --jq '.[] | select(.user.login != "greptile-apps[bot]") | "\(.id)\t\(.created_at)\t\((.body // "") | @json)"') \ - || { echo "FATAL: could not fetch trigger comments β€” aborting gate"; exit 1; } + || { + # Fail safe (same reasoning as every guard below): a failed fetch can't prove a trigger + # doesn't already exist, but it *also* can't prove one does β€” and aborting here used to + # exit the gate outright without posting, which on Step 2i's mandatory final run meant + # the sweep could stop having never sent the required last review trigger. Post instead. + echo "Could not fetch trigger comments β€” posting @greptileai (fail safe)." + gh api repos//issues//comments -f body="@greptileai" + exit 0 + } # # ── Decision half: pure text, NO network. Reads ONLY $trigger_candidates (newline-separated, @@ -558,9 +566,25 @@ else # `--jq` only SELECTS the bodies; grep does the extraction, so this needs neither a standalone # `jq` binary nor jq-flavour named-capture regex. grep is line-oriented and the footer is a # single line, so `.*` cannot run past it into another comment's link. + # + # The fetch is captured SEPARATELY from the grep extraction below, and ITS OWN exit status + # checked directly with `||` β€” piping straight into `grep -o` would erase the difference + # between "the API call failed" and "the API call succeeded and legitimately found no marker + # yet" (Greptile hasn't summarised, or the footer format changed): `grep` exits 1 on "no + # match" either way, so a guard on the whole pipeline would misfire on that ordinary, non-error + # case too and defeat itself. A genuine fetch failure must fail safe by posting β€” it must NOT + # fall through to the timestamp-proxy branch below as if no marker existed, because the proxy + # can read "nothing pushed since the trigger" and conclude satisfied, silently permitting a + # skip of a head Greptile's marker was never actually checked against. + reviewed_bodies=$(gh api repos//issues//comments --paginate \ + --jq '.[] | select(.user.login == "greptile-apps[bot]") | .body' 2>/dev/null) \ + || { + echo "Could not fetch Greptile comments for the reviewed-commit marker β€” posting @greptileai (fail safe)." + gh api repos//issues//comments -f body="@greptileai" + exit 0 + } # inline-sweep GREPTILE-LAST-REVIEWED extractor [#930] - reviewed_shas=$(gh api repos//issues//comments --paginate \ - --jq '.[] | select(.user.login == "greptile-apps[bot]") | .body' 2>/dev/null \ + reviewed_shas=$(printf '%s\n' "$reviewed_bodies" \ | grep -o 'Last reviewed commit:.*/commit/[0-9a-fA-F]\{40\}' \ | grep -o '[0-9a-fA-F]\{40\}$' | tr 'A-Z' 'a-z') diff --git a/.github/scripts/test_sweep_greptile_gate.py b/.github/scripts/test_sweep_greptile_gate.py index cd5aa4ddf..170f23621 100644 --- a/.github/scripts/test_sweep_greptile_gate.py +++ b/.github/scripts/test_sweep_greptile_gate.py @@ -23,9 +23,14 @@ produced review #9 of a commit Greptile had already scored 5/5. The fix prefers Greptile's own `Last reviewed commit` marker and falls back to the proxy only when it can't parse it. -`test_mutation_*` are the load-bearing tests: they mutate the gate back to the pre-fix -behaviour and assert the #930 scenario FLIPS to posting. Without them, the regression -assertion could pass with and against the bug it names. +`test_mutation_*` are the load-bearing tests: they mutate the gate back to a pre-fix +behaviour and assert the affected scenario FLIPS. Without them, the corresponding +regression assertion could pass with and against the bug it names. Three cover the +#930 marker-vs-timestamp regression above; two more (added in #2486) cover a pair of +comment-fetch failures that used to defeat this gate's own fail-safe design β€” one +aborted the whole gate instead of posting, the other let a failed marker fetch look +identical to "no marker yet" and fall through to a timestamp proxy that could +conclude satisfied. Run: python -m pytest .github/scripts/test_sweep_greptile_gate.py or: cd .github/scripts && python3 test_sweep_greptile_gate.py (stdlib runner below) @@ -254,7 +259,20 @@ def fail_if(key): cid = re.search(r"/issues/comments/(\d+)/reactions$", url).group(1) sys.exit(jq(filt, fx["reactions"].get(cid, []))) if re.search(r"/issues/\d+/comments$", url): - fail_if("issue_comments") + # Three call sites in the gate hit this SAME endpoint with different `--jq` filters: + # the trigger-candidates scan (`!=`), condition (3)'s after-trigger count (`.id >`), + # and condition (4)'s reviewed-commit marker fetch (selects `.body`, neither of the + # others' substrings). Distinguishing by filter content lets a test fail exactly ONE + # call site instead of every call to this URL, so fail-safe coverage can isolate each + # of the gate's three independent guards on this endpoint. + f = filt or "" + if "!=" in f: + fail_if("trigger_fetch") + elif ".id >" in f: + fail_if("issue_raw_fetch") + elif ".body" in f: + fail_if("reviewed_shas_fetch") + fail_if("issue_comments") # blanket: fail every call to this endpoint sys.exit(jq(filt, fx["issue_comments"])) if re.search(r"/pulls/\d+/comments$", url): fail_if("inline_comments") @@ -390,6 +408,19 @@ def _run_gate( True, "no marker and no commit time -> post (fail safe)", ), + ( + "trigger_candidates_fetch_fails", + {"fail": ["trigger_fetch"]}, + True, + "cannot fetch trigger comments -> post rather than abort the gate outright (#2486)", + ), + ( + "reviewed_shas_fetch_fails_must_not_fall_through", + {"fail": ["reviewed_shas_fetch"], "marker_sha": None, "commit_ts": COMMIT_BEFORE_TRIGGER}, + True, + "marker fetch fails -> must post, NOT silently fall through to the timestamp proxy " + "(which would read 'nothing pushed since trigger' here and wrongly conclude satisfied) (#2486)", + ), # ── THE OTHER THREE CONDITIONS MUST STILL BITE ───────────────────────────────────── ( "no_trigger_comment_at_all", @@ -514,6 +545,89 @@ def test_mutation_ignoring_a_marker_mismatch_is_caught(): ) +# ── Fail-safe regression coverage (#2486) ──────────────────────────────────────────── +# Two comment-fetch failures in the gate used to defeat its own stated fail-safe design: +# a failed trigger-comments fetch aborted the whole gate instead of posting, and a failed +# marker fetch was indistinguishable from "no marker yet", so it could fall through to the +# timestamp proxy and conclude satisfied. Both are exercised in SCENARIOS above; the mutants +# below prove that coverage is load-bearing by reverting each fix and requiring the flip. + + +def test_mutation_trigger_fetch_abort_reintroduces_the_stall(): + """Pre-#2486 behaviour: a failed trigger-comments fetch aborted the gate outright rather + than failing safe by posting. On Step 2i's mandatory final run, that meant a transient + API hiccup could stop the sweep having never sent the required last trigger. Revert to + the abort-only shape and require `trigger_candidates_fetch_fails` to flip away from POST.""" + src = _gate_source() + pattern = re.compile( + r" \|\| \{\n # Fail safe \(same reasoning as every guard below\):.*?\n \}\n", + re.S, + ) + mutated, n = pattern.subn( + ' || { echo "FATAL: could not fetch trigger comments β€” aborting gate"; exit 1; }\n', + src, + ) + assert n == 1, f"expected to mutate exactly one trigger-fetch guard, matched {n}" + fixture = _fixture(fail=["trigger_fetch"]) + if shutil.which("jq") is None: + raise AssertionError("this test needs `jq`") + with tempfile.TemporaryDirectory() as td: + td_p = Path(td) + bindir = td_p / "bin" + bindir.mkdir() + gh = bindir / "gh" + gh.write_text(GH_STUB, "utf-8") + gh.chmod(0o755) + (td_p / "fixture.json").write_text(json.dumps(fixture), "utf-8") + posts = td_p / "posts.txt" + posts.write_text("", "utf-8") + script = td_p / "gate.sh" + script.write_text(mutated, "utf-8") + env = { + **os.environ, + "PATH": f"{bindir}:{os.environ['PATH']}", + "GH_STUB_FIXTURE": str(td_p / "fixture.json"), + "GH_STUB_POSTS": str(posts), + } + proc = subprocess.run(["bash", str(script)], capture_output=True, text=True, env=env) + posted = [ln for ln in posts.read_text("utf-8").splitlines() if ln.strip()] + # MUTATION NOT DETECTED would mean the mutant still posts (proc exits 0 with a post) + # exactly like the fixed gate β€” i.e. the scenario doesn't actually depend on the fix. + assert not (proc.returncode == 0 and posted == ["body=@greptileai"]), ( + "MUTATION NOT DETECTED: reverting to the old abort-on-failure shape still " + f"produced the same outcome as the fix. exit={proc.returncode} posted={posted!r} " + f"stdout:\n{proc.stdout}\nstderr:\n{proc.stderr}" + ) + + +def test_mutation_reviewed_fetch_failure_silently_falls_through(): + """Pre-#2486 behaviour: the reviewed-commit marker fetch piped `gh api` straight into + `grep`, so a failed API call was indistinguishable from a successful call that found no + marker yet β€” both left `reviewed_shas` empty and fell into the timestamp-proxy fallback. + Revert to that direct-pipe shape and require `reviewed_shas_fetch_fails_must_not_fall_through` + (fetch fails, but the fallback's own timestamp check would otherwise read 'satisfied') to + flip from POST to SKIP, proving the separate fetch-then-extract shape is load-bearing.""" + src = _gate_source() + pattern = re.compile(r" reviewed_bodies=\$\(gh api.*?tr 'A-Z' 'a-z'\)\n", re.S) + replacement = ( + " reviewed_shas=$(gh api repos/" + REPO_SLUG + "/issues/" + PR + "/comments --paginate \\\n" + " --jq '.[] | select(.user.login == \"greptile-apps[bot]\") | .body' 2>/dev/null \\\n" + " | grep -o 'Last reviewed commit:.*/commit/[0-9a-fA-F]\\{40\\}' \\\n" + " | grep -o '[0-9a-fA-F]\\{40\\}$' | tr 'A-Z' 'a-z')\n" + ) + mutated, n = pattern.subn(replacement, src) + assert n == 1, f"expected to mutate exactly one reviewed-fetch guard, matched {n}" + out, posted = _run_gate( + _fixture(fail=["reviewed_shas_fetch"], marker_sha=None, commit_ts=COMMIT_BEFORE_TRIGGER), + source=mutated, + ) + assert posted == [], ( + "MUTATION NOT DETECTED: reverting to the direct gh-api-into-grep pipe still posted " + f"even though the fetch fails β€” the fixed guard is not actually load-bearing. " + f"Gate said: {out.strip()}" + ) + + TESTS = [ test_gate_scenarios, test_regression_skip_cites_the_marker_not_the_proxy, @@ -521,6 +635,8 @@ def test_mutation_ignoring_a_marker_mismatch_is_caught(): test_mutation_removing_the_marker_lookup_reintroduces_the_930_bug, test_mutation_breaking_the_sha_regex_reintroduces_the_930_bug, test_mutation_ignoring_a_marker_mismatch_is_caught, + test_mutation_trigger_fetch_abort_reintroduces_the_stall, + test_mutation_reviewed_fetch_failure_silently_falls_through, ] diff --git a/.github/workflows/sweep-gate.yml b/.github/workflows/sweep-gate.yml index 9899ced2f..cf6d42905 100644 --- a/.github/workflows/sweep-gate.yml +++ b/.github/workflows/sweep-gate.yml @@ -6,10 +6,11 @@ # re-reviews and makes a converged sweep look unconverged (posted when it should skip). # # The suite extracts the real fenced block from SKILL.md β€” no copy β€” and drives it with a -# stub `gh`, so a change to the gate is a change to the tested code. Three of its cases are -# mutation tests that revert the gate to the pre-fix timestamp proxy and REQUIRE the +# stub `gh`, so a change to the gate is a change to the tested code. Several of its cases +# are mutation tests that revert the gate to a pre-fix shape and REQUIRE the corresponding # regression assertion to fail, so that assertion cannot rot into one that passes with and -# without the bug it names. +# without the bug it names β€” covering both the #930 marker-vs-timestamp regression and the +# #2486 comment-fetch fail-safe gaps. # # zsh is installed and REQUIRED rather than skipped: it is the maintainers' interactive # shell, and a /sweep guard in a sibling repo once silently PASSED under zsh while failing From 1b508fd62178b976667d15011e4b84ac444608f6 Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Fri, 14 Aug 2026 18:30:40 -0600 Subject: [PATCH 3/3] fix(sweep): the fail-safe recovery post must not silently swallow its own failure (#2486) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every branch of the Greptile trigger gate that needs to post @greptileai did so inline and then unconditionally exited 0 (or fell through to an implicit success), without checking whether that POST itself succeeded. A double failure β€” the original fetch AND the recovery post both failing β€” would still report success, reproducing the exact bug this PR fixes at one remove: the sweep believing the mandatory trigger reached Greptile when it never left the machine. Introduces a shared post_trigger_or_die helper used by all nine posting branches (including the ordinary 'not satisfied' verdict): it exits 0 only if the post actually succeeds, and exits 1 with a loud message otherwise. Adds a scenario and a mutation test proving the double- failure case now fails loudly instead of silently. --- .claude/skills/sweep/SKILL.md | 54 ++++---- .github/scripts/test_sweep_greptile_gate.py | 137 +++++++++++++------- 2 files changed, 115 insertions(+), 76 deletions(-) diff --git a/.claude/skills/sweep/SKILL.md b/.claude/skills/sweep/SKILL.md index d6c6adcbd..748895c0a 100644 --- a/.claude/skills/sweep/SKILL.md +++ b/.claude/skills/sweep/SKILL.md @@ -374,6 +374,24 @@ echo "All Greptile comments have replies β€” safe to re-trigger." # TRIGGER comment counts. If ANY condition fails, post `@greptileai`. Same idempotent gate is # safe to re-run, so Step 2i calls it verbatim as the final mandatory check. +# Posts the mandatory `@greptileai` trigger and terminates the gate. EVERY branch below that +# needs to post β€” a fail-safe fetch-failure fallback, "no trigger has ever existed", or the +# ordinary "not satisfied" verdict β€” calls this instead of posting inline and falling through. +# A failed fetch can't prove a trigger already exists, so the gate must attempt to notify +# Greptile; but if THIS post also fails, silently letting the script end with its default exit 0 +# would reproduce the exact bug it fixes at one remove (Greptile review, PR #2486): the sweep +# would believe the mandatory trigger landed when it never left this machine. That double +# failure is rare, but the caller (a human or the sweep session) needs a LOUD signal instead of +# false confidence, so a failed post here exits non-zero rather than swallowing the error. +post_trigger_or_die() { + echo "$1" + if gh api repos//issues//comments -f body="@greptileai" > /dev/null; then + exit 0 + fi + echo "FATAL: the @greptileai POST itself failed β€” Greptile was NOT notified. Investigate (network/auth/rate-limit) and retrigger manually." >&2 + exit 1 +} + # Candidate issue-stream comments, oldest first: ``. # The body rides through jq's `@json` so every record stays ONE line β€” a raw multi-line body would # break the line-oriented classifier below. `@json` is core jq needing no regex, so this does not @@ -386,9 +404,7 @@ trigger_candidates=$(gh api repos//issues//comments --paginate \ # doesn't already exist, but it *also* can't prove one does β€” and aborting here used to # exit the gate outright without posting, which on Step 2i's mandatory final run meant # the sweep could stop having never sent the required last review trigger. Post instead. - echo "Could not fetch trigger comments β€” posting @greptileai (fail safe)." - gh api repos//issues//comments -f body="@greptileai" - exit 0 + post_trigger_or_die "Could not fetch trigger comments β€” posting @greptileai (fail safe)." } # @@ -480,8 +496,7 @@ trigger_ts=$(printf '%s' "$trigger_pair" | cut -f2) if [ -z "$trigger_id" ]; then # (1) fails β€” no trigger has ever reflected this PR. This is the case the old reply-reaction # shortcut got wrong: a πŸ‘ on a reply is not a trigger, so we MUST post one. - echo "No @greptileai trigger comment exists β€” posting trigger." - gh api repos//issues//comments -f body="@greptileai" + post_trigger_or_die "No @greptileai trigger comment exists β€” posting trigger." else # Trigger timestamp (ISO-8601 β†’ lexicographically comparable as a string) came from the same # record as the id, so it needs no second fetch. Keep the guard anyway: an empty created_at @@ -490,9 +505,7 @@ else # Fail safe: we found a trigger id but could not resolve its timestamp, so the # after-trigger inline-comment check (3) and the push-staleness check (4) can't # run. Never assume the conditions they control are met β€” post the trigger. - echo "Could not resolve trigger timestamp β€” posting @greptileai (fail safe)." - gh api repos//issues//comments -f body="@greptileai" - exit 0 + post_trigger_or_die "Could not resolve trigger timestamp β€” posting @greptileai (fail safe)." fi # (2) positive reaction ON THE TRIGGER (never on a reply). @@ -512,9 +525,7 @@ else if [ -z "$issue_raw" ] || [ -z "$inline_raw" ]; then # Fail safe (same reasoning as the trigger_ts/head_ts guards): a failed fetch can't prove # "no new comments" β€” post the trigger rather than assume condition (3) is met. - echo "Could not fetch after-trigger comments β€” posting @greptileai (fail safe)." - gh api repos//issues//comments -f body="@greptileai" - exit 0 + post_trigger_or_die "Could not fetch after-trigger comments β€” posting @greptileai (fail safe)." fi # Sum the per-page counts from each stream. Shell arithmetic, NOT `awk '{s+=$1}'`: a positional # awk variable in skill text is rewritten by argument substitution before the agent ever sees it @@ -532,9 +543,7 @@ else [ -n "$page_count" ] || continue case "$page_count" in *[!0-9]*) - echo "Non-numeric after-trigger count ('$page_count') β€” posting @greptileai (fail safe)." - gh api repos//issues//comments -f body="@greptileai" - exit 0 + post_trigger_or_die "Non-numeric after-trigger count ('$page_count') β€” posting @greptileai (fail safe)." ;; esac comments_after=$((comments_after + page_count)) @@ -557,9 +566,7 @@ else # Fail safe (same reasoning as the trigger_ts guard above): with no head SHA, neither the # marker comparison nor the timestamp fallback can prove the trigger reflects the current # head. Post rather than assume β€” a failed fetch must never count as satisfied. - echo "Could not resolve PR head SHA β€” posting @greptileai (fail safe)." - gh api repos//issues//comments -f body="@greptileai" - exit 0 + post_trigger_or_die "Could not resolve PR head SHA β€” posting @greptileai (fail safe)." fi # Every SHA Greptile has published as "last reviewed" (normally one β€” the sticky summary). @@ -578,11 +585,7 @@ else # skip of a head Greptile's marker was never actually checked against. reviewed_bodies=$(gh api repos//issues//comments --paginate \ --jq '.[] | select(.user.login == "greptile-apps[bot]") | .body' 2>/dev/null) \ - || { - echo "Could not fetch Greptile comments for the reviewed-commit marker β€” posting @greptileai (fail safe)." - gh api repos//issues//comments -f body="@greptileai" - exit 0 - } + || post_trigger_or_die "Could not fetch Greptile comments for the reviewed-commit marker β€” posting @greptileai (fail safe)." # inline-sweep GREPTILE-LAST-REVIEWED extractor [#930] reviewed_shas=$(printf '%s\n' "$reviewed_bodies" \ | grep -o 'Last reviewed commit:.*/commit/[0-9a-fA-F]\{40\}' \ @@ -611,9 +614,7 @@ else # Fail safe (same reasoning as the trigger_ts guard above): if the head commit time # can't be fetched, the staleness check (4) can't run. Don't let it short-circuit to # "not stale" β€” post the trigger so a failed fetch never silently counts as satisfied. - echo "Could not resolve head commit time β€” posting @greptileai (fail safe)." - gh api repos//issues//comments -f body="@greptileai" - exit 0 + post_trigger_or_die "Could not resolve head commit time β€” posting @greptileai (fail safe)." fi # awk does the string compare (portable across bash/zsh; avoids `[ \> ]`, which zsh rejects). # ISO-8601 values are non-numeric, so awk compares them lexically = chronologically. @@ -625,8 +626,7 @@ else if [ "$positive" -gt 0 ] && [ "$comments_after" -eq 0 ] && [ "$pushed_after" -eq 0 ]; then echo "Greptile satisfied with current head $head_sha (reacted to trigger $trigger_id; no new comments; head reviewed per $head_basis) β€” skipping re-trigger." else - echo "Not satisfied (positive=$positive comments_after=$comments_after pushed_after=$pushed_after basis=$head_basis) β€” posting @greptileai." - gh api repos//issues//comments -f body="@greptileai" + post_trigger_or_die "Not satisfied (positive=$positive comments_after=$comments_after pushed_after=$pushed_after basis=$head_basis) β€” posting @greptileai." fi fi ``` diff --git a/.github/scripts/test_sweep_greptile_gate.py b/.github/scripts/test_sweep_greptile_gate.py index 170f23621..fa51d3a8a 100644 --- a/.github/scripts/test_sweep_greptile_gate.py +++ b/.github/scripts/test_sweep_greptile_gate.py @@ -26,11 +26,14 @@ `test_mutation_*` are the load-bearing tests: they mutate the gate back to a pre-fix behaviour and assert the affected scenario FLIPS. Without them, the corresponding regression assertion could pass with and against the bug it names. Three cover the -#930 marker-vs-timestamp regression above; two more (added in #2486) cover a pair of -comment-fetch failures that used to defeat this gate's own fail-safe design β€” one +#930 marker-vs-timestamp regression above; four more (added in #2486) cover the gate's +own fail-safe design: two comment-fetch failures that used to defeat it outright β€” one aborted the whole gate instead of posting, the other let a failed marker fetch look -identical to "no marker yet" and fall through to a timestamp proxy that could -conclude satisfied. +identical to "no marker yet" and fall through to a timestamp proxy that could conclude +satisfied β€” and two covering the shared `post_trigger_or_die` recovery-post itself, +which a Greptile review round on this same PR pointed out could fail silently: posting +`@greptileai` without checking whether that post succeeded before exiting 0, so a +double failure (the original fetch AND the recovery post) still reported success. Run: python -m pytest .github/scripts/test_sweep_greptile_gate.py or: cd .github/scripts && python3 test_sweep_greptile_gate.py (stdlib runner below) @@ -249,6 +252,7 @@ def fail_if(key): url = next(a for a in argv[1:] if not a.startswith("-")) # A POST: `gh api -f body=@greptileai` if "-f" in argv: + fail_if("trigger_post") body = opt(["-f"]) with open(os.environ["GH_STUB_POSTS"], "a") as fh: fh.write(body + "\n") @@ -323,9 +327,18 @@ def _shells() -> list[str]: def _run_gate( - fixture: dict, source: str | None = None, shell: str = "bash" -) -> tuple[str, list[str]]: - """Run the gate under `shell` with a stub `gh` on PATH. Returns (stdout, posted bodies).""" + fixture: dict, + source: str | None = None, + shell: str = "bash", + expect_returncode: int | None = 0, +) -> tuple[str, list[str], int]: + """Run the gate under `shell` with a stub `gh` on PATH. Returns (stdout, posted bodies, returncode). + + `expect_returncode` defaults to 0 (the gate's normal, ever-successful exit) and asserts it β€” + every scenario before #2486 only ever exercised that path. Pass `None` to skip the assertion + entirely (the caller checks `returncode` itself), which the fail-safe-post-failure tests below + need: they deliberately drive the gate into its one legitimate non-zero exit. + """ if shutil.which("jq") is None: raise AssertionError("this test needs `jq` to evaluate the gate's real --jq filters") src = _gate_source() if source is None else source @@ -348,12 +361,13 @@ def _run_gate( "GH_STUB_POSTS": str(posts), } proc = subprocess.run([shell, str(script)], capture_output=True, text=True, env=env) - assert proc.returncode == 0, ( - f"gate exited {proc.returncode} under {shell}\n" - f"stdout:\n{proc.stdout}\nstderr:\n{proc.stderr}" - ) + if expect_returncode is not None: + assert proc.returncode == expect_returncode, ( + f"gate exited {proc.returncode} (expected {expect_returncode}) under {shell}\n" + f"stdout:\n{proc.stdout}\nstderr:\n{proc.stderr}" + ) posted = [ln for ln in posts.read_text("utf-8").splitlines() if ln.strip()] - return proc.stdout, posted + return proc.stdout, posted, proc.returncode # (name, fixture kwargs, expect_post, why) @@ -446,7 +460,7 @@ def _run_gate( def test_gate_scenarios(): for shell in _shells(): for name, kwargs, expect_post, why in SCENARIOS: - out, posted = _run_gate(_fixture(**kwargs), shell=shell) + out, posted, _rc = _run_gate(_fixture(**kwargs), shell=shell) got_post = len(posted) > 0 assert got_post == expect_post, ( f"[{shell}/{name}] expected {'POST' if expect_post else 'SKIP'}, got " @@ -462,7 +476,7 @@ def test_regression_skip_cites_the_marker_not_the_proxy(): """The #930 case must be decided BY THE MARKER. A skip reached via the timestamp proxy would be the right answer for the wrong reason and would not survive a real push, so pin the basis the gate reports.""" - out, posted = _run_gate(_fixture()) + out, posted, _rc = _run_gate(_fixture()) assert not posted assert "basis" not in out, f"a satisfied gate should not report a not-satisfied basis: {out}" assert "head reviewed per marker" in out, ( @@ -472,7 +486,7 @@ def test_regression_skip_cites_the_marker_not_the_proxy(): def test_fallback_skip_cites_the_proxy(): - out, posted = _run_gate(_fixture(marker_sha=None, commit_ts=COMMIT_BEFORE_TRIGGER)) + out, posted, _rc = _run_gate(_fixture(marker_sha=None, commit_ts=COMMIT_BEFORE_TRIGGER)) assert not posted assert "head reviewed per timestamp-proxy" in out, f"expected the proxy basis, got: {out}" @@ -500,7 +514,7 @@ def test_mutation_removing_the_marker_lookup_reintroduces_the_930_bug(): """Pre-fix behaviour: with no marker SHA the gate falls back to the committedDate proxy, which is byte-identical to the code that shipped the bug. The #930 scenario must POST.""" mutated = _mutate_extractor(' reviewed_shas=""\n') - out, posted = _run_gate(_fixture(), source=mutated) + out, posted, _rc = _run_gate(_fixture(), source=mutated) assert posted == ["body=@greptileai"], ( "MUTATION NOT DETECTED: with the marker lookup removed, the #930 scenario still " f"skipped the re-trigger β€” the regression assertion is not load-bearing. " @@ -517,7 +531,7 @@ def test_mutation_breaking_the_sha_regex_reintroduces_the_930_bug(): broken = _gate_source() assert broken.count(r"[0-9a-fA-F]\{40\}") == 2, "expected two 40-hex patterns in the extractor" mutated = broken.replace(r"[0-9a-fA-F]\{40\}", r"[0-9a-fA-F]\{41\}") - out, posted = _run_gate(_fixture(), source=mutated) + out, posted, _rc = _run_gate(_fixture(), source=mutated) assert posted == ["body=@greptileai"], ( "MUTATION NOT DETECTED: a broken SHA pattern still produced a satisfied gate β€” the " f"extractor is not actually being exercised. Gate said: {out.strip()}" @@ -537,7 +551,7 @@ def test_mutation_ignoring_a_marker_mismatch_is_caught(): # The mutant accepts ANY marker as satisfaction, so it SKIPS where the real gate POSTs. # Seeing the mutant skip is what proves `marker_names_other_commit`'s POST is produced by # the equality check and not by some other condition incidentally failing. - out, posted = _run_gate(_fixture(marker_sha=OTHER), source=mutated) + out, posted, _rc = _run_gate(_fixture(marker_sha=OTHER), source=mutated) assert posted == [], ( "MUTATION NOT DETECTED: dropping the marker==head equality check still produced a " f"re-trigger, so `marker_names_other_commit` passes for some other reason and is not " @@ -568,36 +582,13 @@ def test_mutation_trigger_fetch_abort_reintroduces_the_stall(): src, ) assert n == 1, f"expected to mutate exactly one trigger-fetch guard, matched {n}" - fixture = _fixture(fail=["trigger_fetch"]) - if shutil.which("jq") is None: - raise AssertionError("this test needs `jq`") - with tempfile.TemporaryDirectory() as td: - td_p = Path(td) - bindir = td_p / "bin" - bindir.mkdir() - gh = bindir / "gh" - gh.write_text(GH_STUB, "utf-8") - gh.chmod(0o755) - (td_p / "fixture.json").write_text(json.dumps(fixture), "utf-8") - posts = td_p / "posts.txt" - posts.write_text("", "utf-8") - script = td_p / "gate.sh" - script.write_text(mutated, "utf-8") - env = { - **os.environ, - "PATH": f"{bindir}:{os.environ['PATH']}", - "GH_STUB_FIXTURE": str(td_p / "fixture.json"), - "GH_STUB_POSTS": str(posts), - } - proc = subprocess.run(["bash", str(script)], capture_output=True, text=True, env=env) - posted = [ln for ln in posts.read_text("utf-8").splitlines() if ln.strip()] - # MUTATION NOT DETECTED would mean the mutant still posts (proc exits 0 with a post) - # exactly like the fixed gate β€” i.e. the scenario doesn't actually depend on the fix. - assert not (proc.returncode == 0 and posted == ["body=@greptileai"]), ( - "MUTATION NOT DETECTED: reverting to the old abort-on-failure shape still " - f"produced the same outcome as the fix. exit={proc.returncode} posted={posted!r} " - f"stdout:\n{proc.stdout}\nstderr:\n{proc.stderr}" - ) + out, posted, rc = _run_gate(_fixture(fail=["trigger_fetch"]), source=mutated, expect_returncode=None) + # MUTATION NOT DETECTED would mean the mutant still posts (proc exits 0 with a post) + # exactly like the fixed gate β€” i.e. the scenario doesn't actually depend on the fix. + assert not (rc == 0 and posted == ["body=@greptileai"]), ( + "MUTATION NOT DETECTED: reverting to the old abort-on-failure shape still " + f"produced the same outcome as the fix. exit={rc} posted={posted!r} gate said: {out.strip()}" + ) def test_mutation_reviewed_fetch_failure_silently_falls_through(): @@ -617,7 +608,7 @@ def test_mutation_reviewed_fetch_failure_silently_falls_through(): ) mutated, n = pattern.subn(replacement, src) assert n == 1, f"expected to mutate exactly one reviewed-fetch guard, matched {n}" - out, posted = _run_gate( + out, posted, _rc = _run_gate( _fixture(fail=["reviewed_shas_fetch"], marker_sha=None, commit_ts=COMMIT_BEFORE_TRIGGER), source=mutated, ) @@ -628,6 +619,52 @@ def test_mutation_reviewed_fetch_failure_silently_falls_through(): ) +# ── Recovery-POST fail-safe coverage (Greptile review round 2, PR #2486) ──────────────────── +# Every fail-safe branch above posts `@greptileai` through the shared `post_trigger_or_die` +# helper β€” but Greptile pointed out that the helper itself, as first written, didn't check +# whether THAT post succeeded before exiting 0. A double failure (the original fetch AND the +# recovery post) would still silently report success, reproducing the exact bug this PR fixes +# at one remove: the sweep believing the mandatory trigger reached Greptile when it never left +# this machine. `trigger_post` is a fixture fail-key matching ANY `-f`-flagged POST call. + + +def test_post_failure_after_fetch_failure_exits_loudly_not_silently(): + """When BOTH the original fetch and the fail-safe recovery POST fail, the gate must not + silently report success. `post_trigger_or_die` must exit non-zero and record no post.""" + out, posted, rc = _run_gate( + _fixture(fail=["trigger_fetch", "trigger_post"]), expect_returncode=None + ) + assert rc != 0, ( + f"expected a non-zero exit when the recovery POST itself fails, got {rc}. " + f"Gate said: {out.strip()}" + ) + assert posted == [], f"expected no successful post recorded, got {posted!r}" + + +def test_mutation_ignoring_post_failure_silently_reports_success(): + """Revert `post_trigger_or_die` to the shape Greptile flagged: post without checking + success, then unconditionally exit 0. The double-failure scenario above must flip from a + loud non-zero exit back to a silent 0, proving the success check is load-bearing.""" + src = _gate_source() + pattern = re.compile(r"post_trigger_or_die\(\) \{\n.*?\n\}\n", re.S) + replacement = ( + "post_trigger_or_die() {\n" + ' echo "$1"\n' + " gh api repos/" + REPO_SLUG + "/issues/" + PR + '/comments -f body="@greptileai" > /dev/null\n' + " exit 0\n" + "}\n" + ) + mutated, n = pattern.subn(replacement, src) + assert n == 1, f"expected to mutate exactly one post_trigger_or_die definition, matched {n}" + out, posted, rc = _run_gate( + _fixture(fail=["trigger_fetch", "trigger_post"]), source=mutated, expect_returncode=None + ) + assert rc == 0, ( + "MUTATION NOT DETECTED: reverting to the unchecked-POST shape still exited non-zero β€” " + f"the fix is not actually load-bearing. exit={rc}. Gate said: {out.strip()}" + ) + + TESTS = [ test_gate_scenarios, test_regression_skip_cites_the_marker_not_the_proxy, @@ -637,6 +674,8 @@ def test_mutation_reviewed_fetch_failure_silently_falls_through(): test_mutation_ignoring_a_marker_mismatch_is_caught, test_mutation_trigger_fetch_abort_reintroduces_the_stall, test_mutation_reviewed_fetch_failure_silently_falls_through, + test_post_failure_after_fetch_failure_exits_loudly_not_silently, + test_mutation_ignoring_post_failure_silently_reports_success, ]