docs(reborn): agent loop skeleton framework spec + 9 workstream briefs - #3544
Conversation
Introduces the design for a strategy-composition agent-loop framework (`ironclaw_agent_loop` crate) that sits above `ironclaw_turns`' runner-facing contracts. Defines `AgentLoopPlanner` as a composition of nine swappable strategies, `CanonicalAgentLoopExecutor` as the canonical tick body, and `PlannedDriver` as the runner-facing adapter. Default behavior models pi-mono mechanics with three production-safe escape nets (iteration cap, retry budget, no-progress detection). Ships docs only — no code. Trait scaffolding lands in WS-0; full sequencing is master doc §13. Master spec: docs/reborn/agent-loop-skeleton.md Eight per-workstream briefs: docs/reborn/agent-loop-briefs/ Six follow-up workstreams (WS-9..WS-14) for end-to-end wiring documented in master doc §12. Reviews completed: Codex review + 3 Explore-subagent passes (architecture soundness, complexity audit, crate boundaries + CLAUDE.md coverage). All P0/P1 feedback applied. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces the core framework for the Reborn agent loop, including the AgentLoopExecutor trait, its canonical implementation, the AgentLoopPlanner facade, and the necessary strategy traits and state management structures. The review identified several critical issues in the canonical executor's logic, including flawed batch result summary calculation, incorrect iteration limit check placement, missing per-iteration signature deduplication, and incorrect failure kind tracking during retries. Additionally, there were concerns regarding the use of mutable references in helper functions and the lack of assistant reply content in the turn summary. I have filtered out comments that did not provide actionable feedback or were purely validation-oriented.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a79b48fdc2
ℹ️ 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".
…ature mentions Three small spec consistency fixes flagged by /review on PR #3544: - e2e-integration-tests.md: rename `InvokeCapabilitySingle` enum variant in MockHostCall to `InvokeCapability` to match the actual host method name (we reuse the existing single-call API, not a new method). - agent-loop-skeleton.md §10/§11: align `iteration_limit()` mentions with WS-3's signature `iteration_limit(&state)`. - strategy-traits-gamma.md: doc-comment for BudgetStrategy was using `>` (off-by-one) and `iteration_limit()` (wrong arity); both fixed to match the canonical executor's `>=` semantics + actual signature. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses 8 real spec issues flagged by automated review of the docs PR. Six fixes in canonical-executor.md (master doc §8 mirrored): - Reply branch now finalizes BEFORE consulting the stop strategy. The prior shape only finalized on GracefulStop, so the default-strategy Continue→Completed and NoProgressDetected→Failed paths returned without ever calling finalize_assistant_message — LoopExit validation rejects non-NoReply Completed without reply_message_refs, so a plain successful reply was silently lost. (codex P1) - CapabilityCalls match now handles `Denied(reason)` and `SpawnedProcess(handle)` outcomes. EmptyLoopCapabilityPort returns Denied today, so without the Denied arm any model-issued tool call would hit an unreachable match. SpawnedProcess gets BeforeBlock checkpoint + Blocked exit. (codex P1) - Per-iteration HashSet wraps every push to recent_call_signatures so the dedup contract (master doc §10 + WS-0 §3.4) is actually enforced in code, not just documentation. Without this, three identical calls in a single batch trip NoProgressDetected immediately. (gemini high, codex P1) - recent_failure_kinds.push moved out of the inner retry loop so a single failing call retried 3× no longer satisfies the failure-run- length escape (which checks for 3 identical failures *across iterations*). (gemini high) - Iteration cap check moved to the TOP of the loop body so a resumed executor with state.iteration == limit exits immediately instead of running one extra body. (gemini high) - batch_result_refs computed from a snapshot index taken before invoking the batch, not by slicing `last_batch_total` calls from the tail of state.result_refs. The old shape over-included refs from prior iterations whenever this batch had any non-completing outcome (Skip/Block/Failed-with-no-retry). (gemini high) Two medium fixes in canonical-executor.md: - summary_of(call) now receives &surface so concurrency hints can be read from the visible-capability descriptors. (gemini medium) - drain_followup_into returns (LoopExecutionState, bool) instead of taking &mut LoopExecutionState — honors the value-immutable contract in master doc §8 property 3. drain_steering_into already had the right shape; both helpers now also filter LoopInputPort batches to user-facing message kinds (UserMessage), leaving control kinds (Cancel, Interrupt, GateResolved, CapabilitySurfaceChanged) for dedicated executor paths. (gemini medium, codex P2 partial) One fix in planned-driver-adapter.md: - validate_resume_request reads checkpoint_schema_id from request.resolved_run_profile.loop_driver descriptor instead of a fictional request.checkpoint_schema_id field that doesn't exist on AgentLoopDriverResumeRequest. (codex P2) One design clarification in strategy-traits-gamma.md: - TurnSummary doc-comment explains why reply CONTENT is intentionally not included — strategies access content via assistant_message_ref through the host port (per turns-agent-loop.md §6, refs only in loop state). Pi-mono passes content because it has no host abstraction; Reborn maintains the trust-boundary by routing content reads through the host. (gemini medium — design choice clarified, not changed) Already addressed in earlier commits: iteration_limit() arity (codex P2), InvokeCapabilitySingle naming (review pass). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ty wiring Adds two pre-written follow-up workstream briefs covering the gaps left by the skeleton PR around how identity-style content (AGENTS.md, SOUL.md, etc.) and profile-scoped capability access flow into the agent loop: - WS-15 (prompt-context-assembly.md): designs `HostIdentityContextSource` to populate `LoopContextBundle.identity_messages`. Stable vs. volatile classification keeps the identity prefix byte-stable for Anthropic prompt caching; HEARTBEAT.md routes to `instruction_snippets` post-cache boundary. Per-run `Arc<OnceLock<...>>` cache + deterministic ordering contract. - WS-9 (capability-host-wiring.md): designs `HostRuntimeLoopCapabilityPort` wrapping `CapabilityHost`, plus `CapabilitySurfaceProfileFilter` decorator gating the surface by a `CapabilityAllowSet` snapshot resolved from `ResolvedRunProfile.capability_surface_profile_id`. Allowset frozen per run (master doc §5 layer-1). Batch partition truncates on inner suspension rather than padding; `stopped_on_suspension` propagated verbatim. - Master doc §6 / §11 / §12: cross-link both briefs; new §6 paragraphs clarify that strategies pick request shape only — host-side ports own file-to-prompt assembly and surface materialization. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Overall I like this design direction. The runner/driver/executor/planner split, host-owned side effects, fail-closed capability posture, and value-immutable loop state all feel like the right shape for Reborn. Before implementation starts, I think the spec should tighten a few correctness/security seams:
After those fixes, I’d be comfortable with the overall architecture. One thing I’d like to open for discussion: simplicity. Nine strategies are very extensible, but also a lot of surface area. Maybe that is right for the framework crate, but we may want stronger presets or grouped policies ( |
zmanian
left a comment
There was a problem hiding this comment.
Review
Read the master doc top-to-bottom. Substantive design with good bones — the strategy/executor/planner split, value-immutable state model, executor-owned checkpoint discipline, and three-layer safety net (iteration cap + retry budget + no-progress detection) are all the right shape. serrrfirat already covered the local correctness items (identity trust attenuation, identity caching key, checkpoint payload flow, model recovery routing, capability denial telemetry, cancellation boundaries, parallel batch policy hints) — won't repeat those.
What I want to add — the structural gaps that aren't yet on the list:
1. Strategy ↔ Hook coordination is unspecified
This is the biggest structural seam. The strategy framework in this PR and the hooks framework in #3524 / the design comment at #3524 (comment) have substantial overlap:
| Concern | Strategy in this PR | Hook in #3524 |
|---|---|---|
| Deny / pause a capability invocation | GateHandlingStrategy returning GateOutcome |
before_capability Gate hook returning Deny/PauseApproval |
| Mutate prompt context | ContextStrategy.plan_context_request (picks mode + inline_messages) |
before_prompt / before_context Mutator hook with HookPatch::AddSnippet |
| Stop the loop | StopConditionStrategy |
Observer hooks at after_model see the same signal |
| Retry/skip on capability error | RecoveryStrategy.on_capability_error |
before_capability Gate hook can deny-then-let-recovery-classify |
Two parallel decision frameworks with overlapping concerns is exactly the "loop crate as junk drawer" hazard serrrfirat called out. The doc needs to say explicitly where hooks fire relative to strategies — three plausible orderings, only one is right:
- Hook around port (middleware on
LoopCapabilityPort): hook fires before strategy sees the outcome.GateHandlingStrategysees only post-hook outcomes; an Installed hook can override a strategy's intent. - Hook inside strategy (strategy calls hook dispatcher): strategy stays in control; hook is a delegation point. But every strategy author must remember to call hooks.
- Strategy-as-Builtin-hook (strategies are the Builtin tier of the hook primitive, with Trusted/Installed hooks composed on top): unified mental model; strategies become the highest-trust tier of one machinery.
My take: the third. The strategy machinery and the hooks machinery should be one machinery with trust tiers. Otherwise the spec ships two systems that solve the same problems with different vocabularies, and every future loop family has to learn both. Worth a section in the master doc — or an explicit pointer to #3524 — saying what the relationship is.
2. Trust class for strategies themselves
The doc never says what trust class a strategy belongs to. "Loop families ship as factory functions in ironclaw_agent_loop/src/families/" implies Builtin-only. But the architecture allows arbitrary Arc<dyn ContextStrategy> to be wired into DefaultPlanner — nothing structural prevents a Trusted or Installed extension from shipping its own strategy.
This intersects with the prompt-injection envelope work in #3471 / #3540 directly: a ContextStrategy returning a LoopPromptBundleRequest with inline_messages populated from extension-author code is a clean prompt-injection channel that bypasses the envelope helper. Same hazard as the original #3540 concern but at a different layer.
Either:
- Lock strategies to Builtin trust at the type level (sealed trait, only
ironclaw_agent_loop::families::*constructors), or - Apply the trust-tier taxonomy from the hooks design and define what Trusted/Installed strategies can and cannot do.
The doc doesn't pick. It should.
3. The skeleton ships an identity_messages-empty path
§11 acknowledges WS-15 (identity-file surface) is deferred. That means the skeleton ships with the LoopContextBundle.identity_messages = Vec::new() state — the same dangling-contract concern from PR #3476 and follow-up issue #3547. Skills appear in the prompt before identity until WS-15 lands.
The spec should be explicit that PlannedDriver must not be registered in production-resolvable run profiles until WS-15 ships. Otherwise WS-14 (driver registration) + WS-9 (capability wiring) becomes "production turns running through the new framework with identity inverted to skill" — which is exactly the #3547 hazard.
Suggested edit to §12 minimum-E2E set: WS-9 + WS-13 + WS-14 + WS-15 (not just the first three).
4. Versioning primitive — reuse one shape across the system
The doc has PlannerId newtype in checkpoint payload metadata for replay. The hooks design has (hook_id, hook_version). The skill-snapshot work in #3470 uses SHA-256 length-prefixed snapshot version. The model-route work in #3462 has auth_version. Four places, four conventions.
The Reborn replay story needs one primitive — something like ComponentIdentity { id: Blake3Digest, version: u64 } — with consistent semantics about what "version" means (content-addressed digest vs monotonic counter — pick one). The skeleton spec is the right place to establish this, since the checkpoint payload is the data plane all of them flow through.
5. Cross-cutting baseline alignment (#3492)
The spec implicitly assumes several #3492 baseline patterns but doesn't surface them:
- Sealed state slots.
LoopExecutionStatefields are shown aspub— readable by strategies, which is fine, butRecoveryStrategyState,ControlStrategyState, etc. need to be constructable only by their owning strategy. Otherwise a misbehaving (or compromised)ContextStrategycan mint aControlStrategyStateclaiming a terminate hint was seen and trickStopConditionStrategyinto stopping. Same #3460 sealing pattern. - Error classification on
LoopFailureKind. The variants in the spec (IterationLimit,NoProgressDetected, original-class for retry-budget exhaustion) don't carry the Transient/Permanent/Misconfigured/PolicyDenied taxonomy. Should align with the work done in #3454, #3462, #3541. Otherwise the same retry-classification gap surfaces. BoundedRing<T, 8>is hardcoded. N=8 is fine for the text-only default, but long-running missions with telemetry/heartbeat capabilities will saturate the ring with noise. Worth surfacing as a config onDefaultStopConditionStrategyso families can tune it.
6. CapabilityCallSignature JSON canonicalization
ArgsHash is over "canonicalized JSON args" but the canonicalization scheme is unspecified. JCS RFC 8785 is the only formal spec and has limited Rust implementation quality. Worth nailing down: which canonicalizer, what about float NaN/Inf, what about map key ordering for non-string keys. Two implementations diverging on canonicalization means the same logical call from two driver versions hash differently and the no-progress detector misfires across upgrades.
Verdict
Approve in principle, with one structural ask before WS-0 lands: resolve the strategy ↔ hook relationship explicitly in the master doc. Everything else above can land as iterative refinements during the workstreams. But shipping two overlapping decision frameworks with undefined coordination is the kind of thing that's much cheaper to nail down in the spec than to retrofit across six implementation PRs.
zmanian
left a comment
There was a problem hiding this comment.
Follow-up: going deeper after sitting with the spec
The prior review (#3544 (review) — strategy↔hook coordination, trust class, identity messages, versioning primitive, baseline alignment, JSON canonicalization) covered the surface. A few structural things I want to add after more time with the doc.
1. The spec's scope claim may be too broad
The framing positions this as "the framework for all future loop families" — routines, missions, general assistant, coding, planning. Look at those families more carefully:
- Routines are scheduled polls of a fixed capability set; they want repetition (the no-progress detector is the wrong default); they want indefinite runtime, not iteration caps.
- Missions are multi-day plans with first-class checkpoint→approval→resume; that's not a
GateHandlingStrategy::Blockexit followed by a fresh claim, it's a fundamentally different control flow. - Coding wants edit-then-test loops with language-specific tools; not a strategy choice.
- Planning wants multi-turn search with backtracking and state revival; not a linear
iteration++model.
The factoring axis the spec picks ("nine swappable strategies, same executor") is right for the text-tool-use family. It's the wrong axis for routines/missions/planning, which need different executors, not different strategy compositions. The spec acknowledges this implicitly ("a family graduates to its own crate only when it pulls heavyweight external deps") but I think the right factoring axis isn't "heavyweight deps" — it's "control flow shape."
Recommendation: narrow the scope claim in §1 to "framework for text-tool-use loop families" and note that routines/missions/planning likely need sibling frameworks (or distinct AgentLoopDriver impls that share helpers but not the executor). Otherwise WS-1–WS-8 produces a generic-looking framework that won't actually generalize, and we end up with three forks.
2. The missing top layer — profile → family → strategy resolution
The four mutability layers in §5 (run context, execution state, run state, durable backends) miss the profile layer above them. ResolvedRunProfile.capability_surface_profile_id is mentioned in §6 but the doc never says how the profile selects a loop family.
This is the layer where trust lives, and pinning it answers the trust-class question I raised in the prior review:
RunProfile (user-grantable, audited, channel-to-user ratified)
│
▼
LoopFamily (Builtin, factory function, type-sealed)
│
▼
nine Strategy slots (implementation detail of the family)
If resolution is forced through this chain, strategies don't need their own trust class because they're never directly user-configurable. Trust is enforced at the profile layer; strategy composition is private to family factories.
Concrete asks:
LoopFamilybecomes an explicit type (pub struct LoopFamily { id: LoopFamilyId, planner: Box<dyn AgentLoopPlanner>, ... }) withpub(crate)constructors.- Profile selects family by
LoopFamilyId; cannot enumerate strategies directly. - Strategy traits become
pub(crate)inironclaw_agent_loop; only the families module can compose them.
This collapses several open questions: the trust-class problem (prior review §2), the combinatorial-explosion problem (a profile gets a LoopFamily, not 9 independent knobs), and the version-drift problem (LoopFamilyId + content-addressed version pins replayable state).
3. The state model is mid-iteration mutable, with checkpoint implications
The banner property is "value-immutable, rebound per tick." But the retry loop in §8 does in-place mutation:
Retry { recovery, alter }:
state.recovery_state = recovery;
honor_alteration(alter)
retry_outcome = host.invoke_capability(call)
The value-immutable invariant only holds at iteration boundaries, not within a tick. Concrete consequence for checkpoint correctness: the four checkpoint kinds (BeforeModel, BeforeSideEffect, BeforeBlock, Final) are placed at iteration-significant moments, but retry attempts happen between checkpoints. If the process crashes between attempt 1 and attempt 2, the retry counter is lost — on resume, the recovery strategy sees recovery_state.attempts = 0 and the bounded-retry guarantee is silently violated.
Two fixes, spec should pick one explicitly:
- Re-derive on resume from the
recent_failure_kindsring (which is checkpointed). Cheap if the ring carries enough info. - Checkpoint at every recovery-state transition. Expensive but durable.
Today the spec is silent — which means the framework will work in tests and have subtle retry-budget violations in production.
4. LoopExit should be a witness type at the framework→reborn boundary
PR #3460 sealed LoopExitValidationPolicy so only LoopExitApplier can mint trusted policies. The same pattern should apply to LoopExit itself when it crosses the framework boundary:
ironclaw_agent_loop → UnvalidatedLoopExit
ironclaw_reborn → LoopExitApplier::validate(UnvalidatedLoopExit) → ValidatedLoopExit
ironclaw_turns → TurnRunner accepts only ValidatedLoopExit
Right now the spec relies on convention ("LoopExit from PlannedDriver::run goes through the applier"). A typed boundary makes this unbypassable and compile-time enforceable. Invariants by types, not by convention.
5. Only the iteration cap is structurally unbypassable
§10 names three safety nets but two of them are enforced by strategies (retry budget by RecoveryStrategy, no-progress by StopConditionStrategy). Only the iteration cap is enforced by the executor itself. If strategies are Builtin-only this is fine — they're trusted code. If strategies are ever Trusted/Installed, a malicious one defeats two of the three nets and only iteration cap saves you.
This couples to point 2: if LoopFamily is the type-sealed Builtin-only composition layer, the safety story is fine. If the spec ever wants to allow extension-supplied strategies, the executor itself must enforce no-progress detection independently.
The spec should state this trade-off explicitly. Either "strategies are Builtin-only and that's the trust story" or "extension-supplied strategies need executor-level safety nets" — pick one.
6. Cancellation is cooperative, coupled to trust class
The spec observes cancellation "between every strategy call" — cooperative, not preemptive. A strategy that hangs in an await is uncancelable. Same coupling as point 5: Builtin-only strategies make cooperative fine; Trusted/Installed strategies would require tokio::select! against the cancellation token at every strategy call site.
7. Strategy combinatorial explosion: serrrfirat's grouped-policy hint is right
Nine strategies × ~3 sensible variants = ~20K compositions, most nonsense or unsafe. Concrete coupling examples:
ContextStrategy::TextOnly+CapabilityStrategy::AllVisible= contradiction.BatchPolicyStrategy::Parallel+RecoveryStrategy::SequentialBackoff= contradiction.StopConditionStrategy::IgnoreNoProgress+BudgetStrategy::HighIterationCap= infinite-loop attractor.GateHandlingStrategy::SkipAndContinue+RecoveryStrategy::Abort= inconsistent recovery model.
Grouped policies (PromptPolicy = Context+Model+Drain, EffectPolicy = Capability+Batch+Recovery+Gate, ControlPolicy = Stop+Budget) capture the coupling and collapse the configuration space to ~27 sensible combinations. Worth doing before WS-1/2/3 ship nine separate traits — retrofitting groups across separately-released trait crates is expensive.
This also resolves part of point 1's scope question: with grouped policies, a "loop family" is three policy choices, and the framework can statically reject nonsense compositions.
8. Three things to borrow from NousResearch's Hermes / Forge
After looking at how the open-source agent line solves analogous problems:
a. The Reply | CapabilityCalls parent protocol is too binary. Hermes' ChatML format, like Claude Sonnet 4.x and GPT-5 in practice, supports interleaved output: text → tool call → text → tool call in a single response. The spec's binary ParentLoopOutput is the right target shape but doesn't match what providers actually emit. The framework needs a normalization step at the model boundary that flattens interleaved responses, with explicit semantics for "text emitted before tool calls" (transcript-only? shown to user? part of the next assistant_refs?). Currently underspecified.
b. Wire-format ownership is implicit and matters most for open-weights providers. Hermes 2 Pro/3 use <tool_call> and <tool_response> ChatML tags — the de facto standard for Llama 3.1+/Qwen/DeepSeek/Mistral tool calling. The spec leaves wire format to LoopModelPort impls per provider. That's fine for Anthropic/OpenAI native tool-calling APIs but produces N divergent shapes for Ollama + open-weights backends (which IronClaw already supports via the LLM backend list). Spec should either pin a canonical WireToolCall and require providers to translate, or be explicit that wire format is per-provider.
c. JSON canonicalization has a free answer. Hermes' published function-call format uses sorted-keys JSON with deterministic number formatting. If CapabilityCallSignature::ArgsHash adopts the same convention, the no-progress detector is cross-model consistent — same tool call from Claude and Hermes 3 hash identically, replay across model swaps works. Pick this from Hermes rather than inventing one.
Beyond these: Hermes/Forge proves you can ship a working tool-using loop with ~3 abstractions and 500 LoC. The framework weight IronClaw is paying (4251 spec lines, 9 strategies, 4 layers, 8 workstreams) is only justified by the trust-model work — multi-tenancy, durable audit, profile-driven family selection, envelope-wrapped untrusted content. The framework weight isn't free; it's earned by trust requirements Hermes/Forge doesn't carry. The spec should say this explicitly in §2 ("why this exists"), because absent the trust framing the nine-strategy abstraction is hard to justify against a Forge-shaped alternative.
The clean separation: borrow Hermes' interface shapes (wire format, interleaved-output handling, JSON canonicalization, possibly self-evaluation framing) but not their trust shapes.
Updated verdict
Still approve in principle. The structural ask before WS-0 lands is sharper now: define LoopFamily as the top-layer abstraction, with profile-driven selection, Builtin-only sealed strategy composition, content-addressed family version baked into the checkpoint payload, and grouped policies (not nine independent traits) as the composition shape.
That single change resolves: the trust-class question, the combinatorial-explosion question, the version-drift question on resume, the safety-net-bypass question, and the cooperative-vs-preemptive cancellation question. All of them collapse into "Builtin family is the only thing that composes strategies; extensions don't enter at this layer."
Everything else above can land as iterative refinements during WS-1–WS-8. The strategy↔hook coordination from the prior review is the only other thing I'd nail down before WS-0; the rest can iterate.
Closes the five remaining "scope at pickup" placeholders in the master doc §12 table with full, code-grounded briefs: - WS-10 (checkpoint-store-and-resume): adds `load_checkpoint_payload` to `LoopCheckpointPort`; extends the existing `HostManagedLoopCheckpointPort` with the read path; wires `PlannedDriver::resume`. Adds `LoopFailureKind::CheckpointUnavailable`. - WS-11 (loop-input-port): defines a neutral `HostInputQueue` trait and `HostQueueLoopInputPort` adapter; specifies cursor / ack idempotency contract; control-input variants pass through unfiltered. - WS-12 (loop-progress-port): six additive `LoopProgressEvent` variants + matching `LoopHostMilestoneEmitter` methods; expands the existing `HostManagedLoopProgressPort` match in `loop_driver_host.rs` and the `DurableLoopHostMilestoneSink` projection in `milestone_events.rs`. - WS-13 (host-cancellation-accessor): new `LoopCancellationPort` trait added to the `AgentLoopDriverHost` supertrait list; sync snapshot semantics + idempotent reads; `RunStateLoopCancellationPort` adapter backed by an `AtomicBool` + signal-payload handle. Honors `validate_cancelled_exit`'s `require_final_checkpoint` policy. - WS-14 (planned-driver-registration): `default_planned_driver()`, registration helpers with explicit `RequirementLevel::Required` for the seven `DriverRequirements` dimensions, additive `InMemoryRunProfileRegistry::register` mutator, and a real-host smoke test that hard-gates on WS-9/10/11/12/13. Updates `agent-loop-skeleton.md` §12 table to link each new brief, update crate ownership and Minimum E2E set to include all five parallel siblings, and reflect WS-14's hard-gate. Briefs survived four Codex review passes; final pass found no brief-internal issues (only pre-existing untracked workspace artifacts that are not part of this change). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four high-level architectural decisions from human review (serrrfirat, zmanian) on PR #3544 land as spec amendments before WS-0 seals the trait shapes: 1. LoopFamily as first-class abstraction (zmanian #2.2). New WS-3.5 brief: LoopFamilyId + ComponentIdentity + LoopFamily + LoopFamilyRegistry (Guice-style singleton built once at startup). 2. Strategies sealed pub(crate) (zmanian #1.2, #2.5, #2.6; serrrfirat #1). AgentLoopPlanner is pub but uses sealed-trait pattern; strategy access lives on pub(crate) AgentLoopPlannerInternal extension trait. 3. Hooks as middleware (zmanian #1.1). Adopt the four-scenario design in PR #3523-comment-4435808547. Two concrete follow-ups: composition seam architecture test in WS-9's brief; LoopFailureKind::PolicyDenied variant in WS-0. 4. Broad scope retained with stress-test discipline (zmanian #2.1, #2.7; serrrfirat #8). New §12.5 enumerates anticipated families (default + hypothetical routine/mission/coding/planning) and the strategies each would swap; trait shapes accommodate every row. Side effects: - Split ControlStrategyState → StopStrategyState + GateStrategyState in WS-0 so stop and gate strategies grow independently. - Drop generics on PlannedDriver — non-generic { family, executor }. WS-7 + downstream briefs (WS-9, WS-10, WS-11, WS-13, WS-14) updated. - Subsume PlannerId into LoopFamilyId + ComponentIdentity (one versioning primitive per zmanian #1.4, partly addressed). - Executor entry point becomes execute_family(&LoopFamily, host, state). - Document HostXxxContextSource as the pattern for family-specific durable context (mirrors WS-15's HostIdentityContextSource). 15 files changed (1 new brief: loop-family-registry.md). Spec-only; no code changes. Three remaining comment clusters (checkpoint durability + LoopExit witness; versioning + JSON canonicalization; serrrfirat's remaining 7 correctness seams) are noted as follow-up work. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
LoopFamily-cluster review: applied in 49d1506Thanks to @serrrfirat and @zmanian for the substantive reviews. The amendments below land docs only before WS-0 seals trait shapes — locking the load-bearing decisions in spec is the cheap window; the same fixes after WS-1/2/3 ship would mean reshaping This commit addresses one cluster (the LoopFamily / strategy↔hook / trust cluster). Three remaining comment clusters are noted at the bottom and will land as separate amendments. Four high-level decisions captured
Per-reviewer point-by-pointserrrfirat:
zmanian first review:
zmanian follow-up:
Deferred clusters (will land as separate amendments)
The LoopFamily cluster was load-bearing — several of the seams above collapse or simplify once the sealed-Builtin shape is in place. The remaining clusters are independent and can iterate. |
Two follow-up amendments addressing zmanian #2.3 and #2.4 from PR #3544 review (the checkpoint-durability + LoopExit-witness-type cluster): 1. Retry budget durability (#2.3). Add an explicit §10 note that retry budgets are bounded within a single iteration but reset across resumes (a crash mid-retry resumes from the BeforeSideEffect checkpoint with recovery_state.attempts = 0). The iteration cap — the only structural safety net — still bounds total retries across resumes because each resume costs one iteration. Adding an AfterRetryAttempt checkpoint kind would produce checkpoint storms under retry pressure; we prefer being honest about what we structurally guarantee. WS-6's brief gains the matching inline durability note next to the retry loop pseudocode. 2. LoopExit validation as structural enforcement (#2.4). Add a §9 bullet stating that AgentLoopDriver returns raw LoopExit and only LoopExitApplier::validate (sealed via LoopExitValidationPolicy per PR #3460) produces the LoopExitValidationDecision flowing into TurnRunTransitionPort::apply_validated_loop_exit via ApplyValidatedLoopExitRequest. The runner's transition port accepts the validated request, not raw LoopExit. The structural enforcement is already in place via the existing #3460 seal plus ApplyValidatedLoopExitRequest shape; the bullet codifies the invariant in the spec. Spec-only; no code changes. The cluster's remaining work (serrrfirat's 7 correctness seams; versioning + JSON canonicalization) lands as separate follow-up amendments. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Checkpoint durability + LoopExit witness type: applied in 2b20998Two short amendments addressing @zmanian's #2.3 and #2.4 from the follow-up review: #2.3 — retry-budget durability (master doc §10)The concern was that Three options considered: (A) re-derive from Picked (C). Adding a fifth checkpoint kind would produce checkpoint storms under retry pressure; re-deriving from §10 now carries the explicit note. WS-6's brief gains the matching durability comment next to the retry loop pseudocode so implementers see the rationale at the call site. #2.4 —
|
…n PR #3544 Third cluster of follow-up amendments addressing zmanian #1.4 and #1.6/#2.8c from PR #3544 review (versioning + JSON canonicalization). 1. ComponentIdentity as the one versioning primitive (zmanian #1.4). Master doc §9 adds a new bullet stating that ComponentIdentity { id, digest } from WS-3.5 is the canonical shape across loop families, checkpoint payload metadata, hooks (#3524 future), skill snapshots (#3470 future), and model routes (#3462 future). Content-addressed only — monotonic counters false-drift and false-agree, both silent replay-correctness bugs. The existing String identities on LoopModelRouteSnapshot (auth_version, config_version) migrate alongside #3462's model-route work; NOT in this PR. WS-3.5 brief gains a Migration / propagation table enumerating the four components, their current shape, and per- component migration cost. 2. JCS RFC 8785 for ArgsHash canonicalization (zmanian #1.6, #2.8c). Master doc §9 adds a new bullet pinning JCS as the canonicalization scheme. WS-0 brief gains new §3.4a spelling out the rules explicitly: sort object keys by UTF-16 code-unit order, reject NaN/Infinity, preserve number representation, minimal whitespace. Implementation reference: jcs crate. Cross-model compatibility note: for typical tool args (no floats), JCS output is byte- identical to the Hermes/Forge sorted-keys-minimal-whitespace convention used in the open-weights tool-calling ecosystem — replay across model swaps hashes identically for typical args. WS-0 acceptance criteria strengthened: from_call is JCS-stable under key reordering at multiple nesting depths; NaN/Infinity inputs return Err. WS-8 gains a matching args_hash_jcs_stable integration test. Spec-only; no code changes. The skill-snapshot, hook, and model-route migrations to ComponentIdentity land in their owning PRs (#3470, #3524, #3462) — this PR sets the target shape only. Final cluster (serrrfirat's 7 correctness seams) remains as the next follow-up amendment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Versioning + JSON canonicalization: applied in 4c12192Two short amendments addressing @zmanian's #1.4 (one identity primitive) and #1.6 / #2.8c (JCS canonicalization). #1.4 — one
|
Final cluster of follow-up amendments addressing the 7 seams from
serrrfirat's review:
1. Identity trust attenuation as enforceable types. Mirror the skill
pattern: HostIdentityContextCandidate.message_ref becomes
Option<LoopMessageRef> (None for Installed, Some for Trusted).
Construction goes through new_trusted / new_installed_summary_only
constructors so the invariant is structural. LoopContextMessage.
message_ref also becomes Optional (additive contract change in
ironclaw_turns). build_identity_messages honors the structural
absence; downstream bugs cannot resolve content for Installed
entries because there's no ref to resolve.
2. Identity cache key vs PromptMode. WS-15 brief amends the §3.4
caching design to cache the raw Vec<HostIdentityContextCandidate>
(which is mode-independent), not the filtered Vec<LoopContextMessage>.
build_identity_messages re-applies the applies_when filter against
the request's PromptMode on every call. A PlannedDriver that
switches mode mid-run no longer locks in the first iteration's
filter result.
3. Checkpoint payload-vs-state-ref API shape. WS-6 §3.4 pseudocode
now shows the explicit two-step write: executor serializes state
bytes → host.stage_checkpoint_payload(...) returns
LoopCheckpointStateRef → host.checkpoint(LoopCheckpointRequest
{ kind, state_ref }) writes metadata only. stage_checkpoint_payload
is a new additive method on AgentLoopDriverHost (NOT on
LoopCheckpointPort — staging port is separate from metadata port).
4. Model recovery routing. RecoveryStrategy::on_model_error is no
longer dead code — WS-6 wraps stream_model in a recovery loop
that consults the strategy on host_err. Retry with Backoff alter
works; AdvanceFallback alter is rejected as PlannerContract until
ModelRouteChain lands. SkipResult on model error is also rejected
(skip-what?). Master doc §8 pseudocode updated to match.
5a. DefaultRecoveryStrategy on PolicyDenied: changed from Abort to
SkipResult. A capability denial means "this tool isn't authorized
for this run"; aborting the whole turn on first denial is harsh.
The no-progress detector still catches a stuck model repeatedly
issuing denied calls.
5b. Denial telemetry pathway: master doc §9 cross-references WS-12's
LoopProgressPort denied_count milestone field. Until WS-12 lands,
denials accumulate only in state.recent_failure_kinds as
LoopFailureKind::PolicyDenied. Per-call denial evidence is out of
skeleton scope; a ProfileDenialObserved variant lands in a follow-up
only when a real consumer demands it.
6. Cancellation boundary precision. The locked invariant ("between
every strategy call") now has explicit boundaries in WS-6 §3.5
listing all eight awaited sites; pseudocode in §3.2 and master doc
§8 carries `// CANCEL_BOUNDARY` markers at each site. Adding a new
strategy call to the executor MUST add a matching boundary check.
7. Concurrency hint on CapabilityDescriptorView. WS-0 adds
`concurrency_hint: ConcurrencyHint` field to the descriptor
(additive contract change in ironclaw_turns). WS-9's
HostRuntimeLoopCapabilityPort derives the hint at the adapter
boundary from CapabilityDescriptor.effects (write/spawn/exclusive
→ Exclusive; otherwise → SafeForParallel). Lower-layer descriptor
unchanged; tool authors keep declaring effects and the system
infers conservatively.
Net effect: framework becomes more modular (structural enforcement,
single sources of truth, clean layer splits) and more extensible
(future loop families and extensions inherit working recovery,
telemetry, and cancellation pathways without patching framework
internals).
Spec-only; no code changes. PR #3544's review feedback is now
fully addressed across four amendment commits:
- 49d1506 (LoopFamily cluster)
- 2b20998 (checkpoint durability + LoopExit witness)
- 4c12192 (versioning + JCS canonicalization)
- (this commit) (serrrfirat's 7 correctness seams)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
serrrfirat's 7 correctness seams: applied in 1f808faFinal cluster. All seven seams from @serrrfirat's review now have spec coverage. Three subagent investigations (read-only) on Sonnet confirmed each was a real gap (not hypothetical), grounded the fixes in actual code locations, and surfaced one cross-seam contradiction worth flagging. 1. Identity trust attenuation as enforceable typesThe skill subsystem already enforces trust attenuation by structural absence —
2. Identity cache key vs
|
| # | Site |
|---|---|
| 1 | Top of iteration |
| 2 | Pre-drain().drain_steering |
| 3 | Pre-context().plan_context_request |
| 4 | Pre-capability().filter |
| 5 | Pre-model().preference |
| 6 | Pre-stop().should_stop_after_turn (Reply path) |
| 7 | Pre-stop().should_stop_after_turn (CapabilityCalls path) |
| 8 | Inside capability-batch loop before each gate().handle / recovery().on_capability_error |
Pseudocode in WS-6 §3.2 and master doc §8 carries // CANCEL_BOUNDARY markers at each site. Adding a new strategy call to the executor MUST add a matching boundary check. WS-8's integration suite gets a cancellation-fires-at-each-boundary test covering all eight sites.
7. Concurrency hint on CapabilityDescriptorView
The WS-6 spec called descriptor.concurrency_hint() — but the field didn't exist on the type. Fixed: WS-0 adds concurrency_hint: ConcurrencyHint to CapabilityDescriptorView (additive contract change in ironclaw_turns). The hint is derived at the adapter boundary in WS-9: HostRuntimeLoopCapabilityPort::visible_capabilities maps CapabilityDescriptor.effects → hint (write/spawn/exclusive → Exclusive; otherwise → SafeForParallel). Lower-layer CapabilityDescriptor unchanged. Tool authors keep declaring effects (which they already do); the system infers conservatively.
PR #3544 is ready for re-review. Four amendment commits on top of the original spec:
49d150691— LoopFamily cluster2b20998ae— checkpoint durability + LoopExit witness4c1219255— versioning + JCS canonicalization1f808fae6— this commit (serrrfirat's 7 seams)
All review threads have been responded to. Happy to iterate further if any of these need different shapes.
Three Opus subagents reviewed the four amendment commits and surfaced 5 critical implementation blockers, 5 cross-doc consistency drifts, and 5 architectural gaps. This commit fixes the blockers, the drifts, and three of the quick architectural wins. Two strategic items deferred for separate discussion (future-fork story; §9 cleanup). Critical blockers: - B1: ConcurrencyHint circular dependency. Moved type definition from ironclaw_agent_loop (WS-2) to ironclaw_turns (WS-0) — the field on CapabilityDescriptorView lives in turns, so the type must live in turns. WS-2 imports the type rather than defining it. - B2: stage_checkpoint_payload was specified on AgentLoopDriverHost (a method-less marker trait). Moved declaration to LoopCheckpointPort alongside load_checkpoint_payload; callers still use host.stage_checkpoint_payload(...) via deref-through- supertrait. - B3: WS-7 family.id().to_string().as_str() snippet was E0716 (temporary dropped while borrowed) AND gratuitous — LoopFamilyId is already &'static str. Fixed to family.id().0. - B4: CapabilityDescriptorView field-add is BREAKING (public fields, struct-literal constructors). WS-0 brief now explicitly lists consumers that need updating in the same PR. - B5: WS-5 acceptance criterion still said "aborts on PolicyDenied" — straggler from seam-5a SkipResult amendment. Fixed. Cross-doc consistency: - D1: load_checkpoint_payload signature drift across WS-0, WS-7, WS-10. WS-10 is source of truth; WS-0 drops the inline stub and WS-7's resume pseudocode uses the canonical request/response shape. - D2: from_checkpoint_payload signature drift (Value vs bytes). Bytes-based two-arg shape is now canonical in WS-0; matches the reality that checkpoint storage stores bytes. - D3: Cancellation boundary count was inconsistent (prose said "Eight," table had 9 rows). Combined rows #6 (Reply path) and #7 (CapabilityCalls path) — they're mutually exclusive branches at the same model-response match point. Eight rows everywhere now. - D4: Cancellation helper name was inconsistent across briefs. Standardized on checkpoint_and_exit_if_cancelled across master doc, WS-6, WS-13. - D5: WS-8 had no test for the Denied → SkipResult path. Added two rows to strategy_interactions.rs: denied_call_skips_and_continues and repeated_denied_calls_trip_no_progress. Quick architectural wins: - G1: WS-9 now enumerates EffectKind → ConcurrencyHint mapping per variant. Network → Exclusive (conservative; POSTs are causal). UseSecret → SafeForParallel (read-only secret access). DispatchCapability → Exclusive (recursive depth unsafe). Empty effects → SafeForParallel (pure function). All write/spawn/ modify variants → Exclusive. - G3: WS-6 §3.5a documents strategy-decision observability via tracing::debug! at every strategy call site. Durable typed strategy-decision telemetry deferred to a future workstream pending production debugging need. - G4: Master doc §10 documents in-flight Blocked run behavior when ComponentIdentity.digest changes: LoopExit::Failed { CheckpointUnavailable }; never silently resume against changed digest. Operators expected to plan deploys with this in mind. Deferred for separate discussion: - G2: future-fork story — §4 claims families graduate to own crates but pub(crate) strategy seal makes this impossible without a pub(in family-factory) escape hatch. - G5: §9 has 17 cross-referenced bullets with PR-comment URLs that are institutional memory rather than documentation; needs editorial cleanup with worked decisions inline. Spec-only; no code changes. 9 files touched. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Opus final-review pass: fixes applied in e48a584Three Opus subagents did a final-pass review of the four amendment commits — cross-doc consistency, implementation feasibility, and architectural coherence. They surfaced 5 critical blockers, 5 consistency drifts, and 5 architectural gaps. This commit closes the blockers, the drifts, and three of the quick architectural wins. Two strategic items are deferred for follow-up. Critical blockers (B1-B5) — all fixed
Consistency drifts (D1-D5) — all fixed
Quick architectural wins (G1, G3, G4) — fixed
Deferred for separate discussion (G2, G5)
Final statePR #3544 now carries five amendment commits on top of the original spec:
The spec is ready for re-review. Happy to discuss G2 and G5 separately when you're ready. |
…rate
G2 — families live in ironclaw_agent_loop permanently. §4 now states
explicitly that the strategy-composition seal has no escape hatch
(no pub(in family-factory) visibility, no plugin loaders). If a family
needs heavyweight external deps (tree-sitter, ripgrep, etc.), they
land as feature-gated optional deps inside ironclaw_agent_loop itself
— not in a sibling crate. The cost (one crate accumulates external
deps) is borne so the sealed-strategy invariant remains structurally
unbreakable. Extensions plug into the loop via hooks, never via
families. This decision was the last open question from the Opus
architectural-coherence review.
G5 — §9 editorial cleanup. Two changes:
(a) New §9.1 "Strategies vs hooks: the extension contract" inlines
the full hooks-as-middleware design previously linked only via
PR-comment URL. Includes: responsibility table, layer-cake
diagram, two-boundary invariants, and four worked scenarios
(hook deny / hook mutate / hook pause / event-triggered hook).
Section is meant to be the standalone reference for the
strategy-hook split; future hooks PR (#3524) implements against
this contract.
(b) Stripped PR-comment URLs that pointed at institutional memory:
14 "PR #3544 serrrfirat #N" trailing attributions, two
"Addresses PR #3544 Opus review §c gap" footers, six
"PR #3523-comment-4435808547" references. The content these
pointed to is already inline in the spec body. A single
consolidated "Review history" note in §15 Credits preserves the
attribution without requiring a fresh reader to fetch PR
comments to understand the spec.
Spec now reads standalone — a new engineer six months from now can
follow it without consulting PR threads. The remaining PR references
(PR #3523/#3524 in cross-PR target context, PR #3460 in the LoopExit
witness-type bullet) are legitimate forward/sibling-PR pointers, not
institutional-memory bookmarks.
Spec-only; no code changes. 8 files touched.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Final editorial pass: G2 + G5 in 93f0865The last two open items from the Opus architectural-coherence review are now closed. G2 — families live in
|
| Commit | Cluster |
|---|---|
49d150691 |
LoopFamily / strategy↔hook / trust |
2b20998ae |
checkpoint durability + LoopExit witness |
4c1219255 |
versioning + JCS canonicalization |
1f808fae6 |
serrrfirat's 7 correctness seams |
e48a584b1 |
Opus-review fixes (blockers + drifts + G1/G3/G4) |
93f08654d |
this commit (G2 + G5 editorial cleanup) |
All review threads have been responded to; no open items remain. The spec reads standalone — a new engineer can follow it without consulting PR threads. Ready for re-review or merge.
|
Reviewed current head
Remaining uncertainty: after the above changes, factual confidence still depends on code-level verification, especially atomic input/checkpoint semantics and capability-host denial/approval paths. Minimum verification I would want before implementation confidence:
|
|
Addressed the review in 56d67ad. Summary of changes:
Validation run locally in the isolated worktree:
|
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>
…ature mentions Three small spec consistency fixes flagged by /review on PR nearai#3544: - e2e-integration-tests.md: rename `InvokeCapabilitySingle` enum variant in MockHostCall to `InvokeCapability` to match the actual host method name (we reuse the existing single-call API, not a new method). - agent-loop-skeleton.md §10/§11: align `iteration_limit()` mentions with WS-3's signature `iteration_limit(&state)`. - strategy-traits-gamma.md: doc-comment for BudgetStrategy was using `>` (off-by-one) and `iteration_limit()` (wrong arity); both fixed to match the canonical executor's `>=` semantics + actual signature. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses 8 real spec issues flagged by automated review of the docs PR. Six fixes in canonical-executor.md (master doc §8 mirrored): - Reply branch now finalizes BEFORE consulting the stop strategy. The prior shape only finalized on GracefulStop, so the default-strategy Continue→Completed and NoProgressDetected→Failed paths returned without ever calling finalize_assistant_message — LoopExit validation rejects non-NoReply Completed without reply_message_refs, so a plain successful reply was silently lost. (codex P1) - CapabilityCalls match now handles `Denied(reason)` and `SpawnedProcess(handle)` outcomes. EmptyLoopCapabilityPort returns Denied today, so without the Denied arm any model-issued tool call would hit an unreachable match. SpawnedProcess gets BeforeBlock checkpoint + Blocked exit. (codex P1) - Per-iteration HashSet wraps every push to recent_call_signatures so the dedup contract (master doc §10 + WS-0 §3.4) is actually enforced in code, not just documentation. Without this, three identical calls in a single batch trip NoProgressDetected immediately. (gemini high, codex P1) - recent_failure_kinds.push moved out of the inner retry loop so a single failing call retried 3× no longer satisfies the failure-run- length escape (which checks for 3 identical failures *across iterations*). (gemini high) - Iteration cap check moved to the TOP of the loop body so a resumed executor with state.iteration == limit exits immediately instead of running one extra body. (gemini high) - batch_result_refs computed from a snapshot index taken before invoking the batch, not by slicing `last_batch_total` calls from the tail of state.result_refs. The old shape over-included refs from prior iterations whenever this batch had any non-completing outcome (Skip/Block/Failed-with-no-retry). (gemini high) Two medium fixes in canonical-executor.md: - summary_of(call) now receives &surface so concurrency hints can be read from the visible-capability descriptors. (gemini medium) - drain_followup_into returns (LoopExecutionState, bool) instead of taking &mut LoopExecutionState — honors the value-immutable contract in master doc §8 property 3. drain_steering_into already had the right shape; both helpers now also filter LoopInputPort batches to user-facing message kinds (UserMessage), leaving control kinds (Cancel, Interrupt, GateResolved, CapabilitySurfaceChanged) for dedicated executor paths. (gemini medium, codex P2 partial) One fix in planned-driver-adapter.md: - validate_resume_request reads checkpoint_schema_id from request.resolved_run_profile.loop_driver descriptor instead of a fictional request.checkpoint_schema_id field that doesn't exist on AgentLoopDriverResumeRequest. (codex P2) One design clarification in strategy-traits-gamma.md: - TurnSummary doc-comment explains why reply CONTENT is intentionally not included — strategies access content via assistant_message_ref through the host port (per turns-agent-loop.md §6, refs only in loop state). Pi-mono passes content because it has no host abstraction; Reborn maintains the trust-boundary by routing content reads through the host. (gemini medium — design choice clarified, not changed) Already addressed in earlier commits: iteration_limit() arity (codex P2), InvokeCapabilitySingle naming (review pass). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…view Four high-level architectural decisions from human review (serrrfirat, zmanian) on PR nearai#3544 land as spec amendments before WS-0 seals the trait shapes: 1. LoopFamily as first-class abstraction (zmanian #2.2). New WS-3.5 brief: LoopFamilyId + ComponentIdentity + LoopFamily + LoopFamilyRegistry (Guice-style singleton built once at startup). 2. Strategies sealed pub(crate) (zmanian #1.2, #2.5, #2.6; serrrfirat #1). AgentLoopPlanner is pub but uses sealed-trait pattern; strategy access lives on pub(crate) AgentLoopPlannerInternal extension trait. 3. Hooks as middleware (zmanian #1.1). Adopt the four-scenario design in PR nearai#3523-comment-4435808547. Two concrete follow-ups: composition seam architecture test in WS-9's brief; LoopFailureKind::PolicyDenied variant in WS-0. 4. Broad scope retained with stress-test discipline (zmanian #2.1, #2.7; serrrfirat #8). New §12.5 enumerates anticipated families (default + hypothetical routine/mission/coding/planning) and the strategies each would swap; trait shapes accommodate every row. Side effects: - Split ControlStrategyState → StopStrategyState + GateStrategyState in WS-0 so stop and gate strategies grow independently. - Drop generics on PlannedDriver — non-generic { family, executor }. WS-7 + downstream briefs (WS-9, WS-10, WS-11, WS-13, WS-14) updated. - Subsume PlannerId into LoopFamilyId + ComponentIdentity (one versioning primitive per zmanian #1.4, partly addressed). - Executor entry point becomes execute_family(&LoopFamily, host, state). - Document HostXxxContextSource as the pattern for family-specific durable context (mirrors WS-15's HostIdentityContextSource). 15 files changed (1 new brief: loop-family-registry.md). Spec-only; no code changes. Three remaining comment clusters (checkpoint durability + LoopExit witness; versioning + JSON canonicalization; serrrfirat's remaining 7 correctness seams) are noted as follow-up work. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two follow-up amendments addressing zmanian #2.3 and #2.4 from PR nearai#3544 review (the checkpoint-durability + LoopExit-witness-type cluster): 1. Retry budget durability (#2.3). Add an explicit §10 note that retry budgets are bounded within a single iteration but reset across resumes (a crash mid-retry resumes from the BeforeSideEffect checkpoint with recovery_state.attempts = 0). The iteration cap — the only structural safety net — still bounds total retries across resumes because each resume costs one iteration. Adding an AfterRetryAttempt checkpoint kind would produce checkpoint storms under retry pressure; we prefer being honest about what we structurally guarantee. WS-6's brief gains the matching inline durability note next to the retry loop pseudocode. 2. LoopExit validation as structural enforcement (#2.4). Add a §9 bullet stating that AgentLoopDriver returns raw LoopExit and only LoopExitApplier::validate (sealed via LoopExitValidationPolicy per PR nearai#3460) produces the LoopExitValidationDecision flowing into TurnRunTransitionPort::apply_validated_loop_exit via ApplyValidatedLoopExitRequest. The runner's transition port accepts the validated request, not raw LoopExit. The structural enforcement is already in place via the existing nearai#3460 seal plus ApplyValidatedLoopExitRequest shape; the bullet codifies the invariant in the spec. Spec-only; no code changes. The cluster's remaining work (serrrfirat's 7 correctness seams; versioning + JSON canonicalization) lands as separate follow-up amendments. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…n PR nearai#3544 Third cluster of follow-up amendments addressing zmanian #1.4 and #1.6/#2.8c from PR nearai#3544 review (versioning + JSON canonicalization). 1. ComponentIdentity as the one versioning primitive (zmanian #1.4). Master doc §9 adds a new bullet stating that ComponentIdentity { id, digest } from WS-3.5 is the canonical shape across loop families, checkpoint payload metadata, hooks (nearai#3524 future), skill snapshots (nearai#3470 future), and model routes (nearai#3462 future). Content-addressed only — monotonic counters false-drift and false-agree, both silent replay-correctness bugs. The existing String identities on LoopModelRouteSnapshot (auth_version, config_version) migrate alongside nearai#3462's model-route work; NOT in this PR. WS-3.5 brief gains a Migration / propagation table enumerating the four components, their current shape, and per- component migration cost. 2. JCS RFC 8785 for ArgsHash canonicalization (zmanian #1.6, #2.8c). Master doc §9 adds a new bullet pinning JCS as the canonicalization scheme. WS-0 brief gains new §3.4a spelling out the rules explicitly: sort object keys by UTF-16 code-unit order, reject NaN/Infinity, preserve number representation, minimal whitespace. Implementation reference: jcs crate. Cross-model compatibility note: for typical tool args (no floats), JCS output is byte- identical to the Hermes/Forge sorted-keys-minimal-whitespace convention used in the open-weights tool-calling ecosystem — replay across model swaps hashes identically for typical args. WS-0 acceptance criteria strengthened: from_call is JCS-stable under key reordering at multiple nesting depths; NaN/Infinity inputs return Err. WS-8 gains a matching args_hash_jcs_stable integration test. Spec-only; no code changes. The skill-snapshot, hook, and model-route migrations to ComponentIdentity land in their owning PRs (nearai#3470, nearai#3524, nearai#3462) — this PR sets the target shape only. Final cluster (serrrfirat's 7 correctness seams) remains as the next follow-up amendment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Final cluster of follow-up amendments addressing the 7 seams from
serrrfirat's review:
1. Identity trust attenuation as enforceable types. Mirror the skill
pattern: HostIdentityContextCandidate.message_ref becomes
Option<LoopMessageRef> (None for Installed, Some for Trusted).
Construction goes through new_trusted / new_installed_summary_only
constructors so the invariant is structural. LoopContextMessage.
message_ref also becomes Optional (additive contract change in
ironclaw_turns). build_identity_messages honors the structural
absence; downstream bugs cannot resolve content for Installed
entries because there's no ref to resolve.
2. Identity cache key vs PromptMode. WS-15 brief amends the §3.4
caching design to cache the raw Vec<HostIdentityContextCandidate>
(which is mode-independent), not the filtered Vec<LoopContextMessage>.
build_identity_messages re-applies the applies_when filter against
the request's PromptMode on every call. A PlannedDriver that
switches mode mid-run no longer locks in the first iteration's
filter result.
3. Checkpoint payload-vs-state-ref API shape. WS-6 §3.4 pseudocode
now shows the explicit two-step write: executor serializes state
bytes → host.stage_checkpoint_payload(...) returns
LoopCheckpointStateRef → host.checkpoint(LoopCheckpointRequest
{ kind, state_ref }) writes metadata only. stage_checkpoint_payload
is a new additive method on AgentLoopDriverHost (NOT on
LoopCheckpointPort — staging port is separate from metadata port).
4. Model recovery routing. RecoveryStrategy::on_model_error is no
longer dead code — WS-6 wraps stream_model in a recovery loop
that consults the strategy on host_err. Retry with Backoff alter
works; AdvanceFallback alter is rejected as PlannerContract until
ModelRouteChain lands. SkipResult on model error is also rejected
(skip-what?). Master doc §8 pseudocode updated to match.
5a. DefaultRecoveryStrategy on PolicyDenied: changed from Abort to
SkipResult. A capability denial means "this tool isn't authorized
for this run"; aborting the whole turn on first denial is harsh.
The no-progress detector still catches a stuck model repeatedly
issuing denied calls.
5b. Denial telemetry pathway: master doc §9 cross-references WS-12's
LoopProgressPort denied_count milestone field. Until WS-12 lands,
denials accumulate only in state.recent_failure_kinds as
LoopFailureKind::PolicyDenied. Per-call denial evidence is out of
skeleton scope; a ProfileDenialObserved variant lands in a follow-up
only when a real consumer demands it.
6. Cancellation boundary precision. The locked invariant ("between
every strategy call") now has explicit boundaries in WS-6 §3.5
listing all eight awaited sites; pseudocode in §3.2 and master doc
§8 carries `// CANCEL_BOUNDARY` markers at each site. Adding a new
strategy call to the executor MUST add a matching boundary check.
7. Concurrency hint on CapabilityDescriptorView. WS-0 adds
`concurrency_hint: ConcurrencyHint` field to the descriptor
(additive contract change in ironclaw_turns). WS-9's
HostRuntimeLoopCapabilityPort derives the hint at the adapter
boundary from CapabilityDescriptor.effects (write/spawn/exclusive
→ Exclusive; otherwise → SafeForParallel). Lower-layer descriptor
unchanged; tool authors keep declaring effects and the system
infers conservatively.
Net effect: framework becomes more modular (structural enforcement,
single sources of truth, clean layer splits) and more extensible
(future loop families and extensions inherit working recovery,
telemetry, and cancellation pathways without patching framework
internals).
Spec-only; no code changes. PR nearai#3544's review feedback is now
fully addressed across four amendment commits:
- c945926 (LoopFamily cluster)
- 6f3a750 (checkpoint durability + LoopExit witness)
- 81c429a (versioning + JCS canonicalization)
- (this commit) (serrrfirat's 7 correctness seams)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rai#3544 Three Opus subagents reviewed the four amendment commits and surfaced 5 critical implementation blockers, 5 cross-doc consistency drifts, and 5 architectural gaps. This commit fixes the blockers, the drifts, and three of the quick architectural wins. Two strategic items deferred for separate discussion (future-fork story; §9 cleanup). Critical blockers: - B1: ConcurrencyHint circular dependency. Moved type definition from ironclaw_agent_loop (WS-2) to ironclaw_turns (WS-0) — the field on CapabilityDescriptorView lives in turns, so the type must live in turns. WS-2 imports the type rather than defining it. - B2: stage_checkpoint_payload was specified on AgentLoopDriverHost (a method-less marker trait). Moved declaration to LoopCheckpointPort alongside load_checkpoint_payload; callers still use host.stage_checkpoint_payload(...) via deref-through- supertrait. - B3: WS-7 family.id().to_string().as_str() snippet was E0716 (temporary dropped while borrowed) AND gratuitous — LoopFamilyId is already &'static str. Fixed to family.id().0. - B4: CapabilityDescriptorView field-add is BREAKING (public fields, struct-literal constructors). WS-0 brief now explicitly lists consumers that need updating in the same PR. - B5: WS-5 acceptance criterion still said "aborts on PolicyDenied" — straggler from seam-5a SkipResult amendment. Fixed. Cross-doc consistency: - D1: load_checkpoint_payload signature drift across WS-0, WS-7, WS-10. WS-10 is source of truth; WS-0 drops the inline stub and WS-7's resume pseudocode uses the canonical request/response shape. - D2: from_checkpoint_payload signature drift (Value vs bytes). Bytes-based two-arg shape is now canonical in WS-0; matches the reality that checkpoint storage stores bytes. - D3: Cancellation boundary count was inconsistent (prose said "Eight," table had 9 rows). Combined rows #6 (Reply path) and #7 (CapabilityCalls path) — they're mutually exclusive branches at the same model-response match point. Eight rows everywhere now. - D4: Cancellation helper name was inconsistent across briefs. Standardized on checkpoint_and_exit_if_cancelled across master doc, WS-6, WS-13. - D5: WS-8 had no test for the Denied → SkipResult path. Added two rows to strategy_interactions.rs: denied_call_skips_and_continues and repeated_denied_calls_trip_no_progress. Quick architectural wins: - G1: WS-9 now enumerates EffectKind → ConcurrencyHint mapping per variant. Network → Exclusive (conservative; POSTs are causal). UseSecret → SafeForParallel (read-only secret access). DispatchCapability → Exclusive (recursive depth unsafe). Empty effects → SafeForParallel (pure function). All write/spawn/ modify variants → Exclusive. - G3: WS-6 §3.5a documents strategy-decision observability via tracing::debug! at every strategy call site. Durable typed strategy-decision telemetry deferred to a future workstream pending production debugging need. - G4: Master doc §10 documents in-flight Blocked run behavior when ComponentIdentity.digest changes: LoopExit::Failed { CheckpointUnavailable }; never silently resume against changed digest. Operators expected to plan deploys with this in mind. Deferred for separate discussion: - G2: future-fork story — §4 claims families graduate to own crates but pub(crate) strategy seal makes this impossible without a pub(in family-factory) escape hatch. - G5: §9 has 17 cross-referenced bullets with PR-comment URLs that are institutional memory rather than documentation; needs editorial cleanup with worked decisions inline. Spec-only; no code changes. 9 files touched. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rate
G2 — families live in ironclaw_agent_loop permanently. §4 now states
explicitly that the strategy-composition seal has no escape hatch
(no pub(in family-factory) visibility, no plugin loaders). If a family
needs heavyweight external deps (tree-sitter, ripgrep, etc.), they
land as feature-gated optional deps inside ironclaw_agent_loop itself
— not in a sibling crate. The cost (one crate accumulates external
deps) is borne so the sealed-strategy invariant remains structurally
unbreakable. Extensions plug into the loop via hooks, never via
families. This decision was the last open question from the Opus
architectural-coherence review.
G5 — §9 editorial cleanup. Two changes:
(a) New §9.1 "Strategies vs hooks: the extension contract" inlines
the full hooks-as-middleware design previously linked only via
PR-comment URL. Includes: responsibility table, layer-cake
diagram, two-boundary invariants, and four worked scenarios
(hook deny / hook mutate / hook pause / event-triggered hook).
Section is meant to be the standalone reference for the
strategy-hook split; future hooks PR (nearai#3524) implements against
this contract.
(b) Stripped PR-comment URLs that pointed at institutional memory:
14 "PR nearai#3544 serrrfirat #N" trailing attributions, two
"Addresses PR nearai#3544 Opus review §c gap" footers, six
"PR nearai#3523-comment-4435808547" references. The content these
pointed to is already inline in the spec body. A single
consolidated "Review history" note in §15 Credits preserves the
attribution without requiring a fresh reader to fetch PR
comments to understand the spec.
Spec now reads standalone — a new engineer six months from now can
follow it without consulting PR threads. The remaining PR references
(PR nearai#3523/nearai#3524 in cross-PR target context, PR nearai#3460 in the LoopExit
witness-type bullet) are legitimate forward/sibling-PR pointers, not
institutional-memory bookmarks.
Spec-only; no code changes. 8 files touched.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
docs(reborn): agent loop skeleton framework spec + 9 workstream briefs
Summary
Architecture spec for the Reborn agent-loop framework — a new
ironclaw_agent_loopcrate that adds a reusable loop body and a strategy-composition planner above the existingTurnCoordinator → TurnRunner → AgentLoopDriver → AgentLoopHostchain.docs/reborn/agent-loop-skeleton.md): 15 sections covering architecture, four mutability layers, nine strategies, immutable execution state, canonical executor tick, three production-safe escape nets, follow-up workstreams (WS-9..WS-14) for end-to-end wiring.docs/reborn/agent-loop-briefs/: WS-0 state + checkpoints; WS-1/2/3 nine strategy traits split into three groups; WS-4 planner facade; WS-5 nineDefault*impls; WS-6 canonical executor; WS-7PlannedDriveradapter; WS-8 cross-crate integration suite with feature-gatedtest_supportmodule.The default behavior is modeled on pi-mono agent-loop mechanics (single async function,
Reply | CapabilityCallsparent protocol, steering/follow-up queues). The framework absorbs pi's hooks into typed host ports and adds production-grade safety nets (no-progress detection via call-signature ring + failure run-length, retry budgets, gate suspension, evidence-validatedLoopExit).Reviews completed
/codex:review) — 5 issues (stop-after-batch, cancellation-as-LoopExit, real retry mechanic, iteration cap off-by-one, repetition iteration semantics). All applied.Test plan
This is a docs-only PR. Verification is light:
grep -nE '/(Users|home)/[a-zA-Z0-9_-]+/|/tmp/' docs/reborn/agent-loop-skeleton.md docs/reborn/agent-loop-briefs/*.mdreturns no matches (per.claude/rules/doc-hygiene.md)git diff origin/reborn-integration --stat -- ':!Cargo.lock'shows onlydocs/reborn/...pathsTurnSummary,StopOutcome,RecoveryOutcome,CapabilityErrorSummary,ModelErrorSummaryall defined in the appropriate brief)§3,§8, etc.) resolve to current section numberingEnd-to-end agent-loop execution is verifiable only after WS-0..WS-8 land. Each brief carries its own per-workstream verification section.
🤖 Generated with Claude Code