Reviewed head a163123. I checked the fetched PR state, linked issue #6345, the existing #6345 triage comment, the full diff, the config-schema source around TelegramConfig, CI status at fetch time, and the relevant ZeroClaw review/docs/governance guidance. There are no prior PR comments, inline comments, or formal reviews. I did not run local cargo; CI is green. Local checks were git merge-tree --write-tree origin/master origin/pr/6389 and git diff --check origin/master...origin/pr/6389.

🟢 What looks good — the schema slice is narrow and default-safe

The actual code change is small and low-behavior-risk: reply_min_interval_secs is added to TelegramConfig with #[serde(default)], and every existing TelegramConfig struct literal is updated with 0. That preserves existing behavior for users who do not set the new key. The follow-up commits also cleaned up the stale present-tense docstring claims and removed the unrelated generated docs reorder, which keeps this PR focused.

🔴 Blocking — Closes #6345 would close work this PR explicitly defers

The PR body says this is only the Telegram schema-field slice. It explicitly defers mirroring the field to other outbound channels, load-time range validation, and the per-channel/per-peer outbound scheduler. The linked issue #6345 is broader than this PR: its acceptance criteria require the field across supported channels, 0..=3600 validation, delayed outbound delivery, bounded queue behavior, and docs.

If this merges with Closes #6345, GitHub will close the tracker even though the actual pacing behavior has not landed and the remaining acceptance criteria are still open. That would make the work look complete when the PR itself says it is only the first step.

Please either change this PR to Refs #6345 and keep #6345 open, or open/link concrete follow-up issues for the remaining scheduler, validation, cross-channel, queue, and docs work before closing the tracker. If the intent is for this PR to fully close #6345, then the remaining acceptance criteria need to land here.

🟡 Warning — validation evidence should include command output

The PR says fmt, clippy, and check all pass locally, but the validation section summarizes the results without command output. CI is green, so I am not treating this as a code blocker, but the section should paste the command output tails or explain why local output is intentionally omitted.

Review — #6389 feat(config/channels): add reply_min_interval_secs to TelegramConfig

Verdict: comment
Reviewer: WareWolf-MoonWall
Head checked: a163123 (checks passing)

I read the full diff, the PR body, the existing review (Audacity88: CHANGES_REQUESTED), linked issue #6345, and cross-checked the schema change against the local TelegramConfig definition and all struct literal sites. I'm not approving over Audacity88's active block.

State of the queue: Audacity88 holds an active CHANGES_REQUESTED. No other reviews. I'm seconding Audacity88's block and adding one additional observation.

On Audacity88's block — I agree

The Closes #6345 trailer will auto-close the issue on merge. The PR body explicitly says this is only the first step: the scheduler, range validation, cross-channel mirroring, and docs are all deferred. The issue tracker closing would misrepresent the feature as complete when the agent doesn't yet pace anything — reply_min_interval_secs is added to the schema but nothing reads it yet at runtime.

Resolution path: change Closes #6345 to Refs #6345 in the PR body, and either open explicit tracking issues for the deferred work or add Depends on links when those PRs exist. This keeps the issue open as the active tracker for the full feature.

🔴 Blocking — cargo test was not run

Audacity88 flagged this as 🟡; I'm treating it as 🔴 here because this PR touches zeroclaw-runtime/src/ — a high-risk path per AGENTS.md. The validation section lists fmt, clippy, and check, but not cargo test. The changes are confined to struct literal construction sites in test code and the schema definition, so the tests almost certainly pass, and CI is green. But the template requires cargo test for code changes, and touching zeroclaw-runtime/src/daemon/mod.rs and zeroclaw-runtime/src/integrations/registry.rs — even in #[cfg(test)] blocks — is a code change, not a docs change. The PR is labeled risk: high; the evidence bar for that risk tier includes the full test sweep.

Please paste the cargo test tail before this lands.

🟡 Warning — cargo test skip is undocumented

The template says: "If any command was intentionally skipped, why." The validation section doesn't acknowledge the skip. Even if cargo test passes, the omission leaves a gap in the reviewable evidence record. Documenting "skipped because X" is what turns a gap into a deliberate decision.

🟢 Praise — every struct literal site updated, default is zero

All TelegramConfig struct literal construction sites across schema.rs (tests), orchestrator/mod.rs (test), daemon/mod.rs (tests), integrations/registry.rs (test), and src/config/mod.rs (test) were found and updated with reply_min_interval_secs: 0. A zero-default means existing configs deserialize cleanly with no behavioral change. The #[serde(default)] annotation handles TOML configs that omit the new key. The diff is narrow and correct.

🟢 Praise — docstring quality

The field-level docstring on reply_min_interval_secs is the right length and covers the right things: the default, the intended range, the identity-security rationale, and the deferred-scheduler caveat. It reads like a field that will be used, not like a placeholder.

🔵 Suggestion — add a telegram.md docs mention, even as a "coming soon" note

The field will surface in cargo mdbook refs automatically, but a user reading docs/book/src/channels/telegram.md won't know the field exists until someone looks at the config reference. Mentioning it — even briefly, e.g. "pacing: reply_min_interval_secs sets a floor on reply cadence; scheduler enforcement lands in a follow-up" — would make the staged intent visible in the channel docs rather than only in the schema reference.

🔵 Suggestion — consider a test for the new field's serde default

There are existing targeted tests for TelegramConfig serde behavior (telegram_config_ack_reactions_missing_defaults_to_none, etc.). Adding one for reply_min_interval_secs — verifying that a TOML input omitting the field deserializes to 0 — follows the established pattern and documents the contract explicitly. Not blocking, but consistent with the test style already present.

Template

The PR body is well-filled. The security section correctly notes that the field is inert at this commit. The rollback section is accurate. The Closes #6345 issue is the only template-level problem, and it's the same issue as the blocking item above.

Summary: Audacity88's block is correct. Change Closes to Refs, run cargo test and paste the tail, and this schema-only slice is ready for another look. The field itself is well-formed and the literal-site update is thorough.

Reviewed current head ad800e8. I read the PR body, @Audacity88's CHANGES_REQUESTED at a163123, @WareWolf-MoonWall's COMMENTED at a163123, the full diff, the schema diff against TelegramConfig, every struct-literal call site touched, and the new test.

✅ [resolved] Audacity88's blocker (Closes #6345) is addressed

The PR body now reads:

Refs #6345 (kept open as the active feature tracker; #6421 holds the deferred implementation work).

Filing #6421 explicitly for the deferred scheduler / range validation / cross-channel mirroring / queue / docs work — and changing Closes to Refs — is exactly the resolution path @Audacity88 asked for. #6345 stays open as the active feature tracker, and this PR no longer over-claims completion.

✅ [resolved] WareWolf-MoonWall's blocker (cargo test evidence)

The validation evidence section now includes both targeted and workspace test runs:

$ cargo test -p zeroclaw-config --lib telegram
running 9 tests
... (all 9 pass, including the new telegram_config_reply_min_interval_secs_missing_defaults_to_zero)

$ cargo test --workspace --exclude zeroclaw-desktop --features ci-all --lib
# 1452 / 612 / 165 / 136 / 58 / 295 / ... per-crate suites: all passing

The new telegram_config_reply_min_interval_secs_missing_defaults_to_zero test is exactly the targeted serde-default test WareWolf-MoonWall suggested as a 🔵 — it pins the #[serde(default)] contract so a regression that drops the attribute fails CI.

✅ [resolved] Telegram docs mention added

docs/book/src/channels/chat-others.md now includes:

reply_min_interval_secs (default 0) sets a per-peer floor on reply cadence. Field is recognised by the schema; the per-channel pacing scheduler that actually delays delivery lands in a follow-up to #6345.

This is the staged-intent docs note WareWolf-MoonWall suggested. The "field exists but scheduler is deferred" framing is exactly right and matches the PR body.

🟢 What's working well

• All TelegramConfig struct literal sites updated (schema tests, orchestrator test, daemon tests, integrations registry test, src/config/mod.rs test) — the 0 literal is consistent everywhere, no missed sites.
• Field-level docstring is the right length and covers default, range, identity-security rationale, and deferred-scheduler caveat. Reads like a field that will be used, not a placeholder.
• Schema-only diff with zero-default means existing TOML configs deserialize cleanly with no behavioral change. Backward compat is byte-equivalent until #6421 lands.
• Refs #6345 + spawn of #6421 is the right governance shape — keeps the tracker honest while allowing this slice to merge.

Verdict

All prior findings are addressed at HEAD. CI is green. The schema slice is narrow, default-safe, and well-tested.

Leaving as --comment rather than --approve because @Audacity88's CHANGES_REQUESTED is still on the books at a163123. @Audacity88, would you take another look at HEAD ad800e8? The Closes → Refs change + #6421 spawn directly resolves your block; if you agree, dismissing or converting your review clears the merge gate.

Re-review — #6389 feat(channels): per-(channel,recipient) reply pacing — wrapper + 9 channels + validation

Verdict: --approve
Reviewer: @WareWolf-MoonWall
Head reviewed: 134614c (current HEAD; checks passing)
Prior review head: a163123 (COMMENTED)

Take-stock

@Audacity88's CHANGES_REQUESTED (at a163123) is DISMISSED. My prior COMMENTED review is not a block. theonlyhennygod reviewed an intermediate state at ad800e8 (COMMENTED). No active CHANGES_REQUESTED holds. I'm reviewing the expanded implementation fresh.

Prior findings — resolution status

✅ [resolved] 🔴 Closes #6345 premature

At a163123 this PR was a Telegram schema-only slice and the tracker would have closed prematurely. The resolution path here was not Refs + deferral — it was shipping the full implementation. The diff confirms all acceptance criteria from #6345 are present: PacedChannel wrapper, field on all 9 channels, orchestrator wrapping at every Arc::new(...) push site, Config::validate() range enforcement (0..=3600), 4 unit tests + 3 config tests, and the chat-others.md pacing section. Closes #6345 is now accurate and appropriate.

✅ [resolved] 🔴 cargo test evidence absent

The validation section now includes:

• cargo test -p zeroclaw-channels --lib paced — 4 tests, all passing
• cargo test -p zeroclaw-config --lib reply_min_interval — 3 tests, all passing
• cargo test --workspace --exclude zeroclaw-desktop --features ci-all --lib — full workspace suite passing

High-risk-path evidence bar met.

✅ [resolved] 🟡 cargo test skip undocumented

cargo test was run. The gap is gone.

✅ [resolved] 🔵 Serde default test for reply_min_interval_secs

telegram_config_reply_min_interval_secs_missing_defaults_to_zero is present in the diff. It pins the #[serde(default)] contract so a regression that drops the attribute fails CI.

✅ [resolved] 🔵 Channel docs mention

docs/book/src/channels/chat-others.md now has a top-level ## Pacing outbound replies (reply_min_interval_secs) section documenting the wire contract: per-(channel, recipient) floor, 0 is passthrough, draft updates not paced, recipients independent. More comprehensive than the per-channel telegram.md note I originally suggested — the right call given the field is on nine channels.

New findings

🟡 [warning] — Telegram-specific note in chat-others.md is now stale

Under the Telegram section, the new bullet reads:

`reply_min_interval_secs` (default `0`) sets a per-peer floor on reply cadence. Field is recognised by the schema; the per-channel pacing scheduler that actually delays delivery lands in a follow-up to #6345.

This was accurate when theonlyhennygod reviewed an intermediate state where the PR body said Refs #6345. It is not accurate now: PacedChannel IS the delivery mechanism and it ships in this PR. A user reading that bullet today will believe pacing is deferred when it is live.

Resolution: update the bullet to reflect the shipped state, e.g.:

reply_min_interval_secs (default 0) sets a per-peer floor on reply cadence. Pacing is active: the orchestrator wraps the channel in PacedChannel so consecutive sends to the same peer wait at least N seconds. 0 is a zero-overhead passthrough.

Not blocking the approve — the top-level pacing section is accurate and describes the correct behavior. But the stale note should be cleaned up before or shortly after merge, since it will be the first thing a Telegram user sees in that section.

🟡 [warning] — size: XS label is stale

The diff is +680 -80. size: XS was appropriate for the original Telegram schema-only slice; the full implementation warrants at least size: M. Worth updating for changelog generation and queue hygiene — the label is used by the PR template for size signal.

What's working well

🟢 Zero-overhead default path is correct

PacedChannel::wrap returns inner unchanged when min_interval_secs == 0. No allocation, no Arc::new, no Mutex. The default config pays exactly zero overhead for this feature. That's the right design — the constraint is "no penalty for the common case."

🟢 Lock discipline in PacedChannel::send is correct

The lock is acquired to compute the sleep duration and update the per-recipient slot, then released before tokio::time::sleep. Multiple concurrent senders to different recipients proceed in parallel. Multiple concurrent senders to the same recipient each reserve their slot atomically and then sleep independently — the map records now + wait (the scheduled fire time) so a burst of N replies stacks to (N-1) * min_interval wall-clock correctly. There is no contention during sleep.

🟢 Nine channels, every struct literal site updated

All nine channel configs (TelegramConfig, DiscordConfig, SlackConfig, MattermostConfig, WebhookConfig, IMessageConfig, MatrixConfig, SignalConfig, WhatsAppConfig) carry reply_min_interval_secs: 0 in every test struct literal across schema.rs, orchestrator/mod.rs, daemon/mod.rs, integrations/registry.rs, src/config/mod.rs, and the channel-specific test modules. No missed sites visible in the diff.

🟢 Config::validate() upper-bound test pins the typo-prevention contract

validate_accepts_reply_min_interval_at_upper_bound (3600 passes) and validate_rejects_reply_min_interval_above_one_hour (3601 fails) pin the boundary that catches the milliseconds-in-seconds typo before runtime. The 60000 → 16-hour-wedge failure mode is now a config validation error, not a silent runtime regression.

Summary

All prior findings from my COMMENTED review at a163123 are resolved. The full pacing feature is correctly implemented, tested, and documented. The two 🟡 items above (stale doc note, label) are cleanup and do not block merge. CI is green.

Approving. Once the Telegram-specific doc note is updated, this is complete.

Re-review — PR #6389 · feat(channels): per-recipient reply pacing across 9 channels

Verdict: APPROVE
Reviewed at commit db82da2.

Resolved since prior review

✅ Orchestrator integration complete. collect_configured_channels now wraps every Some-bound channel construction in PacedChannel::wrap(..., <config>.reply_min_interval_secs). The prior head shipped the PacedChannel implementation and config schema; this head wires it into all nine channels at the construction sites. The implementation is complete end-to-end.

✅ Stale doc bullet fixed. The PR body confirms the chat-others.md Telegram bullet was updated to reflect that pacing is live in this PR (not "landing in a follow-up"). Verified in the updated PR body.

🟢 The reply_min_interval_secs == 0 passthrough optimisation (PacedChannel::wrap returns the inner Arc unchanged when the interval is zero) means the default config pays zero overhead — no wrapper allocation, no Mutex contention. This is the right design for an opt-in feature.

🟢 Matrix test config updated to include reply_min_interval_secs: 0 — confirms the struct initialisation pattern is consistent across all channels with exhaustive field coverage.

One non-blocking cleanup remaining

🟡 size: XS label is stale — the diff is ~1 270 lines; warrants size: M or size: L. Not a merge blocker, but a quick sidebar edit before landing keeps the metadata useful.

No active CHANGES_REQUESTED from any reviewer. Approving.