Skip to content

fix: persist agent respond-to edits; quiet partition overlap errors - #2505

Open
Chessing234 wants to merge 3 commits into
block:mainfrom
Chessing234:fix/respond-to-partitions
Open

fix: persist agent respond-to edits; quiet partition overlap errors#2505
Chessing234 wants to merge 3 commits into
block:mainfrom
Chessing234:fix/respond-to-partitions

Conversation

@Chessing234

@Chessing234 Chessing234 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Summary

Test plan

  • Edit a custom agent definition’s respond-to to Anyone, restart the agent, confirm harness log shows respond_to=anyone
  • Same for Allowlist with a member pubkey
  • Instance set to anyone, persona gate unset: edit only the system prompt — instance stays anyone
  • Broken owner-only instance + definition already Anyone: open dialog, save without changing the gate — instance becomes anyone

Fixes #2501

Made with Cursor

@Chessing234

Copy link
Copy Markdown
Contributor Author

Partition half of this PR only quiets the overlap ERROR (#2474). Prefer #2456 for the real roll-forward; happy to drop that hunk here if it helps review.

@georgerous

Copy link
Copy Markdown

Thanks for the incredibly thorough writeup — this matches what I saw and adds
detail I hadn't isolated. Confirming your key points from my side (Desktop
v0.4.23, self-hosted relay), plus one wrinkle worth adding.

Confirmed: the instance record is what matters. You nailed it. Patching only
the definition entry does nothing — the harness keeps starting owner-only.
The fix only takes once you write respond_to (+ respond_to_allowlist) onto
the instance record (the one with a pubkey, no slug). After hand-editing
both records to allowlist + a member pubkey, the harness starts with
respond_to=allowlist(1) and stays that way across app restart and agent
stop/start.

One wrinkle to add: it's not just the "Who can talk" control. Saving any
edit to the agent from the UI resets it. In my case, editing the agent's
system prompt rewrote respond_to back to owner-only on the definition
record (the running instance kept my patch, so it kept working until the next
regeneration). So the practical failure mode is broader than the permissions
form — any UI save on the persona silently reverts the security setting, and you
have to re-apply the manual patch afterward.

On the child-process point — fully agree, and it bit me too: since every
buzz-acp is a child of buzz-desktop, the file has to be patched with the app
fully quit, by something outside the app (a human in a terminal). Quitting to
release the file also kills the agents.

Net: with the env override (rightly) reserved and the UI path broken, manually
editing managed-agents.json on both records, app quit, is the only thing
that works today. Happy to test a build once #2505 lands.

@Chessing234

Copy link
Copy Markdown
Contributor Author

thanks for confirming — yeah the instance record is the one that sticks; happy to have you poke a build once this lands

Who-can-talk edits on a persona only wrote definition_respond_to; the harness reads instance respond_to, so linked agents stayed owner-only.

Signed-off-by: Taksh <takshkothari09@gmail.com>

Signed-off-by: Taksh <takshkothari09@gmail.com>
Match on the overlap message alone so missing SQLSTATE 42P17 no longer logs ERROR on every boot against *_p_future.

Signed-off-by: Taksh <takshkothari09@gmail.com>

Signed-off-by: Taksh <takshkothari09@gmail.com>
@Chessing234
Chessing234 force-pushed the fix/respond-to-partitions branch from 47be6e9 to e456daa Compare July 29, 2026 15:00
@georgerous

Copy link
Copy Markdown

Read through the diff — the instance-record half is right, and it matches what I confirmed by hand: writing respond_to (+ respond_to_allowlist) onto the records the harness actually reads is what makes the setting stick. Passing the gate explicitly in buildInstanceInputForDefinition rather than leaning on the mint-time lookup is a good call too.

Two things I'd want addressed before I test a build, one of which is the "any UI save reverts it" wrinkle I raised — I think this PR makes that one worse rather than better.

  1. Unrelated persona edits now push owner-only onto the instance.

In personaManagedAgentUpdate (UserProfilePanelUtils.ts):

const personaRespondTo = persona.respondTo ?? "owner-only";
if (personaRespondTo !== agent.respondTo) {
input.respondTo = personaRespondTo;
hasChanges = true;
}

That helper runs on every persona save from the profile panel (submitProfilePersonaDialog calls it unconditionally on the edit path), and it already returns an update whenever any field differs — prompt, model, envVars, name. So for a persona that carries no explicit gate (respondTo undefined, i.e. "unset, harness default applies"), the ?? "owner-only" coercion turns an unrelated edit into an explicit downgrade written onto the instance record.

Concretely: instance set to anyone on the agent's own settings, persona with no gate set, edit the persona's system prompt → instance silently goes back to owner-only. That's the failure mode I reported, except before this change the instance record kept its value and the agent kept working. Now the revert reaches the record the harness reads.

