[Graphite MQ] Draft PR GROUP:spec_1b1eef (PRs 3624, 3625)

[graphite-app]

This draft PR was created by the Graphite merge queue.
Trunk will be fast forwarded to the HEAD of this PR when CI passes, and the original PRs will be closed.

The following PRs are included in this draft PR:

• #3624
• #3625

[vercel]

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

4 Skipped Deployments

Project	Deployment	Review	Updated (UTC)
rivet-cloud	Ignored		Dec 25, 2025 3:10am
rivet-inspector	Ignored	Preview	Dec 25, 2025 3:10am
rivet-site	Ignored	Preview	Dec 25, 2025 3:10am
rivetkit-serverless	Skipped		Dec 25, 2025 3:10am

[claude]

Pull Request Review

This PR combines two separate PRs that add comprehensive test coverage for Rivet Actors functionality, specifically for alarms and KV operations. Overall, the code quality is excellent with thorough testing patterns, but there are a few areas that could be improved.

Positive Aspects

Excellent Test Coverage - Comprehensive test suites for actor alarms (1453 lines) and KV operations (3194 lines across 4 files). Tests cover edge cases, race conditions, and failure scenarios.

Well-Structured Test Infrastructure - Reusable actor behaviors in behaviors.rs promote DRY principles. Helper functions in test_helpers.rs reduce code duplication. Lifecycle event system provides precise test synchronization.

Clear Documentation - MARK comments organize code sections effectively. Detailed inline comments explain test scenarios. Actor behavior names are self-documenting.

Robust Error Handling - Proper use of anyhow::Result throughout. Structured error reporting with context.

Critical Issue: Import Style Violations

Multiple files use glob imports from anyhow, which violates CLAUDE.md conventions:

Affected files:

• engine/packages/engine/tests/actors_alarm.rs:1
• engine/packages/engine/tests/actors_kv_crud.rs:1
• engine/packages/engine/tests/actors_kv_drop.rs:1
• engine/packages/engine/tests/actors_kv_list.rs:1
• engine/packages/engine/tests/actors_kv_misc.rs:1
• engine/packages/engine/tests/common/test_runner/behaviors.rs:2
• engine/packages/engine/tests/common/test_runner/actor.rs:1

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

Recommendation: Replace use anyhow::*; with use anyhow::{anyhow, bail, Context, Result};

Other Recommendations

Timing-Sensitive Tests - The alarm_fires_at_correct_time test uses ±500ms tolerance which might be too strict for CI environments under load, potentially causing flaky tests. Consider configurable tolerances.

Deprecated Function - wait_for_actor_wake_polling is marked DEPRECATED but still used extensively. Either remove the deprecation or document when to use it.

Magic Numbers - Extract hardcoded values like 50ms polling intervals to named constants with documentation.

Logging Inconsistency - Some log messages do not follow lowercase convention per CLAUDE.md.

Security & Performance

No security concerns identified. Test isolation is proper. Performance considerations are appropriate for integration tests.

Test Coverage Assessment

Excellent coverage for alarm edge cases, KV CRUD/list/drop operations, actor lifecycle, and error conditions.

Potential gaps: alarm behavior across actor migrations, KV operations under high concurrency, KV storage limits.

Summary

This is high-quality test code that significantly improves reliability. The test infrastructure is well-designed and will provide excellent regression protection.

Conditional Approval - Please address the critical import style violations before merging.

The other recommendations can be addressed in follow-up PRs if preferred.