Closed
Conversation
- Add history storage with save/load/clear functionality - Add new 'history' CLI command with -c/--clear and -l/--limit options - Integrate history saving into single and batch processing flows - Add licensingUrl extraction to AI summary with new schema field - Display Spotify link and licensing URL in verdict output - Improve MusicBrainz API error handling and response parsing - Enhance AI client rate limit detection with additional error types
- Add Spotify search feature to both desktop app and CLI with interactive track selection - Replace Modal AI provider with Groq for improved performance and cost-effectiveness - Update UI to include search mode alongside URL input in desktop application - Add IPC handlers for Spotify search between renderer and main processes - Refactor CLI check logic into shared function for reuse in search command - Update configuration types and validation to support Groq integration - Add test files for Spotify search functionality - Update project descriptions to reflect Renderdragon branding
- Add release.yml workflow for CLI tarball + Desktop (Win/Mac/Linux) builds - Inject env vars at compile time via electron-vite define - Bundle CLI dist as extraResources for packaged desktop app - Update .gitignore for build artifacts
- Change author field to object format and add homepage URL - Add maintainer field for Linux target and update icon reference - Add new icon1.png file for Linux build assets
- Add Convex backend with secrets query to fetch API keys securely - Update config system to fetch secrets from Convex when available - Modify desktop app to use CONVEX_URL instead of embedding API keys - Update CI/CD workflows to use Convex for secret management - Add licensing URL validation in summary agent for HTTPS URLs - Include Convex generated files and dependencies in build process
📝 WalkthroughWalkthroughThis PR introduces a complete Electron-based desktop application alongside CLI enhancements, new Convex backend integration for secrets management, Groq AI fallback support, Spotify track search functionality, and persistent history tracking. A GitHub Actions CI/Release workflow is added to automate building and publishing both CLI and desktop distributions. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Desktop User
participant Renderer as Renderer Process
participant Main as Main Process
participant CLI as CLI Distribution
participant Convex as Convex API
participant Spotify as Spotify API
User->>Renderer: Enter track input/search
Renderer->>Main: invoke check:run(trackInput)
Main->>Main: Resolve CLI dist path
Main->>CLI: Import & instantiate PipelineRunner
Main->>Convex: Fetch secrets via getApiKeys
Convex-->>Main: Return API keys
CLI->>CLI: Run copyright check pipeline
CLI-->>Main: Emit progress events
Main->>Renderer: window.osmium events (progress)
Renderer->>Renderer: Accumulate progress, update UI
CLI-->>Main: Return verdict
Main->>Main: Store verdict (max 50)
Main-->>Renderer: Verdict & success status
Renderer->>Renderer: Display VerdictView
User->>Renderer: Search for track
Renderer->>Main: invoke spotify:search(query)
Main->>Spotify: searchTracks(query, token)
Spotify-->>Main: Track results
Main-->>Renderer: SpotifySearchResult[]
Renderer->>Renderer: Display search results
User->>Renderer: Select track
Renderer->>Main: invoke check:run(trackId)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/musicbrainz.ts (1)
82-82:⚠️ Potential issue | 🟠 MajorField name mismatch between API response and TypeScript type in
getRecording.The MusicBrainz API returns hyphenated field names (e.g.,
artist-credit,label-info), but theMusicBrainzRecordinginterface uses camelCase (artistCredit). Line 82 uses a type cast (as Promise<MusicBrainzRecording>) without transforming the response data, causing fields to beundefinedat runtime. This mirrors thelabel-infoissue inlookupByISRC(line 49), which handles it with defensive null-coalescing:(release?.labelInfo ?? release?.["label-info"]). Apply the same pattern togetRecording.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/musicbrainz.ts` at line 82, The response from response.json() in getRecording is being cast directly to MusicBrainzRecording which leaves hyphenated API fields (e.g., "artist-credit", "label-info") undefined; instead, await response.json() into a raw object, then construct and return a MusicBrainzRecording by mapping hyphenated keys to camelCase (use the same defensive pattern used in lookupByISRC: e.g., release?.labelInfo ?? release?.["label-info"]) for fields like artistCredit and labelInfo; update getRecording to perform that transformation before returning so callers receive the correct typed shape.
🟡 Minor comments (5)
README.md-1-3 (1)
1-3:⚠️ Potential issue | 🟡 MinorMention the desktop app in the opening description.
This PR adds desktop release artifacts, but the README still introduces Osmium as a CLI-only tool. Please surface the desktop app here so users know there is more than one install path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 1 - 3, The README header currently reads "# Osmium CLI 💿" which implies CLI-only; update the opening title and first sentence to mention both the CLI and the new desktop app (e.g., change the header to include "CLI and Desktop" and amend "Osmium is a music copyright checker" to reference both install paths), and add a brief note/link indicating desktop release artifacts are available so users know there is more than one install path.tmp/test2.ts-5-23 (1)
5-23:⚠️ Potential issue | 🟡 MinorRestore top-level error handling.
getConfig()andgetSpotifyToken()run before the innertry, so config/auth failures now surface as unhandled rejections.Proposed fix
async function run() { console.log("Starting..."); const config = getConfig(); const token = await getSpotifyToken(config.spotify.clientId, config.spotify.clientSecret); console.log("Token acquired"); const url = `https://api.spotify.com/v1/search?q=blinding%20lights&type=track&limit=20`; console.log("Fetching:", url); - try { - const response = await fetchJSON(url, { - headers: { Authorization: `Bearer ${token}` } - }); - console.log("Success:", !!response); - } catch (err) { - console.error(err); - } + const response = await fetchJSON(url, { + headers: { Authorization: `Bearer ${token}` } + }); + console.log("Success:", !!response); } -run(); +run().catch((err) => { + console.error(err); + process.exitCode = 1; +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tmp/test2.ts` around lines 5 - 23, The current run() function calls getConfig() and await getSpotifyToken(...) before the inner try/catch, causing failures there to be unhandled; move those calls inside the existing try block or wrap the entire run() body in a top-level try/catch so that errors from getConfig and getSpotifyToken are caught and logged; update the run function (referencing run, getConfig, getSpotifyToken, and the existing try/catch around fetchJSON) to ensure any thrown errors are handled consistently and logged in the catch.src/services/spotify.ts-187-198 (1)
187-198:⚠️ Potential issue | 🟡 MinorHonor the
limitparameter in the Spotify request.
limitis part of the public API, but it never reaches Spotify, so callers always get the service default instead of the requested page size.Proposed fix
export async function searchTracks( query: string, token: string, limit: number = 20 ): Promise<SpotifySearchResult[]> { const response = await fetchJSON<{ tracks: { items: SpotifyTrack[]; }; - }>(`${SPOTIFY_API_BASE}/search?q=${encodeURIComponent(query)}&type=track`, { + }>(`${SPOTIFY_API_BASE}/search?q=${encodeURIComponent(query)}&type=track&limit=${limit}`, { headers: { Authorization: `Bearer ${token}` }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/spotify.ts` around lines 187 - 198, The searchTracks function never sends the limit to Spotify; update the request URL built in searchTracks to include the limit query parameter (e.g., append &limit=${encodeURIComponent(limit.toString())} or similar) so the passed-in limit value is honored when calling fetchJSON; ensure the change references the existing function name searchTracks and the fetchJSON call where the URL is constructed (keep encodeURIComponent for query and use the numeric limit value directly or converted to string).desktop-app/src/renderer/src/components/Sidebar.tsx-26-31 (1)
26-31:⚠️ Potential issue | 🟡 MinorExpose the selected session state semantically.
The active entry is only indicated by background color on Lines 29-31, so assistive tech users can't tell which history item is open. Add an explicit selected/current state such as
aria-currentoraria-pressedon the active button.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop-app/src/renderer/src/components/Sidebar.tsx` around lines 26 - 31, The button indicating an active session only uses color, so update the <button> in Sidebar (the element rendered for each session, keyed by session.id and using onSelect) to expose semantic selected state for assistive tech by adding an ARIA attribute (e.g., aria-current="page" or aria-pressed="true") when activeId === session.id and omitting it otherwise; ensure the attribute is applied to the same button element that toggles via onSelect so screen readers can identify the current/selected history item.desktop-app/src/renderer/src/components/HomePage.tsx-27-38 (1)
27-38:⚠️ Potential issue | 🟡 MinorShow search failures in the UI instead of folding them into the empty state.
When the search call fails, this only logs to the console and clears
searchResults, so the pane falls back to the same "Search for a track to begin" message used before any search. That makes backend/config failures indistinguishable from a legitimate zero-result query.💡 Suggested fix
const [searchQuery, setSearchQuery] = useState('') const [isSearching, setIsSearching] = useState(false) const [searchResults, setSearchResults] = useState<SpotifySearchResult[]>([]) + const [searchError, setSearchError] = useState<string | null>(null) const handleSearch = async (e: React.FormEvent) => { e.preventDefault() if (!searchQuery.trim() || isSearching || isChecking) return setIsSearching(true) + setSearchError(null) try { const response = await window.osmium.spotify.search(searchQuery.trim()) if (response.success && response.results) { setSearchResults(response.results) } else { - console.error('Search failed:', response.error) + setSearchError(response.error ?? 'Search failed.') setSearchResults([]) } } catch (err) { console.error(err) + setSearchError('Search failed.') setSearchResults([]) } finally { setIsSearching(false) } } @@ - <div className="flex-1 overflow-auto bg-bg-tertiary border border-white/10 rounded"> - {searchResults.length === 0 ? ( + <div className="flex-1 overflow-auto bg-bg-tertiary border border-white/10 rounded"> + {searchError ? ( + <div className="h-full flex items-center justify-center text-red-300 text-sm px-4 text-center"> + {searchError} + </div> + ) : searchResults.length === 0 ? ( <div className="h-full flex items-center justify-center text-white/30 text-sm"> No results. Search for a track to begin. </div>Also applies to: 114-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop-app/src/renderer/src/components/HomePage.tsx` around lines 27 - 38, The search failure handling currently just logs and clears searchResults (in the HomePage component where window.osmium.spotify.search is called using setIsSearching, setSearchResults and searchQuery), which collapses backend errors into the empty-state UI; add a dedicated search error state (e.g. searchError with setter) and set it when response.success is false or the call throws, clear it on successful searches, and update the render to show the error message in the results pane (instead of the "Search for a track to begin" message) so configuration/backend failures are visible to users; adjust the same pattern for the other search call mentioned (the second occurrence that uses setSearchResults).
🧹 Nitpick comments (5)
src/services/musicbrainz.ts (1)
48-52: Type assertions reduce type safety; consider defining a more precise type.The chain of
as unknown as Record<string, unknown>followed by indexing and another cast obscures the actual response shape. If the MusicBrainz API returns inconsistent field names (labelInfovslabel-info), consider defining a union type or a more explicit interface to preserve type safety and improve readability.♻️ Suggested type-safe approach
interface MBRelease { id: string; title: string; "label-info"?: Array<{ label?: { name: string } }>; labelInfo?: Array<{ label?: { name: string } }>; }Then update the extraction:
- const release = primaryRecording.releases?.[0] as unknown as Record<string, unknown>; - const labelInfo = (release?.labelInfo ?? release?.["label-info"]) as Array<{ label?: { name: string } }> | undefined; + const release = primaryRecording.releases?.[0] as MBRelease | undefined; + const labelInfo = release?.["label-info"] ?? release?.labelInfo;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/musicbrainz.ts` around lines 48 - 52, Replace the unsafe double-cast used when reading release/label data by defining a precise interface (e.g., MBRelease with fields id, title, "label-info"?: Array<{label?:{name:string}}>, labelInfo?: Array<{label?:{name:string}}>), use that type for primaryRecording.releases items and for the local variable currently named release, and then read the label name from the typed union (check release["label-info"] ?? release.labelInfo) instead of using as unknown as Record<string, unknown>; update usages of primaryRecording and labelInfo to rely on the new MBRelease type to preserve type safety and readability.src/services/spotify.ts (1)
176-182: Avoid duplicating the search result contract.
SpotifySearchResulthere matchesdesktop-app/src/renderer/src/types.tslines 63-69. Please move it to a shared module so the preload/renderer boundary cannot drift silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/spotify.ts` around lines 176 - 182, The SpotifySearchResult interface is duplicated; extract the definition into a shared types module (e.g., create a shared export like SharedSpotifyTypes or types.ts) and export SpotifySearchResult from there, then replace the local declaration in src/services/spotify.ts with an import of SpotifySearchResult and update the renderer/preload code that currently defines the same shape to import the shared type instead; ensure all references to SpotifySearchResult (the interface name) in functions and signatures are updated to the new shared import so the preload/renderer boundary uses the single canonical type.convex/secrets.js (1)
1-20: Avoid checking generated Convex JS into the functions directory.The
sourceMappingURLon Line 20 makes this look like build output forconvex/secrets.ts. Keeping both copies inconvex/creates two edit surfaces for the same query and makes drift easy; prefer keeping the TS source as the single source of truth here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/secrets.js` around lines 1 - 20, The committed convex/secrets.js appears to be generated build output (contains sourceMappingURL) and duplicates the TS source, causing drift; remove convex/secrets.js from the repo and stop checking generated Convex JS into the functions directory, ensure the original convex/secrets.ts (which defines the getApiKeys query/handler) remains the single source of truth, add convex/*.js (or the specific generated file pattern) to .gitignore, and update your build/output configuration so generated Convex JS is written to a build/dist directory instead of the convex/ source directory.desktop-app/src/renderer/src/types.ts (1)
1-52: Share the canonical IPC types instead of redefining wider copies.This renderer contract has already drifted from
src/types/index.ts, Lines 200-275:PipelineEvent.name,samples.riskLevel, andcopyrightTypeare all wider here than in the source type. That drops useful exhaustiveness checks at the main/renderer boundary and lets invalid payloads type-check. Prefer importing a shared contract or mirroring the exact unions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop-app/src/renderer/src/types.ts` around lines 1 - 52, The renderer types have drifted from the canonical main/shared contract—PipelineEvent, samples.riskLevel, and copyrightType are broader here than in src/types/index.ts—so replace the local definitions with the shared IPC contract: import and re-export (or reference) the canonical types from the main/shared module (the definitions used around Lines 200-275 in src/types/index.ts) instead of redefining them, ensuring PipelineEvent.name, samples.riskLevel, and copyrightType use the exact union types from the shared file so the renderer/main boundary preserves exhaustiveness and correct type-checking.desktop-app/src/main/index.ts (1)
59-81: Import shared verdict/progress types instead of copying them here.These inline IPC/store types have already drifted from the renderer contract:
desktop-app/src/renderer/src/types.tsincludesaiSummary.webSearchSources, but this localCopyrightVerdictdoes not. Keeping the payloads duplicated makes main/store/renderer desyncs easy to miss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop-app/src/main/index.ts` around lines 59 - 81, Replace the duplicated local interfaces by importing the shared types used by the renderer (do not keep PipelineEvent and CopyrightVerdict inline); remove the local declarations for PipelineEvent and CopyrightVerdict and import the canonical types from the shared renderer/store types module so the main process uses the same shape (including aiSummary.webSearchSources) and update any local uses to the imported names to avoid payload drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Around line 10-11: The workflow currently sets top-level permissions
"contents: write" which grants write access to GITHUB_TOKEN for all jobs
(including "ci"); change this so the default top-level permissions are read-only
(e.g., "contents: read") and then explicitly scope "contents: write" under only
the release job by adding a permissions block inside the "release" job; update
the "ci" job (and any other non-release jobs) to rely on the default read-only
permission so only the "release" job receives write access.
- Around line 34-35: Add a dedicated desktop typecheck step that runs the
desktop-app's strict TypeScript check by adding a step named "Typecheck Desktop"
that executes npm run typecheck with working-directory set to desktop-app;
insert this step in the ci job immediately after the existing root/CLI
"Typecheck" step and in the release-desktop job immediately before the "build"
step (the existing electron-vite build step), so the desktop-app's tsconfig (and
its tsc --noEmit script) is validated during CI and release workflows.
In `@convex/secrets.d.ts`:
- Around line 1-13: The public Convex query getApiKeys is exposing sensitive
process.env values to clients; change it to a server-only endpoint by using
internalQuery()/RegisteredAction or the server-only registration equivalent
instead of query/RegisteredQuery<"public">, and update any callers (e.g., where
the client currently calls getApiKeys) to use server-side invocation;
alternatively, if some non-secret config must be returned, remove all secret
fields (SPOTIFY_CLIENT_SECRET, OPENROUTER_API_KEY, GROQ_API_KEY, DISCOGS_TOKEN,
etc.) from the returned object and only return safe, non-sensitive values (or
return masked/boolean flags) from getApiKeys to prevent secret exfiltration.
In `@convex/secrets.ts`:
- Around line 3-18: The handler getApiKeys is currently exposed via query() and
returns secrets; change its declaration to use internalQuery() so it becomes
server-only, remove any sensitive fields from the returned object
(spotifyClientSecret, openrouterApiKey, groqApiKey, tavilyApiKey,
acoustidApiKey, nocodeSpotifyToken, discogsToken) and only return non-secret
configuration values (e.g., openrouterModel, groqModel, nocodeSpotifyCloudName)
or move secret-using calls entirely server-side; update any callers of
getApiKeys to call the new internal-only function from server code or to use a
new server-side function that performs secret-backed API calls rather than
exposing raw credentials.
In `@desktop-app/out/preload/index.js`:
- Around line 9-10: The IPC handler for "shell:open-external" currently passes
arbitrary URLs straight to shell.openExternal; update the handler in main (the
"shell:open-external" IPC callback in index.ts) to validate the incoming URL by
parsing its scheme and only allow a whitelist (e.g., "http" and "https"); if the
scheme is not allowed, reject/throw and do not call shell.openExternal, and
return/report a clear error to the renderer. Ensure validation uses a proper URL
parser and normalize input (reject empty or relative strings) before calling
shell.openExternal, and keep the IPC channel name "shell:open-external"
unchanged.
In `@desktop-app/package.json`:
- Around line 41-44: The extraResources entry in package.json currently copies
the repo root node_modules ("from": "../node_modules", "to":
"cli-dist/node_modules"), which pulls devDeps and platform-specific files into
installers; change the packaging to stage only the built CLI bundle and its
production dependencies instead — remove or stop using the "../node_modules"
mapping and instead populate cli-dist/node_modules from the CLI package's
production install (e.g., run a production npm/yarn install or use npm pack /
pnpm pack for the CLI package) so only runtime deps are included; update the
package.json extraResources to point at the CLI bundle/artifact (or a prepared
cli-dist directory) rather than the monorepo root node_modules.
In `@desktop-app/postcss.config.js`:
- Around line 1-6: The config uses ESM "export default" but the package is
CommonJS, causing a syntax error; update postcss.config.js to use CommonJS
export (replace "export default { ... }" with a CommonJS assignment like
"module.exports = { plugins: { tailwindcss: {}, autoprefixer: {} } }") or
alternatively add "type": "module" to package.json—pick the CommonJS change so
Node/PostCSS/Tailwind can load the file without changing package scope.
In `@desktop-app/src/main/index.ts`:
- Around line 112-115: The window-open handler currently sends untrusted
renderer URLs straight to shell.openExternal (see
mainWindow.webContents.setWindowOpenHandler and calls to shell.openExternal);
update the handler to validate and sanitize the URL before calling
shell.openExternal: construct and parse the value with the URL constructor in a
try/catch, ensure the protocol is http or https, optionally
normalize/percent-decode and check against an allowlist or blocklist of domains
if appropriate, and only call shell.openExternal when the URL passes validation;
for invalid or unsafe inputs, deny the open and log or ignore the attempt.
In `@desktop-app/src/renderer/index.html`:
- Around line 3-10: Add a strict Content-Security-Policy meta tag to the <head>
of the renderer index.html to harden against XSS (e.g. default-src 'self';
script-src 'self'; connect-src 'self' https:; object-src 'none'; frame-ancestors
'none'; base-uri 'self') and ensure it does not break the renderer's runtime; if
your dev setup or HMR in src/main.tsx requires relaxed rules during development,
conditionally relax only the necessary directives for dev builds and keep the
production policy strict; verify that the policy allows your
contextBridge-exposed API (window.osmium) to function by not requiring
unsafe-inline or unsafe-eval in production.
In `@desktop-app/src/renderer/src/App.tsx`:
- Around line 39-46: The active-session updates are not scoped to the session
being updated: in the window.osmium.check handlers (e.g., the onProgress,
completion and failure callbacks where setSessions and setActiveSession are
called) you must only mutate active state when the active session matches the
session being reported; change the setActiveSession calls to check prev?.id ===
session.id before applying the update (mirror the guard used in setSessions
which compares s.id === session.id) so a different open session doesn't receive
live progress/completion/error for this running check.
- Around line 14-23: The effect that calls window.osmium.store.getChecks()
currently overwrites sessions when its promise resolves; change it to merge
hydrated history into the current state instead of replacing it and handle
promise rejection: call getChecks() and on success map to CheckSession objects
(as currently done) then call setSessions(prev => { const merged = [...prev];
add each hydrated session only if it doesn't conflict with an existing session
id or in-flight marker (use id like `history-${i}` or another stable key) —
e.g., skip adding when prev.some(s => s.id === hydrated.id); return
merged.concat(newOnes); }) to preserve in-flight checks, and add a .catch(err =>
{/* log via console.error or app logger */}) to handle failures. Ensure the code
uses the same symbols (useEffect, window.osmium.store.getChecks, setSessions,
CheckSession) so the change is localized.
- Around line 39-51: The progress subscription created via
window.osmium.check.onProgress must always be removed and isChecking reset even
if window.osmium.check.run(input) rejects; wrap the run call and
unsubscribe/setIsChecking logic in a try/finally (or ensure a finally block) so
that unsubscribe() is invoked and setIsChecking(false) is called regardless of
errors. Update the block around onProgress/unsubscribe/run to guarantee
unsubscribe and setIsChecking(false) in the finally path, referring to the
existing unsubscribe variable and setIsChecking/state updates (and keep the
event handling that updates sessions and setActiveSession).
In `@desktop-app/src/renderer/src/components/TitleBar.tsx`:
- Around line 8-30: TitleBar's icon-only buttons (handlers
window.osmium.window.minimize, .maximize, .close) lack accessible names; add
descriptive accessible labels by adding an aria-label (e.g.,
aria-label="Minimize window", "Maximize window", "Close window") to each button
or include a visually-hidden text node (sr-only) inside the respective button
for screen readers; ensure the labels match the action names and keep existing
onClick handlers and visual markup intact in the TitleBar component.
- Around line 3-7: The code uses React.CSSProperties for inline style types but
never imports React, causing a TypeScript error; add an explicit type import
(e.g. import type { CSSProperties } from 'react') at the top of TitleBar.tsx and
replace React.CSSProperties usages in the style props (the inline style objects
on the root div and the inner div with WebkitAppRegion: 'drag' and 'no-drag')
with CSSProperties so the type reference resolves without importing the React
runtime.
In `@package.json`:
- Line 15: The test NPM script in package.json currently exits with success when
the built tests directory is missing or when no .test.js files are found; update
the "test" script logic (the script string named "test" that defines const dir =
'dist/tests', const files = readdirSync(dir)... etc.) so that when
!existsSync(dir) or files.length === 0 the process exits with a non-zero status
(e.g., process.exit(1)) and optionally prints a clear error message before
exiting to ensure CI fails if no built tests are discovered.
In `@src/agents/summary-agent.ts`:
- Around line 66-82: The current logic accepts any HTTPS URL from
parsed.licensingUrl and returns it in the AISummary even if it never appears in
fetched evidence; change the validation so licensingUrl is only returned when
the normalized parsed.licensingUrl exactly matches (or has the same origin as)
at least one URL from webSearchResults. In the block around licensingUrl (refer
to parsed.licensingUrl, licensingUrl, and webSearchResults), parse and normalize
parsed.licensingUrl, then iterate webSearchResults entries and compare either
url.href equality or url.origin equality against the normalized value; only
assign licensingUrl when a match is found, otherwise keep it null/undefined.
Ensure any URL parsing errors still fall back to null.
In `@src/config/index.ts`:
- Around line 35-40: The sync and async config builders disagree on defaults and
share a single _config cache so the first caller locks in both the
openrouter.model fallback and whether Convex secrets were loaded; fix by
centralizing config construction so both getConfig() and getConfigAsync() use
the same builder: have buildConfigFromEnv() produce a plain config object
(without applying async Convex lookups or differing model defaults) and then
have a single async initializer (e.g., validateConfigAsync or a new
initConfigAsync) that applies the final openrouter.model fallback and loads
Convex secrets before writing to the shared _config cache; ensure getConfig()
returns a config only if _config is already initialized (or throws), and
getConfigAsync() awaits the unified async initializer so the cache is always
populated consistently for future sync/async callers.
In `@src/index.ts`:
- Around line 94-95: Wrap calls to saveToHistory in a try/catch so write
failures are logged but do not change the check result or pipeline exit status;
specifically, in runCheck (where await saveToHistory(verdict) is called) and the
other occurrence around lines referenced (200-202), catch any error, log it with
the existing logger (e.g., processLogger or logger.error) including the error
details and context, and do not rethrow or return a non-zero status—ensure the
original verdict is preserved and returned/pushed as before.
In `@src/utils/history.ts`:
- Around line 33-36: The loadHistory function currently returns parsed.checks
directly after JSON.parse which can be non-array and later break code; update
loadHistory to validate parsed.checks with Array.isArray(parsed.checks) and only
return it when it's an array, otherwise return an empty array as a fallback
(e.g., const checks = Array.isArray(parsed.checks) ? parsed.checks : []; return
checks;). Ensure you reference the parsed variable and the loadHistory function
in your change so callers (where [verdict, ...existing] is used) always receive
an array.
In `@tmp/test2.ts`:
- Around line 8-9: The console.log that prints the partial bearer token
("Token:", token.substring(...)) must be removed to avoid exposing secrets; in
the code path that calls getSpotifyToken ensure you do not output the token
variable to stdout or logs—either drop the console.log entirely or replace it
with a non-sensitive status message (e.g., "Spotify token acquired" or log only
token length) and keep the actual token confined to secure variables/usage in
the token variable.
---
Outside diff comments:
In `@src/services/musicbrainz.ts`:
- Line 82: The response from response.json() in getRecording is being cast
directly to MusicBrainzRecording which leaves hyphenated API fields (e.g.,
"artist-credit", "label-info") undefined; instead, await response.json() into a
raw object, then construct and return a MusicBrainzRecording by mapping
hyphenated keys to camelCase (use the same defensive pattern used in
lookupByISRC: e.g., release?.labelInfo ?? release?.["label-info"]) for fields
like artistCredit and labelInfo; update getRecording to perform that
transformation before returning so callers receive the correct typed shape.
---
Minor comments:
In `@desktop-app/src/renderer/src/components/HomePage.tsx`:
- Around line 27-38: The search failure handling currently just logs and clears
searchResults (in the HomePage component where window.osmium.spotify.search is
called using setIsSearching, setSearchResults and searchQuery), which collapses
backend errors into the empty-state UI; add a dedicated search error state (e.g.
searchError with setter) and set it when response.success is false or the call
throws, clear it on successful searches, and update the render to show the error
message in the results pane (instead of the "Search for a track to begin"
message) so configuration/backend failures are visible to users; adjust the same
pattern for the other search call mentioned (the second occurrence that uses
setSearchResults).
In `@desktop-app/src/renderer/src/components/Sidebar.tsx`:
- Around line 26-31: The button indicating an active session only uses color, so
update the <button> in Sidebar (the element rendered for each session, keyed by
session.id and using onSelect) to expose semantic selected state for assistive
tech by adding an ARIA attribute (e.g., aria-current="page" or
aria-pressed="true") when activeId === session.id and omitting it otherwise;
ensure the attribute is applied to the same button element that toggles via
onSelect so screen readers can identify the current/selected history item.
In `@README.md`:
- Around line 1-3: The README header currently reads "# Osmium CLI 💿" which
implies CLI-only; update the opening title and first sentence to mention both
the CLI and the new desktop app (e.g., change the header to include "CLI and
Desktop" and amend "Osmium is a music copyright checker" to reference both
install paths), and add a brief note/link indicating desktop release artifacts
are available so users know there is more than one install path.
In `@src/services/spotify.ts`:
- Around line 187-198: The searchTracks function never sends the limit to
Spotify; update the request URL built in searchTracks to include the limit query
parameter (e.g., append &limit=${encodeURIComponent(limit.toString())} or
similar) so the passed-in limit value is honored when calling fetchJSON; ensure
the change references the existing function name searchTracks and the fetchJSON
call where the URL is constructed (keep encodeURIComponent for query and use the
numeric limit value directly or converted to string).
In `@tmp/test2.ts`:
- Around line 5-23: The current run() function calls getConfig() and await
getSpotifyToken(...) before the inner try/catch, causing failures there to be
unhandled; move those calls inside the existing try block or wrap the entire
run() body in a top-level try/catch so that errors from getConfig and
getSpotifyToken are caught and logged; update the run function (referencing run,
getConfig, getSpotifyToken, and the existing try/catch around fetchJSON) to
ensure any thrown errors are handled consistently and logged in the catch.
---
Nitpick comments:
In `@convex/secrets.js`:
- Around line 1-20: The committed convex/secrets.js appears to be generated
build output (contains sourceMappingURL) and duplicates the TS source, causing
drift; remove convex/secrets.js from the repo and stop checking generated Convex
JS into the functions directory, ensure the original convex/secrets.ts (which
defines the getApiKeys query/handler) remains the single source of truth, add
convex/*.js (or the specific generated file pattern) to .gitignore, and update
your build/output configuration so generated Convex JS is written to a
build/dist directory instead of the convex/ source directory.
In `@desktop-app/src/main/index.ts`:
- Around line 59-81: Replace the duplicated local interfaces by importing the
shared types used by the renderer (do not keep PipelineEvent and
CopyrightVerdict inline); remove the local declarations for PipelineEvent and
CopyrightVerdict and import the canonical types from the shared renderer/store
types module so the main process uses the same shape (including
aiSummary.webSearchSources) and update any local uses to the imported names to
avoid payload drift.
In `@desktop-app/src/renderer/src/types.ts`:
- Around line 1-52: The renderer types have drifted from the canonical
main/shared contract—PipelineEvent, samples.riskLevel, and copyrightType are
broader here than in src/types/index.ts—so replace the local definitions with
the shared IPC contract: import and re-export (or reference) the canonical types
from the main/shared module (the definitions used around Lines 200-275 in
src/types/index.ts) instead of redefining them, ensuring PipelineEvent.name,
samples.riskLevel, and copyrightType use the exact union types from the shared
file so the renderer/main boundary preserves exhaustiveness and correct
type-checking.
In `@src/services/musicbrainz.ts`:
- Around line 48-52: Replace the unsafe double-cast used when reading
release/label data by defining a precise interface (e.g., MBRelease with fields
id, title, "label-info"?: Array<{label?:{name:string}}>, labelInfo?:
Array<{label?:{name:string}}>), use that type for primaryRecording.releases
items and for the local variable currently named release, and then read the
label name from the typed union (check release["label-info"] ??
release.labelInfo) instead of using as unknown as Record<string, unknown>;
update usages of primaryRecording and labelInfo to rely on the new MBRelease
type to preserve type safety and readability.
In `@src/services/spotify.ts`:
- Around line 176-182: The SpotifySearchResult interface is duplicated; extract
the definition into a shared types module (e.g., create a shared export like
SharedSpotifyTypes or types.ts) and export SpotifySearchResult from there, then
replace the local declaration in src/services/spotify.ts with an import of
SpotifySearchResult and update the renderer/preload code that currently defines
the same shape to import the shared type instead; ensure all references to
SpotifySearchResult (the interface name) in functions and signatures are updated
to the new shared import so the preload/renderer boundary uses the single
canonical type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ecd9e142-7484-4ed9-bf4d-d8a5fa6293d0
⛔ Files ignored due to path filters (13)
convex/_generated/api.d.tsis excluded by!**/_generated/**convex/_generated/api.jsis excluded by!**/_generated/**convex/_generated/dataModel.d.tsis excluded by!**/_generated/**convex/_generated/server.d.tsis excluded by!**/_generated/**convex/_generated/server.jsis excluded by!**/_generated/**convex/secrets.d.ts.mapis excluded by!**/*.mapconvex/secrets.js.mapis excluded by!**/*.mapdesktop-app/package-lock.jsonis excluded by!**/package-lock.jsondesktop-app/public/icon.pngis excluded by!**/*.pngdesktop-app/public/icon1.pngis excluded by!**/*.pngdesktop-app/public/icon_mac.pngis excluded by!**/*.pngicon.pngis excluded by!**/*.pngpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (42)
.github/workflows/release.yml.gitignoreREADME.mdconvex/README.mdconvex/secrets.d.tsconvex/secrets.jsconvex/secrets.tsconvex/tsconfig.jsondesktop-app/electron.vite.config.tsdesktop-app/out/main/index.jsdesktop-app/out/preload/index.jsdesktop-app/package.jsondesktop-app/postcss.config.jsdesktop-app/src/main/index.tsdesktop-app/src/preload/index.tsdesktop-app/src/preload/types.d.tsdesktop-app/src/renderer/index.htmldesktop-app/src/renderer/src/App.tsxdesktop-app/src/renderer/src/components/CheckView.tsxdesktop-app/src/renderer/src/components/HomePage.tsxdesktop-app/src/renderer/src/components/Sidebar.tsxdesktop-app/src/renderer/src/components/TitleBar.tsxdesktop-app/src/renderer/src/index.cssdesktop-app/src/renderer/src/main.tsxdesktop-app/src/renderer/src/types-window.d.tsdesktop-app/src/renderer/src/types.tsdesktop-app/tailwind.config.jsdesktop-app/tsconfig.jsondesktop-app/tsconfig.node.jsonpackage.jsonsrc/agents/summary-agent.tssrc/config/index.tssrc/index.tssrc/services/musicbrainz.tssrc/services/spotify.tssrc/types/index.tssrc/utils/ai-client.tssrc/utils/convex-client.tssrc/utils/history.tstmp/test2.tstmp/test_search.tstsconfig.json
Comment on lines
+10
to
+11
| permissions: | ||
| contents: write |
There was a problem hiding this comment.
Scope contents: write to the release jobs only.
These permissions apply to every job, including ci on every branch push. That gives install/build steps a write-capable GITHUB_TOKEN when they only need read access.
Proposed fix
-permissions:
- contents: write
+permissions:
+ contents: read
@@
release-cli:
name: Release CLI
needs: ci
if: startsWith(github.ref, 'refs/tags/v')
runs-on: ubuntu-latest
+ permissions:
+ contents: write
@@
release-desktop:
name: Desktop (${{ matrix.os }})
needs: ci
if: startsWith(github.ref, 'refs/tags/v')
runs-on: ${{ matrix.os }}
+ permissions:
+ contents: write🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yml around lines 10 - 11, The workflow currently
sets top-level permissions "contents: write" which grants write access to
GITHUB_TOKEN for all jobs (including "ci"); change this so the default top-level
permissions are read-only (e.g., "contents: read") and then explicitly scope
"contents: write" under only the release job by adding a permissions block
inside the "release" job; update the "ci" job (and any other non-release jobs)
to rely on the default read-only permission so only the "release" job receives
write access.
Comment on lines
+34
to
+35
| - name: Typecheck | ||
| run: npm run typecheck |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== root package.json scripts =="
jq '.scripts' package.json
echo "== desktop-app package.json scripts =="
jq '.scripts' desktop-app/package.json
echo "== desktop TypeScript configs =="
jq '{include, references, compilerOptions}' desktop-app/tsconfig.json desktop-app/tsconfig.node.jsonRepository: Renderdragonorg/Osmium
Length of output: 1254
🏁 Script executed:
wc -l .github/workflows/release.ymlRepository: Renderdragonorg/Osmium
Length of output: 99
🏁 Script executed:
sed -n '95,110p' .github/workflows/release.ymlRepository: Renderdragonorg/Osmium
Length of output: 555
🏁 Script executed:
cat -n desktop-app/tsconfig.json | head -20Repository: Renderdragonorg/Osmium
Length of output: 726
🏁 Script executed:
fd 'vite.config' desktop-appRepository: Renderdragonorg/Osmium
Length of output: 101
🏁 Script executed:
cat -n desktop-app/vite.config.ts 2>/dev/null || cat -n desktop-app/vite.config.js 2>/dev/null || echo "No vite config found"Repository: Renderdragonorg/Osmium
Length of output: 86
🏁 Script executed:
cat -n desktop-app/electron.vite.config.tsRepository: Renderdragonorg/Osmium
Length of output: 1184
🏁 Script executed:
cat -n .github/workflows/release.ymlRepository: Renderdragonorg/Osmium
Length of output: 3806
Add a typecheck step for the desktop app in the CI and desktop release workflows.
The root typecheck (lines 34-35) only validates the root project. The desktop-app has its own tsconfig.json with strict settings and a typecheck script (tsc --noEmit), but this is never invoked in the workflow. The npm run build step (line 105) uses electron-vite build, which is a bundler without type checking—TypeScript errors in the main, preload, or renderer code can ship without being caught.
Add - name: Typecheck Desktop / run: npm run typecheck step in the desktop-app directory for both the ci job (after the CLI typecheck) and the release-desktop job (before the build step at line 101).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yml around lines 34 - 35, Add a dedicated desktop
typecheck step that runs the desktop-app's strict TypeScript check by adding a
step named "Typecheck Desktop" that executes npm run typecheck with
working-directory set to desktop-app; insert this step in the ci job immediately
after the existing root/CLI "Typecheck" step and in the release-desktop job
immediately before the "build" step (the existing electron-vite build step), so
the desktop-app's tsconfig (and its tsc --noEmit script) is validated during CI
and release workflows.
Comment on lines
+1
to
+13
| export declare const getApiKeys: import("convex/server").RegisteredQuery<"public", {}, Promise<{ | ||
| spotifyClientId: string | undefined; | ||
| spotifyClientSecret: string | undefined; | ||
| openrouterApiKey: string | undefined; | ||
| openrouterModel: string; | ||
| nocodeSpotifyCloudName: string | undefined; | ||
| nocodeSpotifyToken: string | undefined; | ||
| tavilyApiKey: string | undefined; | ||
| acoustidApiKey: string | undefined; | ||
| groqApiKey: string | undefined; | ||
| groqModel: string; | ||
| discogsToken: string | undefined; | ||
| }>>; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'export const getApiKeys|query\(|SPOTIFY_CLIENT_SECRET|OPENROUTER_API_KEY|GROQ_API_KEY|client\.query\(api\.secrets\.getApiKeys' \
convex/secrets.ts \
convex/secrets.d.ts \
src/utils/convex-client.tsRepository: Renderdragonorg/Osmium
Length of output: 1516
🏁 Script executed:
cat -n convex/secrets.tsRepository: Renderdragonorg/Osmium
Length of output: 1067
🏁 Script executed:
rg -i 'auth|access|public|private' convex/ -t ts -t js --max-count=20 | head -50Repository: Renderdragonorg/Osmium
Length of output: 4576
This public Convex endpoint is returning secrets to clients.
query({...}) creates a public, client-accessible endpoint, and it returns raw process.env.* values including SPOTIFY_CLIENT_SECRET, OPENROUTER_API_KEY, GROQ_API_KEY, and DISCOGS_TOKEN. The client calls it directly at src/utils/convex-client.ts Line 26. This turns the deployment into a secret-exfiltration endpoint. Use internalQuery() for server-only access or return only non-sensitive config values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@convex/secrets.d.ts` around lines 1 - 13, The public Convex query getApiKeys
is exposing sensitive process.env values to clients; change it to a server-only
endpoint by using internalQuery()/RegisteredAction or the server-only
registration equivalent instead of query/RegisteredQuery<"public">, and update
any callers (e.g., where the client currently calls getApiKeys) to use
server-side invocation; alternatively, if some non-secret config must be
returned, remove all secret fields (SPOTIFY_CLIENT_SECRET, OPENROUTER_API_KEY,
GROQ_API_KEY, DISCOGS_TOKEN, etc.) from the returned object and only return
safe, non-sensitive values (or return masked/boolean flags) from getApiKeys to
prevent secret exfiltration.
Comment on lines
+3
to
+18
| export const getApiKeys = query({ | ||
| args: {}, | ||
| handler: async () => { | ||
| return { | ||
| spotifyClientId: process.env.SPOTIFY_CLIENT_ID, | ||
| spotifyClientSecret: process.env.SPOTIFY_CLIENT_SECRET, | ||
| openrouterApiKey: process.env.OPENROUTER_API_KEY, | ||
| openrouterModel: process.env.OPENROUTER_MODEL || "z-ai/glm-4.5-air:free", | ||
| nocodeSpotifyCloudName: process.env.NOCODE_SPOTIFY_CLOUD_NAME, | ||
| nocodeSpotifyToken: process.env.NOCODE_SPOTIFY_TOKEN, | ||
| tavilyApiKey: process.env.TAVILY_API_KEY, | ||
| acoustidApiKey: process.env.ACOUSTID_API_KEY, | ||
| groqApiKey: process.env.GROQ_API_KEY, | ||
| groqModel: process.env.GROQ_MODEL || "qwen/qwen3-32b", | ||
| discogsToken: process.env.DISCOGS_TOKEN, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the actual convex/secrets.ts file
find . -type f -name "secrets.ts" | head -20Repository: Renderdragonorg/Osmium
Length of output: 85
🏁 Script executed:
# List files in the convex directory to understand structure
git ls-files convex/ | head -30Repository: Renderdragonorg/Osmium
Length of output: 348
🏁 Script executed:
# Search for any related JavaScript mirror file
find . -type f -name "secrets.js"Repository: Renderdragonorg/Osmium
Length of output: 85
🏁 Script executed:
# Search for usages of getApiKeys to understand how it's called
rg "getApiKeys" -A 2 -B 2Repository: Renderdragonorg/Osmium
Length of output: 1055
🏁 Script executed:
# Check if there's a convex.json or similar config
git ls-files | grep -E "convex\.(json|config)"Repository: Renderdragonorg/Osmium
Length of output: 48
🏁 Script executed:
# Read the full convex/secrets.ts file
cat -n convex/secrets.tsRepository: Renderdragonorg/Osmium
Length of output: 1067
🏁 Script executed:
# Read the convex-client.ts file to see how getApiKeys is used
cat -n src/utils/convex-client.tsRepository: Renderdragonorg/Osmium
Length of output: 2960
🏁 Script executed:
# Check the generated server.js to understand Convex API
head -50 convex/_generated/server.jsRepository: Renderdragonorg/Osmium
Length of output: 1720
🏁 Script executed:
# Look for any environment variable documentation or README
cat -n convex/README.mdRepository: Renderdragonorg/Osmium
Length of output: 3233
🏁 Script executed:
# Check .env files or documentation about which vars are secrets
find . -type f \( -name ".env*" -o -name "*.md" \) | xargs grep -l "SPOTIFY_CLIENT_SECRET\|OPENROUTER_API_KEY\|GROQ_API_KEY" 2>/dev/null | head -10Repository: Renderdragonorg/Osmium
Length of output: 93
🏁 Script executed:
# Search for any middleware or access control on the getApiKeys query
rg "getApiKeys|internalQuery" -B 3 -A 3Repository: Renderdragonorg/Osmium
Length of output: 2610
🏁 Script executed:
# Check if there's any custom auth middleware in Convex
find convex -type f \( -name "*.ts" -o -name "*.js" \) | xargs grep -l "auth\|isAuthenticated\|checkAuth" 2>/dev/nullRepository: Renderdragonorg/Osmium
Length of output: 50
🏁 Script executed:
# Look at the project structure to understand if this is browser or server context
cat -n src/utils/convex-client.ts | head -30Repository: Renderdragonorg/Osmium
Length of output: 1154
🏁 Script executed:
# Search for where fetchSecretsFromConvex is called
rg "fetchSecretsFromConvex" -B 2 -A 5Repository: Renderdragonorg/Osmium
Length of output: 1346
🏁 Script executed:
# Check if there's environment variable filtering or Convex security documentation
find . -type f \( -name "convex.json" -o -name "convex.config.ts" \) -exec cat {} \;Repository: Renderdragonorg/Osmium
Length of output: 48
🏁 Script executed:
# Check if there's any authentication or access control on Convex functions
rg "auth|permission|isAdmin|getIdentity" convex/ -A 2 -B 2Repository: Renderdragonorg/Osmium
Length of output: 392
🏁 Script executed:
# Look at the project readme to understand what this tool is
head -50 README.mdRepository: Renderdragonorg/Osmium
Length of output: 2056
🏁 Script executed:
# Check if CONVEX_URL or API keys are public or private
rg "CONVEX_URL|convex\\.dev" -B 2 -A 2Repository: Renderdragonorg/Osmium
Length of output: 3666
🏁 Script executed:
# Verify if there's any difference between secrets.ts and secrets.js in terms of implementation
diff -u convex/secrets.js convex/secrets.tsRepository: Renderdragonorg/Osmium
Length of output: 434
🏁 Script executed:
# Verify that internalQuery exists and could be used as an alternative
grep -n "internalQuery" convex/_generated/server.d.ts | head -5Repository: Renderdragonorg/Osmium
Length of output: 141
🏁 Script executed:
# Check if there are any other secrets or internal functions to show pattern of correct usage
find convex -name "*.ts" -type f | xargs grep -l "internalQuery\|internalMutation" 2>/dev/nullRepository: Renderdragonorg/Osmium
Length of output: 95
Do not return deployment secrets from a public Convex query.
query() creates a client-callable function, so returning spotifyClientSecret, openrouterApiKey, groqApiKey, tavilyApiKey, acoustidApiKey, nocodeSpotifyToken, and discogsToken exposes credentials to every client caller. Even in Electron, anything returned to the renderer is user-extractable. Use internalQuery() instead, which is server-only and not accessible from clients. Keep secret-backed API calls server-side and only return non-sensitive derived data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@convex/secrets.ts` around lines 3 - 18, The handler getApiKeys is currently
exposed via query() and returns secrets; change its declaration to use
internalQuery() so it becomes server-only, remove any sensitive fields from the
returned object (spotifyClientSecret, openrouterApiKey, groqApiKey,
tavilyApiKey, acoustidApiKey, nocodeSpotifyToken, discogsToken) and only return
non-secret configuration values (e.g., openrouterModel, groqModel,
nocodeSpotifyCloudName) or move secret-using calls entirely server-side; update
any callers of getApiKeys to call the new internal-only function from server
code or to use a new server-side function that performs secret-backed API calls
rather than exposing raw credentials.
Comment on lines
+9
to
+10
| shell: { | ||
| openExternal: (url) => electron.ipcRenderer.invoke("shell:open-external", url) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "shell:open-external|openExternal\\(" -C 3Repository: Renderdragonorg/Osmium
Length of output: 3685
The shell:open-external IPC handler must validate URL schemes and reject unsafe protocols.
The handler at desktop-app/src/main/index.ts:142-144 passes arbitrary URL strings directly to shell.openExternal()without any validation. This allows a compromised renderer to openfile://, mailto:`, custom protocols, or any other URL scheme the OS handles. URLs come from user-controlled sources (AI summaries at line 185, external sources at line 212), multiplying the attack surface.
Add scheme whitelisting (e.g., http, https) or reject dangerous schemes before calling shell.openExternal().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@desktop-app/out/preload/index.js` around lines 9 - 10, The IPC handler for
"shell:open-external" currently passes arbitrary URLs straight to
shell.openExternal; update the handler in main (the "shell:open-external" IPC
callback in index.ts) to validate the incoming URL by parsing its scheme and
only allow a whitelist (e.g., "http" and "https"); if the scheme is not allowed,
reject/throw and do not call shell.openExternal, and return/report a clear error
to the renderer. Ensure validation uses a proper URL parser and normalize input
(reject empty or relative strings) before calling shell.openExternal, and keep
the IPC channel name "shell:open-external" unchanged.
Comment on lines
+66
to
82
| let licensingUrl: string | null = null; | ||
| if (parsed.licensingUrl) { | ||
| try { | ||
| const trimmed = parsed.licensingUrl.trim(); | ||
| const url = new URL(trimmed); | ||
| if (url.protocol === 'https:') { | ||
| licensingUrl = trimmed; | ||
| } | ||
| } catch { | ||
| // Invalid URL, keep as null | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| ...parsed, | ||
| licensingUrl: licensingUrl ?? undefined, | ||
| webSearchSources: webSearchResults, |
There was a problem hiding this comment.
Don't promote a model-supplied licensing URL unless it matches fetched evidence.
This only enforces https:. A hallucinated URL will still be emitted in AISummary even if it never appeared in webSearchResults, which defeats the purpose of sourcing it from the fetched evidence. Validate the normalized URL against the URLs/origins you already searched before returning it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agents/summary-agent.ts` around lines 66 - 82, The current logic accepts
any HTTPS URL from parsed.licensingUrl and returns it in the AISummary even if
it never appears in fetched evidence; change the validation so licensingUrl is
only returned when the normalized parsed.licensingUrl exactly matches (or has
the same origin as) at least one URL from webSearchResults. In the block around
licensingUrl (refer to parsed.licensingUrl, licensingUrl, and webSearchResults),
parse and normalize parsed.licensingUrl, then iterate webSearchResults entries
and compare either url.href equality or url.origin equality against the
normalized value; only assign licensingUrl when a match is found, otherwise keep
it null/undefined. Ensure any URL parsing errors still fall back to null.
Comment on lines
+35
to
+40
| model: process.env.OPENROUTER_MODEL || "z-ai/glm-4.5-air:free", | ||
| baseURL: "https://openrouter.ai/api/v1", | ||
| defaultHeaders: { | ||
| "HTTP-Referer": "https://github.com/osmium-cli/osmium", | ||
| "X-OpenRouter-Title": "Osmium CLI" | ||
| } |
There was a problem hiding this comment.
Unify the sync and async config builders before caching them together.
buildConfigFromEnv() and validateConfigAsync() already disagree on the fallback openrouter.model, and getConfig() / getConfigAsync() share the same _config cache. That means the first caller permanently decides both the default model and whether Convex secrets are ever loaded for the rest of the process.
🧭 Suggested direction
+const DEFAULT_OPENROUTER_MODEL = "z-ai/glm-4.5-air:free";
+
let _config: AppConfig | null = null;
+let _asyncConfig: Promise<AppConfig> | null = null;
@@
openrouter: {
apiKey: process.env.OPENROUTER_API_KEY!,
- model: process.env.OPENROUTER_MODEL || "z-ai/glm-4.5-air:free",
+ model: process.env.OPENROUTER_MODEL || DEFAULT_OPENROUTER_MODEL,
baseURL: "https://openrouter.ai/api/v1",
@@
openrouter: {
apiKey: process.env.OPENROUTER_API_KEY || "",
- model: process.env.OPENROUTER_MODEL || "openai/gpt-4o",
+ model: process.env.OPENROUTER_MODEL || DEFAULT_OPENROUTER_MODEL,
baseURL: "https://openrouter.ai/api/v1",
@@
export async function getConfigAsync(): Promise<AppConfig> {
- if (_config) return _config;
- _config = await validateConfigAsync();
- return _config;
+ if (_asyncConfig) return _asyncConfig;
+ _asyncConfig = validateConfigAsync().then((config) => {
+ _config = config;
+ return config;
+ });
+ return _asyncConfig;
}Also applies to: 113-120, 149-158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/config/index.ts` around lines 35 - 40, The sync and async config builders
disagree on defaults and share a single _config cache so the first caller locks
in both the openrouter.model fallback and whether Convex secrets were loaded;
fix by centralizing config construction so both getConfig() and getConfigAsync()
use the same builder: have buildConfigFromEnv() produce a plain config object
(without applying async Convex lookups or differing model defaults) and then
have a single async initializer (e.g., validateConfigAsync or a new
initConfigAsync) that applies the final openrouter.model fallback and loads
Convex secrets before writing to the shared _config cache; ensure getConfig()
returns a config only if _config is already initialized (or throws), and
getConfigAsync() awaits the unified async initializer so the cache is always
populated consistently for future sync/async callers.
Comment on lines
+94
to
+95
| // Save to history | ||
| await saveToHistory(verdict); |
There was a problem hiding this comment.
Don’t let history persistence turn successful checks into failures.
A saveToHistory() write error is currently treated like a pipeline failure. In runCheck, check/search will exit non-zero after a verdict was already produced; in playlist mode the verdict is pushed before the write, so the summary can count the track while the console prints [ERROR].
🛠️ Suggested fix
- await saveToHistory(verdict);
+ try {
+ await saveToHistory(verdict);
+ } catch (error) {
+ logger.debug(
+ `Failed to persist history: ${
+ error instanceof Error ? error.message : String(error)
+ }`
+ );
+ }
@@
- await saveToHistory(verdict);
+ try {
+ await saveToHistory(verdict);
+ } catch (error) {
+ logger.debug(
+ `Failed to persist history: ${
+ error instanceof Error ? error.message : String(error)
+ }`
+ );
+ }Also applies to: 200-202
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.ts` around lines 94 - 95, Wrap calls to saveToHistory in a
try/catch so write failures are logged but do not change the check result or
pipeline exit status; specifically, in runCheck (where await
saveToHistory(verdict) is called) and the other occurrence around lines
referenced (200-202), catch any error, log it with the existing logger (e.g.,
processLogger or logger.error) including the error details and context, and do
not rethrow or return a non-zero status—ensure the original verdict is preserved
and returned/pushed as before.
Comment on lines
+33
to
+36
| try { | ||
| const data = await readFile(storePath, "utf-8"); | ||
| const parsed = JSON.parse(data) as StoreData; | ||
| return parsed.checks || []; |
There was a problem hiding this comment.
Validate the persisted shape before returning it.
JSON.parse only proves the file is syntactically valid. If checks is a string/object, loadHistory() returns it as-is and Line 51 later throws on [verdict, ...existing]. Guard with Array.isArray(parsed.checks) and fall back to [].
🔧 Suggested fix
- const parsed = JSON.parse(data) as StoreData;
- return parsed.checks || [];
+ const parsed = JSON.parse(data) as Partial<StoreData>;
+ return Array.isArray(parsed.checks) ? parsed.checks : [];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try { | |
| const data = await readFile(storePath, "utf-8"); | |
| const parsed = JSON.parse(data) as StoreData; | |
| return parsed.checks || []; | |
| try { | |
| const data = await readFile(storePath, "utf-8"); | |
| const parsed = JSON.parse(data) as Partial<StoreData>; | |
| return Array.isArray(parsed.checks) ? parsed.checks : []; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/history.ts` around lines 33 - 36, The loadHistory function
currently returns parsed.checks directly after JSON.parse which can be non-array
and later break code; update loadHistory to validate parsed.checks with
Array.isArray(parsed.checks) and only return it when it's an array, otherwise
return an empty array as a fallback (e.g., const checks =
Array.isArray(parsed.checks) ? parsed.checks : []; return checks;). Ensure you
reference the parsed variable and the loadHistory function in your change so
callers (where [verdict, ...existing] is used) always receive an array.
Comment on lines
8
to
+9
| const token = await getSpotifyToken(config.spotify.clientId, config.spotify.clientSecret); | ||
| console.log("Token:", token.substring(0, 5) + "..."); |
There was a problem hiding this comment.
Do not print bearer tokens to stdout.
Line 9 logs part of a live access token. Even partial secrets tend to end up in terminal history, screenshots, and CI logs.
Proposed fix
const config = getConfig();
const token = await getSpotifyToken(config.spotify.clientId, config.spotify.clientSecret);
- console.log("Token:", token.substring(0, 5) + "...");
+ console.log("Token acquired");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const token = await getSpotifyToken(config.spotify.clientId, config.spotify.clientSecret); | |
| console.log("Token:", token.substring(0, 5) + "..."); | |
| const config = getConfig(); | |
| const token = await getSpotifyToken(config.spotify.clientId, config.spotify.clientSecret); | |
| console.log("Token acquired"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tmp/test2.ts` around lines 8 - 9, The console.log that prints the partial
bearer token ("Token:", token.substring(...)) must be removed to avoid exposing
secrets; in the code path that calls getSpotifyToken ensure you do not output
the token variable to stdout or logs—either drop the console.log entirely or
replace it with a non-sensitive status message (e.g., "Spotify token acquired"
or log only token length) and keep the actual token confined to secure
variables/usage in the token variable.
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 by CodeRabbit
New Features
Chores