The Rust side has the same coercion (None => RespondTo::default() in propagate_persona_respond_to) but it's gated behind behavior_present, so it's narrower — it fires on the explicit-clear path that submits {}.

Suggested fix: don't coerce. If the definition carries no explicit gate, leave the instance alone — undefined on the definition means "unset", not "owner-only", and only an explicit definition-level choice should overwrite an instance-level one. Plus a test that a prompt-only save produces an update with no respondTo key.

  1. No in-app repair path for instances that are already wrong.

behaviorForSubmit returns undefined when the draft equals its seed, and apply_persona_behavior is a no-op on None. So for anyone sitting on a broken instance today: open the dialog, see "Anyone" already selected (the definition was saved correctly — that half always worked), hit save → nothing submitted → nothing propagated. You have to toggle to a different mode, save, toggle back, save. Nothing reconciles at agent start either, so upgrading to this build doesn't fix existing instances, only future edits. Worth either reconciling definition → instance at start, or propagating on save even when the behavior group is unchanged.

  1. Minor / procedural
  • The partition hunk: dropping the 42P17 check leaves the match on message text alone, and the code never verifies the overlapping partition is actually the *_p_future catch-all — it assumes it. A genuine overlap now logs info and boots. You already offered to drop this in favour of fix(db): roll the events/delivery_log catch-all partition forward #2456; I'd take that offer, it'd also make this PR single-purpose.
  • CI here is only DCO / Semgrep / zizmor — no Rust build or test job, so the new respond_to_propagation_tests aren't gating anything.
  • update.rs: the if avatar_changed || name_changed || behavior_present goes multi-line but the body keeps its old indentation; cargo fmt --check would flag it.

Happy to run a build once (1) is sorted — that's the one that would bite me directly.

Chessing234 added a commit to Chessing234/buzz that referenced this pull request Jul 29, 2026
Address review on block#2505: leave instance gates alone when the definition
has no explicit respond-to, reconcile an explicit gate on any persona
save so stale instances heal, drop the partition overlap hunk, and add
regression coverage for the prompt-only path.

Co-authored-by: Cursor <cursoragent@cursor.com>
@Chessing234

Copy link
Copy Markdown
Contributor Author

Addressed your review:

  1. No coercepersonaManagedAgentUpdate and propagate_persona_respond_to now leave the instance alone when the definition gate is unset (null/None). Added a prompt-only regression test so an anyone instance can't be silently downgraded by an unrelated persona edit.

  2. Repair path — any persona save whose definition already carries an explicit gate re-reconciles that gate onto linked instances, so you don't have to toggle the control to heal a stale owner-only instance.

  3. Partition hunk dropped — reverted to main; prefer fix(db): roll the events/delivery_log catch-all partition forward #2456 for the real roll-forward.

Happy for another look when you have a moment.

Address review on block#2505: leave instance gates alone when the definition
has no explicit respond-to, reconcile an explicit gate on any persona
save so stale instances heal, drop the partition overlap hunk, and add
regression coverage for the prompt-only path.

Signed-off-by: Taksh <takshkothari09@gmail.com>
Chessing234 added a commit to Chessing234/buzz that referenced this pull request Jul 29, 2026
Address review on block#2505: leave instance gates alone when the definition
has no explicit respond-to, reconcile an explicit gate on any persona
save so stale instances heal, drop the partition overlap hunk, and add
regression coverage for the prompt-only path.

Signed-off-by: Taksh <takshkothari09@gmail.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@Chessing234
Chessing234 force-pushed the fix/respond-to-partitions branch from c4ee7a3 to 7dc7922 Compare July 29, 2026 18:22
@georgerous

Copy link
Copy Markdown

Went through the new diff — all three are genuinely addressed, not just
described:

  • No-coerce: persona.respondTo != null on the TS side (catches undefined
    as well as null, which was the trap) and the
    let Some(wire) = … else { return Ok(0) } early-out in
    propagate_persona_respond_to. The prompt-only save leaves unset respondTo alone and respond_to_unset_on_definition_leaves_instances_alone tests are
    the right shape.
  • Repair path: gating on result.respond_to.is_some() rather than
    behavior_present means a plain Save reconciles, since
    submitProfilePersonaDialog calls updatePersona unconditionally. No
    toggling needed — good.
  • Partition hunk: gone, and the cargo fmt indentation went with it.

One thing left, and it's the half of my original report that actually broke me
rather than the permissions form. My patch survived app restart and agent
stop/start — what killed it was regeneration: the instance getting re-minted
from the definition, at which point it came back owner-only. That's why
hand-patching was never really a workaround, just something I had to re-apply
without knowing when it had lapsed.

buildInstanceInputForDefinition now carries the gate, which covers the
regeneration paths that go through it. But not all of them do:

  • buildWelcomeStarterCreateInput (onboarding/welcomeGuide.ts) spreads
    buildInstanceInputForDefinition(...) and then hard-pins
    respondTo: "owner-only" immediately after, so the spread's gate is
    discarded. Plausibly deliberate for welcome-team starters — worth confirming,
    and confirming it can't catch a persona the user later reuses outside the
    welcome team.
  • channelAgents.ts builds its own createManagedAgent input with its own
    respondTo / respondToAllowlist plumbing rather than going through the
    helper, so it inherits the definition gate only if the caller passes it.

Could you enumerate the mint/regeneration paths and confirm each one carries the
definition gate? If any path still mints owner-only from a definition that has
an explicit gate, this lands, passes its tests, and I'm back to re-patching
JSON.

Related: the test plan here is unit-level. The check that would settle it from
my side is end-to-end with no hand-patching — set "Who can talk" to
Allowlist with a member pubkey, save, then force a regeneration (whatever
triggers a re-mint: config-change restart, runtime edit, team resync) and
confirm the harness log still shows respond_to=allowlist(1) afterwards. If you
add that to the test plan I'll run exactly it on a build.

Separately, for your own bookkeeping: the "any UI save rewrites the definition
back to owner-only" part of my report looks already guarded on main
behaviorForSubmit returns undefined when the draft equals its seed, so a
prompt-only edit submits no behavior group. My original observation was on
Desktop v0.4.23, which likely predates that. So don't chase that half on my
account; I'll re-verify against a current build.

@georgerous

Copy link
Copy Markdown

Independent repro just landed on #2501 (MaxWynnDev, Desktop v0.5.2 / Windows 11 /
hosted relay), and it splits this into two defects rather than one:

  1. The selected principals are never persisted at all — after picking
    Allowlist and adding two members, respond_to_allowlist stays [] on both
    records. definition_respond_to captures the mode; nothing captures the
    people.
  2. The mode lands on the persona record but not the instance record — which
    is what this PR fixes.

So this PR closes (2) and leaves (1) untouched. That's fine as scope, but the
combination needs a guard, because (1) produces exactly the input that makes the
new propagation write an invalid state:

propagate_persona_respond_to has no empty-allowlist guard.
validate_respond_to_allowlist(&[]) returns Ok(vec![]) rather than erroring,
so with a stored definition in allowlist mode and an empty list, the helper
writes respond_to = Allowlist + respond_to_allowlist = [] straight onto every
linked instance record. Both other write paths reject that state —
resolve_mint_behavioral_defaults fails the mint loudly ("respond-to mode
'allowlist' requires at least one pubkey in the allowlist"), and
apply_persona_behavior rejects it on the request. The new helper is the one
that doesn't.

And it's the repair path I asked for that makes it reachable: it fires on
result.respond_to.is_some() even when input.behavior is None, i.e. off the
stored definition, so apply_persona_behavior's rejection never runs. My
request, my problem to flag. Suggested fix is small — mirror the mint guard and
skip (not fail) when mode == Allowlist && allowlist.is_empty(), so a bad stored
definition can't be pushed onto instances and can't fail an unrelated persona
save either. Worth a test: definition allowlist + empty list, instance
anyoneupdated == 0, instance untouched.

Given (1), it may be worth pulling that fix in here too, or at least confirming
which PR owns it — as it stands, a user who upgrades to a build with this PR
still can't get an allowlist to the harness, because the people never reach
storage in the first place. #3030 looks like the same write path from the
built-in agent side.

@georgerous

Copy link
Copy Markdown

One more interaction, surfaced by a comment on #2501: the working way to set
this today is EditRespondToDialog from the channel members sidebar, which
writes respondTo + respondToAllowlist straight onto the instance record by
pubkey.

That's a deliberate per-instance choice made through the only path that
currently works — and this PR's repair path will overwrite it. It reconciles the
definition's gate onto linked instances on any persona save whenever
result.respond_to.is_some(), so a definition explicitly set to owner-only
plus an instance the user set to allowlist via that dialog means the next
unrelated persona edit silently reverts the instance to owner-only.

The no-coerce fix covers "definition gate unset". This is the other case:
definition gate explicitly set, instance deliberately different. Worth deciding
which wins, and saying so — right now definition-wins is implicit and it beats
the only control that works end to end. If definition-wins is intended, the
per-instance dialog probably needs to say so, otherwise it hands users a setting
that quietly expires.

@Chessing234

Copy link
Copy Markdown
Contributor Author

@tlongwell-block @wesbillman @wpfleger96 mind taking a look when you get a chance?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] <Desktop: agent respond_to / allowlist edits don't persist — buzz-acp always starts with owner-only>

2 participants