Skip to content

refactor(state): dedupe session-checkpoint helper boilerplate#825

Closed
kelsonpw wants to merge 1 commit into
mainfrom
refactor/discovered-facts-and-state
Closed

refactor(state): dedupe session-checkpoint helper boilerplate#825
kelsonpw wants to merge 1 commit into
mainfrom
refactor/discovered-facts-and-state

Conversation

@kelsonpw
Copy link
Copy Markdown
Member

@kelsonpw kelsonpw commented May 17, 2026

Summary

Two small, behavior-preserving cleanups inside src/lib/session-checkpoint.ts. Scope was originally discovered-facts/ + agent-state* + session-checkpoint, but src/lib/discovered-facts/ and a standalone agent-state-hydrate.ts don't exist on main (they live on feat/discovered-facts-vertical-and-app-type), and agent-state.ts had no safe dedup left after a careful read — so this PR is intentionally narrow.

What landed

Net lines: +17 / -16 (all in session-checkpoint.ts). No test changes, no schema changes, no on-disk format changes.

Lines touched in session-checkpoint.ts

  • Lines 288–294 (return body of loadCheckpoint) — collapsed the inline checkpoint.selectedProjectId && checkpoint.selectedProjectId.trim().length > 0 ? ... : null ternary into isNonEmpty(checkpoint.selectedProjectId) ? ... : null. isNonEmpty was already defined at line 348 and used by checkpointHasMeaningfulSelection. Safe because isNonEmpty(value) returns typeof value === 'string' && value.trim().length > 0 — semantically identical for the string | null | undefined input type.
  • Lines 364–380 (hasProjectBinding) — replaced two near-identical try { if (existsSync(x)) return true } catch { return true } blocks with a single loop over a 2-element candidates array. The EACCES/EIO → "treat as present" invariant is preserved (and now documented inline rather than duplicated above the function). Result for every input is identical: returns true iff either path exists OR existsSync itself throws on either path.

session-checkpoint.ts safety analysis

Every condition added by the recent fix history is preserved:

Checkpoint on-disk format (version, field names, schema transform legacy reads) and 0o600 mode are all unchanged. loadCheckpoint's null-return predicates (file missing, parse failure, schema failure, >24h age, installDir mismatch, companion-file missing) remain identical in count and order.

Patterns deferred

  • Duplicate checkpointHasMeaningfulSelection / isNonEmpty between session-checkpoint.ts and self-heal.ts — would require touching self-heal.ts, which is out of scope here.
  • Shared withCacheRoot() test helper for agent-state.test.ts + agent-state-hydrate.test.ts boilerplate — would require a brand-new _helpers test file, which CLAUDE.md guidance discourages without explicit need.
  • buildRetryHint / buildRecoveryNote section-builder dedup in agent-state.ts — the wording diverges enough that any shared helper would obscure intent. Left alone.
  • discovered-facts/ classifier dedup — directory doesn't exist on main; lives on a feature branch.

Tests

  • Before: 4473 passing
  • After: 4473 passing
  • Focused session-checkpoint runs: 25 green (16 in session-checkpoint.test.ts + 9 in session-checkpoint-events.test.ts)

Test plan

  • pnpm tsc --noEmit green
  • pnpm lint green
  • pnpm test green (4473/4473)
  • Focused: pnpm exec vitest run --pool=forks --maxWorkers=1 src/lib/__tests__/session-checkpoint*.test.ts src/lib/__tests__/agent-state*.test.ts green
  • Checkpoint on-disk format unchanged
  • wizard-abort.ts untouched
  • wizard-session.ts untouched
  • loadCheckpoint() invalidation conditions preserved

🤖 Generated with Claude Code


Note

Low Risk
Low risk: behavior-preserving refactors limited to checkpoint loading and file-existence checks, with no schema, persistence format, or control-flow changes intended.

Overview
Refactors loadCheckpoint() to normalize selectedProjectId via the shared isNonEmpty() helper instead of an inline trim/ternary check.

Refactors hasProjectBinding() to iterate over the canonical and legacy binding file paths in a single loop, preserving the existing "existsSync errors count as present" behavior while removing duplicated try/catch blocks.

Reviewed by Cursor Bugbot for commit f4090a0. Bugbot is set up for automated code reviews on this repo. Configure here.

Two small, behavior-preserving cleanups inside session-checkpoint.ts:

1. loadCheckpoint() return — collapse the inline `s && s.trim().length > 0`
   guard for `selectedProjectId` into the existing `isNonEmpty()` helper.
   Identical semantics; one fewer place that has to know the empty/whitespace
   rule.

2. hasProjectBinding() — replace two identical `try { existsSync(x) } catch
   { return true }` blocks with a single loop over a candidates array.
   The EACCES/EIO → "present" invariant is preserved (and now documented
   inline rather than duplicated).

No checkpoint-load invalidation logic is altered. The #778 companion-file
check (`meaningful selection && !hasProjectBinding → invalidate`) keeps the
same predicates with the same fall-through behavior. Schema, on-disk
format, and 0o600 perm all untouched.

discovered-facts/ and agent-state-hydrate.ts mentioned in the brief don't
exist on main, so they were out of reach this pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@kelsonpw kelsonpw requested a review from a team as a code owner May 17, 2026 11:53
@kelsonpw kelsonpw removed the request for review from a team May 18, 2026 18:04
@kelsonpw
Copy link
Copy Markdown
Member Author

Closing as part of pat-leave handoff to reduce open-PR load on the team. Branch preserved on origin — reopen if specific changes are wanted. The highest-impact wins from this refactor round already merged.

@kelsonpw kelsonpw closed this May 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant