ci: fix release scripts for engine #3258

NathanFlurry · 2025-10-24T20:41:17Z

No description provided.

vercel · 2025-10-24T20:41:22Z

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

Project	Deployment	Preview	Comments	Updated (UTC)
rivet-inspector	Ready	Preview	Comment	Oct 25, 2025 9:23am

2 Skipped Deployments

Project	Deployment	Preview	Comments	Updated (UTC)
rivet-cloud	Ignored			Oct 25, 2025 9:23am
rivet-site	Ignored	Preview		Oct 25, 2025 9:23am

NathanFlurry · 2025-10-24T20:41:32Z

fix(fe): invalid types #3260
fix: make inspector working again #3229
fix(fe): bring back ts type check #3259
ci: fix release scripts for engine #3258 👈 (View in Graphite)
chore(scripts): add pg backup & restore scripts for dev #3251
feat(examples): cursors with raw websockets example #3245
feat(examples-cursors): cursors example #3244
feat(core): allow routing traffic to actors via path #3250
fix: fix biome ignore path #3249
chore: fix rivetkit release script #3242 : 1 other dependent PR (#3243 )
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 · 2025-10-24T20:43:16Z

Code Review Summary

This PR updates the release and CI scripts to fix file paths after the engine/ directory restructuring. The changes are primarily path corrections, with a small OpenAPI spec regeneration and minor code improvements in the TypeScript runner SDK.

✅ Positive Aspects

Correct Path Updates: All file path updates properly reflect the new engine/ directory structure
Comprehensive Coverage: Both release and CI workflows are updated consistently
Minor Code Improvements: The TypeScript SDK changes include small enhancements to code clarity

📝 Code Quality Observations

Path Updates (.github/workflows/)

All Dockerfile and build script path updates are correct: docker/ → engine/docker/
Whitespace cleanup (trailing spaces removed) improves consistency

TypeScript Runner SDK (engine/sdks/typescript/runner/)

The changes in src/mod.ts and src/tunnel.ts improve code organization:

engine/sdks/typescript/runner/src/mod.ts:170

Duplicate call to onActorStop on lines 163 and 170-177 - the second call (170-177) appears to be dead code since the actor was already stopped and callback was already invoked on line 163
Consider removing the duplicate call or clarifying if both are intentional

engine/sdks/typescript/test-runner/src/index.ts:82

Good: Using array destructuring for multiple return values improves readability
The SSE stream implementation looks correct

engine/sdks/typescript/runner/src/tunnel.ts:

Line 469: Redundant condition check - actor is already validated on line 434-449, so the check on line 469 will always be true
Consider removing the redundant check or refactoring for clarity

🔍 Potential Issues

Dead Code in forceStopActor (engine/sdks/typescript/runner/src/mod.ts:170-177)

// This appears after the actor is already stopped
this.#config.onActorStop(actorId, actor.generation).catch((err) => {
    logger()?.error({
        msg: "error in onactorstop for actor",
        runnerId: this.runnerId,
        actorId,
        err,
    });
});

This seems like leftover code from a refactor. The callback was already invoked with await on line 163, so this second call will execute after the actor is already stopped.

Redundant Actor Validation (engine/sdks/typescript/runner/src/tunnel.ts:469)
```
// Track this WebSocket for the actor
if (actor) {
    actor.webSockets.add(webSocketId);
}
```
The actor variable is guaranteed to be non-null at this point due to the early return on lines 434-449.

🎯 Recommendations

Remove Duplicate onActorStop Call: Clean up the duplicate callback invocation in forceStopActor at line 170-177
Remove Redundant Check: Remove the if (actor) guard at line 469 in tunnel.ts since it's always true
Consider Comments: If the duplicate calls are intentional (e.g., fire-and-forget vs awaited), add comments explaining the rationale

✅ Security & Performance

No security concerns identified
No performance regressions expected
Path updates don't introduce any runtime changes

📊 Test Coverage

The changes to CI workflows don't affect test execution paths. The TypeScript SDK changes are minor refactors that maintain existing functionality.

Final Verdict

Status: ✅ Approved with minor suggestions

The core purpose of this PR (fixing release scripts) is achieved successfully. The minor code quality issues identified are non-blocking and can be addressed in a follow-up PR if desired.

🤖 Generated with Claude Code

claude · 2025-10-24T21:12:24Z

PR Review: CI Fix for Engine Release Scripts

Overview

This PR updates file paths in CI workflows and scripts to reflect the reorganization of the docker/ directory to engine/docker/. The changes are primarily path corrections with some minor improvements.

Code Quality & Best Practices ✅

Strengths:

Consistent path updates across all affected files
Follows the repository's existing patterns and conventions
Simple, focused change with clear intent
No complex logic changes that could introduce bugs

Minor Issues:

Whitespace-only changes (.github/workflows/rust.yml): Lines 35, 39, 65, 68, 83, 86 contain trailing whitespace removal. While this is good hygiene, it's unrelated to the PR's stated purpose.
Large OpenAPI diff: The engine/artifacts/openapi.json file shows a complete regeneration with formatting changes (1759 additions, 1563 deletions). This suggests the file was regenerated but it's unclear if this is intentional or a side effect.

File-by-File Analysis

GitHub Actions Workflows

.github/workflows/release.yaml:

✅ Correctly updates Docker build script path: docker/engine/build.sh → engine/docker/engine/build.sh
✅ Properly updates Dockerfile paths for universal builds
⚠️ All three path updates are consistent with the new structure

.github/workflows/rust.yml:

✅ Whitespace cleanup is good, but consider whether it should be in a separate commit

TypeScript SDK Changes

engine/sdks/typescript/runner/src/mod.ts (+68/-18):

✅ Good improvements to error handling and logging
✅ Better structured logging using tracing patterns (e.g., line 172: logger()?.error({ msg, runnerId, actorId, err }))
✅ Improved cleanup logic in forceStopActor method

🔍 Potential Issue (Line 170): onActorStop is called twice - once with try/catch at line 163, then again at line 170 with just error logging. This looks like a bug:

// Line 163-166
try {
    await this.#config.onActorStop(actorId, actor.generation);
} catch (err) {
    console.error(`Error in onActorStop for actor ${actorId}:`, err);
}

// Line 170-177 - This will execute regardless
this.#config.onActorStop(actorId, actor.generation).catch((err) => {
    logger()?.error({ ... });
});

Recommendation: Remove one of these calls. The second one (170-177) appears to be the intended version with proper structured logging.

engine/sdks/typescript/runner/src/tunnel.ts (+14/-5):

✅ Improved error messages with more context
✅ Better structured logging throughout (lines 170-172, 341)
✅ Good defensive programming with null checks

engine/sdks/typescript/test-runner/src/index.ts (+46/-29):

✅ Better separation of concerns with startRunner function
✅ Improved serverless configuration logic
✅ More flexible WebSocket handling
⚠️ Line 28: Uses Promise.withResolvers() which requires Node.js 22+. Ensure this is documented in requirements.

Test Deletions

examples/cursors/tests/cursors.test.ts (deleted):

❓ Question: Why was this test file completely removed (132 lines)? The PR description doesn't mention test removals. If this test is obsolete or broken, it should be mentioned in the PR description.

Infrastructure Scripts

scripts/docker/build-push.sh:

✅ Correctly updates default Dockerfile path to engine/docker/universal/Dockerfile

justfile:

✅ All Docker build commands properly updated to use new paths

Potential Bugs 🐛

Critical: Duplicate onActorStop call in mod.ts:170 (see analysis above)
Minor: Large OpenAPI regeneration may indicate unintended schema changes

Performance Considerations ⚡

No performance impact expected. The changes are primarily path corrections and logging improvements.

Security Concerns 🔒

No security issues identified. All changes are infrastructure and path-related.

Test Coverage 📊

Concerns:

The deletion of examples/cursors/tests/cursors.test.ts reduces test coverage without explanation
No new tests added to verify the CI path changes work correctly
Recommendation: Consider adding a simple CI smoke test that verifies the Docker build paths are correct

Recommendations

High Priority:

✅ Fix the duplicate onActorStop call in engine/sdks/typescript/runner/src/mod.ts:170
✅ Add explanation in PR description for why cursors.test.ts was deleted
✅ Verify the OpenAPI regeneration was intentional and document any API changes

Medium Priority:
4. Consider separating whitespace cleanup into a separate commit for cleaner git history
5. Document the Node.js 22+ requirement if Promise.withResolvers() is being used

Low Priority:
6. Add CI smoke tests to prevent future path breakage

Summary

This is a necessary cleanup PR to fix release script paths after a directory reorganization. The changes are mostly correct, but there's a potential bug with duplicate function calls that should be addressed before merging. The test file deletion should also be explained.

Recommendation: Request Changes - Fix the onActorStop duplicate call and clarify the test deletion.

