fix: remediate comprehensive repository review#17
Merged
Conversation
📝 WalkthroughWalkthroughThe CLI now propagates structured errors, supports build metadata and version reporting, and uses context-aware HTTP, download, and storage operations. Catalog caching, watch outcomes, path handling, notifications, CI, release automation, and documentation contracts were also updated. ChangesCLI reliability and repository operations
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as cmd/nugs
participant API as internal/api
participant Cache as internal/cache
participant Download as internal/download
participant Storage as internal/rclone
CLI->>API: Execute context-scoped request
API->>API: Apply domain circuit breaker and retry policy
API-->>CLI: Return typed response or error
CLI->>Cache: Read durable catalog or artist index
Cache-->>CLI: Return cached metadata
CLI->>Download: Run context-aware download
Download->>Storage: Upload or verify through process slot
Storage-->>Download: Return storage result
Download-->>CLI: Return aggregated operation errors
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7db44e5652
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cmd/nugs/main.go (1)
173-180: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve cancellation status for pre-dispatch commands.
A cancellation error returned by
handleCatalogGapsFillorhandleWatchCheckCommandleavesrunCancelledfalse, so the defer records"failed". CheckisCrawlCancelledErr(runErr)when finalizing.Proposed fix
- if runCancelled { + if runCancelled || isCrawlCancelledErr(runErr) { finalizeRuntimeStatus("cancelled") return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/nugs/main.go` around lines 173 - 180, Update the finalization logic around runCancelled and runErr to treat errors recognized by isCrawlCancelledErr(runErr) as cancellation before recording failure. Ensure cancellations returned by handleCatalogGapsFill or handleWatchCheckCommand finalize with "cancelled", while other errors continue to finalize with "failed".internal/download/audio.go (1)
723-734: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winDelay context cancellation until the response body is closed.
Calling
reqCancel()immediately afterapi.Do(req)returns aborts the request's context while the response body is still open. Thenet/httptransport will forcefully tear down the underlying TCP connection instead of returning it to the idle pool. Move the cancellation to after the body is closed to preserve connection keep-alive behavior and drastically improve throughput for sequential/concurrent requests.⚡ Proposed fix for keep-alive connection reuse
- resp, err := api.Do(req) - reqCancel() - if err != nil { - continue - } - if resp.ContentLength > 0 { - mu.Lock() - totalSize += resp.ContentLength - mu.Unlock() - } - _, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, 64<<10)) - _ = resp.Body.Close() + resp, err := api.Do(req) + if err != nil { + reqCancel() + continue + } + if resp.ContentLength > 0 { + mu.Lock() + totalSize += resp.ContentLength + mu.Unlock() + } + _, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, 64<<10)) + _ = resp.Body.Close() + reqCancel()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/download/audio.go` around lines 723 - 734, In the request loop around api.Do(req), defer reqCancel until after the response body has been fully consumed and closed; retain immediate cancellation on the error path, and ensure resp.Body.Close occurs before cancellation so connections remain reusable.internal/api/client.go (1)
154-167: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftFix permanent circuit breaker deadlock on probe network errors.
If a network error (
err != nilfromDo) or a request construction error (makeReqfailure) occurs during a half-open probe,retryDoreturns without updating the circuit breaker. BecauseAllow()setsprobeInFlight = true, the breaker leaks this state and permanently deadlocks incircuitHalfOpen. All subsequent requests to this domain will be continually rejected.Treat these probe-time failures as circuit failures so the breaker transitions back to
circuitOpenand clears theprobeInFlightlock.🔒️ Proposed fix to correctly trip the breaker on probe errors
// 3. Build and execute the request. req, err := makeReq() if err != nil { + if cbState == circuitHalfOpen { + breaker.RecordFailure() + } return nil, err } start := time.Now() resp, err := Do(req) duration := time.Since(start) if err != nil { // Network-level error: log but do NOT trip the circuit breaker. // Network hiccups are distinct from the API being overloaded. LogRequest(label, 0, duration, attempt, cbState.String(), err) + if cbState == circuitHalfOpen { + breaker.RecordFailure() + } return nil, err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/client.go` around lines 154 - 167, Update retryDo’s makeReq and Do error paths to detect when the request is a half-open circuit probe and record a circuit failure through the existing circuit-breaker failure transition, clearing probeInFlight and returning to circuitOpen. Preserve the current behavior for non-probe errors, including logging and returning the original error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 81: Replace the direct go build invocation in the CI build step with the
repository’s make build target. Update the Makefile build target as needed so it
consumes the matrix’s BUILD_TARGET and output-related variables while preserving
the existing cross-build artifact path and build metadata contract.
In @.github/workflows/openwiki-update.yml:
- Around line 51-52: Update the shell validation around
OPENAI_COMPATIBLE_API_KEY to emit a clear error message before exiting when the
variable is missing, while preserving the existing non-empty key validation and
set -euo pipefail behavior.
In `@cmd/nugs/main.go`:
- Around line 493-512: Propagate validation failures as non-zero CLI errors
instead of printing messages and returning nil: in cmd/nugs/main.go lines
493-512, return errors for unknown catalog actions and subcommands; at lines
348-350, 437-439, and 467-469, return errors for non-positive list/catalog
limits and missing catalog artist IDs; in cmd/nugs/watch.go lines 68-83, return
errors when watch add or remove lacks an artist ID. Anchor the changes to the
existing command dispatch and validation functions, preserving successful paths.
In `@cmd/nugs/version_test.go`:
- Around line 9-21: The TestIsVersionRequest test currently groups multiple
cases in separate loops without subtests; refactor it into one table-driven case
list containing each input and expected result, then iterate with t.Run using
descriptive case names and assert isVersionRequest against the expected value.
In `@internal/cache/atomic.go`:
- Around line 33-43: Add a parent-directory fsync in the atomic publish flow
after os.Rename in internal/cache/atomic.go:33-43, before returning success.
Also fsync the catalog-generations directory in the generation
creation/population flow before writing catalog-current.json in
internal/cache/cache.go:197-228; preserve the existing error propagation style
and publish ordering.
In `@internal/cache/cache.go`:
- Around line 236-250: The pruneCatalogGenerations implementation must not
remove generations that an unlocked reader may still reference. Synchronize
generation reads and pruning, or introduce safe deferred reclamation so a
generation remains available from resolution through artifact opening; also stop
relying on sorted MkdirTemp names to identify the previous generation.
In `@internal/cache/full_catalog.go`:
- Around line 29-46: Update ReadFullCatalogArtist and the corresponding
full-catalog write paths to avoid unmarshalling and rewriting the entire
FullCatalogIndex for each artist. Use durable per-artist records, or load the
index once and batch all artist updates within a crawl or analysis operation,
while preserving existing lookup and persistence behavior.
In `@internal/catalog/watch_systemd.go`:
- Around line 224-246: Update snapshotUnitFiles to return the captured
unitSnapshot plus an error, propagating any os.ReadFile failure instead of
recording it as missing; update restoreUnitFiles to return errors from os.Remove
and cache.WriteFileAtomic. At each caller, handle snapshot errors before
proceeding and combine any restoration error with the original operation error
during rollback.
- Around line 86-103: Update the watch installation flow around
writeWatchUnitFiles and systemctlUser to capture the timer’s prior enabled and
active state, then restore both states in rollback when daemon-reload or enable
fails; also update the removal/disable flow at internal/catalog/watch_systemd.go
lines 183-213 to restore the captured state when unit removal or daemon reload
fails. Keep unit-file restoration and existing error propagation intact.
In `@internal/catalog/watch.go`:
- Around line 207-213: Update the catalogUpdateErr branch in the WatchOutcome
error handling to use %w for catalogUpdateErr instead of %v, preserving wrapping
of both outcome and the underlying catalog update error.
In `@internal/config/config.go`:
- Around line 273-281: Update the directory sync flow around dirFile.Sync in the
config synchronization function to skip Sync on Windows while retaining the
existing sync-and-close error handling on other platforms. Ensure dirFile.Close
still runs on every platform and preserves the current error propagation
behavior.
In `@internal/download/video.go`:
- Around line 226-228: Remove the context.Background()-based wrappers
GetSegUrls, GetDuration, and TsToMp4 from internal/download/video.go, then
update every caller to invoke the corresponding context-aware functions and pass
through its existing context. Apply this change at internal/download/video.go
lines 226-228, 404-406, and 517-519; callers should no longer use the removed
legacy APIs.
In `@internal/notify/gotify.go`:
- Around line 50-54: Update the URL construction following validateServerURL so
"/message" is added to the parsed URL's Path rather than concatenated onto
base.String(). Preserve or explicitly handle the existing query and fragment
components, then serialize the modified URL for requestURL.
In `@internal/rclone/concurrency_test.go`:
- Line 18: Update both command stubs assigned to adapter.commandContext to use
their supplied ctx with exec.CommandContext(ctx, "true") instead of
exec.Command("true"), preserving the injected factory contract and eliminating
no-context command creation.
In `@internal/rclone/storage_adapter.go`:
- Around line 39-41: The rclone package must propagate caller-provided contexts
instead of creating context.Background(). In internal/rclone/storage_adapter.go
lines 39-41, remove the WithProcessSlot fallback and require a valid context; in
internal/rclone/rclone.go lines 38-45, add ctx to CheckRcloneAvailable, use it
with exec.CommandContext and WithProcessSlot, and update callers such as
cmd/nugs/rclone.go. Apply the same propagation pattern to CheckRclonePathOnline
and Upload, removing their internal background contexts and updating downstream
callers accordingly.
In `@internal/runtime/detach_unix.go`:
- Line 28: Update the log file setup around os.OpenFile to explicitly apply 0600
via logFile.Chmod after opening, including existing files; if chmod fails, close
the file and return the existing safe error path rather than continuing.
In `@README.md`:
- Around line 194-213: Add a dedicated section to README.md documenting how the
project uses the Claude API and the applicable rate-limit considerations,
including any relevant handling or constraints. Keep the existing Development
and documentation references intact.
---
Outside diff comments:
In `@cmd/nugs/main.go`:
- Around line 173-180: Update the finalization logic around runCancelled and
runErr to treat errors recognized by isCrawlCancelledErr(runErr) as cancellation
before recording failure. Ensure cancellations returned by handleCatalogGapsFill
or handleWatchCheckCommand finalize with "cancelled", while other errors
continue to finalize with "failed".
In `@internal/api/client.go`:
- Around line 154-167: Update retryDo’s makeReq and Do error paths to detect
when the request is a half-open circuit probe and record a circuit failure
through the existing circuit-breaker failure transition, clearing probeInFlight
and returning to circuitOpen. Preserve the current behavior for non-probe
errors, including logging and returning the original error.
In `@internal/download/audio.go`:
- Around line 723-734: In the request loop around api.Do(req), defer reqCancel
until after the response body has been fully consumed and closed; retain
immediate cancellation on the error path, and ensure resp.Body.Close occurs
before cancellation so connections remain reusable.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 5ef7441c-ebe3-4573-9d9f-9b35c929bd57
⛔ Files ignored due to path filters (1)
tools/openwiki/package-lock.jsonis excluded by!**/package-lock.jsonand included by**/*
📒 Files selected for processing (100)
.github/dependabot.yml.github/workflows/ci.yml.github/workflows/openwiki-update.yml.github/workflows/release-please.yml.github/workflows/release.ymlCHANGELOG.mdCLAUDE.mdINCREMENTAL_CATALOG_UPDATE.mdMakefileREADME.mdcmd/nugs/api_client.gocmd/nugs/batch.gocmd/nugs/cache.gocmd/nugs/cancel_unix.gocmd/nugs/cancel_windows.gocmd/nugs/catalog_analysis_mediaaware.gocmd/nugs/catalog_autorefresh.gocmd/nugs/catalog_handlers.gocmd/nugs/cli_integration_test.gocmd/nugs/completions.gocmd/nugs/config.gocmd/nugs/config_ffmpeg_test.gocmd/nugs/detach_common.gocmd/nugs/detach_unix.gocmd/nugs/detach_windows.gocmd/nugs/download.gocmd/nugs/filelock.gocmd/nugs/format.gocmd/nugs/helpers.gocmd/nugs/hotkey_input.gocmd/nugs/list_commands.gocmd/nugs/main.gocmd/nugs/main_flow_test.gocmd/nugs/model_aliases.gocmd/nugs/output.gocmd/nugs/rclone.gocmd/nugs/runtime_status.gocmd/nugs/signal_persistence.gocmd/nugs/structs.gocmd/nugs/url_parser.gocmd/nugs/version.gocmd/nugs/version_test.gocmd/nugs/video.gocmd/nugs/watch.godocs/ARCHITECTURE.mddocs/COMMANDS.mddocs/CONFIG.mddocs/QUICK_REFERENCE.mddocs/nugs-api-endpoints.mddocs/nugs-client-README.mdgo.modinternal/api/apilog.gointernal/api/apilog_test.gointernal/api/circuit.gointernal/api/circuit_test.gointernal/api/client.gointernal/cache/artist_cache.gointernal/cache/atomic.gointernal/cache/cache.gointernal/cache/containers_index_test.gointernal/cache/filelock_windows.gointernal/cache/full_catalog.gointernal/cache/regression_test.gointernal/cache/remediation_test.gointernal/catalog/catalog_update_test.gointernal/catalog/handlers.gointernal/catalog/handlers_coverage_test.gointernal/catalog/helpers.gointernal/catalog/media_filter.gointernal/catalog/request_budget_test.gointernal/catalog/watch.gointernal/catalog/watch_systemd.gointernal/catalog/watch_test.gointernal/config/atomic_test.gointernal/config/config.gointernal/download/audio.gointernal/download/batch.gointernal/download/deps.gointernal/download/deps_test.gointernal/download/precalculate_test.gointernal/download/regression_test.gointernal/download/video.gointernal/helpers/errors.gointernal/helpers/paths.gointernal/helpers/paths_security_test.gointernal/list/list.gointernal/model/constants.gointernal/model/progress_box.gointernal/model/progress_box_test.gointernal/model/types.gointernal/notify/gotify.gointernal/notify/gotify_security_test.gointernal/rclone/concurrency_test.gointernal/rclone/rclone.gointernal/rclone/storage_adapter.gointernal/rclone/storage_adapter_test.gointernal/runtime/detach_unix.gointernal/runtime/status.goscripts/check-docs.pytools/openwiki/package.json
💤 Files with no reviewable changes (2)
- docs/nugs-client-README.md
- internal/model/types.go
Owner
Author
|
Final outside-diff review findings are addressed in
The earlier half-open circuit-breaker cleanup finding was addressed in |
There was a problem hiding this comment.
💡 Codex Review
nugs-cli/internal/download/audio.go
Line 447 in 84c1506
Honor requested audio format when TrackURL is present
When album metadata includes track.TrackURL, this fast path returns that URL immediately and never compares qual.Format with wantFmt or runs the format probing/fallback logic below. For users requesting a different audio format (-f 1, -f 2, etc.) than the metadata URL, the download uses the metadata URL's quality and merely reports a fallback even when the requested quality is available through GetStreamMeta, so normal album downloads can ignore the configured format selection.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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
1 participant
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
Validation
mise x go@1.25.12 -- make verifyactionlintzizmor --pedantic .git diff --checkThe full review artifacts are intentionally local under ignored
.full-review/; all reconciled P0-P3 findings were remediated before this PR was opened.Summary by cubic
Remediates all P0–P3 findings from the comprehensive repo review. Hardens CI/release, makes catalog/download/cache/storage/runtime/notifications fail-safe and truthful, and raises the minimum toolchain to Go 1.25.12.
Bug Fixes
gotifyrequires HTTPS off loopback and never forwards tokens on redirects; runtime/API logs rotate safely with mode 0600.rclonesubprocess slots cap concurrency and preserve the last good bulk index; album uploads are skipped on partial failures; explicit error whenrcloneis enabled without a storage provider.api.WithHTTPClientscopes clients for tests; circuit breaker allows only one half-open probe.WatchOutcomeErrorso the unit fails while counts are preserved; unit sets PATH and quotesExecStart.Refactors
versioncommand; Makefile produces versioned binaries via-ldflagsand installs a stable symlink; version printing works without config and accepts--version/-v; clearer process exit codes.scripts/check-docs.py; actions pinned; workflows use concurrency groups; Dependabot cooldown added.LockFileEx; process-level cache lock; catalog artifacts resolve by generation; durable per-artist shards for the full catalog.README.mdand guides trimmed; Node 22 toolchain foropenwikiwith lockfile checked in.Written for commit 84c1506. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation