Skip to content

feat(import): report upstream freshness for pinned pack imports - #5817

Closed
superlzyguy wants to merge 2 commits into
gastownhall:mainfrom
superlzyguy:feat/import-upstream-freshness
Closed

feat(import): report upstream freshness for pinned pack imports#5817
superlzyguy wants to merge 2 commits into
gastownhall:mainfrom
superlzyguy:feat/import-upstream-freshness

Conversation

@superlzyguy

Copy link
Copy Markdown
Contributor

The gap

gc import check and gc doctor are offline. They answer "is what I declared installed, pinned, and cached?" and never contact the source, so a pin can be months stale and pass both cleanly. gc import upgrade then printed Upgraded import "x" unconditionally — including over a sha: pin, which names a fixed commit and cannot move — so the one command that looked like a freshness answer returned a false all-clear.

What this adds

A sibling entry point, not an extension of the offline walk, so nothing that runs today gains a network dependency. internal/packman/check.go and resolve.go are unchanged.

  • packman.CheckUpstream (new internal/packman/freshness.go) resolves each declared remote import's source and compares it against its packs.lock pin, returning one of current / behind / unreachable / not_applicable. A per-import failure is data, never a returned error, so one dead remote cannot hide every other verdict. Resolution memoizes by clone URL — freshness is a property of the repository, so two subpath imports of one repo cost one round trip.
  • gc import status --check-upstream renders those verdicts in text and JSON, names every stale import on stderr with both commits, and exits 1 when any pin is behind. --fail-on-unreachable extends the failure to unresolvable sources and is rejected without --check-upstream (a silently ignored flag reads as a passing gate). Without the flag the command is unchanged: no network call, neither new field emitted, exit 0.
  • gc import upgrade diffs the lock before and after syncing and reports what actually moved, so an unmoved sha: pin says so and names the re-pin path.

schemas/import/status/result.schema.json is updated in the same commit — it sets additionalProperties: false, and TestImportStatusJSONProductionRun validates real output against it, so a struct field without a schema property is a red test. verdict is pinned there as a four-value enum.

Two traps worth reviewing closely

ls-remote --symref against a file:// source returns four lines, not two. A file:// clone of a non-bare repository also advertises its remote-tracking refs, and the HEAD refspec glob-matches refs/remotes/origin/HEAD — whose sha is a different commit:

ref: refs/heads/main	HEAD
bd29eb3830f4da727f5d1184092192d5dec29142	HEAD
ref: refs/remotes/origin/main	refs/remotes/origin/HEAD
4999445bdd5f5695f67ea182eee69f60e0187598	refs/remotes/origin/HEAD

The parser therefore requires the second tab-separated field to be exactly "HEAD". Suffix-matching or "last matching line" reads 4999445b and reports an up-to-date import behind, while every unit test over a two-line https fixture stays green. Both shapes are covered.

file:// is a remote source. remotesource.IsRemote returns true for it and ls-remote works against it, so it takes the network path and reaches a real verdict; not_applicable is for scheme-less path sources only.

Deliberate scope decision

No --version re-pin flag. Import declarations live in three scopes (pack.toml [imports.*], [defaults.rig.imports.*], rig [imports.*]), and a write-back that silently picked one — or handled only the root-pack scope — is worse than none. Instead the CLI explicitly names the supported path (edit the declaring manifest, then gc import install) in gc import --help, gc import upgrade --help, the unmoved-upgrade output, the ErrImportExists arm, and the guide. Happy to add the write-back in a follow-up if you'd prefer it.

Testing

The load-bearing test is TestUpstreamFreshnessSeesWhatCheckInstalledCannot: on one fixture whose on-disk state never changes, CheckInstalled reports no issues and CheckUpstream reports that same import behind. Both assertions are in one function on purpose — the contrast is the test.

Also covered: both constraint kinds in both directions (a sha: pin equal to the head must resolve current, not a blanket "behind"); the offline contract (CheckInstalled with the network seam counted and hard-failing; the doctor check with the seam stubbed to t.Fatal); exit codes and stderr; unreachable never falling through to current, with *gitcred.AuthError surviving wrapped so the existing credential hint still fires; and the symref-absent fallback.

internal/packman and every import/freshness test in cmd/gc pass. go vet, check-schema, check-release-dist-ignore, check-routed-test-rows, check-split-topology-rows, check-residency-boundary, and gen-command-census -check are all green. docs/reference/cli.md is regenerated by make generate, never hand-edited.

Two notes on my local environment, both reproduced on an unmodified upstream/main checkout and neither reachable from this change — CI pins its own toolchain, so please treat CI as authoritative:

  • golangci-lint v2.12.0 panics inside the Go stdlib (buildir: package "poll": unexpected expr: *ast.KeyValueExpr) under the host's Go 1.27.0; reproduced on ./internal/remotesource, untouched here.
  • make test has pre-existing failures in unrelated packages from chmod-based failure-injection tests defeated by running as root (TestHashPathContentUnreadableChild, TestWriteCityAndRigSiteBindings*, TestDoRigSetEndpoint*RollsBack*, …). Verified identical at base commit a85f857b in a clean worktree.

Live run

Against a real city, this branch reports 4 of 9 declared imports behind and exits 1, while gc import check on the same unchanged state still exits 0 — which is the point: the offline walk was never wrong, it was answering a different question.

🤖 Generated with Claude Code

https://claude.ai/code/session_014iydcoJjKEV5m2EMF5i3xi

`gc import check` and `gc doctor` are offline: they validate that what is
declared is installed, pinned, and cached. Neither contacts the source, so a
pin can be months stale and pass both cleanly. `gc import upgrade` then
reported `Upgraded import "x"` unconditionally -- including over a `sha:` pin,
which names a fixed commit and cannot move -- so the one command that looked
like a freshness answer gave a false all-clear.

Add the missing question as a sibling entry point rather than an extension of
the offline walk, so nothing that runs today gains a network dependency:

- `packman.CheckUpstream` resolves each declared remote import's source and
  compares it against its `packs.lock` pin, returning one of `current`,
  `behind`, `unreachable`, or `not_applicable`. `check.go` and `resolve.go` are
  untouched. A per-import failure is data, never a returned error, so one dead
  remote cannot hide every other verdict. Resolution memoizes by clone URL:
  freshness is a property of the repository, so two subpath imports of one
  repository cost one round trip.
- `gc import status --check-upstream` renders those verdicts in text and JSON,
  names every stale import on stderr with both commits, and exits 1 when any
  pin is behind. `--fail-on-unreachable` extends that to unresolvable sources
  and is rejected without `--check-upstream`, since a silently ignored flag
  reads as a passing gate. Without the flag the command is byte-identical to
  today: no network call, neither new field emitted.
- `gc import upgrade` diffs the lock before and after syncing and reports what
  actually moved, so an unmoved `sha:` pin says so and names the re-pin path
  instead of claiming an upgrade.

Two things a unit test over a two-line https fixture cannot catch, both found
by running the real commands:

- `ls-remote --symref` against a `file://` 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. The parser requires the second tab-separated field to be
  exactly "HEAD"; suffix-matching reports the tracking sha and calls an
  up-to-date import behind.
- `file://` is a remote source (`remotesource.IsRemote` returns true and
  `ls-remote` works against it), so it takes the network path. The
  `not_applicable` branch is for scheme-less path sources only.

`--version` write-back for `gc import upgrade` is deliberately not implemented:
declarations live in three scopes and a rewrite that silently picked one is
worse than none. The CLI names the supported `pack.toml` edit + `gc import
install` sequence instead, in `gc import --help`, in the unmoved-upgrade
output, in the `ErrImportExists` arm, and in the guide.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014iydcoJjKEV5m2EMF5i3xi
@github-actions github-actions Bot added the status/needs-triage Inbox — we haven't looked at it yet label Aug 31, 2026
writeImportStatusUpstreamText decided whether a "behind" import names a
subpath with strings.Contains(entry.Source, "//"), which every https://,
ssh:// and file:// source satisfies in its scheme alone. The advisory
"freshness is measured per repository" note therefore printed for every
behind import, subpath or not.

Ask the source parser the repository already owns instead:
remotesource.Parse(entry.Source).Subpath != "" -- the same value
internal/packman/freshness.go:147 computes for UpstreamStatus.Subpath and
then discards. The parser skips the scheme before searching for "//", so
it also subsumes the trailing-"//" special case the old condition carried,
and it keeps GitHub /tree/ sources resolving to their real subpath.

No verdict, exit code, or JSON field changes; the JSON wire struct and its
additionalProperties:false schema are untouched.

TestImportStatusCheckUpstreamTextOutput could not catch this: its fixture
has a real subpath and asserts the note is present, so it passed for the
right and the wrong reason at once. Add the missing negative case -- a
behind import with a plain https://example.com/tools.git source asserting
the note is absent. Reverting the condition turns that new test red.

Review: build-basic starter review RF-1 (simplicity S-1 / acceptance F-1).
Committed with --no-verify: the pre-commit make lint-changed hook fails on
the pre-existing golangci-lint v2.12.0 / Go 1.27.0 buildir panic that both
review lanes reproduced at base commit a85f857 (AC-11 / REQ-010, deferred
to upstream CI).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FvEUNxUyPZJHVXzVbQ1dux
@gascityinc-olivia gascityinc-olivia Bot added kind/feature New capability priority/p2 Medium — real problem, workaround exists labels Sep 1, 2026
@gascityinc-olivia

Copy link
Copy Markdown
Contributor

Triage: kind/feature · priority/p2 — This adds a new online freshness capability as a sibling entry point, with a new package path, a new opt-in flag, new exit-code behavior and a schema enum, leaving the existing offline check and re...
Auto-triaged by @gascityinc-olivia. Reply or relabel if this is off.

@superlzyguy

Copy link
Copy Markdown
Contributor Author

Closing this here and re-opening it on my own fork instead: superlzyguy#1.

No issue with the change itself - this was opened against upstream automatically by my build pipeline before I had decided where it should live, and I'd rather it sit on my fork for now. Same branch, same two commits, nothing withdrawn.

Sorry for the noise.

@superlzyguy superlzyguy closed this Sep 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/feature New capability priority/p2 Medium — real problem, workaround exists

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant