Skip to content

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

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 8 commits into from
+44 −0
Closed

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

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
de40314
feat(retro): add flapping detection to retro analysis skill (#5512)
Benkapner Jul 30, 2026
ca31c01
fix(retro): address review findings on flapping detection section
Benkapner Jul 30, 2026
6495895
fix(retro): address review findings on flapping detection
Benkapner Jul 30, 2026
30c8f64
fix(retro): wire issue-URL PR lookup and add --repo to dispatch prompt
Benkapner Aug 2, 2026
b11fab1
fix(retro): address round-3 flapping detection review findings
Benkapner Aug 4, 2026
f581d10
fix(retro): address round-4 flapping detection review findings
Benkapner Aug 9, 2026
ca4b912
fix(retro): address round-5 flapping detection review findings
Benkapner Aug 12, 2026
bd3fb8d
fix(retro): address round-6 flapping detection review findings
Benkapner Aug 17, 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
44 changes: 44 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,50 @@ 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).

### Signals to check

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##*/}"` and set `REPO="$REPO_FULL_NAME"`.
- If it matches `/issues/`, check for a linked PR 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`. If multiple PRs are linked, prefer the one in `$REPO_FULL_NAME`; otherwise use the most recently updated entry and note the ambiguity in the retro summary. Set `REPO` to the matching entry's `repository.owner.login/repository.name` and `PR_NUMBER` to its `number`. If no linked PR is found, 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.

HIGH — "Most recently updated entry" tie-break (added this round) can't be implemented with the specified command

The issue-URL branch now says: "If multiple PRs are linked, prefer the one in $REPO_FULL_NAME; otherwise use the most recently updated entry and note the ambiguity." I verified live: gh issue view <url> --json closedByPullRequestsReferences (e.g. against this PR's own linked issue) returns only id, number, repository{id,name,owner}, and url per entry — no timestamp field at all. An agent following this instruction literally has no data to determine which entry is "most recently updated." This appears to be a new gap introduced by this round's fix to a previously-flagged ambiguity (the round-6 fix added the tie-break rule but the rule references data the specified command doesn't return).

Suggestion: Either specify a command that actually returns the needed timestamp (e.g., a gh api graphql query requesting updatedAt on closedByPullRequestsReferences, or a follow-up gh pr view per candidate), or drop the recency tie-break for a simpler, verifiable rule (e.g., highest PR number, or note all candidates without picking one).


Derive `ISSUE_REF` from the PR branch name using the `agent/{issue}-{slug}` convention documented in "From a PR" above (e.g. branch `agent/5512-flapping` yields `ISSUE_REF="5512"`).

@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 — ISSUE_REF derivation has no fallback and is used as an unconstrained grep substring

ISSUE_REF is derived solely from the PR branch name via the agent/{issue}-{slug} convention (this line), with no fallback for branches that don't follow it (human-created branches, force-pushed/renamed branches, etc.) — the skill never suggests falling back to headRefName metadata or the Closes #<N> text in the PR body. The derived value is then used directly as grep -i '<ISSUE_REF>' against full run logs (line 141): for small issue numbers this is a raw substring match that can hit unrelated SHAs, timestamps, or other issue/PR numbers embedded in the log, since case-insensitivity does nothing to disambiguate digit sequences.

Suggestion: Constrain ISSUE_REF to ^[0-9]+$ and fail closed (skip flapping detection) if it can't be derived; anchor the grep pattern (e.g., word boundaries or matching within the parsed event_payload JSON field) instead of a bare substring match.


Dispatch subagents to gather the data. Substitute `<DISPATCH_REPO>` (from Setup), `<REPO>`, `<PR_NUMBER>`, and `<ISSUE_REF>` with the concrete values derived above before dispatching.

Dispatch Run discovery and Review history in parallel. CI results depends on Run discovery's output (the correlated commit SHAs), so dispatch it after Run discovery returns.

- **Run discovery:** "List all code, fix, and review workflow runs via `gh run list --workflow=code.yml --repo <DISPATCH_REPO> --limit 100`, `gh run list --workflow=fix.yml --repo <DISPATCH_REPO> --limit 100`, and `gh run list --workflow=review.yml --repo <DISPATCH_REPO> --limit 100`. Filter to runs belonging to PR #<PR_NUMBER> by grepping each run's logs (`gh run view <RUN_ID> --repo <DISPATCH_REPO> --log | grep -i '<ISSUE_REF>'`). For each matching code/fix 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 a short window, mark the correlation as uncertain. Then fetch that commit's changed files via `gh api repos/<REPO>/commits/<SHA>` (`.files`). Use workflow-run boundaries to define 'runs', not individual commits; a single run may produce more than one commit."

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

HIGH — "No direct run-to-SHA mapping" claim is false — the exact SHA is already in the log the subagent greps

The Run discovery subagent prompt says to correlate a run to a PR commit "by matching the run's timestamp against the PR's commit history (no direct run-to-SHA mapping is exposed)" and to mark two-candidates-in-a-short-window correlations as uncertain. I verified live against a real fullsend-ai/.fullsend review.yml run (gh run view <id> --repo fullsend-ai/.fullsend --log) that the workflow_dispatch event_payload input is printed verbatim in the log: event_payload: {"issue":null,"pull_request":{"number":6223,...,"head":{"sha":"9a8192a14be44da6e825eb6c732f22c75efd1492",...}},...}. This is the exact same log content the subagent is already told to grep for <ISSUE_REF>. The harness even already parses .pull_request.head.sha out of this payload internally (visible later in the same log as COMMIT_SHA=$(... jq -r '.pull_request.head.sha')). So the direct mapping is exposed, in data the prompt already fetches — the prescribed timestamp-correlation-with-uncertainty fallback is unnecessary and strictly worse (it can misattribute commits when two runs land close together, which the real data resolves exactly).

Suggestion: Have the Run discovery subagent parse pull_request.head.sha (or .issue/PR number) directly from the matched run's event_payload log line instead of timestamp correlation, and drop the "mark as uncertain" fallback for this case.

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

HIGH — Run discovery scans up to 300 full run logs with no time bound, risking the retro's 30-minute budget and duplicating an existing cheaper approach

The Run discovery subagent lists up to 100 runs each for code.yml/fix.yml/review.yml (up to 300 total) in the shared org-wide $DISPATCH_REPO, then downloads and greps the full log of every one via gh run view <RUN_ID> --log, with no date/time filter. I confirmed harness/retro.yaml sets timeout_minutes: 30 for the entire retro run, of which flapping detection is only one dispatch among several — hundreds of full-log downloads against a shared org repo risks exhausting that budget or hitting GitHub secondary rate limits before synthesis even starts. This also duplicates a cheaper, already-existing mechanism: skills/finding-agent-runs/SKILL.md (also loaded by this same harness) correlates dispatch-repo runs to a specific issue/PR via timestamp-matching against the source-repo's own dispatch event, without needing to download/grep hundreds of logs.

Suggestion: Bound gh run list with a --created window derived from the PR's created/updated timestamps (both cheaply available via gh pr view) before falling back to log inspection, and reuse skills/finding-agent-runs' correlation approach (or extend the existing Workflow tracer/Comment analyzer subagents) instead of dispatching a new, redundant, unbounded scan.

@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 — Run-matching grep on bare issue number risks cross-repo false positives in the shared dispatch repo

$DISPATCH_REPO is a single org-wide repo serving every repo under the org. The Run discovery prompt filters candidate runs with gh run view <RUN_ID> --log | grep -i '<ISSUE_REF>' on the bare issue/PR number, without also requiring the log's source_repo: value to equal <REPO>. I verified live that source_repo: <owner>/<repo> is printed on an adjacent line in the very same log dump as the event_payload line already being grepped. Since issue/PR numbers are not unique across repos in the same org, a numerically-coincident issue/PR in a different repo under the same org can be misattributed as belonging to this PR's flapping analysis.

Suggestion: Require the log to contain both source_repo: <REPO> and the issue/PR reference (or better, parse the event_payload JSON blob directly for pull_request.number and match it against <REPO>/<PR_NUMBER>) before treating a run as a match.

@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 — Bulk workflow-log ingestion isn't covered by the existing redaction rule

Run discovery instructs subagents to pull full workflow logs (gh run view --log) for up to hundreds of runs into model context. agents/retro.md's only redaction rule (verified: line 129) is scoped to "issue bodies, PR descriptions, comment text" — it says nothing about workflow log content. CI/Actions logs commonly contain URLs, tokens, or other sensitive strings not covered by GitHub's automatic secret masking, so this bulk ingestion increases the chance such content gets summarized or copied into a generated proposal issue without the existing guidance ever having considered logs as a source needing the same care.

Suggestion: Bound log reads to the small correlated-run set (per the run-discovery-cost finding above), and extend the existing "summarize, do not paste verbatim" rule in agents/retro.md to explicitly cover workflow log content, not just issue/PR/comment text.

- **Review history:** "Fetch all reviews for PR #<PR_NUMBER> via `gh api repos/<REPO>/pulls/<PR_NUMBER>/reviews --paginate`, then fetch per-review comments. Summarize the findings from each review cycle so that finding content can be compared across cycles."
- **CI results** (after Run discovery): "For each commit SHA from the Run discovery results, query `gh api repos/<REPO>/commits/<SHA>/check-runs` and report the test pass/fail results."

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.

@waynesun09 waynesun09 Aug 4, 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 — 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.

3. **Cycle count:** more than 2 review-fix cycles on the same PR without convergence (the review keeps requesting changes on the same or alternating findings, e.g. a fix for one issue reintroducing a previously resolved one counts as flapping too). 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.

@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 — Cycle-count pattern contradicts the file's own "When NOT to flag" guidance

Pattern 3 flags "more than 2 review-fix cycles on the same PR without convergence," counting cycles regardless of whether changes are reversed. But "When NOT to flag" (lines 164-166) states a single rework cycle is normal and instructs to "only flag when you see the same changes being applied and reversed repeatedly." Three clean forward-progress review rounds (e.g., tests requested, then error handling requested, then approved — no reversal at all) satisfies Pattern 3's raw cycle count while explicitly violating the "When NOT to flag" rule. The two sections give an agent contradictory instructions for the same scenario.

Suggestion: Pick one coherent rule: either cycle count alone (and drop/soften the "only when reversed" language in "When NOT to flag"), or require reversal/recurrence of the same finding for all three patterns, not just Patterns 1 and (implicitly) 3's parenthetical.


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

Comment on lines +124 to +167

@qodo-code-review qodo-code-review Bot Jul 30, 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 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

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