feat(#433): add in-progress action for triage agent - #434
Merged
Conversation
The triage agent's "Existing PR gate" forced every open PR that addresses an issue into the prerequisites action, which applies the blocked label. When a PR directly fixes an issue, that framing is incorrect — the issue is being resolved, not blocked. Add a new in-progress action that distinguishes fixing PRs from true prerequisite PRs: - agents/triage.md: update the Existing PR gate to route fixing PRs to in-progress and true prerequisites to prerequisites; add in-progress action documentation - schemas/triage-result.schema.json: add in-progress to the action enum; add pull_requests array property required when action is in-progress - scripts/post-triage.sh: add in-progress handler that clears blocked/ready-to-code/needs-info, applies pr-open label, and appends Addressed by section with PR links; add pr-open to control labels - scripts/post-triage-test.sh: add test cases covering comment posting, label mutations, PR link appending, missing comment validation, and control label refusal; update mock to capture stdin for plain comments Closes #433
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>
…y review Independent 3-agent review (Claude, Grok) of the previous commit surfaced several real gaps, each verified against the actual code and CI runtime before fixing: - agents/triage.md's new "note the draft status in `reasoning`" guidance pointed at a field post-triage.sh never reads (only `.action` and `.comment` reach GitHub) -- moved to `comment`. - The Anti-premature-in-progress and Anti-premature-resolution hard constraints could both apply with no stated precedence when a fixing PR is open but the issue still has user-facing gaps -- in-progress now explicitly takes priority, with remaining gaps surfaced via `comment`. - The partial-fix/draft-PR guidance contradicted the `prerequisites` action's own "does not itself fix the issue" definition and gave no checkable criteria for "trivial" vs "substantial" -- rewritten around Fixes/Closes references, with an explicit fallback and a precedence rule for the fixing-PR-plus-separate-blocker case. - pr-open's removal on the six actions that leave the in-progress state (insufficient, duplicate, prerequisites, sufficient, question, not-planned) had no regression coverage -- added one test per handler. - Added schema coverage for `pull_requests: []` and a malformed PR URL, mirroring the existing `prerequisites` fixture coverage. - docs/triage.md's pr-open row only credited the `in-progress` action; pre-code.sh also applies pr-open independently -- documented both sources and pr-open's staleness if the linked PR is abandoned. - Corrected the CONTROL_LABELS comment in post-triage.sh, which was already inaccurate before this PR. One reviewer-reported finding was investigated and found to be a false positive: a claimed `seq 0 $((PR_COUNT - 1))` null-entry bug when PR_COUNT is 0. Verified against GNU coreutils 9.4 in an ubuntu-24.04 container (matching the CI runner) that `seq 0 -1` produces zero iterations there, not two -- the behavior only reproduces on BSD/macOS `seq`. The path is additionally unreachable in production since the schema requires `pull_requests` to be non-empty whenever action is "in-progress". Assisted-by: Claude (fix, review), Grok (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
Two independent re-review passes (Claude x2) found that the previous commit added explicit precedence for Anti-premature-in-progress over Anti-premature-resolution, but left an analogous, unaddressed conflict with the untouched Anti-premature-prerequisites rule: a fixing PR plus a separate genuine blocker (PR or issue) could be read as requiring both `in-progress` and `prerequisites` with no stated tie-break. - Extended the Existing PR gate's fixing-PR-plus-separate-blocker sentence to also cover a separate blocking *issue*, not just a PR. - Added a reciprocal exception to Anti-premature-prerequisites and an explicit precedence clause to Anti-premature-in-progress, so the tie-break is stated from both rules regardless of which one an agent (or future editor) reads first. Both re-review passes also independently re-verified the `seq` false-positive conclusion from the prior commit against a real ubuntu-24.04/GNU-coreutils-9.4 container (one of them starting from a stale memory of its own that wrongly generalized BSD/macOS `seq` behavior to GNU, and self-correcting after re-testing) -- no code change needed there, already covered by the previous commit's message. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
Member
|
/review |
PR Reviewer Guide 🔍Warning
Here are some key observations to aid the review process:
|
Member
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 3:54 PM UTC · Completed 4:13 PM UTC |
ReviewFindingsMedium
Low
|
fullsend-ai-review
Bot
added
the
requires-manual-review
A multi-agent review of this branch surfaced one high and five medium findings against the new in-progress action. Address them: - The Existing PR gate told the agent to prefer `prerequisites` when it could not tell a fixing PR from a true prerequisite. That contradicted the Anti-premature-in-progress hard constraint, which only exempts PRs positively determined to be prerequisites, and routed every ambiguous PR back to the `blocked` framing this issue was filed to stop. Flip the tiebreak to `in-progress` and reword the constraint to match. - The handler applied `pr-open` and posted "an open PR addresses this issue" without checking that any PR URL was actually present, so an empty `pull_requests` produced a claim with nothing to link. The schema requires the field, but the agent is instructed to emit its best JSON after three failed validation attempts, so re-check it here. Replace the seq/xargs URL loop with a read over `jq -r`, which renders a byte-identical comment and drops the platform-dependent seq bounds. - in-progress is a durable status but posted a fresh comment on every run, and triage re-runs on every issue edit. Route it through the sticky comment path under its own marker so re-runs update in place. - Triage discovered PRs with an unfiltered listing capped at 50 while the code agent's pre-check uses a targeted search, so the gate could silently miss a PR and fall through to `sufficient`. Add the targeted search and state that coder-bot PRs count for the gate. - A result carrying both `prerequisites` and `pull_requests` had the former dropped with no trace. Warn instead, matching how the script already reports refused control labels. - The `pr-open` docs implied recovery was automatic. State plainly that nothing clears the label when the linked PR closes unmerged, and define in-progress against prerequisites while there. Also fix two wording defects in the same regions: the comment template asked the agent to link PRs the post-script appends anyway, and the "an open PR already addresses this issue" bullet sat under blocking relationships, priming the agent on `blocked` before it reached the gate. Assisted-by: Claude (fix, review), Grok (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
Follow-up review of the previous commit found the jq call that collects PR URLs had moved into a process substitution, where its exit status is invisible to set -e. A pull_requests that clears the count check but is not an array of objects — a bare string, a number, an array of strings — then rendered an empty "Addressed by:" list and continued, applying pr-open and posting the linkless comment the count guard exists to prevent. The previous code aborted before any label mutation. Capture with command substitution again, and use jq -e so a null url fails too. Also: - Warn about dropped prerequisites only when there is something to drop; an empty existing/create pair is schema-valid and was tripping a spurious warning in the run log. - Request url in the targeted PR search. The agent has to emit a full pull_requests[].url, so omitting it forced the URL to be rebuilt by hand; the code agent's equivalent search already asks for it. - List the new in-progress marker in the prior-triage detection step, which is the one place that enumerates markers. - Correct the pr-open doc line, which said the issue is not re-evaluated until someone runs /fs-triage manually and so contradicted the trigger list above it — an issue edit re-runs triage too. Note that the in-progress comment is likewise not retracted. - Assert the marker in the sufficient sticky-comment test, so swapping the two markers cannot pass unnoticed in either direction. Assisted-by: Claude (fix, review), Grok (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
waynesun09
approved these changes
Jul 24, 2026
waynesun09
left a comment
waynesun09
left a comment
Member
There was a problem hiding this comment.
Approving after a multi-agent review and fix cycle on this branch.
Review found 1 high and 5 medium findings against the new in-progress action; all are addressed in 428a846 and 27f95f0:
- The Existing PR gate's "when unsure, prefer
prerequisites" tiebreak contradicted the Anti-premature-in-progress hard constraint and routed ambiguous PRs back toblockedframing — the exact behavior #433 was filed about. Tiebreak flipped toin-progress; the constraint now permitsprerequisitesonly when positively determined. - The handler applied
pr-openand claimed a PR addressed the issue without verifying any PR URL was present. Added a count guard, plus a follow-up fix restoringset -epropagation on the jq call so a malformedpull_requestsaborts instead of rendering an empty "Addressed by:" list. in-progressre-posted a fresh comment on every triage run; it is a durable status and triage re-runs on every issue edit. Now routed through the sticky comment path under its own marker.- Triage's PR discovery (unfiltered listing capped at 50) was not equivalent to the code agent's pre-check (targeted search); the gate could silently miss a PR and fall through to
sufficient. Added the targeted search. - A result carrying both
prerequisitesandpull_requestshad the former dropped silently; now warns, gated on there actually being something to drop. - The
pr-opendocs implied automatic recovery. Corrected to state that nothing clears the label when the linked PR closes unmerged.
Verification: 80 assertions pass in scripts/post-triage-test.sh (up from 56, including guard-branch regression tests that fail against the pre-fix script), schema tests pass, shellcheck clean, and all required CI checks are green. Verified from a clean checkout of the head ref.
|
🤖 Finished Retro · ✅ Success · Started 8:36 PM UTC · Completed 8:51 PM UTC |
Retro: PR #434 — Add in-progress action for triage agentTimeline
Key findingsReview quality gap — correctness sub-agent missed all 6 bugs. The correctness sub-agent (Opus) analyzed handler logic, schema correctness, label lifecycle, and test coverage but reported zero findings. The most critical miss was a logical contradiction: the Existing PR gate's "when unsure, prefer prerequisites" tiebreak directly undermined the new Anti-premature-in-progress hard constraint — ambiguous cases would still route to blocked framing, re-introducing the exact bug #433 was filed about. The human also found: missing PR URL count guard, missing Cross-repo-contracts sub-agent false positives: This sub-agent (Sonnet) produced 5 findings including a CRITICAL false positive ("missing handler for in-progress") because it read from disk (base branch) instead of using the API-fetched PR head content provided in its prompt. The orchestrator correctly filtered 4 of 5 as false positives, but the token cost was wasted and the orchestrator's challenger pass had to reason through each one. Fix agent not involved: The review agent's Evidence for existing issues
Proposals filed
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
in-progresstriage action to distinguish PRs that fix an issue from true prerequisite PRs. Previously, the "Existing PR gate" forced all addressing PRs intoaction: "prerequisites"(blocked framing), even when the PR directly resolves the reported problem.blocked/ready-to-code/needs-info/pr-openlabels as appropriate, applies apr-openlabel, and appends an "Addressed by:" section with PR links to the comment.scripts/pre-triage.shanddocs/triage.mdto keeppr-open's lifecycle and documentation consistent with the rest of the control-label set.Changes
agents/triage.md— Updated the "Existing PR gate" hard constraint to route fixing PRs toin-progressand true prerequisites toprerequisites; addedin-progressaction documentation, anAnti-premature-in-progresshard constraint with explicit precedence overAnti-premature-resolution, anisDraftfield on the PR search query, and checkable guidance for partial-fix/draft-PR classification.schemas/triage-result.schema.json— Addedin-progressto the action enum; addedpull_requestsarray property (required when action isin-progress) with URL pattern validation for/pull/URLs.scripts/post-triage.sh— Added thein-progresscase handler; addedpr-opento the control labels array and to theremove_labelcalls in the other six action handlers so it clears when an issue leaves the in-progress state; creates thepr-openlabel with its intended description/color before use, matchingpre-code.sh's existing convention.scripts/pre-triage.sh— Addedpr-opento the label reset list and verification query.scripts/post-triage-test.sh/scripts/validate-output-schema-test.sh— Test cases for the new action, its conditional schema rule, multi-PR handling, andpr-openremoval across all six other action handlers.docs/triage.md— Documented thepr-opencontrol label, including that it's also applied independently by the code agent's pre-check.Test plan
in-progresstest cases pass (comment posting, label mutations, PR link appending, missing comment validation, control label refusal, multi-PR)post-retro-test.sh's proposal tests observed locally, environment-specific — not present in this PR's CI runs and unrelated to this change)pull_requestsconditional rule, empty-array, and malformed-URL casespr-openlabel is created with its intended description/color (gh label create --force) before being applied, matchingpre-code.sh's existing conventionCloses #433
Post-script verification
agent/433-in-progress-action)e3293716ee87a1b1a6d07a403ccdeba4d724c9b9..HEAD)Follow-up commits (post-handoff)
The initial commit reused the same fix that was previously attempted against the wrong repo (fullsend-ai/fullsend#5414, closed). Two follow-up commits close the gaps found there and in a fresh 3-agent review of this PR:
additional-properties-lists-known-keys(newpull_requestsproperty wasn't in the expected sorted list), closed thepr-openlabel-lifecycle gap (never cleared when an issue leaves in-progress) and its missinggh label createfallback, and ported the remaining review feedback from #5414 that applies to this repo's current (more evolved) triage pipeline.reasoning(never read bypost-triage.sh) tocomment; added explicit precedence between the twoAnti-premature-*hard constraints that could otherwise both apply; rewrote the partial-fix/draft-PR guidance to stop contradicting theprerequisitesaction's own definition; added regression tests forpr-openremoval across all six handlers and schema coverage for empty/malformedpull_requests. One reported finding (aseq-based null-entry bug) was investigated and confirmed to be a false positive specific to BSD/macOSseq— verified against GNU coreutils 9.4 in an ubuntu-24.04 container matching the actual CI runner.