skills-init: full-clone SHA pinning always fails (checkout treats commit SHA as pathspec, not revision)

[haakotsm]

Summary

skills-init's full-clone-then-checkout path (CloneGit in go/core/internal/skillsinit/git.go) always fails when Full: true is used with a commit SHA ref, because the checkout command passes the SHA as a pathspec rather than a revision:

if err := runGitIn(ref.Dest, "checkout", "--", ref.Ref); err != nil {

The -- separator before ref.Ref forces git to interpret ref.Ref as a pathspec, not a commit-ish. Since no file in the repo is literally named after the 40-char SHA, this always fails with:

error: pathspec '<sha>' did not match any file(s) known to git

Reproduction

git init repo && cd repo
echo x > f && git add f && git commit -m init
SHA=$(git rev-parse HEAD)

git checkout -- "$SHA"
# error: pathspec '<sha>' did not match any file(s) known to git

git checkout "$SHA"
# Note: switching to '<sha>'.  ... (works correctly)

Confirmed against the released v0.9.12 tag (same code present on main as of this report) — the bug affects every deployment using a gitRefs[].full: true entry pinned to a commit SHA, which is exactly the pattern the skills-init docs recommend for supply-chain-safe SHA pinning (full clone + immutable SHA, as opposed to the depth-1 --branch path which only accepts mutable branch/tag names).

Impact

Any Agent CRD that references a skill via a SHA-pinned full: true git ref will have its skills-init init container crash-loop forever with Init:Error, because the container restarts and re-runs from a directory that was already (partially) cloned on the previous attempt, compounding with a second bug: retries against an already-cloned dest fail with destination path ... already exists and is not an empty directory before ever reaching the checkout step. Users relying on SHA pinning for supply-chain integrity have no working code path today.

Suggested fix

Drop the -- separator (or place it after the ref) in the checkout call:

if err := runGitIn(ref.Dest, "checkout", ref.Ref, "--"); err != nil {

ref.Ref is already validated upstream as a 40/64-char hex string when Full is true, so there's no argument-injection risk from treating it as a revision instead of a pathspec.

Environment

• kagent skills-init image: v0.9.12
• Discovered while live-verifying a kagent Agent CRD using a SHA-pinned skill in a lab cluster.

[DsThakurRawat]

reproduced both halves, and there is a wrinkle in the suggested fix worth catching before it
lands.

the diagnosis is right. on a throwaway repo:

$ git checkout -- 625d2b1c830ca29c25436cc07b396f9ba963587a
error: pathspec '625d2b1c...' did not match any file(s) known to git
$ git checkout 625d2b1c830ca29c25436cc07b396f9ba963587a
Note: switching to '625d2b1c...'.

the wrinkle is the reason the -- is there in the first place. the comment above the line says:

// `--` separator prevents a ref starting with `-` from being parsed
// as a flag. Refs are already validated upstream as 40-char hex when
// Full is true, but defense in depth costs nothing.
if err := runGitIn(ref.Dest, "checkout", "--", ref.Ref); err != nil {

that is not what -- does for checkout. it separates revisions from pathspecs, so it forces
the argument to be read as a path, which is the bug. it does not stop flag parsing, and moving
it to the end does not either:

$ git checkout "--exec=/bin/echo" --
error: unknown option `exec=/bin/echo'

git still parsed it as an option. so checkout ref.Ref -- fixes the SHA case but quietly drops
the property the comment claims to be protecting. that is harmless today because the upstream
hex validation makes a leading - impossible, but the comment would then be describing a
guarantee that is not there, and it is the kind of thing that bites later if the validation is
ever loosened for tags.

the other thing: CloneGitCommit, in this same file, already does the immutable-commit case
properly and more cheaply:

runGitIn(destination, "fetch", "--depth", "1", "origin", commit)
runGitIn(destination, "checkout", "--detach", "FETCH_HEAD")

no -- ambiguity, no full history, and it already rejects anything that is not a full SHA. the
Full: true path exists because clone --depth 1 --branch will not take a SHA, but a depth-1
fetch of one commit will, which is exactly what this function is. routing the SHA case through
it would fix the checkout bug and drop the clone cost at the same time.

that also happens to sidestep the retry problem you mention at the end, since CloneGitCommit
does MkdirAll and git init rather than git clone into a path that must not exist.

can send whichever version you prefer.