Skip to content

fix: preserve seek positions and paginated track ranks - #84

Merged
steipete merged 1 commit into
mainfrom
fix/cli-numeric-boundaries
Sep 15, 2026
Merged

steipete merged 1 commit into
mainfrom
fix/cli-numeric-boundaries

Conversation

@steipete

Copy link
Copy Markdown
Collaborator

Seek parsed mm:ss through nanosecond time.Duration arithmetic, so large valid positions wrapped into negative playback positions. It also accepted negative inputs and malformed seconds, sometimes reporting success after an engine silently clamped the value. Top-track output separately restarted ranks at 1 for every page, so --offset 20 mislabeled tracks 21 and 22 as 1 and 2.

Validate seek components and check the millisecond integer boundary before constructing a playback client. Convert and format durations with integer arithmetic so large representable values remain exact. Include Spotify's response offset in human/plain top-track ranks; JSON keys are unchanged. Update command/output docs and the Unreleased changelog.

Validation: new regressions failed before the fix and pass afterward, covering rejected inputs without playback, integer boundaries, duration formatting, and page ranks in both output formats. Built CLI checks intercept AppleScript with a synthetic executable: invalid inputs invoke no playback, 2:03 yields 123000 ms, and 153722868:00 yields 9223372080000 ms. Rank fixtures are synthetic; no live Spotify account or real playback device was used.

Full local race/coverage gate passes at 90.6%; lint, deadcode, and the docs build pass. Independent autoreview is clean through P2.

@clawsweeper

clawsweeper Bot commented Sep 15, 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.

@steipete
steipete merged commit 3d4edc2 into main Sep 15, 2026
11 checks passed
@steipete
steipete deleted the fix/cli-numeric-boundaries branch September 15, 2026 07:37
@clawsweeper clawsweeper Bot added 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. labels Sep 15, 2026
@clawsweeper

clawsweeper Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed September 15, 2026, 3:36 AM ET / 07:36 UTC.

ClawSweeper review

What this changes

The PR validates seek positions before initializing playback, preserves large millisecond values when parsing and formatting, and includes pagination offsets in displayed top-track ranks.

Merge readiness

✅ Ready for maintainer review

The fixes remain necessary on main and v0.13.0. No actionable introduced defect was found; this collaborator-authored PR is suitable for normal merge review.

Likely related people: steipete and hibachipapi are routing candidates based on prior area work.

Priority: P2
Reviewed head: a22ff680a380cd0115677156a20c39b303352509

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused, maintainable repair with useful boundary coverage and no identified blocking defect; runtime proof is exempt.
Proof confidence 🌊 off-meta tidepool Not applicable: The collaborator exemption applies. Reported built-CLI checks exercise seek parsing through a synthetic AppleScript executable, and ranking checks use fixtures; neither is represented as live Spotify proof. No material authority change requires additional 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. Reported built-CLI checks exercise seek parsing through a synthetic AppleScript executable, and ranking checks use fixtures; neither is represented as live Spotify proof. No material authority change requires additional proof.
Evidence reviewed 7 items Seek validation and arithmetic: The complete introduced patch validates before client construction, bounds seconds before multiplication, and checks the final integer boundary before converting minutes to milliseconds. Duration rendering uses integer division.
Pagination contract: The renderer now uses the result offset. The Web client preserves the response offset, and the Connect client returns its requested offset, so both production engines supply the existing result field consumed by this change.
Still needed on main: Pinned main still converts seek values through time.Duration, accepts negative positions, and calculates top-track ranks as i + 1. No equivalent supported configuration path was found.
Findings None None.
Security None None.

How this fits together

Spogo translates terminal commands into Spotify playback requests and formats Spotify results for terminal users. These changes affect seek input validation and human/plain output.

flowchart LR
  A[Seek argument] --> B[Validate and convert milliseconds]
  B --> C[Spotify playback engine]
  B --> D[Format seek confirmation]
  E[Spotify tracks and page offset] --> F[Calculate full-result ranks]
  F --> G[Human and plain output]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta Production +23/-13 (net +10); tests +90/-0 The small production increase is justified by numeric validation, with focused boundary and pagination regressions.

Technical review

Best possible solution:

Keep checked millisecond arithmetic and response-offset ranking within the existing CLI paths, preserving valid inputs and JSON output contracts.

Do we have a high-confidence way to reproduce the issue?

Yes, from source: main overflows duration arithmetic for large minute values and restarts ranks at 1 for a nonzero page offset. These paths were not executed during this review.

Is this the best way to solve the issue?

Yes. Checked integer arithmetic and reuse of the existing response offset directly repair the defects without introducing a parallel implementation or configuration surface.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning medium; reviewed against f00ba02a4b11.

Labels

Label changes:

  • add P2: This repairs bounded seek-input and pagination-output defects without evidence of an urgent widespread outage.
  • 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. Reported built-CLI checks exercise seek parsing through a synthetic AppleScript executable, and ranking checks use fixtures; neither is represented as live Spotify proof. No material authority change requires additional proof.

Label justifications:

  • P2: This repairs bounded seek-input and pagination-output defects without evidence of an urgent widespread outage.
  • 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. Reported built-CLI checks exercise seek parsing through a synthetic AppleScript executable, and ranking checks use fixtures; neither is represented as live Spotify proof. No material authority change requires additional proof.

Evidence

What I checked:

  • Seek validation and arithmetic: The complete introduced patch validates before client construction, bounds seconds before multiplication, and checks the final integer boundary before converting minutes to milliseconds. Duration rendering uses integer division. (internal/cli/playback.go:147, a22ff680a380)
  • Pagination contract: The renderer now uses the result offset. The Web client preserves the response offset, and the Connect client returns its requested offset, so both production engines supply the existing result field consumed by this change. (internal/cli/history.go:78, a22ff680a380)
  • Still needed on main: Pinned main still converts seek values through time.Duration, accepts negative positions, and calculates top-track ranks as i + 1. No equivalent supported configuration path was found. (internal/cli/playback.go:238, f00ba02a4b11)
  • Latest release remains affected: The v0.13.0 source retains duration-based seek conversion and page-local ranking; this release does not contain the proposed fixes. (internal/cli/playback.go:238, 34935e20659b)
  • Regression coverage and proof scope: The new tests cover rejected seek inputs, integer boundaries, large duration formatting, and ranks 21/22 in both output formats. The supplied PR body reports passing regression and synthetic AppleScript checks, explicitly without a live Spotify account or playback device. Tests were inspected, not executed during this read-only review. (internal/cli/numeric_test.go:16, a22ff680a380)
  • Prior area contributors: Path history identifies prior CLI rendering work by Peter Steinberger and listening-history work by Oscar; GitHub commit metadata resolves their handles to steipete and hibachipapi. These are routing signals, not claims of causal responsibility. (internal/cli/history.go, 243315d1e7c5)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • hibachipapi: 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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