fix(#4783): add in-progress action to triage agent for fixing PRs - #5414
fix(#4783): add in-progress action to triage agent for fixing PRs#5414fullsend-ai-coder[bot] wants to merge 1 commit into
Conversation
The triage agent's "Existing PR gate" forced all open PRs that address an issue into the prerequisites action, applying the blocked label. This is semantically wrong when a PR directly fixes the issue — the issue is not blocked, it is being addressed. Add a new in-progress action that distinguishes PRs that fix an issue (addressed by) from PRs that are true prerequisites (blocked by). The post-script applies a pr-open label instead of blocked and uses "Addressed by:" framing in the comment. Changes: - triage.md: split Existing PR gate into two cases (fix vs prerequisite), add in-progress action documentation - triage-result.schema.json: add in-progress to action enum, add pull_requests property with URL validation - post-triage.sh: add in-progress handler, add pr-open to control labels - post-triage-test.sh: add 8 tests for in-progress action, update mock gh to capture comment body content Closes #4783
|
🤖 Finished Review · ✅ Success · Started 1:25 PM UTC · Completed 1:43 PM UTC |
Site previewPreview: https://bacb610c-site.fullsend-ai.workers.dev Commit: |
ReviewFindingsHigh
Medium
Low
Labels: PR modifies triage agent behavior (agent definition, schema, post-script) and fixes a bug in issue classification. |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| @@ -445,6 +451,39 @@ run_test "ready-to-code-applied-without-label-actions" \ | |||
| '{"action":"sufficient","reasoning":"all clear","clarity_scores":{"symptom":0.9,"cause":0.85,"reproduction":0.9,"impact":0.8,"overall":0.87},"triage_summary":{"title":"Fix crash","severity":"high","category":"bug","problem":"Crash","root_cause_hypothesis":"Buffer overflow","reproduction_steps":["step 1"],"environment":"Linux","impact":"All users","recommended_fix":"Fix buffer","proposed_test_case":"test_crash"},"comment":"## Triage Summary\n\nReady."}' \ | |||
| "gh api repos/test-org/test-repo/issues/42/labels -f labels[]=ready-to-code --silent" | |||
|
|
|||
There was a problem hiding this comment.
[low] test-adequacy
No test for the multiple-PR case (pull_requests array with 2+ entries). All 8 new tests use single-element arrays.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Independent multi-agent review pass (3 agents). Three new findings posted inline above (1 HIGH, 2 MEDIUM); several other issues these agents rediscovered were already covered by the existing automated review and are not repeated here.
One additional finding has no valid inline location since the file isn't part of this diff:
[MEDIUM] docs/architecture.md:316,321 — The "Label state machine guard" and "triage agent runtime" building-block descriptions still describe only blocked/duplicate label semantics and the mutual-exclusion guarantee, without mentioning the new pr-open label or in-progress action. This is the doc that documents the very invariant the pr-open-cleanup finding above shows is now incomplete in code. Suggest updating both the mutual-exclusion sentence and the triage-runtime description to include pr-open/in-progress alongside blocked.
A couple of items independently rediscovered by these agents were verified and found to be false positives after empirical testing (e.g., a claim that adding a not-yet-existing label would hard-fail the script — confirmed GitHub's labels API auto-creates missing labels rather than erroring, so that specific failure mode doesn't occur) and are omitted accordingly.
| remove_label "ready-to-code" | ||
| remove_label "needs-info" | ||
| remove_label "blocked" | ||
| add_label "pr-open" |
There was a problem hiding this comment.
[MEDIUM] premature-decision — pr-open label creation skips this repo's established branding convention
Finding: add_label "pr-open" goes straight to the generic labels-API helper without first calling gh label create, unlike every other place this codebase introduces a label outside GitHub's defaults — e.g. pre-code.sh creates this exact pr-open label with --description "An open PR already addresses this issue" --color "D4C5F9" --force before applying it, and post-fix.sh/post-retro.sh/post-review.sh do the same for their respective labels. I verified empirically that GitHub's "Add labels to an issue" endpoint auto-creates a missing label (default gray color, no description) rather than erroring, so this won't break the handler — but on any repo where pre-code.sh's human-PR-detection branch hasn't already created pr-open (triage/Stage 1 always runs before code/Stage 2), the first in-progress triage result will create pr-open with GitHub's default styling instead of the intended purple, described label. That's an unverified assumption about label state shipped without the fallback step the rest of the codebase consistently uses.
Suggestion: Add gh label create "pr-open" --repo "${REPO}" --description "An open PR already addresses this issue" --color "D4C5F9" --force 2>/dev/null || true immediately before add_label "pr-open", matching pre-code.sh's handling of the same label.
Flagged in different forms by 2 of 3 independent review passes; severity/mechanism corrected here after empirically confirming GitHub's label-add API auto-creates rather than rejects unknown labels.
| # pipeline itself applies (pre-triage.sh resets the first five; the action | ||
| # handlers apply blocked/triaged/feature). | ||
| CONTROL_LABELS=("needs-info" "ready-to-code" "duplicate" "feature" "blocked" "triaged" "question") | ||
| CONTROL_LABELS=("needs-info" "ready-to-code" "duplicate" "feature" "blocked" "triaged" "question" "pr-open") |
There was a problem hiding this comment.
[HIGH] pr-open control label is never cleared when an issue leaves the in-progress state
Finding: pr-open is added to CONTROL_LABELS here and applied by the new in-progress handler, but none of the other five action handlers (insufficient, duplicate, prerequisites, sufficient, question) ever call remove_label "pr-open" — each of them already clears blocked on this kind of transition, but pr-open has no equivalent. pre-triage.sh's label-reset loop (line 30) and its remaining-labels verification query weren't extended to include pr-open either. Once an issue is marked in-progress and later re-triaged to a different outcome (e.g. the linked PR closes unmerged, a case this PR's own updated gate anticipates), the stale pr-open label persists permanently and can't even be cleared through label_actions, since is_control_label() now refuses to touch it. This breaks the mutual-exclusion guarantee docs/architecture.md's label-state-machine section describes for blocked, which this PR doesn't extend to the new label.
Suggestion: Add remove_label "pr-open" alongside the existing remove_label "blocked" calls in the insufficient, duplicate, prerequisites, sufficient, and question branches, and add pr-open to pre-triage.sh's reset list and its verification --jq filter.
Flagged independently by all three review passes. This refines and escalates a previously-posted pre-triage.sh:30 note that rated the same underlying gap as low-severity consistency polish — the missing handlers plus the label_actions lockout make it a functional gap rather than a nice-to-have.
| - The issue's fix requires a design decision that is being discussed in another issue | ||
|
|
||
| **Existing PR gate (HARD CONSTRAINT):** If an open PR already addresses this issue — even partially — treat it as a prerequisite. Use `action: "prerequisites"` with the PR URL in the `existing` array. Do not emit `action: "sufficient"` when an open PR covers the reported problem; dispatching a second implementation would create duplicates. Only skip this rule if the PR is closed without merging (the work was abandoned) or if the PR is clearly unrelated despite mentioning the issue number. | ||
| **Existing PR gate (HARD CONSTRAINT):** If an open PR already addresses this issue, do not emit `action: "sufficient"` — dispatching a second implementation would create duplicates. Instead, distinguish between two cases: |
There was a problem hiding this comment.
[MEDIUM] premature-decision — partial-fix and draft-PR edge cases dropped without guidance
Finding: The rewritten "Existing PR gate" splits existing-PR handling into "fixes this issue" (in-progress) vs. "true prerequisite" (prerequisites), but drops the prior text's "even partially" qualifier without replacing it — a PR that only partially fixes the issue has no clear classification. Issue #4783's own triage analysis (which this PR closes) explicitly listed "PR partially addresses the issue" and "PR is in draft state" as edge cases to handle; neither is addressed here, and the Step 2b gh pr list query this gate depends on doesn't fetch isDraft, so the agent has no signal to distinguish a draft PR from one ready for review. The existing sufficient/prerequisites actions each have a dedicated "Anti-premature-*" hard-constraint rule in the clarity-scoring section; no analogous "Anti-premature-in-progress" rule was added for the new action.
Suggestion: State how partial fixes should be classified (e.g., treat as in-progress only if the remaining scope is trivial, otherwise prerequisites), add isDraft to the Step 2b query with guidance on how draft status affects classification, and add a matching "Anti-premature-in-progress" rule for consistency with the other two hard constraints.
Flagged independently by 2 of 3 review passes; verified against issue #4783's own listed edge cases.
|
This shouldn't go here. @waynesun09 could you take care of porting this to fullsend-ai/agents? |
|
Closing — this change targets the wrong repo. The triage agent was extracted out of the scaffold into The
Issue fullsend-ai/agents#433 stays open to track it. Re-dispatch against |
|
🤖 Finished Retro · ✅ Success · Started 1:40 PM UTC · Completed 1:56 PM UTC |
Retro: PR #5414 — Code agent targeted deprecated scaffold instead of agents repoTimeline
What went well
Root cause and existing coverageThis is the 4th+ documented instance of agent work targeting stale scaffold copies instead of
All improvement opportunities from this retro are already covered by existing issues. No new proposals are warranted. New evidence from this PR
Cost of inactionThis single PR consumed ~30 minutes of agent compute (code + review), substantial human review effort across two reviewers over 3 days, and the fix for agents#433 remains unimplemented. Each instance of this pattern repeats the same waste until one of the defensive layers (#3157, #3476, or #5552) is implemented. Autonomy assessmentThe review agent's code-quality findings were valid — on a correctly-targeted PR, they would have improved the code. However, the review agent lacks architectural awareness to detect location-level misalignment. Implementing #3476 (path-based deprecated-location check) would close this gap. Until then, PRs touching |
The re-ported fix for #433 (agents/triage.md, the triage-result schema, post-triage.sh, and their tests) carried over the same gaps that were flagged during review of the original attempt at this fix, fullsend-ai/fullsend#5414, which targeted a deprecated scaffold copy and was closed in favor of this PR. - Fix additional-properties-lists-known-keys: the expected sorted property list didn't include the new pull_requests field, so this test failed deterministically in CI. - Add valid-in-progress / in-progress-missing-pull-requests schema validation coverage for the in-progress conditional rule. - Clear pr-open in every handler that already clears blocked (insufficient, duplicate, prerequisites, sufficient, question, not-planned), and add pr-open to pre-triage.sh's label reset list and verification query. Without this, pr-open stuck permanently once an issue left the in-progress state, and label_actions couldn't remove it either since it's a control label. - Create the pr-open label with its intended description/color before applying it, matching the convention pre-code.sh already uses for the same label. - Document pr-open in the triage agent's control-label reference (the table and the issue-labels skill example). - Add isDraft to the PR search query and give the agent guidance on classifying partial fixes and draft PRs, plus a matching Anti-premature-in-progress hard constraint alongside the other action's constraints. - Add a multi-PR test case for the in-progress action. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
3 participants
Summary
The triage agent's "Existing PR gate" hard constraint forced all open PRs that address an issue into the
prerequisitesaction, which applies theblockedlabel and uses "blocked by" framing. This is semantically incorrect when a PR directly fixes the reported problem — the issue is not blocked, it is being addressed. This PR adds a newin-progressaction that correctly distinguishes between PRs that fix an issue and PRs that are true prerequisites.Related Issue
Fixes fullsend-ai/agents#433
Changes
agents/triage.md: Updated the "Existing PR gate" to distinguish two cases: (1) PR fixes the issue → usein-progressaction, (2) PR is a true prerequisite → useprerequisitesaction. Addedin-progressaction documentation with JSON format and hard constraint.schemas/triage-result.schema.json: Addedin-progressto the action enum, addedpull_requestsproperty (array of PR URLs restricted to/pull/paths), added conditional validation requiringpull_requestswhen action isin-progress.scripts/post-triage.sh: Addedin-progresscase handler that removesblocked/ready-to-code/needs-infolabels, appliespr-openlabel, and appends "Addressed by:" section with PR links. Addedpr-opentoCONTROL_LABELS.scripts/post-triage-test.sh: Added 8 test cases covering: comment posting, label application/removal, missing comment validation, "Addressed by:" content, and control label refusal. Updated mockghto capture body content piped via--body-file -.Testing
in-progresstests passyqin sandbox is unrelated)Closes fullsend-ai/agents#433
Post-script verification
agent/4783-triage-in-progress-action)c088a3c72eabffcc350196a71b6351fb7d6af659..HEAD)