fix: preserve CLI arguments and isolate AppleScript input - #79
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: blocked before merge. Reviewed September 13, 2026, 2:35 AM ET / 06:35 UTC. ClawSweeper reviewWhat this changesPreserves literal CLI arguments and supplied output writers, expands Spotify share-URL parsing, rejects malformed resources, and passes playback URIs to AppleScript as data. Merge readiness⛔ Blocked before merge - 1 item remains This PR remains useful: current main retains the reported defects, and the related merged PR addresses different behavior. No concrete introduced correctness defect was found; collaborator-authored work also remains protected from automatic closure. Priority: P2 Review scores
Verification
How this fits togetherSpogo turns terminal commands into Spotify searches and playback requests. Resource parsing feeds its playback engines, including a macOS adapter that controls Spotify.app through AppleScript. flowchart TD
A[Terminal arguments] --> B[CLI parser]
B --> C[Search requests]
B --> D[Spotify resource validation]
D --> E[Remote playback engine]
D --> F[AppleScript argument transport]
F --> G[Local Spotify app]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep the documented strict resource boundary while preserving valid existing inputs, literal arguments, and AppleScript data isolation. Do we have a high-confidence way to reproduce the issue? Yes, source establishes the current-main mechanisms: literal --no-input is moved across --, and playback URIs enter AppleScript source. No runtime reproduction was executed during this read-only review. Is this the best way to solve the issue? Yes. Using Kong directly and passing AppleScript arguments separately removes the problematic transformations; resource validation preserves documented valid forms and explicitly records the compatibility change. AGENTS.md: not found in the target repository. Codex review notes: model internal, reasoning medium; reviewed against 5d3585930863. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
A pre-parser moved every
--no-inputtoken to the front, including literal text after--. Remove that obsolete path: current Kong already accepts the flag before or after commands. Route results through the writers supplied to the CLI runner so command-level tests observe the real output.Parse complete Spotify resources, including locale-prefixed and embedded share URLs and case-insensitive schemes. Reject missing identifiers, extra URI/path segments, unrelated URL hosts, and userinfo instead of truncating or misidentifying resources; this validation compatibility change is recorded in the changelog.
AppleScript playback previously interpolated a URI into script source. Pass it through
osascript --arguments to a fixed script so quotes, backslashes, and newlines remain data.Validation: new command/resource/AppleScript regressions failed before the fix. The implementation-only argument-reordering assertion is replaced by stronger command-level cases for both flag placements and literal
--no-input. All eight packages pass under the race detector; targeted tests pass on Go 1.26.7; lint/deadcode and docs build pass. Isolated Codex autoreview is scoped-clean through P2. An earlier HTTP-scheme finding was rejected after executable Go-floor probes confirmednet/urlalready normalizes schemes; the regression cases are retained. Rebase onto #78 changed only the changelog merge; all reviewed code/test/doc files were byte-identical.Live proof: a built CLI sends
search track -- --no-inputunchanged in one synthetic HTTPS request; bothauth pasteflag placements still work. It passes quoted playback URIs as separate arguments and normalizes uppercase/localized/embedded resources. A native/usr/bin/osascriptround-trip verifies argument transport without controlling Spotify. Synthetic HTTP proof uses only a temporary CA trust overlay; playback uses an argument-capturing stub. No real account or playback was changed.