Repository navigation
arch(ws-0): state, checkpoints, BoundedRing, CapabilityCallSignature, NoProgressDetected - #3550
Conversation
github-actions
Bot
added
size: XL
![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_agent_loop crate, which establishes the framework for agent loop execution state and strategy contracts. Key components include the LoopExecutionState for managing iteration data, a BoundedRing for tracking recent failures and signatures, and a canonicalization mechanism for stable hashing of capability calls. Additionally, the PR extends ironclaw_turns with support for LoopInlineMessage and a new NoProgressDetected failure kind. Review feedback highlights opportunities to improve performance by avoiding unnecessary clones during deserialization and canonicalization, and identifies a potential resource exhaustion vulnerability in the BoundedRing deserialization logic.
| let state = object | ||
| .get("state") | ||
| .ok_or(CheckpointPayloadError::MissingField { field: "state" })?; | ||
| serde_json::from_value(state.clone()).map_err(|error| { |
There was a problem hiding this comment.
Avoid cloning the state value here. Since &serde_json::Value implements Deserializer, you can deserialize directly from the reference, which is more efficient for large execution states.
| serde_json::from_value(state.clone()).map_err(|error| { | |
| Self::deserialize(state).map_err(|error| { |
References
- To improve performance, avoid unnecessary heap allocations and clones when processing data structures.
| let mut keys = object.keys().collect::<Vec<_>>(); | ||
| keys.sort(); | ||
| for (index, key) in keys.into_iter().enumerate() { | ||
| if index > 0 { | ||
| out.push(','); | ||
| } | ||
| out.push_str(&serde_json::Value::String(key.clone()).to_string()); | ||
| out.push(':'); | ||
| if let Some(child) = object.get(key) { | ||
| canonicalize(child, out); | ||
| } | ||
| } | ||
| out.push('}'); |
There was a problem hiding this comment.
The object canonicalization loop is inefficient. It collects and sorts keys, then performs a redundant lookup for each value, and uses an expensive way to JSON-escape keys (cloning into a Value then formatting). Refactoring to iterate over entries directly and using serde_json::to_string for escaping reduces overhead.
let mut entries: Vec<_> = object.iter().collect();
entries.sort_by_key(|&(k, _)| k);
for (index, (key, value)) in entries.into_iter().enumerate() {
if index > 0 {
out.push(',');
}
out.push_str(&serde_json::to_string(key).unwrap());
out.push(':');
canonicalize(value, out);
}
out.push('}');References
- To improve performance, avoid unnecessary heap allocations and use iterators directly instead of collecting them into a Vec when possible.
github-actions
Bot
added
the
risk: medium
PR #3590 originally wired the Reborn ProductAdapter / Telegram v2 channel into the v1 agent binary at src/channels/reborn/, gated only by a runtime flag. Per @serrrfirat's review the v1 agent should not be the host for Reborn-experimental code at all. This commit removes that coupling entirely. The Reborn host is now a separate workspace crate (crates/ironclaw_reborn_telegram_v2_host/) with its own binary (ironclaw-reborn-telegram-host). The v1 ironclaw binary has zero awareness it exists: no Reborn crate dependencies in v1's Cargo.toml, no wiring code, no shared in-process state, no runtime flag, no v1/v2 exclusivity guard. Reply-path stub --------------- The current PR's tracer bridged through v1's in-process ChannelManager to produce an actual Telegram reply. That bridge cannot exist across processes, and no Reborn agent loop ships in src/ yet (PRs #3544 / #3550 / #3586 still open). The new host terminates inbound at the durable ledger / binding write and acks 200 to Telegram; no reply is produced until the Reborn loop lands, at which point swapping StubInboundTurnService for DefaultInboundTurnService is the only required change. zmanian's review items ---------------------- Fixed in this commit alongside the extraction (verified by tests): 1. TOCTOU in IdempotencyLedger::begin_or_replay (Major) — both libSQL and Postgres ledgers used SELECT-then-INSERT, racing the UNIQUE constraint on concurrent webhook retries. Both switched to INSERT-first patterns (libSQL catches SqliteFailure(2067), Postgres uses ON CONFLICT DO NOTHING RETURNING). New concurrent regression test spawns 8 racing callers; exactly one wins New, rest surface as Transient. Bonus: fixed the same wrong-error-code bug in binding_libsql.rs which was matching code 19 (primary SQLITE_CONSTRAINT) when libsql 0.6 actually surfaces 2067 (extended SQLITE_CONSTRAINT_UNIQUE); the existing concurrent handler was silently never firing. 3. bot_token / webhook_secret lifecycle (Major) — wrapped in secrecy::SecretString in HostConfig so they zeroize on drop and accidental Debug prints reveal [REDACTED]. Residual exposure inside StaticCredentialResolver / SharedSecretHeaderAuth documented inline; full fix requires re-reading through EgressCredentialResolver, flagged as follow-up. 5. parse_phase/phase_to_str duplicated between ledger files (Minor) — extracted into crates/ironclaw_product_workflow_storage/src/phase.rs with roundtrip + reject tests. 11. with_base_url_for_test was #[doc(hidden)] but not compile-gated (Minor) — added a `test-support` feature; the helper now physically does not exist in release builds without it. Items 2, 6, 7, 10 (ProductChannel-related) made moot by removing the in-process bridge entirely. Diff shape ---------- V1 source tree: 22 files changed, 60 insertions, 2810 deletions — net subtraction. Removed src/channels/reborn/ (7 files), the register_reborn_channels call in main.rs, the reborn_telegram_v2_enabled config field + parser, validate_telegram_v1_v2_exclusivity + all its tests, the v1/v2 hot-activation guard in ExtensionManager + 3 tests, the V28 Postgres migration, the V26 libSQL migration entry + 2 tests, and 9 optional Reborn workspace deps. New crate: 12 files. Owns its own migrations (no entry in v1's migration set), boot path, config (env-driven, no shared Config type with v1), webhook router, composition root, stubbed inbound turn service, and e2e tests. Verification ------------ cargo check # clean cargo check --no-default-features --features libsql # clean cargo check --all-features # clean cargo build -p ironclaw_reborn_telegram_v2_host --bin ironclaw-reborn-telegram-host # clean cargo clippy --all --tests --benches --examples --all-features # zero warnings cargo deny check # advisories/bans/licenses/sources ok cargo fmt --all -- --check # clean cargo test -p ironclaw_product_workflow_storage --features libsql --lib # 16/16 cargo test -p ironclaw_reborn_telegram_v2_host # 5/5 e2e cargo test --lib # 4951/4952 (1 pre-existing # Postgres-connection # failure, unrelated) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Reviewed as paranoid architect. Approving with one non-blocking defense-in-depth note:
StageCheckpointPayloadRequest currently derives Debug, Serialize, and Deserialize while exposing raw payload: Vec<u8>, whereas PutCheckpointStateRequest keeps payload private and custom-redacts Debug. Since checkpoint bytes are intended to remain host-owned/internal, consider making payload private, adding constructor/accessor validation, custom redacted Debug, and reconsidering serde derives.
No blocking correctness/security findings found.
…, NoProgressDetected (iter 4, approved)
… on rebase)
- Split ControlStrategyState into StopStrategyState + GateStrategyState
(slots.rs); LoopExecutionState now carries both as independent slots.
- Add LoopFailureKind::PolicyDenied with snake_case serde + #[non_exhaustive]
on the enum; downstream matchers in reborn updated to handle the
non-exhaustive shape with a fail-closed wildcard.
- JCS RFC 8785 canonicalization for CapabilityCallSignature::from_call via
the serde_jcs crate; from_call is now fallible (returns Result) and
rejects non-finite numbers (NaN/Infinity) via an explicit guard.
- LoopExecutionState::from_checkpoint_payload signature flipped from
&serde_json::Value to (&[u8], kind: CheckpointKind); envelope carries
schema_id + kind metadata so the boundary the checkpoint was taken at is
authenticated on resume.
- LoopContextMessage.message_ref is now Option<LoopMessageRef>; None means
"summary-only entry; prompt port MUST NOT resolve content from
safe_summary." Call sites in loop_support, reborn, and tests updated to
wrap in Some(...) on writes and filter_map on reads.
- LoopCheckpointPort: removed the premature load_checkpoint_payload stub
(WS-10 owns it); added stage_checkpoint_payload(
StageCheckpointPayloadRequest { schema_id, payload }
) -> LoopCheckpointStateRef with a fail-closed default impl.
- ConcurrencyHint { SafeForParallel, Exclusive } added to
ironclaw_turns::run_profile::host; CapabilityDescriptorView gains a
concurrency_hint field. All struct-literal constructors updated
(defaulting to Exclusive until WS-9 derives from CapabilityDescriptor.effects).
- Tests: JCS-stable across pretty/minified, nested-shuffled, key-reordering;
grep-style assertion that LoopExecutionState has no control_state;
StopStrategyState / GateStrategyState default round-trip; PolicyDenied
serializes as "policy_denied"; checkpoint kind-mismatch path.
- Cargo.toml: add serde_jcs + blake3 deps; drop siphasher (the hand-rolled
canonicalization is replaced wholesale).
Rebased onto docs HEAD 93f0865 to incorporate:
ebd2dc9 ca648b3 49d1506 2b20998 4c12192 1f808fa e48a584 93f0865
…xes) Address two P2 findings from codex review: - StageCheckpointPayloadRequest now carries LoopCheckpointKind so adapters can bridge to CheckpointStateStore::put_checkpoint_state without guessing. - HostManagedLoopCheckpointPort and RebornLoopDriverHost both implement stage_checkpoint_payload; the trait's default Unavailable body remains as defense-in-depth.
…text (codex iter 2 fixes)
Three findings from codex review:
- from_checkpoint_payload now reads raw state bytes (matches the
staging contract); metadata stays out of the payload.
- stage_checkpoint_payload returns a run-scoped LoopCheckpointStateRef
(checkpoint:{run_id}:{token}); is_for_run validators no longer reject.
- HostManagedLoopPromptPort materializes summary-only LoopContextMessage
entries from safe_summary instead of dropping them via filter_map.
SummaryReviewed WS0 PR #3550 only. Highest risk: prompt/model surface-version validation bypass. Merge stance: block until fixed. Tests run:
Findings
Security/data-flow notes
Correctness/invariant notes
Missing tests
Suggested fixes
|
Review notes — WS0 foundationThe contract surface here is load-bearing for the rest of the stack (WS1–WS17). The shape is solid; flagging four forward-compat items worth doing before downstream PRs lock the shape in. Forward-compat hardening
|
|
Addressed the surface-version validation finding in d299ca3. The Reborn model gateway now rebuilds the host prompt bundle with |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d299ca3ec1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .build_prompt_bundle(LoopPromptBundleRequest { | ||
| mode: PromptMode::TextOnly, | ||
| context_cursor: None, | ||
| surface_version: request.surface_version.clone(), | ||
| checkpoint_state_ref: None, |
There was a problem hiding this comment.
This path forwards request.surface_version into build_prompt_bundle without configuring a current-surface lookup on HostManagedLoopPromptPort, so any non-None surface version is rejected as InvalidInvocation before provider execution. In HostManagedLoopPromptPort::validate_request (crates/ironclaw_turns/src/run_profile/prompt.rs, around lines 168-179), a Some(surface_version) requires current_surface_version to be set; this gateway never sets it, so valid host-issued surface versions will be blocked.
Useful? React with 👍 / 👎.
| self.lock_state()?.latest_by_run.insert( | ||
| context.run_id.to_string(), |
There was a problem hiding this comment.
The shared authority is process-global (OnceLock) and stores full prompt grants per run in latest_by_run, but entries are only removed by authorize_latest_model_request. If a prompt is built and the run exits before model authorization (e.g., cancellation/error path), that grant remains for the process lifetime, causing unbounded growth of retained message vectors over time. This should be bounded or explicitly cleaned up on run end.
Useful? React with 👍 / 👎.
… NoProgressDetected (nearai#3550) * arch: ws-0 — state, checkpoints, BoundedRing, CapabilityCallSignature, NoProgressDetected (iter 4, approved) * arch(ws-0): align state + checkpoints with evolved spec (12 gap-fixes on rebase) - Split ControlStrategyState into StopStrategyState + GateStrategyState (slots.rs); LoopExecutionState now carries both as independent slots. - Add LoopFailureKind::PolicyDenied with snake_case serde + #[non_exhaustive] on the enum; downstream matchers in reborn updated to handle the non-exhaustive shape with a fail-closed wildcard. - JCS RFC 8785 canonicalization for CapabilityCallSignature::from_call via the serde_jcs crate; from_call is now fallible (returns Result) and rejects non-finite numbers (NaN/Infinity) via an explicit guard. - LoopExecutionState::from_checkpoint_payload signature flipped from &serde_json::Value to (&[u8], kind: CheckpointKind); envelope carries schema_id + kind metadata so the boundary the checkpoint was taken at is authenticated on resume. - LoopContextMessage.message_ref is now Option<LoopMessageRef>; None means "summary-only entry; prompt port MUST NOT resolve content from safe_summary." Call sites in loop_support, reborn, and tests updated to wrap in Some(...) on writes and filter_map on reads. - LoopCheckpointPort: removed the premature load_checkpoint_payload stub (WS-10 owns it); added stage_checkpoint_payload( StageCheckpointPayloadRequest { schema_id, payload } ) -> LoopCheckpointStateRef with a fail-closed default impl. - ConcurrencyHint { SafeForParallel, Exclusive } added to ironclaw_turns::run_profile::host; CapabilityDescriptorView gains a concurrency_hint field. All struct-literal constructors updated (defaulting to Exclusive until WS-9 derives from CapabilityDescriptor.effects). - Tests: JCS-stable across pretty/minified, nested-shuffled, key-reordering; grep-style assertion that LoopExecutionState has no control_state; StopStrategyState / GateStrategyState default round-trip; PolicyDenied serializes as "policy_denied"; checkpoint kind-mismatch path. - Cargo.toml: add serde_jcs + blake3 deps; drop siphasher (the hand-rolled canonicalization is replaced wholesale). Rebased onto docs HEAD 878a119 to incorporate: 341c8ab edbc72e c945926 6f3a750 81c429a b4971df ef839f2 878a119 * arch(ws-0): wire stage_checkpoint_payload end-to-end (codex iter 1 fixes) Address two P2 findings from codex review: - StageCheckpointPayloadRequest now carries LoopCheckpointKind so adapters can bridge to CheckpointStateStore::put_checkpoint_state without guessing. - HostManagedLoopCheckpointPort and RebornLoopDriverHost both implement stage_checkpoint_payload; the trait's default Unavailable body remains as defense-in-depth. * arch(ws-0): align checkpoint contracts + materialize summary-only context (codex iter 2 fixes) Three findings from codex review: - from_checkpoint_payload now reads raw state bytes (matches the staging contract); metadata stays out of the payload. - stage_checkpoint_payload returns a run-scoped LoopCheckpointStateRef (checkpoint:{run_id}:{token}); is_for_run validators no longer reject. - HostManagedLoopPromptPort materializes summary-only LoopContextMessage entries from safe_summary instead of dropping them via filter_map. * fix(ws-0): bind model requests to prompt bundles * fix(ws-0): issue prompt authority in reborn callers * fix(ws-0): address review feedback
3 participants
Context
Foundation workstream for the Reborn agent-loop framework. This branch establishes the new
ironclaw_agent_loopcrate and extendsironclaw_turnswith the minimal host/request surface the later strategy, executor, and runtime branches need.Master spec:
docs/reborn/agent-loop-skeleton.mdWorkstream brief:
docs/reborn/agent-loop-briefs/state-and-checkpoints.mdStack base:
reborn-integrationLatest stack maintenance on 2026-05-14:
origin/reborn-integrationthrough WS17 before publishing the PR descriptions.What landed
crates/ironclaw_agent_loopcrate with the value-immutableLoopExecutionStatemodel, per-strategy state slots, checkpoint marker/types, and crate-level ownership guardrails.BoundedRing<T, N>with a deserialization guard that rejects over-capacity checkpoint payloads instead of rehydrating invalid state.CapabilityCallSignatureand stable argument hashing for no-progress detection without storing raw capability arguments.LoopFailureKind::NoProgressDetectedandLoopFailureKind::PolicyDeniedplus sanitized failure mapping.LoopPromptBundleRequest.inline_messages,LoopInlineMessage, optionalLoopContextMessage.message_ref, and prompt-port support for summary-only context rows.LoopCheckpointPort, with run-scoped state refs and schema/kind validation boundaries.ironclaw_turnsand the Reborn host adapter.Reviewer focus
BoundedRingserialization/deserialization invariants.ironclaw_agent_loopdefines loop state and framework types, while runner/host execution still belongs to downstream branches.Non-goals / deferred work
Validation
cargo check -p ironclaw_turns -p ironclaw_loop_support -p ironclaw_reborncargo test -p ironclaw_turns host_managed_prompt_port --libcargo test -p ironclaw_reborn --test loop_driver_host text_only_hostcargo test -p ironclaw_reborn --features root-llm-provider --test llm_gatewaygit diff --checkStack position