Conversation
📝 WalkthroughWalkthroughThe pull request introduces continuous integration and code quality configuration alongside a comprehensive refactoring of the cache module. Changes include new CI pipeline definitions, updated dependency versions, hardened binary and HTTP cache write operations, centralised error handling and constants, expanded test coverage, and SonarQube analysis configuration. Changes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 2/5 reviews remaining, refill in 29 minutes and 41 seconds. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cache_test.go (1)
2967-3145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThese race tests lock in an unsupported same-key write guarantee.
This block expects concurrent
Set/Deletework on the same key to stay race-clean and internally serialised, but the cache contract says callers must synchronise same-key writes themselves. Please keep the concurrency coverage on distinct keys, or add explicit external synchronisation in the test harness.As per coding guidelines,
The Cache struct has no mutex; concurrent reads are safe, but concurrent writes to the same key require external synchronization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cache_test.go` around lines 2967 - 3145, The tests TestCache_ThreatTOCTOU_ConcurrentSetSameKeyRaceClean and TestCache_ThreatTOCTOU_ConcurrentGetSetDeleteSameKeyRaceClean incorrectly assume the Cache serializes concurrent writes to the same key; change the tests to either (A) use distinct keys per goroutine (e.g. append the worker index to "race/same" and testRaceMixedKey) so concurrent operations target different keys, or (B) add explicit external synchronization around same-key mutations (e.g. a shared sync.Mutex or a per-key lock map used by runMixedRaceWorker/runMixedRaceOperation) so Set/Delete on the same key are serialized; update related helpers (runMixedRaceWorker, runMixedRaceOperation, checkMixedRaceRead) and the final assertions to reflect the chosen approach.
🧹 Nitpick comments (2)
.woodpecker.yml (1)
11-30: ⚡ Quick winPin the CI images instead of using
latest.Both
golangci/golangci-lint:latest-alpineandsonarsource/sonar-scanner-cli:latestlet upstream image changes alter a previously green pipeline with no repo change. Pin explicit tags here so linting and analysis stay reproducible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.woodpecker.yml around lines 11 - 30, Replace the floating image tags in the CI config by pinning to explicit versions: update the image value for the golangci lint step (currently "golangci/golangci-lint:latest-alpine") to a specific released tag (e.g., "golangci/golangci-lint:v1.X.Y-alpine") and update the sonar step image (currently "sonarsource/sonar-scanner-cli:latest") to a specific scanner CLI tag (e.g., "sonarsource/sonar-scanner-cli:4.X"), ensuring the job names "golangci-lint" and "sonar" keep the new pinned image strings so the pipeline is reproducible.cache_ax7_test.go (1)
5-11: 🏗️ Heavy liftKeep this new suite on
*testing.Trather than the custom test façade.The file is built around
*TandAssert*/Require*, which diverges from the repository’s test contract for_test.gofiles. I’d switch the new helpers and cases to*testing.Tnow, before this style spreads further.As per coding guidelines,
Use testing.T directly in tests, not testify.Also applies to: 32-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cache_ax7_test.go` around lines 5 - 11, The tests are using the repository's custom test façade and Assert*/Require* calls instead of the standard *testing.T; update all test helpers, suite types, and test cases (including any suite struct and methods like SetupTest/TearDownTest and helper functions used across lines ~32-52) to accept t *testing.T and use testing.T APIs (t.Errorf/t.Fatalf/t.Helper/t.Cleanup) instead of Assert*/Require*; change each test function signature to func TestXxx(t *testing.T), update helper signatures to take t *testing.T, replace assertions with equivalent t.* calls, and remove reliance on the custom facade so the file uses testing.T directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.woodpecker.yml:
- Around line 1-3: The comment flags that an admin-scoped Sonar token is being
referenced in .woodpecker.yml; replace it with a project-scoped token by
creating a new Sonar token with only "project analysis" permissions in your
Sonar instance, update the CI secret (sonar_token) in Woodpecker to the new
project-scoped value, revoke/rotate the old admin-scoped token, and update the
comment in .woodpecker.yml to reflect the least-privilege token usage so the
sonar analysis step uses only project-scoped credentials.
In `@cache_ax7_test.go`:
- Around line 67-83: The tests TestCache_New_Ugly and
TestCache_NewCacheStorage_Ugly pass nil as the medium which causes cache.New and
cache.NewCacheStorage to use the real filesystem; replace the nil medium with a
mock medium by calling io.NewMockMedium() and pass that into cache.New and
cache.NewCacheStorage so the tests remain backend-agnostic and hermetic (i.e.,
use io.NewMockMedium() instead of nil when calling cache.New and
cache.NewCacheStorage).
---
Outside diff comments:
In `@cache_test.go`:
- Around line 2967-3145: The tests
TestCache_ThreatTOCTOU_ConcurrentSetSameKeyRaceClean and
TestCache_ThreatTOCTOU_ConcurrentGetSetDeleteSameKeyRaceClean incorrectly assume
the Cache serializes concurrent writes to the same key; change the tests to
either (A) use distinct keys per goroutine (e.g. append the worker index to
"race/same" and testRaceMixedKey) so concurrent operations target different
keys, or (B) add explicit external synchronization around same-key mutations
(e.g. a shared sync.Mutex or a per-key lock map used by
runMixedRaceWorker/runMixedRaceOperation) so Set/Delete on the same key are
serialized; update related helpers (runMixedRaceWorker, runMixedRaceOperation,
checkMixedRaceRead) and the final assertions to reflect the chosen approach.
---
Nitpick comments:
In @.woodpecker.yml:
- Around line 11-30: Replace the floating image tags in the CI config by pinning
to explicit versions: update the image value for the golangci lint step
(currently "golangci/golangci-lint:latest-alpine") to a specific released tag
(e.g., "golangci/golangci-lint:v1.X.Y-alpine") and update the sonar step image
(currently "sonarsource/sonar-scanner-cli:latest") to a specific scanner CLI tag
(e.g., "sonarsource/sonar-scanner-cli:4.X"), ensuring the job names
"golangci-lint" and "sonar" keep the new pinned image strings so the pipeline is
reproducible.
In `@cache_ax7_test.go`:
- Around line 5-11: The tests are using the repository's custom test façade and
Assert*/Require* calls instead of the standard *testing.T; update all test
helpers, suite types, and test cases (including any suite struct and methods
like SetupTest/TearDownTest and helper functions used across lines ~32-52) to
accept t *testing.T and use testing.T APIs
(t.Errorf/t.Fatalf/t.Helper/t.Cleanup) instead of Assert*/Require*; change each
test function signature to func TestXxx(t *testing.T), update helper signatures
to take t *testing.T, replace assertions with equivalent t.* calls, and remove
reliance on the custom facade so the file uses testing.T directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 82056975-c372-4c09-b24f-d2612b28d1df
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
.woodpecker.ymlcache.gocache_ax7_test.gocache_test.gogo.modsonar-project.propertiestests/cli/cache/Taskfile.yaml
| # Woodpecker CI pipeline. | ||
| # Server: ci.lthn.sh. Lint + sonar in parallel, both depend only on clone. | ||
| # sonar_token is admin-scoped on the Woodpecker server. |
There was a problem hiding this comment.
Use a least-privilege Sonar token here.
Line 3 documents an admin-scoped secret for a step that should only need project analysis rights. That unnecessarily increases the blast radius of any CI or branch compromise; please rotate this to a project-scoped token.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.woodpecker.yml around lines 1 - 3, The comment flags that an admin-scoped
Sonar token is being referenced in .woodpecker.yml; replace it with a
project-scoped token by creating a new Sonar token with only "project analysis"
permissions in your Sonar instance, update the CI secret (sonar_token) in
Woodpecker to the new project-scoped value, revoke/rotate the old admin-scoped
token, and update the comment in .woodpecker.yml to reflect the least-privilege
token usage so the sonar analysis step uses only project-scoped credentials.
| func TestCache_New_Ugly(t *T) { | ||
| dir := t.TempDir() | ||
| c, err := cache.New(nil, dir, 0) | ||
|
|
||
| AssertNoError(t, err) | ||
| AssertNotNil(t, c) | ||
| AssertNoError(t, c.Set("agent/defaults", "ready")) | ||
| } | ||
|
|
||
| func TestCache_NewCacheStorage_Ugly(t *T) { | ||
| dir := t.TempDir() | ||
| storage, err := cache.NewCacheStorage(nil, dir) | ||
|
|
||
| AssertNoError(t, err) | ||
| AssertNotNil(t, storage) | ||
| AssertNoError(t, storage.Close()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Don't fall back to coreio.Local in these tests.
Passing nil here routes both cases to the real filesystem, which breaks the repo rule to keep tests on io.NewMockMedium(). Please inject a mock medium explicitly so these cases stay backend-agnostic and hermetic.
As per coding guidelines, Use io.NewMockMedium() for all tests instead of accessing the real filesystem.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cache_ax7_test.go` around lines 67 - 83, The tests TestCache_New_Ugly and
TestCache_NewCacheStorage_Ugly pass nil as the medium which causes cache.New and
cache.NewCacheStorage to use the real filesystem; replace the nil medium with a
mock medium by calling io.NewMockMedium() and pass that into cache.New and
cache.NewCacheStorage so the tests remain backend-agnostic and hermetic (i.e.,
use io.NewMockMedium() instead of nil when calling cache.New and
cache.NewCacheStorage).
1 participant
Brings this repo to verdict: COMPLIANT against the v0.9.0 audit.
🤖 Generated with Claude Code + Codex
Co-Authored-By: Codex noreply@openai.com
Co-Authored-By: Virgil virgil@lethean.io
Summary by CodeRabbit
Release Notes
Bug Fixes
Chores