Skip to content

feat(jira): add --jira-trailer flag to extract Jira issue key from git trailer - #1109

Open
vidhu-balad wants to merge 2 commits into
mainfrom
feat/jira-trailer-flag
Open

feat(jira): add --jira-trailer flag to extract Jira issue key from git trailer#1109
vidhu-balad wants to merge 2 commits into
mainfrom
feat/jira-trailer-flag

Conversation

@vidhu-balad

Copy link
Copy Markdown
Contributor

Summary

  • Adds a --jira-trailer <key> flag to kosli attest jira
  • When set, the command reads only lines of the form <key>: <value> from the commit message and uses those values as the sole source of Jira issue references
  • The full commit message body and branch name scan are skipped entirely, eliminating false positives from other trailers (e.g. Ona-Environment-Id: ONA-456) whose values happen to match the Jira key pattern
  • Key match is case-insensitive; multiple occurrences of the same trailer key are supported
  • All existing behaviour is unchanged for users who do not set the flag

Changes

  • internal/gitview/gitView.go — new GetTrailerValues(message, key string) []string function
  • internal/gitview/gitView_test.go — 6 unit tests covering no match, single match, case-insensitive key, multiple occurrences, non-matching trailers ignored, whitespace trimming
  • cmd/kosli/root.gojiraTrailerFlag constant
  • cmd/kosli/attestJira.go--jira-trailer flag wired into the issue-finding logic
  • cmd/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 flag
  • go test ./internal/gitview/... -run TestGitViewTestSuite/TestGetTrailerValues — unit tests for GetTrailerValues
  • make lint — passes with 0 issues

🤖 Generated with Claude Code

…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

claude Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Claude finished @vidhu-balad's task in 3m 36s —— View job


Review — PR #1109 --jira-trailer

  • Gather context (diff, CLAUDE.md, related code)
  • Phase 1: static analysis
  • Phase 2: code review
  • Post findings

Phase 1 — static analysis

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

  1. --assert error 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

  1. 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 not ONA-999 leaks in. Adding --assert makes it fail in both directions.
  2. --jira-trailer silently no-ops --jira-secondary-source and --ignore-branch-match (attestJira.go:309) — a MuXRequiredFlags(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.
  3. Trailer values still go through the Jira key regex with no warningJira: EX1 or Jira: 1234 produces 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 a logger.Warn when len(trailerValues) > 0 && len(issueIDs) == 0.
  4. Prefix match is stricter than commit messages in the wild (gitView.go:327) — " Jira: EX-1" doesn't match (no TrimSpace on the line), and --jira-trailer "Jira:" builds the prefix jira:: and silently never matches.
  5. GetTrailerValues doesn'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 assume git interpret-trailers behaviour.
  6. Long description 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-match without noting --jira-trailer supersedes it, and lines 60–63 still recommend --jira-secondary-source as the workaround for the CVE--style project-key collision when --jira-trailer is now the better answer. CLAUDE.md's slice checklist calls out --help explicitly, and Long is part of it. An attestJiraExample entry 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-456 false-positive class — better than piling more exclusion heuristics onto FindJiraIssueKeys.
  • GetTrailerValues is pure, dependency-free and lives outside the GitView receiver, 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 than nil is 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:172 is 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

Comment on lines +334 to +344
{
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",
},
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Suggested change
{
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",
},
},

Comment on lines +325 to +327
for _, line := range strings.Split(message, "\n") {
if strings.HasPrefix(strings.ToLower(line), prefix) {
value := strings.TrimSpace(line[len(prefix):])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Two edge cases in the prefix match:

  1. Leading whitespace is not tolerated. HasPrefix runs on the raw line, so " Jira: EX-1" does not match. Commit messages written through editors/templates or pasted from git log output (which indents by 4 spaces) will silently produce no matches.
  2. 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):

Suggested change
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), ":")) + ":".

Comment thread cmd/kosli/attestJira.go
Comment on lines +306 to +313
// 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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

--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.

Comment on lines +319 to +321
// 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread cmd/kosli/root.go
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."

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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-match but not that --jira-trailer supersedes it.
  • Lines 60–63 recommend --jira-secondary-source as the workaround for the CVE--style project-key collision — --jira-trailer is 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>
Comment thread cmd/kosli/attestJira.go
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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Fix this →

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant