Skip to content

arch(level2-merged): integrate ws-4 + ws-5 - #3557

Closed
henrypark133 wants to merge 11 commits into
arch/level1-mergedfrom
arch/level2-merged
Closed

henrypark133 wants to merge 11 commits into
arch/level1-mergedfrom
arch/level2-merged

Conversation

@henrypark133

@henrypark133 henrypark133 commented May 13, 2026 •

Copy link
Copy Markdown
Collaborator

Context

Integration branch for the planner/default-strategy layer. It produces the base used by the canonical executor workstream.

Master spec: docs/reborn/agent-loop-skeleton.md
Workstream brief: docs/reborn/agent-loop-skeleton.md
Stack base: arch/level1-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

  • Merged arch/ws-4 and arch/ws-5 onto the current level1 integration branch.
  • Resolved planner/default-strategy composition conflicts.
  • Verified the combined planner facade, sealed family registry, and default strategy layer compile together.
  • Kept the branch integration-only so downstream diffs can focus on executor/runtime behavior.

Reviewer focus

  • Planner facade and default-strategy composition boundaries.
  • No accidental exposure of internal strategy traits.
  • No unrelated behavior beyond integrating the two sibling branches.

Non-goals / deferred work

  • Canonical executor implementation starts in WS6a.
  • Planned driver bridge is WS7.
  • Host port adapters and product cutover are later.

Validation

  • cargo check -p ironclaw_agent_loop -p ironclaw_reborn
  • Final ancestry check verified arch/level1-merged -> arch/level2-merged.

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

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request implements the Planner facade and the reference baseline implementations for the nine agent-loop strategies, including Context, Capability, Model, Batch, Gate, Recovery, Stop, Drain, and Budget. It introduces the DefaultPlanner as a composition layer and the PlannerId type for stable identification in checkpoints. The reviewer feedback correctly identifies that while the production-ready Default*Strategy implementations are now available, the DefaultPlanner is still using internal PlaceholderStrategy instances. Suggestions were provided to update the imports and the Default implementation of the planner to use these new production strategies.

Comment thread crates/ironclaw_agent_loop/src/default_planner.rs Outdated
Comment thread crates/ironclaw_agent_loop/src/default_planner.rs
# Conflicts:
#	crates/ironclaw_agent_loop/Cargo.toml
#	crates/ironclaw_agent_loop/src/strategies/budget.rs
#	crates/ironclaw_agent_loop/src/strategies/context.rs
#	crates/ironclaw_agent_loop/src/strategies/drain.rs
#	crates/ironclaw_agent_loop/src/strategies/stop.rs
@serrrfirat

Copy link
Copy Markdown
Collaborator

Summary

Reviewed Level2 integration PR #3557 only.
Base 22a8e1d4df8022962a2a9911100d47e5c397c816 → head 255c8c0db65d5960526bba961bef3dcefd36e187.

Merge stance: no unique Level2 merge-only finding, but this PR inherits blocking issues from #3555 and #3556.

Tests run:

  • cargo test -p ironclaw_agent_loop --lib ✅

Findings

# Sev Category File:Line Issue Fix suggestion
inherited Medium Boundaries / Replay safety #3555 Planner/default family digest remains fixed zero while planner composition is replay-relevant. Fix in #3555/source branch, then rebase.
inherited High Correctness / Recovery #3556 Recovery retry budget is documented per-error-class but implemented as one global attempts counter. Fix in #3556/source branch, then rebase.

Security/data-flow notes

  • No unique data leak/auth bypass found in Level2 merge glue.
  • Planner/default strategy composition boundaries otherwise remain crate-internal.

Correctness/invariant notes

  • Level2 safety depends on inherited planner identity and recovery budget fixes.

Missing tests

Suggested fixes

  1. Fix arch(ws-4): planner facade - AgentLoopPlanner and DefaultPlanner #3555 and arch(ws-5): default strategies - Default* impls for all nine traits #3556 source branches.
  2. Rebase Level2 and rerun integration tests.

@zmanian

zmanian commented May 14, 2026

Copy link
Copy Markdown
Collaborator

Review notes — Level2 integration

Clean mechanical join with good encapsulation discipline. Two observations worth tracking.

Silent defaults promotion

WS5 defaults are now implicitly the production defaults for LoopFamilyId::DEFAULT the moment any executor lands (WS6a). No feature gate, no opt-in. The promotion is structural here, not behavioral — but downstream PRs (WS14/WS16/WS17) inherit these defaults silently.

This is the same class of risk as the HostTrustPolicy::empty() issue (#3603): a placeholder becomes load-bearing because no caller is required to override it. Worth a tracking note that WS14's default registration must either replace or explicitly opt-in to each WS5 default.

No end-to-end test

Title says "integration" but the verification is mechanical join + already-present unit tests. There is no test that simultaneously drives multiple strategies through a shared LoopExecutionState transition. Acceptable for the WS6a-precursor role; reviewers should just not read "integration" as "executor wired."

Minor

  • LoopFamily.planner accessor is #[allow(dead_code)]; production code never reads it yet. Add a TODO(WS6a #3596) comment so the allow doesn't ossify.
  • DefaultPlanner is exported via pub use at lib.rs but its constructors (compose_default/compose/with_*) are pub(crate). Public re-export of a type with no public constructor is awkward; gate behind a families:: factory only.

DefaultModelStrategy.fallback_index is read but never advanced by any strategy in this stack — acknowledged in the doc comment referencing master §9. Worth confirming WS6a will own the advancement.

# Conflicts:
#	crates/ironclaw_agent_loop/src/families/mod.rs
#	crates/ironclaw_agent_loop/src/family.rs
#	crates/ironclaw_agent_loop/src/strategies/batch.rs
#	crates/ironclaw_agent_loop/src/strategies/budget.rs
#	crates/ironclaw_agent_loop/src/strategies/capability.rs
#	crates/ironclaw_agent_loop/src/strategies/context.rs
#	crates/ironclaw_agent_loop/src/strategies/drain.rs
#	crates/ironclaw_agent_loop/src/strategies/gate.rs
#	crates/ironclaw_agent_loop/src/strategies/mod.rs
#	crates/ironclaw_agent_loop/src/strategies/model.rs
#	crates/ironclaw_agent_loop/src/strategies/recovery.rs
@henrypark133

Copy link
Copy Markdown
Collaborator Author

Addressed the Level2 review feedback in 18c118264.

What changed:

  • Refreshed Level2 onto the fixed arch/level1-merged base so the PR is mergeable again.
  • Kept the fixed WS4/WS5 integration content in the Level2 tree.
  • DefaultPlanner::compose_default() now uses the non-zero DEFAULT_FAMILY_DIGEST.
  • DefaultStrategySlots::default() composes the real Default*Strategy implementations, including the real default gate and recovery strategies.
  • Added planner-level assertions covering the non-zero default digest, all-kind default gate blocking, and context-overflow shrink retry behavior.

Local validation:

  • cargo test -p ironclaw_agent_loop --lib
  • cargo test -p ironclaw_loop_support --test thread_loop_support_contract
  • cargo test -p ironclaw_turns --test agent_loop_host_contract
  • cargo test -p ironclaw_reborn --test loop_driver_host
  • cargo test -p ironclaw_reborn --test llm_gateway --features root-llm-provider
  • cargo test -p ironclaw_reborn --test loop_milestone_event_projection
  • cargo check -p ironclaw_agent_loop -p ironclaw_reborn
  • cargo fmt --check
  • git diff --check
  • cargo clippy -p ironclaw_agent_loop -p ironclaw_loop_support -p ironclaw_turns -p ironclaw_reborn --all-targets --all-features -- -D warnings

@henrypark133

Copy link
Copy Markdown
Collaborator Author

Follow-up from the final review pass landed in 922e7eea2.

The default family digest now has an explicit DEFAULT_FAMILY_DIGEST_SEED naming DefaultPlanner and all nine real default strategy slots, and the digest bytes were refreshed to the SHA-256 of that seed. I also added a unit test that recomputes the SHA-256 so a stale seed/constant mismatch fails locally.

Additional validation after this follow-up:

  • cargo test -p ironclaw_agent_loop --lib
  • cargo test -p ironclaw_agent_loop --lib default_family_digest_matches_current_seed
  • cargo fmt --check
  • git diff --check
  • cargo clippy -p ironclaw_agent_loop -p ironclaw_loop_support -p ironclaw_turns -p ironclaw_reborn --all-targets --all-features -- -D warnings

# Conflicts:
#	crates/ironclaw_agent_loop/Cargo.toml
#	crates/ironclaw_agent_loop/src/default_planner.rs
#	crates/ironclaw_agent_loop/src/families/mod.rs
#	crates/ironclaw_agent_loop/src/strategies/budget.rs
#	crates/ironclaw_agent_loop/src/strategies/context.rs
#	crates/ironclaw_agent_loop/src/strategies/drain.rs
#	crates/ironclaw_agent_loop/src/strategies/stop.rs
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 scope: dependencies Dependency updates size: XL 500+ changed lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants