feat(runtime): add host-requested graceful tool-loop finalizer

[Kevin-K-W]

Summary

• Base branch: memorybox. PR feat(providers): stable MemBox prompt-cache boundary for Anthropic #25 has merged and is already included in this base; this PR contains the graceful-stop runtime work only.
• What changed and why:
  • Hosts can request a one-shot GracefulStopSignal (NoProgress or MaxIterations) through task-local GRACEFUL_STOP, independently of hard cancellation.
  • After every tool already admitted to the current wave finishes, the loop makes exactly one final provider request with tools=None, allowing a non-empty answer from evidence already gathered.
  • User cancellation has deterministic precedence before, during, and immediately after the final provider call. Empty or protocol-shaped text, native tool calls, timeout, provider failure, and budget rejection remain errors.
  • Reason-specific telemetry and errors preserve the distinction between NoProgress, maximum iterations, and budget failures.
• Scope boundary: This PR does not change MemBox no-progress thresholds, hard-stop policy, user-visible terminal mapping, or protect MemBox from Done(""); those belong to the MemBox integration follow-up.
• Blast radius: Shared tool-loop finalization in zeroclaw-runtime and the exported signal API in zeroclaw-api. Unscoped callers retain existing behavior; hosts opt into NoProgress finalization by scoping and requesting the task-local signal.
• Linked issue(s): Related MemVerge/MemBox#3412. Related feat(providers): stable MemBox prompt-cache boundary for Anthropic #25 (merged; included in the current base).
• Labels: enhancement

Testing (required)

How you can test (when useful)

• Reviewer testing requested? N/A — this PR adds a deterministic runtime contract without a standalone user-facing CLI, TUI, or web flow. End-to-end MemBox behavior will be tested in the integration follow-up.

How I tested

The focused tests cover:

• one-shot signal visibility and first-request-wins semantics;
• completing every tool already admitted to the current parallel wave;
• preventing a subsequent ordinary provider/tool wave;
• issuing exactly one final provider request with tools=None;
• preserving the same non-empty finalizer text in the returned result and emitted event;
• cancellation before the finalizer, while waiting, and when cancellation occurs during the provider poll immediately before a response is returned;
• rejecting empty text, protocol-shaped text, and native tool calls;
• preserving timeout, provider, and USD-budget errors; and
• reason-aware NoProgress telemetry.

Current-head validation (head 4e9f16d):

cargo fmt --all -- --check
cargo clippy -p zeroclaw-api --all-targets -- -D warnings
cargo clippy -p zeroclaw-runtime --all-targets -- -D warnings
cargo test -p zeroclaw-api graceful_stop --lib
cargo test -p zeroclaw-runtime graceful --lib

• cargo fmt --all -- --check: exit 0.
• cargo clippy -p zeroclaw-api --all-targets -- -D warnings: exit 0.
• cargo clippy -p zeroclaw-runtime --all-targets -- -D warnings: exit 0.
• cargo test -p zeroclaw-api graceful_stop --lib: 2 passed, 0 failed.
• cargo test -p zeroclaw-runtime graceful --lib: 14 passed, 0 failed, including the post-provider-poll cancellation and real loop-boundary regressions.
• CI checks relied on and why they cover this change: None for Rust. The visible Apply path labels and PR-title checks pass but do not compile, lint, or test the changed crates.
• Known CI coverage gap, if any: The repository Quality Gate is configured only for pull requests targeting master, so it does not run on this PR targeting memorybox.
• Commands run and tail output: See the current-head command summary above and the complete output in the PR conversation.
• Beyond CI, what did you manually verify? Static review verified cancellation precedence, loop-boundary ordering, finalizer protocol rejection, usage/history/event ordering, and reason-specific error/telemetry handling. No live GLM/Doubao retrieval loop or MemBox integration was run in this PR.
• If any command was intentionally skipped, why: Live MemBox behavior is outside this repository change and will be validated after the ZeroClaw pin and synchronous ToolResult-boundary wiring land in MemBox.

Security & Privacy Impact (required)

• New permissions, capabilities, or file system access scope? (No)
• New external network calls? (No)
• Secrets / tokens / credentials handling changed? (No)
• PII, real identities, or personal data in diff, tests, fixtures, or docs? (No)
• Prompt injection or untrusted model-visible text introduced/changed? (Yes)
• If any Yes, describe the risk and mitigation: The finalizer adds a host-owned, localized prompt instructing the provider to answer only from evidence already gathered. It does not introduce a new untrusted input source. Protocol-shaped text and native tool calls returned from the tools=None request are rejected rather than executed.

Compatibility (required)

• Backward compatible? (Yes)
• Config / env / CLI surface changed? (No)
• Rust/MSRV/toolchain floor changed? (No)
• If backward compatibility is No or either surface/floor question is Yes: N/A

Rollback (required for medium/high-risk PRs)

• Fast rollback command/path: Revert the merge commit for this PR; remove the MemBox signal wiring first if the downstream integration has already landed.
• Feature flags or config toggles: None. The host-requested NoProgress path is opt-in through task-local signal scoping.
• Observable failure symptoms: A provider result is accepted after user cancellation, a new ordinary tool wave starts after a graceful-stop request, the final provider call receives tools, or an invalid/empty finalizer result is exposed as successful output.

[sheperdh]

Review summary

Reviewed current head bf7409b against the MemBox zeroclaw-labs#3412 requirement.

The overall owning-layer split looks right: MemBox should own the no-progress classifier and hard budgets, while ZeroClaw provides a per-turn graceful-stop primitive and a single tools-free finalizer. The no-signal path is opt-in and the finalizer correctly rejects empty text, native tool calls, and textual tool protocol.

I do not think the current head is ready to pin in MemBox yet, because one hard-stop regression is reproducible and the claimed wave-boundary behavior is not covered at the real loop boundary.

🔴 Blocking — user cancellation can lose the race to the finalizer

Both finalizer wait paths use an unbiased tokio::select!. When the cancellation token is already cancelled and the provider future is immediately ready, either branch may win. The loop also checks GRACEFUL_STOP before checking the cancellation token, so a pending graceful stop can enter this race instead of returning ToolLoopCancelled immediately.

I reproduced this on the reviewed head. The focused test passed once, but failed under repetition, and then failed again on a direct run:

thread 'agent::turn::max_iter::graceful_summary_metering_tests::cancel_during_finalizer_returns_cancelled' panicked at .../max_iter.rs:724:9:
assertion failed: result.is_err()

Reproduction:

for iteration in {1..10}; do
  RUSTC_WRAPPER= cargo test -q -p zeroclaw-runtime \
    cancel_during_finalizer_returns_cancelled --lib -- --nocapture || exit 1
done

The full zeroclaw-runtime --lib run also hit the same failure. Please give cancellation deterministic precedence:

• check an already-cancelled token before entering the graceful finalizer;
• check hard cancellation before requested_graceful_stop() at the loop boundary; and
• use a cancellation-biased wait so simultaneous readiness cannot accept a provider result after user cancellation.

Relevant code: turn/mod.rs, turn/max_iter.rs.

🔴 Blocking — test the signal at the actual tool-loop boundary

The new tests prove that an already-set task-local signal is visible and that direct finalizer calls use tools=None. They do not prove the primary behavioral contract: a signal requested while a real tool loop is running must finish the current tool wave, execute no tool from wave N+1, and issue exactly one tools-free final model call.

Please add a run_tool_call_loop regression test in which a tool (or a synchronous host hook at the tool-result boundary) requests NoProgress, then assert:

1. all calls already admitted to the current wave complete;
2. no next-wave tool executes;
3. exactly one final provider call is made with tools == None; and
4. the returned and emitted answer is the same non-empty finalizer text.

This is important because MemBox currently observes ToolResult through asynchronous channels. A host that sets the signal only after consuming the outer event can lose the checkpoint race and allow another provider/tool wave to begin. The MemBox follow-up must therefore request the shared signal synchronously from its StreamEventTool/tool-result boundary, while token, model-call, time, step, and user-cancel stops continue to use hard cancellation.

🟡 Warning — preserve the graceful-stop reason on finalizer failure

Provider errors and unusable finalizer responses currently return Agent exceeded maximum tool iterations (...) even when the reason is NoProgress. That misclassifies telemetry and makes host error mapping depend on side state. Please preserve NoProgress in the error contract, preferably through a typed error or, at minimum, a reason-specific stable message.

Relevant code: turn/max_iter.rs, commit_finalizer_response.

🟡 Warning — end-to-end empty-success protection still belongs in MemBox

ZeroClaw correctly returns an error for empty/invalid finalizer output. However, current MemBox runaway handling can still convert a pending-runaway error with an empty partial answer into a successful Done(""). The MemBox pin-bump/wiring follow-up must only keep a non-empty finalizer answer; empty output, provider error, timeout, or budget rejection must remain a visible error.

This is not a request to move the MemBox classifier into ZeroClaw. The clean ownership remains:

• ZeroClaw: signal, safe checkpoint, one tools=None finalizer, cancellation/error semantics;
• MemBox: no-progress detection, soft-vs-hard stop policy, user-visible terminal mapping.

🟡 Warning — advertised CI evidence is not present

The PR currently has only Validate PR title and Apply path labels checks; no Rust test/clippy job ran, although the PR body says it relies on repo Rust test/clippy CI. Also, cargo fmt --all -- --check fails, including formatting differences in files touched by this PR (and additional inherited differences from the base branch).

Local evidence on the reviewed head:

• cargo test -p zeroclaw-api graceful_stop --lib: 2 passed;
• cargo test -p zeroclaw-runtime graceful_summary --lib: 11 passed in one run, but the cancellation test is flaky and reproducibly fails under repetition;
• cargo test -p zeroclaw-runtime --lib: 3486 passed, 35 failed; 34 failures were environment/sandbox-related, and the remaining directly relevant failure was the cancellation race above;
• cargo fmt --all -- --check: failed.

🟢 What looks good — narrow, opt-in graceful-stop seam

• The signal is per turn and opt-in; normal loops without a scoped signal retain their ordinary control flow.
• The finalizer uses the same resolved provider/history and sends tools=None.
• Empty, protocol-shaped, and native-tool-call finalizer outputs fail closed.
• Cost metering, timeout, output bounding, history persistence, and ordered chunk emission are handled at the ZeroClaw owner.
• No MemBox thresholds or hard-budget semantics are embedded into the shared runtime.

Once the cancellation precedence and real loop-boundary test are fixed, this PR should be a good ZeroClaw foundation for the MemBox zeroclaw-labs#3412 follow-up. @Kevin-K-W

[Kevin-K-W]

Addressed the blocking review notes on this follow-up commit:

• Cancellation precedence: loop checks an already-cancelled token before GRACEFUL_STOP; the same check runs again before entering the finalizer; both select! waits are biased so cancel wins over a ready provider future. cancel_during_finalizer_returns_cancelled passed 10/10 repeats locally.
• Loop-boundary coverage: no_progress_signal_finishes_current_wave_then_tools_none drives run_tool_call_loop. Wave-1 tools search_a/search_b both execute (search_a requests NoProgress); the next provider call is the only tools=None finalizer; returned and emitted text match.
• NoProgress error contract: unusable/provider-error finalizer failures now use Agent stopped: retrieval made no progress instead of the max-iteration string.

MemBox empty-Done mapping stays in the pin-bump follow-up (already implemented there: empty keep-answer → TOOL_LOOP_ABORTED). This fork still does not run the full Rust CI suite on this PR path; local evidence is the focused crate tests above.

[sheperdh]

Follow-up review

Reviewed updated head 242d40ff39bcb7d13207968111cbe13ba63b25b2 against the previous blocking comments and the MemBox zeroclaw-labs#3412 graceful-finalization contract.

The real loop-boundary coverage is now good, the ordinary pre-cancelled race is fixed, and NoProgress failures no longer reuse the max-iteration error. One cancellation race remains reproducible, so I do not think this head is ready to approve yet.

🔴 Blocking — cancellation can still lose when it becomes ready during the provider poll

The new pre-checks and biased; tokio::select! fix the previously reported case where cancellation is already ready before entering the finalizer. The existing test now passes repeatedly (50/50 locally).

However, biased only controls the order in which branches that are ready when polled are selected. If the provider future causes or observes cancellation during its poll and then returns a response, the response branch can still win and commit_finalizer_response accepts the text without another cancellation check.

I reproduced this deterministically with a local diagnostic provider whose chat implementation performs:

self.token.cancel();
Ok(ChatResponse {
    text: Some("answer after cancellation".to_string()),
    tool_calls: Vec::new(),
    usage: None,
    reasoning_content: None,
})

Expected: ToolLoopCancelled.

Actual:

cancelled finalizer must not commit provider output:
Ok("answer after cancellation")

Relevant waits: max_iter.rs; response commit: max_iter.rs.

Please add a cancellation check after await_finalizer_response() returns and before accepting any Done result, mutating history, or emitting the final chunk. A regression test should cancel immediately before the provider returns rather than only pre-cancelling the token.

🟡 Warning — budget/timeout errors and telemetry still lose the real reason

• run_model_query can return the explicit Budget exceeded: ... error before making a provider call, but SummaryCall::Done(Err(error)) replaces it with the generic NoProgress/max-iteration failure. This hides the actionable budget cause from the caller; the test currently checks only that an error occurred and the provider was skipped.
• A NoProgress finalizer still logs tool_loop_exhausted, Action::Fail, and Max iterations reached, requesting final summary before the finalizer runs, even when it later succeeds. That can misclassify NoProgress traffic and inflate failure telemetry.

Please preserve the budget/timeout cause in the returned error and make the start telemetry reason-aware. Relevant code: finalizer_failure_message and log_finalizer_start.

✅ Previous loop-boundary blocker is resolved

The new run_tool_call_loop regression now proves the important behavior:

• both tools already admitted to the current parallel wave complete;
• no ordinary next-wave provider/tool call starts;
• exactly one final provider call is made with tools=None; and
• returned and emitted finalizer text agree.

The direct NoProgress tests also cover the reason-specific prompt and rejection of empty, protocol-shaped, and native-tool-call responses.

Local verification

• cargo test -p zeroclaw-api graceful_stop --lib: 2 passed
• cargo test -p zeroclaw-runtime graceful --lib: 13 passed
• Existing cancellation test repeated 50 times: 50 passed
• Added same-poll cancellation diagnostic: reproduces the blocking failure
• cargo test -p zeroclaw-runtime --lib: 3522 passed, 2 ignored
• cargo fmt --all -- --check: still fails only in inherited #25 files

The PR still has only path-label and PR-title checks; no Rust Quality Gate ran for this branch path.

Recommendation

This update is substantially closer and resolves the real wave-boundary coverage gap. Please add the post-finalizer cancellation check and regression test before approval. The MemBox pin/wiring follow-up still needs to request the signal synchronously at the ToolResult boundary and retain the empty-Done protection.

[Kevin-K-W]

Addressed the follow-up review on this amended commit:

• Post-await cancel check: after await_finalizer_response() returns Done, we check the token again before mutating history or emitting a chunk. biased select! can still pick a provider Ready if chat() cancelled during its own poll; that path now returns ToolLoopCancelled instead of committing. Regression: cancel_during_provider_return_does_not_commit_answer (provider cancel() then Ok("answer after cancellation")).
• Budget errors: Budget exceeded: ... from run_model_query is returned unchanged instead of being rewritten as a generic NoProgress/max-iter failure. The budget-gate test now asserts that string.
• NoProgress telemetry: start logs no longer emit tool_loop_exhausted / Action::Fail / “Max iterations reached…” for NoProgress. That path logs No progress detected, requesting final summary with Action::Note.

Pre-cancelled cancel_during_finalizer_returns_cancelled is unchanged. MemBox empty-Done mapping remains in the pin-bump follow-up.

[sheperdh]

Follow-up review

Reviewed current head 072651d after #25 merged and this branch was rebased onto memorybox. The PR is now mergeable and clean. The graceful-stop runtime files are unchanged from the amended implementation reviewed at a2e50be.

✅ Resolved — cancellation after the provider poll

The previous blocking race is fixed. After await_finalizer_response() returns Done, the runtime checks the cancellation token again before accepting the result, mutating history, or emitting the final chunk. The new cancel_during_provider_return_does_not_commit_answer regression covers a provider that cancels the token during its own chat() poll and then returns text.

✅ Resolved — budget errors and NoProgress telemetry

• ZeroClaw USD-budget errors are returned unchanged instead of being rewritten as a generic NoProgress or max-iteration failure.
• NoProgress finalization no longer emits tool_loop_exhausted, Action::Fail, or the max-iteration start message. It uses reason-aware Action::Note telemetry.
• Timeout, unusable text, protocol-shaped text, native tool calls, provider failure, and cancellation continue to terminate as errors rather than empty success.

