Improvements

[theolampert]

Summary by cubic

Brings Quilt to production-ready Peritext CRDT with persistence, undo/redo, extended formatting, and major performance gains. Also adds the Ralph autonomous development loop with specs and 95 passing tests.

• New Features

  • Full Peritext CRDT: tombstones, mark ops, intent-preserving formatting.
  • Undo/redo via compensating operations and operation-log compaction.
  • Extended marks: link (non-expanding), color, font family, font size, highlight.
  • Persistence: JSON and binary snapshots with versioning; restore rebuilds indexes/cache.
  • Performance: OpID→index map for O(1) lookups and render caching; comprehensive tests and specs.

• Dependencies

  • Upgrade swift-tools-version to 6.2 and remove external swift-testing dependency.
  • Add @retroactive Codable for Character to satisfy Swift 6 requirements.

^{Written for commit 975758a. Summary will update on new commits.}

[cubic-dev-ai]

13 issues found across 20 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name=".ralph/validate.sh">

<violation number="1" location=".ralph/validate.sh:4">
P1: Hardcoded absolute path makes the script non-portable. Derive the project root relative to the script's location so it works for any developer or environment.</violation>
</file>

<file name="Tests/QuiltTests/QuiltTests.swift">

<violation number="1" location="Tests/QuiltTests/QuiltTests.swift:469">
P2: Comment/code mismatch: comment says "Mark indices 1-2" with "toIndex 3 is exclusive," but the code uses `fromIndex: 1, toIndex: 2`, which marks only index 1 ('e'). The assertions are correct for the actual code, but the comment describes a different scenario.</violation>
</file>

<file name="Sources/Quilt/Persistence.swift">

<violation number="1" location="Sources/Quilt/Persistence.swift:34">
P2: `currentVersion` is declared but never used, and `from(snapshot:)` never validates `snapshot.version`. This defeats the purpose of having version information in the snapshot — a newer-format snapshot could be silently loaded and produce corrupted state. Add a version check in `from(snapshot:)`.</violation>
</file>

<file name=".ralph/README.md">

<violation number="1" location=".ralph/README.md:9">
P2: Documentation contains hardcoded user-specific absolute paths (`/Users/theo/repos/quilt/Quilt`). These should use relative paths or a generic placeholder so the instructions work for any contributor.</violation>
</file>

<file name="Sources/Quilt/MarkType.swift">

<violation number="1" location="Sources/Quilt/MarkType.swift:25">
P2: Non-expanding marks like `.link` need a `startAnchorType` method to complement `endAnchorType`. Currently, `Quilt.swift:addMark` hardcodes `.before` for the start anchor, making links asymmetrically expand at their start boundary but not at their end. Per Peritext, a non-expanding mark should use `.after(opId)` for the start anchor as well.</violation>

<violation number="2" location="Sources/Quilt/MarkType.swift:41">
P1: Bug: `.link` should have a conflict group to prevent multiple concurrent links on the same text. Currently, if two users concurrently apply different links to the same range, `resolveConflicts()` will keep both since `conflictGroup` returns `nil` for links. A character cannot meaningfully link to two different URLs.</violation>
</file>

<file name=".claude/PROMPT_build.md">

<violation number="1" location=".claude/PROMPT_build.md:41">
P2: Numbered list in "Exit Condition" skips from 3 to 5 — item 4 is missing. This appears to be an accidental deletion of a requirement. If no item 4 is intended, renumber 5 and 6 to 4 and 5 for clarity.</violation>
</file>

<file name="specs/peritext-implementation.md">

<violation number="1" location="specs/peritext-implementation.md:521">
P1: Bug in spec code example: `duplicates.min` should be `duplicates.max`. The prose correctly states the greatest OpID wins, and the real implementation uses `.max`, but this code snippet uses `.min` which would select the smallest OpID — the exact opposite of the intended behavior. This will mislead anyone implementing from the spec.</violation>
</file>

<file name="Sources/Quilt/Quilt.swift">

<violation number="1" location="Sources/Quilt/Quilt.swift:621">
P1: `compact()` is a no-op: every operation's `opId` is unconditionally inserted into `referencedOpIds` in step 1, so the filter in step 3 (`referencedOpIds.contains($0.opId)`) will always be true for every operation. Nothing is ever removed.

The line `referencedOpIds.insert(op.opId)` should be conditional — only insert the opId if the operation is actually still needed (e.g., it produces a non-deleted character, or is a mark operation still in use). As written, the method gives a false sense of compaction while silently doing nothing.</violation>

<violation number="2" location="Sources/Quilt/Quilt.swift:695">
P1: Undo/redo uses a stale `visibleIndex` captured at operation time, but intervening operations can shift character positions. This will cause undo to remove/insert at the wrong position.

Instead of storing a visible index, the undo for `.insert` should locate the character by its `operation.opId` (which is stable), convert that to the current visible index, and then call `remove`. Similarly, undo for `.remove` should compute the correct current visible index from the surrounding context.</violation>
</file>

<file name="AGENTS.md">

<violation number="1" location="AGENTS.md:107">
P3: Self-reference uses incorrect casing: `agents.md` should be `AGENTS.md` to match the actual filename.</violation>
</file>

<file name=".ralph/ralph-loop.sh">

<violation number="1" location=".ralph/ralph-loop.sh:167">
P0: Bug: `((iteration++))` will crash the script on the first iteration due to `set -e`. When `iteration` is 0, the post-increment expression evaluates to 0, which gives exit code 1 in an arithmetic context. Since this line is in the main loop body (not inside an `if` condition), `set -e` will terminate the script immediately. Use `((++iteration))` (pre-increment) or `iteration=$((iteration + 1))` instead.</violation>

<violation number="2" location=".ralph/ralph-loop.sh:173">
P1: Bug: `exit_code=$?` always captures 0 here because the `!` operator inverts the return value of `check_exit_criteria`. The specific exit codes (1=max iterations, 2=test failures, 3=build failures, 4=stuck) are lost and the script always exits with 0. Restructure to capture the exit code before negation.</violation>
</file>

---

Since this is your first cubic review, here's how it works:

• cubic automatically reviews your code and comments on bugs and improvements
• Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
• Add one-off context when rerunning by tagging @cubic-dev-ai with guidance or docs links (including llms.txt)
• Ask questions if you need clarification on any suggestion

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[cubic-dev-ai]

P0: Bug: ((iteration++)) will crash the script on the first iteration due to set -e. When iteration is 0, the post-increment expression evaluates to 0, which gives exit code 1 in an arithmetic context. Since this line is in the main loop body (not inside an if condition), set -e will terminate the script immediately. Use ((++iteration)) (pre-increment) or iteration=$((iteration + 1)) instead.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At .ralph/ralph-loop.sh, line 167:

<comment>Bug: `((iteration++))` will crash the script on the first iteration due to `set -e`. When `iteration` is 0, the post-increment expression evaluates to 0, which gives exit code 1 in an arithmetic context. Since this line is in the main loop body (not inside an `if` condition), `set -e` will terminate the script immediately. Use `((++iteration))` (pre-increment) or `iteration=$((iteration + 1))` instead.</comment>

<file context>
@@ -0,0 +1,206 @@
+echo "" | tee -a "$LOG_FILE"
+
+while true; do
+    ((iteration++))
+    log_iteration "$iteration" "Starting iteration..."
+    echo "" | tee -a "$LOG_FILE"
</file context>

[cubic-dev-ai]

P1: Hardcoded absolute path makes the script non-portable. Derive the project root relative to the script's location so it works for any developer or environment.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At .ralph/validate.sh, line 4:

<comment>Hardcoded absolute path makes the script non-portable. Derive the project root relative to the script's location so it works for any developer or environment.</comment>

<file context>
@@ -0,0 +1,35 @@
+#!/usr/bin/env bash
+set -euo pipefail
+
+cd /Users/theo/repos/quilt/Quilt
+
+echo "🔨 Building..."
</file context>

[cubic-dev-ai]

P1: Bug: .link should have a conflict group to prevent multiple concurrent links on the same text. Currently, if two users concurrently apply different links to the same range, resolveConflicts() will keep both since conflictGroup returns nil for links. A character cannot meaningfully link to two different URLs.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Quilt/MarkType.swift, line 41:

<comment>Bug: `.link` should have a conflict group to prevent multiple concurrent links on the same text. Currently, if two users concurrently apply different links to the same range, `resolveConflicts()` will keep both since `conflictGroup` returns `nil` for links. A character cannot meaningfully link to two different URLs.</comment>

<file context>
@@ -3,4 +3,52 @@ public enum MarkType: Codable, Equatable, Sendable, Hashable {
+            return "fontSize"
+        case .highlight:
+            return "highlight"
+        case .bold, .italic, .underline, .link:
+            return nil  // Non-conflicting marks
+        }
</file context>

