Skip to content

arch(ws-6a): add canonical loop executor - #3596

Merged
henrypark133 merged 10 commits into
reborn-integrationfrom
arch/ws-6a
May 15, 2026
Merged

henrypark133 merged 10 commits into
reborn-integrationfrom
arch/ws-6a

Conversation

@henrypark133

@henrypark133 henrypark133 commented May 13, 2026 •

Copy link
Copy Markdown
Collaborator

Context

Canonical executor foundation. This is the branch that turns the strategy/family/planner model into a reusable loop tick.

Master spec: docs/reborn/agent-loop-skeleton.md
Workstream brief: docs/reborn/agent-loop-briefs/canonical-executor.md
Stack base: arch/level2-merged

Latest stack maintenance on 2026-05-14:

  • Rebased this branch onto its current stack base after the WS0 prompt-authority fix and the follow-up WS8/WS14-parent/WS16/WS17 conflict resolutions.
  • Pushed the updated branch with force-with-lease where the remote already existed, or published it as a new branch where it did not.
  • Verified the final ancestry chain from origin/reborn-integration through WS17 before publishing the PR descriptions.

What landed

  • CanonicalAgentLoopExecutor and AgentLoopExecutor::execute_family(...) entry point.
  • Executor flow for prompt request, model call, capability batching, checkpoint boundaries, gate/recovery/stop decisions, and final loop exits.
  • Batch policy derived from visible capability calls rather than hardcoded behavior.
  • Checkpoint staging calls through host ports and the WS0 state payload format.
  • Model/capability error classification that preserves fail-closed behavior for unavailable credentials and host errors.
  • Executor tests and support wiring without depending on product-specific runtime composition.

Reviewer focus

  • The executor should be the only owner of canonical loop mechanics.
  • Strategies return decisions; the executor sequences host side effects and checkpoint boundaries.
  • All durable effects must go through AgentLoopDriverHost ports.
  • Credential/auth/gate states must not be flattened into generic chat failure handling.

Non-goals / deferred work

  • Runner-facing AgentLoopDriver adapter lands in WS7.
  • Cross-crate test support and E2E coverage land in WS8.
  • Real capability/checkpoint/input/progress/cancellation/identity host adapters land in WS9 through WS15.

Validation

  • cargo check -p ironclaw_agent_loop
  • Included in final ancestry verification: arch/level2-merged -> arch/ws-6a.

Stack position

[#3550 ws-0] state/checkpoint foundation -> reborn-integration
   |-- #3551 ws-1 strategy alpha -> ws-0
   |-- #3552 ws-2 strategy beta -> ws-0
   |-- #3553 ws-3 strategy gamma -> ws-0
   |-- #3643 ws-3.5 loop family registry -> ws-0
   '-- #3554 level1-merged -> ws-0
         |-- #3555 ws-4 planner facade -> level1
         |-- #3556 ws-5 default strategies -> level1
         '-- #3557 level2-merged -> level1
               '-- #3596 ws-6a canonical executor -> level2
                     '-- #3597 ws-7 PlannedDriver adapter -> ws-6a
                           '-- #3598 ws-8 integration/test support -> ws-7
                                 |-- #3644 ws-9 capability host wiring -> ws-8
                                 |-- #3645 ws-10 checkpoint load/resume -> ws-8
                                 |-- #3646 ws-11 input port -> ws-8
                                 |-- #3647 ws-12 progress port -> ws-8
                                 |-- #3648 ws-13 cancellation accessor -> ws-8
                                 |-- #3649 ws-15 prompt/identity context -> ws-8
                                 '-- #3650 ws-14-parent integrated host ports -> ws-8
                                       '-- #3651 ws-14 planned default registration -> ws-14-parent
                                             '-- #3652 ws-16 live runtime wiring -> ws-14
                                                   '-- #3653 ws-17 product live cutover -> ws-16

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@github-actions github-actions Bot added size: XL 500+ changed lines risk: low Changes to docs, tests, or low-risk modules contributor: core 20+ merged PRs labels May 13, 2026
Comment thread crates/ironclaw_agent_loop/src/executor.rs Outdated
Comment thread crates/ironclaw_agent_loop/src/executor.rs Outdated
@serrrfirat

Copy link
Copy Markdown
Collaborator

Summary

Reviewed WS6a PR #3596 only.
Base 255c8c0db65d5960526bba961bef3dcefd36e187 → head c936938e8968c3c2dbc444a3f8c322ad679f9ff5.

Canonical executor has blocking cancellation/semantic-error handling issues, plus test suite does not compile.

Validation:

  • cargo check -p ironclaw_agent_loop ✅
  • cargo test -p ironclaw_agent_loop --lib ❌: executor.rs:1136 missing LoopPromptBundle.instruction_fingerprint in test MockHost initializer.

Findings

# Sev Category File:Line Issue Fix suggestion
1 High Cancellation / Correctness crates/ironclaw_agent_loop/src/executor.rs:266-287, crates/ironclaw_agent_loop/src/executor.rs:851-856 AgentLoopHostErrorKind::Cancelled is classified as ModelErrorClass::Transient, so model cancellation enters recovery/retry instead of terminal cancellation. Capability paths also collapse host errors to HostUnavailable at executor.rs:377-388 and executor.rs:514-520. User/host cancellation can keep work alive after cancel and never return the executor’s Cancelled variant. Plumb Cancelled through all host call sites as terminal cancellation. Add model + capability cancellation tests asserting no retry/no extra side effects and correct checkpoint behavior.
2 High Retry / Correctness crates/ironclaw_agent_loop/src/state/slots.rs:12-21, crates/ironclaw_agent_loop/src/strategies/recovery.rs:140-194, crates/ironclaw_agent_loop/src/executor.rs:262-264, crates/ironclaw_agent_loop/src/executor.rs:510-520 Inherits #3556 global retry-counter bug, and executor does not reset recovery_state after successful progress. Earlier unrelated transient failures can exhaust retry budget for later model/capability errors, causing premature abort. Scope retry attempts by failure class/episode, and reset recovery state after successful model/capability progress. Add mixed-error and success-then-error regression tests.
3 Medium Recovery / Boundary crates/ironclaw_agent_loop/src/executor.rs:377-388, crates/ironclaw_agent_loop/src/executor.rs:514-520 Capability host semantic errors (StaleSurface, InvalidInvocation, PolicyDenied, Cancelled) are erased into HostUnavailable. Normal race/policy/cancel outcomes become generic infra failures and bypass clean loop-level handling. Classify capability host errors explicitly and route stale-surface/policy/cancel through typed recovery/cancel paths.
4 Medium Tests / Regression coverage crates/ironclaw_agent_loop/src/executor.rs:1136-1142 cargo test -p ironclaw_agent_loop --lib fails to compile because test MockHost builds LoopPromptBundle without instruction_fingerprint. Executor regression suite currently cannot run. Add instruction_fingerprint: None or expected fingerprint in test fixture.

Security/data-flow notes

  • Main risks are cancellation and semantic boundary collapse, not raw secret leakage.
  • Checkpoint and side-effect ordering need cancellation-specific tests before relying on executor safety.

Missing tests

  • Cancellation from model and capability ports: no retry, no extra side effects.
  • Stale surface from capability batch: typed handling, not HostUnavailable.
  • Mixed error classes and success-reset retry budget.
  • Fix compiling executor tests.

@serrrfirat serrrfirat mentioned this pull request May 14, 2026
4 tasks done
@zmanian

zmanian commented May 14, 2026

Copy link
Copy Markdown
Collaborator

Review notes — WS6a canonical executor

Architecture is solid; checkpoint protocol is WS10-ready. Four items to address before un-drafting.

1. Dead AgentLoopExecutorError::Cancelled variant

The variant is declared but never constructed. Meanwhile, host Cancelled errors are mapped to ModelErrorClass::Transient at executor.rs:861 — so a host cancel becomes a retried model error. This conflicts with WS13's cancellation port intent. Either remove the variant or wire a check (between iterations, before model stream). Reconcile with WS13 (#3648) — that PR's boundary checks need somewhere to land their Cancelled signal in this enum.

2. Stringly-typed capability error classification

capability_error_class / capability_failure_kind dispatch on match kind { "transient" => ... } over CapabilityFailure::error_kind: String. A typo in any producer silently maps to Permanent/CapabilityProtocolError. Fail-closed correctness depends on this classification — replace with an enum in ironclaw_turns and have producers emit it directly.

3. opaque_token char-replace collision

LoopGateRef::new(format!("gate:process-{}", opaque_token)) silently mangles disallowed chars to -. Risk of gate-ref collision across two distinct processes that share a stripped form. Use a hash, not a char-replace.

4. failed_exit discards _state

Final state is captured via checkpoint, but the in-memory LoopExecutionState is dropped — recent_failure_kinds, recovery_state, etc., live only inside the staged checkpoint payload. If a caller wants the in-memory final state for diagnostics this is unreachable. Either return state in the exit or document the intent.

Checkpoint contract observations

Solid for WS10:

  • Four kinds (BeforeModel, BeforeSideEffect, BeforeBlock, Final) with clear semantics.
  • 2-step stage→commit returning (checkpoint_id, state_ref) is exactly the shape WS10 resume needs.
  • PolicyDenied short-circuits retry even on RecoveryOutcome::Retry — correct.

One non-atomicity note: checkpoint() mutates state.last_checkpoint before stage_checkpoint_payload commits. If staging fails the in-memory state carries a marker for an absent payload. Today this is moot (executor consumes state on CheckpointFailed), but if a caller catches and retries the marker is stale. Document or move the marker mutation after successful host commit.

Minor

  • MAX_MODEL_RETRIES=8, MAX_CAPABILITY_RETRIES=8, MAX_INPUT_DRAIN=32 are spec constants worth pulling out of executor.rs and citing the spec section.
  • Box<LoopExecutionState> in ModelStep::Response is a clippy large_enum_variant boxing — fine, but explicit #[allow] with rationale would be cleaner.
  • denied_calls invokes recovery sequentially per-denied-call — a 10-call all-denied batch invokes the recovery strategy 10 times. Note in spec or batch the denial path.

Test surface (10+ executor-level tests through execute_family) genuinely satisfies "test through the caller."

@henrypark133

Copy link
Copy Markdown
Collaborator Author

WS6a follow-up pushed in 7de21b2a1.

Addressed the remaining review items I could verify locally:

  • model-host cancellation now exits as AgentLoopExecutorError::Cancelled before recovery retry classification.
  • user-facing input drains only ack whole batches that contain no control inputs, so mixed [cancel, message] / [interrupt, follow-up] batches are left unacked for the control path.
  • retry accounting is cleared after successful model progress and successful capability-result progress, with caller-level executor tests covering both paths.
  • process gate refs now derive from a blake3 digest of the full LoopProcessRef instead of a lossy sanitized token.
  • production panic fallbacks in completed/failed exit construction were removed in the prior commit.

Local validation on this head:

  • cargo test -p ironclaw_agent_loop executor --lib
  • python3 scripts/check_no_panics.py --base arch/level2-merged --head HEAD
  • cargo fmt --check
  • git diff --check
  • cargo test -p ironclaw_agent_loop --lib
  • cargo clippy -p ironclaw_agent_loop -p ironclaw_loop_support -p ironclaw_turns -p ironclaw_reborn --all-targets --all-features -- -D warnings

@henrypark133
henrypark133 changed the base branch from arch/level2-merged to reborn-integration May 15, 2026 03:25
@henrypark133
henrypark133 marked this pull request as ready for review May 15, 2026 04:12
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@henrypark133
henrypark133 merged commit 887526d into reborn-integration May 15, 2026
14 checks passed
@henrypark133
henrypark133 deleted the arch/ws-6a branch May 15, 2026 04:13

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: af9a668b72

ℹ️ 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".

Comment on lines +657 to +658
match planner.gate().handle(&state, &summary).await {
GateOutcome::Block { gate } => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Enforce gate outcome validation before honoring skips

handle_gate consumes planner.gate().handle(...) directly without calling GateOutcome::validate_for_gate_kind. That lets an approval gate be returned as SkipAndContinue, so an ApprovalRequired capability can be silently dropped and the loop continues instead of blocking for approval. The gate contract explicitly marks this combination invalid and expects a fail-closed path (LoopFailureKind::DriverBug), so this omission weakens approval enforcement for custom families/strategies.

Useful? React with 👍 / 👎.

theredspoon pushed a commit to theredspoon/ironclaw that referenced this pull request Jun 21, 2026
* arch(ws-6a): add canonical loop executor

* fix(ws-6a): enforce capability filter and checkpoint marker

* fix(ws-6a): pin executor calls to current surface

* fix(ws-6a): derive batch policy from visible calls

* fix(ws-6a): preserve control inputs and cancellation

* fix(ws-6a): align executor with level2 contracts

* fix(ws-6a): remove panic fallback exits

* fix(ws-6a): clear retry state after progress

* fix(ws-6a): satisfy executor batch step clippy

* fix(ws-6a): address canonical loop review feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor: core 20+ merged PRs risk: low Changes to docs, tests, or low-risk modules size: XL 500+ changed lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants