From c35ccb79715c015a9cdff4f45c4461a6cc7cc69c Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Fri, 30 Oct 2026 15:48:25 -0400 Subject: [PATCH 1/2] feat(review): warn when a profile agent source is behind its upstream Profile agent sources are local checkouts, so a reviewer merged into a shared catalog does not run until that checkout is pulled, and nothing said so. Before a review, fetch each Git-backed source's upstream and warn with the fast-forward command when the checkout is behind. Note sources cr cannot check. The check never fails a review. Closes #697 --- README.md | 9 + internal/agentfreshness/agentfreshness.go | 171 +++++++++++++++++ .../agentfreshness/agentfreshness_test.go | 176 ++++++++++++++++++ internal/app/runtime.go | 23 ++- internal/app/runtime_test.go | 37 ++++ internal/cmd/reviewcmd/reviewcmd.go | 1 + internal/cmd/reviewcmd/reviewcmd_test.go | 3 + 7 files changed, 414 insertions(+), 6 deletions(-) create mode 100644 internal/agentfreshness/agentfreshness.go create mode 100644 internal/agentfreshness/agentfreshness_test.go diff --git a/README.md b/README.md index a69dd928..e45bdb0c 100644 --- a/README.md +++ b/README.md @@ -1126,6 +1126,15 @@ Inspects and updates the active profile's `agent_sources`. `add` normalizes the path, skips duplicates, and preserves unrelated sources. `remove` removes only matching normalized paths from the active profile. +Before each `cr review`, cr checks whether every profile agent source is +current. For a source in a Git checkout whose branch tracks an upstream, cr +fetches that upstream (up to 15 seconds). If the checkout is behind, cr prints +a warning on stderr with the commit count and the `git merge --ff-only` command +that updates it, so reviewers merged upstream don't go missing without notice. +If the fetch fails, cr compares against the last fetched upstream state. A +source outside Git, or on a branch with no upstream, gets a one-line note that +cr cannot check it. The check never fails a review. + ### `cr config retention` ```text diff --git a/internal/agentfreshness/agentfreshness.go b/internal/agentfreshness/agentfreshness.go new file mode 100644 index 00000000..bd06bef1 --- /dev/null +++ b/internal/agentfreshness/agentfreshness.go @@ -0,0 +1,171 @@ +// Package agentfreshness reports profile agent sources whose local checkout may +// not match the shared catalog it tracks. +// +// Profile agent sources are local directories, so a reviewer merged upstream +// does not run until someone updates the checkout. This package makes that +// visible before a review: it fetches each source's upstream branch and reports +// checkouts that are behind, plus sources cr cannot check at all. It never +// fails a review. +package agentfreshness + +import ( + "context" + "errors" + "fmt" + "io" + "os/exec" + "strconv" + "strings" + "time" + + "github.com/open-cli-collective/codereview-cli/internal/agents" +) + +// fetchTimeout bounds the upstream fetch so an unreachable remote delays a +// review by seconds, not minutes. +const fetchTimeout = 15 * time.Second + +// GitCommand runs git in dir and returns its standard output. +type GitCommand func(ctx context.Context, dir string, args ...string) ([]byte, error) + +// Kind classifies a notice. +type Kind string + +const ( + // KindUnversioned reports a source outside any Git worktree. + KindUnversioned Kind = "unversioned" + // KindNoUpstream reports a source whose checked-out branch tracks no upstream. + KindNoUpstream Kind = "no_upstream" + // KindBehind reports a checkout missing commits from its upstream. + KindBehind Kind = "behind" +) + +// Notice describes one agent source that may not reflect its shared catalog. +type Notice struct { + Kind Kind + Source string + RepoRoot string + Upstream string + Behind int + // FetchError is set when the upstream could not be fetched, so Behind was + // counted against the last fetched upstream state. + FetchError string +} + +// Warn checks dirs with the host git and writes any notices to w. A nil w +// skips the check entirely. +func Warn(ctx context.Context, dirs []string, w io.Writer) { + if w == nil || len(dirs) == 0 { + return + } + Write(w, Check(ctx, dirs, defaultGitCommand)) +} + +// Check reports sources that are unversioned, track no upstream, or are behind +// their upstream. Missing or unreadable sources are left to the agent loader, +// and sources sharing a checkout are checked once. +func Check(ctx context.Context, dirs []string, run GitCommand) []Notice { + var notices []Notice + seenSources := map[string]bool{} + seenRepos := map[string]bool{} + for _, source := range agents.InspectProfileSources(dirs) { + if ctx.Err() != nil { + return notices + } + if source.CanonicalPath == "" || seenSources[source.CanonicalPath] { + continue + } + seenSources[source.CanonicalPath] = true + out, err := run(ctx, source.CanonicalPath, "rev-parse", "--show-toplevel") + if err != nil { + notices = append(notices, Notice{Kind: KindUnversioned, Source: source.ConfiguredPath}) + continue + } + root := strings.TrimSpace(string(out)) + if seenRepos[root] { + continue + } + seenRepos[root] = true + if notice, ok := checkRepo(ctx, run, source.ConfiguredPath, root); ok { + notices = append(notices, notice) + } + } + return notices +} + +func checkRepo(ctx context.Context, run GitCommand, source, root string) (Notice, bool) { + out, err := run(ctx, root, "rev-parse", "--abbrev-ref", "--symbolic-full-name", "@{upstream}") + if err != nil { + return Notice{Kind: KindNoUpstream, Source: source, RepoRoot: root}, true + } + upstream := strings.TrimSpace(string(out)) + fetchCtx, cancel := context.WithTimeout(ctx, fetchTimeout) + _, fetchErr := run(fetchCtx, root, "fetch", "--quiet", "--no-tags") + cancel() + out, err = run(ctx, root, "rev-list", "--count", "HEAD..@{upstream}") + if err != nil { + return Notice{}, false + } + behind, err := strconv.Atoi(strings.TrimSpace(string(out))) + if err != nil || behind == 0 { + return Notice{}, false + } + notice := Notice{Kind: KindBehind, Source: source, RepoRoot: root, Upstream: upstream, Behind: behind} + if fetchErr != nil { + notice.FetchError = fetchErr.Error() + } + return notice, true +} + +// Write renders notices as one line each. +func Write(w io.Writer, notices []Notice) { + for _, notice := range notices { + _, _ = fmt.Fprintln(w, notice.String()) + } +} + +func (n Notice) String() string { + switch n.Kind { + case KindUnversioned: + return fmt.Sprintf("note: agent source %s is not in a Git repository; cr cannot check whether its reviewers are current", n.Source) + case KindNoUpstream: + return fmt.Sprintf("note: agent source %s is in %s, whose branch tracks no upstream; cr cannot check whether its reviewers are current", n.Source, n.RepoRoot) + case KindBehind: + line := fmt.Sprintf("warning: agent source %s is %d %s behind %s; reviewer changes merged there will not run. Update with: git -C %s merge --ff-only %s", + n.Source, n.Behind, plural(n.Behind, "commit", "commits"), n.Upstream, n.RepoRoot, n.Upstream) + if n.FetchError != "" { + reason, _, _ := strings.Cut(n.FetchError, "\n") + line += fmt.Sprintf(" (fetch failed, counted from the last fetch: %s)", reason) + } + return line + default: + return fmt.Sprintf("note: agent source %s: %s", n.Source, n.Kind) + } +} + +func plural(n int, one, many string) string { + if n == 1 { + return one + } + return many +} + +func defaultGitCommand(ctx context.Context, dir string, args ...string) ([]byte, error) { + cmd := exec.CommandContext(ctx, "git", args...) // #nosec G204 -- fixed git command with structured arguments. + cmd.Dir = dir + cmd.Env = append(cmd.Environ(), "LC_ALL=C", "LANG=C", "GIT_TERMINAL_PROMPT=0") + output, err := cmd.Output() + if err != nil { + if ctxErr := ctx.Err(); ctxErr != nil { + return nil, ctxErr + } + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + if message := strings.TrimSpace(string(exitErr.Stderr)); message != "" { + return nil, fmt.Errorf("git %s: %s", strings.Join(args, " "), message) + } + } + return nil, fmt.Errorf("git %s: %w", strings.Join(args, " "), err) + } + return output, nil +} diff --git a/internal/agentfreshness/agentfreshness_test.go b/internal/agentfreshness/agentfreshness_test.go new file mode 100644 index 00000000..e5de49cf --- /dev/null +++ b/internal/agentfreshness/agentfreshness_test.go @@ -0,0 +1,176 @@ +package agentfreshness + +import ( + "bytes" + "context" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/open-cli-collective/codereview-cli/internal/gittest" +) + +func TestCheckReportsCheckoutBehindItsUpstream(t *testing.T) { + upstream, clone := catalogClone(t) + pushCommits(t, upstream, 2) + + notices := Check(context.Background(), []string{agentsDir(t, clone)}, testGit) + + if len(notices) != 1 { + t.Fatalf("notices = %+v, want one", notices) + } + got := notices[0] + if got.Kind != KindBehind || got.Behind != 2 || got.Upstream != "origin/main" || got.FetchError != "" { + t.Fatalf("notice = %+v, want behind origin/main by 2 after a successful fetch", got) + } + line := got.String() + if !strings.Contains(line, "2 commits behind origin/main") || !strings.Contains(line, "merge --ff-only origin/main") { + t.Fatalf("line = %q, want behind count and fast-forward command", line) + } +} + +func TestCheckIsQuietForCurrentCheckout(t *testing.T) { + _, clone := catalogClone(t) + + if notices := Check(context.Background(), []string{agentsDir(t, clone)}, testGit); len(notices) != 0 { + t.Fatalf("notices = %+v, want none", notices) + } +} + +func TestCheckFallsBackToLastFetchWhenUpstreamIsUnreachable(t *testing.T) { + upstream, clone := catalogClone(t) + pushCommits(t, upstream, 1) + git(t, clone, "fetch", "--quiet") + git(t, clone, "remote", "set-url", "origin", filepath.Join(t.TempDir(), "gone.git")) + + notices := Check(context.Background(), []string{agentsDir(t, clone)}, testGit) + + if len(notices) != 1 || notices[0].Kind != KindBehind || notices[0].Behind != 1 || notices[0].FetchError == "" { + t.Fatalf("notices = %+v, want behind by 1 with a fetch error", notices) + } + if line := notices[0].String(); strings.Contains(line, "\n") || !strings.Contains(line, "1 commit behind") { + t.Fatalf("line = %q, want a single line naming 1 commit", line) + } +} + +func TestCheckIsQuietWhenUpstreamIsUnreachableAndLastFetchIsCurrent(t *testing.T) { + _, clone := catalogClone(t) + git(t, clone, "remote", "set-url", "origin", filepath.Join(t.TempDir(), "gone.git")) + + if notices := Check(context.Background(), []string{agentsDir(t, clone)}, testGit); len(notices) != 0 { + t.Fatalf("notices = %+v, want none", notices) + } +} + +func TestCheckNotesUnversionedAndUntrackedSources(t *testing.T) { + plain := t.TempDir() + local := t.TempDir() + git(t, local, "init", "--quiet", "--initial-branch=main") + commitFile(t, local, "agents/a.md") + + notices := Check(context.Background(), []string{plain, filepath.Join(local, "agents")}, testGit) + + if len(notices) != 2 || notices[0].Kind != KindUnversioned || notices[1].Kind != KindNoUpstream { + t.Fatalf("notices = %+v, want unversioned then no-upstream", notices) + } + if line := notices[0].String(); !strings.Contains(line, "not in a Git repository") || !strings.HasPrefix(line, "note: ") { + t.Fatalf("line = %q, want an unversioned note", line) + } + if line := notices[1].String(); !strings.Contains(line, "tracks no upstream") || !strings.HasPrefix(line, "note: ") { + t.Fatalf("line = %q, want a no-upstream note", line) + } +} + +func TestCheckSkipsMissingSourcesAndChecksEachCheckoutOnce(t *testing.T) { + upstream, clone := catalogClone(t) + pushCommits(t, upstream, 1) + dir := agentsDir(t, clone) + other := filepath.Join(clone, "more-agents") + if err := os.MkdirAll(other, 0o750); err != nil { + t.Fatal(err) + } + var fetches int + run := func(ctx context.Context, dir string, args ...string) ([]byte, error) { + if args[0] == "fetch" { + fetches++ + } + return testGit(ctx, dir, args...) + } + + notices := Check(context.Background(), []string{filepath.Join(t.TempDir(), "missing"), dir, dir, other}, run) + + if len(notices) != 1 || notices[0].Kind != KindBehind || fetches != 1 { + t.Fatalf("notices = %+v, fetches = %d; want one behind notice from one fetch", notices, fetches) + } +} + +func TestWarnSkipsCheckWithoutWriter(t *testing.T) { + Warn(context.Background(), []string{t.TempDir()}, nil) + + var out bytes.Buffer + Warn(context.Background(), []string{t.TempDir()}, &out) + if !strings.Contains(out.String(), "not in a Git repository") { + t.Fatalf("output = %q, want unversioned note", out.String()) + } +} + +func testGit(ctx context.Context, dir string, args ...string) ([]byte, error) { + cmd := exec.CommandContext(ctx, "git", args...) // #nosec G204 -- test git fixture. + cmd.Dir = dir + cmd.Env = append(gittest.Env(), "GIT_TERMINAL_PROMPT=0") + return cmd.Output() +} + +// catalogClone returns a bare upstream with one commit and a clone tracking it. +func catalogClone(t *testing.T) (upstream, clone string) { + t.Helper() + upstream = filepath.Join(t.TempDir(), "catalog.git") + git(t, filepath.Dir(upstream), "init", "--quiet", "--bare", "--initial-branch=main", upstream) + seed := t.TempDir() + git(t, seed, "clone", "--quiet", upstream, ".") + commitFile(t, seed, "agents/a.md") + git(t, seed, "push", "--quiet", "origin", "HEAD:main") + clone = t.TempDir() + git(t, clone, "clone", "--quiet", upstream, ".") + return upstream, clone +} + +func pushCommits(t *testing.T, upstream string, n int) { + t.Helper() + work := t.TempDir() + git(t, work, "clone", "--quiet", upstream, ".") + for i := range n { + commitFile(t, work, filepath.Join("agents", "new-"+string(rune('a'+i))+".md")) + } + git(t, work, "push", "--quiet", "origin", "HEAD:main") +} + +func commitFile(t *testing.T, repo, rel string) { + t.Helper() + path := filepath.Join(repo, rel) + if err := os.MkdirAll(filepath.Dir(path), 0o750); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(rel+"\n"), 0o600); err != nil { + t.Fatal(err) + } + git(t, repo, "add", rel) + git(t, repo, "-c", "user.name=Test", "-c", "user.email=test@example.com", "commit", "--quiet", "-m", rel) +} + +func agentsDir(t *testing.T, clone string) string { + t.Helper() + return filepath.Join(clone, "agents") +} + +func git(t *testing.T, dir string, args ...string) { + t.Helper() + cmd := exec.Command("git", args...) // #nosec G204 -- test git fixture. + cmd.Dir = dir + cmd.Env = gittest.Env() + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %s: %v\n%s", strings.Join(args, " "), err, out) + } +} diff --git a/internal/app/runtime.go b/internal/app/runtime.go index 8964780b..bbf73b8d 100644 --- a/internal/app/runtime.go +++ b/internal/app/runtime.go @@ -13,6 +13,7 @@ import ( "github.com/open-cli-collective/cli-common/credstore" + "github.com/open-cli-collective/codereview-cli/internal/agentfreshness" "github.com/open-cli-collective/codereview-cli/internal/approvaloverride" "github.com/open-cli-collective/codereview-cli/internal/appruntime" "github.com/open-cli-collective/codereview-cli/internal/config" @@ -72,12 +73,15 @@ type OpenRequest struct { PRRef gitprovider.PRRef PRURL string RequireOpinionatedReviewAuthority bool - MaxAgents int - MaxConcurrency int - Retention datalifecycle.RetentionPolicy - RetentionManualOnly bool - KeepWorkbench bool - Dependencies Dependencies + // CheckAgentSourceFreshness reports profile agent sources that are behind + // their upstream or cannot be checked, before the runtime is assembled. + CheckAgentSourceFreshness bool + MaxAgents int + MaxConcurrency int + Retention datalifecycle.RetentionPolicy + RetentionManualOnly bool + KeepWorkbench bool + Dependencies Dependencies } // SelectionOpenRequest carries command-resolved values for selection-only @@ -112,6 +116,7 @@ type Dependencies struct { RuntimeLayout func() (statepaths.Layout, error) OpenLedger func(context.Context, string) (*ledger.Store, error) NewLimiter func() (outbox.Limiter, error) + CheckAgentSources func(context.Context, []string, io.Writer) } func (d Dependencies) withDefaults() Dependencies { @@ -142,6 +147,9 @@ func (d Dependencies) withDefaults() Dependencies { if d.OpenLedger == nil { d.OpenLedger = ledger.Open } + if d.CheckAgentSources == nil { + d.CheckAgentSources = agentfreshness.Warn + } if d.NewLimiter == nil { d.NewLimiter = func() (outbox.Limiter, error) { return outbox.NewTokenBucket(livePostLimiterInterval, livePostLimiterBurst) @@ -156,6 +164,9 @@ func Open(ctx context.Context, req OpenRequest) (Runtime, error) { command := commandName(req.Command) profile := normalizeRuntimeProfile(req.Profile) req.Profile = profile + if req.CheckAgentSourceFreshness { + deps.CheckAgentSources(ctx, profile.AgentSources, req.Warnings) + } dispatcher := newHookDispatcher(req, nil) stores := newRuntimeCredentialStores(req.Config, req.Backend, req.BackendFlagChanged, command, req.Progress) cleanup := stores.Close diff --git a/internal/app/runtime_test.go b/internal/app/runtime_test.go index 3e3c8d77..76d70df3 100644 --- a/internal/app/runtime_test.go +++ b/internal/app/runtime_test.go @@ -7,6 +7,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "os" "os/exec" "path/filepath" @@ -73,6 +74,42 @@ func TestOpenCanInstantiateWithoutCobra(t *testing.T) { } } +func TestOpenChecksAgentSourceFreshnessOnlyWhenRequested(t *testing.T) { + for _, requested := range []bool{true, false} { + cfg := testConfig() + profile := cfg.Profiles["home"] + profile.AgentSources = []string{"/catalog/agents"} + var warnings bytes.Buffer + var gotDirs []string + var gotWriter io.Writer + deps := testDependencies(t, nil, nil, nil) + deps.CheckAgentSources = func(_ context.Context, dirs []string, w io.Writer) { + gotDirs = dirs + gotWriter = w + } + runtime, err := Open(context.Background(), OpenRequest{ + Config: cfg, + Profile: profile, + PRRef: testPRRef(), + Warnings: &warnings, + CheckAgentSourceFreshness: requested, + Dependencies: deps, + }) + if err != nil { + t.Fatalf("Open: %v", err) + } + if runtime.Cleanup != nil { + runtime.Cleanup() + } + if requested && (!slices.Equal(gotDirs, profile.AgentSources) || gotWriter != &warnings) { + t.Fatalf("checked dirs/writer = %v/%v, want profile sources and request warnings", gotDirs, gotWriter) + } + if !requested && gotDirs != nil { + t.Fatalf("checked dirs = %v, want no check when not requested", gotDirs) + } + } +} + func TestOpenRunsPipelineWithOneCatalogSnapshot(t *testing.T) { catalog, catalogDir := runtimeCatalogFixture(t) repoDir, baseSHA, headSHA := runtimeGitFixture(t) diff --git a/internal/cmd/reviewcmd/reviewcmd.go b/internal/cmd/reviewcmd/reviewcmd.go index a0b2ed24..8a68895d 100644 --- a/internal/cmd/reviewcmd/reviewcmd.go +++ b/internal/cmd/reviewcmd/reviewcmd.go @@ -339,6 +339,7 @@ func runReview(ctx context.Context, cmd *cobra.Command, opts *root.Options, fact PRRef: ref, PRURL: prArg, RequireOpinionatedReviewAuthority: !flags.dryRun, + CheckAgentSourceFreshness: true, Retention: appruntime.RetentionPolicyFromConfig(cfg.Data.Retention), RetentionManualOnly: cfg.Data.Retention.Enforcement == config.RetentionManualOnly, KeepWorkbench: keepWorkbench, diff --git a/internal/cmd/reviewcmd/reviewcmd_test.go b/internal/cmd/reviewcmd/reviewcmd_test.go index d730ed52..878a1cc0 100644 --- a/internal/cmd/reviewcmd/reviewcmd_test.go +++ b/internal/cmd/reviewcmd/reviewcmd_test.go @@ -255,6 +255,9 @@ func TestReviewCommandAcceptanceHarnessComposesDryRun(t *testing.T) { if gotRuntime.RequireOpinionatedReviewAuthority { t.Fatal("dry-run runtime should not require opinionated review authority") } + if !gotRuntime.CheckAgentSourceFreshness { + t.Fatal("dry-run runtime should check agent source freshness") + } if len(runner.requests) != 1 { t.Fatalf("runner calls = %d, want 1", len(runner.requests)) } From 8436ab952a26eaa4b5511c4a0ce35bf51575499b Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Fri, 30 Oct 2026 15:54:01 -0400 Subject: [PATCH 2/2] fix(review): note unconfirmed agent sources and quote the update command --- README.md | 3 +- internal/agentfreshness/agentfreshness.go | 31 ++++++++++++++++--- .../agentfreshness/agentfreshness_test.go | 20 ++++++++++-- 3 files changed, 45 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index e45bdb0c..209e154e 100644 --- a/README.md +++ b/README.md @@ -1131,7 +1131,8 @@ current. For a source in a Git checkout whose branch tracks an upstream, cr fetches that upstream (up to 15 seconds). If the checkout is behind, cr prints a warning on stderr with the commit count and the `git merge --ff-only` command that updates it, so reviewers merged upstream don't go missing without notice. -If the fetch fails, cr compares against the last fetched upstream state. A +If the fetch fails, cr compares against the last fetched upstream state and +notes that it could not confirm the source is current. A source outside Git, or on a branch with no upstream, gets a one-line note that cr cannot check it. The check never fails a review. diff --git a/internal/agentfreshness/agentfreshness.go b/internal/agentfreshness/agentfreshness.go index bd06bef1..eaf29967 100644 --- a/internal/agentfreshness/agentfreshness.go +++ b/internal/agentfreshness/agentfreshness.go @@ -38,6 +38,9 @@ const ( KindNoUpstream Kind = "no_upstream" // KindBehind reports a checkout missing commits from its upstream. KindBehind Kind = "behind" + // KindFetchFailed reports a checkout that matched its last fetched upstream + // but whose upstream could not be fetched to confirm it is current. + KindFetchFailed Kind = "fetch_failed" ) // Notice describes one agent source that may not reflect its shared catalog. @@ -107,14 +110,17 @@ func checkRepo(ctx context.Context, run GitCommand, source, root string) (Notice return Notice{}, false } behind, err := strconv.Atoi(strings.TrimSpace(string(out))) - if err != nil || behind == 0 { + if err != nil { return Notice{}, false } notice := Notice{Kind: KindBehind, Source: source, RepoRoot: root, Upstream: upstream, Behind: behind} if fetchErr != nil { notice.FetchError = fetchErr.Error() + if behind == 0 { + notice.Kind = KindFetchFailed + } } - return notice, true + return notice, notice.Kind == KindFetchFailed || behind > 0 } // Write renders notices as one line each. @@ -132,17 +138,32 @@ func (n Notice) String() string { return fmt.Sprintf("note: agent source %s is in %s, whose branch tracks no upstream; cr cannot check whether its reviewers are current", n.Source, n.RepoRoot) case KindBehind: line := fmt.Sprintf("warning: agent source %s is %d %s behind %s; reviewer changes merged there will not run. Update with: git -C %s merge --ff-only %s", - n.Source, n.Behind, plural(n.Behind, "commit", "commits"), n.Upstream, n.RepoRoot, n.Upstream) + n.Source, n.Behind, plural(n.Behind, "commit", "commits"), n.Upstream, shellQuote(n.RepoRoot), shellQuote(n.Upstream)) if n.FetchError != "" { - reason, _, _ := strings.Cut(n.FetchError, "\n") - line += fmt.Sprintf(" (fetch failed, counted from the last fetch: %s)", reason) + line += fmt.Sprintf(" (fetch failed, counted from the last fetch: %s)", firstLine(n.FetchError)) } return line + case KindFetchFailed: + return fmt.Sprintf("note: agent source %s matches the last fetch of %s, but cr could not fetch it to confirm it is current: %s", n.Source, n.Upstream, firstLine(n.FetchError)) default: return fmt.Sprintf("note: agent source %s: %s", n.Source, n.Kind) } } +func firstLine(s string) string { + line, _, _ := strings.Cut(s, "\n") + return line +} + +// shellQuote returns s as one POSIX shell word, so the suggested update +// command can be pasted as-is. +func shellQuote(s string) string { + if s != "" && strings.Trim(s, "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789/._-+:@%=,") == "" { + return s + } + return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" +} + func plural(n int, one, many string) string { if n == 1 { return one diff --git a/internal/agentfreshness/agentfreshness_test.go b/internal/agentfreshness/agentfreshness_test.go index e5de49cf..34781b81 100644 --- a/internal/agentfreshness/agentfreshness_test.go +++ b/internal/agentfreshness/agentfreshness_test.go @@ -55,12 +55,26 @@ func TestCheckFallsBackToLastFetchWhenUpstreamIsUnreachable(t *testing.T) { } } -func TestCheckIsQuietWhenUpstreamIsUnreachableAndLastFetchIsCurrent(t *testing.T) { +func TestCheckNotesUnreachableUpstreamWhenLastFetchIsCurrent(t *testing.T) { _, clone := catalogClone(t) git(t, clone, "remote", "set-url", "origin", filepath.Join(t.TempDir(), "gone.git")) - if notices := Check(context.Background(), []string{agentsDir(t, clone)}, testGit); len(notices) != 0 { - t.Fatalf("notices = %+v, want none", notices) + notices := Check(context.Background(), []string{agentsDir(t, clone)}, testGit) + + if len(notices) != 1 || notices[0].Kind != KindFetchFailed || notices[0].FetchError == "" { + t.Fatalf("notices = %+v, want one fetch-failed notice", notices) + } + if line := notices[0].String(); !strings.HasPrefix(line, "note: ") || !strings.Contains(line, "could not fetch") { + t.Fatalf("line = %q, want a fetch-failed note", line) + } +} + +func TestBehindNoticeQuotesUpdateCommandArguments(t *testing.T) { + line := Notice{Kind: KindBehind, Source: "~/catalog", RepoRoot: "/Users/me/My Catalog's", Upstream: "origin/main", Behind: 1}.String() + + want := `git -C '/Users/me/My Catalog'\''s' merge --ff-only origin/main` + if !strings.HasSuffix(line, want) { + t.Fatalf("line = %q, want suffix %q", line, want) } }