Skip to content

fix: get resolves pinned version instead of defaulting to latest tag #479

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jpower432 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: get resolves pinned version instead of defaulting to latest tag #479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
05cba55
fix: doctor reports pinned-version registries as unreachable
jpower432 Apr 17, 2026
e17d702
fix: get resolves pinned version instead of defaulting to latest tag
jpower432 Apr 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions cmd/complyctl/cli/doctor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,18 +39,30 @@ type registryVersionResolver struct {
}

func (r *registryVersionResolver) ResolveLatestVersion(registryURL, repository string) (string, error) {
return r.resolve(registryURL, repository, "")
}

func (r *registryVersionResolver) ResolveVersion(registryURL, repository, version string) (string, error) {
return r.resolve(registryURL, repository, version)
}

func (r *registryVersionResolver) resolve(registryURL, repository, version string) (string, error) {
credFunc, err := registry.NewCredentialFunc()
if err != nil {
credFunc = nil
}
client := registry.NewClient(registryURL, credFunc)
ctx, cancel := context.WithTimeout(context.Background(), r.timeout)
defer cancel()
_, version, err := client.DefinitionVersion(ctx, repository)
lookup := repository
if version != "" {
lookup = repository + ":" + version
}
_, resolved, err := client.DefinitionVersion(ctx, lookup)
if err != nil {
return "", err
}
return version, nil
return resolved, nil
}

// See FR-039, R44, R51, R52, R55: specs/001-gemara-native-workflow/spec.md
Expand Down
7 changes: 6 additions & 1 deletion internal/cache/sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,12 @@ func (s *Sync) SyncPolicy(ctx context.Context, policyID, version string) error {
return fmt.Errorf("policy ID cannot be empty")
}

remoteDigest, remoteVersion, err := s.source.DefinitionVersion(ctx, policyID)
lookupRef := policyID
if version != "" && version != "latest" {
lookupRef = policyID + ":" + version
}
Copy link
Copy Markdown
Contributor

@marcusburghardt marcusburghardt Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] Missing test coverage for version-specific ref construction

The new branching (version != "" && version != "latest" → policyID + ":" + version) is not exercised by existing sync tests — they all call SyncPolicy with "latest". CRAP score rose from 11.4 → 13.9.

A test like TestSync_CopyOnSuccess_PinnedVersion calling SyncPolicy(ctx, "test-policy", "v1.0.0") and verifying that the source receives the "test-policy:v1.0.0" ref would cover this path.


