feat(reborn): add host runtime contract facade#3095
Conversation
github-actions
Bot
added
scope: dependencies
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the ironclaw_host_runtime crate, establishing a contract-first facade for the IronClaw Reborn host runtime. It defines essential data structures for capability invocations, suspension states (approvals, auth, resources), and work management, alongside a HostRuntime trait and a FakeHostRuntime testkit. Feedback focuses on improving code maintainability and test utility: specifically, extracting shared validation logic for IdempotencyKey and CapabilitySurfaceVersion to reduce duplication and optimize performance, enhancing the FakeHostRuntime to support scripting full cancellation outcomes, and using more idiomatic import patterns in test files.
| pub fn new(value: impl Into<String>) -> Result<Self, HostRuntimeError> { | ||
| let value = value.into(); | ||
| if value.is_empty() { | ||
| return Err(HostRuntimeError::invalid_request( | ||
| "idempotency key must not be empty", | ||
| )); | ||
| } | ||
| if value.len() > 256 { | ||
| return Err(HostRuntimeError::invalid_request( | ||
| "idempotency key must be at most 256 bytes", | ||
| )); | ||
| } | ||
| if value.chars().any(|c| c == '\0' || c.is_control()) { | ||
| return Err(HostRuntimeError::invalid_request( | ||
| "idempotency key must not contain NUL/control characters", | ||
| )); | ||
| } | ||
| Ok(Self(value)) | ||
| } |
There was a problem hiding this comment.
The validation logic in IdempotencyKey::new is very similar to the logic in CapabilitySurfaceVersion::new (lines 119-137). Consider extracting this logic into a shared private helper function. Additionally, ensure the helper performs cheaper validation checks (like length, emptiness, and character set) on a trimmed slice before performing more expensive operations like replace that allocate a new string to avoid unnecessary allocations.
References
- When canonicalizing a string, perform cheaper validation checks (like length, emptiness, and character set) on a trimmed slice before performing more expensive operations like
replacethat allocate a new string.
| async fn cancel_work( | ||
| &self, | ||
| request: CancelRuntimeWorkRequest, | ||
| ) -> Result<CancelRuntimeWorkOutcome, HostRuntimeError> { | ||
| let mut state = self.state.lock().expect("fake mutex poisoned"); | ||
| state.cancellations.push(request); | ||
| let cancelled = state.cancelled_work.pop_front().unwrap_or_default(); | ||
| Ok(CancelRuntimeWorkOutcome { | ||
| cancelled, | ||
| already_terminal: Vec::new(), | ||
| unsupported: Vec::new(), | ||
| }) | ||
| } |
There was a problem hiding this comment.
The current implementation of cancel_work in FakeHostRuntime only allows scripting the cancelled portion of the CancelRuntimeWorkOutcome. The already_terminal and unsupported fields are always empty.
To make the testkit more powerful and cover more scenarios, I suggest modifying it to allow scripting the entire CancelRuntimeWorkOutcome.
This would involve:
- Changing
cancelled_work: VecDeque<Vec<RuntimeWorkId>>inFakeHostRuntimeState(line 439) tocancel_outcomes: VecDeque<CancelRuntimeWorkOutcome>. - Replacing
with_cancelled_work(line 482) with a new method likewith_cancel_outcome(self, outcome: CancelRuntimeWorkOutcome) -> Self. - Updating
cancel_workto pop from the new queue and return an error if it's empty, for consistency withinvoke_capabilityandvisible_capabilities.
| }); | ||
| let runtime = FakeHostRuntime::new() | ||
| .with_outcome(RuntimeCapabilityOutcome::ApprovalRequired( | ||
| ironclaw_host_runtime::RuntimeApprovalGate { |
There was a problem hiding this comment.
The type RuntimeApprovalGate is used with its full path ironclaw_host_runtime::RuntimeApprovalGate. For consistency and to follow idiomatic Rust patterns, consider adding it to a use statement. If importing from a parent module, use super:: for the relative import instead of the full crate path.
References
- When importing items from a parent module into a child module in Rust, use
super::for relative imports instead of the full crate path.
|
I like the direction of introducing a host-runtime facade here. The contract is moving toward the right shape: upper Reborn services get stable runtime outcomes ( One boundary concern I’d like to tighten before this becomes contract vocabulary:
Because of that, Suggested direction:
That would make the dependency story cleaner: upper workflow calls |
serrrfirat
force-pushed
the
reborn-land-04f-host-runtime-contract
branch
from
73e56d7 to
466e6ac
Compare
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Review: remove production panic paths from the host runtime testkit
The contract facade is directionally clean: it is narrowly scoped, keeps the Reborn layering intact, and models approval/auth/resource waits as structured outcomes instead of errors. The focused contract tests also cover the important facade behavior.
What looks good:
- The new crate depends only on
ironclaw_host_api, which matches the Reborn dependency-boundary direction. - The public facade keeps substrate handles hidden behind structured runtime outcomes.
- The test coverage exercises invocation recording, visible surfaces, cancellation/status/health, and sanitized failures.
Concerning: exported testkit introduces panic-style calls in production code
crates/ironclaw_host_runtime/src/lib.rs exports testkit::FakeHostRuntime from the library, and its mutex accessors call .expect("fake mutex poisoned") in multiple places. Because this lives under src/lib.rs, the repo's production no-panics gate flags it as production code rather than test-only code.
I reproduced the CI failure locally with:
python3 scripts/check_no_panics.py --base origin/reborn-integration --head HEAD
That reports 15 violations in crates/ironclaw_host_runtime/src/lib.rs, starting at line 467. Please either make the testkit locking fallible/non-panicking, or move/gate the fake so it is not part of production-checked library code.
Local verification:
cargo test -p ironclaw_host_runtimepassed.cargo clippy -p ironclaw_host_runtime --all-targets --all-featurespassed.cargo test -p ironclaw_architecture reborn_crate_dependency_boundaries_holdpassed.scripts/check_no_panics.py --base origin/reborn-integration --head HEADfailed with the 15 new.expect()violations above.
|
@henrypark133 I checked the current head (
Verification run from an isolated worktree:
|
|
Addressed the boundary concern by removing the request-origin vocabulary from the host runtime contract entirely. Why: Verification: All passed. |
serrrfirat
force-pushed
the
reborn-land-04f-host-runtime-contract
branch
from
42e9e81 to
4ff3a2f
Compare
|
Rebased onto Re: @henrypark133's review — the testkit panic-paths concern was already addressed by commit Plus:
|
serrrfirat
force-pushed
the
reborn-land-04f-host-runtime-contract
branch
4 times, most recently
from
0213c13 to
76becc4
Compare
Replaces the FakeHostRuntime testkit with DefaultHostRuntime, the production composition that wraps ironclaw_capabilities::CapabilityHost behind the HostRuntime contract. DefaultHostRuntime composes: - ExtensionRegistry (capability descriptors) - CapabilityDispatcher (already-authorized runtime dispatch) - TrustAwareCapabilityDispatchAuthorizer (host-policy-aware authorization) - RunStateStore + ApprovalRequestStore (invocation lifecycle + approval prompts; required for approval-gated paths) - CapabilityLeaseStore (resume paths) - ProcessManager (future spawn paths) Trust is host-validated and supplied per request via the new RuntimeCapabilityRequest::trust_decision field — callers must not synthesize trust decisions or override host policy. Capability invocation outcomes translate as: - Ok(result) -> Completed - Err(AuthorizationRequiresApproval) -> ApprovalRequired (looks up the persisted approval_request_id from run-state) - Err(other) -> Failed with sanitized RuntimeFailureKind Layering hygiene: the projection-surface dimension is now an opaque SurfaceKind bounded string newtype rather than a CapabilitySurfaceKind enum baking in upper-stack vocabulary (agent_loop, adapter, admin). The host treats the label as a cache/version dimension only — caller-authority filtering of which surface a particular UI or upper service may render stays an upper-layer concern. The four request structs (RuntimeCapabilityRequest, VisibleCapabilityRequest, CancelRuntimeWorkRequest, RuntimeStatusRequest) are now #[non_exhaustive] with explicit ::new() constructors so future fields can be added without breaking external callers. Tests now drive DefaultHostRuntime against InMemoryRunStateStore, InMemoryApprovalRequestStore, InMemoryCapabilityLeaseStore, and a recording dispatcher — exercising completed, approval-required, failed, visible-surface, status, cancel, and health paths.
serrrfirat
force-pushed
the
reborn-land-04f-host-runtime-contract
branch
from
76becc4 to
721f889
Compare
2 participants
Summary
Adds PR4f-a: a contract-first
ironclaw_host_runtimefacade for Reborn.This is intentionally not production host-runtime composition yet. It gives upper Reborn layers (
TurnCoordinator,AgentLoopHost, model gateway, adapter plumbing) one stable API to build against without depending directly on dispatcher/runtime/process/network/secret/filesystem internals.Scope
New crate:
Adds:
HostRuntimehigh-level traitRuntimeCapabilityOutcometestkit::FakeHostRuntimeDesign decisions
This PR is contract-first only:
RuntimeDispatcher,CapabilityHost, orProcessHosthandles exposedironclaw_host_apivocabulary where possibleFakeHostRuntimeso upper-stack contracts can start before PR4f-b production wiring landsFollow-up PR4f-b should wire concrete Reborn services behind this facade after the remaining runtime/capability work is ready.
Links
Verification
All passed locally.