Repository navigation
Conversation
…ns dynamically When the triage agent marks an issue with action: completed, post-triage.sh currently reuses JIRA_SPLIT_TRANSITION (defaulting to "Obsolete" per ADR 0080/0081) to transition the issue in Jira Cloud. In workflows where "Obsolete" is not present, tracker_close_issue fails and post-triage.sh exits with status 1 under set -e. - Add JIRA_COMPLETED_TRANSITION (defaulting to "Closed") in harness/triage.yaml. - Decouple action: split from action: completed in scripts/post-triage.src.sh, mapping split to GitHub reason "completed" in github-triage-ops.lib.sh. - Add dynamic transition resolution and idempotency check in jira-triage-ops.lib.sh. - Rebuild script bundles via make script-build. - Document JIRA_COMPLETED_TRANSITION in docs/triage.md and LOCAL.md. - Add unit tests for completed transition, fallback, and idempotency in post-triage-test.sh. Signed-off-by: Jeffery Bagirimvano <jefferyb@redhat.com>
PR Summary by QodoDecouple Jira completed and split transitions with safe fallback
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Signed-off-by: Jeffery Bagirimvano <jefferyb@redhat.com>
Signed-off-by: Jeffery Bagirimvano <jefferyb@redhat.com>
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review findings: 1 HIGH, 3 MEDIUM. Details are in the inline comments.
| # `fullsend issues` reads tracker.jira from this directory's config. | ||
| FULLSEND_DIR: ${FULLSEND_DIR} | ||
| JIRA_BASE_URL: ${JIRA_BASE_URL} | ||
| JIRA_COMPLETED_TRANSITION: "Closed" |
There was a problem hiding this comment.
HIGH: Default for completed changes on upgrade, but the PR has no breaking-change marker
Before this PR, completed used JIRA_SPLIT_TRANSITION (harness default "Obsolete"). The harness now always sets JIRA_COMPLETED_TRANSITION: "Closed", so every managed Jira run picks a different transition. The documented NOTICE fallback to JIRA_SPLIT_TRANSITION (lib lines 368-381, docs/triage.md) can never fire through the shipped harness.
Two groups break on upgrade:
- (a) Users whose overlay set only
JIRA_SPLIT_TRANSITIONto get their owncompletedtransition now get "Closed" or a Done fallback instead, with no warning. - (b) Custom workflows that offer "Obsolete" but none of Done/Closed/Resolved/Complete used to succeed and now exit 1: the fallback regex at line 443 excludes "obsolete".
COMMITS.md:79 lists "Default values change in ways that alter existing behavior" as breaking, and COMMITS.md:115 asks reviewers to flag a missing ! as high severity. The title and all three commits are plain fix(triage): with no BREAKING CHANGE: trailer.
Suggestion: Retitle to fix(triage)!: and add a BREAKING CHANGE: trailer: completed now uses JIRA_COMPLETED_TRANSITION (harness default "Closed", with a Done-category fallback), and overlays that set only JIRA_SPLIT_TRANSITION must also set JIRA_COMPLETED_TRANSITION. Also fix docs/triage.md and the lib header so they no longer suggest the fallback to JIRA_SPLIT_TRANSITION applies under the harness. The alternative is to leave the harness value empty so the documented fallback really runs.
There was a problem hiding this comment.
Fixed in a274b35: removed JIRA_COMPLETED_TRANSITION: "Closed" from harness/triage.yaml (and updated LOCAL.md to match) so existing overlays and custom workflows preserve the documented fallback to JIRA_SPLIT_TRANSITION unless explicitly overridden.
| "" true | ||
|
|
||
| # Jira completed action falls back to JIRA_SPLIT_TRANSITION when JIRA_COMPLETED_TRANSITION is unset. | ||
| ( |
There was a problem hiding this comment.
MEDIUM: Test jira-completed-fallback-to-split-var runs in a subshell, so its failures never reach the suite
Lines 2437-2442 call run_jira_test_stdout inside ( unset JIRA_COMPLETED_TRANSITION; ... ). The helper (line 2341) records a failure only with FAILURES=$((FAILURES + 1)) and returns 0. That increment is lost when the subshell exits, so the suite still prints All tests passed and exits 0. Changing the expected string so it cannot match confirms this: the run prints FAIL for this test and still exits 0. The JIRA_COMPLETED_TRANSITION -> JIRA_SPLIT_TRANSITION fallback therefore has no working regression test. The same file already handles env-changing cases correctly at lines 2967-2977 (JIRA_CASE_FAILURES file plus jira_case_fail).
Suggestion: Drop the subshell. Save JIRA_COMPLETED_TRANSITION, unset it, call the helper, then restore it. Or record the failure through the JIRA_CASE_FAILURES file mechanism. Then repeat the mutation check to confirm a broken expectation now fails the suite.
There was a problem hiding this comment.
Fixed in a274b35: replaced the subshell around jira-completed-fallback-to-split-var in scripts/post-triage-test.sh with save/restore of JIRA_COMPLETED_TRANSITION in the parent shell so FAILURES increments propagate to the suite.
| local is_rejection_status=0 | ||
| local dup_name="${JIRA_DUPLICATE_TRANSITION:-}" | ||
| local not_planned_name="${JIRA_NOT_PLANNED_TRANSITION:-}" | ||
| if echo "${current_status_name}" | grep -qiE "won.?t do|not.?planned|duplicate|obsolete|reject|cancel|invalid|cannot reproduce|abandon"; then |
There was a problem hiding this comment.
MEDIUM: Re-running split on an issue already in "Obsolete" overwrites it to Done/Closed
The shipped split status is JIRA_SPLIT_TRANSITION="Obsolete", but "obsolete" is in the rejection regex in both the idempotency check (line 399) and the fallback exclusion (line 443). On a split re-run for an issue already in "Obsolete": is_rejection_status=1, so the split skip at lines 407-411 does not fire. The exact "Obsolete" transition is usually not offered from the Obsolete status, so the fallback picks Done/Closed and POSTs it. The issue moves out of its split resolution into a plain completed one. The split path has no tests for idempotency or fallback.
Suggestion: For reason=split, count the current status as handled when it matches the configured JIRA_SPLIT_TRANSITION name (case-insensitive), before the rejection regex is checked. More generally, skip when the current status matches the configured target for this reason. Add tests for split re-run on an Obsolete issue and for split when the Obsolete transition is unavailable.
There was a problem hiding this comment.
Fixed in a274b35: tracker_close_issue in scripts/lib/jira-triage-ops.lib.sh now checks case-insensitive equality against the target transition_name before evaluating the rejection regex, so re-running split when already in "Obsolete" skips idempotently. Added jira-split-idempotent-obsolete and jira-split-fallback-done tests.
| local not_planned_name="${JIRA_NOT_PLANNED_TRANSITION:-}" | ||
| if echo "${current_status_name}" | grep -qiE "won.?t do|not.?planned|duplicate|obsolete|reject|cancel|invalid|cannot reproduce|abandon"; then | ||
| is_rejection_status=1 | ||
| elif [[ -n "${dup_name}" ]] && echo "${current_status_name}" | grep -qiF "${dup_name}"; then |
There was a problem hiding this comment.
MEDIUM: Configured transition names are matched as status substrings, which breaks the documented "set all four to the same name" setup
Lines 401-404 mark the current status as a rejection when its name contains JIRA_DUPLICATE_TRANSITION or JIRA_NOT_PLANNED_TRANSITION (grep -qiF, substring match). Those values are transition names, not status names. docs/triage.md tells users whose workflow does not tell outcomes apart to "Set all four to the same transition name", e.g. all "Done". With that setup, an issue already in status "Done" counts as a rejection, so the completed/split idempotent skip never fires. The exact "Done" transition is normally not offered from Done. The fallback (lines 438-441) then removes every transition whose name equals the duplicate/not-planned names, which is "Done" again. The run ends with ERROR: transition 'Done' ... is not available and exits 1, the failure this PR set out to remove, reached by following the docs.
Suggestion: Do not use the duplicate/not-planned names to classify status when they equal the name configured for the current reason. Better, compare the current status to the configured name for the requested reason first and skip on a match. Add a test with all four variables set to "Done" and the issue already in Done.
There was a problem hiding this comment.
Fixed in a274b35: dup_name and not_planned_name are now compared case-insensitively for equality only when they differ from transition_name, both in the pre-flight status check and in the fallback jq exclusion selector. Added jira-all-four-done-idempotent-completed covering all four transition vars set to "Done".
…tatus checks Signed-off-by: Jeffery Bagirimvano <jefferyb@redhat.com>
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review findings: 4 MEDIUM. Details are in the inline comments.
| # the raw REST calls in this file and the `fullsend` shell-outs below. | ||
| # | ||
| # Optional env vars: | ||
| # JIRA_COMPLETED_TRANSITION — transition name for the "completed" action |
There was a problem hiding this comment.
MEDIUM: Header comment and PR body still say JIRA_COMPLETED_TRANSITION defaults to "Closed"; the shipped harness has no default
Commit a274b35 removed JIRA_COMPLETED_TRANSITION: "Closed" from harness/triage.yaml, and harness/triage.yaml is no longer in the diff. This header (lines 46-48) still says (default: "Closed"; falls back to JIRA_SPLIT_TRANSITION when unset), and the rebuilt bundles carry the same text (scripts/post-triage.sh, scripts/pre-code-jira.sh, scripts/pre-triage.sh). The PR body also still says it "Adds JIRA_COMPLETED_TRANSITION (defaulting to "Closed") in harness/triage.yaml" and lists triage.yaml under Changes.
With the shipped harness, completed falls back to JIRA_SPLIT_TRANSITION="Obsolete". So completed is decoupled from split only for operators who set the variable, and the reported failure is fixed by the name fallback, not by decoupling. LOCAL.md and docs/triage.md are correct; this header, which operators read first, is wrong. (The earlier header thread asked to add a "Closed" default; that default was later removed, so this is a new mismatch.)
Suggestion: Change the header to "no default; falls back to JIRA_SPLIT_TRANSITION when unset" and run make script-build so all four copies match. Update the PR title and body to say that, by default, completed still routes through JIRA_SPLIT_TRANSITION (with the Done-name fallback) and that harness/triage.yaml is unchanged.
There was a problem hiding this comment.
Fixed in 20bebaa (20bebaa): updated the JIRA_COMPLETED_TRANSITION header comment in scripts/lib/jira-triage-ops.lib.sh (and rebuilt the bundles) to state (no default; falls back to JIRA_SPLIT_TRANSITION when unset), and updated the PR body to remove the stale "Closed" default / harness/triage.yaml references and note that validation used mocked workflow responses (linking fullsend-ai/fullsend#6704).
| | not | ||
| ) | ||
| ] as $done_transitions | ||
| | ([$done_transitions[] | select(.name | test("(?i)^(done|closed|resolved|complete(d)?)$"))][0].id // empty) |
There was a problem hiding this comment.
MEDIUM: Fallback matches only exact transition names, so common workflows still fail; docs describe a broader fallback
The fallback keeps a done-category transition only when its transition .name exactly matches ^(done|closed|resolved|complete(d)?)$. In Jira, the transition name and the target status (.to.name) are separate fields. Transitions named "Resolve Issue", "Close Issue", "Close", "Mark as done", "Finished", "Fixed" and so on do not match, even when they lead to a Done/Closed/Resolved status. The action then still exits 1 with "transition ... is not available" under set -e, which is the failure this PR set out to remove.
docs/triage.md (lines 292-295) promises a fallback to "an available positive Done-category transition (preferring "Done", "Closed", "Resolved", or "Complete")". That wording implies other positive Done transitions are accepted and that there is a priority order. The code accepts only those exact names and takes [0] in API order. Only mocked tests back the change.
Suggestion: Select candidates by .to.statusCategory.key == "done" and match the positive list against (.to.name // .name), keeping the rejection exclusion on both .name and .to.name. Rank matches explicitly if the docs keep the word "preferring". Otherwise narrow docs/triage.md to "the first available transition named exactly Done, Closed, Resolved, or Complete(d)". Please also state in the PR whether this was validated against a live team-managed or company-managed project.
There was a problem hiding this comment.
Fixed in 20bebaa (20bebaa): the fallback selector in scripts/lib/jira-triage-ops.lib.sh now matches positive Done-category names against (.to.name // .name) while keeping rejection exclusions on both .name and .to.name, and ranks tier-1 exact matches (Done, Closed, Resolved, Complete(d)) ahead of tier-2 word-boundary matches so transitions such as "Resolve Issue" -> "Resolved" work cleanly. Added jira-completed-fallback-resolve-issue coverage in scripts/post-triage-test.sh and rebuilt bundles.
| is_rejection_status=1 | ||
| elif [[ -n "${not_planned_name}" && "${not_planned_name,,}" != "${transition_name,,}" && "${current_status_name,,}" == "${not_planned_name,,}" ]]; then | ||
| is_rejection_status=1 | ||
| fi |
There was a problem hiding this comment.
MEDIUM: duplicate and not planned treat any rejection status as already handled, and this path has no tests
In the idempotency pre-flight (lines 401-414), any done-category status that matches the broad rejection regex (won't do|not planned|duplicate|obsolete|reject|cancel|invalid|cannot reproduce|abandon), or that equals the dup or not-planned name, returns 0 for both duplicate and not planned. So a not planned action on an issue already in "Duplicate" (or a duplicate action on an issue in "Won't Do" or "Obsolete") reports success and keeps the wrong resolution. Before this PR, the transition was attempted and failed visibly if unavailable.
This is the same class as the earlier "wrong resolution" thread, which was fixed only for completed/split. The new (rejected); skipping transition branch has no test: MOCK_JIRA_STATUS_REJECTED is used only for the split-Obsolete case (scripts/post-triage-test.sh ~2456-2477).
Suggestion: For duplicate and not planned, skip only when the current status equals the configured transition name or its target status; otherwise attempt the transition as before. Add MOCK_JIRA_STATUS_REJECTED tests for duplicate and not planned, including one where the issue is in the other rejection status.
There was a problem hiding this comment.
Fixed in 20bebaa (20bebaa): removed the broad rejection-regex short-circuit for duplicate and not planned so they only skip idempotently when the current Jira status matches the configured transition name or its .to.name target status. Added jira-duplicate-on-wont-do-transitions and jira-not-planned-idempotent-wont-do (plus -no-post) tests in scripts/post-triage-test.sh.
| transition_id=$(echo "${transitions}" | jq -r --arg name "${transition_name}" \ | ||
| '[.transitions[] | select(.name == $name)][0].id // empty') | ||
|
|
||
| # Fallback resolution when configured transition name is not directly available |
There was a problem hiding this comment.
MEDIUM: An explicitly configured transition that does not exist is now silently replaced by the heuristic fallback
For completed and split, when the configured name is missing from the issue's transitions, the code falls back to any Done/Closed/Resolved/Complete transition. It logs only a plain stdout line and the run succeeds. A typo or stale override (for example JIRA_COMPLETED_TRANSITION="Closd", or a renamed workflow transition) used to fail the post-script. Now it quietly closes the issue through a different transition, with no CI annotation for the operator. No linked issue or ADR records this change to the operator contract; the fullsend Jira user guide points tracker-neutral transition discovery at fullsend-ai/fullsend#6704.
Suggestion: Print the fallback message as a ::warning:: annotation so operators see the substitution in the run summary, and link the PR to fullsend-ai/fullsend#6704 or to an issue that records the new behaviour.
There was a problem hiding this comment.
Fixed in 20bebaa (20bebaa): emitted a ::warning::Transition '...' is not available on issue ...; falling back to '...' via Done-category heuristics annotation (sanitized via _gha_sanitize) when substituting a missing transition, updated jira-completed-fallback-done to assert it, and linked fullsend-ai/fullsend#6704 in the PR description.
Signed-off-by: Jeffery Bagirimvano <jefferyb@redhat.com>
jefferyb
changed the title
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review findings: 1 MEDIUM. Details are in the inline comment.
| | select( | ||
| ($dup != "" and ($dup | ascii_downcase) != ($target | ascii_downcase) and (($n | ascii_downcase) == ($dup | ascii_downcase) or ($to | ascii_downcase) == ($dup | ascii_downcase))) or | ||
| ($not_planned != "" and ($not_planned | ascii_downcase) != ($target | ascii_downcase) and (($n | ascii_downcase) == ($not_planned | ascii_downcase) or ($to | ascii_downcase) == ($not_planned | ascii_downcase))) or | ||
| ($n | test("(?i)won.?t do|not.?planned|duplicate|obsolete|reject|cancel|invalid|cannot reproduce|abandon")) or |
There was a problem hiding this comment.
MEDIUM: The rejection denylist misses common Jira rejection names, so the completed/split fallback can close an issue as "Won't Fix"
The fallback added in 20bebaa ranks a transition as positive when its name OR its target status (.to.name) matches the tier-1 or tier-2 positive regex (lines 458-459). Rejections are excluded only by a fixed denylist: won.?t do|not.?planned|duplicate|obsolete|reject|cancel|invalid|cannot reproduce|abandon. The same list appears at line 403 (bash grep) and lines 452-453 (jq).
Common Jira rejection names are not on it: "Won't Fix", "Won't Implement", "Declined", "Deferred", "Not a Bug" and "As Designed".
- Fallback: a transition named "Close Issue" (tier 2 matches
\bclose\bon.name), or one named "Closed", whose.to.nameis "Won't Fix", passes the filter and can be picked. Acompletedorsplitaction then closes the issue as rejected, whichdocs/triage.mdsays is excluded. - Pre-flight (line 403): an issue already in "Won't Fix" or "Declined" gets
is_rejection_status=0, socompletedskips with "already in status ... (done)" and never moves the issue to a positive Done status.
The earlier Qodo comment covered the older last-resort [0] fallback. This one is about the tier-2 .name/.to.name matching added in 20bebaa.
Suggestion
- Do not accept a match on
.namewhen.to.nameis present and not itself positive. For tier 1, match only exact positive.to.namevalues. - Add
won.?t (fix|implement)|declin|defer|not a bug|as designed|out of scopeto the denylist. - Move the regex into one shared variable and pass it to jq with
--arg, so the bash and jq copies cannot drift apart. - Change
jira-completed-fallback-doneto assert the POST body ({"transition":{"id":"51"}}) instead of only the warning text. Add a mock with a "Close Issue" -> "Won't Fix" transition next to Done.
For a more robust check, read the resolution field (fields=status,resolution) in the pre-flight.
There was a problem hiding this comment.
Addressed in f7716a7b (f7716a7b5536d3b2c09b37841c1360551c9508e1):
- Shared rejection regex (
rejection_re) & expanded terms: Extracted the rejection pattern into one sharedrejection_revariable in_jira_transition_issue(used by pre-flightgrep -qiEand passed tojqvia--arg reject_re), addingwon.?t (do|fix|implement),declin,defer,not a bug,as designed, andout of scope. - Require positive
.to.namewhen present: Updated tier-1 and tier-2 fallback selection so when.to.nameis non-empty,.to.nameitself must satisfy the positive pattern (falling back to.nameonly when.to.nameis empty), preventing transitions likeClose Issue->Won't Fixfrom matching on.name. - Bundled scripts & strengthened tests: Rebuilt all bundled scripts (
make -B script-build), verified zero diff withmake check-bundle, passedshellcheck -S warning,post-triage-test.sh(bothsourceandbundled),pre-triage-test.sh, andpre-code-jira-test.sh. Updatedjira-completed-fallback-doneto assert the{"transition":{"id":"51"}}POST payload + warning, and addedjira-completed-fallback-skips-close-issue-wont-fix.
…et status Signed-off-by: Jeffery Bagirimvano <jefferyb@redhat.com>
| fi | ||
| fi | ||
|
|
||
| if [[ -z "${transition_id}" ]]; then |
There was a problem hiding this comment.
MEDIUM: The Done-category fallback POSTs a transition the operator never configured, and sends no resolution
When the configured transition is missing for completed/split, the fallback (lines 437-471) now picks a heuristic transition. One example is classic "Resolve Issue" -> "Resolved", which the jira-completed-fallback-resolve-issue test models. The code then POSTs only {transition:{id}} here. It does not check hasScreen or expand=transitions.fields, and it does not set fields.resolution.
In company-managed or classic workflows, the transition screen supplies Resolution. Through REST, such a transition either fails validation (400) or leaves the issue Done but "Unresolved". Before this PR, the code only POSTed a transition the operator had chosen. The fallback now does the same with transitions nobody vetted. fullsend-ai/fullsend#6704, which this PR links, lists supplying a resolution as part of the transition as a requirement.
The pre-flight has the same blind spot. It requests fields=status only (line 392), so a "Closed" issue with resolution "Won't Fix" still counts as already completed. An earlier comment mentioned reading the resolution field only as an optional extra. The new part here is the fallback POST.
Suggestion
- In the fallback, prefer candidates with
hasScreen == false. - Otherwise fetch
?expand=transitions.fields. If the screen has a Resolution field, setfields.resolution(for example "Done" or "Fixed"). If not, skip that candidate and fail loudly. - Add a mock test for a candidate whose screen requires a resolution.
- Optional: request
fields=status,resolutionin the pre-flight and treat a rejection-class resolution like a rejection status.
Summary
When the triage agent marks an issue with
action: completed,post-triage.shpreviously reusedJIRA_SPLIT_TRANSITION(defaulting to"Obsolete"per ADR 0080/0081) to transition the issue in Jira Cloud.In standard Jira Cloud and Team-Managed software projects,
"Obsolete"is not part of the active workflow. When the transition name is missing from the issue's available transitions,tracker_close_issuefails andpost-triage.shterminates withexit status 1underset -e.This PR:
action: splitfromaction: completedinscripts/post-triage.src.sh, mappingsplitto GitHub reason"completed"ingithub-triage-ops.lib.shwhile allowingJIRA_COMPLETED_TRANSITIONin Jira (falling back throughJIRA_SPLIT_TRANSITIONwhen unset so existing overlays and custom workflows are preserved).scripts/lib/jira-triage-ops.lib.sh:completedandsplit, skips idempotently if the current Jira status already matches the configured transition name (or its.to.nametarget status on available transitions) or is a positivestatusCategory == "done"status (excluding rejection statuses unless matched intentionally).duplicateandnot planned, skips idempotently only when the current status matches the configured transition name or its.to.nametarget status.completed/split(emitting a::warning::annotation when substituting) to availableto.statusCategory.key == "done"transitions whose target status (.to.name // .name) matches positive Done names (preferring exact matches for"Done","Closed","Resolved","Complete","Completed"before word-boundary matches), while excluding rejection names on both.nameand.to.name.make script-build.Related: fullsend-ai/fullsend#6704
Changes
scripts/post-triage.src.sh: Updatedaction: splitto pass"split"instead of"completed"totracker_close_issue.scripts/lib/github-triage-ops.lib.sh: Mapped close reason"split"to GitHub state reason"completed".scripts/lib/jira-triage-ops.lib.sh:JIRA_COMPLETED_TRANSITIONoverride (no default inharness/triage.yaml; falls back toJIRA_SPLIT_TRANSITIONwhen unset).completed/splitand exact target-status idempotency forduplicate/not planned.to.statusCategory.key == "done"fallback matching(.to.name // .name)with::warning::annotation and rejection exclusions on both.nameand.to.name.docs/triage.md&LOCAL.md: DocumentedJIRA_COMPLETED_TRANSITIONand fallback/idempotency behavior.scripts/post-triage-test.sh: Added unit tests using mocked Jira REST API responses inpost-triage-test.sh(not tested against a live Jira Cloud project).scripts/post-triage.sh,scripts/pre-code-jira.sh,scripts/pre-triage.sh.Testing
make script-buildandmake check-bundlepass cleanly.shellcheck --severity=warning scripts/lib/jira-triage-ops.lib.sh scripts/post-triage.src.sh scripts/post-triage.sh scripts/pre-code-jira.sh scripts/pre-triage.sh scripts/post-triage-test.shpasses cleanly.SCRIPT_TEST_TARGET=source bash scripts/post-triage-test.shandSCRIPT_TEST_TARGET=bundled bash scripts/post-triage-test.shpass all tests (mocked Jira workflow responses).