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

Closed
Benkapner wants to merge 3 commits into from
Closed

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

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
af4e4c1
feat(retro): add flapping detection to retro analysis skill
Benkapner Aug 26, 2026
fc5482d
fix(retro): rewrite flapping run-PR correlation model
Benkapner Sep 9, 2026
ad1613d
fix(retro): bound code-run discovery and move flapping gh recipes
Benkapner Sep 10, 2026
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
143 changes: 143 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 @@ -119,6 +119,149 @@ If you are unsure whether a failure is a genuine flake or a real bug,
say so explicitly in your proposal and recommend investigation before
applying either fix category.

## Flapping detection

Check whether the workflow exhibits fix-break oscillation: the agent
repeatedly applies and reverses the same change, or the review-fix loop
never converges. Flapping wastes agent cycles and often signals a deeper
problem (conflicting instructions or an approach the agent cannot settle
on).

Flapping is distinct from test flakiness (above): flakiness is the *same
commit* passing and failing with no diff between runs; flapping is driven
by the agent's *own changes* across successive code/fix runs.

### Applicability

Flapping detection applies to PR-based workflows with code/fix cycles.
Resolve the target PR before gathering data:

- If the retro originates from a PR, use it directly: `PR_NUMBER` is its
number and `PR_REPO` is its `owner/repo` (parse both from
`$ORIGINATING_URL`).
- If the retro originates from an issue, resolve linked PRs with the
`github-forge` / `gitlab-forge` skill's linked-PR/MR recipe
(`closedByPullRequestsReferences` / `closed_by`) — not the
forge-specific retro-analysis skill, which has no linkage recipe.
That query returns a list of up to 50 nodes, each with `state` and
`url`. Tie-break: prefer the single open linked PR; if none is open,

@waynesun09 waynesun09 Sep 10, 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: Linked-PR tie-break selects on updatedAt, which the query it cites does not return

Lines 142-150 say to resolve linked PRs with the github-forge skill's closedByPullRequestsReferences recipe and, if none is open, "take the most recently updated".

Verified at head, skills/github-forge/SKILL.md:41 returns nodes { number url author { login } state } — there is no updatedAt field, so the tie-break has no data to operate on and the agent must make an undocumented extra call per node.

(Distinct from the round-2 thread, which established that the query returns a list of up to 50 rather than one PR; the tie-break added in response to it is the new, unexecutable part.)

Suggested fix: Add updatedAt to the closedByPullRequestsReferences node selection in skills/github-forge/SKILL.md (and updated_at to the gitlab-forge equivalent), or state in this section that the field must be resolved per node with gh pr view <url> --json updatedAt.

take the most recently updated; if several are open, either skip
flapping detection or analyse each separately and say which PR each
finding refers to. If no linked PR exists, skip flapping detection.
Derive `PR_REPO` by parsing the node's `url` — it may differ from
`$REPO_FULL_NAME`.

Once `PR_REPO` is resolved, re-derive `DISPATCH_REPO` from `PR_REPO`'s
org (`${PR_ORG}/.fullsend`). `$DISPATCH_REPO` built from
`$REPO_FULL_NAME` is the wrong dispatch repo when the PR lives under a
different org. Use `PR_NUMBER` and `PR_REPO` (not `$REPO_FULL_NAME`) for
all data gathering below.

### Data gathering

Dispatch a subagent to identify the code/fix/review workflow runs for

@waynesun09 waynesun09 Aug 26, 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: Run discovery is unbounded, and every existing recipe caps at 10 runs

"Find all code, fix, and review workflow runs related to PR #PR_NUMBER in $DISPATCH_REPO" has no bound and no server-side filter available: dispatch-repo runs are workflow_dispatch with no PR in their ref/branch, so per this section's own premise the only way to identify a run is to download its full log and parse event_payload.

Verified at head, every run-listing recipe the retro agent has uses --limit 10 (skills/retro-analysis/github/SKILL.md: --workflow=code.yml --limit 10, review.yml --limit 10, fix.yml --limit 10; the GitLab file uses per_page=20). In a busy org dispatch repo those windows cover on the order of an hour or two. A PR that flaps for days is exactly the case where the target runs are furthest back, so as written the collector will usually find none of them — or will download hundreds of run logs against the retro harness's 30-minute timeout_minutes (harness/retro.yaml) before synthesis begins.

