From 4e229c0546b93479c27cfd702a639b65fb47bd5a Mon Sep 17 00:00:00 2001 From: Matthias Wenz Date: Thu, 30 Jul 2026 14:42:18 +0200 Subject: [PATCH 1/3] feat(repo): add `entire repo mirror use` to repoint a clone at a mirror MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the last manual step in mirror onboarding. Previously adopting a mirror meant reading the clone URL out of `mirror get` and hand-running `git remote set-url` — the `mirror use` verb from the standalone entiredb CLI was deliberately not ported ("a git-config + git-remote-entire concern outside the control-plane API"). `use` is the local half of that concern and nothing more: it resolves the repo's pullable placements, picks one, and rewrites local git config. It creates no server-side state — an unmirrored repo errors with a pointer at `mirror create`. - Bare `entire repo mirror use` resolves the repo from the clone's own remotes, lists its placements, and prompts for the cluster when there is more than one. - On a terminal it then asks whether to repoint the remote (keeping the old URL as `upstream`) or add the mirror under a separate name. - Non-interactively it repoints `--remote` (default `origin`) and preserves the replaced URL under `--upstream`; `--upstream ''` discards it. A cluster can be pinned with `--cluster` or the `[cluster-host]` positional. - The replaced URL is always echoed (credential-redacted), and an existing `upstream` is never clobbered, so the previous state stays recoverable. Repo identity and the write target are kept as separate roles: `--remote` names what gets rewritten, which need not exist yet, so identity resolves from the target remote first and then `origin`. The cluster picker is factored out of `repo clone` as `selectPlacement` + `placementPicker` so both verbs share one selection path with per-verb wording; `repo clone`'s messages are unchanged. Option order in the replace-or-add prompt is load-bearing and commented as such: huh answers an unreadable accessible prompt with the first option and a nil error, so the first branch must match what the same flags do non-interactively. A test pins that invariant. Co-Authored-By: Claude Opus 5 (1M context) Entire-Checkpoint: 01KYSGR4W1ZATZRW47J510EZGJ --- CLAUDE.md | 9 +- cmd/entire/cli/corecmd_json_flag_test.go | 22 +- cmd/entire/cli/repo_clone.go | 68 +++- cmd/entire/cli/repo_mirror.go | 7 +- cmd/entire/cli/repo_mirror_use.go | 491 +++++++++++++++++++++++ cmd/entire/cli/repo_mirror_use_test.go | 463 +++++++++++++++++++++ 6 files changed, 1027 insertions(+), 33 deletions(-) create mode 100644 cmd/entire/cli/repo_mirror_use.go create mode 100644 cmd/entire/cli/repo_mirror_use_test.go diff --git a/CLAUDE.md b/CLAUDE.md index ff99bc07a2..81fa96c652 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -72,7 +72,14 @@ the commands are always runnable in every build. - `project`: control-plane project management — `create`, `list`, `get`, `delete` - `repo`: control-plane repository lifecycle — `create`, `list`, `get`, `delete`, `clone`, plus the `mirror` and `visibility` subtrees. Git content operations - (log, diff, …) are intentionally out of scope. + (log, diff, …) are intentionally out of scope. The `mirror` subtree is + server-side (`create`, `list`, `get`, `remove`, `collaborators`) with one + exception: `mirror use` repoints the *current clone's* git remote at a mirror + (local git config only — it creates nothing server-side). Interactively it + picks among the repo's placements and asks whether to replace the remote + (preserving the old URL under `--upstream`) or add a separate one; + non-interactively it repoints `--remote` directly. Both `use` and `clone` + choose a placement through the shared `selectPlacement` picker. - `grant`: manage access grants and org membership — `org`, `project`, and `repo` each support `add` / `list` / `remove` diff --git a/cmd/entire/cli/corecmd_json_flag_test.go b/cmd/entire/cli/corecmd_json_flag_test.go index 3180cbf62e..137ff33f18 100644 --- a/cmd/entire/cli/corecmd_json_flag_test.go +++ b/cmd/entire/cli/corecmd_json_flag_test.go @@ -34,15 +34,19 @@ func TestControlPlaneJSONFlag_OnlyOnHonoringCommands(t *testing.T) { "project get": true, "project delete": false, // repo - "repo create": true, - "repo list": true, - "repo get": true, - "repo delete": false, - "repo clone": false, - "repo mirror create": false, - "repo mirror list": true, - "repo mirror get": true, - "repo mirror remove": false, + "repo create": true, + "repo list": true, + "repo get": true, + "repo delete": false, + "repo clone": false, + "repo mirror create": false, + "repo mirror list": true, + "repo mirror get": true, + "repo mirror remove": false, + // `use` writes local git config and reports what it changed; there is no + // object to render, so it stays off the --json surface like the other + // side-effect verbs. + "repo mirror use": false, "repo mirror collaborators list": true, "repo visibility get": true, "repo visibility set": true, diff --git a/cmd/entire/cli/repo_clone.go b/cmd/entire/cli/repo_clone.go index aa360fa0e8..4d7189e091 100644 --- a/cmd/entire/cli/repo_clone.go +++ b/cmd/entire/cli/repo_clone.go @@ -2,7 +2,6 @@ package cli import ( "context" - "errors" "fmt" "os/exec" "regexp" @@ -241,14 +240,42 @@ func resolvePullablePlacements(ctx context.Context, c *coreapi.Client, owner, re return out.Placements, nil } -// selectCloneTarget resolves which mirror placement to clone from. With one -// placement it returns it directly. With --cluster it picks the matching one (or -// errors listing the available hosts). With more than one and no flag it prompts -// interactively, failing fast with a --cluster pointer when there's no terminal. +// placementPicker adapts selectPlacement's messages to the calling verb. The +// picker logic is identical for every consumer (dedupe by host, honor an +// explicit selector, prompt only when there's a real choice); only the words +// differ, so they're passed in rather than duplicated per command. +type placementPicker struct { + // selector names the non-interactive way to choose a cluster, as the user + // would type it (e.g. `--cluster`). Interpolated into the no-terminal error + // so the pointer names a flag the calling command actually accepts. + selector string + // title is the interactive single-select's prompt. + title string + // action names the operation in the cancellation message, capitalized + // ("Clone", "Remote update") — handleFormCancellation prints + // " cancelled." + action string +} + +// selectCloneTarget resolves which mirror placement to clone from, with the +// clone verb's wording. See selectPlacement for the selection rules. func selectCloneTarget(cmd *cobra.Command, placements []coreapi.ResolvedPlacement, clusterFlag string) (coreapi.ResolvedPlacement, error) { - // Dedupe by cluster host: one placement per cluster is what a clone targets, + return selectPlacement(cmd, placements, clusterFlag, placementPicker{ + selector: "--cluster", + title: "This repo is mirrored on more than one cluster — pick one to clone from", + action: "Clone", + }) +} + +// selectPlacement resolves which mirror placement a verb should act on. With one +// placement it returns it directly. With an explicit clusterSel it picks the +// matching one (or errors listing the available hosts). With more than one and no +// selector it prompts interactively, failing fast with a p.selector pointer when +// there's no terminal. +func selectPlacement(cmd *cobra.Command, placements []coreapi.ResolvedPlacement, clusterSel string, p placementPicker) (coreapi.ResolvedPlacement, error) { + // Dedupe by cluster host: one placement per cluster is what a caller acts on, // and the same host appearing twice would only confuse the picker. Key on the - // case-folded host — DNS is case-insensitive, so a --cluster value differing + // case-folded host — DNS is case-insensitive, so a selector value differing // only in case from the API's ClusterHost must still match (the alternative is // a misleading "not mirrored on ..." after a successful lookup + dial). byHost := make(map[string]coreapi.ResolvedPlacement, len(placements)) @@ -263,12 +290,12 @@ func selectCloneTarget(cmd *cobra.Command, placements []coreapi.ResolvedPlacemen } sort.Strings(hosts) - if clusterFlag != "" { - p, ok := byHost[strings.ToLower(strings.TrimSpace(clusterFlag))] + if clusterSel != "" { + match, ok := byHost[strings.ToLower(strings.TrimSpace(clusterSel))] if !ok { - return coreapi.ResolvedPlacement{}, fmt.Errorf("repo is not mirrored on %q; available: %s", clusterFlag, strings.Join(hosts, ", ")) + return coreapi.ResolvedPlacement{}, fmt.Errorf("repo is not mirrored on %q; available: %s", clusterSel, strings.Join(hosts, ", ")) } - return p, nil + return match, nil } if len(hosts) == 1 { @@ -276,7 +303,7 @@ func selectCloneTarget(cmd *cobra.Command, placements []coreapi.ResolvedPlacemen } if !interactive.CanPromptInteractively() { - return coreapi.ResolvedPlacement{}, fmt.Errorf("repo is mirrored on %d clusters; pass --cluster to choose one of: %s", len(hosts), strings.Join(hosts, ", ")) + return coreapi.ResolvedPlacement{}, fmt.Errorf("repo is mirrored on %d clusters; pass %s to choose one of: %s", len(hosts), p.selector, strings.Join(hosts, ", ")) } options := make([]huh.Option[string], len(hosts)) @@ -287,26 +314,27 @@ func selectCloneTarget(cmd *cobra.Command, placements []coreapi.ResolvedPlacemen form := NewAccessibleForm( huh.NewGroup( huh.NewSelect[string](). - Title("This repo is mirrored on more than one cluster — pick one to clone from"). + Title(p.title). Options(options...). Value(&selected), ), ) + cancelled := NewSilentError(fmt.Errorf("%s cancelled", strings.ToLower(p.action))) if err := form.RunWithContext(cmd.Context()); err != nil { - // handleFormCancellation prints "Clone cancelled." and returns nil for a + // handleFormCancellation prints " cancelled." and returns nil for a // Ctrl+C / cancelled-context abort. Surface that as a SilentError so the - // caller stops instead of falling through to clone a zero-value target + // caller stops instead of falling through to act on a zero-value target // (the `entire:///gh/...` empty-host bug); a real form error propagates. - if cerr := handleFormCancellation(cmd.ErrOrStderr(), "Clone", err); cerr != nil { + if cerr := handleFormCancellation(cmd.ErrOrStderr(), p.action, err); cerr != nil { return coreapi.ResolvedPlacement{}, cerr } - return coreapi.ResolvedPlacement{}, NewSilentError(errors.New("clone cancelled")) + return coreapi.ResolvedPlacement{}, cancelled } - p, ok := byHost[selected] + match, ok := byHost[selected] if !ok { - return coreapi.ResolvedPlacement{}, NewSilentError(errors.New("clone cancelled")) + return coreapi.ResolvedPlacement{}, cancelled } - return p, nil + return match, nil } // mirrorCellLabel is the human label for a mirror placement in the clone picker: diff --git a/cmd/entire/cli/repo_mirror.go b/cmd/entire/cli/repo_mirror.go index d0e322a491..c256c79163 100644 --- a/cmd/entire/cli/repo_mirror.go +++ b/cmd/entire/cli/repo_mirror.go @@ -450,9 +450,9 @@ func validateClusterHost(host string) error { // newRepoMirrorCmd is the `entire repo mirror` subtree: manage EntireDB // GitHub-mirror placements on a cluster. Mirrors the standalone entiredb // CLI's `entire repo mirror` surface for the server-side half (create / -// list / get / remove). The local-clone rewrite (`mirror use`) is not -// ported — it's a git-config + git-remote-entire concern outside the -// control-plane API. +// list / get / remove), plus the local-clone rewrite (`use`) — the one verb +// here that touches no control-plane state beyond a placement lookup and +// instead edits the current clone's git config (see repo_mirror_use.go). func newRepoMirrorCmd() *cobra.Command { cmd := &cobra.Command{ Use: "mirror", @@ -461,6 +461,7 @@ func newRepoMirrorCmd() *cobra.Command { cmd.AddCommand(newRepoMirrorCreateCmd()) cmd.AddCommand(newRepoMirrorListCmd()) cmd.AddCommand(newRepoMirrorGetCmd()) + cmd.AddCommand(newRepoMirrorUseCmd()) cmd.AddCommand(newRepoMirrorRemoveCmd()) cmd.AddCommand(newRepoMirrorCollaboratorsCmd()) return cmd diff --git a/cmd/entire/cli/repo_mirror_use.go b/cmd/entire/cli/repo_mirror_use.go new file mode 100644 index 0000000000..37e10f62c0 --- /dev/null +++ b/cmd/entire/cli/repo_mirror_use.go @@ -0,0 +1,491 @@ +package cli + +import ( + "context" + "errors" + "fmt" + "io" + "os/exec" + "regexp" + "strings" + + "charm.land/huh/v2" + "github.com/spf13/cobra" + + "github.com/entireio/cli/cmd/entire/cli/gitremote" + "github.com/entireio/cli/cmd/entire/cli/interactive" + "github.com/entireio/cli/cmd/entire/cli/paths" + "github.com/entireio/cli/internal/coreapi" +) + +// defaultMirrorRemote is the remote `mirror use` repoints by default: the one +// git itself defaults to for fetch/push, so pointing it at the mirror is what +// "use the mirror" means with no further flags. +const defaultMirrorRemote = "origin" + +// defaultMirrorUpstreamRemote is where a replaced URL is preserved, so +// repointing origin is never a lossy operation — the forge stays reachable under +// the name git's own fork workflow uses for it. +const defaultMirrorUpstreamRemote = "upstream" + +// defaultMirrorSideRemote is the suggested name when the user opts to add the +// mirror alongside their existing remote rather than replace it. +const defaultMirrorSideRemote = "entire" + +// gitRemoteNameRe is the remote-name charset `mirror use` accepts. Git itself is +// laxer, but these names are written into `.git/config` section headers and +// passed as argv to `git remote`, so the value is pinned to a conservative +// shape: it must start alphanumeric (so it can never be read as a flag) and +// carries no path or glob metacharacters. +var gitRemoteNameRe = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._/-]*$`) + +// validateGitRemoteName rejects names git would refuse (or that would land +// somewhere unintended in .git/config) before they reach `git remote`. +func validateGitRemoteName(name string) error { + if name == "" { + return errors.New("remote name cannot be empty") + } + if !gitRemoteNameRe.MatchString(name) { + return fmt.Errorf("%q is not a valid remote name (letters, digits, and . _ - / after a leading alphanumeric)", name) + } + // ".." would escape the intended config path; a ".lock" suffix collides with + // git's own lockfile naming. + if strings.Contains(name, "..") || strings.HasSuffix(name, ".lock") { + return fmt.Errorf("%q is not a valid remote name", name) + } + return nil +} + +// gitRunner runs a git subcommand in dir. A package var so tests exercise the +// planning and prompt logic without mutating a real repository's config. +var gitRunner = func(ctx context.Context, dir string, args ...string) (string, error) { + cmd := exec.CommandContext(ctx, "git", args...) + cmd.Dir = dir + out, err := cmd.Output() + if err != nil { + return "", fmt.Errorf("git %s: %w", strings.Join(args, " "), err) + } + return strings.TrimSpace(string(out)), nil +} + +// listGitRemotes returns the names of every configured remote in dir. +func listGitRemotes(ctx context.Context, dir string) (map[string]bool, error) { + out, err := gitRunner(ctx, dir, "remote") + if err != nil { + return nil, fmt.Errorf("list git remotes: %w", err) + } + remotes := make(map[string]bool) + for _, line := range strings.Split(out, "\n") { + if name := strings.TrimSpace(line); name != "" { + remotes[name] = true + } + } + return remotes, nil +} + +// mirrorRemotePlan is the resolved set of git-config writes `mirror use` will +// perform. It is computed in full before anything is written so the command can +// echo exactly what it is about to do (and so the planning is unit-testable +// without touching a repo). +type mirrorRemotePlan struct { + // remote is the remote that ends up pointing at mirrorURL. + remote string + // mirrorURL is the entire:// clone URL being adopted. + mirrorURL string + // add is true when remote does not exist yet (`git remote add` rather than + // `git remote set-url`). + add bool + // replacedURL is the URL remote currently holds, when it is being + // repointed. Empty when add is true. + replacedURL string + // preserveAs, when non-empty, is a new remote that will be created holding + // replacedURL so the previous URL stays reachable. + preserveAs string + // noop is true when remote already points at mirrorURL. + noop bool +} + +// planMirrorRemote resolves what to write for a `mirror use` invocation. +// remotes is the set of already-configured remote names and currentURL the +// URL of the target remote ("" when it does not exist). +// +// upstream is the requested preserve-under name; it is honored only when the +// target remote is actually being repointed and the name is free. An occupied +// name is skipped rather than clobbered — the replaced URL is echoed either way, +// so nothing is lost, and silently rewriting an existing `upstream` would be the +// one genuinely destructive thing this command could do. +func planMirrorRemote(remote, mirrorURL, currentURL, upstream string, remotes map[string]bool) mirrorRemotePlan { + plan := mirrorRemotePlan{remote: remote, mirrorURL: mirrorURL} + if !remotes[remote] { + plan.add = true + return plan + } + if strings.EqualFold(strings.TrimSpace(currentURL), mirrorURL) { + plan.noop = true + return plan + } + plan.replacedURL = currentURL + if upstream != "" && upstream != remote && !remotes[upstream] { + plan.preserveAs = upstream + } + return plan +} + +// applyMirrorRemotePlan performs the plan's git-config writes. The preserve step +// runs first so a failure there aborts before the original URL is overwritten. +func applyMirrorRemotePlan(ctx context.Context, dir string, plan mirrorRemotePlan) error { + if plan.noop { + return nil + } + if plan.preserveAs != "" { + if _, err := gitRunner(ctx, dir, "remote", "add", plan.preserveAs, plan.replacedURL); err != nil { + return fmt.Errorf("preserve current %s URL as %q: %w", plan.remote, plan.preserveAs, err) + } + } + verb := "set-url" + if plan.add { + verb = "add" + } + if _, err := gitRunner(ctx, dir, "remote", verb, plan.remote, plan.mirrorURL); err != nil { + return fmt.Errorf("point remote %q at the mirror: %w", plan.remote, err) + } + return nil +} + +// reportMirrorRemotePlan echoes what was written, in recovery-friendly terms: +// every replaced URL is printed even when it was also preserved under another +// remote, so the previous value is always visible in the transcript. +func reportMirrorRemotePlan(out io.Writer, plan mirrorRemotePlan) { + if plan.noop { + fmt.Fprintf(out, "Remote %q already points at the mirror:\n %s\n", plan.remote, plan.mirrorURL) + return + } + if plan.add { + fmt.Fprintf(out, "✓ Added remote %q\n %s\n", plan.remote, plan.mirrorURL) + } else { + fmt.Fprintf(out, "✓ Repointed remote %q at the mirror\n %s\n", plan.remote, plan.mirrorURL) + fmt.Fprintf(out, " was: %s\n", gitremote.RedactURL(plan.replacedURL)) + if plan.preserveAs != "" { + fmt.Fprintf(out, "✓ Kept the previous URL as remote %q\n", plan.preserveAs) + } + } + fmt.Fprintf(out, "\nFetch through it:\n git fetch %s\n", plan.remote) +} + +// mirrorUseChoice is the outcome of the interactive replace-or-add prompt. +type mirrorUseChoice struct { + // remote is the remote name to write (the target remote when replacing, a + // new side remote when adding). + remote string + // upstream is the preserve-under name, or "" when adding a side remote + // (nothing is being replaced, so there is nothing to preserve). + upstream string +} + +// promptMirrorRemoteChoice asks whether to repoint the existing target remote or +// add the mirror under a separate name. It is only reached on a terminal, and +// only when the target remote already exists with a different URL — the two +// cases where the write is not self-evidently what the user wanted. +func promptMirrorRemoteChoice(cmd *cobra.Command, remote, currentURL, mirrorURL, upstream string, remotes map[string]bool) (mirrorUseChoice, error) { + const ( + choiceReplace = "replace" + choiceAdd = "add" + ) + // Replace is listed first deliberately. huh answers an unreadable accessible + // prompt with the first option (see the comment on `selected` below), so the + // first option decides what a Ctrl+D / closed-stdin prompt does — and the only + // self-consistent answer is the same thing the non-interactive path does with + // these exact flags: repoint `remote`, preserving the old URL under + // `upstream`. Putting "add" first would make an interrupted prompt diverge + // from the documented default. The write is reported in full either way + // (reportMirrorRemotePlan echoes the replaced URL), and it is local git + // config, so it stays trivially reversible. + replaceLabel := fmt.Sprintf("Replace %q — point it at the mirror", remote) + if upstream != "" && upstream != remote && !remotes[upstream] { + replaceLabel = fmt.Sprintf("Replace %q — point it at the mirror, keep the current URL as %q", remote, upstream) + } + sideName := defaultMirrorSideRemote + for remotes[sideName] { + sideName += "-mirror" + } + // Left empty rather than pre-seeded so the switch below can tell "huh handed + // back something we don't recognise" from a real choice. Note this does NOT + // make EOF safe: huh's accessible mode answers an unreadable prompt by + // writing the FIRST option's value and returning a nil error (verified + // behavior), so at EOF `selected` becomes choiceReplace regardless of what + // it started as. That is why the option order matters below. + var selected string + if err := runMirrorUseForm(cmd, "Remote update", NewAccessibleForm( + huh.NewGroup( + huh.NewSelect[string](). + Title(fmt.Sprintf("%q currently points at %s", remote, gitremote.RedactURL(currentURL))). + Description("Mirror: "+mirrorURL). + Options( + huh.NewOption(replaceLabel, choiceReplace), + huh.NewOption("Add the mirror as a separate remote instead", choiceAdd), + ). + Value(&selected), + ), + )); err != nil { + return mirrorUseChoice{}, err + } + switch selected { + case choiceReplace: + return mirrorUseChoice{remote: remote, upstream: upstream}, nil + case choiceAdd: + // fall through to the name prompt + default: + // Unreachable with the options above (huh always writes one of them). + // Kept so an unrecognised value can never fall through into a write. + return mirrorUseChoice{}, NewSilentError(errors.New("no remote update selected")) + } + + name := sideName + if err := runMirrorUseForm(cmd, "Remote update", NewAccessibleForm( + huh.NewGroup( + huh.NewInput(). + Title("Name for the new remote"). + Value(&name). + Validate(func(v string) error { + v = strings.TrimSpace(v) + if err := validateGitRemoteName(v); err != nil { + return err + } + if remotes[v] { + return fmt.Errorf("remote %q already exists", v) + } + return nil + }), + ), + )); err != nil { + return mirrorUseChoice{}, err + } + // Re-check outside the form: an unreadable accessible prompt leaves an Input + // at its default without running Validate. The default computed above is + // already free and well-formed, so this is belt-and-braces — but it keeps the + // "never write an unvalidated remote name" invariant local to this function + // instead of resting on how the default was derived. + name = strings.TrimSpace(name) + if err := validateGitRemoteName(name); err != nil { + return mirrorUseChoice{}, fmt.Errorf("invalid remote name: %w", err) + } + if remotes[name] { + return mirrorUseChoice{}, fmt.Errorf("remote %q already exists", name) + } + // A side remote replaces nothing, so there is no URL to preserve. + return mirrorUseChoice{remote: name}, nil +} + +// runMirrorUseForm runs a huh form, mapping a Ctrl+C / cancelled-context abort +// to a SilentError so the caller stops instead of falling through to write a +// zero-value remote name. +func runMirrorUseForm(cmd *cobra.Command, action string, form *huh.Form) error { + if err := form.RunWithContext(cmd.Context()); err != nil { + if cerr := handleFormCancellation(cmd.ErrOrStderr(), action, err); cerr != nil { + return cerr + } + return NewSilentError(fmt.Errorf("%s cancelled", strings.ToLower(action))) + } + return nil +} + +// mirrorUseForge is the only forge mirrors support today; a remote pointing +// anywhere else cannot name a mirrorable upstream. +const mirrorUseForge = "gh" + +// resolveMirrorUseUpstream determines the GitHub upstream `mirror use` should +// look for mirrors of. An explicit [github-url] wins. Otherwise the coordinates +// are read from a configured remote — which already names the repo the user is +// standing in. +// +// Note the two distinct roles a remote name plays here: `remote` is the *write +// target* (what gets pointed at the mirror), while repo identity can come from +// any remote that names the upstream. So the target remote is consulted first +// (re-running `use --remote entire` on an already-mirrored side remote must +// resolve), then `origin` — otherwise `--remote entire` on a fresh clone would +// fail purely because the remote it is about to create does not exist yet. +// +// entire:// remotes resolve as readily as forge remotes (their forge lives in +// the URL path), so switching clusters never needs the repo retyped. +func resolveMirrorUseUpstream(ctx context.Context, dir, remote, arg string) (owner, repo string, err error) { + if arg != "" { + owner, repo, err = parseGitHubURL(arg) + if err != nil { + return "", "", fmt.Errorf("invalid : %w", err) + } + return owner, repo, nil + } + + candidates := []string{remote} + if remote != defaultMirrorRemote { + candidates = append(candidates, defaultMirrorRemote) + } + // Track why each candidate was rejected so the error can say which remotes + // were tried and what was wrong with them, rather than a bare "not found". + var tried []string + for _, name := range candidates { + rawURL, gerr := gitremote.GetRemoteURLInDir(ctx, dir, name) + if gerr != nil { + tried = append(tried, name+" (not configured)") + continue + } + info, perr := gitremote.ParseURL(rawURL) + if perr != nil { + tried = append(tried, name+" (unparseable URL)") + continue + } + if info.Forge != mirrorUseForge { + tried = append(tried, name+" (not a GitHub repo — mirrors are GitHub-only)") + continue + } + return strings.ToLower(info.Owner), strings.ToLower(info.Repo), nil + } + return "", "", fmt.Errorf("cannot tell which repo to mirror from the git remotes (tried %s); pass the GitHub URL explicitly", strings.Join(tried, ", ")) +} + +func newRepoMirrorUseCmd() *cobra.Command { + var remote, upstream, cluster string + cmd := &cobra.Command{ + Use: "use [github-url] [cluster-host]", + Short: "Point this clone's git remote at an Entire mirror", + Long: "Rewrites the local git remote so fetch and push go through an " + + "Entire mirror instead of the forge.\n\n" + + "With no arguments, resolves the repo from the current clone's " + + "`origin` remote, lists the clusters it is mirrored on, and — when " + + "there is more than one — asks which to use. On a terminal it then " + + "asks whether to repoint `origin` or add the mirror as a separate " + + "remote; when repointing, the previous URL is kept as `upstream` so " + + "the forge stays reachable.\n\n" + + "Non-interactively it repoints --remote (default `origin`) directly, " + + "preserving the replaced URL under --upstream. It only ever edits " + + "local git config — the mirror must already exist (`entire repo " + + "mirror create`); nothing server-side is changed.", + Example: " entire repo mirror use\n" + + " entire repo mirror use --cluster aws-us-east-2.entire.io\n" + + " entire repo mirror use github.com/octocat/hello-world\n" + + " entire repo mirror use github.com/octocat/hello-world aws-us-east-2.entire.io\n" + + " entire repo mirror use --remote entire\n" + + " entire repo mirror use --upstream ''", + Args: cobra.RangeArgs(0, 2), + RunE: func(cmd *cobra.Command, args []string) error { + cmd.SilenceUsage = true + if err := validateGitRemoteName(remote); err != nil { + return fmt.Errorf("invalid --remote: %w", err) + } + // An empty --upstream is the documented opt-out of preserving the + // replaced URL, so only a non-empty value is validated. + if upstream != "" { + if err := validateGitRemoteName(upstream); err != nil { + return fmt.Errorf("invalid --upstream: %w", err) + } + } + // Positional args are validated before the repo is resolved so a + // malformed invocation fails identically inside and outside a clone. + var upstreamArg, clusterArg string + if len(args) > 0 { + upstreamArg = strings.TrimSpace(args[0]) + } + if len(args) > 1 { + clusterArg = strings.TrimSpace(args[1]) + } + // --cluster is the way to pin a cluster without also naming the repo + // (the positional slot is second, so it would otherwise need an empty + // first arg). Both forms setting different hosts is a contradiction, + // not a precedence question. + if cluster = strings.TrimSpace(cluster); cluster != "" { + if clusterArg != "" && !strings.EqualFold(clusterArg, cluster) { + return fmt.Errorf("[cluster-host] (%s) and --cluster (%s) disagree; pass only one", clusterArg, cluster) + } + clusterArg = cluster + } + if clusterArg != "" { + if err := validateClusterHost(clusterArg); err != nil { + return fmt.Errorf("invalid cluster host: %w", err) + } + } + + ctx := cmd.Context() + repoRoot, err := paths.WorktreeRoot(ctx) + if err != nil { + fmt.Fprintln(cmd.ErrOrStderr(), "Not a git repository. Run `entire repo mirror use` from inside the clone whose remote you want to repoint.") + return NewSilentError(errors.New("not a git repository")) + } + + owner, repo, err := resolveMirrorUseUpstream(ctx, repoRoot, remote, upstreamArg) + if err != nil { + return err + } + + // The pull-gated placement lookup is the same authority the clone's + // STS exchange enforces, so anything the user could clone resolves + // here — public mirrors included. + var placements []coreapi.ResolvedPlacement + if err := runCore(cmd, func(ctx context.Context, c *coreapi.Client) error { + ps, lerr := resolvePullablePlacements(ctx, c, owner, repo) + if lerr != nil { + return lerr + } + placements = ps + return nil + }); err != nil { + return err + } + if len(placements) == 0 { + return fmt.Errorf("%s/%s is not mirrored (or you have no access to its mirrors); create one first:\n entire repo mirror create github.com/%s/%s", owner, repo, owner, repo) + } + + chosen, err := selectPlacement(cmd, placements, clusterArg, placementPicker{ + selector: "--cluster", + title: fmt.Sprintf("%s/%s is mirrored on more than one cluster — pick the one to use", owner, repo), + action: "Remote update", + }) + if err != nil { + return err + } + mirrorURL := mirrorCloneURL(chosen.ClusterHost, owner, repo) + + remotes, err := listGitRemotes(ctx, repoRoot) + if err != nil { + return err + } + // GetRemoteURLInDir errors when the remote is absent; that is the + // "add" case, which carries no current URL. + currentURL := "" + if remotes[remote] { + if currentURL, err = gitremote.GetRemoteURLInDir(ctx, repoRoot, remote); err != nil { + return fmt.Errorf("read current URL of remote %q: %w", remote, err) + } + } + + target, preserve := remote, upstream + // Prompt only when the write is ambiguous: the remote exists and + // holds a different URL. A missing remote, or one already pointing + // at this mirror, has exactly one sensible outcome. + if remotes[remote] && !strings.EqualFold(strings.TrimSpace(currentURL), mirrorURL) && interactive.CanPromptInteractively() { + choice, perr := promptMirrorRemoteChoice(cmd, remote, currentURL, mirrorURL, upstream, remotes) + if perr != nil { + return perr + } + target, preserve = choice.remote, choice.upstream + } + + // currentURL was read for `remote`. When the prompt selected a + // different (side) remote, that name was validated as free, so it + // carries no current URL of its own. + targetURL := currentURL + if target != remote { + targetURL = "" + } + plan := planMirrorRemote(target, mirrorURL, targetURL, preserve, remotes) + if err := applyMirrorRemotePlan(ctx, repoRoot, plan); err != nil { + return err + } + reportMirrorRemotePlan(cmd.OutOrStdout(), plan) + return nil + }, + } + cmd.Flags().StringVar(&remote, "remote", defaultMirrorRemote, "Git remote to point at the mirror") + cmd.Flags().StringVar(&upstream, "upstream", defaultMirrorUpstreamRemote, "Remote to preserve the replaced URL under; empty to discard it") + cmd.Flags().StringVar(&cluster, "cluster", "", "Cluster host to use when the repo is mirrored on several (same as [cluster-host])") + return cmd +} diff --git a/cmd/entire/cli/repo_mirror_use_test.go b/cmd/entire/cli/repo_mirror_use_test.go new file mode 100644 index 0000000000..ae855dfe7d --- /dev/null +++ b/cmd/entire/cli/repo_mirror_use_test.go @@ -0,0 +1,463 @@ +package cli + +import ( + "cmp" + "context" + "os/exec" + "strings" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/entireio/cli/cmd/entire/cli/testutil" +) + +func TestValidateGitRemoteName(t *testing.T) { + t.Parallel() + tests := []struct { + name string + remote string + wantErr bool + }{ + {name: "origin", remote: "origin"}, + {name: "entire", remote: "entire"}, + {name: "digits and dashes", remote: "mirror-2"}, + {name: "dotted", remote: "my.remote"}, + {name: "slashed", remote: "team/mirror"}, + {name: "empty", remote: "", wantErr: true}, + {name: "leading dash reads as a flag", remote: "-f", wantErr: true}, + {name: "leading dot", remote: ".hidden", wantErr: true}, + {name: "space", remote: "my remote", wantErr: true}, + {name: "glob", remote: "mirror*", wantErr: true}, + {name: "traversal", remote: "a/../b", wantErr: true}, + {name: "lock suffix", remote: "origin.lock", wantErr: true}, + {name: "newline", remote: "origin\nfetch", wantErr: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := validateGitRemoteName(tt.remote) + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + }) + } +} + +func TestPlanMirrorRemote(t *testing.T) { + t.Parallel() + const mirrorURL = "entire://aws-us-east-2.entire.io/gh/octocat/hello-world" + const forgeURL = "git@github.com:octocat/hello-world.git" + + t.Run("adds a remote that does not exist", func(t *testing.T) { + t.Parallel() + plan := planMirrorRemote("entire", mirrorURL, "", "upstream", map[string]bool{"origin": true}) + require.True(t, plan.add) + require.False(t, plan.noop) + require.Empty(t, plan.replacedURL) + require.Empty(t, plan.preserveAs, "nothing was replaced, so nothing is preserved") + require.Equal(t, mirrorURL, plan.mirrorURL) + }) + + t.Run("replaces and preserves the previous URL", func(t *testing.T) { + t.Parallel() + plan := planMirrorRemote("origin", mirrorURL, forgeURL, "upstream", map[string]bool{"origin": true}) + require.False(t, plan.add) + require.False(t, plan.noop) + require.Equal(t, forgeURL, plan.replacedURL) + require.Equal(t, "upstream", plan.preserveAs) + }) + + t.Run("skips preserving when the upstream name is taken", func(t *testing.T) { + t.Parallel() + plan := planMirrorRemote("origin", mirrorURL, forgeURL, "upstream", + map[string]bool{"origin": true, "upstream": true}) + require.Equal(t, forgeURL, plan.replacedURL) + require.Empty(t, plan.preserveAs, "an existing upstream must not be clobbered") + }) + + t.Run("skips preserving when disabled", func(t *testing.T) { + t.Parallel() + plan := planMirrorRemote("origin", mirrorURL, forgeURL, "", map[string]bool{"origin": true}) + require.Equal(t, forgeURL, plan.replacedURL) + require.Empty(t, plan.preserveAs) + }) + + t.Run("skips preserving onto itself", func(t *testing.T) { + t.Parallel() + plan := planMirrorRemote("origin", mirrorURL, forgeURL, "origin", map[string]bool{"origin": true}) + require.Empty(t, plan.preserveAs) + }) + + t.Run("noop when already pointing at the mirror", func(t *testing.T) { + t.Parallel() + plan := planMirrorRemote("origin", mirrorURL, mirrorURL, "upstream", map[string]bool{"origin": true}) + require.True(t, plan.noop) + require.Empty(t, plan.preserveAs) + }) + + t.Run("noop tolerates surrounding whitespace and case", func(t *testing.T) { + t.Parallel() + plan := planMirrorRemote("origin", mirrorURL, " "+strings.ToUpper(mirrorURL)+" ", "upstream", + map[string]bool{"origin": true}) + require.True(t, plan.noop) + }) +} + +// applyPlanRepo is a temp git repo with the given remotes configured, for the +// apply-path tests. +func applyPlanRepo(t *testing.T, remotes map[string]string) string { + t.Helper() + dir := t.TempDir() + testutil.InitRepo(t, dir) + for name, url := range remotes { + cmd := exec.CommandContext(t.Context(), "git", "remote", "add", name, url) + cmd.Dir = dir + require.NoError(t, cmd.Run(), "add remote %q", name) + } + return dir +} + +func remoteURL(t *testing.T, dir, name string) string { + t.Helper() + cmd := exec.CommandContext(t.Context(), "git", "remote", "get-url", name) + cmd.Dir = dir + out, err := cmd.Output() + require.NoError(t, err, "get-url %q", name) + return strings.TrimSpace(string(out)) +} + +func TestApplyMirrorRemotePlan(t *testing.T) { + t.Parallel() + const mirrorURL = "entire://aws-us-east-2.entire.io/gh/octocat/hello-world" + const forgeURL = "git@github.com:octocat/hello-world.git" + + t.Run("replace preserves the old URL under upstream", func(t *testing.T) { + t.Parallel() + dir := applyPlanRepo(t, map[string]string{"origin": forgeURL}) + plan := planMirrorRemote("origin", mirrorURL, forgeURL, "upstream", map[string]bool{"origin": true}) + require.NoError(t, applyMirrorRemotePlan(t.Context(), dir, plan)) + require.Equal(t, mirrorURL, remoteURL(t, dir, "origin")) + require.Equal(t, forgeURL, remoteURL(t, dir, "upstream")) + }) + + t.Run("add creates a side remote and leaves origin alone", func(t *testing.T) { + t.Parallel() + dir := applyPlanRepo(t, map[string]string{"origin": forgeURL}) + plan := planMirrorRemote("entire", mirrorURL, "", "upstream", map[string]bool{"origin": true}) + require.NoError(t, applyMirrorRemotePlan(t.Context(), dir, plan)) + require.Equal(t, mirrorURL, remoteURL(t, dir, "entire")) + require.Equal(t, forgeURL, remoteURL(t, dir, "origin"), "origin must be untouched") + }) + + t.Run("replace without preserving discards the old URL", func(t *testing.T) { + t.Parallel() + dir := applyPlanRepo(t, map[string]string{"origin": forgeURL}) + plan := planMirrorRemote("origin", mirrorURL, forgeURL, "", map[string]bool{"origin": true}) + require.NoError(t, applyMirrorRemotePlan(t.Context(), dir, plan)) + require.Equal(t, mirrorURL, remoteURL(t, dir, "origin")) + cmd := exec.CommandContext(t.Context(), "git", "remote", "get-url", "upstream") + cmd.Dir = dir + require.Error(t, cmd.Run(), "no upstream remote should have been created") + }) + + t.Run("noop writes nothing", func(t *testing.T) { + t.Parallel() + dir := applyPlanRepo(t, map[string]string{"origin": mirrorURL}) + plan := planMirrorRemote("origin", mirrorURL, mirrorURL, "upstream", map[string]bool{"origin": true}) + require.NoError(t, applyMirrorRemotePlan(t.Context(), dir, plan)) + require.Equal(t, mirrorURL, remoteURL(t, dir, "origin")) + cmd := exec.CommandContext(t.Context(), "git", "remote", "get-url", "upstream") + cmd.Dir = dir + require.Error(t, cmd.Run()) + }) + + t.Run("a failed preserve leaves the target URL intact", func(t *testing.T) { + t.Parallel() + dir := applyPlanRepo(t, map[string]string{"origin": forgeURL}) + // preserveAs collides with the existing origin, so `git remote add` + // fails. The target must not have been rewritten. + plan := mirrorRemotePlan{ + remote: "origin", + mirrorURL: mirrorURL, + replacedURL: forgeURL, + preserveAs: "origin", + } + require.Error(t, applyMirrorRemotePlan(t.Context(), dir, plan)) + require.Equal(t, forgeURL, remoteURL(t, dir, "origin")) + }) +} + +func TestListGitRemotes(t *testing.T) { + t.Parallel() + dir := applyPlanRepo(t, map[string]string{ + "origin": "git@github.com:octocat/hello-world.git", + "upstream": "https://github.com/octocat/hello-world", + }) + remotes, err := listGitRemotes(t.Context(), dir) + require.NoError(t, err) + require.Equal(t, map[string]bool{"origin": true, "upstream": true}, remotes) +} + +func TestListGitRemotes_NoRemotes(t *testing.T) { + t.Parallel() + dir := applyPlanRepo(t, nil) + remotes, err := listGitRemotes(t.Context(), dir) + require.NoError(t, err) + require.Empty(t, remotes) +} + +func TestResolveMirrorUseUpstream(t *testing.T) { + t.Parallel() + tests := []struct { + name string + // remotes configures the repo's remotes before resolving. + remotes map[string]string + // remote is the write target passed to resolveMirrorUseUpstream; + // defaults to "origin" when empty. + remote string + arg string + wantOwner string + wantRepo string + wantErr string + }{ + { + name: "explicit github url wins over origin", + remotes: map[string]string{"origin": "git@github.com:other/repo.git"}, + arg: "github.com/OctoCat/Hello-World", + wantOwner: "octocat", wantRepo: "hello-world", + }, + { + name: "derives from an ssh origin", + remotes: map[string]string{"origin": "git@github.com:OctoCat/Hello-World.git"}, + wantOwner: "octocat", wantRepo: "hello-world", + }, + { + name: "derives from an https origin", + remotes: map[string]string{"origin": "https://github.com/octocat/hello-world"}, + wantOwner: "octocat", wantRepo: "hello-world", + }, + { + // Re-running `use` on a clone that already goes through a mirror + // must resolve, so switching clusters needs no retyped URL. + name: "derives from an entire origin", + remotes: map[string]string{"origin": "entire://aws-us-east-2.entire.io/gh/octocat/hello-world"}, + wantOwner: "octocat", wantRepo: "hello-world", + }, + { + // --remote names the WRITE target, which need not exist yet; repo + // identity must still come from origin. + name: "falls back to origin when the target remote is absent", + remotes: map[string]string{"origin": "git@github.com:octocat/hello-world.git"}, + remote: "entire", + wantOwner: "octocat", wantRepo: "hello-world", + }, + { + // The target remote wins over origin, so re-running on an existing + // side remote resolves from the repo it actually points at. + name: "prefers the target remote over origin", + remotes: map[string]string{ + "origin": "git@github.com:other/other-repo.git", + "entire": "entire://aws-us-east-2.entire.io/gh/octocat/hello-world", + }, + remote: "entire", + wantOwner: "octocat", wantRepo: "hello-world", + }, + { + // A target remote that cannot name an upstream must not shadow a + // perfectly good origin. + name: "falls back to origin when the target remote is not a GitHub repo", + remotes: map[string]string{ + "origin": "git@github.com:octocat/hello-world.git", + "weird": "git@gitlab.com:acme/app.git", + }, + remote: "weird", + wantOwner: "octocat", wantRepo: "hello-world", + }, + { + name: "invalid explicit url errors", + arg: "https://gitlab.com/a/b", + wantErr: "invalid ", + }, + { + name: "no remotes errors with a pointer", + wantErr: "pass the GitHub URL explicitly", + }, + { + name: "non-github origin errors naming the reason", + remotes: map[string]string{"origin": "git@gitlab.com:acme/app.git"}, + wantErr: "GitHub-only", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + remote := cmp.Or(tt.remote, "origin") + owner, repo, err := resolveMirrorUseUpstream(t.Context(), applyPlanRepo(t, tt.remotes), remote, tt.arg) + if tt.wantErr != "" { + require.ErrorContains(t, err, tt.wantErr) + return + } + require.NoError(t, err) + require.Equal(t, tt.wantOwner, owner) + require.Equal(t, tt.wantRepo, repo) + }) + } +} + +func TestReportMirrorRemotePlan(t *testing.T) { + t.Parallel() + const mirrorURL = "entire://aws-us-east-2.entire.io/gh/octocat/hello-world" + + t.Run("replace reports the old URL and the preserve remote", func(t *testing.T) { + t.Parallel() + var b strings.Builder + reportMirrorRemotePlan(&b, mirrorRemotePlan{ + remote: "origin", + mirrorURL: mirrorURL, + replacedURL: "git@github.com:octocat/hello-world.git", + preserveAs: "upstream", + }) + out := b.String() + require.Contains(t, out, "Repointed remote \"origin\"") + require.Contains(t, out, mirrorURL) + require.Contains(t, out, "was: git@github.com:octocat/hello-world.git") + require.Contains(t, out, "as remote \"upstream\"") + require.Contains(t, out, "git fetch origin") + }) + + // Even with no preserve remote, the replaced URL must be printed so the + // previous value stays recoverable from the transcript. + t.Run("replace without preserve still prints the old URL", func(t *testing.T) { + t.Parallel() + var b strings.Builder + reportMirrorRemotePlan(&b, mirrorRemotePlan{ + remote: "origin", + mirrorURL: mirrorURL, + replacedURL: "https://github.com/octocat/hello-world", + }) + out := b.String() + require.Contains(t, out, "was: https://github.com/octocat/hello-world") + require.NotContains(t, out, "Kept the previous URL") + }) + + t.Run("credentials in the replaced URL are redacted", func(t *testing.T) { + t.Parallel() + var b strings.Builder + reportMirrorRemotePlan(&b, mirrorRemotePlan{ + remote: "origin", + mirrorURL: mirrorURL, + replacedURL: "https://user:s3cret@github.com/octocat/hello-world", + }) + out := b.String() + require.NotContains(t, out, "s3cret") + require.Contains(t, out, "github.com/octocat/hello-world") + }) + + t.Run("add reports no replacement", func(t *testing.T) { + t.Parallel() + var b strings.Builder + reportMirrorRemotePlan(&b, mirrorRemotePlan{remote: "entire", mirrorURL: mirrorURL, add: true}) + out := b.String() + require.Contains(t, out, "Added remote \"entire\"") + require.NotContains(t, out, "was:") + require.Contains(t, out, "git fetch entire") + }) + + t.Run("noop reports no change", func(t *testing.T) { + t.Parallel() + var b strings.Builder + reportMirrorRemotePlan(&b, mirrorRemotePlan{remote: "origin", mirrorURL: mirrorURL, noop: true}) + out := b.String() + require.Contains(t, out, "already points at the mirror") + require.NotContains(t, out, "git fetch") + }) +} + +func TestRepoMirrorUseCmd_FlagValidation(t *testing.T) { + t.Parallel() + tests := []struct { + name string + args []string + want string + }{ + {name: "bad remote", args: []string{"--remote", "-f"}, want: "invalid --remote"}, + {name: "bad upstream", args: []string{"--upstream", "bad name"}, want: "invalid --upstream"}, + {name: "bad positional cluster host", args: []string{"github.com/a/b", "not a host"}, want: "invalid cluster host"}, + {name: "bad cluster flag", args: []string{"--cluster", "not a host"}, want: "invalid cluster host"}, + { + name: "positional and flag disagree", + args: []string{"github.com/a/b", "aws-us-east-2.entire.io", "--cluster", "aws-eu-central-1.entire.io"}, + want: "disagree; pass only one", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + cmd := newRepoMirrorUseCmd() + cmd.SetArgs(tt.args) + cmd.SetOut(&strings.Builder{}) + cmd.SetErr(&strings.Builder{}) + err := cmd.ExecuteContext(t.Context()) + require.ErrorContains(t, err, tt.want) + }) + } +} + +// The command must be reachable at `entire repo mirror use`, and must not have +// been registered as hidden. +func TestRepoMirrorUseCmd_Registered(t *testing.T) { + t.Parallel() + var found bool + for _, c := range newRepoMirrorCmd().Commands() { + if c.Name() == "use" { + require.False(t, c.Hidden, "`repo mirror use` must be visible") + found = true + } + } + require.True(t, found, "`use` must be registered under `repo mirror`") +} + +// huh answers an unreadable accessible prompt by writing the FIRST option's +// value and returning a nil error, so an interrupted prompt takes whichever +// branch is listed first. That must be the same outcome the non-interactive path +// produces with the same flags (repoint the target remote, preserving the old +// URL), or a Ctrl+D would silently diverge from the documented default. This +// pins that invariant: the prompt's first branch and the no-prompt path must +// plan identically. +// Not parallel: t.Setenv forces accessible mode process-wide so the prompt takes +// the deterministic text path instead of trying to open a TTY. +func TestPromptMirrorRemoteChoice_FirstOptionMatchesNonInteractive(t *testing.T) { + t.Setenv("ACCESSIBLE", "1") + const mirrorURL = "entire://aws-us-east-2.entire.io/gh/octocat/hello-world" + const forgeURL = "git@github.com:octocat/hello-world.git" + remotes := map[string]bool{"origin": true} + + cmd := newRepoMirrorUseCmd() + cmd.SetOut(&strings.Builder{}) + cmd.SetErr(&strings.Builder{}) + cmd.SetContext(t.Context()) + + // Runs with stdin at EOF under `go test`, so huh selects the first option. + choice, err := promptMirrorRemoteChoice(cmd, "origin", forgeURL, mirrorURL, "upstream", remotes) + require.NoError(t, err) + + fromPrompt := planMirrorRemote(choice.remote, mirrorURL, forgeURL, choice.upstream, remotes) + fromFlags := planMirrorRemote("origin", mirrorURL, forgeURL, "upstream", remotes) + require.Equal(t, fromFlags, fromPrompt, + "the prompt's first option must plan the same writes as the non-interactive path") + require.False(t, fromPrompt.add, "the first option must repoint, not add") + require.Equal(t, "upstream", fromPrompt.preserveAs, "the replaced URL must still be preserved") +} + +// gitRunner is the single chokepoint for the command's git writes; a failure +// must surface rather than being reported as success. +func TestApplyMirrorRemotePlan_GitFailureSurfaces(t *testing.T) { + t.Parallel() + plan := mirrorRemotePlan{remote: "origin", mirrorURL: "entire://h/gh/a/b"} + // A path that is not a git repository makes `git remote set-url` fail. + err := applyMirrorRemotePlan(context.Background(), t.TempDir(), plan) + require.ErrorContains(t, err, "point remote \"origin\" at the mirror") +} From e7ba3209f7fbec7506d04850e07d6a12a8423f28 Mon Sep 17 00:00:00 2001 From: Matthias Wenz Date: Fri, 31 Jul 2026 11:23:30 +0200 Subject: [PATCH 2/3] fix(repo): warn when `mirror use` cannot preserve the replaced URL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses trail 955 finding (medium): in a fork checkout — `origin` plus `upstream` both already configured, which is the common layout, not an edge case — repointing `origin` found `--upstream` occupied, skipped preservation to avoid clobbering it, and said nothing. The run printed a bare ✓ while the forge URL left `.git/config` for good, surviving only on the `was:` line. The absence of the "Kept the previous URL" line was the only signal, which a reader (or an agent scanning for ✓) would miss. Behavior is unchanged — an existing remote is still never clobbered. The skip is now recorded on the plan (`preserveSkipped`) and reported as an explicit stderr warning naming the occupied remote and carrying the URL needed to recover it. `--upstream ''` stays silent: that is an explicit opt-out, not a skipped preservation. The recovery hint redacts credentials like the `was:` line does, and says so, since this warning is as likely to land in a log as anything else printed. Reproduced before the fix (old origin URL absent from .git/config with no warning) and verified after, in a temp repo against the live API. Co-Authored-By: Claude Opus 5 (1M context) Entire-Checkpoint: 01KYVQRVPDB3J6SWCNVKECHKFM --- cmd/entire/cli/repo_mirror_use.go | 46 +++++++++++-- cmd/entire/cli/repo_mirror_use_test.go | 90 +++++++++++++++++++------- 2 files changed, 107 insertions(+), 29 deletions(-) diff --git a/cmd/entire/cli/repo_mirror_use.go b/cmd/entire/cli/repo_mirror_use.go index 37e10f62c0..73a8d739df 100644 --- a/cmd/entire/cli/repo_mirror_use.go +++ b/cmd/entire/cli/repo_mirror_use.go @@ -101,6 +101,13 @@ type mirrorRemotePlan struct { // preserveAs, when non-empty, is a new remote that will be created holding // replacedURL so the previous URL stays reachable. preserveAs string + // preserveSkipped names the remote replacedURL would have been kept under, + // when preservation was asked for but could not be done (the name is already + // taken). Mutually exclusive with preserveAs, and empty when preservation was + // never requested (`--upstream ''`). Set so the report can say out loud that + // the previous URL did not make it into git config — the difference matters: + // this is the one path where a successful-looking run drops the old URL. + preserveSkipped string // noop is true when remote already points at mirrorURL. noop bool } @@ -111,9 +118,11 @@ type mirrorRemotePlan struct { // // upstream is the requested preserve-under name; it is honored only when the // target remote is actually being repointed and the name is free. An occupied -// name is skipped rather than clobbered — the replaced URL is echoed either way, -// so nothing is lost, and silently rewriting an existing `upstream` would be the -// one genuinely destructive thing this command could do. +// name is never clobbered — silently rewriting an existing `upstream` would be +// the one genuinely destructive thing this command could do — but it is recorded +// in preserveSkipped rather than dropped quietly, because a fork checkout +// (`origin` + `upstream` both already configured) hits that path by default and +// would otherwise see a clean ✓ while the replaced URL left git config for good. func planMirrorRemote(remote, mirrorURL, currentURL, upstream string, remotes map[string]bool) mirrorRemotePlan { plan := mirrorRemotePlan{remote: remote, mirrorURL: mirrorURL} if !remotes[remote] { @@ -125,8 +134,14 @@ func planMirrorRemote(remote, mirrorURL, currentURL, upstream string, remotes ma return plan } plan.replacedURL = currentURL - if upstream != "" && upstream != remote && !remotes[upstream] { - plan.preserveAs = upstream + if upstream != "" { + // `remote` is known to exist in this branch, so an upstream naming it is + // "occupied" too and lands in the skipped case — no separate check needed. + if remotes[upstream] { + plan.preserveSkipped = upstream + } else { + plan.preserveAs = upstream + } } return plan } @@ -155,7 +170,13 @@ func applyMirrorRemotePlan(ctx context.Context, dir string, plan mirrorRemotePla // reportMirrorRemotePlan echoes what was written, in recovery-friendly terms: // every replaced URL is printed even when it was also preserved under another // remote, so the previous value is always visible in the transcript. -func reportMirrorRemotePlan(out io.Writer, plan mirrorRemotePlan) { +// +// When preservation was requested but skipped, that gets an explicit stderr +// warning rather than just the absence of the "Kept the previous URL" line — the +// old URL is then only in this output, and an omitted line is far too quiet a +// signal for "your previous remote URL is no longer in git config" (a reader, or +// an agent scanning for ✓, would miss it). +func reportMirrorRemotePlan(out, errW io.Writer, plan mirrorRemotePlan) { if plan.noop { fmt.Fprintf(out, "Remote %q already points at the mirror:\n %s\n", plan.remote, plan.mirrorURL) return @@ -170,6 +191,17 @@ func reportMirrorRemotePlan(out io.Writer, plan mirrorRemotePlan) { } } fmt.Fprintf(out, "\nFetch through it:\n git fetch %s\n", plan.remote) + + if plan.preserveSkipped != "" { + // The URL is redacted here for the same reason it is on the "was:" line: + // a replaced URL can carry credentials, and this warning is as likely to + // end up in a log or a pasted transcript as anything else we print. Say so, + // so a reader who needs the credentialed original knows to reconstruct it. + fmt.Fprintf(errW, "\nWARNING: the previous URL of %q was NOT saved to git config — remote %q already exists.\n", plan.remote, plan.preserveSkipped) + fmt.Fprintf(errW, " It now only appears in the output above. To keep it under another name:\n") + fmt.Fprintf(errW, " git remote add %s\n", gitremote.RedactURL(plan.replacedURL)) + fmt.Fprintf(errW, " (credentials, if the URL had any, are redacted and must be re-supplied.)\n") + } } // mirrorUseChoice is the outcome of the interactive replace-or-add prompt. @@ -480,7 +512,7 @@ func newRepoMirrorUseCmd() *cobra.Command { if err := applyMirrorRemotePlan(ctx, repoRoot, plan); err != nil { return err } - reportMirrorRemotePlan(cmd.OutOrStdout(), plan) + reportMirrorRemotePlan(cmd.OutOrStdout(), cmd.ErrOrStderr(), plan) return nil }, } diff --git a/cmd/entire/cli/repo_mirror_use_test.go b/cmd/entire/cli/repo_mirror_use_test.go index ae855dfe7d..87f4a206b9 100644 --- a/cmd/entire/cli/repo_mirror_use_test.go +++ b/cmd/entire/cli/repo_mirror_use_test.go @@ -70,25 +70,48 @@ func TestPlanMirrorRemote(t *testing.T) { require.Equal(t, "upstream", plan.preserveAs) }) - t.Run("skips preserving when the upstream name is taken", func(t *testing.T) { + // The fork layout (origin + upstream both configured) hits this by default, + // so the skip must be recorded for the report to warn about — not silently + // dropped, which would leave a clean ✓ over a lost URL. + t.Run("records the skip when the upstream name is taken", func(t *testing.T) { t.Parallel() plan := planMirrorRemote("origin", mirrorURL, forgeURL, "upstream", map[string]bool{"origin": true, "upstream": true}) require.Equal(t, forgeURL, plan.replacedURL) require.Empty(t, plan.preserveAs, "an existing upstream must not be clobbered") + require.Equal(t, "upstream", plan.preserveSkipped) }) - t.Run("skips preserving when disabled", func(t *testing.T) { + // `--upstream ''` is an explicit opt-out, so there is nothing to warn about. + t.Run("skips preserving silently when disabled", func(t *testing.T) { t.Parallel() plan := planMirrorRemote("origin", mirrorURL, forgeURL, "", map[string]bool{"origin": true}) require.Equal(t, forgeURL, plan.replacedURL) require.Empty(t, plan.preserveAs) + require.Empty(t, plan.preserveSkipped, "an explicit opt-out is not a skipped preservation") }) - t.Run("skips preserving onto itself", func(t *testing.T) { + t.Run("records the skip when preserving onto itself", func(t *testing.T) { t.Parallel() plan := planMirrorRemote("origin", mirrorURL, forgeURL, "origin", map[string]bool{"origin": true}) require.Empty(t, plan.preserveAs) + require.Equal(t, "origin", plan.preserveSkipped) + }) + + t.Run("a successful preserve records no skip", func(t *testing.T) { + t.Parallel() + plan := planMirrorRemote("origin", mirrorURL, forgeURL, "upstream", map[string]bool{"origin": true}) + require.Equal(t, "upstream", plan.preserveAs) + require.Empty(t, plan.preserveSkipped) + }) + + // add/noop never replace anything, so neither can strand a URL. + t.Run("add and noop never record a skip", func(t *testing.T) { + t.Parallel() + add := planMirrorRemote("entire", mirrorURL, "", "upstream", map[string]bool{"origin": true, "upstream": true}) + require.Empty(t, add.preserveSkipped) + noop := planMirrorRemote("origin", mirrorURL, mirrorURL, "upstream", map[string]bool{"origin": true, "upstream": true}) + require.Empty(t, noop.preserveSkipped) }) t.Run("noop when already pointing at the mirror", func(t *testing.T) { @@ -311,68 +334,91 @@ func TestReportMirrorRemotePlan(t *testing.T) { t.Parallel() const mirrorURL = "entire://aws-us-east-2.entire.io/gh/octocat/hello-world" + // report returns the plan's stdout and stderr separately. + report := func(plan mirrorRemotePlan) (stdout, stderr string) { + var o, e strings.Builder + reportMirrorRemotePlan(&o, &e, plan) + return o.String(), e.String() + } + t.Run("replace reports the old URL and the preserve remote", func(t *testing.T) { t.Parallel() - var b strings.Builder - reportMirrorRemotePlan(&b, mirrorRemotePlan{ + out, errOut := report(mirrorRemotePlan{ remote: "origin", mirrorURL: mirrorURL, replacedURL: "git@github.com:octocat/hello-world.git", preserveAs: "upstream", }) - out := b.String() require.Contains(t, out, "Repointed remote \"origin\"") require.Contains(t, out, mirrorURL) require.Contains(t, out, "was: git@github.com:octocat/hello-world.git") require.Contains(t, out, "as remote \"upstream\"") require.Contains(t, out, "git fetch origin") + require.Empty(t, errOut, "a successful preserve warns about nothing") }) // Even with no preserve remote, the replaced URL must be printed so the // previous value stays recoverable from the transcript. t.Run("replace without preserve still prints the old URL", func(t *testing.T) { t.Parallel() - var b strings.Builder - reportMirrorRemotePlan(&b, mirrorRemotePlan{ + out, errOut := report(mirrorRemotePlan{ remote: "origin", mirrorURL: mirrorURL, replacedURL: "https://github.com/octocat/hello-world", }) - out := b.String() require.Contains(t, out, "was: https://github.com/octocat/hello-world") require.NotContains(t, out, "Kept the previous URL") + require.Empty(t, errOut, "an explicit --upstream '' opt-out is not warned about") }) - t.Run("credentials in the replaced URL are redacted", func(t *testing.T) { + // The finding this guards: a skipped preservation must be stated outright, not + // signalled by the absence of the "Kept the previous URL" line. + t.Run("a skipped preserve warns loudly on stderr", func(t *testing.T) { t.Parallel() - var b strings.Builder - reportMirrorRemotePlan(&b, mirrorRemotePlan{ - remote: "origin", - mirrorURL: mirrorURL, - replacedURL: "https://user:s3cret@github.com/octocat/hello-world", + out, errOut := report(mirrorRemotePlan{ + remote: "origin", + mirrorURL: mirrorURL, + replacedURL: "git@github.com:octocat/hello-world.git", + preserveSkipped: "upstream", + }) + require.Contains(t, out, "was: git@github.com:octocat/hello-world.git") + require.NotContains(t, out, "Kept the previous URL") + require.Contains(t, errOut, "WARNING") + require.Contains(t, errOut, "NOT saved to git config") + require.Contains(t, errOut, "remote \"upstream\" already exists") + require.Contains(t, errOut, "git remote add git@github.com:octocat/hello-world.git", + "the warning must carry the URL needed to recover it") + }) + + t.Run("credentials are redacted in both the report and the warning", func(t *testing.T) { + t.Parallel() + out, errOut := report(mirrorRemotePlan{ + remote: "origin", + mirrorURL: mirrorURL, + replacedURL: "https://user:s3cret@github.com/octocat/hello-world", + preserveSkipped: "upstream", }) - out := b.String() require.NotContains(t, out, "s3cret") require.Contains(t, out, "github.com/octocat/hello-world") + require.NotContains(t, errOut, "s3cret", "the recovery hint must not leak credentials either") + require.Contains(t, errOut, "redacted") }) t.Run("add reports no replacement", func(t *testing.T) { t.Parallel() - var b strings.Builder - reportMirrorRemotePlan(&b, mirrorRemotePlan{remote: "entire", mirrorURL: mirrorURL, add: true}) - out := b.String() + out, errOut := report(mirrorRemotePlan{remote: "entire", mirrorURL: mirrorURL, add: true}) require.Contains(t, out, "Added remote \"entire\"") require.NotContains(t, out, "was:") require.Contains(t, out, "git fetch entire") + require.Empty(t, errOut) }) t.Run("noop reports no change", func(t *testing.T) { t.Parallel() - var b strings.Builder - reportMirrorRemotePlan(&b, mirrorRemotePlan{remote: "origin", mirrorURL: mirrorURL, noop: true}) - out := b.String() + out, errOut := report(mirrorRemotePlan{remote: "origin", mirrorURL: mirrorURL, noop: true}) require.Contains(t, out, "already points at the mirror") require.NotContains(t, out, "git fetch") + require.Empty(t, errOut) }) } From 69c50e57948532e65b2443e01e3a63fd51cafc69 Mon Sep 17 00:00:00 2001 From: Matthias Wenz Date: Fri, 31 Jul 2026 11:57:15 +0200 Subject: [PATCH 3/3] fix(repo): stop leaking credentials in git argv errors; drop silent no-message exits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses both Copilot review comments on PR #1875. Credential leak (repo_mirror_use.go:66). gitRunner echoed the raw argv into its error, and `git remote add ` passes the user's previous remote URL — which can embed a token. That error is a plain (printed) error, so it reached stderr and from there logs and pasted transcripts. Demonstrated before the fix: preserve current origin URL as "origin": git remote add origin https://user:ghp_SUPERSECRET@github.com/octocat/hello-world: exit status 3 and after: ... git remote add origin https://github.com/octocat/hello-world: exit status 3 New redactGitArgs runs URL-shaped args through gitremote.RedactURL — the same redaction reportMirrorRemotePlan already applied to the printed URL, so the failure path is no longer the one place that leaked. Only URL-shaped args are touched: RedactURL turns a bare word like "remote" into "://remote", so it cannot be applied blanket-fashion. The command and host still survive for diagnosis. Undiagnosable silent exits. The unreachable `default` branch of the replace-or-add prompt returned a SilentError without printing anything, so if it were ever reached the command would exit non-zero with no message at all. Now a plain error. The same defect existed on the analogous path in the shared picker (`selectPlacement`'s `!ok` after a successful form run, pre-existing in selectCloneTarget) — also converted, since silence there is never right. The two remaining SilentError uses are correct: each follows a message already printed. Co-Authored-By: Claude Opus 5 (1M context) Entire-Checkpoint: 01KYVSPNJA0Z7AB5HFRH6V33BS --- cmd/entire/cli/repo_clone.go | 11 ++++-- cmd/entire/cli/repo_mirror_use.go | 31 +++++++++++++-- cmd/entire/cli/repo_mirror_use_test.go | 55 ++++++++++++++++++++++++++ 3 files changed, 89 insertions(+), 8 deletions(-) diff --git a/cmd/entire/cli/repo_clone.go b/cmd/entire/cli/repo_clone.go index 4d7189e091..ea906cf9e1 100644 --- a/cmd/entire/cli/repo_clone.go +++ b/cmd/entire/cli/repo_clone.go @@ -319,20 +319,23 @@ func selectPlacement(cmd *cobra.Command, placements []coreapi.ResolvedPlacement, Value(&selected), ), ) - cancelled := NewSilentError(fmt.Errorf("%s cancelled", strings.ToLower(p.action))) if err := form.RunWithContext(cmd.Context()); err != nil { // handleFormCancellation prints " cancelled." and returns nil for a // Ctrl+C / cancelled-context abort. Surface that as a SilentError so the // caller stops instead of falling through to act on a zero-value target - // (the `entire:///gh/...` empty-host bug); a real form error propagates. + // (the `entire:///gh/...` empty-host bug) without main.go reprinting the + // message handleFormCancellation already wrote; a real form error propagates. if cerr := handleFormCancellation(cmd.ErrOrStderr(), p.action, err); cerr != nil { return coreapi.ResolvedPlacement{}, cerr } - return coreapi.ResolvedPlacement{}, cancelled + return coreapi.ResolvedPlacement{}, NewSilentError(fmt.Errorf("%s cancelled", strings.ToLower(p.action))) } match, ok := byHost[selected] if !ok { - return coreapi.ResolvedPlacement{}, cancelled + // The form succeeded but handed back a host that is not on offer. Nothing + // has been printed here, so this must NOT be a SilentError — main.go + // suppresses those, and the command would exit non-zero with no message. + return coreapi.ResolvedPlacement{}, fmt.Errorf("no cluster selected from the %d offered", len(hosts)) } return match, nil } diff --git a/cmd/entire/cli/repo_mirror_use.go b/cmd/entire/cli/repo_mirror_use.go index 73a8d739df..e5aff6a972 100644 --- a/cmd/entire/cli/repo_mirror_use.go +++ b/cmd/entire/cli/repo_mirror_use.go @@ -56,6 +56,26 @@ func validateGitRemoteName(name string) error { return nil } +// redactGitArgs returns args with anything that could carry credentials replaced +// by its redacted form, so the argv echoed in an error message is safe to print. +// A replaced remote URL can embed a token (https://user:token@host/...), and +// these errors reach stderr through main.go and from there into logs and pasted +// transcripts — the same reason reportMirrorRemotePlan redacts what it prints. +// +// Only URL-shaped args are touched: gitremote.RedactURL would turn a bare word +// like "remote" into "://remote", so it cannot be applied blanket-fashion. +func redactGitArgs(args []string) []string { + safe := make([]string, len(args)) + for i, a := range args { + if strings.Contains(a, "://") || strings.Contains(a, "@") { + safe[i] = gitremote.RedactURL(a) + continue + } + safe[i] = a + } + return safe +} + // gitRunner runs a git subcommand in dir. A package var so tests exercise the // planning and prompt logic without mutating a real repository's config. var gitRunner = func(ctx context.Context, dir string, args ...string) (string, error) { @@ -63,7 +83,7 @@ var gitRunner = func(ctx context.Context, dir string, args ...string) (string, e cmd.Dir = dir out, err := cmd.Output() if err != nil { - return "", fmt.Errorf("git %s: %w", strings.Join(args, " "), err) + return "", fmt.Errorf("git %s: %w", strings.Join(redactGitArgs(args), " "), err) } return strings.TrimSpace(string(out)), nil } @@ -267,9 +287,12 @@ func promptMirrorRemoteChoice(cmd *cobra.Command, remote, currentURL, mirrorURL, case choiceAdd: // fall through to the name prompt default: - // Unreachable with the options above (huh always writes one of them). - // Kept so an unrecognised value can never fall through into a write. - return mirrorUseChoice{}, NewSilentError(errors.New("no remote update selected")) + // Unreachable with the options above (huh always writes one of them), and + // kept so an unrecognised value can never fall through into a write. + // Deliberately a plain error, not a SilentError: nothing has been printed + // on this path, and main.go suppresses SilentError — so a silent one would + // exit non-zero with no message at all, which is undiagnosable. + return mirrorUseChoice{}, errors.New("no remote update selected") } name := sideName diff --git a/cmd/entire/cli/repo_mirror_use_test.go b/cmd/entire/cli/repo_mirror_use_test.go index 87f4a206b9..ad99f67d6e 100644 --- a/cmd/entire/cli/repo_mirror_use_test.go +++ b/cmd/entire/cli/repo_mirror_use_test.go @@ -46,6 +46,39 @@ func TestValidateGitRemoteName(t *testing.T) { } } +func TestRedactGitArgs(t *testing.T) { + t.Parallel() + got := redactGitArgs([]string{ + "remote", "add", "upstream", + "https://user:ghp_SECRET@github.com/octocat/hello-world", + }) + require.Equal(t, []string{ + "remote", "add", "upstream", + "https://github.com/octocat/hello-world", + }, got) + + t.Run("leaves non-URL args untouched", func(t *testing.T) { + t.Parallel() + // RedactURL would mangle bare words into "://word", so they must be + // passed through rather than redacted blanket-fashion. + require.Equal(t, []string{"remote"}, redactGitArgs([]string{"remote"})) + require.Equal(t, + []string{"remote", "set-url", "origin"}, + redactGitArgs([]string{"remote", "set-url", "origin"})) + }) + + t.Run("passes through URL forms that carry no credentials", func(t *testing.T) { + t.Parallel() + require.Equal(t, + []string{"entire://aws-us-east-2.entire.io/gh/octocat/hello-world"}, + redactGitArgs([]string{"entire://aws-us-east-2.entire.io/gh/octocat/hello-world"})) + // SCP-style has no embeddable credentials; the "@" must not mangle it. + require.Equal(t, + []string{"git@github.com:octocat/hello-world.git"}, + redactGitArgs([]string{"git@github.com:octocat/hello-world.git"})) + }) +} + func TestPlanMirrorRemote(t *testing.T) { t.Parallel() const mirrorURL = "entire://aws-us-east-2.entire.io/gh/octocat/hello-world" @@ -197,6 +230,28 @@ func TestApplyMirrorRemotePlan(t *testing.T) { require.Error(t, cmd.Run()) }) + // A failing `git remote add` echoes its argv into the error, and that error is + // a plain (printed) error — so a credentialed replaced URL must not survive + // into it. Guards the same property reportMirrorRemotePlan already has. + t.Run("a failed git command does not leak credentials from the argv", func(t *testing.T) { + t.Parallel() + dir := applyPlanRepo(t, map[string]string{"origin": "git@github.com:octocat/hello-world.git"}) + plan := mirrorRemotePlan{ + remote: "origin", + mirrorURL: mirrorURL, + replacedURL: "https://user:ghp_SUPERSECRET@github.com/octocat/hello-world", + // Collides with the existing origin, so `git remote add` fails. + preserveAs: "origin", + } + err := applyMirrorRemotePlan(t.Context(), dir, plan) + require.Error(t, err) + require.NotContains(t, err.Error(), "ghp_SUPERSECRET", "credentials must not reach the error message") + require.NotContains(t, err.Error(), "user:", "userinfo must not reach the error message") + // Still useful for diagnosis: the command and the host survive. + require.Contains(t, err.Error(), "git remote add") + require.Contains(t, err.Error(), "github.com/octocat/hello-world") + }) + t.Run("a failed preserve leaves the target URL intact", func(t *testing.T) { t.Parallel() dir := applyPlanRepo(t, map[string]string{"origin": forgeURL})