Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions internal/scaffold/fullsend-repo/agents/triage.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,14 @@ Also look for **blocking relationships** — open issues or PRs that must be res
- The issue describes a feature that depends on infrastructure or API changes tracked in another issue
- The issue references an upstream library, service, or repository that has a known open bug
- A PR is already in flight that would conflict with or must land before work on this issue
- An open PR already addresses this issue, even partially — the work is already in progress
- 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:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] 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.


1. **PR fixes this issue** — the PR directly resolves the reported problem. Use `action: "in-progress"` with the PR URL in the `pull_requests` array. This signals that work is already underway, not that the issue is blocked.
2. **PR is a true prerequisite** — the PR does not fix this issue but must land before work on this issue can start (e.g., infrastructure changes, API additions, dependency upgrades). Use `action: "prerequisites"` with the PR URL in the `existing` array.

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.

If the issue mentions other repositories, libraries, or upstream projects, search those too:

Expand Down Expand Up @@ -238,6 +242,23 @@ At least one of the two arrays must have entries.
}
```

### Action: `in-progress`

An open pull request already addresses this issue — the work is underway. Use this action when a PR directly fixes or resolves the reported problem. This is distinct from `prerequisites`, which is for PRs that must land *before* work on this issue can start.

**HARD CONSTRAINT:** Never emit `sufficient` when an open PR already fixes this issue. Use `in-progress` instead — dispatching a second implementation would create duplicates.

```json
{
"action": "in-progress",
"reasoning": "Brief explanation of how the PR addresses this issue",
"pull_requests": [
{ "url": "https://github.com/org/repo/pull/123" }
],
"comment": "A professional comment explaining that this issue is already being addressed by an existing PR. Link to the PR(s) and briefly describe how they resolve the reported problem. Use 'addressed by' or 'fixed by' framing — not 'blocked by'."
}
```

### Action: `sufficient`

Information is sufficient for a developer to investigate and fix.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"properties": {
"action": {
"type": "string",
"enum": ["insufficient", "duplicate", "sufficient", "prerequisites", "question"]
"enum": ["insufficient", "duplicate", "sufficient", "prerequisites", "question", "in-progress"]
},
"reasoning": {
"type": "string",
Expand Down Expand Up @@ -73,6 +73,21 @@
},
"additionalProperties": false
},
"pull_requests": {
"type": "array",
"minItems": 1,
"items": {
"type": "object",
"required": ["url"],
"properties": {
"url": {
"type": "string",
"pattern": "^https://github\\.com/[a-zA-Z0-9._-]+/[a-zA-Z0-9._-]+/pull/[0-9]+$"
}
},
"additionalProperties": false
}
},
"label_actions": {
"$ref": "#/$defs/label_actions"
}
Expand Down Expand Up @@ -103,6 +118,10 @@
}
}
}
},
{
"if": { "properties": { "action": { "const": "in-progress" } }, "required": ["action"] },
"then": { "required": ["pull_requests"] }
}
],
"$defs": {
Expand Down
39 changes: 39 additions & 0 deletions internal/scaffold/fullsend-repo/scripts/post-triage-test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@ if [[ "\$1" == "issue" ]] && [[ "\$2" == "create" ]]; then
echo "https://github.com/mock-org/mock-repo/issues/999"
exit 0
fi
# Capture body content piped via --body-file - so tests can verify comment bodies.
if [[ "\$1" == "issue" ]] && [[ "\$2" == "comment" ]] && [[ "\$*" == *"--body-file -"* ]]; then
BODY=\$(cat)
echo "gh \$* <<BODY:\${BODY}:BODY>>" >> "${GH_LOG}"
exit 0
fi
echo "gh \$*" >> "${GH_LOG}"
MOCKEOF
chmod +x "${MOCK_BIN}/gh"
Expand Down Expand Up @@ -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"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] test-adequacy

No test for the multiple-PR case (pull_requests array with 2+ entries). All 8 new tests use single-element arrays.

run_test "in-progress-posts-comment" \
'{"action":"in-progress","reasoning":"PR fixes the reported bug","pull_requests":[{"url":"https://github.com/test-org/test-repo/pull/100"}],"comment":"This issue is already being addressed by an existing pull request."}' \
"gh issue comment 42 --repo test-org/test-repo --body-file -"

run_test "in-progress-applies-pr-open-label" \
'{"action":"in-progress","reasoning":"PR fixes the reported bug","pull_requests":[{"url":"https://github.com/test-org/test-repo/pull/100"}],"comment":"This issue is already being addressed by an existing pull request."}' \
"gh api repos/test-org/test-repo/issues/42/labels -f labels[]=pr-open --silent"

run_test "in-progress-removes-blocked-label" \
'{"action":"in-progress","reasoning":"PR fixes the reported bug","pull_requests":[{"url":"https://github.com/test-org/test-repo/pull/100"}],"comment":"This issue is already being addressed by an existing pull request."}' \
"gh api repos/test-org/test-repo/issues/42/labels/blocked -X DELETE --silent"

run_test "in-progress-removes-ready-to-code-label" \
'{"action":"in-progress","reasoning":"PR fixes the reported bug","pull_requests":[{"url":"https://github.com/test-org/test-repo/pull/100"}],"comment":"This issue is already being addressed by an existing pull request."}' \
"gh api repos/test-org/test-repo/issues/42/labels/ready-to-code -X DELETE --silent"

run_test "in-progress-removes-needs-info-label" \
'{"action":"in-progress","reasoning":"PR fixes the reported bug","pull_requests":[{"url":"https://github.com/test-org/test-repo/pull/100"}],"comment":"This issue is already being addressed by an existing pull request."}' \
"gh api repos/test-org/test-repo/issues/42/labels/needs-info -X DELETE --silent"

run_test "in-progress-missing-comment-fails" \
'{"action":"in-progress","reasoning":"PR fixes the reported bug","pull_requests":[{"url":"https://github.com/test-org/test-repo/pull/100"}]}' \
"" \
"true"

run_test "in-progress-appends-addressed-by" \
'{"action":"in-progress","reasoning":"PR fixes the reported bug","pull_requests":[{"url":"https://github.com/test-org/test-repo/pull/100"}],"comment":"This issue is already being addressed."}' \
"Addressed by:"

run_test_stdout "in-progress-control-label-refused" \
'{"action":"in-progress","reasoning":"PR fixes the reported bug","pull_requests":[{"url":"https://github.com/test-org/test-repo/pull/100"}],"comment":"This issue is addressed.","label_actions":{"reason":"Tried to set pr-open label.","actions":[{"action":"add","label":"pr-open"}]}}' \
"::warning::Refused to add control label 'pr-open' -- control labels are managed by the triage pipeline"

# --- Summary ---

echo ""
Expand Down
34 changes: 33 additions & 1 deletion internal/scaffold/fullsend-repo/scripts/post-triage.sh
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ remove_label() {
# add or remove these via label_actions. This list covers labels that the
# 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")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] 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.


is_control_label() {
local label="$1"
Expand Down Expand Up @@ -122,6 +122,38 @@ case "${ACTION}" in
add_label "duplicate"
;;

in-progress)
if [[ -z "${COMMENT}" ]]; then
echo "ERROR: action is 'in-progress' but no comment provided" >&2
exit 1
fi

# Collect PR URLs from the pull_requests array.
PR_COUNT=$(jq '.pull_requests // [] | length' "${RESULT_FILE}")
PR_URLS=""
for i in $(seq 0 $((PR_COUNT - 1))); do
URL=$(jq -r ".pull_requests[${i}].url" "${RESULT_FILE}")
PR_URLS="${PR_URLS} ${URL}"
done
PR_URLS=$(echo "${PR_URLS}" | xargs) # trim whitespace

if [[ -n "${PR_URLS}" ]]; then
PR_LIST=""
for url in ${PR_URLS}; do
PR_LIST="${PR_LIST}
- ${url}"
done
COMMENT="${COMMENT}

**Addressed by:**${PR_LIST}"
fi

remove_label "ready-to-code"
remove_label "needs-info"
remove_label "blocked"
add_label "pr-open"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] 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.

;;

prerequisites)
if [[ -z "${COMMENT}" ]]; then
echo "ERROR: action is 'prerequisites' but no comment provided" >&2
Expand Down
Loading