pkg-pr-new · 2025-10-24T21:13:23Z

More templates

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: a707646

claude · 2025-10-25T00:52:27Z

PR Review - CI: Fix Release Scripts for Engine

This PR primarily addresses path corrections in CI/CD workflows following a repository restructuring. Overall, the changes are straightforward and necessary. Here's my detailed feedback:

✅ Positive Aspects

Consistent Path Updates: All docker-related paths have been correctly updated from docker/ to engine/docker/ across CI workflows, justfile, and scripts
Artifact Regeneration: OpenAPI and error artifacts appear to have been properly regenerated
Test Cleanup: Removed unused test file (examples/cursors/tests/cursors.test.ts)

📋 Code Quality & Best Practices

GitHub Workflows (.github/workflows/)

✅ Path corrections are consistent across both release.yaml and rust.yml
⚠️ Minor Issue: Whitespace-only changes (trailing spaces removed on lines 114, 35, 37, etc.) - while good, these should ideally be in a separate formatting commit

TypeScript SDK Changes

engine/sdks/typescript/runner/src/mod.ts (lines 154-178):

⚠️ Code Duplication Bug: The forceStopActor method calls onActorStop twice:
- Line 163: First call with try-catch
- Line 170-177: Duplicate call with error logging
This appears to be a copy-paste error. Recommend removing the duplicate call at lines 170-177.

engine/sdks/typescript/runner/src/tunnel.ts:

✅ Good error handling and cleanup logic
✅ Proper resource management with garbage collection
✅ Well-structured WebSocket and HTTP tunneling

engine/sdks/typescript/test-runner/src/index.ts:

✅ Good refactoring with better promise handling
✅ Proper WebSocket lifecycle management
⚠️ Lines 168-241: The fetch handler mixes concerns (ping, sleep endpoints). Consider extracting to separate handler functions for better testability

RivetKit Changes

rivetkit-typescript/packages/rivetkit/src/drivers/engine/actor-driver.ts:

✅ Properly adds runner: Runner parameter to #runnerFetch and #runnerWebSocket methods
✅ Matches the updated RunnerConfig interface signature

🔒 Security Considerations

✅ No apparent security issues introduced
✅ Authorization headers properly handled in test runner
✅ WebSocket connections have proper close handlers with timeouts

⚡ Performance Considerations

Tunnel Garbage Collection (tunnel.ts:119-122):
- ✅ 60-second interval is reasonable
- ✅ 5-second message ack timeout is appropriate for local/regional communication
KV Request Cleanup (mod.ts:1462-1484):
- ✅ 30-second timeout for KV requests is sensible
- ✅ 15-second cleanup interval prevents memory leaks

🧪 Test Coverage

⚠️ Missing Tests: The deletion of examples/cursors/tests/cursors.test.ts (132 lines) without replacement is concerning. Was this test:
- Obsolete?
- Moved elsewhere?
- Planned to be rewritten?
Please clarify the reason for removal.

🐛 Potential Bugs

Critical: Duplicate onActorStop call in mod.ts:170-177 (mentioned above)
Minor: In test-runner/src/index.ts:217, WebSocket is stored but never cleaned up if the actor stops before WebSocket closes

📦 Artifact Changes

The OpenAPI spec and error JSON files show formatting changes (tabs → spaces, removed trailing newlines). These appear to be auto-generated artifacts and are fine.

✨ Recommendations

Fix the duplicate onActorStop call in engine/sdks/typescript/runner/src/mod.ts:170-177
Consider splitting formatting changes (whitespace-only) from functional changes in future PRs
Add a comment explaining why examples/cursors/tests/cursors.test.ts was removed
Consider adding integration tests for the updated runner tunnel functionality

📊 Overall Assessment

Status: ✅ Approve with minor fixes recommended

The PR successfully addresses the CI path issues. The main concern is the duplicate onActorStop call which should be fixed before merge. Otherwise, the changes are well-structured and maintain good code quality.

Risk Level: Low (primarily path corrections and minor refactoring)

Generated with Claude Code 🤖

claude · 2025-10-25T00:58:29Z

PR Review: Fix Release Scripts for Engine

Overview

This PR updates release scripts and build paths to reflect the new engine directory structure. The changes are primarily infrastructure-related with some substantive TypeScript SDK improvements.

✅ Positive Aspects

Consistent Path Updates: All Docker and build script paths have been systematically updated from docker/engine/ to engine/docker/engine/, which aligns with the new monorepo structure.
Improved Error Handling: The tunnel implementation now has better error handling with proper cleanup of pending requests and WebSockets during shutdown.
Enhanced WebSocket Tracking: Added proper tracking of WebSocket connections per actor with cleanup when actors stop (tunnel.ts:179-201).
Better Logging: The test-runner now includes structured logging for HTTP requests, which is helpful for debugging.

🐛 Potential Issues

1. Critical: Duplicate `onActorStop` Call (mod.ts:163-177)

async forceStopActor(actorId: string, generation?: number) {
    const actor = this.#removeActor(actorId, generation);
    if (!actor) return;

    this.#tunnel?.unregisterActor(actor);

    // If onActorStop times out, Pegboard will handle this timeout...
    try {
        await this.#config.onActorStop(actorId, actor.generation);  // First call
    } catch (err) {
        console.error(`Error in onActorStop for actor ${actorId}:`, err);
    }

    this.#sendActorStateUpdate(actorId, actor.generation, "stopped");

    this.#config.onActorStop(actorId, actor.generation).catch((err) => {  // Second call - DUPLICATE!
        logger()?.error({...});
    });
}

Issue: onActorStop is called twice - once with await and try-catch, then again without await. This could cause issues if the callback has side effects or is not idempotent.

Recommendation: Remove the duplicate call. Keep only one:

try {
    await this.#config.onActorStop(actorId, actor.generation);
} catch (err) {
    logger()?.error({
        msg: "error in onactorstop for actor",
        runnerId: this.runnerId,
        actorId,
        err,
    });
}

2. Logging Inconsistency (mod.ts:164)

console.error(`Error in onActorStop for actor ${actorId}:`, err);

Issue: Uses console.error instead of the structured logger used elsewhere. This breaks logging consistency and structured logging patterns mentioned in CLAUDE.md.

Recommendation:

logger()?.error({
    msg: "error in onactorstop for actor",
    runnerId: this.runnerId,
    actorId,
    err,
});

3. Potential Memory Leak (tunnel.ts:52-53)

for (const [_, request] of this.#actorPendingRequests) {
    request.reject(new Error("Tunnel shutting down"));
}

Issue: Iterates and rejects but doesn't delete. The clear() on line 55 handles it, but the stream controllers should be explicitly closed before rejection.

Recommendation:

for (const [_, request] of this.#actorPendingRequests) {
    if (request.streamController) {
        try {
            request.streamController.close();
        } catch (e) {
            // Already closed
        }
    }
    request.reject(new Error("Tunnel shutting down"));
}

4. Edge Case: Concurrent Actor Stop (mod.ts:192-213)

The getActor method logs errors when an actor isn't found, but in distributed systems with concurrent operations, this might create unnecessary error noise.

Recommendation: Consider using debug level for "actor not found" logs in getActor, or distinguish between expected vs unexpected missing actors.

📊 Code Quality Observations

Good: Proper use of private fields with # prefix
Good: Comprehensive error handling in tunnel message processing
Good: Actor tracking with Sets for requests and WebSockets
Inconsistent: Mix of console.error and structured logging

🔒 Security Considerations

Positive: Proper WebSocket cleanup prevents resource exhaustion
Positive: Timeout mechanisms prevent infinite waits (MESSAGE_ACK_TIMEOUT)
Note: The test-runner auto-configures serverless endpoints on startup (index.ts:112-141), which is fine for development but ensure this is documented

🧪 Test Coverage

The PR removes examples/cursors/tests/cursors.test.ts (132 lines deleted) but doesn't add replacement tests. Consider whether test coverage is maintained elsewhere.

📝 Documentation

The JSON formatting changes in error artifacts are cosmetic (tabs → spaces) and improve consistency
OpenAPI changes are generated artifacts (196 additions, large diff) - ensure these were regenerated correctly

🎯 Summary

Severity Levels:

🔴 High: Duplicate onActorStop call could cause unexpected behavior
🟡 Medium: Logging inconsistencies, potential memory leak in edge cases
🟢 Low: Minor improvements for error log levels

Recommendation: Request changes to fix the duplicate onActorStop call before merging. The other issues are lower priority but should be addressed for code quality.

Overall Assessment: The path updates are solid and necessary. The TypeScript changes improve functionality but have a critical bug that needs fixing. With the duplicate call issue resolved, this PR is good to merge.

Generated with Claude Code

claude · 2025-10-25T01:02:56Z

PR Review: CI Fix for Engine Release Scripts

Summary

This PR fixes the release scripts for the engine by updating file paths from docker/ to engine/docker/, along with some formatting improvements and test file removal.

Overall Assessment

✅ APPROVED - This is a clean infrastructure fix with no functional code changes. The PR correctly updates paths to match the current repository structure.

Code Quality

Strengths:

Single focused commit with clear purpose
Consistent path updates across all affected files
No breaking changes to functionality

Path Updates:

.github/workflows/release.yaml - Updated Docker build paths
.github/workflows/rust.yml - Formatting cleanup
justfile, scripts/, and engine/docker/ - Updated script paths

Key Changes

1. CI/CD Workflow Updates ✅
The workflow changes correctly update paths from docker/ to engine/docker/. This suggests a recent repo restructuring.

2. TypeScript SDK Improvements ✅

engine/sdks/typescript/runner/src/mod.ts (+68, -18): Added actorWebSockets map for tracking WebSocket connections per actor, enhanced cleanup logic in removeActor and forceStopActor
engine/sdks/typescript/runner/src/tunnel.ts (+14, -5): Added actorId field to PendingRequest, improved cleanup in error paths
Proper resource cleanup prevents WebSocket leaks with good error handling

3. Test Runner Updates ✅

engine/sdks/typescript/test-runner/src/index.ts (+46, -29): Implements actorWebSockets map with proper event handlers

4. JSON/OpenAPI Formatting 📋

engine/artifacts/errors/*.json (8 files): Changed indentation from tabs to 2 spaces
engine/artifacts/openapi.json (+1759, -1563): Large diff due to re-formatting
Recommendation: Verify the OpenAPI schema was regenerated from source

5. Test Deletion ⚠️

examples/cursors/tests/cursors.test.ts (-132 lines): Entire test file removed
Recommendation: Add a note explaining why this test was removed

Security & Performance

✅ No security concerns identified
✅ Positive performance impact: WebSocket connection tracking prevents memory leaks
✅ Resource cleanup improvements enhance security

Recommendations

Minor Improvements:

Add PR description context explaining the repo restructuring
Document why cursors.test.ts was removed
Verify OpenAPI schema was regenerated from source
Consider adding tests for the new WebSocket cleanup behavior

Final Verdict

Status: ✅ APPROVED - Ready to Merge

This is a solid infrastructure fix that correctly updates paths after a repository restructuring. The TypeScript SDK improvements add proper resource cleanup, which is a net positive. The only minor concern is the undocumented test file removal.

Confidence Level: High - The changes are straightforward, well-scoped, and follow best practices.

Review generated by Claude Code

claude · 2025-10-25T01:30:45Z

Pull Request Review: CI Fix for Release Scripts

Summary

This PR updates file paths in CI workflows and build scripts to reflect the new engine/ directory structure. The changes are primarily path corrections and formatting improvements to generated artifacts.

Code Quality & Best Practices ✅

Positive Aspects:

Consistent Path Updates: All path changes consistently add the engine/ prefix where needed
No Logic Changes: The PR correctly focuses on path fixes without introducing behavioral changes
Comprehensive Coverage: Updates span CI workflows, build scripts, and Docker configurations

Changes Breakdown:

CI/CD Workflows (.github/workflows/)

✅ Updated Docker build paths: docker/engine/build.sh → engine/docker/engine/build.sh
✅ Updated Dockerfile paths: docker/universal/Dockerfile → engine/docker/universal/Dockerfile
✅ Whitespace cleanup (trailing spaces removed)

Build Scripts

✅ engine/docker/universal/Dockerfile: CMD path correction
✅ scripts/docker/build-push.sh: Path fix for engine Dockerfile
✅ scripts/release/sdk.ts: Path corrections for SDK directories

Generated Artifacts

✅ JSON formatting standardization (tabs → 2 spaces, trailing newlines removed)
✅ OpenAPI spec regeneration (1563 → 1759 lines)

Potential Issues & Concerns

1. Test Coverage ⚠️

The test file deletion is concerning:

-examples/cursors/tests/cursors.test.ts | 132 deletions

Question: Was this test file intentionally removed? If so, what replaces this test coverage?

2. TypeScript SDK Changes 🔍

engine/sdks/typescript/runner/src/mod.ts (68 additions, 18 deletions)

Significant logic changes in the runner SDK
Multiple improvements to error handling and WebSocket management
These changes appear unrelated to "CI fix" and should potentially be in a separate PR

Notable changes include:

Duplicate onActorStop call at lines 163-177 (appears to be called twice in forceStopActor)
Enhanced tunnel support and message handling
Improved KV request cleanup

Recommendation: Consider splitting SDK improvements into a separate PR for better traceability.

3. Type Safety Improvement Opportunity 💡

In engine/sdks/typescript/runner/src/tunnel.ts:

async #runnerWebSocket(
    runner: Runner,
    actorId: string,
    websocketRaw: any,  // ⚠️ Using 'any' type
    request: Request,
): Promise<void> {
    const websocket = websocketRaw as UniversalWebSocket;

Suggestion: Replace any with proper typing:

websocketRaw: UniversalWebSocket | WebSocket

4. Duplicate Code in `mod.ts` 🐛

Lines 163-177 in engine/sdks/typescript/runner/src/mod.ts:

async forceStopActor(actorId: string, generation?: number) {
    // ... unregister actor, call onActorStop ...
    
    // ⚠️ onActorStop is called again here!
    this.#config.onActorStop(actorId, actor.generation).catch((err) => {
        logger()?.error({
            msg: "error in onactorstop for actor",
            // ...
        });
    });
}

The onActorStop callback appears to be invoked twice (lines 163 with await, and again at line 170 without await). This seems unintentional.

Fix: Remove the duplicate call or clarify the intent.

Performance Considerations ✅

No performance regressions identified:

Path changes are compile-time only
SDK improvements appear to enhance performance (better cleanup, message batching)

Security Concerns ✅

No security issues detected:

No credential exposure
No insecure patterns introduced
Token handling remains consistent

Documentation 📝

Missing: No updates to documentation reflecting the path changes. Consider updating:

engine/contrib-docs/DOCKER.md (already updated ✅)
Any developer setup guides that reference these paths
CI/CD documentation

Recommendations

Split PR: Consider separating SDK improvements from CI path fixes
Fix Duplicate Call: Remove duplicate onActorStop invocation in forceStopActor
Restore or Document: Clarify why examples/cursors/tests/cursors.test.ts was removed
Type Safety: Replace any types with proper interfaces where possible
Add Tests: Ensure CI changes are covered by integration tests

Conclusion

The core objective (fixing release script paths) is achieved correctly. However, the PR contains additional changes that would benefit from separation and review. The functional changes to the TypeScript SDK are improvements but make the PR scope broader than the title suggests.

Recommendation: ✅ Approve with requested changes

Address the duplicate onActorStop call
Clarify the test file deletion
Consider splitting SDK improvements into a follow-up PR

Review generated via automated analysis

claude · 2025-10-25T01:34:18Z

Pull Request Review: ci: fix release scripts for engine

Overview

This PR updates file paths in CI workflows and build scripts to reflect the new engine/ directory structure. The changes are primarily path corrections with some formatting updates to JSON/YAML files.

✅ Positive Aspects

1. Correct Path Updates

All workflow and script paths have been correctly updated from docker/ to engine/docker/
Justfile paths updated appropriately
Release script paths fixed

2. Consistent Formatting

Error JSON files reformatted from tabs to 2-space indentation (consistent with JSON standards)
Trailing newlines removed from JSON files
Whitespace normalized in YAML files

📝 Observations & Minor Issues

1. Large OpenAPI Schema Changes (engine/artifacts/openapi.json)

The 200-line diff in the OpenAPI schema is mostly formatting (tabs → spaces), but it's difficult to verify if there are any substantive changes mixed in. Consider:

Using a JSON diff tool to verify only formatting changed
Running jq to validate the JSON structure hasn't been altered inadvertently

2. Test File Deletion (examples/cursors/tests/cursors.test.ts)

132 lines deleted from examples/cursors/tests/cursors.test.ts
Question: Is this intentional or should these tests be preserved/relocated?
This could be reducing test coverage unintentionally

3. TypeScript SDK Changes

The TypeScript runner SDK changes show functional improvements:

engine/sdks/typescript/runner/src/mod.ts (+68/-18):

Better separation of concerns with tunnel registration
Improved actor lifecycle management
Better cleanup in forceStopActor

engine/sdks/typescript/runner/src/tunnel.ts (+14/-5):

Enhanced error handling
Better logging for unacked tunnel messages
Clearer warning messages about Engine disconnection

engine/sdks/typescript/test-runner/src/index.ts (+46/-29):

Refactored startRunner to return resolvers properly
Better separation of autostart logic
Improved configuration handling

4. Actor Driver Updates (rivetkit-typescript/packages/rivetkit/src/drivers/engine/actor-driver.ts)

Added inspector.token override logic (lines 81-85)
Appears to be a configuration improvement
Minor concern: The comment says "likely to be removed later" - consider if this is the right approach for a release

🔍 Code Quality Assessment

Strengths:

No breaking API changes - purely internal path updates
Maintains backward compatibility - no changes to public interfaces
Consistent code style - formatting improvements align with standards
Proper error handling - SDK changes improve error handling

Areas for Attention:

Missing Test Verification

# Recommend running to verify tests still pass:
cargo test
cd engine && npm test

Docker Build Verification
The Dockerfile path change should be tested:

# Verify Docker builds still work:
engine/docker/engine/build.sh <target>
docker build -f engine/docker/universal/Dockerfile .

OpenAPI Schema Validation

# Verify the OpenAPI schema is still valid:
cd engine && jq empty artifacts/openapi.json

🛡️ Security Considerations

No security concerns identified
Path changes are safe
No credential exposure
Token handling changes appear appropriate

🎯 Recommendations

Required Before Merge:

✅ Verify CI workflows pass with new paths
✅ Confirm Docker builds succeed
❓ Clarify intention of deleted test file (examples/cursors/tests/cursors.test.ts)
✅ Validate OpenAPI schema formatting hasn't introduced errors

Nice to Have:

Consider adding a migration note in CHANGELOG if this affects external users
Update any documentation that references the old docker/ paths
Consider splitting formatting changes from functional changes in future PRs for easier review

Overall Assessment

Status: ✅ Approve with minor clarifications

The PR correctly updates paths for the engine directory restructure. The code quality is good, and the TypeScript SDK improvements are beneficial. The main concern is the deleted test file which should be clarified before merge.

Risk Level: Low

Changes are primarily path updates
No breaking API changes
Formatting changes are mechanical

Review generated with attention to the CLAUDE.md conventions for this repository.

claude · 2025-10-25T01:44:47Z

Pull Request Review

This PR fixes release script paths for the engine. Overall, the changes look good and are primarily focused on path corrections and formatting cleanup. Here's my detailed review:

✅ Positive Observations

Path Corrections: The main purpose of this PR is successfully achieved - fixing paths from docker/ to engine/docker/ in release workflows and scripts.
Consistent Changes: The path updates are applied consistently across:
- GitHub Actions workflows (.github/workflows/release.yaml)
- Build scripts (engine/docker/engine/build.sh)
- Dockerfiles
ARM64 Support: Added Linux ARM64 build target in the release workflow (lines 86-91 in release.yaml), which is a nice addition for broader platform support.

🔍 Code Quality Observations

TypeScript SDK Changes

engine/sdks/typescript/runner/src/mod.ts (+68/-18 lines):

Changes appear to improve WebSocket handling and actor lifecycle management
Good error handling patterns with try-catch blocks
Proper cleanup in forceStopActor method

engine/sdks/typescript/tunnel.ts (+14/-5 lines):

Enhanced tunnel message handling
Better error propagation

engine/sdks/typescript/test-runner/src/index.ts (+46/-29 lines):

Improved serverless configuration
Better structured actor lifecycle callbacks

Minor Issues

JSON Formatting: The error artifact JSON files changed from tabs to spaces, and removed trailing newlines. While this is just formatting, it's good to ensure your team's formatting standards are consistent.
Whitespace Changes: .github/workflows/rust.yml has some trailing whitespace cleanup (lines with just spacing changes). Consider adding a pre-commit hook to prevent these in the future.

🐛 Potential Issues

None identified - The changes are straightforward path corrections.

🔒 Security Considerations

No security concerns - The changes don't introduce new attack vectors or expose sensitive data.

⚡ Performance Considerations

No performance impact - These are primarily path and configuration changes that don't affect runtime performance.

🧪 Test Coverage

The PR removes a test file:

examples/cursors/tests/cursors.test.ts (deleted 132 lines)

Question: Was this test file obsolete or is it being moved elsewhere? If it provided valuable test coverage, consider migrating it rather than deleting it.

📋 Recommendations

Consider adding a comment in the build script about why the Dockerfile paths use engine/docker/engine/ to prevent future confusion.
Version bumps: Several package.json files have version updates (test-runner, frontend). Ensure these follow your versioning strategy.
OpenAPI changes: The engine/artifacts/openapi.json file has significant changes (1759 additions, 1563 deletions). While this appears to be auto-generated, ensure the changes are intentional.
Missing test coverage: The deleted cursors test should either be replaced or documented as intentionally removed.

✅ Conclusion

This is a solid cleanup PR that fixes release script paths. The changes are well-scoped and don't introduce breaking changes. Once you address the test coverage question, this should be good to merge.

Overall Rating: ✅ Approve with minor questions

Review generated with Claude Code

claude · 2025-10-25T01:48:15Z

Pull Request Review: CI Fix for Engine Release Scripts

Summary

This PR fixes the release scripts for the engine by correcting Docker build paths and enabling ARM64 support. The changes are primarily infrastructure-focused with path corrections and CI improvements.

Positive Aspects

1. Path Corrections

✅ Correctly updated Docker paths: Changed from docker/engine/ to engine/docker/engine/ throughout workflows
✅ Consistent path updates: All references properly updated in both release.yaml and build scripts

2. ARM64 Support Restoration

✅ Re-enabled ARM64 builds: Removed the TODO comment and restored ARM64 Docker image building
✅ Proper architecture tagging: Added -amd64 and -arm64 suffixes to Docker images for clarity

3. macOS Build Improvements

✅ Native macOS runners: Switched from depot-ubuntu-24.04-4 to depot-macos-15 for macOS builds (lines 97, 102 in release.yaml)

This should improve build reliability and potentially performance for macOS targets

Issues & Recommendations

1. Critical: Duplicate `onActorStop` Call (TypeScript SDK)

Location: engine/sdks/typescript/runner/src/mod.ts:170-177

await this.#config.onActorStop(actorId, actor.generation);
// ... other code ...
this.#config.onActorStop(actorId, actor.generation).catch((err) => {

Issue: The onActorStop handler is called twice in the forceStopActor method:

First with await (line 163)
Second without await (line 170)

Impact: This could lead to:

Duplicate cleanup operations
Race conditions if the callback has side effects
Confusing logs/metrics showing actors stopped twice

Recommendation: Remove one of these calls. Based on the error handling pattern, I suggest keeping only the second call with .catch():

async forceStopActor(actorId: string, generation?: number) {
    const actor = this.#removeActor(actorId, generation);
    if (!actor) return;

    // Unregister actor from tunnel
    this.#tunnel?.unregisterActor(actor);

    this.#sendActorStateUpdate(actorId, actor.generation, "stopped");

    // Single call with error handling
    this.#config.onActorStop(actorId, actor.generation).catch((err) => {
        logger()?.error({
            msg: "error in onactorstop for actor",
            runnerId: this.runnerId,
            actorId,
            err,
        });
    });
}

2. Code Quality: Inconsistent Error Handling

Location: engine/sdks/typescript/runner/src/mod.ts:162-177

The error handling comment mentions "If onActorStop times out, Pegboard will handle this timeout" but then wraps the call in try/catch. The logic flow is confusing:

Try/catch that logs but doesn't prevent the second call
Second call that uses .catch() for error handling

Recommendation: Clarify the intended behavior and use a single, consistent error handling approach.

3. Test File Deletion

Location: examples/cursors/tests/cursors.test.ts (deleted)

Question: Was this test file removed intentionally? The PR description doesn't mention removing tests. If this is cleanup, consider mentioning it in the PR description.

4. OpenAPI Schema Changes

The engine/artifacts/openapi.json file shows significant regeneration (1,759 additions, 1,563 deletions). While mostly formatting changes, I notice:

Observation: The schema version remains 25.8.1 - if this is a new release, should the version be bumped?

5. Minor: Formatting Inconsistencies

Location: Error JSON files (engine/artifacts/errors/*.json)

The changes convert from tabs to 2-space indentation and remove trailing newlines. While this is fine, ensure your .editorconfig or formatting tools are configured consistently to avoid these changes appearing in future PRs.

Security Considerations

✅ No security issues identified

Secrets handling appears correct (R2_RELEASES_* secrets)
No hardcoded credentials
Token handling in TypeScript SDK looks appropriate

Performance Considerations

✅ Improved performance expected:

Native macOS runners should build faster than cross-compilation
ARM64 support enables better performance on ARM-based systems

⚠️ Potential concern: The duplicate onActorStop call could cause performance issues if the cleanup is expensive.

Testing Recommendations

Verify ARM64 builds: Test that ARM64 Docker images build and run correctly
Test macOS builds: Ensure native macOS builds work on both architectures
Actor lifecycle testing: Specifically test the forceStopActor path to ensure the duplicate call issue doesn't cause problems
Integration tests: Run the deleted cursor tests manually to confirm functionality isn't broken

Conclusion

This is a solid infrastructure fix that corrects important path issues and re-enables ARM64 support. The main concern is the duplicate onActorStop call in the TypeScript SDK which should be addressed before merging.

Recommendation: Address the duplicate call issue, then merge.

Review generated with Claude Code

claude · 2025-10-25T02:07:32Z

PR Review: Fix Release Scripts for Engine

Summary

This PR adds ARM64 (aarch64) support to the release pipeline and reorganizes Docker build paths from docker/ to engine/docker/. Overall, the changes are well-structured and address the stated goal of fixing release scripts.

✅ Strengths

ARM64 Support: Proper addition of aarch64 Linux builds to the release matrix, addressing modern ARM server requirements
Path Consolidation: Moving Docker files to engine/docker/ improves project organization
Consistent Formatting: JSON artifact files now use consistent 2-space indentation
Multi-platform Support: The build script properly handles all 5 target platforms (Linux x86_64/ARM64, macOS x86_64/ARM64, Windows)

🔍 Code Quality Observations

GitHub Actions Workflow (.github/workflows/release.yaml)

Good:

Proper use of depot-ubuntu-24.04-arm-4 runner for ARM builds
Clear comments explaining why Linux runners are used for macOS cross-compilation
Restored ARM64 Docker image builds (was TODOed out)
Consistent architecture suffix naming (-amd64, -arm64)

Suggestions:

Line 121: The workflow now uses -amd64 suffix for x86_64 images. Ensure this doesn't break any existing deployments expecting the old tagless format
Platform string inconsistency: Uses linux/arm64 for platform but aarch64 for target triple - this is correct but worth documenting
Build script path: Changed from docker/engine/build.sh to engine/docker/engine/build.sh - make sure this path exists in all branches where this workflow runs

Docker Build Script (engine/docker/engine/build.sh)

Good:

Clean case statement for all 5 platforms
Proper binary naming with .exe extension for Windows
Safe error handling with set -e
Correct platform mapping for cross-compilation

Potential Issues:

Line 51: Uses --platform $PLATFORM flag which may not be needed for all Docker build scenarios. BuildKit handles cross-compilation differently
Line 63-64: The chmod check uses bash-specific [[ ]] syntax, but the shebang is /bin/bash, so this is fine
Missing validation: No check that $DOCKERFILE file exists before attempting build
Cache consideration: No cache mount usage - rebuilding from scratch every time could be slow

Recommendation: Add file existence check:

if [ ! -f "engine/docker/engine/$DOCKERFILE" ]; then
  echo "Dockerfile not found: engine/docker/engine/$DOCKERFILE"
  exit 1
fi

Dockerfiles (linux-aarch64.Dockerfile)

Good:

Uses pinned Rust version (1.88.0)
Proper musl toolchain setup for static binaries
OpenSSL built from source for musl compatibility
BuildKit cache mounts for cargo registry/git/target

Concerns:

Line 52-59: Downloads and builds OpenSSL 1.1.1w - this version is EOL as of September 2023. Consider upgrading to OpenSSL 3.x for security updates
- Current: SSL_VER=1.1.1w (EOL)
- Recommended: SSL_VER=3.0.13 or later
Line 8-25: Many system packages installed, including git-lfs. Verify all are necessary for the build to minimize attack surface
Security: Downloading OpenSSL over HTTP (line 52) - should use HTTPS:
```
RUN wget https://www.openssl.org/source/openssl-$SSL_VER.tar.gz
```
Actually, checking line 52, it already uses HTTPS - good!
Build cache: The cargo cache mounts (lines 82-84) are excellent for CI performance

TypeScript SDK Changes

engine/sdks/typescript/runner/src/mod.ts:

Concerns:

Lines 168-170: Duplicate onActorStop call - this appears to be a bug:

await this.#config.onActorStop(actorId, actor.generation);  // Line 163
// ... error handling ...
this.#config.onActorStop(actorId, actor.generation).catch((err) => { // Line 170

The second call at line 170 will execute even if the first one completes successfully, potentially calling the callback twice. This should likely be removed or the logic restructured.

Error handling inconsistency: First call has try-catch, second call uses .catch() - pick one pattern

engine/sdks/typescript/runner/src/tunnel.ts:

No significant issues found. The tunnel implementation looks solid with:

Proper garbage collection of unacked messages
Clean WebSocket lifecycle management
Good error handling

engine/sdks/typescript/test-runner/src/index.ts:

Minor issues:

Line 28: Uses Promise.withResolvers() which is a relatively new API (2024) - ensure target Node.js version supports it
Auto-configuration at startup (lines 112-141): Makes HTTP request without retry logic - could fail on transient network issues

🔒 Security Considerations

OpenSSL version: Using EOL OpenSSL 1.1.1w (see above)
No checksum verification: OpenSSL tarball downloaded without signature/checksum verification
RIVET_TOKEN default: Line 24 in test-runner uses "dev" as default token - ensure this isn't used in production

🧪 Testing Recommendations

Build verification: Test all 5 platform builds actually produce working binaries
ARM64 smoke test: Deploy and run basic workload on ARM64 to verify functionality
Cross-compilation validation: Ensure macOS binaries built on Linux actually work on macOS
Docker manifest: Verify multi-arch Docker images are properly tagged and pullable

📊 Performance Considerations

OpenSSL build time: Building OpenSSL from source adds significant time to Docker builds - consider multi-stage builds with cached SSL layers
Cargo cache: Good use of BuildKit cache mounts, but consider adding sccache for even faster Rust compilation

🐛 Bugs Found

Critical:

mod.ts:168-170: Duplicate onActorStop callback execution (see above)

Minor:

Test file deletion (examples/cursors/tests/cursors.test.ts) - ensure this was intentional

✏️ Style & Conventions

Good adherence to CLAUDE.md:

Hard tabs in Rust (would be enforced by rustfmt)
Proper use of workspace dependencies
Structured logging patterns maintained

Whitespace changes:

Several .yml and Dockerfile changes include trailing whitespace removal (good cleanup)

📝 Final Recommendations

Must Fix Before Merge:

Fix duplicate onActorStop call in mod.ts:168-170

Should Consider:

Upgrade OpenSSL to 3.x in ARM64 Dockerfile
Add checksum verification for OpenSSL download
Add file existence check in build.sh
Document the arch suffix change for Docker images
Verify deleted test file (cursors.test.ts) was intentional

Nice to Have:

Add retry logic to auto-configuration in test-runner
Consider sccache for faster Rust builds
Multi-stage Docker build to cache OpenSSL compilation

Overall Assessment

Quality: Good ⭐⭐⭐⭐ (4/5)
Security: Moderate ⚠️ (OpenSSL EOL version)
Testing: Needs verification 🧪
Documentation: Could be improved 📚

The PR successfully achieves its goal of adding ARM64 support and reorganizing paths. The main concern is the duplicate callback invocation bug that should be fixed before merge.

Review generated with assistance from Claude Code

claude · 2025-10-25T04:37:09Z

Pull Request Review: CI Fix for Engine Release Scripts

Summary

This PR addresses critical issues in the release workflow for the Rivet engine, adding ARM64 support, fixing file paths, and improving Docker build configurations. Overall, this is a well-structured fix that modernizes the build infrastructure.

✅ Positive Changes

1. ARM64 Support Re-enabled

Lines: .github/workflows/release.yaml:86-90, 163-168
Re-introduces ARM64 Linux binary builds (aarch64-unknown-linux-musl)
Properly configures ARM-specific runners (depot-ubuntu-24.04-arm-4)
Adds architecture-specific AWS CLI installation for ARM runners (138-142)
Fixes Docker image manifest generation with proper -arm64 and -amd64 suffixes

Impact: Enables production ARM deployments, critical for cost optimization on ARM-based infrastructure.

2. Path Corrections

Lines: .github/workflows/release.yaml:123, 195, 212
Corrects paths from docker/engine/ to engine/docker/engine/
Fixes docker/universal/Dockerfile to engine/docker/universal/Dockerfile

Impact: Essential fix - previous paths would cause build failures.

3. Improved .dockerignore

Lines: .dockerignore:3-26
Adds comprehensive exclusions for CI/CD configs, documentation, and editor files
Reduces Docker build context size, improving build performance

Impact: Faster builds and smaller context transfers to Docker daemon.

4. TypeScript SDK Improvements

Runner SDK (`engine/sdks/typescript/runner/src/mod.ts`)

Better error handling for WebSocket connection failures
Improved cleanup logic for actor WebSockets
More robust request/response tracking

Test Runner (`engine/sdks/typescript/test-runner/src/index.ts`)

Adds serverless initialization packet support via SSE streaming
Improves runner lifecycle management with Promise.withResolvers()
Better separation of concerns between runner start/stop

Impact: More reliable SDK behavior in production environments.

⚠️ Issues & Concerns

1. Critical: Deleted Test File Without Explanation

Line: examples/cursors/tests/cursors.test.ts (deleted, -132 lines)
Severity: High
Issue: A complete test file was removed with no explanation in the PR description or commit message.

Recommendation:

If this test is obsolete, document why in the PR description
If this was accidental, restore the file
If the test was moved, show the new location

2. Security: AWS Credentials in CI

Lines: .github/workflows/release.yaml:130-131, 246-247
Uses R2/S3 credentials directly in workflow
Credentials are properly stored as secrets ✓

Observation: Current implementation is secure, but consider:

Using OIDC/Workload Identity for secret-less authentication
Adding checksum verification for uploaded binaries (currently only using CRC32)

3. Whitespace-Only Changes in Artifacts

Files: engine/artifacts/errors/*.json
Lines: All error JSON files show formatting changes (tabs → spaces, missing newline)
Issue: These appear to be auto-formatter changes that add noise to the diff

Recommendation:

According to CLAUDE.md, you should NOT run formatters manually
These changes suggest a formatter was run
Consider reverting these if they weren't intentional, or document the formatting tool change

4. Massive OpenAPI Schema Changes

File: engine/artifacts/openapi.json
Lines: 1863 deletions, 1759 additions
Issue: The diff is too large to review effectively in this PR

Recommendation:

Was this regenerated from code changes?
If so, what code changes triggered this?
Consider splitting schema regeneration into a separate commit with explanation

5. Docker Build Context Location

Lines: engine/docker/engine/build.sh:51, 53
Builds from repo root (.) but Dockerfile is in engine/docker/engine/
This requires careful .dockerignore management

Observation: Works correctly, but could be more explicit:

# Consider adding a comment explaining why context is repo root
DOCKER_BUILDKIT=1 docker build --platform $PLATFORM --target $TARGET_STAGE \
  -f engine/docker/engine/$DOCKERFILE \
  -t rivet-engine-builder-$TARGET \
  .  # Context is repo root to access all workspace packages

6. Hardcoded OpenSSL Version

Line: engine/docker/engine/linux-aarch64.Dockerfile:56
Uses OpenSSL 1.1.1w (EOL version)

Recommendation:

OpenSSL 1.1.1 reached EOL in September 2023
Consider upgrading to OpenSSL 3.x for security updates
If 1.1.1w is required for compatibility, document why

🔍 Code Quality Notes

Good Practices Followed

✅ Proper error handling with set -e in bash scripts
✅ Informative echo statements for build progress
✅ Platform-specific logic cleanly organized in case statements
✅ Proper use of build caches in Dockerfiles
✅ Environment variables properly scoped

Adherence to CLAUDE.md

✅ Uses cargo build --release as documented
✅ Proper use of workspace structure
⚠️ Possible violation of "Do not format the code yourself" (artifact changes)

📊 Performance Considerations

Positive

Docker layer caching with --mount=type=cache reduces rebuild times
Parallel builds across architectures via matrix strategy
Efficient binary extraction from containers

Potential Improvements

Consider using cargo chef for better Rust dependency caching
Multi-stage builds could be optimized to share more layers between architectures

🧪 Test Coverage

Concerns

❌ Deleted test file (cursors.test.ts) reduces coverage
⚠️ No evidence of CI tests running against new ARM64 builds
⚠️ No integration tests visible for cross-compilation correctness

Recommendations

Add smoke tests for each architecture's binary
Test that ARM64 binaries actually work on ARM64 runners
Verify cross-compiled macOS binaries (built on Linux) are valid

🔐 Security Review

Low Risk Items

Secrets properly stored in GitHub secrets
No hardcoded credentials
Proper use of HTTPS for downloads

Medium Risk Items

Using wget/curl without checksum verification (line 27 in Dockerfile)
CRC32 checksums for S3 uploads (CRC32 is not cryptographically secure)

Recommendations

# Add checksum verification for security-critical downloads
RUN wget -q https://musl.cc/aarch64-linux-musl-cross.tgz && \
    echo "EXPECTED_SHA256_HERE  aarch64-linux-musl-cross.tgz" | sha256sum -c - && \
    tar -xzf aarch64-linux-musl-cross.tgz -C /opt/

📝 Final Recommendations

Must Address Before Merge

Explain or restore deleted test file (cursors.test.ts)
Document why OpenAPI schema changed so drastically
Clarify if artifact JSON formatting changes were intentional

Should Address

Consider OpenSSL version upgrade to 3.x
Add checksums for downloaded toolchains
Add architecture-specific smoke tests

Nice to Have

Add more detailed PR description explaining the changes
Split large formatting/regeneration changes into separate commits
Consider cargo-chef for better layer caching

✅ Approval Status

Pending - Please address the "Must Address" items, particularly:

Explain the deleted test file
Document the OpenAPI schema changes
Clarify the artifact formatting changes

Once these are addressed, this PR will be ready to merge. The core functionality improvements (ARM64 support, path fixes) are solid and well-implemented.

Overall Assessment: Good technical implementation with some documentation and cleanup concerns. The infrastructure improvements are valuable, but the PR needs better change documentation.

claude · 2025-10-25T05:18:25Z

PR Review: ci: fix release scripts for engine

Summary

This PR updates the release infrastructure to fix engine build and release processes. The changes include adding ARM64 support, fixing Docker build paths, improving the .dockerignore, and making TypeScript SDK improvements.

Positive Aspects ✅

ARM64 Support Added: Properly adds Linux ARM64 builds for both binaries and Docker images, addressing the TODO comment about RVT-4479.
Correct Path Updates: Properly updates paths from docker/engine/ to engine/docker/engine/ throughout the workflow, maintaining consistency.
Enhanced .dockerignore: Much better Docker build performance with comprehensive exclusions for CI/CD, documentation, and editor configs. This will significantly reduce Docker context size.
Improved TypeScript SDK:
- Better error handling and logging
- Added useNodejsImportProtocol Biome rule for consistency
- WebSocket improvements in the runner

Issues and Recommendations 🔍

Critical Issues

1. Docker Build Path Consistency (release.yaml:123)

engine/docker/engine/build.sh ${{ matrix.target }}

The script path was updated from docker/engine/build.sh to engine/docker/engine/build.sh, but we should verify this path is correct. Looking at the build.sh file, it references engine/docker/engine/$DOCKERFILE internally, which suggests the script location is correct.

2. Dockerfile Path References (build.sh:51,53)
The build.sh script now references paths like:

DOCKER_BUILDKIT=1 docker build --platform $PLATFORM --target $TARGET_STAGE -f engine/docker/engine/$DOCKERFILE -t rivet-engine-builder-$TARGET .

This assumes the script is run from the repository root. We should ensure the GitHub Actions workflow runs from the correct directory.

Moderate Issues

3. Missing Whitespace Changes (rust.yml)
The changes to .github/workflows/rust.yml only modify trailing whitespace. While this is good for consistency, it's cosmetic and could have been in a separate commit.

4. Duplicate onActorStop Call (engine/sdks/typescript/runner/src/mod.ts:170)
In the forceStopActor function, there appears to be a duplicate call to onActorStop:

await this.#config.onActorStop(actorId, actor.generation);  // Line 163
// ...
this.#config.onActorStop(actorId, actor.generation).catch(...);  // Line 170

The second call is not awaited and has error handling, while the first one is awaited with a try-catch. This seems redundant and could cause confusion.

5. OpenAPI Schema Changes
The massive OpenAPI schema file changes are just formatting (tabs to spaces), but make the diff very large. Consider using a consistent formatter for these generated files.

Minor Issues

6. Test File Deletion (examples/cursors/tests/cursors.test.ts)
A test file was completely deleted. While this might be intentional cleanup, there's no explanation in the PR description. If this test is no longer needed, it should be documented why.

7. Platform Architecture Naming (release.yaml:168)
For consistency with the binaries job, consider whether linux/x86_64 should be linux/amd64 in the Docker platform specification. The binaries use x86_64 but Docker typically uses amd64.

8. Commented Code (release.yaml:201-204, 218-221)
Multiple sections have commented-out code for FontAwesome secrets. Either remove these entirely or add a comment explaining why they're kept for future use.

Code Quality Assessment

Strengths

✅ Follows workspace conventions (Cargo workspace patterns)
✅ Good error handling in TypeScript SDK
✅ Proper use of structured logging patterns
✅ Comments explain complex logic (e.g., "Use Linux instead of macOS builders")

Concerns

⚠️ Large generated file changes (openapi.json) obscure meaningful changes
⚠️ Possible duplicate function calls in TypeScript
⚠️ No test updates accompanying the changes

Security Considerations

✅ No security concerns identified

Secrets handling looks appropriate
No credential leakage in code
Docker build process follows security best practices

Performance Considerations

✅ Improvements identified

Better .dockerignore will reduce Docker context transfer time
Parallel ARM/x86 builds should improve CI performance
Cache mounts in Dockerfiles are properly configured

Test Coverage

⚠️ Test coverage concerns

No new tests added for the release infrastructure changes
One test file deleted (cursors.test.ts) without explanation
Should consider adding integration tests for the release pipeline

Recommendations

High Priority

Verify the duplicate onActorStop call in engine/sdks/typescript/runner/src/mod.ts:163-170 - this looks like a bug
Document the cursors test deletion or restore if it was removed accidentally
Test the release pipeline end-to-end before merging, especially the new ARM64 builds

Medium Priority

Consider excluding generated files like openapi.json from formatting changes in separate commits
Remove or document the commented FontAwesome secret code blocks
Add integration tests for the release pipeline

Low Priority

Clean up whitespace-only changes in rust.yml to a separate commit
Consider standardizing platform architecture names (amd64 vs x86_64)

Conclusion

This is a solid infrastructure improvement that adds important ARM64 support and fixes path issues. The main concern is the potential duplicate function call in the TypeScript SDK that needs verification. Once that's addressed, this should be good to merge.

Recommendation: Request changes to address the duplicate onActorStop call, then approve after verification.

Review generated by Claude Code

claude · 2025-10-25T05:39:14Z

Pull Request Review: CI/CD Release Scripts Fixes

Overview

This PR fixes and improves the release scripts for the engine, focusing on ARM64 support, Docker build paths, and code formatting.

Strengths

ARM64 Support Addition

Added ARM64 Linux builder to the release workflow
Properly configured ARM64-specific runners
Docker image manifest generation now includes ARM64 variants
Cross-compilation setup is well-configured

Path Corrections

Fixed Docker build script paths
Updated Dockerfile paths consistently
Consistent path updates across workflow files

Docker Improvements

Enhanced .dockerignore with comprehensive exclusions
Multi-stage Docker builds properly configured

TypeScript SDK Enhancements

Added Node.js import protocol enforcement
Improved import statements throughout
Better structured logging

Issues and Concerns

1. Critical: Docker Cache Mount ID Typos
File: engine/docker/universal/Dockerfile:64-67

Typo: univseral instead of universal (lines 64-66)
Impact: Creates separate cache entries instead of reusing existing caches, slowing down builds
Recommendation: Fix typos to universal

2. Critical: Missing Integration Tests

No test coverage for ARM64 builds
No validation of cross-platform Docker images
Recommendation: Add integration tests or smoke tests

3. TypeScript: Duplicate onActorStop Call
File: engine/sdks/typescript/runner/src/mod.ts:163 and 170

onActorStop is called twice
Second call has no await, creating a fire-and-forget promise
Recommendation: Remove duplicate or document why needed

4. Deleted Test File
File: examples/cursors/tests/cursors.test.ts (deleted, 132 lines)

No explanation for removal
Recommendation: Clarify if intentional

5. Security: .dockerignore Gaps

.env files not excluded (only .env*.local)
No exclusion for credentials.json, *.pem, *.key
Recommendation: Add comprehensive secret file exclusions

6. Build Script Error Handling
File: engine/docker/engine/build.sh

No validation of Docker availability
No DOCKERFILE existence check
Recommendation: Add error handling

Recommendations

High Priority:

Fix Docker cache mount typos (univseral to universal)
Add platform validation tests for ARM64
Remove duplicate onActorStop call or document
Explain deleted cursors.test.ts

Medium Priority:
5. Enhance .dockerignore with secret patterns
6. Add error handling to build.sh
7. Verify OpenAPI changes are formatting-only

Approval Status

Overall Assessment: The PR makes meaningful improvements but has several issues that should be addressed.

Recommended Action: Request Changes

Key Blockers:

Docker cache mount typos will significantly impact build performance
Missing test coverage for new ARM64 platform
Duplicate async call in runner needs clarification

Great work on adding ARM64 support! The foundation is solid, but please address the cache mount typos and consider adding platform validation tests before merging.

claude · 2025-10-25T07:47:53Z

Pull Request Review: ci: fix release scripts for engine

Overview

This PR addresses CI/CD release workflow issues for the engine, primarily fixing Docker file paths and improving build configurations. The changes span CI workflows, Docker configurations, TypeScript SDK improvements, and code quality enhancements.

✅ Strengths

1. Correct Path Updates

The PR properly fixes the Docker file paths from the old docker/ location to the new engine/docker/ structure:

.github/workflows/release.yaml:124 - Updated build script path
.github/workflows/release.yaml:196,213 - Updated Dockerfile paths
Good consistency across all references

2. Improved .dockerignore

The enhanced .dockerignore file (.dockerignore:1-26) significantly improves build performance:

Excludes CI/CD configs, documentation, and editor files
Reduces Docker context size
Follows best practices with explicit inclusions like !README.md

3. Code Quality Improvements

TypeScript changes show attention to code quality:

Added useNodejsImportProtocol: error in biome.json:35 enforces Node.js import protocol (node: prefix)
Consistent use of const instead of let (e.g., engine/sdks/typescript/runner/src/mod.ts:292,301,314)
Improved formatting for readability with multi-line function signatures

4. Multi-Architecture Support

Re-enabled ARM64 Docker builds (.github/workflows/release.yaml:164-166):

Proper use of depot-ubuntu-24.04-arm-4 runner
Correct arch_suffix for both architectures (-arm64 and -amd64)

⚠️ Areas of Concern

1. Commented Out ARM Binary Build

Lines .github/workflows/release.yaml:86-91 comment out ARM Linux binary build with TODO:

# TODO: Add back when working
# - platform: linux
#   runner: depot-ubuntu-24.04-arm-4
#   target: aarch64-unknown-linux-musl

Question: What's blocking the ARM binary build? The Docker ARM builds are enabled, so there may be an inconsistency here.

2. Whitespace-Only Changes

Several files have only formatting changes:

.github/workflows/rust.yml - trailing whitespace cleanup
Multiple error JSON files - just indentation changes
These inflate the PR size unnecessarily

Recommendation: These could have been in a separate formatting PR, though they don't harm functionality.

3. Large Generated File Changes

engine/artifacts/openapi.json has 1759 additions and 1563 deletions, but appears to be mostly reformatting. This makes the diff harder to review.

Question: Was this regenerated as part of the build process? If so, consider adding a note in the PR description.

4. Missing Build Script Context

The build script path changed from docker/engine/build.sh to engine/docker/engine/build.sh, but there's no context about:

Why the reorganization happened
Whether this is part of a larger monorepo restructuring
If there are other path references that might need updating

5. Deleted Test File

examples/cursors/tests/cursors.test.ts was completely deleted (132 lines).
Question: Was this test obsolete, or is it being replaced elsewhere? Loss of test coverage should be explained.

🔍 Specific Technical Observations

Docker Build Configuration

The Dockerfile at engine/docker/engine/linux-x86_64.Dockerfile looks solid:

Proper multi-stage build
Correct musl toolchain setup for static linking
Good use of build caches (--mount=type=cache)
Static linking flags are appropriate for portable binaries

Build Script (`engine/docker/engine/build.sh`)

Well-structured with good error handling:

Proper platform detection
Good use of set -e
Skips chmod +x for Windows binaries (correct)
Clear error messages

TypeScript SDK Changes

Mostly formatting improvements, but one substantive change:

Better structured logging with multi-line object literals
Changed let to const where appropriate (good immutability practice)
Added missing semicolon at mod.ts:315

🛡️ Security Considerations

No security concerns identified
The build uses official Rust Docker images
No credential exposure in the changes
Proper use of GitHub secrets in workflows

🚀 Performance Considerations

Positive impacts:

Improved .dockerignore reduces Docker context size
Multi-architecture builds in parallel improve CI efficiency
Build cache mounts in Dockerfiles speed up rebuilds

📋 Recommendations

Add PR Description: Include context about:
- Why ARM binary builds are disabled
- The path reorganization rationale
- What triggered the OpenAPI regeneration
Consider Splitting: For future PRs, consider separating:
- Functional fixes (path updates, build fixes)
- Code formatting (biome, whitespace)
- Generated artifacts (OpenAPI)
Document ARM Binary Issue: Create a ticket for the ARM binary build issue if not already tracked
Test Coverage: Verify that deleting cursors.test.ts doesn't leave functionality untested

✅ Approval Recommendation

APPROVE with minor suggestions

The core changes are solid and achieve the stated goal of fixing release scripts. The concerns raised are mostly about process and documentation, not functionality. The code quality improvements are a nice bonus.

Review generated by Claude Code

claude · 2025-10-25T09:25:22Z

PR Review: CI Fix for Engine Release Scripts

Strengths

Path Corrections: Successfully updated paths from docker/ to engine/docker/
Enhanced .dockerignore: Good additions to reduce build context size
Architecture Support: ARM64 Docker support re-enabled with proper arch_suffix
TypeScript Improvements: Added useNodejsImportProtocol biome rule

Critical Issues

1. ARM64 Binary Build Commented Out

Linux ARM64 binary build is disabled (release.yaml:86-91)
But ARM64 Docker builds ARE enabled (lines 164-166)
Risk: Docker images built without corresponding binaries
Recommendation: Enable ARM64 binary builds or document why Docker-only is OK

2. Test File Deletion

132 lines removed from examples/cursors/tests/cursors.test.ts
Risk: Loss of test coverage
Recommendation: Explain in commit message or separate PR

3. Missing Error Handling (engine/docker/engine/build.sh)

Docker commands lack explicit error checking beyond set -e
docker cp could fail silently if container lacks expected binary
Recommendation: Add checks to verify binary exists after extraction

4. Hardcoded Token (engine/sdks/typescript/test-runner/src/index.ts:24)

Default token dev should only be in development
Consider warning if production environment detected with default

Code Quality Issues

JSON formatting changed (tabs to spaces, missing trailing newlines)
Magic numbers in test-runner config (hardcoded ports, defaults)
Missing input validation in RunnerConfig (no bounds checking)
Commented dead code in release.yaml (lines 202-205, 219-222) - remove it
Repeated comments could be consolidated

Performance Notes

Docker build context still entire monorepo despite .dockerignore improvements
Docker job depends on binaries but doesn't use them - could run in parallel

Minor Suggestions

Log messages should be lowercase per CLAUDE.md
Whitespace changes ideally in separate formatting commit

Testing Recommendations

Test ARM64 Docker build without ARM64 binary
Verify binary extraction from all Dockerfiles
Test .dockerignore doesn't exclude required files
Verify release script end-to-end with new paths
Confirm cursors test deletion was intentional

Summary

This PR fixes the core path issue effectively. Main concerns:

ARM64 binary/Docker inconsistency
Deleted test file unexplained
Missing error handling in build scripts

Recommendation: Address critical issues 1, 2, and 3 before merging.

Overall: Good foundation, needs minor fixes before merge

NathanFlurry force-pushed the 10-24-ci_fix_release_scripts_for_engine branch from c6b0205 to db025c7 Compare October 24, 2025 21:10

This was referenced Oct 24, 2025

fix(fe): bring back ts type check #3259

Closed

fix: make inspector working again #3229

Closed

fix(fe): invalid types #3260

Closed

NathanFlurry force-pushed the 10-24-ci_fix_release_scripts_for_engine branch from db025c7 to df5221f Compare October 25, 2025 00:50

NathanFlurry force-pushed the 10-24-ci_fix_release_scripts_for_engine branch from df5221f to e32f86f Compare October 25, 2025 00:56

NathanFlurry force-pushed the 10-24-ci_fix_release_scripts_for_engine branch from e32f86f to eafdf05 Compare October 25, 2025 00:59

NathanFlurry force-pushed the 10-24-ci_fix_release_scripts_for_engine branch from eafdf05 to 22b7294 Compare October 25, 2025 01:29

vercel bot deployed to Preview – rivet-inspector October 25, 2025 01:32 View deployment

NathanFlurry force-pushed the 10-24-ci_fix_release_scripts_for_engine branch from 22b7294 to 6aaeac5 Compare October 25, 2025 01:32

vercel bot deployed to Preview – rivet-inspector October 25, 2025 01:34 View deployment

NathanFlurry force-pushed the 10-24-ci_fix_release_scripts_for_engine branch from 6aaeac5 to 0b5b7da Compare October 25, 2025 01:42

vercel bot deployed to Preview – rivet-inspector October 25, 2025 01:44 View deployment

NathanFlurry force-pushed the 10-24-ci_fix_release_scripts_for_engine branch from 0b5b7da to 33d40c9 Compare October 25, 2025 01:46

vercel bot deployed to Preview – rivet-inspector October 25, 2025 01:47 View deployment

NathanFlurry force-pushed the 10-24-ci_fix_release_scripts_for_engine branch from 33d40c9 to 6ea8762 Compare October 25, 2025 02:05

vercel bot deployed to Preview – rivet-inspector October 25, 2025 02:07 View deployment

NathanFlurry force-pushed the 10-24-ci_fix_release_scripts_for_engine branch from 6ea8762 to efbb703 Compare October 25, 2025 04:35

vercel bot deployed to Preview – rivet-inspector October 25, 2025 04:36 View deployment

NathanFlurry force-pushed the 10-24-ci_fix_release_scripts_for_engine branch from efbb703 to c2d4617 Compare October 25, 2025 05:16

vercel bot deployed to Preview – rivet-inspector October 25, 2025 05:19 View deployment

NathanFlurry force-pushed the 10-24-ci_fix_release_scripts_for_engine branch from c2d4617 to 5bf04f4 Compare October 25, 2025 05:35

vercel bot deployed to Preview – rivet-inspector October 25, 2025 05:36 View deployment

NathanFlurry force-pushed the 10-24-ci_fix_release_scripts_for_engine branch from 5bf04f4 to a707646 Compare October 25, 2025 07:45

vercel bot deployed to Preview – rivet-inspector October 25, 2025 07:47 View deployment

NathanFlurry added 2 commits October 25, 2025 02:22

chore(scripts): add pg backup & restore scripts for dev

21324dd

ci: fix release scripts for engine

1fd2b52

NathanFlurry force-pushed the 10-23-chore_scripts_add_pg_backup_restore_scripts_for_dev branch from 00e258e to 21324dd Compare October 25, 2025 09:22

NathanFlurry force-pushed the 10-24-ci_fix_release_scripts_for_engine branch from a707646 to 1fd2b52 Compare October 25, 2025 09:22

Base automatically changed from 10-23-chore_scripts_add_pg_backup_restore_scripts_for_dev to main October 25, 2025 09:23

NathanFlurry merged commit 8ef51b0 into main Oct 25, 2025
8 of 12 checks passed

NathanFlurry deleted the 10-24-ci_fix_release_scripts_for_engine branch October 25, 2025 09:23

vercel bot deployed to Preview – rivet-inspector October 25, 2025 09:23 View deployment

ci: fix release scripts for engine #3258

ci: fix release scripts for engine #3258

Conversation

NathanFlurry commented Oct 24, 2025

Uh oh!

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

Uh oh!

Uh oh!

NathanFlurry commented Oct 24, 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 Oct 24, 2025

Code Review Summary

✅ Positive Aspects

📝 Code Quality Observations

🔍 Potential Issues

🎯 Recommendations

✅ Security & Performance

📊 Test Coverage

Final Verdict

Uh oh!

claude bot commented Oct 24, 2025

PR Review: CI Fix for Engine Release Scripts

Overview

Code Quality & Best Practices ✅

File-by-File Analysis

GitHub Actions Workflows

TypeScript SDK Changes

Test Deletions

Infrastructure Scripts

Potential Bugs 🐛

Performance Considerations ⚡

Security Concerns 🔒

Test Coverage 📊

Recommendations

Summary

Uh oh!

pkg-pr-new bot commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude bot commented Oct 25, 2025

PR Review - CI: Fix Release Scripts for Engine

✅ Positive Aspects

📋 Code Quality & Best Practices

GitHub Workflows (.github/workflows/)

TypeScript SDK Changes

RivetKit Changes

🔒 Security Considerations

⚡ Performance Considerations

🧪 Test Coverage

🐛 Potential Bugs

📦 Artifact Changes

✨ Recommendations

📊 Overall Assessment

Uh oh!

claude bot commented Oct 25, 2025

PR Review: Fix Release Scripts for Engine

Overview

✅ Positive Aspects

🐛 Potential Issues

1. Critical: Duplicate onActorStop Call (mod.ts:163-177)

2. Logging Inconsistency (mod.ts:164)

3. Potential Memory Leak (tunnel.ts:52-53)

4. Edge Case: Concurrent Actor Stop (mod.ts:192-213)

📊 Code Quality Observations

🔒 Security Considerations

🧪 Test Coverage

📝 Documentation

🎯 Summary

Uh oh!

claude bot commented Oct 25, 2025

PR Review: CI Fix for Engine Release Scripts

Summary

Overall Assessment

Code Quality

Key Changes

Security & Performance

Recommendations

Final Verdict

vercel bot commented Oct 24, 2025 •

edited

Loading

NathanFlurry commented Oct 24, 2025 •

edited

Loading

pkg-pr-new bot commented Oct 24, 2025 •

edited

Loading

1. Critical: Duplicate `onActorStop` Call (mod.ts:163-177)

4. Duplicate Code in `mod.ts` 🐛