fix(repo-info): preserve full CI-detected repo path in repo_info.name#1017
Conversation
The --repository flag defaults to a short repo name in some CIs (GitLab's CI_PROJECT_NAME, Bitbucket's slug) while getGitRepoInfoFrom* detects the full path (GitLab's CI_PROJECT_PATH). Because mergeGitRepoInfo always applied the flag value, the short default clobbered the full CI-detected name, so repo_info.name lost its namespace (e.g. "creator" instead of "cyber-dojo/creator"). Only override the CI-detected name when --repository is set explicitly (or when no CI name was detected), threading repoNameExplicit through the attest/begin-trail commands. The GitLab API ProjectID path is untouched. Also fixes the same latent issue for Bitbucket. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ment Add RepoInfoWiringTestSuite covering the three run() paths that build repo_info (attest generic, begin trail, attest artifact): the CI-detected full-path name is used by default and an explicit --repository overrides it. A mutation (removing a repoNameExplicit assignment) fails the explicit cases, so the suite guards the command wiring. Also add a mergeGitRepoInfo unit case for a fully-populated base with no flags, and a comment on attestPRGitlab clarifying that GitlabConfig. Repository (short, for the API ProjectID) is separate from repo_info.name (full CI_PROJECT_PATH). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Claude finished @FayeSGW's task in 3m 11s —— View job PR Review: preserve full CI-detected repo path in
|
| Check | Result |
|---|---|
go build ./... |
✅ pass |
go vet ./... |
✅ pass |
go mod tidy + git diff |
✅ clean |
Correctness — verified against the source of the "fuller" name
The fix hinges on the CI-detected base name being richer than the --repository default. I confirmed this is true for exactly the two providers the PR targets, and a no-op for the rest:
| Provider | base Name (getGitRepoInfoFrom…) |
--repository default (ciTemplates) |
Effect of fix |
|---|---|---|---|
| GitLab | CI_PROJECT_PATH (full) |
CI_PROJECT_NAME (short) |
full path preserved ✅ |
| Bitbucket | BITBUCKET_REPO_FULL_NAME (full) |
BITBUCKET_REPO_SLUG (short) |
full path preserved ✅ |
| GitHub | GITHUB_REPOSITORY (full) |
GITHUB_REPOSITORY (same) |
no change ✅ |
| Azure | BUILD_REPOSITORY_NAME |
same | no change ✅ |
| CircleCI | CIRCLE_PROJECT_REPONAME |
same | no change ✅ |
So the repoName != "" && (repoNameExplicit || base.Name == "") guard does exactly what's claimed, and only GitLab/Bitbucket behavior changes — matching the PR description.
Explicit-override via env var also works (worth noting for reviewers)
I checked whether cmd.Flags().Changed("repository") misses users who set KOSLI_REPOSITORY rather than the CLI flag. It does not: bindFlags in root.go:603 applies viper values through cmd.Flags().Set(...) during PersistentPreRunE, which marks the flag Changed=true before RunE reads it. CI env vars like CI_PROJECT_NAME aren't KOSLI_-prefixed, so they don't trip this — only a genuine explicit KOSLI_REPOSITORY does. The design is coherent.
Tests
TestMergeGitRepoInfocases are well-chosen (full-path preservation, explicit override, fully-populated base, and the "apply flag when base has no name" fallback).RepoInfoWiringTestSuiteis a nice touch — it exercises all three realrun()paths as dry-run payload assertions, and the header comment honestly documents the mutation test and theattest pr *coverage gap.- Wiring confirmed present in all 14 commands + the two standalone
run()s (attestArtifact,beginTrail);go buildpassing means no struct is missing the embedded field.
Minor observations (non-blocking)
- Duplication of
o.repoNameExplicit = cmd.Flags().Changed("repository")across 14RunEbodies — already raised and you've agreed to defer. Agreed it's fine for now; it mirrors the existingrepoURLExplicitconvention. - Asymmetry with
repoID:repoIDstill overridesbase.IDunconditionally whilerepoNameis now conditional. This is correct (therepo-iddefault isCI_PROJECT_ID, which equals the base ID, so there's nothing to clobber), but a one-line comment noting why the two fields differ would save a future reader the same investigation I just did. Optional.
Good work worth calling out
- Root cause nailed precisely, and the chosen fix leaves the GitLab API
ProjectID()path untouched — the alternative (changing the default toCI_PROJECT_PATH) would have duplicated the namespace, as the PR notes. - The inline comment in
attestPRGitlab.godistinguishingGitlabConfig.Repository(API) fromrepo_info.nameis exactly the kind of note that prevents a future "helpful" refactor from reintroducing the bug. - The backward-compatibility analysis (upsert by
repo_id, name as mutable field, old-short-name lookups breaking) is thorough and honest about the behavior change.
•gitlab-repo-name-full-path
Problem
For GitLab (and Bitbucket) CI,
repo_info.namewas reported as the short project name (e.g.creator) instead of the full path (e.g.cyber-dojo/creator), losing the namespace/sub-group context.Root cause:
getGitRepoInfoFromGitLab()correctly detects the full path (CI_PROJECT_PATH), but the--repositoryflag defaults to a shorter value (CI_PROJECT_NAME), andmergeGitRepoInfounconditionally applied the flag value over the CI-detected base — so the short default always clobbered the full path. Bitbucket had the same latent issue (BITBUCKET_REPO_SLUGvsBITBUCKET_REPO_FULL_NAME).Fix
--repositorynow overrides the CI-detected name only when set explicitly (or when the CI base has no name). Otherwise the fuller CI-detected value is preserved:repoNameExplicitis threaded from each command viacmd.Flags().Changed("repository"), mirroring the existingrepoURLExplicitpattern.Why this approach
The alternative — changing the GitLab
--repositorydefault toCI_PROJECT_PATH— would have broken GitLab's APIProjectID()(Org/Repository→namespace/full-path, a duplicated namespace) and changed the meaning of--gitlab-org. This approach leaves the GitLab PR/API path completely untouched and fixes Bitbucket for free.Testing
TestMergeGitRepoInfo— added cases for full-path preservation, explicit override, and a fully-populated base with no flags.RepoInfoWiringTestSuite(new) — dry-run payload assertions across the threerun()paths that buildrepo_info(attest generic,begin trail,attest artifact): CI-detected name used by default, explicit--repositoryoverrides. A mutation (removing arepoNameExplicitassignment) fails the explicit cases, confirming the suite guards the wiring.attest pr *(call the live VCS API before the dry-run guard; token-gated, as the existing PR tests are).Backward compatibility
(org_id, repo_id)(externalCI_PROJECT_ID), treatingnameas a mutable field. The next attestation updates the name in place — sameinner_id, no fork. All downstream records (artifacts, trails, environment events, deployments, tags) key offinner_id, not name. The CLI only emitsrepo_infowhenid+name+urlare all present, so the server's name-fallback (fork risk) is never triggered.--repositorynow report the full path.kosli get repo <short>,list artifacts --repo <short>,log environment --repo <short>, and the list-reposname=filter. Callers should use the full path,--repo-id, orsearch=. Not data loss.--repositoryusers, and GitHub/Azure/CircleCI users, are unaffected.repo_id; identity stays a single document throughout.Follow-ups (not in this PR)
list artifacts --repohelp/example still saysyourRepoName; could be updated to reflect full-path naming.