chore(eval): add rework rate tracking script for agent PRs (#5516) - #5517
chore(eval): add rework rate tracking script for agent PRs (#5516)#5517Benkapner wants to merge 15 commits into
Conversation
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
PR Summary by QodoAdd script to measure human rework after agent-merged PRs
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Site previewPreview: https://69414218-site.fullsend-ai.workers.dev Commit: |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
rh-hemartin
left a comment
rh-hemartin
left a comment
There was a problem hiding this comment.
Hello! You need to fix the body, your process didn't expand \n characters into newlines.
Also I'm running the script locally and it looks too static. You should include something that indicates that the script is actively working. I asked my local agent to introduce a progress system and it looks something like this:
Rework Rate Report
Repository: fullsend-ai/fullsend
Window: last 7 days (since 2026-07-16T00:00:00Z)
Follow-up window: 1 days after merge
Checking PR 7/39 (#5468)...
|
I think something is better than nothing, but I'm not sure about the metric being really a rework one. Do you have any reference to decide file changes? Maybe doing this per line would be better? Also I think we could include the coder into the rework metric, currently we are using it more and more for fixes, so it makes sense that if it passes multiple times on the same file (or line or whatever) then it is considered rework, even if it was itself. What do you think? |
|
thanks for the review and for running it locally. pushed fixes addressing all points:
on the metric question you raised (file-level vs line-level, and whether to include the coder agent's own rework): those are good points. i think this first version is a starting point to get a baseline, and we can refine the metric definition based on what the data shows. happy to iterate on the detection granularity in a follow-up. |
|
You need to install pre-commit locally, the CI is failing because of that: |
|
thanks @rh-hemartin , fixed the shellcheck findings (SC2086 double-quoting, SC2181 direct exit code checks). should pass now i hope |
|
Example run: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
thanks for testing it, appreciate the example run output. ready for your approval when you're good. |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review sweep findings below, posted as inline comments. Summary: 1 critical, 2 high, 8 medium. The critical and first high finding are backed by live verification against this repo's actual PR/commit history (see inline comments for specifics).
|
|
||
| # Get commits after merge by non-bot authors | ||
| if ! FOLLOWUP_COMMITS=$(gh api "repos/${REPO}/commits?since=${MERGED_AT}&until=${FOLLOWUP_UNTIL}&per_page=100" \ | ||
| --jq '[.[] | select(.author.type != "Bot" and .author.login != "fullsend-ai-coder[bot]" and .author.login != "fullsend-ai-fullsend[bot]") | {sha: .sha, author: .author.login, message: .commit.message}]' 2>&1); then |
There was a problem hiding this comment.
[CRITICAL] Follow-up detection never excludes merge commits, so ordinary PR merges get misclassified as human "rework"
The follow-up-commit filter (this line) excludes bot accounts and two hardcoded bot logins but never checks parent count, so any 2-parent merge commit passes straight through as a "human follow-up." I independently verified this live: PR #5643, #5623, and #5636 in this repo (all authored by fullsend-ai-coder[bot], merged in the last day) were each merged via genuine 2-parent merge commits (confirmed parents array length 2) authored by human accounts (author.type: "User", e.g. ggallen, ifireball). For PR #5643 I fetched both the PR's own file list and its merge commit's files list (the same field the script reads at line 98) — they are byte-for-byte identical (11/11 files) — because a merge commit's file diff is computed against its first parent, i.e. the entire merged PR, not incremental work. This repo has allow_merge_commit: true and merges bot PRs this way routinely (121 bot PRs merged in the last 30 days per my live query). Since the follow-up window scans ALL repo-wide commits (not just the examined PR's own branch) for FOLLOWUP_DAYS after merge, any human-merged PR whose files overlap even slightly with a bot PR's files will trip the comm -12 check at line 103 and get counted as "reworked by humans," mechanically inflating the rate for essentially any bot PR that isn't perfectly isolated to unique files. This directly undermines the tool's stated purpose (evidence for autonomy-level decisions per linked issue #5516), and nothing in the PR conversation discusses merge commits at all.
Suggestion: Before comparing file lists, fetch each follow-up commit's parent count and skip any commit with more than one parent — a merge commit's files field reflects the entire merged PR's diff, not incremental follow-up work, so it can never validly count as a fix. Also consider explicitly excluding the examined PR's own merge_commit_sha. Re-run against this repo's real history afterward and sanity-check that the rate drops to a plausible number before treating this as decision-grade data.
There was a problem hiding this comment.
now filters out commits with 2+ parents, and excludes the PR's own merge commit SHA
| PR_NUM=$(echo "$pr_json" | jq -r '.number') | ||
| PR_TITLE=$(echo "$pr_json" | jq -r '.title') | ||
| MERGED_AT=$(echo "$pr_json" | jq -r '.closed_at') | ||
| TOTAL=$((TOTAL + 1)) |
There was a problem hiding this comment.
[HIGH] Rate denominator (TOTAL) counts API-error-skipped PRs as "not reworked," silently deflating the rework rate
TOTAL is incremented unconditionally on this line for every bot PR before any of the three failure paths that continue past a PR without fully checking it: files-fetch failure (lines 61-63), date-fallback failure (lines 75-77), and follow-up-commits-fetch failure (lines 83-86). Each path increments SKIPPED but never decrements TOTAL, and the final RATE = REWORKED / TOTAL * 100 (lines 117-121) treats every skipped PR identically to a confirmed non-reworked PR. Any transient gh api error (rate limiting, auth hiccup, network blip) across the dozens of sequential API calls this script makes will quietly pull the reported rate down. This is unaddressed in the existing PR conversation, which only discusses the WARNING/SKIPPED surfacing mechanism, never the rate formula itself.
Suggestion: Compute the rate over only the PRs actually checked, e.g. RATE = REWORKED / (TOTAL - SKIPPED) * 100 (guarding div-by-zero), and print both the raw PR count and the checked count.
There was a problem hiding this comment.
rate now uses CHECKED (successfully evaluated PRs) instead of TOTAL
| echo "" | ||
|
|
||
| # Fetch merged PRs by bot authors (both app identity and [bot] login) | ||
| BOT_PRS=$(gh api "search/issues?q=repo:${REPO}+is:pr+is:merged+author:fullsend-ai-coder[bot]+merged:>=${SINCE}&per_page=100&sort=created&order=desc" \ |
There was a problem hiding this comment.
[HIGH] Pagination fix from the earlier review round covered only one of three flagged endpoints — search and follow-up-commit queries still truncate at 100
An earlier review comment on this PR ("Missing pagination") explicitly named three call sites needing --paginate — the bot-PR search query, the PR-files call, and the follow-up-commits query — and was replied to as "fixed, added --paginate to the PR files call." That reply is accurate but incomplete: checking the current head commit, only the PR-files call (line 59) received --paginate. The bot-PR search query (this line, and its fallback at line 32) and the follow-up-commits query (line 81) still lack it. I confirmed this is live, not hypothetical: querying this exact repo with the script's own default 30-day window right now returns total_count: 121 but only 100 items without --paginate (121 with it added) — running ./scripts/rework-rate.sh today silently drops ~17% of the population from TOTAL. Separately, the commits endpoint returns newest-first, so truncating the follow-up-commits query drops precisely the commits closest to each PR's merge time — the highest-signal "quick fix" commits this script exists to detect — biasing the metric in the opposite direction.
Suggestion: Add --paginate to the search calls at this line and line 32, and to the commits call at line 81, matching the pattern already used at line 59. Consider printing a warning if a search response's total_count exceeds the number of items actually retrieved.
There was a problem hiding this comment.
added --paginate to bot PR search and follow-up commits queries (all 3 call sites now paginated)
|
|
||
| # Get commits after merge by non-bot authors | ||
| if ! FOLLOWUP_COMMITS=$(gh api "repos/${REPO}/commits?since=${MERGED_AT}&until=${FOLLOWUP_UNTIL}&per_page=100" \ | ||
| --jq '[.[] | select(.author.type != "Bot" and .author.login != "fullsend-ai-coder[bot]" and .author.login != "fullsend-ai-fullsend[bot]") | {sha: .sha, author: .author.login, message: .commit.message}]' 2>&1); then |
There was a problem hiding this comment.
[MEDIUM] Hardcoded bot-login exclusion (fullsend-ai-fullsend[bot]) is unverified, redundant, and names an unrelated bot
The follow-up-commit filter excludes .author.login != "fullsend-ai-fullsend[bot]" alongside fullsend-ai-coder[bot]. I checked this repo's authoritative table, docs/contributing/bot-identities.md (on main), which lists only fullsend-ai-coder[bot], fullsend-ai-review[bot], fullsend-ai-triage[bot], fullsend-ai-retro[bot], fullsend-ai-prioritize[bot], and renovate-fullsend[bot] — no fullsend-ai-fullsend[bot] entry — and that doc's closing line instructs: "always verify the login name against this table." I also confirmed fullsend-ai-fullsend[bot] is a real, active identity in this repo (e.g. PR #5559, #5198, "chore: update fullsend shim workflow"), but it's an unrelated automation with nothing to do with the coding agent this script measures. It's also dead code in practice: .author.type != "Bot" on the same line already excludes every GitHub-App-based bot account, including this one.
Suggestion: Drop the two hardcoded login checks (the .author.type != "Bot" check already covers all bot accounts), or if a name-based exclusion is genuinely needed, verify it against docs/contributing/bot-identities.md as that doc instructs.
There was a problem hiding this comment.
dropped hardcoded fullsend-ai-fullsend[bot], .author.type != "Bot" covers all bot accounts
| @@ -0,0 +1,137 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
[MEDIUM] No companion test added, breaking this repo's enforced scripts/ test convention
scripts/ on the PR head contains exactly check-e2e-authorization.sh, its companion check-e2e-authorization-test.sh (mocks gh via a fake binary on PATH), and this new rework-rate.sh — no rework-rate-test.sh. The Makefile's script-test target (run in CI via .github/workflows/lint.yml's make script-test step) explicitly invokes bash scripts/check-e2e-authorization-test.sh plus a *-test.sh per script under internal/scaffold/fullsend-repo/scripts/, confirming this is an established, CI-enforced, repo-wide convention that this 137-line script (three gh api call sites, GNU/BSD date fallbacks, multi-stage error handling, file-overlap logic) does not follow. A mocked-gh test simulating a two-parent merge-commit follow-up or a >100-item search response would very plausibly have caught the critical and high findings in this review before this PR was approved.
Suggestion: Add scripts/rework-rate-test.sh following the check-e2e-authorization-test.sh mock-gh pattern, and add it to the script-test target in the Makefile. At minimum cover: a merge-commit follow-up (must not count as rework), a >100-item search/commits response, and a genuine single-parent follow-up commit on an overlapping file (must count).
There was a problem hiding this comment.
Added scripts/rework-rate-test.sh following the check-e2e-authorization-test.sh mock-gh pattern, wired into make script-test. Covers all three minimum cases: merge-commit follow-up (must not count), >100-item paginated response, and genuine single-parent file-overlap (must count). Also covers PR's own merge SHA exclusion and API failure handling.
| COMMIT_SHA=$(echo "$commit_json" | jq -r '.sha') | ||
| COMMIT_AUTHOR=$(echo "$commit_json" | jq -r '.author') | ||
|
|
||
| if ! COMMIT_FILES=$(gh api "repos/${REPO}/commits/${COMMIT_SHA}" \ |
There was a problem hiding this comment.
[MEDIUM] Per-commit file-fetch failures still silently continue with no warning or SKIPPED accounting, unlike the two sibling fetches that were fixed
The earlier "Silent api failure skip" review comment's Fix Focus Areas named three call sites (PR-files fetch, follow-up-commits fetch, and this per-commit files fetch) and was replied to as "fixed, API failures now surface as WARNING: lines and are tracked in a Skipped (API errors) count." That's true for the first two (lines 61/83) but not this third one: lines 98-101 were refactored to check the real exit status (if ! COMMIT_FILES=$(... 2>&1); then continue; fi), but the continue here has no echo "WARNING: ..." and no SKIPPED=$((SKIPPED+1)), unlike its siblings. A transient failure fetching one commit's files is silently treated as "this commit doesn't overlap" — if it was the only overlapping follow-up commit for that PR, the PR is wrongly reported as not reworked, with nothing in the final "Skipped (API errors)" count reflecting it.
Suggestion: Add the same echo "WARNING: ..." + SKIPPED=$((SKIPPED+1)) treatment here for consistency with lines 61 and 83.
There was a problem hiding this comment.
commit file fetch failures now surface as warnings with SKIPPED count
| if [ -n "$REWORKED_LIST" ]; then | ||
| echo "" | ||
| echo "Reworked PRs:" | ||
| echo -e "$REWORKED_LIST" |
There was a problem hiding this comment.
[MEDIUM] echo -e on unsanitized, externally-supplied PR titles can corrupt or truncate the final report
REWORKED_LIST is built at line 107 by concatenating literal \n/\t-style two-character escape markers with ${PR_TITLE} — text sourced directly from the GitHub API with no sanitization — and the whole accumulated string is rendered once via echo -e "$REWORKED_LIST" on this line. If a bot PR title contains a backslash sequence echo -e recognizes (e.g. \c, which stops output immediately with no trailing newline, or \t/\0NNN), the "Reworked PRs" section — or everything after that point — can be silently garbled or truncated. Note: this is distinct from the earlier "body rendering" issue that was fixed, which was confirmed to be a PR-description/tooling issue, not this code path; this exact code is unchanged across both fix commits and was never reviewed for this behavior.
Suggestion: Accumulate report lines in a bash array and print with printf '%s\n' "${arr[@]}" instead of building one string later fed through echo -e, so title text is never re-interpreted as an escape sequence.
There was a problem hiding this comment.
replaced with bash array + printf
| DAYS="${2:-30}" | ||
| FOLLOWUP_DAYS="${3:-7}" | ||
|
|
||
| SINCE=$(date -d "-${DAYS} days" +%Y-%m-%dT00:00:00Z 2>/dev/null || date -v-"${DAYS}"d +%Y-%m-%dT00:00:00Z) |
There was a problem hiding this comment.
[MEDIUM] Date window boundaries computed in local time but labeled Z (UTC), causing up to a day of boundary drift
Neither the GNU branch of SINCE (this line: date -d "-${DAYS} days" +%Y-%m-%dT00:00:00Z) nor FOLLOWUP_UNTIL's GNU branch (line 71) passes -u/--utc, so the %Y-%m-%d fields reflect the executing machine's local calendar date while the trailing Z merely asserts UTC without converting anything. I reproduced this directly: the identical date -d "-30 days" +%Y-%m-%dT00:00:00Z expression gives 2026-06-28T00:00:00Z under TZ=UTC but 2026-06-27T00:00:00Z under TZ=Pacific/Midway (UTC-11) — a full day of drift for the same logical "30 days ago." Since SINCE feeds GitHub's merged:>= qualifier and FOLLOWUP_UNTIL feeds the commits until= parameter, the actual reporting window silently shifts depending on where the script executes.
Suggestion: Add -u/--utc to the GNU date -d invocations at this line and line 71, and use date -u on the BSD/macOS fallback branches too, so the window is timezone-independent.
There was a problem hiding this comment.
added -u to all date commands for UTC
| fi | ||
|
|
||
| # Check for human commits touching the same files after merge | ||
| FOLLOWUP_UNTIL=$(date -d "${MERGED_AT} +${FOLLOWUP_DAYS} days" +%Y-%m-%dT23:59:59Z 2>/dev/null \ |
There was a problem hiding this comment.
[MEDIUM] PRs merged within the last FOLLOWUP_DAYS are scored as "not reworked" before their follow-up window has actually elapsed
For any PR merged fewer than FOLLOWUP_DAYS days ago — with the defaults (DAYS=30, FOLLOWUP_DAYS=7) roughly the most recent quarter of every reporting window — FOLLOWUP_UNTIL (this line) extends into the future, but the commits search can only return commits up to "now." The script has no check for this and scores such a PR identically to one whose window has fully elapsed: "no follow-up commits found yet" and "no follow-up commits will ever exist" produce the same output. This structurally biases the reported rate downward for the most recently merged slice of PRs on every default run, with nothing in the report flagging it.
Suggestion: Skip (or separately/visibly report) PRs whose merge date is within FOLLOWUP_DAYS of "now" rather than folding them into the same denominator as PRs with a fully-elapsed window.
There was a problem hiding this comment.
addressed in ffcdde9. PRs whose follow-up window extends past now are skipped with a message ("follow-up window not elapsed yet, skipping") and counted in SKIPPED, not CHECKED. The rate denominator only includes PRs with a fully-elapsed window.
|
|
||
| # Get files changed in this PR (paginated) | ||
| if ! PR_FILES=$(gh api "repos/${REPO}/pulls/${PR_NUM}/files" --paginate \ | ||
| --jq '.[].filename' 2>&1); then |
There was a problem hiding this comment.
[MEDIUM] 2>&1 on the fixed API calls can let stray stderr text corrupt the file/commit lists used for comparison
This line, plus lines 82 and 99, all capture gh api ... 2>&1 into a variable later treated as pure data — a newline-separated filename list, or JSON handed to jq. This was introduced by the fix for the "Silent api failure skip" review comment (the original version used 2>/dev/null, discarding stderr entirely). Now, any warning gh writes to stderr on an otherwise-successful call (rate-limit notices, deprecation warnings) gets merged into PR_FILES/FOLLOWUP_COMMITS/COMMIT_FILES. For PR_FILES/COMMIT_FILES a stray text line becomes a bogus "filename" fed into comm -12 (line 103), capable of producing a spurious overlap match (a false "reworked" verdict). For FOLLOWUP_COMMITS, stray text would silently break the jq -c '.[]' parse at line 110.
Suggestion: Redirect stderr to a separate location for inspection only on failure, e.g. PR_FILES=$(gh api ... 2>/tmp/rework-rate.err), keeping the captured variable to stdout only, and surface the error file's contents in the existing WARNING branch when the call fails.
There was a problem hiding this comment.
Addressed in ffcdde9. All gh api calls now redirect stderr to a temp file instead of 2>&1, so stray warnings never mix into the captured JSON/filename data. The temp file contents are surfaced in the WARNING message if the call fails, then cleaned up.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Follow-up review sweep on the latest commits. Summary: 1 critical, 2 high, 4 medium — all newly introduced by, or newly exposed by, the fixes applied since the last review round (BSD date fallback, the new companion test file, and the per-commit-fetch error path). Verified live against this actual macOS box and the current PR head where noted.
|
|
||
| # Skip PRs whose follow-up window hasn't fully elapsed yet | ||
| FOLLOWUP_UNTIL=$(date -u -d "${MERGED_AT} +${FOLLOWUP_DAYS} days" +%Y-%m-%dT23:59:59Z 2>/dev/null \ | ||
| || date -u -j -f "%Y-%m-%dT%H:%M:%SZ" "${MERGED_AT}" -v+"${FOLLOWUP_DAYS}"d +%Y-%m-%dT23:59:59Z 2>/dev/null \ |
There was a problem hiding this comment.
[CRITICAL] BSD date argument order breaks follow-up-window check; script always reports 0% rework on macOS
The BSD/macOS fallback date -u -j -f "%Y-%m-%dT%H:%M:%SZ" "${MERGED_AT}" -v+"${FOLLOWUP_DAYS}"d +%Y-%m-%dT23:59:59Z places -v+Nd AFTER the date operand. I reproduced this live on an actual macOS box: date -u -j -f "%Y-%m-%dT%H:%M:%SZ" "2026-07-25T10:00:00Z" -v+1d +%Y-%m-%dT23:59:59Z returns Sat Jul 25 10:00:00 UTC 2026 (exit 0) — BSD date silently ignores both the day offset and the custom output format. FOLLOWUP_UNTIL then becomes a ctime-style string starting with an uppercase day abbreviation (Sun/Mon/Tue/.../Sat). The comparison [[ "$FOLLOWUP_UNTIL" > "$NOW" ]] (line 75) is a lexicographic bash string comparison, and since uppercase ASCII letters (0x41-0x5A) sort after any digit (0x30-0x39), this is unconditionally true regardless of actual dates — every PR is always reported as "follow-up window not elapsed," SKIPPED increments, CHECKED stays 0, and the script exits 0 printing a clean "Rework rate: 0.0%" with no error or warning. This makes the script silently non-functional (confidently wrong) on macOS.
Suggestion: Reorder to date -u -j -f "%Y-%m-%dT%H:%M:%SZ" -v+"${FOLLOWUP_DAYS}"d "${MERGED_AT}" +%Y-%m-%dT23:59:59Z — verified locally this correctly produces 2026-07-26T23:59:59Z. Add a unit test that stubs/exercises the BSD date fallback branch specifically, since it currently has zero coverage in both CI (ubuntu-only) and the mock-gh test suite (which never stubs date).
There was a problem hiding this comment.
Fixed. Reordered the BSD fallback to place -v+Nd before the date operand
| MOCK_EOF | ||
|
|
||
| # Replace placeholders with actual paths | ||
| sed -i "s|GHLOG_PLACEHOLDER|${GH_LOG}|g" "${MOCK_BIN}/gh" |
There was a problem hiding this comment.
[HIGH] sed -i (GNU-only syntax) crashes the brand-new companion test suite immediately on macOS
Lines 86-91 template the mock gh binary via sed -i "s|...|...|g" "${MOCK_BIN}/gh" (six call sites, no backup-suffix argument) — GNU sed's in-place syntax. BSD/macOS sed requires an explicit suffix (even empty, -i ''). I reproduced this live on an actual macOS machine using the same content pattern as this file's heredoc: it fails immediately with a sed: 1: "...": ... parse error, exit 1 — zero test cases execute. This test file is wired into make script-test (Makefile line 156: bash scripts/rework-rate-test.sh), and that target runs in CI only on ubuntu-24.04 (.github/workflows/lint.yml, the test job at line 15 runs make script-test at line 53); the only macOS CI job, test-sandbox-darwin, runs solely go test -race ./internal/sandbox/... and never touches make script-test. So CI passes cleanly while any contributor running this Makefile target locally on macOS gets a hard failure with no tests run. This repo already has an established, portable convention for exactly this scenario: scripts/check-e2e-authorization-test.sh builds its mock gh via an unquoted heredoc that interpolates variables directly at generation time with \$-escaping for parts that must stay literal, avoiding sed -i entirely — this PR introduces the only sed -i usage in the repo's shell scripts.
Separately: fixing only this bug in isolation would reveal that 2 of the file's 5 tests ("merge commit excluded", "PR's own merge SHA excluded") currently pass vacuously — both assert Rework rate: 0.0%, which is exactly what the companion date bug (see the inline comment on rework-rate.sh) also produces by skipping every PR before the merge-commit-exclusion logic ever runs, so those two tests don't actually exercise the code they claim to.
Suggestion: Drop the placeholder+sed -i approach; construct the mock gh script with an unquoted heredoc that interpolates ${GH_LOG}, ${SEARCH_RESULTS}, etc. directly at generation time (escaping \$ for parts of the mock body that must remain literal at runtime), exactly as check-e2e-authorization-test.sh already does in the same directory. This removes the portability bug rather than working around it. Fix in tandem with the BSD date bug so Tests 2 and 3 actually validate the logic they name.
There was a problem hiding this comment.
Rewrote the mock using an unquoted heredoc that interpolates paths at generation time, matching the pattern from check-e2e-authorization-test.sh. No more sed -i
| --jq '.files[].filename' 2>"$COMMIT_FILES_ERR"); then | ||
| echo " WARNING: could not fetch files for commit ${COMMIT_SHA:0:7}: $(cat "$COMMIT_FILES_ERR")" | ||
| rm -f "$COMMIT_FILES_ERR" | ||
| SKIPPED=$((SKIPPED + 1)) |
There was a problem hiding this comment.
[HIGH] Per-commit file-fetch failure is counted as both SKIPPED and CHECKED, silently deflating the reported rework rate
When gh api repos/.../commits/${COMMIT_SHA} fails inside the inner follow-up-commit loop, the script increments SKIPPED (this line) and continues — but that continue only advances the inner while loop over commits (closed by done < <(...) at line 151), not the outer per-PR loop. No flag is set to signal this failure to the outer scope: after the inner loop ends for any reason, line 153 unconditionally runs CHECKED=$((CHECKED + 1)) for that same PR. A PR whose commit-file lookup failed is thus counted in both SKIPPED and CHECKED, and gets reported as "checked, not reworked" even though the commit that failed to fetch might have been the one that overlaps files with the bot PR. Since the published rate is REWORKED/CHECKED, this systematically biases the metric downward on any transient API error — directly undermining the tool's stated purpose (a trustworthy rework-rate signal). No existing test simulates a mid-PR commit-fetch failure to catch this.
Suggestion: Track a per-PR error flag (e.g., PR_HAD_ERROR="") set when any commit-file fetch fails inside the inner loop, and branch on it after the loop to increment SKIPPED instead of CHECKED for that PR.
There was a problem hiding this comment.
Fixed. Added a PR_HAD_ERROR flag in the inner loop. If any commit-file fetch failed and no rework was found, the PR goes to SKIPPED instead of CHECKED
| fi | ||
|
|
||
| COMMIT_FILES_ERR=$(mktemp) | ||
| if ! COMMIT_FILES=$(gh api "repos/${REPO}/commits/${COMMIT_SHA}" \ |
There was a problem hiding this comment.
[HIGH] No caching across PRs for per-commit/per-PR API calls; unbounded fan-out risks rate limits on the script's own default target repo
For every bot PR, the script fetches its own N-day follow-up commit window (line 102) and then makes a dedicated gh api .../commits/${SHA} call per surviving commit for its file list (this line) — with no cross-PR memoization, even though consecutive bot PRs' follow-up windows overlap heavily on an active repo. Verified live against the script's own documented default target: gh api "search/issues?q=repo:fullsend-ai/fullsend+is:pr+is:merged+author:fullsend-ai-coder[bot]" --jq '.total_count' returns 300+. The documented default invocation (DAYS=30) will process a large share of these, each independently re-querying/re-fetching commits that likely fall inside several overlapping windows, generating large numbers of redundant sequential gh api calls (each a separate process spawn + network round trip). This risks GitHub secondary/abuse rate limiting on top of the 5000/hr core budget, making the "default" usage shown in the PR description impractically slow or unreliable. The existing WARNING/SKIPPED handling means this degrades rather than crashes, but it silently shrinks CHECKED and inflates SKIPPED — eroding the accuracy of the exact metric the tool exists to produce, and compounding with the SKIPPED/CHECKED double-counting issue flagged separately on line 138 (a rate-limited call is indistinguishable from any other API failure in this script's error handling).
Suggestion: Cache per-commit-SHA to file-list lookups in an associative array (declare -A COMMIT_FILES_CACHE), populated once per run and reused across all bot-PR iterations, instead of re-fetching a given commit's files once per overlapping PR window. Consider fetching the full report-window commit history once up front and slicing it in memory per PR instead of re-querying /commits per PR. Also consider a small delay/backoff between calls.
There was a problem hiding this comment.
Acknowledged. This is a valid optimization for the 300+ PR default case. Will track as a follow-up; the current version handles rate-limit errors gracefully via SKIPPED accounting but doesn't cache
| fi | ||
|
|
||
| # Get the PR's own merge commit SHA to exclude it from follow-up detection | ||
| PR_MERGE_SHA=$(gh api "repos/${REPO}/pulls/${PR_NUM}" --jq '.merge_commit_sha' 2>/dev/null || echo "") |
There was a problem hiding this comment.
[MEDIUM] PR_MERGE_SHA fetch fails silently with no warning, reopening the merge-commit false-rework bug for squash/rebase merges
PR_MERGE_SHA=$(gh api "repos/${REPO}/pulls/${PR_NUM}" --jq '.merge_commit_sha' 2>/dev/null || echo "") is the only remaining gh api call site with no WARNING message and no SKIPPED accounting on failure — every sibling call (PR_FILES, FOLLOWUP_COMMITS, per-commit files) surfaces failures with a WARNING and increments SKIPPED. If this specific call fails transiently, PR_MERGE_SHA silently becomes empty, disabling the [ "$COMMIT_SHA" = "$PR_MERGE_SHA" ] exclusion at line 129 with no indication in the output. This is a live possibility: this repo has squash and rebase merge enabled alongside merge commits, so squash/rebase-merged PRs (whose "merge commit" is an ordinary single-parent commit that the parent-count check at line 124 cannot catch) are a real scenario — exactly what the companion test's "PR's own merge SHA excluded" case was written to guard against. A transient failure on this one call quietly reopens the previously-fixed false-rework bug, specifically for squash/rebase-merged PRs.
Suggestion: Match the pattern used at every other call site: capture stderr to a temp file, check exit status, and on failure emit a WARNING + increment SKIPPED + continue (excluding that PR from the denominator) instead of silently defaulting to "no exclusion."
There was a problem hiding this comment.
Added stderr-to-tmpfile error handling matching all other gh api call sites
| # Get commits after merge by non-bot authors (paginated) | ||
| COMMITS_ERR=$(mktemp) | ||
| if ! FOLLOWUP_COMMITS=$(gh api "repos/${REPO}/commits?since=${MERGED_AT}&until=${FOLLOWUP_UNTIL}&per_page=100" \ | ||
| --paginate --jq '[.[] | select(.author.type != "Bot") | {sha: .sha, author: .author.login, parents: (.parents | length)}]' 2>"$COMMITS_ERR"); then |
There was a problem hiding this comment.
[MEDIUM] FOLLOWUP_COMMITS fetch wraps its --jq filter in [...], violating this repo's own documented --paginate/--jq convention
gh api "repos/${REPO}/commits?..." --paginate --jq '[.[] | select(.author.type != "Bot") | {...}]' wraps the per-item transform in [...], making it a page-scoped aggregating filter. This repo's own docs/contributing/shell-scripting.md explicitly documents that --jq applies independently per page under --paginate and instructs reviewers to "Flag --paginate --jq '... | length' (or any other aggregating filter in --jq) as a medium-severity finding." This is exactly that anti-pattern: with more than 100 raw commits in the window, this yields multiple concatenated JSON-array documents instead of one merged array, breaking the exact-string check [ "$FOLLOWUP_COMMITS" = "[]" ] at line 111 for a multi-page-all-empty case. It currently self-heals (the downstream jq -c '.[]' at line 151 still flattens correctly across concatenated top-level JSON documents, so no data loss occurs today), but it's fragile and inconsistent with the correct, unwrapped per-item pattern already used for BOT_PRS and PR_FILES in the same file. The mock-gh test harness can't simulate real multi-page HTTP pagination, so this class of bug is structurally untestable with the current suite.
Suggestion: Drop the outer [...] to match the BOT_PRS/PR_FILES pattern already used in this file (--jq '.[] | select(...) | {...}'), then aggregate downstream via the repo's documented defensive pattern (| jq -s 'add | ...') if a single flattened array is needed, and replace the "$FOLLOWUP_COMMITS" = "[]" check with [ -z "$FOLLOWUP_COMMITS" ].
There was a problem hiding this comment.
Dropped the outer [...] aggregation. Now uses per-item --jq pattern matching BOT_PRS and PR_FILES
| fi | ||
| rm -f "$COMMIT_FILES_ERR" | ||
|
|
||
| OVERLAP=$(comm -12 <(echo "$PR_FILES" | sort) <(echo "$COMMIT_FILES" | sort) 2>/dev/null || echo "") |
There was a problem hiding this comment.
[MEDIUM] Rework signal is repo-wide same-filename overlap, not scoped to the bot PR's actual merge lineage — risks false positives on shared/hot files, unvalidated against real history
The underlying ask was "human commits touching the same files" as the rework signal; the implementation treats any single-parent, non-bot commit anywhere in repo history within the follow-up date window whose changed files intersect the bot PR's files (via comm -12 on sorted filenames, this line) as a match — fed from a repo-wide commit listing (line 102, filtered only by date and author type, with no path or ancestry scoping) rather than commits that are actual descendants of the bot's merge. On a busy repo like this one (300+ merged bot PRs confirmed live, presumably more human PRs), two unrelated PRs that both happen to touch a frequently-shared file (Makefile, go.mod, a shared CI/config file) within the same week would count as "rework" of each other despite being unrelated. There's no evidence in the PR that this heuristic was run against real historical data and spot-checked for false positives before being proposed as a trust/autonomy metric.
Suggestion: Scope the follow-up commit search to actual descendants of the PR's merge commit (e.g., local-clone git rev-list, or per-changed-file commit history via the commits API with path=) instead of "all repo commits in the date window." At minimum, run the script against a real window of this repo's history before merging and report the observed false-positive rate for hot/shared files in the PR description so reviewers can judge whether the heuristic is trustworthy enough to ship as-is.
There was a problem hiding this comment.
Acknowledged as a known limitation. The heuristic can produce false positives on hot files (Makefile, go.mod). Scoping to merge-commit descendants would be more precise but requires a local clone. Will note as a caveat in the PR description
b819b3f to
d22ad23
Compare
…i#5516) Add scripts/rework-rate.sh that calculates how often agent-merged PRs need human follow-up commits touching the same files. Provides a baseline trust metric for autonomy decisions. Usage: ./scripts/rework-rate.sh [REPO] [DAYS] [FOLLOWUP_DAYS] Outputs total agent PRs, reworked count, rework rate percentage, and a list of reworked PRs with follow-up commit details. Signed-off-by: Benjamin Kapner <bkapner@redhat.com>
- Fix bot identity: use fullsend-ai-coder[bot] with app/ fallback - Add progress indicator (Checking PR N/M) - Add --paginate to PR files API call - Surface API errors as warnings instead of silently skipping - Track and report skipped PRs count Signed-off-by: Benjamin Kapner <bkapner@redhat.com>
- Double-quote variable expansions in date commands - Use if ! command instead of $? checks Signed-off-by: Benjamin Kapner <bkapner@redhat.com>
Address waynesun09 review (1 critical, 2 high, 8 medium): Critical/High fixes: - Filter out merge commits (parent count > 1) to prevent inflated rework rate from merge commit file lists - Fix rate denominator: use CHECKED (not TOTAL) to exclude skipped PRs - Add --paginate to bot PR search and follow-up commits queries - Exclude the PR's own merge commit SHA from follow-up detection Medium fixes: - Drop hardcoded fullsend-ai-fullsend[bot] (author.type != Bot covers it) - Add -u/--utc to all date commands for timezone-independent windows - Skip PRs whose follow-up window hasn't fully elapsed yet - Redirect stderr to temp files instead of 2>&1 to prevent corruption - Use printf + bash array instead of echo -e for output - Surface stderr content in WARNING messages Signed-off-by: Benjamin Kapner <bkapner@redhat.com>
Follows the mock-gh pattern from check-e2e-authorization-test.sh. Covers: genuine single-parent rework detection, merge-commit exclusion, PR own merge SHA exclusion, >100-item paginated response, and API failure handling. Wired into the Makefile script-test target. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Benjamin Kapner <bkapner@redhat.com>
- Fix BSD date argument order: place -v+Nd before the date operand so macOS produces ISO timestamps instead of ctime-style strings - Add error handling for PR_MERGE_SHA fetch (was the only silent gh api call site) - Fix per-commit file-fetch failure double-counting: track PR_HAD_ERROR flag so failed PRs go to SKIPPED, not both SKIPPED and CHECKED - Drop outer [...] aggregation in FOLLOWUP_COMMITS jq filter to match the per-item --paginate/--jq pattern used elsewhere - Rewrite test mock using unquoted heredoc instead of sed -i for macOS/BSD portability Signed-off-by: Benjamin Kapner <bkapner@redhat.com>
|
about the known Limitations bullet missing from PR description you're right, that slipped through. Updated the PR description, third bullet is now in the Known Limitations section. about the eest 4 fixture missing pull_request.merged_at: fixed in 3cdc3a2. The 101-item fixture now includes pull_request.merged_at on each item, and the assertion checks Agent PRs checked: 101 instead of just Found 101 agent PRs, so a regression in the per-PR processing loop would actually get caught. |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review sweep finding below, posted as an inline comment. Verified against the full existing review history on this PR (67 comments) — the exit-status-vs-partial-output discard interaction on the paginated BOT_PRS fetch has not been previously raised; prior threads on that region covered adding --paginate itself and the post-hoc >=1000 count warning, but not this specific case where the discard makes that warning unreachable.
|
|
||
| # Fetch merged PRs by bot authors (paginated) | ||
| BOT_PRS_ERR=$(mktemp) | ||
| if ! BOT_PRS=$(gh api "search/issues?q=repo:${REPO}+is:pr+is:merged+author:${BOT_LOGIN}+merged:>=${SINCE}&per_page=100&sort=created&order=desc" \ |
There was a problem hiding this comment.
[MEDIUM] Top-level bot-PR search discards already-fetched pages and hard-aborts on any mid-pagination failure, making the 1000-result-cap warning unreachable
Lines 37-44: if ! BOT_PRS=$(gh api "search/issues?... --paginate --jq '...' 2>"$BOT_PRS_ERR"); then echo "ERROR: could not fetch bot PRs: ..."; exit 1; fi. In bash, var=$(cmd) captures whatever the command wrote to stdout before it exited, even on non-zero exit — I reproduced this exact pattern with a mock script that emits two successfully-fetched pages of JSON then fails (simulating GitHub's documented 422 "Only the first 1000 search results are available" error, or a secondary rate limit mid-pagination): the if ! branch fires, prints the generic ERROR, and exits 1, even though BOT_PRS at that point already contains the earlier successfully-fetched pages — they are simply discarded.
This means the PR_COUNT -ge 1000 warning added at line 52 specifically to handle the >1000-result scenario (per an earlier review round) can never actually be reached for the exact case it targets, since exceeding the cap is precisely what triggers GitHub's error on the boundary page. Any REPO/DAYS combination with more than 1000 matching merged bot PRs turns into a hard failure with zero report, instead of a capped-but-usable one.
Suggestion: don't gate on the exit status of the whole paginated fetch. Either (a) capture stdout unconditionally, check $BOT_PRS non-empty, and treat a trailing-page error distinguishable via the captured stderr (e.g. matching GitHub's "Only the first 1000 search results are available" message) as a soft warning instead of an abort, or (b) proactively stop requesting further pages once ~1000 items have been collected so the failure never occurs. At minimum, use whatever data was already fetched instead of discarding it on any failure during the paginated sequence.
gh --paginate can fail on the boundary page (e.g. GitHub's 1000-result Search API cap returns 422) after emitting valid data on earlier pages. The previous if-! pattern discarded that data and hard-aborted. Now stdout is captured regardless of exit status: if we have data but the command failed, continue with a warning instead of aborting. Signed-off-by: Benjamin Kapner <bkapner@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Fixed in bbfeb37. The bot-PR search no longer gates on exit status. Stdout is captured regardless; if we got data but pagination failed (e.g. the 422 at the 1000-result boundary), we continue with a warning instead of discarding everything. If we got zero data and a non-zero exit, that's still a hard abort. This also makes the >=1000 count warning reachable. |
|
The script logic looks good — solid error handling and test coverage after the review iterations. Question on placement: should this live in As a standalone script here, there's no persistence or trending — each run recomputes from scratch. If this is meant to be a trust metric for autonomy decisions (as the PR description says), it needs to be tracked over time, and metrics already solves that problem. If there's also value in shipping it in fullsend as a customer-facing diagnostic (so orgs running fullsend can assess agent quality on their own repos), that's a separate use case worth calling out — but the fullsend-ai org's own data should still be collected in metrics. |
|
@maruiz93 i think this serves two complementary use cases:
so i think both placements are right for different reasons, and this PR doesn't need to pick one over the other. BTW the CI failure here is unrelated to this PR. gitlint_rules_test.py fails with ModuleNotFoundError: No module named 'gitlint' during make script-test. All rework-rate tests pass. Looks like a pre-existing issue with the test setup importing from gitlint instead of gitlint-core. |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review sweep findings below, posted as inline comments. Summary: 1 critical, 3 medium. Deduplicated against the extensive existing review history on this PR (68 prior comments checked) — none of these overlap with prior threads.
| continue | ||
| fi | ||
|
|
||
| # Skip merge commits (2+ parents); their files list reflects the full merge, not incremental work |
There was a problem hiding this comment.
[CRITICAL] Parent-count filter excludes the exact commit shape this repo's normal human PR-merge workflow produces, making the tool report near-zero rework against its own default target repo
The follow-up-commit loop hard-skips any commit with more than one parent (if [ "$PARENT_COUNT" -gt 1 ]; then continue; fi), on the premise that 2-parent commits are "noise merges" whose file list isn't incremental work. I independently verified live against gh api repos/fullsend-ai/fullsend/commits (this script's own default target repo): of the last 30 commits, every human-authored commit (e.g. maruiz93, ifireball, ascerra, waynesun09) has parents: 2 except one, while every bot-authored commit (fullsend-ai-coder[bot], renovate-fullsend[bot]) has parents: 1. This confirms the normal, dominant merge shape for human PRs in this repo is a 2-parent merge commit, and GitHub's commit API diffs a merge commit against its first parent (i.e. the PR's own diff, not some unrelated combined tree) — so there is no correctness basis for excluding it. As written, the filter therefore discards essentially all genuine human follow-up/fix commits merged the normal way, driving the reported "Rework rate" toward 0% regardless of actual human cleanup activity.
This is materially broader than the already-acknowledged "pre-merge fix branches" Known Limitation on this PR (which describes a narrow timing edge case where a fix branch predates the bot PR's merge): the bug identified here fires on ordinary, correctly-timed human follow-up PRs merged via the repo's standard 2-parent merge-commit convention, with no timing precondition at all. The companion test suite locks in the same wrong assumption: Test 2 in scripts/rework-rate-test.sh ("merge commit (2 parents) excluded from rework") asserts a 2-parent commit touching the same file as the bot PR must NOT count as rework, hard-coding the exact backward behavior as correct, so make script-test passes while the shipped tool silently misses most real rework in its own default repo.
Suggestion: Do not filter follow-up commits by parent count at all — GitHub's commits API already diffs any commit (1- or 2-parent) against its first parent, so the files list already reflects that PR's own diff regardless of parent count. If double-counting a long-lived branch's intermediate single-parent commits alongside its own merge commit is the real concern, dedupe on overlap-found-once-per-PR (which the script already does via break) rather than excluding all 2-parent commits outright. Update/remove scripts/rework-rate-test.sh Test 2 accordingly and add a case asserting a 2-parent human PR-merge commit touching an overlapping file IS correctly detected as rework, then re-validate the resulting rate against this repo's real history before treating the metric as trustworthy.
| PARENT_COUNT=$(echo "$commit_json" | jq -r '.parents') | ||
|
|
||
| # Skip commits with no linked GitHub identity | ||
| if [ "$COMMIT_AUTHOR" = "null" ]; then |
There was a problem hiding this comment.
[MEDIUM] "Follow-up commits with no linked GitHub identity" counter is incremented before the file-overlap check, so it counts commits unrelated to the PR
Inside the per-commit loop, SKIPPED_NULL_AUTHOR is incremented and the commit is skipped as soon as COMMIT_AUTHOR is null (this line) — before COMMIT_FILES is ever fetched or compared against PR_FILES (the overlap check happens later, at line 178, and only for commits that survive this and the merge-commit/self-SHA filters). Since the follow-up-commit query scans ALL repo commits in the date window (not scoped to the PR's own files), every unrelated null-author commit merged anywhere in the repo during the follow-up window gets counted into "Follow-up commits with no linked GitHub identity (excluded)" for every bot PR whose window it falls in, even if it never touches any file the PR touched. This inflates a line item that's presented as a diagnostic about missed detections for that specific PR, when most of the count may have never been a candidate for overlap. The existing test (scripts/rework-rate-test.sh Test 5, "null-author commit excluded with accounting") only covers a null-author commit that DOES touch the same file (src/main.go), so it can't catch this over-counting.
Suggestion: Move the null-author check after computing OVERLAP, and only increment SKIPPED_NULL_AUTHOR when that specific commit's files intersect PR_FILES — or relabel the line to something like "Follow-up commits in window with no linked identity (not evaluated for overlap)" to stop implying a causal link to the PR. Add a fixture where a null-author commit touches an unrelated file and assert it is not counted.
| exit 0 | ||
| fi | ||
|
|
||
| if [ "$BOT_PRS_EXIT" -ne 0 ]; then |
There was a problem hiding this comment.
[MEDIUM] The newly-added "continue on partial pagination failure" recovery path (bbfeb37) has zero test coverage
The most recent commit on this PR (bbfeb37, the current head) changed the bot-PR search so that a non-zero exit from gh api --paginate no longer discards already-fetched stdout: if BOT_PRS is non-empty despite BOT_PRS_EXIT != 0, the script now prints a WARNING and continues with partial results (this line and the next) instead of hard-failing. This is exactly the documented GitHub Search 1000-result-boundary 422 scenario called out in the comment at lines 37-38. scripts/rework-rate-test.sh's only failure case (Test 6, "API failure on bot-PR search exits with error") simulates a total failure via GH_FAIL=true, which makes the mock gh return empty stdout with a non-zero exit — it never exercises the "valid data on stdout + non-zero exit" branch. This design-bearing recovery path (which determines whether the script degrades gracefully or silently under-reports on the exact GitHub API quirk it was written to handle) currently ships with no regression protection.
Suggestion: Extend the mock gh (or add a dedicated fixture) to emit valid JSON on stdout combined with a non-zero exit code for the search/issues call, and assert the script prints the WARNING and still reports the partial PR_COUNT/results instead of erroring out.
| # Get commits after merge by non-bot authors (paginated) | ||
| COMMITS_ERR=$(mktemp) | ||
| if ! FOLLOWUP_COMMITS=$(gh api "repos/${REPO}/commits?since=${MERGED_AT}&until=${FOLLOWUP_UNTIL}&per_page=100" \ | ||
| --paginate --jq '.[] | select(.author == null or .author.type != "Bot") | {sha: .sha, author_login: (if .author != null then (.author.login // "unknown") else null end), parents: (.parents | length)}' 2>"$COMMITS_ERR"); then |
There was a problem hiding this comment.
[MEDIUM] Human-vs-automation classification relies solely on GitHub's author.type == "Bot", with no allowance for PAT/machine-user automation
The follow-up-commit filter (select(.author == null or .author.type != "Bot"), this line) treats any commit not linked to a GitHub App/Bot-type account as human rework signal. This holds for the two automations actually observed in fullsend-ai/fullsend (fullsend-ai-coder[bot] and renovate-fullsend[bot], both type: Bot, confirmed live), but is an unverified assumption for any other org this script is pointed at (its own usage comment documents REPO as an arbitrary argument, e.g. myorg/myrepo). Any automated fixup process authenticated via a personal access token or machine user — a common pattern for internal tooling/formatters/CI auto-fix bots — reports author.type: User and would be silently counted as "human rework," inflating the reported rate for reasons unrelated to actual human review burden. This was never confirmed against any target org's full inventory of automation identities and isn't mentioned as a caveat anywhere in the script or PR description.
Suggestion: Document this assumption in the script's own usage header, and/or accept an optional exclude-list of additional known automation logins to filter alongside the GitHub "Bot" type check, so the heuristic doesn't silently break when pointed at a different org.
…tests Remove the 2-parent merge-commit filter: GitHub's commits API diffs merge commits against first parent, so the files list already reflects the PR's own diff. The filter was excluding all normal human PR merges, driving rework rate toward 0% on repos using merge commits. Move null-author skip after merge-SHA exclusion and relabel the output to clarify it is a window-wide count, not per-PR. Add test for partial pagination recovery (stdout + data, non-zero exit) and for null-author commits on unrelated files. Document PAT/machine- user limitation in script header. Signed-off-by: Benjamin Kapner <bkapner@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Addressed all 4 findings in 7999ebd: Parent-count filter (critical): removed entirely. You're right, GitHub's commits API already diffs merge commits against first parent, so the files list reflects the PR's own diff. The filter was excluding exactly the commits it was supposed to detect. Test 2 now asserts the opposite: a 2-parent human merge commit IS detected as rework. Null-author counter (medium): moved the skip after merge-SHA exclusion and relabeled to "Follow-up commits in window with no linked GitHub identity (not evaluated)" so it's clear this is a window-wide count, not per-PR. Keeping the skip before the file fetch to avoid unnecessary API calls. Partial pagination test (medium): added a GH_PARTIAL_FAIL mode to the mock. Test 7 asserts the script prints the WARNING and continues with partial results. PAT/machine-user classification (medium): documented in the script header. An exclude-list parameter feels like scope creep for this version. also this PR has been through 7 review rounds and the script is significantly more robust for it. That said, each fix introduces new surface for the next sweep, and i'd like to avoid an infinite loop. Could you do a final pass and let me know what, if anything, is still blocking approval? Happy to track remaining non-blocking items as |
| fi | ||
|
|
||
| if [ "$BOT_PRS_EXIT" -ne 0 ]; then | ||
| echo "WARNING: pagination error during bot PR fetch ($(cat "$BOT_PRS_ERR" | head -1)). Continuing with partial results." |
There was a problem hiding this comment.
[HIGH] shellcheck SC2002 (useless cat) is currently failing the required test CI check
Verified live against PR head 318e13c: gh pr checks 5517 shows the test check currently failing (run 31596690783), and the job log shows make lint-all's pre-commit shellcheck hook fails with:
In scripts/rework-rate.sh line 63:
echo "WARNING: pagination error during bot PR fetch ($(cat "$BOT_PRS_ERR" | head -1)). Continuing with partial results."
^------------^ SC2002 (style): Useless cat.
followed by make: *** [Makefile:91: lint-all] Error 1 and the job exiting non-zero. I reproduced the same warning locally with shellcheck -o useless-use-of-cat scripts/rework-rate.sh. This is the only lint failure and it is actively blocking the PR (mergeStateStatus: BLOCKED). Every other error-message call site in the file (e.g. lines 111, 126, 137, 170) correctly uses $(cat "$VAR") directly without piping into head; only this line pipes cat | head -1.
Suggestion: replace $(cat "$BOT_PRS_ERR" | head -1) with $(head -1 "$BOT_PRS_ERR") to fix the shellcheck violation and unblock the required test status check.
| COMMIT_AUTHOR=$(echo "$commit_json" | jq -r '.author_login') | ||
|
|
||
| # Skip the PR's own merge commit | ||
| if [ -n "$PR_MERGE_SHA" ] && [ "$COMMIT_SHA" = "$PR_MERGE_SHA" ]; then |
There was a problem hiding this comment.
[MEDIUM] PR description's Known-limitations bullet references a 'parent-count filter' that no longer exists in the code
The PR description's third Known-limitations bullet states pre-merge fix branches are invisible because "the merge commit is excluded by the parent-count filter." Commit 7999ebd ("remove parent-count filter, fix null-author counting") removed that filter entirely. Verified against the current head: there is no PARENT_COUNT/parent-count logic anywhere in scripts/rework-rate.sh — the only SHA-based exclusion left is the exact-match check against this PR's own merge_commit_sha (this line and the following few, if [ -n "$PR_MERGE_SHA" ] && [ "$COMMIT_SHA" = "$PR_MERGE_SHA" ]). rework-rate-test.sh Test 2 ("merge commit (2 parents) detected as rework") explicitly asserts that other 2-parent merge commits ARE counted as rework, confirming no parent-count filtering happens anywhere. This makes the PR's own documentation of its real blind spot inaccurate — readers relying on the Known Limitations section to understand what the tool actually misses will be misled about the true mechanism (and the true limitation, since the real exclusion is far narrower — only this exact PR's own merge SHA — than a general parent-count filter would be).
Suggestion: update the third Known-limitations bullet to describe the actual exclusion mechanism (exact merge_commit_sha match only, no parent-count filtering), and re-derive the "pre-merge fix branch" limitation description from what the code truly does.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review sweep findings below, posted as inline comments. Summary: 2 medium. One additional medium candidate (docs/glossary.md naming collision) was checked and skipped as a duplicate of the already-raised fullsend-ai/metrics naming-collision finding in this thread.
| if [ -n "$OVERLAP" ]; then | ||
| FOUND_REWORK="yes" | ||
| REWORKED_LINES+=(" #${PR_NUM} - ${PR_TITLE}") | ||
| REWORKED_LINES+=(" Follow-up: ${COMMIT_SHA:0:7} by @${COMMIT_AUTHOR} (same files: $(echo "$OVERLAP" | head -3 | tr '\n' ', '))") |
There was a problem hiding this comment.
[MEDIUM] set -o pipefail + head can abort the whole script mid-run when overlap file lists are large
REWORKED_LINES+=("... (same files: $(echo "$OVERLAP" | head -3 | tr '\n' ', '))") is an array-append assignment, and under set -euo pipefail (line 18) a SIGPIPE from head -3 closing early propagates and kills the whole script. I reproduced this live: with a small/realistic OVERLAP (4-5 short filenames) the script completes fine (confirmed no crash), but once OVERLAP grows large enough to exceed the pipe buffer before head -3 reads and closes (empirically ~5000 short filenames in my repro, fewer needed for longer paths), the echo | head pipeline SIGPIPEs and the script exits 141 immediately, discarding all accumulated report output including PRs already confirmed as reworked.
This differs from the already-flagged line-63 cat "$BOT_PRS_ERR" | head -1 pattern (existing unresolved review comment, HIGH, shellcheck SC2002) which I verified does NOT actually crash the script even under pipefail, because it's interpolated into a plain echo argument rather than an assignment/array-append context — bash's set -e/pipefail only propagates command-substitution failures out of assignment-like contexts (var=$(...), arr+=(...)), not out of substitutions embedded in an unrelated command's arguments.
Suggestion: Replace with a construct that can't SIGPIPE under pipefail, e.g. printf '%s\n' "$OVERLAP" | awk 'NR<=3{printf "%s%s", (NR>1?", ":""), $0}', or head -n 3 <<<"$OVERLAP" 2>/dev/null || true before interpolating. Add a test case with a very large overlap file list (thousands of entries) to catch a regression.
| continue | ||
| fi | ||
|
|
||
| CHECKED=$((CHECKED + 1)) |
There was a problem hiding this comment.
[MEDIUM] PRs whose only follow-up commits are null-author are silently reported as "checked, not reworked" rather than excluded/flagged
When every follow-up commit for a PR has a null/unlinked author, each one hits the continue (a few lines up) after incrementing only SKIPPED_NULL_AUTHOR, without ever setting PR_HAD_ERROR or FOUND_REWORK. After the inner loop exits, the PR_HAD_ERROR check above is false, so execution falls through to this line (CHECKED=$((CHECKED + 1))) with FOUND_REWORK still empty — the PR is counted in the denominator and reported as clean (not reworked), even though its rework status was never actually evaluated.
This is a different bug from the existing unresolved review comment on this file (about SKIPPED_NULL_AUTHOR being incremented for commits before the file-overlap check) — that comment is about the diagnostic counter being noisy, not about the reported rework rate itself silently including unresolved PRs as confirmed-clean.
Suggestion: Track whether every surviving follow-up commit for a PR was null-author (no evaluable commit at all), and route that case to SKIPPED_ERROR (or a new SKIPPED_UNRESOLVED bucket) instead of CHECKED, so the published rate doesn't count "never evaluated" as "evaluated and clean."
…or PR accounting - Replace `$(cat "$file" | head -1)` with `$(head -1 "$file")` to fix SC2002 (useless cat) that was failing the CI shellcheck hook - Replace `echo "$OVERLAP" | head -3` with `head -n 3 <<<"$OVERLAP"` to avoid SIGPIPE under pipefail when overlap lists are large - PRs whose only follow-up commits are null-author are now routed to SKIPPED instead of CHECKED, so the rework rate denominator only includes PRs that were actually evaluated Signed-off-by: Benjamin Kapner <bkapner@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Addressed all 4 findings in d35926d: SC2002 useless cat (high): replaced $(cat "$BOT_PRS_ERR" | head -1) with $(head -1 "$BOT_PRS_ERR"). Should unblock CI. SIGPIPE on large overlap (medium): replaced echo "$OVERLAP" | head -3 with head -n 3 <<<"$OVERLAP" so pipefail can't kill the script mid-report. Null-author-only PRs counted as clean (medium): PRs whose only follow-up commits are null-author are now routed to SKIPPED instead of CHECKED. The rework rate denominator only includes PRs that were actually evaluated. Stale Known Limitations bullet (medium): updated the PR description. Third bullet now correctly describes the exact-match merge SHA exclusion instead of the removed parent-count filter. Also added a fourth bullet documenting the PAT/machine-user limitation. |
| SKIPPED_ERROR=$((SKIPPED_ERROR + 1)) | ||
| continue | ||
| fi | ||
| if [ -n "$PR_HAD_NULL_AUTHOR" ] && [ -z "$PR_HAD_EVALUABLE" ] && [ -z "$FOUND_REWORK" ]; then |
There was a problem hiding this comment.
[MEDIUM] Null-author-only follow-up PRs are mislabeled as "Skipped (API errors)"
This is a new regression introduced in the latest commit (fix for shellcheck SC2002 / SIGPIPE / null-author accounting), which fixes the earlier "PRs whose only follow-up commits are null-author get counted as clean" bug — but routes the fix into the wrong counter:
if [ -n "$PR_HAD_NULL_AUTHOR" ] && [ -z "$PR_HAD_EVALUABLE" ] && [ -z "$FOUND_REWORK" ]; then
SKIPPED_ERROR=$((SKIPPED_ERROR + 1))
continue
fi
When every follow-up commit for a bot PR has author: null (no linked GitHub identity) and none of the gh api calls actually failed, this isn't an API error — the script already has a dedicated, correctly-labeled counter for exactly this case (SKIPPED_NULL_AUTHOR, printed as "Follow-up commits in window with no linked GitHub identity"). Instead this path bumps SKIPPED_ERROR, which the final report prints under "Skipped (API errors): ${SKIPPED_ERROR}". An operator reading the report will conclude GitHub API calls failed when in fact zero calls failed.
Confirmed via the companion test suite: scripts/rework-rate-test.sh Test 6 ("null-author-only PR not counted as checked") only asserts Agent PRs checked: 0 and never asserts on the printed skip-reason label, so this mislabeling has zero test coverage and won't be caught by make script-test.
Failure scenario: A bot PR's only follow-up commit in the window is from an unlinked/unsigned identity that doesn't touch an overlapping file (or is the sole follow-up commit overall). The script correctly excludes it from CHECKED, but reports it under "Skipped (API errors): N", misleading anyone reading this as a trust metric about the reliability of the data collection itself.
Suggestion: Introduce a distinct counter (e.g. SKIPPED_NO_IDENTITY) for this per-PR case instead of reusing SKIPPED_ERROR, and print it under its own line (e.g. "Skipped (no evaluable human identity in follow-up window): N"), separate from the genuine "Skipped (API errors)" line. Add a test assertion on the printed label, not just the checked count.
Route PRs whose only follow-up commits lack a linked GitHub identity to SKIPPED_NO_IDENTITY instead of SKIPPED_ERROR, so the report distinguishes "no evaluable human identity" from genuine API failures. Signed-off-by: Benjamin Kapner <bkapner@redhat.com>
|
Fixed in 9a47fb9. Null-author-only PRs now increment a dedicated SKIPPED_NO_IDENTITY counter reported as "Skipped (no evaluable human identity in follow-up window)" instead of reusing SKIPPED_ERROR. Test 6 now asserts on the printed label. @waynesun09 |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Follow-up review focused on the new scripts/rework-rate-test.sh coverage gaps.
| [{"sha":"abc1234","author":{"type":"User","login":"human"},"parents":[{"sha":"p1"}]}] | ||
| EOF | ||
|
|
||
| run_case "PR own merge commit SHA excluded" "Rework rate: 0.0%" |
There was a problem hiding this comment.
[MEDIUM] Test suite has no true-negative file-overlap case
Every test that asserts a 0.0% rework rate does so via a mechanism other than "human commit touched a different file": Test 3 relies on the PR's-own-merge-SHA exclusion, and Tests 5/6 rely on null-author exclusion (and Test 6 is itself dead code per the other finding below). There is no fixture where a real, evaluable (non-null, non-bot) human follow-up commit touches a file disjoint from PR_FILES and the script is asserted to report "Rework rate: 0.0%" with that PR correctly counted in CHECKED but not REWORKED. A regression that made the comm -12 overlap check always "match" (e.g. an accidental unconditional FOUND_REWORK=yes, or a broken sort/comm pipeline) would not be caught by any existing test.
Suggestion: Add a test case with a human (author.type: User) follow-up commit touching a file not in PR_FILES, asserting "Rework rate: 0.0%" with "Agent PRs checked: 1", and ideally a mixed two-PR case asserting 50.0% to lock in the CHECKED/REWORKED arithmetic.
|
|
||
| run_case "null-author commit excluded with accounting" "no linked GitHub identity" | ||
|
|
||
| # --- Test 6: Null-author commit on unrelated file is not counted --- |
There was a problem hiding this comment.
[MEDIUM] Test 6 never exercises the code path its name and comment claim to test
Test 6's header comment says "Null-author commit on unrelated file is not counted", and it sets up a COMMIT_DETAIL fixture with unrelated/other.go specifically to verify that file-overlap logic correctly excludes it. But in rework-rate.sh, the null-author check (if [ "$COMMIT_AUTHOR" = "null" ]) fires and continues before COMMIT_FILES is ever fetched or compared against PR_FILES — so the unrelated-file fixture is never read, and Test 6 is functionally identical to Test 5 (which uses a same-filename fixture) except for which output string it greps for ("no evaluable human identity" vs "no linked GitHub identity"). This test cannot catch a regression in the actual file-overlap comparison for null-author commits, contrary to what its name/comment imply.
Suggestion: Either restructure the null-author skip to occur after the overlap check (if per-PR accounting should depend on whether the commit would have matched), or rewrite Test 6's comment and fixtures to reflect what it actually verifies (that a PR whose only follow-up commits are null-author lands in SKIPPED_NO_IDENTITY, independent of file content), and drop the now-misleading unused COMMIT_DETAIL fixture divergence from Test 5.
|
Closing this — the script doesn't fit in this repo. Why:
Suggested path: re-open this against Thanks for the iteration on the script logic — it's solid, it just needs a different home. |
|
🤖 Finished Retro · ✅ Success · Started 7:10 PM UTC · Completed 7:26 PM UTC Commit: |
Retro: PR #5517 — rework-rate tracking script (closed without merge)What happenedPR #5517 was a human-authored fork PR by Benkapner adding a Because the PR came from a fork, the review agent was never dispatched — all review was performed by humans (waynesun09: 45 findings across 11 rounds; rh-hemartin: initial approval; qodo-code-review[bot]: 3 automated findings). The only agent workflow triggered was this retro on PR close. Review finding breakdownOf the 48 distinct reviewer findings: 12 were logic/correctness bugs (merge-commit misclassification, BSD date silent wrong output, parent-count filter excluding all human PRs), 12 were test gaps (no companion test file initially, vacuous assertions, missing coverage for new code paths), 6 were design/architecture concerns, 5 were lintable issues (SC2002, sed -i portability, echo -e), 3 were portability issues, and 3 were documentation issues. The dominant pattern was fix-introduces-new-bugs: corrections for logic bugs frequently created new logic bugs, driven by an initially absent and persistently weak test suite. Evidence for existing issues
Proposals filed
|
4 participants
Summary
Adds
scripts/rework-rate.sh, a script that calculates how often agent-merged PRs need human cleanup afterward.How it works
Usage
Why this matters
Rework rate is a concrete trust metric. If 5% of agent PRs need human cleanup, you can trust the agent more. If 40% do, you should require human review on everything. The trustworthiness-evidence problem doc identifies this as a gap, and the roadmap references it under Testing (#295).
Known limitations
since=cutoff while the only commit that lands inside the window is the PR's own merge SHA (which is excluded by exact-match). This makes that rework invisible to the metric.author.typefield. Commits from GitHub Apps/Bots (type: "Bot") are excluded, but automated processes using PATs or machine users (type: "User") will be counted as human rework.Related Issue
Closes #5516
Checklist