feat(retro): add flapping detection to retro analysis skill - #834
feat(retro): add flapping detection to retro analysis skill#834Benkapner wants to merge 3 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
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). |
There was a problem hiding this comment.
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
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Automated review sweep: 5 findings on the flapping detection addition (1 critical, 4 medium).
|
|
||
| ### 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). |
There was a problem hiding this comment.
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.
|
|
||
| ### 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. |
There was a problem hiding this comment.
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.
|
|
||
| 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." |
There was a problem hiding this comment.
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). | ||
| 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: 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").
|
|
||
| 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 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. |
There was a problem hiding this comment.
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.
f7b5838 to
b1d7da8
Compare
|
Thanks for the detailed review. I rebased this onto latest
The literal |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass on the new Flapping detection section. 9 inline findings (5 high, 4 medium), all on skills/retro-analysis/SKILL.md.
The five high findings cluster on one root cause: the run→PR correlation model in the data-gathering prompt does not match how dispatches actually carry their payload. Code runs have no pull_request object at all, and where one exists its head.sha is the pre-run head — so the first file-changing run is dropped and every remaining run is attributed its predecessor's diff. That shifts the run N / N+1 / N+2 numbering both Pattern 1 and Pattern 2 are built on. Separately, the collector never requests patch content, so Pattern 1's line-level A→B→A test has no data to run against, and Pattern 2 lacks the repeat bar Pattern 1 was given, so it fires on ordinary regress-and-fix.
The medium findings are narrower: linked-PR resolution assumes a single PR where the recipe returns a list, the linkage pointer names a skill that does not hold the recipe, run discovery is unbounded against --limit 10 recipes, and "order runs by commit" is undefined across the rebases the fix agent performs.
Not a blocking verdict — flagging for the author and code owners to triage.
| - **Flapping data collector:** "Find all code, fix, and review workflow | ||
| runs related to PR #`PR_NUMBER` in `$DISPATCH_REPO`. Each run's log | ||
| carries an `event_payload` JSON line with `pull_request.head.sha` and | ||
| `pull_request.number`; parse it to correlate runs to PR commits and |
There was a problem hiding this comment.
HIGH: Code runs carry no pull_request in event_payload, so PR-number matching silently drops the first file-changing run
The collector prompt tells the subagent to parse pull_request.head.sha / pull_request.number and "confirm the run belongs to this PR". Code runs never carry a pull_request object.
Verified from primary sources in fullsend-ai/fullsend: ADR 0033 states the per-org dispatch.yml "builds a minimal payload from $GITHUB_EVENT_PATH (extracting only issue, pull_request, and comment fields)", and docs/contributing/ci-workflows.md gives the canonical reusable-code.yml concurrency key as fullsend-code-agent-${{ inputs.source_repo }}-${{ fromJSON(inputs.event_payload).issue.number || fromJSON(inputs.event_payload).pull_request.number }} — the platform's own code-agent key falls back to issue.number precisely because pull_request is absent for code dispatches. This repo's skills/finding-agent-runs/SKILL.md independently confirms the trigger: "Code dispatches from issue events when ready-to-code is applied."
The code run is the first file-changing run (it creates the branch and the PR), so under the rule as written it fails confirmation and is discarded, destroying the baseline for Pattern 1's run N / N+1 / N+2 numbering. Conversely, a comment-triggered fix run puts the PR number in issue.number, so issue.number is not always an issue number.
Suggestion: spell out a three-way match rule mirroring the platform's own concurrency key — a run belongs to this PR if pull_request.number == PR_NUMBER, OR issue.number == PR_NUMBER (comment-triggered fix), OR issue.number equals the issue extracted from the agent/{issue}-{slug} branch (code runs). That branch convention is already documented at line 30 of this same file and in skills/finding-agent-runs/SKILL.md.
| runs related to PR #`PR_NUMBER` in `$DISPATCH_REPO`. Each run's log | ||
| carries an `event_payload` JSON line with `pull_request.head.sha` and | ||
| `pull_request.number`; parse it to correlate runs to PR commits and | ||
| confirm the run belongs to this PR. For each matched run, fetch the |
There was a problem hiding this comment.
HIGH: pull_request.head.sha is the pre-run head, so "the commit's changed files" attributes each run the previous run's diff
Lines 159-160 instruct: "For each matched run, fetch the commit's changed files ... from PR_REPO." But the payload is built at dispatch time from the triggering event — ADR 0033 (fullsend-ai/fullsend): the dispatch workflow "builds a minimal payload from $GITHUB_EVENT_PATH" — so head.sha is the branch head before the run does any work.
Every code/fix run is therefore attributed the diff its predecessor produced, shifting the entire sequence by one cycle. That directly corrupts the A→B→A reversal analysis in Pattern 1 (lines 170-174) that this whole section exists to support: the reversal will appear to occur one run earlier than it did, and the last run's actual output is never examined at all.
Useful corollary from the same dispatch model: a review run is triggered by a pull_request event on the pushed commit, so a review run's head.sha is the output commit of the preceding code/fix run — making review runs a reliable per-cycle anchor.
Suggestion: define run N's changes as the diff between head.sha(run N) and head.sha(next run) — equivalently, the head SHA of the review run that follows it — rather than the changed files of head.sha(run N). State this explicitly in the collector prompt, since the naive reading ("the run's commit") is wrong for every code/fix run.
| `PR_NUMBER` and collect what pattern detection needs: | ||
|
|
||
| - **Flapping data collector:** "Find all code, fix, and review workflow | ||
| runs related to PR #`PR_NUMBER` in `$DISPATCH_REPO`. Each run's log |
There was a problem hiding this comment.
HIGH: Run/PR correlation ignores the source repo, but the dispatch repo is org-wide
$DISPATCH_REPO is ${ORG}/.fullsend (line 19) — a single repo serving every repository in the org. Issue/PR numbers are not unique across repos in an org, so matching on pull_request.number == PR_NUMBER alone can pull a numerically-coincident PR's runs from a completely different repo into the flapping analysis, producing fabricated oscillation.
Disambiguation is available and is already the platform's own practice: source_repo is a first-class dispatch input (internal/harnessdispatch/ref.go: SourceRepo string \json:"source_repo"`; ADR 0034 lists it among the workflow_callinputs), andreusable-code.yml's concurrency key keys on inputs.source_repo` alongside the number for exactly this reason.
Compounding this, DISPATCH_REPO is derived at lines 18-19 from $REPO_FULL_NAME's org, yet lines 144-145 explicitly permit PR_REPO to differ from $REPO_FULL_NAME — when the resolved PR lives under a different org, this line searches the wrong dispatch repo entirely and returns nothing, silently.
Suggestion: require both halves — (a) a run matches only if source_repo (or pull_request.base.repo.full_name) equals PR_REPO AND the number matches; and (b) re-derive DISPATCH_REPO from PR_REPO's org once PR_REPO is resolved, stated alongside the existing "use PR_NUMBER and PR_REPO (not $REPO_FULL_NAME)" instruction at line 147.
| `pull_request.number`; parse it to correlate runs to PR commits and | ||
| confirm the run belongs to this PR. For each matched run, fetch the | ||
| commit's changed files and the named CI check results from `PR_REPO`. | ||
| Also fetch the PR's review comments/findings across all pages so |
There was a problem hiding this comment.
HIGH: Data collector cannot supply the line-level diffs Pattern 1 requires
Pattern 1 (lines 170-174) requires detecting that "the same lines in a file are changed, reverted, then changed again" — a repeated A→B→A on specific lines. That needs per-run patches/hunks.
The collector prompt (lines 155-162) only asks for "the commit's changed files" (a file-path list), "the named CI check results", and review comments. It never requests patch content, so an agent following the recipe literally can observe only that the same file path was touched three times. The skill's own "When NOT to flag" list states "Different files changing across runs is normal iteration", and the section repeatedly warns against path-level inference — so the agent will either skip Pattern 1 entirely (no data) or over-flag any file touched three times. Pattern 1 is the flagship pattern of the section and is not executable as specified.
Suggestion: update the collector instructions to fetch per-commit/per-range patches for file-changing runs (the commit or compare diff), and instruct the agent to compare hunks/line ranges across runs rather than file-path lists. This pairs with the head.sha finding above: the diff to fetch is between consecutive runs' head SHAs.
| 2. **Check-status flipping:** the same named CI check (e.g. `unit-tests`) | ||
| flips pass→fail→pass across three or more runs whose commits touch | ||
| overlapping changed files. CI results are reported at check/job level, | ||
| not per test, so compare at the named-check level. A check that flips |
There was a problem hiding this comment.
HIGH: Pattern 2 lacks the repeat bar Pattern 1 was given, so it fires on ordinary regress-and-fix
Pattern 2 (lines 175-180) flags whenever the same named CI check goes pass→fail→pass across three or more runs with overlapping changed files. That is exactly the shape of an ordinary single-mistake recovery: code lands green, one fix regresses a test, the next fix restores it. Since "overlapping changed files" is true for nearly every fix sequence on the same PR, this will fire routinely.
The asymmetry is textual and self-contradicting: Pattern 1 was deliberately tightened in this same PR to require a repeated A→B→A and explicitly excludes "a single add-then-remove" as "the normal review-fix cycle", and "When NOT to flag" says "A single rework cycle ... is normal" and "Only flag when the same change is applied and reversed repeatedly". Pattern 2 carries no equivalent safeguard.
The asymmetry is also substantive, not just cosmetic: for line diffs, A→B→A signals a contested change with no stable answer, whereas for CI checks pass is the desired attractor, so pass→fail→pass is convergence, not oscillation. As written, Pattern 2 will file "Flapping detected:" issues against healthy PRs — the exact noise the mandatory "Before proposing: check for existing issues" section exists to prevent.
Suggestion: tighten Pattern 2 to require a second flip (pass→fail→pass→fail, or two full oscillations) before flagging, or require the failure to reappear after an intervening pass on the same check with overlapping agent diffs — mirroring the stricter bar Pattern 1 uses. A single regress-then-recover should be listed under "When NOT to flag".
| number and `PR_REPO` is its `owner/repo` (`$REPO_FULL_NAME`). | ||
| - If the retro originates from an issue, resolve the linked PR using your | ||
| forge-specific skill's issue-to-PR linkage recipe. If no linked PR | ||
| exists, skip flapping detection for this retro. Otherwise set |
There was a problem hiding this comment.
MEDIUM: "Resolve the linked PR" assumes exactly one linked PR; the only available recipe returns a list of up to 50
The Applicability branch reads as a two-way choice — exactly one linked PR, or none ("If no linked PR exists, skip flapping detection ... Otherwise set PR_NUMBER and PR_REPO from the linked PR").
The actual recipe contradicts that shape: skills/github-forge/SKILL.md returns closedByPullRequestsReferences(first: 50) { nodes { number url author { login } state } } — a list of up to 50, each carrying state. This PR's own originating issue (fullsend-ai/fullsend#5512) resolves to multiple linked PRs across two repos, including the superseded attempt #540. With no tie-break, the retro agent may analyse an abandoned attempt's run history and report flapping for a PR nobody is working on. Note the same query returns no repository field, so PR_REPO can only be derived by parsing each node's url.
Suggestion: state the tie-break explicitly, using the state field the query already returns — prefer the single open linked PR; if none is open, 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. Also say to derive PR_REPO by parsing the node's url.
| - If the retro originates from a PR, use it directly: `PR_NUMBER` is its | ||
| number and `PR_REPO` is its `owner/repo` (`$REPO_FULL_NAME`). | ||
| - If the retro originates from an issue, resolve the linked PR using your | ||
| forge-specific skill's issue-to-PR linkage recipe. If no linked PR |
There was a problem hiding this comment.
MEDIUM: The issue-to-PR linkage reference points at a skill that does not contain the recipe
Lines 141-142 say to "resolve the linked PR using your forge-specific skill's issue-to-PR linkage recipe." Everywhere else in this file "your forge-specific skill" means the forge variant of this skill — see line 13 ("Use the forge-specific retro-analysis skill for CLI recipes"), and lines 43 and 213.
Verified at head: neither skills/retro-analysis/github/SKILL.md nor skills/retro-analysis/gitlab/SKILL.md contains any linkage recipe (the GitHub file covers workflow tracing, log reading, agents-repo discovery, and duplicate search only). The recipe actually lives in the separate shared forge skills — skills/github-forge/SKILL.md has the closedByPullRequestsReferences GraphQL query, and gitlab-forge is the GitLab counterpart. Both are loaded for the retro agent (harness/retro.yaml lists skills/github-forge and skills/gitlab-forge under forge.*.skills), so the capability is present — the pointer just sends the agent to a file that does not have it, and the agent will find nothing at the location the file's own convention names.
Suggestion: name the skill explicitly — "use the github-forge / gitlab-forge skill's linked-PR/MR recipe (closedByPullRequestsReferences / closed_by)" — rather than the ambiguous "your forge-specific skill", which this file uses to mean retro-analysis/{github,gitlab}.
|
|
||
| ### Data gathering | ||
|
|
||
| Dispatch a subagent to identify the code/fix/review workflow runs for |
There was a problem hiding this comment.
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.
|
|
||
| ### Patterns to detect | ||
|
|
||
| Order runs by commit and count only **file-changing** (code/fix) runs as |
There was a problem hiding this comment.
MEDIUM: "Order runs by commit" is undefined across force-pushes and does not de-duplicate same-SHA reruns
This line says "Order runs by commit and count only file-changing (code/fix) runs as steps". Commit order is only a partial order in practice.
The fix agent rebases and amends, so a head.sha recorded in an earlier run's payload is frequently no longer reachable from the PR head — the rewritten commits are new objects with no ancestry relation to the recorded SHAs, leaving "order by commit" undefined for exactly the long-lived PRs this section targets. Worse, a rebase makes a file look "reverted then re-applied" when nothing oscillated — a direct false positive for Pattern 1.
Separately, workflow reruns on the same SHA produce duplicate runs with no file change, shifting the N / N+1 / N+2 step indexing that both Pattern 1 and Pattern 2 depend on.
Suggestion: order runs by workflow run creation time rather than by commit; collapse same-SHA reruns into a single step; and add force-push/rebase to "When NOT to flag" — if a recorded head.sha is no longer reachable from the PR head, treat the reversal signal as unreliable rather than as oscillation.
Teach the retro agent to detect fix-break oscillation during post-workflow analysis, kept distinct from test flakiness: file oscillation (repeated A->B->A reversals across file-changing runs), check-status flipping (a named CI check flipping across runs whose commits touch overlapping files), and non-converging review-fix cycle count. Resolves the target PR and its repo explicitly (PR_NUMBER, PR_REPO), handling the cross-repo issue-to-PR case, stays forge-agnostic (linkage via the forge-specific skill), and treats the cycle threshold as a provisional heuristic per flapping-convergence.md rather than a fixed number. Supersedes fullsend-ai#540. Closes fullsend-ai/fullsend#5512 Signed-off-by: Benjamin Kapner <bkapner@redhat.com>
Match runs with the platform concurrency key (issue or pull_request number, gated on source_repo), take diffs between consecutive head SHAs, fetch patches not path lists, and require a second CI flip before calling oscillation. Signed-off-by: Benjamin Kapner <bkapner@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
b1d7da8 to
fc5482d
Compare
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Round-3 review-only pass on the Flapping detection section, after the fixes for the round-2 threads. 7 inline findings (3 high, 4 medium), all on skills/retro-analysis/SKILL.md.
The three high findings are residuals of the round-2 fixes rather than restatements of them. Two of them cancel each other out inside the same section: the new createdAt..updatedAt search window excludes the code run (a code run predates the PR it creates), while the newly added issue.number match branch and the Diffs paragraph exist specifically to keep that run in — and the compare-patch recipe still has no start anchor for it, which is the A Pattern 1's A→B→A needs. The third is scope: the procedure is written entirely against GitHub payload fields and gh flags but sits in the shared skill that GitLab retros also load, against this file's own line 13 convention.
The mediums are executability gaps in the new text: the recipe pointer names a skill that does not list fix.yml, the tie-break selects on a field the cited query does not return, Pattern 2 needs per-SHA check results that no recipe provides, and Pattern 2's lead-in phrasing read literally matches the shape the following sentence excludes.
Review-only — no approval or change request implied.
|
|
||
| Dispatch a subagent to identify the code/fix/review workflow runs for | ||
| this PR. Reuse the `finding-agent-runs` discovery recipe rather than | ||
| inventing a new listing; bound the search to the PR's |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
|
|
||
| ### Data gathering | ||
|
|
||
| Dispatch a subagent to identify the code/fix/review workflow runs for |
There was a problem hiding this comment.
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.
| ### Data gathering | ||
|
|
||
| Dispatch a subagent to identify the code/fix/review workflow runs for | ||
| this PR. Reuse the `finding-agent-runs` discovery recipe rather than |
There was a problem hiding this comment.
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.
| 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 | ||
| run)` and the named CI check results from `PR_REPO`. Also fetch the |
There was a problem hiding this comment.
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.
| (`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, |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
Search code.yml at or before PR.createdAt, anchor the code-run patch at the PR base, and keep gh recipes in the GitHub skill so GitLab retros skip flapping. Signed-off-by: Benjamin Kapner <bkapner@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
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.
Supersedes #540, which accumulated 7 review rounds and diverged from the file's conventions. This version incorporates the key insight from that review process (the
event_payloadJSON in dispatch-repo logs exposespull_request.head.shaandpull_request.numberdirectly) and keeps the section at the same altitude as the rest of the file.What it adds
Guidance for detecting three flapping patterns:
Design decisions
event_payloadfrom dispatch-repo run logs for run-to-commit correlation (no timestamp approximation)Related
Checklist