Skip to content

fix: preserve CLI arguments and isolate AppleScript input - #79

Merged
steipete merged 1 commit into
mainfrom
fix/phase-five-inputs
Sep 13, 2026
Merged

fix: preserve CLI arguments and isolate AppleScript input#79
steipete merged 1 commit into
mainfrom
fix/phase-five-inputs

Conversation

@steipete

Copy link
Copy Markdown
Collaborator

A pre-parser moved every --no-input token 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 confirmed net/url already 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-input unchanged in one synthetic HTTPS request; both auth paste flag placements still work. It passes quoted playback URIs as separate arguments and normalizes uppercase/localized/embedded resources. A native /usr/bin/osascript round-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.

@clawsweeper

clawsweeper Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 13, 2026
@clawsweeper

clawsweeper Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Codex review: blocked before merge. Reviewed September 13, 2026, 2:35 AM ET / 06:35 UTC.

ClawSweeper review

What this changes

Preserves 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
Reviewed head: 85bfa17e43373978ef3c03102161ffb139deaa52

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused implementation with useful regression coverage and no identified blocking defect; the deliberate parsing compatibility change is disclosed.
Proof confidence 🌊 off-meta tidepool Not applicable: The collaborator exemption applies. The captured body reports CLI search transport and native osascript argument checks, with playback captured by a stub; actual Spotify playback is not demonstrated, and no new cross-principal authority requires exceptional proof.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The collaborator exemption applies. The captured body reports CLI search transport and native osascript argument checks, with playback captured by a stub; actual Spotify playback is not demonstrated, and no new cross-principal authority requires exceptional proof.
Evidence reviewed 8 items Repository and policy boundary: The origin remote identifies openclaw/spogo. Root and nested AGENTS.md searches found no policy files; no maintainer-note files were found. The final working-tree status was clean.
Current main still reorders literal arguments: Main calls normalizeArgs before Kong parsing; that helper moves every --no-input token without respecting the -- delimiter. The PR removes this preprocessing and tests literal text plus both global-flag placements.
Playback data isolation: Main interpolates the URI into executable AppleScript source. The introduced implementation uses fixed source and a separate argv value, including an option terminator; its regression test checks exact argument boundaries with quotes, backslashes, and a newline.
Findings None None.
Security None None.

How this fits together

Spogo 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]
Loading

Before merge

  • Resolve merge risk (P1) - Existing scripts that relied on silently truncating malformed Spotify resources will now receive errors; the extent of that reliance is unknown, although the compatibility change is explicitly acknowledged in the PR.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta production +38/-50 (net -12); tests +116/-10 (net +106) The implementation shrinks while adding command-level argument, resource-validation, and AppleScript transport coverage.

Merge-risk options

Maintainer options:

  1. Accept the documented malformed-input rejection (recommended)
    Retain the explicitly acknowledged validation change, with affected automation corrected to supply complete Spotify resources.

Technical review

Best 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.

Labels

Label changes:

  • add P2: This is a bounded correction to CLI input handling and local playback with no demonstrated widespread outage.
  • add merge-risk: 🚨 compatibility: Previously truncated malformed resources now fail, which can stop automation relying on that permissive behavior.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The collaborator exemption applies. The captured body reports CLI search transport and native osascript argument checks, with playback captured by a stub; actual Spotify playback is not demonstrated, and no new cross-principal authority requires exceptional proof.

Label justifications:

  • P2: This is a bounded correction to CLI input handling and local playback with no demonstrated widespread outage.
  • merge-risk: 🚨 compatibility: Previously truncated malformed resources now fail, which can stop automation relying on that permissive behavior.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The collaborator exemption applies. The captured body reports CLI search transport and native osascript argument checks, with playback captured by a stub; actual Spotify playback is not demonstrated, and no new cross-principal authority requires exceptional proof.

Evidence

What I checked:

  • Repository and policy boundary: The origin remote identifies openclaw/spogo. Root and nested AGENTS.md searches found no policy files; no maintainer-note files were found. The final working-tree status was clean. (85bfa17e4337)
  • Current main still reorders literal arguments: Main calls normalizeArgs before Kong parsing; that helper moves every --no-input token without respecting the -- delimiter. The PR removes this preprocessing and tests literal text plus both global-flag placements. (cmd/spogo/main.go:41, 5d3585930863)
  • Playback data isolation: Main interpolates the URI into executable AppleScript source. The introduced implementation uses fixed source and a separate argv value, including an option terminator; its regression test checks exact argument boundaries with quotes, backslashes, and a newline. (internal/spotify/applescript.go:28, 5d3585930863)
  • Compatibility change is explicit: The new parser rejects extra URI/path segments, userinfo, unrelated URL hosts, and missing identifiers. Ordinary URIs, URLs, and bare IDs retain regression coverage. The collaborator-authored body and CHANGELOG explicitly describe the rejection change. (internal/spotify/parse.go:33, 85bfa17e4337)
  • Release comparison: The v0.12.0 parser retains the old truncating URI/path behavior, as does fetched main. The requested parsing changes are not already present in either inspected version. (internal/spotify/parse.go:26, 64ea6efc2dec)
  • Execution context remains initialized: NewContext already initializes commandCtx with context.Background(), so removing the redundant setter does not remove the command context. Assigning the supplied output writers preserves the existing writer configuration. (internal/app/context_init.go:31, 85bfa17e4337)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Adam Holt: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@steipete
steipete merged commit 3e084f4 into main Sep 13, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant