chore(engine): move smoke test to engine/tests/ #3337

NathanFlurry · 2025-11-04T00:37:51Z

No description provided.

vercel · 2025-11-04T00:37:56Z

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

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Ready	Preview	Comment	Nov 4, 2025 7:00pm

3 Skipped Deployments

Project	Deployment	Preview	Updated (UTC)
rivet-cloud	Ignored		Nov 4, 2025 7:00pm
rivet-inspector	Ignored	Preview	Nov 4, 2025 7:00pm
rivet-site	Ignored	Preview	Nov 4, 2025 7:00pm

NathanFlurry · 2025-11-04T00:38:18Z

Warning

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

How to use the Graphite Merge Queue

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

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

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

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

claude · 2025-11-04T00:40:33Z

PR Review: Move smoke test to engine/tests/

Summary

This PR relocates the smoke test from examples/smoke-test/ to engine/tests/smoke/, consolidating test infrastructure. Overall, the changes are straightforward and reasonable.

Code Quality & Best Practices

Positive Observations

✅ Consistent renaming across all files (package.json, pnpm-workspace.yaml)
✅ Proper workspace dependency handling in pnpm-lock.yaml
✅ File structure preserved during migration

Issues Identified

1. Debug Code Left in Production (High Priority)

Location: engine/tests/smoke/src/smoke-test/spawn-actor.ts:93-96

// Wait for actor to sleep
// const sleepTime = 1_100 + Math.random() * 800;
const sleepTime = Math.random() * 2500;
console.log("sleeping", sleepTime);
// const sleepTime = 1000;
await new Promise((res) => setTimeout(res, sleepTime));

Issues:

Commented-out code with multiple sleep time variants suggests experimentation
console.log("sleeping", sleepTime) is debug logging
Unclear which sleep time configuration is correct

Recommendation:

Remove commented code
Replace console.log with structured logging using tracing (per CLAUDE.md guidelines):
```
tracing::debug!(?sleep_time, "waiting for actor to sleep");
```
Document why the random sleep time is needed (appears to be testing actor sleep/wake cycles)

2. Dead Code (Medium Priority)

Location: engine/tests/smoke/src/smoke-test/spawn-actor.ts:107-108

if (succeeded) {
}

Issue: Empty conditional block serves no purpose

Recommendation: Remove this dead code entirely

3. Function Order Inconsistency (Low Priority)

Location: engine/tests/smoke/src/smoke-test/spawn-actor.ts:36-60, 62-109

The diff shows spawnActorHttp and spawnActorSleepCycle were swapped in order. While this doesn't affect functionality, it's inconsistent with the switch statement order in spawnActor() (lines 24-33) which calls them in the order: sleep-cycle, then http.

Recommendation: Keep function definitions in the same order as they appear in the switch statement for readability.

4. Logging Standards Violation (Low Priority)

Per CLAUDE.md:

Use tracing for logging. Do not format parameters into the main message, instead use tracing's structured logging.

The console.log("sleeping", sleepTime) should use the tracing crate with structured fields.

Performance Considerations

⚠️ Blocking Sleep in Test Loop
The spawnActorSleepCycle function runs 20 iterations with random sleep times (0-2500ms). This means each actor spawn could take up to 50 seconds (20 * 2.5s). While acceptable for smoke tests, ensure this doesn't cause test timeouts in CI/CD.

Security Concerns

✅ No security issues identified. The changes are purely organizational.

Test Coverage

Missing Context

The PR doesn't show test execution results or validation that the moved tests still pass
No information about whether CI/CD configurations were updated to point to the new test location

Recommendations

Verify all smoke tests pass in the new location
Update any CI/CD configs that reference examples/smoke-test/
Ensure Docker dev environment configs don't need updates (per CLAUDE.md, template-based configs should be regenerated if needed)

Additional Notes

Documentation Updates

The README.md was simplified, removing references to RivetKit branding and links. This makes sense for an internal test, but ensure:

Any external documentation linking to the example is updated
The example-as-documentation value isn't lost if this was serving that purpose

Actor Lifecycle Testing

The addition of onStart and onStop handlers (each with 1-second delays) in registry.ts:10-15 appears intentional for testing actor lifecycle management. This is good test coverage but should be documented in comments explaining the test purpose.

Summary of Required Changes

Must Fix:

Remove or properly implement the debug code at spawn-actor.ts:93-96
Remove dead code at spawn-actor.ts:107-108
Replace console.log with structured tracing

Should Consider:
4. Verify tests pass in new location
5. Update any CI/CD references to old path
6. Document the purpose of lifecycle delays and sleep randomization

Overall, this is a clean refactor with a few housekeeping items to address. The main concern is ensuring debug/experimental code is cleaned up before merge.

claude · 2025-11-04T19:03:22Z

Pull Request Review

This PR moves the smoke test from examples/smoke-test to engine/tests/smoke and makes some modifications to the test behavior. Overall, this is a good organizational change, but there are several issues that need to be addressed.

Code Quality Issues

1. Commented-out debug code in spawn-actor.ts (engine/tests/smoke/src/smoke-test/spawn-actor.ts:93-96)

There are multiple commented-out lines and debug console.log statements that should be cleaned up:

// Wait for actor to sleep
// const sleepTime = 1_100 + Math.random() * 800;
const sleepTime = Math.random() * 2500;
console.log("sleeping", sleepTime);
// const sleepTime = 1000;

Recommendation:

Remove commented-out code or add a proper explanation if they're needed for debugging
Replace console.log with structured logging using tracing as per CLAUDE.md guidelines
If this is for debugging, consider making it configurable via environment variable

2. Dead code in spawn-actor.ts (engine/tests/smoke/src/smoke-test/spawn-actor.ts:107-108)

Empty conditional block that serves no purpose:

if (succeeded) {
}

Recommendation: Remove this dead code.

3. Inconsistent function ordering (engine/tests/smoke/src/smoke-test/spawn-actor.ts:36-109)

The functions spawnActorHttp and spawnActorSleepCycle appear to have been swapped compared to the diff, which may cause confusion. The names don't match their implementations anymore.

Issue in the diff:

Lines 36-60 show spawnActorHttp but it appears to be the simpler test
Lines 62-109 show spawnActorSleepCycle but it has the loop logic

Recommendation: Verify function names match their implementations.

Potential Bugs

4. Missing variable usage in spawnActorHttp (engine/tests/smoke/src/smoke-test/spawn-actor.ts:36-60)

The original spawnActorSleepCycle function had:

succeeded = true;
onSuccess();

But the new spawnActorHttp function just calls:

onSuccess();

The succeeded variable was removed but it might have been used for tracking. Please verify this is intentional.

5. Error handling inconsistency

The spawnActorSleepCycle function declares a succeeded variable but spawnActorHttp does not. This inconsistency could lead to maintenance issues.

Recommendation: Standardize error handling patterns across both test functions.

Testing & Documentation

6. No automated tests

This is a test suite but there are no automated tests (no .test.ts or .spec.ts files). The smoke tests appear to be manual.

Recommendation: Consider adding automated tests that can run in CI/CD to validate the smoke test behavior.

7. Documentation needs updating (engine/tests/smoke/README.md)

The README still references the old path:

cd rivetkit/examples/smoke-test

Recommendation: Update the README with:

Correct paths reflecting the new location
Better documentation on what BEHAVIOR values are supported
Expected output and success criteria
How to run the tests in the context of the monorepo

Performance Considerations

8. Random sleep times may cause flaky tests (engine/tests/smoke/src/smoke-test/spawn-actor.ts:94)

const sleepTime = Math.random() * 2500;

Using random sleep times can make test results non-deterministic and harder to debug.

Recommendation:

Consider using fixed sleep times or configurable values
Document why randomization is needed (if testing race conditions, etc.)
Add comments explaining the timing strategy

9. Added 1-second delays to lifecycle hooks (engine/tests/smoke/src/server/registry.ts:10-15)

onStart: async () => {
    await new Promise((resolve) => setTimeout(resolve, 1000));
},
onStop: async () => {
    await new Promise((resolve) => setTimeout(resolve, 1000));
},

These artificial delays will slow down the tests significantly.

Recommendation:

Document why these delays are necessary (testing timeout behavior?)
Consider making them configurable via environment variables
If testing specific timing scenarios, add comments explaining the intent

Security & Best Practices

10. Package naming consistency

Good change renaming packages to follow the test-* convention:

✅ @rivet/load-tests → test-load
✅ example-smoke-test → test-smoke

This clearly identifies them as test packages.

11. Workspace integration

Good addition to pnpm-workspace.yaml:

- engine/tests/*

This properly integrates the test packages into the monorepo workspace.

Summary

Positive aspects:

Good organizational move from examples/ to engine/tests/
Consistent package naming
Proper workspace integration

Issues to address:

Remove debug code and console.log statements
Clean up dead code (empty if block)
Verify function names match implementations
Document the purpose of added delays
Update README documentation
Consider adding automated tests
Standardize error handling patterns

Priority fixes:

🔴 HIGH: Remove dead code and debug statements (spawn-actor.ts:93-96, 107-108)
🟡 MEDIUM: Update documentation (README.md)
🟡 MEDIUM: Document/configure timing strategies (registry.ts:10-15, spawn-actor.ts:94)
🟢 LOW: Add automated tests for the smoke test suite

Please address these issues before merging. Happy to review again once updated!

vercel · 2025-11-04T19:24:56Z

Deployment failed with the following error:

Too many requests - try again in 4 minutes (more than 120, code: "api-deployments-flood-pro").

Learn More: https://vercel.com/docs/limits#rate-limits

claude · 2025-11-04T19:30:17Z

PR Review: Move smoke test to engine/tests/

Summary

This PR moves the smoke test from examples/smoke-test/ to engine/tests/smoke/, consolidating test infrastructure. Overall this is a good organizational change, but there are several issues that need to be addressed.

🐛 Issues Found

1. Commented-out debug code (engine/tests/smoke/src/smoke-test/spawn-actor.ts:93-96)

// Wait for actor to sleep
// const sleepTime = 1_100 + Math.random() * 800;
const sleepTime = Math.random() * 2500;
console.log("sleeping", sleepTime);
// const sleepTime = 1000;

Issue: Multiple commented-out variations of sleepTime and a console.log statement suggest this code is still being debugged. This should be cleaned up before merging.

Recommendation:

Remove commented-out code
Either remove the console.log or convert it to proper tracing: tracing::debug!(?sleepTime, "waiting for actor to sleep")
Document why the random sleep time range was chosen (0-2500ms)

2. Dead code (engine/tests/smoke/src/smoke-test/spawn-actor.ts:107-108)

if (succeeded) {
}

Issue: Empty conditional block serves no purpose.

Recommendation: Remove this dead code.

3. Function names swapped in diff (engine/tests/smoke/src/smoke-test/spawn-actor.ts:36-62)

The diff shows spawnActorHttp and spawnActorSleepCycle were swapped in order. While not technically broken, this makes the diff harder to review. Consider if this reordering was intentional.

4. Inconsistent error handling

spawnActorHttp catches errors but doesn't track succeeded flag
spawnActorSleepCycle tracks succeeded but it's only used in the empty if block

Recommendation: Standardize error handling between both functions.

5. Outdated README (engine/tests/smoke/README.md:13)

cd rivetkit/examples/smoke-test

Issue: Path is now outdated after the move.

Recommendation: Update to cd rivet/engine/tests/smoke or make it relative to the repository root.

📝 Code Quality Observations

Positive:

Good separation of concerns with different test behaviors (http vs sleep-cycle)
Addition of onStart and onStop lifecycle hooks for testing
Package naming follows conventions (test-smoke, test-load)

Suggestions:

Connection method selection (spawn-actor.ts:76): The random 50/50 split between HTTP and WebSocket is interesting for testing, but should be documented. Consider making this configurable via environment variable.
Magic numbers:
- sleepTimeout: 500 (registry.ts:5)
- setTimeout(resolve, 1000) (registry.ts:11, 14)
- Loop count 20 (spawn-actor.ts:74)
- These should be constants with descriptive names or configurable via environment variables.
Type safety: Consider using an enum for BEHAVIOR instead of string literals to catch typos at compile time.

✅ Security & Performance

No security concerns identified. Performance considerations:

Random sleep times (0-2500ms) could make test results inconsistent
Consider adding metrics collection to track actual performance characteristics

🧪 Testing

The PR appears to be moving existing test infrastructure, so existing test coverage should be preserved. However:

Verify that CI/CD pipelines reference the new path
Update any documentation that references the old path
Consider adding a test that validates both http and sleep-cycle behaviors work correctly

Recommendation: Request Changes

The organizational change is good, but the code quality issues (commented code, dead code, console.log, outdated README) should be addressed before merging.

claude · 2025-11-04T19:34:47Z

PR Review: Move smoke test to engine/tests/

Summary

This PR relocates the smoke test from examples/smoke-test to engine/tests/smoke, which is a sensible reorganization. However, there are several issues that need attention before merging.

Critical Issues

1. Dead Code in `spawn-actor.ts:107-108`

if (succeeded) {
}

Empty if block at the end of spawnActorSleepCycle function. This should be removed entirely as it serves no purpose.

Location: engine/tests/smoke/src/smoke-test/spawn-actor.ts:107-108

2. Debug Console Statements

console.log("sleeping", sleepTime);

The code contains a debug console.log statement at line 95 that should be removed or replaced with proper structured logging using tracing.

Location: engine/tests/smoke/src/smoke-test/spawn-actor.ts:95

3. Commented-Out Code

// const sleepTime = 1_100 + Math.random() * 800;
const sleepTime = Math.random() * 2500;
// const sleepTime = 1000;

Multiple commented-out sleep time calculations suggest this is still experimental. The code should either:

Use a consistent sleep time calculation
Make this configurable via environment variable
Remove the commented code and document why the current formula is chosen

Location: engine/tests/smoke/src/smoke-test/spawn-actor.ts:93-96

4. Function Name/Behavior Mismatch

The functions spawnActorHttp and spawnActorSleepCycle appear to have been swapped based on the diff:

spawnActorHttp (lines 36-60) performs a simple single HTTP request
spawnActorSleepCycle (lines 62-109) performs multiple iterations with sleep cycles

However, looking at the implementation:

Line 76: const connMethod = Math.random() > 0.5 ? "http" : "websocket";

This suggests the naming is now correct (the diff showed them as swapped before), but this should be verified to ensure the BEHAVIOR switch in line 24-33 correctly routes to the intended test type.

Code Quality Issues

5. Inconsistent Error Handling

In spawnActorHttp, exceptions are caught and added to the errors array, but the error type is unknown. Consider:

Using more specific error types
Logging errors for debugging purposes (using structured logging as per CLAUDE.md)

6. README Outdated After Move

The README still references the old path:

cd rivetkit/examples/smoke-test

Should be updated to reflect the new location in engine/tests/smoke.

Location: engine/tests/smoke/README.md:13

7. Package Naming Consistency

The PR renames packages:

example-smoke-test → test-smoke
@rivet/load-tests → test-load

This is good for consistency, but ensure this doesn't break any internal references or CI/CD pipelines that might reference the old names.

Minor Issues

8. Missing Variable Usage

The succeeded variable is set but only checked in the empty if block. Either use it properly or remove it entirely.

9. Actor Lifecycle Hooks

The addition of onStart and onStop hooks in registry.ts (lines 10-15) each have 1-second delays. This is likely for testing actor lifecycle, but:

Should be documented why these delays exist
Consider making the delay configurable for testing different scenarios

Security & Performance

No major concerns, but:

The random sleep calculation (Math.random() * 2500) could lead to unpredictable test durations
Consider adding upper bounds or making test parameters configurable via environment variables

Test Coverage

No test files modified or added - this is just a move operation, which is acceptable. However, consider:

Adding integration tests that verify the smoke test itself works correctly
Documenting expected behaviors and exit codes

Recommendations

Before merging:

✅ Remove dead code (lines 107-108 in spawn-actor.ts)
✅ Remove or replace console.log with structured logging
✅ Clean up commented-out code and document the sleep time calculation
✅ Update README with correct path
⚠️ Verify the BEHAVIOR switch correctly routes to intended test functions
💡 Consider making test parameters (sleep times, iteration counts) configurable

Nice to have:

Add structured logging using tracing as per CLAUDE.md guidelines
Document the purpose of the 1-second delays in onStart/onStop hooks
Make sleep time calculations configurable via environment variables

Conclusion

This is a good organizational change, but the code contains several issues that suggest it may have been committed mid-development. Please address the critical issues before merging.

Recommendation: ⚠️ Request Changes

graphite-app · 2025-11-04T20:10:03Z

Merge activity

Nov 4, 8:10 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
Nov 4, 8:11 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:53 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
Nov 4, 8:53 PM UTC: CI is running for this pull request on a draft pull request (#3353) due to your merge queue CI optimization settings.
Nov 4, 8:56 PM UTC: Merged by the Graphite merge queue via draft PR: #3353.

vercel · 2025-11-04T20:25:30Z

Deployment failed with the following error:

Too many requests - try again in 5 minutes (more than 120, code: "api-deployments-flood-pro").

Learn More: https://vercel.com/docs/limits#rate-limits

claude · 2025-11-04T20:44:18Z

Pull Request Review

Summary

This PR moves the smoke test from examples/smoke-test to engine/tests/smoke, along with some modifications to the test behavior. The move reorganizes the codebase to better reflect that this is a test suite rather than an example.

Code Quality & Best Practices

Positives:

Good organizational change moving tests to the appropriate location
Workspace configuration updated correctly in pnpm-workspace.yaml
Package naming follows conventions (test-smoke, test-load)

Issues Found:

Debugging Code Left In (engine/tests/smoke/src/smoke-test/spawn-actor.ts:93-96)
- Commented-out code and console.log statements should be removed before merging:
```
// const sleepTime = 1_100 + Math.random() * 800;
const sleepTime = Math.random() * 2500;
console.log("sleeping", sleepTime);
// const sleepTime = 1000;
```
- Recommendation: Either remove the commented code or add a proper explanation for why multiple sleep time options exist. The console.log should use structured logging with tracing per CLAUDE.md guidelines.
Dead Code (engine/tests/smoke/src/smoke-test/spawn-actor.ts:107-108)
```
if (succeeded) {
}
```
- Empty conditional block serves no purpose and should be removed.
Logging Standards Violation (engine/tests/smoke/src/smoke-test/spawn-actor.ts:95)
- Per CLAUDE.md: "Use tracing for logging. Do not format parameters into the main message"
- Current: console.log("sleeping", sleepTime);
- Should be: tracing::info!(?sleep_time, "sleeping"); (or TypeScript equivalent with structured logging)
Function Order Confusion (engine/tests/smoke/src/smoke-test/spawn-actor.ts:36-60)
- The spawnActorHttp function appears before spawnActorSleepCycle but was previously named spawnActorSleepCycle (based on git diff)
- This suggests the functions may have been swapped, which could be confusing. Verify the implementation matches the intended behavior.

Potential Bugs

Missing succeeded Assignment (engine/tests/smoke/src/smoke-test/spawn-actor.ts:45-59)
- The spawnActorHttp function doesn't set succeeded = true but the previous version did
- This appears to be intentional but should be verified - is success tracking needed for this test mode?
Inconsistent Error Handling
- Both functions catch errors and call onFailure(), but only spawnActorSleepCycle tracks the succeeded variable
- Consider whether spawnActorHttp needs similar success tracking

Performance Considerations

Sleep Time Randomization
- Random sleep time of 0-2500ms seems arbitrary
- Consider documenting why this range was chosen and what behavior it's testing
- The commented-out versions suggest uncertainty about the right value
Actor Lifecycle Delays (engine/tests/smoke/src/server/registry.ts:10-15)
- Added 1000ms delays in onStart and onStop hooks
- These will add 2 seconds per actor lifecycle - ensure this is intentional for testing purposes
- Consider making this configurable via environment variables for different test scenarios

Security Concerns

No security issues identified. This is test code with no sensitive operations.

Test Coverage

No new tests added, this is a refactor/move operation
The smoke test itself serves as an integration test
Consider documenting what scenarios this test is meant to cover in the README.md

Additional Recommendations

Documentation - The README.md was simplified, removing useful context about RivetKit. Consider retaining some documentation about what this test validates.
Configuration - Document the BEHAVIOR environment variable options in the README or add a comment explaining valid values.
Package Consistency - The test-load package was renamed from @rivet/load-tests - ensure this naming convention is consistent across all test packages.

Verdict

Request Changes - The debugging code and dead code should be cleaned up before merging. The other issues are minor but should be addressed for code quality.

Checklist

Remove commented-out code and console.log debugging statements
Remove empty if (succeeded) {} block
Consider using structured logging if console.log is needed
Verify function implementations match their names after the swap
Document the purpose of the 1000ms delays in actor lifecycle hooks
Consider documenting what the smoke test validates

NathanFlurry requested a review from MasterPtato November 4, 2025 00:37

This was referenced Nov 4, 2025

fix(rivetkit): fix not pino bindings #3334

Closed

chore(rivetkit): add debug logging around sleep condition #3336

Closed

fix(rivetkit): fix conns not being cleaned up if actor.createConn errors #3335

Closed

vercel bot deployed to Preview – rivetkit-serverless November 4, 2025 00:38 View deployment

MasterPtato approved these changes Nov 4, 2025

View reviewed changes

This was referenced Nov 4, 2025

fix(guard): include method in cache key #3338

Closed

chore(guard): simplify gateway routing #3339

Closed

chore(guard): add support for routing runner ws to /runners/connect #3340

Closed

chore(guard): handle cors on the gateway #3341

Closed

NathanFlurry force-pushed the 11-04-chore_engine_move_smoke_test_to_engine_tests_ branch from 3689716 to dce28f8 Compare November 4, 2025 18:59

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

NathanFlurry force-pushed the 11-03-chore_rivetkit_add_debug_logging_around_sleep_condition branch from 62a6fbf to 43de939 Compare November 4, 2025 19:24

NathanFlurry force-pushed the 11-04-chore_engine_move_smoke_test_to_engine_tests_ branch from dce28f8 to 30bd99b Compare November 4, 2025 19:24

This was referenced Nov 4, 2025

chore(rivetkit): debounce ws message ack #3345

Closed

chore(rivetkit): auto-update auto-start engine version #3344

Closed

ci: trigger rust ci for generated bare artifacts #3346

Closed

graphite-app bot pushed a commit that referenced this pull request Nov 4, 2025

chore(engine): move smoke test to engine/tests/ (#3337)

bfb0fd1

chore(engine): move smoke test to engine/tests/

20dd048

NathanFlurry force-pushed the 11-03-chore_rivetkit_add_debug_logging_around_sleep_condition branch from 43de939 to 8afd075 Compare November 4, 2025 20:25

NathanFlurry force-pushed the 11-04-chore_engine_move_smoke_test_to_engine_tests_ branch from 30bd99b to 20dd048 Compare November 4, 2025 20:25

graphite-app bot pushed a commit that referenced this pull request Nov 4, 2025

chore(engine): move smoke test to engine/tests/ (#3337)

4d5b5c7

graphite-app bot closed this Nov 4, 2025

graphite-app bot deleted the 11-04-chore_engine_move_smoke_test_to_engine_tests_ branch November 4, 2025 20:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

chore(engine): move smoke test to engine/tests/ #3337

chore(engine): move smoke test to engine/tests/ #3337

Uh oh!

NathanFlurry commented Nov 4, 2025

Uh oh!

vercel bot commented Nov 4, 2025 •

edited

Loading

Uh oh!

NathanFlurry commented Nov 4, 2025 •

edited

Loading

Uh oh!

claude bot commented Nov 4, 2025

Uh oh!

claude bot commented Nov 4, 2025

Uh oh!

vercel bot commented Nov 4, 2025

Uh oh!

claude bot commented Nov 4, 2025

Uh oh!

claude bot commented Nov 4, 2025

Uh oh!

graphite-app bot commented Nov 4, 2025 •

edited

Loading

Uh oh!

vercel bot commented Nov 4, 2025

Uh oh!

claude bot commented Nov 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chore(engine): move smoke test to engine/tests/ #3337

chore(engine): move smoke test to engine/tests/ #3337

Uh oh!

Conversation

NathanFlurry commented Nov 4, 2025

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

How to use the Graphite Merge Queue

Uh oh!

claude bot commented Nov 4, 2025

PR Review: Move smoke test to engine/tests/

Summary

Code Quality & Best Practices

Positive Observations

Issues Identified

1. Debug Code Left in Production (High Priority)

2. Dead Code (Medium Priority)

3. Function Order Inconsistency (Low Priority)

4. Logging Standards Violation (Low Priority)

Performance Considerations

Security Concerns

Test Coverage

Missing Context

Recommendations

Additional Notes

Documentation Updates

Actor Lifecycle Testing

Summary of Required Changes

Uh oh!

claude bot commented Nov 4, 2025

Pull Request Review

Code Quality Issues

1. Commented-out debug code in spawn-actor.ts (engine/tests/smoke/src/smoke-test/spawn-actor.ts:93-96)

2. Dead code in spawn-actor.ts (engine/tests/smoke/src/smoke-test/spawn-actor.ts:107-108)

3. Inconsistent function ordering (engine/tests/smoke/src/smoke-test/spawn-actor.ts:36-109)

Potential Bugs

4. Missing variable usage in spawnActorHttp (engine/tests/smoke/src/smoke-test/spawn-actor.ts:36-60)

5. Error handling inconsistency

Testing & Documentation

6. No automated tests

7. Documentation needs updating (engine/tests/smoke/README.md)

Performance Considerations

8. Random sleep times may cause flaky tests (engine/tests/smoke/src/smoke-test/spawn-actor.ts:94)

9. Added 1-second delays to lifecycle hooks (engine/tests/smoke/src/server/registry.ts:10-15)

Security & Best Practices

10. Package naming consistency

11. Workspace integration

Summary

Uh oh!

vercel bot commented Nov 4, 2025

Uh oh!

claude bot commented Nov 4, 2025

PR Review: Move smoke test to engine/tests/

Summary

🐛 Issues Found

1. Commented-out debug code (engine/tests/smoke/src/smoke-test/spawn-actor.ts:93-96)

2. Dead code (engine/tests/smoke/src/smoke-test/spawn-actor.ts:107-108)

3. Function names swapped in diff (engine/tests/smoke/src/smoke-test/spawn-actor.ts:36-62)

4. Inconsistent error handling

5. Outdated README (engine/tests/smoke/README.md:13)

📝 Code Quality Observations

Positive:

Suggestions:

✅ Security & Performance

🧪 Testing

Recommendation: Request Changes

Uh oh!

claude bot commented Nov 4, 2025

PR Review: Move smoke test to engine/tests/

Summary

Critical Issues

1. Dead Code in spawn-actor.ts:107-108

2. Debug Console Statements

3. Commented-Out Code

4. Function Name/Behavior Mismatch

Code Quality Issues

vercel bot commented Nov 4, 2025 •

edited

Loading

NathanFlurry commented Nov 4, 2025 •

edited

Loading

1. Dead Code in `spawn-actor.ts:107-108`

graphite-app bot commented Nov 4, 2025 •

edited

Loading