arch(ws-6): canonical executor — strategy dispatch + checkpoint contract - #3586
henrypark133 wants to merge 15 commits into
Conversation
…, NoProgressDetected (iter 4, approved)
# Conflicts: # crates/ironclaw_agent_loop/src/lib.rs # crates/ironclaw_agent_loop/src/strategies/mod.rs
# Conflicts: # crates/ironclaw_agent_loop/src/lib.rs # crates/ironclaw_agent_loop/src/strategies/mod.rs
…ate, recovery, stop, drain, budget) (iter 1, approved)
…ner) (iter 2, approved)
# Conflicts: # crates/ironclaw_agent_loop/Cargo.toml # crates/ironclaw_agent_loop/src/lib.rs # crates/ironclaw_agent_loop/src/strategies/mod.rs
…ontract (iter 9, approved)
There was a problem hiding this comment.
Code Review
This pull request introduces persistent wall-clock budget enforcement by adding a start timestamp to the loop execution state and defining a new WallClockLimit failure kind. It also implements per-capability concurrency controls (Exclusive vs. SafeForParallel) and per-profile checkpoint size limits, raising the absolute system ceiling to 256 KiB. A potential integer truncation bug was identified on 32-bit platforms within the checkpoint size validation logic.
| ) -> Result<(), String> { | ||
| validate_checkpoint_payload_len(len)?; | ||
| if let Some(cap) = profile_cap { | ||
| let effective_cap = (cap as usize).min(MAX_CHECKPOINT_STATE_PAYLOAD_BYTES); |
There was a problem hiding this comment.
On 32-bit platforms, casting a u64 to usize before performing the min comparison can lead to incorrect truncation if the profile-declared cap exceeds u32::MAX. Since MAX_CHECKPOINT_STATE_PAYLOAD_BYTES is a small constant (256 KiB), it is safer to perform the comparison using u64 types and then cast the result to usize.
| let effective_cap = (cap as usize).min(MAX_CHECKPOINT_STATE_PAYLOAD_BYTES); | |
| let effective_cap = cap.min(MAX_CHECKPOINT_STATE_PAYLOAD_BYTES as u64) as usize; |
…ost + executor tests
…advance on ack failure
Two final codex /review findings on the WS-6 executor. [P1] Bound StaleSurface reloads — `executor/canonical.rs:117-125`. The canonical loop's `ReloadSurface` branch cleared `surface_version` and continued without consuming iteration budget or consulting recovery, so a buggy host that always reports `StaleSurface` could spin forever inside one tick (iteration_limit, wall_clock_limit, and no-progress detection never observe a fresh tick). Add `MAX_STALE_SURFACE_RELOADS_PER_ITERATION = 3` and a per-iteration counter. On cap, synthesize a `Transient` `ModelErrorSummary` and route through `RecoveryStrategy::on_model_error`: `Retry`/`SkipResult` advance the iteration so outer caps eventually trip, `Abort` exits with `Final` checkpoint via the existing helper. [P2] Honor `Parallel` batch policy — `loop_driver_host.rs:691-704`. The executor computed a `BatchPolicy` but never forwarded it to the host, making `CapabilityConcurrency`/batch-policy a no-op. Add `BatchExecutionPolicy` to `CapabilityBatchInvocation` with `#[serde(default = Sequential)]` for wire compat, map `BatchPolicy → BatchExecutionPolicy` at the executor boundary, and implement the `Parallel` arm via `futures_util::future::join_all` in the host. Parallel is honored only when `stop_on_first_suspension = false`; today the executor always sets `true`, so the parallel branch becomes live when WS-7+ flips a profile. Cancellation across concurrent in-flight calls on first-suspension would require a `JoinSet`-with-abort pattern that hasn't shipped yet — keeping that case on the serial path is strictly better than today's "always serial" state. Smoke tests: - `unbounded_stale_surface_reloads_terminate_via_recovery` (agent_loop): MockHost returning StaleSurface every model call; executor exits Failed within a bounded number of calls. - `sequential_policy_forwards_to_host_when_any_descriptor_exclusive` (agent_loop): planner's `Sequential` reaches the host as `BatchExecutionPolicy::Sequential`. - `parallel_policy_with_all_safe_summaries_stops_on_suspension` (agent_loop, augmented): also asserts `policy = Parallel` reaches the host. - `host_accepts_parallel_batch_policy_and_returns_outcomes_in_order` (reborn): host accepts `Parallel`, processes all invocations, returns outcomes in original order. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
0b43ead to
2072ba5
Compare
|
Superseded by the pushed skeleton continuation stack: arch/ws-6a (canonical executor), arch/ws-7 (planned driver), and arch/ws-8 (integration suite). The monolithic arch/ws-6 branch is no longer the review path. |
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>
Context
The canonical agent-loop executor — the heart of the Reborn agent-loop
framework. Master spec: `docs/reborn/agent-loop-skeleton.md` §8 (executor
algorithm), §10 (safety-net + durability). Brief:
`docs/reborn/agent-loop-briefs/canonical-executor.md`.
This is #9 of the 11-workstream cascade, the last one in the skeleton.
Everything above (state, strategy traits, planner facade, defaults) was
foundation; this PR drives the actual iteration loop.
What landed
New file `crates/ironclaw_agent_loop/src/executor.rs` (~1800 production lines + ~2700 test lines):
loop: pulls strategies from the planner; orders each tick as
iteration cap → cancellation observe → drain steering → context →
surface → BeforeModel checkpoint → stream model → branch reply vs
capability calls → BeforeSideEffect → BeforeBlock for gates → Final.
`CheckpointFailed`).
wall-clock budget (durable across resume), per-iteration capability
signature dedup, no-progress detection (3 distinct iterations within
window of 5), terminate-hint stop, retry budget, gate blocking.
iter-cycle regression locks.
Minimal trait extensions on `ironclaw_turns`:
— legacy hosts fall through to the existing `checkpoint()`-only path
via `LoopCheckpointStateRef::legacy_unknown()`).
(`SafeForParallel`/`Exclusive`, `#[serde(default)] = Exclusive` for
legacy payload safety).
ceiling (profile cap honored over legacy default).
`LoopExecutionState` extension:
persisted to checkpoint payload so wall-clock budget survives resume).
`ironclaw_reborn` host adapter changes:
via `CheckpointStateStore::put_checkpoint_state(.with_max_payload_bytes(...))`,
returning the allocated state ref the executor cites in `checkpoint()`.
effects (`WriteFilesystem`, `SpawnProcess`, `ApproveRequired`, etc.) +
`default_permission == Ask` to `Exclusive`; else `SafeForParallel`.
Signature deviation from the brief
The brief sketches `execute_family(family: LoopFamily, host, state)` with
a sealed-strategy invariant via `AgentLoopPlannerInternal`. WS-4 didn't
ship `LoopFamily` (the realized planner is `AgentLoopPlanner` + `DefaultPlanner`),
so the executor signature is:
```rust
async fn execute(
&self,
planner: &dyn AgentLoopPlanner,
host: &dyn AgentLoopDriverHost,
state: &mut LoopExecutionState,
) -> Result<LoopExit, AgentLoopExecutorError>;
```
WS-7 (Planned Driver Adapter) should target this signature — not the
brief's hypothetical `execute_family(...)`. Object-safety + planner-only
access to strategies are preserved.
Review history
This branch went through 9 codex /review iterations. Each iter found
a handful of edge-case correctness issues; a sonnet sub-agent fixed them
and the next /review run verified. The trend:
Final sonnet spec-conformance review: all brief acceptance criteria
implemented; every addition traces to either the brief, a master-spec
section, or a real review finding; no scope creep.
Test plan
PR cascade
```
#3550 ws-0
└── #3554 level1-merged
└── #3557 level2-merged
└── #TBD ws-6 (this PR — canonical executor)
└── ws-7 (Planned Driver Adapter) — next workstream
```
🤖 Generated with Claude Code