Skip to content

feat(reborn): add text-only loop driver host factory - #3439

Merged
serrrfirat merged 4 commits into
reborn-integrationfrom
reborn/issue-3407-text-host-factory
May 10, 2026
Merged

serrrfirat merged 4 commits into
reborn-integrationfrom
reborn/issue-3407-text-host-factory

Conversation

@serrrfirat

@serrrfirat serrrfirat commented May 9, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #3407.

Summary

  • add Reborn text-only AgentLoopDriverHost factory with trait-object-backed ports
  • compose context, prompt, input, model, capability, transcript, checkpoint, and progress ports
  • add scoped no-extra input, checkpoint id -> state ref persistence, and progress milestone ports
  • validate claimed run/profile/driver/thread scope before returning host
  • address ilblackdragon review: typed default-profile normalization, agent-scoped host boundary docs/tests, checkpoint error summaries, redacted milestone contract comment

Follow-ups

Risk

  • medium: additive Reborn production host factory plus turn checkpoint persistence surface and DB-backed contracts

Tests

  • cargo fmt --all -- --check
  • CARGO_TARGET_DIR=/tmp/ironclaw-target-pr3439-review cargo test -p ironclaw_turns --test run_profile_contract --test checkpoint_state_store_contract
  • CARGO_TARGET_DIR=/tmp/ironclaw-target-pr3439-review cargo test -p ironclaw_reborn --test loop_driver_host
  • CARGO_TARGET_DIR=/tmp/ironclaw-target-pr3439-review cargo test -p ironclaw_turns --features postgres
  • CARGO_TARGET_DIR=/tmp/ironclaw-target-pr3439-review cargo test -p ironclaw_reborn --tests
  • CARGO_TARGET_DIR=/tmp/ironclaw-target-pr3439-review cargo test -p ironclaw_architecture --test reborn_dependency_boundaries -- --nocapture
  • CARGO_TARGET_DIR=/tmp/ironclaw-target-pr3439-review cargo clippy -p ironclaw_turns -p ironclaw_reborn --all-targets -- -D warnings
  • git diff --check
  • bash scripts/pre-commit-safety.sh

@github-actions github-actions Bot added scope: dependencies Dependency updates size: XL 500+ changed lines risk: low Changes to docs, tests, or low-risk modules contributor: core 20+ merged PRs labels May 9, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces the RebornLoopDriverHost and its factory to manage agent loops, implementing several ports for context, prompts, inputs, and checkpoints. It also extends the ironclaw_turns crate with a LoopCheckpointStore for record persistence. Feedback identifies an unnecessary async keyword, recommends using constants for hardcoded strings, and suggests an optimization to avoid a clone during scope validation.

}
}

pub async fn build_text_only_host(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The build_text_only_host method is marked as async, but it does not perform any asynchronous operations. All validations and object constructions within the method are synchronous. Unless future implementations or trait requirements necessitate it, the async keyword can be removed to simplify the API.

}

