feat(jira): add --jira-trailer flag to extract Jira issue key from git trailer - #1109
feat(jira): add --jira-trailer flag to extract Jira issue key from git trailer#1109vidhu-balad wants to merge 2 commits into
Conversation
…iler When --jira-trailer <key> is set, the command reads lines of the form '<key>: <value>' from the commit message and uses those values as the sole source of Jira issue references, skipping the full commit message and branch name scan. This avoids false positives from other trailers (e.g. Ona-Environment-Id) whose values happen to match the Jira key pattern. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Claude finished @vidhu-balad's task in 3m 36s —— View job Review — PR #1109
|
| Check | Result |
|---|---|
go build ./... |
✅ clean |
go vet ./... |
✅ clean |
go mod tidy + git diff --exit-code go.mod go.sum |
✅ no drift |
I could not execute go test in this environment (the commands needed approval, and AttestJiraCommandTestSuite additionally needs a local Kosli server plus KOSLI_JIRA_API_TOKEN/KOSLI_JIRA_USERNAME). The test-plan boxes in the PR body are still unticked — please confirm make test_integration_single TARGET=AttestJiraCommandTestSuite and make lint pass locally.
What changed since the last review
The new commit (7f8d11d9) only registers jira-trailer in testdata/empty-flag-audit-coverage.json. That's the right and only registration needed — refuseEmptyFlagValues in root.go:485 already rejects --jira-trailer "" generically, so the mode switch o.trailerKey != "" can't be silently reached with an empty value. Good catch adding it rather than waiting for CI to complain.
All five findings from the previous review are still open — none of the source files changed. Summarised by severity:
New finding
--asserterror text is wrong in trailer mode (attestJira.go:374,:382) — both messages say "in commit message or branch name", the two sources trailer mode deliberately skips. Test 29 pins the wrong wording as golden. Inline comment
Still open from the previous review
- Test 27 doesn't test the headline behaviour (
attestJira_test.go:340) — its golden is byte-identical to test 28's (the no issue found case), so it passes whether or not the trailer is read, and whether or notONA-999leaks in. Adding--assertmakes it fail in both directions. --jira-trailersilently no-ops--jira-secondary-sourceand--ignore-branch-match(attestJira.go:309) — aMuXRequiredFlags(cmd, []string{"jira-trailer", "jira-secondary-source"}, false)matches how this file already handles the same shape (lines 220–233). A--jira-secondary-source ${{ github.head_ref }}becoming a no-op is a compliance-relevant silent change.- Trailer values still go through the Jira key regex with no warning —
Jira: EX1orJira: 1234produces a non-compliant attestation and zero output. In trailer mode the user has explicitly declared where the key lives, so a mismatch is almost certainly a typo; worth alogger.Warnwhenlen(trailerValues) > 0 && len(issueIDs) == 0. - Prefix match is stricter than commit messages in the wild (
gitView.go:327) —" Jira: EX-1"doesn't match (noTrimSpaceon the line), and--jira-trailer "Jira:"builds the prefixjira::and silently never matches. GetTrailerValuesdoesn't have git-trailer semantics (gitView.go:319) — it matches<key>:on any line, including the subject and prose in the body, not just the last paragraph. Harmless here, but the doc comment should say so or the next caller will assumegit interpret-trailersbehaviour.Longdescription not updated (attestJira.go:41–68) — still says the command parses "the given commit's message, current branch name or the content of--jira-secondary-source", still documents--ignore-branch-matchwithout noting--jira-trailersupersedes it, and lines 60–63 still recommend--jira-secondary-sourceas the workaround for theCVE--style project-key collision when--jira-traileris now the better answer.CLAUDE.md's slice checklist calls out--helpexplicitly, andLongis part of it. AnattestJiraExampleentry would help too.
What's good
- The core idea is right, and the flag is genuinely the cleanest fix for the
Ona-Environment-Id: ONA-456false-positive class — better than piling more exclusion heuristics ontoFindJiraIssueKeys. GetTrailerValuesis pure, dependency-free and lives outside theGitViewreceiver, so it's trivially unit-testable — and it is unit-tested, table-driven, six cases, including the exact false-positive scenario that motivated the feature. That's the right split of unit vs. integration coverage.- Returning
[]string{}rather thannilis a deliberate, documented choice and the tests assert it. - The refactor of the search-source block in
run()keeps the original path byte-for-byte intact, so the no-flag behaviour is provably unchanged. - Flag help text in
root.go:172is precise about the mode switch — it explicitly says the body and branch are not scanned, which is the non-obvious part.
Findings 1, 2 and 3 are the ones I'd want resolved before merge; 4–7 are polish.
· branch feat/jira-trailer-flag
| { | ||
| name: "27 can attest jira using --jira-trailer to extract issue key from commit trailer", | ||
| cmd: fmt.Sprintf(`attest jira --name bar | ||
| --jira-base-url https://kosli-test.atlassian.net | ||
| --jira-trailer Jira | ||
| --repo-root %s %s`, suite.tmpDir, suite.defaultKosliArguments), | ||
| golden: "jira attestation 'bar' is reported to trail: test-123\n", | ||
| additionalConfig: jiraTestsAdditionalConfig{ | ||
| commitMessage: "fix: some change\n\nJira: EX-1\nOna-Environment-Id: ONA-999", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Test 27 does not actually exercise the new behaviour. Its golden is byte-identical to test 28's, and test 28 is the no issue found case — the "reported to trail" line is printed regardless of how many issue IDs were resolved. So this test still passes if GetTrailerValues returns nothing, or if the whole commit message leaks through and ONA-999 is also resolved (a not-found issue produces no output difference either).
Adding --assert makes the assertion meaningful in both directions: it fails if no reference is found (trailer not read) and if ONA-999 leaks in (issueFoundCount != len(issueIDs) → error). That's the actual headline claim of the PR, and nothing currently covers it.
| { | |
| name: "27 can attest jira using --jira-trailer to extract issue key from commit trailer", | |
| cmd: fmt.Sprintf(`attest jira --name bar | |
| --jira-base-url https://kosli-test.atlassian.net | |
| --jira-trailer Jira | |
| --repo-root %s %s`, suite.tmpDir, suite.defaultKosliArguments), | |
| golden: "jira attestation 'bar' is reported to trail: test-123\n", | |
| additionalConfig: jiraTestsAdditionalConfig{ | |
| commitMessage: "fix: some change\n\nJira: EX-1\nOna-Environment-Id: ONA-999", | |
| }, | |
| }, | |
| { | |
| name: "27 can attest jira using --jira-trailer to extract issue key from commit trailer", | |
| cmd: fmt.Sprintf(`attest jira --name bar | |
| --jira-base-url https://kosli-test.atlassian.net | |
| --jira-trailer Jira | |
| --assert | |
| --repo-root %s %s`, suite.tmpDir, suite.defaultKosliArguments), | |
| golden: "jira attestation 'bar' is reported to trail: test-123\n", | |
| additionalConfig: jiraTestsAdditionalConfig{ | |
| commitMessage: "fix: some change\n\nJira: EX-1\nOna-Environment-Id: ONA-999", | |
| }, | |
| }, |
| for _, line := range strings.Split(message, "\n") { | ||
| if strings.HasPrefix(strings.ToLower(line), prefix) { | ||
| value := strings.TrimSpace(line[len(prefix):]) |
There was a problem hiding this comment.
Two edge cases in the prefix match:
- Leading whitespace is not tolerated.
HasPrefixruns on the raw line, so" Jira: EX-1"does not match. Commit messages written through editors/templates or pasted fromgit logoutput (which indents by 4 spaces) will silently produce no matches. - A key supplied with a trailing colon breaks silently.
--jira-trailer "Jira:"builds the prefix"jira::", which never matches, with no error or warning.
Trimming both sides handles (1) cheaply and keeps everything else identical (TrimSpace on the value already covers \r from CRLF messages):
| for _, line := range strings.Split(message, "\n") { | |
| if strings.HasPrefix(strings.ToLower(line), prefix) { | |
| value := strings.TrimSpace(line[len(prefix):]) | |
| for _, line := range strings.Split(message, "\n") { | |
| line = strings.TrimSpace(line) | |
| if strings.HasPrefix(strings.ToLower(line), prefix) { | |
| value := strings.TrimSpace(line[len(prefix):]) |
For (2), consider prefix := strings.ToLower(strings.TrimSuffix(strings.TrimSpace(key), ":")) + ":".
| // Find Jira issue keys either from a named git trailer or by scanning the | ||
| // commit message, branch name, and secondary source. | ||
| var issueIDs []string | ||
| if o.trailerKey != "" { | ||
| trailerValues := gitview.GetTrailerValues(commitInfo.Message, o.trailerKey) | ||
| combinedTrailerText := strings.Join(trailerValues, "\n") | ||
| issueIDs = jira.FindJiraIssueKeys(combinedTrailerText, o.projectKeys) | ||
| logger.Debug("Checked for Jira issue references in trailer '%s' of Git commit %s: %v", o.trailerKey, commitInfo.Sha1, trailerValues) |
There was a problem hiding this comment.
--jira-trailer silently disables --jira-secondary-source and --ignore-branch-match. Both are accepted without complaint and then have no effect, which is easy to get wrong in a CI pipeline (--jira-secondary-source ${{ github.head_ref }} quietly becoming a no-op is a compliance-relevant silent change). This file already uses the repo's helper for exactly this shape of problem (lines 220–233), so:
err = MuXRequiredFlags(cmd, []string{"jira-trailer", "jira-secondary-source"}, false)
if err != nil {
return err
}For --ignore-branch-match a logger.Warn in run() would be enough, since it's already implied by the trailer mode.
Separately: trailer values are still fed through jira.FindJiraIssueKeys, so Jira: EX1 or Jira: 1234 yields nothing at all — no warning, just a non-compliant attestation. Worth a logger.Warn when len(trailerValues) > 0 && len(issueIDs) == 0, since in trailer mode the user has explicitly declared where the key lives and a mismatch is almost certainly a mistake rather than an absent reference.
| // GetTrailerValues extracts the values of all trailer lines in a commit message | ||
| // that match the given key. The key comparison is case-insensitive. Trailer lines | ||
| // have the format "<key>: <value>". Returns an empty (non-nil) slice if none are found. |
There was a problem hiding this comment.
Naming/semantics nit worth resolving before this ships, since the name sets an expectation the implementation doesn't meet: real git trailers (per git interpret-trailers) live only in the last paragraph of the message. This function matches <key>: on any line, including the subject and prose in the body — so fix: EX-1 handled\n\nJira: ask the team which ticket applies would treat the prose line as a trailer value.
For the current use case that leniency is harmless (the value goes through the Jira key regex anyway), but the doc comment should say so explicitly rather than calling them "trailer lines", e.g. "matches any line of the form <key>: <value> anywhere in the message, not only trailers in the final paragraph". Otherwise the next caller will reasonably assume git trailer semantics.
| jiraIssueFieldFlag = "[optional] The comma separated list of fields to include from the Jira issue. Default no fields are included. '*all' will give all fields." | ||
| jiraSecondarySourceFlag = "[optional] An optional string to search for Jira ticket reference, e.g. '--jira-secondary-source ${{ github.head_ref }}'" | ||
| ignoreBranchMatchFlag = "Ignore branch name when searching for Jira ticket reference." | ||
| jiraTrailerFlag = "[optional] The git trailer key to use as the sole source of Jira issue references (e.g. '--jira-trailer Jira' extracts the value of 'Jira: <issue-key>' lines from the commit message). When set, the commit message body and branch name are not scanned." |
There was a problem hiding this comment.
Flag help is clear and accurate. The gap is the command's Long description in cmd/kosli/attestJira.go:41–68, which documents the search sources in prose and wasn't updated:
- Line 41 still says the command "Parses the given commit's message, current branch name or the content of
--jira-secondary-source" with no mention of trailer mode. - Line 68 documents
--ignore-branch-matchbut not that--jira-trailersupersedes it. - Lines 60–63 recommend
--jira-secondary-sourceas the workaround for theCVE--style project-key collision —--jira-traileris now the better answer to that exact problem and should be mentioned there.
Per the slice checklist in CLAUDE.md ("Does kosli <command> --help reflect the change?"), the prose in Long is part of --help. An attestJiraExample entry would help too, since every other non-obvious flag has one.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| issueIDs := jira.FindJiraIssueKeys(combinedText, o.projectKeys) | ||
| logger.Debug("Checked for Jira issue references in Git commit %s on branch %s commit message:\n%s", commitInfo.Sha1, commitInfo.Branch, commitInfo.Message) | ||
| logger.Debug("the following Jira references are found in commit message or branch name: %v", issueIDs) | ||
| logger.Debug("the following Jira references are found: %v", issueIDs) |
There was a problem hiding this comment.
The --assert failure messages still say "commit message or branch name", which is exactly what trailer mode does not read.
Both assert paths below hardcode the old wording:
attestJira.go:374—"no Jira references are found in commit message or branch name"attestJira.go:382—"missing Jira issues from references found in commit message or branch name"
With --jira-trailer Jira the commit body and branch were never scanned, so a user who hits the first error is told to look in two places the command deliberately ignored. The actual cause is "the commit has no Jira: trailer" (or the trailer value didn't match the Jira key pattern) — a materially different fix on the user's side, and this is the one message they get in a failing CI job.
Test 29 pins the wrong wording as golden (attestJira_test.go:364), so the wording is now covered by a test asserting it, which makes it harder to notice later.
Threading the source through both messages keeps them accurate in either mode:
searchedIn := "commit message or branch name"
if o.trailerKey != "" {
searchedIn = fmt.Sprintf("the '%s' trailer of the commit message", o.trailerKey)
}then fmt.Errorf("%sno Jira references are found in %s", errString, searchedIn) and fmt.Errorf("%smissing Jira issues from references found in %s%s", errString, searchedIn, issueLog), with test 29's golden updated to match.
Summary
--jira-trailer <key>flag tokosli attest jira<key>: <value>from the commit message and uses those values as the sole source of Jira issue referencesOna-Environment-Id: ONA-456) whose values happen to match the Jira key patternChanges
internal/gitview/gitView.go— newGetTrailerValues(message, key string) []stringfunctioninternal/gitview/gitView_test.go— 6 unit tests covering no match, single match, case-insensitive key, multiple occurrences, non-matching trailers ignored, whitespace trimmingcmd/kosli/root.go—jiraTrailerFlagconstantcmd/kosli/attestJira.go—--jira-trailerflag wired into the issue-finding logiccmd/kosli/attestJira_test.go— 3 integration tests: trailer used successfully, trailer absent (non-compliant but reported), trailer absent with--assert(error)Test plan
make test_integration_single TARGET=AttestJiraCommandTestSuite— tests 27, 28, 29 cover the new flaggo test ./internal/gitview/... -run TestGitViewTestSuite/TestGetTrailerValues— unit tests forGetTrailerValuesmake lint— passes with 0 issues🤖 Generated with Claude Code