fix(desktop): scope managed-agent roster per community (#7184) - #7204
Kingvelazquez013 wants to merge 1 commit into
Conversation
🔐 Codex Security Review
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd21a5bc8b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for record in all { | ||
| if record.pubkey.is_empty() { | ||
| definitions_out.push(record); | ||
| } else { | ||
| legacy_out.push(record); |
There was a problem hiding this comment.
Every scoped instance is first copied from shards into all, but this loop then puts every keyed record into legacy_out regardless of its relay. The same record is subsequently written to its shard, and load_agent_store concatenates both files, so creating the first scoped agent immediately returns duplicate roster entries; later saves persist and multiply those duplicates. Repartition the post-keyring records by relay instead of using pubkey alone.
Useful? React with 👍 / 👎.
| // community that no longer has any records is rewritten as `[]` rather | ||
| // than deleted — deletion would race a concurrent reader and `[]` keeps | ||
| // the fail-loud parse contract uniform (a missing file is also valid). | ||
| for (host, mut shard_records) in shards { |
There was a problem hiding this comment.
When the last agent in a community is deleted, partition_by_community produces no entry for that host, so this loop never rewrites the existing shard as [] despite the preceding comment. Because future loads enumerate every old shard, the deleted record reappears after delete_managed_agent reports success, now after its key was deleted and its relay head tombstoned. Enumerate existing shards and explicitly empty or remove those absent from the new partition.
Useful? React with 👍 / 👎.
| let host = host_port | ||
| .rsplit_once(':') | ||
| .map(|(host, _)| host) | ||
| .unwrap_or(host_port); |
There was a problem hiding this comment.
Discarding the scheme, port, path, and query collapses distinct valid community relays onto one key. For example, the UI permits both ws://localhost:3000 and ws://localhost:3030, while this function maps both to localhost; the active-community filter therefore exposes each community's agents in the other, leaving the reported isolation bug unfixed for these configurations. Use the repository's canonical full relay URL identity and encode or hash it for filenames.
AGENTS.md reference: AGENTS.md:L612-L617
Useful? React with 👍 / 👎.
| // keyring-backed case (it is the user's own agent store) and closes the | ||
| // umask window a post-write `chmod` would leave open. | ||
| atomic_write_json_restricted(&path, &payload) | ||
| atomic_write_json_restricted(&legacy_path, &legacy_payload)?; |
There was a problem hiding this comment.
This legacy-file commit is followed by independent commits for each shard, with no transaction marker or durable retry plan. If a later shard write fails—for example from disk exhaustion or a per-file permission problem—the command returns an error after earlier files already contain the update; a failed create can consequently persist an agent without running the remaining publication/setup steps, and retrying creates another identity. Persist one atomic snapshot or journal the remaining shard commits so every observable prefix is recoverable.
AGENTS.md reference: AGENTS.md:L204-L209
Useful? React with 👍 / 👎.
a77c5b7 to
046ce14
Compare
|
@wpfleger96 @jmecom @loganj — this PR has been rebased onto current main (046ce14) and the conflicts are resolved;
Background: #7184 is a tenant-isolation breach — managed agents created in one community appear in all communities. This fix scopes the roster per community via per-relay shards ( |
Managed agents were stored in a single managed-agents.json shared by every community, so an agent created in one community appeared in all of them with full configuration — a tenant-isolation breach. - Storage: split the keyed-instance store into per-community shards (managed-agents-community.<relay-host>.json) next to the legacy global store. Key-less definitions and unpinned records stay in the legacy file, so existing stores need no migration and a rollback build reads the same file. - Load: load_managed_agents_for_active_community() filters instances to the active workspace relay host (fail-open: unresolvable host or unpinned record = visible, matching pre-fix behavior; never an empty-roster data-loss appearance). - Create: an empty request relay is stamped with the active workspace relay at mint time, so new agents are born scoped to the community they were created in. Spawn eligibility is unchanged (block#2122 agents-everywhere): the pin scopes roster visibility only. - Reconcile: boot reconcile enumerates legacy store + all shards so kind:30177 heads for every community stay published. Closes block#7184 Signed-off-by: Kingvelazquez013 <258349814+Kingvelazquez013@users.noreply.github.com>
046ce14 to
a4e1bf5
Compare
|
@wpfleger96 — this PR is now rebased onto current Focused verification passed:
Could you please authorize the security review for this exact head by commenting exactly:
The tenant-isolation fix is ready for review. Thank you. |
Security: Managed agents created in one community appear in all communities
Closes #7184
Problem
Managed agents are stored in a single
agents/managed-agents.jsonunder the app-data dir, read and written identically for every configured community. Creating an agent in Community 1 therefore makes it appear — with name, system prompt, env vars, and full configuration — in Communities 2 and 3. The agent list is also re-polled every 5 seconds from this one file, so the leak is continuous, not transient.Why the fix goes here (and not at spawn)
The per-record
relay_urlpin is deliberately ignored at spawn (effective_agent_relay_url, #2122 "agents-everywhere") — every agent is eligible on every community, and the pair is keyed by the workspace relay. This PR does not reopen that decision. Spawn eligibility is untouched; what changes is roster visibility and storage scope, which is what #7184 reports.What this PR does
1. Community shards in storage (
managed_agents/storage.rs)Keyed instances are partitioned by the host of their
relay_url:managed-agents-community.bookd.communities.buzz.xyz.jsonmanaged-agents-community.av0.communities.buzz.xyz.jsonThe legacy
managed-agents.jsoncontinues to hold key-less definitions and unpinned records. Consequences:relay_host_of) is strict: scheme required,[a-z0-9.-]only, no empty/dotted labels. Unparseable relays fail open to the legacy store — a record is never dropped by a save.managed-agents-community.is deliberately distinct frommanaged-agents.jsonso hand-made backup copies (.bak, Copilot-style per-community copies) are never mistaken for shards.2. Community-scoped roster read (
load_managed_agents_for_active_community)list_managed_agents(the 5s-polled agents menu) now loads via a scoped reader that filters instances to the active workspace relay host (relay_ws_url_with_override— workspace override first, then env/build vars, then default, exactly matching how the rest of the app resolves the active community).Fail-open rules, so the fix can never look like data loss:
relay_url→ visible everywhere (it cannot be attributed to a community).3. Mint-time scoping (
create_managed_agent)An agent created with no explicit relay is stamped with the active workspace relay at creation. New agents are therefore born scoped to the community where they were created — the exact bug in #7184. Explicit relays still pin as before, and spawn still ignores the pin (#2122).
4. Boot reconcile covers shards (
managed_agents/reconcile.rs)The boot-time kind:30177 reconcile enumerates the legacy store plus every shard, so an agent's published identity head stays reconciled regardless of which community was active when its shard was last written. Per-file fail-loud parse contract is preserved (malformed shard →
.invalidbackup + loud error).Testing
wss://, ports, paths, traversal/unsafe-host rejection), partition routing (pinned → shard, unpinned → legacy), shard enumeration (ignores backups/hand copies/.invalid), shard filename shape.cargo clippyandcargo fmt --checkclean.Deliberately out of scope
list_relay_agents; local storage is the boundary reported in Security: Managed agents created in one community appear in all communities (tenant isolation breach) #7184).