From d07c64b82beff0167c8994b771f8a4f2c97272cc Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Thu, 12 Nov 2026 06:12:48 +0000 Subject: [PATCH] chore(lock): read the lock as written; drop the pre-release spellings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No release wrote pkg:git/… or bare-URL lock resources or string usages, so the translation of them goes; a non-canonical resource is now refused with the form this tool writes. Co-Authored-By: jason.han --- .../unreleased/lock-canonical-only.removed.md | 1 + docs/packages.md | 3 ++ internal/install/install.go | 31 ++++++------ internal/install/install_test.go | 48 +++++++++++-------- internal/lock/legacy_test.go | 48 ------------------- internal/lock/lock.go | 17 ------- internal/lock/lock_test.go | 34 +++++++++++++ 7 files changed, 83 insertions(+), 99 deletions(-) create mode 100644 changes/unreleased/lock-canonical-only.removed.md delete mode 100644 internal/lock/legacy_test.go diff --git a/changes/unreleased/lock-canonical-only.removed.md b/changes/unreleased/lock-canonical-only.removed.md new file mode 100644 index 0000000..b748863 --- /dev/null +++ b/changes/unreleased/lock-canonical-only.removed.md @@ -0,0 +1 @@ +- A `.project.lock.json` is read exactly as this tool writes it: a repository `resource` in a non-canonical spelling (`pkg:git/…`, a bare `+://…`) or a `usage` given as a bare string is refused with the canonical form, instead of being translated as pre-release locks were. diff --git a/docs/packages.md b/docs/packages.md index 09daf1a..151db8b 100644 --- a/docs/packages.md +++ b/docs/packages.md @@ -130,6 +130,9 @@ closure from the lock alone. Two packages that would install under the same published into `lib/` only after every usage resolved and verified; a failing run leaves `lib/` and the lock as they were. A lock entry wins over the constraint in `.project.json` as long as it satisfies it; `install` then verifies the installed copy's digest and fetches only what is missing or changed. +A lock is read as written: a repository `resource` in any spelling other than the canonical +identity above, or a `usage` entry that is a bare string, is refused with the form this tool +writes, not translated. ## Install layout diff --git a/internal/install/install.go b/internal/install/install.go index 90d05ff..fe3133d 100644 --- a/internal/install/install.go +++ b/internal/install/install.go @@ -116,7 +116,9 @@ func Run(ctx context.Context, opts Options) (*Result, error) { if old == nil { old = lock.New() } - keyResources(old) + if err := keyResources(old); err != nil { + return nil, err + } r := &resolver{ctx: ctx, opts: opts, proj: proj, old: old, fresh: lock.New(), chosen: map[string]*lock.Entry{}, res: &Result{}, libDir: filepath.Join(root, project.LibDir), slots: map[string]slot{}} for _, u := range proj.Usage { c, err := usageConstraint(u) @@ -472,20 +474,21 @@ func (r *resolver) requeueLocked(e *lock.Entry, old *lock.Entry) error { return nil } -// keyResources rewrites each lock resource to its matching key, so a lock -// written in an older or differently suffixed spelling still matches its usage. -func keyResources(l *lock.Lock) { - for i := range l.Packages { - l.Packages[i].Resource = lockKey(l.Packages[i].Resource) - } -} - -// lockKey is origin.Key for a repository resource and the resource itself otherwise. -func lockKey(resource string) string { - if o, err := origin.Parse(resource); err == nil && o.Kind == origin.VCS { - return o.Key() +// keyResources rewrites each repository resource to its key, so an entry and a +// usage match however the .git suffix was spelled. Any other spelling is not an +// identity this tool writes, and the lock is refused rather than guessed at. +func keyResources(l *lock.Lock) error { + for i, e := range l.Packages { + o, err := origin.Parse(e.Resource) + if err != nil || o.Kind != origin.VCS { + continue + } + if id := o.Identity(); id != e.Resource { + return fmt.Errorf("%s: packages[%d] resource %s is not canonical (this tool writes %s); delete the lock and run syspi lock", project.LockFile, i, e.Resource, id) + } + l.Packages[i].Resource = o.Key() } - return resource + return nil } func (r *resolver) download(url string, dst string) error { diff --git a/internal/install/install_test.go b/internal/install/install_test.go index 175e807..a52448d 100644 --- a/internal/install/install_test.go +++ b/internal/install/install_test.go @@ -220,20 +220,15 @@ func TestGitInstallLockAndOffline(t *testing.T) { t.Fatalf("re-resolved lock differs:\n%s\n%s", first, third) } - // A lock whose resources carry an older, non-canonical spelling still - // matches the installed trees offline. - legacy := strings.ReplaceAll(string(first), `"`+ident(t, wres)+`"`, `"`+wres+`"`) - if legacy == string(first) { + // A lock whose resources carry a non-canonical spelling is not one this + // tool wrote and is refused, not guessed at. + stale := strings.ReplaceAll(string(first), `"`+ident(t, wres)+`"`, `"`+wres+`"`) + if stale == string(first) { t.Fatal("lock did not contain the canonical widgets identity") } - write(t, dir, project.LockFile, legacy) - res5, log5 := run(t, dir, Options{Offline: true}) - if len(res5.Reused) != 2 || !res5.LockWritten { - t.Fatalf("legacy lock run %+v\n%s", res5, log5) - } - if again, _ := os.ReadFile(filepath.Join(dir, project.LockFile)); !bytes.Equal(again, first) { - t.Fatalf("legacy lock not rewritten canonically:\n%s", again) - } + write(t, dir, project.LockFile, stale) + runErr(t, dir, Options{Offline: true}, "resource "+wres+" is not canonical") + write(t, dir, project.LockFile, string(first)) // Offline with the repository gone: still satisfied. os.RemoveAll(widgets) @@ -253,27 +248,37 @@ func TestGitInstallLockAndOffline(t *testing.T) { func TestKeyResources(t *testing.T) { l := lock.New() - l.Put(lock.Entry{Resource: "pkg:git/example.org/team/widgets", Kind: lock.KindVCS}) - l.Put(lock.Entry{Resource: "git+ssh://example.org/team/gears.git#packages/gears", Kind: lock.KindVCS}) l.Put(lock.Entry{Resource: "https://example.org/dist/tools_1.0.0.kpar", Kind: lock.KindArchive}) l.Put(lock.Entry{Resource: "pkg:generic/rover?vcs_url=git%2Bhttps:%2F%2Fexample.org%2Fteam%2Frover.git", Kind: lock.KindVCS}) - keyResources(l) + l.Put(lock.Entry{Resource: "pkg:generic/gears?vcs_url=git%2Bssh:%2F%2Fexample.org%2Fteam%2Fgears%23packages%2Fgears", Kind: lock.KindVCS}) + if err := keyResources(l); err != nil { + t.Fatal(err) + } want := []string{ - "pkg:generic/widgets?vcs_url=git%2Bhttps:%2F%2Fexample.org%2Fteam%2Fwidgets", - "pkg:generic/gears?vcs_url=git%2Bssh:%2F%2Fexample.org%2Fteam%2Fgears%23packages%2Fgears", "https://example.org/dist/tools_1.0.0.kpar", "pkg:generic/rover?vcs_url=git%2Bhttps:%2F%2Fexample.org%2Fteam%2Frover", + "pkg:generic/gears?vcs_url=git%2Bssh:%2F%2Fexample.org%2Fteam%2Fgears%23packages%2Fgears", } for i, w := range want { if l.Packages[i].Resource != w { t.Errorf("entry %d = %s, want %s", i, l.Packages[i].Resource, w) } } + for _, res := range []string{ + "pkg:git/example.org/team/widgets", + "git+ssh://example.org/team/gears.git#packages/gears", + "pkg:generic/widgets?vcs_url=git%2Bhttps:%2F%2Fexample.org%2Fteam%2Fwidgets%401.0.0", + } { + l := lock.New() + l.Put(lock.Entry{Resource: res, Kind: lock.KindVCS}) + if err := keyResources(l); err == nil || !strings.Contains(err.Error(), "not canonical") { + t.Errorf("%s: want a non-canonical error, got %v", res, err) + } + } } -// A lock written in any spelling of the repository, with or without .git, -// matches a usage in any spelling of the same location, and the installed -// tree is reused offline; a usage naming the other location (repo for +// A lock written with or without .git matches a usage in any spelling of the +// same location, and the installed tree is reused offline; a usage naming the other location (repo for // repo.git) is a different repository and is not served from that tree. func TestLockMatchesAcrossGitSuffixSpellings(t *testing.T) { widgets, _ := fixture(t) @@ -310,6 +315,9 @@ func TestLockMatchesAcrossGitSuffixSpellings(t *testing.T) { t.Fatalf("older source not rewritten:\n%s", again) } for _, lk := range spellings { + if !strings.HasPrefix(lk.res, "pkg:") { + continue // the lock holds canonical identities only + } for _, us := range spellings { p := &project.Project{Name: "app", Publisher: "test", Version: "0.1.0", Usage: []project.Usage{usage(us.res)}} if err := p.Write(dir); err != nil { diff --git a/internal/lock/legacy_test.go b/internal/lock/legacy_test.go deleted file mode 100644 index 7b94b34..0000000 --- a/internal/lock/legacy_test.go +++ /dev/null @@ -1,48 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 - -package lock - -import ( - "os" - "path/filepath" - "testing" -) - -func TestParseLegacyStringUsages(t *testing.T) { - l, err := Parse([]byte(`{"lockVersion":1,"packages":[{"resource":"pkg:git/example.org/app","kind":"vcs","name":"app","version":"1.0.0","usage":["pkg:git/example.org/core@1.2.0",{"resource":"pkg:git/example.org/util","versionConstraint":"^2.0.0"}]}]}`)) - if err != nil { - t.Fatal(err) - } - u := l.Packages[0].Usage - if len(u) != 2 || u[0] != (Usage{Resource: "pkg:git/example.org/core@1.2.0"}) || u[1] != (Usage{Resource: "pkg:git/example.org/util", VersionConstraint: "^2.0.0"}) { - t.Fatalf("%+v", u) - } - if _, err := Parse(l.Marshal()); err != nil { - t.Fatalf("migrated form must round-trip: %v", err) - } -} - -func TestWriteThroughSymlink(t *testing.T) { - root := t.TempDir() - shared := filepath.Join(root, "shared.lock.json") - os.WriteFile(shared, []byte("{}"), 0o644) - dir := filepath.Join(root, "a") - os.Mkdir(dir, 0o755) - if err := os.Symlink(shared, filepath.Join(dir, "lock.json")); err != nil { - t.Skip(err) - } - l := New() - l.Packages = append(l.Packages, Entry{Resource: "pkg:git/example.org/core", Kind: KindVCS, Name: "core", Version: "1.0.0"}) - if wrote, err := l.Write(dir, "lock.json"); err != nil || !wrote { - t.Fatal(wrote, err) - } - if fi, err := os.Lstat(filepath.Join(dir, "lock.json")); err != nil || fi.Mode()&os.ModeSymlink == 0 { - t.Fatal("the symlink must be kept") - } - if got, _ := os.ReadFile(shared); string(got) != string(l.Marshal()) { - t.Fatalf("target not updated:\n%s", got) - } - if left, _ := filepath.Glob(filepath.Join(root, ".*")); len(left) != 0 { - t.Fatalf("temporary files left: %v", left) - } -} diff --git a/internal/lock/lock.go b/internal/lock/lock.go index fd712c9..e73499a 100644 --- a/internal/lock/lock.go +++ b/internal/lock/lock.go @@ -56,23 +56,6 @@ type Usage struct { VersionConstraint string `json:"versionConstraint,omitempty"` } -// UnmarshalJSON also accepts the bare resource string earlier locks wrote -// (its @, if any, is the constraint, as in .project.json). -func (u *Usage) UnmarshalJSON(data []byte) error { - var s string - if err := json.Unmarshal(data, &s); err == nil { - *u = Usage{Resource: s} - return nil - } - type plain Usage - var p plain - if err := json.Unmarshal(data, &p); err != nil { - return err - } - *u = Usage(p) - return nil -} - func sortUsages(u []Usage) { sort.Slice(u, func(i, j int) bool { if u[i].Resource != u[j].Resource { diff --git a/internal/lock/lock_test.go b/internal/lock/lock_test.go index 324d625..511bd45 100644 --- a/internal/lock/lock_test.go +++ b/internal/lock/lock_test.go @@ -4,6 +4,8 @@ package lock import ( "bytes" + "os" + "path/filepath" "strings" "testing" ) @@ -58,3 +60,35 @@ func TestDeterministicRoundTrip(t *testing.T) { } } } + +func TestParseRejectsStringUsages(t *testing.T) { + _, err := Parse([]byte(`{"lockVersion":1,"packages":[{"resource":"pkg:generic/app?vcs_url=git%2Bhttps:%2F%2Fexample.org%2Fapp","kind":"vcs","name":"app","version":"1.0.0","usage":["pkg:generic/core?vcs_url=git%2Bhttps:%2F%2Fexample.org%2Fcore%401.2.0"]}]}`)) + if err == nil || !strings.Contains(err.Error(), "lock:") { + t.Fatalf("a bare usage string must be refused, got %v", err) + } +} + +func TestWriteThroughSymlink(t *testing.T) { + root := t.TempDir() + shared := filepath.Join(root, "shared.lock.json") + os.WriteFile(shared, []byte("{}"), 0o644) + dir := filepath.Join(root, "a") + os.Mkdir(dir, 0o755) + if err := os.Symlink(shared, filepath.Join(dir, "lock.json")); err != nil { + t.Skip(err) + } + l := New() + l.Packages = append(l.Packages, Entry{Resource: "pkg:git/example.org/core", Kind: KindVCS, Name: "core", Version: "1.0.0"}) + if wrote, err := l.Write(dir, "lock.json"); err != nil || !wrote { + t.Fatal(wrote, err) + } + if fi, err := os.Lstat(filepath.Join(dir, "lock.json")); err != nil || fi.Mode()&os.ModeSymlink == 0 { + t.Fatal("the symlink must be kept") + } + if got, _ := os.ReadFile(shared); string(got) != string(l.Marshal()) { + t.Fatalf("target not updated:\n%s", got) + } + if left, _ := filepath.Glob(filepath.Join(root, ".*")); len(left) != 0 { + t.Fatalf("temporary files left: %v", left) + } +}