Suggestion: bound the search explicitly — derive a time window from the PR's createdAt..updatedAt and pass --created <start>..<end> with a raised --limit per workflow, and note that each candidate costs a full log fetch so the window must be bounded before iterating. Reuse the finding-agent-runs discovery recipe rather than reinventing run discovery here.

@waynesun09 waynesun09 Sep 10, 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.

HIGH: Procedure is GitHub-only but lives in the shared skill both forges load, contradicting the file's own convention and the PR's forge-agnostic claim

The whole Data gathering / Matching / Diffs block hard-codes GitHub-only surface: gh run list --created and --limit, workflow_dispatch, event_payload, source_repo, pull_request.number, pull_request.head.sha, pull_request.base.repo.full_name, issue.number. It sits in skills/retro-analysis/SKILL.md, which both GitHub and GitLab retros load, and whose own line 13 says "Use the forge-specific retro-analysis skill for CLI recipes (GitHub or GitLab)".

Verified at head: no other part of this shared file contains a gh command or a GitHub payload field — the convention was clean before this section. Verified too that skills/retro-analysis/gitlab/SKILL.md offers no equivalents: it lists pipelines with curl .../pipelines?per_page=20 (no created-range filter), reads job traces, and has no event_payload, no source_repo, and no MR-number matching. A GitLab retro following this section literally runs commands that do not exist and looks for fields that are never emitted, burning the 30-minute retro timeout. The PR description's claim that the section "stays forge-agnostic" holds only for the PR/issue linkage step, not for the bulk of the procedure.

Suggested fix: Keep the conceptual model in the shared file (match on source repo + number, anchor diffs on consecutive pre-run head SHAs, order by run creation time) and move the GitHub field names and the gh run list --created/--limit recipe into skills/retro-analysis/github/SKILL.md. Either add GitLab equivalents (state.change_proposal.head_sha, entity.id, pipelines updated_after/updated_before) to the gitlab skill, or state plainly that flapping detection is GitHub-only for now and skip it when the forge is GitLab.

this PR. Reuse the `finding-agent-runs` discovery recipe rather than

@waynesun09 waynesun09 Sep 10, 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: The finding-agent-runs recipe the section tells the agent to reuse does not list fix.yml and caps at 5, not 10

Line 163 says "Reuse the finding-agent-runs discovery recipe rather than inventing a new listing", and line 169 justifies the bounded window with "existing recipes cap at 10".

Verified at head: skills/finding-agent-runs/github/SKILL.md lists triage.yml, code.yml, review.yml and retro.yml at --limit 5 and contains no fix.yml recipe at all. The recipe that does list fix.yml (and does use --limit 10) is skills/retro-analysis/github/SKILL.md, a different skill. Fix runs are precisely the file-changing runs Pattern 1 and the step count depend on, so an agent following the literal pointer discovers no fix runs, and the parenthetical justification cites a limit that recipe does not use.

(The round-2 thread on line 162 pointed at finding-agent-runs and asserted the cap-of-10 figure; both were inaccurate — worth correcting now that the text has been written around them.)

Suggested fix: Point the collector at skills/retro-analysis/github/SKILL.md (which already covers code/fix/review), name fix.yml explicitly, give a concrete numeric --limit instead of "raised", and fix or drop the "cap at 10" parenthetical.

inventing a new listing; bound the search to the PR's

@waynesun09 waynesun09 Sep 10, 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.

HIGH: The new createdAt..updatedAt search window filters out the code run the same section says must not be dropped

Lines 164-166 bound run discovery to the PR's createdAt..updatedAt window (--created <start>..<end>). The code run is what creates the PR — post-code opens the PR at the end of the run — so a code run's createdAt is always earlier than PR.createdAt (typically by many minutes). With this window the code run is filtered out before matching ever runs, which is exactly the outcome lines 183-186 warn against: "Matching only on pull_request.number drops the first file-changing run and destroys Pattern 1's baseline." The same commit that added the issue.number match branch to keep code runs also added the window that discards them, so the two halves of the fix cancel out.

(This window was added in response to the round-2 comment on line 162 about unbounded discovery; that thread is addressed — this is a new defect in the fix itself, not a restatement.)

Suggested fix: Anchor the code run separately rather than widening the window (widening would also admit older code runs for the same issue that produced earlier, closed PRs): search code.yml with --created "<=<PR.createdAt>", match on issue.number from the agent/{issue}-{slug} branch, and confirm via the run log printing the created PR URL (PR_REPO/pull/PR_NUMBER). Keep PR.createdAt..PR.updatedAt for fix/review runs only.

`createdAt`..`updatedAt` window (pass `--created <start>..<end>` with a
raised `--limit` per workflow). Dispatch-repo runs are
`workflow_dispatch` with no PR in their ref, so identifying a candidate
requires downloading its log and parsing `event_payload` — an unbounded
"find all runs" will miss long-lived flaps (existing recipes cap at 10)
or blow the retro timeout.

**Matching.** `$DISPATCH_REPO` is org-wide; PR numbers are not unique
across repos. A run belongs to this PR only if `source_repo` (or
`pull_request.base.repo.full_name`) equals `PR_REPO` **and** a
three-way number match mirroring the platform concurrency key holds:

- `pull_request.number == PR_NUMBER`, **or**
- `issue.number == PR_NUMBER` (comment-triggered fix; the PR number
lands in `issue.number`), **or**
- `issue.number` equals the issue parsed from the `agent/{issue}-{slug}`
branch (code runs).

Code runs carry **no** `pull_request` object in `event_payload` (code
dispatches from issue events when `ready-to-code` is applied). Matching
only on `pull_request.number` drops the first file-changing run and
destroys Pattern 1's baseline.

**Diffs.** `event_payload` is built at dispatch time from the triggering
event, so `pull_request.head.sha` is the branch head *before* the run
does any work. Attributing "the commit's changed files" at that SHA
assigns each code/fix run its predecessor's diff. Define run N's
changes as the patch between `head.sha(run N)` and `head.sha(the next
run)` — equivalently, the head SHA of the review run that follows it
(a review is triggered on the pushed commit, so its `head.sha` *is*
the preceding code/fix output). Fetch that compare diff (patch/hunks),
not a file-path list: Pattern 1 needs line-level A→B→A.

**Ordering.** Order runs by workflow `createdAt`, not by commit.
Collapse same-SHA reruns into a single step. The fix agent rebases and
amends, so a `head.sha` recorded in an earlier payload is often no
longer reachable from the PR head — commit order is not a total order
on the PRs this section targets.

Collector prompt sketch: "Using `finding-agent-runs`, list code, fix,
and review runs in `$DISPATCH_REPO` whose `createdAt` falls in this
PR's lifetime. A run matches only if `source_repo == PR_REPO` and the
three-way number rule above holds. For each matched file-changing run,
fetch the compare patch from `head.sha(this run)` to `head.sha(next

@waynesun09 waynesun09 Sep 10, 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.

HIGH: First file-changing run has no start SHA, so the compare-patch recipe is undefined for the run Pattern 1 depends on

Lines 191-196 and the collector prompt at 207-209 define every file-changing run's changes as the compare patch from head.sha(this run) to head.sha(next run). But lines 183-184 state — correctly — that code runs carry no pull_request object in event_payload, so head.sha(this run) does not exist for the code run (the branch is created during that run).

The section goes to some length to re-include the code run via the issue.number match branch and then gives the collector no start anchor for it. Pattern 1 (lines 218-223) needs exactly that run as the A of A→B→A; with no start anchor the agent either skips it — collapsing three file-changing runs into two patches, which the doc itself calls the normal add-then-remove case that must NOT be flagged — or invents one. This is a residual of the round-2 head.sha thread, whose suggested fix assumed head.sha(run N) is always available.

Suggested fix: State the code run's anchors explicitly: start = the PR base commit (gh pr view PR_NUMBER --repo PR_REPO --json baseRefOid, or the merge-base of base and the first review run's head.sha), end = the first review run's head.sha. Add this to the collector prompt sketch so the baseline patch is computable.

run)` and the named CI check results from `PR_REPO`. Also fetch the

@waynesun09 waynesun09 Sep 10, 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 needs check results per anchor SHA; the collector specifies no SHA and no per-commit check recipe exists

Pattern 2 (lines 224-231) compares a named check's status across runs, which requires results at each anchor SHA. The collector prompt asks only for "the named CI check results from PR_REPO" with no SHA specified.

Verified at head: skills/retro-analysis/github/SKILL.md contains no check-run recipe at all (no occurrence of "check"), and the obvious tool, gh pr checks, returns results for the current head only. So an agent gets one snapshot and cannot build the pass/fail sequence the two-flip bar requires. Worse, the natural fallback — reading checks at the run's dispatch-time pull_request.head.sha — reintroduces the exact off-by-one the Diffs paragraph (188-196) was added to fix, attributing the previous run's CI status to this run. This is a residual the round-2 head.sha fix covered for diffs but not for checks.

Suggested fix: Specify per-SHA retrieval at the run's output anchor (the following review run's head.sha, matching the Diffs definition): gh api repos/PR_REPO/commits/<sha>/check-runs --jq '.check_runs[] | {name, conclusion}', and say to use the PR head at retro time for the last file-changing run. Add that recipe to skills/retro-analysis/github/SKILL.md; note checks are keyed to the commit, so they remain queryable for SHAs no longer on the branch.

PR's review comments/findings across all pages."

### Patterns to detect

Count only **file-changing** (code/fix) runs as steps after collapsing
same-SHA reruns; review runs typically touch no files and serve as
diff anchors and finding-text correlation.

1. **File oscillation:** across three or more consecutive file-changing
runs, the same lines in a file are changed, reverted, then changed
again — a repeated A→B→A pattern (run N adds a line, run N+1 removes
it, run N+2 adds it back). Compare hunks from the consecutive-head
patches, not file-path lists. A single add-then-remove is the normal
review-fix cycle, not oscillation.
2. **Check-status flipping:** the same named CI check (e.g. `unit-tests`)
flips *twice* (pass→fail→pass→fail, or two full oscillations) across

@waynesun09 waynesun09 Sep 10, 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's "flips twice" read literally is the same shape the next sentence excludes as convergence

Lines 224-228 give three descriptions of one rule: the check "flips twice", the parenthetical "pass→fail→pass→fail", and "two full oscillations".

Counting flips literally, pass→fail→pass is two flips — and lines 227-228 explicitly exclude that shape ("A single pass→fail→pass is ordinary regress-then-recover ... convergence, not oscillation"), as does the When-NOT-to-flag bullet at line 258. pass→fail→pass→fail is three flips, and "two full oscillations" from a passing start is pass→fail→pass→fail→pass. An implementing agent applying the lead-in phrase fires on exactly the case the paragraph was tightened to suppress, reintroducing the false positives the round-2 Pattern 2 thread was about.

Suggested fix: Drop the flip-counting phrasing and commit to one concrete sequence — e.g. "the check must go pass→fail→pass→fail: the failure must reappear after an intervening pass" — and use that same wording in Pattern 2, in the convergence carve-out at 227-228, and in the When NOT to flag bullet at 258.

runs whose patches touch overlapping files. A single
pass→fail→pass is ordinary regress-then-recover: `pass` is the
desired attractor, so that shape is convergence, not oscillation.
CI results are reported at check/job level, not per test. A check
that flips with no overlap in the agent's changed files is likely
test flakiness (above), not agent-caused flapping.
3. **Cycle count:** repeated review-fix cycles on the same PR without
convergence — the review keeps raising the same or alternating
findings (a fix for one issue reintroduces a previously resolved one).
As a starting heuristic, treat more than two cycles as a signal, but
this is a provisional default: the flapping budget should be
configurable per repo and per agent role. See
[`flapping-convergence.md`](https://github.com/fullsend-ai/fullsend/blob/main/docs/problems/flapping-convergence.md),
which lists the right default as an open question. A single rework
cycle that the review then approves is normal iteration.

### When flapping is detected

Include a proposal with these specifics:

- **target_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 run IDs, which files changed, and how the changes reversed
- **what_could_go_better:** identify what might be driving the loop (conflicting review criteria, ambiguous instructions, an unconverging approach)
- **proposed_change:** suggest a concrete intervention (clarify the conflicting instruction, add a convergence guard)
- **validation_criteria:** a measurable outcome tied to the 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.
- A CI check flipping with no overlap in the agent's changed files is flakiness, not flapping.
- A single pass→fail→pass on a named check that then stays green is recovery, not oscillation.
- If a recorded `head.sha` is no longer reachable from the PR head
(rebase / force-push), treat a reversal signal as unreliable rather
than as oscillation.
- Same-SHA workflow reruns are not additional steps.
- Only flag when the same change is applied and reversed repeatedly, or the review-fix loop clearly fails to converge.

## 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