feat(retro): add flapping detection to retro analysis skill - #540
feat(retro): add flapping detection to retro analysis skill#540Benkapner wants to merge 5 commits into
Conversation
Functional tests did not runFunctional tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the |
PR Summary by QodoAdd flapping detection guidance to retro-analysis skill
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
55 rules 1. Protected skills/ file modified
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| ## Flapping detection | ||
|
|
||
| Check whether the workflow exhibits fix-break oscillation. Flapping wastes agent cycles and often indicates a deeper problem (conflicting instructions, flaky tests, or an approach the agent cannot converge on). | ||
|
|
||
| ### Signals to check | ||
|
|
||
| Dispatch a subagent to gather the data. First, derive the PR number from the originating URL (PR_NUMBER is not exported to the retro agent's environment): | ||
|
|
||
| > "Find all code and fix agent workflow runs for PR #N in `$DISPATCH_REPO`. For each run, get the resulting commit SHA from the run's artifacts or branch history, the list of changed files, and the CI test results (pass/fail)." | ||
|
|
||
| Use workflow-run boundaries (each `gh run list --workflow=code.yml|fix.yml` entry) to define "runs", not individual commits. A single run may produce more than one commit (amend, fixup, incremental push). | ||
|
|
||
| Then check for these patterns: | ||
|
|
||
| 1. **File oscillation:** the same file was changed in two or more consecutive runs, and the changes reverse each other (lines added in run N were removed in run N+1, or vice versa). | ||
| 2. **Test result flipping:** a test that passed after run N fails after run N+1, then passes again after run N+2. | ||
| 3. **Cycle count:** more than N review-fix cycles on the same PR without convergence (the review keeps requesting changes on the same findings). The right threshold depends on repo and task type (default N=2; see flapping-convergence.md open questions on configurability). | ||
|
|
||
| ### When flapping is detected | ||
|
|
||
| Include a proposal with these specifics: | ||
|
|
||
| - **target_repo:** the repo where the fix should land (see Localization guidance below) | ||
| - **title:** Start with "Flapping detected:" followed by what oscillated | ||
| - **what_happened:** List each cycle with the run IDs, which files changed, and how the changes reversed | ||
| - **what_could_go_better:** Identify what might be causing the loop (conflicting review criteria, flaky test, ambiguous instructions) | ||
| - **proposed_change:** Suggest a concrete intervention (clarify the conflicting instruction, fix the flaky test, add a convergence guard) | ||
| - **validation_criteria:** Define a measurable outcome tied to the specific pattern. For example: "The next 2 fix cycles touching <file> should not re-introduce the change reverted in run N+1." | ||
|
|
||
| ### When NOT to flag | ||
|
|
||
| - A single rework cycle (review requested changes, fix addressed them, review approved) is normal, not flapping. | ||
| - Different files changing across runs is normal iteration, not oscillation. | ||
| - Only flag when you see the same changes being applied and reversed repeatedly. | ||
|
|
There was a problem hiding this comment.
1. Protected skills/ file modified 📜 Skill insight § Compliance
This PR modifies a protected governance/infrastructure path (skills/retro-analysis/SKILL.md), so it must not be auto-approved and requires explicit human review controls. Without enforcing this, governance-critical content can change without appropriate oversight.
Agent Prompt
## Issue description
The PR modifies a protected path (`skills/`), which must not be auto-approved and should require explicit human/CODEOWNERS review.
## Issue Context
Compliance requires raising a protected-path finding whenever files under paths like `skills/` are modified.
## Fix Focus Areas
- skills/retro-analysis/SKILL.md[124-158]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review findings below (inline comments on specific lines, plus one PR-level finding that isn't tied to a single line).
HIGH — DCO check failing: first commit's author/committer email doesn't match its Signed-off-by trailer
gh pr checks shows DCO failing, and mergeStateStatus is BLOCKED on it. Commit 92802ce0 ("feat(retro): add flapping detection to retro analysis skill (#5512)") has author/committer identity Benjamin Kapner <67113211+Benkapner@users.noreply.github.com>, but its trailer reads Signed-off-by: Benjamin Kapner <bkapner@redhat.com> — a different email than the commit's own author/committer field, which the DCO app requires to match. The second commit (1999d137, current HEAD) is fine: both author/committer and Signed-off-by use bkapner@redhat.com. The PR description's own checklist claims "Commits are signed off (DCO)", which doesn't match the actual failing check state.
Suggestion: rebase and re-sign commit 92802ce0 so its author/committer identity matches its Signed-off-by trailer (e.g., amend the first commit's author to bkapner@redhat.com, or rewrite the trailer to use the noreply GitHub address), then force-push and confirm the DCO check turns green before merge.
|
|
||
| ### Signals to check | ||
|
|
||
| Dispatch a subagent to gather the data. First, derive the PR number from the originating URL (PR_NUMBER is not exported to the retro agent's environment): |
There was a problem hiding this comment.
MEDIUM: PR-number extraction from $ORIGINATING_URL is required but no snippet is shown.
This correctly notes PR_NUMBER isn't exported to the retro agent's env (runner_env only sets ORIGINATING_URL, REPO_FULL_NAME, GH_TOKEN) and says to "derive the PR number from the originating URL" before referencing "PR #N" on the next line — but gives no extraction mechanism. This file's own Setup section shows a concrete one-liner for a comparable derived value (ORG=$(echo "$REPO_FULL_NAME" | cut -d/ -f1)), and scripts/post-code.src.sh already has a working idiom for exactly this extraction (PR_NUMBER_FROM_URL="${PR_URL##*/}") that this section doesn't reference or reuse.
Suggestion: add a one-line snippet consistent with the file's own convention, e.g. PR_NUMBER="${ORIGINATING_URL##*/}", mirroring the existing PR_NUMBER_FROM_URL idiom in scripts/post-code.src.sh.
|
|
||
| Dispatch a subagent to gather the data. First, derive the PR number from the originating URL (PR_NUMBER is not exported to the retro agent's environment): | ||
|
|
||
| > "Find all code and fix agent workflow runs for PR #N in `$DISPATCH_REPO`. For each run, get the resulting commit SHA from the run's artifacts or branch history, the list of changed files, and the CI test results (pass/fail)." |
There was a problem hiding this comment.
MEDIUM: Sourcing a run's "resulting commit SHA" from "artifacts or branch history" is an unverified, likely-wrong shortcut.
gh run list --json does expose a headSha field, but code.yml/fix.yml runs execute in $DISPATCH_REPO (a separate org-level dispatch repo), while the commit they produce is pushed to the source repo's PR branch by post-fix.src.sh only after the sandbox is torn down (git remote set-url origin .../${REPO_FULL_NAME}.git then git push -u origin -- "${BRANCH}"). post-fix.src.sh tracks the agent's own commits internally via PRE_AGENT_HEAD (set by fix.yml before the harness runs) but never echoes a resulting SHA anywhere retrievable later — the final summary only logs "Pushed: yes/no", never a SHA. So a $DISPATCH_REPO run's headSha reflects that repo's own checkout, not the commit landed on the PR, and no artifact or logged SHA exists to bridge the two.
Suggestion: either point to a concrete, confirmed mechanism (e.g., grep the run's job log via gh run view <RUN_ID> --repo $DISPATCH_REPO --log for the pushed branch — verify first, since today post-fix/post-code only log push success/failure, not a SHA) or soften the instruction to acknowledge the correlation is approximate, e.g. "correlate each run to a commit by timestamp against the PR's commit history, since no direct run-to-SHA mapping is exposed today."
|
|
||
| Dispatch a subagent to gather the data. First, derive the PR number from the originating URL (PR_NUMBER is not exported to the retro agent's environment): | ||
|
|
||
| > "Find all code and fix agent workflow runs for PR #N in `$DISPATCH_REPO`. For each run, get the resulting commit SHA from the run's artifacts or branch history, the list of changed files, and the CI test results (pass/fail)." |
There was a problem hiding this comment.
MEDIUM: Dispatch prompt never collects the review-verdict data signal 3 (cycle count) needs.
This subagent prompt only asks to find code and fix agent workflow runs (backed by --workflow=code.yml / --workflow=fix.yml). But signal 3, "Cycle count" (below), is defined as "more than N review-fix cycles ... the review keeps requesting changes on the same findings" — evaluating that requires review verdicts/comments (review.yml runs, which this same file's own "From a PR" section already documents tracing via gh run list --workflow=review.yml), and the data-gathering prompt as written never collects that. A subagent following only the given prompt has no data to evaluate this signal.
Suggestion: extend the dispatch prompt to also gather review.yml runs (or PR review decisions/comments), so "review-fix cycles" and "the review keeps requesting changes on the same findings" are actually answerable from the collected data.
|
|
||
| Dispatch a subagent to gather the data. First, derive the PR number from the originating URL (PR_NUMBER is not exported to the retro agent's environment): | ||
|
|
||
| > "Find all code and fix agent workflow runs for PR #N in `$DISPATCH_REPO`. For each run, get the resulting commit SHA from the run's artifacts or branch history, the list of changed files, and the CI test results (pass/fail)." |
There was a problem hiding this comment.
MEDIUM: Dispatch prompt asks for CI test results from the wrong repo.
The prompt asks to get "the CI test results (pass/fail)" from code.yml/fix.yml runs in $DISPATCH_REPO, but those are agent-orchestration runs, not CI runs — no capture/upload of check-run or test-conclusion data exists in post-code.src.sh or post-fix.src.sh. The PR's actual CI results are computed by workflows in the source repo ($REPO_FULL_NAME) against the resulting commit SHA — a separate repo and separate query this instruction never mentions. As written, a subagent has no path to the "CI test results (pass/fail)" data point needed for signal 2 (Test result flipping).
Suggestion: split the instruction — get run IDs/changed files from $DISPATCH_REPO's code.yml/fix.yml runs, then separately query $REPO_FULL_NAME's checks for each resulting commit (e.g. gh api repos/$REPO_FULL_NAME/commits/<sha>/check-runs) to get pass/fail. Make the two-repo nature of this lookup explicit, as the rest of the file already does for review/fix dispatch tracing.
|
|
||
| Use workflow-run boundaries (each `gh run list --workflow=code.yml` or `--workflow=fix.yml` entry) to define "runs", not individual commits. A single run may produce more than one commit (amend, fixup, incremental push). | ||
|
|
||
| Flapping detection applies to PR-based workflows with code/fix cycles. For issue-only retros (no PR created yet), skip this section. |
There was a problem hiding this comment.
MEDIUM: Issue-triggered retro carve-out skips flapping detection even when a PR already exists.
This carve-out tells the agent when to skip flapping detection but never says how to check whether a PR already exists when ORIGINATING_URL is an issue URL. This file's own "From an issue" section documents that issue-triggered retros routinely have downstream code dispatches once the issue reaches "ready-to-code", and retros fire either automatically on PR close or on-demand via a comment (which can be posted on the original issue after its PR already went through code/review/fix cycles) — precisely the scenario most likely to contain flapping and most likely to be silently skipped by the blanket "no PR created yet" language.
Suggestion: add an explicit lookup before deciding to skip, e.g. gh issue view <url> --json closedByPullRequestsReferences, to check whether a PR already exists/is linked to the issue, and only skip if none is found.
| Then check for these patterns: | ||
|
|
||
| 1. **File oscillation:** the same file was changed in two or more consecutive runs, and the changes reverse each other (lines added in run N were removed in run N+1, or vice versa). | ||
| 2. **Test result flipping:** a test that passed after run N fails after run N+1, then passes again after run N+2. |
There was a problem hiding this comment.
MEDIUM: "Test result flipping" signal can't distinguish agent-caused oscillation from pre-existing flaky tests.
This signal is defined purely as "a test that passed after run N fails after run N+1, then passes again after run N+2," with no requirement that the flip correlate with a file the agent actually touched. Signal 1 (file oscillation) requires the diffs to reverse each other; this signal has no equivalent correlation check, so an independently flaky test (timing/network-dependent, unrelated to the PR's changes) satisfies this pattern verbatim even though the agent did nothing wrong. The source doc this feature is ported from (docs/problems/flapping-convergence.md's "Types of flapping" section) explicitly lists "Fix-break oscillation" and "Flaky-test loops" as distinct types, precisely because causes and fixes differ — this text collapses them into one undifferentiated signal. "When NOT to flag" doesn't cover this case either: it only guards against different files changing and requires reversed changes to be the same, neither of which applies to a bare pass/fail/pass sequence.
Suggestion: require this signal to correlate with the agent's own changes (e.g. "the flapping test covers a file the agent modified in the same run" or "the same test does not flap on unrelated PRs/main"), or explicitly separate flakiness-only flipping into a distinct "flaky test" finding rather than folding it into "flapping", consistent with the source doc's own type taxonomy.
|
|
||
| 1. **File oscillation:** the same file was changed in two or more consecutive runs, and the changes reverse each other (lines added in run N were removed in run N+1, or vice versa). | ||
| 2. **Test result flipping:** a test that passed after run N fails after run N+1, then passes again after run N+2. | ||
| 3. **Cycle count:** more than N review-fix cycles on the same PR without convergence (the review keeps requesting changes on the same findings). The right threshold depends on repo and task type (default N=2; see [flapping-convergence.md](https://github.com/fullsend-ai/fullsend/blob/main/docs/problems/flapping-convergence.md) open questions on configurability). |
There was a problem hiding this comment.
MEDIUM: "default N=2" cycle threshold is attributed to a doc that calls the default an open question.
flapping-convergence.md's "Thresholds and configuration" section gives "2 review cycles" only as one illustrative example ("A documentation repo might tolerate only 2 review cycles ... a complex backend service might allow 5 cycles"), not a recommended default. Its "Open questions" section explicitly lists "What is the right default flapping budget (max iterations before circuit-breaking)?" as unresolved. The doc's one actual stated default ("default: 3 iterations") is for a different, harness-level state-hashing mechanism, not review-fix cycles. No configurability mechanism for N exists anywhere in this repo.
Suggestion: either state N=2 as this skill's own house default without attributing it to flapping-convergence.md (since that doc leaves the default unresolved), or cite the actual source (the originating issue, which used "2 fix cycles" in its own description) instead of a doc section that explicitly calls the default an open question. Drop the "configurability" claim until a real per-repo/task-type threshold mechanism exists.
Signed-off-by: Benjamin Kapner <bkapner@redhat.com>
- Clarify gh run list workflow syntax (split pipe into separate entries) - Add note that flapping detection is PR-only, skip for issue retros - Convert flapping-convergence.md to full URL link (file lives in fullsend-ai/fullsend, not in this repo) Signed-off-by: Benjamin Kapner <bkapner@redhat.com>
- Add PR_NUMBER extraction snippet matching file's Setup convention - Extend dispatch prompt to include review.yml runs and per-commit check-runs from $REPO_FULL_NAME (not $DISPATCH_REPO) - Add issue-triggered retro handling (check for linked PR before skip) - Require test-result flipping to correlate with agent-changed files - Own the N=2 threshold as a starting point instead of attributing it to flapping-convergence.md - Note that run-to-commit correlation is approximate (by timestamp) Signed-off-by: Benjamin Kapner <bkapner@redhat.com>
1999d13 to
6495895
Compare
|
Addressed all findings:
|
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Follow-up review on the flapping-detection fixes from the previous round. Two new HIGH-severity issues found in the just-added code (details inline).
|
|
||
| ### Signals to check | ||
|
|
||
| Flapping detection applies to PR-based workflows with code/fix cycles. If `$ORIGINATING_URL` is an issue URL, check whether a PR is linked (`gh issue view <url> --json closedByPullRequestsReferences`) before skipping; issue-triggered retros routinely have downstream code dispatches once the issue reaches `ready-to-code`. |
There was a problem hiding this comment.
HIGH — PR_NUMBER derivation ignores the linked-PR lookup it just told the agent to perform
This line says: "If $ORIGINATING_URL is an issue URL, check whether a PR is linked (gh issue view <url> --json closedByPullRequestsReferences) before skipping" — but the code block a few lines below unconditionally does PR_NUMBER="${ORIGINATING_URL##*/}", which never consumes the closedByPullRequestsReferences result. When $ORIGINATING_URL is an issue URL (the exact case this sentence says must not be skipped), this silently sets PR_NUMBER to the issue number rather than the linked PR's number, so the subagent dispatch ("Find all code and fix agent workflow runs for PR #N") then queries the wrong PR — silently returning empty/wrong data instead of erroring.
This looks like a new gap introduced by this round's fix: the previous review round asked for (a) a PR_NUMBER extraction snippet and (b) an issue-linked-PR check, and both were added literally but never wired together.
Suggestion: Branch on the URL shape — if $ORIGINATING_URL matches /pull/, use ${ORIGINATING_URL##*/} directly; if it matches /issues/, extract the PR number from the closedByPullRequestsReferences check (e.g. .closedByPullRequestsReferences[0].number), or skip flapping detection if no PR is linked.
There was a problem hiding this comment.
Fixed. The derivation now branches on URL shape: /pull/ URLs extract directly via ${ORIGINATING_URL##*/}, /issues/ URLs query closedByPullRequestsReferences for the linked PR number and skip flapping detection if none is found.
|
|
||
| Dispatch a subagent to gather the data: | ||
|
|
||
| > "Find all code and fix agent workflow runs for PR #N. For each `gh run list --workflow=code.yml` and `gh run list --workflow=fix.yml` run, get the list of changed files. Also find `gh run list --workflow=review.yml` runs to identify review-fix cycles. For CI test results, query `$REPO_FULL_NAME`'s check runs for each resulting commit (`gh api repos/$REPO_FULL_NAME/commits/<sha>/check-runs`)." |
There was a problem hiding this comment.
HIGH — Subagent dispatch prompt drops --repo "$DISPATCH_REPO" on all three gh run list calls, breaking the file's own established convention
This dispatch prompt runs gh run list --workflow=code.yml, gh run list --workflow=fix.yml, and gh run list --workflow=review.yml with no --repo flag. Every other workflow-run lookup for these exact workflow names elsewhere in this same file (Setup / "From an issue" / "From a PR" sections above) explicitly scopes the call with --repo "$DISPATCH_REPO", because code.yml/fix.yml/review.yml execute in the org's dispatch repo (this file's own Setup section derives DISPATCH_REPO="${ORG}/.fullsend" specifically for this purpose), not in $REPO_FULL_NAME. Without the flag, gh run list resolves against whatever repo context the subagent is actually operating in (the source repo), which doesn't contain these runs — producing a silently empty or wrong result set.
This wasn't addressed by this round's fix (which added the review.yml and check-runs querying but didn't add the --repo flags).
Suggestion: Add --repo "$DISPATCH_REPO" to all three gh run list --workflow=... invocations in the subagent prompt, consistent with every other run-lookup example in the file.
There was a problem hiding this comment.
Fixed. All three gh run list calls now include --repo "$DISPATCH_REPO", consistent with the rest of the file. Missed those when adding review.yml in the previous round
- Branch PR_NUMBER derivation on URL shape: /pull/ uses direct extraction, /issues/ queries closedByPullRequestsReferences and skips flapping detection if no PR is linked - Add --repo "$DISPATCH_REPO" to all three gh run list calls in the subagent dispatch prompt, matching the file's existing convention Signed-off-by: Benjamin Kapner <bkapner@redhat.com>
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review findings below (inline comments on specific lines).
| --jq '.closedByPullRequestsReferences[0].number // empty') | ||
| if [[ -z "$PR_NUMBER" ]]; then | ||
| # No linked PR; skip flapping detection. | ||
| return |
There was a problem hiding this comment.
HIGH — Bare return outside a function silently fails, so the "skip flapping detection" branch never actually skips
The issue-URL branch does:
if [[ -z "$PR_NUMBER" ]]; then
# No linked PR; skip flapping detection.
return
fiThis is a top-level bash block, not a function or sourced script. Verified directly: bash -c 'PR_NUMBER=""; if [[ -z "$PR_NUMBER" ]]; then echo hi; return; fi; echo after' prints bash: return: can only \return' from a function or sourced scriptto stderr, then still prints "after" and exits 0 — the early-exit never happens; execution falls through into the dispatch-a-subagent step with an emptyPR_NUMBER`.
Suggestion: wrap the whole PR-number derivation in a shell function (e.g. derive_pr_number() { ... }) so return is valid, or replace the bare return with prose guidance consistent with the rest of the file (e.g. "If PR_NUMBER is empty, skip the rest of this section") rather than a literal shell statement that silently no-ops when copy-pasted as a top-level command.
| # once the issue reaches ready-to-code. | ||
| PR_NUMBER=$(gh issue view "$ORIGINATING_URL" \ | ||
| --json closedByPullRequestsReferences \ | ||
| --jq '.closedByPullRequestsReferences[0].number // empty') |
There was a problem hiding this comment.
HIGH — Cross-repo issue-to-PR resolution can silently pick a PR number from a different repository
PR_NUMBER is derived via gh issue view "$ORIGINATING_URL" --json closedByPullRequestsReferences --jq '.closedByPullRequestsReferences[0].number // empty' with no check that the referenced PR lives in $REPO_FULL_NAME. Verified concretely against this very PR's own linked issue: gh issue view https://github.com/fullsend-ai/fullsend/issues/5512 --json closedByPullRequestsReferences returns PR #540 in fullsend-ai/agents — a different repo than the issue's own repo (fullsend-ai/fullsend). If $REPO_FULL_NAME were fullsend-ai/fullsend for that retro, PR_NUMBER would be set to 540 from a completely unrelated repo, and every downstream call (gh api repos/$REPO_FULL_NAME/commits/<sha>/check-runs, the subagent prompt "for PR #N") would silently operate against the wrong PR number in the wrong repo, producing a plausible-looking but wrong flapping analysis with no error surfaced.
Suggestion: filter the jq query to entries whose repository.owner.login + "/" + repository.name matches $REPO_FULL_NAME before taking [0], and skip flapping detection if no same-repo match exists.
|
|
||
| Dispatch a subagent to gather the data: | ||
|
|
||
| > "Find all code and fix agent workflow runs for PR #N. For each `gh run list --workflow=code.yml --repo "$DISPATCH_REPO"` and `gh run list --workflow=fix.yml --repo "$DISPATCH_REPO"` run, get the list of changed files. Also find `gh run list --workflow=review.yml --repo "$DISPATCH_REPO"` runs to identify review-fix cycles. For CI test results, query `$REPO_FULL_NAME`'s check runs for each resulting commit (`gh api repos/$REPO_FULL_NAME/commits/<sha>/check-runs`)." |
There was a problem hiding this comment.
MEDIUM — Subagent dispatch prompt gives no method to correlate dispatch-repo runs to PR #N specifically
The dispatch prompt says "Find all code and fix agent workflow runs for PR #N" but the given commands (gh run list --workflow=code.yml --repo "$DISPATCH_REPO", etc.) have no PR-number filter — gh run list cannot filter by PR number, and $DISPATCH_REPO is a shared org-level repo where many PRs' runs coexist. Other correlation needs elsewhere in this file at least reference the agent/{issue}-{slug} branch-name convention; this new prompt doesn't reuse or reference it, and even that convention encodes the issue number, not the PR number, so it wouldn't directly solve "for PR #N" either. As written, a subagent has no concrete way to narrow results to the runs belonging to PR #N.
Suggestion: add explicit correlation guidance to the subagent prompt (e.g., map PR #N back to its source issue number and filter dispatch-repo runs by the agent/{issue}-{slug} branch, or cross-reference run timestamps against PR #N's commit/comment timeline) so a subagent isn't left to invent its own filtering approach.
|
|
||
| Dispatch a subagent to gather the data: | ||
|
|
||
| > "Find all code and fix agent workflow runs for PR #N. For each `gh run list --workflow=code.yml --repo "$DISPATCH_REPO"` and `gh run list --workflow=fix.yml --repo "$DISPATCH_REPO"` run, get the list of changed files. Also find `gh run list --workflow=review.yml --repo "$DISPATCH_REPO"` runs to identify review-fix cycles. For CI test results, query `$REPO_FULL_NAME`'s check runs for each resulting commit (`gh api repos/$REPO_FULL_NAME/commits/<sha>/check-runs`)." |
There was a problem hiding this comment.
MEDIUM — Data-gathering subagent prompt still doesn't collect review comment content needed to detect Pattern 3 (recurring findings)
Pattern 3 ("Cycle count") requires knowing whether "the review keeps requesting changes on the same findings" across cycles, i.e. comparing review comment/finding content across runs. The current subagent prompt (after the earlier fix that added review.yml run enumeration) only instructs the subagent to find review.yml runs "to identify review-fix cycles" — it counts cycles but never asks the subagent to fetch or compare the actual review comments/findings text. As written, the collected data can tell you there were N review-fix cycles but not whether they concerned the same recurring issue (true flapping) vs. N distinct, legitimate rounds of feedback (normal iteration, which "When NOT to flag" explicitly excludes). This is a narrower, unaddressed remainder of a previously-fixed review comment, which also asked for "PR review decisions/comments" in addition to review.yml runs.
Suggestion: extend the subagent prompt to fetch and summarize the review comments/findings for each review.yml run (e.g. via PR review comments or review-run artifacts) so the retro agent can compare finding content across cycles, not just count cycles.
- Replace bash code block with prose guidance for PR_NUMBER derivation,
removing the bare return that silently fails outside a function
- Filter closedByPullRequestsReferences to same-repo PRs by matching
repository.owner.login/repository.name against $REPO_FULL_NAME
- Add agent/{issue}-{slug} branch convention to dispatch prompt so
subagent can correlate dispatch-repo runs to a specific PR
- Extend review.yml collection to fetch review comments/findings
content (not just run counts) for cycle-count comparison
Signed-off-by: Benjamin Kapner <bkapner@redhat.com>
|
Fixed all four in the latest push:
|
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review findings below (inline comments on specific lines) — round 4, on top of the round-3 fixes just landed.
|
|
||
| Dispatch a subagent to gather the data: | ||
|
|
||
| > "Find all code, fix, and review agent workflow runs related to PR #N. The PR branch follows the `agent/{issue}-{slug}` convention; extract the issue number from the branch name and use it to filter dispatch-repo runs (e.g. match the branch or issue reference in run inputs/logs). For each `gh run list --workflow=code.yml --repo "$DISPATCH_REPO"` and `gh run list --workflow=fix.yml --repo "$DISPATCH_REPO"` run, get the list of changed files. For each `gh run list --workflow=review.yml --repo "$DISPATCH_REPO"` run, fetch the review comments/findings from the corresponding PR review (via `gh api repos/$REPO_FULL_NAME/pulls/N/reviews` and per-review comments) so that finding content can be compared across cycles. For CI test results, query `$REPO_FULL_NAME`'s check runs for each resulting commit (`gh api repos/$REPO_FULL_NAME/commits/<sha>/check-runs`)." |
There was a problem hiding this comment.
HIGH — Subagent dispatch prompt can't obtain changed files: run→commit correlation lives outside the quoted prompt and is itself underspecified
The quoted subagent dispatch prompt (this line) tells the subagent, for each code.yml/fix.yml run, to "get the list of changed files." The only way to do that is via a commit SHA (e.g. gh api repos/$REPO_FULL_NAME/commits/<sha>), but the technique for obtaining that SHA — "Correlate each run to a commit by timestamp against the PR's commit history, since no direct run-to-SHA mapping is exposed today" — is stated two lines below, in a plain prose paragraph outside the closing quote of the subagent prompt. Every other subagent dispatch example in this file is a self-contained one-liner; nothing outside the quote is passed to the subagent. As written, a subagent executing exactly this prompt has no reachable method to correlate a run to a commit, so it cannot fulfill the "get the list of changed files" instruction. Compounding this, even the timestamp-correlation technique itself (once made reachable) is underspecified: no tolerance window is given, no rule for resolving multiple commits/runs clustered within a short interval (e.g., amend immediately followed by a real push), and no acknowledgment that GitHub Actions run timestamps can diverge from git commit timestamps. Verified empirically against the actual dispatch repo (gh api repos/fullsend-ai/.fullsend/actions/runs) that runs in $DISPATCH_REPO all report the dispatch repo's own headSha/headBranch (e.g. every sampled run showed head_branch=main and an identical head_sha regardless of workflow type), confirming there genuinely is no direct SHA shortcut and timestamp correlation is the only lever — which makes it more important, not less, that the technique be both reachable by the subagent and concretely specified.
Suggestion: Move the run→commit correlation instruction inside the quoted subagent prompt itself, and make it concrete, e.g.: "...for each run, correlate it to a PR commit by matching the run's timestamp against the PR's commit history (no direct run-to-SHA mapping is exposed); if two candidate commits/runs fall within N minutes of each other, mark the correlation as uncertain rather than guessing. Then fetch that commit's changed files via gh api repos/$REPO_FULL_NAME/commits/<sha> (.files)."
| Flapping detection applies to PR-based workflows with code/fix cycles. Derive the PR number from the originating URL, branching on its shape: | ||
|
|
||
| - If `$ORIGINATING_URL` matches `/pull/`, extract directly: `PR_NUMBER="${ORIGINATING_URL##*/}"` | ||
| - If it matches `/issues/`, check for a linked PR in the same repo before skipping (issue-triggered retros routinely have downstream code dispatches once the issue reaches `ready-to-code`). Query `gh issue view "$ORIGINATING_URL" --json closedByPullRequestsReferences` and filter to entries whose `repository.owner.login + "/" + repository.name` matches `$REPO_FULL_NAME`. If no same-repo PR is linked, skip flapping detection for this retro. |
There was a problem hiding this comment.
HIGH — Same-repo-only PR filter will skip flapping detection in the exact cross-repo pattern this codebase itself uses
For issue-triggered retros, this line filters closedByPullRequestsReferences to entries whose repository.owner.login + "/" + repository.name matches $REPO_FULL_NAME, skipping flapping detection entirely if no same-repo PR is linked. But this file's own "Discovering the agents repo"/"Localization guidance" sections describe a scaffold model where agent-layer work routinely lands in a different repo than the originating issue. This is not hypothetical: I verified it live — gh issue view 5512 --repo fullsend-ai/fullsend --json closedByPullRequestsReferences shows the very issue this PR closes (#5512, in fullsend-ai/fullsend) is closed by this PR itself, #540, in fullsend-ai/agents — a different repo. Under the rule as written, a retro run with $REPO_FULL_NAME=fullsend-ai/fullsend on issue #5512 would find no "same-repo" PR and skip flapping detection entirely, even though the exact downstream code/fix/review cycle this feature exists to catch happened in fullsend-ai/agents. This same-repo restriction was added in response to an earlier (already-fixed) review comment warning that an unfiltered lookup could pick a PR from an unrelated repo — but that fix over-corrected and now excludes the platform's own legitimate cross-repo pattern along with the unrelated-repo case it was meant to guard against.
Suggestion: Don't restrict the linked-PR search to same-repo. Accept a linked PR in $REPO_FULL_NAME or in the agents/dispatch repo discovered via the existing "Discovering the agents repo" procedure (or any repo, with the correlation left to context), and only skip flapping detection if no linked PR is found in any relevant repo — not just $REPO_FULL_NAME.
|
|
||
| Dispatch a subagent to gather the data: | ||
|
|
||
| > "Find all code, fix, and review agent workflow runs related to PR #N. The PR branch follows the `agent/{issue}-{slug}` convention; extract the issue number from the branch name and use it to filter dispatch-repo runs (e.g. match the branch or issue reference in run inputs/logs). For each `gh run list --workflow=code.yml --repo "$DISPATCH_REPO"` and `gh run list --workflow=fix.yml --repo "$DISPATCH_REPO"` run, get the list of changed files. For each `gh run list --workflow=review.yml --repo "$DISPATCH_REPO"` run, fetch the review comments/findings from the corresponding PR review (via `gh api repos/$REPO_FULL_NAME/pulls/N/reviews` and per-review comments) so that finding content can be compared across cycles. For CI test results, query `$REPO_FULL_NAME`'s check runs for each resulting commit (`gh api repos/$REPO_FULL_NAME/commits/<sha>/check-runs`)." |
There was a problem hiding this comment.
MEDIUM — Dispatch prompt tells the subagent to match runs via "run inputs", a field gh CLI does not expose
The dispatch prompt instructs: "extract the issue number from the branch name and use it to filter dispatch-repo runs (e.g. match the branch or issue reference in run inputs/logs)." Verified directly against the installed gh CLI: gh run list --json and gh run view --json both list only attempt, conclusion, createdAt, databaseId, displayTitle, event, headBranch, headSha, jobs, name, number, startedAt, status, updatedAt, url, workflowDatabaseId, workflowName as valid fields — there is no "inputs" field on either command, and the GitHub Actions REST API does not expose workflow_dispatch inputs on the run object either. The "branch" half of the instruction is also unusable for this purpose: verified empirically against the live dispatch repo (gh api repos/fullsend-ai/.fullsend/actions/runs) that every sampled run reports head_branch: main regardless of workflow — because a $DISPATCH_REPO run's headBranch is the dispatch repo's own branch, not the source repo's agent/{issue}-{slug} PR branch. Only the "logs" half of the instruction (grepping gh run view --log) is actually viable.
Suggestion: Drop the "run inputs"/"branch" halves of the instruction (neither is retrievable via gh CLI for a $DISPATCH_REPO run) and rely only on log-grepping: gh run list --workflow=code.yml --repo "$DISPATCH_REPO" to list run IDs, then gh run view <RUN_ID> --repo "$DISPATCH_REPO" --log | grep -i "<issue-or-branch-reference>" to confirm each run belongs to the target issue/PR, mirroring the pattern already used in this file's "Discovering the agents repo" section.
| Then check for these patterns: | ||
|
|
||
| 1. **File oscillation:** the same file was changed in two or more consecutive runs, and the changes reverse each other (lines added in run N were removed in run N+1, or vice versa). | ||
| 2. **Test result flipping:** a test that passed after run N fails after run N+1, then passes again after run N+2, and the flapping test covers a file the agent modified in the same run. Tests that flip independently of agent changes may be pre-existing flaky tests, not agent-caused oscillation. |
There was a problem hiding this comment.
MEDIUM — Test-result-flipping signal requires a test-to-file coverage mapping that no described data source provides
Pattern 2 requires knowing that "the flapping test covers a file the agent modified in the same run" to distinguish real agent-caused oscillation from pre-existing flaky tests. But the data-gathering subagent prompt only collects changed-file lists (from code/fix runs) and check-run pass/fail status per commit (via gh api repos/$REPO_FULL_NAME/commits/<sha>/check-runs) — check-runs are named CI jobs (e.g. "unit-tests", "lint"), not per-test results with file-level coverage data. No mechanism (test-name-to-file heuristic, coverage report parsing, etc.) is described anywhere for establishing that a specific flapping test actually "covers" a specific changed file, so the retro agent has no way to actually apply this exclusion criterion from the data it's instructed to collect.
Suggestion: Either specify a concrete heuristic (e.g., match test file paths whose names substring-match a changed file's basename, or parse coverage-report artifacts if one exists), or relax the pattern to something checkable from the collected data (e.g., "a test flips status across 3+ runs; treat as higher-confidence flapping if a related-by-name file also changed in the same runs") and note the strict per-file-coverage version as a future refinement once that data source exists.
|
|
||
| Dispatch a subagent to gather the data: | ||
|
|
||
| > "Find all code, fix, and review agent workflow runs related to PR #N. The PR branch follows the `agent/{issue}-{slug}` convention; extract the issue number from the branch name and use it to filter dispatch-repo runs (e.g. match the branch or issue reference in run inputs/logs). For each `gh run list --workflow=code.yml --repo "$DISPATCH_REPO"` and `gh run list --workflow=fix.yml --repo "$DISPATCH_REPO"` run, get the list of changed files. For each `gh run list --workflow=review.yml --repo "$DISPATCH_REPO"` run, fetch the review comments/findings from the corresponding PR review (via `gh api repos/$REPO_FULL_NAME/pulls/N/reviews` and per-review comments) so that finding content can be compared across cycles. For CI test results, query `$REPO_FULL_NAME`'s check runs for each resulting commit (`gh api repos/$REPO_FULL_NAME/commits/<sha>/check-runs`)." |
There was a problem hiding this comment.
MEDIUM — Unpaginated PR-reviews API call risks truncating exactly the high-review-count PRs flapping detection targets
The subagent prompt instructs fetching gh api repos/$REPO_FULL_NAME/pulls/N/reviews (and per-review comments) with no --paginate or explicit per_page. This GitHub REST endpoint defaults to 30 items per page. Flapping detection by definition targets PRs with an unusually high number of review-fix cycles — exactly the PRs most likely to exceed 30 reviews once bot reviews, human reviews, and re-review events are counted. A silently truncated review list would under-count cycles and could cause the retro agent to miss real flapping or produce an incomplete timeline for Pattern 3 (cycle count) and the recurring-findings comparison this same paragraph exists to support.
Suggestion: Add --paginate (or an explicit per_page=100) to the pulls/N/reviews call in the subagent prompt, consistent with treating a small returned review count with suspicion on PRs known to have many comments.
|
|
||
| 1. **File oscillation:** the same file was changed in two or more consecutive runs, and the changes reverse each other (lines added in run N were removed in run N+1, or vice versa). | ||
| 2. **Test result flipping:** a test that passed after run N fails after run N+1, then passes again after run N+2, and the flapping test covers a file the agent modified in the same run. Tests that flip independently of agent changes may be pre-existing flaky tests, not agent-caused oscillation. | ||
| 3. **Cycle count:** more than 2 review-fix cycles on the same PR without convergence (the review keeps requesting changes on the same findings). This threshold is a starting point; see [flapping-convergence.md](https://github.com/fullsend-ai/fullsend/blob/main/docs/problems/flapping-convergence.md) for open questions on making it configurable per repo/task type. |
There was a problem hiding this comment.
MEDIUM — Cycle-count pattern is narrower than the "review ping-pong" pattern it's modeled on
Signal 3 is defined as "more than 2 review-fix cycles on the same PR without convergence (the review keeps requesting changes on the same findings)." The linked design doc, docs/problems/flapping-convergence.md, describes "Review ping-pong" as: "The code agent addresses the feedback, but the fix introduces a new issue. The review agent catches the new issue. The code agent fixes it but reintroduces the original problem." That is an alternating pattern (finding A → finding B → finding A again), not repetition of "the same findings" every cycle. By this section's literal wording, the canonical ping-pong example from the doc it cites would not obviously qualify as "the same findings" being repeated and could be under-detected, even though it's precisely the scenario the referenced doc uses as its motivating example for this feature.
Suggestion: Broaden the parenthetical to cover alternation as well as repetition, e.g. "(the review keeps requesting changes on the same or alternating findings — a fix for one issue reintroducing a previously-resolved one counts as flapping too)."
2 participants
Summary
Adds a "Flapping detection" section to the retro-analysis skill, teaching the retro agent to identify fix-break oscillation patterns during post-workflow analysis.
Ported from fullsend-ai/fullsend#5513 (closed; the scaffold extraction moved this file to this repo). Adapted to the current version of the file and incorporates review feedback from the original PR.
What it adds
Guidance for detecting three flapping patterns:
Key design decisions
$ORIGINATING_URLrather than assuming$PR_NUMBERis exported (it is not in the retro agent's env)target_repoin the flapping proposal checklist (required by the output schema)validation_criteriafollows the file's existing convention (measurable outcome with specific pattern reference)Related
Checklist