fix(attest): don't fail when a CI-defaulted --commit has no repository - #1127
fix(attest): don't fail when a CI-defaulted --commit has no repository#1127FayeSGW wants to merge 2 commits into
Conversation
--commit is populated from CI environment variables (GITHUB_SHA, CI_COMMIT_SHA, BITBUCKET_COMMIT, ...) whether or not the user asked for it. The common attestation flow and begin trail then ran a git lookup because commitSHA was non-empty, so a CI job that has not checked out the repository failed with "failed to get commit info. failed to open git repository at .: repository does not exist" for a commit the user never requested. A commit that arrived from the CI default now warns and proceeds without commit info; a commit the user passed explicitly still errors, so a wrong --repo-root is not silently swallowed. An unresolvable commit in a shallow clone takes the same route, being the same surprise for the same reason. Doing nothing was not an option for affected users: the empty-value rule in refuseEmptyFlagValues rejects --commit "", so there is no way to opt out of the CI default from the command line. The two copies of the lookup are now one resolveCommitInfo, which is also the only level at which the CI default is testable — DefaultValue returns "" whenever KOSLI_TESTS is set, so no command-level test can produce an implicitly defaulted --commit. attest pr * and attest jira need the commit to do their work and dereferenced payload.Commit unguarded. That was unreachable while the empty-value rule blocked every route to a nil commit; soft-failing opens one, so both now report what is missing instead of panicking. Refs kosli-dev/server#6094 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
On macOS os.MkdirTemp returns a path under /var/folders, which is a symlink to /private/var/folders. osfs resolves the symlink for the worktree root but leaves the caller's unresolved path in place for the git dir, so go-git sees a git dir that is not ".git" relative to the worktree, decides the repository lives elsewhere, and tries to write a "gitdir:" file over the directory it just created: open /private/var/folders/.../testDir123/.git: is a directory InitializeGitRepo in the same file already resolves the path for exactly this reason. CloneGitRepo did not, so AttestGitlabPRCommandTestSuite and AttestBitbucketPRCommandTestSuite fail in SetupTest on any machine whose temp dir is symlinked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @FayeSGW's task in 6m 5s —— View job Review
Static analysis
What's rightThe diagnosis is precise and the fix is aimed at the actual boundary. Three things stood out:
The symlink fix in the second commit is correctly diagnosed and mirrors Main points (details inline)
None of these block merging. #1 and #2 are the ones I'd act on; #3-#5 are polish. One question for the teamThis trades a loud failure for a quiet one on a code path where the commit is what later binds an attestation to an artifact. That's the right call for the reported CI job, but it does mean a misconfigured pipeline can now emit commit-less attestations for a long time before anyone notices. Does the server surface anything when an attestation arrives with no |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // resolveCommitInfo returns nil when git cannot supply the commit info and the | ||
| // commit was not asked for explicitly, so a CI-defaulted --commit does not fail | ||
| // the command in a job with no checked-out repository (#6094). | ||
| func resolveCommitInfo(srcRepoRoot, commitSHA string, explicit bool, redactedCommitInfo []string) (*gitview.BasicCommitInfo, error) { | ||
| gv, err := gitview.New(srcRepoRoot) | ||
| if err == nil { | ||
| var commitInfo *gitview.CommitInfo | ||
| commitInfo, err = gv.GetCommitInfoFromCommitSHA(commitSHA, false, redactedCommitInfo) | ||
| if err == nil { | ||
| return &commitInfo.BasicCommitInfo, nil | ||
| } | ||
| } | ||
| if explicit { | ||
| return nil, fmt.Errorf("failed to get commit info. %s", err) | ||
| } | ||
| logger.Warn("attesting without commit info: --commit defaulted to %s from the CI environment, but %s. Point --repo-root at a repository containing that commit to attach it.", commitSHA, err.Error()) |
There was a problem hiding this comment.
Two things about the soft-fail branch:
1. The warning is the only signal a user gets that commit info is now missing. In CI it competes with everything else on stderr, and the consequence isn't stated: the --commit help text itself says a commit is required "when reporting an attestation for an artifact before reporting it to Kosli", so an attestation that quietly loses its commit can fail to bind to an artifact later, at a point far from this warning. Worth naming that consequence here.
2. A wrong --repo-root is still swallowed when --commit came from CI. The PR reasons that an explicit --commit protects against a misconfigured --repo-root, but the reverse case — CI-defaulted --commit plus an explicitly passed, wrong --repo-root — takes the warn path. The user did explicitly ask for that repo. Since cmd.Flags().Changed(...) is already the idiom here, repoRootExplicit could hard-fail that case; that keeps the fix scoped to "no repository was asked for at all", which is the actual #6094 report.
Minor: #6094 in the doc comment resolves to kosli-dev/cli#6094 on GitHub. Use kosli-dev/server#6094.
| // | ||
| // The production trigger (a CI-defaulted --commit in a job with no checked-out | ||
| // repository) cannot be reproduced through the command harness, because | ||
| // DefaultValue returns "" whenever KOSLI_TESTS is set. resolveCommitInfo is | ||
| // therefore exercised directly, and the command cases below guard only that | ||
| // each command assigns commitSHAExplicit. |
There was a problem hiding this comment.
The stated limitation isn't quite true — the harness can produce a CI-defaulted --commit. DefaultValue reads KOSLI_TESTS at flag-construction time, which happens inside executeCommandC → newRootCmd, so a test can unset it around the call. cli_utils_test.go:305-322 already does exactly this unset-and-restore dance for the same reason.
That makes an end-to-end case possible, and it is the one behaviour nothing currently covers — that the command exits 0 and emits the warning:
value, inTests := os.LookupEnv("KOSLI_TESTS")
require.NoError(suite.T(), os.Unsetenv("KOSLI_TESTS"))
defer func() { if inTests { os.Setenv("KOSLI_TESTS", value) } }()
suite.T().Setenv("GITHUB_RUN_NUMBER", "1") // WhichCI() -> github
suite.T().Setenv("GITHUB_SHA", suite.headHash)
// attest generic ... --repo-root testdata, no --commit
// expect: no error, stderr contains "attesting without commit info"Without it, the warning text — the only thing the user sees on the new path — is unasserted, and nothing catches a regression that turns the soft-fail back into an error at the command level. restoreLogger (cli_utils_test.go:1334) is available if you'd rather capture the warning at the unit level instead.
Nit: resolveCommitInfo lives in attestation.go, which already has an attestation_test.go; a commitInfoResolution_test.go with no matching production file breaks the file-pairing convention.
| } | ||
|
|
||
| if o.payload.Commit == nil { | ||
| return fmt.Errorf("failed to get commit info, which is required to search for Jira issue keys. Pass --commit and point --repo-root at a repository containing it") |
There was a problem hiding this comment.
Good catch on the nil deref (line 304 dereferences o.payload.Commit.Sha1), but the advice doesn't fit this command: commit is in RequireFlags at line 267, so a user reaching this line either passed --commit already or is in CI where it was defaulted. "Pass --commit" reads as though they forgot it.
The attest pr * copy in pullrequest.go:42 has the same wording and the same RequireFlags situation. Something closer to the cause would help both:
| return fmt.Errorf("failed to get commit info, which is required to search for Jira issue keys. Pass --commit and point --repo-root at a repository containing it") | |
| return fmt.Errorf("--commit %s could not be resolved in the git repository at --repo-root %s, and the commit is required to search for Jira issue keys", o.commitSHA, o.srcRepoRoot) |
Also worth noting: whenever this guard passes, lines 300-305 immediately redo the gitview.New + GetCommitInfoFromCommitSHA that resolveCommitInfo just did (with different redaction args). Pre-existing, but now that the lookup is centralised it's a visible duplicate.
| // Resolved for the same reason as in InitializeGitRepo below. | ||
| resolvedCloneTo, err := filepath.EvalSymlinks(cloneTo) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| // the repo worktree filesystem. It has to be osfs so that we can give it a path |
There was a problem hiding this comment.
The diagnosis is right and matches InitializeGitRepo below. One behavioural note: EvalSymlinks errors on a path that doesn't exist, so CloneGitRepo now requires cloneTo to be pre-created — previously osfs/git.Clone would create it. Both current callers (attestPRGitlab_test.go:44, attestPRBitbucket_test.go:42) os.MkdirTemp first, so nothing breaks today; it's just an unstated precondition on a shared helper. A line in the comment saying cloneTo must exist would save the next caller a confusing no such file or directory from a clone helper.
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| o.repoURLExplicit = cmd.Flags().Changed("repo-url") | ||
| o.repoNameExplicit = cmd.Flags().Changed("repository") | ||
| o.commitSHAExplicit = cmd.Flags().Changed("commit") |
There was a problem hiding this comment.
This line is now repeated in 13 RunE bodies. It follows the existing repoURLExplicit/repoNameExplicit pattern, so it's consistent — but the failure mode is new and silent: a future attest command that embeds CommonAttestationOptions and forgets this line still compiles, still passes tests, and quietly downgrades an explicit --commit from a hard error to a warning. Nothing catches it.
addAttestationFlags(cmd, o.CommonAttestationOptions, ...) already receives both cmd and o, so it could own the assignment for all three flags — e.g. by chaining a PreRunE there, or by stashing o.flags = cmd.Flags() and reading Changed at use-site. Worth doing here or in a follow-up, and worth a note in .claude/skills/new-command/ either way, since that skill scaffolds new attest commands.
1 participant
Fixes the CI failure reported in kosli-dev/server#6094.
--commitis populated from CI environment variables (GITHUB_SHA,CI_COMMIT_SHA,BITBUCKET_COMMIT, ...) whether or not the user asked for it. The common attestation flow andbegin trailthen ran a git lookup becausecommitSHAwas non-empty, so a CI job that has not checked out the repository failed with:for a commit the user never requested.
What changes
A commit that arrived from the CI default now warns and proceeds without commit info. A commit passed explicitly still errors, so a wrong
--repo-rootis not silently swallowed. An unresolvable commit in a shallow clone takes the same route, being the same surprise for the same reason.The two copies of the lookup (
attestation.goandbeginTrail.go— the duplication is why this was reported twice, see also kosli-dev/server#5615) are now oneresolveCommitInfo.attest pr *andattest jiraneed the commit to do their work and dereferencedpayload.Commitunguarded. That was unreachable before; soft-failing opens a route to it, so both now report what is missing instead of panicking.Why not just document an opt-out
The issue suggested documenting
--commit ""as the escape hatch. That does not work:refuseEmptyFlagValuesinroot.gorejects empty values for every flag, with no exemption list. There is no way to opt out of the CI default from the command line, which is why a behaviour change is needed rather than a docs note.Not affected
attest artifact,report artifact, andattest jira's owngitview.Newall genuinely require the repository and are left alone.Testing
resolveCommitInfois unit-tested directly because that is the only level at which the CI default is reachable —DefaultValuereturns""wheneverKOSLI_TESTSis set, so no command-level test can produce an implicitly defaulted--commit. Command-level tests cover the explicit---commitpath and both new guards.Second commit
test(helpers): resolve symlinks in CloneGitRepo before cloningis unrelated to the above and can be split out if preferred. On macOSos.MkdirTempreturns a path under/var/folders, which is a symlink;osfsresolves it for the worktree root but not the git dir, so go-git tries to write agitdir:file over the.gitdirectory it just created.AttestGitlabPRCommandTestSuiteandAttestBitbucketPRCommandTestSuitefail inSetupTeston any machine whose temp dir is symlinked.InitializeGitRepoin the same file already resolves the path for exactly this reason.Checklist
charts/k8s-reporter/) updated, if needed. Note: these changes live in a separate PR🤖 Generated with Claude Code