fix: atomically replace cookie credentials

[steipete]

Cookie import and paste overwrote the existing file in place. An older file with mode 0644 retained those broad permissions after new credentials were written, and concurrent readers could see a truncated or partial JSON cookie jar.

Commit cookies through a synced, owner-only temporary file and atomic replacement. Share that writer with config and OAuth persistence, removing duplicated implementations while retaining bounded Windows reader retries, OAuth write-through replacement, and config locking. Cookie output symlinks keep their existing behavior. Newly created cookie directories use mode 0700.

Regression tests demonstrate both failures on the old implementation and cover fixed permissions, reader snapshots, symlink targets, complete replacements, cleanup after failures, and Windows readers. Documentation describes the storage guarantees. The shared implementation removes duplicated production code.

Validation: full Crabbox Linux race/coverage gate passed at 90.9%, lint reported zero issues, and dead-code, docs, and CLI build checks passed. A built-binary synthetic workflow verified auth paste changes an existing 0644 cookie file to 0600, preserves a custom symlink while replacing its target, and leaves auth status working. Independent review found no actionable P0–P2 defects; CI supplies the macOS, Windows, and Go-floor matrix.

[clawsweeper]

🦞👀
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]

Codex review: needs maintainer review before merge. Reviewed September 22, 2026, 8:22 AM ET / 12:22 UTC (Revision 2).

ClawSweeper review

What this changes

Replace cookie credentials atomically with private permissions and consolidate file replacement for cookies, configuration, and OAuth tokens.

Merge readiness

✅ Ready for maintainer review

The fix remains necessary on current main and v0.13.0. No actionable introduced defect was found; the focused regression coverage and preserved storage contracts support normal merge review.

Priority: P2
Reviewed head: 38738b7d5dc34d5d922afe022f7a868ab7c271a8

Review scores

Measure	Result	What it means
Overall readiness	🐚 platinum hermit (4/6)	A focused consolidation with meaningful regression coverage and no identified correctness or security blocker.
Proof confidence	🌊 off-meta tidepool	Not applicable: COLLABORATOR authorship exempts the ordinary contributor proof gate. The captured body reports a built-binary auth paste/status run through the changed cookie writer, including existing-file permissions and symlink preservation. Serialized data contracts are unchanged, so no data-model migration is required.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Not applicable	Not applicable: COLLABORATOR authorship exempts the ordinary contributor proof gate. The captured body reports a built-binary auth paste/status run through the changed cookie writer, including existing-file permissions and symlink preservation. Serialized data contracts are unchanged, so no data-model migration is required.
Evidence reviewed	8 items	Pinned introduced changes: Inspected the complete introduced delta from the pinned main/base to the original PR head, including platform implementations, tests, documentation, and removed writers. The host-verified test merge has the pinned main and head as its two parents. Current main still needs the fix: The cookie writer still calls os.WriteFile directly with mode 0600, which does not tighten permissions on an existing file and overwrites its contents in place. Latest release also retains the old writer: The v0.13.0 cookie writer contains the same direct os.WriteFile operation; this release does not already provide the proposed atomic cookie replacement.
Findings	None	None.
Security	None	None.

How this fits together

spogo stores imported Spotify cookies, configuration, and OAuth tokens locally for later CLI commands. The shared writer commits complete files while callers retain responsibility for serialization, paths, and locking.

flowchart TD
  A[Cookie import or paste] --> D[Shared private file writer]
  B[Configuration updates] --> D
  C[OAuth token saves] --> D
  D --> E[Write and sync temporary file]
  E --> F[Atomic replacement]
  F --> G[Complete file for later readers]

Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Production versus test LOC	Production −14 net lines; tests +202 net lines	Consolidating three writers reduces production code while adding targeted storage regressions.

Technical review

Best possible solution:

Use one private atomic writer while preserving existing serializers, config and OAuth locks, and cookie symlink behavior.

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

Yes, from source: rewriting an existing 0644 cookie file retains its permissions, and an already-open reader shares the inode being overwritten. The new regression tests target both cases; this review did not execute them.

Is this the best way to solve the issue?

Yes. Reusing the existing temporary-file replacement approach repairs cookie writes and removes duplication without changing stored formats or caller locking.

AGENTS.md: not found in the target repository.

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

Labels

Label changes:

No label changes.

Label justifications:

• P2: This bounded credential-storage repair prevents partial cookie reads and retained broad permissions without changing the authentication workflow.
• 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: COLLABORATOR authorship exempts the ordinary contributor proof gate. The captured body reports a built-binary auth paste/status run through the changed cookie writer, including existing-file permissions and symlink preservation. Serialized data contracts are unchanged, so no data-model migration is required.

Evidence

What I checked:

• Pinned introduced changes: Inspected the complete introduced delta from the pinned main/base to the original PR head, including platform implementations, tests, documentation, and removed writers. The host-verified test merge has the pinned main and head as its two parents. (38738b7d5dc3)
• Current main still needs the fix: The cookie writer still calls os.WriteFile directly with mode 0600, which does not tighten permissions on an existing file and overwrites its contents in place. (internal/cookies/store.go:75, e532324d5e82)
• Latest release also retains the old writer: The v0.13.0 cookie writer contains the same direct os.WriteFile operation; this release does not already provide the proposed atomic cookie replacement. (internal/cookies/store.go:75, 34935e20659b)
• Replacement and compatibility review: The shared writer creates a same-directory temporary file, sets mode 0600 before writing, syncs and closes it, then replaces the destination. Cookie symlinks are resolved before replacement. Config transaction locking and OAuth locking remain in their callers, and all serialized fields and formats are unchanged. Windows replacement retains bounded busy-reader retries and OAuth write-through flags. (internal/atomicfile/write.go:11, 38738b7d5dc3)
• Focused regression coverage: Added tests cover tightening an existing 0644 cookie file, preserving an open reader's old snapshot, and updating existing or missing symlink targets. Shared-writer tests cover complete replacement, cleanup, destination preservation on replacement failure, and Windows readers. Existing config alias-lock and OAuth permission tests remain applicable. Tests were inspected, not executed during this read-only review. (internal/cookies/store_atomic_test.go:12, 38738b7d5dc3)
• Contributor validation and proof exemption: The captured PR body reports a built-binary auth paste workflow that tightened an existing file to 0600, preserved a custom symlink, and kept auth status working, plus a 90.9% race/coverage gate. These are reported results, not reviewer-executed observations. GitHub confirms COLLABORATOR authorship, so the external-contributor proof gate does not apply. (38738b7d5dc3)

Likely related people:

• Peter Steinberger: Raw commit ff7cb31 adds internal/spotify/oauth_cache.go:71 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: ff7cb3164a22; files: internal/spotify/oauth_cache.go)

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.

History

Review history (1 earlier review cycle)

• reviewed 2026-09-22T12:12:27.601Z sha 59b9367 :: needs maintainer review before merge. :: none