Skip to content

arch(level1-merged): integrate ws-1 + ws-2 + ws-3 + ws-3.5 - #3554

Closed
henrypark133 wants to merge 38 commits into
reborn-integrationfrom
arch/level1-merged
Closed

henrypark133 wants to merge 38 commits into
reborn-integrationfrom
arch/level1-merged

Conversation

@henrypark133

@henrypark133 henrypark133 commented May 13, 2026 •

Copy link
Copy Markdown
Collaborator

Context

Integration branch for the first parallel layer above WS0. It combines alpha/beta/gamma strategy traits and the loop-family registry so downstream planner/default-strategy work has one coherent base.

Master spec: docs/reborn/agent-loop-skeleton.md
Workstream brief: docs/reborn/agent-loop-skeleton.md
Stack base: arch/ws-0

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-1, arch/ws-2, arch/ws-3, and arch/ws-3.5 on top of the current WS0.
  • Resolved shared strategy-module and family-registry conflicts produced by the current WS0 prompt-authority fix.
  • Produced a single downstream base for WS4 and WS5.
  • Kept integration-only scope: no additional product/runtime behavior beyond making the workstreams compose.

Reviewer focus

  • Cross-branch strategy exports and module visibility.
  • Registry/family naming consistency with the skeleton spec.
  • Whether this branch contains only integration conflict resolution and no unrelated refactor.

Non-goals / deferred work

  • Planner facade behavior lands in WS4.
  • Default strategy implementations land in WS5.
  • Executor/runtime behavior lands later.

Validation

  • cargo check -p ironclaw_agent_loop -p ironclaw_reborn
  • Final ancestry check verified arch/ws-0 -> arch/level1-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 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 introduces the strategy trait contracts for the agent-loop framework, defining traits and associated types for batch policy, budget, capability filtering, context planning, input draining, gate handling, recovery, and stop conditions. The review feedback highlights a naming contradiction in the UnlimitedBudget implementation, a misplaced test in drain.rs, and an inconsistency in the serialization tagging of the StopOutcome enum relative to other strategy outcomes.

Comment thread crates/ironclaw_agent_loop/src/strategies/budget.rs Outdated
Comment thread crates/ironclaw_agent_loop/src/strategies/budget.rs
Comment thread crates/ironclaw_agent_loop/src/strategies/drain.rs Outdated
Comment thread crates/ironclaw_agent_loop/src/strategies/stop.rs Outdated
Comment thread crates/ironclaw_agent_loop/src/strategies/stop.rs Outdated
Comment thread crates/ironclaw_agent_loop/src/strategies/stop.rs Outdated
… on rebase)

- Split ControlStrategyState into StopStrategyState + GateStrategyState
  (slots.rs); LoopExecutionState now carries both as independent slots.
- Add LoopFailureKind::PolicyDenied with snake_case serde + #[non_exhaustive]
  on the enum; downstream matchers in reborn updated to handle the
  non-exhaustive shape with a fail-closed wildcard.
- JCS RFC 8785 canonicalization for CapabilityCallSignature::from_call via
  the serde_jcs crate; from_call is now fallible (returns Result) and
  rejects non-finite numbers (NaN/Infinity) via an explicit guard.
- LoopExecutionState::from_checkpoint_payload signature flipped from
  &serde_json::Value to (&[u8], kind: CheckpointKind); envelope carries
  schema_id + kind metadata so the boundary the checkpoint was taken at is
  authenticated on resume.
- LoopContextMessage.message_ref is now Option<LoopMessageRef>; None means
  "summary-only entry; prompt port MUST NOT resolve content from
  safe_summary." Call sites in loop_support, reborn, and tests updated to
  wrap in Some(...) on writes and filter_map on reads.
- LoopCheckpointPort: removed the premature load_checkpoint_payload stub
  (WS-10 owns it); added stage_checkpoint_payload(
  StageCheckpointPayloadRequest { schema_id, payload }
  ) -> LoopCheckpointStateRef with a fail-closed default impl.
- ConcurrencyHint { SafeForParallel, Exclusive } added to
  ironclaw_turns::run_profile::host; CapabilityDescriptorView gains a
  concurrency_hint field. All struct-literal constructors updated
  (defaulting to Exclusive until WS-9 derives from CapabilityDescriptor.effects).
- Tests: JCS-stable across pretty/minified, nested-shuffled, key-reordering;
  grep-style assertion that LoopExecutionState has no control_state;
  StopStrategyState / GateStrategyState default round-trip; PolicyDenied
  serializes as "policy_denied"; checkpoint kind-mismatch path.
- Cargo.toml: add serde_jcs + blake3 deps; drop siphasher (the hand-rolled
  canonicalization is replaced wholesale).

Rebased onto docs HEAD 93f0865 to incorporate:
ebd2dc9 ca648b3 49d1506 2b20998 4c12192 1f808fa e48a584 93f0865
…xes)

Address two P2 findings from codex review:
- StageCheckpointPayloadRequest now carries LoopCheckpointKind so adapters
  can bridge to CheckpointStateStore::put_checkpoint_state without guessing.
