feat(turn): add ResolvedAgentExecution::resolve and route prod turn paths - #8179
Conversation
Documents the agent-policy parity harness for the enforcement-unification program: the distinct turn-construction paths, the (setting x path) divergence matrix and the secure-by-default principle (omission is not a grant), the convergence target (one ResolvedAgentExecution::resolve seam, the zeroclaw-labs#8156 trunk), and how the harness in agent/parity.rs grows one surface at a time, asserting only the cross-path parity property no per-primitive or safety_net test covers. Motivated by zeroclaw-labs#8120 (cross-agent MCP tool divergence). Docs-only; registered in the mdbook SUMMARY under Contributing.
Audacity88
left a comment
There was a problem hiding this comment.
Thanks @Nillth. I reviewed the live PR state at 9bbe5ec, the added mdBook page and SUMMARY.md entry, the merged #8120 and #8156 context the page cites, current ResolvedAgentExecution source, and the public CI/check status. I did not run local docs validation for this review-only pass; I relied on the PR's reported docs validation, green public CI, and source review.
🟢 What looks good — The parity-harness scope is useful
The page is otherwise pointed at the right gap. It keeps the harness focused on cross-path parity instead of duplicating per-primitive tests or the safety_net turn-engine oracle, and it explains the secure-by-default rule clearly: omission is not a grant. That is the right principle to preserve once the current/future wording is corrected.
🔴 Blocking — The page presents future seams as current source
The new page needs to distinguish the carrier that #8156 actually merged from the future resolver/harness work this page is describing. Right now it says every path resolves through ResolvedAgentExecution::resolve as "the behavior-neutral trunk introduced by #8156", and the "Adding a surface" steps tell contributors to move each surface into that associated function. Current crates/zeroclaw-runtime/src/agent/turn/execution.rs says the opposite: #8156/G0 is the behavior-neutral regrouping, and later epics move resolution into a single resolve() constructor. There is no ResolvedAgentExecution::resolve today.
The same issue appears in the harness section. It says the parity harness lands in crates/zeroclaw-runtime/src/agent/parity.rs, but current master has agent/safety_net.rs and agent/turn/execution.rs; it does not have agent/parity.rs. Since this PR's value is the contributor-facing architecture/methodology doc, the doc cannot imply those source seams already exist.
Please rephrase the page so the current/future boundary is explicit. For example: #8156 introduced the ResolvedAgentExecution carrier/trunk; future surface PRs should add the resolver/sealed inputs and parity harness, with agent/parity.rs as the intended location if that remains the chosen file shape.
…aths Adds the resolution seam the agent-policy parity doc (zeroclaw-labs#8179) describes, so the doc documents present-tense code instead of future work (addresses the review). - New `ResolvedAgentExecution::resolve(model_access, io, runtime)` constructor in agent/turn/execution.rs, plus the two grouped input layers `ResolvedIo` and `ResolvedRuntimeKnobs` (the future sealed sub-bundles, introduced now as resolve()'s inputs). Re-exported via agent::turn and agent::loop_. - Routes all 7 production turn-construction sites (loop_ run/process_message/agent_turn, Agent::turn x2, delegate, skills review) through resolve(). Pure field spread: behavior-neutral. The ~56 #[cfg(test)] literals keep the flat struct (fields stay pub). - Doc rephrased to mark the current/future boundary and flipped to present tense for resolve() (it exists + every prod path uses it); the parity harness / agent/parity.rs and the per-field resolution + field sealing remain explicitly future. Validation (toolchain 1.93.0): cargo check + clippy --all-targets -D warnings clean; cargo test -p zeroclaw-runtime 2342 passed, 1 failed = the pre-existing cron flaky (zero cron files in the diff; passes in isolation).
|
Good call, @Audacity88 - you were right that the page implied Rather than just soften the wording, I packed the seam into this PR: added I kept the current/future boundary explicit: the parity harness ( The seam is behavior-neutral (a pure field spread; the ~56 test literals are untouched). Validation: clippy |
Audacity88
left a comment
There was a problem hiding this comment.
Thanks @Nillth. I reviewed the live PR state at 2032445, the pushed update after my prior review, the top-level comment, the current captured PR diff against master, the added parity-harness doc, the current ResolvedAgentExecution source, and the linked #8120 / #8156 context. I did not run local Cargo validation for this review-only pass; the remaining issue is visible from source and does not depend on test execution.
✅ Resolved — The resolver and current/future boundary now exist
The earlier doc/source mismatch is addressed for the runtime-side seam. ResolvedAgentExecution::resolve, ResolvedIo, and ResolvedRuntimeKnobs now exist in agent/turn/execution.rs, and the added doc now says agent/parity.rs / the parity harness are future work rather than current source. That fixes the core problem from my prior review: the page no longer describes the parity harness as already present.
🔴 Blocking — The channel construction path still bypasses resolve()
The PR still claims more coverage than the diff provides. The new doc lists Channel | the channels orchestrator as one of the construction paths and then says every production path constructs through ResolvedAgentExecution::resolve. The PR body also frames this as routing every production turn-construction path through the new seam.
But crates/zeroclaw-channels/src/orchestrator/mod.rs still calls run_tool_call_loop(ToolLoop { exec: ResolvedAgentExecution { ... } }) directly. That is production channel code, not a test literal. So the channel path still assembles the execution bundle inline instead of going through ResolvedAgentExecution::resolve(ResolvedModelAccess, ResolvedIo, ResolvedRuntimeKnobs).
That matters because the whole point of this slice is to establish the single seam the parity work can build on. Leaving the channel path outside that seam keeps one of the documented production construction paths in the old shape, and it is the same class of path that made #8120 worth documenting.
Please either route the channel orchestrator through ResolvedAgentExecution::resolve with the same field values, or narrow the doc/PR body so they do not claim every production construction path uses the seam. Given the doc explicitly includes the channel path in the parity matrix, routing the orchestrator through resolve() in this PR is the cleaner fix.
…lve()
The channels orchestrator was the one production turn-construction path still
assembling the engine bundle inline (ResolvedAgentExecution { .. }) instead of going
through ResolvedAgentExecution::resolve, which the parity doc's matrix lists as a
construction path and the PR claims uses the seam.
Route crates/zeroclaw-channels/src/orchestrator/mod.rs through
resolve(ResolvedModelAccess, ResolvedIo, ResolvedRuntimeKnobs) with the same field
values (behavior-neutral), and import the two input types. All 8 production sites (this
one plus the 7 in zeroclaw-runtime) now construct through the single seam. The channel
path is the same class as the zeroclaw-labs#8120 leak, so it belongs inside the seam.
Validation (1.93.0): cargo check -p zeroclaw-channels EXIT 0; clippy --all-targets
-D warnings clean across zeroclaw-channels + zeroclaw-runtime; fmt clean.
|
You're right, thanks @Audacity88 - the channels orchestrator was still assembling the bundle inline, and since the doc's matrix lists the Channel path and the body claimed every production path uses the seam, that was an overclaim. Fixed it the way you suggested: routed Also merged current with master (three landed commits touched files this PR edits, including a channels orchestrator fix) and re-validated on the merged tree: clippy |
singlerider
left a comment
There was a problem hiding this comment.
First review from me. Checked the current head f9fcb080 (newer than @Audacity88's reviewed 2032445c). Read the source; did not run local Cargo. @Audacity88's blocker is resolved at this head; one CI item is still pending.
🟢 RESOLVED at head: the channel path now routes through the seam
@Audacity88's 🔴 was that crates/zeroclaw-channels/src/orchestrator/mod.rs still built ResolvedAgentExecution { ... } inline, bypassing resolve(). At f9fcb080 that call site (orchestrator/mod.rs:4845) is now exec: ResolvedAgentExecution::resolve(...). I swept all production construction paths and they now go through the seam: agent.rs:1970/:2371, loop_.rs:752/:1885/:2394, skills/review.rs:98, tools/delegate.rs:1895, and the channel orchestrator. The remaining inline ResolvedAgentExecution { ... } literals (loop_.rs:4919+) are all past the #[cfg(test)] at loop_.rs:806, i.e. test code, not production paths. So the doc/PR-body claim that every production turn-construction path uses resolve() is now accurate.
🔴 Blocking: CI must go green on this head before merge
The earlier Format check failed on the prior commit, and at f9fcb080 the checks (Format, main, Apply path labels) are currently QUEUED, not yet run. The seam routing is correct in source, but I will not call this merge-ready until Format and the build/test gate pass on the current head, since a cargo fmt --check failure is a hard gate. Push nothing further if CI comes back green; if Format fails again, run cargo fmt --all and repush.
Code substance is sound and @Audacity88's channel blocker is closed. The only open item is green CI on f9fcb080. Once it lands green, this is approvable. Not approving over @Audacity88's active changes-requested; @Audacity88, your channel blocker is resolved at this head if you want to re-check.
Audacity88
left a comment
There was a problem hiding this comment.
Thanks @Nillth. I re-reviewed current head f9fcb08 after the channel-orchestrator follow-up and the merge from current master. The live PR was still CHANGES_REQUESTED because my earlier review was active, so this approval is meant to clear that source-level block. I checked your latest update, @singlerider's review, the current diff and source for the channel orchestrator / runtime / delegate / skill-review call sites, the added parity-harness doc, and the linked #8120 / #8156 context. I did not run local Cargo validation for this review-only pass; this approval is based on source review, the PR's validation evidence, and current GitHub CI status.
✅ Resolved — The channel path now uses the resolver seam
My prior blocker was that crates/zeroclaw-channels/src/orchestrator/mod.rs still built ResolvedAgentExecution { ... } inline while the doc and PR body claimed every production turn-construction path used ResolvedAgentExecution::resolve.
That is resolved at this head. The channel orchestrator now passes exec: ResolvedAgentExecution::resolve(...) with the same model access, I/O, and runtime-knob values. I agree with @singlerider's source sweep here: the production construction sites now route through the seam across Agent::turn, loop_, delegate, skill review, and the channel orchestrator. The remaining direct ResolvedAgentExecution { ... } literals I saw are in test / safety-net code, so they do not contradict the production-path claim.
🟢 What looks good — The current/future boundary is now honest
The doc now distinguishes the current seam from the future per-field resolution, sealing, and parity-harness work. That keeps #8156's carrier and this PR's resolve() constructor from being oversold as complete policy parity, while preserving the important rule that an omitted policy setting is a defect rather than a grant.
🟡 Warning — Wait for current-head CI before merge
This approval clears my source-level blocker; it is not a merge-readiness claim. At live readback, several Quality Gate jobs for f9fcb08 were still queued or in progress. If those all pass, I do not see a remaining blocker from my side. If any check fails, handle that before merge.
…AgentExecution::resolve (#8629) #8179 routed the channel orchestrator's turn construction through the ResolvedAgentExecution::resolve seam. #8011 (per-sender /thinking overrides), branched before that landed, rewrote the same block and re-inlined the flat ResolvedAgentExecution { .. } literal - reverting the routing without meaning to. CI stayed green because resolve() is a pure field spread, so an un-routing is invisible to tests; only the structure regressed. Restore the routing: construct through resolve(ResolvedModelAccess, ResolvedIo, ResolvedRuntimeKnobs) with the exact field values the inline literal used - including thinking.effective_temperature, so #8011's per-sender thinking behavior is untouched. Behavior-neutral by construction. With this, every production turn path again constructs through the one seam the agent-policy parity-harness doc describes (docs/book/src/contributing/ agent-policy-parity-harness.md). Follow-up work makes this class of silent un-routing uncompilable. Validation (toolchain 1.93.0): cargo check -p zeroclaw-channels clean; cargo clippy -p zeroclaw-channels -p zeroclaw-runtime --all-targets -- -D warnings clean (2m27s); cargo fmt --all --check clean.
The parity-harness comments narrated specific merged-PR numbers (zeroclaw-labs#8640/zeroclaw-labs#8700/zeroclaw-labs#8701/zeroclaw-labs#7415/zeroclaw-labs#8179) in free-text prose, which goes stale the moment the next cut-over lands and duplicates what git blame and the row's own tracking/evidence fields already carry. Rewrite the narration in timeless behavioral terms and drop the bare issue numbers, keeping the structural owner_epic/tracking/evidence fields and the Epic-surface naming the harness is organized around. No test or behavior change; the parity matrix and L1/L2 locks are untouched.
Summary
masterResolvedAgentExecution::resolveinagent/turn/execution.rs- the singleconstructor seam the agent-policy parity-harness design page describes - and routes
every production turn-construction path through it. refactor(turn): bundle per-agent ToolLoop fields into ResolvedAgentExecution #8156 landed the flat
ResolvedAgentExecutioncarrier but left it assembled inline at each call site; thiscollapses that into one seam.
ResolvedIo(the per-turn I/O wiring: toolregistry, observer, approval, hooks, receipts, multimodal, activation set,
model-switch callback) and
ResolvedRuntimeKnobs(the values derived from theagent's resolved config: iteration cap, excluded/dedup-exempt tools, pacing,
parsing/parallel flags, budgets, loop knobs). Named
ResolvedRuntimeKnobs(notResolvedRuntime) so it does not shadow the existingzeroclaw_config::schema::ResolvedRuntime. These are the future sealed sub-bundles,introduced now as
resolve()'s inputs. Re-exported viaagent::turn+agent::loop_.resolve(), across bothcrates: the channels orchestrator (
zeroclaw-channels),loop_(run /process_message / agent_turn),
Agent::turn(x2),tools/delegate,skills/review.The channel path is the same class of construction site that fix(mcp): scope MCP tools per-agent and enforce the denylist #8120 was about, so it is
routed here rather than left outside the seam.
resolve()and its inputlayers (they exist + every prod path uses them), with the current/future boundary made
explicit. The harness (
agent/parity.rs), per-field resolution, and field sealing staymarked as later surface PRs.
documents present-tense code. This resolves the review on the doc (it referred to
resolve()/ sealed inputs as if they already existed).change:
resolve()is a pure field spread, and the#[cfg(test)]literals are untouched(the bundle's fields stay
pub).run_tool_call_loop's signature isunchanged; the channels orchestrator now constructs through
resolve()(so it imports thenew input types); no other consumer is affected.
cross-agent tool divergence the seam exists to make uncompilable).
agent,tool,skills,runtime,docs,size: M,risk: medium.Validation Evidence (required)
Toolchain pinned to 1.93.0 (matches CI). Branch merged current with
upstream/master(clean auto-merge, no conflicts) and validated on the MERGED tree, since CI builds the PR
merged with base. Literal output:
cargo fmt --all -- --check: clean.cargo clippy -p zeroclaw-channels -p zeroclaw-runtime --all-targets -- -D warnings:Finishedin 58.21s, EXIT 0 (compiles the new channels routing plus the ~56#[cfg(test)]literals).cargo check -p zeroclaw-channels -p zeroclaw-runtimeon the merged tree: EXIT 0.validate.shworkspace battery (fmt + clippy--all-targets+cargo test, the CI-equivalent set):RESULT: PASS(0 blocking, 0 warning); the knowncron log-capture flaky did not fire this run.
scripts/ci/docs_quality_gate.sh: no prose em-dashes, markdownlint 0 errors.Security & Privacy Impact (required)
resolve()spreads the same already-resolved values each site computed,so approval policy, excluded tools, and every other policy field reach the engine exactly
as before. No trust-boundary or default change. The doc references only merged, public
facts and names no still-unmitigated divergence.
Compatibility (required)
resolve()and the two input structs are additive; thebundle's
pubfields are unchanged, so existing constructors (including the testliterals) still compile. The channels orchestrator additionally imports
ResolvedIo/ResolvedRuntimeKnobsto construct through the seam.Rollback (required for
risk: mediumandrisk: high)Low risk:
git revert <squash-sha>. Three self-contained commits (the doc, the seam +runtime routing, then the channels-orchestrator routing) on top of the merged #8156 carrier;
nothing else depends on
resolve()yet.