refactor(resolve): decompose resolver and unify pin resolution - #24
Closed
nodeselector wants to merge 6 commits into
Closed
refactor(resolve): decompose resolver and unify pin resolution#24nodeselector wants to merge 6 commits into
nodeselector wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
⚠️ Not ready to approve
There are correctness issues in newly added logic (notably a goroutine loop-variable capture in workflow rewriting and incorrect semver sorting for tag selection) that can lead to wrong behavior at runtime.
Pull request overview
This PR refactors the action resolution subsystem by introducing a new internal/resolve package and a new internal/tag tag-lister/tagging layer, while also extracting shared concurrency primitives (internal/syncmap) and strengthening test coverage around reachability, reverse lookup, and pin lifecycle behavior.
Changes:
- Introduces
internal/resolve(resolver, reachability verification, reverse lookup, peel/pick helpers) and removes legacyinternal/resolverbits. - Adds
internal/tagfor tag listing + release metadata + cooldown logic, including handling annotated tag-object SHAs. - Adds/updates supporting infrastructure and tests (
internal/syncmap,internal/pinpool,internal/pin,internal/dep) and removes the oldinternal/httpmockin favor ofinternal/ghapi/httpmock.
File summaries
| File | Description |
|---|---|
| internal/tag/testing.go | Test helper to construct a tag lister using ghapi + httpmock. |
| internal/tag/tags.go | Implements tag listing/caching + release enrichment and cooldown-aware helpers. |
| internal/tag/tags_test.go | Tests SHA matching and immutable tag-object SHA suggestion behavior. |
| internal/tag/tags_keys_test.go | Ensures tag lister caches are case-normalized and shared across case variants. |
| internal/tag/tagging.go | Tag suggestion, picker curation, and display helpers for tag selection. |
| internal/tag/cooldown.go | Cooldown configuration and helper methods. |
| internal/syncmap/syncmap.go | Adds a generic mutex-guarded map primitive used by caches. |
| internal/syncmap/syncmap_test.go | Concurrency + zero-value usability tests for syncmap.Map. |
| internal/resolver/retry.go | Deleted (retry transport moved/centralized elsewhere). |
| internal/resolver/reachability_integration_test.go | Deleted (integration tests removed from old resolver package). |
| internal/resolve/reverse_lookup.go | Adds reverse lookup (SHA → containing tag/branch) and ref canonicalization. |
| internal/resolve/reverse_lookup_test.go | Unit tests for reverse lookup and branch containment selection. |
| internal/resolve/resolver.go | Constructs the new resolver, caches, options, and progress hooks. |
| internal/resolve/reachability.go | Implements reachability verification with GraphQL batching + REST fallback and pooling. |
| internal/resolve/reachability_test.go | Tests cache/singleflight behavior and reachability classification paths. |
| internal/resolve/pick.go | Helper pickers for preferred candidates and tag selection. |
| internal/resolve/pick_test.go | Tests pick helper behavior. |
| internal/resolve/peel.go | Adds annotated tag-object peeling + cache, and thin wrappers for GH API calls. |
| internal/resolve/peel_test.go | Tests peel caching behavior. |
| internal/resolve/errors.go | Defines ImpostorError used for fail-closed impostor signaling. |
| internal/resolve/errors_test.go | Tests ImpostorError formatting expectations. |
| internal/resolve/discovery.go | Implements recursive resolution + progress accounting and latest-ref selection. |
| internal/resolve/discover_test.go | Tests discovery/containment behavior and reverse lookup effects. |
| internal/resolve/cacheentry.go | Defines cache entry value types used by resolver caches. |
| internal/resolve/cacheentry_test.go | Tests cacheentry constants/struct behavior. |
| internal/resolve/ancestry.go | Adds ancestry check logic (Compare API) for lockfile forgery detection. |
| internal/resolve/ancestry_test.go | Tests ancestry classification and helper truncation. |
| internal/pinpool/pool.go | Implements a worker pool with UI reporting + stall hinting. |
| internal/pinpool/pool_test.go | Extensive tests for pool scheduling, reporting serialization, and stall watcher behavior. |
| internal/pin/resolution.go | Adds Resolution type + JSON marshal/unmarshal helpers. |
| internal/pin/resolution_test.go | Tests Resolution string/JSON behavior. |
| internal/pin/record.go | Implements run record schema, JSON output, and log retention GC. |
| internal/pin/record_test.go | Tests record rollups, dedup, JSON schema output, and GC behavior. |
| internal/pin/plan.go | Implements pin planning pipeline: resolve → reachability → narrowing → reverse lookup → record. |
| internal/pin/plan_test.go | Tests planning behavior including partial resolution failures. |
| internal/pin/commit.go | Implements commit phase: workflow rewrites + lockfile updates + save. |
| internal/pin/commit_test.go | Tests commit helpers and grouping/filtering logic. |
| internal/httpmock/httpmock.go | Deleted (replaced by internal/ghapi/httpmock). |
| internal/httpmock/httpmock_test.go | Deleted (tests moved with new mock package). |
| internal/dep/diff.go | Adds structured diffing and ref preservation for dependency lists. |
| internal/dep/diff_test.go | Tests dep diffing and ref preservation. |
| internal/dep/dependency.go | Defines dependency shape, keying, string formatting, and parent map rekeying. |
| internal/dep/dependency_test.go | Tests dependency formatting and key helpers. |
Copilot's findings
- Files reviewed: 46/46 changed files
- Comments generated: 8
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+41
to
+48
| for _, wp := range rec.Workflows { | ||
| g.Go(func() error { | ||
| if err := rewriteWorkflow(wp); err != nil { | ||
| return fmt.Errorf("rewriting %s: %w", wp.Path, err) | ||
| } | ||
| return nil | ||
| }) | ||
| } |
Comment on lines
+5
to
+18
| import "time" | ||
|
|
||
| // CooldownConfig controls the minimum age threshold for tag upgrade suggestions. | ||
| type CooldownConfig struct { | ||
| DefaultDays int | ||
| RepoOverrides map[string]int | ||
| } | ||
|
|
||
| // CooldownDays returns the cooldown period for a repo, falling back to the default. | ||
| func (c CooldownConfig) CooldownDays(owner, repo string) int { | ||
| if days, ok := c.RepoOverrides[owner+"/"+repo]; ok { | ||
| return days | ||
| } | ||
| return c.DefaultDays |
Comment on lines
+126
to
+132
| // Sort: latest semver first, major tags last. | ||
| sort.Slice(tags, func(i, j int) bool { | ||
| if tags[i].IsMajor != tags[j].IsMajor { | ||
| return !tags[i].IsMajor | ||
| } | ||
| return tags[i].Name > tags[j].Name | ||
| }) |
Comment on lines
+235
to
+237
| // Skip tags younger than the cooldown period. | ||
| if tl.isTagTooNew(owner, repo, t.Name) && !strings.EqualFold(t.SHA, pinnedSHA) { | ||
| continue |
Comment on lines
+249
to
+252
| var result []PickerTag | ||
| for _, b := range buckets { | ||
| installed := strings.EqualFold(b.tag.SHA, pinnedSHA) | ||
| result = append(result, PickerTag{ |
Comment on lines
+268
to
+275
| for _, t := range all { | ||
| if strings.EqualFold(t.SHA, pinnedSHA) && !t.IsMajor { | ||
| label := t.Name + " 📦 installed" | ||
| result = append([]PickerTag{{ | ||
| Tag: t, | ||
| Label: label, | ||
| Installed: true, | ||
| }}, result...) |
Comment on lines
+18
to
+20
| func (e *ImpostorError) Error() string { | ||
| return fmt.Sprintf("%s@%s is not on any branch — fork-network / impostor signal; refusing to pin", e.NWO, parserlock.ShortSHA(e.SHA)) | ||
| } |
Comment on lines
+184
to
+186
| resolver, err := resolve.New("github.com", pool, resolve.WithTransport(reg), | ||
| resolve.WithCheckReachabilityFunc(reachFn)) | ||
|
|
nodeselector
force-pushed
the
ns/release/resolve
branch
5 times, most recently
from
June 8, 2026 13:10
58d9e5c to
378bb07
Compare
Reorganize the resolution engine: split wire types out of resolver.go, extract pick/peel helpers and a concurrent syncmap, unify tag listing caches, and route all live resolution through ghapi. Keying is NWO@Ref end to end so SHA-pinned and tag-pinned uses of the same action no longer collide. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the direct go-gh *api.HTTPError type assertion in CheckAncestry with ghapi.StatusCode, so the resolve package no longer imports go-gh. This closes the last API-transport leak outside internal/ghapi. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ct matching, semver ordering Reachability now fails closed: CheckReachabilityAll captures the worker pool error and backfills any unset result as ReachabilityUnknown, so a pool cancel/error can no longer leave a dep with an empty status that slips past plan.go and gets pinned unverified (B1). reachabilityScan falls back to serial REST Compare whenever the batched GraphQL check returns an error without a positive match, including partial failures — previously a batch that errored after checking only some branches was reported as a definitive miss (false Unreachable) (C1). Tag-object-aware matching: the cooldown filter and picker 'installed' checks in tagging.go use Info.MatchesSHA instead of comparing the peeled commit SHA only, so immutable-release pins (which target the annotated tag-object SHA) are recognized (Copilot #4-6). ListTags orders tags by semantic version via SemVer.Greater rather than a lexical name compare, so v10 sorts ahead of v9 (Copilot #3). CooldownDays falls back to a case-insensitive RepoOverrides lookup (Copilot #2). ImpostorError.Error now includes the ref (Copilot #7). plan_test checks the resolve.New error (Copilot #8). Adds regression tests for semver ordering and case-insensitive cooldown, and asserts the ref appears in ImpostorError output. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Resolution peels annotated tags to the commit (`^{commit}`) before any SHA is
written to a lockfile, so a pin is always a commit SHA. Drop the speculative
tag-object machinery — the Info.TagObjectSHA field, the fetchTagObjectSHAs
enrichment round trip, and MatchesSHA's dual commit/tag-object comparison —
and match on the commit SHA alone. Annotated-tag-object pins, which we never
emit, are handled where it matters by the misleading-SHA check peeling the
pinned ref at scan time.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The discovery walk resolved one ref per GraphQL POST inside the worker pool, defeating ResolveActionFiles' batched a0..aN design — a 59-ref scan fired 59 round-trips. Chunk each wave's uncached refs into batches of 20 and resolve a chunk per pool job, folding the wave into a handful of POSTs. Per-ref results are still cached and their errors joined individually, so one bad ref fails only itself, not its chunk-mates. The pool's own '[done/total] label' would count chunks while the ref-denominated resolve progress bar counts refs, so the two writers would fight over the spinner label. Give pinpool.Run an empty-label convention: '' suppresses the pool's label writes (per-worker status rows and stall hints are unaffected) and hands label ownership to the caller. Dedup the first-wave progress preseed by cache key so duplicate input refs don't inflate the total. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
nodeselector
force-pushed
the
ns/release/resolve
branch
from
June 8, 2026 13:46
378bb07 to
02ac875
Compare
Collaborator
Author
|
Consolidated into #30. Closing this stacked PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Layer 2/7. Base:
ns/release/ghapi.Decompose the resolution engine: split wire types out of
resolver.go, extract pick/peel helpers and a concurrentsyncmap, unify tag-listing caches, and route live resolution throughghapi. Keying isNWO@Refend to end so SHA-pinned and tag-pinned uses of the same action no longer collide (the root MISLEADING_SHA false-positive).Part of a stacked series for the pre-release hardening of
gh actions-pin. Review bottom-up; each PR is based on the one below it so the diff shows only that layer.