Conversation
…rnings init - Extract buildByRepoIndex() as a pure function from sync.Once closure, grouping and sorting pins by repo. Improves testability and separates concerns from the initialization path. - Extract countPinKeyMismatches() to validate pin entry consistency outside the initialization closure, making the logic independently inspectable. - Extract initWarnings() to eliminate two identical lazy-init blocks for ctx.Warnings in ResolveActionPin, reducing mutation duplication. All existing tests pass. No behavior changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…s-functional-helpers-5fc1192535e0e569 # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/951ca789-b015-485d-b1e2-89ae9685c175 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Done: merged Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors pkg/actionpins to improve functional purity/testability by extracting helper logic out of the sync.Once initializer and removing duplicated warnings-map initialization, while keeping behavior the same.
Changes:
- Extracted
buildByRepoIndex()to build the per-repo index (version-descending) outside thesync.Onceclosure. - Extracted
countPinKeyMismatches()to isolate mismatch detection/logging and return a count. - Added
initWarnings()helper and internal tests for the new helpers.
Show a summary per file
| File | Description |
|---|---|
| pkg/actionpins/actionpins.go | Extracts helper functions, simplifies sync.Once init, and deduplicates warnings-map initialization. |
| pkg/actionpins/actionpins_internal_test.go | Adds package-internal unit tests covering the extracted helpers. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
…ionpins Generated by Design Decision Gate workflow run 24726693548. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
|
Commit pushed:
|
Contributor
Author
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (113 new lines in AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Draft ADR SummaryDecision captured: Extract three inline behaviours ( Alternatives captured:
Key consequences: Helpers are now independently unit-testable; duplicate nil-guard risk eliminated; package surface area grows by three unexported symbols; 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
References: §24726693548
|
Contributor
Author
🧪 Test Quality Sentinel ReportTest Quality Score: 90/100✅ Excellent test quality
Test Classification DetailsView All Test Classifications (3 tests)
Flagged Tests — Requires ReviewNo tests require mandatory remediation. One minor observation: i️
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
Author
There was a problem hiding this comment.
✅ Test Quality Sentinel: 90/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All 3 tests verify observable behavioral contracts of the extracted pure helpers with descriptive assertions and proper build tags.
pelikhan
deleted the
fp-enhancer/actionpins-functional-helpers-5fc1192535e0e569
branch
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.
Summary
Functional/immutability improvements to
pkg/actionpinsas part of the round-robin FP enhancer cycle. Next package:pkg/agentdrain.Three targeted refactors were applied to improve clarity, testability, and reduce mutation duplication — all with zero behavior change.
Changes
1. Extract
buildByRepoIndex()— pure function fromsync.OnceclosureBefore: The logic to group and sort pins by repository was embedded inside the
sync.Onceinitializer, mixing concerns and making it untestable in isolation.After: Extracted as a standalone pure function that takes
[]ActionPinand returnsmap[string][]ActionPin, sorted version-descending per repo. Thesync.Onceclosure now reads cleanly.2. Extract
countPinKeyMismatches()— validation logic out of initBefore: Key/version mismatch counting used a mutable
mismatchCountvariable iterated inline insidesync.Once.After: Extracted as a focused function that iterates entries and logs each mismatch, returning the count. The call site collapses to one conditional line.
3. Extract
initWarnings()— eliminate two duplicate lazy-init blocksBefore:
ResolveActionPinduplicatedif ctx.Warnings == nil { ctx.Warnings = make(map[string]bool) }in two separate code paths.After: Single
initWarnings(ctx)helper called at each site, making the intent explicit and the duplication gone.Benefits
buildByRepoIndexextractioncountPinKeyMismatchesextractioninitWarningsextractionTesting
go test ./pkg/actionpins/)gofmt -lreports no changes)Round-Robin Progress
pkg/actionpins(1/22)pkg/agentdrain