fix: use correct vbare functions for runner tunnel

[MasterPtato]

No description provided.

[vercel]

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

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Error			Nov 4, 2025 8:26pm

3 Skipped Deployments

Project	Deployment	Preview	Comments	Updated (UTC)
rivet-cloud	Ignored			Nov 4, 2025 8:26pm
rivet-inspector	Ignored	Preview		Nov 4, 2025 8:26pm
rivet-site	Ignored	Preview		Nov 4, 2025 8:26pm

[MasterPtato]

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

• fix: use correct vbare functions for runner tunnel #3300 : 2 dependent PRs (#3301 , #3332 ) 👈 (View in Graphite)
• feat(grafana): fix api dashboards #3193
• fix: docker compose build #3273
• chore: fix remaining warnings for all targets #3305
• fix(gas): fix from_workflow when creating standalone ctx from op ctx #3342
• ci: udpate ci filter to succeed if no relevant changes #3343
• main

---

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]

Code Review

Summary

This PR fixes the usage of vbare protocol serialization/deserialization functions in the pegboard runner tunnel code. The changes replace the deprecated serialize_version/deserialize_version + unwrap_latest pattern with the newer serialize/deserialize convenience methods.

Changes Overview

Files Modified:

• engine/packages/pegboard-runner/src/client_to_pubsub_task.rs (11 additions, 14 deletions)
• engine/packages/pegboard-runner/src/pubsub_to_client_task.rs (1 addition, 1 deletion)

---

✅ Positive Aspects

1. Correct API Usage: The PR correctly migrates from the low-level deserialize_version().and_then(|x| x.unwrap_latest()) pattern to the simpler deserialize() method, which handles version conversion automatically.

2. Consistency: After this change, all serialization/deserialization calls in these files use the correct vbare convenience methods (serialize and deserialize), improving codebase consistency.

3. Code Simplification: The client_to_pubsub_task.rs:36 change reduces 9 lines to 6 lines while maintaining the same functionality, improving readability.

4. No Functional Changes: The behavior remains identical since deserialize() internally calls deserialize_version() and applies version converters, which includes the unwrap_latest() functionality.

---

📋 Code Quality Assessment

Strengths:

• Error handling remains consistent with the existing pattern
• Structured logging format is preserved (?err, data_len)
• All call sites are updated consistently

Observations:

• The change aligns with the vbare OwnedVersionedData trait design, which provides serialize/deserialize as the primary API surface
• Looking at engine/sdks/rust/runner-protocol/src/versioned.rs, the trait implementation shows that:
  • deserialize(payload, version) automatically handles version conversion
  • serialize(version) automatically serializes to the correct format

---

🔍 Potential Concerns

None identified. This is a straightforward fix that:

1. Uses the correct API as designed by the vbare trait
2. Maintains identical behavior
3. Improves code clarity

---

🧪 Testing Recommendations

1. Protocol Compatibility: Verify that v1 and v2 protocol versions still work correctly:

   • Test with runners using protocol version 1
   • Test with runners using protocol version 2
   • Verify backward compatibility is maintained

2. Tunnel Operations: Test the full tunnel flow:

   • WebSocket message forwarding (client → pubsub)
   • Response serialization (pubsub → client)
   • KV operations (all request/response types)
   • Tunnel messages (HTTP and WebSocket variants)

3. Error Cases: Verify error handling:

   • Invalid protocol versions
   • Malformed messages
   • Deserialization failures

---

📊 Performance Considerations

No performance impact expected. The deserialize method internally calls the same deserialize_version function, so the execution path is essentially identical with potentially one less function call due to the combined operation.

---

🔒 Security Assessment

No security concerns. The change doesn't affect:

• Input validation
• Authentication/authorization
• Data sanitization
• Error information disclosure

---

📝 Recommendations

1. ✅ Approve and Merge: This is a clean, correct fix
2. 🧪 Test Coverage: If not already covered, consider adding integration tests that exercise the protocol version conversion logic
3. 📚 Documentation: Consider adding a comment in the vbare trait implementation or CLAUDE.md noting the preferred serialize/deserialize methods over serialize_version/deserialize_version

---

Conclusion

This PR correctly addresses a bug in protocol function usage. The changes are minimal, focused, and improve code quality without introducing any risks. Recommended for approval.

---

Review generated with Claude Code

[claude]

Code Review

This PR correctly updates the vbare function calls in the runner tunnel code. The changes look good overall and follow the proper patterns used elsewhere in the codebase.

✅ Positive Observations

1. Correct API Usage: The changes properly use the vbare helper methods that internally call the trait methods:

   • deserialize(buf, version) instead of deserialize_version(buf, version).and_then(|x| x.unwrap_latest())
   • serialize(version) instead of serialize_version(version)

2. Consistency: These changes align with the patterns used in epoxy-protocol (see epoxy-protocol/src/versioned.rs:42,82,122,126)

3. Improved Error Handling: In http_client.rs:138-140, moving .serialize() earlier and adding a .context() provides better error messages

4. Workflow Registration: Adding the purger workflow registration in lib.rs:22 appears intentional and correct

🔍 Code Quality Notes

client_to_pubsub_task.rs:36 - The simplified deserialization is cleaner and more idiomatic:

// Before: Two-step process
versioned::ToServer::deserialize_version(&data, conn.protocol_version)
    .and_then(|x| x.unwrap_latest())

// After: Direct deserialization to latest
versioned::ToServer::deserialize(&data, conn.protocol_version)

pubsub_to_client_task.rs:95 - Correct usage matching the serialization pattern

✅ Testing Considerations

The changes affect core message serialization/deserialization in the runner tunnel:

• Client → Server messages (client_to_pubsub_task.rs)
• Server → Client messages (pubsub_to_client_task.rs)
• Epoxy HTTP requests (http_client.rs)

Consider testing:

1. Runner connection establishment
2. Bidirectional message flow through the tunnel
3. KV operations over the tunnel
4. HTTP tunnel messages

Security & Performance

• ✅ No security concerns
• ✅ No performance regressions - these are equivalent operations with cleaner API surface
• ✅ Error handling is maintained or improved

Summary

LGTM - This is a straightforward fix that corrects the vbare API usage. The code quality is good and follows established patterns in the codebase.

[claude]

PR Review: Fix vbare function usage for runner tunnel

Overview

This PR corrects the usage of vbare versioned serialization/deserialization functions in the pegboard-runner and epoxy packages. The changes align the code with the proper vbare API patterns and fix a related issue in the epoxy registry.

Code Quality: ✅ Good

Positive aspects:

• Correct API usage: The changes properly use the vbare OwnedVersionedData trait methods
• Consistent patterns: All changes follow the same correction pattern across multiple files
• Clean diff: Small, focused changes that address a specific issue

Specific improvements:

1. client_to_pubsub_task.rs:36 - Simplified deserialization:

   // Before: Two-step process with unwrap_latest
   versioned::ToServer::deserialize_version(&data, conn.protocol_version)
       .and_then(|x| x.unwrap_latest())
   
   // After: Direct deserialization to latest version
   versioned::ToServer::deserialize(&data, conn.protocol_version)

   • ✅ Correct: The deserialize() method internally calls deserialize_version() and applies converters to return the Latest type directly
   • ✅ Eliminates unnecessary .unwrap_latest() call

2. pubsub_to_client_task.rs:95 - Fixed serialization method:

   // Before
   versioned::ToClient::wrap_latest(msg).serialize_version(conn.protocol_version)
   
   // After
   versioned::ToClient::wrap_latest(msg).serialize(conn.protocol_version)

   • ✅ Correct: serialize() applies version converters before serializing, while serialize_version() doesn't
   • This ensures proper version downgrading if the client protocol version is older

3. http_client.rs:138-140 - Error handling improvement:

   // Before: Deferred error handling
   let request = versioned::Request::wrap_latest(request);
   .body(request.serialize()?)
   
   // After: Early error handling
   let request = versioned::Request::wrap_latest(request)
       .serialize()
       .context("failed to serialize epoxy request")?;

   • ✅ Better error context with .context()
   • ✅ More explicit error handling before passing to body()

4. lib.rs:22 - Registry addition:

   registry.register_workflow::<purger::Workflow>()?;

   • ✅ Registers the purger workflow (likely missed in a previous PR)
   • This appears unrelated to the main vbare fix but is a valid correction

Potential Issues: ⚠️ Minor Concern

Missing context on purger workflow:

• The addition of purger::Workflow to the registry in epoxy/src/lib.rs:22 seems unrelated to the PR title
• Question: Is this intentional cleanup, or should it be in a separate PR?
• Impact: Low - the change is valid, but PR scope clarity would be helpful

Performance Considerations: ✅ Neutral/Positive

• Removed unnecessary step: Eliminating .unwrap_latest() in the deserialization path reduces function call overhead
• Proper version handling: Using serialize() instead of serialize_version() ensures correct version negotiation without performance penalties
• No allocations added: All changes maintain the same memory allocation patterns

Security Concerns: ✅ None

• No security issues identified
• Error handling is improved with better context
• Version handling is more robust with proper converter application

Test Coverage: ⚠️ Needs Verification

Recommendations:

1. Integration tests: Verify that the runner tunnel communication works correctly with these changes
2. Version compatibility: Test with clients using different protocol versions to ensure converters work properly
3. Purger workflow: If adding purger workflow registration, ensure it has corresponding tests

Suggested test approach:

# Run runner-related tests
cargo test -p pegboard-runner -- --nocapture 2>&1 | tee /tmp/runner_tests.log

# Run epoxy tests
cargo test -p epoxy -- --nocapture 2>&1 | tee /tmp/epoxy_tests.log

# Check for any deserialization/serialization errors
grep -i "deserialize\|serialize" /tmp/*.log

Style & Conventions: ✅ Compliant

• ✅ Follows existing error handling patterns with anyhow::Context
• ✅ Uses structured logging conventions (lowercase messages)
• ✅ Maintains existing code formatting
• ✅ Consistent with CLAUDE.md guidelines

Summary

Verdict: LGTM with minor clarification needed ✅

The PR correctly fixes the vbare API usage throughout the runner tunnel code. The changes are sound and improve code correctness. The only question is whether the purger workflow registration should be documented in the PR description or split into a separate change.

Recommendations:

1. ✅ Approve the vbare API fixes - they're correct
2. ⚠️ Clarify the purger workflow addition in the PR description
3. 📋 Run integration tests to verify runner tunnel functionality
4. 🔍 Consider adding a test that exercises version negotiation if one doesn't exist

Files reviewed:

• engine/packages/epoxy/src/http_client.rs ✅
• engine/packages/epoxy/src/lib.rs ⚠️ (unrelated change)
• engine/packages/pegboard-runner/src/client_to_pubsub_task.rs ✅
• engine/packages/pegboard-runner/src/pubsub_to_client_task.rs ✅

[claude]

PR Review: Use correct vbare functions for runner tunnel

Summary:
This PR updates the usage of vbare serialization/deserialization functions to use the correct helper methods instead of directly calling trait methods. The changes improve code consistency and error handling.

Code Quality - Positive aspects:

• Improved error handling in epoxy/src/http_client.rs:138-140: Serialization is now done with proper context using .context() before passing to the request body
• Consistent API usage: The changes align with the helper methods defined in the versioned protocol implementations
• Simplified deserialization logic in client_to_pubsub_task.rs:36: The change from deserialize_version().and_then(|x| x.unwrap_latest()) to just deserialize() is cleaner

Potential Issues:

1. Unrelated change in epoxy/src/lib.rs:22 - The addition of registry.register_workflow::purger::Workflow()? appears unrelated to the vbare functions theme of this PR
2. Please verify that the purger module is properly defined in the workflows module

Testing Recommendations:

• Test runner connectivity and tunnel messages
• Test protocol version compatibility (v1 and v2)
• Test KV operations with the new deserialization approach

Security: No concerns identified
Performance: Changes are performance-neutral or slightly positive

Recommendation: Approve with minor clarification needed on the purger workflow registration

[claude]

Code Review

Summary

This PR correctly updates the runner tunnel code to use the simplified vbare API methods (serialize() and deserialize()) instead of the lower-level version-specific methods. The changes are clean and improve code readability.

Positive Observations

✅ Correct API Usage: The migration from deserialize_version().and_then(|x| x.unwrap_latest()) to deserialize() is correct. Looking at epoxy-protocol/src/versioned.rs, the wrapper types provide convenience methods that handle versioning automatically.

✅ Improved Error Handling: In http_client.rs:138-140, the error handling is now cleaner with a proper context message for serialization failures.

✅ Code Simplification: The changes reduce boilerplate and make the code more maintainable:

• client_to_pubsub_task.rs:36: Simplified from a verbose match with and_then to a direct deserialize() call
• pubsub_to_client_task.rs:95: Cleaner serialization call

✅ Consistency: All three modified files now use the same pattern for serialization/deserialization, improving consistency across the codebase.

✅ Registry Registration: Good catch adding the purger::Workflow registration in lib.rs:22. This appears to be a separate but necessary fix.

Questions/Observations

1. Missing purger import? In epoxy/src/lib.rs, you're registering purger::Workflow but I don't see a mod purger; or use statement. Is this module defined elsewhere in the crate? Please verify this compiles.

2. Protocol version handling: The new serialize() and deserialize() methods automatically use PROTOCOL_VERSION. Verify this is the intended behavior for all call sites, especially:

   • client_to_pubsub_task.rs:36: Previously used conn.protocol_version, now uses the default
   • pubsub_to_client_task.rs:95: Still correctly passes conn.protocol_version

   The fact that one site passes the version explicitly (pubsub_to_client) while the other doesn't (client_to_pubsub) suggests they may have different requirements. Please confirm this is intentional.

Testing Recommendations

• Test with clients using different protocol versions (v1 and v2)
• Verify tunnel messages are correctly serialized/deserialized in both directions
• Test error paths for malformed messages
• Confirm the epoxy purger workflow registration doesn't introduce issues

Code Quality

• Style: Follows Rust conventions ✓
• Error Handling: Improved with better context ✓
• Safety: No unsafe code introduced ✓

Verdict

The core changes look good, but please verify:

1. The purger module import/visibility
2. The intentional protocol version handling difference between the two task files

Otherwise, this is a clean refactoring that improves code quality. Nice work!

[claude]

Code Review

This PR fixes the usage of vbare versioning functions for the runner tunnel by using the newer convenience methods. Overall, the changes look good and improve code clarity. Here are my findings:

✅ Strengths

1. Correct API Usage: The PR correctly migrates from the older verbose API pattern to the newer convenience methods:

   • Old: deserialize_version(&data, version).and_then(|x| x.unwrap_latest())
   • New: deserialize(&data, version)
   • This is cleaner and less error-prone.

2. Consistent Changes: All locations using the old pattern have been updated:

   • client_to_pubsub_task.rs:36 - ToServer deserialization
   • pubsub_to_client_task.rs:95 - ToClient serialization
   • http_client.rs:138-140 - Request serialization

3. Error Handling Improvement: In http_client.rs, the serialization error is now handled earlier with better context:

   let request = versioned::Request::wrap_latest(request)
       .serialize()
       .context("failed to serialize epoxy request")?;

4. Registry Update: Added the purger::Workflow to the epoxy registry at lib.rs:22, which appears to be a necessary addition for the epoxy package.

🔍 Observations

1. API Consistency: The changes align with the vbare trait implementation seen in runner-protocol/src/versioned.rs, where types like ToClient and ToServer implement OwnedVersionedData with convenience methods like serialize(version) and deserialize(buf, version).

2. Protocol Handling: The changes correctly maintain protocol version handling, passing conn.protocol_version to ensure compatibility across v1 and v2 protocol versions.

⚠️ Minor Concerns

1. Testing: There's no explicit test coverage visible in this PR. Consider:

   • Testing serialization/deserialization with both v1 and v2 protocol versions
   • Testing error paths (malformed messages, version mismatches)

2. Purger Workflow: The addition of purger::Workflow to the registry seems unrelated to the "fix vbare functions" commit message. This might be:

   • A necessary dependency for the tunnel fixes (should be mentioned in PR description)
   • An unrelated change that should be in a separate commit

📝 Recommendations

1. PR Description: Add context about why the purger workflow registration was needed. If it's unrelated, consider splitting it into a separate PR.

2. Testing: Consider adding integration tests that verify tunnel messages are correctly serialized/deserialized across protocol versions.

3. Commit Message: The commit message "fix: use correct vbare functions for runner tunnel" is clear, but could mention what was wrong with the old approach (e.g., "fix: use vbare convenience methods instead of manual unwrap_latest calls").

✅ Verdict

The changes are technically sound and improve code quality. The migration to convenience methods is correct and makes the code more maintainable. The only concern is the unexplained purger workflow addition, which should be documented or separated.

Recommendation: Approve with minor comment about the purger workflow addition.

[claude]

Code Review

Summary

This PR updates the pegboard-runner and epoxy packages to use the correct vbare API functions. The changes replace deprecated version-specific methods with the newer simplified API.

Positive Observations

Correct API Migration
The changes properly migrate from the old vbare API to the new one:

• deserialize_version().unwrap_latest() to deserialize() - This is the correct pattern
• serialize_version() to serialize() - Properly updated
• The new API handles version conversion internally, making the code cleaner

Consistent Changes
All four files have been updated consistently:

• client_to_pubsub_task.rs: Line 36 - Correctly uses deserialize()
• pubsub_to_client_task.rs: Line 95 - Correctly uses serialize()
• http_client.rs: Lines 138-140 - Properly chains .serialize() with error context
• lib.rs: Registers purger workflow (appears related to cleanup functionality)

Error Handling Improvement
In http_client.rs:138-140, the code now properly handles serialization errors before passing to the HTTP client. This is better than the previous inline error handling in the .body() call.

Code Quality

Good:

• Changes align with the vbare OwnedVersionedData trait implementation
• Proper error propagation using ? operator
• Maintains existing structured logging patterns
• Follows the codebase convention of using anyhow::Context for error messages

Note:
The epoxy/src/lib.rs change adding registry.register_workflow for purger::Workflow appears unrelated to the tunnel fix mentioned in the PR title. This might be an incidental but necessary change - consider mentioning it in the PR description if it is required for the fix to work properly.

Testing Considerations

Since this changes protocol serialization/deserialization:

• Ensure tests cover both v1 and v2 protocol versions
• Verify backward compatibility with existing runners
• Test the tunnel message flow end-to-end

Consider running integration tests that exercise:

1. WebSocket message handling with different protocol versions
2. HTTP request/response serialization in epoxy
3. Tunnel message forwarding between client, server, and gateway

Security

No security concerns identified. The changes are purely refactoring existing serialization logic without altering the protocol semantics.

Performance

The new API may be slightly more efficient as it reduces method calls, though the impact would be negligible.

Minor Suggestions

1. PR Description: Consider adding a note about the purger::Workflow registration if it is related to the fix
2. Testing: Add a note in the PR about which tests were run to verify the changes work correctly

Recommendation

APPROVE - This is a clean refactoring that correctly updates to the newer vbare API. The changes are minimal, focused, and maintain proper error handling throughout.

[claude]

Code Review

Summary

This PR fixes incorrect usage of vbare API functions in the runner tunnel code and registers a missing workflow. The changes correctly update the serialization/deserialization methods to use the proper vbare API.

Code Quality ✅

Positive findings:

1. Correct API usage: The changes properly use the vbare library's simplified API:

   • Changed from deserialize_version(...).and_then(|x| x.unwrap_latest()) to deserialize(...)
   • Changed from serialize_version(...) to serialize(...)
   • These helper methods are defined in the protocol crates (e.g., engine/sdks/rust/runner-protocol/src/versioned.rs:36-46, engine/sdks/rust/epoxy-protocol/src/versioned.rs:41-44)

2. Consistent error handling: The error handling pattern is preserved and proper context is added in http_client.rs:139-140

3. Complete fix: All occurrences of the old API pattern were updated:

   • client_to_pubsub_task.rs:36 - deserialize
   • pubsub_to_client_task.rs:95 - serialize
   • http_client.rs:138-140 - serialize with error context

4. Workflow registration: Added missing purger::Workflow registration in epoxy/src/lib.rs:22, which is required for the token revoking functionality

Potential Issues 🔍

Minor concerns:

1. No tests included: This PR doesn't include tests. While the changes are straightforward, it would be beneficial to verify:

   • Tunnel messages serialize/deserialize correctly with the new API
   • Protocol version negotiation still works correctly
   • Consider adding integration tests for the tunnel message flow

2. Inconsistent style in http_client.rs: The refactor splits the serialize call across multiple lines (lines 138-140), which differs from the previous one-liner style. While this improves error handling (by adding context), you could consider using .context() inline for consistency:

   let request = versioned::Request::wrap_latest(request)
       .serialize()
       .context("failed to serialize epoxy request")?;

Performance Considerations ✅

No performance concerns. The new API is actually more efficient as it:

• Removes the extra .and_then() call for deserialization
• Directly returns the unwrapped latest version instead of the enum wrapper

Security Considerations ✅

No security concerns identified. The changes:

• Maintain the same protocol version checking
• Don't alter the underlying serialization format
• Don't change authentication or authorization logic

Best Practices ✅

The code follows the repository's conventions:

• Uses anyhow::Result for error handling per CLAUDE.md
• Maintains structured logging with tracing
• Error messages are lowercase per logging patterns
• Imports are at the top of files

Recommendations

1. Add integration tests (optional but recommended):

   • Test tunnel message round-trip serialization
   • Verify protocol version compatibility

2. Consider the inline .context() style mentioned above for consistency

Conclusion

This is a solid bug fix that correctly updates the vbare API usage. The changes are minimal, focused, and maintain backward compatibility. The code quality is good and follows repository conventions.

Approval recommendation: ✅ Approve with minor suggestions for improvement

[graphite-app]

Merge activity

• Nov 4, 8:09 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Nov 4, 8:10 PM UTC: CI is running for this pull request on a draft pull request (#3349) due to your merge queue CI optimization settings.
• Nov 4, 8:41 PM UTC: The Graphite merge queue removed this pull request due to removal of a downstack PR #3342.
• Nov 4, 8:49 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Nov 4, 8:50 PM UTC: CI is running for this pull request on a draft pull request (#3352) due to your merge queue CI optimization settings.
• Nov 4, 8:52 PM UTC: Merged by the Graphite merge queue via draft PR: #3352.

[claude]

Code Review - PR #3300: Fix vbare function usage for runner tunnel

Summary

This PR corrects the usage of vbare versioned data API functions across the runner tunnel communication layer. The changes align the code with the proper vbare API patterns used elsewhere in the codebase.

---

✅ Positive Findings

1. Correct API Usage
The changes fix inconsistent usage of the vbare versioned data API:

• ✅ epoxy/http_client.rs: Now properly uses .serialize() convenience method instead of chaining .serialize_version()
• ✅ pegboard-runner: Correctly uses .deserialize() and .serialize() convenience wrappers that internally handle protocol version
• ✅ Aligns with patterns used in pegboard-gateway and other components

2. Improved Error Handling
The serialization in http_client.rs now happens earlier with proper context:

let request = versioned::Request::wrap_latest(request)
    .serialize()
    .context("failed to serialize epoxy request")?;

This provides clearer error messages and fails fast before the HTTP request.

3. Code Simplification
The deserialization in client_to_pubsub_task.rs is now cleaner:

• Removed unnecessary .and_then(|x| x.unwrap_latest()) chain
• The .deserialize() convenience method already returns the unwrapped latest version

4. Registry Fix
Added missing purger::Workflow registration in epoxy registry - important for workflow system completeness.

---

🔍 Code Quality Observations

API Consistency
The PR correctly identifies that epoxy-protocol and runner-protocol define convenience methods that wrap the lower-level vbare trait methods:

// epoxy-protocol/versioned.rs
impl Request {
    pub fn serialize(self) -> Result<Vec<u8>> {
        <Self as OwnedVersionedData>::serialize(self, PROTOCOL_VERSION)
    }
}

impl Response {
    pub fn deserialize(buf: &[u8]) -> Result<v1::Response> {
        <Self as OwnedVersionedData>::deserialize(buf, PROTOCOL_VERSION)
    }
}

The changes properly use these convenience methods instead of the trait methods directly.

---

🎯 Technical Correctness

Before (Incorrect):

// In client_to_pubsub_task.rs - line 36
versioned::ToServer::deserialize_version(&data, conn.protocol_version)
    .and_then(|x| x.unwrap_latest())

After (Correct):

versioned::ToServer::deserialize(&data, conn.protocol_version)

The runner-protocol .deserialize() method internally calls deserialize_version() and unwrap_latest(), so the old code was redundant.

Similarly for serialization:

Before:

// In pubsub_to_client_task.rs - line 95
versioned::ToClient::wrap_latest(msg).serialize_version(conn.protocol_version)

After:

versioned::ToClient::wrap_latest(msg).serialize(conn.protocol_version)

The .serialize() convenience method is the proper API to use here.

---

⚠️ Minor Observations

1. Protocol Version Handling
The code correctly respects conn.protocol_version for backward compatibility with older runners. This is important since the runner protocol supports both V1 and V2.

2. Consistency with Rest of Codebase
After reviewing other uses of these APIs in:

• pegboard-gateway/src/server_tunnel.rs
• pegboard-gateway/src/actor_tunnel.rs
• universalpubsub crate

The changes in this PR now match the established patterns, confirming these are the correct functions to use.

---

📊 Impact Assessment

Files Changed: 4
Lines Changed: +17/-17 (net zero)
Risk Level: Low

• ✅ No logic changes, only API method corrections
• ✅ Error handling improved (earlier failure with better context)
• ✅ Maintains backward compatibility via conn.protocol_version
• ✅ Aligns with codebase conventions

---

🧪 Testing Recommendations

Since these changes affect the runner tunnel communication protocol:

1. Integration Testing: Verify runner-to-server communication works correctly
2. Version Compatibility: Test with both V1 and V2 protocol versions if supported
3. Error Scenarios: Verify deserialization failures are handled gracefully
4. Tunnel Operations: Test HTTP tunneling and WebSocket tunneling through runners

Consider running:

cargo test -p pegboard-runner
cargo test -p epoxy

And integration tests that exercise the tunnel functionality.

---

✅ Final Verdict

LGTM - Approved

This is a clean refactoring that fixes incorrect API usage. The changes are technically correct, improve error handling, and align with established patterns in the codebase. No security concerns, no performance issues, and maintains backward compatibility.

The PR title accurately describes the changes: "fix: use correct vbare functions for runner tunnel"

[claude]

PR Review: Fix vbare Function Usage for Runner Tunnel

Summary

This PR correctly fixes improper usage of the vbare versioned data API. The changes align with the library's intended usage patterns and fix potential bugs in the runner tunnel communication.

---

Code Quality & Best Practices ✅

Overall Assessment: EXCELLENT

The changes demonstrate a proper understanding of the vbare library API:

1. Correct API Usage - The PR changes from using trait methods directly (serialize_version(), deserialize_version()) to using the proper public API methods (.serialize(version), .deserialize(data, version))

2. Consistency - After this PR, the codebase uses versioned serialization/deserialization consistently across epoxy and pegboard-runner packages, matching the patterns already used in epoxy/http_routes.rs

3. Follows Conventions - The changes align with existing usage patterns found in engine/packages/epoxy/src/http_routes.rs:19,33

---

Detailed Changes Analysis

1. http_client.rs (epoxy) ✅

// Before:
let request = versioned::Request::wrap_latest(request);
client.post(url).body(request.serialize()?)

// After:
let request = versioned::Request::wrap_latest(request)
    .serialize()
    .context("failed to serialize epoxy request")?;
client.post(url).body(request)

Analysis:

• Moves serialization earlier with proper error context
• Improves error handling by adding context message
• Follows the wrap_latest() -> serialize(version) pattern correctly
• Better separation of concerns: serialize then pass to body

2. client_to_pubsub_task.rs (pegboard-runner) ✅

// Before:
let msg = versioned::ToServer::deserialize_version(&data, conn.protocol_version)
    .and_then(|x| x.unwrap_latest())?;

// After:
let msg = versioned::ToServer::deserialize(&data, conn.protocol_version)?;

Analysis:

• Critical Fix: The .deserialize() method automatically handles version conversion and unwrapping
• Removes unnecessary .and_then() chain that was manually unwrapping
• Cleaner, more idiomatic code
• Consistent with epoxy's usage pattern

3. pubsub_to_client_task.rs (pegboard-runner) ✅

// Before:
versioned::ToClient::wrap_latest(msg).serialize_version(conn.protocol_version)

// After:  
versioned::ToClient::wrap_latest(msg).serialize(conn.protocol_version)

Analysis:

• Changes from trait method serialize_version() to public API .serialize()
• Correct usage pattern: wrap latest message, then serialize with protocol version
• Matches the pattern used throughout the codebase

4. lib.rs (epoxy) ✅

// Added:
registry.register_workflow::<purger::Workflow>()?;

Analysis:

• Registers the existing purger::Workflow that was already defined in workflows/purger.rs
• This workflow handles KV cache purging for token revocation
• Good catch - The workflow exists but wasn't registered, which would prevent it from being discoverable by the workflow engine

---

Potential Bugs or Issues 🟢

No bugs identified. These changes actually fix existing bugs:

1. Bug Fixed: Incorrect deserialization in client_to_pubsub_task.rs that was using deserialize_version().and_then(unwrap_latest()) instead of the simpler .deserialize()
2. Bug Fixed: Incorrect serialization in pubsub_to_client_task.rs using serialize_version() directly instead of .serialize()
3. Bug Fixed: Missing workflow registration for the purger workflow

---

Performance Considerations 🟢

Positive impact:

1. Reduced allocations - The refactored http_client.rs code moves serialization outside the request builder, allowing for better compiler optimizations
2. Cleaner code path - Removing the .and_then(unwrap_latest()) chain simplifies the deserialization path
3. No performance regressions - All changes are API usage fixes with no algorithmic changes

---

Security Concerns 🟢

No security issues identified.

The changes are purely about correct API usage and don't introduce any security vulnerabilities. The vbare library handles version negotiation, which is security-relevant for protocol compatibility, and these changes ensure it's used correctly.

---

Test Coverage 📋

Recommendation: While the changes are straightforward API fixes, consider:

1. Integration tests - Verify the runner tunnel communication still works correctly with these changes
2. Protocol version tests - Ensure version negotiation works properly with the corrected serialization/deserialization
3. Purger workflow test - Now that it's registered, verify the purger workflow can be triggered

According to CLAUDE.md testing guidelines:

# Run tests for affected packages
cargo test -p epoxy
cargo test -p pegboard-runner

# Pipe to file and grep for relevant logs per guidelines
cargo test -p epoxy 2>&1 | tee /tmp/epoxy_tests.log
grep -i "tunnel|versioned|serialize" /tmp/epoxy_tests.log

---

Additional Notes

Follows CLAUDE.md conventions:

• ✅ Uses anyhow::* for error handling
• ✅ Proper error context added with .context()
• ✅ Structured logging preserved (tracing with debug fields)
• ✅ Workspace dependencies maintained

Code locations for reference:

• http_client.rs: engine/packages/epoxy/src/http_client.rs:138-140
• client_to_pubsub_task.rs: engine/packages/pegboard-runner/src/client_to_pubsub_task.rs:36
• pubsub_to_client_task.rs: engine/packages/pegboard-runner/src/pubsub_to_client_task.rs:95
• lib.rs: engine/packages/epoxy/src/lib.rs:22

---

Final Verdict: ✅ APPROVE

This is a clean, well-focused PR that fixes incorrect API usage. The changes improve code quality, fix latent bugs, and ensure proper protocol versioning in the runner tunnel communication system. The addition of the purger workflow registration is a good catch.

Recommended next steps:

1. Run integration tests to verify runner tunnel functionality
2. Consider adding test coverage for the purger workflow now that it's registered
3. Merge when tests pass