Skip to content

feat(agent,config): activate HMAC tool receipts — wiring stripped from #5168#6214

Merged
Audacity88 merged 3 commits intozeroclaw-labs:masterfrom
singlerider:feat/6182-tool-receipts-activation
May 5, 2026
Merged

feat(agent,config): activate HMAC tool receipts — wiring stripped from #5168#6214
Audacity88 merged 3 commits intozeroclaw-labs:masterfrom
singlerider:feat/6182-tool-receipts-activation

Conversation

@singlerider
Copy link
Copy Markdown
Collaborator

@singlerider singlerider commented Apr 29, 2026

Summary

  • Base branch: master
  • What changed and why: feat(agent): HMAC tool execution receipts for hallucination detection #5168 shipped the HMAC receipt crypto core + threaded Option<&ReceiptGenerator> through run_tool_call_loop / tool_execution, but the activation pieces (config struct, channel-context plumbing, system-prompt addendum, response-block render) were stripped before the squash. Every caller passed None; no receipt ever fired despite docs describing the activated shape. This PR re-lands the stripped wiring exactly as the original branch commit ba16cac had it, adapted to the post-workspace-split crate layout, plus full activation through delegate sub-agent loops so [Feature]: Re-activate HMAC tool receipts — wiring stripped before #5168 merged, docs already describe the activated shape #6182 closes against every acceptance criterion (no remaining "delegate continues to pass None" carve-out).
  • What changed and why (cont.): New [agent.tool_receipts] config section (enabled, show_in_response, inject_system_prompt); ChannelRuntimeContext carries Option<ReceiptGenerator> + show_receipts_in_response; start_channels instantiates the generator from config and appends the receipt-echo addendum to the system prompt; process_channel_message declares a per-turn Arc<Mutex<Vec<String>>> collector and threads both into run_tool_call_loop; after the main reply lands, when show_in_response = true, the collector is rendered as a trailing Tool receipts: block sent as a follow-up message in the same thread.
  • Delegate forwarding: agent::tool_receipts introduces a TOOL_LOOP_RECEIPT_CONTEXT task-local mirroring TOOL_LOOP_COST_TRACKING_CONTEXT. The orchestrator scopes the per-turn ReceiptScope (generator clone + Arc'd collector) before entering the tool loop. DelegateTool::execute_sync reads it and forwards generator + collector into the sub-agent's run_tool_call_loop, so subagent tool calls produce receipts that land in the same per-turn collector the user-visible block renders from. Multi-agent: execute_parallel captures the parent scope and re-enters it inside each tokio::spawn (task-locals don't auto-propagate), so parallel sub-agents share the collector via Arc. Multi-channel: each process_channel_message invocation builds its own Arc<Mutex<Vec<String>>> and its own task-local scope; concurrent channel turns never cross-pollute. Background spawns intentionally stay unsigned because the per-turn collector is rendered before they finish; documented as a known limitation in the security doc.
  • Scope boundary: CLI and legacy agent_turn callers also stay at None, None — receipts are a channel-mode feature.
  • Blast radius: Default-off (enabled = false), so existing deployments deserialize unchanged and observe identical behavior. When enabled: every tool result gains a [receipt: zc-receipt-...] trailer in the model's view of history (including delegate sub-agent results); the system prompt grows by ~400 chars; one extra channel message is sent per turn that ran any tool when show_in_response = true; daemon-process-lifetime HMAC key (rotated on restart, never persisted, never logged).
  • Linked issue(s): Closes [Feature]: Re-activate HMAC tool receipts — wiring stripped before #5168 merged, docs already describe the activated shape #6182. References feat(agent): HMAC tool execution receipts for hallucination detection #5168 (the squash that lost this wiring).

Validation Evidence (required)

cargo fmt --all -- --check
cargo clippy --workspace --exclude zeroclaw-desktop --all-targets --features ci-all -- -D warnings
cargo test --workspace --exclude zeroclaw-desktop --features ci-all
  • Commands run and tail output (against current head a12c8d59b):
    • cargo fmt --all -- --check: clean (no output).
    • cargo clippy ... --features ci-all -- -D warnings: Finished dev profile [unoptimized + debuginfo] target(s) in 1m 39s (zero warnings, zero errors).
    • cargo test --workspace --exclude zeroclaw-desktop --features ci-all: all green; relevant counts include zeroclaw-runtime (1573 + 4 doctests, all pass), zeroclaw-channels orchestrator (24 process_channel_message tests including the new strict-disabled coverage, all pass), zeroclaw-runtime delegate suite (6 execute_agentic tests including the two new receipt-forwarding tests, all pass).
  • Beyond CI — what was manually verified? Walked the original stripped commit (ba16cac) field-by-field against the post-workspace-split crate layout and confirmed every piece (config struct, ChannelRuntimeContext fields, start_channels instantiation, process_channel_message collector + call-site flip, response-block render, system-prompt addendum) re-lands at the equivalent location. Verified the receipt-generator + collector parameters in run_tool_call_loop already exist in master (no signature changes needed). 34 of 34 ChannelRuntimeContext constructors updated with the two new fields (the test sites all set them to None / false, plus three new test sites for receipt coverage). Verified delegate forwarding end-to-end: execute_agentic_forwards_receipt_scope_into_subagent_loop exercises the real EchoTool path through a scoped TOOL_LOOP_RECEIPT_CONTEXT and asserts the receipt lands in the parent collector with a valid zc-receipt- HMAC token. Strict disabled coverage: process_channel_message_disabled_receipt_generator_emits_no_receipts_anywhere proves no zc-receipt- token in any sent message and no [receipt: trailer in any conversation history when receipt_generator: None.
  • Test surface added by this PR:
    • process_channel_message_renders_trailing_tool_receipts_block_when_enabled (full activated path).
    • process_channel_message_omits_receipts_block_when_disabled (show_in_response = false with generator on).
    • process_channel_message_disabled_receipt_generator_emits_no_receipts_anywhere (strict [Feature]: Re-activate HMAC tool receipts — wiring stripped before #5168 merged, docs already describe the activated shape #6182 enabled-false: no receipt anywhere — sent messages, history, anywhere).
    • execute_agentic_forwards_receipt_scope_into_subagent_loop (delegate forwards receipts into sub-loop when scope is set).
    • execute_agentic_emits_no_receipts_when_scope_absent (delegate runs unsigned when scope is unset).

Security & Privacy Impact (required)

  • New permissions, capabilities, or file system access scope? No
  • New external network calls? No
  • Secrets / tokens / credentials handling changed? No — the HMAC key is ephemeral per daemon process, generated via ring::SystemRandom, never persisted or logged, rotated on every restart. Same key handling as feat(agent): HMAC tool execution receipts for hallucination detection #5168. The security doc clarifies that "session" in this design means daemon-process lifetime (one shared key across every channel and conversation in the process); cross-channel and cross-conversation isolation is not part of the threat model and is documented as such.
  • PII, real identities, or personal data in diff, tests, fixtures, or docs? No
  • If any Yes, describe the risk and mitigation: N/A.

Compatibility (required)

  • Backward compatible? Yes
  • Config / env / CLI surface changed? Yes (additive)
  • If No or Yes to either: exact upgrade steps for existing users: None required. The new [agent.tool_receipts] section is fully optional — #[serde(default)] on the field plus per-field defaults mean existing config.toml files load unchanged with enabled = false. Operators who want receipts add:
    [agent.tool_receipts]
    enabled = true
    show_in_response = false   # opt in for user-visible auditable block
    inject_system_prompt = true
  • The generated config reference (docs/book/src/reference/config.md) is gitignored and regenerated on every cargo mdbook build (which the docs-deploy workflow runs). Local regeneration produces a new ### agent.tool_receipts section with enabled, inject_system_prompt, and show_in_response rows; CI's docs deploy will pick this up automatically — no manual edit is possible (or appropriate) since the reference is a derived artifact.

Rollback (required for risk: medium and risk: high)

  • Fast rollback command/path: git revert <merge-sha> against master. The PR squash-merges to a single commit on master regardless of how many commits land in the branch, so the revert is always single-commit. For deployed instances: set [agent.tool_receipts] enabled = false and reload; no code revert needed for users.
  • Feature flags or config toggles: [agent.tool_receipts] enabled is the master switch. show_in_response controls the user-visible block. inject_system_prompt controls the system-prompt addendum. All three default to safe values.
  • Observable failure symptoms:
    • System prompt blow-out: model context overflows after enabling — grep daemon logs for system_prompt truncation warnings; cap via agent.max_system_prompt_chars.
    • Channel double-message: Tool receipts: block sent on a channel where threading is broken would produce a stray top-level message — symptom is two messages per turn on the affected channel.
    • Model failing to echo receipts: provider-specific behavior; not a regression but the auditable signal is degraded. Mitigation: show_in_response = true to surface receipts independent of the model.

…zeroclaw-labs#5168

zeroclaw-labs#5168 shipped the HMAC receipt crypto core (`tool_receipts.rs`),
threaded `Option<&ReceiptGenerator>` through `run_tool_call_loop` /
`tool_execution`, and added the leak-detector regression so receipts
survive scrubbing — but the activation pieces were stripped before the
squash-merge. Every caller passed `None` and no receipt ever fired.
Docs at docs/book/src/security/tool-receipts.md described the activated
shape; the runtime did not deliver it.

This commit re-lands the stripped wiring:

- `[agent.tool_receipts]` config section in zeroclaw-config with
  `enabled` (default false), `show_in_response` (default false), and
  `inject_system_prompt` (default true), gated under #[serde(default)]
  so existing configs deserialize unchanged.
- `ChannelRuntimeContext` carries an optional `ReceiptGenerator` and a
  `show_receipts_in_response` flag; `start_channels` instantiates the
  generator from config when `enabled = true`.
- `process_channel_message` declares a per-turn `Mutex<Vec<String>>`
  collector and passes both `ctx.receipt_generator.as_ref()` and
  `Some(&collector)` into `run_tool_call_loop` so `execute_one_tool`
  signs every tool result and appends `[receipt: zc-receipt-...]`.
- After the main reply is sent, when `show_in_response = true`, a
  trailing `Tool receipts:` block is rendered from the collector and
  sent as a follow-up message in the same thread.
- `start_channels` appends the receipt-echo addendum to the system
  prompt when `enabled && inject_system_prompt`, matching the wording
  from the original branch commit.
- Docs `Current state` table flips the "System-prompt instruction"
  row from `In flight` to `Shipped`.

Delegate sub-agents continue to pass `None, None` per the original
design — that path was deliberately deferred and is not in scope here.

Closes zeroclaw-labs#6182.
@github-actions github-actions Bot added the docs Auto scope: docs/markdown/template files changed. label Apr 29, 2026
@singlerider singlerider added enhancement New feature or request agent Auto scope: src/agent/** changed. runtime Auto scope: src/runtime/** changed. security Auto scope: src/security/** changed. risk: medium Auto risk: src/** or dependency/config changes. labels Apr 29, 2026
@singlerider singlerider added the size: S Auto size: 81-250 non-doc changed lines. label Apr 29, 2026
Copy link
Copy Markdown
Collaborator

@WareWolf-MoonWall WareWolf-MoonWall left a comment

Choose a reason for hiding this comment

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

Review: #6214feat(agent,config): activate HMAC tool receipts

Reviewer: @WareWolf-MoonWall
Verdict: Changes Requested
Commit reviewed: 29a779b


What I read before writing this

Full diff (3 files: crates/zeroclaw-channels/src/orchestrator/mod.rs,
crates/zeroclaw-config/src/schema.rs, docs/book/src/security/tool-receipts.md),
all inline threads (none), all formal reviews (none), the linked issue #6182, PR
#5168 context from PR body, and cross-check against local source for:

  • crates/zeroclaw-runtime/src/agent/tool_receipts.rsReceiptGenerator impl +
    tests
  • crates/zeroclaw-runtime/src/agent/loop_.rsrun_tool_call_loop signature at
    the receipt params
  • crates/zeroclaw-runtime/src/tools/delegate.rs:1183–1184 — delegate subagent
    None, None passthrough

Context

#5168 shipped the HMAC crypto core and threaded Option<&ReceiptGenerator> /
Option<&Mutex<Vec<String>>> through run_tool_call_loop at the loop_.rs
signature level — but stripped the activation wiring before squashing, so every
caller kept passing None, None. Confirmed locally: process_channel_message
in master still reads None, // receipt_generator / None, // collected_receipts
at the run_tool_call_loop call. This PR fills that gap.

The approach is correct: config struct in AgentConfig, two new fields on
ChannelRuntimeContext, instantiation in start_channels, per-turn collector
in process_channel_message, trailing send when show_in_response = true. All
four config toggles are default-safe. The delegate subagent path correctly stays
at None, None per the design note in the PR body — verified locally at
delegate.rs:1183–1184.


🔴 The show_receipts_in_response code path has no test coverage

All 34 ChannelRuntimeContext literal constructions in the test suite set
receipt_generator: None and show_receipts_in_response: false. That means
the entire activated path in process_channel_message

let receipts_block = if ctx.show_receipts_in_response {
    // collector drain → block render → Some(block)
} else { None };
// ...
if let Some(ref block) = receipts_block {
    let _ = channel.send(&SendMessage::new(block, ...).in_thread(...)).await;
}

— is dead code from the test suite's perspective. The HMAC generation and
verification logic in tool_receipts.rs is well-tested; this PR adds no tests
for the orchestrator integration.

This matters for two concrete reasons:

  1. The send error is silenced. let _ = channel.send(...) discards the result.
    A test is the only way to confirm the trailing message actually fires and carries
    the expected content. Without it, a regression in the block-render or send call
    is invisible until an operator enables show_in_response = true in production.

  2. Security-labeled, user-visible behavior. The trailing Tool receipts: block
    is the primary way operators audit that receipts are active. If it misfires
    silently, the audit surface is gone without any log signal.

The existing tests already drive process_channel_message with scripted providers
and mock channels in exactly the pattern needed. A targeted test would:

  • Construct a ChannelRuntimeContext with a real ReceiptGenerator and
    show_receipts_in_response: true
  • Use a scripted provider that returns a tool call followed by a final response
  • Assert the mock channel receives a second message whose body starts with
    "---\nTool receipts:" and contains a valid zc-receipt- token

That's a straight-line addition following the existing test scaffolding. It doesn't
need to cover every edge case — just the happy path confirming the feature fires
when enabled.


🟡 Template validation: cargo clippy and cargo test not run locally

The PR body explicitly notes that the full clippy and test runs were skipped "at
maintainer's request to keep the iteration fast" and delegates to CI. CI is green
and that covers the gap in practice — but the template marks these as required
local output, specifically because "local validation is the signal CI cannot
replace." The "at maintainer's request" framing is also unusual: the template
isn't a per-PR choice, and whoever made that request should have explicitly
suspended the requirement if intentional.

If the skip was deliberate for a reason specific to this PR's context (e.g., a
known pre-existing failure elsewhere in the workspace), please note that in the
body. If it was just an iteration shortcut, paste the tail output before merge.


✅ What this gets right

ReceiptGenerator is well-implemented. #[derive(Clone)], uses
ring::SystemRandom for the 256-bit ephemeral key (correct: ring provides
cryptographically secure randomness), comprehensive unit tests covering
determinism, format parsability, HMAC verification, tamper detection for
name/args/result/key, and fabricated/malformed input rejection.

All 34 test contexts updated in sync. Every ChannelRuntimeContext literal in
the test module received receipt_generator: None, show_receipts_in_response: false
— CI proves none were missed. The mechanical update is tedious and correct.

Backward compatibility is airtight. #[serde(default)] on the struct field
means existing config.toml files deserialise to enabled = false with zero
config changes required. The blast radius is documented accurately.

Delegate subagents correctly inherit the deferred path. delegate.rs:1183–1184
keeps None, None with the same comment style as shared_budget. The design note
in the PR body matches the code.

inject_system_prompt defaulting to true is the right call. When enabled = true, the whole point is that the model echoes receipts into its responses. Forcing
operators to also opt into the system-prompt addendum would produce a confusing
half-activated state. The current default makes activation a single toggle.

Rollback section is among the best I've seen on a feature PR. Three specific
failure symptoms with concrete mitigation steps (system-prompt blow-out, stray
top-level message on broken threading, model failing to echo). That's genuinely
useful operational runbook content.

Docs table update is accurate. System-prompt instruction to echo receipts: In flight → Shipped is correct — the addendum in start_channels is exactly that
feature.


🔵 Non-blocking observations

Add a tracing::warn! on the receipt block send failure. The current let _ = channel.send(...) silently swallows errors. Since this is the audit surface for the
feature, a dropped message should at least be visible in logs:

if let Err(e) = channel.send(...).await {
    tracing::warn!(channel = channel.name(), error = %e, "failed to send tool receipts block");
}

The collector is always allocated and passed as Some. Some(&tool_receipts_collector)
is passed regardless of whether ctx.receipt_generator.is_some(). Inside
run_tool_call_loop the collector is only written to when the generator is also
Some, so the behaviour is correct — but the coupling is implicit. Alternatively:

ctx.receipt_generator.as_ref().map(|_| &tool_receipts_collector),

makes the "collector is meaningful only when generator is active" relationship
explicit in the call site. Minor style point; not a correctness issue.


Summary

The code is correct and the design is right. One thing needs to land before merge:

  1. 🔴 Add a test for show_receipts_in_response = true in process_channel_message
    — confirm the trailing Tool receipts: block is rendered and the channel receives
    it when the feature is enabled. Follows the existing test scaffolding exactly.

The 🟡 validation gap is secondary and resolvable by pasting the clippy/test output
or adding a brief note explaining the deliberate skip.

Happy to re-review promptly once the test lands.

@WareWolf-MoonWall
Copy link
Copy Markdown
Collaborator

@JordanTheJet — milestone alignment needed: this PR does not clearly fit within the scope boundary of any open milestone. v0.7.4 explicitly excludes security paths; v0.7.5 is pipeline-only; v0.7.5-web is gateway/web bugfix only; v0.8.0's scope is schema v3 breaking migrations (this PR is additive config, not a breaking change). Please advise on placement or deferral.

…r surface

Addresses @WareWolf-MoonWall's review on zeroclaw-labs#6214 (29a779b).

🔴 Add `process_channel_message_renders_trailing_tool_receipts_block_when_enabled`
in `crates/zeroclaw-channels/src/orchestrator/mod.rs::tests` that runs the
full activated path (Some<ReceiptGenerator> + show_receipts_in_response=true)
and asserts the mock channel receives a second send carrying the documented
`---\nTool receipts:` separator and a valid `zc-receipt-*` token tied to the
tool name. Plus a paired `_omits_receipts_block_when_disabled` test
asserting the toggle actually gates the second send.

These tests required `AutonomyLevel::Full` + an explicit
`auto_approve.push("mock_price")` so mock_price actually reaches
`execute_one_tool` — the existing `process_channel_message_*` tests in
this file pass under default Supervised because `ToolCallingProvider`
returns the BTC reply regardless of whether the tool ran (the LLM only
needs to see a `[Tool results]` user message — even a denied/cancelled
payload triggers the canned response). Receipts only generate on the
actual execute path, so the gate has to be open here.

Drop `append_receipt_footer` from `crates/zeroclaw-runtime/src/agent/loop_.rs`
and its two call sites (the `_omits_receipts_block_when_disabled` test
exposed the bug that surfaced it). Pre-fix, the footer mechanism
appended a `---\nTool receipts:\n...` block to the agent's response text
*regardless* of `show_receipts_in_response`, while the orchestrator
*also* sent the same content as a separate channel message *gated* on
the toggle — so:

  show_receipts_in_response = true  → footer + separate block (duplicate)
  show_receipts_in_response = false → footer only (toggle ignored)

Both modes were wrong. The orchestrator's separate-message render is
the design Wolf's review expects (and matches the original PR body), so
keep that path and remove the footer entirely. The
`[receipt: ...]` markers on individual tool results in history are
*kept* — those are how the LLM echoes receipts when the system-prompt
addendum is active. Their corresponding tests live in
`agent::tool_receipts::tests` and are unaffected.

🔵 Replace `let _ = channel.send(...)` for the receipts block with a
`tracing::warn!` on `Err(e)`. The block is the operator-facing audit
surface for the feature; a dropped send must leave a log signal rather
than silently disappear.

🔵 Make the `collected_receipts` arg to `run_tool_call_loop` conditional
on `ctx.receipt_generator.is_some()` instead of unconditional `Some(&...)`.
Inside the loop the collector is only written to when the generator is
also Some, so behaviour is unchanged — the explicit `.map(|_| ...)`
just makes the "collector is meaningful only when generator is active"
relationship visible at the call site.
@github-actions github-actions Bot removed agent Auto scope: src/agent/** changed. runtime Auto scope: src/runtime/** changed. security Auto scope: src/security/** changed. labels May 4, 2026
@singlerider singlerider dismissed WareWolf-MoonWall’s stale review May 4, 2026 11:43

All findings addressed in 1776b6d. 🔴 integration test added (renders_trailing_tool_receipts_block_when_enabled) plus a paired disabled-test that exposed a real bug: append_receipt_footer in loop_.rs surfaced receipts in the response text regardless of show_receipts_in_response, double-rendering when the toggle was on and ignoring it when off. Removed append_receipt_footer; the orchestrator's separate-message block is now the only user-facing surface. 🔵 channel.send error now logs via tracing::warn!. 🔵 collector arg now gated on receipt_generator.is_some(). Re-requesting fresh review.

@singlerider
Copy link
Copy Markdown
Collaborator Author

@WareWolf-MoonWall — your review (4208736151) addressed in 1776b6d. Summary:

  • 🔴 integration testprocess_channel_message_renders_trailing_tool_receipts_block_when_enabled runs the full activated path (real ReceiptGenerator + show_receipts_in_response = true) and asserts the mock channel receives a second send with the ---\nTool receipts: separator and a valid zc-receipt-* token tied to mock_price. Plus a paired _omits_receipts_block_when_disabled test asserting the toggle gates the send.
  • 🔵 silent sendlet _ = channel.send(...) replaced with tracing::warn! on Err(e) for the receipts block. The audit surface is no longer silent.
  • 🔵 conditional collectorSome(&tool_receipts_collector) is now ctx.receipt_generator.as_ref().map(|_| &tool_receipts_collector) to make the "collector is meaningful only when generator is active" relationship explicit at the call site. Inside the loop the write is already gated on the generator being Some, so behavior is unchanged.

Bonus bug exposed by the disabled-test that I dropped on the way out:

run_tool_call_loop's append_receipt_footer was appending a ---\nTool receipts: footer to the agent's response text regardless of show_receipts_in_response. The orchestrator's separate-message block is gated on the toggle, but the footer wasn't — so:

show_receipts_in_response Pre-fix behavior
true footer + separate block → duplicate output
false footer only → toggle ignored

Both modes were wrong. Removed append_receipt_footer and its two call sites; the orchestrator's separate-message render (gated on the toggle) is now the only user-facing surface. The [receipt: ...] markers on individual tool results in agent history are kept — those are how the LLM echoes receipts when the system-prompt addendum is active. The four receipt_footer_* tests in loop_.rs::tests are removed as the function they covered no longer exists; agent::tool_receipts::tests (HMAC determinism, verification, tamper detection, malformed input rejection) are unaffected.

🟡 The validation-output skip note ("at maintainer's request") in the PR body is still there — I'll either paste the clippy/test tail in a follow-up commit or rewrite that section before merge.

Re-requested both of you.

Copy link
Copy Markdown
Collaborator

@Audacity88 Audacity88 left a comment

Choose a reason for hiding this comment

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

I reviewed current head 1776b6d against the live PR state, linked issue #6182, the dismissed @WareWolf-MoonWall review, the current diff, adjacent receipt/config call sites, FND-002, and current CI. I did not run local cargo for this review; CI is green, and my local checks were git merge-tree master origin/pr/6214 and git diff --check master...origin/pr/6214.

✅ Resolved — the prior runtime integration review items are addressed

The dismissed review asked for activated-path coverage, a warning on receipt-block send failure, and explicit collector gating. Current head adds process_channel_message_renders_trailing_tool_receipts_block_when_enabled plus the disabled-block companion test, replaces the silent receipt-block send with tracing::warn!, and only passes the collector when the generator is active. I am not re-raising those points.

🟢 What looks good — the default-off channel wiring is narrow

The channel path is additive and default-off. Existing configs deserialize unchanged, start_channels only creates the generator when [agent.tool_receipts] enabled = true, the removed footer helper avoids the duplicate user-visible receipt surface, and the separate Tool receipts: block is gated by show_in_response.

🔴 Blocking — Closes #6182 would close work this PR intentionally leaves open

The current PR body says Closes #6182, but #6182's acceptance criteria still include forwarding receipts through the delegate call site at crates/zeroclaw-runtime/src/tools/delegate.rs:1184, enabled-false coverage that proves no receipt is generated or surfaced anywhere, and a CHANGELOG-next entry. This PR explicitly keeps delegate subagents at None, None; the new disabled-path test covers the visible response block but does not fully satisfy the broader no-receipt-anywhere criterion; and the diff does not update CHANGELOG-next.md.

That scope boundary may be a reasonable split, but it means this PR is a partial activation rather than a full closure of #6182. If this merges as-is, GitHub will close #6182 even though the issue's own checklist is not satisfied.

I would either implement the remaining #6182 criteria here, or change the PR to Refs #6182 and file/link concrete follow-up issue(s) for the intentionally deferred surfaces before merge.

🔴 Blocking — the config reference is missing the new [agent.tool_receipts] section

crates/zeroclaw-config/src/schema.rs adds the new [agent.tool_receipts] config surface, and docs/book/src/security/tool-receipts.md points readers at that shape. The current diff does not update docs/book/src/reference/config.md; git grep tool_receipts origin/pr/6214 -- docs/book/src/reference/config.md returns no matches.

FND-002's documentation standard says config-format PRs must update the config reference. Users relying on the canonical reference would not see enabled, show_in_response, or inject_system_prompt, so this should be regenerated or manually updated before merge.

🔴 Blocking — the PR body rollback and validation sections are stale after the follow-up commit

The rollback section still says git revert 29a779be9 and calls the PR a clean single-commit revert, but current head includes the follow-up commit 1776b6d. For a risk: medium config/security PR, the rollback path needs to match the actual merge shape.

The validation section also still says "No clippy or test changes are introduced by this PR", but the current diff adds the enabled/disabled process_channel_message tests and removes the old receipt-footer tests. CI is green, so this is not a code-test gap, but the required validation narrative should be updated before merge.

🟡 Warning — receipt key lifetime should be called out or narrowed

The code creates one ReceiptGenerator in start_channels, so the HMAC key is per channel-server process and shared across every channel conversation until restart. The docs and #6182 describe the design as an ephemeral per-session key, with cross-session verification out of scope.

If "session" means channel-server process here, the PR body and security doc should say that explicitly. If it means conversation/session, the generator needs to move to conversation/session scope or the HMAC input needs a session binding.

@JordanTheJet
Copy link
Copy Markdown
Collaborator

@WareWolf-MoonWall — milestone policy:

Non-breaking additive PRs with approvals land in the nearest next release (currently v0.7.5) without expanding the milestone scope statement. This PR is non-breaking and additive, but currently CHANGES_REQUESTED. Once @magicalsoup's review concerns are addressed and approvals are restored, this PR can be milestoned to v0.7.5 by the merger.

Reason: HMAC receipt activation is additive config + channel orchestrator wiring on top of #5168's already-merged crypto core; nothing in this PR breaks an existing on-disk config or removes a public symbol.

…rict disabled coverage

Address zeroclaw-labs#6214 review feedback against zeroclaw-labs#6182's full acceptance criteria.

- Add `TOOL_LOOP_RECEIPT_CONTEXT` task-local in `agent::tool_receipts`,
  matching the existing `TOOL_LOOP_COST_TRACKING_CONTEXT` pattern. The
  orchestrator scopes the per-turn `Arc<Mutex<Vec<String>>>` collector
  plus the process-lifetime `ReceiptGenerator` clone before entering the
  tool-call loop.
- `DelegateTool::execute_sync` reads the scope and forwards generator +
  collector into the sub-agent's `run_tool_call_loop`, replacing the
  prior `None, None` placeholders at delegate.rs:1184. Multi-agent
  resilience: `execute_parallel` captures the parent scope and re-enters
  it inside each spawned sub-agent so parallel sub-tool receipts land in
  the same per-turn collector via `Arc` sharing. Background spawns stay
  unsigned by design (per-turn collector is already rendered before they
  finish; documented as a known limitation).
- Strict zeroclaw-labs#6182 disabled coverage:
  `process_channel_message_disabled_receipt_generator_emits_no_receipts_anywhere`
  asserts no `zc-receipt-` token in any sent message and no `[receipt:`
  trailer in conversation history when `receipt_generator: None`.
  Distinct from the existing `show_in_response = false` test (which
  keeps the generator on but suppresses the user-visible block).
- Delegate forwarding coverage: positive test exercises `execute_agentic`
  inside a scoped `TOOL_LOOP_RECEIPT_CONTEXT` and verifies a real
  `echo_tool` sub-call lands in the parent collector with a valid
  `zc-receipt-` HMAC token; negative test confirms unsigned sub-loop
  output when no scope is set.
- Clarify "session" semantics in `docs/book/src/security/tool-receipts.md`:
  the HMAC key is per daemon process (not per conversation or channel),
  generated at `start_channels` and rotated on restart. Add explicit
  "what receipts don't isolate" entries for cross-channel and
  background-delegate spawns.
@singlerider
Copy link
Copy Markdown
Collaborator Author

Thanks for the detailed pass. Addressed every blocking item plus the warning, with one explicit decline. New head: a12c8d5.

Resolved

Closes #6182 is now accurate — every acceptance criterion is satisfied in this PR. No follow-up issues, no deferred surfaces.

  • Delegate forwarding (crates/zeroclaw-runtime/src/tools/delegate.rs:1184): added a TOOL_LOOP_RECEIPT_CONTEXT task-local in agent::tool_receipts, mirroring the existing TOOL_LOOP_COST_TRACKING_CONTEXT pattern. The orchestrator scopes the per-turn ReceiptScope (generator clone + Arc'd collector) before entering the tool loop. DelegateTool::execute_sync reads it and forwards generator + collector into the sub-agent's run_tool_call_loop, replacing the prior None, None placeholders. Multi-agent: execute_parallel captures the parent scope and re-enters it inside each tokio::spawn (task-locals don't auto-propagate), so parallel sub-agents share the per-turn collector via Arc. Background spawns (background: true) intentionally stay unsigned, the per-turn collector is rendered before they finish so late receipts have nowhere to surface; documented as a known limitation in the security doc rather than left as a dangling carve-out.
  • Strict no-receipt-anywhere coverage: process_channel_message_disabled_receipt_generator_emits_no_receipts_anywhere exercises receipt_generator: None and asserts no zc-receipt- token in any sent message and no [receipt: trailer in any conversation history entry. Distinct from the existing show_in_response = false test (which keeps the generator on but suppresses the visible block).
  • Delegate forwarding coverage: execute_agentic_forwards_receipt_scope_into_subagent_loop runs a real EchoTool sub-call inside a scoped TOOL_LOOP_RECEIPT_CONTEXT and asserts the receipt lands in the parent collector with a valid zc-receipt- HMAC token; execute_agentic_emits_no_receipts_when_scope_absent confirms unsigned sub-loop output when no scope is set.

docs/book/src/reference/config.md: the file is gitignored (.gitignore:8), it's a derived artifact regenerated on every cargo mdbook build (which the docs-deploy workflow runs on every push to master). Locally regenerating now produces a new ### agent.tool_receipts section with enabled, inject_system_prompt, and show_in_response rows; CI's docs deploy will pick this up automatically. Hand-editing the reference would be wrong (it gets overwritten); the schema-export macros on ToolReceiptsConfig are what generate the entry, and they're correct.

PR body rollback + validation sections: rewritten. Rollback is now SHA-agnostic (git revert <merge-sha>, squash-merge to master is always single-commit regardless of how many commits land in the branch). Validation now lists the actual fmt/clippy/test runs against the current head and enumerates the test surface added by this PR.

Receipt key lifetime clarified. docs/book/src/security/tool-receipts.md now spells it out: the HMAC key is generated once at start_channels and held in ChannelRuntimeContext for the daemon-process lifetime, rotated only on restart. Every channel, every conversation, and every delegate sub-agent inside that process verifies against the same key. The "What receipts don't do" list now explicitly covers cross-daemon-restart, cross-channel-within-a-process, and background-delegate-spawn boundaries. PR body Security section mirrors this.

Declined (with rationale)

CHANGELOG-next entry, not adding. Project convention (enforced across recent PRs) is that CHANGELOG-next.md is release-only: entries are curated by the /changelog-generation skill at release-prep time, not authored from individual PRs. This is the same reason recent PRs across the repo don't add CHANGELOG-next entries even when they ship user-visible behavior changes. Happy to revisit if the policy is changing, but as it stands, adding one here would invert the convention.

Copy link
Copy Markdown
Collaborator

@WareWolf-MoonWall WareWolf-MoonWall left a comment

Choose a reason for hiding this comment

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

Reviewed at head 1776b6d. I read the current diff, the updated PR body, @Audacity88's CHANGES_REQUESTED, and my prior DISMISSED review.

✅ Resolved — items from my prior review

The dismissed review asked for: activated-path test coverage, tracing::warn! on the receipt-block send failure, and explicit collector gating. All three landed in the follow-up commit. I am not re-raising them — they're addressed. @Audacity88 already confirmed this in their review.

🔵 @Audacity88's active CHANGES_REQUESTED — I have no independent basis to override

@Audacity88 holds three 🔴 blocking items at the current head. I've read them and the code. All three are real issues:

1. Closes #6182 prematurely closes work this PR intentionally defers. The PR body explicitly scopes delegate subagents to None, None as a known limitation. #6182's acceptance criteria include delegate forwarding. When this merges, GitHub will auto-close #6182 despite that criterion being unmet. Refs #6182 plus a concrete follow-up issue for the delegate path is the right fix.

2. docs/book/src/reference/config.md is missing the new [agent.tool_receipts] section. FND-002 requires config-format PRs to update the config reference. Operators relying on the canonical reference won't find enabled, show_in_response, or inject_system_prompt. This is a straight-line addition.

3. Rollback section references git revert 29a779be9 but the PR now has two commits. The rollback path should match the actual merge shape, and the validation narrative says "No clippy or test changes" despite the current diff adding tests.

These are all fixable in one commit. I'm commenting rather than approving to respect @Audacity88's active block — per protocol I won't approve over another reviewer's live CHANGES_REQUESTED.

@singlerider — the three fixes are quick: swap ClosesRefs, add the config reference section, update the rollback commit hash and validation prose. Once those land, I can turn this around fast.

@singlerider singlerider requested a review from Audacity88 May 5, 2026 03:26
@Audacity88 Audacity88 added risk: high Auto risk: security/runtime/gateway/tools/workflows. size: L Auto size: 501-1000 non-doc changed lines. agent Auto scope: src/agent/** changed. runtime Auto scope: src/runtime/** changed. security Auto scope: src/security/** changed. channel Auto scope: src/channels/** changed. config Auto scope: src/config/** changed. tool Auto scope: src/tools/** changed. tool:delegate Auto module: tool/delegate changed. and removed size: S Auto size: 81-250 non-doc changed lines. risk: medium Auto risk: src/** or dependency/config changes. labels May 5, 2026
Copy link
Copy Markdown
Collaborator

@Audacity88 Audacity88 left a comment

Choose a reason for hiding this comment

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

I re-reviewed current head a12c8d5 against the live PR state, the prior @Audacity88 and @WareWolf-MoonWall reviews, linked issue #6182, the PR body, the current diff, the docs-generation path, and current CI. This is a fresh approval at the current head; the older request-changes reviews are stale/dismissed, and @WareWolf-MoonWall's latest review was comment-only. I did not run local cargo; CI is green. Local checks were git merge-tree origin/master origin/pr/6214 and git diff --check origin/master...origin/pr/6214.

✅ Resolved — #6182 closure now matches the implementation

The previous block on Closes #6182 is resolved. The PR no longer leaves the delegate call site at None, None: DelegateTool::execute_sync now reads TOOL_LOOP_RECEIPT_CONTEXT and forwards both the generator and shared collector into the sub-agent loop. The parallel path also re-enters the receipt scope inside each spawned task, which is the important bit because Tokio task-locals do not propagate automatically.

The acceptance-criteria coverage is now materially better too: process_channel_message_disabled_receipt_generator_emits_no_receipts_anywhere covers the strict disabled path, and the delegate tests cover both scoped receipt forwarding and the no-scope fallback. Background delegate tasks remain intentionally unsigned because they detach from the user turn; the security doc now calls that out rather than leaving it implicit.

I am not treating the missing CHANGELOG-next entry as a blocker. The current changelog procedure is release-driven: CHANGELOG-next.md is generated during release preparation, not maintained one PR at a time. That makes the author's decline reasonable unless maintainers change the release convention.

✅ Resolved — the config-reference concern is not a checked-in file gap

The previous review was right to ask whether [agent.tool_receipts] would appear in the config reference. Current head answers that through the schema path: ToolReceiptsConfig derives Configurable and JsonSchema, has the agent.tool_receipts prefix, and is wired into AgentConfig as a nested defaulted section.

docs/book/src/reference/config.md is gitignored in this repo, and the docs deploy workflow regenerates it from live code during cargo mdbook build. Given that setup, I do not think hand-editing or checking in that generated file is the right requirement for this PR.

✅ Resolved — PR body, rollback, and key lifetime are current

The PR body no longer points at the stale first commit or claims no tests changed. It now describes the current head, lists the fmt/clippy/test commands, names the new tests, and uses the actual squash-merge rollback shape: git revert <merge-sha>.

The key-lifetime warning is also addressed. The security doc and PR body now say the HMAC key is process-lifetime, shared across channels/conversations inside one daemon, and rotated on restart. That matches where the generator is actually created in start_channels.

🟢 What looks good — the receipt scope stays narrow

The follow-up keeps the feature default-off and avoids changing the Tool trait signature. The task-local receipt scope is only established when channel-mode receipts are enabled, and the per-turn collector is shared with delegate sub-loops through Arc<Mutex<Vec<String>>>. That is a pragmatic fit for the existing delegate/cost-tracking patterns.

Approving the code changes.

@Audacity88 Audacity88 added this to the v0.7.5 milestone May 5, 2026
@Audacity88 Audacity88 merged commit 6731104 into zeroclaw-labs:master May 5, 2026
12 checks passed
github-actions Bot pushed a commit that referenced this pull request May 5, 2026
…pped from #5168 (#6214)

- 29a779b feat(agent,config): activate HMAC tool receipts — wiring stripped from #5168
- 1776b6d fix(agent,channels): receipts integration test + drop duplicate footer surface
- a12c8d5 fix(agent,channels): forward receipts through delegate sub-loops + strict disabled coverage 6731104
github-actions Bot pushed a commit to nagyist/zeroclaw that referenced this pull request May 5, 2026
…pped from zeroclaw-labs#5168 (zeroclaw-labs#6214)

- 29a779b feat(agent,config): activate HMAC tool receipts — wiring stripped from zeroclaw-labs#5168
- 1776b6d fix(agent,channels): receipts integration test + drop duplicate footer surface
- a12c8d5 fix(agent,channels): forward receipts through delegate sub-loops + strict disabled coverage 6731104
github-actions Bot pushed a commit to tidux/zeroclaw that referenced this pull request May 5, 2026
…pped from zeroclaw-labs#5168 (zeroclaw-labs#6214)

- 29a779b feat(agent,config): activate HMAC tool receipts — wiring stripped from zeroclaw-labs#5168
- 1776b6d fix(agent,channels): receipts integration test + drop duplicate footer surface
- a12c8d5 fix(agent,channels): forward receipts through delegate sub-loops + strict disabled coverage 6731104
github-actions Bot pushed a commit to vmisunv/my_zeroclaw that referenced this pull request May 6, 2026
…pped from zeroclaw-labs#5168 (zeroclaw-labs#6214)

- 29a779b feat(agent,config): activate HMAC tool receipts — wiring stripped from zeroclaw-labs#5168
- 1776b6d fix(agent,channels): receipts integration test + drop duplicate footer surface
- a12c8d5 fix(agent,channels): forward receipts through delegate sub-loops + strict disabled coverage 6731104
github-actions Bot pushed a commit to drowning-in-codes/zeroclaw that referenced this pull request May 6, 2026
…pped from zeroclaw-labs#5168 (zeroclaw-labs#6214)

- 29a779b feat(agent,config): activate HMAC tool receipts — wiring stripped from zeroclaw-labs#5168
- 1776b6d fix(agent,channels): receipts integration test + drop duplicate footer surface
- a12c8d5 fix(agent,channels): forward receipts through delegate sub-loops + strict disabled coverage 6731104
github-actions Bot pushed a commit to DavisLi2013/zeroclaw that referenced this pull request May 6, 2026
…pped from zeroclaw-labs#5168 (zeroclaw-labs#6214)

- 29a779b feat(agent,config): activate HMAC tool receipts — wiring stripped from zeroclaw-labs#5168
- 1776b6d fix(agent,channels): receipts integration test + drop duplicate footer surface
- a12c8d5 fix(agent,channels): forward receipts through delegate sub-loops + strict disabled coverage 6731104
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent Auto scope: src/agent/** changed. channel Auto scope: src/channels/** changed. config Auto scope: src/config/** changed. docs Auto scope: docs/markdown/template files changed. enhancement New feature or request risk: high Auto risk: security/runtime/gateway/tools/workflows. runtime Auto scope: src/runtime/** changed. security Auto scope: src/security/** changed. size: L Auto size: 501-1000 non-doc changed lines. tool:delegate Auto module: tool/delegate changed. tool Auto scope: src/tools/** changed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: Re-activate HMAC tool receipts — wiring stripped before #5168 merged, docs already describe the activated shape

4 participants