feat(core): support running engine #1353
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Claude finished @NathanFlurry's task —— View job PR Review: feat(core): support running engineTodo List:
Analysis SummaryThis PR adds support for running a Rivet engine process automatically within the RivetKit runtime. The implementation includes:
🚨 Critical Issues1. Debug Code Left in Production (
|
|
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.
How to use the Graphite Merge QueueAdd either label to this PR to merge it via 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. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| console.log("check 1 ===="); | ||
| try { | ||
| await fs.access(path); | ||
| return true; | ||
| } catch { | ||
| console.log("check 2 ===="); | ||
| console.trace("pathExists checking ======== lksjdflkdsjf", path); | ||
| return false; |
There was a problem hiding this comment.
The pathExists function has been modified to always return false in the try block without actually checking if the path exists. This breaks the function's intended behavior.
The original implementation correctly used fs.access(path) to check for path existence, returning true when the path exists and catching exceptions when it doesn't.
Please restore the original implementation:
try {
await fs.access(path);
return true;
} catch {
return false;
}The debug logging can be kept if needed, but the core functionality should be preserved.
| console.log("check 1 ===="); | |
| try { | |
| await fs.access(path); | |
| return true; | |
| } catch { | |
| console.log("check 2 ===="); | |
| console.trace("pathExists checking ======== lksjdflkdsjf", path); | |
| return false; | |
| console.log("check 1 ===="); | |
| try { | |
| console.log("check 2 ===="); | |
| console.trace("pathExists checking ======== lksjdflkdsjf", path); | |
| await fs.access(path); | |
| return true; | |
| } catch { | |
| return false; |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| ): Promise<void> { | ||
| console.log("ensuring path ====", directoryPath); | ||
| if (!(await pathExists(directoryPath))) { | ||
| throw "ERROR"; |
There was a problem hiding this comment.
The throw "ERROR"; statement prevents the directory creation logic from executing. This appears to be debugging code that was accidentally left in the PR. Removing this line will allow the function to properly create directories when they don't exist, which is the intended behavior of ensureDirectoryExists().
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
More templates
@rivetkit/actor
@rivetkit/cloudflare-workers
@rivetkit/core
@rivetkit/db
@rivetkit/framework-base
@rivetkit/next-js
@rivetkit/react
rivetkit
@rivetkit/sql-loader
commit: |
NathanFlurry
force-pushed
the
10-04-feat_core_support_running_engine
branch
from
e42ae28 to
d97a688
Compare
|
Claude finished @NathanFlurry's task —— View job Reviewing PR: feat(core): support running engine Todo List:
Analysis SummaryThis PR adds support for running a Rivet engine process automatically within the RivetKit runtime. The implementation includes:
🚨 Critical Issues1. Asynchronous Promise Handling in Synchronous Context (
|
NathanFlurry
force-pushed
the
10-04-fix_next_fix_local_dev
branch
from
75b93de to
03216a5
Compare
NathanFlurry
force-pushed
the
10-04-feat_core_support_running_engine
branch
from
d97a688 to
fd1d789
Compare
|
Claude finished @NathanFlurry's task —— View job Reviewing PR: feat(core): support running engine Todo List:
Analysis SummaryThis PR adds support for running a Rivet engine process automatically within the RivetKit runtime. The implementation includes:
✅ Positive Aspects
🐛 Issues and Recommendations1. Hardcoded Default Values (
|
|
Claude finished @NathanFlurry's task —— View job Reviewing PR: feat(core): support running engine Todo List:
Analysis SummaryThis PR adds support for running a Rivet engine process automatically within the RivetKit runtime. The implementation includes:
✅ Critical Issues Resolved: The debug code issues from previous reviews have been fixed - file system utilities now work correctly. 🚨 Areas for Improvement1. Limited Platform Support (
|
| invariant( | ||
| config.endpoint === undefined, | ||
| "cannot specify 'endpoint' with 'runEngine'", | ||
| ); | ||
| config.endpoint = ENGINE_ENDPOINT; |
There was a problem hiding this comment.
There appears to be contradictory logic in this section. The code first uses invariant() to assert that config.endpoint must be undefined when using runEngine, but then immediately sets config.endpoint = ENGINE_ENDPOINT in the next line.
This creates a confusing flow where the code requires a condition and then immediately changes it. Consider either:
- Removing the invariant check since you're setting the endpoint anyway, or
- Modifying the check to verify that if
config.endpointis defined, it matchesENGINE_ENDPOINT
For example:
invariant(
config.endpoint === undefined || config.endpoint === ENGINE_ENDPOINT,
"When using 'runEngine', 'endpoint' must either be undefined or match the engine endpoint"
);
config.endpoint = ENGINE_ENDPOINT;| invariant( | |
| config.endpoint === undefined, | |
| "cannot specify 'endpoint' with 'runEngine'", | |
| ); | |
| config.endpoint = ENGINE_ENDPOINT; | |
| invariant( | |
| config.endpoint === undefined || config.endpoint === ENGINE_ENDPOINT, | |
| "When using 'runEngine', 'endpoint' must either be undefined or match the engine endpoint", | |
| ); | |
| config.endpoint = ENGINE_ENDPOINT; |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| for (let i = 0; i < maxRetries; i++) { | ||
| try { | ||
| const response = await fetch(`${ENGINE_ENDPOINT}/health`); | ||
| if (response.ok) { | ||
| logger().debug({ msg: "engine health check passed" }); | ||
| return; | ||
| } | ||
| } catch (error) { |
There was a problem hiding this comment.
The current implementation doesn't handle the case where the health check response is not OK but doesn't throw an exception. Consider adding an else clause to handle non-200 responses explicitly:
try {
const response = await fetch(`${ENGINE_ENDPOINT}/health`);
if (response.ok) {
logger().debug({ msg: "engine health check passed" });
return;
} else {
if (i === maxRetries - 1) {
throw new Error(`engine health check failed after ${maxRetries} retries: HTTP ${response.status}`);
}
}
} catch (error) {
// existing error handling
}This ensures proper error reporting when the health endpoint returns a non-200 status code.
| for (let i = 0; i < maxRetries; i++) { | |
| try { | |
| const response = await fetch(`${ENGINE_ENDPOINT}/health`); | |
| if (response.ok) { | |
| logger().debug({ msg: "engine health check passed" }); | |
| return; | |
| } | |
| } catch (error) { | |
| for (let i = 0; i < maxRetries; i++) { | |
| try { | |
| const response = await fetch(`${ENGINE_ENDPOINT}/health`); | |
| if (response.ok) { | |
| logger().debug({ msg: "engine health check passed" }); | |
| return; | |
| } else { | |
| if (i === maxRetries - 1) { | |
| throw new Error(`engine health check failed after ${maxRetries} retries: HTTP ${response.status}`); | |
| } | |
| } | |
| } catch (error) { |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
NathanFlurry
force-pushed
the
10-04-feat_core_support_running_engine
branch
from
fd1d789 to
6c3e109
Compare
|
Claude finished @NathanFlurry's task —— View job PR Review: feat(core): support running engineTodo List:
Analysis SummaryThis PR adds support for running a Rivet engine process automatically within the RivetKit runtime. The implementation includes:
✅ Excellent Implementation AspectsRobust Process Management
Cross-Platform Support
Code Quality
🐛 Issues and RecommendationsHigh Priority1. Limited Platform Support (
|
NathanFlurry
changed the base branch from
10-04-fix_next_fix_local_dev
to
graphite-base/1353
NathanFlurry
force-pushed
the
graphite-base/1353
branch
from
03216a5 to
5359991
Compare
NathanFlurry
force-pushed
the
10-04-feat_core_support_running_engine
branch
from
6c3e109 to
4fcc430
Compare
NathanFlurry
changed the base branch from
graphite-base/1353
to
10-04-fix_example-deno_fix_check-types
|
Claude finished @NathanFlurry's task —— View job Reviewing PR: feat(core): support running engine Todo List:
Analysis SummaryThis PR adds support for running a Rivet engine process automatically within the RivetKit runtime. The implementation includes:
✅ Excellent Implementation AspectsRobust Process Management
Cross-Platform Support
Code Quality
🐛 Issues and RecommendationsHigh Priority1. Limited Platform Support (
|
|
Claude finished @NathanFlurry's task —— View job Reviewing PR: feat(core): support running engine Todo List:
Analysis SummaryThis PR adds support for running a Rivet engine process automatically within the RivetKit runtime. The implementation includes:
✅ Excellent Implementation AspectsRobust Process Management
Cross-Platform Support
Code Quality
🐛 Issues and RecommendationsHigh Priority1. Limited Platform Support (
|
Merge activity
|
No description provided.