Conversation
Contributor
|
Claude finished @mbevc1's task in 2m 27s —— View job Follow-up review complete ✅
Result: The pushed range ( All four previously-raised findings were already resolved in the prior round and remain resolved:
No new bugs, security issues, or regressions found. Posted a summary as a PR comment; no new inline comments needed since all four review threads were already marked resolved. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
mbevc1
pushed a commit
that referenced
this pull request
Sep 17, 2026
…cit-flag detection A config or env value for --repo-root marked the flag Changed even when it equaled the "." default, because bindFlags applies it with Flags().Set() regardless of value. That made KOSLI_REPO_ROOT=. or a config-file repo-root: "." hard-fail a CI-defaulted commit instead of warning, contradicting the help text. repoRootExplicit now also requires the value to differ from ".". begin trail kept its own commitExplicit/repoRootExplicit booleans because it doesn't go through addAttestationFlags. commitInfoRequest now takes the flag set directly and derives both from it, so there is one place, not two, that knows how "explicit" is decided. Adds coverage for the KOSLI_COMMIT env route (explicit, hard-fails) and the KOSLI_REPO_ROOT=. case (still warns), and converts a map-based test loop to a slice for deterministic failure output. Addresses review feedback on #1202. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014HZK6c41JmmUMAB71ZsDpX
--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
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.
…ed commit end to end A --commit defaulted from the CI environment is only downgraded to a warning when --repo-root was left at its default too: a user who pointed at a repository that is not there asked for something and should hear that it failed. The check for whether a flag was passed now reads the command's flag set kept by addAttestationFlags, so no attest command has to remember to capture it. attest pullrequest * and attest jira cannot do their work without the commit, so they name that need in one error rather than warning and then failing. The CI default is reachable through the command harness by unsetting KOSLI_TESTS around the run, as TestDefaultValue already does, so the fix is now exercised end to end: the command exits 0, warns, and sends no git_commit_info. Refs kosli-dev/server#6094, kosli-dev/server#5615
The help for --commit on the attest commands and begin trail now states the rule: only when both --commit and --repo-root are left at their defaults is a commit that cannot be read downgraded to a warning. Refs kosli-dev/server#6094
…ttest skill CloneGitRepo resolves symlinks before cloning, so cloneTo has to exist; say so on the helper. The new-command attest archetype explains why --commit and --repo-root are not captured in RunE and when to set commitRequiredFor.
Drop comments that restate a name or duplicate a sibling field, and trim the rest to the one fact the code cannot show: why a flag set sits on the options, what a false commitExplicit means, why resolve returns nil, nil, and why CloneGitRepo resolves symlinks first.
…cit-flag detection A config or env value for --repo-root marked the flag Changed even when it equaled the "." default, because bindFlags applies it with Flags().Set() regardless of value. That made KOSLI_REPO_ROOT=. or a config-file repo-root: "." hard-fail a CI-defaulted commit instead of warning, contradicting the help text. repoRootExplicit now also requires the value to differ from ".". begin trail kept its own commitExplicit/repoRootExplicit booleans because it doesn't go through addAttestationFlags. commitInfoRequest now takes the flag set directly and derives both from it, so there is one place, not two, that knows how "explicit" is decided. Adds coverage for the KOSLI_COMMIT env route (explicit, hard-fails) and the KOSLI_REPO_ROOT=. case (still warns), and converts a map-based test loop to a slice for deterministic failure output. Addresses review feedback on #1202.
mbevc1
force-pushed
the
20260917_fix_attest
branch
from
September 17, 2026 18:26
4261dca to
07848ba
Compare
Contributor
Follow-up review: commit 4261dca → 07848baThe pushed range turned out to be a rebase, not new edits:
No new bugs, security issues, or regressions found in the (nonexistent) diff. 0 new inline comments. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes kosli-dev/server#6094 and kosli-dev/server#5615.
Closes #1127
--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. There is no escape hatch:
refuseEmptyFlagValuesinroot.gorejects--commit "", so a pipeline could not opt out from the command line.What changes
--committhat was defaulted from the CI environment (not passed, not set viaKOSLI_COMMITor config) now warns and proceeds without commit info when--repo-rootwas also left at its default. Anything asked for explicitly —--commit, or--repo-rootpointing at a repository that isn't there — still fails, so a misconfigured pipeline isn't silently swallowed.addAttestationFlagsalready keeps on the shared options — no per-commandRunEline to remember.attest pullrequest *andattest jiracannot do their work without the commit, so they name that need and fail with one clear error instead of warning and then hitting a nil dereference.--commitfailed, whether it was defaulted from CI, the underlying git error, and what to do (--repo-root).--commithelp text on the attest commands andbegin traildocuments the new behaviour; the docs golden fixture is updated.Testing
The CI-defaulted path is exercised end to end:
KOSLI_TESTSis unset around the command run (the same trickTestDefaultValuealready uses), a GitHub Actions job is simulated, and the test asserts the command exits 0, warns, and sends nogit_commit_info. Also covered: the shallow-clone case, an explicit--repo-rootwith no repository, an explicit--commitwith no repository, a successful explicit--commit, and the two commit-required commands.Checklist
charts/k8s-reporter/) updated, if needed. Note: these changes live in a separate PR