Fix package-level mutable slice/map reassignment in virtual_fs.go and actionpins/data.go#53896
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Hey The refactoring is well-focused:
The changes preserve public APIs and behavior while eliminating the data-race hazards flagged by the linter. This looks ready for review!
|
|
Triage: category= Small, contained bug fix (4 files, 46+/30-) with lgtm label. Undraft to trigger CI, then group with similar low-risk fixes for review.
|
|
❌ Ponytail Reviewer failed. Please review the logs for details. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #53896 does not have the 'implementation' label and has only 46 new lines of code in business logic directories (threshold is 100).
|
|
✅ PR Code Quality Reviewer completed the code quality review. Reviewed PR #53896. No actionable changed-line issues found, so no PR review comments were needed.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
There was a problem hiding this comment.
Pull request overview
Refactors package-level caches to avoid mutable slice/map reassignment while preserving concurrency guarantees and public behavior.
Changes:
- Uses copy-on-write map pointers for built-in virtual files, including WASM reads.
- Consolidates action-pin caches behind a
sync.Once-initialized structure. - Updates internal cache access tests.
Show a summary per file
| File | Description |
|---|---|
pkg/parser/virtual_fs.go |
Stores virtual-file snapshots through a guarded map pointer. |
pkg/parser/virtual_fs_wasm.go |
Updates WASM built-in file lookup. |
pkg/actionpins/data.go |
Consolidates action and container pin caches. |
pkg/actionpins/actionpins_internal_test.go |
Uses the consolidated cache accessor. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Balanced
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The refactor is correct and clean.
pkg/actionpins/data.go — consolidating three mutable package-level variables (cachedActionPins, cachedActionPinsByRepo, cachedContainerPins) into a single *actionPinsCache pointer written exactly once via sync.Once is the right approach. Callers now get a single, stable pointer to an immutable struct rather than racing against three independent assignments.
pkg/parser/virtual_fs.go — using *map[string][]byte (a pointer-to-map) and swapping the pointer atomically under the write lock correctly prevents bare map reassignment while keeping the copy-on-write semantics intact. All read sites consistently dereference the pointer inside the read lock.
No correctness, security, or reliability issues found in the changed lines. ✅
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 17.1 AIC · ⌖ 8.82 AIC · ⊞ 5.7K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes on nil-safety, test coverage, and idiom clarity.
📋 Key Themes & Highlights
Key Themes
- Nil-safety gap:
getCachedActionPins()returnsnilif thesync.Oncebody panics; callers will then panic with an opaque nil-deref instead of a clear error message. - Unusual idiom:
*map[string][]byteas a package-level variable is uncommon in Go and warrants either a named wrapper type or a brief comment explaining the design intent. - Missing regression test: The test file removes the explicit initialiser call but doesn't add a test that validates the new
getCachedActionPins()invariants directly.
Positive Highlights
- ✅ Bundling three separate package-level vars into a single
*actionPinsCacheis a clean structural improvement — fewer globals, clearer ownership. - ✅ The
sync.Onceandsync.RWMutexcontracts are preserved exactly, so the concurrency model is sound. - ✅ No public API changes; the refactor is truly internal.
- ✅ Copy-on-write snapshot pattern in
virtual_fs.gois well-described in the updated comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 33.2 AIC · ⌖ 10.3 AIC · ⊞ 7.8K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/parser/virtual_fs.go:27
[/codebase-design] *map[string][]byte is an uncommon Go idiom — the pointer itself is still being reassigned inside the write-lock, so callers need to understand both the pointer indirection and the mutex contract to reason about safety.
<details>
<summary>💡 Consider a named struct wrapper for clarity</summary>
A thin named wrapper communicates intent more clearly than a raw double-pointer:
type builtinFiles struct {
m map[string][]byte
}
var builtinVirtualFiles = &builtinF…
</details>
<details><summary>pkg/actionpins/data.go:47</summary>
**[/diagnosing-bugs]** `getCachedActionPins()` can return `nil` if `actionPinsOnce.Do` panics or returns early — callers like `getActionPins()` and `GetContainerPin()` will then panic with a nil-pointer dereference.
<details>
<summary>💡 Add a nil guard or document the invariant</summary>
`actionPinsOnce.Do` won't run again after a panic, and `cachedPins` will remain `nil`. Any subsequent call to `getCachedActionPins().pins` will panic.
Either:
1. Document explicitly that `getCachedActionPin…
</details>
<details><summary>pkg/actionpins/actionpins_internal_test.go:422</summary>
**[/tdd]** The test update removes the explicit `getActionPins()` call but doesn't add a regression test that verifies the cache is populated correctly through the new `getCachedActionPins()` path.
<details>
<summary>💡 Add a test that exercises the new accessor directly</summary>
The removed `getActionPins()` call was acting as a test-local initialiser. Now the test relies on side-effects from elsewhere in the test file. Consider adding an explicit test:
```go
func TestGetCachedActionPins_I…
</details>
|
@copilot Please address the latest maintainer-visible blockers on this PR and then run the Current maintainer-visible blockers:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in 90a0010: |
|
@copilot Please address the latest maintainer-visible blockers on this PR, refresh the branch if needed, and then run the
|
…package-level-cleanup Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Refreshed the branch with current |
LintMonster flagged 5 findings where package-level slice/map variables were mutated via wholesale reassignment, risking shared-state leakage and data races:
builtinVirtualFilesinpkg/parser/virtual_fs.go, andcachedActionPins,cachedActionPinsByRepo,cachedContainerPinsinpkg/actionpins/data.go.pkg/parser/virtual_fs.gobuiltinVirtualFileschanged frommap[string][]byteto*map[string][]byte. Registration still builds a fresh copy-on-write snapshot, but now swaps the pointer instead of reassigning the map itself, so the package-level variable is never mutated in place.BuiltinVirtualFileExists,readFileFunc, wasm build) dereference the pointer; existingsync.RWMutexguarding is unchanged.pkg/actionpins/data.goactionPinsCachestruct bundlingpins,byRepo, andcontainers.*actionPinsCachepointer (cachedPins), populated exactly once inside the existingsync.Once.getActionPins(),GetActionPinsByRepo(), andGetContainerPin()now read through agetCachedActionPins()accessor instead of touching separate globals.Public APIs and behavior are unchanged — this is purely an internal storage-representation change to eliminate the reassignment pattern the linter flags.