[Repo Assist] refactor: extract SortedSetKeys and atomicWriteFile to eliminate code duplication#8042
Merged
lpcox merged 1 commit intoJun 24, 2026
Conversation
- Add strutil.SortedSetKeys to eliminate the duplicated map-to-sorted- slice pattern found in 3 locations (closes #8022) - Add logger.atomicWriteFile to unify the atomic temp-file+rename pattern used by ToolsLogger and ObservedURLDomainsLogger (closes #8023) - Fix latent bug in tools_logger.go: os.Remove on rename failure now uses the os.IsNotExist guard that was already present in observed_url_domains_logger.go - Replace 3 call sites in urlutil/domains.go and logger/ with strutil.SortedSetKeys, removing 18 lines of duplicated code Both improvements address patterns identified in duplicate code analysis report #8021.⚠️ Test Status: Build/test infrastructure (proxy.golang.org) is blocked in this environment; tests are expected to pass in CI. Code passes gofmt and all 6 changed files parse without errors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors duplicated “set → sorted slice” and “atomic write (temp + rename)” patterns into shared helpers to reduce divergence risk and keep behavior consistent across internal/urlutil, internal/strutil, and internal/logger.
Changes:
- Added
strutil.SortedSetKeys(map[string]struct{}) []stringand replaced three call sites that previously hand-rolled map key sorting. - Added
logger.atomicWriteFile(filePath, data, perm)and updatedToolsLogger+ObservedURLDomainsLoggerto delegate atomic file writes through it. - Added unit tests for
SortedSetKeys.
Show a summary per file
| File | Description |
|---|---|
| internal/urlutil/domains.go | Replaces duplicated domain-set key sorting with strutil.SortedSetKeys. |
| internal/strutil/strutil.go | Introduces SortedSetKeys helper for consistent, reusable sorted key extraction. |
| internal/strutil/util_test.go | Adds TestSortedSetKeys covering sorting + empty/nil behavior. |
| internal/logger/common.go | Adds shared atomicWriteFile helper to unify atomic-write + cleanup behavior. |
| internal/logger/tools_logger.go | Delegates ToolsLogger.writeToFile atomic write logic to atomicWriteFile. |
| internal/logger/observed_url_domains_logger.go | Delegates both domain-set sorting and atomic write logic to shared helpers. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 0
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.
🤖 This is an automated PR from Repo Assist.
Addresses duplicate code patterns identified in #8021, specifically sub-issues #8022 and #8023.
What Changed
1.
strutil.SortedSetKeys— new utility (closes #8022)Added
SortedSetKeys(set map[string]struct{}) []stringtointernal/strutil/strutil.go. This eliminates the identical 5-line map-to-sorted-slice block that appeared in three places:internal/urlutil/domains.go(×2:ExtractURLDomainsFromValue,ExtractURLDomains)internal/logger/observed_url_domains_logger.go(×1:writeToFile)2.
atomicWriteFile— shared logger helper + bug fix (closes #8023)Added
atomicWriteFile(filePath string, data []byte, perm os.FileMode) errortointernal/logger/common.go. This helper:os.IsNotExistguard on cleanup that was already present inobserved_url_domains_logger.gobut missing intools_logger.go(latent bug fixed)Both
ToolsLogger.writeToFileandObservedURLDomainsLogger.writeToFilenow delegate to this helper.Root Cause
The
writeToFilemethods intools_logger.goandobserved_url_domains_logger.gowere written independently and diverged in error handling. Theos.Removecall intools_logger.gosilently discarded cleanup errors, whileobserved_url_domains_logger.gocorrectly guarded againstos.IsNotExist.atomicWriteFileadopts the correct variant for both callers.Impact
Test Status
proxy.golang.org) is blocked in this environment. All 6 changed files passgofmt -ewith no parse errors. CI will run the full test suite includingTestWriteToFile_RenameFailswhich exercises the cleanup path.Existing tests that cover the changed paths:
TestWriteToFile_RenameFails— verifies temp file is cleaned up on rename failureTestWriteToFile_WriteFileFails— verifies error propagation when write failsTestWriteToFile_Success— verifies normal write pathTestSortedSetKeys(new) — 4 subtests covering sorted output, empty/nil maps, single elementWarning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.orgSee Network Configuration for more information.
Add this agentic workflows to your repo
To install this agentic workflow, run