Skip to content

feat(retro): add flapping detection to retro analysis skill #834

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Benkapner wants to merge 1 commit into
base: main
Choose a base branch
from
+37 −0
Open

feat(retro): add flapping detection to retro analysis skill #834

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions skills/retro-analysis/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,43 @@ After subagents return their findings, use your main context to:
3. Form hypotheses about root causes
4. Decide what changes to propose and where

## 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).
Comment on lines +124 to +126

@qodo-code-review qodo-code-review Bot Aug 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Protected skills/ file modified 📜 Skill insight § Compliance

This PR modifies skills/retro-analysis/SKILL.md, which is a protected governance/infrastructure
path requiring explicit human review and must not be auto-approved. Ensure appropriate
reviewers/CODEOWNERS sign off before merge.
Agent Prompt 
## Issue description
The PR changes a protected path (`skills/`), which must not be auto-approved and requires explicit human review.

## Issue Context
Protected governance/infrastructure paths require elevated scrutiny. This PR updates the retro-analysis skill content under `skills/`.

## Fix Focus Areas
- skills/retro-analysis/SKILL.md[124-126]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


### Applicability

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 "$ORIGINATING_URL" --json closedByPullRequestsReferences`) before skipping. If no linked PR exists, skip flapping detection for this retro.

@waynesun09 waynesun09 Aug 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MEDIUM: <PR_NUMBER> placeholder used in the data-gathering prompt is never derived

The Applicability section (this line) only covers the case where $ORIGINATING_URL is an issue URL ("check whether a PR is linked ... before skipping"); it says nothing about the common case where $ORIGINATING_URL is already a PR URL, and in neither branch does it bind a value to the <PR_NUMBER> placeholder used two lines later in the data-gathering prompt ("Find all code, fix, and review workflow runs related to PR #<PR_NUMBER>", line 136). No earlier section in SKILL.md defines $ORIGINATING_URL, PR_NUMBER, or any variable a subagent could substitute here (the Setup section only defines $REPO_FULL_NAME/$DISPATCH_REPO). A subagent following this literally has no stated source for <PR_NUMBER> in either branch.

Suggestion: add one line per branch — if $ORIGINATING_URL is a PR URL, extract its number directly; if it's an issue URL, use the linked PR's number from closedByPullRequestsReferences — and reference that resolved value explicitly when introducing the data-gathering prompt.


### Data gathering

Dispatch a subagent to identify code/fix/review workflow runs for the PR and collect the data needed for pattern detection:

- **Flapping data collector:** "Find all code, fix, and review workflow runs related to PR #<PR_NUMBER> in `<DISPATCH_REPO>`. Each run's log contains an `event_payload` JSON line with `pull_request.head.sha` and `pull_request.number`; parse it to correlate runs to PR commits and to confirm the run belongs to this PR. For each matched run, fetch the commit's changed files and CI check-run results from `<REPO>`. Also fetch the PR's review comments/findings (`--paginate`) so finding content can be compared across review cycles."

@qodo-code-review qodo-code-review Bot Aug 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

2. Unresolved repo placeholders 🐞 Bug ≡ Correctness

The new flapping data collector prompt uses literal <DISPATCH_REPO> and <REPO> tokens even
though this skill defines $DISPATCH_REPO and uses $REPO_FULL_NAME in its recipes. If these
placeholders aren’t substituted, the subagent can run gh commands against an invalid/wrong repo
and the flapping analysis will fail or collect the wrong data.
Agent Prompt 
### Issue description
The flapping data collector prompt uses `<DISPATCH_REPO>` and `<REPO>` placeholders that are not established elsewhere in the skill, despite the skill defining `$DISPATCH_REPO` and consistently using `$REPO_FULL_NAME` for repo-qualified `gh` calls.

### Issue Context
Within `skills/retro-analysis/SKILL.md`, the Setup section defines `DISPATCH_REPO` and subsequent commands use `$DISPATCH_REPO` / `$REPO_FULL_NAME`. The newly added flapping collector prompt should follow the same convention (or explicitly define how placeholders are substituted).

### Fix Focus Areas
- skills/retro-analysis/SKILL.md[124-136]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

@qodo-code-review qodo-code-review Bot Aug 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

3. Assumed event_payload log line 🐞 Bug ☼ Reliability

The flapping data collector prompt unconditionally claims each dispatch-repo run log contains an
event_payload JSON line with pull_request.head.sha and pull_request.number. Repo documentation
for locating dispatch runs currently relies on timestamp/headBranch correlation and does not
establish this log line as a guaranteed interface, so the new guidance can break flapping detection
if the log format differs or the line is absent.
Agent Prompt 
### Issue description
The new flapping collector instructions depend on a specific `event_payload` JSON log line for correlating runs to PR commits. This is not documented elsewhere as a guaranteed log output, and existing run-finding guidance uses timestamp/headBranch matching.

### Issue Context
`skills/finding-agent-runs/SKILL.md` demonstrates the established approach: find shim runs in the source repo and match dispatch-repo runs by timestamp/headBranch. `scripts/pre-code.sh` and `scripts/pre-fix.sh` mention `event_payload` only as an input-validation concern, not as a logged artifact.

### Fix Focus Areas
- skills/retro-analysis/SKILL.md[124-137]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

@waynesun09 waynesun09 Aug 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MEDIUM: <REPO> in the data-gathering prompt is not bound to the linked PR's repo in the cross-repo issue case

This PR's own linked issue demonstrates the gap: the issue lives in one repo and the linked PR (this one) lives in a different repo (closedByPullRequestsReferences on the issue resolves to a PR in a different repo than the issue itself). The Applicability check resolves a linked PR from an issue URL but never states which repo <REPO> (used here: "fetch the commit's changed files and CI check-run results from <REPO>") should resolve to when the linked PR lives in a different repo than $REPO_FULL_NAME.

Note this is distinct from the existing bot review comment on this line (which flags that <DISPATCH_REPO>/<REPO> are literal unsubstituted tokens vs. the skill's $DISPATCH_REPO/$REPO_FULL_NAME convention) — even if that comment's suggested fix of substituting $REPO_FULL_NAME is applied, it would resolve to the wrong repo in exactly this cross-repo scenario, since the actual PR/CI data lives in the linked PR's own repo, not the issue's repo.

Suggestion: explicitly bind <REPO> to the repository field returned by the linked PR (from closedByPullRequestsReferences) when flapping detection is entered via the issue-URL branch, noting it may differ from $REPO_FULL_NAME.


### Patterns to detect

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).

@waynesun09 waynesun09 Aug 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CRITICAL: Pattern 1 (file oscillation) fires on a single reversal, contradicting the skill's own "When NOT to flag" rule

Pattern 1's definition (line 140: "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") triggers on a single N/N+1 reversal — exactly two runs. But the "When NOT to flag" section added in the same diff (lines 155-159) states "A single rework cycle (review requested changes, fix addressed them, review approved) is normal" and "Only flag when you see the same changes being applied and reversed repeatedly." These two sections of the same PR contradict each other: following Pattern 1 literally will flag the default happy-path review-fix cycle (code adds X, review flags X, fix removes X) as flapping, which is precisely the false-positive the exclusion section exists to prevent.

This is compounded by "consecutive runs" being ambiguous given the real workflow sequence is code → review → fix → review → fix, where review runs typically touch no files — if "consecutive" means consecutive workflow runs, oscillation almost never fires; if it means consecutive file-changing runs, it collapses back into the single-reversal false positive. It's also the only one of the three patterns with a 2-run threshold: Pattern 2 requires pass-fail-pass (3 runs) and Pattern 3 requires "more than 2" cycles (3+), so Pattern 1 is inconsistent with its siblings as well as with the exclusion list.

Suggestion: align Pattern 1 with the "repeated reversals" bar used everywhere else in the section — require at least two reversals (e.g. A→B→A across three file-changing runs) rather than a single undo, and explicitly define "consecutive" as consecutive file-changing (code/fix) runs, with review runs used only to correlate finding text rather than for the file-diff comparison.

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.

@waynesun09 waynesun09 Aug 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MEDIUM: Pattern 2 (test result flipping) needs per-test/file-coverage data the collector never fetches

Pattern 2 requires identifying "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" — i.e., both a specific failing test and a test-to-file coverage mapping. The only CI data the collector prompt (line 136) fetches is "CI check-run results," which GitHub's Checks API returns at job/suite granularity (e.g. a single "unit-tests" check), not per-test, and carries no file-coverage mapping. No heuristic is given for how a subagent would derive individual test identity or test-to-file coverage from check-run-level data alone.

Suggestion: either specify a concrete heuristic (e.g. parse per-test names out of CI log output/test-report artifacts, if available) or relax Pattern 2 to what check-run-level data actually supports (e.g. "the same named CI job flips status across 3+ runs while covering the same changed files").

3. **Cycle count:** more than 2 review-fix cycles on the same PR without convergence (the review keeps raising the same or alternating findings, e.g. a fix for one issue reintroducing a previously resolved one). A single rework cycle where the fix addresses the feedback and the review approves is normal iteration, not flapping.

@waynesun09 waynesun09 Aug 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MEDIUM: Hardcoded "more than 2" cycle threshold presents an explicitly unresolved design question as settled

Pattern 3 states as fact that "more than 2 review-fix cycles on the same PR without convergence" is flapping. The design doc this feature implements (docs/problems/flapping-convergence.md in fullsend-ai/fullsend) explicitly says thresholds must vary per repo/task type in its "Thresholds and configuration" section ("A documentation repo might tolerate only 2 review cycles ... A complex backend service might allow 5 cycles ... Default thresholds should be conservative and configurable per repo and per agent role"), and its "Open questions" section lists "What is the right default flapping budget?" as unanswered. This PR hardcodes a fixed, non-configurable "2" in the skill without referencing the doc or noting it as a provisional default.

Suggestion: reference docs/problems/flapping-convergence.md and phrase the threshold as a starting heuristic pending configurable per-repo thresholds, rather than an evidence-based fixed number.


### 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, e.g. "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.
- Different files changing across runs is normal iteration, not oscillation.
- Only flag when you see the same changes being applied and reversed repeatedly.

## Before proposing: check for existing issues

**This step is mandatory.** Before including any proposal in your output, verify that no open issue already covers the same improvement. The retro agent is the primary source of systemic proposals — without this check, repeated runs produce duplicate issues that waste human triage time.
Expand Down
Loading