remoteDigest, remoteVersion, err := s.source.DefinitionVersion(ctx, lookupRef)
if err != nil {
return fmt.Errorf(
"policy %s: registry unreachable: %w (cached data may still be available)",
Expand Down
15 changes: 14 additions & 1 deletion internal/doctor/doctor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ type PolicyGraphResolver interface {
// See R55: specs/001-gemara-native-workflow/spec.md
type VersionResolver interface {
ResolveLatestVersion(registry, repository string) (version string, err error)
ResolveVersion(registry, repository, version string) (string, error)
}

const registryTimeout = 5 * time.Second
Expand Down Expand Up @@ -252,11 +253,23 @@ func CheckPolicyVersions(cfg *complytime.WorkspaceConfig, cacheDir string, versi

latestVersion, err := versionResolver.ResolveLatestVersion(ref.Registry, ref.Repository)
if err != nil {
if ref.Version != "" {
_, pinnedErr := versionResolver.ResolveVersion(ref.Registry, ref.Repository, ref.Version)
if pinnedErr == nil {
results = append(results, CheckResult{
Name: fmt.Sprintf("policy/%s", eid),
Status: StatusPass,
Message: fmt.Sprintf("%s (pinned — latest tag unavailable for staleness check)", cachedState.Version),
Blocking: false,
})
continue
}
}
unreachable[ref.Registry] = true
results = append(results, CheckResult{
Name: fmt.Sprintf("registry/%s", ref.Registry),
Status: StatusWarn,
Message: "unreachable — version check skipped",
Message: fmt.Sprintf("unreachable: %v", err),
Blocking: false,
})
continue
Expand Down
100 changes: 94 additions & 6 deletions internal/doctor/doctor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,23 +17,30 @@ import (
// --- Mock VersionResolver ---

type mockVersionResolver struct {
versions map[string]string // "registry|repo" -> version
unreachable map[string]bool // registry -> true
errOnResolve map[string]error // "registry|repo" -> error
versions map[string]string // "registry|repo" -> latest version
pinnedVersions map[string]string // "registry|repo|version" -> resolved version
unreachable map[string]bool // registry -> true
errOnResolve map[string]error // "registry|repo" -> error
latestMissing map[string]bool // registry -> true (reachable but no latest tag)
}

func newMockVersionResolver() *mockVersionResolver {
return &mockVersionResolver{
versions: make(map[string]string),
unreachable: make(map[string]bool),
errOnResolve: make(map[string]error),
versions: make(map[string]string),
pinnedVersions: make(map[string]string),
unreachable: make(map[string]bool),
errOnResolve: make(map[string]error),
latestMissing: make(map[string]bool),
}
}

func (m *mockVersionResolver) ResolveLatestVersion(registry, repository string) (string, error) {
if m.unreachable[registry] {
return "", fmt.Errorf("connection refused")
}
if m.latestMissing[registry] {
return "", fmt.Errorf("OCI version resolution failed for %s/%s:latest: not found", registry, repository)
}
key := registry + "|" + repository
if err, ok := m.errOnResolve[key]; ok {
return "", err
Expand All @@ -44,6 +51,17 @@ func (m *mockVersionResolver) ResolveLatestVersion(registry, repository string)
return "", fmt.Errorf("not found: %s/%s", registry, repository)
}

func (m *mockVersionResolver) ResolveVersion(registry, repository, version string) (string, error) {
if m.unreachable[registry] {
return "", fmt.Errorf("connection refused")
}
key := registry + "|" + repository + "|" + version
if v, ok := m.pinnedVersions[key]; ok {
return v, nil
}
return "", fmt.Errorf("not found: %s/%s:%s", registry, repository, version)
}

// --- Mock PolicyGraphResolver ---

type mockPolicyGraphResolver struct {
Expand Down Expand Up @@ -231,6 +249,76 @@ func TestCheckPolicyVersions_RegistryUnreachable(t *testing.T) {
if results[0].Status != StatusWarn {
t.Errorf("expected warn, got %s", results[0].Status)
}
if !strings.Contains(results[0].Message, "connection refused") {
Copy link
Copy Markdown
Contributor

@marcusburghardt marcusburghardt Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[INFO] Good coverage — two new test cases cover the pinned-version fallback

TestCheckPolicyVersions_LatestMissing_PinnedResolves (positive) and TestCheckPolicyVersions_LatestMissing_NoPinnedVersion (negative) properly verify both outcomes of the new fallback logic. The existing TestCheckPolicyVersions_RegistryUnreachable was also updated to assert the actual error message. This is the model the other changed files should follow.

t.Errorf("expected actual error in message, got %q", results[0].Message)
}
}

func TestCheckPolicyVersions_LatestMissing_PinnedResolves(t *testing.T) {
tmpDir := t.TempDir()

state := &cache.State{Policies: map[string]cache.PolicyState{
"policies/nist": {Version: "v1.0.0"},
}}
if err := cache.SaveState(state, tmpDir); err != nil {
t.Fatal(err)
}

cfg := &complytime.WorkspaceConfig{
Policies: []complytime.PolicyEntry{
{URL: "reg.io/policies/nist@v1.0.0"},
},
}

vr := newMockVersionResolver()
vr.latestMissing["reg.io"] = true
vr.pinnedVersions["reg.io|policies/nist|v1.0.0"] = "v1.0.0"

results := CheckPolicyVersions(cfg, tmpDir, vr)
if len(results) != 1 {
t.Fatalf("expected 1 result, got %d: %+v", len(results), results)
}
if results[0].Status != StatusPass {
t.Errorf("expected pass for reachable registry with pinned version, got %s: %s",
results[0].Status, results[0].Message)
}
if !strings.Contains(results[0].Message, "pinned") {
t.Errorf("expected 'pinned' in message, got %q", results[0].Message)
}
if !strings.Contains(results[0].Message, "latest tag unavailable") {
t.Errorf("expected 'latest tag unavailable' in message, got %q", results[0].Message)
}
}

func TestCheckPolicyVersions_LatestMissing_NoPinnedVersion(t *testing.T) {
tmpDir := t.TempDir()

state := &cache.State{Policies: map[string]cache.PolicyState{
"policies/nist": {Version: "v1.0.0"},
}}
if err := cache.SaveState(state, tmpDir); err != nil {
t.Fatal(err)
}

cfg := &complytime.WorkspaceConfig{
Policies: []complytime.PolicyEntry{
{URL: "reg.io/policies/nist"},
},
}

vr := newMockVersionResolver()
vr.latestMissing["reg.io"] = true

results := CheckPolicyVersions(cfg, tmpDir, vr)
if len(results) != 1 {
t.Fatalf("expected 1 result, got %d: %+v", len(results), results)
}
if results[0].Status != StatusWarn {
t.Errorf("expected warn, got %s: %s", results[0].Status, results[0].Message)
}
if results[0].Name != "registry/reg.io" {
t.Errorf("expected registry warning, got %q", results[0].Name)
}
}

func TestCheckPolicyVersions_BadCacheState(t *testing.T) {
Expand Down
5 changes: 4 additions & 1 deletion internal/registry/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,10 @@ func (c *Client) DefinitionVersion(ctx context.Context, modulePath string) (stri
return c.fetcher.DefinitionVersion(ctx, modulePath)
}

ref := fmt.Sprintf("%s/%s:latest", c.registryHost(), modulePath)
ref := fmt.Sprintf("%s/%s", c.registryHost(), modulePath)
if !strings.Contains(modulePath, ":") && !strings.Contains(modulePath, "@") {
ref += ":latest"
Copy link
Copy Markdown
Contributor

@marcusburghardt marcusburghardt Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] CRAP regression: DefinitionVersion crossed CRAPload threshold (6.7 → 15.3)

The new strings.Contains branching (lines 56-58) is untested — all existing tests in client_test.go use NewClientWithFetcher with a mock, which returns at the c.fetcher != nil guard (line 52) before reaching this logic.

Consider adding tests that use a Client without a fetcher (or with a nil fetcher) and exercise three paths:

  1. modulePath without : or @ → should append :latest
  2. modulePath with : (e.g., "policies/nist:v1.0.0") → should NOT append :latest
  3. modulePath with @ (e.g., "policies/nist@sha256:abc") → should NOT append :latest

Since these hit a real registry resolve, you'd likely need to test at the ref-construction level (e.g., extract a buildRef helper that's independently testable) or use fetchVersion with a test server.

}
digest, version, err := c.fetchVersion(ctx, ref)
if err != nil {
return "", "", fmt.Errorf("failed to fetch version for %s: %w", modulePath, err)
Expand Down
Loading