feat(retro): add flapping detection to retro analysis skill (#5512) - #5513
feat(retro): add flapping detection to retro analysis skill (#5512)#5513Benkapner wants to merge 2 commits into
Conversation
…-ai#5512) Add a "Flapping detection" section to the retro-analysis skill that teaches the retro agent how to identify fix-break oscillation patterns: file-level oscillation (same files changed and reverted across runs), test result flipping, and excessive review-fix cycles. Includes concrete signals to check, bash commands for diff comparison, guidance on when to flag vs when NOT to flag (single rework is normal), and how to structure proposals when flapping is detected. Signed-off-by: Benjamin Kapner <bkapner@redhat.com>
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
1 similar comment
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
PR Summary by QodoAdd flapping detection guidance to retro-analysis skill
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Site previewPreview: https://4aebfafa-site.fullsend-ai.workers.dev Commit: |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| ```bash | ||
| # Compare diffs between consecutive code/fix runs | ||
| gh api "repos/$REPO_FULL_NAME/pulls/$PR_NUMBER/files" \ | ||
| --jq '.[].filename' | sort > /tmp/current_files.txt | ||
|
|
||
| # Get previous run's changed files from the commit before | ||
| git diff --name-only HEAD~1 HEAD | sort > /tmp/prev_files.txt | ||
|
|
||
| # Files changed in both runs (potential oscillation) | ||
| comm -12 /tmp/prev_files.txt /tmp/current_files.txt | ||
| ``` |
There was a problem hiding this comment.
1. Run diff comparison wrong 🐞 Bug ≡ Correctness
The flapping-detection snippet compares the full PR file list against git diff HEAD~1 HEAD, which is not a comparison between consecutive code/fix runs and only yields filename overlap (not reversed changes). This can cause the retro agent to miss real oscillations or generate false flapping candidates that aren’t tied to specific run-to-run transitions.
Agent Prompt
### Issue description
The current flapping-detection bash example doesn’t compare *consecutive code/fix runs* (it compares PR-wide files to the last commit), and it doesn’t provide a way to verify that changes were actually reversed.
### Issue Context
The section describes detecting oscillation between *runs* (run N vs run N+1), but the snippet uses `HEAD~1..HEAD` and PR-wide file listing, which is not run-scoped.
### Fix Focus Areas
- internal/scaffold/fullsend-repo/skills/retro-analysis/SKILL.md[115-125]
### Suggested direction
- Change the snippet to:
- Use run-specific commit SHAs (e.g., from each run’s produced commit / PR head at the time) and compare consecutive SHAs via `gh api repos/$REPO_FULL_NAME/compare/<shaN>...<shaNplus1>` (or `git diff <shaN>..<shaNplus1>` if available locally).
- Add an explicit step that checks *reversal* for candidate files (e.g., show `git diff <shaN>..<shaNplus1> -- <file>` and `git diff <shaNplus1>..<shaNplus2> -- <file>` to demonstrate add/remove reversal).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
fixed. now compares consecutive commit SHAs instead of using HEAD~1.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Address review feedback: - Compare consecutive commit SHAs instead of HEAD~1 - Add --paginate to gh api calls for PR files Signed-off-by: Benjamin Kapner <bkapner@redhat.com>
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
CRITICAL — PR targets a file/directory tree already deleted from fullsend-ai/fullsend main; real, verified merge conflict
File: internal/scaffold/fullsend-repo/skills/retro-analysis/SKILL.md
This PR's only change is to internal/scaffold/fullsend-repo/skills/retro-analysis/SKILL.md, but that file no longer exists on fullsend-ai/fullsend's main branch. Verified independently via three methods:
git ls-tree origin/main -- internal/scaffold/fullsend-repo/skills/retro-analysis/SKILL.mdreturns nothing, andgit cat-file -e origin/main:<path>fails.- Commit 8b8c3bb ("refactor(scaffold)!: delete agent files from scaffold embed (#5552)", merged to main via PR #5588) deleted the entire
internal/scaffold/fullsend-repo/{skills,agents,harness,schemas,env,plugins,policies}tree, with the commit message stating "Agent resolution now uses fullsend-ai/agents repo exclusively... No migration needed." gh pr view 5513currently reportsmergeable: CONFLICTING,mergeStateStatus: DIRTY, and a localgit merge-tree $(git merge-base origin/main pr-5513) origin/main pr-5513confirms a genuine modify/delete conflict on this exact path ("removed in local" vs "their" still modifying it) — this is the PR's actual current state, not a hypothetical.
The canonical copy of this skill today is fullsend-ai/agents:skills/retro-analysis/SKILL.md (confirmed via the GitHub contents API), which has no flapping-detection content yet, so the underlying feature is still wanted — it is simply aimed at a file fullsend-ai/fullsend's main has deleted. Confirming the root cause, the linked issue #5512 itself still cites the stale paths internal/scaffold/fullsend-repo/skills/retro-analysis/ and internal/scaffold/fullsend-repo/schemas/retro-result.schema.json (also deleted by the same commit), which is presumably why the author targeted the wrong repo.
Suggestion: Close or redirect this PR. Re-implement the "Flapping detection" section (adjusted per the inline findings below) as a PR against fullsend-ai/agents at skills/retro-analysis/SKILL.md instead — note that repo's copy has already diverged from the base this PR assumes (e.g. it has since gained a "Discovering the agents repo" section and a different run-tracing model based on gh run list --workflow=code.yml|fix.yml in $DISPATCH_REPO, not PR commits), so the content needs manual rework rather than a straight port. Do not attempt to resolve the current conflict by keeping the deleted path in fullsend-ai/fullsend. Also consider updating issue #5512's description to point at the fullsend-ai/agents paths.
The inline comments below cover additional findings on the section's content itself, in case it's ported to the correct repo.
|
|
||
| ```bash | ||
| # Get the last two code/fix run SHAs for this PR | ||
| COMMITS=$(gh api "repos/$REPO_FULL_NAME/pulls/$PR_NUMBER/commits" \ |
There was a problem hiding this comment.
HIGH — New bash example uses $PR_NUMBER, which is never exported to the retro agent's sandbox
The new "Flapping detection" bash block references $PR_NUMBER in two gh api calls (line 117: pulls/$PR_NUMBER/commits; line 126: pulls/$PR_NUMBER/files). Confirmed directly against fullsend-ai/agents that the retro agent's environment never sets this variable: env/retro.env exports only ORIGINATING_URL, RETRO_COMMENT, REPO_FULL_NAME, and GH_TOKEN; harness/retro.yaml's runner_env block confirms the same three (plus GH_TOKEN) with no PR_NUMBER. By contrast, env/review.env does export PR_NUMBER, because the review agent is always dispatched against a specific PR — the retro agent is dispatched from an ORIGINATING_URL that this same skill's pre-existing "From an issue"/"From a PR" sections treat as possibly-an-issue-or-possibly-a-PR, and nothing (old or new) in the skill derives a PR number from it. The file's own established convention (the pre-existing "Setup" section explicitly derives ORG/DISPATCH_REPO from the given $REPO_FULL_NAME before using them) is not followed here — $PR_NUMBER is used as if it were an established env var with no derivation step, and the rest of the file only ever refers to PR numbers via the narrative placeholder #N in subagent prompts, never as a shell variable. As written, $PR_NUMBER expands to empty, producing malformed paths like repos/owner/repo/pulls//commits that will 404 if an agent runs this snippet literally.
Suggestion: Add an explicit step deriving the PR number from $ORIGINATING_URL (mirroring how the Setup section derives ORG from REPO_FULL_NAME), e.g. gh pr view "$ORIGINATING_URL" --json number -q .number, before using it — or replace the bash variable with the same narrative-placeholder convention (#N) used elsewhere in the file, instructing the agent to substitute the number it already has from context.
| 3. **Cycle count:** more than 2 review-fix cycles on the same PR without convergence (the review keeps requesting changes on the same findings). | ||
|
|
||
| ```bash | ||
| # Get the last two code/fix run SHAs for this PR |
There was a problem hiding this comment.
MEDIUM — Run-boundary logic assumes one PR commit equals one code/fix agent run, an unconfirmed and internally inconsistent assumption
Lines 116-120 take tail -2/tail -1 of pulls/$PR_NUMBER/commits as "the last two code/fix run SHAs," silently equating one PR commit with one agent run. This is only partially supported: docs/agents/fix.md states "Post-script pushes the commit" (singular), consistent with one commit per fix run, but docs/agents/code.md only says the code agent "commits locally" and the post-script "pushes the branch" — it never confirms exactly one commit reaches the pushed branch. More importantly, this same skill file's own pre-existing "Tracing the workflow graph" section (unchanged by this PR) defines a "run" as a distinct GitHub Actions execution discovered via gh run list --repo $DISPATCH_REPO --workflow=code.yml|fix.yml, not a commit-list position — a discontinuity with the new snippet's methodology that is never reconciled or flagged. If any run ever produces more than one commit (amend, fixup, incremental push), tail -2 would silently compare two commits from the same run with no error surfaced.
Suggestion: Either derive PREVIOUS_SHA/CURRENT_SHA from actual workflow-run boundaries (mapping each gh run list --workflow=code.yml|fix.yml run to its resulting commit SHA, consistent with the file's own established run-tracing approach), or explicitly caveat that this heuristic assumes exactly one commit per run and may misfire otherwise.
|
|
||
| Include a proposal with these specifics: | ||
|
|
||
| - **title:** Start with "Flapping detected:" followed by what oscillated |
There was a problem hiding this comment.
MEDIUM — Flapping proposal checklist omits target_repo, which the schema requires
The "When flapping is detected" checklist (lines 137-141) lists exactly five fields: title, what_happened, what_could_go_better, proposed_change, validation_criteria. Confirmed directly against fullsend-ai/agents:schemas/retro-result.schema.json that a proposal object's required array is ["target_repo", "title", "what_happened", "what_could_go_better", "proposed_change", "validation_criteria"] with additionalProperties: false — target_repo is mandatory, not merely allowed. This new, self-contained checklist reads as a complete spec for a flapping proposal and appears before the general "Output format" section (which does list all six fields) later in the same file. An agent that treats this checklist as authoritative risks emitting a proposal missing target_repo, which would fail fullsend-check-output schema validation (mitigated somewhat by the harness's 2-iteration validation retry, but still a real inconsistency in the guidance).
Suggestion: Add target_repo to the bullet list, e.g. "- target_repo: the repo where the fix should land — see Localization guidance above," so the checklist is self-consistent with the schema described later in the file.
| - **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:** "The next code/fix cycle on similar PRs should converge within 2 iterations" |
There was a problem hiding this comment.
MEDIUM — validation_criteria guidance breaks this file's own established convention for that field
Line 141 gives validation_criteria as a bare quoted sentence — "The next code/fix cycle on similar PRs should converge within 2 iterations" — with no "for example" framing, no instance-specific detail, and no timeframe/sample-size guidance. This file's own pre-existing, unchanged "Writing good proposals" section explicitly establishes a different convention for this exact field: "validation_criteria: Define measurable or observable outcomes. Include a timeframe or sample size. For example: 'The next 5 code agent runs on this repo should not trigger the same review comment about missing error handling.'" — i.e., instruction plus a labeled example, with explicit timeframe/sample-size guidance. The new bullet is also inconsistent with its own sibling bullet one line above (what_happened), which asks for instance-specific detail ("run IDs, which files changed") while validation_criteria stays generic and copy-pasteable.
Suggestion: Rewrite to match the file's established convention, e.g.: "validation_criteria: Define a measurable, checkable outcome tied to the specific files/pattern involved, with a timeframe or sample size. For example: 'The next 2 fix cycles touching <file> should not re-introduce the change reverted in run N+1.'"
|
|
||
| 1. **File oscillation:** the same file was changed in two or more consecutive runs, and the changes reverse each other (lines added in run N were removed in run N+1, or vice versa). | ||
| 2. **Test result flipping:** a test that passed after run N fails after run N+1, then passes again after run N+2. | ||
| 3. **Cycle count:** more than 2 review-fix cycles on the same PR without convergence (the review keeps requesting changes on the same findings). |
There was a problem hiding this comment.
MEDIUM — Hardcoded 2-cycle threshold contradicts the linked design doc's explicit open question on configurability
Line 113 sets "more than 2 review-fix cycles" as the cycle-count signal, and line 141 hardcodes "converge within 2 iterations" as the validation bar. docs/problems/flapping-convergence.md — the doc this PR's description cites as motivation — treats this exact parameter as explicitly unresolved: its "Open questions" section asks "What is the right default flapping budget (max iterations before circuit-breaking)? Should it be count-based, cost-based, or time-based?", and its "Thresholds and configuration" section states thresholds should vary by repo/task type ("A documentation repo might tolerate only 2 review cycles... A complex backend service might allow 5 cycles...") and be "configurable per repo and per agent role," not a single constant. This PR ships "2" as settled fact in agent-facing guidance without that decision having been made in the design doc it references. (Issue #5512 does list "> 2 fix cycles" as one bullet, so the number isn't fabricated, but the issue doesn't reconcile it with the doc's open-question status either.)
Suggestion: Phrase the threshold as an explicit, overridable default rather than a bare fact — e.g., "more than N review-fix cycles (default N=2; see flapping-convergence.md open questions on configurability)" — so a future config-driven threshold change doesn't silently contradict shipped agent guidance.
|
Closing this PR. The scaffold extraction (PR #5552/#5588) moved internal/scaffold/fullsend-repo/skills/retro-analysis/SKILL.md to fullsend-ai/agents. Re-opened as fullsend-ai/agents#540, adapted to the current version of the file and incorporating review feedback from this thread. @waynesun09 |
|
🤖 Finished Retro · ✅ Success · Started 5:39 AM UTC · Completed 5:49 AM UTC |
Retro: PR #5513 — feat(retro): add flapping detection to retro analysis skillOutcome: Closed without merge. The PR modified Timeline
Agent involvement
Review quality assessment
Evidence for existing issues (no new proposals needed)
ConclusionThe workflow functioned as designed — security gates correctly blocked agent dispatch for an external contributor, and human review eventually caught the fundamental issue. The 6-day latency was the main cost. Implementing #3488 (CI-level deprecated-path check) would have reduced this to near-zero for this class of problem. No new proposals are warranted. |
2 participants
Summary
Adds flapping detection guidance to the
retro-analysisskill. The retro agent now knows how to identify fix-break oscillation patterns during its post-workflow analysis.What it adds to the skill
Why this matters
Flapping wastes agent cycles and often indicates a deeper problem (conflicting instructions, flaky tests, or an approach the agent cannot converge on). Today, the retro agent has no specific guidance to detect this pattern, even though the flapping-convergence problem doc identifies it as a key concern.
Related Issue
Closes #5512
Testing
Checklist