From 4424e2994de7dccb0b287b51cc01344dbfd165f5 Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Thu, 20 Aug 2026 22:47:17 +0100 Subject: [PATCH 1/4] fix(attest jira): accept `--jira-project-key` lists written with spaces `--jira-project-key` is a `StringSliceVar`, and `pflag` splits those with encoding/csv, which never trims. So a list written the way most people type one arrived with the whitespace attached: `--jira-project-key "ABC, DEF" -> {"ABC", " DEF"}` validateJiraProjectKeys then rejected `" DEF"`, because a leading space fails `^[A-Za-z][A-Za-z0-9_]{1,9}$`, and the command exited non-zero on input a user reasonably expects to work. Verified against a binary built before this change: `--jira-project-key "ABC, DEF" Error: invalid Jira project keys: [ DEF]` `--jira-project-key " ABC" Error: invalid Jira project keys: [ ABC]` Trim the keys in place before validating, so one canonical key reaches both the validator and `jira.FindJiraIssueKeys` instead of each trimming separately and having to agree. internal/jira already trims the keys it interpolates; that now becomes belt-and-braces rather than the only thing standing between a padded key and a pattern matching `" ABC-123"`. Two neighbouring behaviours are deliberately left alone. A trailing comma, as in `--jira-project-key "ABC,"`, is already refused while pflag is still parsing, by the empty-element rule in `nonEmptyValue.go`, with clearer wording than this validator could give it. A whitespace-only element slips past that rule, since it is not empty, so `"ABC, "` still fails here - now displayed as an empty key rather than a space. Teaching the repo-wide empty-value rule to treat whitespace as empty would change all 165 flags and wants its own audit. Closes #1115 --- cmd/kosli/attestJira.go | 12 ++++++ cmd/kosli/attestJira_test.go | 78 ++++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+) diff --git a/cmd/kosli/attestJira.go b/cmd/kosli/attestJira.go index c0c10f19e..c1cd78cb6 100644 --- a/cmd/kosli/attestJira.go +++ b/cmd/kosli/attestJira.go @@ -287,6 +287,7 @@ func (o *attestJiraOptions) run(args []string) error { o.payload.JiraResults = []*jira.JiraIssueInfo{} + o.normaliseJiraProjectKeys() err = o.validateJiraProjectKeys() if err != nil { return err @@ -388,6 +389,17 @@ func jiraSearchText(commitInfo *gitview.CommitInfo, secondarySource string, igno // But Jira itself will accept lower case letters when searching a repository for matching branches and commits. var jiraProjectKeyRegexp = regexp.MustCompile("^[A-Za-z][A-Za-z0-9_]{1,9}$") +// normaliseJiraProjectKeys trims each project key in place. cobra splits a comma-separated +// list with encoding/csv, which does not trim, so --jira-project-key "ABC, DEF" arrives as +// {"ABC", " DEF"} and the untrimmed fragment fails validation. Normalising once here means a +// single canonical key reaches both validateJiraProjectKeys and jira.FindJiraIssueKeys, +// rather than each trimming separately and having to agree. +func (o *attestJiraOptions) normaliseJiraProjectKeys() { + for i, projectKey := range o.projectKeys { + o.projectKeys[i] = strings.TrimSpace(projectKey) + } +} + func (o *attestJiraOptions) validateJiraProjectKeys() error { invalidKeys := []string{} for _, projectKey := range o.projectKeys { diff --git a/cmd/kosli/attestJira_test.go b/cmd/kosli/attestJira_test.go index 0fd926802..de8900200 100644 --- a/cmd/kosli/attestJira_test.go +++ b/cmd/kosli/attestJira_test.go @@ -260,6 +260,30 @@ func (suite *AttestJiraCommandTestSuite) TestAttestJiraCmd() { commitMessage: "low-1 test commit", }, }, + { + // cobra splits the list with encoding/csv, which does not trim, so without + // normalisation the " ABC" fragment fails validation + name: "20b can specify jira project keys as a comma-separated list with spaces", + cmd: fmt.Sprintf(`attest jira --name bar + --jira-base-url https://kosli-test.atlassian.net + --jira-project-key "EX, ABC" + --repo-root %s %s`, suite.tmpDir, suite.defaultKosliArguments), + golden: "jira attestation 'bar' is reported to trail: test-123\n", + additionalConfig: jiraTestsAdditionalConfig{ + commitMessage: "EX-1 test commit", + }, + }, + { + name: "20c a jira project key padded with spaces is accepted", + cmd: fmt.Sprintf(`attest jira --name bar + --jira-base-url https://kosli-test.atlassian.net + --jira-project-key " EX " + --repo-root %s %s`, suite.tmpDir, suite.defaultKosliArguments), + golden: "jira attestation 'bar' is reported to trail: test-123\n", + additionalConfig: jiraTestsAdditionalConfig{ + commitMessage: "EX-1 test commit", + }, + }, { wantError: true, name: "21 fails with an invalid Jira project key specified", @@ -397,6 +421,60 @@ func TestJiraSearchText(t *testing.T) { } } +func TestNormaliseJiraProjectKeys(t *testing.T) { + for _, tc := range []struct { + name string + projectKeys []string + want []string + }{ + { + // cobra splits on the comma without trimming, so this is what + // --jira-project-key "ABC, DEF" actually delivers + name: "a space after the comma is trimmed", + projectKeys: []string{"ABC", " DEF"}, + want: []string{"ABC", "DEF"}, + }, + { + name: "space on both sides is trimmed", + projectKeys: []string{" EX "}, + want: []string{"EX"}, + }, + { + name: "tabs and newlines are trimmed", + projectKeys: []string{"\tEX", "ABC\n"}, + want: []string{"EX", "ABC"}, + }, + { + name: "keys that need no trimming are left alone", + projectKeys: []string{"ABC", "low", "A_99"}, + want: []string{"ABC", "low", "A_99"}, + }, + { + // a trailing comma yields an empty fragment, which stays empty so that + // validateJiraProjectKeys still rejects it + name: "an empty key stays empty", + projectKeys: []string{"ABC", ""}, + want: []string{"ABC", ""}, + }, + { + name: "a whitespace-only key becomes empty and is still rejected downstream", + projectKeys: []string{"ABC", " "}, + want: []string{"ABC", ""}, + }, + { + name: "no keys is left alone", + projectKeys: []string{}, + want: []string{}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + o := &attestJiraOptions{projectKeys: tc.projectKeys} + o.normaliseJiraProjectKeys() + require.Equal(t, tc.want, o.projectKeys) + }) + } +} + // In order for 'go test' to run this suite, we need to create // a normal test function and pass our suite to suite.Run func TestAttestJiraCommandTestSuite(t *testing.T) { From d7594dc61a2dbe2dcf42a615bb93bad92b08ebbd Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Thu, 20 Aug 2026 22:47:19 +0100 Subject: [PATCH 2/4] fix(attest jira): name the offending key, and normalise inside validation Review of the whitespace fix, three points. validateJiraProjectKeys now normalises the keys itself instead of relying on run() to have called normaliseJiraProjectKeys first. Nothing enforced that ordering, so a future caller reaching the validator directly got the untrimmed keys rejected again. Invalid keys are reported with %q rather than %v. A key that is only whitespace normalises to "", which %v renders as nothing at all, so --jira-project-key "ABC, " reported `invalid Jira project keys: []` - an error naming no key and reading as though nothing were wrong. It now reads `invalid Jira project keys: [""]`. Test 21's golden is quoted to match; it is the only assertion on this message in the suite. The two CLI cases gain --assert. Without it they only proved validation passed: the success line is printed whether or not any issue matched, so neither showed the trimmed key reaching the matcher. 20b's list is also reordered to "ABC, EX", putting the padded key in the position that has to match the EX-1 commit - as "EX, ABC" the clean first alternative matched either way and --assert would have proved nothing. Note that these cases cannot attribute a failure to this fix alone: jira.MakeJiraIssueKeyPattern trims its keys independently, so an untrimmed key would still match; what fails without this fix is validation, before matching is reached. --- cmd/kosli/attestJira.go | 15 ++++++--- cmd/kosli/attestJira_test.go | 61 +++++++++++++++++++++++++++++++++--- 2 files changed, 66 insertions(+), 10 deletions(-) diff --git a/cmd/kosli/attestJira.go b/cmd/kosli/attestJira.go index c1cd78cb6..b300c57f7 100644 --- a/cmd/kosli/attestJira.go +++ b/cmd/kosli/attestJira.go @@ -287,7 +287,6 @@ func (o *attestJiraOptions) run(args []string) error { o.payload.JiraResults = []*jira.JiraIssueInfo{} - o.normaliseJiraProjectKeys() err = o.validateJiraProjectKeys() if err != nil { return err @@ -391,16 +390,22 @@ var jiraProjectKeyRegexp = regexp.MustCompile("^[A-Za-z][A-Za-z0-9_]{1,9}$") // normaliseJiraProjectKeys trims each project key in place. cobra splits a comma-separated // list with encoding/csv, which does not trim, so --jira-project-key "ABC, DEF" arrives as -// {"ABC", " DEF"} and the untrimmed fragment fails validation. Normalising once here means a -// single canonical key reaches both validateJiraProjectKeys and jira.FindJiraIssueKeys, -// rather than each trimming separately and having to agree. +// {"ABC", " DEF"} and the untrimmed fragment fails validation. Normalising means a single +// canonical key reaches both the validation below and jira.FindJiraIssueKeys, rather than +// each trimming separately and having to agree. func (o *attestJiraOptions) normaliseJiraProjectKeys() { for i, projectKey := range o.projectKeys { o.projectKeys[i] = strings.TrimSpace(projectKey) } } +// validateJiraProjectKeys normalises the keys first, so that a caller cannot reach the +// validation without it and get the untrimmed keys rejected. +// +// Keys are reported with %q, because a key that is only whitespace normalises to "" and %v +// renders that as nothing at all, leaving an error naming no key. func (o *attestJiraOptions) validateJiraProjectKeys() error { + o.normaliseJiraProjectKeys() invalidKeys := []string{} for _, projectKey := range o.projectKeys { isValid := jiraProjectKeyRegexp.MatchString(projectKey) @@ -409,7 +414,7 @@ func (o *attestJiraOptions) validateJiraProjectKeys() error { } } if len(invalidKeys) > 0 { - return fmt.Errorf("invalid Jira project keys: %v", invalidKeys) + return fmt.Errorf("invalid Jira project keys: %q", invalidKeys) } return nil } diff --git a/cmd/kosli/attestJira_test.go b/cmd/kosli/attestJira_test.go index de8900200..915d0d46f 100644 --- a/cmd/kosli/attestJira_test.go +++ b/cmd/kosli/attestJira_test.go @@ -262,23 +262,30 @@ func (suite *AttestJiraCommandTestSuite) TestAttestJiraCmd() { }, { // cobra splits the list with encoding/csv, which does not trim, so without - // normalisation the " ABC" fragment fails validation + // normalisation the " EX" fragment fails validation. EX is deliberately the + // padded one: with --assert the case only passes if the key that has to match + // the EX-1 commit is the one that was trimmed. name: "20b can specify jira project keys as a comma-separated list with spaces", cmd: fmt.Sprintf(`attest jira --name bar --jira-base-url https://kosli-test.atlassian.net - --jira-project-key "EX, ABC" - --repo-root %s %s`, suite.tmpDir, suite.defaultKosliArguments), + --jira-project-key "ABC, EX" + --repo-root %s + --assert %s`, suite.tmpDir, suite.defaultKosliArguments), golden: "jira attestation 'bar' is reported to trail: test-123\n", additionalConfig: jiraTestsAdditionalConfig{ commitMessage: "EX-1 test commit", }, }, { + // --assert so this covers the whole path rather than validation alone: the + // success line above is printed either way, but a padded key that reached the + // matcher unusable would find no references and fail the assert. name: "20c a jira project key padded with spaces is accepted", cmd: fmt.Sprintf(`attest jira --name bar --jira-base-url https://kosli-test.atlassian.net --jira-project-key " EX " - --repo-root %s %s`, suite.tmpDir, suite.defaultKosliArguments), + --repo-root %s + --assert %s`, suite.tmpDir, suite.defaultKosliArguments), golden: "jira attestation 'bar' is reported to trail: test-123\n", additionalConfig: jiraTestsAdditionalConfig{ commitMessage: "EX-1 test commit", @@ -292,7 +299,7 @@ func (suite *AttestJiraCommandTestSuite) TestAttestJiraCmd() { --jira-project-key 1AB --jira-project-key AB-44 --repo-root %s %s`, suite.tmpDir, suite.defaultKosliArguments), - golden: "Error: invalid Jira project keys: [1AB AB-44]\n", + golden: "Error: invalid Jira project keys: [\"1AB\" \"AB-44\"]\n", additionalConfig: jiraTestsAdditionalConfig{ commitMessage: "EX-1 test commit", }, @@ -475,6 +482,50 @@ func TestNormaliseJiraProjectKeys(t *testing.T) { } } +// TestValidateJiraProjectKeys pins that validation normalises the keys itself, rather than +// relying on its caller to have done it, and that a key which is only whitespace is named in +// the error instead of rendering as nothing. +func TestValidateJiraProjectKeys(t *testing.T) { + for _, tc := range []struct { + name string + projectKeys []string + wantErr string + wantKeys []string + }{ + { + name: "a padded key is accepted and left trimmed", + projectKeys: []string{" EX "}, + wantKeys: []string{"EX"}, + }, + { + name: "a padded comma-separated list is accepted", + projectKeys: []string{"ABC", " EX"}, + wantKeys: []string{"ABC", "EX"}, + }, + { + name: "an invalid key is still rejected, and quoted", + projectKeys: []string{"1AB", "AB-44"}, + wantErr: `invalid Jira project keys: ["1AB" "AB-44"]`, + }, + { + name: "a whitespace-only key is named in the error rather than rendering as nothing", + projectKeys: []string{"ABC", " "}, + wantErr: `invalid Jira project keys: [""]`, + }, + } { + t.Run(tc.name, func(t *testing.T) { + o := &attestJiraOptions{projectKeys: tc.projectKeys} + err := o.validateJiraProjectKeys() + if tc.wantErr != "" { + require.EqualError(t, err, tc.wantErr) + return + } + require.NoError(t, err) + require.Equal(t, tc.wantKeys, o.projectKeys) + }) + } +} + // In order for 'go test' to run this suite, we need to create // a normal test function and pass our suite to suite.Run func TestAttestJiraCommandTestSuite(t *testing.T) { From a29e537654af0c6d40deb3f5ec565efd0499b94f Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Thu, 20 Aug 2026 22:47:20 +0100 Subject: [PATCH 3/4] docs(attest jira): say that --jira-project-key takes a comma-separated list The flag help said only "Can be repeated", so the input shape this branch exists to fix - --jira-project-key "ABC, DEF" - was not documented anywhere. Every other list flag in root.go spells it out ("The comma-separated list of ..."), so this one was the outlier. The published CLI reference is generated from this string, and client_reference/ is gitignored rather than committed, so the string is the whole docs change with nothing to regenerate here. Confirmed with make docs CMD="attest jira" that the generated page picks it up. --- cmd/kosli/root.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/kosli/root.go b/cmd/kosli/root.go index 144650b3d..37d417fcb 100644 --- a/cmd/kosli/root.go +++ b/cmd/kosli/root.go @@ -165,7 +165,7 @@ The ^.kosli_ignore^ will be treated as part of the artifact like any other file, jiraUsernameFlag = "Jira username (for Jira Cloud)" jiraAPITokenFlag = "Jira API token (for Jira Cloud)" jiraPATFlag = "Jira personal access token (for self-hosted Jira)" - jiraProjectKeyFlag = "[optional] Jira project key to match against. Can be repeated. Defaults to matching any jira project key." + jiraProjectKeyFlag = "[optional] Jira project key to match against. Can be repeated, or given as a comma-separated list. Defaults to matching any jira project key." 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." From 71a392c6944ae73e74f37b0916d588e7872aa42e Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Thu, 20 Aug 2026 22:50:03 +0100 Subject: [PATCH 4/4] chore: amend testing comment to match the actual code --- cmd/kosli/attestJira_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/cmd/kosli/attestJira_test.go b/cmd/kosli/attestJira_test.go index 915d0d46f..3946bd77d 100644 --- a/cmd/kosli/attestJira_test.go +++ b/cmd/kosli/attestJira_test.go @@ -262,9 +262,8 @@ func (suite *AttestJiraCommandTestSuite) TestAttestJiraCmd() { }, { // cobra splits the list with encoding/csv, which does not trim, so without - // normalisation the " EX" fragment fails validation. EX is deliberately the - // padded one: with --assert the case only passes if the key that has to match - // the EX-1 commit is the one that was trimmed. + // normalisation the " EX" fragment fails validation and the command never + // reaches the matcher. --assert carries the rest of the path end to end. name: "20b can specify jira project keys as a comma-separated list with spaces", cmd: fmt.Sprintf(`attest jira --name bar --jira-base-url https://kosli-test.atlassian.net