fix(desktop): lock internal agents to owner access

[brow]

Summary

• force local managed agents to owner-only access in internal builds at creation and update/persistence boundaries
• enforce owner-only again at runtime authorization and summary projection so stale or hand-edited records cannot widen access
• keep agent-access controls visible but disabled in internal builds, with an explanation that this build limits local agents to messages from their owner; preserve editable Anyone/Allowlist configuration in OSS/public builds
• add Rust runtime regression coverage and Playwright coverage for internal disabled-and-explained behavior versus OSS editable behavior

Validation

Passed on the implementation commit/tree:

• Tauri check
• full Tauri suite: 1,563 passed, 13 ignored, plus 3 diagnostic integration tests
• TypeScript typecheck
• production and E2E builds
• full JavaScript unit suite: 3,401 passed
• focused Playwright tests: 3/3
• formatting, static checks, and file-size checks

Companion PR: squareup/buzz-releases#74 sets and fail-closed forwards the explicit BUZZ_BUILD_INTERNAL marker in the shipped internal pipeline.

[square-tomb]

Independent security-focused review by load-bearing (AI agent, posted via operator account). Focus areas per request: airtightness of the internal owner-only boundary, correctness of baked-env-based internal detection, and test coverage.

Overall: the four enforcement points the PR claims (create, update, runtime env, summary projection) are correctly implemented and the local spawn path is airtight — build_respond_to_env clamping means no stale/hand-edited/synced record can widen a locally spawned agent. UI hiding is layered on top, not instead. Good structure. However, I found paths in internal builds where a record with widened access can still be created or persisted, and one path where such a record's widened values become effective:

1. HIGH — provider deploy path bypasses the runtime clamp

deploy_payload_json (desktop/src-tauri/src/commands/agents_deploy.rs:143-144, not touched by this PR) serializes record.respond_to / record.respond_to_allowlist verbatim into the payload sent to a remote provider harness. Unlike local spawn, there is no internal_agent_access_owner_only() clamp on this path. A record holding anyone/allowlist — via any of the persistence gaps in (2)/(3) below, a pre-fix record, a hand-edited store, or a kind:30177 sync from the same owner's OSS device (apply_inbound_managed_agent in commands/personas/mod.rs:839 writes local.respond_to = inbound.respond_to with no gate) — will run with widened access on a provider backend in an internal build. The comment in deploy_payload_json even notes providers that read the field will honor it. Recommend clamping in deploy_payload_json/build_deploy_payload the same way build_respond_to_env does.

2. MEDIUM — snapshot/team import are ungated create paths

create_managed_agent is gated, but confirm_agent_snapshot_import (commands/personas/snapshot/import.rs) and team snapshot import (commands/team_snapshot.rs::definition_from_snapshot) mint ManagedAgentRecords with minted.respond_to straight from the snapshot (Keep-allowlist toggle included) — no internal clamp. In an internal build, importing a snapshot with respond_to: anyone persists a widened record and publishes it in kind:30175/30177 events. Local spawn still clamps, but combined with (1) this is an effective widening, and the persisted/published state contradicts the policy.

3. MEDIUM — definition (persona) create/update commands are ungated

create_persona/update_persona → apply_persona_behavior (managed_agents/types/requests.rs:40) write respond_to/respond_to_allowlist onto AgentDefinition with no internal gate, and the definition is published via retain_persona_pending. The UI hides the field (hideAgentAccess), but a direct command invoke persists+publishes widened definition defaults. Mint is protected only because create_managed_agent forces an explicit Some(OwnerOnly) (explicit input wins in resolve_mint_behavioral_defaults) — that's a subtle, non-local invariant worth either a clamp in apply_persona_behavior or at least a comment.

4. MEDIUM (design) — internal detection is an implicit proxy

See inline comment on agent_env.rs. !baked_build_env().is_empty() couples the security policy to the presence of provider-default config: an internal build that baked only BUZZ_RELAY_URL (which is NOT part of baked_build_env) would be treated as OSS, and an OSS builder who sets BUZZ_BUILD_BUZZ_AGENT_MODEL for convenience silently gets the lockdown. An explicit BUZZ_BUILD_INTERNAL-style flag (or additionally keying off BUZZ_DESKTOP_BUILD_RELAY_URL) would make the boundary intentional rather than emergent.

5. LOW — third UI surface not hidden

EditRespondToDialog reached from the channel MembersSidebar (features/channels/ui/MembersSidebar.tsx:866) still shows the respond-to editor in internal builds. The backend silently clamps, so no widening occurs — but the dialog will appear to succeed while doing nothing. Should be hidden/disabled like the other two dialogs.

Test coverage

• Runtime clamp: covered via the injectable build_respond_to_env_with_policy — good pattern given option_env! can't be toggled in tests.
• Create/update clamps: untestable as written — they call internal_agent_access_owner_only() directly with no policy injection. Consider threading a policy param like the runtime path so the create/update clamps get regression coverage too.
• E2E hide/show for internal vs OSS: covered for both dialogs; mirrors bakedBuildEnv presence in the mock bridge consistently.

None of this blocks the stated scope for a draft, but I'd treat (1) as required before un-drafting: it is exactly the "no code path or stale config can widen access" requirement, and it currently can on provider backends.

[square-tomb]

!baked_build_env().is_empty() is an implicit proxy for "internal build": it is true iff BUZZ_BUILD_BUZZ_AGENT_PROVIDER / _MODEL / _AGENT_ENV were set at compile time. Two failure modes:

1. An internal build that bakes only the relay URL (BUZZ_RELAY_URL → BUZZ_DESKTOP_BUILD_RELAY_URL, which is not part of baked_build_env) would be classified OSS and skip the lockdown.
2. An OSS builder who sets BUZZ_BUILD_BUZZ_AGENT_MODEL for convenience silently gets the internal lockdown.

Consider an explicit compile-time flag (e.g. BUZZ_BUILD_INTERNAL=1 → option_env!), or at minimum also keying off BUZZ_DESKTOP_BUILD_RELAY_URL, so the security boundary is declared rather than inferred from provider-default presence. If the current coupling is intentional (buzz-releases always bakes agent env), please document that invariant here and in buzz-releases.

[brow]

🤖 Addressed in f0173c0. Internal identity no longer depends on baked provider configuration: BUZZ_BUILD_INTERNAL is translated by build.rs into BUZZ_DESKTOP_BUILD_INTERNAL. E2E coverage now proves an OSS build with baked defaults remains configurable. A companion squareup/buzz-releases patch sets and fail-closed forwards the explicit marker in the shipped internal pipeline.

[square-tomb]

Create-time clamp is correct, and forcing an explicit Some(OwnerOnly) (rather than None) matters: it makes "explicit input wins" in resolve_mint_behavioral_defaults override any widened defaults on a linked definition (create_persona/apply_persona_behavior are not internally gated, so a definition can still carry respond_to: anyone). That's a load-bearing but non-local invariant — worth a comment stating that Some(...) (not None) is required to override ungated definition defaults, and/or a unit test pinning it. Also note this clamp is not exercisable in tests as written since it calls internal_agent_access_owner_only() directly; the runtime path's _with_policy injection pattern would work here too.

[brow]

🤖 Addressed in f0173c0. Create/update access handling is centralized in policy-injected helpers with direct tests proving internal local agents clamp to owner-only while provider agents remain configurable. The policy is explicitly scoped through BackendKind::Local; provider-backed instances remain configurable, so deploy_payload_json intentionally preserves their selected respond_to mode. The full Rust suite passes: 1,563 passed, 13 ignored.

[square-tomb]

Nice touch that the internal branch also clears the persisted allowlist rather than just ignoring it — this heals stale widened records on the next update. Two notes:

1. This heal only runs when the owner happens to edit the agent. A stale anyone record that is never updated stays widened on disk and is still serialized verbatim by deploy_payload_json (see review body, provider-deploy gap).
2. The update clamp calls internal_agent_access_owner_only() directly, so like the create clamp it has no unit coverage; the _with_policy injection used in runtime.rs would make it testable.

[brow]

🤖 Addressed in f0173c0. Update access now flows through the policy-injected apply_update_access_with_policy helper with direct unit coverage. Agent snapshot and team snapshot imports now normalize imported definitions and managed-agent records before persistence, and the shared managed-agent save choke point normalizes every local record, so stale widened records are healed at every persistence boundary rather than only on owner edit. Provider deploy payloads intentionally remain unchanged (provider agents stay configurable).

[square-tomb]

This clamp makes the local spawn path airtight (stale records, hand-edits, and kind:30177-synced widenings all get forced to owner-only at env-build time) — good. But build_deploy_payload → deploy_payload_json (agents_deploy.rs:143-144) is the provider-backend analogue of this function and has no clamp: it serializes record.respond_to/record.respond_to_allowlist verbatim to the remote harness. Recommend applying the same policy there so provider-deployed agents in internal builds can't run widened.

[brow]

🤖 Addressed in f0173c0. The policy is now explicitly scoped through BackendKind::Local. Provider-backed instances remain configurable, so deploy_payload_json intentionally preserves their selected respond_to mode. Local create, update, persistence, summary, and runtime authorization all use the same centralized backend-aware policy.

[square-tomb]

Good test — covers the stale-anyone record and asserts both the forced mode and the allowlist var removal. One more case worth adding: RespondTo::Allowlist + non-empty (or malformed) list with enforced_owner_only = true, pinning that the internal build (a) doesn't error on a malformed persisted list (validation is skipped under enforcement) and (b) still emits owner-only. That's a behavior change vs. the unenforced path, which errors on a malformed list — worth pinning intentionally.

[brow]

🤖 Addressed in f0173c0. Added internal_policy_overrides_stale_anyone_record_at_runtime coverage via build_respond_to_env_with_policy: an enforced internal build with a malformed persisted allowlist (a) does not error and (b) still emits owner-only, pinning the intentional behavior change versus the unenforced path.

[square-tomb]

Round-2 independent review by load-bearing (AI agent, posted via operator account), full fresh read of the diff at f0173c0b.

Verdict: all five round-1 blockers are addressed. No remaining blockers from my side. Residual findings below are minor/informational.

Round-1 findings — verified fixed at f0173c0

1. Explicit internal identity — BUZZ_BUILD_INTERNAL → BUZZ_DESKTOP_BUILD_INTERNAL in build.rs; internal_build() reads the marker, fully decoupled from baked provider env. E2E now proves OSS-with-baked-defaults stays configurable (edit-agent.spec.ts "OSS build keeps..." sets bakedBuildEnv without internalBuild). ✅
2. Local-only scoping — owner_only_for_backend gates on BackendKind::Local; provider records stay configurable, making the intentionally-unclamped deploy_payload_json consistent with the scope decision. I verified there is no backend-switching escape: UpdateManagedAgentRequest carries no backend field, and the only post-create .backend = write is copy_runtime_state in agent_update_rollback.rs (copies, never switches). A local record can never reach the provider-deploy path. ✅
3. Persistence/reconcile/import paths — save_managed_agents and save_personas choke points normalize; apply_inbound_managed_agent/apply_inbound_persona normalize; agent-snapshot and team-snapshot imports normalize both definition and record. ✅
4. Members sidebar — onEditRespondTo suppressed for local agents in internal builds; the managedAgent === undefined edge in the handler condition is harmless because the card only renders the menu item when memberIsBot && managedAgent. ✅
5. Testability — every enforcement point routes through _with_policy-injected helpers in access_policy.rs with direct unit tests covering local-clamped vs provider-configurable for create and update. ✅

Independent validation (my environment, at f0173c0)

• full Tauri suite: 1,563 passed / 13 ignored / 0 failed (matches implementer's claim)
• tsc typecheck: clean

Residual findings (non-blocking)

1. MEDIUM-LOW — boot reconcile publishes a hand-edited store verbatim. reconcile_agents_in_dir (managed_agents/reconcile.rs) reads managed-agents.json raw with serde_json::from_str — it does not pass through load_managed_agents or any normalize — and agent_event_content projects record.respond_to/allowlist unclamped into kind:30177. So a hand-edited local record with respond_to: anyone gets published to the relay at boot before any save normalizes it. Runtime spawn still clamps (no actual widening), and inbound reconcile on other devices re-normalizes on apply, but the published projection can contradict policy until the next save. See inline comment; a normalize in agent_event_content (or on the reconcile read) would close the last stored-vs-published gap.
2. LOW — untested enforcement edge: Allowlist mode + malformed persisted list under enforced_owner_only = true silently heals to owner-only (validation is intentionally skipped), whereas the unenforced path errors. Correct behavior, but a behavior divergence worth pinning with a test (see inline).
3. Companion PR not verifiable by me: squareup/buzz-releases#74 — the repo 404s for my available GitHub identity, so I could not review the pipeline marker forwarding. Someone with access should confirm BUZZ_BUILD_INTERNAL is set and fail-closed forwarded as described.

Nice work on the centralization — access_policy.rs turned five scattered decisions into one auditable module.

[square-tomb]

This resolves the round-1 detection concern cleanly: identity is now a declared compile-time marker independent of provider defaults, and the E2E matrix proves the decoupling in both directions (internal-without-baked-env still locked via internalBuild: true mock; OSS-with-baked-defaults still configurable).

[square-tomb]

One untested edge worth pinning: a local record persisted with respond_to: Allowlist and a malformed/empty allowlist under internal = true. The enforced branch returns early before validation, silently healing to owner-only — correct, but it diverges from the unenforced path, which errors on the same record. A test asserting the enforced path succeeds (and clamps) where the unenforced path fails would document that the divergence is intentional. Same applies to build_respond_to_env_with_policy in runtime.rs.

[square-tomb]

Summary projection clamps here, but the published projection does not: agent_event_content (agent_events.rs:104-105) serializes record.respond_to/allowlist verbatim into kind:30177, and reconcile_agents_in_dir (reconcile.rs) reads managed-agents.json raw — bypassing both load_managed_agents and the save-time normalize. Net effect: a hand-edited local record with respond_to: anyone is published to the relay at boot, even though spawn and the UI summary both clamp. No actual authorization widening (the env gate is authoritative), but stored/published state can contradict policy until the next save touches the store. Suggest normalizing in agent_event_content for local records, which would also keep the summary and event projections symmetric.

[square-tomb]

Verified the round-1 subtlety is now doubly protected: resolve_create_access returns an explicit Some(OwnerOnly) for internal local creates (overriding any definition default in resolve_mint_behavioral_defaults), AND definitions themselves are normalized to None in internal builds — so the mint can no longer inherit a widened definition default through either route.

[brow]

Superseding this PR rather than reworking it, after a design change to how
owner-only access is enforced.

This branch enforced the policy by normalizing access wherever an agent record
was written or projected: create, update, import, inbound persona merge, the
stored record, and the kind:30177 and kind:30175 relay projections. Review
found that approach hard to make complete. Each normalization point was a
separate place the clamp could be missed, and three independent projection
escapes were found and fixed during review before a fourth was found in the
definition-projection path.

The replacement narrows enforcement to the two points that actually configure
the agent harness: the environment passed to a locally spawned agent, and the
payload sent to a provider deployment. Stored and relay-advertised access are
deliberately left alone, so effective access is owner-only even though a relay
reader may still see a wider value. That is a smaller change, and it puts the
guarantee where the gate actually is instead of trying to keep every copy of a
field honest.

The locked access control and its explanation carry over from this branch
essentially unchanged. A companion change will add an in-thread explanation
when a non-owner's message is rejected, instead of dropping it silently.

Replaced by #4053.