[fp-enhancer] Improve pkg/actionpins

[github-actions]

refactor(actionpins): remove redundant slice write-back and simplify warning message

Summary

Pure refactor inside pkg/actionpins/actionpins.go. No behaviour change, no new features, no breaking changes.

Changes

File	Type	Impact
pkg/actionpins/actionpins.go	refactor	low

pkg/actionpins/actionpins.go (1 file changed, 2 insertions(+), 3 deletions(−))

• buildByRepoIndex — slices.SortFunc sorts the backing array in-place; the subsequent byRepo[repo] = repoPins map write-back was a no-op because both the map value and the range variable already share the same underlying array. The redundant assignment and its now-unused loop key variable have been removed.
• ResolveActionPin — replaced a full fmt.Sprintf reassignment of warningMsg with a string-append (warningMsg += ": resolution failed"), making it immediately clear that the base message is a fixed string and only the suffix varies.

Risk assessment

• Breaking change: No
• Test impact: None — pure clean-up with no observable runtime difference
• Diff size: 5 lines touched, net −1 line

Generated by PR Description Updater for issue #39534 · ◷

[github-actions]

⚠️ PR Code Quality Reviewer failed during code quality review.

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR #39534 does not have the 'implementation' label (has_implementation_label=false) and has only 2 new lines of code in business logic directories, well below the 100-line threshold (requires_adr_by_default_volume=false).

[Copilot]

Pull request overview

This PR performs a small refactor in pkg/actionpins to make existing behavior more explicit and slightly simplify message construction, without changing outcomes.

Changes:

• Removes a redundant map write-back after in-place sorting of per-repo []ActionPin slices in buildByRepoIndex.
• Simplifies warning message assembly in ResolveActionPin by appending a conditional suffix instead of reformatting the whole string.

Show a summary per file

File	Description
pkg/actionpins/actionpins.go	Clarifies in-place sort behavior in buildByRepoIndex and simplifies ResolveActionPin warning message construction.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 1/1 changed files
• Comments generated: 0

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

No test files were added or modified in this PR. Test Quality Sentinel skipped.

[github-actions]

Skills-Based Review 🧠

Applied /zoom-out — approving with one non-blocking suggestion.

Both changes are correct and well-reasoned:

• buildByRepoIndex: Removing the write-back is sound. slices.SortFunc mutates only the backing array; the map value already shares that pointer, so byRepo[repo] = repoPins was a genuine no-op. TestBuildByRepoIndex_GroupsByRepoAndSortsDescending continues to serve as the behavioral regression guard. ✅
• warningMsg +=: Semantically identical, cleaner intent — the base message is invariant and only the suffix is conditional. ✅

📋 One suggestion (non-blocking)

The inline comment on the for _, repoPins loop (see inline thread) would prevent future readers from re-adding the write-back as a perceived bug fix. The in-place-sort-with-range-copy idiom is a genuine Go subtlety worth a one-liner.

Positive Highlights

• ✅ Excellent PR description — explains the Go slice semantics clearly
• ✅ Existing test validates the sort order through the map, providing a solid regression guard
• ✅ Minimal footprint: 2 additions / 3 deletions in a single file

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 215.8 AIC · ⌖ 13.9 AIC · ⊞ 29.3K

[github-actions]

[/zoom-out] The write-back removal is correct, but a brief inline comment would future-proof this against someone re-adding it as a perceived bug fix.

slices.SortFunc only rearranges elements in the backing array — it never modifies the slice header (pointer/len/cap). The range copy repoPins shares that same backing array, so after sorting, reading byRepo[repo] already reflects the sorted order. Without a comment, future readers might mistake the missing write-back for a bug.

💡 Suggested comment

for _, repoPins := range byRepo {
    // slices.SortFunc sorts the backing array in-place; repoPins shares
    // the same pointer as the map value, so no write-back is needed.
    slices.SortFunc(repoPins, func(a, b ActionPin) int {

This mirrors the existing // descending by semver comment that already documents the other non-obvious aspect of the sort.

[github-actions]

Both changes are semantically correct refactorings with no behavioral delta.

Change 1 (sort write-back removal): Sound — slices.SortFunc sorts in-place; the range copy of the slice header shares the same backing array, so byRepo[repo] already reflects the sorted order without an explicit write-back. The write-back was a provable no-op.

Change 2 (string append): Equivalent and cleaner. Minor extra allocation on the warning path is negligible.

Non-blocking suggestion

Without the now-deleted byRepo[repo] = repoPins write-back, the sort loop body has no visible side effect on the map — the key is _ and nothing is assigned. A future reader unfamiliar with Go slice aliasing may conclude the sort is dead code and delete it, or add the write-back back as a "fix". A one-line comment would prevent that regression:

for _, repoPins := range byRepo {
    // slices.SortFunc sorts the backing array in place; byRepo shares that
    // array, so no write-back is needed.
    slices.SortFunc(repoPins, ...)
}

This is purely advisory — not a blocking concern.

🔎 Code quality review by PR Code Quality Reviewer · 207.1 AIC · ⌖ 13.4 AIC · ⊞ 17.3K

[github-actions]

Hey @github-actions 👋 — nice clean-up pass on pkg/actionpins! Removing the redundant byRepo[repo] = repoPins write-back and collapsing the warningMsg reassignment into a string append are both clear, correct improvements that make the code’s intent explicit.

One gap before this is fully ready:

• No test changes in the diff — both changes touch production-path logic in buildByRepoIndex and ResolveActionPin. The PR notes existing tests pass, which is good, but a targeted regression test for the sort order invariant (e.g. assert descending semver order after buildByRepoIndex) would lock in the in-place semantics and prevent a future reader from re-introducing the write-back under the mistaken belief it’s necessary.

If you’d like a hand adding that coverage:

Add a unit test to pkg/actionpins/actionpins_test.go for buildByRepoIndex that:
1. Constructs a []ActionPin with at least three entries for the same repo, with versions in non-sorted order (e.g. "v1.0.0", "v3.0.0", "v2.0.0").
2. Calls buildByRepoIndex.
3. Asserts the slice for that repo is sorted in descending semver order (v3 > v2 > v1).
This acts as a regression guard confirming the in-place sort does not require a write-back.

Generated by ✅ Contribution Check · 341 AIC · ⌖ 13.7 AIC · ⊞ 24.5K · ◷