feat(product_workflow): expose get_run_state on RebornServicesApi facade#3735
Open
italic-jinxin wants to merge 2 commits into
Open
feat(product_workflow): expose get_run_state on RebornServicesApi facade#3735italic-jinxin wants to merge 2 commits into
italic-jinxin wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces the get_run_state endpoint to the RebornServices API, enabling the retrieval of run states through a stable DTO. The implementation includes mandatory ownership verification to prevent unauthorized cross-user access and provides comprehensive test coverage for validation, error mapping, and security. Review feedback highlighted that the security checks and the structure of the data transfer objects align with best practices for resource authorization and API stability.
serrrfirat
approved these changes
May 18, 2026
Collaborator
serrrfirat
left a comment
There was a problem hiding this comment.
Approved after focused review passes.
Reviewed from three angles:
- Security/correctness: no findings. The new get_run_state path validates request IDs, performs the thread ownership probe before coordinator access, maps missing run state through the stable facade error taxonomy, and does not expose backend/internal error details.
- Paranoid review: no Critical/High/Medium/Low/Nit findings across correctness, edge cases, DTO leakage, error handling, architecture boundary, and test coverage.
- Multi-tenancy correctness: no findings. The method derives TurnScope from the authenticated caller, verifies thread ownership with owner_user_id before calling TurnCoordinator, returns 404 for cross-user probes, and has caller-level regression coverage proving cross-user reads short-circuit before coordinator access.
Validation checked locally:
- cargo test -p ironclaw_product_workflow
- cargo clippy -p ironclaw_product_workflow --all-targets -- -D warnings
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds the fourth M2 facade method called out in issue #3627: a WebUI-facing
get_run_stateonRebornServicesApiso route handlers can read run statewithout importing
TurnCoordinator,HostRuntime, dispatcher, run-statestores, or DB stores. The other three methods named in the ticket
(
submit_turn,cancel_run,resolve_gate) already exist on the trait;this PR completes the set.
The new
RebornGetRunStateResponseis a stable DTO suitable for M1 and M5:it deliberately omits
TurnRunState's M3-internal fields — rawscope,source_binding_ref,reply_target_binding_ref, andresolved_model_route— and mirrors the wire shape of
RebornSubmitTurnResponseforturn_id/resolved_run_profile_id/resolved_run_profile_version.get_run_statereuses the existingassert_thread_owned_byownership probethat
cancel_runandresolve_gatealready enforce, so a caller sharing a(tenant, agent, project)scope cannot read another user's run by guessingrun_id. The cross-user case surfaces asNotFoundrather thanForbiddento avoid leaking the existence of another user's thread.
Method locations
submit_turnRebornServicesApi(existing)RebornServices::submit_turncancel_runRebornServicesApi(existing)RebornServices::cancel_runresolve_gateRebornServicesApi(existing)RebornServices::resolve_gateget_run_stateRebornServicesApi(new)RebornServices::get_run_state(new)All four live in
crates/ironclaw_product_workflow/src/reborn_services.rs.Acceptance criteria
whose surface depends only on
WebUiAuthenticatedCaller, request DTOs,and
RebornServicesError. The staticfacade_source_avoids_forbidden_runtime_dependenciesgrep test guards the import boundary.
TurnCoordinatorthrough the published M3 methodset (
submit_turn,resume_turn,cancel_run,get_run_state).reborn_services/types.rs— strongly typednewtypes for run IDs/cursors/refs,
Stringfor fields that alreadyappear stringified elsewhere in the facade (
turn_id,resolved_run_profile_id), and explicit omission of M3-internal fields.tests/reborn_services_contract.rsdrive thetrait directly with a fake
TurnCoordinatorand a real or stubbedSessionThreadService. Tests cover: stable-DTO redaction, validationfor blank/non-UUID inputs,
ScopeNotFound→NotFoundmapping, andthe cross-user ownership probe (which must short-circuit before
TurnCoordinatoris touched). The pre-existing facade-source greptest continues to lock the M2/M3 import boundary.
RebornServicesError/RebornServicesErrorCode(the M2 taxonomy). The validation testasserts that error JSON does not contain
TurnCoordinatororHostRuntimesubstrings.Change Type
Linked Issue
Closes #3627
Validation
cargo fmt --all -- --checkcargo clippy --all --benches --tests --examples --all-features -- -D warningscargo buildcargo test --features integrationif database-backed or integration behavior changedreview-prorpr-shepherd --fixwas run before requesting reviewSecurity Impact
Database Impact
Blast Radius
Rollback Plan
Review Follow-Through
Review track: