feat(config): delete_with_cascade for channels (#7175) - #7839
Conversation
a4afa27 to
0568269
Compare
singlerider
left a comment
There was a problem hiding this comment.
I reviewed slice 5 on its own delta (0568269), ignoring the cumulative ancestors. I read the new delete_channel / scrub_channel_refs arm and collect_channel_refs, and cross-checked the HARD detection against Config::validate()'s channel sites in schema.rs. I confirmed the channel suite green locally (cargo test -p zeroclaw-config cascade_channel 6/6, full alias_refs 35/35, clippy clean). This is a clean library arm with the subtle validate-mirroring done correctly. Approving on merits.
This stays draft and merges strictly after slice 4 (#7838), which currently has an outstanding changes-requested from me on the workspace-archive ordering bug. Nothing here depends on that fix, so this approval is independent of it.
π’ The two HARD channel-ref classes mirror validate() exactly, including the trim asymmetry
The hard part of this slice is the bare-type peer-group orphaning case, and it is right. collect_channel_refs emits a HARD ref at peer_groups.<g>.agents[i] when a bare-type group member's only <type>.* channel is the target. I traced the survivor test against validate()'s bare-membership check (schema.rs ~17682, the None arm):
- validate's survival test is
ch.as_str().starts_with("<type>.")on the untrimmed string. - collect's survival test (line ~710) is
ch.trim() != target && ch.as_str().starts_with(&type_prefix)- the membership-match clause trims (mirroring what the scrub removes), the survival clause does not trim (mirroring validate).
That asymmetry is deliberate and correct: a whitespace-padded " discord.other" would not count as a surviving channel in validate's eyes, so collect must refuse rather than report a success that yields a config validate rejects. Getting trim-vs-no-trim aligned per-clause is exactly the kind of thing that silently rots; the comment explains why, and the survivor test (cascade_channel_proceeds_when_bare_group_member_keeps_another) plus the orphan test (cascade_channel_refuses_orphaning_bare_group_member) pin both sides.
π’ The type-level last-alias guard is the right companion to the member-level guard
removes_last_alias (sole key of the type) reports every bare-type group whose channel = "<type>" as HARD, because emptying the [channels.<type>.*] block makes peer_groups.<g>.channel = "<type>" resolve to nothing and validate bails (schema.rs ~17657). This fires at the type level (the block disappears) while the member-level guard fires even when another alias keeps the block present but the member's own only matching channel is the target. Both are needed; neither subsumes the other.
π’ Scrub mirrors collect, post-condition re-walk, dotted-ref safety
scrub_channel_refsdrops exactly the two SOFT sites collect marks (agents.<X>.channels[]andescalation.alert_channels[]), trimming to matchfind_all_references, and never touchespeer_groups.<g>.channel(HARD, refused before reaching the scrub).- Removal goes through the same generic
delete_map_key("channels.<type>", alias)the gateway/CLI use, and the post-mutationfind_all_referencesre-walk converts any future scrub/collect drift into a loudPostConditionerror. - Correctly distinguishes the dotted target (
discord.work) from a bare group channel (discord): the direct peer-group-channel HARD ref only matches the dotted form, and the bare cases are handled by the two type/member guards. DryRun mutates nothing; NotFound short-circuits.
Channels carry no owned non-config state, so unlike the agent arm this is the complete cascade for the kind with no surface-side half. Clean slice.
Completes agent deletion: the gateway scrubs config references via
delete_with_cascade AND cascades the agent's owned non-config state (memory
rows, workspace dir, cron jobs, ACP sessions, session attribution), refusing
on HARD references.
Surface (zeroclaw-gateway):
- New `agent_owned_state` coordinator: export-then-delete each store into the
same `agents/_deleted/<alias>-<ts>/` archive (`cascade/*.json` + manifest),
then remove. Per-store failures are SURFACED in `OwnedStateReport.warnings`
+ manifest + a WARN log β never masked as success.
- `handle_delete_map_key` routes `path == "agents"` through `delete_agent_cascade`:
refuse if any HARD config ref (enabled `heartbeat.agent`) OR live ACP session
(`killed_at IS NULL`). The live-ACP gate FAILS CLOSED β if the ACP store can't
be read it refuses rather than risk orphaning active sessions. Else
`delete_with_cascade(Agent)` scrubs config refs + removes the entry (fixing a
latent gap where the old path left heartbeat/peer-group/delegate refs
dangling), archive workspace, run the owned-state cascade, persist.
Store methods:
- zeroclaw-infra acp_session_store: `count_live_sessions_by_agent` (HARD signal),
`list_sessions_by_agent`, `delete_sessions_by_agent` (children cascade via FK).
- zeroclaw-infra `SessionBackend::clear_agent_attribution` (sqlite: agent_alias
β NULL) β keep the possibly channel-shared conversation, drop stale attribution.
- zeroclaw-runtime `cron::{list,remove}_jobs_by_agent` (cron_runs cascade off job_id).
- zeroclaw-api `Memory::{export_agent}` + `purge_agent` now implemented for ALL
per-agent backends β SqliteMemory, LucidMemory (delegates to inner sqlite),
PostgresMemory (DELETE/SELECT by agent), QdrantMemory (scroll + delete_points)
β so non-sqlite backends no longer silently orphan agent memory.
Tests: memory export_agent (only that agent's rows, no delete); acp live-count
+ delete-by-agent. Full workspace builds; clippy --all-targets (incl. all memory
features) + fmt clean.
Persistence fix: save_dirty writes only marked-dirty paths, and the config
cascade does not mark dirty. delete_agent_cascade previously marked only
agents.<alias>, so a soft-ref scrubbed in ANOTHER entry (another agent's
delegates, a peer group's agents) was correct in memory but left STALE on disk
and reappeared as a dangling reference on the next config reload (which
validate() then rejects). The handler now marks every entry the cascade
touched β the removed entry plus each scrubbed referrer's entry β via
delete_cascade_dirty_paths/dirty_entry_for (mirrors rename's
RenameReport.dirty_paths). + a dirty_entry_for unit test.
β¦ the entry (zeroclaw-labs#7175) Review (zeroclaw-labs#7838, singlerider): delete_agent_cascade resolved working.agent_workspace_dir(alias) AFTER delete_with_cascade removed the agents entry. agent_workspace_dir only returns an operator-set custom workspace.path while the entry exists, so for a custom-workspace agent the resolution fell through to the default install_root/agents/<alias>/workspace path; workspace.exists() then found nothing and the archive was silently skipped, leaving the real workspace on disk and defeating export-then-delete recoverability precisely for the agents most likely to hold operator data. Resolve the workspace dir before the cascade; archive after. + a regression test (delete_cascade_resolves_custom_workspace_before_removing_entry) pinning that agent_workspace_dir yields the custom path while the entry is present and the default once removed, locking the ordering the handler relies on.
Adds the channel arm of delete_with_cascade, completing the
provider/agent/channel trio. On top of the agent cascade.
- delete_with_cascade(.., AliasKind::Channel { channel_type }, ..) for
channels.<type>.<alias>: refuses on HARD refs, otherwise scrubs the
SOFT references β drops the alias from every agent's channels[] and
from escalation.alert_channels[] (retain, trimmed to mirror
find/validate) β removes the entry via the generic
delete_map_key("channels.<type>", alias) the gateway/CLI already use,
and verifies no dangling reference remains. DryRun mutates nothing.
- Two HARD channel-ref classes, both mirroring Config::validate()
(schema.rs:17408-17479):
1. a mandatory dotted `peer_groups.<g>.channel` naming the target;
2. a member of a BARE-type group (`channel = "discord"`) whose only
`<type>.*` channel is the target β scrubbing it would leave the
member without a required channel, yielding a config validate()
rejects. Detected and refused (fail-closed) rather than reported as
a success. The survivor test mirrors validate()'s untrimmed bare
membership check exactly.
Channels carry no owned non-config state (unlike agents), so this is the
full cascade for the kind. TTS/transcription providers remain
NotImplemented.
6 channel cascade tests (scrub+remove, refuse-on-hard-peer-group,
refuse-on-orphaning-bare-group-member, proceed-when-member-keeps-another,
dry-run, not-found). zeroclaw-config suite green; clippy --all-targets +
fmt clean.
0568269 to
b3c1191
Compare
singlerider
left a comment
There was a problem hiding this comment.
Re-reviewed at b3c1191 (my prior APPROVED was at 0568269). The head moved because the stack was rebased, not because the channel arm changed. I diffed alias_refs.rs between the two commits and md5'd the three channel functions: delete_channel, scrub_channel_refs, and collect_channel_refs are byte-identical to what I approved. The only file deltas are already-merged-and-approved slice-3 material now appearing in the rebased base (the RefStrength::Hard doc-comment correction and the cascade_agent_refuses_when_solely_owned_channel test from c02c8aa). I re-ran the channel suite green on the new head (cargo test -p zeroclaw-config cascade_channel 6/6). Re-approving.
The channel cascade review stands unchanged: both HARD channel-ref classes mirror Config::validate() exactly including the deliberate trim-vs-no-trim asymmetry, the type-level and member-level orphan guards are correct companions, scrub mirrors collect, and the post-condition re-walk is fail-closed. RESOLVED β
status for everything in the prior pass.
One note for the merge mechanics, not a review finding: the rebased head now carries the slice-4 fix 927921e (resolve the workspace dir before the cascade removes the entry) in its history, which correctly fixes the π΄ I raised on #7838 - I confirmed the fix and its regression test (delete_cascade_resolves_custom_workspace_before_removing_entry) green. That is the right fix. But #7838 itself is still open with my changes-requested on record. Merging this slice does not substitute for landing slice 4 in order through its own PR; sequence the merges so slice 4 lands first and shows Merged rather than being swept in here.
This slice (own delta):
crates/zeroclaw-config/src/alias_refs.rsonly β +298 / β29. Cumulative shown until #3β#4 merge: 14 files, +1294 / β76.Summary
Adds the channel arm of
delete_with_cascade, completing the provider/agent/channel trio. Forchannels.<type>.<alias>it refuses on HARD refs, otherwise scrubs the SOFT refs, removes the entry via the same genericdelete_map_key("channels.<type>", alias)the gateway/CLI use, and re-checks no dangling reference remains.DryRunmutates nothing.channels[]andescalation.alert_channels[]entry naming the target (retain, trimmed to mirrorfind_all_references+validate()).Config::validate():peer_groups.<g>.channelnaming the target;channel = "discord") whose only<type>.*channel is the target β scrubbing it would leave that member without a required channel.Review: 1 blocker found + fixed
Bare-type peer-group member orphaning.
validate()enforces three channel sites, not two: besidesagents.<X>.channels[](soft) andpeer_groups.<g>.channel(hard), every member of a bare-type group must keep some<type>.*channel. The first cut enumerated only the first two β so deleting a member's onlydiscord.*channel returnedOkbut produced a config thatvalidate()rejects. Fixed:collect_channel_refsnow emits a HARD ref atpeer_groups.<g>.agents[i]when scrubbing would orphan a bare-group member; the survivor test mirrorsvalidate()'s untrimmed bare-membership comparison exactly. (This guard was subsequently lifted into the #7785 foundation during its review; retained here so the channel arm is self-contained.)Validation
cargo clippy --workspace --all-targets+cargo fmt --all --checkclean at the stack tip. Channel test matrix: scrub-soft+remove Β· refuse-on-peer-group-channel Β· refuse-on-orphaning-bare-member Β· proceed-when-member-keeps-another Β· dry-run Β· not-found. Full CI runs on this draft. Pure library arm (no new surface) β exercised through thehandle_delete_map_keyroute/verify'd in slice 4.Security & Privacy β no new fs/network/permissions/secrets; pure in-memory config mutation; synthetic-alias tests.
Compatibility β backward compatible; adds a delete arm that previously returned
NotImplemented. No surface change.Rollback (risk: low) β
git revert <sha>; no migration; no persisted-state effect.Related #7175.