chore: check JIRA vars before making api calls - #876
Conversation
This will also help with debugging when wiring up jira poller to dispatch. Signed-off-by: Ralph Bean <rbean@redhat.com>
PR Summary by QodoFail Fast on Missing Jira API Configuration
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
|
🤖 Finished Review · ✅ Success · Started 1:58 AM UTC · Completed 2:12 AM UTC Commit: |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
ReviewFindingsHigh
Low
Next steps:
Previous runReviewFindingsHigh
Next steps:
Previous run (2)ReviewFindingsHigh
Medium
Next steps:
Previous run (3)ReviewFindingsCritical
High
Medium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/fs-fix address the open findings, please |
|
🤖 Finished Fix · ✅ Success · Started 7:47 PM UTC · Completed 7:51 PM UTC Commit: |
- Fix JIRA_JIRA_BASE_URL typo → JIRA_BASE_URL in _jira_api guard - Redirect guard error messages to stderr (>&2) to match file idiom - Extract _jira_require_vars helper and call from both _jira_api and _jira_api_with_status so both API entry points validate env vars Addresses review feedback on #876
🔧 Fix agent — iteration 1 (human-triggered)Addressed all 3 review findings: fixed JIRA_JIRA_BASE_URL typo, redirected error messages to stderr, and added env var guards to _jira_api_with_status via a shared _jira_require_vars helper. Fixed (3):
Tests: passedNext steps:
|
|
🤖 Review · ❌ Terminated · Started 7:53 PM UTC · Ended 8:08 PM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only follow-up on the current head (9ac6268): two MEDIUM findings that are not covered by the existing threads.
Note: these anchor to lines outside this PR's diff hunks, so GitHub rejects them as inline comments; they are included here with explicit file:line references instead.
1. scripts/lib/jira-triage-ops.lib.sh:208
MEDIUM — Guard diagnostics are swallowed by 2>/dev/null at every redirecting call site — survives the exit-1 -> return-1 fix
Distinct from the already-posted exit-vs-return thread and from the already-fixed stdout->stderr issue. Even after _jira_require_vars was moved to >&2 (commit 9ac6268), five call sites redirect the helper's stderr to /dev/null, so the new diagnostic is written and immediately discarded:
tracker_remove_label— line 149tracker_strip_labels— line 156tracker_verify_labels_stripped— line 163tracker_list_repo_labels— line 208tracker_close_issue— line 281
This residue is not resolved by changing exit 1 to return 1 — the message is lost either way. Verified empirically with a reduced repro of the exact call shapes under set -euo pipefail:
(a) At the || true sites the script terminates with rc=1 and produces zero output — no ERROR line at all, which is the exact opposite of this PR's stated goal of helping debug jira-poller wiring.
(b) At batch=$(_jira_api GET "/label?..." 2>/dev/null) || break (line 208) the subshell dies, the message is discarded, break ends pagination, and tracker_list_repo_labels returns an empty list with rc=0. post-triage.src.sh:601 consumes that as EXISTING_LABELS, so label_exists() then reports every label as non-existent and the "will not auto-create labels" logic silently changes behavior with no diagnostic anywhere.
Confirmed that pre-triage.src.sh reaches tracker_strip_labels (line 35) as its first Jira operation, right after tracker_parse_issue_url (line 29).
Suggestion: Hoist the credential validation to script startup rather than per-call, using the idiom pre-triage.src.sh:18-20 already uses (: "${JIRA_TOKEN:?JIRA_TOKEN must be set}"), gated on the resolved FULLSEND_TRACKER being jira. That fails once, early, on an unredirected stderr, before any label mutation, and simultaneously resolves the exit-vs-return question already raised on this PR. If per-call guards are kept instead, drop the blanket 2>/dev/null at lines 149/156/163/208/281 so the credential error is visible, and make tracker_list_repo_labels distinguish "API failed" from "no labels" rather than returning empty with rc=0.
2. scripts/lib/jira-triage-ops.lib.sh:244
MEDIUM — tracker_post_comment / tracker_post_sticky_comment reach the Jira API with the same credentials but bypass the new guard
New location, not covered by the existing threads (qodo finding 5 and the review bot's file-level note both concerned _jira_api_with_status / tracker_create_issue, which the fix commit already addressed).
tracker_post_comment (line 228) and tracker_post_sticky_comment (line 250) also call the Jira Cloud API — via fullsend issues post-comment --tracker jira --jira-url "${JIRA_BASE_URL}" --jira-email "${JIRA_USER_EMAIL}" --token "${JIRA_TOKEN}" at lines 244-247 and 253-256 — and never call _jira_require_vars, so the PR title's claim ("check JIRA vars before making api calls") does not hold for these paths.
I verified against the fullsend CLI source that this path does self-validate (internal/cli/tracker_client.go:63/75/120 return --jira-url or JIRA_BASE_URL required for Jira tracker, --jira-email or JIRA_USER_EMAIL required for Jira tracker (Jira Cloud auth is email+token, not a bare token), --token or JIRA_TOKEN required for Jira tracker), which is why this is MEDIUM and not HIGH: the consequence is inconsistent, differently-formatted diagnostics arriving at a different point in the run for identical missing config, not a silent bad API call.
Suggestion: Cover this with the single up-front startup validation suggested above (validate once when the jira tracker is selected), which guards every Jira path — raw curl and CLI shell-out alike — instead of adding a second per-function guard. If per-call guards are kept, add _jira_require_vars || return 1 at the top of both comment functions for consistency.
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 5:56 PM UTC · Completed 6:05 PM UTC Commit: |
…funcs
Hoist JIRA_USER_EMAIL / JIRA_TOKEN validation to source time using the
: "${VAR:?msg}" idiom already established by pre-triage.src.sh and
post-triage.src.sh. This ensures the diagnostic fires once on
unredirected stderr before any call site can swallow it with
2>/dev/null — addressing the five call sites (lines 149/156/163/208/281)
where the per-call guard's message was previously discarded.
Narrow _jira_require_vars to only check JIRA_BASE_URL (which is derived
at runtime by tracker_parse_issue_url and cannot be validated at source
time), and change exit 1 to return 1 for correct subshell propagation.
Use _jira_require_vars || return 1 in all API entry points.
Add _jira_require_vars || return 1 to tracker_post_comment and
tracker_post_sticky_comment, which reach the Jira API via the fullsend
CLI and previously bypassed the credential guard entirely.
Addresses review feedback on #876
🔧 Fix agent — iteration 2 (human-triggered)Addressed both review findings. Hoisted credential validation to source time to prevent diagnostics from being swallowed by 2>/dev/null at call sites. Added JIRA_BASE_URL guard to comment functions that previously bypassed validation. All triage tests pass. Fixed (2):
Tests: passed Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 6:07 PM UTC · Completed 6:21 PM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Automated multi-agent review pass (independent parallel reviewers, findings cross-checked against the current head before posting — all three reviewers independently flagged the missing negative-path test coverage).
|
/fs-fix Add the test case as suggested by @waynesun09 |
|
🤖 Finished Fix · ✅ Success · Started 2:57 PM UTC · Completed 3:04 PM UTC Commit: |
Add test cases for unset and empty JIRA_TOKEN / JIRA_USER_EMAIL in both pre-triage-test.sh and post-triage-test.sh, verifying the source-time :? guards reject missing credentials before any API call is made. Addresses review feedback from @waynesun09 on #876.
🔧 Fix agent — iteration 3 (human-triggered)Addressed @waynesun09 review feedback: added 8 negative-path test cases (4 per test file) for the Jira credential guards introduced in this PR. Tests cover both unset and empty-string scenarios for JIRA_TOKEN and JIRA_USER_EMAIL, ensuring the source-time :? guards reject missing credentials before any API call is made. Fixed (1):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 3:06 PM UTC · Completed 3:23 PM UTC Commit: |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Approving at head e2ec81f.
Validated the credential-guard fix empirically: ran both pre-triage-test.sh and post-triage-test.sh from a clean worktree at this exact head — all 8 new negative-path cases pass and both suites report "All tests passed". Confirmed the source-time :? guards sit inside the jira) branch of the bundled scripts, so github/gitlab tracker runs are unaffected, and that :? (colon form) rejects both unset and empty values.
Remaining nit (non-blocking, tracked in the two open threads): the post-triage cases assert only a non-zero exit, while the pre-triage cases assert the exact message plus no-mutation. Note for whoever picks it up — passing an expected_pattern alone will not fix it: run_jira_test's expect_failure branch returns before the pattern grep, so the harness needs the check added inside that branch plus an expect_no_mutation arg to match run_test.
|
🤖 Finished Retro · ❌ Failure · Started 8:03 PM UTC · Completed 8:03 PM UTC Commit: |
2 participants
This will also help with debugging when wiring up jira poller to dispatch.