diff --git a/cmd/gc/cmd_import.go b/cmd/gc/cmd_import.go index b8e1601181..158906ca41 100644 --- a/cmd/gc/cmd_import.go +++ b/cmd/gc/cmd_import.go @@ -29,6 +29,7 @@ var ( syncImportsSelective = packman.SyncLockSelectiveUpgrade installLockedImports = packman.InstallLocked checkInstalledImports = packman.CheckInstalled + checkUpstreamImports = packman.CheckUpstream readImportLockfile = packman.ReadLockfile writeImportLockfile = packman.WriteLockfile resolveImportVersion = packman.ResolveVersion @@ -103,7 +104,18 @@ func newImportCmd(stdout, stderr io.Writer) *cobra.Command { cmd := &cobra.Command{ Use: "import", Short: "Manage pack imports", - Args: cobra.NoArgs, + Long: `Manage pack imports. + +Freshness: "gc import check" and "gc doctor" validate import state offline -- +they answer "is what I declared installed and consistent?", never "has upstream +moved?". A pin can be months stale and pass both. To compare each declared +remote import against its source, run: + + gc import status --check-upstream + +Re-pinning: "gc import upgrade" moves a pin only as far as its declared +constraint allows, so it cannot move a "sha:" pin at all. ` + importRePinHint + `.`, + Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { return cmd.Help() }, @@ -250,7 +262,14 @@ func newImportUpgradeCmd(stdout, stderr io.Writer) *cobra.Command { return &cobra.Command{ Use: "upgrade [name]", Short: "Upgrade imported packs within their constraints", - Args: cobra.MaximumNArgs(1), + Long: `Upgrade imported packs within their constraints. + +Only within them: the declared constraint is not rewritten, so a "sha:" +pin names a fixed commit and this command cannot move it. In that case the +output says so rather than reporting an upgrade that did not happen. + +` + importRePinHint + `.`, + Args: cobra.MaximumNArgs(1), RunE: func(_ *cobra.Command, args []string) error { cityPath, err := resolveImportRoot() if err != nil { @@ -674,7 +693,11 @@ func importAddErrorLine(source, nameOverride string, err error) string { return "gc import add: could not derive import name; use --name" case errors.Is(err, importsvc.ErrReservedPrefix): return fmt.Sprintf("gc import add: import name %q uses reserved prefix \"default-rig:\"", nameOverride) - case errors.Is(err, importsvc.ErrScopeLoad), errors.Is(err, importsvc.ErrImportExists): + case errors.Is(err, importsvc.ErrImportExists): + // Dead-ending here was the whole complaint: the operator's next move is + // a re-pin, and nothing in the old line said where to make it. + return fmt.Sprintf("gc import add: %v; %s", err, importRePinHint) + case errors.Is(err, importsvc.ErrScopeLoad): return fmt.Sprintf("gc import add: %v", err) default: // Redact any userinfo in the source so a credential-bearing URL never @@ -807,7 +830,18 @@ func doImportUpgrade(cityPath, target string, stdout, stderr io.Writer) int { return 1 } + // Read the pins before syncing. "Upgraded" is a claim about movement, and + // movement is only observable against the prior state: the unconditional + // success lines this replaces reported an upgrade even for a sha: pin, + // which names a fixed commit and cannot move at all. + priorLock, priorErr := readImportLockfile(fsys.OSFS{}, cityPath) + if priorErr != nil { + fmt.Fprintf(stderr, "gc import upgrade: %v\n", priorErr) //nolint:errcheck + return 1 + } + var lock *packman.Lockfile + targetSource, targetConstraint := "", "" if target == "" { lock, err = syncImports(cityPath, allImports, packman.InstallUpgrade) } else { @@ -825,6 +859,7 @@ func doImportUpgrade(cityPath, target string, stdout, stderr io.Writer) int { fmt.Fprintf(stderr, "gc import upgrade: import %q is a path import and cannot be upgraded\n", target) //nolint:errcheck return 1 } + targetSource, targetConstraint = targetImp.Source, targetImp.Version lock, err = syncImportsSelective(cityPath, allImports, map[string]struct{}{ targetImp.Source: {}, }) @@ -844,13 +879,93 @@ func doImportUpgrade(cityPath, target string, stdout, stderr io.Writer) int { return 1 } if target == "" { - fmt.Fprintf(stdout, "Upgraded %d remote import(s)\n", len(lock.Packs)) //nolint:errcheck + writeImportUpgradeAllSummary(stdout, priorLock, lock) } else { - fmt.Fprintf(stdout, "Upgraded import %q\n", target) //nolint:errcheck + writeImportUpgradeTargetSummary(stdout, target, targetSource, targetConstraint, priorLock, lock) } return 0 } +// importRePinHint names the supported way to move an import onto a different +// commit or constraint. +// +// "gc import upgrade" deliberately takes no --version flag: a declaration lives +// in whichever of the three scopes owns it (root pack [imports.*], +// [defaults.rig.imports.*], or a rig's [rigs.imports.*]), and directing the +// operator at the declaring file is unambiguous where a rewrite that silently +// picked a scope would not be. +const importRePinHint = `to re-pin, edit that import's version in the file that declares it ` + + `(pack.toml [imports.*] or [defaults.rig.imports.*], city.toml [imports.*], ` + + `or the rig's [imports.*]) and run "gc import install"` + +// writeImportUpgradeAllSummary reports what the all-imports sync moved. A +// source with no prior pin counts as moved: the lock did change for it. +func writeImportUpgradeAllSummary(stdout io.Writer, prior, current *packman.Lockfile) { + moved, unchanged := 0, 0 + for source, pack := range current.Packs { + if before, ok := lockedImportCommit(prior, source); ok && before == pack.Commit { + unchanged++ + continue + } + moved++ + } + fmt.Fprintf(stdout, "Upgraded %d of %d remote import(s); %d unchanged.\n", //nolint:errcheck + moved, len(current.Packs), unchanged) +} + +// writeImportUpgradeTargetSummary reports what the single-target sync moved. +// Only the moved and first-locked cases may use the word "Upgraded"; saying it +// over an unmoved pin is the false success this replaces. +func writeImportUpgradeTargetSummary(stdout io.Writer, target, source, constraint string, prior, current *packman.Lockfile) { + after, _ := lockedImportCommit(current, source) + before, hadBefore := lockedImportCommit(prior, source) + switch { + case !hadBefore: + // First resolution for this source. There is no prior commit to move + // from, so this is a lock rather than an upgrade; rendering it as + // `"" -> abc1234` would invent a move that never happened. + fmt.Fprintf(stdout, "Upgraded import %q: locked at %s\n", target, shortImportCommit(after)) //nolint:errcheck + case before != after: + fmt.Fprintf(stdout, "Upgraded import %q: %s -> %s\n", //nolint:errcheck + target, shortImportCommit(before), shortImportCommit(after)) + case strings.HasPrefix(constraint, "sha:"): + fmt.Fprintf(stdout, "Import %q is unchanged: constraint %q pins a fixed commit, so there is nothing to upgrade.\n", //nolint:errcheck + target, constraint) + // The sync did rewrite pin.fetched. Saying so is the difference + // between "nothing happened" and "something happened that was not an + // upgrade" -- an operator who sees packs.lock change otherwise has + // been told the opposite of what the file shows. + fmt.Fprintf(stdout, "Its packs.lock fetched timestamp was refreshed; commit %s did not change.\n", //nolint:errcheck + shortImportCommit(after)) + fmt.Fprintf(stdout, "%s\n", importRePinHint) //nolint:errcheck + default: + version, _ := lockedImportVersion(current, source) + if constraint == "" { + fmt.Fprintf(stdout, "Import %q is already at the highest available version (%s).\n", //nolint:errcheck + target, version) + } else { + fmt.Fprintf(stdout, "Import %q is already at the highest version matching %q (%s).\n", //nolint:errcheck + target, constraint, version) + } + } +} + +func lockedImportCommit(lock *packman.Lockfile, source string) (string, bool) { + if lock == nil { + return "", false + } + pack, ok := lock.Packs[source] + return pack.Commit, ok +} + +func lockedImportVersion(lock *packman.Lockfile, source string) (string, bool) { + if lock == nil { + return "", false + } + pack, ok := lock.Packs[source] + return pack.Version, ok +} + func doImportList(cityPath string, tree bool, stdout, stderr io.Writer) int { scope, err := loadImportScopeFS(fsys.OSFS{}, cityPath) if err != nil { diff --git a/cmd/gc/cmd_import_status.go b/cmd/gc/cmd_import_status.go index d7513550e0..ed51768b44 100644 --- a/cmd/gc/cmd_import_status.go +++ b/cmd/gc/cmd_import_status.go @@ -9,10 +9,13 @@ import ( "os" "path/filepath" "sort" + "strings" "time" + "github.com/gastownhall/gascity/internal/config" "github.com/gastownhall/gascity/internal/fsys" "github.com/gastownhall/gascity/internal/packman" + "github.com/gastownhall/gascity/internal/remotesource" "github.com/spf13/cobra" ) @@ -42,6 +45,47 @@ type ImportStatusJSON struct { // LockedPacks mirrors the full packs.lock closure (direct and // transitive pins), sorted by source. LockedPacks []ImportStatusLockedPack `json:"locked_packs"` + // Upstream summarizes the freshness walk. Present only under + // --check-upstream, so the default document is unchanged. + Upstream *ImportStatusUpstreamSummary `json:"upstream,omitempty"` + + // upstreamErrors holds the resolution failures as typed errors, which the + // document's own string fields cannot carry. It is unexported, so it never + // reaches the wire; it exists so a *gitcred.AuthError still reaches + // printCredentialHint and the operator is told to register a credential + // rather than reading a raw git rejection. + upstreamErrors []error +} + +// ImportStatusUpstreamSummary is the aggregate freshness verdict. +// +// It follows the "gc lint --json" precedent: "ok" keeps meaning "the command +// ran" and a separate "passed" carries the verdict, so a consumer never has to +// infer the exit code from the entry list. +type ImportStatusUpstreamSummary struct { + // Passed mirrors the process exit code: false when any import is behind, + // or when any is unreachable and --fail-on-unreachable was passed. + Passed bool `json:"passed"` + Checked int `json:"checked"` + Current int `json:"current"` + Behind int `json:"behind"` + Unreachable int `json:"unreachable"` + NotApplicable int `json:"not_applicable"` +} + +// ImportStatusUpstream is one import's freshness answer. +type ImportStatusUpstream struct { + // Verdict is one of "current", "behind", "unreachable", or + // "not_applicable". The schema pins it as an enum. + Verdict string `json:"verdict"` + // ResolvedRef names what upstream was resolved through: a symbolic ref + // such as "refs/heads/main", or the selected tag for a semver constraint. + ResolvedRef string `json:"resolved_ref,omitempty"` + // ResolvedCommit is the commit upstream resolved to. + ResolvedCommit string `json:"resolved_commit,omitempty"` + // Error explains an unreachable verdict, or why an import has no upstream + // to compare against. + Error string `json:"error,omitempty"` } // ImportStatusEntry is one declared import binding in the status output. @@ -62,6 +106,9 @@ type ImportStatusEntry struct { // Pin is the packs.lock resolution for kind "remote" entries. // Omitted when the source has no lock entry (unlocked). Pin *ImportStatusPin `json:"pin,omitempty"` + // Upstream is this import's freshness answer. Present only under + // --check-upstream. + Upstream *ImportStatusUpstream `json:"upstream,omitempty"` } // ImportStatusPin is the packs.lock resolution pinned for a remote import. @@ -80,7 +127,7 @@ type ImportStatusLockedPack struct { } func newImportStatusCmd(stdout, stderr io.Writer) *cobra.Command { - var jsonOut bool + var opts importStatusOptions cmd := &cobra.Command{ Use: "status", Short: "Report declared imports and packs.lock pins", @@ -89,48 +136,86 @@ func newImportStatusCmd(stdout, stderr io.Writer) *cobra.Command { Covers every import scope (root pack [imports.*], [defaults.rig.imports.*], and rig-scoped [rigs.imports.*]) plus the full packs.lock closure and the lockfile content hash. With --json the output is a stable machine-readable -document for drift checkers.`, +document for drift checkers. + +Without --check-upstream the command is entirely offline and reports only what +is already on disk: a pin can be years stale and still look healthy. With +--check-upstream each declared remote import's source is resolved over the +network and compared against its packs.lock pin, and the command exits 1 if any +pin is behind.`, Args: cobra.NoArgs, RunE: func(_ *cobra.Command, _ []string) error { + // A flag that is silently ignored reads as a passing gate, which is + // the failure mode this whole command exists to remove. + if opts.FailOnUnreachable && !opts.CheckUpstream { + fmt.Fprintln(stderr, "gc import status: --fail-on-unreachable requires --check-upstream") //nolint:errcheck + return errExit + } cityPath, err := resolveImportRoot() if err != nil { fmt.Fprintf(stderr, "gc import status: %v\n", err) //nolint:errcheck return errExit } - if doImportStatus(cityPath, jsonOut, stdout, stderr) != 0 { + if doImportStatusWithOptions(cityPath, opts, stdout, stderr) != 0 { return errExit } return nil }, } - cmd.Flags().BoolVar(&jsonOut, "json", false, "emit JSON result") + cmd.Flags().BoolVar(&opts.JSON, "json", false, "emit JSON result") + cmd.Flags().BoolVar(&opts.CheckUpstream, "check-upstream", false, + "resolve each remote import's source and compare it against its packs.lock pin (network)") + cmd.Flags().BoolVar(&opts.FailOnUnreachable, "fail-on-unreachable", false, + "with --check-upstream, also exit 1 when an import's upstream cannot be resolved") return cmd } +// importStatusOptions carries the "gc import status" flag set. The zero value +// is today's offline behavior. +type importStatusOptions struct { + JSON bool + CheckUpstream bool + FailOnUnreachable bool +} + // doImportStatus is the pure logic for "gc import status". It reads the // declared import set across all scopes plus packs.lock and renders // either the human-readable summary or the typed JSON document. func doImportStatus(cityPath string, jsonOut bool, stdout, stderr io.Writer) int { - status, err := buildImportStatus(cityPath) + return doImportStatusWithOptions(cityPath, importStatusOptions{JSON: jsonOut}, stdout, stderr) +} + +// doImportStatusWithOptions is doImportStatus with the freshness flags. With +// opts.CheckUpstream false it is the offline command exactly as it has always +// behaved: no network call, no new field emitted, exit 0. +func doImportStatusWithOptions(cityPath string, opts importStatusOptions, stdout, stderr io.Writer) int { + status, err := buildImportStatus(cityPath, opts) if err != nil { fmt.Fprintf(stderr, "gc import status: %v\n", err) //nolint:errcheck return 1 } - if jsonOut { + if opts.JSON { data, err := json.MarshalIndent(status, "", " ") if err != nil { fmt.Fprintf(stderr, "gc import status: encoding JSON: %v\n", err) //nolint:errcheck return 1 } fmt.Fprintln(stdout, string(data)) //nolint:errcheck + } else { + writeImportStatusText(stdout, status) + } + if status.Upstream == nil { + return 0 + } + writeImportStatusUpstreamStderr(stderr, status) + if status.Upstream.Passed { return 0 } - writeImportStatusText(stdout, status) - return 0 + return 1 } // buildImportStatus assembles the import status document for cityPath. -func buildImportStatus(cityPath string) (*ImportStatusJSON, error) { +func buildImportStatus(cityPath string, opts importStatusOptions) (*ImportStatusJSON, error) { fs := fsys.OSFS{} allImports, err := collectAllImportsFS(cityPath) if err != nil { @@ -207,9 +292,60 @@ func buildImportStatus(cityPath string) (*ImportStatusJSON, error) { Fetched: formatImportStatusTime(pack.Fetched), }) } + if opts.CheckUpstream { + if err := addImportStatusUpstream(status, cityPath, allImports, lock, opts); err != nil { + return nil, err + } + } return status, nil } +// addImportStatusUpstream runs the freshness walk and folds it into the +// document: one verdict per entry plus the aggregate summary. +func addImportStatusUpstream(status *ImportStatusJSON, cityPath string, allImports map[string]config.Import, lock *packman.Lockfile, opts importStatusOptions) error { + report, err := checkUpstreamImports(cityPath, allImports, lock) + if err != nil { + return err + } + byName := make(map[string]packman.UpstreamStatus, len(report.Statuses)) + for _, upstream := range report.Statuses { + byName[upstream.Name] = upstream + } + for i := range status.Imports { + upstream, ok := byName[status.Imports[i].Name] + if !ok { + continue + } + entry := &ImportStatusUpstream{ + Verdict: string(upstream.Verdict), + ResolvedRef: upstream.ResolvedRef, + ResolvedCommit: upstream.ResolvedCommit, + } + if upstream.Err != nil { + entry.Error = upstream.Err.Error() + if upstream.Verdict == packman.UpstreamUnreachable { + status.upstreamErrors = append(status.upstreamErrors, upstream.Err) + } + } + status.Imports[i].Upstream = entry + } + + behind := report.Count(packman.UpstreamBehind) + unreachable := report.Count(packman.UpstreamUnreachable) + status.Upstream = &ImportStatusUpstreamSummary{ + // An unreachable import is only a failure when the caller asks for it: + // a laptop offline in a tunnel should not fail the same gate a stale + // pin does, but a CI job that wants "prove it" can say so. + Passed: behind == 0 && (unreachable == 0 || !opts.FailOnUnreachable), + Checked: report.Checked, + Current: report.Count(packman.UpstreamCurrent), + Behind: behind, + Unreachable: unreachable, + NotApplicable: report.Count(packman.UpstreamNotApplicable), + } + return nil +} + // formatImportStatusTime renders a lock timestamp as RFC 3339 UTC, or // "" for the zero value so omitempty drops it from the JSON output. func formatImportStatusTime(t time.Time) string { @@ -240,4 +376,110 @@ func writeImportStatusText(stdout io.Writer, status *ImportStatusJSON) { fmt.Fprintf(stdout, "%s\t%s\t%s\t%s\t%s\t%s\n", //nolint:errcheck entry.Name, entry.Source, entry.Constraint, entry.Kind, pinnedVersion, pinnedCommit) } + writeImportStatusUpstreamText(stdout, status) +} + +// writeImportStatusUpstreamText renders the freshness block appended under +// --check-upstream. It is a no-op otherwise, which is what keeps the default +// text output byte-identical. +func writeImportStatusUpstreamText(stdout io.Writer, status *ImportStatusJSON) { + if status.Upstream == nil { + return + } + fmt.Fprintln(stdout, "\nupstream freshness:") //nolint:errcheck + width := 0 + for _, entry := range status.Imports { + if entry.Upstream != nil && len(entry.Name) > width { + width = len(entry.Name) + } + } + subpathBehind := false + for _, entry := range status.Imports { + if entry.Upstream == nil { + continue + } + // Ask the source parser whether this import names a subpath rather + // than matching "//" by hand: every https://, ssh:// and file:// + // source contains "//" in its scheme, so a hand-rolled test prints + // the caveat for every behind import. + if entry.Upstream.Verdict == string(packman.UpstreamBehind) && + remotesource.Parse(entry.Source).Subpath != "" { + subpathBehind = true + } + fmt.Fprintf(stdout, " %-*s %-14s %s\n", //nolint:errcheck + width, entry.Name, entry.Upstream.Verdict, importStatusUpstreamDetail(entry)) + } + sum := status.Upstream + fmt.Fprintf(stdout, "%d checked: %d current, %d behind, %d unreachable, %d not applicable\n", //nolint:errcheck + sum.Checked, sum.Current, sum.Behind, sum.Unreachable, sum.NotApplicable) + if subpathBehind { + // Without this, a subpath import reported behind reads as "the files + // under this subpath changed", which the walk never establishes. + fmt.Fprintln(stdout, `note: freshness is measured per repository; a "behind" verdict does not`) //nolint:errcheck + fmt.Fprintln(stdout, " necessarily mean this pack's subpath changed") //nolint:errcheck + } +} + +// importStatusUpstreamDetail renders the trailing evidence for one freshness +// line: what is pinned, and what the source resolved to. +func importStatusUpstreamDetail(entry ImportStatusEntry) string { + upstream := entry.Upstream + if upstream.Verdict == string(packman.UpstreamNotApplicable) { + if upstream.Error != "" { + return upstream.Error + } + return "no upstream to compare (path source)" + } + var b strings.Builder + if entry.Pin != nil { + fmt.Fprintf(&b, "pinned %s", shortImportCommit(entry.Pin.Commit)) + if entry.Pin.Fetched != "" { + fmt.Fprintf(&b, " (fetched %s)", entry.Pin.Fetched) + } + } + if upstream.Verdict == string(packman.UpstreamUnreachable) { + fmt.Fprintf(&b, " source unresolved: %s", upstream.Error) + return b.String() + } + fmt.Fprintf(&b, " source") + if upstream.ResolvedRef != "" { + fmt.Fprintf(&b, " %s", upstream.ResolvedRef) + } + fmt.Fprintf(&b, " %s", shortImportCommit(upstream.ResolvedCommit)) + return b.String() +} + +// writeImportStatusUpstreamStderr names every stale or unresolved import on +// stderr with both commits, so a CI log that only captures stderr still says +// which pin is stale and what it should move to. +func writeImportStatusUpstreamStderr(stderr io.Writer, status *ImportStatusJSON) { + for _, entry := range status.Imports { + if entry.Upstream == nil { + continue + } + pinned := "" + if entry.Pin != nil { + pinned = entry.Pin.Commit + } + switch entry.Upstream.Verdict { + case string(packman.UpstreamBehind): + fmt.Fprintf(stderr, "gc import status: import %q is behind upstream: pinned %s, source %s\n", //nolint:errcheck + entry.Name, pinned, strings.TrimSpace(entry.Upstream.ResolvedRef+" "+entry.Upstream.ResolvedCommit)) + case string(packman.UpstreamUnreachable): + fmt.Fprintf(stderr, "gc import status: import %q upstream is unreachable (pinned %s): %s\n", //nolint:errcheck + entry.Name, pinned, entry.Upstream.Error) + } + } + for _, err := range status.upstreamErrors { + printCredentialHint(stderr, err) + } +} + +// shortImportCommit abbreviates a commit for the human-readable block. The +// stderr lines and the JSON document always carry the full value. +func shortImportCommit(commit string) string { + if len(commit) > 8 { + return commit[:8] + } + return commit } diff --git a/cmd/gc/cmd_import_status_test.go b/cmd/gc/cmd_import_status_test.go index f05092b327..8b89b0ca44 100644 --- a/cmd/gc/cmd_import_status_test.go +++ b/cmd/gc/cmd_import_status_test.go @@ -12,6 +12,7 @@ import ( "testing" "time" + "github.com/gastownhall/gascity/internal/config" "github.com/gastownhall/gascity/internal/fsys" "github.com/gastownhall/gascity/internal/packman" ) @@ -336,3 +337,442 @@ func TestImportStatusCommandRegistered(t *testing.T) { } t.Fatal("gc import status subcommand not registered") } + +// upstreamStatusFixtureCity writes the three-scope import fixture the +// freshness tests share: a pinned remote, an unlocked remote, and a path +// import. +func upstreamStatusFixtureCity(t *testing.T) string { + t.Helper() + dir := t.TempDir() + writeCityToml(t, dir, "[workspace]\nname = \"demo\"\n") + writePackToml(t, dir, `[pack] +name = "demo" +schema = 1 + +[imports.tools] +source = "https://example.com/tools.git" +version = "^1.4" + +[imports.local] +source = "./packs/local" + +[imports.unlocked] +source = "https://example.com/unlocked.git" +version = "^9.9" +`) + importStatusLockFixture(t, dir) + return dir +} + +// stubCheckUpstreamImports swaps the freshness seam for a fixed report and +// returns a pointer to the call count, so a test can assert the seam was not +// reached at all. +func stubCheckUpstreamImports(t *testing.T, statuses ...packman.UpstreamStatus) *int { + t.Helper() + calls := 0 + prev := checkUpstreamImports + t.Cleanup(func() { checkUpstreamImports = prev }) + checkUpstreamImports = func(_ string, _ map[string]config.Import, _ *packman.Lockfile) (*packman.UpstreamReport, error) { + calls++ + return &packman.UpstreamReport{Checked: len(statuses), Statuses: statuses}, nil + } + return &calls +} + +func upstreamStatusEntry(t *testing.T, doc *ImportStatusJSON, name string) ImportStatusEntry { + t.Helper() + for _, entry := range doc.Imports { + if entry.Name == name { + return entry + } + } + t.Fatalf("no import entry %q in %#v", name, doc.Imports) + return ImportStatusEntry{} +} + +func decodeImportStatusDoc(t *testing.T, data []byte) *ImportStatusJSON { + t.Helper() + var doc ImportStatusJSON + if err := json.Unmarshal(data, &doc); err != nil { + t.Fatalf("decoding status document: %v\n%s", err, data) + } + return &doc +} + +// TestImportStatusWithoutCheckUpstreamStaysOffline is REQ-003 at the command +// layer: the default invocation must not resolve anything over the network, +// and must not emit either new field. The seam is stubbed to fail the test if +// it is reached. +func TestImportStatusWithoutCheckUpstreamStaysOffline(t *testing.T) { + clearGCEnv(t) + dir := upstreamStatusFixtureCity(t) + + prev := checkUpstreamImports + t.Cleanup(func() { checkUpstreamImports = prev }) + checkUpstreamImports = func(_ string, _ map[string]config.Import, _ *packman.Lockfile) (*packman.UpstreamReport, error) { + t.Fatal("gc import status resolved upstream freshness without --check-upstream") + return nil, nil + } + + var stdout, stderr bytes.Buffer + if code := doImportStatus(dir, true, &stdout, &stderr); code != 0 { + t.Fatalf("code = %d, stderr = %s", code, stderr.String()) + } + if strings.Contains(stdout.String(), "upstream") { + t.Fatalf("default document mentions upstream:\n%s", stdout.String()) + } + doc := decodeImportStatusDoc(t, stdout.Bytes()) + if doc.Upstream != nil { + t.Fatalf("Upstream = %#v, want nil", doc.Upstream) + } + for _, entry := range doc.Imports { + if entry.Upstream != nil { + t.Fatalf("entry %q carries upstream %#v, want nil", entry.Name, entry.Upstream) + } + } +} + +// TestImportStatusCheckUpstreamJSONGolden is AC-06: the document under +// --check-upstream carries, for each import, the fields a drift checker needs +// to act -- name, source, constraint, the pin it holds, and what upstream +// resolved to -- and validates against the schema whose verdict is an enum. +func TestImportStatusCheckUpstreamJSONGolden(t *testing.T) { + clearGCEnv(t) + dir := upstreamStatusFixtureCity(t) + stubCheckUpstreamImports(t, + packman.UpstreamStatus{ + Name: "pack:local", Source: "./packs/local", Verdict: packman.UpstreamNotApplicable, + }, + packman.UpstreamStatus{ + Name: "pack:tools", Source: "https://example.com/tools.git", Constraint: "^1.4", + LockedCommit: "aaaa", ResolvedRef: "1.9.0", ResolvedCommit: "dddd", + Verdict: packman.UpstreamBehind, + }, + packman.UpstreamStatus{ + Name: "pack:unlocked", Source: "https://example.com/unlocked.git", Constraint: "^9.9", + Verdict: packman.UpstreamNotApplicable, + Err: fmt.Errorf("no packs.lock entry for %q; run \"gc import install\"", "https://example.com/unlocked.git"), + }, + ) + + var stdout, stderr bytes.Buffer + code := run([]string{"--city", dir, "import", "status", "--json", "--check-upstream"}, &stdout, &stderr) + if code != 1 { + t.Fatalf("code = %d, want 1 for a behind import; stderr=%s", code, stderr.String()) + } + + doc := decodeImportStatusDoc(t, stdout.Bytes()) + tools := upstreamStatusEntry(t, doc, "pack:tools") + if tools.Source != "https://example.com/tools.git" || tools.Constraint != "^1.4" { + t.Fatalf("tools entry = %#v", tools) + } + if tools.Pin == nil || tools.Pin.Commit != "aaaa" || tools.Pin.Fetched != "2026-01-02T03:04:05Z" { + t.Fatalf("tools pin = %#v, want commit aaaa with a fetched timestamp", tools.Pin) + } + if tools.Upstream == nil { + t.Fatal("tools entry carries no upstream verdict") + } + if tools.Upstream.Verdict != "behind" || tools.Upstream.ResolvedCommit != "dddd" || tools.Upstream.ResolvedRef != "1.9.0" { + t.Fatalf("tools upstream = %#v", tools.Upstream) + } + unlocked := upstreamStatusEntry(t, doc, "pack:unlocked") + if unlocked.Pin != nil { + t.Fatalf("unlocked pin = %#v, want nil", unlocked.Pin) + } + if unlocked.Upstream == nil || unlocked.Upstream.Verdict != "not_applicable" || + !strings.Contains(unlocked.Upstream.Error, "gc import install") { + t.Fatalf("unlocked upstream = %#v", unlocked.Upstream) + } + if local := upstreamStatusEntry(t, doc, "pack:local"); local.Upstream == nil || local.Upstream.Verdict != "not_applicable" { + t.Fatalf("local upstream = %#v", local.Upstream) + } + if doc.Upstream == nil { + t.Fatal("document carries no upstream summary") + } + want := ImportStatusUpstreamSummary{Passed: false, Checked: 3, Current: 0, Behind: 1, Unreachable: 0, NotApplicable: 2} + if *doc.Upstream != want { + t.Fatalf("summary = %#v, want %#v", *doc.Upstream, want) + } + + assertTopLevelOKTrue(t, stdout.Bytes()) + validateJSONAgainstResultSchema(t, []string{"import", "status"}, stdout.Bytes()) +} + +// TestImportStatusCheckUpstreamExitCodeAndStderr is AC-07. A behind import +// exits 1, an all-current run exits 0, "passed" mirrors the exit code either +// way, and every stale import is named on stderr with both commits -- a CI log +// that captures only stderr still says which pin is stale and where it should +// move to. +func TestImportStatusCheckUpstreamExitCodeAndStderr(t *testing.T) { + t.Run("behind exits 1 and names the import on stderr", func(t *testing.T) { + clearGCEnv(t) + dir := upstreamStatusFixtureCity(t) + stubCheckUpstreamImports(t, packman.UpstreamStatus{ + Name: "pack:tools", Source: "https://example.com/tools.git", Constraint: "^1.4", + LockedCommit: "aaaa", ResolvedRef: "refs/heads/main", ResolvedCommit: "dddd", + Verdict: packman.UpstreamBehind, + }) + + var stdout, stderr bytes.Buffer + code := run([]string{"--city", dir, "import", "status", "--json", "--check-upstream"}, &stdout, &stderr) + if code != 1 { + t.Fatalf("code = %d, want 1", code) + } + for _, want := range []string{`"pack:tools"`, "aaaa", "dddd", "behind"} { + if !strings.Contains(stderr.String(), want) { + t.Fatalf("stderr missing %q:\n%s", want, stderr.String()) + } + } + if doc := decodeImportStatusDoc(t, stdout.Bytes()); doc.Upstream.Passed { + t.Fatalf("passed = true, want false to mirror exit 1") + } + }) + + t.Run("all current exits 0", func(t *testing.T) { + clearGCEnv(t) + dir := upstreamStatusFixtureCity(t) + stubCheckUpstreamImports(t, packman.UpstreamStatus{ + Name: "pack:tools", Source: "https://example.com/tools.git", Constraint: "^1.4", + LockedCommit: "aaaa", ResolvedRef: "refs/heads/main", ResolvedCommit: "aaaa", + Verdict: packman.UpstreamCurrent, + }) + + var stdout, stderr bytes.Buffer + code := run([]string{"--city", dir, "import", "status", "--json", "--check-upstream"}, &stdout, &stderr) + if code != 0 { + t.Fatalf("code = %d, want 0; stderr=%s", code, stderr.String()) + } + if strings.Contains(stderr.String(), "behind") { + t.Fatalf("stderr names a stale import with none present:\n%s", stderr.String()) + } + if doc := decodeImportStatusDoc(t, stdout.Bytes()); !doc.Upstream.Passed { + t.Fatalf("passed = false, want true to mirror exit 0") + } + }) +} + +// TestImportStatusCheckUpstreamUnreachable is AC-08. A source that will not +// resolve is reported unreachable with its error surfaced -- never current, +// which would be a false all-clear, and never behind, which would name a +// commit nothing resolved. --fail-on-unreachable changes only the exit code +// and "passed", not a single verdict. +func TestImportStatusCheckUpstreamUnreachable(t *testing.T) { + unreachable := packman.UpstreamStatus{ + Name: "pack:tools", Source: "https://example.com/tools.git", Constraint: "^1.4", + LockedCommit: "aaaa", Verdict: packman.UpstreamUnreachable, + Err: fmt.Errorf("resolving head for %q: dial tcp: no route to host", "https://example.com/tools.git"), + } + + clearGCEnv(t) + dir := upstreamStatusFixtureCity(t) + stubCheckUpstreamImports(t, unreachable) + + var stdout, stderr bytes.Buffer + code := run([]string{"--city", dir, "import", "status", "--json", "--check-upstream"}, &stdout, &stderr) + if code != 0 { + t.Fatalf("code = %d, want 0 without --fail-on-unreachable; stderr=%s", code, stderr.String()) + } + doc := decodeImportStatusDoc(t, stdout.Bytes()) + tools := upstreamStatusEntry(t, doc, "pack:tools") + if tools.Upstream.Verdict != "unreachable" { + t.Fatalf("verdict = %q, want unreachable", tools.Upstream.Verdict) + } + if !strings.Contains(tools.Upstream.Error, "no route to host") { + t.Fatalf("error = %q, want the resolution failure surfaced", tools.Upstream.Error) + } + if tools.Upstream.ResolvedCommit != "" { + t.Fatalf("resolved_commit = %q, want empty", tools.Upstream.ResolvedCommit) + } + if !doc.Upstream.Passed || doc.Upstream.Unreachable != 1 { + t.Fatalf("summary = %#v, want passed with 1 unreachable", *doc.Upstream) + } + if !strings.Contains(stderr.String(), "unreachable") { + t.Fatalf("stderr does not name the unresolved import:\n%s", stderr.String()) + } + validateJSONAgainstResultSchema(t, []string{"import", "status"}, stdout.Bytes()) + + // Same fixture, same verdicts, one more flag. + stubCheckUpstreamImports(t, unreachable) + var strictOut, strictErr bytes.Buffer + strictCode := run([]string{"--city", dir, "import", "status", "--json", "--check-upstream", "--fail-on-unreachable"}, &strictOut, &strictErr) + if strictCode != 1 { + t.Fatalf("code = %d, want 1 with --fail-on-unreachable; stderr=%s", strictCode, strictErr.String()) + } + strictDoc := decodeImportStatusDoc(t, strictOut.Bytes()) + if strictDoc.Upstream.Passed { + t.Fatal("passed = true, want false to mirror exit 1") + } + if got := upstreamStatusEntry(t, strictDoc, "pack:tools").Upstream.Verdict; got != "unreachable" { + t.Fatalf("verdict = %q, want unreachable: the flag changes the exit code, not the verdict", got) + } + strictDoc.Upstream.Passed = true + if *strictDoc.Upstream != *doc.Upstream { + t.Fatalf("summary changed beyond passed: %#v vs %#v", *strictDoc.Upstream, *doc.Upstream) + } +} + +// TestImportStatusFailOnUnreachableRequiresCheckUpstream: a flag that is +// silently ignored reads as a passing gate, which is the failure mode this +// command exists to remove. +func TestImportStatusFailOnUnreachableRequiresCheckUpstream(t *testing.T) { + clearGCEnv(t) + dir := upstreamStatusFixtureCity(t) + + var stdout, stderr bytes.Buffer + code := run([]string{"--city", dir, "import", "status", "--fail-on-unreachable"}, &stdout, &stderr) + if code == 0 { + t.Fatalf("code = 0, want non-zero; stdout=%s", stdout.String()) + } + if !strings.Contains(stderr.String(), "--fail-on-unreachable requires --check-upstream") { + t.Fatalf("stderr = %q", stderr.String()) + } +} + +// TestImportStatusCheckUpstreamTextOutput pins the human-readable block: a +// verdict per import, an aggregate line, and the per-repository note that +// keeps a subpath import's "behind" from being read as "this pack changed". +func TestImportStatusCheckUpstreamTextOutput(t *testing.T) { + clearGCEnv(t) + dir := t.TempDir() + writeCityToml(t, dir, "[workspace]\nname = \"demo\"\n") + writePackToml(t, dir, `[pack] +name = "demo" +schema = 1 + +[imports.bd] +source = "https://example.com/mono.git//examples/bd" +version = "sha:aaaa" +`) + if err := packman.WriteLockfile(fsys.OSFS{}, dir, &packman.Lockfile{ + Schema: packman.LockfileSchema, + Packs: map[string]packman.LockedPack{ + "https://example.com/mono.git//examples/bd": {Version: "sha:aaaa", Commit: "aaaa", Fetched: time.Date(2026, 1, 2, 3, 4, 5, 0, time.UTC)}, + }, + }); err != nil { + t.Fatalf("WriteLockfile: %v", err) + } + stubCheckUpstreamImports(t, packman.UpstreamStatus{ + Name: "pack:bd", Source: "https://example.com/mono.git//examples/bd", Constraint: "sha:aaaa", + LockedCommit: "aaaa", ResolvedRef: "refs/heads/main", ResolvedCommit: "bbbbbbbbbbbb", + Verdict: packman.UpstreamBehind, + }) + + var stdout, stderr bytes.Buffer + code := run([]string{"--city", dir, "import", "status", "--check-upstream"}, &stdout, &stderr) + if code != 1 { + t.Fatalf("code = %d, want 1; stderr=%s", code, stderr.String()) + } + for _, want := range []string{ + "upstream freshness:", + "pack:bd", + "behind", + "refs/heads/main", + "1 checked: 0 current, 1 behind, 0 unreachable, 0 not applicable", + "freshness is measured per repository", + } { + if !strings.Contains(stdout.String(), want) { + t.Fatalf("stdout missing %q:\n%s", want, stdout.String()) + } + } +} + +// TestImportStatusCheckUpstreamTextNoSubpathOmitsNote is the negative half of +// TestImportStatusCheckUpstreamTextOutput: a behind import whose source names +// no subpath must not carry the per-repository caveat. That sibling test's +// fixture has a real subpath, so it passes whether the note is gated on the +// parsed subpath or on any "//" in the source -- including the "//" every +// scheme contributes. +func TestImportStatusCheckUpstreamTextNoSubpathOmitsNote(t *testing.T) { + clearGCEnv(t) + dir := t.TempDir() + writeCityToml(t, dir, "[workspace]\nname = \"demo\"\n") + writePackToml(t, dir, `[pack] +name = "demo" +schema = 1 + +[imports.tools] +source = "https://example.com/tools.git" +version = "sha:aaaa" +`) + if err := packman.WriteLockfile(fsys.OSFS{}, dir, &packman.Lockfile{ + Schema: packman.LockfileSchema, + Packs: map[string]packman.LockedPack{ + "https://example.com/tools.git": {Version: "sha:aaaa", Commit: "aaaa", Fetched: time.Date(2026, 1, 2, 3, 4, 5, 0, time.UTC)}, + }, + }); err != nil { + t.Fatalf("WriteLockfile: %v", err) + } + stubCheckUpstreamImports(t, packman.UpstreamStatus{ + Name: "pack:tools", Source: "https://example.com/tools.git", Constraint: "sha:aaaa", + LockedCommit: "aaaa", ResolvedRef: "refs/heads/main", ResolvedCommit: "bbbbbbbbbbbb", + Verdict: packman.UpstreamBehind, + }) + + var stdout, stderr bytes.Buffer + code := run([]string{"--city", dir, "import", "status", "--check-upstream"}, &stdout, &stderr) + if code != 1 { + t.Fatalf("code = %d, want 1; stderr=%s", code, stderr.String()) + } + if !strings.Contains(stdout.String(), "1 checked: 0 current, 1 behind, 0 unreachable, 0 not applicable") { + t.Fatalf("stdout missing the aggregate line:\n%s", stdout.String()) + } + if strings.Contains(stdout.String(), "freshness is measured per repository") { + t.Fatalf("per-repository note printed for a source with no subpath:\n%s", stdout.String()) + } +} + +// TestImportStatusJSONProductionRunWithCheckUpstream runs the real command +// with --check-upstream and validates the emitted document against the +// schema, so a struct field added without a schema property is a red test +// rather than a silent pass. The fixture keeps all three entry shapes -- a +// pinned remote, an unlocked remote, and a path import -- so the shape +// coverage guard stays meaningful on the freshness path too. +func TestImportStatusJSONProductionRunWithCheckUpstream(t *testing.T) { + clearGCEnv(t) + dir := upstreamStatusFixtureCity(t) + stubCheckUpstreamImports(t, + packman.UpstreamStatus{ + Name: "pack:local", Source: "./packs/local", Verdict: packman.UpstreamNotApplicable, + }, + packman.UpstreamStatus{ + Name: "pack:tools", Source: "https://example.com/tools.git", Constraint: "^1.4", + LockedCommit: "aaaa", ResolvedRef: "1.4.2", ResolvedCommit: "aaaa", + Verdict: packman.UpstreamCurrent, + }, + packman.UpstreamStatus{ + Name: "pack:unlocked", Source: "https://example.com/unlocked.git", Constraint: "^9.9", + Verdict: packman.UpstreamNotApplicable, + Err: fmt.Errorf("no packs.lock entry; run \"gc import install\""), + }, + ) + + var stdout, stderr bytes.Buffer + code := run([]string{"--city", dir, "import", "status", "--json", "--check-upstream"}, &stdout, &stderr) + if code != 0 { + t.Fatalf("code = %d, stderr=%q stdout=%q", code, stderr.String(), stdout.String()) + } + + doc := decodeImportStatusDoc(t, stdout.Bytes()) + shapes := map[string]bool{} + for _, imp := range doc.Imports { + if imp.Upstream == nil { + t.Fatalf("entry %q carries no upstream verdict under --check-upstream", imp.Name) + } + switch { + case imp.Kind == "path": + shapes["path"] = true + case imp.Pin != nil: + shapes["pinned remote"] = true + default: + shapes["unlocked remote"] = true + } + } + for _, shape := range []string{"pinned remote", "unlocked remote", "path"} { + if !shapes[shape] { + t.Fatalf("fixture emitted no %s import entry, so the schema validation no longer covers that shape:\n%s", shape, stdout.String()) + } + } + + assertTopLevelOKTrue(t, stdout.Bytes()) + validateJSONAgainstResultSchema(t, []string{"import", "status"}, stdout.Bytes()) +} diff --git a/cmd/gc/cmd_import_test.go b/cmd/gc/cmd_import_test.go index 903ef524f2..0d6db597b4 100644 --- a/cmd/gc/cmd_import_test.go +++ b/cmd/gc/cmd_import_test.go @@ -1520,7 +1520,7 @@ func TestDoImportAddExactLineWhenImportExists(t *testing.T) { if code == 0 { t.Fatal("expected duplicate import add to fail") } - want := "gc import add: import already exists: import \"tools\" already exists\n" + want := "gc import add: import already exists: import \"tools\" already exists; " + importRePinHint + "\n" if stderr.String() != want { t.Fatalf("stderr = %q, want %q", stderr.String(), want) } @@ -3460,3 +3460,169 @@ func TestDefaultImportHeadCommitRedactsUserinfo(t *testing.T) { t.Fatalf("resolve error leaked the userinfo token: %v", err) } } + +// upgradeReportFixture writes a city declaring one remote import at the given +// constraint, pins it in packs.lock at priorCommit, and stubs the selective +// sync to resolve it to syncedCommit. +func upgradeReportFixture(t *testing.T, source, constraint, priorCommit, syncedVersion, syncedCommit string) string { + t.Helper() + dir := t.TempDir() + writeCityToml(t, dir, "[workspace]\nname = \"demo\"\n") + writePackToml(t, dir, fmt.Sprintf(`[pack] +name = "demo" +schema = 1 + +[imports.tools] +source = %q +version = %q +`, source, constraint)) + if priorCommit != "" { + if err := packman.WriteLockfile(fsys.OSFS{}, dir, &packman.Lockfile{ + Schema: packman.LockfileSchema, + Packs: map[string]packman.LockedPack{ + source: {Version: constraint, Commit: priorCommit}, + }, + }); err != nil { + t.Fatalf("WriteLockfile: %v", err) + } + } + prevSelective := syncImportsSelective + t.Cleanup(func() { syncImportsSelective = prevSelective }) + syncImportsSelective = func(_ string, _ map[string]config.Import, _ map[string]struct{}) (*packman.Lockfile, error) { + return &packman.Lockfile{ + Schema: packman.LockfileSchema, + Packs: map[string]packman.LockedPack{ + source: {Version: syncedVersion, Commit: syncedCommit}, + }, + }, nil + } + return dir +} + +// TestDoImportUpgradeUnmovedShaPinReportsNoUpgrade is AC-09. A "sha:" +// constraint names a fixed commit, so `gc import upgrade` over it can never +// move the pin -- yet the old code printed an unconditional `Upgraded import +// "x"` and exited 0, which reads as "you are now up to date" to both an +// operator and a CI log. +// +// The bare success line must not survive anywhere in this path, and the output +// has to say plainly that the commit did not move and where to re-pin. +func TestDoImportUpgradeUnmovedShaPinReportsNoUpgrade(t *testing.T) { + clearGCEnv(t) + const source = "https://example.com/tools.git" + dir := upgradeReportFixture(t, source, "sha:aaaa", "aaaa", "sha:aaaa", "aaaa") + + var stdout, stderr bytes.Buffer + code := doImportUpgrade(dir, "tools", &stdout, &stderr) + if code != 0 { + // Being already current is not an error; REQ-007 asks only that the + // output stop being ambiguous. + t.Fatalf("code = %d, want 0; stderr = %s", code, stderr.String()) + } + out := stdout.String() + if strings.Contains(out, `Upgraded import "tools"`) { + t.Fatalf("unmoved pin still claims an upgrade:\n%s", out) + } + for _, want := range []string{ + `Import "tools" is unchanged`, + `"sha:aaaa"`, + "did not change", + "gc import install", + } { + if !strings.Contains(out, want) { + t.Fatalf("stdout missing %q:\n%s", want, out) + } + } +} + +// TestDoImportUpgradeMovedPinReportsBothCommits keeps the moved case honest in +// the other direction: a real upgrade still says so, and names what it moved +// from and to. +func TestDoImportUpgradeMovedPinReportsBothCommits(t *testing.T) { + clearGCEnv(t) + const source = "https://example.com/tools.git" + dir := upgradeReportFixture(t, source, "^1.4", "aaaaaaaaaaaa", "1.4.9", "bbbbbbbbbbbb") + + var stdout, stderr bytes.Buffer + if code := doImportUpgrade(dir, "tools", &stdout, &stderr); code != 0 { + t.Fatalf("code = %d, stderr = %s", code, stderr.String()) + } + want := `Upgraded import "tools": aaaaaaaa -> bbbbbbbb` + if !strings.Contains(stdout.String(), want) { + t.Fatalf("stdout = %q, want %q", stdout.String(), want) + } +} + +// TestDoImportUpgradeUnmovedSemverPinNamesTheVersion covers the semver arm of +// the unmoved case: already at the highest matching tag is a distinct answer +// from "a fixed commit cannot move", and the operator should be told which. +func TestDoImportUpgradeUnmovedSemverPinNamesTheVersion(t *testing.T) { + clearGCEnv(t) + const source = "https://example.com/tools.git" + dir := upgradeReportFixture(t, source, "^1.4", "aaaa", "1.4.7", "aaaa") + + var stdout, stderr bytes.Buffer + if code := doImportUpgrade(dir, "tools", &stdout, &stderr); code != 0 { + t.Fatalf("code = %d, stderr = %s", code, stderr.String()) + } + out := stdout.String() + if strings.Contains(out, `Upgraded import "tools"`) { + t.Fatalf("unmoved pin still claims an upgrade:\n%s", out) + } + want := `Import "tools" is already at the highest version matching "^1.4" (1.4.7).` + if !strings.Contains(out, want) { + t.Fatalf("stdout = %q, want %q", out, want) + } +} + +// TestDoImportUpgradeAllReportsMovedAndUnchangedCounts pins the all-imports +// form. "Upgraded 3 remote import(s)" counted the lockfile, not the movement, +// so it reported three upgrades on a run that moved nothing. +func TestDoImportUpgradeAllReportsMovedAndUnchangedCounts(t *testing.T) { + clearGCEnv(t) + dir := t.TempDir() + writeCityToml(t, dir, "[workspace]\nname = \"demo\"\n") + writePackToml(t, dir, `[pack] +name = "demo" +schema = 1 + +[imports.tools] +source = "https://example.com/tools.git" +version = "^1.4" + +[imports.base] +source = "https://example.com/base.git" +version = "sha:bbbb" +`) + if err := packman.WriteLockfile(fsys.OSFS{}, dir, &packman.Lockfile{ + Schema: packman.LockfileSchema, + Packs: map[string]packman.LockedPack{ + "https://example.com/tools.git": {Version: "1.4.2", Commit: "aaaa"}, + "https://example.com/base.git": {Version: "sha:bbbb", Commit: "bbbb"}, + }, + }); err != nil { + t.Fatalf("WriteLockfile: %v", err) + } + + prevSync := syncImports + t.Cleanup(func() { syncImports = prevSync }) + syncImports = func(_ string, _ map[string]config.Import, _ packman.InstallMode) (*packman.Lockfile, error) { + return &packman.Lockfile{ + Schema: packman.LockfileSchema, + Packs: map[string]packman.LockedPack{ + // tools moved; base is a sha: pin and cannot. + "https://example.com/tools.git": {Version: "1.4.9", Commit: "cccc"}, + "https://example.com/base.git": {Version: "sha:bbbb", Commit: "bbbb"}, + }, + }, nil + } + + var stdout, stderr bytes.Buffer + if code := doImportUpgrade(dir, "", &stdout, &stderr); code != 0 { + t.Fatalf("code = %d, stderr = %s", code, stderr.String()) + } + want := "Upgraded 1 of 2 remote import(s); 1 unchanged." + if !strings.Contains(stdout.String(), want) { + t.Fatalf("stdout = %q, want %q", stdout.String(), want) + } +} diff --git a/cmd/gc/import_state_doctor_check_test.go b/cmd/gc/import_state_doctor_check_test.go index 56ea349956..9f541439cd 100644 --- a/cmd/gc/import_state_doctor_check_test.go +++ b/cmd/gc/import_state_doctor_check_test.go @@ -1102,3 +1102,48 @@ schema = 1 t.Fatalf("status after fix = %v, want OK; result=%#v", after.Status, after) } } + +// TestImportStateDoctorCheckNeverChecksUpstream is AC-05, the cmd-layer half of +// the offline contract. `gc doctor` runs on machines with no network and must +// keep passing there, so the freshness walk stays opt-in behind +// `gc import status --check-upstream` and this check must never reach it. +// +// The seam is stubbed to fail the test if it is called at all, which is a +// stronger assertion than checking the doctor's verdict: a future refactor that +// called CheckUpstream and merely ignored a failure would still be caught. +func TestImportStateDoctorCheckNeverChecksUpstream(t *testing.T) { + clearGCEnv(t) + cityDir := t.TempDir() + writeCityToml(t, cityDir, "[workspace]\nname = \"demo\"\n") + writePackToml(t, cityDir, `[pack] +name = "demo" +schema = 1 + +[imports.tools] +source = "https://example.com/tools.git" +version = "^1.0" +`) + + prevCheck := checkInstalledImports + t.Cleanup(func() { checkInstalledImports = prevCheck }) + checkInstalledImports = func(_ string, _ map[string]config.Import) (*packman.CheckReport, error) { + return &packman.CheckReport{CheckedSources: 1}, nil + } + + called := false + prevUpstream := checkUpstreamImports + t.Cleanup(func() { checkUpstreamImports = prevUpstream }) + checkUpstreamImports = func(_ string, _ map[string]config.Import, _ *packman.Lockfile) (*packman.UpstreamReport, error) { + called = true + t.Fatal("import-state doctor check resolved upstream freshness; it must stay offline") + return nil, nil + } + + result := newImportStateDoctorCheck(cityDir).Run(&doctor.CheckContext{CityPath: cityDir}) + if result.Status != doctor.StatusOK { + t.Fatalf("status = %v, want OK; result=%#v", result.Status, result) + } + if called { + t.Fatal("checkUpstreamImports was called") + } +} diff --git a/docs/guides/understanding-packs.md b/docs/guides/understanding-packs.md index 69f26ffaed..62f1d2780c 100644 --- a/docs/guides/understanding-packs.md +++ b/docs/guides/understanding-packs.md @@ -314,6 +314,61 @@ $ gc config show --validate $ gc config show | rg 'planner' ``` +### Import Freshness + +`gc import install` and `gc import check` are **offline** and answer a question +about internal consistency: is what you declared installed, pinned, and cached? +They never contact the source, so they cannot tell you that upstream has moved. +A pin can be months stale and pass both cleanly. + +To ask the other question -- has the source moved since we pinned it? -- run: + +```text +$ gc import status --check-upstream +``` + +It resolves each declared remote import's source and compares it against the +`packs.lock` pin, reporting one verdict per import: + +| Verdict | Meaning | +|---|---| +| `current` | The pin already names the resolved upstream commit. | +| `behind` | Upstream resolved to a different commit than the pin. | +| `unreachable` | Resolution failed, so freshness is unknown -- never assumed current. | +| `not_applicable` | A local path source, or a remote with no `packs.lock` entry yet. | + +The command exits `1` when any import is behind, so it works as a CI gate. Add +`--fail-on-unreachable` to also fail when a source cannot be resolved; without +it, an unreachable source is reported but does not fail the run, so a laptop +offline in a tunnel is not treated the same as a stale pin. Every stale import +is also named on stderr with both commits, and `--json` carries the same +verdicts plus a `passed` field mirroring the exit code. + +Freshness is measured **per repository**. Two imports of different subpaths of +one repository share a verdict, so `behind` means the repository moved -- not +necessarily that this pack's files changed. + +Nothing on this path mutates state: no fetch, no re-pin, no cache write. It is +a read-only report by design, which is also why `gc doctor` does not run it -- +`gc doctor` must keep passing on a machine with no network. + +### Re-pinning An Import + +`gc import upgrade` moves a pin only as far as its declared constraint allows. +A `sha:` constraint names one fixed commit, so upgrade cannot move it +at all, and says so rather than reporting an upgrade that did not happen: + +```text +$ gc import upgrade gascity +Import "gascity" is unchanged: constraint "sha:28e2e84e" pins a fixed commit, so there is nothing to upgrade. +``` + +To actually move such an import, edit the `version` in the file that declares +it -- `pack.toml` under `[imports.*]` or `[defaults.rig.imports.*]`, +`city.toml` under `[imports.*]`, or the rig's own `[imports.*]` -- and then run +`gc import install`. The declaration is the source of truth; the lockfile +follows it. + ### Private Packs And Credentials A pack whose source (or a transitive import) is a **private** repository needs a @@ -406,6 +461,7 @@ state. | Share a chosen dependency with the team | `[imports.]` in checked-in TOML | | Install or repair authored imports | `gc import install` | | Check installed import state without mutating | `gc import check` | +| Check whether pinned imports are behind their sources | `gc import status --check-upstream` | | Validate the composed city | `gc config show --validate` | This separation keeps local discovery flexible without making shared config @@ -437,3 +493,7 @@ gc pack registry show main:gascity --refresh Freshness affects discovery, not authored imports. A stale registry cache can hide a newly published pack record from search/show output, but shared `pack.toml` still stores durable import `source` and `version` values. + +Registry freshness and import freshness are different questions: this window +governs the cached *catalog*, while `gc import status --check-upstream` (above) +compares your `packs.lock` pins against the sources they were resolved from. diff --git a/docs/reference/cli.md b/docs/reference/cli.md index de64eea5f1..4027fd9cab 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -2014,7 +2014,17 @@ gc hook run -- [flags] ## gc import -Manage pack imports +Manage pack imports. + +Freshness: "gc import check" and "gc doctor" validate import state offline -- +they answer "is what I declared installed and consistent?", never "has upstream +moved?". A pin can be months stale and pass both. To compare each declared +remote import against its source, run: + + gc import status --check-upstream + +Re-pinning: "gc import upgrade" moves a pin only as far as its declared +constraint allows, so it cannot move a "sha:" pin at all. to re-pin, edit that import's version in the file that declares it (pack.toml [imports.*] or [defaults.rig.imports.*], city.toml [imports.*], or the rig's [imports.*]) and run "gc import install". ``` gc import @@ -2197,17 +2207,31 @@ and rig-scoped [rigs.imports.*]) plus the full packs.lock closure and the lockfile content hash. With --json the output is a stable machine-readable document for drift checkers. +Without --check-upstream the command is entirely offline and reports only what +is already on disk: a pin can be years stale and still look healthy. With +--check-upstream each declared remote import's source is resolved over the +network and compared against its packs.lock pin, and the command exits 1 if any +pin is behind. + ``` gc import status [flags] ``` | Flag | Type | Default | Description | |------|------|---------|-------------| +| `--check-upstream` | bool | | resolve each remote import's source and compare it against its packs.lock pin (network) | +| `--fail-on-unreachable` | bool | | with --check-upstream, also exit 1 when an import's upstream cannot be resolved | | `--json` | bool | | emit JSON result | ## gc import upgrade -Upgrade imported packs within their constraints +Upgrade imported packs within their constraints. + +Only within them: the declared constraint is not rewritten, so a "sha:<commit>" +pin names a fixed commit and this command cannot move it. In that case the +output says so rather than reporting an upgrade that did not happen. + +to re-pin, edit that import's version in the file that declares it (pack.toml [imports.*] or [defaults.rig.imports.*], city.toml [imports.*], or the rig's [imports.*]) and run "gc import install". ``` gc import upgrade [name] diff --git a/docs/reference/system-packs.md b/docs/reference/system-packs.md index da45490bf5..f97e213305 100644 --- a/docs/reference/system-packs.md +++ b/docs/reference/system-packs.md @@ -84,6 +84,14 @@ $ gc import check $ find "$(gc config show --json | jq -r '.pack_dirs[] | select(test("packs/core"))')" -maxdepth 2 -type f | sort ``` +`gc import check` validates this state offline; it does not tell you whether +the pinned commit is still the source's head. Use +`gc import status --check-upstream` for that -- it resolves each declared +remote import's source and reports `current`, `behind`, `unreachable`, or +`not_applicable`, exiting `1` when any pin is behind. See +[Understanding Packs](/guides/understanding-packs) for the full verdict table +and the re-pin path. + The cached files are implementation assets owned by `gc`. They are useful for learning and debugging, but local edits are not a stable customization surface (the binary restores its embedded content). Put custom behavior in @@ -98,6 +106,7 @@ Some commands show the artifacts after the builtin packs are loaded: | `gc skill list` | Skills contributed by loaded packs, including `core.gc-*` skills. | | `gc formula list` | Available formulas, including formulas from builtin packs. See the [Formula Specification](/reference/specs/formula-spec-v2#11-file-naming-and-layers). | | `gc order list` | Available orders, including orders from builtin packs. See [Tutorial 07 - Orders](/tutorials/07-orders). | +| `gc import status --check-upstream` | Whether each declared remote import's pin is still current with its source. | `gc pack registry ...` commands discover public registry entries. They do not make the built-in `core` pack a registry dependency. diff --git a/internal/packman/check_test.go b/internal/packman/check_test.go index dc28cb3bb4..6f2cd98ebf 100644 --- a/internal/packman/check_test.go +++ b/internal/packman/check_test.go @@ -857,3 +857,52 @@ func markCachedPackDirty(t *testing.T, source, commit string) { t.Fatalf("WriteFile(.packman-test-dirty): %v", err) } } + +// TestCheckInstalledMakesNoNetworkCall is the offline contract (REQ-003, +// AC-04). CheckInstalled is what `gc import check` and the import-state doctor +// check run, and adding a freshness walk to this package must not put a +// network call on their path. +// +// runNetworkGit -- the seam every fetch, clone, and ls-remote in this package +// goes through -- is stubbed to fail on any call, so reaching the network at +// all turns the report red rather than merely slow. runGit stays on the local +// cache stub because CheckInstalled legitimately reads the materialized +// checkout (rev-parse HEAD, status --porcelain); those are local, and failing +// them would prove nothing about the network. +func TestCheckInstalledMakesNoNetworkCall(t *testing.T) { + home := t.TempDir() + city := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("GC_HOME", filepath.Join(home, ".gc")) + stubCachedPackGit(t) + + networkCalls := 0 + oldRunNetworkGit := runNetworkGit + t.Cleanup(func() { runNetworkGit = oldRunNetworkGit }) + runNetworkGit = func(_, _, _ string, args ...string) (string, error) { + networkCalls++ + return "", fmt.Errorf("network unavailable for git %s", strings.Join(args, " ")) + } + + writeTestLockfile(t, city, map[string]LockedPack{ + "https://example.com/tools.git": {Version: "1.0.0", Commit: "aaaa"}, + }) + stageCachedPack(t, "https://example.com/tools.git", "aaaa", ` +[pack] +name = "tools" +schema = 1 +`) + + report, err := CheckInstalled(city, map[string]config.Import{ + "pack:tools": {Source: "https://example.com/tools.git", Version: "^1.0"}, + }) + if err != nil { + t.Fatalf("CheckInstalled: %v", err) + } + if report.HasIssues() { + t.Fatalf("issues = %#v, want none", report.Issues) + } + if networkCalls != 0 { + t.Fatalf("CheckInstalled made %d network call(s), want 0", networkCalls) + } +} diff --git a/internal/packman/freshness.go b/internal/packman/freshness.go new file mode 100644 index 0000000000..1247bf7e43 --- /dev/null +++ b/internal/packman/freshness.go @@ -0,0 +1,280 @@ +package packman + +import ( + "fmt" + "strings" + "time" + + "github.com/gastownhall/gascity/internal/config" + "github.com/gastownhall/gascity/internal/fsys" + "github.com/gastownhall/gascity/internal/gitcred" +) + +// UpstreamVerdict is the closed freshness vocabulary a declared import can be +// reported under. It is deliberately small: consumers switch on it, and the +// JSON schema pins it as an enum, so a new value is a contract change. +type UpstreamVerdict string + +const ( + // UpstreamCurrent means the pin already names the resolved upstream commit. + UpstreamCurrent UpstreamVerdict = "current" + // UpstreamBehind means upstream resolved to a different commit than the pin. + UpstreamBehind UpstreamVerdict = "behind" + // UpstreamUnreachable means resolution failed; the pin's freshness is + // unknown, which is never the same answer as "current". + UpstreamUnreachable UpstreamVerdict = "unreachable" + // UpstreamNotApplicable means the import has no upstream to compare against + // (a local path source, or a remote source with no packs.lock entry yet). + UpstreamNotApplicable UpstreamVerdict = "not_applicable" +) + +// UpstreamStatus is one import's freshness answer. +type UpstreamStatus struct { + Name string + Source string + CloneURL string + Subpath string + Constraint string + LockedVersion string + LockedCommit string + LockedFetched time.Time + // ResolvedRef names what upstream was resolved through: a symbolic ref + // such as "refs/heads/main" for a sha: pin, or the selected tag for a + // semver constraint. + ResolvedRef string + ResolvedCommit string + Verdict UpstreamVerdict + // Err carries the resolution failure for an unreachable verdict, and the + // explanation for a not-applicable one. A *gitcred.AuthError survives + // wrapped so the CLI's credential hint still fires. + Err error +} + +// UpstreamReport is the freshness walk over a city's declared imports. +type UpstreamReport struct { + // Checked is the number of declared imports examined, which is also + // len(Statuses): every verdict, not just the ones that hit the network. + Checked int + Statuses []UpstreamStatus // sorted by Name +} + +// Count returns how many statuses carry verdict v. +func (r *UpstreamReport) Count(v UpstreamVerdict) int { + if r == nil { + return 0 + } + count := 0 + for _, status := range r.Statuses { + if status.Verdict == v { + count++ + } + } + return count +} + +// Behind returns the statuses whose pin is behind upstream. +func (r *UpstreamReport) Behind() []UpstreamStatus { + return r.withVerdict(UpstreamBehind) +} + +// Unreachable returns the statuses whose upstream could not be resolved. +func (r *UpstreamReport) Unreachable() []UpstreamStatus { + return r.withVerdict(UpstreamUnreachable) +} + +func (r *UpstreamReport) withVerdict(v UpstreamVerdict) []UpstreamStatus { + if r == nil { + return nil + } + var out []UpstreamStatus + for _, status := range r.Statuses { + if status.Verdict == v { + out = append(out, status) + } + } + return out +} + +// CheckUpstream resolves each declared import's source and compares it against +// the packs.lock pin. Unlike CheckInstalled it performs network operations, so +// it is a sibling entry point rather than an extension of the offline walk: +// nothing that calls CheckInstalled gains a network dependency by its +// existence. A nil lock is read from cityRoot. +// +// It returns an error only when it cannot read its own inputs. A per-import +// resolution failure is data (UpstreamUnreachable), never a returned error -- +// otherwise one dead remote hides the verdict for every other import. +func CheckUpstream(cityRoot string, imports map[string]config.Import, lock *Lockfile) (*UpstreamReport, error) { + if lock == nil { + var err error + lock, err = ReadLockfile(fsys.OSFS{}, cityRoot) + if err != nil { + return nil, err + } + } + + resolver := &upstreamResolver{ + cityRoot: cityRoot, + heads: make(map[string]upstreamResolution), + versions: make(map[string]upstreamResolution), + } + report := &UpstreamReport{} + for _, name := range sortedImportNames(imports) { + report.Statuses = append(report.Statuses, resolver.status(name, imports[name], lock)) + } + report.Checked = len(report.Statuses) + return report, nil +} + +func (r *upstreamResolver) status(name string, imp config.Import, lock *Lockfile) UpstreamStatus { + status := UpstreamStatus{ + Name: name, + Source: imp.Source, + Constraint: imp.Version, + } + + // A scheme-less path source has no upstream to resolve. Note that a + // file:// source is *not* one of these: remotesource.IsRemote treats it as + // remote and ls-remote works against it, so it takes the network path and + // reaches a real verdict like any other remote. + if !isRemoteSource(imp.Source) { + status.Verdict = UpstreamNotApplicable + return status + } + + parsed := normalizeRemoteSource(imp.Source) + status.CloneURL = parsed.CloneURL + status.Subpath = parsed.Subpath + + locked, ok := lock.Packs[imp.Source] + if !ok { + // A missing pin is CheckInstalled's missing-lock-entry to report. The + // two walks must not double-blame the same defect, so this one stops + // at not-applicable with an explanation rather than a verdict. + status.Verdict = UpstreamNotApplicable + status.Err = fmt.Errorf("no %s entry for %q; run \"gc import install\"", + LockfileName, gitcred.RedactUserinfo(imp.Source)) + return status + } + status.LockedVersion = locked.Version + status.LockedCommit = locked.Commit + status.LockedFetched = locked.Fetched + + ref, commit, err := r.resolve(imp) + if err != nil { + status.Verdict = UpstreamUnreachable + status.Err = err + return status + } + status.ResolvedRef = ref + status.ResolvedCommit = commit + if commit == status.LockedCommit { + status.Verdict = UpstreamCurrent + } else { + status.Verdict = UpstreamBehind + } + return status +} + +// upstreamResolver memoizes resolution within one CheckUpstream call. The key +// is the clone URL, not the import: two imports of different subpaths of the +// same repository share one round trip, because freshness is a property of the +// repository. +type upstreamResolver struct { + cityRoot string + heads map[string]upstreamResolution + versions map[string]upstreamResolution +} + +type upstreamResolution struct { + ref string + commit string + err error +} + +func (r *upstreamResolver) resolve(imp config.Import) (string, string, error) { + cloneURL := normalizeRemoteSource(imp.Source).CloneURL + + // A sha: constraint pins a fixed commit, so ResolveVersion short-circuits + // and echoes it back -- comparing that against the pin would report every + // sha: import current no matter how far upstream had moved. The question + // worth asking for a sha: pin is what the source's default branch points at + // now, which is the one genuinely new network call in this package. + if strings.HasPrefix(imp.Version, "sha:") { + res, ok := r.heads[cloneURL] + if !ok { + res.ref, res.commit, res.err = resolveSourceHead(r.cityRoot, imp.Source) + r.heads[cloneURL] = res + } + return res.ref, res.commit, res.err + } + + key := cloneURL + "\x00" + imp.Version + res, ok := r.versions[key] + if !ok { + resolved, err := ResolveVersion(r.cityRoot, imp.Source, imp.Version) + res = upstreamResolution{ref: resolved.Version, commit: resolved.Commit, err: err} + r.versions[key] = res + } + return res.ref, res.commit, res.err +} + +// resolveSourceHead reports the ref and commit the source's HEAD points at. +// +// It goes through runNetworkGit rather than exec'ing git directly, which is +// what makes it inherit credential injection, the SSRF/transport hardening, +// the packman-local network timeout, and the existing test seam. +func resolveSourceHead(cityRoot, source string) (string, string, error) { + cloneURL := normalizeRemoteSource(source).CloneURL + out, err := runNetworkGit(cityRoot, cloneURL, "", "ls-remote", "--symref", cloneURL, "HEAD") + if err != nil { + return "", "", fmt.Errorf("resolving head for %q: %w", gitcred.RedactUserinfo(source), err) + } + ref, commit := parseSymrefHead(out) + if commit == "" { + return "", "", fmt.Errorf("resolving head for %q: no HEAD in ls-remote output", + gitcred.RedactUserinfo(source)) + } + return ref, commit, nil +} + +// parseSymrefHead extracts the default-branch ref and head commit from an +// `ls-remote --symref HEAD` response. +// +// The response is two lines against a normal https remote, but a file:// clone +// of a *non-bare* repository also advertises its remote-tracking refs, and the +// HEAD refspec glob-matches refs/remotes/origin/HEAD -- so the same request can +// come back with four lines and a second, different sha: +// +// ref: refs/heads/main\tHEAD +// \tHEAD +// ref: refs/remotes/origin/main\trefs/remotes/origin/HEAD +// \trefs/remotes/origin/HEAD +// +// Only a line whose second tab-separated field is exactly "HEAD" describes the +// repository's own head. Matching on a "HEAD" suffix, or taking the last +// matching line, silently reports the remote-tracking sha instead and calls an +// up-to-date import behind. +// +// A source that advertises no symref line yields an empty ref and the commit +// alone, which is enough to reach a verdict. +func parseSymrefHead(out string) (string, string) { + ref, commit := "", "" + for _, line := range strings.Split(out, "\n") { + fields := strings.Split(strings.TrimRight(line, "\r"), "\t") + if len(fields) != 2 || fields[1] != "HEAD" { + continue + } + value := strings.TrimSpace(fields[0]) + if rest, found := strings.CutPrefix(value, "ref:"); found { + if ref == "" { + ref = strings.TrimSpace(rest) + } + continue + } + if commit == "" { + commit = value + } + } + return ref, commit +} diff --git a/internal/packman/freshness_test.go b/internal/packman/freshness_test.go new file mode 100644 index 0000000000..b6aa136963 --- /dev/null +++ b/internal/packman/freshness_test.go @@ -0,0 +1,472 @@ +package packman + +import ( + "errors" + "fmt" + "path/filepath" + "strings" + "testing" + + "github.com/gastownhall/gascity/internal/config" + "github.com/gastownhall/gascity/internal/gitcred" +) + +const ( + upstreamToolsSource = "https://example.com/tools.git" + upstreamLockedHead = "1111111111111111111111111111111111111111" + upstreamMovedHead = "2222222222222222222222222222222222222222" +) + +// TestUpstreamFreshnessSeesWhatCheckInstalledCannot is the non-vacuity proof +// for the whole freshness feature. On one fixture city whose on-disk state +// never changes between the two halves, the offline walk reports a perfectly +// healthy import state and the upstream walk reports that same import behind. +// +// Both assertions stay in one function on purpose: the contrast is the test. +// Split across two, a later refactor can delete the half that hurts and leave +// the other one green, and the check goes back to being vacuous without +// anything turning red. +func TestUpstreamFreshnessSeesWhatCheckInstalledCannot(t *testing.T) { + city := newUpstreamFixtureCity(t) + imports := map[string]config.Import{ + "pack:tools": {Source: upstreamToolsSource, Version: "sha:" + upstreamLockedHead}, + } + writeTestLockfile(t, city, map[string]LockedPack{ + upstreamToolsSource: {Version: "sha:" + upstreamLockedHead, Commit: upstreamLockedHead}, + }) + stageCachedPack(t, upstreamToolsSource, upstreamLockedHead, "\n[pack]\nname = \"tools\"\nschema = 1\n") + + installed, err := CheckInstalled(city, imports) + if err != nil { + t.Fatalf("CheckInstalled: %v", err) + } + if installed.HasIssues() { + t.Fatalf("CheckInstalled issues = %#v, want none: declared, locked, and materialized all agree", installed.Issues) + } + + // Same unchanged on-disk state. Only upstream has moved. + stubUpstreamNetworkGit(t, func([]string) (string, error) { + return symrefHeadResponse("refs/heads/main", upstreamMovedHead), nil + }) + + report, err := CheckUpstream(city, imports, nil) + if err != nil { + t.Fatalf("CheckUpstream: %v", err) + } + status := findUpstreamStatus(t, report, "pack:tools") + if status.Verdict != UpstreamBehind { + t.Fatalf("verdict = %q, want %q; err=%v", status.Verdict, UpstreamBehind, status.Err) + } + if status.LockedCommit != upstreamLockedHead { + t.Fatalf("LockedCommit = %q, want %q", status.LockedCommit, upstreamLockedHead) + } + if status.ResolvedCommit != upstreamMovedHead { + t.Fatalf("ResolvedCommit = %q, want %q", status.ResolvedCommit, upstreamMovedHead) + } + if status.ResolvedRef != "refs/heads/main" { + t.Fatalf("ResolvedRef = %q, want refs/heads/main", status.ResolvedRef) + } +} + +// TestCheckUpstreamResolvesEachConstraintKind covers both constraint kinds in +// both directions. The current cases are the load-bearing ones: a walk that +// answered "behind" unconditionally would satisfy the behind rows alone. +func TestCheckUpstreamResolvesEachConstraintKind(t *testing.T) { + const ( + tagCommit147 = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + tagCommit200 = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + ) + tags := fmt.Sprintf("%s\trefs/tags/v1.4.7\n%s\trefs/tags/v2.0.0\n", tagCommit147, tagCommit200) + + tests := []struct { + name string + constraint string + locked string + wantVerdict UpstreamVerdict + wantRef string + wantCommit string + }{ + { + name: "sha pin equal to the source head is current", + constraint: "sha:" + upstreamLockedHead, + locked: upstreamLockedHead, + wantVerdict: UpstreamCurrent, + wantRef: "refs/heads/main", + wantCommit: upstreamLockedHead, + }, + { + name: "sha pin behind the source head is behind", + constraint: "sha:" + upstreamMovedHead, + locked: upstreamMovedHead, + wantVerdict: UpstreamBehind, + wantRef: "refs/heads/main", + wantCommit: upstreamLockedHead, + }, + { + name: "semver pin at the highest matching tag is current", + constraint: "^1.4", + locked: tagCommit147, + wantVerdict: UpstreamCurrent, + wantRef: "1.4.7", + wantCommit: tagCommit147, + }, + { + name: "semver pin below the highest matching tag is behind", + constraint: "^1.4", + locked: "cccccccccccccccccccccccccccccccccccccccc", + wantVerdict: UpstreamBehind, + wantRef: "1.4.7", + wantCommit: tagCommit147, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + city := newUpstreamFixtureCity(t) + stubUpstreamNetworkGit(t, func(args []string) (string, error) { + return upstreamFixtureResponse(args, symrefHeadResponse("refs/heads/main", upstreamLockedHead), tags) + }) + writeTestLockfile(t, city, map[string]LockedPack{ + upstreamToolsSource: {Version: tt.constraint, Commit: tt.locked}, + }) + + report, err := CheckUpstream(city, map[string]config.Import{ + "pack:tools": {Source: upstreamToolsSource, Version: tt.constraint}, + }, nil) + if err != nil { + t.Fatalf("CheckUpstream: %v", err) + } + status := findUpstreamStatus(t, report, "pack:tools") + if status.Verdict != tt.wantVerdict { + t.Fatalf("verdict = %q, want %q; err=%v", status.Verdict, tt.wantVerdict, status.Err) + } + if status.ResolvedRef != tt.wantRef { + t.Fatalf("ResolvedRef = %q, want %q", status.ResolvedRef, tt.wantRef) + } + if status.ResolvedCommit != tt.wantCommit { + t.Fatalf("ResolvedCommit = %q, want %q", status.ResolvedCommit, tt.wantCommit) + } + }) + } +} + +// TestCheckUpstreamParsesFileSourceSymrefWithRemoteTrackingRefs pins T-1. +// +// `ls-remote --symref HEAD` against a clone of a *non-bare* +// repository returns four lines, not two: the clone advertises its +// remote-tracking refs and the HEAD refspec glob-matches +// refs/remotes/origin/HEAD, whose sha is a different commit. A parser that +// suffix-matches "HEAD", or takes the last matching line, reads that second +// sha and calls an up-to-date import behind. +func TestCheckUpstreamParsesFileSourceSymrefWithRemoteTrackingRefs(t *testing.T) { + const ( + source = "file:///gc/apicity" + realHead = "bd29eb3830f4da727f5d1184092192d5dec29142" + trackingHead = "4999445bdd5f5695f67ea182eee69f60e0187598" + ) + city := newUpstreamFixtureCity(t) + stubUpstreamNetworkGit(t, func([]string) (string, error) { + return strings.Join([]string{ + "ref: refs/heads/main\tHEAD", + realHead + "\tHEAD", + "ref: refs/remotes/origin/main\trefs/remotes/origin/HEAD", + trackingHead + "\trefs/remotes/origin/HEAD", + }, "\n") + "\n", nil + }) + writeTestLockfile(t, city, map[string]LockedPack{ + source: {Version: "sha:" + realHead, Commit: realHead}, + }) + + report, err := CheckUpstream(city, map[string]config.Import{ + "pack:apicity-release": {Source: source, Version: "sha:" + realHead}, + }, nil) + if err != nil { + t.Fatalf("CheckUpstream: %v", err) + } + status := findUpstreamStatus(t, report, "pack:apicity-release") + if status.ResolvedCommit == trackingHead { + t.Fatalf("resolved the refs/remotes/origin/HEAD sha %q instead of the repository head", trackingHead) + } + if status.ResolvedCommit != realHead { + t.Fatalf("ResolvedCommit = %q, want %q", status.ResolvedCommit, realHead) + } + if status.ResolvedRef != "refs/heads/main" { + t.Fatalf("ResolvedRef = %q, want refs/heads/main", status.ResolvedRef) + } + if status.Verdict != UpstreamCurrent { + t.Fatalf("verdict = %q, want %q", status.Verdict, UpstreamCurrent) + } +} + +// TestCheckUpstreamTreatsFileSourceAsRemote pins T-2. A file:// source is a +// remote source -- remotesource.IsRemote says so and ls-remote works against +// it -- so it must reach a real verdict. The not-applicable branch is for +// scheme-less path sources, and routing file:// there would silently drop the +// one import most likely to be reported current. +func TestCheckUpstreamTreatsFileSourceAsRemote(t *testing.T) { + city := newUpstreamFixtureCity(t) + pathSource := writeLocalPack(t, "[pack]\nname = \"local\"\nschema = 1\n") + calls := stubUpstreamNetworkGit(t, func([]string) (string, error) { + return symrefHeadResponse("refs/heads/main", upstreamMovedHead), nil + }) + writeTestLockfile(t, city, map[string]LockedPack{ + "file:///srv/pack": {Version: "sha:" + upstreamLockedHead, Commit: upstreamLockedHead}, + }) + + report, err := CheckUpstream(city, map[string]config.Import{ + "pack:file": {Source: "file:///srv/pack", Version: "sha:" + upstreamLockedHead}, + "pack:local": {Source: pathSource}, + }, nil) + if err != nil { + t.Fatalf("CheckUpstream: %v", err) + } + if got := findUpstreamStatus(t, report, "pack:file").Verdict; got != UpstreamBehind { + t.Fatalf("file:// verdict = %q, want %q: file:// is a remote source", got, UpstreamBehind) + } + if got := findUpstreamStatus(t, report, "pack:local").Verdict; got != UpstreamNotApplicable { + t.Fatalf("path verdict = %q, want %q", got, UpstreamNotApplicable) + } + if len(*calls) != 1 { + t.Fatalf("network calls = %v, want exactly the file:// resolution", *calls) + } +} + +// TestCheckUpstreamFallsBackWhenSymrefAbsent covers OQ-3: a source that +// advertises no symref line still yields a usable verdict from the commit line +// alone, with an empty ref rather than a failure. +func TestCheckUpstreamFallsBackWhenSymrefAbsent(t *testing.T) { + city := newUpstreamFixtureCity(t) + stubUpstreamNetworkGit(t, func([]string) (string, error) { + return upstreamMovedHead + "\tHEAD\n", nil + }) + writeTestLockfile(t, city, map[string]LockedPack{ + upstreamToolsSource: {Version: "sha:" + upstreamLockedHead, Commit: upstreamLockedHead}, + }) + + report, err := CheckUpstream(city, map[string]config.Import{ + "pack:tools": {Source: upstreamToolsSource, Version: "sha:" + upstreamLockedHead}, + }, nil) + if err != nil { + t.Fatalf("CheckUpstream: %v", err) + } + status := findUpstreamStatus(t, report, "pack:tools") + if status.Verdict != UpstreamBehind { + t.Fatalf("verdict = %q, want %q; err=%v", status.Verdict, UpstreamBehind, status.Err) + } + if status.ResolvedRef != "" { + t.Fatalf("ResolvedRef = %q, want empty", status.ResolvedRef) + } + if status.ResolvedCommit != upstreamMovedHead { + t.Fatalf("ResolvedCommit = %q, want %q", status.ResolvedCommit, upstreamMovedHead) + } +} + +// TestCheckUpstreamReportsUnlockedSourceNotApplicable keeps the two walks from +// double-blaming one defect: a declared remote with no packs.lock entry is +// CheckInstalled's missing-lock-entry to report, so this walk stops at +// not-applicable and explains itself rather than issuing a freshness verdict +// it has no pin to compare against. +func TestCheckUpstreamReportsUnlockedSourceNotApplicable(t *testing.T) { + city := newUpstreamFixtureCity(t) + calls := stubUpstreamNetworkGit(t, func([]string) (string, error) { + return "", fmt.Errorf("unlocked sources must not reach the network") + }) + writeTestLockfile(t, city, map[string]LockedPack{}) + + report, err := CheckUpstream(city, map[string]config.Import{ + "pack:tools": {Source: upstreamToolsSource, Version: "^1.0"}, + }, nil) + if err != nil { + t.Fatalf("CheckUpstream: %v", err) + } + status := findUpstreamStatus(t, report, "pack:tools") + if status.Verdict != UpstreamNotApplicable { + t.Fatalf("verdict = %q, want %q", status.Verdict, UpstreamNotApplicable) + } + if status.Err == nil || !strings.Contains(status.Err.Error(), "gc import install") { + t.Fatalf("Err = %v, want an explanation naming \"gc import install\"", status.Err) + } + if len(*calls) != 0 { + t.Fatalf("network calls = %v, want none", *calls) + } +} + +// TestCheckUpstreamReportsResolutionFailureUnreachable asserts a dead remote +// is reported unreachable rather than falling through to current, that it does +// not fail the whole walk, and that a typed *gitcred.AuthError survives the +// wrap so the CLI still prints its credential hint (TS-3). +func TestCheckUpstreamReportsResolutionFailureUnreachable(t *testing.T) { + const liveSource = "https://example.com/live.git" + city := newUpstreamFixtureCity(t) + authErr := &gitcred.AuthError{Host: "example.com", OrgPrefix: "example.com/dead", Repo: "https://example.com/dead.git"} + stubUpstreamNetworkGit(t, func(args []string) (string, error) { + if strings.Contains(strings.Join(args, " "), "dead.git") { + return "", authErr + } + return symrefHeadResponse("refs/heads/main", upstreamLockedHead), nil + }) + writeTestLockfile(t, city, map[string]LockedPack{ + "https://example.com/dead.git": {Version: "sha:" + upstreamLockedHead, Commit: upstreamLockedHead}, + liveSource: {Version: "sha:" + upstreamLockedHead, Commit: upstreamLockedHead}, + }) + + report, err := CheckUpstream(city, map[string]config.Import{ + "pack:dead": {Source: "https://example.com/dead.git", Version: "sha:" + upstreamLockedHead}, + "pack:live": {Source: liveSource, Version: "sha:" + upstreamLockedHead}, + }, nil) + if err != nil { + t.Fatalf("CheckUpstream returned an error for a per-import failure: %v", err) + } + dead := findUpstreamStatus(t, report, "pack:dead") + if dead.Verdict != UpstreamUnreachable { + t.Fatalf("verdict = %q, want %q", dead.Verdict, UpstreamUnreachable) + } + if dead.ResolvedCommit != "" { + t.Fatalf("ResolvedCommit = %q, want empty for an unresolved source", dead.ResolvedCommit) + } + var got *gitcred.AuthError + if !errors.As(dead.Err, &got) { + t.Fatalf("Err = %v, want a wrapped *gitcred.AuthError", dead.Err) + } + // One dead remote must not hide every other verdict. + if live := findUpstreamStatus(t, report, "pack:live").Verdict; live != UpstreamCurrent { + t.Fatalf("live verdict = %q, want %q", live, UpstreamCurrent) + } + if n := report.Count(UpstreamUnreachable); n != 1 { + t.Fatalf("Count(unreachable) = %d, want 1", n) + } + if n := len(report.Unreachable()); n != 1 { + t.Fatalf("len(Unreachable()) = %d, want 1", n) + } +} + +// TestCheckUpstreamMemoizesByCloneURL covers OQ-5: freshness is a property of +// the repository, so two subpath imports of one repository cost one round trip. +func TestCheckUpstreamMemoizesByCloneURL(t *testing.T) { + const ( + bdSource = "https://example.com/mono.git//examples/bd" + coreSource = "https://example.com/mono.git//internal/core" + ) + city := newUpstreamFixtureCity(t) + calls := stubUpstreamNetworkGit(t, func([]string) (string, error) { + return symrefHeadResponse("refs/heads/main", upstreamMovedHead), nil + }) + writeTestLockfile(t, city, map[string]LockedPack{ + bdSource: {Version: "sha:" + upstreamLockedHead, Commit: upstreamLockedHead}, + coreSource: {Version: "sha:" + upstreamLockedHead, Commit: upstreamLockedHead}, + }) + + report, err := CheckUpstream(city, map[string]config.Import{ + "pack:bd": {Source: bdSource, Version: "sha:" + upstreamLockedHead}, + "pack:core": {Source: coreSource, Version: "sha:" + upstreamLockedHead}, + }, nil) + if err != nil { + t.Fatalf("CheckUpstream: %v", err) + } + if len(*calls) != 1 { + t.Fatalf("network calls = %v, want 1 for two subpaths of one repository", *calls) + } + if n := report.Count(UpstreamBehind); n != 2 { + t.Fatalf("Count(behind) = %d, want 2", n) + } + if len(report.Behind()) != 2 { + t.Fatalf("len(Behind()) = %d, want 2", len(report.Behind())) + } + if report.Checked != 2 { + t.Fatalf("Checked = %d, want 2", report.Checked) + } +} + +// TestParseSymrefHeadRequiresExactHEADField pins the parse rule directly, so +// the T-1 regression is caught even if the surrounding walk is refactored. +func TestParseSymrefHeadRequiresExactHEADField(t *testing.T) { + tests := []struct { + name string + out string + wantRef string + wantCommit string + }{ + { + name: "two-line https response", + out: "ref: refs/heads/main\tHEAD\ndead\tHEAD\n", + wantRef: "refs/heads/main", + wantCommit: "dead", + }, + { + name: "remote-tracking refs are not HEAD", + out: "ref: refs/remotes/origin/main\trefs/remotes/origin/HEAD\nbeef\trefs/remotes/origin/HEAD\n", + wantRef: "", + wantCommit: "", + }, + { + name: "symref absent", + out: "dead\tHEAD\n", + wantRef: "", + wantCommit: "dead", + }, + { + name: "empty response", + out: "", + wantRef: "", + wantCommit: "", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ref, commit := parseSymrefHead(tt.out) + if ref != tt.wantRef || commit != tt.wantCommit { + t.Fatalf("parseSymrefHead() = (%q, %q), want (%q, %q)", ref, commit, tt.wantRef, tt.wantCommit) + } + }) + } +} + +func newUpstreamFixtureCity(t *testing.T) string { + t.Helper() + home := t.TempDir() + city := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("GC_HOME", filepath.Join(home, ".gc")) + stubCachedPackGit(t) + return city +} + +// stubUpstreamNetworkGit swaps the network seam and returns a pointer to the +// recorded argv of every call, so a test can assert what was *not* fetched. +func stubUpstreamNetworkGit(t *testing.T, respond func(args []string) (string, error)) *[]string { + t.Helper() + calls := []string{} + prev := runNetworkGit + runNetworkGit = func(_, _, _ string, args ...string) (string, error) { + calls = append(calls, strings.Join(args, " ")) + return respond(args) + } + t.Cleanup(func() { runNetworkGit = prev }) + return &calls +} + +func upstreamFixtureResponse(args []string, symref, tags string) (string, error) { + joined := strings.Join(args, " ") + switch { + case strings.Contains(joined, "--symref"): + return symref, nil + case strings.Contains(joined, "--tags"): + return tags, nil + } + return "", fmt.Errorf("unexpected git invocation: %s", joined) +} + +func symrefHeadResponse(ref, commit string) string { + return fmt.Sprintf("ref: %s\tHEAD\n%s\tHEAD\n", ref, commit) +} + +func findUpstreamStatus(t *testing.T, report *UpstreamReport, name string) UpstreamStatus { + t.Helper() + for _, status := range report.Statuses { + if status.Name == name { + return status + } + } + t.Fatalf("no status for %q in %#v", name, report.Statuses) + return UpstreamStatus{} +} diff --git a/schemas/import/status/result.schema.json b/schemas/import/status/result.schema.json index 9e4d99095a..005bfd1789 100644 --- a/schemas/import/status/result.schema.json +++ b/schemas/import/status/result.schema.json @@ -90,6 +90,38 @@ "description": "RFC 3339 UTC fetch timestamp, when recorded." } } + }, + "upstream": { + "type": "object", + "description": "Freshness answer for this import. Present only under --check-upstream.", + "additionalProperties": false, + "required": [ + "verdict" + ], + "properties": { + "verdict": { + "type": "string", + "enum": [ + "current", + "behind", + "unreachable", + "not_applicable" + ], + "description": "Closed freshness vocabulary: \"current\" when the pin names the resolved upstream commit, \"behind\" when upstream resolved to a different commit, \"unreachable\" when resolution failed, and \"not_applicable\" when there is no upstream to compare against." + }, + "resolved_ref": { + "type": "string", + "description": "What upstream was resolved through: a symbolic ref such as \"refs/heads/main\", or the selected tag for a semver constraint." + }, + "resolved_commit": { + "type": "string", + "description": "Commit the source resolved to." + }, + "error": { + "type": "string", + "description": "Explains an unreachable verdict, or why an import has no upstream to compare against." + } + } } } } @@ -124,6 +156,45 @@ } } } + }, + "upstream": { + "type": "object", + "description": "Aggregate freshness verdict. Present only under --check-upstream.", + "additionalProperties": false, + "required": [ + "passed", + "checked", + "current", + "behind", + "unreachable", + "not_applicable" + ], + "properties": { + "passed": { + "type": "boolean", + "description": "Mirrors the process exit code: false when any import is behind, or when any is unreachable and --fail-on-unreachable was passed." + }, + "checked": { + "type": "integer", + "description": "Number of declared imports examined." + }, + "current": { + "type": "integer", + "description": "Imports whose pin names the resolved upstream commit." + }, + "behind": { + "type": "integer", + "description": "Imports whose upstream resolved to a different commit than the pin." + }, + "unreachable": { + "type": "integer", + "description": "Imports whose upstream could not be resolved." + }, + "not_applicable": { + "type": "integer", + "description": "Imports with no upstream to compare against." + } + } } } }