Skip to content

Conversation

@MasterPtato
Copy link
Contributor

No description provided.

Copy link
Contributor Author

MasterPtato commented Jan 12, 2026

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
Copy link

claude bot commented Jan 12, 2026

Code Review - PR #3846: chore: clean up errors

Summary

This PR cleans up error definitions by:

  1. Removing unused guard.actor_destroyed and guard.actor_not_found error artifacts
  2. Moving test-only errors (api_rate_limited, namespace_invalid_name) from production groups to the test group
  3. Consolidating Datacenter::NotFound error from api-public to api-util (making it shared)
  4. Improving error handling in guard-core tests by using anyhow::anyhow!() instead of bail!

✅ Positive Findings

Good Consolidation: Moving Datacenter::NotFound from api-public/src/errors.rs to api-util/src/errors.rs and making the errors module public is a good consolidation that reduces duplication.

Proper Test Organization: Moving test-specific errors to the test group improves organization and makes it clear these are not production errors.

Cleaner Error Handling: The change in streaming_response.rs from using bail! to explicit early return with anyhow::anyhow!() is more explicit and follows best practices.

⚠️ Issues Found

1. Dead Code in proxy_service.rs (Medium Priority)

Location: engine/packages/guard-core/src/proxy_service.rs:2530-2531

The error mappings for guard.actor_destroyed and guard.actor_not_found are still present in the code, but the error artifacts have been deleted:

("guard", "actor_not_found") => StatusCode::NOT_FOUND,
("guard", "actor_destroyed") => StatusCode::NOT_FOUND,

Issue: These mappings are now dead code since the errors no longer exist. If these errors were genuinely unused, these mappings should be removed. If they are still needed, the error artifacts should not have been deleted.

Recommendation: Either:

  • Remove these two lines if the errors are truly unused, OR
  • Restore the error artifacts if they are still being generated somewhere in the codebase

2. Potential Missing Import in streaming_response.rs (Low Priority)

Location: engine/packages/guard-core/tests/streaming_response.rs:256

The PR removes use anyhow::bail; but adds usage of anyhow::anyhow!(). While this works, it follows the pattern of importing the macro path rather than importing from the prelude.

Current Code:

Err(anyhow::anyhow!("bad path"))

Per CLAUDE.md Guidelines:

Do not glob import (::*) from anyhow. Instead, import individual types and traits

Recommendation: The current approach is acceptable and follows the guideline. However, for consistency with the return type (which likely comes from anyhow), consider adding:

use anyhow::anyhow;

Then use:

Err(anyhow!("bad path"))

This is a minor style point and not a blocking issue.

📋 Test Coverage

The PR updates tests in engine/packages/error/tests/basic.rs to use the new test group naming, which is correct. The error artifact files have been properly renamed to match the new structure.

Recommendation: Ensure CI passes, particularly the error tests.

🔒 Security Considerations

No security concerns identified.

⚡ Performance Considerations

No performance impact expected.

📝 Code Style & Best Practices

Follows CLAUDE.md Guidelines:

  • ✅ Uses workspace dependencies appropriately
  • ✅ Follows error handling patterns
  • ✅ Avoids glob imports from anyhow
  • ⚠️ Potential dead code remains (see Issue [SVC-2555] Set up issue templates #1)

🎯 Recommendations

  1. Must Fix: Investigate the guard.actor_destroyed and guard.actor_not_found mappings in proxy_service.rs. Remove them if unused, or restore the errors if needed.

  2. Nice to Have: Consider adding an explicit use anyhow::anyhow; import for consistency.

  3. Verification: Ensure all tests pass, especially:

    • cargo test -p rivet-error
    • cargo test -p rivet-guard-core
    • Any integration tests that might generate these errors

Overall Assessment

The PR is mostly good with one medium-priority issue that should be addressed. The consolidation of errors and cleanup is valuable, but the dead code mappings suggest the cleanup may be incomplete.

Suggested Action: Address the dead code in proxy_service.rs before merging.


Review updated: 2026-01-14

@pkg-pr-new
Copy link

pkg-pr-new bot commented Jan 12, 2026

More templates

@rivetkit/cloudflare-workers

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/virtual-websocket

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/virtual-websocket@3846

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: a4f42a6

@MasterPtato MasterPtato force-pushed the 01-12-chore_clean_up_errors branch from 66f6892 to 485634a Compare January 13, 2026 00:27
@MasterPtato MasterPtato force-pushed the 01-12-fix_pb_have_restart_policy_actors_sleep_after_pending_for_too_long branch 2 times, most recently from 363b7a5 to 3f06e1b Compare January 13, 2026 00:44
@MasterPtato MasterPtato force-pushed the 01-12-chore_clean_up_errors branch from 485634a to 964ae0f Compare January 13, 2026 00:44
@MasterPtato MasterPtato force-pushed the 01-12-fix_pb_have_restart_policy_actors_sleep_after_pending_for_too_long branch from 3f06e1b to 8cd75d7 Compare January 13, 2026 01:01
@MasterPtato MasterPtato force-pushed the 01-12-chore_clean_up_errors branch from 964ae0f to f3c17ef Compare January 13, 2026 01:02
@MasterPtato MasterPtato force-pushed the 01-12-fix_pb_have_restart_policy_actors_sleep_after_pending_for_too_long branch from 8cd75d7 to 75ce8e5 Compare January 13, 2026 01:29
@MasterPtato MasterPtato force-pushed the 01-12-chore_clean_up_errors branch from f3c17ef to b5999a3 Compare January 13, 2026 01:29
@MasterPtato MasterPtato force-pushed the 01-12-fix_pb_have_restart_policy_actors_sleep_after_pending_for_too_long branch from 75ce8e5 to 50ba096 Compare January 14, 2026 02:05
@MasterPtato MasterPtato force-pushed the 01-12-chore_clean_up_errors branch 2 times, most recently from 27fc351 to 77119ca Compare January 14, 2026 02:53
@MasterPtato MasterPtato force-pushed the 01-12-fix_pb_have_restart_policy_actors_sleep_after_pending_for_too_long branch from 50ba096 to b9a9c50 Compare January 14, 2026 03:06
@MasterPtato MasterPtato force-pushed the 01-12-chore_clean_up_errors branch from 77119ca to 1fe301c Compare January 14, 2026 03:06
@MasterPtato MasterPtato force-pushed the 01-12-fix_pb_have_restart_policy_actors_sleep_after_pending_for_too_long branch from b9a9c50 to 9a4ab5a Compare January 14, 2026 19:47
@MasterPtato MasterPtato force-pushed the 01-12-chore_clean_up_errors branch 2 times, most recently from 830c32d to dfbb13a Compare January 14, 2026 20:01
@MasterPtato MasterPtato force-pushed the 01-12-fix_pb_have_restart_policy_actors_sleep_after_pending_for_too_long branch from 9a4ab5a to 33e8961 Compare January 14, 2026 20:01
@MasterPtato MasterPtato force-pushed the 01-12-fix_pb_have_restart_policy_actors_sleep_after_pending_for_too_long branch from 33e8961 to 633544a Compare January 14, 2026 22:52
@MasterPtato MasterPtato force-pushed the 01-12-chore_clean_up_errors branch from dfbb13a to 81663b7 Compare January 14, 2026 22:52
@MasterPtato MasterPtato force-pushed the 01-12-fix_pb_have_restart_policy_actors_sleep_after_pending_for_too_long branch from 633544a to e073155 Compare January 14, 2026 23:02
@MasterPtato MasterPtato force-pushed the 01-12-chore_clean_up_errors branch from 81663b7 to 260900c Compare January 14, 2026 23:02
@MasterPtato MasterPtato force-pushed the 01-12-chore_clean_up_errors branch from 260900c to a4f42a6 Compare January 14, 2026 23:07
@MasterPtato MasterPtato force-pushed the 01-12-fix_pb_have_restart_policy_actors_sleep_after_pending_for_too_long branch from e073155 to e96badf Compare January 14, 2026 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants