# Conventions Run checks from the repository root. On Windows, `check.bat` runs formatting, vet, lint when installed, and race tests. The equivalent individual commands are under [Formatting & CI](). Live playback needs the separate [manual checks](). For a first change: 1. Read the [architecture]() section for the component you will change. 2. Check its [ownership contract]() and the relevant rules below. 3. Edit canonical files. For the README, edit `README.md.tmpl` and regenerate with `go run ./cmd/readme`; do not edit the generated output. 4. Run focused tests, then the project checks. Verify an affected playback path in a test server before claiming live behavior works. [Extension steps](), [testing](), [release notes]() and [documentation]() are lookup sections. ## How to read this Rules distinguish automated enforcement from runtime invariants and project practices. - **[enforced: ]** — a check fails, and `` says which one. `internal/conventions` is an ordinary Go test, so `go test ./...` and CI run it with everything else; `.golangci.yml`, `gofmt` and the race detector cover the tags they own. `TestDocumentAndChecksAgree` keeps these tags and those checks in step, so this file cannot advertise enforcement that does not run, and a check cannot enforce something this file never mentions. The wording of each enforced rule is read back out of this document for the failure message, so the sentence you are reading is the one the build quotes at you. - **[invariant]** — nothing checks it, and violating it breaks something nameable, often silently and often for users. These are the expensive ones. Each states the failure it prevents, so the cost is on the page rather than in someone's memory. - **[practice]** — how things are done here. Nothing breaks if you deviate; the codebase just gets less coherent. Deliberately few. If a rule gets in the way of something that genuinely needs doing, pragmatism wins — take the exception and leave a note in the code explaining it. `parsers/ytnative/chunked.go`'s `fetchChunk` is the worked example: it retries inside a parser, which the design principles forbid, and says in place why the usual answer is wrong there. **This file is executable.** `internal/conventions` reads it while the tests run, so editing it is a code change, not a documentation change. Reword a paragraph tagged `[enforced: x]` and you have reworded a build failure message. Remove the tag, rename it, or move this file, and the build goes red until the checks are updated to match — deliberately, because a rule that quietly stops being enforced is exactly the failure this whole arrangement exists to prevent. Prose under `[invariant]` and `[practice]` is free to change; nothing reads it. **Adding a rule.** Give it a tier. If it would be [enforced], write the check in `internal/conventions/conventions_test.go` first, let it fail, then fix every violation — a rule arrives at zero or it does not arrive. If it would be [invariant], name the failure in the rule itself. If it is neither, do not add it. ## Design principles **[enforced: discord-free]** `pkg/music` never imports Discord. The CLI is the proof it holds — same engine, no Discord in sight. Anything Discord-specific belongs in `internal/`. Checked by `TestLibraryStaysDiscordFree`. **[enforced: adapter-boundary]** Only the Discord transport names a Discord client library: the `internal/discord` root, `audit`, `perm`, `reply`, `session`, `slashsync` and `voice/voicesink`, listed package by package. Everything else — commands, middleware, readme generation, `cmd` — reaches Discord through `adapter`'s neutral types, so swapping the library is the transport's problem rather than the whole tree's. Checked by `TestDiscordStaysBehindTheAdapter`. **[enforced: frontend-boundary]** The CLI (`internal/cli`) and the bot (`internal/discord`) are equal frontends over the shared `internal/music` layer. Neither imports the other, and nothing outside `cmd` and `internal/readme` imports either: shared code that leans on one frontend has stopped being shared. Checked by `TestFrontendsStayApart`. **[practice]** Go stays minimal here. No frameworks, no speculative abstraction — an interface only exists if it has two real implementations or a real test seam behind it. Everything else stays concrete. **[practice]** New capability shows up as an implementation of one of the three extension points rather than as a new layer: `sources.Source` (input → metadata), `parsers.Streamer` (track → Opus packets), and `sink.AudioSink`/`Provider` (Opus packets → audio). `sources.Searcher` is an optional extra a source may also implement, and is deliberately not folded into `Source`: radio has nothing to rank, and resolving a query to one track is a different job from listing candidates. The whole engine speaks 20ms Opus packets (`opus.Reader`) end to end. See [architecture.md](). **[invariant]** Parsers are expendable by design. They fail fast with a clear error, and `RecoveryStream`'s fallback chain is what provides reliability. Don't add retry logic inside a parser: recovery already covers it from outside, and a parser that retries turns one failed source into a stall the chain cannot route around. The two deliberate exceptions both live in `ytnative` and both say so in place. **[invariant]** No signature deciphering, ever. If a platform requires it, the track falls through to kkdai or yt-dlp instead. That boundary is what keeps this project out of the cat-and-mouse game that eats the maintenance budget of every tool that took the other road. ## Naming **[practice]** The playback entity is `parsers.Track`; whatever the resolver produces is `sources.TrackInfo`. Please don't add a third track-shaped type. **[practice]** Scripted edits over Go source fail in specific ways here, and all of them are cheap only because they happened to be compile errors. One type name can be a prefix of another (`MessageEmbed` of `MessageEmbedField`), so a plain string replacement rewrites the wrong one; a transform run over a whole tree will add a package's own import to a file inside it; and a replacement aimed at code will reach inside a string literal, which is the one that produces no error at all — `/commands status` shipped telling users commands are grouped "(ctx.Event.g., purge, core, translate)". Read what a scripted edit touched, especially where it touched text. **[practice]** Package names are single lowercase words describing what the package does (`reply`, `perm`, `watchdog`). A package that just wraps one dependency can be named after it (`kkdai`, in the `goja` style) — that's being honest about what it is, not laziness. **[practice]** Strings that select behavior get a named type plus constants (see `player.TransportRecoveryMode`); identifier strings get constants too (`sources.Parser*`, `sources.YouTube`). ## Frozen identifiers **[enforced: frozen-identifiers]** Parser registry keys (`ytnative-link`, `kkdai-pipe`, and so on) and source names (`youtube`, etc.) are persisted in guild playback history and registered as slash-command choices, so they cannot be renamed later — add new ones instead of touching existing ones. The constants live in `pkg/music/sources/parsers.go` and `sources.go`; the registry mapping lives in `pkg/music/stream/stream.go`. `TestFrozenIdentifiers` pins every value. **[enforced: frozen-identifiers]** `/search`'s button ids are frozen for a different reason but just as hard. The format is `search::`, and a chooser that has already been posted keeps sitting in a channel: its ids come back whenever someone presses a button, possibly long after a restart or a deploy. So the source tags (`yt`, `sc` in `internal/discord/command/music/search`) can gain new values but must never be renamed or re-pointed. Pinned by the same test. **[invariant]** An unrecognised tag has to fail closed — telling the user to run `/search` again — rather than resolve as some default source. A tag that silently resolves elsewhere plays the wrong track from a button the user pressed in good faith. ## Concurrency contracts The full set — who owns what, and what enforces each rule rather than merely stating it — is [ownership.md](). What follows is the part that is about style rather than about ownership, and is not repeated there: two documents restating the same rule is how one of them ends up wrong. **[invariant]** `Player.PlayerStatus` has exactly one long-lived consumer per player — the `Hooks.Watch` a frontend hands `music.Service`, which starts it once per player: the voice service's `WatchPlayerStatus`, or the CLI's printer. Don't attach per-interaction listeners: competing receivers steal events from each other, so a status update simply goes missing rather than failing loudly. Interaction outcomes get rendered synchronously by the handler that already knows the result. **[invariant]** Callback fields on `Player` are set once, at construction, via `player.Options`, and never touched again. They are read from the playback goroutine without a lock, which is only safe because of that. **[invariant]** Every goroutine has an owner and a clear way to exit. A playback run is a `*run` holding its own stop and done channels, track and flags; the goroutines serving it are handed that run rather than reading one off the player — otherwise a goroutine scheduled late acts on a newer run's track. See [ownership.md]() rule 5. **[practice]** Package-level loggers use `atomic.Pointer[zerolog.Logger]` with `SetLogger` and a `Nop` fallback (see `parsers/ffmpeg/pcm.go`), wired once in `internal/discord/session_bootstrap.go`. ## Errors and logging **[enforced: error-prefix]** Errors in `pkg/music` carry the package name as a prefix, like `ytnative: player request: …`. Exported sentinels are exempt: their text is also the string a user is shown, and `ytnative: no tracks in queue` is not a sentence anyone wants in a Discord embed. The rule covers wrapped and internal errors — everything that exists to be read in a log. **[invariant]** Sentinel errors (`ErrCipherOnly`, `ErrNoTracksInQueue`) are exported and matched with `errors.Is`. Never pattern-match the error text: it is display copy, it gets reworded, and a match against it fails silently when that happens. `errorlint` rejects the `==` comparison and the `%v` wrap that would break `errors.Is`; matching on the text is left to review. **[enforced: log-event-naming]** Log events are lowercase, snake_case, verb-last (`playback_running`, `stream_open_failed`), with structured fields rather than interpolated strings. `Msgf` is a violation for the same reason — an interpolated message cannot be grepped or aggregated. **[practice]** External process stderr goes through `ffmpeg.NewPCMCommand`'s classifier rather than straight to the process's own stderr. ## Comments A comment earns its place by saying something the code cannot. The code already says what it happens to do; a restatement is just a second thing to keep in sync, and it sits in the way of the comment that actually matters. `// Create embed` above a struct literal called `embed` is the shape to avoid, and so is a doc comment that only expands the identifier back into a sentence. The comments worth writing answer a question the code raises but cannot settle. Why VISIONOS rather than the library's default client. Why the read-ahead buffer sits above recovery instead of under it. Why a stream copies the Track it is given instead of writing the caller's. Whoever asks those next — a maintainer months from now, or an agent told to "clean this up" — cannot recover the answer from the code, and will helpfully undo it. **[invariant]** Four things make such a comment hold up: **Say whether it was measured or assumed.** "Verified against the live CDN, not inferred", or "measured on one live broadcast, the renditions ran 269, 507, 962, 1282, 2922 and 5552 kbit/s", is worth more than any amount of confident prose: it tells a reader which claims they may reason from and which they should re-check. A guess is fine to write down — label it as one. **Name the failure it prevents.** "which killed every playback on its first read" turns an arbitrary-looking constant into something nobody deletes by accident. A rule with no consequence attached reads as a preference. **Say what not to do.** `don't reach for it when a 403 turns up`, `do not switch to CBR`, `do NOT advance parserIndex`. A deliberate non-obvious choice needs a fence around it or it gets optimized away — this is the highest-value kind of comment here, and the one an agent is likeliest to violate in its absence. **Point at the next hop by name.** `see ytnative/visitor.go`, `see VisionOSClient`, `see the note in SetCommand`. A reader who needs more should be told where it is, in a form that greps. **[invariant] One fact, one home.** The four qualities above say what makes a comment worth writing; they do not license writing it four times. A fact told in two places will disagree with itself within a release, and the disagreement is silent — both copies read as authoritative. When two comments would explain the same decision, the one at the point of decision keeps it and the other points there by name. This is the rule that bites hardest right after a design is worked out, when the reasoning feels worth repeating everywhere it touches: that is exactly when to write it once. There is no threshold on how much a file may explain, and there should not be — `resume.go` and `chunked.go` are mostly prose because the decisions behind them are genuinely unobvious. What the scorecard in `internal/conventions` prints is comment density per file, not as a gate but so drift is visible. Read the top of that list occasionally and ask whether those files are explaining hard things or repeating themselves. Long explanations belong in one prose block at the top of the file or above the declaration they explain — `parsers/ytnative/resume.go`, `parsers/ytnative/chunked.go`, `parsers/ytdlp/runtime.go` and `sources/youtube/playlist.go` are the pattern — rather than sprinkled line by line through the body. Struct fields carrying a contract get their own comment, and concurrency-relevant ones say who owns the field and what lock covers it ("belongs to the reading goroutine alone", "never held across a read or an HTTP round trip"). **[practice]** Every exported identifier gets a doc comment starting with its own name, per Go convention — except methods implementing an interface, which inherit the interface's doc and should not repeat it. `revive`'s exported-docs linter is deliberately off: `pkg/music` is documented by hand and `internal/` favors self-explanatory names over comment ceremony. **[enforced: comment-width]** Comments wrap at 80 columns and are written in English. A tab counts as one column, and a line carrying an unbreakable token (a URL, a long identifier) is exempt. **[enforced: file-headers]** No file-path headers (`// FILE: internal/…`). Nothing checks them, so they survive every rename — server-domme carried one naming `melodix/internal/discord/middleware/command_logger.go`, a path that has never existed in either project, and it had been wrong since the commit that introduced it. **[invariant]** Two more things go stale silently, so they don't get written at all: a claim about what does not exist yet ("only YouTube is offered today"), which still reads as fact long after it stopped being one — describe what the design allows instead; and any restatement of a constant's value, which the constant already carries. A comment that has drifted from the code is worse than no comment, because it is believed. ## Adding things To add a **source** (metadata only, reusing existing parsers): 1. New package under `pkg/music/sources//`, implementing `Source`. 2. Add the name constant to `pkg/music/sources/sources.go`. 3. Register it in `resolve.New()`, and add it to the auto-detect precedence list in `Resolver.Resolve` — this is deliberately explicit, not automatic. 4. If it's searchable by bare query, extend the query branch in the resolver. 5. Add it to `/play`'s `source` choices. To make a source pickable in `/search`, implement the optional `sources.Searcher` and add a case to the command's `pick` and `trackURL`. The `SearchResult` id has to be the source's own compact identifier rather than a URL — it round-trips through a Discord component id, and those are a frozen wire format; see [Frozen identifiers](). To add a **parser** (a new playback backend), implement `Streamer.Open` returning an `opus.Reader`: 1. New package under `pkg/music/parsers//`. If the source is a native Opus container, use `opus.Demux` on the HTTP body for passthrough; otherwise build ffmpeg with `ffmpeg.NewPCMCommand` and wrap it in `ffmpeg.OpusReader`, which encodes PCM to Opus packets. A parser that supports both link and pipe modes carries a `Mode` field on its streamer. 2. Add the instance to `stream.registryEntries` (`pkg/music/stream/stream.go`), under its frozen key constant from `pkg/music/sources/parsers.go`. 3. List it in the owning source's `AvailableParsers()` and in `/play`'s `parser` choices. The player, queue, recovery, sinks and persistence use the shared interfaces. Check the owning command's choices and documentation when adding an entry. **[enforced: frozen-identifiers]** A new parser key or source name is a frozen identifier from the moment it ships, and the check derives the set from the source rather than a list: add an exported string constant to a frozen package and the build fails until it is pinned in `frozenPackages`. That is deliberate friction — it forces the question "is this string persisted?" at the moment the answer is still cheap. **[invariant]** A `SearchResult` `ID` is the source's own compact identifier, never a URL. It has to survive a round trip through a Discord component id, which caps at 100 characters, and SoundCloud permalinks routinely exceed the budget that leaves. `trackURL` is what turns that id back into a page URL. **[invariant]** A parser that talks to a live endpoint gets an opt-in live test (`MELODIX_LIVE_TESTS=1`) so upstream drift is caught early, and an entry-point test that does not need the network — see below for why both. ## Testing & verification **[enforced: race]** `go test -race ./...` is the bar to clear; the race detector isn't optional for anything that touches `Player`. CI runs it on every push. **[practice]** A green build proves only what is tested, and a type assertion proves nothing at all until it runs. The disgo migration changed an interface and left one implementation behind; the assertion between them stopped matching and every command silently resolved to no slash definition for six commits, which would have deleted every guild's commands on the next sync. Where a type switch or an assertion carries load, a compile-time `var _ Iface = (*T)(nil)` costs one line and turns the next drift into a build failure. Where behaviour depends on the assertion matching, test that it does. **[practice]** A check that has never failed has not been shown to work. Make a new test fail against the bug it is for before leaving it passing, and the same for any rule added to `internal/conventions` — a tag in the document plus a check that cannot fire buys the full appearance of enforcement with nothing behind it. **[practice]** Prefer fakes over mocks: swap the registry via `stream.SetRegistry`, stub `sink.Provider`, and use httptest for HTTP clients. Endpoint and tuning values are `var` rather than `const` specifically so tests can point them somewhere else (`playerEndpoint`, `homeEndpoint`, `chunkSize`). **[invariant]** Live-endpoint behavior only gets opt-in `Live` tests, never unconditional ones. A network call in the default suite makes CI fail for reasons that have nothing to do with the commit, and the usual response is to stop trusting the suite. **[invariant]** Test the entry point, not only the helpers underneath it. A parser's `Open` is the thing the engine actually calls, so it needs its own coverage even when every piece it composes is already tested — a check sitting between the helpers and the caller otherwise belongs to no test at all. That is not hypothetical: a live-stream gate in `ytnative`'s `Open` rejected every playable YouTube video for a whole release while both the unit tests and the opt-in live tests stayed green, because all of them called `fetchPlayer` and `pickOpusFormat` directly and nothing called `Open`. **[invariant]** A regression test has to be watched failing before it is trusted. Reintroduce the bug, confirm the new test goes red, then take it back out. A test that passes for a reason other than the one you intended is the normal outcome, not the rare one: `chunked.go`'s cache-corruption guard was written, reviewed, and green — against a mutation of the exact bug it existed to catch — because it exercised a neighbouring code path. Only the mutation found that. **[practice]** Before cutting a release, run the manual checks below. ### Manual verification - Manual smoke checklist, which needs a real guild: a `/play` multi-track batch, checking the status message updates on every auto-advance; `/play` while already playing, which should give "Track(s) Added"; `/next`; `/stop` mid-track, which should return promptly; a natural queue end, which should give a single voice disconnect and "Playback Finished"; and one `/play` per parser override. Also check: - a `/search` pick, which should post a public Now Playing that everyone in the channel sees and that later tracks edit, not the ephemeral chooser; - `/next` while a status message is live, which should mark the old one "⏭ Skipped" and post a new one, leaving the old one off the new track; - a second listener joining the voice channel mid-track, who should hear it — the DAVE session has to take in the new member; - an error after a public defer (`/play` from outside a voice channel, say), which only the caller should see; - a button on a `/search` chooser after `/settings` has disabled the music group, which should be refused. - `cmd/cli` exercises everything but Discord: `go run ./cmd/cli`, then `play `, `search `, `next`, `queue`, `history`, `stop`. ## Formatting & CI **[enforced: golangci]** Code is gofmt-clean and `go vet`-clean, and `.golangci.yml` passes with zero findings — the set is kept deliberately curated so a finding always means something. `exhaustive` is in it so a new player status or mode has to be placed in every switch over that type, not silently fall through them. `internal/conventions` runs as part of the same `go test ./...`. The enforced checks are absolute: a violation anywhere fails the build. They used to ratchet against a recorded baseline, so that a rule could be adopted on a codebase that already broke it and only fail on files that got *worse*. The debt was burned down in one pass (263 violations) and every rule has held at zero since, so the baseline, the update mode, the guard stopping CI rewriting it and the scorecard were all bookkeeping for a balance of nothing — and have been removed. `git log` has them if a rule ever has to be adopted against real debt again; `golangci-lint run --new-from-merge-base=origin/main` does the same job line-exactly and off the shelf. On Windows, `conventions.bat` runs the checks. `check.bat` runs the whole gate — gofmt, vet, lint, race tests — which is what CI does. **[practice]** CI (`.github/workflows/build.yml`) runs vet, race tests, and lint on every push and PR, then cross-compiles all release targets. **[invariant]** `README.md` is generated, not hand-edited: change `README.md.tmpl` and run `go run ./cmd/readme` from the repo root. Editing the output means losing the edit on the next regeneration. The docs under `docs/` and `pkg/music/README.md` are hand-written. The bot itself never writes files at runtime. ## Release notes **[practice]** Release notes are read by people deciding whether to run this bot, not by whoever fixed the bug. Lead with what changed *for them*, in plain language — "YouTube plays again, and now without ffmpeg" — and say whether upgrading costs them anything: config changes, a migration, a new dependency. **[practice]** The body stays under 200 words: a bold one-line headline, at most two short paragraphs, then the upgrade cost. That is a budget, not an aspiration. The failure mode is a post-mortem written for the author, and it arrives by addition — every sentence is defensible on its own, and the essay is what they add up to. A release carrying three user-facing changes gets three headlines with a short paragraph each, not three essays. **[practice]** Root-cause detail does not go on the release page at all — not at the top, and not folded into a `
` block at the end either. It lives in the commit messages, where the next maintainer actually looks. Parser keys, InnerTube client names, HTTP header syntax and internal log-event names mean nothing to someone choosing a music bot. Neither does how the bug was found, what it cost some other project, or why the previous approach was wrong. Commit messages are the opposite case and stay as technical as they need to be, which is what makes leaving the detail out here cost nothing. **[invariant]** The release body comes from the **annotated tag's message body** — `.github/workflows/release.yml` reads `%(contents:body)` — so that is where the notes get written: ```bash git tag -a --cleanup=verbatim vYYYY.MM.DD -F notes.md git for-each-ref refs/tags/vYYYY.MM.DD --format='%(contents:body)' # read it back ``` `--cleanup=verbatim` is not optional. Git's default cleanup strips every line beginning with `#` as a comment, which silently eats Markdown headings and publishes a release with its structure missing. Read the body back before pushing the tag: that second command is the entire check, and nothing else catches a forgotten flag, because the tag is created successfully either way and nothing complains until the release page is already public. The tag's subject line is not part of the release body either, so nothing load-bearing goes there. ## Documentation Each document owns a reader's task: the README introduces the bot; [running.md]() owns setup and configuration; [playback.md]() owns routine use, and [troubleshooting.md]() owns symptom-led diagnosis; [architecture.md]() explains components and flows; this file owns contributor workflows. [ownership.md]() is the specialist reference for lifecycle contracts. Docker and library setup stay beside their artifacts and are linked from the README. Historical evidence belongs in [internal/implementation-notes.md](). **[practice]** Give each fact one canonical home and link to it elsewhere. Keep public guides reachable from the README and [task index](). Check claims against source, including defaults, supported flags and the actual scope of a test. Preserve configuration-table and convention-tag contracts when editing documentation. Run the relevant existing checks; their existence does not verify every prose claim or demonstrate a successful live Discord session. The [documentation tooling guide]() explains site builds, publication boundaries and offline bundle selection. Public articles are listed in `_data/docs.json`; internal plans and operational skills stay outside the site. The reusable [documentation pattern]() is versioned in this repository. Apply it when restructuring docs or adding a new guide; adapt specialist documents to the project rather than copying a fixed file list.