fn persisted_profile_id(profile_id: &RunProfileId) -> RunProfileId {
if profile_id.as_str() == "interactive_default" {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The string "interactive_default" is hardcoded for profile ID normalization. It is recommended to use a constant defined in the ironclaw_turns crate (e.g., within RunProfileId) to ensure consistency and avoid issues if the underlying protocol string changes.

run_context: &LoopRunContext,
) -> Result<(), RebornLoopDriverHostError> {
if thread_scope.tenant_id != run_context.scope.tenant_id
|| Some(thread_scope.agent_id.clone()) != run_context.scope.agent_id

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The agent_id comparison can be performed without cloning by using as_ref() on the Option in run_context.scope.agent_id.

Suggested change
|| Some(thread_scope.agent_id.clone()) != run_context.scope.agent_id
|| run_context.scope.agent_id.as_ref() != Some(&thread_scope.agent_id)

@serrrfirat
serrrfirat force-pushed the reborn/issue-3407-text-host-factory branch from 9e24eb7 to 8cacf0e Compare May 9, 2026 21:59
@ilblackdragon

Copy link
Copy Markdown
Member

Code Review: PR #3439 — feat(reborn): add text-only loop driver host factory

Where this fits in the Reborn integration

This PR closes the loop on the Reborn agent-loop architecture. PR #3446 (KB-001) introduced a TurnRunnerWorker that takes an Arc<dyn HostFactory>, but the only implementation was a test mock. This PR is the production HostFactory. Concretely:

TurnRunnerWorker (#3446)
  → claim_next_run() → ClaimedTurnRun
  → DriverRegistry::get(driver_id) → registered driver
  → HostFactory::create_host(claimed) ← THIS PR builds the host
  → driver.run(req, &host) → LoopExit
  → LoopExitApplier (#3446) validates → TurnRunTransitionPort

The PR also introduces a new architectural primitive: two-stage checkpoints. Until now, CheckpointStateStore held content-addressed payloads keyed by LoopCheckpointStateRef. This PR adds LoopCheckpointStore that holds the mapping from TurnCheckpointId → LoopCheckpointStateRef. The flow:

  1. Driver computes durable state, stages it via CheckpointStateStore::put_checkpoint_state(payload) → gets state_ref.
  2. Driver calls host.checkpoint(LoopCheckpointRequest { state_ref, kind }).
  3. HostManagedLoopCheckpointPort verifies the state_ref is real, scoped to this run, and matches the requested kind/schema before minting a TurnCheckpointId and recording the mapping.

Same trust pattern as the LoopExitApplier: drivers can name evidence (state_refs, message_refs, gate_refs), but cannot forge it — the host verifies durable existence and scope before sealing anything. Defense-in-depth.

Overview

12 files, +2104/−32. Core additions:

  • crates/ironclaw_reborn/src/loop_driver_host.rs (593 LOC): RebornLoopDriverHostFactory<S, G> builds a RebornLoopDriverHost that implements 8 loop ports.
  • crates/ironclaw_turns/src/checkpoint_state.rs (+108): new LoopCheckpointStore trait, LoopCheckpointRecord, Put/GetLoopCheckpointRequest, InMemoryLoopCheckpointStore.
  • crates/ironclaw_turns/src/db.rs (+138): libSQL + Postgres trait impls plus a new turn_loop_checkpoints table on both backends.
  • crates/ironclaw_turns/src/memory.rs (+63): InMemoryTurnStateStore also implements LoopCheckpointStore, threading loop_checkpoints through the persistence snapshot.
  • 846 LOC of tests plus DB-backed contract tests.

Strengths

  • Validation discipline at host construction. Seven distinct scope invariants checked before any port is built. The 7 negative validation tests cover each path.
  • Trust boundary on checkpoint binding. A driver cannot bind a TurnCheckpointId to a forged or wrong-scope state_ref — text_only_host_checkpoint_port_rejects_foreign_state_ref and ..._rejects_kind_mismatch lock this.
  • Empirical milestone redaction test. assert_public_milestones_hide_raw_payloads stages raw payloads with sentinel tokens (RAW_CHECKPOINT_PAYLOAD, sk-secret, /host/path, tool_input, model says hi) and asserts none appear in serialized milestones. Empirical, not documentary.
  • Forward-compatible persistence: #[serde(default, skip_serializing_if = "Vec::is_empty")] on TurnPersistenceSnapshot::loop_checkpoints means old snapshots deserialize cleanly.
  • Compile-time trait verification: fn assert_loop_checkpoint_store<T: LoopCheckpointStore>() for both LibSqlTurnStateStore and PostgresTurnStateStore — catches drift at build time.

Issues to address

Correctness — typed-internals violation (blocking)

persisted_profile_id directly violates .claude/rules/types.md (loop_driver_host.rs:608-614):

fn persisted_profile_id(profile_id: &RunProfileId) -> RunProfileId {
    if profile_id.as_str() == "interactive_default" {
        RunProfileId::default_profile()
    } else {
        profile_id.clone()
    }
}

The rule is explicit: "Match-on-string-literals means the type should be an enum. Fix the type." RunProfileId is the typed identifier — comparing its .as_str() against a string literal is the exact pattern that shipped bugs #2561, #2473, #2512, #2574. Two failure modes here:

  1. Any unrelated profile literally named "interactive_default" would be silently rewritten.
  2. If the resolver's default name changes, this normalization stops working without compile-time signal.

The fix is to either:

  • Make RunProfileId::default_profile() return the same value the resolver produces (so the normalization is unnecessary), OR
  • Add an explicit RunProfileId::is_interactive_default(&self) -> bool method backed by a typed comparison against a private static RunProfileId constant.

Don't paper over it with a comment — the rule says "the compiler is the only durable enforcement."

Related upstream stringly-typed leak (not introduced here, but exposed by this PR's tests): HostManagedModelRequest.run_id / .turn_id are String (tests at lines 866–871 compare them via .to_string() from typed TurnRunId/TurnId). And CapabilityDenied.reason_kind: String (test at line 1269: denied.reason_kind == "empty_surface") — sibling Cancel { reason_kind: LoopCancelReasonKind } shows the right shape. These should become typed in ironclaw_loop_support / ironclaw_turns::run_profile::host respectively. Out of scope for this PR; worth a follow-up issue.

Correctness — other

  • validate_thread_scope requires Some(thread_scope.agent_id) == run_context.scope.agent_id but TurnScope::agent_id is Option<AgentId>. User-only runs (no agent) would always fail this validation. If the factory is deliberately agent-only, document it. If not, relax the check to allow agent-less scopes when both sides are None.
  • turn_error_to_host_error collapses ScopeNotFound | Conflict | InvalidTransition | LeaseMismatch into CheckpointRejected. Lossy mapping — lease expiry vs scope mismatch are operationally distinct. Preserve more detail in AgentLoopHostError discriminants or carry the original TurnErrorCategory through safe_summary.

Performance / scale (flag as follow-up)

  • The libSQL/Postgres LoopCheckpointStore impls are O(snapshot) per call. Every get_loop_checkpoint does load_snapshot() → from_persistence_snapshot() → in-memory get. Every put_loop_checkpoint is full snapshot replace under SHARE ROW EXCLUSIVE on PG. Drivers checkpoint frequently (per the contract: before model, before side-effect, before block, terminal); resumes will read often. This will not survive production load. The pattern is inherited from sibling stores, so a workspace-wide follow-up issue is the right framing — but flag it loudly. A direct SELECT WHERE checkpoint_id = ? path is needed before any traffic.

Style / project conventions

  • No .unwrap()/.expect() violations in production code ✅.
  • RebornLoopDriverHost manually delegates 8 traits to inner Arc<dyn …> fields — ~150 LOC of pure forwarding. The delegate crate or a small macro could halve this; explicit form is also clear. Not a blocker.

Test coverage gaps

  • No concurrent-write test on InMemoryLoopCheckpointStore — multiple parallel put_loop_checkpoint calls. Probably fine (UUIDv4) but a quick test locks it.
  • No fault-injection test for HostManagedLoopCheckpointPort when the underlying store returns errors — the turn_error_to_host_error path exists but isn't exercised.
  • The milestone-redaction test should have a comment explaining the trust contract — why is "model says hi" forbidden in milestones? (Because milestones carry refs/metadata, not content.)

Risk labeling

risk: low is misleading for an XL PR (2104 lines) that introduces a new persistence layer, three backend trait impls, a new schema migration, and the production wiring for the entire Reborn agent loop. The diff is additive and well-tested, but the surface area is significant. "Medium" feels more accurate. Pre-merge: cargo test --features postgres (the test list doesn't include this; postgres tests are gated and need a live PG to run).

Summary

Approve with the following addressed before merge:

  1. Fix the persisted_profile_id typed-internals violation — the rule is non-negotiable.
  2. Document or relax validate_thread_scope's agent-required check.
  3. File a follow-up issue for snapshot-per-call DB scale and link it from the PR description.
  4. File a follow-up for the HostManagedModelRequest/CapabilityDenied stringly-typed upstream fields.
  5. Add a comment to the milestone-redaction test explaining the refs-only contract.
  6. Run cargo test --features postgres at least once or note that it's blocked on infra.

The architecture and validation discipline here are excellent. The typed-internals violation is the primary blocker; the performance shape is the secondary concern (inherited from existing patterns, not introduced here).

@serrrfirat serrrfirat added risk: medium Business logic, config, or moderate-risk modules reborn IronClaw Reborn architecture and landing work and removed risk: low Changes to docs, tests, or low-risk modules labels May 10, 2026
@github-actions github-actions Bot added risk: low Changes to docs, tests, or low-risk modules and removed risk: medium Business logic, config, or moderate-risk modules labels May 10, 2026
@serrrfirat serrrfirat added risk: medium Business logic, config, or moderate-risk modules and removed risk: low Changes to docs, tests, or low-risk modules labels May 10, 2026
@serrrfirat

Copy link
Copy Markdown
Collaborator Author

Addressed the review feedback in c4621da3:

  • Replaced the host-side raw "interactive_default" string comparison with typed RunProfileId helpers (is_interactive_default, interactive_default, long_running_mission).
  • Documented and tested that the text-only host is currently agent-scoped, rejecting agentless turn scopes until ironclaw_threads::ThreadScope grows an explicit agentless boundary.
  • Split checkpoint TurnError mapping summaries so scope-not-found, conflict, invalid transition, and lease mismatch remain operationally distinguishable.
  • Added a milestone redaction contract comment explaining refs/metadata-only public milestones.
  • Added fault-injection coverage for loop checkpoint store failures and a parallel put_loop_checkpoint test.
  • Filed follow-ups for out-of-scope items:
  • Updated PR body and changed risk label to risk: medium.

Verification run locally:

  • cargo fmt --all -- --check
  • cargo test -p ironclaw_turns --test run_profile_contract --test checkpoint_state_store_contract
  • cargo test -p ironclaw_reborn --test loop_driver_host
  • cargo test -p ironclaw_turns --features postgres
  • cargo test -p ironclaw_reborn --tests
  • cargo test -p ironclaw_architecture --test reborn_dependency_boundaries -- --nocapture
  • cargo clippy -p ironclaw_turns -p ironclaw_reborn --all-targets -- -D warnings
  • git diff --check
  • bash scripts/pre-commit-safety.sh

GitHub checks are green.

@serrrfirat
serrrfirat merged commit c787cbf into reborn-integration May 10, 2026
14 checks passed
@serrrfirat
serrrfirat deleted the reborn/issue-3407-text-host-factory branch May 10, 2026 18:25
theredspoon pushed a commit to theredspoon/ironclaw that referenced this pull request Jun 21, 2026
* feat(reborn): add text-only loop driver host factory

* fix(reborn): persist text host checkpoint mapping

* test(reborn): expand text host coverage

* fix(reborn): address ilblackdragon review — typed profile and checkpoint hardening (nearai#3439)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor: core 20+ merged PRs reborn IronClaw Reborn architecture and landing work risk: medium Business logic, config, or moderate-risk modules scope: dependencies Dependency updates size: XL 500+ changed lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants