The no-space form after the timestamp is correct and unchanged: LRC is
[mm:ss.xx]text, and the Deezer output already matches. Two real issues behind
that question, though.
Qobuz separates verses with an entry whose line is empty and which has no
start or end at all. Those were dropped entirely, so the plain lyrics tag came
out as one undifferentiated block. They are now kept as single blank lines,
collapsed so a run of separators cannot produce several blanks, and trimmed at
both ends so a tag never begins or ends with one. Checked against the live
track: 41 plain lines with 8 stanza breaks, where before it was 33 flat lines.
They are still excluded from the LRC, which is the correct behaviour rather
than an omission: a synced entry needs a timestamp, and these have none, so
emitting one would park a blank lyric at the wrong moment.
Separately, an empty line that *does* carry a timestamp is a different thing:
a display-clear marker. Qobuz does not appear to emit those, but other lyrics
sources do, and the parser now handles them properly by writing a bare
[mm:ss.xx] entry into the LRC and keeping it out of the plain text.
The LRC itself is unchanged for real data: 33 entries, every one timestamped.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two related changes: a 68-track Qobuz album was gated by the API throttle
rather than the network, and Qobuz lyrics are now supported, which is what the
faster album path had to be designed around.
--- perf ---
ratelimit: token bucket instead of fixed spacing
The limiter spaced every request exactly interval apart with no burst, so at
the default 60 requests/minute it allowed exactly one request per second no
matter how many workers ran. Measured: 8 requests took 7s with 1 worker and
7s with 6, i.e. max_connections had no effect on anything touching the API.
Since a track needs its metadata and stream URL before downloading, workers
could only start a track every two seconds, which is why an album looked
serial even at 57 MiB/s per track.
New() now takes a burst (providers pass max_connections), idle time accrues
budget up to that burst, and burst is clamped to one minute's worth of
requests so a large max_connections cannot bypass a small configured rate.
app: reuse the track objects already in the album payload
ripAlbum fetched the album, kept only the track ids, then asked the provider
for every track again, doubling the request count. Providers opt in via
AlbumTracksAreComplete(); only Qobuz does, because it is only verified there
and because Tidal does extra work in its track lookup.
Verified against the live Qobuz API on a 68-track release: the embedded track
objects already carry title, version, track_number, media_number,
parental_warning, isrc, copyright, performer, performers, composer, duration,
audio_info, maximum_bit_depth, maximum_sampling_rate and
release_date_original. The only field a per-track lookup adds that this app
reads is the nested album object, so albumTrackMetadata attaches the parent
album with its own track list stripped, or a 68-track album would hold 68
copies of itself. trackMetaUsable refetches anyway when title or track number
is missing, so a thin payload cannot produce a wrongly named file.
Measured end-to-end through the real app and limiter at the shipped defaults,
68 tracks: 137 API calls / 2m11s before, 69 calls / 1m3s after.
--- qobuz lyrics ---
Qobuz's main API does not serve lyrics: has_lyrics is only a flag, extra=lyrics
adds nothing, and track/getLyrics does not exist. Its apps use a two-step flow
instead, confirmed live:
GET track/lyricsUrl?track_id=&app_id=&request_ts=&request_sig=
-> {"track_id","album_id","lyrics_url"} (signed CloudFront URL)
GET <lyrics_url>
-> {"original":{"type","lang","lines":[{"line","start","end"}]},
"translation_langs":[...],"writers","publishers":[...]}
type is plain, lsync or wsync; start/end are milliseconds; the line text is
under "line". Passing a language returns a translation instead of the original,
and only for languages that exist for the track (en 403s on an English song),
so the language parameter is deliberately omitted. A 403 on the signed URL is
how Qobuz represents "no lyrics", so it is not treated as an error.
Lyrics are exposed through a TrackLyrics method rather than inside GetMetadata,
because album rips skip the per-track metadata call. That keeps the fast path:
with lyrics on, an album costs the same two provider calls per track it always
did, and the lyrics document itself comes from a CDN that does not consume the
provider's request budget. With metadata.exclude = ["lyrics"] it drops to one
call per track and the full speedup applies.
Existing behaviour is preserved: Tidal and Deezer already attach lyrics in
their metadata, so attachLyrics leaves them alone, and a lyrics failure is
logged and ignored rather than failing a download.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Duplicated helpers
cmd/rip had its own copies of six helpers that already existed in
internal/jsonutil: asString (StringFromAny), searchInt (IntFromAny),
nestedSearchInt and nestedSearchString (NestedAny/NestedString),
firstNonEmpty (FirstNonEmpty) and firstPositiveInt. firstPositiveInt was
duplicated a third and fourth time in provider/yandex and provider/beatport,
so it moves into jsonutil alongside the rest. jsonutil is back to 100%
coverage.
searchBool is deliberately left alone, with a comment saying why: it accepts
only a real bool, and switching it to jsonutil.BoolFromAny would start
coercing strings and numbers, changing how search results get labelled.
config auto-upgrade is no longer destructive
UpgradeOutdated re-serializes the whole config from the parsed struct, so any
key this version does not know about is dropped, and it runs automatically
from run() without prompting. It now copies the original to
<path>.bak-<unixtime> (0600, since it holds the same credentials) and
validates the result before writing. It returns the backup path so the CLI
can tell the user where it went. The comment states plainly that this is a
version stamp rather than a migration framework.
cmd/rip tests
The command bodies are testable now that they are functions returning exit
codes rather than arms of a switch that called os.Exit. Covers the usage
errors for every command, the quality floor on `rip id`, config path output,
the --version flag, and that applyQualityOverride reaches all six sources and
clamps only the two with a floor. cmd/rip coverage 17% -> 26%.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
internal/download
The ~40-line progress-bar decorator stack was copy-pasted into three download
paths and had started to drift; it is now newDownloadBar, which returns a nil
bar when progress is off (so callers use it unconditionally) plus a finish
func to defer. The three hand-rolled read/write loops that differed only in
whether they touched a bar become copyWithProgress.
FileDeezerEncrypted and FileYandexEncrypted were ~90% identical: mkdir, GET,
status check, create output, bar, cleanup-on-failure, Content-Length check,
fsync. That scaffolding is now streamEncrypted, and each function supplies
only its decryption step. Also switches the io.EOF/io.ErrUnexpectedEOF
comparisons to errors.Is.
internal/app
Three copies of "coerce a JSON array that may be []any or []map[string]any"
become anySlice, which also reports whether the value was an array at all so
callers can still distinguish missing from empty.
The worker-pool dispatch existed three times and the copies disagreed: two
counted failures under a mutex and logged inline with the track id, while the
third collected errors in a channel and logged them all after wg.Wait()
without the id. runTrackWorkers unifies them on the inline form, which
reports failures as they happen and names the track, and keeps the mutex held
across the log so failure lines cannot interleave.
Tests
The progress-bar paths had no coverage at all, because progress is disabled
when not attached to a terminal; four new tests force it on via the existing
STREAMRIP_GO_FORCE_PROGRESS escape hatch and cover the determinate bar, the
unknown-length spinner, Deezer decryption with a bar attached, and the
failure path still removing the partial file. internal/download coverage
53% -> 63%.
Three cancellation tests pin the Ctrl-C fix. Verified as genuine regression
tests by reverting the guards: without them the rip walks all 40 tracks of a
cancelled album, logs a failure for each, and Rip() returns nil.
Adds store.SQLite.IsFailed, the read complement to MarkFailed, used by the
bookkeeping test to confirm failure records still land under a cancelled
context.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Completes the restructuring started when main() became run() int. main.go goes
from 1217 lines to 130: global option handling, config loading, signal setup,
and a dispatch switch that delegates to one function per command.
The command bodies move to cmd_rip.go (url, file, id), cmd_search.go (search,
lastfm), cmd_config.go (config, database) and cmd_smoke.go (the developer smoke
commands), each taking (ctx, cfg, gopts, args) and returning an exit code.
They also stop reading the process global. Previously run() did
os.Args = append([]string{os.Args[0], gopts.command}, gopts.commandArgs...)
and the arms then indexed back into os.Args[2:], os.Args[5:] and so on, so the
already-parsed options were round-tripped through os.Args. The arms now take
the parsed gopts.commandArgs directly, which is a uniform index shift of two
(os.Args[5:] -> args[3:], len(os.Args) < 5 -> len(args) < 3).
No behaviour change. Verified by running each command's argument validation and
help path against the built binary, plus the existing suite.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The repo had no README, Makefile, CI, linter config or version string.
- README.md: requirements (ffmpeg/ffprobe, plus yt-dlp for SoundCloud), build,
the per-source credentials table, usage, the quality scale and the download
database's migration behaviour. Every claim was checked against the code
rather than written from memory, which is how the quality-range bug below was
found. It also records the two deferred refactors (no shared HTTP layer, and
metadata as map[string]any) so they are documented rather than lore, and
notes that there is no LICENSE file.
- Makefile: build, install, test, race, cover, fmt, fmt-check, vet, lint,
check, clean, help. `make build` stamps version/commit/date via -ldflags.
- .github/workflows/ci.yml: gofmt, vet, build, test and test -race, plus a
lint job. The linter is intentionally unpinned, with a comment: a
golangci-lint built against an older Go cannot analyse a go1.25 module, which
is the same reason a locally installed staticcheck built with go1.24 cannot
run here.
- .golangci.yml: standard set plus bodyclose, errorlint, nilerr and
govet/copylocks. copylocks matters now that config.Config carries a mutex;
bodyclose and errorlint target defects this codebase actually has.
- `rip --version` / `-V`, documented in the help output, falling back to the
VCS revision the toolchain embeds for `go install` builds.
Fixes a bug introduced in the previous commit: Validate capped deezer quality
at 2 and soundcloud at 1, but config.toml.example documents deezer as 2/3/4 and
the global -q flag accepts 0-4, so a documented config would have failed to
load. deezer's buildFormatPriority treats anything >= 2 as FLAC and soundcloud
ignores the value entirely, so the real ceiling is 4 everywhere; only the Qobuz
and Beatport floor of 1 is a genuine constraint. Pinned with a test that walks
the whole documented range.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The provider apps' own identifiers and protocol keys were scattered as
constants across six packages. They are not user secrets and stay baked in so
the tool still works out of the box, but they are now mirrored into the config
so that when a provider rotates one, a user can fix their install with a config
edit instead of waiting for a release.
Moved, each with the current value as the default in internal/config:
qobuz mobile_app_id, mobile_app_secret
tidal app_client_id, app_client_secret
deezer api_key, gateway_key, blowfish_secret
yandex sign_key, legacy_mp3_sign_salt
beatport app_client_id
applyVendorDefaults backfills any empty value, so configs written before these
keys existed keep working and the clients never have to nil-check them. Each
client reads through a small accessor; the package-level signing helpers
(mobileSignRequest, yandexDownloadSign, legacyDirectURL, deriveGatewayAuthToken
and friends, deriveDeezerBlowfishKey) now take the key as a parameter instead
of closing over a constant. The blowfish secret reaches internal/download
through a field on Downloader set by app.New.
The blowfish key derivation XORs 16 bytes of the secret, so a mistyped override
would have panicked; a wrong-length value is now rejected with a warning and
the built-in value is used. Tests pin the length and that the qobuz mobile
secret is valid hex, since the HKDF step requires it.
Also drops soundcloud.client_id and soundcloud.app_version: soundcloud goes
through yt-dlp, which supplies its own, so nothing read them.
config.toml.example documents each new key as "leave empty unless the vendor
rotates theirs".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Dead code
Removed, each verified to have no non-test caller: promptSearchSelection
(77 lines, superseded by promptSearchSelectionMenu), AddPlaylistByTrackIDs,
download.NewWithVerifySSL, qobuz.Client.GetTrackMetadata,
tidal.Client.trackSupportsAtmos (test-only, and the comment above
GetDownloadable already documented its removal from the live path) and its
now-orphaned test, yandex.canonicalTrackRequestID (a no-op returning its
trimmed input; the call site uses strings.TrimSpace directly) and the unused
defaultEstimatedKbps constant.
Config keys that did nothing
qobuz.download_booklets and misc.check_for_updates were declared, defaulted
and documented in config.toml.example but read by no code. The whole
[youtube] block went too: there is no YouTube provider. go-toml ignores
unknown keys, verified, so existing user configs still load.
Duplicated helpers
tidal.stringify and yandex.firstNonEmpty were byte-identical copies of
jsonutil.StringFromAny and jsonutil.FirstNonEmpty. displayArtistNames
existed three times, byte-identical, in internal/app, provider/yandex and
provider/beatport; it now lives in jsonutil. The CLI's normalizeCodec
delegates to config.NormalizeCodec so the flag and the config file are
checked by the same rules.
Tests for packages that had none
internal/jsonutil (0% -> 100%) and internal/netutil (0% -> 78%), the latter
covering redactURL, which is a security control, and the zero-timeout
contract that internal/download and the qobuz body copy depend on.
internal/config rose to 82% with coverage of the new Validate(), the
0600 permissions, CloneConfigData not sharing slices, and a concurrent
UpdateTokens test that exercises the mutex under -race.
Test suite is ~85x faster
Provider tests built clients with the default 60 requests/minute, so the
rate limiter slept 1s per HTTP call: deezer took 31s, tidal 11s, qobuz 8s.
Each provider test package now builds its config through a testConfigData
helper that disables throttling. Whole suite: ~60s -> 0.7s, and 2.8s
under -race.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Completes the correctness pass.
Download database migration no longer destroys dedupe state
The pre-source-column migration carried every legacy row over with
source='' and then dropped downloads_legacy, while IsDownloaded matched
source exactly. The result was that every previously recorded download
looked new, the user silently re-downloaded their whole library, and the
original rows were gone. Now the file is copied to <path>.bak-<unixtime>
first, downloads_legacy is kept, and IsDownloaded falls back to the
sourceless rows (over-matching across sources costs a skipped re-download,
which is the cheaper error). Schema state is tracked in PRAGMA user_version
so later migrations can be ordered explicitly. Databases already migrated by
the old code are detected on open.
Also enables WAL and a 10s busy_timeout and caps the pool at one connection,
so a concurrent reader no longer gets SQLITE_BUSY with no retry, and builds
the DSN through url.URL so a path containing '?' is not parsed as query
parameters.
--quality now applies to every source
applyGlobalConfigOverrides set only qobuz, tidal, deezer and soundcloud, so
`rip -q 4 url <beatport-url>` was a silent no-op even though
qualityForSource reads Yandex and Beatport. All six are set, with the
per-source floor applied and reported rather than silently ignored.
config: validation and a fixed reset
Load checked nothing but misc.version, so a negative max_connections or a
bogus conversion codec was accepted and only surfaced mid-rip. Adds
Validate() covering per-source quality ranges, max_connections,
requests_per_minute and truncate_to, and normalizes the conversion codec
from the file through the same path as --codec. `config reset` assigned one
value to both Config.File and Config.Session, so their Secrets slices shared
a backing array; it now takes separate copies.
Tests cover the shipped defaults and config.toml.example, which is how the
over-strict artwork max-width bound in the first draft was caught: -1 is the
documented "do not downscale" sentinel.
soundcloud: use the shared HTTP client
It was the only provider constructing its own http.Client, so it silently
ignored downloads.verify_ssl and max_connections and bypassed the transport
that redacts credentials from -vv logs. It also had no rate limiter at all.
qobuz: mobile fallback could not download a long track
http.Client.Timeout bounds the whole request including the body read, so the
30s API client aborted any track that took longer than 30s to stream. The
body copy now uses a no-timeout client, as internal/download already does.
qobuz: HKDF no longer fails silently
hexDecodeOrNil discarded its error, so a non-hex app secret keyed HKDF on
nil, derived a wrong KEK and produced silently corrupt audio instead of an
error.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Six correctness bugs that the existing tests could not see. The suite passed
and `go test -race` was clean throughout, because nothing exercised concurrent
token refresh, shared-folder artwork, or hostile metadata.
main(): extract run() int
main() was a single 1196-line function that registered 20
`defer mainApp.Close()` handlers and then called os.Exit 150 times, so none
of them ever ran. Close() is what closes the SQLite handle, flushes the
progress bars and removes the artwork temp dirs, so every failed rip leaked
temp directories. main() is now a wrapper around run() int and the command
bodies return exit codes.
Ctrl-C now stops a rip
ctx was threaded everywhere but never checked: zero ctx.Err()/ctx.Done()
in internal/app and internal/download. Cancellation left Rip() iterating
the whole remaining queue, and the retry ladder retried on a dead context,
so one Ctrl-C cost three doomed attempts per remaining track. Adds checks to
the dispatch loops, the worker-spawn loops and the retry ladder, and routes
the MarkFailed/MarkDownloaded bookkeeping through a detached context so the
records survive cancellation. Convert and TagFLAC now take a ctx and use
exec.CommandContext, so a stalled ffmpeg/ffprobe no longer outlives Ctrl-C.
Config race
Provider token refresh mutated the shared *config.Config and called
SaveFile() from inside download workers with no lock, racing the struct
fields and issuing concurrent os.WriteFile against one path. Adds a mutex to
config.Config plus UpdateTokens(), and routes all five refresh sites through
it. Each provider client gets an authMu guarding loggedIn and its
login/refresh path; beatport needed it most, since ensureToken is reached
from apiRequest.
Artwork cover race
artwork.Prepare was check-then-write on a path shared by every worker in a
playlist rip. Two workers both saw a missing cover.jpg, both created it, and
the loser's cleanup deleted the winner's finished file. Adds a per-folder
lock and downloads via a unique temp file + rename. The regression test
fails against the old code with 15 of 16 workers getting no cover.
Path traversal from remote metadata
naming.CleanName stripped <>:"/\|?* and control chars but not "." , so a
title of ".." survived into filepath.Join and escaped the download root.
SoundCloud and Tidal playlist names are user-generated. Now rejects "." and
"..", trims trailing dots, handles Windows device names, adds DEL to the
invalid set and caps components at 255 bytes without splitting a rune.
app.go gains safeJoin as a second line of defence.
Credentials no longer world-readable
The config holds plaintext provider passwords, Deezer ARL cookies and OAuth
tokens and was written 0o644. Now 0o600, with 0o700 directories, and an
existing 0o644 file is tightened on write.
Also: deletes the 9 duplicated *-rip-smoke dev commands (~300 lines fully
subsumed by `rip id`) and the unreachable second dev-help arm; registers
image/png so PNG covers are downscaled instead of being silently saved as
cover.jpg containing PNG bytes; adds the missing .gitignore, which was keeping
a 21MB build artifact one `git add -A` away from the repo.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- Extracted common JSON parsing helpers into internal/jsonutil
- Removed duplicated helper functions from provider packages
- Removed dead code in internal/app/app.go and downloader.go
- Replaced deprecated strings.Title with jsonutil.TitleCase
- Added graceful shutdown with signal handling in main.go
- Split monolithic cmd/rip/main.go into args.go, helpers.go, lastfm.go, search.go
JorenandClaude Opus 5