fix(jira): differentiate rejected token from a missing issue - #1125
Conversation
|
Claude finished @mbevc1's task in 5m 21s —— View job Review:
|
| check | result |
|---|---|
go build ./... |
✅ clean |
go vet ./... |
✅ clean |
go mod tidy + git diff --exit-code go.mod go.sum |
✅ no drift |
go test ./... |
go test is blocked by this runner's sandbox. Findings below are from reading the code, not from a run. |
.golangci.yml only enables forbidigo (with binary.NativeEndian / unsafe.Pointer patterns); nothing in this diff touches either.
Findings
Two inline comments, both carried over from earlier rounds and still open. Neither blocks.
-
internal/jira/jira.go:96— theurl.Parsefailure is the last path returning a nil result. Every other path now honours "always returns a result, andLookupUnverifiedalways carries a reason" —NewJiraClientwas fixed at line 108, both abort branches setLookupReason, andTestGetJiraIssueInfoassertsrequire.NotNilon every case. Consistency, not a live bug:attestJira.go:319returns on the error first. -
internal/jira/jira.go:45— nothing pins thejson:"-"tags. This is the PR's only API-compatibility claim and the only one without a test. There is nojson.Marshalcall anywhere ininternal/jira/jira_test.go; dropping either tag would shipLookupStatusand the fullLookupReason— which embeds the username and up to 200 runes of Jira's body — in every attestation, with the suite still green.
One design note, for a follow-up rather than this PR. The CLI output no longer misleads, but the attestation still does: with an expired token, Kosli records issue_exists: false for every issue and, because the classification is json:"-", the platform has no way to tell that apart from a genuinely missing issue. The permanent audit record is the thing an expired token now silently falsifies. Preserving the payload is clearly the right call for a fix PR — worth an issue for adding the field to the API contract later.
Verified as correct
I re-derived the pieces most likely to be subtly wrong and they hold:
- The abort set really is unchanged. Old guard:
err != nil && response != nil && status != 404. New: 404 (either classification) → no error;response == nil→ no error (old code fell through toissue != niland returnedniltoo); 2xx-undecodable and every other status → error. Same set, so every exit code and abort path stands, as the description claims. errorDetail's trimming matches go-jira v1.17.0 exactly. I readerror.goin the module cache:NewJiraErrorrenders the generic sentence bare (parsed JSON with no messages, line 61), as a suffix behind"<status>: <body>: "(line 44), and as a prefix in front ofcould not parse JSON(lines 37-38). Trimming without the separator from both ends covers all three, and the deepest-cause walk handles the unmarshal reason. The(*Error).Error()return-inside-rangeat line 57-59 is real, so readingjiraErr.Errorswith sorted keys is both the complete and the deterministic fix.credentialsRejectedis safe againstnilheaders (http.Header(nil).Getreturns""), andlogLookupguardsnilon both the logger and the response.logger.Warn("%s", …)— the reason is passed as an argument, not as the format string, so a%in a Jira response body can't corrupt the warning.
Worth acknowledging
The test suite is the strongest part of this. wantErrExact pinning whole messages (rather than Contains, which is what let two earlier bugs through), the two properties asserted as properties — TestGetJiraIssueInfoTransportReasonIsIssueIndependent pinning the invariant the dedup depends on, and the 404 carrying neither header case pinning "absence is not evidence" — and TestAttestJiraAssertNamesUnconfirmedIssues deliberately reaching past the !global.DryRun guard with NotContains(errOut, "missing Jira issues") to catch a swapped argument. The NotContains(errOut, "secret") assertion on the warning is a good instinct too.
lookupError is the right call over %w: it keeps errors.Is/As working without letting the wrapped text undo the flattening, and TestGetJiraIssueInfoErrorUnwraps pins both halves.
• branch 20260821_jira_msgs
21bfc1b to
2bd6de4
Compare
0b850d6 to
c55fcc2
Compare
c55fcc2 to
e6c82c5
Compare
e6c82c5 to
7400731
Compare
7400731 to
71822e4
Compare
Jira answers 404 both for an issue that does not exist and for one the caller may not view, and a caller whose credentials Jira rejected may not view any issue. GetJiraIssueInfo looked only at the status code, so an expired Atlassian API token came back as IssueExists=false with no error and was reported as a missing issue - sending users to look for a ticket instead of renewing a token. A transport failure reached the same outcome through the response==nil case, because the guard it fell through required a response to be present. Classify the outcome in LookupStatus, with LookupReason carrying a message for the caller to report, and reclassify a 404 only when the response carries positive evidence that the credentials were rejected. That evidence is Seraph-era, so its absence proves nothing and leaves the lookup classified exactly as it was. Behaviour is unchanged throughout: an error is returned in exactly the cases it was before, so every abort path and exit code stands, and the two new fields carry json:"-" so the attestation payload is what the API accepts today. An unconfirmed lookup still counts as not found, so compliance and --assert are untouched - only what the user reads changes. kosli attest jira warns once per run rather than once per issue, naming every affected issue and each distinct cause, and the --assert failure names unconfirmed issues in its headline instead of announcing missing ones. The messages themselves are built from what Jira actually said: - go-jira's generic "request failed ... Status code: N" sentence is trimmed in all three of its renderings, along with the status it repeats, so a message that has nothing to add ends at the status it already named. - every field in an errors body is quoted, in a stable order, rather than letting go-jira return one arbitrary entry from inside a range. - a 2xx that cannot be decoded names the credentials when the response says they were not accepted, which is what an SSO login page reached through a redirect looks like. - a failure that produced no response reports the cause inside the *url.Error rather than its text, which embeds the per-issue URL and would give one run-level cause a different reason per issue. The status and the two headers are logged at debug level, which is what a run against a genuinely expired token needs to show whether Jira Cloud still sends them. If it does not, the remaining step is a lazy /rest/api/2/myself probe; TODO.md records it. The tests run against a fake Jira, and a fake Kosli host where the assert path needs one, so they need neither Jira credentials nor a local server.
71822e4 to
9b773cd
Compare
Jira answers 404 both for an issue that does not exist and for one the
caller may not view, and a caller whose credentials Jira rejected may not
view any issue. GetJiraIssueInfo looked only at the status code, so an
expired Atlassian API token came back as IssueExists=false with no error
and was reported as a missing issue - sending users to look for a ticket
instead of renewing a token. A transport failure reached the same outcome
through the response==nil case, because the guard it fell through required
a response to be present.
Classify the outcome in LookupStatus, with LookupReason carrying a message
for the caller to report, and reclassify a 404 only when the response
carries positive evidence that the credentials were rejected. That
evidence is Seraph-era, so its absence proves nothing and leaves the
lookup classified exactly as it was.
Behaviour is unchanged throughout: an error is returned in exactly the
cases it was before, so every abort path and exit code stands, and the two
new fields carry json:"-" so the attestation payload is what the API
accepts today. An unconfirmed lookup still counts as not found, so
compliance and --assert are untouched - only what the user reads changes.
kosli attest jira warns once per run rather than once per issue, naming
every affected issue and each distinct cause, and the --assert failure
names unconfirmed issues in its headline instead of announcing missing
ones.
The messages themselves are built from what Jira actually said:
trimmed in all three of its renderings, along with the status it
repeats, so a message that has nothing to add ends at the status it
already named.
letting go-jira return one arbitrary entry from inside a range.
says they were not accepted, which is what an SSO login page reached
through a redirect looks like.
*url.Error rather than its text, which embeds the per-issue URL and
would give one run-level cause a different reason per issue.
The status and the two headers are logged at debug level, which is what a
run against a genuinely expired token needs to show whether Jira Cloud
still sends them.
Follow-up from #1118 troubleshooting issues
Also adjusts GHA label filter.
Checklist
charts/k8s-reporter/) updated, if needed. Note: these changes live in a separate PR