chore(gas): add test for history with holes #3477

MasterPtato · 2025-11-18T02:30:08Z

No description provided.

vercel · 2025-11-18T02:30:14Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Ready	Preview	Comment	Nov 22, 2025 2:46am

3 Skipped Deployments

Project	Deployment	Preview	Updated (UTC)
rivet-cloud	Ignored		Nov 22, 2025 2:46am
rivet-inspector	Ignored	Preview	Nov 22, 2025 2:46am
rivet-site	Ignored	Preview	Nov 22, 2025 2:46am

MasterPtato · 2025-11-18T02:30:25Z

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

_{An organization admin has enabled the Graphite Merge Queue in this repository.} _{Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.}

This stack of pull requests is managed by Graphite. Learn more about stacking.

claude · 2025-11-18T02:31:19Z

PR Review: Add test for history with holes

Summary

This PR adds two test cases (holes_in_coordinates and holes_in_coordinates2) to verify that the cursor correctly handles sparse coordinate sequences in workflow history. The tests validate important edge cases for the gasoline workflow engine's history traversal mechanism.

Code Quality ✅

Positives:

Tests follow existing patterns and conventions in the file
Clear test names that describe the scenario being tested
Good use of the existing test macros (loc![], coord![])
Tests are focused and verify specific behavior

Suggestions:

Test documentation: Consider adding doc comments to explain what "holes in coordinates" means and why this edge case matters:

/// Tests that cursor correctly handles non-sequential coordinates in history.
/// When coordinates have gaps (e.g., 1, 3, 6), the cursor should:
/// - Navigate through existing events correctly
/// - Continue from the last coordinate when reaching the end
#[test]
fn holes_in_coordinates() {

Test naming: The second test is named holes_in_coordinates2 which doesn't clearly communicate how it differs from the first. Consider a more descriptive name like holes_in_coordinates_insertion or insert_between_sparse_coordinates since it specifically tests insertion behavior.

Code duplication: Both tests create the exact same event structure. Consider extracting this into a helper function to reduce duplication:

fn create_sparse_history() -> Arc<HashMap<Location, Vec<Event>>> {
    [(
        loc![],
        vec![
            Event {
                coordinate: coord![1],
                version: 1,
                data: EventData::VersionCheck,
            },
            Event {
                coordinate: coord![3],
                version: 1,
                data: EventData::VersionCheck,
            },
            Event {
                coordinate: coord![6],
                version: 1,
                data: EventData::VersionCheck,
            },
        ],
    )]
    .into_iter()
    .collect::<HashMap<_, _>>()
    .into()
}

Test Coverage ✅

The tests cover important scenarios:

Test 1: Validates that the cursor navigates through sparse coordinates and continues correctly after the last event
Test 2: Validates that insertions between sparse coordinates follow the correct cardinality rules (inserting 1.1 between coordinates 1 and 3)

Additional test cases to consider:

Inserting between coordinates 3 and 6 (should be 3.1 based on the logic in current_location_for)
Multiple insertions in the same gap
Sparse coordinates at deeper nesting levels (e.g., coord![1, 1], coord![1, 5])

Potential Issues 🔍

No bugs detected. The tests correctly verify the expected behavior based on the Cursor implementation:

Lines 43-66 in cursor.rs show coord_at() handles sparse coordinates by continuing from the last coordinate's head
Lines 81-118 show current_location_for() uses cardinality comparison to determine insertion points
The assertions align with this logic

Performance Considerations ✅

No performance concerns. These are unit tests with minimal overhead.

Security Concerns ✅

No security issues. Standard test code.

Adherence to CLAUDE.md Guidelines ✅

✅ Uses appropriate test structure
✅ Follows existing code patterns
✅ Uses structured event data correctly
⚠️ Could benefit from the testing guideline about piping output to /tmp/ and grepping, though for unit tests this may not be necessary

Recommendation

Approve with minor suggestions. The tests are solid and cover important edge cases. The suggested improvements (documentation, naming, deduplication) would enhance maintainability but aren't blocking issues.

🤖 Generated with Claude Code

claude · 2025-11-18T23:51:39Z

PR Review

Summary

This PR adds test coverage for an important edge case in the history cursor implementation: handling "holes" in coordinate sequences (e.g., coordinates 1, 3, 6 instead of 1, 2, 3). The tests verify correct behavior when events have non-sequential coordinates.

Code Quality ✅

Strengths:

Good test naming: holes_in_coordinates and holes_in_coordinates2 clearly describe what's being tested
Clear assertions: Each assertion includes a descriptive comment explaining what should happen
Consistent style: Follows existing test patterns in the file (uses coord! and loc! macros)
Good coverage: Tests both cursor advancement through holes AND insertion behavior with holes

Test Coverage ✅

The tests cover two important scenarios:

holes_in_coordinates: Verifies that the cursor correctly traverses events with gaps in coordinates (1→3→6) and that subsequent coordinates after the last event continue from the last coordinate (7 after 6)
holes_in_coordinates2: Verifies insertion logic when holes exist - specifically that inserting after coordinate 1 (before coordinate 3) produces coord![1, 1] rather than coord![2]

Both tests are valuable for preventing regressions in coordinate handling logic.

Suggestions for Improvement

1. Better Test Names

Consider more descriptive names that explain the behavior being tested:

#[test]
fn cursor_traverses_sparse_coordinates_correctly()  // instead of holes_in_coordinates

#[test]
fn insertion_between_sparse_coordinates_uses_nested_coordinate()  // instead of holes_in_coordinates2

2. Consolidate or Differentiate Tests

holes_in_coordinates and holes_in_coordinates2 use identical event setup but test different behaviors. Consider:

Option A: Combine them into a single comprehensive test

#[test]
fn sparse_coordinate_handling() {
    // Setup (same events)
    
    // Test 1: Cursor traversal
    // ... existing holes_in_coordinates logic ...
    
    // Test 2: Insertion behavior  
    let mut cursor2 = Cursor::new(Arc::clone(&events), Location::empty());
    cursor2.update(&cursor2.current_location_for(&HistoryResult::<()>::Event(())));
    let new = cursor2.current_location_for(&HistoryResult::<()>::Insertion);
    assert_eq!(loc![coord![1, 1]], new);
}

Option B: Make the second test name more distinct to clarify it tests a different aspect

3. Test Edge Cases

Consider adding tests for:

Very large gaps (e.g., coordinates 1, 100, 1000)
Multiple insertions between the same sparse coordinates
Sparse coordinates with nested levels (e.g., coord![1], coord![5, 1], coord![10])

4. Add Comments Explaining Why Holes Matter

A brief comment explaining when/why coordinate holes can occur in practice would help future maintainers understand the importance of these tests:

/// Tests cursor behavior when history has gaps in coordinate sequences.
/// This can occur when events are forgotten/removed from history, or when
/// specific coordinates are deliberately chosen for ordering purposes.
#[test]
fn holes_in_coordinates() {

Potential Issues ⚠️

Minor: Code Duplication

The event setup is duplicated between the two tests. While not a major issue for tests, extracting a helper function could improve maintainability:

fn create_sparse_coordinate_events() -> HashMap<Location, Vec<Event>> {
    [(
        loc![],
        vec![
            Event {
                coordinate: coord![1],
                version: 1,
                data: EventData::VersionCheck,
            },
            Event {
                coordinate: coord![3],
                version: 1,
                data: EventData::VersionCheck,
            },
            Event {
                coordinate: coord![6],
                version: 1,
                data: EventData::VersionCheck,
            },
        ],
    )]
    .into_iter()
    .collect()
}

Performance Considerations ✅

No performance concerns - these are lightweight unit tests.

Security Concerns ✅

No security issues identified.

Overall Assessment

Verdict: Approve with minor suggestions ✅

This PR adds valuable test coverage for an important edge case. The tests are well-written and follow existing patterns. The suggestions above are minor improvements that could enhance clarity and maintainability, but they're not blockers for merging.

Great work adding this test coverage! Testing sparse coordinate handling is important for ensuring the workflow history system remains robust.

claude · 2025-11-18T23:51:57Z

Code Review

Thanks for adding test coverage for history cursor behavior with coordinate holes! This is valuable for ensuring the cursor handles non-sequential coordinates correctly.

✅ Positive Observations

Good test coverage: The tests validate an important edge case where coordinates have gaps (e.g., 1, 3, 6 instead of 1, 2, 3)
Consistent with existing patterns: The tests follow the same structure and style as other tests in the file (using the coord! and loc! macros, creating events with HashMap, etc.)
Clear test intent: The test names clearly describe what's being tested

🔍 Issues & Suggestions

1. Test naming: `holes_in_coordinates2` is not descriptive

#[test]
fn holes_in_coordinates2() {

Following the pattern of other tests in this file (e.g., cardinality, cardinality2, insert_before_first), this test should have a more descriptive name that explains what specific scenario it's testing. Based on the comment on line 850, this appears to be testing insertion behavior when there are holes, not just coordinate traversal.

Suggestion: Consider renaming to something like:

holes_in_coordinates_insertion
insert_between_holes
insert_after_first_with_holes

2. Missing test assertion context

In holes_in_coordinates (line 781), the test verifies that after processing all events, cursor.current_coord() returns coord![7]. While this is correct based on the implementation in coord_at() (lines 43-67), a comment explaining why the next coordinate is 7 (i.e., last event was at 6, so next is 6 + 1 = 7) would improve test readability.

3. Test could validate more edge cases

The holes_in_coordinates2 test only checks insertion after coordinate 1. Consider also testing:

Insertion after coordinate 3 (would it be coord![3, 1]?)
Insertion after coordinate 6 (would it be coord![6, 1]?)

This would provide more comprehensive coverage of the insertion logic when holes exist.

4. Potential for duplicate test logic

Looking at line 654 (coord_with_sparse_events), there's already a test for sparse/non-sequential coordinates. While your tests add value by testing the full cursor update flow (not just coord_at()), it might be worth considering if these tests could be consolidated or if the distinction should be made clearer in comments.

📝 Minor Observations

Both tests use the same event setup (coordinates 1, 3, 6). This is fine for consistency, but using different hole patterns in one test could catch more edge cases
The tests properly use Arc::new(events) to match the History type requirement
Good use of Location::empty() for the root location in the first test

Summary

The tests are well-structured and provide valuable coverage. The main improvement would be renaming holes_in_coordinates2 to be more descriptive and potentially adding a few more assertions to test insertion at different positions when holes exist.

Recommendation: Approve with minor suggested improvements.

claude · 2025-11-19T22:07:40Z

PR Review: Add test for history with holes

Overview

This PR adds two test cases to verify the behavior of the history cursor when dealing with sparse/non-contiguous event coordinates. The tests validate that the cursor correctly handles "holes" in coordinate sequences.

Code Quality ✅

Strengths:

Tests are well-structured and follow existing patterns in the test module
Clear test names that describe what they're testing: holes_in_coordinates and holes_in_coordinates2
Good use of descriptive comments explaining what each assertion validates
Follows the existing macro patterns (loc!, coord!) for test data construction
Tests are focused and test specific edge cases

Areas for improvement:

Test Naming - Consider more descriptive names that differentiate the two tests:
- holes_in_coordinates → holes_in_coordinates_sequential_traversal
- holes_in_coordinates2 → holes_in_coordinates_insertion_between_holes
This makes it clearer that the first test validates sequential cursor movement through holes, while the second validates insertion behavior between holes.

Code Duplication - Both tests use identical event setup. Consider extracting this into a helper function:

fn create_sparse_history() -> Arc<HashMap<Location, Vec<Event>>> {
    Arc::new([(
        loc![],
        vec![
            Event {
                coordinate: coord![1],
                version: 1,
                data: EventData::VersionCheck,
            },
            Event {
                coordinate: coord![3],
                version: 1,
                data: EventData::VersionCheck,
            },
            Event {
                coordinate: coord![6],
                version: 1,
                data: EventData::VersionCheck,
            },
        ],
    )]
    .into_iter()
    .collect())
}

Test Coverage ✅

The tests provide good coverage for this edge case:

✅ Sequential cursor movement through sparse coordinates
✅ Coordinate generation after exhausting sparse events
✅ Insertion coordinate calculation between holes

Additional test cases to consider:

Holes with nested coordinates (e.g., coord![1], coord![2, 1], coord![5])
Multiple consecutive holes at the end of the event list
Insertion behavior at different positions (before first hole, after last hole)

Correctness ✅

The test logic appears correct:

First test validates that current_coord() returns the actual sparse coordinates (1, 3, 6) and then continues sequentially (7)
Second test validates that inserting after coordinate 1 (before hole at 2) produces coord![1, 1] - this follows the cardinality rules from current_location_for

The assertions align with the cursor logic in coord_at() (lines 43-67) and current_location_for() (lines 81-118).

Performance Considerations ✅

No performance concerns - these are unit tests with minimal overhead.

Security Concerns ✅

No security implications.

Best Practices

Minor suggestions:

Consider adding a doc comment above each test explaining the specific scenario being tested
The tests could benefit from more assertions on intermediate states (e.g., checking prev_coord after updates)

Summary

This is a solid addition to the test suite. The tests are correct, well-written, and test important edge cases in the cursor's coordinate handling logic. The suggested improvements are minor and mostly focused on maintainability.

Recommendation: ✅ Approve with minor suggestions

The code is ready to merge as-is, but the suggested refactoring (extract helper, improve naming) would enhance long-term maintainability.

claude · 2025-11-19T22:09:42Z

Code Review

Summary

This PR adds two test cases (holes_in_coordinates and holes_in_coordinates2) to verify cursor behavior when history events have non-contiguous coordinate sequences. The tests validate that the cursor correctly handles "holes" in coordinate numbering.

Positive Observations

✅ Good test coverage: The tests address an important edge case - sparse coordinate sequences in history events

✅ Clear test structure: Both tests follow the existing test patterns with well-organized setup and assertions

✅ Comprehensive scenarios:

Test 1 validates sequential advancement through sparse coordinates
Test 2 validates insertion behavior between sparse coordinates

Issues & Recommendations

1. Code Duplication (Medium Priority)

The two test functions share almost identical setup code (lines 782-803 vs 824-845). Consider extracting the event setup into a helper function:

fn create_sparse_events() -> HashMap<Location, Vec<Event>> {
    [(
        loc![],
        vec![
            Event {
                coordinate: coord![1],
                version: 1,
                data: EventData::VersionCheck,
            },
            Event {
                coordinate: coord![3],
                version: 1,
                data: EventData::VersionCheck,
            },
            Event {
                coordinate: coord![6],
                version: 1,
                data: EventData::VersionCheck,
            },
        ],
    )]
    .into_iter()
    .collect()
}

Location: engine/packages/gasoline/src/history/cursor.rs:782-845

2. Test Naming (Low Priority)

holes_in_coordinates2 is not very descriptive. Consider renaming to holes_in_coordinates_insertion or insert_between_sparse_coordinates to better describe what aspect of holes it is testing.

Location: engine/packages/gasoline/src/history/cursor.rs:823

3. Missing Documentation (Low Priority)

While the existing tests have doc comments explaining their purpose (see lines 678-679, 691-692), these new tests lack them. Consider adding:

/// Verify cursor advances correctly through events with non-contiguous coordinates (1, 3, 6)
#[test]
fn holes_in_coordinates() {
    // ...
}

/// Verify insertions work correctly between events with sparse coordinates
#[test]
fn holes_in_coordinates_insertion() {
    // ...
}

Location: engine/packages/gasoline/src/history/cursor.rs:780-853

Code Quality Assessment

Correctness: ✅ The test logic appears sound and validates the expected behavior
Style: ✅ Follows existing conventions (hard tabs, pattern macros, etc.)
Performance: ✅ No concerns - these are unit tests
Security: N/A

Test Coverage Analysis

The tests effectively cover:

Sequential cursor advancement through sparse coordinates
Correct coordinate calculation after consuming all events (coord![7] after coord![6])
Insertion location between sparse coordinates (coord![1, 1] between coord![1] and coord![3])

However, consider adding tests for:

Insertions at other positions (e.g., between 3 and 6)
Larger gaps in coordinates
Edge case: coordinate 0 handling with holes
Multi-level coordinate hierarchies with holes (e.g., coord![1, 3] to coord![1, 7])

Recommendation

Approve with minor suggestions. The core functionality is solid, but addressing the code duplication would improve maintainability. The other suggestions are nice-to-haves that can be addressed in a follow-up if preferred.

🤖 Generated with Claude Code

claude · 2025-11-20T03:08:55Z

PR Review

Summary

This PR adds two test cases (holes_in_coordinates and holes_in_coordinates2) to validate the cursor behavior when event coordinates have gaps (non-consecutive coordinate values). This is important for ensuring the workflow history system handles sparse event histories correctly.

Code Quality ✅

Strengths:

Tests follow existing patterns and conventions in the test module
Good use of the loc! and coord! macros for readability
Test names are descriptive and follow the existing naming convention
Tests are well-commented explaining what each assertion verifies

Observations:

Both tests are very similar - they set up identical event data but test different scenarios:
- holes_in_coordinates: Tests sequential iteration through sparse coordinates
- holes_in_coordinates2: Tests insertion behavior after the first event

Test Coverage ✅

The tests validate critical cursor behavior:

Sequential traversal through sparse coordinates (test 1):
- Correctly identifies coordinates 1, 3, 6 despite gaps
- Verifies that after exhausting events, the cursor continues from the last coordinate (6 → 7)
Insertion logic with sparse coordinates (test 2):
- After processing coordinate 1, inserting before coordinate 3 correctly produces coordinate 1.1
- This validates the cardinality logic in current_location_for when HistoryResult::Insertion is used

These tests complement existing tests like coord_with_sparse_events and provide good coverage for edge cases in coordinate handling.

Potential Issues

Minor: Code duplication
The two tests share identical event setup. Consider extracting this into a helper function if more tests with this pattern are added in the future. However, this is a minor optimization and not necessary for just two tests.

Performance Considerations ✅

Tests are lightweight unit tests with minimal overhead
No performance concerns

Security Concerns ✅

No security implications for test code

Best Practices

Follows repository conventions:

✅ Uses structured test organization
✅ Includes descriptive assertions with comments
✅ Follows existing test patterns in the module
✅ Commit message follows conventional commits format: chore(gas): add test for history with holes

Suggestion: Consider adding a test case that validates error handling or edge cases like:

What happens with very large gaps in coordinates?
What happens when inserting between non-sequential multi-level coordinates (e.g., between 1.1 and 3.2.1)?

These could be follow-up tests if such scenarios are relevant to the workflow engine.

Recommendation

Approve with optional enhancement ✅

The tests are well-written, validate important cursor behavior with sparse coordinates, and follow all repository conventions. The code is ready to merge.

Optional enhancement: Extract the shared event setup into a helper function if you anticipate adding more tests with this pattern, but this is not blocking.

Great work on adding test coverage for this edge case! 🎯

claude · 2025-11-20T03:09:07Z

Code Review

I've reviewed PR #3477 and here's my feedback:

Overview

This PR adds two test cases (holes_in_coordinates and holes_in_coordinates2) to verify the cursor behavior when event coordinates have gaps ("holes") in the sequence.

Positive Aspects

✅ Good test coverage: These tests address an important edge case - sparse event histories where coordinates don't follow a continuous sequence (e.g., 1, 3, 6 instead of 1, 2, 3).

✅ Clear test structure: Both tests follow the existing pattern established in the file, using the loc! and coord! macros consistently.

✅ Good documentation: Comments explain what each assertion is checking, making the tests easy to understand.

✅ Comprehensive validation: The first test validates cursor traversal through all events with holes, while the second test checks insertion behavior between events with holes.

Issues & Suggestions

1. Test Naming (Minor)

The test name holes_in_coordinates2 is not very descriptive. Consider renaming it to better describe what it's testing:

#[test]
fn holes_in_coordinates2() {  // Current
// Suggested alternatives:
fn insert_between_holes_in_coordinates() {
fn insertion_with_coordinate_holes() {

2. Code Duplication (Moderate)

Both tests have identical event setup code (lines 783-802 vs 825-844). This violates DRY principles and makes maintenance harder. Consider extracting a helper function:

fn create_events_with_holes() -> HashMap<Location, Vec<Event>> {
    [(
        loc![],
        vec![
            Event {
                coordinate: coord![1],
                version: 1,
                data: EventData::VersionCheck,
            },
            Event {
                coordinate: coord![3],
                version: 1,
                data: EventData::VersionCheck,
            },
            Event {
                coordinate: coord![6],
                version: 1,
                data: EventData::VersionCheck,
            },
        ],
    )]
    .into_iter()
    .collect()
}

#[test]
fn holes_in_coordinates() {
    let events = create_events_with_holes();
    let mut cursor = Cursor::new(Arc::new(events), Location::empty());
    // ... rest of test
}

3. Test Independence (Minor)

holes_in_coordinates2 could be more self-contained. Currently it only tests insertion after the first event. Consider adding assertions about the initial state for completeness.

4. Edge Case Coverage (Suggestion)

While these tests cover basic hole scenarios, consider adding tests for:

Holes at the beginning (e.g., coordinates starting at 5, 7, 10)
Large gaps (e.g., coordinates 1, 100, 1000)
Nested coordinate holes (e.g., coord![1, 5], coord![1, 10])

Code Quality Assessment

Overall: Good ✓

Follows project conventions (hard tabs, existing test patterns)
No security concerns (test-only code)
No performance concerns (tests are reasonably sized)
Adds value by testing an important edge case

Recommendation

Approve with minor suggestions - The tests are functionally correct and valuable. The suggestions above would improve maintainability but aren't blocking issues.

Review generated by Claude Code

claude · 2025-11-21T19:58:19Z

PR Review: chore(gas): add test for history with holes

Summary

This PR adds two test cases (holes_in_coordinates and holes_in_coordinates2) to verify cursor behavior when events have non-contiguous coordinates in the workflow history. This is valuable for testing edge cases in the cursor traversal logic.

Code Quality ✅

Positives:

Tests follow existing patterns and conventions in the file (using the loc! and coord! macros)
Good documentation via comments explaining what each assertion tests
Test structure matches existing tests in the module
Clear, descriptive test names

Suggestions for Improvement

Consider renaming holes_in_coordinates2 - The name could be more descriptive. Consider something like insertion_between_sparse_coordinates to better describe what the test validates.
Add more edge case coverage - Consider adding tests for:
- Empty events with holes
- Multiple insertions between sparse coordinates
- Coordinates with holes at different cardinalities (e.g., coord![1, 1], coord![1, 5], coord![1, 10])
Minor: Test documentation - The second test could benefit from a doc comment similar to other tests in the file (e.g., /// Insert between sparse coordinates should produce correct location)

Potential Issues ⚠️

None identified. The tests are straightforward and correctly exercise the cursor logic.

Performance Considerations

No concerns - these are unit tests with small, fixed data sets.

Security Concerns

None - this is test code only.

Test Coverage

This PR improves test coverage for an important edge case. The cursor's handling of sparse/non-contiguous coordinates is a valid scenario that should be tested.

Verdict

Approve ✅ - This is a well-written, focused addition of test coverage for sparse coordinate handling in the cursor implementation. The tests are clear and follow existing conventions.

🤖 Generated with Claude Code

pkg-pr-new · 2025-11-21T19:59:49Z

More templates

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@3477

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@3477

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@3477

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@3477

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@3477

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@3477

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@3477

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@3477

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@3477

commit: 4d81dfd

claude · 2025-11-21T19:59:54Z

PR Review: chore(gas): add test for history with holes

Summary

This PR adds two unit tests (holes_in_coordinates and holes_in_coordinates2) to verify that the Cursor correctly handles sparse coordinate sequences in workflow history where there are gaps between event coordinates.

Code Quality ✅

Good practices observed:

Tests follow the existing test structure and patterns in the file
Uses the established loc![] and coord![] macros for readability
Clear, descriptive comments explaining what each assertion is testing
Tests cover both cursor traversal (holes_in_coordinates) and insertion behavior (holes_in_coordinates2)

Suggestions

Consider more descriptive test names: The tests could have more descriptive names that indicate what behavior they're validating:
- holes_in_coordinates → cursor_traverses_sparse_coordinates_correctly
- holes_in_coordinates2 → insertion_between_sparse_coordinates_uses_sub_coordinate
Consider adding edge case tests: While these tests cover basic hole scenarios, additional edge cases could strengthen coverage:
- Events starting at coordinate > 1 (e.g., first event at coordinate 5)
- Larger gaps between coordinates
- Mixing simple and complex coordinates with holes
Minor: HashMap type annotation: In holes_in_coordinates, the explicit type annotation collect::<HashMap<_, _>>() is used, while in holes_in_coordinates2 it's also used. This is consistent, which is good. However, I notice the other tests in the file use .into_iter().collect() without the type annotation. Consider being consistent with the rest of the test file (either add annotations everywhere or remove them where type inference works).

Potential Issues 🔍

None identified. The tests appear correct and consistent with the cursor implementation logic in coord_at() and current_location_for().

Test Coverage ✅

These tests fill an important gap in coverage - ensuring the cursor handles non-contiguous coordinate sequences, which is important for workflows where events may have been removed or where insertions have occurred between events.

Verdict

Approve - This is a straightforward, well-written test addition that improves coverage for an important edge case in the workflow history cursor.

🤖 Generated with Claude Code

claude · 2025-11-22T02:47:08Z

PR Review: chore(gas): add test for history with holes

Summary

This PR adds two new test cases (holes_in_coordinates and holes_in_coordinates2) to verify cursor behavior when there are gaps in coordinate sequences in the workflow history.

✅ What looks good

Test coverage: The tests cover an important edge case - sparse coordinates in history (e.g., coordinates 1, 3, 6 with gaps at 2, 4, 5). This is valuable for ensuring the cursor correctly handles real-world scenarios where events may have been removed or coordinates are non-contiguous.
Test structure: The tests follow the existing patterns in the file and use the established coord! and loc! macros consistently.
Clear assertions: The comments explain what each assertion is verifying.

🔍 Suggestions for improvement

Consider a more descriptive name for holes_in_coordinates2: The second test specifically tests insertion behavior between sparse coordinates. A name like insertion_between_sparse_coordinates would better communicate intent.
Minor code style: Lines 808, 812, 816, and 848 create a temporary value for current_location_for that is immediately consumed by update. While this works, it reads a bit awkwardly. Consider either:
- Splitting into two lines for clarity:
```
let loc = cursor.current_location_for(&HistoryResult::<()>::Event(()));
cursor.update(&loc);
```
- Or adding a helper method if this pattern is common in tests
Test documentation: Consider adding a doc comment similar to the other tests (e.g., the /// Before 1 is 0.1 style comments above other test functions) to briefly describe the scenario being tested.

✅ Verification

The tests are well-structured and should compile/run correctly
No security concerns
No performance issues (these are unit tests)
The test logic correctly validates the expected behavior based on the coord_at and current_location_for implementations

Verdict

Approve - This is a good addition to the test suite. The suggestions above are minor and optional improvements.

🤖 Generated with Claude Code

claude · 2025-11-22T02:49:13Z

PR Review: chore(gas): add test for history with holes

Summary

This PR adds two unit tests (holes_in_coordinates and holes_in_coordinates2) for the Cursor implementation in the gasoline workflow engine. These tests verify that the cursor correctly handles sparse event coordinates with gaps ("holes") between them.

Code Quality & Best Practices ✅

Positive aspects:

Tests follow existing patterns in the file (same structure as coord_with_sparse_events, cardinality, etc.)
Good use of the existing test macros (loc![], coord![])
Clear, descriptive comments explaining the test scenarios
Tests cover both event traversal and insertion scenarios with holes

Suggestions:

Test naming: Consider renaming holes_in_coordinates2 to something more descriptive like insertion_between_holes or insert_after_first_event_with_holes to clarify what scenario it's testing.
Minor DRY opportunity: Both tests use identical event setup. While test duplication is often acceptable for clarity, you could consider extracting the event setup into a helper function if more hole-related tests are added in the future:

fn create_events_with_holes() -> HashMap<Location, Vec<Event>> {
    [(
        loc![],
        vec![
            Event { coordinate: coord![1], version: 1, data: EventData::VersionCheck },
            Event { coordinate: coord![3], version: 1, data: EventData::VersionCheck },
            Event { coordinate: coord![6], version: 1, data: EventData::VersionCheck },
        ],
    )]
    .into_iter()
    .collect::<HashMap<_, _>>()
}

Potential Issues ⚠️

Type annotation redundancy: In holes_in_coordinates, line 803, you use .collect::<HashMap<_, _>>() which is good, but the same pattern isn't used consistently in all existing tests (some use just .collect() with type inference from Cursor::new). This is fine but worth noting the inconsistency exists in the broader test file.

Test Coverage 📊

The tests effectively cover:

✅ Forward traversal through events with coordinate gaps
✅ Coordinate calculation after exhausting events with holes
✅ Insertion location calculation between events with holes

Additional test cases to consider (optional):

Test with holes at the beginning (e.g., first event at coord [3] instead of [1])
Test insertion when cursor is past the last event with holes
Test with multi-component coordinates that have holes (e.g., [1, 1], [1, 3], [1, 6])

Performance Considerations ✅

No performance concerns - these are unit tests that create small in-memory data structures.

Security Concerns ✅

No security concerns - these are isolated unit tests with no I/O or external dependencies.

Overall Assessment

Approved ✅

This is a solid addition to the test suite that improves coverage for an important edge case in the cursor's coordinate handling. The tests are well-written and follow existing conventions. The minor suggestions above are optional improvements.

🤖 Generated with Claude Code

vercel bot temporarily deployed to Preview – rivetkit-serverless November 18, 2025 02:30 Inactive

This was referenced Nov 18, 2025

chore: add debug scripts #3467

Closed

chore: write/update docs #3468

Closed

fix(udb): update backoff algo #3469

Closed

chore: rename retry -> hibernate in runner protocol #3473

Closed

fix(gas): optimize loops #3475

Draft

NathanFlurry approved these changes Nov 18, 2025

View reviewed changes

MasterPtato force-pushed the 11-17-chore_change_streamingmode_to_exact_for_ranges_with_limits branch from 539e5ed to cd862c4 Compare November 18, 2025 23:50

MasterPtato force-pushed the 11-17-chore_gas_add_test_for_history_with_holes branch from 5c9b75f to 0471792 Compare November 18, 2025 23:50

This was referenced Nov 18, 2025

fix(gas): fix loop forgotten bug due to concurrency #3482

Draft

fix(pb): simplify runner wf #3483

Draft

vercel bot had a problem deploying to Preview – rivetkit-serverless November 18, 2025 23:53 Failure

MasterPtato force-pushed the 11-17-chore_gas_add_test_for_history_with_holes branch from 0471792 to f508120 Compare November 19, 2025 22:06

MasterPtato force-pushed the 11-17-chore_change_streamingmode_to_exact_for_ranges_with_limits branch from cd862c4 to fba43a9 Compare November 19, 2025 22:06

This was referenced Nov 19, 2025

chore(engine-runner): remove echo close on ToClientWebSocketClose #3487

Closed

chore(pegboard-gateway): remove TunnelAck #3488

Closed

fix(tunnel): implement ping pong #3491

Closed

NathanFlurry mentioned this pull request Nov 19, 2025

chore(pegboard-gateway): add new message id format & add deprecated tunnel ack #3492

Closed

vercel bot had a problem deploying to Preview – rivetkit-serverless November 19, 2025 22:07 Failure

MasterPtato force-pushed the 11-17-chore_change_streamingmode_to_exact_for_ranges_with_limits branch from fba43a9 to 00a1885 Compare November 20, 2025 03:07

MasterPtato force-pushed the 11-17-chore_gas_add_test_for_history_with_holes branch from f508120 to 512771d Compare November 20, 2025 03:07

vercel bot deployed to Preview – rivetkit-serverless November 20, 2025 03:08 View deployment

MasterPtato force-pushed the 11-17-chore_gas_add_test_for_history_with_holes branch from 512771d to 22947c1 Compare November 21, 2025 19:57

MasterPtato force-pushed the 11-17-chore_change_streamingmode_to_exact_for_ranges_with_limits branch from 00a1885 to 316c4a8 Compare November 21, 2025 19:57

MasterPtato mentioned this pull request Nov 21, 2025

fix: parallelize publish signal awaits #3523

Draft

vercel bot deployed to Preview – rivetkit-serverless November 21, 2025 19:59 View deployment

chore(gas): add test for history with holes

4d81dfd

MasterPtato force-pushed the 11-17-chore_gas_add_test_for_history_with_holes branch from 22947c1 to 4d81dfd Compare November 22, 2025 02:45

MasterPtato force-pushed the 11-17-chore_change_streamingmode_to_exact_for_ranges_with_limits branch from 316c4a8 to 1bb6d7a Compare November 22, 2025 02:45

This was referenced Nov 22, 2025

chore: fix diff between ee, fix plural api query params #3524

Open

fix: make all uses of protocol handle v4 <-> v3 #3528

Draft

vercel bot deployed to Preview – rivetkit-serverless November 22, 2025 02:46 View deployment

chore(gas): add test for history with holes #3477

Are you sure you want to change the base?

chore(gas): add test for history with holes #3477

Uh oh!

Conversation

MasterPtato commented Nov 18, 2025

Uh oh!

vercel bot commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MasterPtato commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to use the Graphite Merge Queue

Uh oh!

claude bot commented Nov 18, 2025

PR Review: Add test for history with holes

Summary

Code Quality ✅

Test Coverage ✅

Potential Issues 🔍

Performance Considerations ✅

Security Concerns ✅

Adherence to CLAUDE.md Guidelines ✅

Recommendation

Uh oh!

claude bot commented Nov 18, 2025

PR Review

Summary

Code Quality ✅

Test Coverage ✅

Suggestions for Improvement

1. Better Test Names

2. Consolidate or Differentiate Tests

3. Test Edge Cases

4. Add Comments Explaining Why Holes Matter

Potential Issues ⚠️

Minor: Code Duplication

Performance Considerations ✅

Security Concerns ✅

Overall Assessment

Uh oh!

claude bot commented Nov 18, 2025

Code Review

✅ Positive Observations

🔍 Issues & Suggestions

1. Test naming: holes_in_coordinates2 is not descriptive

2. Missing test assertion context

3. Test could validate more edge cases

4. Potential for duplicate test logic

📝 Minor Observations

Summary

Uh oh!

claude bot commented Nov 19, 2025

PR Review: Add test for history with holes

Overview

Code Quality ✅

Test Coverage ✅

Correctness ✅

Performance Considerations ✅

Security Concerns ✅

Best Practices

Summary

Uh oh!

claude bot commented Nov 19, 2025

Code Review

Summary

Positive Observations

Issues & Recommendations

1. Code Duplication (Medium Priority)

2. Test Naming (Low Priority)

3. Missing Documentation (Low Priority)

Code Quality Assessment

Test Coverage Analysis

Recommendation

Uh oh!

claude bot commented Nov 20, 2025

PR Review

Summary

Code Quality ✅

Test Coverage ✅

vercel bot commented Nov 18, 2025 •

edited

Loading

MasterPtato commented Nov 18, 2025 •

edited

Loading

1. Test naming: `holes_in_coordinates2` is not descriptive

pkg-pr-new bot commented Nov 21, 2025 •

edited

Loading