feat(gateway): agent owned-state rename cascade + rename wiring (#7468)

[Nillth]

📋 Stacked series — 7 of 8

Typed delete-with-cascade (#7175) + alias rename (#7468), sliced for review. Opened as draft for CI; merge strictly in order. While the PRs ahead are unmerged this diff is cumulative — it includes the un-merged slices below and collapses to its own delta once they land.

Sequence: 1 #7785 ✅ · 2 #7830 ✅ · 3 #7837 · 4 #7838 · 5 #7839 · 6 #7840 · 7 #7841 ← this PR · 8 #7842

This slice (own delta): 12 files, +620 / −18 (gateway + infra + runtime + memory). Cumulative shown until #3–#6 merge: 14 files, +2610 / −92.

Summary

The surface half of rename (#7468): wires slice 6's rename_with_cascade into the gateway and re-points the agent's owned non-config state. Today POST /api/config/rename-map-key for an agent just swaps the config key — leaving every reference dangling and the agent's persisted state stranded under the old alias.

• handle_rename_map_key routes by section: agents → rename_agent_cascade (config rewrite + workspace move + owned-DB re-point + persist); providers/channels → rename_config_cascade (config rewrite + persist); non-aliased sections → the generic key-swap (unchanged). A missing source alias now returns 404 for aliased paths.
• Persistence correctness (load-bearing): save_dirty writes only marked-dirty paths, and the config-layer cascade never marks dirty — so the handler marks every path in RenameReport.dirty_paths (the renamed entry's old+new key + each rewritten referrer, at entry granularity) before persisting. /verify confirmed all 9 referrer sites rewritten in the on-disk file.
• In-place, no archive; no live-session refusal — a live ACP session simply follows the rename (contrast delete, which refuses).
• Owned-state rename methods (best-effort + reported): Memory::rename_agent (sqlite/postgres update the single agents.alias row — memory rides the UUID; lucid delegates; qdrant rewrites the agent_id payload), cron::rename_jobs_by_agent, AcpSessionStore::rename_sessions_by_agent, SessionBackend::rename_agent_attribution; a cascade_rename_agent coordinator runs them from data_dir.

Review: 2 blockers found + fixed

1. rename_agent UNIQUE(alias) collision silently orphaned memory. Delete purges memories but leaves the agents row (the UNIQUE alias slot), so renaming onto a previously-used-then-deleted alias hit the constraint → the Err was swallowed as a best-effort warning while the config rename committed → split-brain returned as a clean 200. Fixed: rename_agent is now collision-safe — under the connection lock (a transaction for postgres) it refuses if the target alias still owns memory rows, else drops the orphan row and proceeds. + test (orphan-reclaim + live-collision-refusal).
2. Owned-state warnings were dropped from the API response. A partial owned-state failure returned {renamed:true} with the warning only in a server log. Fixed: RenameMapKeyResponse.warnings now carries owned.warnings (omitted when empty).

Validation

cargo clippy --workspace --all-targets + cargo fmt --all --check clean at the stack tip (incl. postgres path). End-to-end /verify (real binary + live gateway): rename victim→survivor → 200, re-read the config FILE → all 9 referrer sites now survivor, grep victim → no matches; probes: rename non-existent → 404, no-op → 400. Full CI matrix runs on this draft.

Security & Privacy

New fs op: the workspace dir move (operator-controlled data_dir). DB writes re-point agent_alias/alias columns. No new network/permissions/secrets. No PII in tests.

Compatibility — backward compatible; agent rename now also rewrites refs + re-points owned state (a fix). New response field warnings is additive (omitted when empty).

Rollback (risk: medium) — git revert; no migration; rename is idempotent/re-runnable.

Deferred

• RPC config/map-key-rename still does the bare key-swap (a pre-existing gap); routing it through rename_with_cascade is a follow-up.
• Markdown/none memory keep the trait default (memory lives in the moved workspace) — a rename_agent bail surfaces as a warning.

Related #7468.

[singlerider]

I reviewed slice 7 on its own delta (f8978da), the surface half of rename (#7468). I read rename_agent_cascade / rename_config_cascade / handle_rename_map_key, the cascade_rename_agent coordinator, and the owned-state rename store methods, cross-checked against the slice-4 delete_agent_cascade it mirrors, and confirmed the memory/infra rename tests green locally. The slice-4 workspace-ordering lesson is correctly applied here, and both self-found blockers are fixed well. One blocker remains: a failed workspace move is the one partial failure this slice does not surface, which contradicts the surfaced-not-masked fix you made for the DB stores in the same PR.

🔴 Blocking: a failed workspace move is silently swallowed, the same split-brain the warnings fix set out to kill

In rename_agent_cascade the workspace move logs a WARN and sets workspace_moved = false on failure, but that outcome never reaches the caller:

match tokio::fs::rename(&old_ws, &new_ws).await {
    Ok(()) => workspace_moved = true,
    Err(err) => { /* WARN log only */ }
}
…
axum::Json(RenameMapKeyResponse { …, renamed: true, warnings: owned.warnings })

owned.warnings comes from cascade_rename_agent, which covers memory / cron / acp / session but not the workspace move (that happens in the handler). So if the config rename + DB re-point succeed but the workspace rename fails (cross-device EXDEV, a permission error, a stale lock), the response is {renamed: true, warnings: []} while the config now points at to and the workspace still sits at the from path. That is exactly the split-brain-returned-as-clean-200 that blocker 2 fixed for the owned DB state, just on the filesystem half. The agent's IDENTITY.md edits and operator notes are now stranded under the old alias dir with nothing telling the operator.

Fix: feed the workspace-move failure into the same surfaced channel, e.g. push format!("workspace move {old_ws:?} -> {new_ws:?}: {err}") onto a warnings vec that the response carries alongside owned.warnings. A handler-level test that forces the move to fail (or at least asserts a simulated owned-state warning round-trips into RenameMapKeyResponse.warnings) would lock it. There is currently no gateway-level test exercising rename_agent_cascade, which is why this and the response-shape contract ride only on /verify - the same coverage blind spot slice 4 had.

🟢 The slice-4 workspace-ordering lesson is applied correctly

old_ws is captured before rename_with_cascade runs (while the entry still lives under from, so a custom workspace.path resolves), and new_ws after. The old_ws == new_ws skip is right: a custom workspace path is alias-independent so there is nothing to move, and only the default <install>/agents/<alias>/workspace location actually relocates. This is exactly the ordering bug I flagged on #7838 (slice 4), and it is not repeated here. Good.

🟢 Both self-found blockers are fixed correctly

• Collision-safe rename_agent: holding the connection lock across the COUNT-check + orphan-drop + UPDATE (a transaction on postgres) closes the TOCTOU, refuses when the target alias still owns memory rows (a genuine conflict, not silently merged), and drops the orphan agents row left behind by a prior delete otherwise. Memory rides the stable agent_id UUID so only the alias column moves. sqlite_rename_agent_reclaims_orphan_and_refuses_live_collision pins both arms. This is the right fix for a real split-brain (delete purges memories but leaves the UNIQUE alias slot).
• Owned-state warnings surfaced: RenameMapKeyResponse.warnings now carries owned.warnings (additive, omitted when empty). Correct - and it is precisely this mechanism the workspace-move failure above should also use.

🟢 Owned-state rename methods and routing are sound

• Memory::rename_agent (sqlite/postgres single agents.alias update, lucid delegates, qdrant rewrites the agent_id payload), cron::rename_jobs_by_agent, AcpSessionStore::rename_sessions_by_agent (live + killed), SessionBackend::rename_agent_attribution - all best-effort + reported, all tested at the store level.
• handle_rename_map_key routes by section (agents → full cascade, providers/channels → config-only rename_config_cascade, non-aliased → generic swap), returns 404 for a missing source on aliased paths, and marks every RenameReport.dirty_paths entry dirty before persist - the same persistence contract slice 6 hands it, consumed correctly.
• No-archive / no-live-session-refusal is the right contract for rename (a live ACP session follows the rename rather than blocking it), in deliberate contrast to delete.

The deferred RPC config/map-key-rename bare-swap gap is a reasonable follow-up to call out. Fix the workspace-move surfacing and this lands.

[singlerider]

This response is the only place a workspace-move failure could reach the caller, but it carries owned.warnings only - and cascade_rename_agent covers memory/cron/acp/session, not the workspace move (which happens above, in this handler). So if the tokio::fs::rename(&old_ws, &new_ws) above fails (cross-device EXDEV, permissions, a stale lock) the response is {renamed: true, warnings: []} while config points at to and the workspace still sits at the from path. That is the same split-brain blocker 2 fixed for the DB stores, on the filesystem half.

Feed the move failure into the surfaced channel, e.g.:

let mut warnings = owned.warnings;
// in the Err(err) arm of the workspace rename above:
warnings.push(format!("workspace move {old_ws:?} -> {new_ws:?}: {err}"));
…
warnings,

A handler-level test that forces the move to fail (or asserts an owned-state warning round-trips into RenameMapKeyResponse.warnings) would lock both this and the response-shape contract, which currently ride only on /verify.

[Nillth]

Thanks — fixed in 780a7a1.

You're right, the workspace move was the one partial failure that didn't surface. Extracted move_renamed_workspace, which returns the warning on a failed move, and merged it with owned.warnings into RenameMapKeyResponse.warnings (and the INFO log) — so a config/DB rename to to with the workspace stranded at from is now reported, not a clean 200. Added a test that forces the move to fail and asserts the surfaced warning.

Addressed

[singlerider]

RE-REVIEW at head 780a7a1. I re-checked my dismissed CHANGES_REQUESTED blocker against the current source. It's resolved. CI 17/17 green, MERGEABLE. This is slice 7 of the rename/delete cascade stack (sibling to #7838-#7842, all on v0.8.1); verdict is on the own delta — the surface half of rename (#7468).

✅ RESOLVED — a failed workspace move during rename is now surfaced, not swallowed

My blocker is fixed. The workspace move is extracted into move_renamed_workspace(old_ws, new_ws) -> Option<String>, which returns the warning on failure (and still WARN-logs). rename_agent_cascade now merges that into the same surfaced channel as the DB stores:

let move_warning = move_renamed_workspace(&old_ws, &new_ws).await;
let mut warnings: Vec<String> = Vec::new();
warnings.extend(move_warning);
…
warnings.extend(owned.warnings);
…
axum::Json(RenameMapKeyResponse { …, renamed: true, warnings })

So a config + owned-DB rename to to with the workspace stranded at from (cross-device EXDEV, permission error, stale lock) now returns the warning instead of a clean {renamed: true, warnings: []}. This closes the split-brain-as-clean-200 on the filesystem half, consistent with the owned-store warnings fix it now matches. The new renamed_workspace_move_failure_is_surfaced test forces the move to fail (new_ws parent is a file) and asserts the warning surfaces, plus that the nothing-to-move paths (old_ws == new_ws, missing source) return None with no spurious warning — that locks both arms. The fix commit is scoped to api_config.rs alone, no creep.

🟢 The rest of the slice still reads clean on re-check

The findings I approved last round all hold on this head:

• Workspace ordering — old_ws is captured before rename_with_cascade (while the entry still lives under from, so a custom workspace.path resolves), new_ws after; the old_ws == new_ws skip is correct since a custom path is alias-independent. The #7838 slice-4 ordering bug is not repeated.
• Collision-safe rename_agent — the COUNT-check + orphan-drop + UPDATE under the connection lock (a transaction on postgres) closes the TOCTOU; refuses on a live target collision, reclaims a prior-delete orphan row otherwise; memory rides the stable agent_id UUID. Pinned by sqlite_rename_agent_reclaims_orphan_and_refuses_live_collision.
• Routing + persistence — handle_rename_map_key routes agents → full cascade, providers/channels → config-only rename_config_cascade, non-aliased → generic swap; 404 on missing source for aliased paths; every RenameReport.dirty_paths entry marked dirty before persist_and_swap. No-archive / no-live-session-refusal is the right contract for rename (a live ACP session follows the rename).

Blocker cleared, no new findings. Approving on the own delta; the stack's in-order merge discipline governs when it lands.

[singlerider]

I re-reviewed slice 7 at head 780a7a1, the surface half of rename (#7468). My prior review (dismissed) carried one 🔴 blocker: a failed workspace move was the single partial failure this slice swallowed, contradicting the surfaced-not-masked fix made for the owned DB stores in the same PR. That is now fixed, verified at head with the regression test green locally.

✅ Resolved: the workspace-move failure is now surfaced, closing the filesystem-half split-brain

move_renamed_workspace (api_config.rs) returns Option<String>: None on success / no-op / missing source, and on a tokio::fs::rename error it WARN-logs and returns the failure as a warning string. rename_agent_cascade seeds its warnings vec with that move warning before extending it with owned.warnings, so the response carries every partial failure, not just the owned-DB ones:

let move_warning = move_renamed_workspace(&old_ws, &new_ws).await;
let mut warnings: Vec<String> = Vec::new();
warnings.extend(move_warning);
…
warnings.extend(owned.warnings);

A config + owned-DB re-point that succeeds while the workspace move fails (cross-device EXDEV, permissions, a stale lock) now returns {renamed: true, warnings: ["workspace move … failed: …"]} instead of a clean {renamed: true, warnings: []}, so the operator can remediate the stranded workspace rather than discovering it later. This is exactly the surfaced channel I asked for, applied to the filesystem half the same way blocker 2 applied it to the DB stores.

renamed_workspace_move_failure_is_surfaced pins it at the unit boundary: it forces the move to fail (new_ws parent is a file so create_dir_all cannot succeed), asserts a "workspace move" warning surfaces, asserts the source dir stays put, and asserts both no-op (old == new) and missing-source paths return None. That is the handler-level coverage this slice's rename_agent_cascade was previously missing. Green locally:

cargo test -p zeroclaw-gateway renamed_workspace_move_failure_is_surfaced
test api_config::tests::renamed_workspace_move_failure_is_surfaced ... ok

🟢 Everything I praised on the prior pass still holds

The slice-4 workspace-ordering lesson is applied correctly (old_ws captured before rename_with_cascade, new_ws after, the old_ws == new_ws skip right for alias-independent custom paths). Both self-found blockers (collision-safe rename_agent under the connection lock; owned-state warnings surfaced in the response) are fixed correctly. Owned-state rename methods and section routing are sound, the dirty-path persistence contract from slice 6 is consumed correctly, and the no-archive / no-live-session-refusal contract is the right deliberate contrast to delete.

The deferred RPC config/map-key-rename bare-swap remains a reasonable follow-up to call out.

Approving. This is a stacked draft-for-CI slice (7 of 8); the merge-strictly-in-order constraint behind #7837-#7840 stands as a merge-time gate, independent of this verdict. Milestone v0.8.1 is already set and matches the series.

[Audacity88]

Reviewed current head 29fb1de after the rebase onto merged #7840. I checked the live PR state, prior review/comment history, the current api_config.rs / owned-state diff, and the store-level rename paths for memory, cron, ACP sessions, and session attribution. I did not run local Cargo for this review-only pass; current CI was still in progress during source review.

✅ Resolved — Workspace move failures now surface to the caller

The prior blocker around swallowed workspace-move failures is fixed. move_renamed_workspace() now returns a warning when the filesystem move fails, and rename_agent_cascade() merges that warning with the owned-state warnings before building RenameMapKeyResponse. That closes the clean-200 case where config and DB state moved to the new alias while the workspace stayed behind with no caller-visible signal.

🟢 What looks good — The owned-state rename coverage is the right shape

The store-level rename additions are pointed at the right canonical state: SQLite/Postgres move the agents.alias row while memory rides the stable agent UUID, Qdrant rewrites the agent_id payload, cron/ACP/session metadata update their owning alias columns, and the Lucid backend delegates to the local SQLite mirror. The collision-safe SQL path that refuses to merge live memory under an existing target alias is also the right safety boundary.

🔴 Blocking — Agent rename moves owned state before config persistence can still split state

rename_agent_cascade() currently performs the irreversible/best-effort side effects before persist_and_swap():

let move_warning = move_renamed_workspace(&old_ws, &new_ws).await;
let owned = crate::agent_owned_state::cascade_rename_agent(...).await;
warnings.extend(owned.warnings);

if let Err(e) = persist_and_swap(state, working).await {
    return error_response(e);
}

That leaves the inverse split-brain still possible. If the workspace move and owned-store re-point succeed but persist_and_swap() fails, persist_and_swap() restores the previous config file snapshot and does not swap the in-memory config, while the workspace, memory alias row, cron jobs, ACP sessions, and session attribution may already be under to. The API returns an error response, not a RenameMapKeyResponse with warnings, so the caller is left with config still naming from and owned state already moved to to.

This matters because the PR's load-bearing contract is that rename either persists the config rewrite and reports partial owned-state failures, or refuses before mutating external state. Right now config durability is last, so a save failure after side effects breaks that contract.

Please persist the config rename before running the workspace move and owned-state cascade, then return warnings for any post-persist side-effect failures. If config persistence must remain last for some reason, the handler needs an equivalent rollback/compensation story for every side effect before returning an error, but persisting first matches the warning-based contract more directly.