Open
Conversation
|
| Filename | Overview |
|---|---|
| apps/server/src/external/llm.ts | Major prompt framework refactor: monolithic system prompts replaced with composable sections (PromptProfile, PromptMode, PromptDistance), prompt sanitization helpers added, new LLM functions (enhancePlaylistPrompt, enhanceSongRequest, refineSessionPrompt, enhanceSessionParams) consolidated from web, and getSongPromptContract added for diagnostics. The buildAlbumPrompt in the autoplayer route (not here) lacks per-field sanitization for source song data, though sanitization does happen at the prompt level downstream. |
| apps/server/src/routes/autoplayer.ts | New server-side endpoints for generate-song, generate-album-track, extract-persona, enhance-prompt, enhance-request, refine-prompt, enhance-session, and prompt-contract. The buildAlbumPrompt function embeds sourceSong fields (title, description, lyrics, etc.) into the prompt without per-field sanitization, relying on downstream sanitization in buildSongUserPrompt. |
| apps/server/src/worker/index.ts | Adds comprehensive worker diagnostics: queue status snapshots, song status counts from SQLite, oldest transient songs, and playlist buffer health checks. Scheduled interval with configurable env vars. stopWorkerDiagnostics exported for graceful shutdown. Well-structured with safeMs guard for NaN/invalid timestamps. |
| apps/server/src/worker/song-worker.ts | Playlist manager refresh moved from awaited call (blocking song generation) to best-effort fire-and-forget via new ensurePlaylistManagerRefresh method. The abort check after the old await is removed since refresh is now independent. Deduplication relies on refreshPlaylistManager's own managerRefreshInFlight guard. |
| apps/web/src/lib/autoplayer-proxy.ts | New thin proxy utility: forwards web route requests to backend API server. Passes query string, content-type, method, body, and client abort signal. Intentionally minimal — only forwards content-type to avoid leaking client headers. Error propagation handled by framework. |
| apps/web/src/lib/pipeline-truth.ts | New utility computing cross-cutting pipeline state from songs array + worker queue status. Hardcodes 20_000 as the persona-job priority threshold — a magic number that should match the server-side PERSONA_PRIORITY constant. |
| apps/server/src/logger.ts | Logger upgraded to multi-target: pino-pretty in dev and optional ndjson file output. Adds API key redaction (authorization, apiKey, openrouterApiKey) and service/session metadata in base. Exports loggingConfig for startup diagnostics. isTest guard prevents file logs and pretty output in test runs. |
| apps/server/src/index.ts | Adds request lifecycle middleware with per-request IDs, slow-request warnings, and aggregated summaries for noisy polling routes. Adds unhandledRejection/uncaughtException handlers. stopWorkerDiagnostics called on shutdown. Well-structured with configurable thresholds via env vars. |
| apps/server/src/events/event-bus.ts | Adds configurable event and handler tracing: LOG_EVENT_BUS controls emit-level debug logging, LOG_EVENT_HANDLER_TRACE enables per-handler completion logs, LOG_EVENT_HANDLER_SLOW_MS emits warnings for slow handlers. Sequence counter added for correlating event logs. |
| apps/server/src/tests/prompt-contract.test.ts | New test suite for getSongPromptContract and resolveSongPromptProfile. Tests cover default profile resolution, compact vs full prompt length comparison, faithful → balanced profile relaxation when prompt includes flex keywords, and mode overrides. Good coverage of the new prompt framework. |
Comments Outside Diff (2)
-
apps/server/src/routes/autoplayer.ts, line 2007-2028 (link)buildAlbumPromptskips per-field sanitizationEvery other prompt-building function in
llm.ts(buildPersonaUserPrompt,buildSongUserPrompt,generatePlaylistManagerPlan) callssanitizePromptOptionalon each individual field before embedding it. Here,source.title,source.description,source.lyrics, etc. from the raw request body are spliced into the prompt string directly without sanitization.The combined prompt is sanitized downstream when
buildSongUserPromptreceives it asoptions.prompt, but that strips control/format characters from the assembled string rather than from each embedded field in isolation. This is a weaker form of protection — a Unicode control character embedded mid-field won't be caught until after it's already placed adjacent to structural prompt text.Consider applying
sanitizePromptOptional/sanitizePromptListto thesourcefields before building the lines, consistent with the rest of the codebase.Prompt To Fix With AI
This is a comment left during a code review. Path: apps/server/src/routes/autoplayer.ts Line: 2007-2028 Comment: **`buildAlbumPrompt` skips per-field sanitization** Every other prompt-building function in `llm.ts` (`buildPersonaUserPrompt`, `buildSongUserPrompt`, `generatePlaylistManagerPlan`) calls `sanitizePromptOptional` on each individual field before embedding it. Here, `source.title`, `source.description`, `source.lyrics`, etc. from the raw request body are spliced into the prompt string directly without sanitization. The combined prompt is sanitized downstream when `buildSongUserPrompt` receives it as `options.prompt`, but that strips control/format characters from the assembled string rather than from each embedded field in isolation. This is a weaker form of protection — a Unicode control character embedded mid-field won't be caught until after it's already placed adjacent to structural prompt text. Consider applying `sanitizePromptOptional` / `sanitizePromptList` to the `source` fields before building the lines, consistent with the rest of the codebase. How can I resolve this? If you propose a fix, please make it concise.
-
apps/server/src/worker/song-worker.ts, line 2836-2864 (link)Implicit deduplication reliance in
ensurePlaylistManagerRefreshThe method logs "Scheduling playlist manager refresh in background" only when the
refreshKeyis NOT already inmanagerRefreshInFlight, but then unconditionally callsthis.refreshPlaylistManager(...)regardless of whether the key is in-flight. This relies onrefreshPlaylistManagerinternally guarding against concurrent refreshes via themanagerRefreshInFlightset.This is functional if
refreshPlaylistManagerdoes its own check (which it almost certainly does given the naming), but the pattern is implicit and the intent isn't obvious from readingensurePlaylistManagerRefreshalone. A brief comment or an early-return guard would make the design explicit:// refreshPlaylistManager guards against concurrent refreshes via managerRefreshInFlight; // this fire-and-forget call is always safe even when a refresh is in progress. void this.refreshPlaylistManager(currentEpoch, provider, model).catch(...);
Prompt To Fix With AI
This is a comment left during a code review. Path: apps/server/src/worker/song-worker.ts Line: 2836-2864 Comment: **Implicit deduplication reliance in `ensurePlaylistManagerRefresh`** The method logs "Scheduling playlist manager refresh in background" only when the `refreshKey` is NOT already in `managerRefreshInFlight`, but then unconditionally calls `this.refreshPlaylistManager(...)` regardless of whether the key is in-flight. This relies on `refreshPlaylistManager` internally guarding against concurrent refreshes via the `managerRefreshInFlight` set. This is functional if `refreshPlaylistManager` does its own check (which it almost certainly does given the naming), but the pattern is implicit and the intent isn't obvious from reading `ensurePlaylistManagerRefresh` alone. A brief comment or an early-return guard would make the design explicit: ```ts // refreshPlaylistManager guards against concurrent refreshes via managerRefreshInFlight; // this fire-and-forget call is always safe even when a refresh is in progress. void this.refreshPlaylistManager(currentEpoch, provider, model).catch(...); ``` How can I resolve this? If you propose a fix, please make it concise.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/web/src/lib/pipeline-truth.ts
Line: 16-22
Comment:
**Hardcoded persona priority magic number**
`20_000` is used as the threshold to detect persona LLM jobs, but this value mirrors the server-side `PERSONA_PRIORITY` constant (imported in `apps/server/src/worker/index.ts`). If that constant ever changes, this client-side check will silently drift out of sync.
Consider extracting this to a named constant with a comment, or sharing the value via a shared package:
```suggestion
const PERSONA_PRIORITY_THRESHOLD = 20_000; // Must match server PERSONA_PRIORITY
let personaLlmJobs = 0;
if (status) {
for (const item of status.queues.llm.activeItems) {
if (item.priority >= PERSONA_PRIORITY_THRESHOLD) personaLlmJobs += 1;
}
for (const item of status.queues.llm.pendingItems) {
if (item.priority >= PERSONA_PRIORITY_THRESHOLD) personaLlmJobs += 1;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/server/src/routes/autoplayer.ts
Line: 2007-2028
Comment:
**`buildAlbumPrompt` skips per-field sanitization**
Every other prompt-building function in `llm.ts` (`buildPersonaUserPrompt`, `buildSongUserPrompt`, `generatePlaylistManagerPlan`) calls `sanitizePromptOptional` on each individual field before embedding it. Here, `source.title`, `source.description`, `source.lyrics`, etc. from the raw request body are spliced into the prompt string directly without sanitization.
The combined prompt is sanitized downstream when `buildSongUserPrompt` receives it as `options.prompt`, but that strips control/format characters from the assembled string rather than from each embedded field in isolation. This is a weaker form of protection — a Unicode control character embedded mid-field won't be caught until after it's already placed adjacent to structural prompt text.
Consider applying `sanitizePromptOptional` / `sanitizePromptList` to the `source` fields before building the lines, consistent with the rest of the codebase.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/server/src/worker/song-worker.ts
Line: 2836-2864
Comment:
**Implicit deduplication reliance in `ensurePlaylistManagerRefresh`**
The method logs "Scheduling playlist manager refresh in background" only when the `refreshKey` is NOT already in `managerRefreshInFlight`, but then unconditionally calls `this.refreshPlaylistManager(...)` regardless of whether the key is in-flight. This relies on `refreshPlaylistManager` internally guarding against concurrent refreshes via the `managerRefreshInFlight` set.
This is functional if `refreshPlaylistManager` does its own check (which it almost certainly does given the naming), but the pattern is implicit and the intent isn't obvious from reading `ensurePlaylistManagerRefresh` alone. A brief comment or an early-return guard would make the design explicit:
```ts
// refreshPlaylistManager guards against concurrent refreshes via managerRefreshInFlight;
// this fire-and-forget call is always safe even when a refresh is in progress.
void this.refreshPlaylistManager(currentEpoch, provider, model).catch(...);
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Increase Codex timeout and add queue run..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+16
to
+22
| } | ||
|
|
||
| function collectQueueSongIds(status: WorkerStatus | null) { | ||
| const llm = new Set<string>(); | ||
| const audio = new Set<string>(); | ||
|
|
||
| if (!status) return { llm, audio }; |
There was a problem hiding this comment.
20_000 is used as the threshold to detect persona LLM jobs, but this value mirrors the server-side PERSONA_PRIORITY constant (imported in apps/server/src/worker/index.ts). If that constant ever changes, this client-side check will silently drift out of sync.
Consider extracting this to a named constant with a comment, or sharing the value via a shared package:
Suggested change
| } | |
| function collectQueueSongIds(status: WorkerStatus | null) { | |
| const llm = new Set<string>(); | |
| const audio = new Set<string>(); | |
| if (!status) return { llm, audio }; | |
| const PERSONA_PRIORITY_THRESHOLD = 20_000; // Must match server PERSONA_PRIORITY | |
| let personaLlmJobs = 0; | |
| if (status) { | |
| for (const item of status.queues.llm.activeItems) { | |
| if (item.priority >= PERSONA_PRIORITY_THRESHOLD) personaLlmJobs += 1; | |
| } | |
| for (const item of status.queues.llm.pendingItems) { | |
| if (item.priority >= PERSONA_PRIORITY_THRESHOLD) personaLlmJobs += 1; | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/src/lib/pipeline-truth.ts
Line: 16-22
Comment:
**Hardcoded persona priority magic number**
`20_000` is used as the threshold to detect persona LLM jobs, but this value mirrors the server-side `PERSONA_PRIORITY` constant (imported in `apps/server/src/worker/index.ts`). If that constant ever changes, this client-side check will silently drift out of sync.
Consider extracting this to a named constant with a comment, or sharing the value via a shared package:
```suggestion
const PERSONA_PRIORITY_THRESHOLD = 20_000; // Must match server PERSONA_PRIORITY
let personaLlmJobs = 0;
if (status) {
for (const item of status.queues.llm.activeItems) {
if (item.priority >= PERSONA_PRIORITY_THRESHOLD) personaLlmJobs += 1;
}
for (const item of status.queues.llm.pendingItems) {
if (item.priority >= PERSONA_PRIORITY_THRESHOLD) personaLlmJobs += 1;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Somewhere a GPU is overheating so I don't have to think.