Address review issues and update docs
This commit is contained in:
parent
a53f61cc81
commit
bb112009a1
12
README.md
12
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
|
||||
|
|
|
|||
17
WORKLOG.md
17
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.
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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 <paths>
|
||||
```
|
||||
2. Record the review output in a notes file (e.g., `docs/reviews/<chunk>.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.
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
30
docs/reviews/config-packaging.md
Normal file
30
docs/reviews/config-packaging.md
Normal file
|
|
@ -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.
|
||||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
27
docs/reviews/matrixbot.md
Normal file
27
docs/reviews/matrixbot.md
Normal file
|
|
@ -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.
|
||||
|
|
@ -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 `<url|text>` 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).
|
||||
|
|
|
|||
13
docs/reviews/ops.md
Normal file
13
docs/reviews/ops.md
Normal file
|
|
@ -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.
|
||||
20
docs/work/2026-01-22-code-review-plan.md
Normal file
20
docs/work/2026-01-22-code-review-plan.md
Normal file
|
|
@ -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.
|
||||
18
docs/work/2026-01-22-tech-debt.md
Normal file
18
docs/work/2026-01-22-tech-debt.md
Normal file
|
|
@ -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.
|
||||
|
|
@ -32,7 +32,7 @@
|
|||
|
||||
devShells.default = pkgs.mkShell {
|
||||
buildInputs = with pkgs; [
|
||||
go
|
||||
go_1_24
|
||||
gopls
|
||||
gotools
|
||||
];
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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{}{}
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue