fix(session): prevent saves failing for long session IDs

[henkterharmsel]

Related: #447

What Problem This Solves

Fixes an issue where users with long but valid ACPX session identifiers could see session persistence fail with ENAMETOOLONG after the collision fix in #447. Appending PID, timestamp, and UUID to the full destination basename can exceed the common 255-byte filesystem component limit even when the destination itself is valid.

Why This Change Was Made

Atomic writes now create a bounded sibling temporary basename in the destination directory. This preserves same-filesystem atomic rename behavior, PID/timestamp diagnostics, and per-write UUID uniqueness without extending the destination basename.

User Impact

Long valid session identifiers remain persistable while concurrent writes continue to use collision-resistant temporary paths.

Evidence

• Before the fix, the new regression test fails because the generated basename exceeds 255 bytes.
• After the fix, both atomic-write regression tests pass.
• TMPDIR=/var/tmp PATH=/tmp/codex-corepack-bin:$PATH corepack pnpm run check passes:
  • 885 regular tests
  • 111 coverage tests
  • formatting, typecheck, lint, build, viewer checks, and coverage thresholds
• codex review --base upstream/main reported no findings.

Real behavior proof

Behavior or issue addressed: A valid 220-byte session identifier must remain writable through the real file session store while concurrent writes use distinct temporary files.

Real environment tested: Linux x86_64 checkout of this branch using Node 24.18.0 and pnpm 10.34.5. The probe imported the built production createFileSessionStore implementation directly; it did not use mocks or snapshots.

Exact steps or command run after this patch:

1. Built the production runtime with corepack pnpm run build.
2. Created a temporary ACPX state directory.
3. Froze Date.now() so all writes shared one millisecond.
4. Used a 220-byte acpxRecordId.
5. Ran 400 concurrent store.save(record) calls.
6. Loaded the session through the public store and checked for leftover .tmp files.

Evidence after fix:

{"saves":400,"sessionIdBytes":220,"loaded":true,"tempFiles":0,"jsonFiles":1}

Observed result after fix: All 400 writes completed without an exception. The long-ID session remained readable, exactly one JSON destination remained, and no temporary files leaked.

What was not tested: Windows and macOS were not tested locally. The same-directory and basename-length behavior is covered through Node's platform path API and CI can provide cross-platform validation.

AI-assisted disclosure

This PR is AI-assisted. I reviewed the implementation and understand that only the private temporary basename changes; the destination path, atomic rename boundary, and UUID uniqueness remain unchanged.

[clawsweeper]

Codex review: needs maintainer review before merge. Reviewed July 27, 2026, 2:48 PM ET / 18:48 UTC.

ClawSweeper review

What this changes

The PR changes atomic session-write temporary paths to use a short unique sibling basename and adds regression coverage for long valid session filenames.

Merge readiness

⚠️ Ready for maintainer review - 1 item remains

This PR remains necessary: current main still appends the destination basename to its unique temporary filename, which can exceed a filesystem component limit for otherwise valid long session IDs. The proposed bounded sibling name preserves same-directory atomic rename and the prior per-write UUID collision protection; no actionable patch defect was found. Likely related people: henkterharmsel is the strongest routing candidate from the merged predecessor work (medium confidence).

Priority: P2
Reviewed head: dc67122e013cfd271bafa90487f4236565b8607d

Review scores

Measure	Result	What it means
Overall readiness	🐚 platinum hermit (4/6)	A focused, source-aligned fix with strong real-store proof; the remaining gate is ordinary maintainer review and completion of required checks.
Proof confidence	🦞 diamond lobster (5/6)	Sufficient (live_output): The PR includes after-fix live output from the built production file-session store: 400 concurrent saves of a 220-byte ID completed, the record reloaded, and no temporary files remained.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (live_output): The PR includes after-fix live output from the built production file-session store: 400 concurrent saves of a 220-byte ID completed, the record reloaded, and no temporary files remained.
Evidence reviewed	4 items	Current-main vulnerable path: The current-base implementation constructs the temporary path by appending PID, timestamp, UUID, and .tmp to the full destination path; a valid 220-byte session filename can therefore exceed the common 255-byte component limit before the atomic write begins. Focused repair and regression coverage: The PR constructs .acpx-write.<pid>.<timestamp>.<uuid>.tmp in the destination directory and adds assertions that it stays in that directory and its basename is at most 255 bytes. Feature-history provenance: The related merged PR introduced UUID-based temporary names to prevent concurrent-save collisions; this PR retains that uniqueness while removing the destination basename from the temporary component.
Findings	None	None.
Security	None	None.

How this fits together

ACPX persists agent session records through a local file session store. A save writes JSON to a unique temporary file beside the destination and atomically renames it into the persistent session record.

flowchart LR
  Session[Agent session update] --> Store[File session store]
  Store --> Destination[Session JSON destination]
  Destination --> Temp[Unique sibling temp file]
  Temp --> Rename[Atomic rename]
  Rename --> Record[Persisted session record]

Loading

Before merge

• Complete next step (P2) - No mechanical repair is needed; retain this PR for normal maintainer merge review after its in-progress required checks finish.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Patch scope	2 files affected; 14 additions, 1 deletion	The repair is tightly limited to the atomic-path helper and its regression test.
Behavior probe	400 concurrent saves with a 220-byte session ID	The supplied live-store proof exercises both the filename-limit case and retained uniqueness behavior.

Technical review

Best possible solution:

Merge the bounded same-directory temporary-name approach once the remaining required checks complete, keeping the UUID uniqueness and existing atomic-rename boundary unchanged.

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

Yes from source inspection: current main appends roughly sixty bytes of uniqueness suffix to the destination basename, so a valid 220-byte ID plus .json exceeds a common 255-byte component limit. The PR also supplies a concrete after-fix real-store probe, though this read-only review did not execute it locally.

Is this the best way to solve the issue?

Yes. A short hidden sibling basename preserves the same-filesystem atomic rename boundary, PID/timestamp diagnostics, and UUID uniqueness without imposing a new session-ID limit or changing persisted destination names.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 504040facb19.

Labels

Label justifications:

• P2: Long valid session identifiers can fail persistence, but the repair is bounded to local session-file writes rather than an emergency-wide outage.
• rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
• status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR includes after-fix live output from the built production file-session store: 400 concurrent saves of a 220-byte ID completed, the record reloaded, and no temporary files remained.
• proof: sufficient: Contributor real behavior proof is sufficient. The PR includes after-fix live output from the built production file-session store: 400 concurrent saves of a 220-byte ID completed, the record reloaded, and no temporary files remained.

Evidence

What I checked:

• Current-main vulnerable path: The current-base implementation constructs the temporary path by appending PID, timestamp, UUID, and .tmp to the full destination path; a valid 220-byte session filename can therefore exceed the common 255-byte component limit before the atomic write begins. (src/session/persistence/atomic-write.ts:7, 504040facb19)
• Focused repair and regression coverage: The PR constructs .acpx-write.<pid>.<timestamp>.<uuid>.tmp in the destination directory and adds assertions that it stays in that directory and its basename is at most 255 bytes. (test/atomic-write.test.ts:24, dc67122e013c)
• Feature-history provenance: The related merged PR introduced UUID-based temporary names to prevent concurrent-save collisions; this PR retains that uniqueness while removing the destination basename from the temporary component. (src/session/persistence/atomic-write.ts:7, 1d17a61f7eb3)
• After-fix real behavior proof: The PR body reports a production-store probe with a 220-byte record ID and 400 concurrent saves, followed by a successful load with one JSON destination and no temporary files remaining. (dc67122e013c)

Likely related people:

• henkterharmsel: Authored the merged concurrent-save repair that added UUID uniqueness to atomic temporary filenames and authored this narrowly related follow-up. (role: introduced adjacent behavior; confidence: medium; commits: 250521750783, 1d17a61f7eb3; files: src/session/persistence/atomic-write.ts, test/atomic-write.test.ts)

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-07-27T17:20:01.254Z sha 68ca211 :: needs maintainer review before merge. :: none