Conversation
rianjs
changed the title
![rianjs-bot[bot]](https://avatars.githubusercontent.com/u/7378790?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: faa52f8075e9
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| frontend:view-state | 0 |
| rust:implementation-tests | 1 |
| tauri:config-ipc | 0 |
| security:credential-boundary | 1 |
rust:implementation-tests (1 finding)
Minor - apps/desktop/src-tauri/src/spotify_commands.rs:265
The new Rust error translation has no behavioral Rust test. The added UI test injects the final
operationErrorstring directly, so it would still pass ifrun_sync_loopreturned the original provider/persistence error instead. Extract the mapping into a small helper (or otherwise make it unit-testable) and assert that a failed sync maps toSPOTIFY_SYNC_ERROR, while success remains unchanged.
security:credential-boundary (1 finding)
Blocking - apps/desktop/src-tauri/src/spotify_commands.rs:266
This newly logs the raw sync error. Sync propagates
retune_spotify::Error, whoseHttpvariant embeds the full remote response body (and transport/OAuth failures can similarly contain uncontrolled diagnostic text). A proxy or endpoint can therefore place an access/refresh credential or other sensitive material in that body and have it written to application logs. Log only a typed/redacted error category plus safe endpoint/status metadata; retain the generic user error. Add a canary test proving a response-body value never reaches the log path.
Reviewer Coverage
frontend:view-state— complete (constrained); inspected 1 assigned file (2 inspected across reviewers):apps/desktop/test/interactions.test.tsx; skipped: none; constraints: Review limited to the assigned interaction-test file; dependencies were not installed, so the focused Vitest run could not execute.rust:implementation-tests— complete (broad); inspected 1 assigned file (2 inspected across reviewers):apps/desktop/src-tauri/src/spotify_commands.rs; skipped: none; constraints: Focused on the assigned Rust file and its direct sync callers/tests.cargo test -p retune-desktop spotify_commands::tests --libcould not build because the environment's clang could not create a temporary file.tauri:config-ipc— complete (constrained); skipped: none; constraints: Review intentionally limited to the two assigned changed files and their IPC/event call paths. Targeted UI tests could not run because the checkout lacks the required @tauri-apps/api npm dependency.security:credential-boundary— complete (constrained); inspected 1 assigned file (2 inspected across reviewers):apps/desktop/src-tauri/src/spotify_commands.rs; skipped: none; constraints: Review limited to the assigned changed Spotify command and its credential/error propagation dependencies.unassigned—⚠️ unassigned; skipped:apps/desktop/src/App.css,apps/desktop/src/App.tsx,apps/desktop/src/LastFmImporter.tsx,apps/desktop/src/appState.ts,apps/desktop/src/dialogViews.tsx,apps/desktop/src/lastfmImporter.css,apps/desktop/src/spotifySearch.ts,apps/desktop/src/spotifyViews.tsx,apps/desktop/src/trackDecisionDialogs.tsx,apps/desktop/src/viewShared.tsx; changed files were not assigned to a selected reviewer
Inspected files (2)
apps/desktop/src-tauri/src/spotify_commands.rsapps/desktop/test/interactions.test.tsx
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 3m 38s | gpt-5.6-terra | cr 0.10.311
| Field | Value |
|---|---|
| Model | gpt-5.6-terra |
| Reviewers | frontend:view-state, rust:implementation-tests, tauri:config-ipc, security:credential-boundary |
| Engine | codex_cli · gpt-5.6-terra |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 3m 38s wall · 7m 56s compute |
| Cost | unavailable |
| Tokens | 1.3M in / 15.9k out |
Per-workstream usage
orchestrator-selection— gpt-5.6-terra- In: 16.5k
- Out: 459
- Cache read: 11.0k
- Cache create: unavailable
- Cost: unavailable
- Duration: 14s
frontend:view-state— gpt-5.6-terra- In: 376.5k
- Out: 3.6k
- Cache read: 329.5k
- Cache create: unavailable
- Cost: unavailable
- Duration: 2m 57s
rust:implementation-tests— gpt-5.6-terra- In: 361.8k
- Out: 3.7k
- Cache read: 309.0k
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 48s
tauri:config-ipc— gpt-5.6-terra- In: 350.8k
- Out: 5.5k
- Cache read: 269.6k
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 59s
security:credential-boundary— gpt-5.6-terra- In: 132.6k
- Out: 2.0k
- Cache read: 94.2k
- Cache create: unavailable
- Cost: unavailable
- Duration: 48s
orchestrator-rollup— gpt-5.6-terra- In: 35.5k
- Out: 586
- Cache read: 22.0k
- Cache create: unavailable
- Cost: unavailable
- Duration: 7s
| schedule_auto_resume(app, deadline); | ||
| } | ||
| return result.map(|_| ()); | ||
| return result.map(|_| ()).map_err(|error| { |
There was a problem hiding this comment.
The new Rust error translation has no behavioral Rust test. The added UI test injects the final operationError string directly, so it would still pass if run_sync_loop returned the original provider/persistence error instead. Extract the mapping into a small helper (or otherwise make it unit-testable) and assert that a failed sync maps to SPOTIFY_SYNC_ERROR, while success remains unchanged.
Reply inline to this comment.
There was a problem hiding this comment.
Fixed - added a pure Rust sync-result translation test covering success and mapping provider failures to SPOTIFY_SYNC_ERROR.
There was a problem hiding this comment.
Thanks—this adds the missing Rust-level proof of the translation boundary for both success and provider-failure paths.
Summary:
Pure Rust tests now verify successful sync results remain unchanged and provider failures map to SPOTIFY_SYNC_ERROR.
| } | ||
| return result.map(|_| ()); | ||
| return result.map(|_| ()).map_err(|error| { | ||
| log::error!("Spotify sync failed: {error}"); |
There was a problem hiding this comment.
This newly logs the raw sync error. Sync propagates retune_spotify::Error, whose Http variant embeds the full remote response body (and transport/OAuth failures can similarly contain uncontrolled diagnostic text). A proxy or endpoint can therefore place an access/refresh credential or other sensitive material in that body and have it written to application logs. Log only a typed/redacted error category plus safe endpoint/status metadata; retain the generic user error. Add a canary test proving a response-body value never reaches the log path.
Reply inline to this comment.
There was a problem hiding this comment.
Fixed - sync failure logs now use the closed spotify_sync_failed category; provider errors and response bodies are never interpolated. Added a canary assertion.
There was a problem hiding this comment.
Thanks—this addresses the finding. The closed category and canary prevent provider-controlled diagnostic text from reaching logs.
Summary:
Sync failures now log only the closed spotify_sync_failed category, with a canary assertion guarding against provider response-body leakage.
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: f455ce8cbe67
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| architecture:seams | 2 |
| frontend:view-state | 0 |
| rust:implementation-tests | 0 |
| tauri:config-ipc | 0 |
| security:credential-boundary | 0 |
architecture:seams (2 findings)
Major - apps/desktop/src/LastFmImporter.tsx:34
The importer/provider boundary is supposed to carry typed failures; display strings must not drive policy.
lastfmOperationErrorinstead regex-classifies raw exception text and appends any unrecognized detail to the UI. This couples React toreqwest/hyperwording and lets provider or native diagnostics cross the boundary whenever their wording misses the regex. Use a closed generic message for unknown failures, or return a typed safe error DTO from the Last.fm gateway and render by its code; do not inspect or expose raw error strings here.
Minor - apps/desktop/src/App.tsx:194
Retune requires provider failures to remain typed through boundaries and forbids parsing display text into policy.
reportMainOperationErroridentifies a Spotify sync failure by exact equality with a TypeScript copy of the RustSPOTIFY_SYNC_ERRORsentence, giving error identity two owners. A wording change on either side will silently reclassify automatic sync failures as generic infrastructure failures and discard the cached-library/recovery guidance. Emit a closed operation code in the main event and let React map that code to copy instead of comparing human-readable messages.
Reviewer Coverage
architecture:seams— complete (broad); inspected 11 assigned files (12 inspected across reviewers):apps/desktop/src-tauri/src/spotify_commands.rs,apps/desktop/src/App.css,apps/desktop/src/App.tsx,apps/desktop/src/LastFmImporter.tsx,apps/desktop/src/appState.ts,apps/desktop/src/dialogViews.tsx,apps/desktop/src/lastfmImporter.css,apps/desktop/src/spotifySearch.ts,apps/desktop/src/spotifyViews.tsx,apps/desktop/src/trackDecisionDialogs.tsx,apps/desktop/src/viewShared.tsx; skipped: none; constraints: Narrow architecture review only; implementation, Tauri configuration, credential handling, and React view-state defects were left to their assigned reviewers. Review was static; tests were not executed.frontend:view-state— complete (constrained); inspected 1 assigned file (12 inspected across reviewers):apps/desktop/test/interactions.test.tsx; skipped: none; constraints: Review limited to the assigned UI interaction test file; related React sources were read only to validate test coverage context.rust:implementation-tests— complete (broad); inspected 1 assigned file (12 inspected across reviewers):apps/desktop/src-tauri/src/spotify_commands.rs; skipped: none; constraints: Review limited to the assigned changed Rust file and implementation/test concerns.tauri:config-ipc— complete (constrained); inspected 2 assigned files (12 inspected across reviewers):apps/desktop/src-tauri/src/spotify_commands.rs,apps/desktop/test/interactions.test.tsx; skipped: none; constraints: Focused Rust regression could not run because the sandboxed macOS toolchain could not create a clang temporary file while building ring. The focused Vitest interaction test completed without reported failures.security:credential-boundary— complete (constrained); inspected 1 assigned file (12 inspected across reviewers):apps/desktop/src-tauri/src/spotify_commands.rs; skipped: none; constraints: Credential-boundary review limited to the assigned changed Rust file. Targeted Cargo test could not run because the sandboxed toolchain could not create a clang temporary file.
Inspected files (12)
apps/desktop/src-tauri/src/spotify_commands.rsapps/desktop/src/App.cssapps/desktop/src/App.tsxapps/desktop/src/LastFmImporter.tsxapps/desktop/src/appState.tsapps/desktop/src/dialogViews.tsxapps/desktop/src/lastfmImporter.cssapps/desktop/src/spotifySearch.tsapps/desktop/src/spotifyViews.tsxapps/desktop/src/trackDecisionDialogs.tsxapps/desktop/src/viewShared.tsxapps/desktop/test/interactions.test.tsx
2 PR discussion threads considered. 2 summarized; 2 resolved.
Completed in 4m 11s | gpt-5.6-sol, gpt-5.6-terra | cr 0.10.311
| Field | Value |
|---|---|
| Model | gpt-5.6-sol, gpt-5.6-terra |
| Reviewers | architecture:seams, frontend:view-state, rust:implementation-tests, tauri:config-ipc, security:credential-boundary |
| Engine | codex_cli · gpt-5.6-sol, gpt-5.6-terra |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 4m 11s wall · 10m 28s compute |
| Cost | unavailable |
| Tokens | 1.8M in / 15.9k out |
Per-workstream usage
orchestrator-selection— gpt-5.6-terra- In: 16.8k
- Out: 622
- Cache read: 11.0k
- Cache create: unavailable
- Cost: unavailable
- Duration: 15s
architecture:seams— gpt-5.6-sol- In: 420.1k
- Out: 4.2k
- Cache read: 341.6k
- Cache create: unavailable
- Cost: unavailable
- Duration: 3m 00s
frontend:view-state— gpt-5.6-terra- In: 627.8k
- Out: 3.8k
- Cache read: 553.2k
- Cache create: unavailable
- Cost: unavailable
- Duration: 2m 58s
rust:implementation-tests— gpt-5.6-terra- In: 114.0k
- Out: 1.0k
- Cache read: 89.1k
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 00s
tauri:config-ipc— gpt-5.6-terra- In: 338.5k
- Out: 3.4k
- Cache read: 277.5k
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 47s
security:credential-boundary— gpt-5.6-terra- In: 233.6k
- Out: 1.9k
- Cache read: 193.8k
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 18s
orchestrator-rollup— gpt-5.6-terra- In: 79.1k
- Out: 987
- Cache read: 64.5k
- Cache create: unavailable
- Cost: unavailable
- Duration: 7s
| const [state, dispatch] = useReducer(reducer, initialState) | ||
| const fail = useCallback((error: unknown) => dispatch({ type: 'error', error: String(error) }), []) | ||
| const reportInfrastructureError = useCallback(() => dispatch({ type: 'error', error: INFRASTRUCTURE_ERROR }), []) | ||
| const reportMainOperationError = useCallback((error: string) => dispatch({ type: 'error', error: error === SPOTIFY_SYNC_ERROR ? error : INFRASTRUCTURE_ERROR }), []) |
There was a problem hiding this comment.
Retune requires provider failures to remain typed through boundaries and forbids parsing display text into policy. reportMainOperationError identifies a Spotify sync failure by exact equality with a TypeScript copy of the Rust SPOTIFY_SYNC_ERROR sentence, giving error identity two owners. A wording change on either side will silently reclassify automatic sync failures as generic infrastructure failures and discard the cached-library/recovery guidance. Emit a closed operation code in the main event and let React map that code to copy instead of comparing human-readable messages.
Reply inline to this comment.
There was a problem hiding this comment.
Fixed - automatic Spotify sync failures now use the closed payload-free spotifySyncError event; generic operation errors remain the safe infrastructure fallback, and the mounted test exercises the typed event.
| const invalidApplyResultMessage = 'Retune received an invalid Last.fm import result.' | ||
| type DisplayError = { message: string; code: ImportApplyErrorCode; retryAt: number | null } | ||
|
|
||
| function lastfmOperationError(reason: unknown) { |
There was a problem hiding this comment.
The importer/provider boundary is supposed to carry typed failures; display strings must not drive policy. lastfmOperationError instead regex-classifies raw exception text and appends any unrecognized detail to the UI. This couples React to reqwest/hyper wording and lets provider or native diagnostics cross the boundary whenever their wording misses the regex. Use a closed generic message for unknown failures, or return a typed safe error DTO from the Last.fm gateway and render by its code; do not inspect or expose raw error strings here.
Reply inline to this comment.
There was a problem hiding this comment.
Fixed - Last.fm transient operation and apply-failed alerts now use closed generic copy and ignore raw reasons; durable queue rows retain their persisted user-relevant failure detail.
This branch has not been deployed
1 participant
Summary
Validation
npm run test(95 Node tests; 86 Vitest passed, 6 skipped)npm run lintnpm run buildcargo fmt --all --checkcargo test -p retune-desktop(747 passed, 10 ignored)The motivating Zscaler/filtered-network condition was not available for reproduction; the React/native event and failure/recovery boundaries are covered by the existing harness. No transport workaround was added.