- HostManagedLoopCheckpointPort and RebornLoopDriverHost both implement
  stage_checkpoint_payload; the trait's default Unavailable body remains as
  defense-in-depth.
…text (codex iter 2 fixes)

Three findings from codex review:
- from_checkpoint_payload now reads raw state bytes (matches the
  staging contract); metadata stays out of the payload.
- stage_checkpoint_payload returns a run-scoped LoopCheckpointStateRef
  (checkpoint:{run_id}:{token}); is_for_run validators no longer reject.
- HostManagedLoopPromptPort materializes summary-only LoopContextMessage
  entries from safe_summary instead of dropping them via filter_map.
# Conflicts:
#	crates/ironclaw_agent_loop/src/strategies/mod.rs
# Conflicts:
#	crates/ironclaw_agent_loop/src/strategies/mod.rs
@henrypark133 henrypark133 changed the title arch(level1-merged): integrate ws-1 + ws-2 + ws-3 arch(level1-merged): integrate ws-1 + ws-2 + ws-3 + ws-3.5 May 14, 2026
@serrrfirat

Copy link
Copy Markdown
Collaborator

Summary

Reviewed Level1 integration PR #3554 only.
Base 3db2d97bf1718554c8936e2ad4346794f02b7ec3 → head 22a8e1d4df8022962a2a9911100d47e5c397c816.

Merge stance: no unique Level1 integration finding beyond WS3.5 issues already posted on #3643.

Tests run:

  • cargo test -p ironclaw_agent_loop --lib ✅

Findings

# Sev Category File:Line Issue Fix suggestion
inherited Medium Correctness / Versioning #3643 This integration PR inherits #3643’s zero-digest ComponentIdentity issue. Fix in #3643/source branch.
inherited Medium Boundary / Registry integrity #3643 This integration PR inherits #3643’s duplicate family-id silent overwrite issue. Fix in #3643/source branch.

Security/data-flow notes

  • Strategy contracts remain crate-internal and sanitized.
  • No unique raw input/secret/auth expansion found in Level1 merge glue.

Correctness/invariant notes

  • Family registry/versioning concerns are inherited from WS3.5, not newly introduced by merge resolution.

Missing tests

Suggested fixes

  1. Fix inherited arch(ws-3.5): add loop family registry #3643 findings at source, then rebase this integration branch.

@zmanian

zmanian commented May 14, 2026

Copy link
Copy Markdown
Collaborator

Review notes — Level1 integration

Mechanical join is clean; nothing to propagate back into WS1/WS2/WS3 individually. One ask before promoting out of draft.

Validation bar is too low

The PR checklist runs cargo check -p ironclaw_agent_loop -p ironclaw_reborn. Per repo CLAUDE.md the bar is fmt + clippy + tests; for an integration PR whose job is to prove four trait sets compose, "compiles" understates the verification. Recommend cargo test -p ironclaw_agent_loop -p ironclaw_reborn plus cargo clippy --all --tests before un-drafting.

One cross-trait state-composition smoke test

Every test here is unit-level: object-safety + serde round-trip per strategy file. There is no test that simultaneously drives multiple strategies through a shared LoopExecutionState transition — e.g., GateOutcome::Block carrying GateStrategyState round-trips through state, or that the recovery re-export in strategies/mod.rs actually composes with WS0's state types. Defensible because executor behavior lands in WS6, but worth one smoke test demonstrating the four PRs compose at the data layer, not just the type layer.

Minor

  • app_loop_family.rs test asserts the registry binds default but doesn't assert the family's version().digest is the all-zero placeholder (which is what families::default() actually returns). If WS-5 swaps in real digests this test silently keeps passing. Sharper assertion would catch the drift.
  • LoopFamilyId::new takes Cow<'static, str> with no validation (empty, length cap, character set), unlike sibling typed IDs (CapabilityId::new returns Result). Inconsistent with the typed-ID pattern.
  • pub(crate) use ironclaw_turns::run_profile::ConcurrencyHint; in strategies/mod.rs is an odd re-export — drop it; callers can import directly.

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

Copy link
Copy Markdown
Collaborator Author

Addressed the Level1 integration feedback in fa888339a.

What changed:

  • Merged the fixed WS0, WS1, WS2, WS3, and WS3.5 heads into Level1.
  • Added a cross-strategy smoke test that applies gate/recovery/stop outcomes into one LoopExecutionState and round-trips the composed state.
  • Switched StopOutcome to the same internally tagged outcome wire shape as the other strategy outcomes and updated the stop tests.
  • Removed the ConcurrencyHint re-export from strategies/mod.rs.
  • Carried the WS3.5 registry fixes into Level1: validated LoopFamilyId, duplicate family IDs return a typed error, and the default family digest is non-zero and asserted.
  • Updated Reborn milestone tests to build prompt bundles through the host prompt port before stream_model.
  • Tightened model-visible redaction so bare env-var names remain readable while assigned credential values are redacted.

Local validation passed:

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

GitHub checks are running on the new head.

Base automatically changed from arch/ws-0 to reborn-integration May 15, 2026 00:20
@henrypark133

Copy link
Copy Markdown
Collaborator Author

Closing pull request. the changes are already merged in.

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