refactor(reborn): make host_api::Resolution non-lossy for the CapabilityOutcome collapse (§5.3 Stage 1) - #6254
Merged
Conversation
Contributor
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe host API adds validated result metadata and waypoint wrappers, updates resolution wire shapes, and preserves loop origins, resume tokens, output metadata, failure kinds, and dependent-run origins through capability mapping and authorization paths. ChangesResolution metadata and waypoint propagation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CapabilityOutcome
participant ResolutionMapping
participant HostResolution
participant GateRecord
CapabilityOutcome->>ResolutionMapping: provide outcome and loop-derived metadata
ResolutionMapping->>HostResolution: create waypoint or typed outcome resolution
ResolutionMapping->>GateRecord: persist dependent-run result origin
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
size: XL
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a non-lossy carry mechanism for capability outcomes by adding a new result_meta module to ironclaw_host_api. This module defines vocabulary types such as FailureKind, ResultProgress, TerminateHint, ResumeToken, and LoopRef to preserve loop-derived metadata, gate resume tokens, and originating loop references. The Blocked and Suspension enums, along with the ToolVerdict and Outcome structures, have been updated to carry these new waypoint and metadata structures, and the mapping logic in ironclaw_turns has been adjusted accordingly. The review feedback suggests two performance optimizations in result_meta.rs: using .bytes() instead of .chars() for ASCII validation to avoid UTF-8 decoding overhead, and implementing a custom Serde Visitor for FailureKind deserialization to prevent unnecessary memory allocations for static variants.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+425
to
+428
| if !value | ||
| .chars() | ||
| .all(|c| c.is_ascii_alphanumeric() || matches!(c, '_' | '-' | '.' | ':')) | ||
| { |
Contributor
There was a problem hiding this comment.
Using .chars() to validate ASCII-only characters introduces unnecessary UTF-8 decoding overhead. Since the allowed character set consists entirely of ASCII characters, we can perform this check more efficiently by iterating over bytes using .bytes().
Suggested change
| if !value | |
| .chars() | |
| .all(|c| c.is_ascii_alphanumeric() || matches!(c, '_' | '-' | '.' | ':')) | |
| { | |
| if !value | |
| .bytes() | |
| .all(|b| b.is_ascii_alphanumeric() || matches!(b, b'_' | b'-' | b'.' | b':')) | |
| { |
Member
Author
There was a problem hiding this comment.
Applied in e50c78e — validate_safe_tag now checks bytes() (any non-ASCII byte fails is_ascii_alphanumeric, so semantics are unchanged).
Comment on lines
+247
to
+255
| impl<'de> Deserialize<'de> for FailureKind { | ||
| fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> | ||
| where | ||
| D: serde::Deserializer<'de>, | ||
| { | ||
| let value = String::deserialize(deserializer)?; | ||
| Ok(Self::from_tag(&value)) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Deserializing FailureKind by first deserializing into an owned String always allocates memory, even for the 18 known static variants that do not hold any inner data. We can avoid these allocations entirely by implementing a custom Serde Visitor that deserializes directly from a borrowed string slice (&str).
impl<'de> Deserialize<'de> for FailureKind {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: serde::Deserializer<'de>,
{
struct Visitor;
impl<'de> serde::de::Visitor<'de> for Visitor {
type Value = FailureKind;
fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
formatter.write_str("a failure kind string")
}
fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
where
E: serde::de::Error,
{
Ok(FailureKind::from_tag(v))
}
}
deserializer.deserialize_str(Visitor)
}
}
Member
Author
There was a problem hiding this comment.
Applied in e50c78e via Cow<'_, str>::deserialize — borrows when the input allows it, so the 18 named variants allocate nothing and only an Unknown tag takes an owned copy. (Kept the Cow form over a full Visitor impl: same allocation profile, a tenth of the code.)
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 1 | 0 | 1 | 6665eb43bbc0 |
Head: 6665eb43bbc0b662739ddaccc8b979a4a7fded90
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
The dependent-run mapping still loses the staged result's loop origin, so Resolution is not yet non-lossy for that outcome.
Findings
Blocking: 1 / Notes: 0
Blocking findings
1. ❌ [HIGH] Preserve the dependent-run result origin
Location: crates/ironclaw_turns/src/run_profile/resolution_mapping.rs:302
AwaitDependentRun carries both a gate ref and a staged result_ref. This waypoint preserves only the gate origin; the minted ResultRef in GateRecord::DependentRun has no origin. bindings.result is outside Resolution and the current production seam persists only the gate record, so a Resolution-based consumer cannot associate this staged result with its loop result ref. Preserve the result origin alongside GateRecord::DependentRun (or durably persist the binding) and add a round-trip assertion.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
| } => { | ||
| let minted_gate = GateRef::new(); | ||
| let minted_result = ResultRef::new(); | ||
| let waypoint = gate_waypoint(minted_gate, &gate_ref, None); |
Contributor
There was a problem hiding this comment.
This preserves the gate origin only. AwaitDependentRun also has a result_ref, but its minted GateRecord::DependentRun.result has no origin and bindings.result is not part of Resolution or persisted by the current seam. Preserve that result origin/durable binding so the staged child result remains reachable after migration.
Member
Author
There was a problem hiding this comment.
Confirmed and fixed in e50c78e. GateRecord::DependentRun gains result_origin: Option<LoopRef> — the staged result's originating loop ref now rides the durable record a later resume turn renders from (the minted ResultRef is a fresh uuid and RefBindings is transient, exactly as you noted). Serde default keeps rows persisted before the field rehydratable as None. The mapping populates it from result_ref, pinned by the mapping test (dependent_run_record_carries_staged_result_and_byte_len now asserts the origin) and the gate-record wire round-trip.
ilblackdragon
force-pushed
the
refactor/reborn-result-wiring-seam
branch
from
July 19, 2026 02:15
56a3850 to
2f2d395
Compare
ilblackdragon
added a commit
that referenced
this pull request
Jul 19, 2026
…origin on the durable record; review perf nits - IronLoop: AwaitDependentRun's staged result_ref was preserved nowhere durable — the minted GateRecord::DependentRun.result is a fresh uuid and the RefBindings side-table is transient, so the child output the loop staged under its own ref would be unreachable from the record a later resume turn renders from. GateRecord::DependentRun gains result_origin: Option<LoopRef> (serde default — pre-existing rows rehydrate as None), the mapping populates it, and the mapping + wire tests pin it. - Gemini: FailureKind deserializes via Cow<str> (no allocation for the 18 named variants); validate_safe_tag checks bytes, not chars. Reported-by: ironloopai, gemini-code-assist (PR #6254 review) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
scope: docs
ilblackdragon
added a commit
that referenced
this pull request
Jul 19, 2026
…origin on the durable record; review perf nits - IronLoop: AwaitDependentRun's staged result_ref was preserved nowhere durable — the minted GateRecord::DependentRun.result is a fresh uuid and the RefBindings side-table is transient, so the child output the loop staged under its own ref would be unreachable from the record a later resume turn renders from. GateRecord::DependentRun gains result_origin: Option<LoopRef> (serde default — pre-existing rows rehydrate as None), the mapping populates it, and the mapping + wire tests pin it. - Gemini: FailureKind deserializes via Cow<str> (no allocation for the 18 named variants); validate_safe_tag checks bytes, not chars. Reported-by: ironloopai, gemini-code-assist (PR #6254 review) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ilblackdragon
force-pushed
the
refactor/reborn-resolution-nonlossy
branch
from
July 19, 2026 02:25
e50c78e to
5edfc56
Compare
ilblackdragon
force-pushed
the
refactor/reborn-result-wiring-seam
branch
from
July 19, 2026 02:26
2f2d395 to
c7ba17e
Compare
ilblackdragon
added a commit
that referenced
this pull request
Jul 19, 2026
…origin on the durable record; review perf nits - IronLoop: AwaitDependentRun's staged result_ref was preserved nowhere durable — the minted GateRecord::DependentRun.result is a fresh uuid and the RefBindings side-table is transient, so the child output the loop staged under its own ref would be unreachable from the record a later resume turn renders from. GateRecord::DependentRun gains result_origin: Option<LoopRef> (serde default — pre-existing rows rehydrate as None), the mapping populates it, and the mapping + wire tests pin it. - Gemini: FailureKind deserializes via Cow<str> (no allocation for the 18 named variants); validate_safe_tag checks bytes, not chars. Reported-by: ironloopai, gemini-code-assist (PR #6254 review) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ilblackdragon
force-pushed
the
refactor/reborn-resolution-nonlossy
branch
from
July 19, 2026 02:37
5edfc56 to
7b4de44
Compare
…ityOutcome collapse (§5.3 Stage 1)
Additive vocabulary work so a later stage can delete `CapabilityOutcome`.
`host_api::Resolution` can now losslessly represent every recoverable field the
loop-facing `CapabilityOutcome` carried, and `capability_outcome_to_resolution`
populates them.
New host_api vocabulary (`result_meta.rs`, plain redacted vocabulary only —
bounded enums, a hash value, bounded validated identifiers; no secrets, raw
paths, backend error strings, or runtime handles):
- `FailureKind` (+ `FailureKindValue`) — recovery classification, on
`ToolVerdict::RecoverableFailure { error_kind }` (was G1-dropped).
- `ResultProgress`, `TerminateHint`, `OutputDigest` — loop-derived completion
signals, on `Outcome`/`OutcomeRefs` (were G4-dropped).
- `ResumeToken` — opaque gate-resume identity, on the gate `GateWaypoint`.
- `LoopRef` — preserved originating loop ref, on `GateWaypoint`/`ProcessWaypoint`
origin and `OutcomeRefs.origin`.
Channel reshapes (constructed only by host_api + the mapping — no production
consumers yet): `Blocked`/`Suspension` variants now carry `GateWaypoint`/
`ProcessWaypoint` (kernel handle + preserved origin + optional resume token);
`OutcomeRefs` gains `origin`/`output_digest`; `Outcome` gains `progress`/
`terminate_hint`; `ToolVerdict::RecoverableFailure` gains `error_kind`.
The mapping enrichment deletes the G1/G4 "dropped" comments and populates the
new fields; a round-trip test (written test-first, watched fail with the
enrichment neutralized) pins each. The kernel uuid handle stays freshly minted
(its host-owned semantics) — the loop ref is preserved additively in `origin`
rather than smuggled into the uuid, keeping the "kernel refs are opaque uuids,
never caller-composed" invariant intact.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…origin on the durable record; review perf nits - IronLoop: AwaitDependentRun's staged result_ref was preserved nowhere durable — the minted GateRecord::DependentRun.result is a fresh uuid and the RefBindings side-table is transient, so the child output the loop staged under its own ref would be unreachable from the record a later resume turn renders from. GateRecord::DependentRun gains result_origin: Option<LoopRef> (serde default — pre-existing rows rehydrate as None), the mapping populates it, and the mapping + wire tests pin it. - Gemini: FailureKind deserializes via Cow<str> (no allocation for the 18 named variants); validate_safe_tag checks bytes, not chars. Reported-by: ironloopai, gemini-code-assist (PR #6254 review) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…st to the Blocked waypoint reshape Semantic rebase fallout: this PR reshaped Blocked to carry GateWaypoint, and the since-merged W1b/W1c authorize folds on main construct Blocked::Approval at three kernel sites (plus the authorized_seal test). Bare waypoints there are correct — the kernel witness's approval resume rides the lease machinery; origin/resume waypoint fields are loop-mapping concerns. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ilblackdragon
force-pushed
the
refactor/reborn-resolution-nonlossy
branch
from
July 19, 2026 02:50
5e3b561 to
9ef2c6d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_host_api/src/result_meta.rs`:
- Around line 265-327: Refactor ResumeToken, LoopRef, and FailureKindValue to
follow the validated-newtype convention: add #[serde(try_from = "String")], move
validation into a shared validate(&str) method reused by fallible new, and
remove each hand-written Deserialize implementation. Preserve explicit accessors
and add as_ref or into_inner where required by the convention, without
introducing infallible string conversions or Deref implementations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: eb783006-51d0-4357-9528-0d3760cf7fca
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (11)
crates/ironclaw_capabilities/src/host.rscrates/ironclaw_host_api/src/gate_record.rscrates/ironclaw_host_api/src/lib.rscrates/ironclaw_host_api/src/resolution.rscrates/ironclaw_host_api/src/result_meta.rscrates/ironclaw_host_api/tests/authorized_seal.rscrates/ironclaw_host_runtime/src/production.rscrates/ironclaw_loop_host/Cargo.tomlcrates/ironclaw_loop_host/src/capability_port.rscrates/ironclaw_run_state/tests/gate_record_store_contract.rscrates/ironclaw_turns/src/run_profile/resolution_mapping.rs
Comment on lines
+265
to
+327
| #[derive(Debug, Clone, PartialEq, Eq, Hash)] | ||
| pub struct ResumeToken(String); | ||
|
|
||
| impl ResumeToken { | ||
| /// Maximum length in bytes — matches the loop's `CapabilityResumeToken` bound, | ||
| /// so any loop-minted token is representable losslessly. | ||
| pub const MAX_BYTES: usize = 128; | ||
|
|
||
| pub fn new(value: impl Into<String>) -> Result<Self, HostApiError> { | ||
| let value = value.into(); | ||
| if value.is_empty() { | ||
| return Err(HostApiError::invalid_id( | ||
| "resume_token", | ||
| value, | ||
| "must not be empty", | ||
| )); | ||
| } | ||
| if value.len() > Self::MAX_BYTES { | ||
| return Err(HostApiError::invalid_id( | ||
| "resume_token", | ||
| value, | ||
| format!("must be at most {} bytes", Self::MAX_BYTES), | ||
| )); | ||
| } | ||
| if value.chars().any(|c| c == '\0' || c.is_control()) { | ||
| return Err(HostApiError::invalid_id( | ||
| "resume_token", | ||
| "<redacted>", | ||
| "must not contain NUL/control characters", | ||
| )); | ||
| } | ||
| Ok(Self(value)) | ||
| } | ||
|
|
||
| pub fn as_str(&self) -> &str { | ||
| &self.0 | ||
| } | ||
| } | ||
|
|
||
| impl std::fmt::Display for ResumeToken { | ||
| fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| formatter.write_str(&self.0) | ||
| } | ||
| } | ||
|
|
||
| impl Serialize for ResumeToken { | ||
| fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error> | ||
| where | ||
| S: serde::Serializer, | ||
| { | ||
| serializer.serialize_str(&self.0) | ||
| } | ||
| } | ||
|
|
||
| impl<'de> Deserialize<'de> for ResumeToken { | ||
| fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> | ||
| where | ||
| D: serde::Deserializer<'de>, | ||
| { | ||
| let value = String::deserialize(deserializer)?; | ||
| Self::new(value).map_err(serde::de::Error::custom) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Validated newtypes skip the mandated #[serde(try_from = "String")] pattern.
ResumeToken (265-327), LoopRef (343-411), and FailureKindValue (156-169) are new validated newtypes but hand-roll Deserialize and expose only as_str. The convention here is #[serde(try_from = "String")] + a shared validate(&str) + as_str/as_ref/into_inner. Functionally equivalent and secure as written, but aligning keeps the contract-crate boundary types uniform and forecloses an infallible-construction drift later.
As per coding guidelines: "New validated newtypes must use #[serde(try_from = "String")], a shared validate(&str), fallible new, explicit as_str/as_ref/into_inner methods, and must not implement infallible From<String>, From<&str>, or Deref<Target = str>."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_host_api/src/result_meta.rs` around lines 265 - 327, Refactor
ResumeToken, LoopRef, and FailureKindValue to follow the validated-newtype
convention: add #[serde(try_from = "String")], move validation into a shared
validate(&str) method reused by fallible new, and remove each hand-written
Deserialize implementation. Preserve explicit accessors and add as_ref or
into_inner where required by the convention, without introducing infallible
string conversions or Deref implementations.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_turns/src/run_profile/resolution_mapping.rs (1)
264-288: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an origin assertion for
SpawnedChildRun.Every other stage-1 non-lossy field (Completed's origin/digest/progress, Approval/Auth's resume+origin, SpawnedProcess's origin) has a dedicated test.
SpawnedChildRun's newrefs.origin(Line 277) has none —child_run_identity_is_preserved_on_the_verdictonly checkschild_run/byte_len.#[test] fn child_run_identity_is_preserved_on_the_verdict() { let child_run_id = TurnRunId::new(); let mapped = capability_outcome_to_resolution(CapabilityOutcome::SpawnedChildRun { child_run_id, result_ref: result_ref(), safe_summary: "spawned".to_string(), byte_len: 64, model_observation: None, }); match mapped.resolution { Resolution::Done(outcome) => { assert_eq!( outcome.verdict.child_run().map(|run| run.as_uuid()), Some(child_run_id.as_uuid()) ); assert_eq!(outcome.refs.byte_len, 64); + assert_eq!( + outcome.refs.origin.as_ref().map(LoopRef::as_str), + Some(result_ref().as_str()) + ); } other => panic!("expected Done, got {other:?}"), } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_turns/src/run_profile/resolution_mapping.rs` around lines 264 - 288, The SpawnedChildRun mapping in the resolution mapper lacks test coverage for preserving refs.origin. Extend the existing child_run_identity_is_preserved_on_the_verdict test, or add a focused test nearby, to assert that the mapped Outcome refs.origin matches the original result_ref identity while retaining the existing child_run and byte_len assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_turns/src/run_profile/resolution_mapping.rs`:
- Around line 36-38: Update the module documentation near the `SpawnedProcess`
summary to state that `AwaitDependentRun`'s `model_observation` is dropped
because `GateRecord::DependentRun` has no preview field; retain the
result-preview behavior only for `SpawnedChildRun`.
---
Outside diff comments:
In `@crates/ironclaw_turns/src/run_profile/resolution_mapping.rs`:
- Around line 264-288: The SpawnedChildRun mapping in the resolution mapper
lacks test coverage for preserving refs.origin. Extend the existing
child_run_identity_is_preserved_on_the_verdict test, or add a focused test
nearby, to assert that the mapped Outcome refs.origin matches the original
result_ref identity while retaining the existing child_run and byte_len
assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c4c881c6-0fbc-41b4-94fb-d379b0d7a51d
📒 Files selected for processing (8)
crates/ironclaw_capabilities/src/host.rscrates/ironclaw_host_api/src/gate_record.rscrates/ironclaw_host_api/src/lib.rscrates/ironclaw_host_api/src/resolution.rscrates/ironclaw_host_api/src/result_meta.rscrates/ironclaw_host_api/tests/authorized_seal.rscrates/ironclaw_run_state/tests/gate_record_store_contract.rscrates/ironclaw_turns/src/run_profile/resolution_mapping.rs
Comment on lines
+36
to
+38
| //! `SpawnedProcess`'s `safe_summary` still has no host channel (a process | ||
| //! suspension carries a [`ProcessRef`], not a summary). `AwaitDependentRun`'s and | ||
| //! `SpawnedChildRun`'s `model_observation` ride the result preview where present. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Doc contradicts the actual AwaitDependentRun mapping.
This doc claims AwaitDependentRun's model_observation "ride[s] the result preview where present," but the match arm (Line 298, ..) drops it entirely, and its own inline comment (Lines 289-292) says it "has no home on DependentRun and is dropped." GateRecord::DependentRun has no preview field to ride. Only SpawnedChildRun actually carries the preview (Line 276). Fix the doc to avoid misleading future readers about what's non-lossy here.
📝 Proposed doc fix
-//! `SpawnedProcess`'s `safe_summary` still has no host channel (a process
-//! suspension carries a [`ProcessRef`], not a summary). `AwaitDependentRun`'s and
-//! `SpawnedChildRun`'s `model_observation` ride the result preview where present.
+//! `SpawnedProcess`'s `safe_summary` still has no host channel (a process
+//! suspension carries a [`ProcessRef`], not a summary). `SpawnedChildRun`'s
+//! `model_observation` rides the result preview where present; `AwaitDependentRun`'s
+//! `model_observation` has no home on `GateRecord::DependentRun` and is dropped.As per coding guidelines, "Comments and documentation that promise guarantees must match the code and tests."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| //! `SpawnedProcess`'s `safe_summary` still has no host channel (a process | |
| //! suspension carries a [`ProcessRef`], not a summary). `AwaitDependentRun`'s and | |
| //! `SpawnedChildRun`'s `model_observation` ride the result preview where present. | |
| //! `SpawnedProcess`'s `safe_summary` still has no host channel (a process | |
| //! suspension carries a [`ProcessRef`], not a summary). `SpawnedChildRun`'s | |
| //! `model_observation` rides the result preview where present; `AwaitDependentRun`'s | |
| //! `model_observation` has no home on `GateRecord::DependentRun` and is dropped. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_turns/src/run_profile/resolution_mapping.rs` around lines 36
- 38, Update the module documentation near the `SpawnedProcess` summary to state
that `AwaitDependentRun`'s `model_observation` is dropped because
`GateRecord::DependentRun` has no preview field; retain the result-preview
behavior only for `SpawnedChildRun`.
Source: Coding guidelines
ilblackdragon
added a commit
that referenced
this pull request
Jul 19, 2026
…ad (§5.3) (#6256) * refactor(reborn): make host_api::Resolution non-lossy for the CapabilityOutcome collapse (§5.3 Stage 1) Additive vocabulary work so a later stage can delete `CapabilityOutcome`. `host_api::Resolution` can now losslessly represent every recoverable field the loop-facing `CapabilityOutcome` carried, and `capability_outcome_to_resolution` populates them. New host_api vocabulary (`result_meta.rs`, plain redacted vocabulary only — bounded enums, a hash value, bounded validated identifiers; no secrets, raw paths, backend error strings, or runtime handles): - `FailureKind` (+ `FailureKindValue`) — recovery classification, on `ToolVerdict::RecoverableFailure { error_kind }` (was G1-dropped). - `ResultProgress`, `TerminateHint`, `OutputDigest` — loop-derived completion signals, on `Outcome`/`OutcomeRefs` (were G4-dropped). - `ResumeToken` — opaque gate-resume identity, on the gate `GateWaypoint`. - `LoopRef` — preserved originating loop ref, on `GateWaypoint`/`ProcessWaypoint` origin and `OutcomeRefs.origin`. Channel reshapes (constructed only by host_api + the mapping — no production consumers yet): `Blocked`/`Suspension` variants now carry `GateWaypoint`/ `ProcessWaypoint` (kernel handle + preserved origin + optional resume token); `OutcomeRefs` gains `origin`/`output_digest`; `Outcome` gains `progress`/ `terminate_hint`; `ToolVerdict::RecoverableFailure` gains `error_kind`. The mapping enrichment deletes the G1/G4 "dropped" comments and populates the new fields; a round-trip test (written test-first, watched fail with the enrichment neutralized) pins each. The kernel uuid handle stays freshly minted (its host-owned semantics) — the loop ref is preserved additively in `origin` rather than smuggled into the uuid, keeping the "kernel refs are opaque uuids, never caller-composed" invariant intact. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(host_api,turns): preserve the dependent-run staged result's loop origin on the durable record; review perf nits - IronLoop: AwaitDependentRun's staged result_ref was preserved nowhere durable — the minted GateRecord::DependentRun.result is a fresh uuid and the RefBindings side-table is transient, so the child output the loop staged under its own ref would be unreachable from the record a later resume turn renders from. GateRecord::DependentRun gains result_origin: Option<LoopRef> (serde default — pre-existing rows rehydrate as None), the mapping populates it, and the mapping + wire tests pin it. - Gemini: FailureKind deserializes via Cow<str> (no allocation for the 18 named variants); validate_safe_tag checks bytes, not chars. Reported-by: ironloopai, gemini-code-assist (PR #6254 review) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(capabilities,host_api): adapt merged W1 authorize folds + seal test to the Blocked waypoint reshape Semantic rebase fallout: this PR reshaped Blocked to carry GateWaypoint, and the since-merged W1b/W1c authorize folds on main construct Blocked::Approval at three kernel sites (plus the authorized_seal test). Bare waypoints there are correct — the kernel witness's approval resume rides the lease machinery; origin/resume waypoint fields are loop-mapping concerns. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(reborn): host-private ReplayPayloadStore for gate/auth resume-read (§5.3) Adds the persistence slice that unblocks the capability-result collapse (docs/reborn/2026-07-17-architecture-simplification-dto-dyn-local.md §3/§5.3). Mirrors the sibling GateRecordStore (#6243) but is unwired: it provides the port + a Filesystem implementation + contract tests only. A later slice (Stage 2a) wires write-at-gate-raise + read-on-resume and flips CapabilityOutcome. Today the raw replay payload rides in-band through the UNTRUSTED loop on CapabilityApprovalResume/CapabilityAuthResume and is stashed in the loop's own serialized checkpoint (raw input+estimate). The collapse makes the loop-facing Resolution carry only an opaque resume token (== InvocationId), so the host must persist the replay payload itself and reconstitute it on resume. Moving it host-side ALSO retires a real exposure — raw tool input no longer round-trips through the loop's checkpoint. Host-privacy is a security requirement here. Record schema (host-private; the opposite of a model-visible GateRecord — no SafeSummary): ReplayPayload { input: serde_json::Value, estimate: ResourceEstimate, prior_approval: Option<AuthResumeApprovalIdentity>, input_ref: CapabilityInputRef, correlation_id: CorrelationId }. The field types are imported from their owners (ironclaw_turns for CapabilityInputRef/AuthResumeApprovalIdentity, host_api for the rest), not re-typed — no lossy re-typing per type-placement.md. Placement: ironclaw_capabilities, NOT ironclaw_run_state. The run_state charter (CLAUDE.md line 7) forbids persisting raw replay input in run-state records, and the ironclaw_turns charter forbids persisting raw tool input in turn state/events — both candidate "clean" homes are charter-hostile to raw replay input. capabilities owns the caller-facing invoke/resume/spawn workflow this payload exists to serve and has no such prohibition (type-placement.md §2). The two new dependency edges (capabilities -> turns, capabilities -> filesystem) are both permitted by the layer matrix (kernel -> kernel, kernel -> substrates) and match existing kernel topology (host_runtime -> turns; run_state -> filesystem); ironclaw_architecture is green. CAS/mount lane reused from the sibling: ScopedFilesystem<F: RootFilesystem> + shared cas_update (fail-closed CasUnsupported on non-CAS backends), a replay_payload_record RecordKind gate rejecting byte-only backends, a private StoredReplayPayload wrapper carrying the scope for a same_scope_owner defense-in-depth check, and a new /replay-payloads mount alias (tenant/user in the MountView, within-tenant axes in the alias-relative path). save() is write-once (dup InvocationId -> ReplayPayloadAlreadyExists); load() returns Option. No removal method — the payload is consumed once on resume; a later retention contract can add deletion (database.md "Data safety"). Test-first: 6 contract tests mirror gate_record_store_contract.rs and were watched red against a stub before implementing — all-fields round-trip (raw input+estimate survive), the auth-without-prior-approval shape, missing -> None, write-once dup rejection (original intact), and cross-tenant + within-tenant scope isolation (tenant1 vs tenant2; project A vs B look unknown). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.54% — 311753 / 364435 lines Per-crate breakdown (65 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (3 entry/entries excluded from the accounting above)
|
Member
Author
✅ Ready for mergeReviewed, all three review comments addressed with fixes, restacked onto main (twice — past the #6245 seam merge and the W1 chain), CI fully green — 58 pass / 0 fail.
🤖 Generated with Claude Code |
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
1 participant
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.
Stage 1 of the result-side collapse (§5.3)
Additive vocabulary work: make
host_api::Resolutionable to carry everything theloop needs so a later stage can delete
CapabilityOutcome.CapabilityOutcomeisuntouched, the
loop_hostseam's call tocapability_outcome_to_resolutioncompilesunchanged, and no loop-facing consumer moves. Nothing produces
Resolutioninproduction yet, so the channel reshapes below are constructed only by
host_apiandthe mapping.
Fields added + which variant they live on + why they satisfy the charter
New
crates/ironclaw_host_api/src/result_meta.rs— every type is plain redactedvocabulary (bounded enum, fixed-width hash, or bounded validated safe identifier);
none carries a secret, raw
HostPath, backend error string, or runtime handle:error_kind: FailureKind(+FailureKindValue)ToolVerdict::RecoverableFailure { error_kind }CapabilityFailure::error_kind(G1-dropped)network/backend/…) + openUnknown; never the raw cause (rawdetailstays host-side)progress: ResultProgressOutcome::progressCapabilityResultMessage::progress(G4-dropped)terminate_hint: TerminateHintOutcome::terminate_hintCapabilityResultMessage::terminate_hint: bool(G4-dropped)output_digest: Option<OutputDigest>OutcomeRefs::output_digestCapabilityResultMessage::output_digest(G4-dropped)u64Blake3 hash, never contentresume: Option<ResumeToken>GateWaypoint(Blocked::Approval/Auth)resume_tokeninsideapproval_resume/auth_resume(G1-dropped)origin: Option<LoopRef>GateWaypoint,ProcessWaypoint,OutcomeRefsresult:*/gate:*/process:*ref (fresh-uuid-minted away)Channel reshapes in
resolution.rs:Blocked::{Approval,Auth,Resource}andSuspension::{DependentRun,ExternalTool}now hold aGateWaypoint(kernelGateReforigin+ optionalresume);Suspension::Processholds aProcessWaypoint.ToolVerdictlosesCopy(aFailureKind::Unknownowns aString). AllOptionadditive fields use#[serde(default, skip_serializing_if)]so a bare waypoint serializes as just its handle; default-backed
with_*buildersper
.claude/rules/default-builders.md.Mapping enrichment
capability_outcome_to_resolutionnow populates every new field (deleting theG1/G4 "dropped" doc comments and the
..-ignored resume tokens). The kernel uuidhandle stays freshly minted — the loop ref is preserved additively on
origin, andthe
MappedResolution { resolution, gate_record, deny_record, bindings }returnshape is unchanged.
Test-first round-trip coverage
Extended the existing
resolution_mappingtest module (no parallel suite) with fourcases asserting the new fields survive
CapabilityOutcome → Resolution: aFailedcarries its
error_kind; aCompletedcarries progress/terminate_hint/digest/origin;an approval/auth gate carries its resume token + preserved origin; a spawned process
preserves its loop process ref on the channel. Watched them fail first by
neutralizing the enrichment — all four panicked for the right reason ("the approval
resume token must cross", "the recovery class must ride the verdict (was G1-dropped)",
origin/digest dropped) — then restored the enrichment to green. host_api gained unit
tests for every new type.
Charter tension resolved (in favor of the charter)
CapabilityApprovalResume/CapabilityAuthResumebundle rawinput: serde_json::Value+estimate+input/approval/correlation ids alongside the token. Only the opaque
ResumeTokencrosses; the raw replay is host execution context (charter forbids raw input in
vocabulary). In the target §3 model input is by-ref and the host reconstitutes the
replay from its own storage keyed by the token — it never needed to round-trip
through the loop.
from_seed(&str)deterministic derivation would have let the mapping "delete thefresh-uuid-minting" literally, but adding a text-taking constructor to the kernel
ref types risks weakening the ids.rs invariant that kernel refs are opaque uuids
"never composed from a caller string." I preserved the loop ref additively on
origininstead — completeness without reopening that fork.Is
Resolutionnow a complete superset?Yes for everything the loop needs. Recoverable-field coverage is complete: error_kind,
progress, terminate_hint, output_digest, resume token, and the originating loop ref
(gate/result/process) all round-trip. Deliberately not represented (host-side by
charter, not loop vocabulary), which Stage 2 must handle host-side:
CapabilityFailure::detail— raw backend cause (ridesTurnLifecycleEvent.detail).SpawnedProcess::safe_summary— no host process-summary channel exists.do) falls back to
origin = Noneand stays reachable via the retainedbindings.Verification (all green)
cargo test -p ironclaw_host_api -p ironclaw_turns -p ironclaw_run_state -p ironclaw_loop_host --all-features— 0 failedcargo clippy -p ironclaw_host_api -p ironclaw_turns -p ironclaw_loop_host --all-targets --all-features -- -D warnings— cleancargo test -p ironclaw_architecture— 0 failedcargo check --workspace --all-features— cleanscripts/pre-commit-safety.sh— exit 0🤖 Generated with Claude Code