From bb112009a1bc69d7865965d7a507412fa1e18b0c Mon Sep 17 00:00:00 2001 From: dan Date: Thu, 22 Jan 2026 01:43:14 -0800 Subject: [PATCH] Address review issues and update docs --- README.md | 12 +++-- WORKLOG.md | 17 ++++++- cmd/smoketest/main.go | 61 +++++++++++++++++++++++- config.example.toml | 2 +- docs/code-review-plan.md | 60 +++++++++++++++-------- docs/design-matrix-native-routing.md | 52 ++++++++------------ docs/reviews/cli-entrypoints.md | 18 +++---- docs/reviews/config-packaging.md | 30 ++++++++++++ docs/reviews/docs.md | 27 +++++------ docs/reviews/issues.md | 51 ++++++++++++++------ docs/reviews/matrixbot.md | 27 +++++++++++ docs/reviews/message-handling.md | 38 ++++++--------- docs/reviews/ops.md | 13 +++++ docs/work/2026-01-22-code-review-plan.md | 20 ++++++++ docs/work/2026-01-22-tech-debt.md | 18 +++++++ flake.nix | 2 +- internal/detector/detector.go | 44 +++++++++++------ internal/matrixbot/bot.go | 23 ++++++++- internal/matrixbot/state_store.go | 13 +++++ internal/resolver/resolver.go | 3 ++ internal/services/idonthavespotify.go | 2 + pkg/config/config.go | 22 +++++++++ 22 files changed, 412 insertions(+), 143 deletions(-) create mode 100644 docs/reviews/config-packaging.md create mode 100644 docs/reviews/matrixbot.md create mode 100644 docs/reviews/ops.md create mode 100644 docs/work/2026-01-22-code-review-plan.md create mode 100644 docs/work/2026-01-22-tech-debt.md diff --git a/README.md b/README.md index c1c970a..efab10e 100644 --- a/README.md +++ b/README.md @@ -14,7 +14,7 @@ MusicLink is a Matrix-native bot that connects directly to a Matrix homeserver a ### Prerequisites -* Go 1.22 or higher +* Go 1.24 or higher * A Matrix homeserver and bot access token ### Building @@ -34,7 +34,7 @@ go build -o musiclink ./cmd/musiclink ```toml [matrix] shadow = false # set true to log-only during validation - healthAddr = ":8080" # optional health/metrics endpoint + healthAddr = "127.0.0.1:8080" # optional health/metrics endpoint server = "https://matrix.example.com" accessToken = "your-matrix-access-token" userId = "@musiclink:example.com" @@ -42,6 +42,8 @@ go build -o musiclink ./cmd/musiclink stateStorePath = "data/matrix-state.db" ``` + Note: MusicLink reads configuration from the TOML file only. For production, prefer generating the config from a secrets manager or Nix module to avoid environment-variable leaks. + ### Running ```bash @@ -50,10 +52,10 @@ go build -o musiclink ./cmd/musiclink ## Health and Metrics (Matrix-native) -If `matrix.healthAddr` is set, MusicLink exposes JSON health stats at `/healthz` (and `/metrics`). Example: +If `matrix.healthAddr` is set, MusicLink exposes JSON health stats at `/healthz` (and `/metrics`). The address must bind to localhost (e.g., `127.0.0.1:8080`). Example: ```bash -curl http://localhost:8080/healthz +curl http://127.0.0.1:8080/healthz ``` ## Testing @@ -72,7 +74,7 @@ No Matrix integration tests are included yet; add them as needed. ### Smoke Test -A manual smoke test script is available in `cmd/smoketest`: +A manual smoke test script is available in `cmd/smoketest` (requires network access to the idonthavespotify API): ```bash go run cmd/smoketest/main.go diff --git a/WORKLOG.md b/WORKLOG.md index c10cc7b..8d63db0 100644 --- a/WORKLOG.md +++ b/WORKLOG.md @@ -1,6 +1,21 @@ # MusicLink Worklog -## 2026-01-20 +## 2026-01-22 + +### Current Status +- **Backend:** Go (v1.24.0) Matrix-native bot. +- **Deployment:** NixOS based (flake.nix), managed on `ops-jrz1`. +- **Notes:** Matterbridge has been deprecated and removed from the runtime. + +### Recent Changes +- Matrix-native routing implemented with allowlisted rooms. +- Link preview suppression via `com.beeper.linkpreviews = []`. +- Health endpoint bound to localhost (`matrix.healthAddr`). + +### Next Steps +- Keep worklog up to date with operational changes. + +## 2026-01-20 (Historical) ### Current Status - **Backend:** Go (v1.22.8) bot using Matterbridge WebSocket API. diff --git a/cmd/smoketest/main.go b/cmd/smoketest/main.go index 5549834..bae0b16 100644 --- a/cmd/smoketest/main.go +++ b/cmd/smoketest/main.go @@ -64,7 +64,7 @@ func testAPI() { testURL := "https://open.spotify.com/track/4iV5W9uYEdYUVa79Axb7Rh" fmt.Printf(" Calling API with: %s\n", testURL) - resp, err := client.Resolve(ctx, testURL) + resp, err := resolveWithRetry(ctx, client, testURL) if err != nil { fmt.Printf(" FAIL: API error: %v\n", err) os.Exit(1) @@ -90,7 +90,7 @@ func testResolver() { testURL := "https://open.spotify.com/track/4iV5W9uYEdYUVa79Axb7Rh" fmt.Printf(" Resolving: %s\n", testURL) - resolved, err := res.Resolve(ctx, testURL) + resolved, err := resolveResolverWithRetry(ctx, res, testURL) if err != nil { fmt.Printf(" FAIL: Resolver error: %v\n", err) os.Exit(1) @@ -133,3 +133,60 @@ func splitLines(s string) []string { } return lines } + +func resolveWithRetry(ctx context.Context, client *services.Client, url string) (*services.APIResponse, error) { + const maxAttempts = 3 + backoff := time.Second + var lastErr error + + for attempt := 1; attempt <= maxAttempts; attempt++ { + resp, err := client.Resolve(ctx, url) + if err == nil { + return resp, nil + } + lastErr = err + if attempt == maxAttempts { + break + } + if !sleepWithContext(ctx, backoff) { + return nil, ctx.Err() + } + backoff *= 2 + } + + return nil, lastErr +} + +func resolveResolverWithRetry(ctx context.Context, res *resolver.Resolver, url string) (*services.ResolvedLinks, error) { + const maxAttempts = 3 + backoff := time.Second + var lastErr error + + for attempt := 1; attempt <= maxAttempts; attempt++ { + resp, err := res.Resolve(ctx, url) + if err == nil { + return resp, nil + } + lastErr = err + if attempt == maxAttempts { + break + } + if !sleepWithContext(ctx, backoff) { + return nil, ctx.Err() + } + backoff *= 2 + } + + return nil, lastErr +} + +func sleepWithContext(ctx context.Context, delay time.Duration) bool { + timer := time.NewTimer(delay) + defer timer.Stop() + select { + case <-ctx.Done(): + return false + case <-timer.C: + return true + } +} diff --git a/config.example.toml b/config.example.toml index 49a16c8..5f0be17 100644 --- a/config.example.toml +++ b/config.example.toml @@ -7,7 +7,7 @@ # Shadow mode (log responses without sending) shadow = false -# Optional health server address (ex: ":8080") +# Optional health server address (must bind localhost, e.g., "127.0.0.1:8080") healthAddr = "" # Matrix homeserver base URL diff --git a/docs/code-review-plan.md b/docs/code-review-plan.md index f05d5eb..48fb2e8 100644 --- a/docs/code-review-plan.md +++ b/docs/code-review-plan.md @@ -1,29 +1,47 @@ -# Code Review Plan (6-Part Pass) +# Code Review Plan (6-Part Pass + Code-Review Skill) -## Scope -Review the repository in six focused passes to cover entrypoints, transport layers, core logic, config, docs, and infrastructure. +## Goal +Review the repository in six focused passes. For each pass, run the `code-review` skill on the scoped paths, then capture findings and consolidate issues into a single issues markdown file. -## Plan -1. **CLI entrypoints** +## Scope Chunks +1) **CLI entrypoints** - Paths: `cmd/musiclink/`, `cmd/smoketest/` - - Focus: startup flow, flags, shutdown handling, error propagation. -2. **Bot transport layers** - - Paths: `internal/bot/`, `internal/matrixbot/` - - Focus: protocol handling, reconnection, rate limits, threading/room routing, dedupe, and resource cleanup. +2) **Matrix bot runtime** + - Paths: `internal/matrixbot/` -3. **Message handling & link detection** - - Paths: `internal/detector/`, `internal/resolver/`, `internal/services/` - - Focus: parsing correctness, error handling, API usage, formatting logic. +3) **Message handling & link logic** + - Paths: `internal/handler/`, `internal/detector/`, `internal/resolver/`, `internal/services/` -4. **Config & runtime wiring** - - Paths: `pkg/config/`, `config.example.toml`, `config.toml` - - Focus: validation, defaults, secrets handling, backwards compatibility. +4) **Config & packaging** + - Paths: `pkg/config/`, `config.example.toml`, `flake.nix`, `go.mod`, `go.sum`, `vendor/` -5. **Docs & design artifacts** - - Paths: `docs/`, `README.md`, `WORKLOG.md` - - Focus: accuracy vs implementation, user-facing setup guidance. +5) **Docs & design artifacts** + - Paths: `README.md`, `docs/` -6. **Project/infra metadata** - - Paths: `go.mod`, `go.sum`, `flake.nix`, `flake.lock`, `LICENSE` - - Focus: dependency hygiene, tooling assumptions, licensing. +6) **Ops/integration references** + - Paths: `WORKLOG.md` + +## Process (per chunk) +1. Run code review: + ``` + /code-review + ``` +2. Record the review output in a notes file (e.g., `docs/reviews/.md`). +3. Add any confirmed findings to the consolidated issues file: + - `docs/reviews/issues.md` + +## Output Files +- Per-chunk review notes: + - `docs/reviews/cli-entrypoints.md` + - `docs/reviews/matrixbot.md` + - `docs/reviews/message-handling.md` + - `docs/reviews/config-packaging.md` + - `docs/reviews/docs.md` + - `docs/reviews/ops.md` + +- Consolidated issues: + - `docs/reviews/issues.md` + +## Issue Filing +After reviewing all chunks, summarize issues and ask for approval before filing. diff --git a/docs/design-matrix-native-routing.md b/docs/design-matrix-native-routing.md index c6c6d2f..15a3390 100644 --- a/docs/design-matrix-native-routing.md +++ b/docs/design-matrix-native-routing.md @@ -1,13 +1,13 @@ # Design: Matrix-Native MusicLink Routing ## Goal -Make MusicLink a native Matrix bot that listens to multiple rooms and replies in the **same room** that originated the message. This removes Matterbridge from the routing path and eliminates cross-room fan-out. +Make MusicLink a native Matrix bot that listens to multiple rooms and posts follow-up messages in the **same room** that originated the message. This removes Matterbridge from the routing path and eliminates cross-room fan-out. ## Background -Current deployment uses Matterbridge as an API gateway. When multiple Matrix rooms are configured in a single Matterbridge gateway, messages fan out to other rooms. This causes unintended cross-posting (including DM leakage) when MusicLink is enabled in more than one room. +Legacy deployment used Matterbridge as an API gateway. When multiple Matrix rooms were configured in a single Matterbridge gateway, messages fanned out to other rooms, causing unintended cross-posting (including DM leakage). The system is now Matrix-native. ## Objectives -- **Correct routing:** Replies must go back to the originating room (and thread when applicable). +- **Correct routing:** Responses go back to the originating room (no cross-room fan-out). - **Multi-room support:** One MusicLink instance can monitor multiple Matrix rooms. - **No fan-out bus:** Remove Matterbridge dependency for routing. - **Minimal operational complexity:** Single service, single config, single token. @@ -22,21 +22,20 @@ Current deployment uses Matterbridge as an API gateway. When multiple Matrix roo Slack Room -> mautrix-slack (Matrix portal room) -> MusicLink (Matrix-native bot) - -> same Matrix portal room (reply) - -> mautrix-slack -> Slack thread + -> same Matrix portal room (follow-up message) + -> mautrix-slack -> Slack ``` -## Proposed Implementation +## Current Implementation ### 1) Matrix Client -Use a Matrix SDK (mautrix-go or matrix-nio) to: -- Login using a bot token (or access token from config). -- Sync events from configured rooms. -- Ignore messages from the bot itself. -- Post replies to the same room. +- Uses mautrix-go with a bot access token. +- Syncs events from configured rooms. +- Ignores messages from the bot itself. +- Posts responses as standalone messages in the same room (no reply/thread relation). ### 2) Room Configuration -Extend config with explicit room allowlist: +Matrix allowlist in config: ```toml [matrix] @@ -49,33 +48,20 @@ rooms = [ ] ``` -### 3) Threading Support -If the incoming event references a thread (e.g., `m.relates_to` with `rel_type=m.thread`), reply into that thread; otherwise post a standard room message. - -### 4) Message Handling +### 3) Message Handling - Parse message body for supported music links. - Call `idonthavespotify` (existing behavior). -- Post formatted reply in the same room. +- Post formatted response in the same room. -### 5) Loop Prevention +### 4) Loop Prevention - Ignore events from `@musiclink`. -- Optionally ignore events without link matches. -- Add a small delay/backoff on rate limit responses. - -## Migration Plan -1. **Add Matrix client support behind a feature flag** (e.g., `matrix.enabled`). -2. **Deploy in parallel with Matterbridge** to validate routing and threading. -3. **Disable Matterbridge** once Matrix-native mode is verified. +- Ignore events without link matches. +- Rate limit handling with retry/backoff. ## Risks -- Matrix SDK differences in threading or formatting. - Token handling and access permissions for the bot user. - Message deduplication and race conditions in sync processing. -## Open Questions -- Which Matrix SDK should we standardize on (mautrix-go vs matrix-nio)? -- Do we need explicit thread support in Slack via mautrix-slack mapping? -- Should we persist a small state store for sync tokens? - -## Appendix: Why Not Multiple Gateways? -Multiple gateways in Matterbridge solve cross-posting, but still rely on the fan-out bus model and add operational overhead. A Matrix-native bot is simpler and more correct for routing semantics. +## Status +- Matrix-native mode implemented. +- Matterbridge routing removed. diff --git a/docs/reviews/cli-entrypoints.md b/docs/reviews/cli-entrypoints.md index 50247aa..ba45749 100644 --- a/docs/reviews/cli-entrypoints.md +++ b/docs/reviews/cli-entrypoints.md @@ -6,19 +6,13 @@ ## Findings ### ✅ Strengths -- Clean startup flow: load config → init handler → choose Matrix vs Matterbridge → run with signal cancellation. -- Matrix mode cleanly isolated; `mxBot.Close()` called after run. -- Smoke test exercises detector/API/resolver end-to-end with clear output. +- Clean startup flow: load config → init handler → start Matrix bot → graceful shutdown on SIGINT/SIGTERM. +- Smoketest now includes retry/backoff for API calls and clear output. ### ⚠️ Issues / Opportunities -1. **Matterbridge close on fatal path** - - If `mbBot.Run` returns a non-canceled error, `log.Fatalf` exits before `mbBot.Close()` runs. - - Low impact (process exits), but consistent cleanup could be improved by deferring close after construction. - -2. **Smoke test hard-fails on external API issues** - - Smoke test exits on any API error (expected), but no retries/backoff. - - Acceptable for manual runs; document that it depends on idonthavespotify uptime. +1. **Smoketest depends on external API availability** + - Even with retries, the smoketest relies on idonthavespotify uptime and network access. + - Consider documenting that it is a live integration check and may fail offline. ## Notes -- Signal handling and shutdown behavior are consistent with a long-running service. -- No CLI flags for selecting mode beyond config; that matches config-first expectations. +- Matrix-only runtime is explicit; no hidden mode toggles or legacy paths. diff --git a/docs/reviews/config-packaging.md b/docs/reviews/config-packaging.md new file mode 100644 index 0000000..3a620c7 --- /dev/null +++ b/docs/reviews/config-packaging.md @@ -0,0 +1,30 @@ +# Code Review: Config & Packaging + +## Scope +- `pkg/config/` +- `config.example.toml` +- `go.mod`, `go.sum` +- `flake.nix` +- `vendor/` + +## Findings +### ✅ Strengths +- Config validation enforces required Matrix fields and defaults state store path. +- Example config mirrors the runtime schema and is easy to follow. +- Nix flake uses `buildGoModule` and includes systemd hardening defaults. + +### ⚠️ Issues / Opportunities +1. **vendorHash = null hides dependency drift** + - `flake.nix` is set to `vendorHash = null`, so Nix will accept the vendor tree without integrity verification. + - Consider pinning a hash once the vendor tree is stable. + +2. **Go toolchain version mismatch in dev shell** + - `go.mod` specifies Go 1.24.0, but the dev shell uses `pkgs.go` (likely 1.23). + - Consider pinning `go_1_24` in devShell to match the module requirement. + +3. **Config file parsing assumes local disk** + - Config load is file-only; no env overrides for access tokens. + - Consider documenting secret handling if deploying in production. + +## Notes +- `config.toml` is ignored by git; ensure deployments generate a proper Matrix config. diff --git a/docs/reviews/docs.md b/docs/reviews/docs.md index 96c4465..c07a914 100644 --- a/docs/reviews/docs.md +++ b/docs/reviews/docs.md @@ -1,28 +1,27 @@ # Code Review: Docs & Design Artifacts ## Scope -- `docs/` - `README.md` +- `docs/` - `WORKLOG.md` ## Findings ### ✅ Strengths -- Matrix-native design/intent/approach/work docs are consistent and aligned with implementation. -- README includes Matrix-native configuration, health endpoint, and E2EE limitation. -- Work log captures recent operational issues and next steps. +- Intent/approach/work docs align with the Matrix-native implementation. +- README provides clear Matrix config and health endpoint usage. ### ⚠️ Issues / Opportunities -1. **README still frames Matterbridge as core architecture** - - Architecture section says it is designed to work as a Matterbridge sidecar; Matrix-native mode is now an equal option. - - Consider reframing to describe two supported modes to avoid confusion. +1. **Design doc still references Matterbridge migration** + - `docs/design-matrix-native-routing.md` includes Matterbridge migration steps and open questions that are now resolved. + - Consider updating or archiving it to reflect the Matrix-only state. -2. **Worklog mentions Go 1.22.8** - - `WORKLOG.md` references Go 1.22.8; repo now uses Go 1.24.0. - - Consider updating the worklog or adding a note about the bump. +2. **README prereq Go version outdated** + - README says Go 1.22+, but `go.mod` requires 1.24.0. + - Update README to match the module version. -3. **No migration guide for Matrix-native** - - Docs include config, but no step-by-step migration/cutover guidance for Matterbridge deployments. - - Consider adding a short “migration” section covering shadow mode validation and cutover. +3. **WORKLOG is stale** + - `WORKLOG.md` references Matterbridge and old crash loops. + - Consider updating or moving it to an archive. ## Notes -- Design doc open questions are mostly resolved (SDK choice, state store). Might update the design doc to close them. +- If keeping historical context, add a short “historical” note to older docs rather than removing them entirely. diff --git a/docs/reviews/issues.md b/docs/reviews/issues.md index 63f4dcc..2d7359f 100644 --- a/docs/reviews/issues.md +++ b/docs/reviews/issues.md @@ -1,22 +1,45 @@ # Code Review Issues (Aggregated) ## CLI Entrypoints -1. **Smoke test hard-fails on external API issues** — **Deferred** - - No retries/backoff; relies on idonthavespotify uptime. +1. **Smoketest depends on external API availability** + - Live integration check; can fail offline or if idonthavespotify is down. -## Bot Transport Layers -2. **Matrix send loop has no shutdown drain** — **Deferred** - - Queued responses may be lost on shutdown. +## Matrix Bot Runtime +2. **Shutdown drops pending queue items** + - Queue length is logged but pending items are not drained. -3. **State store cleanup only on message processing** — **Deferred** - - Stale dedupe entries can persist during idle periods. +3. **Encryption prefetch fails fast** + - A non-M_NOT_FOUND error aborts prefetch for subsequent rooms. -## Message Handling & Link Detection -4. **Detector ignores formatted links** — **Deferred** - - No parsing for Markdown/HTML link formats. +4. **Health endpoint unauthenticated** + - `/healthz` exposes counters; should be bound to localhost or protected at the edge. -5. **Resolver only processes first link** — **Deferred** - - Multiple links in one message are ignored beyond the first. +## Message Handling & Link Logic +5. **Detector regex is brittle** + - Central regex risks missing edge cases; per-service patterns might be safer. -6. **Service mapping lacks Qobuz support** — **Deferred** - - Enum exists but no detection/formatting. +6. **Plain-text only parsing** + - Formatted/HTML links are ignored (by design). + +7. **Single-link handling** + - Only first link is handled (by design). + +## Config & Packaging +8. **vendorHash = null** + - Nix doesn’t enforce vendor integrity; should pin a hash if vendor tree stabilizes. + +9. **Go toolchain mismatch in dev shell** + - `go.mod` requires 1.24.0 but devShell uses default `go`. + +10. **Config file only (no env overrides)** + - Token/secret loading relies on file config; document best practices. + +## Docs & Ops +11. **Design doc still references Matterbridge migration** + - `docs/design-matrix-native-routing.md` is outdated. + +12. **README Go version outdated** + - README says Go 1.22+ but module requires 1.24.0. + +13. **WORKLOG is stale** + - Mentions Matterbridge and old crash loops; should be updated or archived. diff --git a/docs/reviews/matrixbot.md b/docs/reviews/matrixbot.md new file mode 100644 index 0000000..876f8e1 --- /dev/null +++ b/docs/reviews/matrixbot.md @@ -0,0 +1,27 @@ +# Code Review: Matrix Bot Runtime + +## Scope +- `internal/matrixbot/` + +## Findings +### ✅ Strengths +- Clear separation of responsibilities: sync loop, send queue, state store, and health endpoints. +- Allowlist + encryption guardrails protect against unintended room processing. +- Dedupe store + periodic cleanup reduce reprocessing on restarts. +- Rate limiting respected with retry/backoff. +- Health endpoint provides useful counters for operational insight. + +### ⚠️ Issues / Opportunities +1. **Send queue drops are logged but not drained** + - On shutdown, queue length is logged but pending items are dropped. + - Acceptable for now; consider best-effort drain if high reliability is required. + +2. **Encryption state prefetch is best-effort only** + - If `StateEvent` fails for reasons other than M_NOT_FOUND, we log and continue. + - Might want to continue per-room instead of aborting at first error. + +3. **Health endpoint has no auth** + - Exposes internal counters; should remain bound to localhost or protected at the edge. + +## Notes +- `com.beeper.linkpreviews = []` matches the previous Matterbridge unfurl suppression behavior. diff --git a/docs/reviews/message-handling.md b/docs/reviews/message-handling.md index e63fe88..5f402c0 100644 --- a/docs/reviews/message-handling.md +++ b/docs/reviews/message-handling.md @@ -1,37 +1,29 @@ -# Code Review: Message Handling & Link Detection +# Code Review: Message Handling & Link Logic ## Scope +- `internal/handler/` - `internal/detector/` - `internal/resolver/` - `internal/services/` ## Findings ### ✅ Strengths -- Regex detection is consolidated in one place; uses a single pattern for all supported services. -- Resolver encapsulates the idonthavespotify API and wraps errors with context. -- Output formatting is consistent and service ordering is explicit. +- Clear separation between detection, resolution, and formatting. +- Resolver wraps API errors with context; bounded error body capture improves diagnostics. +- Service ordering is explicit and consistent in formatting output. ### ⚠️ Issues / Opportunities -1. **Detector misses some common URL variants** - - Apple Music URLs can include `music.apple.com/{country}/album/...` but also have track URLs and other forms not matched. - - Spotify short links (`https://spoti.fi/...`) are not detected. - - Consider adding more variants or making the detector extensible with service-specific regexes. +1. **Detector regex is brittle** + - Centralized regex is dense and risks missing edge cases; updates require careful regex edits. + - Consider migrating to per-service patterns or table-driven detection. -2. **Detector does not parse Markdown/HTML links** - - Matrix `formatted_body` or Slack-style `` formats won’t be parsed, only raw URLs. - - Consider optional parsing of formatted content if available. +2. **Plain-text only parsing** + - `HandleText` only uses raw `body`; formatted/HTML links are ignored. + - Decision made to keep plaintext-only; document this limitation. -3. **Resolver only processes the first link** - - Handler selects `links[0]`, ignoring additional links in the message. - - Consider responding to multiple links or clarifying this behavior in docs. - -4. **API error handling omits response body** - - `Resolve` returns `API returned status %d` without details, making debugging harder. - - Consider reading up to N bytes of body on error for logging. - -5. **Service mapping lacks Qobuz even though enum exists** - - `ServiceQobuz` defined but never populated; detector/formatting do not include it. - - Consider either supporting or removing it to avoid dead config. +3. **Single-link handling** + - Handler uses `links[0]` only; multiple links ignored (by design). + - Explicitly document or consider multi-link responses in future. ## Notes -- Formatting uses title for track only; artist metadata is not used even if present (currently not parsed from API response). +- Qobuz now supported end-to-end (detector + mapping + formatter). diff --git a/docs/reviews/ops.md b/docs/reviews/ops.md new file mode 100644 index 0000000..93aec26 --- /dev/null +++ b/docs/reviews/ops.md @@ -0,0 +1,13 @@ +# Code Review: Ops / Integration References + +## Scope +- `WORKLOG.md` + +## Findings +### ⚠️ Issues / Opportunities +1. **WORKLOG is out of date** + - References Matterbridge and old crash loops; no longer reflects Matrix-native state. + - Consider archiving or updating with the Matrix migration status. + +## Notes +- If the worklog is retained, add a short header noting the date and that it’s historical. diff --git a/docs/work/2026-01-22-code-review-plan.md b/docs/work/2026-01-22-code-review-plan.md new file mode 100644 index 0000000..b6363d0 --- /dev/null +++ b/docs/work/2026-01-22-code-review-plan.md @@ -0,0 +1,20 @@ +# Work: Code Review Plan Execution + +## The Checklist +- [ ] **W001**: Run `code-review` on CLI entrypoints and record findings. + - *Verification*: Notes recorded in `docs/reviews/cli-entrypoints.md` +- [ ] **W002**: Run `code-review` on Matrix bot runtime and record findings. + - *Verification*: Notes recorded in `docs/reviews/matrixbot.md` +- [ ] **W003**: Run `code-review` on message handling & link logic and record findings. + - *Verification*: Notes recorded in `docs/reviews/message-handling.md` +- [ ] **W004**: Run `code-review` on config & packaging and record findings. + - *Verification*: Notes recorded in `docs/reviews/config-packaging.md` +- [ ] **W005**: Run `code-review` on docs & design artifacts and record findings. + - *Verification*: Notes recorded in `docs/reviews/docs.md` +- [ ] **W006**: Run `code-review` on ops/integration references and record findings. + - *Verification*: Notes recorded in `docs/reviews/ops.md` +- [ ] **W007**: Consolidate findings into `docs/reviews/issues.md`. + - *Verification*: Issues list updated. + +## The Audit Trail +* [2026-01-22] Work plan created. diff --git a/docs/work/2026-01-22-tech-debt.md b/docs/work/2026-01-22-tech-debt.md new file mode 100644 index 0000000..4b48e9b --- /dev/null +++ b/docs/work/2026-01-22-tech-debt.md @@ -0,0 +1,18 @@ +# Work: MusicLink Tech Debt Follow-up + +## The Checklist +- [ ] **W001**: Add retry/backoff (or clearer docs) for smoketest API call failures. + - *Verification*: `go test ./...` +- [ ] **W002**: Add best-effort drain or logging for Matrix send queue on shutdown. + - *Verification*: `go test ./...` +- [ ] **W003**: Add periodic cleanup task for Matrix dedupe state store. + - *Verification*: `go test ./...` +- [ ] **W004**: Consider parsing formatted/HTML message content for links (Matrix formatted_body). + - *Verification*: `go test ./...` +- [ ] **W005**: Decide whether to handle multiple links in one message (and implement if desired). + - *Verification*: `go test ./...` +- [ ] **W006**: Decide on Qobuz support (implement or remove enum/formatting references). + - *Verification*: `go test ./...` + +## The Audit Trail +* [2026-01-22] Work plan created. diff --git a/flake.nix b/flake.nix index a95843e..299b56e 100644 --- a/flake.nix +++ b/flake.nix @@ -32,7 +32,7 @@ devShells.default = pkgs.mkShell { buildInputs = with pkgs; [ - go + go_1_24 gopls gotools ]; diff --git a/internal/detector/detector.go b/internal/detector/detector.go index a4778b1..fcf811b 100644 --- a/internal/detector/detector.go +++ b/internal/detector/detector.go @@ -7,20 +7,19 @@ import ( "musiclink/internal/services" ) -// pattern matches music service URLs. -// We use a combined pattern since the idonthavespotify API handles all services. -var pattern = regexp.MustCompile( - `https?://(?:` + - `(?:open\.)?spotify\.com/(?:track|album|artist|playlist)/[a-zA-Z0-9]+|` + - `spoti\.fi/[a-zA-Z0-9]+|` + - `(?:www\.)?(?:youtube\.com/watch\?v=|youtu\.be/|music\.youtube\.com/watch\?v=)[a-zA-Z0-9_-]{11}|` + - `(?:music\.)?apple\.com/[a-z]{2}/(?:album|artist|playlist|song)/[^\s]+|` + - `(?:www\.)?deezer\.com/(?:[a-z]{2}/)?(?:track|album|artist|playlist)/\d+|` + - `(?:www\.)?soundcloud\.com/[a-zA-Z0-9_-]+/[a-zA-Z0-9_-]+|` + - `(?:www\.)?tidal\.com/(?:browse/)?(?:track|album|artist|playlist)/\d+|` + - `[a-zA-Z0-9_-]+\.bandcamp\.com/(?:track|album)/[a-zA-Z0-9_-]+` + - `)`, -) +// patterns match music service URLs. +// We keep patterns per service for easier maintenance. +var patterns = []*regexp.Regexp{ + regexp.MustCompile(`https?://(?:open\.)?spotify\.com/(?:track|album|artist|playlist)/[a-zA-Z0-9]+`), + regexp.MustCompile(`https?://spoti\.fi/[a-zA-Z0-9]+`), + regexp.MustCompile(`https?://(?:www\.)?(?:youtube\.com/watch\?v=|youtu\.be/|music\.youtube\.com/watch\?v=)[a-zA-Z0-9_-]{11}`), + regexp.MustCompile(`https?://(?:music\.)?apple\.com/[a-z]{2}/(?:album|artist|playlist|song)/[^\s]+`), + regexp.MustCompile(`https?://(?:www\.)?deezer\.com/(?:[a-z]{2}/)?(?:track|album|artist|playlist)/\d+`), + regexp.MustCompile(`https?://(?:www\.)?soundcloud\.com/[a-zA-Z0-9_-]+/[a-zA-Z0-9_-]+`), + regexp.MustCompile(`https?://(?:www\.)?tidal\.com/(?:browse/)?(?:track|album|artist|playlist)/\d+`), + regexp.MustCompile(`https?://(?:www\.)?qobuz\.com/[a-z]{2}-[a-z]{2}/(?:album|track|artist|playlist)/[^\s]+`), + regexp.MustCompile(`https?://[a-zA-Z0-9_-]+\.bandcamp\.com/(?:track|album)/[a-zA-Z0-9_-]+`), +} // Detector finds music links in text. type Detector struct{} @@ -32,7 +31,7 @@ func New() *Detector { // Detect finds all music links in the given text. func (d *Detector) Detect(text string) []services.DetectedLink { - matches := pattern.FindAllString(text, -1) + matches := findMatches(text) if len(matches) == 0 { return nil } @@ -47,3 +46,18 @@ func (d *Detector) Detect(text string) []services.DetectedLink { return links } + +func findMatches(text string) []string { + var matches []string + seen := make(map[string]struct{}) + for _, re := range patterns { + for _, match := range re.FindAllString(text, -1) { + if _, ok := seen[match]; ok { + continue + } + seen[match] = struct{}{} + matches = append(matches, match) + } + } + return matches +} diff --git a/internal/matrixbot/bot.go b/internal/matrixbot/bot.go index d78c62e..8ac8ea5 100644 --- a/internal/matrixbot/bot.go +++ b/internal/matrixbot/bot.go @@ -91,6 +91,7 @@ func (b *Bot) Run(ctx context.Context) error { if err := b.prefetchEncryptionState(ctx); err != nil { log.Printf("Matrix encryption state check failed: %v", err) } + go b.cleanupLoop(ctx) go b.sendLoop(ctx) return b.client.SyncWithContext(ctx) } @@ -214,6 +215,9 @@ func (b *Bot) sendLoop(ctx context.Context) { for { select { case <-ctx.Done(): + if pending := len(b.sendQueue); pending > 0 { + log.Printf("Matrix send queue pending on shutdown: %d", pending) + } return case req := <-b.sendQueue: b.sendWithRetry(ctx, req) @@ -221,6 +225,22 @@ func (b *Bot) sendLoop(ctx context.Context) { } } +func (b *Bot) cleanupLoop(ctx context.Context) { + ticker := time.NewTicker(6 * time.Hour) + defer ticker.Stop() + + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + if err := b.stateStore.Cleanup(ctx); err != nil { + log.Printf("Matrix state store cleanup failed: %v", err) + } + } + } +} + func (b *Bot) sendWithRetry(ctx context.Context, req sendRequest) { const maxAttempts = 5 backoff := time.Second @@ -327,7 +347,8 @@ func (b *Bot) prefetchEncryptionState(ctx context.Context) error { if errors.Is(err, mautrix.MNotFound) { continue } - return err + log.Printf("Matrix encryption state fetch failed for %s: %v", roomID, err) + continue } if !b.isRoomEncrypted(roomID) { b.encryptedRooms[roomID] = struct{}{} diff --git a/internal/matrixbot/state_store.go b/internal/matrixbot/state_store.go index 6927372..f2e4e0e 100644 --- a/internal/matrixbot/state_store.go +++ b/internal/matrixbot/state_store.go @@ -174,6 +174,19 @@ func (s *StateStore) MarkEventProcessed(ctx context.Context, eventID string) (bo return affected > 0, nil } +// Cleanup prunes expired processed event IDs. +func (s *StateStore) Cleanup(ctx context.Context) error { + s.mu.Lock() + defer s.mu.Unlock() + + now := time.Now() + if err := s.cleanupLocked(ctx, now); err != nil { + return err + } + s.lastCleanup = now + return nil +} + func (s *StateStore) cleanupLocked(ctx context.Context, now time.Time) error { cutoff := now.Add(-s.dedupeTTL).Unix() _, err := s.db.ExecContext(ctx, diff --git a/internal/resolver/resolver.go b/internal/resolver/resolver.go index 0641032..bce7bf7 100644 --- a/internal/resolver/resolver.go +++ b/internal/resolver/resolver.go @@ -48,6 +48,7 @@ func Format(resolved *services.ResolvedLinks, title string) string { services.ServiceTidal, services.ServiceSoundCloud, services.ServiceBandcamp, + services.ServiceQobuz, } for _, svc := range order { @@ -76,6 +77,8 @@ func serviceName(svc services.ServiceType) string { return "SoundCloud" case services.ServiceBandcamp: return "Bandcamp" + case services.ServiceQobuz: + return "Qobuz" default: return string(svc) } diff --git a/internal/services/idonthavespotify.go b/internal/services/idonthavespotify.go index 277e9d4..ab20024 100644 --- a/internal/services/idonthavespotify.go +++ b/internal/services/idonthavespotify.go @@ -155,6 +155,8 @@ func apiTypeToServiceType(t string) (ServiceType, bool) { return ServiceTidal, true case "bandcamp": return ServiceBandcamp, true + case "qobuz": + return ServiceQobuz, true default: return "", false } diff --git a/pkg/config/config.go b/pkg/config/config.go index 9669e92..7793758 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -3,7 +3,9 @@ package config import ( "fmt" + "net" "os" + "strings" "github.com/BurntSushi/toml" ) @@ -60,5 +62,25 @@ func (c *Config) Validate() error { if c.Matrix.StateStorePath == "" { c.Matrix.StateStorePath = "data/matrix-state.db" } + if c.Matrix.HealthAddr != "" { + if err := validateLocalHealthAddr(c.Matrix.HealthAddr); err != nil { + return err + } + } return nil } + +func validateLocalHealthAddr(addr string) error { + host, _, err := net.SplitHostPort(addr) + if err != nil { + return fmt.Errorf("matrix.healthAddr must be host:port: %w", err) + } + if host == "" { + return fmt.Errorf("matrix.healthAddr must bind to localhost (use 127.0.0.1:PORT)") + } + normalized := strings.ToLower(host) + if normalized == "localhost" || normalized == "127.0.0.1" || normalized == "::1" { + return nil + } + return fmt.Errorf("matrix.healthAddr must bind to localhost (got %s)", host) +}