Configure ACP session scope per agent - #7578
Conversation
Session scope is an agent behavior, so a global experiment cannot support agents with different context needs. Store it on definitions and use the existing restart-required lifecycle to apply changes safely. Signed-off-by: Salman Mohammed <smohammed@squareup.com>
🔐 Codex Security Review
|
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec590d45fc
ℹ️ 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".
| respond_to: content.respond_to, | ||
| respond_to_allowlist: content.respond_to_allowlist, | ||
| parallelism: content.parallelism, | ||
| session_policy: content.session_policy, |
There was a problem hiding this comment.
Apply session policy during inbound persona merges
When a newer kind:30175 event updates an existing persona—such as an edit arriving from another desktop—this parser carries session_policy, but apply_inbound_persona in commands/personas/inbound.rs copies the other projected fields without copying this one. Consequently, policy edits are ignored on the receiving device, and a persona changed to thread may still launch with channel-wide shared context. Copy the field in that production merge and cover the existing-record inbound path.
AGENTS.md reference: AGENTS.md:L188-L192
Useful? React with 👍 / 👎.
|
🤖 Larry — synthesized independent review Reviewed ec590d4 with three independent lenses: correctness, code simplicity, and UX. The per-agent setting supports the intended mix of a channel-scoped coordinator and thread-scoped peer agents. The definition/resolver/restart-snapshot design follows existing ownership and lifecycle patterns. I found one reproduced correctness issue worth fixing before merge; remaining items are nonblocking. P2 — Preserve session policy when applying an agent-requested behavior edituseAgentManagement.ts:48–54 rebuilds the behavior group for an agent-proposed respondTo change without preserving sessionPolicy. personaBehaviorDraft.ts:41 then defaults the dialog to Entire channel. Repro: configure a thread-scoped agent; have it request a response-mode change; in the resulting edit dialog also change parallelism; Save. The submitted behavior now includes sessionPolicy:"channel" and types/requests.rs:75 persists it, so the next restart loses thread isolation. Merely opening and saving without another behavior edit does not reproduce, because unchanged behavior is omitted. Preserve the existing sessionPolicy in the request overlay and add a regression covering that combined edit. This was reproduced using the HEAD overlay function with production draft/submit functions. Nonblocking improvements
Evidence and limits24 draft/dialog tests passed; the request-overlay regression was reproduced separately. Review traced configuration/persistence, local and remote launch paths, restart snapshots, and ACP session-scope consumers. Catalog test execution was blocked by local loader/dependency setup, not an established product failure. No full Rust build, live Desktop/ACP workflow, browser or assistive-technology run was performed. Source-backed UX findings are not claims of rendered validation. |
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested
Reviewed head ec590d45fcc36645d55637579bf8a987eb6ad2f2 against base 813bbd14121edacc6cb4733301a3af12131aa10e. Two P2 defects prevent the per-definition scope contract from holding across supported edit surfaces.
P2: Apply session policy when reconciling an existing persona
At commands/personas/inbound.rs:682-705, the matched-record branch copies neighboring definition fields but not session_policy. The new parser correctly populates it (managed_agents/persona_events.rs:261), and the insertion branch preserves it, but updates do not.
Reproduction: two devices already have the same persona; change its context from channel to thread on device A. Device B accepts the newer kind:30175 event, calls this merge, saves the old channel policy, and advances retention (inbound.rs:267-275). Restarting there therefore still uses channel scope. The reverse transition also fails. Definitions and subsequent catalog projections can disagree across devices despite successful synchronization.
Copy inbound.session_policy into the matched definition. Add production-reconciliation coverage for channel→thread and thread→channel, including an omitted/default policy clearing a stored thread value.
P2: Preserve scope in owner-reviewed chat-update drafts
useAgentManagement.ts:48-54 rebuilds behavior for an agent-requested respondTo edit. It preserves parallelism but drops the existing sessionPolicy seeded by editPersonaDialogState. draftFromBehavior then defaults it to channel.
Reproduction: start with a thread-scoped personal definition, request an access change through the chat agent-management flow, then open Advanced in the owner-review dialog. It incorrectly displays Entire channel. Change Parallelism and save: behaviorForSubmit sends the complete group, including that unintended channel policy, and apply_persona_behavior persists it. Subsequent restarts lose the selected thread isolation.
Untouched approval does not reset scope: the dialog seeds both draft and comparison baseline from the same input and omits an unchanged behavior group. The regression requires an actual behavior-field edit, as above. Preserve the current session policy when rebuilding this group and cover the owner-review draft→edit→save path.
Validation: source-only review on Wes’s Mac Studio, with independent frontend, runtime, and portability lanes integrated. Traced storage, IPC, inbound updates, copy/import projections, local/remote launch environment, and restart snapshots. No PR checkout, code execution, build, tests, or live UI/relay validation was performed. The removed experiment’s lack of migration is intentional, not a finding.
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
…on-policy Signed-off-by: Salman Mohammed <smohammed@squareup.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Re-review clear
Reviewed head 694f558832c8801d3cb8e66ac851023581c8680d against base e17cdd9d5c7e2b836b4670ae88bb87a79f94337a. Both P2 findings from my prior review are resolved; no new actionable blocker found in the focused fix and integration review.
- Inbound policy propagation: the existing-record merge now copies
session_policy. The production-merge regression covers channel→thread and the default channel value clearing a previously stored thread policy. - Owner-reviewed chat edits: the request overlay preserves the existing
sessionPolicy. The regression follows the overlay through the production draft/submit helpers with another behavior field edited, matching the original failure. - Integration and limits: independent frontend, persistence, and runtime lanes were integrated. Local/remote policy projection, reserved-key precedence, harness routing, and restart-snapshot paths are unchanged from the prior reviewed head; the repaired inputs feed the existing definition-authoritative resolver. The catalog mock fallback and selector/helper accessibility association are also corrected. The old global preview override intentionally has no migration.
Validation was source-only on Wes’s Mac Studio using exact git objects. Added tests were inspected, not run. No PR checkout, build, code execution, live UI/relay, or runtime validation was performed. This clears the prior code findings; it is a comment review, not GitHub approval or a substitute for required CI/security gates.
* origin/main: Configure ACP session scope per agent (#7578) refactor(buzz-acp): point agents at buzz --help instead of a command table (#7586) feat(buzz-cli): render an agent-friendly command tree in --help (#7584) fix(avatars): scale agent squircles from normalized paths (#7307) fix(mobile): bind same-name mentions to exact selected identities (#7385) fix(desktop): isolate quota backoff and reuse channel discovery rosters (#6998) test(desktop): isolate login-shell probe measurements (#7570) feat(git): add default-branch management to relay and CLI (#7562) fix(acp): integrate the Buzz Pi adapter fork (#7552) fix(markdown): align mention chip wrapping (#7501) fix(relay): reject presence updates when Redis storage fails (#7532) fix(desktop): let inbox title and message author names truncate under narrow panes (#7550) fix(buzz-acp): report missing models without retrying (#7538) fix(desktop): require a Codex adapter with Astra support (#7427) fix(desktop): order unnamed roster members by full canonical npub (#7503) fix(mobile): standardize public-key identity display on npub (#7493) fix(desktop): npub identity controls across profile, agents, and workflows (#7489) fix(desktop): npub identity displays for mention, member, and workflow surfaces (#7495) fix(desktop): shared npub identity foundation (canonicalNpub, PubKey gate, strict parser) (#7488) fix(mobile): render push notification sender identity as npub (#7494) Signed-off-by: Sol <478bb5a31222ea2b28a3d1afb8b1d598940628f19c2a87efc3c4b822299eeec6@buzz.block.builderlab.xyz> # Conflicts: # desktop/src-tauri/src/commands/media_download.rs
Summary
PR #6732 taught the ACP harness to honor
BUZZ_ACP_SESSION_POLICY, but the desktop exposed that policy as one global experiment. Turning it on changed every managed agent at once, even though conversation scope is part of how an individual agent should behave. Some agents need continuity across a channel, while agents such as validators may need clean context for each thread.This moves the choice onto the agent definition under Advanced as Conversation context, with Entire channel and Each thread options. Entire channel remains the default, so existing definitions keep their current behavior.
The policy now travels through create and update IPC, returned agent summaries, persona events, snapshots, teams, and community catalog copies. Launches pass the effective definition value to the harness. Editing a running agent uses the same lifecycle as a model edit: it shows the restart-required state, leaves the deployed process untouched, and applies the new policy on restart.
The global Thread Scoped ACP Sessions experiment and its desktop command/state have been removed because the setting no longer has a useful global meaning. There is intentionally no migration for the old preview override. Users who enabled it can choose Each thread for the agents that need it.
Unknown or null policy values degrade to Entire channel across stored agents, persona events, and catalog projections. This keeps forward-version data from dropping complete agent records.
Related issue
No matching issue found. Builds on #6732.
Testing
create_personareceivedsessionPolicy: "thread".session_policy: "thread".Generated with Codex