Skip to content

Implement file-based persistence system#1

Closed
Wirasm wants to merge 21 commits intomainfrom
ralph/file-based-persistence
Closed

Implement file-based persistence system#1
Wirasm wants to merge 21 commits intomainfrom
ralph/file-based-persistence

Conversation

@Wirasm
Copy link
Copy Markdown
Owner

@Wirasm Wirasm commented Jan 9, 2026

Overview

This PR implements a complete file-based persistence system for Shards CLI, replacing the SQLite dependency with lightweight JSON file storage.

Major Features

🗄️ File-Based Persistence System

  • Replace SQLite dependency with JSON file-based session storage
  • Session files stored in directory
  • Atomic file operations for reliable persistence
  • Automatic cleanup when sessions are destroyed
  • Backward compatible session management

📁 Session Storage Format

  • File naming:
  • JSON structure:
  • Atomic writes prevent corruption during concurrent operations
  • Directory auto-creation and proper error handling

Technical Implementation

Core Changes

  • sessions/operations.rs: File I/O operations for session persistence
  • sessions/handler.rs: Updated to use file-based storage
  • sessions/types.rs: Enhanced session data structures
  • Comprehensive error handling with detailed error types

Key Benefits

  1. Zero external dependencies - no SQLite required
  2. Simple deployment - just copy binary, no database setup
  3. Human readable - JSON files can be inspected/debugged
  4. Atomic operations - prevents data corruption
  5. Cross-platform - works on all supported platforms

Testing Results

  • All 69 tests pass
  • Create/List/Destroy workflow works end-to-end
  • Session persistence verified (JSON files created/removed)
  • Concurrent operations handled safely
  • Error recovery tested with invalid files
  • Cross-platform compatibility maintained

Example Session File

{
  "id": "af405012531586b7/feature-auth",
  "project_id": "af405012531586b7", 
  "branch": "feature-auth",
  "worktree_path": "/Users/user/.shards/worktrees/project/feature-auth",
  "agent": "kiro",
  "status": "Active",
  "created_at": "2026-01-09T11:15:12.850824+00:00"
}

Migration Path

This is a breaking change that removes SQLite dependency. Users will need to:

  1. Destroy existing sessions before upgrading
  2. Recreate sessions after upgrade

The simpler architecture and zero-dependency deployment make this worthwhile.

Future Enhancements

  • Session templates and configuration
  • Enhanced session metadata
  • Session history and analytics
  • Integration with external tools

This PR delivers the core "browser tabs for AI agents" experience with reliable, lightweight persistence.

@Wirasm
Copy link
Copy Markdown
Owner Author

Wirasm commented Jan 12, 2026

Code Review Summary

Overall Assessment: NEEDS CHANGES
Risk Level: HIGH
Recommendation: Fix critical error handling issues before merge

🚨 Critical Issues Found:

  • Silent exception handling masking specific errors (lines 15-20)
  • Subprocess calls without timeout/error checking (line 25)
  • Missing input validation and git repository checks

Full detailed report available in: .kiro/artifacts/code-review-reports/PR-1-comprehensive-review.md

Please address critical issues before merge.

Wirasm added 10 commits January 12, 2026 13:46
- Changed terminal handler to use async spawn() instead of blocking output()
- Prevents CLI from hanging when launching terminal applications
- Removed unused error import from terminal handler
- Added comprehensive PRD with 6 user stories for session persistence
- Created Ralph autonomous development workflow
- Includes progress tracking and execution script
- Add Ghostty back to TerminalType enum
- Implement Fish shell workaround for Ghostty's -e parsing issues
- Use 'ghostty -e fish -c "command"' to bypass broken argument parsing
- Update terminal detection to prioritize Ghostty → iTerm → Terminal.app
- Add comprehensive tests for Ghostty functionality
- Update error messages to include Ghostty in supported terminals

This resolves the window visibility issues by using Fish shell to properly
handle command parsing that Ghostty's -e flag currently handles incorrectly.
- Remove Ghostty from TerminalType enum and terminal detection
- Remove unused command_exists function
- Update error messages and tests to exclude Ghostty
- Focus on reliable iTerm/Terminal.app support

Ghostty's -e flag currently runs commands in background without visible
windows. Will re-add support when ghostty-org/ghostty#7032 is resolved.
@Wirasm
Copy link
Copy Markdown
Owner Author

Wirasm commented Jan 12, 2026

PR Review Summary

Critical Issues (3 found)

Source Issue Location
silent-failure-hunter Silent directory cleanup fallback in remove_worktree_by_path() - when worktree not in git registry, silently deletes directory hiding state inconsistency src/git/handler.rs:248-258
silent-failure-hunter Silent session skipping in load_sessions_from_files() - user running shards list has no indication sessions failed to load src/sessions/operations.rs:85-131
pr-test-analyzer Atomic write pattern NOT tested - no test verifies temp file is cleaned up or that atomic behavior works src/sessions/operations.rs:69-78

Important Issues (6 found)

Source Issue Location
code-reviewer Atomic file write not fully atomic - orphaned .json.tmp files will remain if process crashes between write and rename src/sessions/operations.rs:115-125
code-reviewer Session validation doesn't verify worktree_path actually exists on disk - stale sessions appear valid src/sessions/operations.rs:139-147
silent-failure-hunter remove_session_file() silently succeeds when file doesn't exist - masks state inconsistency src/sessions/operations.rs:152-160
silent-failure-hunter Atomic write has no temp file cleanup on failure - orphaned temp files accumulate src/sessions/operations.rs:66-78
silent-failure-hunter Repository opening fallback swallows original error - debugging more difficult src/git/handler.rs:207-233
pr-test-analyzer Missing handler integration test for create-list-destroy flow - primary user workflow untested src/sessions/handler.rs

Suggestions (6 found)

Source Suggestion Location
code-reviewer find_session_by_name loads ALL sessions O(n) - consider more efficient lookup using glob patterns src/sessions/operations.rs:149-159
silent-failure-hunter ensure_sessions_directory() has TOCTOU race condition - use create_dir_all unconditionally src/sessions/operations.rs:58-64
silent-failure-hunter validate_session_structure() returns bool instead of specific error - loses validation context src/sessions/operations.rs:133-141
pr-test-analyzer Missing test for type mismatch JSON - verifies serde errors for wrong types not just missing fields src/sessions/operations.rs
pr-test-analyzer Test all 6 validation conditions - only 2/6 fields tested in validate_session_structure src/sessions/operations.rs:500-520
pr-test-analyzer Add test verifying no .tmp file remains after successful save src/sessions/operations.rs

Strengths

  • Atomic file writes - Uses temp file + rename pattern to prevent corruption
  • Graceful error handling - load_sessions_from_files warns and skips invalid files instead of failing entirely
  • Good test coverage for happy paths - New operations have unit tests covering basic functionality
  • Clean dependency update - Properly removes rusqlite and adds serde/serde_json
  • Consistent structured logging - Uses tracing events with proper event names throughout
  • Type safety maintained - Properly derives Serialize/Deserialize on Session types
  • No .unwrap() or .expect() in production code - Proper error handling with Result types

Recommended Action

  1. Address critical silent failure issues - Add warnings when worktree not in registry, surface skipped sessions to user
  2. Add atomic write test - Verify temp file cleanup behavior to prevent regressions
  3. Add temp file cleanup on failure - Clean up .json.tmp if write or rename fails
  4. Consider integration test - The create-list-destroy workflow is the primary user journey

Verdict: The PR implements file-based persistence correctly with good architecture. Address the silent failure issues (especially in remove_worktree_by_path and load_sessions_from_files) before merging to ensure users get proper feedback about errors. The atomic write pattern should also have test coverage to prevent regressions.

@Wirasm Wirasm force-pushed the ralph/file-based-persistence branch from 08e83a6 to 7f4bcee Compare January 12, 2026 12:15
- Complete PRD with 10 user stories addressing all code review issues
- Progress tracking log with implementation learnings
- Ralph execution script and prompt for autonomous development
Wirasm added a commit that referenced this pull request Jan 12, 2026
- Replace SQLite with JSON file-based session persistence
- Add atomic file writes with temp file + rename pattern
- Implement comprehensive session validation with detailed error reporting
- Fix all critical code review issues from PR #1:
  * Silent failure patterns eliminated with proper logging
  * TOCTOU race conditions fixed in directory creation
  * Enhanced error handling with preserved context
  * Added missing integration tests for core workflows
- Add Ralph autonomous development artifacts and PRD tracking
- Maintain 73 passing tests with no regressions
- Production-ready persistence layer with thread-safe operations

Resolves all PR #1 code review findings and establishes solid foundation
for parallel development tracks.
@Wirasm Wirasm closed this Jan 12, 2026
@Wirasm Wirasm deleted the ralph/file-based-persistence branch January 12, 2026 12:24
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