✅ Resolved — loop-boundary contract

The run_tool_call_loop regression proves that every tool already admitted to the current parallel wave completes, no next ordinary provider/tool wave starts, exactly one final request is sent with tools=None, and the returned and emitted finalizer text agree.

🟡 Warning — refresh the PR description and current-head validation evidence

The live description is stale after the rebase:

• the actual base is now memorybox; #25 is merged, so the old membox-prompt-cache-boundary / 39cc134a base statement and Depends on #25 wording are no longer accurate;
• the pasted test output does not include the new post-await cancellation regression;
• the description says it relies on repository Rust test/clippy CI, but the visible checks are still only path labeling and PR-title validation; and
• cargo fmt and Clippy are described as deferred to CI even though those jobs did not run for this PR path.

Please update the description and record current-head results for cargo fmt --all -- --check, strict Clippy for the two touched crates, and the focused graceful-stop/runtime tests before approval.

🟡 Warning — this remains the ZeroClaw foundation, not the complete zeroclaw-labs#3412 fix

The MemBox pin/wiring follow-up still needs to request GRACEFUL_STOP synchronously at the ToolResult boundary, retain immediate cancellation for token/time/model-call budgets and explicit user cancellation, and reject empty Done output. The TimeBudget blank-response case reported in MemVerge/MemBox#3412 also needs a visible MemBox error path. No live GLM/Doubao retrieval-loop baseline/candidate comparison is included here, so the end-to-end product outcome remains unvalidated even though the deterministic ZeroClaw contract is now sound.

🟢 What looks good — ownership and default behavior remain narrow

• ZeroClaw owns the safe wave boundary, provider history, one tools-free finalizer, usage metering, timeout, protocol rejection, and cancellation precedence.
• MemBox remains responsible for classifying NoProgress, choosing soft versus hard stops, and mapping terminal outcomes to the UI.
• The signal is task-local and opt-in; unscoped callers retain the existing max-iteration behavior.

Recommendation

I found no remaining code blocker in the rebased runtime implementation. After the PR description and current-head validation evidence are corrected, this should be ready to approve as the ZeroClaw foundation for the MemBox follow-up. @Kevin-K-W

[sheperdh]

@Kevin-K-W The implementation blockers from the previous reviews are resolved on current head 072651d. I have refreshed the PR description to match the rebased branch and the actual CI coverage.

The repository Quality Gate only runs for pull requests targeting master, so the visible checks on this PR do not compile, lint, or test the changed Rust crates. Could you please run the following commands on the current head and paste the relevant output tails here?

cargo fmt --all -- --check
cargo clippy -p zeroclaw-api --all-targets -- -D warnings
cargo clippy -p zeroclaw-runtime --all-targets -- -D warnings
cargo test -p zeroclaw-api graceful_stop --lib
cargo test -p zeroclaw-runtime graceful --lib

Once those current-head results are recorded, the remaining validation-evidence gap should be closed and the PR should be ready for approval as the ZeroClaw foundation for the MemBox zeroclaw-labs#3412 follow-up.

[Kevin-K-W]

Current-head validation on 4e9f16d8 (import-order rustfmt only vs 072651db; no runtime logic change).

Quality Gate does not run for PRs targeting memorybox, so these are local results:

cargo fmt --all -- --check
# exit 0

cargo clippy -p zeroclaw-api --all-targets -- -D warnings
# Finished `dev` profile … (exit 0)

cargo clippy -p zeroclaw-runtime --all-targets -- -D warnings
# Checking zeroclaw-runtime … Finished `dev` profile [unoptimized + debuginfo] target(s) in 3m 18s
# exit 0

cargo test -p zeroclaw-api graceful_stop --lib

running 2 tests
test graceful_stop::tests::already_set_signal_is_not_missed ... ok
test graceful_stop::tests::first_request_wins_and_is_visible ... ok
test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 141 filtered out

cargo test -p zeroclaw-runtime graceful --lib

running 14 tests
test agent::turn::max_iter::graceful_summary_metering_tests::requested_graceful_stop_sees_already_set_signal ... ok
test agent::turn::max_iter::graceful_summary_metering_tests::cancel_during_finalizer_returns_cancelled ... ok
test agent::turn::max_iter::graceful_summary_metering_tests::graceful_summary_is_budget_gated_and_skips_the_provider_when_over_budget ... ok
test agent::turn::max_iter::graceful_summary_metering_tests::cancel_during_provider_return_does_not_commit_answer ... ok
test agent::turn::max_iter::graceful_summary_metering_tests::native_tool_calls_on_finalizer_return_error ... ok
test agent::turn::max_iter::graceful_summary_metering_tests::empty_finalizer_text_returns_error ... ok
test agent::turn::max_iter::graceful_summary_metering_tests::no_progress_finalizer_uses_prompt_and_tools_none ... ok
test agent::turn::max_iter::graceful_summary_metering_tests::protocol_shaped_finalizer_text_returns_error ... ok
test agent::turn::max_iter::graceful_summary_metering_tests::graceful_summary_strips_tool_audio_marker_before_dispatch ... ok
test agent::turn::max_iter::graceful_summary_metering_tests::graceful_summary_records_usage_through_the_metered_seam ... ok
test agent::turn::max_iter::graceful_stop_loop_tests::no_progress_signal_finishes_current_wave_then_tools_none ... ok
test agent::agent::safety_net::safety_net_failed_graceful_summary_does_not_persist_prompt ... ok
test agent::agent::safety_net::safety_net_graceful_summary_persists_assistant_summary ... ok
test agent::tests::turn_handles_unknown_tool_gracefully ... ok
test result: ok. 14 passed; 0 failed; 0 ignored; 0 measured; 3511 filtered out

cargo fmt --all -- --check on 072651db failed only on import grouping in max_iter.rs (the files from #25 are clean on memorybox). That grouping is what this amend fixes.

[sheperdh]

Review summary

Reviewed current head 4e9f16d against the prior findings and the ZeroClaw contract needed by the MemBox zeroclaw-labs#3412 follow-up. The head is mergeable and clean, all prior implementation blockers are resolved, and the final amend contains only rustfmt import ordering changes relative to the previously reviewed runtime implementation.

✅ Resolved — cancellation precedence and finalizer safety

• Cancellation is checked before the graceful-stop boundary, before entering the finalizer, in cancellation-biased waits, and once more after the provider future completes but before history mutation or event emission.
• The post-provider-poll regression confirms that a provider cannot cancel the token and then commit a late answer.
• Empty output, protocol-shaped text, native tool calls, provider failure, timeout, and budget rejection continue to fail closed.

✅ Resolved — real loop-boundary behavior

The loop-level regression verifies that every tool already admitted to the current parallel wave completes, no subsequent ordinary provider/tool wave starts, exactly one final request is made with tools=None, and returned and emitted finalizer text agree.

✅ Resolved — error, budget, and telemetry semantics

USD-budget errors remain actionable instead of being rewritten as NoProgress or max-iteration failures. NoProgress uses reason-specific Action::Note telemetry, while max-iteration exhaustion retains its existing failure signal.

🟢 What looks good — focused scope and proportionate coverage

The change is larger than 1,000 diff lines, but it remains one cohesive runtime contract across six files, is concentrated in the existing max-iteration/finalization owner, and much of the added surface is deterministic regression coverage. Splitting the signal, safe checkpoint, finalizer, and boundary tests would make the intermediate slices less useful and harder to validate.

Current-head author validation records formatting success, strict Clippy success for both touched crates, 2/2 graceful-stop API tests, and 14/14 focused runtime tests. The visible GitHub checks also pass; the PR correctly documents that the repository Rust Quality Gate does not run for the memorybox base.

🟢 What looks good — ownership remains clean

ZeroClaw owns the task-local signal, safe wave boundary, one tools-free finalizer, provider-history/event ordering, metering, protocol rejection, and cancellation precedence. MemBox remains responsible for detecting NoProgress, choosing soft versus hard stops, preventing empty Done output, and mapping terminal outcomes to the UI.

Decision

Approved as the ZeroClaw foundation for the MemBox zeroclaw-labs#3412 integration follow-up. This approval does not claim that the end-to-end MemBox GLM/Doubao behavior has already been validated; that evidence belongs with the downstream pin and wiring change. @Kevin-K-W