[cubic-dev-ai]

P1: Bug in spec code example: duplicates.min should be duplicates.max. The prose correctly states the greatest OpID wins, and the real implementation uses .max, but this code snippet uses .min which would select the smallest OpID — the exact opposite of the intended behavior. This will mislead anyone implementing from the spec.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At specs/peritext-implementation.md, line 521:

<comment>Bug in spec code example: `duplicates.min` should be `duplicates.max`. The prose correctly states the greatest OpID wins, and the real implementation uses `.max`, but this code snippet uses `.min` which would select the smallest OpID — the exact opposite of the intended behavior. This will mislead anyone implementing from the spec.</comment>

<file context>
@@ -0,0 +1,801 @@
+        }
+
+        if duplicates.count > 1 && !processed.contains(op.opId) {
+            let winner = duplicates.min { $0.opId < $1.opId }!
+            deduped.append(winner)
+            processed.formUnion(duplicates.map { $0.opId })
</file context>

[cubic-dev-ai]

P1: compact() is a no-op: every operation's opId is unconditionally inserted into referencedOpIds in step 1, so the filter in step 3 (referencedOpIds.contains($0.opId)) will always be true for every operation. Nothing is ever removed.

The line referencedOpIds.insert(op.opId) should be conditional — only insert the opId if the operation is actually still needed (e.g., it produces a non-deleted character, or is a mark operation still in use). As written, the method gives a false sense of compaction while silently doing nothing.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Quilt/Quilt.swift, line 621:

<comment>`compact()` is a no-op: every operation's `opId` is unconditionally inserted into `referencedOpIds` in step 1, so the filter in step 3 (`referencedOpIds.contains($0.opId)`) will always be true for every operation. Nothing is ever removed.

The line `referencedOpIds.insert(op.opId)` should be conditional — only insert the opId if the operation is actually still needed (e.g., it produces a non-deleted character, or is a mark operation still in use). As written, the method gives a false sense of compaction while silently doing nothing.</comment>

<file context>
@@ -168,4 +468,286 @@ public struct Quilt: Sendable {
+                referencedOpIds.insert(afterId)
+            }
+            // The operation itself is always kept if it's creating a character that exists
+            referencedOpIds.insert(op.opId)
+        }
+
</file context>

[cubic-dev-ai]

P2: currentVersion is declared but never used, and from(snapshot:) never validates snapshot.version. This defeats the purpose of having version information in the snapshot — a newer-format snapshot could be silently loaded and produce corrupted state. Add a version check in from(snapshot:).

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Quilt/Persistence.swift, line 34:

<comment>`currentVersion` is declared but never used, and `from(snapshot:)` never validates `snapshot.version`. This defeats the purpose of having version information in the snapshot — a newer-format snapshot could be silently loaded and produce corrupted state. Add a version check in `from(snapshot:)`.</comment>

<file context>
@@ -0,0 +1,97 @@
+/// Extension to add persistence capabilities to Quilt
+extension Quilt {
+    /// Current snapshot format version
+    private static let currentVersion = 1
+
+    /// Creates a Quilt document from a snapshot
</file context>

[cubic-dev-ai]

P2: Documentation contains hardcoded user-specific absolute paths (/Users/theo/repos/quilt/Quilt). These should use relative paths or a generic placeholder so the instructions work for any contributor.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At .ralph/README.md, line 9:

<comment>Documentation contains hardcoded user-specific absolute paths (`/Users/theo/repos/quilt/Quilt`). These should use relative paths or a generic placeholder so the instructions work for any contributor.</comment>

<file context>
@@ -0,0 +1,153 @@
+
+### Test with Dry Run (2 iterations)
+```bash
+cd /Users/theo/repos/quilt/Quilt
+RALPH_MAX_ITERATIONS=2 ./.ralph/ralph-loop.sh
+```
</file context>

[cubic-dev-ai]

P2: Non-expanding marks like .link need a startAnchorType method to complement endAnchorType. Currently, Quilt.swift:addMark hardcodes .before for the start anchor, making links asymmetrically expand at their start boundary but not at their end. Per Peritext, a non-expanding mark should use .after(opId) for the start anchor as well.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Quilt/MarkType.swift, line 25:

<comment>Non-expanding marks like `.link` need a `startAnchorType` method to complement `endAnchorType`. Currently, `Quilt.swift:addMark` hardcodes `.before` for the start anchor, making links asymmetrically expand at their start boundary but not at their end. Per Peritext, a non-expanding mark should use `.after(opId)` for the start anchor as well.</comment>

<file context>
@@ -3,4 +3,52 @@ public enum MarkType: Codable, Equatable, Sendable, Hashable {
+
+    /// Returns the appropriate anchor type for the end of this mark
+    /// - Returns: .before for expanding marks, .after for non-expanding marks
+    func endAnchorType(for opId: OpID) -> SpanMarker {
+        return isExpanding ? .before(opId) : .after(opId)
+    }
</file context>

[cubic-dev-ai]

P2: Numbered list in "Exit Condition" skips from 3 to 5 — item 4 is missing. This appears to be an accidental deletion of a requirement. If no item 4 is intended, renumber 5 and 6 to 4 and 5 for clarity.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At .claude/PROMPT_build.md, line 41:

<comment>Numbered list in "Exit Condition" skips from 3 to 5 — item 4 is missing. This appears to be an accidental deletion of a requirement. If no item 4 is intended, renumber 5 and 6 to 4 and 5 for clarity.</comment>

<file context>
@@ -0,0 +1,329 @@
+1. **ALL Priority 1 tasks completed** ✅
+2. **ALL Priority 2 tasks completed** (in progress)
+3. **ALL Priority 3 tasks completed** (planned)
+5. All tests pass
+6. No compiler warnings
+
</file context>

[cubic-dev-ai]

P3: Self-reference uses incorrect casing: agents.md should be AGENTS.md to match the actual filename.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At AGENTS.md, line 107:

<comment>Self-reference uses incorrect casing: `agents.md` should be `AGENTS.md` to match the actual filename.</comment>

<file context>
@@ -0,0 +1,262 @@
+│   └── peritext-implementation.md # Detailed Peritext implementation spec
+├── Package.swift               # SPM package definition
+├── .swiftlint.yml             # Linting rules
+├── agents.md                   # AI agent guide (this file)
+└── .github/workflows/test.yml # CI configuration
+```
</file context>

[Copilot]

Pull request overview

This is a substantial pull request that transforms Quilt from a basic CRDT into a production-ready rich text collaborative editing library. The PR implements the Peritext CRDT algorithm with tombstone-based deletion, sophisticated mark operations, and several performance optimizations. It also adds undo/redo functionality, persistence, and comprehensive testing including fuzzing and property-based tests. The PR includes autonomous development loop infrastructure (Ralph) for iterative development.

Changes:

• Implements full Peritext CRDT with character metadata, tombstones, and intent-preserving formatting
• Adds undo/redo support via compensating operations (with critical bugs in collaborative scenarios)
• Implements persistence layer with JSON and binary serialization
• Adds performance optimizations: O(1) OpID lookups, render caching (24-44% improvements)
• Extends mark types: links (non-expanding), color, fontFamily, fontSize, highlight
• Adds 75+ new tests including fuzzing, property-based, and stress tests
• Upgrades to Swift 6.2 (version may not exist) and adds @retroactive Codable conformance

Reviewed changes

Copilot reviewed 18 out of 20 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
specs/peritext-implementation.md	Comprehensive 801-line specification document for Peritext implementation (minor discrepancy in code example)
Sources/Quilt/Quilt.swift	Major refactor: adds OpID index, undo/redo, persistence hooks, mark application algorithm (critical bugs in undo with merge)
Sources/Quilt/CharacterMetadata.swift	New structure for rich character metadata with formatting marks
Sources/Quilt/MarkType.swift	Extended with 4 new mark types (link, color, fontFamily, fontSize, highlight)
Sources/Quilt/UndoManager.swift	New undo/redo manager using compensating operations (design issue with visible indices)
Sources/Quilt/Persistence.swift	New persistence layer with JSON/binary serialization and versioning
Sources/Quilt/Character+Codable.swift	Adds @retroactive Codable conformance for Swift 6
Tests/QuiltTests/QuiltTests.swift	Adds 650+ lines of new tests for Peritext, undo/redo, persistence (missing collaborative undo tests)
Tests/QuiltTests/EnhancedTests.swift	New file with 391 lines of fuzzing, property-based, and stress tests
Package.swift	Upgrades to Swift 6.2, removes external swift-testing dependency
.ralph/*	New autonomous development loop infrastructure with hardcoded paths
AGENTS.md	262-line agent guide (outdated, lists implemented features as future)
IMPLEMENTATION_PLAN.md	327-line implementation tracking document
.claude/*	Claude configuration and prompts for autonomous development

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The undo/redo implementation has a correctness issue in collaborative scenarios. When undoing an insert operation, it stores and uses the visible index from when the operation was performed. However, after merging with concurrent operations from other users, the visible indices may have shifted. For example: User A inserts 'X' at visible index 5, then User B concurrently inserts 'Y' at index 3. After merge, 'X' is now at index 6. When User A undos, it will try to remove at index 5 (the stored value), which could now be a different character. The fix is to store the OpID of the inserted character in UndoableAction and use opIdToIndex to find its current position when undoing, rather than relying on the stale visible index.

[Copilot]

The merge() function does not clear or invalidate the undo history. This means that after merging with another document, the undo stack may contain UndoableAction entries with visible indices that are no longer valid due to operations from the merged document. Consider calling clearUndoHistory() after a merge, or documenting that undo/redo is only supported for local operations before any merge occurs.

[Copilot]

The test suite lacks coverage for undo/redo in collaborative scenarios. There are no tests that verify the behavior of undo after a merge operation, which would expose the bug where visible indices become stale after concurrent operations are merged. Consider adding tests like: 1) User A inserts, User B inserts concurrently, documents merge, User A undos - verify correct character is removed. 2) Test undo after multiple merges with different users.

[Copilot]

The REPO_DIR variable is hardcoded to /Users/theo/repos/quilt/Quilt which won't work for other users. Consider making this configurable via an environment variable with a sensible default, or detecting it dynamically using: REPO_DIR="${REPO_DIR:-$(cd "$(dirname "$0")/.." && pwd)}"

Suggested change

	REPO_DIR="/Users/theo/repos/quilt/Quilt"
	REPO_DIR="${REPO_DIR:-$(cd "$(dirname "$0")/.." && pwd)}"

[Copilot]

The AGENTS.md documentation is outdated. Line 256 lists "Undo/redo support" under "Future Considerations (not yet implemented)", but undo/redo has actually been implemented in this PR via UndoManager.swift and integrated into Quilt.swift. This section should be updated to reflect the current state and remove undo/redo from the "not yet implemented" list.

Suggested change

- **Undo/redo support** - Leverage operation log for history navigation

[Copilot]

The deduplication algorithm in the specification document uses .min to select the winner (line 521), but the text description says "the operation with the greatest OpID wins". The actual implementation correctly uses .max to select the greatest OpID. The specification code example should be corrected to use .max { $0.opId < $1.opId }! instead of .min { $0.opId < $1.opId }! to match both the description and the actual implementation.

[Copilot]

The Ralph loop scripts contain hardcoded absolute paths (/Users/theo/repos/quilt/Quilt) that won't work for other users or in CI/CD environments. These should be made relative or use environment variables like $PWD or detect the repository root dynamically using git rev-parse --show-toplevel.

[Copilot]

The hardcoded path /Users/theo/repos/quilt/Quilt on line 4 should be replaced with a relative path or dynamically determined. Consider using: cd "$(dirname "$0")/.." to go to the repository root, or use $REPO_DIR if it's meant to be configurable.

Suggested change

	cd /Users/theo/repos/quilt/Quilt
	cd "$(dirname "$0")/.."

[Copilot]

The AGENTS.md documentation lists several features as "not yet implemented" that have actually been implemented in this PR: operation log compaction (line 255), additional mark types including links/fonts/highlights (line 257), performance optimization (line 258), and persistence layer (line 259). The "Future Considerations" section should be updated to move these to a "Completed" section or removed from this list.

Suggested change

	## Future Considerations
	Potential areas for expansion (not yet implemented):
	- **Operation log compaction/garbage collection** - Remove redundant operations, compress tombstones
	- **Undo/redo support** - Leverage operation log for history navigation
	- **Additional mark types** - Links, font families, font sizes, highlights, nested marks
	- **Performance optimization** - Incremental rendering for very large documents
	- **Persistence layer** - Serialize/deserialize with efficient encoding
	## Completed Features
	The following features described in the Peritext spec are implemented in Quilt:
	- **Operation log compaction/garbage collection** - Remove redundant operations, compress tombstones
	- **Additional mark types** - Links, font families, font sizes, highlights, nested marks
	- **Performance optimizations** - Support for efficient handling of very large documents
	- **Persistence layer** - Serialize/deserialize with efficient encoding
	## Future Considerations
	Potential areas for expansion (not yet implemented):
	- **Undo/redo support** - Leverage operation log for history navigation