Skip to content

B5: one create path — behavioral quad activation, backfill, menu collapse, legacy dialog deletion#1667

Merged
wesbillman merged 14 commits into
mainfrom
feat/b5-one-create-path
Jul 9, 2026
Merged

B5: one create path — behavioral quad activation, backfill, menu collapse, legacy dialog deletion#1667
wesbillman merged 14 commits into
mainfrom
feat/b5-one-create-path

Conversation

@wesbillman

Copy link
Copy Markdown
Collaborator

B5 (Bundle 2): one create path — behavioral quad activation, backfill, menu collapse, legacy dialog deletion

What this PR does

Completes Phase 2 Bundle 2 of the unified agent model: every agent is created through ONE flow (the definition dialog), the NIP-AP behavioral quad (respond_to / allowlist / mcp_toolsets / parallelism) is fully live end-to-end, and the legacy standalone-instance CreateAgentDialog is deleted.

13 commits, each independently reviewed in-channel before landing:

  1. 57927b5ef — activate NIP-AP behavioral defaults: definitions carry the quad, applied at instance mint (resolve_mint_behavioral_defaults; input wins, else definition quad, else defaults; loud failure on invalid definition values).
  2. f7472a212 — apply inbound quad edits on the persona-match branch.
  3. 0e61f2526 — backfill standalone agents into definitions: manufactures a definition per pre-existing standalone agent (slug = agent pubkey), idempotent, .bak backup, hash-quiet (snapshots record's own values so spawn_config_hash and the drift badge stay quiet), system_prompt present-even-if-empty (old readers hard-fail on absent prompt).
  4. cc1547761 — share the spawn-effective prompt filter, exempt team packs.
  5. 4dfac6caa + 685cfe22a — sync-flush ordering: tombstones publish before replacements, and replacements defer behind a failed tombstone (race found during review).
  6. 1dae8a9a3 — file-size split (persona_events tests + pending-retention helpers).
  7. 75cd8d771 — menu collapse: "Custom agent" menu entry removed; the app-wide create-agent shortcut routes to the unified definition flow with the start-after-create toggle.
  8. 60555c41b — provider/mesh re-home: "Where to run" (local / discovered provider / relay mesh) lives in the unified create dialog; mesh preflight runs BEFORE the mint (fused in mintDefinitionWithPreflight, reordering structurally impossible); stale-intent guards on both doors.
  9. 18c8f2779 — split persona request types into types/requests.rs (pure move).
  10. 6daa7e8d0 — definition-quad dialog fields: who-can-talk (respond-to + allowlist), toolsets, parallelism editable on agent definitions; apply_persona_behavior fuses validation unskippably into create_persona AND update_persona; crash-loop guard (empty allowlist blocks submit) in create and edit; hash-quiet dirty-detection (untouched quad on edit submits no behavior group, so renames never rewrite published quad bytes).
  11. 2abbf4aac — delete CreateAgentDialog + Sections (−1,290 lines), collapse the instance create intent, migrate the e2e suite with mock-bridge mint-parity, fix the edit full-clear resurrect.

Disclosures (argued decisions, in commit bodies)

  • One-time republish wave: activating the quad + backfill triggers a second fat→slim republish wave for definition-linked records (same shape as feat(relay,desktop): canonicalize agent definitions on kind:30175 (Phase 2) #1655's); idempotence covered by test rows.
  • Ordering window accepted: a slim 30177 can land before its 30175 on an old device; self-heals when the definition lands — ordering machinery failed the minimal-fix bar.
  • Relay NIP-09 looseness: relay accepts deletes loosely; tombstone-before-replacement ordering in the sync flush is the client-side mitigation.
  • Create-time capability regressions: relayUrl / custom commands / turnTimeout are no longer settable at create (they remain editable on the instance after mint). Env-vars semantic: definition env layered under instance overrides — creates no longer seed pseudo-overrides.
  • Start-toggle-off = no instance: definition-only create mints nothing; "where to run" state is discarded (stale-intent guard, tested both doors).
  • Anthropic-without-model now blocks submit (legacy allowed save + post-spawn nudge); the unified dialog's pre-existing gate is stricter. Readiness shot 02 asserts the stricter gate.
  • Mesh honest copy: the mesh preset never overwrites definition fields — only the minted instance carries mesh commands/env; rendered as user-facing copy under the mesh section.
  • Old-device republish ping-pong + Some("") badge flip: one-time, self-terminating; discussed in-channel.

Testing

  • Rust: 1115/1115 (cargo test --lib, post-rebase onto e0f76b0), clippy clean (2 pre-existing warnings), both fmt passes.
  • Frontend: 2245/2245 unit, tsc, biome, all three check scripts.
  • E2E: full suite verified locally against an isolated relay before push — 536/540, the 3 failures bisect-proven pre-existing on main or flaky-cleared (migrated specs keep their END assertions — the smoke parallelism assertion can only pass through the real definition-quad mint fallback, mirrored honestly in the mock bridge).
  • Every commit reviewed by Pinky against pre-agreed pins (PLANS/B5_REVIEW_GATES.md), including three review-found bugs fixed in-flight (tombstone race, edit full-clear resurrect, migrated-spec persona-card gap).
  1. 18961f24d — sweep dialog-deletion orphans (AgentProviderField/AgentModelField, zero importers) + fix the mesh restart spec for the persona-card grouping the definition flow produces (reveal-dialog dismissal reordered before card assertions; END assertions untouched).

wesbillman and others added 13 commits July 8, 2026 21:13
… step 1)

Carry respond_to/respond_to_allowlist/mcp_toolsets/parallelism on
PersonaRecord in wire shape (kebab-case string), wire them through
persona_from_event / persona_event_content / to_persona_view /
into_agent_record, and copy them onto instances at mint time via
resolve_mint_behavioral_defaults (explicit input wins over definition
default; the only parse point for definition behavioral strings, failing
loudly on unknown modes, empty allowlists, and out-of-range parallelism).

Replaces the staging lock behavioral_defaults_are_staged_not_applied with
behavioral_defaults_survive_record_round_trip, plus hash rows:
quad-absent definitions serialize byte-identically to the reserved era
(persona_content_hash stable, no drift badge), and spawn re-snapshot
never clobbers an instance's own quad with a definition's absent quad.

Co-authored-by: Brain <21994759fc7a6fa6b965551d35cfd7897d262f2495467f2d78694ddcfa6a5c7e@sprout-oss.stage.blox.sqprod.co>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
apply_inbound_persona's match arm copied every projected field except the
behavioral quad, so a remote quad edit never landed on a device that
already held the definition — its next reconcile would republish the
stale quad over the edit, permanently. Copy all four fields like the
other projected fields; quad-absent inbound clears them, matching
prompt/model semantics. Caught by Pinky's step-1 review.

Co-authored-by: Brain <21994759fc7a6fa6b965551d35cfd7897d262f2495467f2d78694ddcfa6a5c7e@sprout-oss.stage.blox.sqprod.co>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
Boot-time migration (migration/backfill.rs, after the Phase-1A fold):
every keyed record with persona_id None gets a key-less definition
manufactured from its own settings — slug = agent pubkey (64-hex passes
the NIP-AP slug grammar, collision-free), prompt present-even-if-empty
(the old-reader heal source, pinned by a load-bearing test), env copied
so later instances inherit a working config, instance quad copied up as
the definition's behavioral defaults. The record links via persona_id +
persona_source_version = the manufactured definition's content hash, so
the drift badge starts clean.

Safety rails per the B5 review gates: idempotent (linked records skip;
second run is a no-op), .bak create-if-absent (a re-run never clobbers
the pristine pre-migration backup), fail-loudly-per-record on slug
collision.

Hash row 2: spawn treats an empty prompt as no prompt — Some("") and
None both spawn with BUZZ_ACP_SYSTEM_PROMPT absent, so the env write and
spawn_config_hash now filter empty-to-absent identically. Without this,
the re-snapshot's Some("") from a manufactured definition would flip the
restart badge on every prompt-less standalone agent at upgrade.

Co-authored-by: Brain <21994759fc7a6fa6b965551d35cfd7897d262f2495467f2d78694ddcfa6a5c7e@sprout-oss.stage.blox.sqprod.co>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
Extract effective_spawn_prompt as the single source of truth for the
Some("")/None collapse: the spawn env write and spawn_config_hash both
call it, so the badge cannot disagree with what a spawn delivers.

Team-pack records are exempt from the collapse (Pinky's wrinkle 1):
buzz-acp inherits the pack persona prompt when BUZZ_ACP_SYSTEM_PROMPT
is absent but treats set-but-empty as deliberate suppression, so for
those records the two states are different spawns and must stay
distinct in both the env write and the hash.

Co-authored-by: Brain <21994759fc7a6fa6b965551d35cfd7897d262f2495467f2d78694ddcfa6a5c7e@sprout-oss.stage.blox.sqprod.co>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
get_pending_sync now orders kind:5 rows first (oldest-first within each
group). Without this, a definition deleted in one session and
resurrected by the B5 backfill on the next boot could publish its fresh
30175 before the pending tombstone — and the relay a-tag deletion
soft-deletes every live row at the coordinate with no timestamp check,
wiping the replacement and leaving devices divergent until the next
content edit. Traced with Pinky during step-2 review.

Co-authored-by: Brain <21994759fc7a6fa6b965551d35cfd7897d262f2495467f2d78694ddcfa6a5c7e@sprout-oss.stage.blox.sqprod.co>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
…c flush

The pending-sync ORDER BY puts kind:5 tombstones before the replacements
that supersede them, but the flush is best-effort per row: a tombstone the
relay rejects mid-sweep would be leapfrogged by its own replacement, and
the tombstone landing on a later sweep would wipe it (the relay's a-tag
deletion ignores created_at). Track failed tombstones within the sweep and
defer any non-kind:5 row whose coordinate a failed tombstone covers; next
sweep the ORDER BY restores tombstone-first ordering.

The starvation direction is safe: a relay that persistently rejects the
tombstone defers its replacement indefinitely, which fails toward
stay-deleted (the user's intent), never toward wiping a live replacement.

The deferral predicate is a pure fn, kind- and pubkey-qualified via
tombstone_retention_d_tag so a coinciding slug under a different kind or
another pubkey never spuriously defers, and kind:5 rows themselves are
never deferred. Covered by predicate unit rows plus a stub-relay flush
test asserting the deferred row stays pending_sync and is excluded from
the flushed count while unrelated rows publish.

Co-authored-by: Brain <21994759fc7a6fa6b965551d35cfd7897d262f2495467f2d78694ddcfa6a5c7e@sprout-oss.stage.blox.sqprod.co>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
…elpers into modules

The B5 test additions pushed persona_events.rs (1202) and
commands/personas/mod.rs (1001) past the 1000-line file-size guard. Fix
the cause instead of adding exceptions: move the persona_events test
module to persona_events/tests.rs (the existing spawn_hash/types
pattern) and extract the retention-enqueue helpers
(retain_persona_pending / tombstone_persona_pending) into
commands/personas/pending.rs. Pure moves, no behavior change; visibility
widened only as far as the existing callers (commands::teams) require.

Co-authored-by: Brain <21994759fc7a6fa6b965551d35cfd7897d262f2495467f2d78694ddcfa6a5c7e@sprout-oss.stage.blox.sqprod.co>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
…flow (B5)

Remove the "Custom agent" entry from the New-agent card menu — the
definition-family create dialog (with its start-after-create toggle) is
now the one create path. The app-wide create shortcut (AppShell command
palette / sidebar, via openCreateAgentEvent) routes to the definition
flow instead of the standalone-instance dialog, and the menu's import
entry is renamed "Import agent file" per the B5 copy pin.

The instance branch of AgentDialog and CreateAgentDialog itself remain
until the provider/mesh re-home lands (next commits); e2e specs that
drive the removed menu entry migrate with the deletion.

Co-authored-by: Brain <21994759fc7a6fa6b965551d35cfd7897d262f2495467f2d78694ddcfa6a5c7e@sprout-oss.stage.blox.sqprod.co>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
…-create flow (B5)

The definition-create dialog gains a "Where to run" section (local default /
discovered backend provider / relay mesh), replacing the runtime surface that
died with the standalone-instance dialog. The section is rendered only while
the start-after-create toggle is ON: "where to run" is instance state, and
with the toggle off no instance exists.

- buildInstanceInputForDefinition gains an optional BackendIntent arg. Absent
  intent produces byte-identical output to before (deep-equality fixture row);
  the 3 existing call sites pass nothing and are untouched. The provider
  branch mirrors the legacy provider-mode create (no local commands, no
  model/provider, startOnAppLaunch forced false, spawnAfterCreate true); the
  mesh branch carries the preset patch as instance-override state
  (harnessOverride true, relayMesh modelRef, startOnAppLaunch forced false).
- whereToRunIntent.ts owns the draft-to-intent resolution. resolveBackendIntent
  discards any selection when the start toggle is off, so a stale provider/mesh
  pick can never silently ride a definition-only create. canSubmitWhereToRun
  carries the legacy submit gates: provider blocks until probe + required
  config; mesh blocks until a concrete serve target.
- The mesh preflight moved into mintDefinitionWithPreflight, which runs
  meshPrepareRelayMeshClient BEFORE the definition mint in
  usePersonaActions.handleSubmit — a dead mesh target aborts the whole create
  and never orphans a definition. Preflight and mint live in one function so
  callers cannot reorder them; a lib test row asserts a failed preflight
  never mints.
- Honest-copy note: the legacy mesh override warning compared the preset to
  the dialog's own field state. The definition path never overwrites the
  definition — only the minted instance carries mesh values — so
  RelayMeshAgentSection gets an empty `current` and the warning stays silent
  by construction (disclosed for the PR body).

Co-authored-by: Brain <21994759fc7a6fa6b965551d35cfd7897d262f2495467f2d78694ddcfa6a5c7e@sprout-oss.stage.blox.sqprod.co>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
types.rs sits at 996/1000 under the file-size guard and the quad fields
about to land on CreatePersonaRequest/UpdatePersonaRequest would breach
it. Pure move; the pub use re-export keeps every existing path working.

Co-authored-by: Brain <21994759fc7a6fa6b965551d35cfd7897d262f2495467f2d78694ddcfa6a5c7e@sprout-oss.stage.blox.sqprod.co>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
Definitions gain editable NIP-AP behavioral fields — respond_to +
allowlist, mcp_toolsets, parallelism — in the definition dialog's
Advanced section, replacing the surface that dies with the legacy
create dialog.

Relationship to instances, for the record: the definition quad is
resolved at MINT time only (resolve_mint_behavioral_defaults, explicit
input wins over definition default); instances snapshot it into their
own record fields and spawn re-snapshot never touches the quad. Editing
a definition's allowlist therefore affects future mints only, never
already-minted instances.

- The quad travels as one grouped optional `behavior` request field with
  env_vars absent-vs-present semantics: absent = don't touch the stored
  quad, present = validate and replace all four as a unit. Grouped
  because update_persona has legacy callers (team import, profile panel)
  that send no behavioral fields — flat replace semantics would have
  silently wiped a stored quad on every team-import edit.
- Validation is fused inside apply_persona_behavior, the single write
  path both create_persona and update_persona route through: allowlist
  entries normalized via validate_respond_to_allowlist (the same
  chokepoint instances use), allowlist mode with an empty list rejected
  (the spawn-time crash-loop build_respond_to_env errors on),
  parallelism range-checked 1..=32, blank toolsets normalized to None.
  Non-allowlist modes store an empty list — mode and list travel as a
  unit both directions, matching spawn_hash and build_respond_to_env
  which only consume the list in allowlist mode.
- The dialog blocks submit on allowlist-with-empty-list in create AND
  edit mode (re-homed respondToValid guard). Parse fidelity matches the
  legacy dialog: parallelism submits only when parseInt > 0, toolsets
  trim to unset.
- Unrelated edits stay hash-quiet: an untouched quad submits no behavior
  group, so a rename cannot rewrite the published definition's quad
  bytes or flip its content hash. Duplicates inherit the source quad.
- Rides along: the mesh section's honest copy is now rendered user-facing
  text ("The started instance runs on the mesh model; the agent profile
  keeps its own settings."), closing the P5 review gap.

Disclosure: pack remove+reinstall mints fresh persona records, so a quad
set on a pack-backed persona is lost across reinstall (launch sync itself
never touches the quad — it patches six named fields only).

Co-authored-by: Brain <21994759fc7a6fa6b965551d35cfd7897d262f2495467f2d78694ddcfa6a5c7e@sprout-oss.stage.blox.sqprod.co>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
…(B5)

Remove CreateAgentDialog.tsx (877 lines) and CreateAgentDialogSections.tsx
(413 lines): the unified definition-create flow now covers everything the
legacy standalone-instance dialog did (provider/mesh backends, parallelism,
system prompt, credentials), so the second create path dies. The AgentDialog
"instance" create mode, the onInstanceCreated prop, and the "instance"
member of AgentCreateIntent go with it; AgentsView's create state collapses
to a boolean. AgentsView's SecretRevealDialog for agents.createdAgent stays —
the card start flow (handleStartPersona) still feeds it.

E2E migration (the specs keep asserting the same behavior, only the click
path changes):
- smoke "create agent supports parallelism and system prompt overrides"
  drives the definition dialog; the END assertions are untouched — the
  harness log must still show parallelism=3 and the prompt override, which
  now can only pass through the definition-quad mint fallback.
- The mock bridge gains mint-parity with the real backend (deef5c043):
  create/update_persona accept the `behavior` group (replace-all-four,
  non-allowlist modes store an empty list), and create_managed_agent
  resolves respond_to/allowlist/parallelism as input-wins, else the linked
  definition's quad, else defaults — the harness log line reads the
  RESOLVED parallelism, so the smoke assertion exercises the fallback.
- agent-readiness screenshots + 5 mesh-compute flows move to the definition
  dialog selectors (menu-based PersonaDropdownField helpers); the mesh
  toggle/model testids are unchanged because RelayMeshAgentSection
  re-mounts inside WhereToRunSection. mesh assertions (prepare-before-create
  ordering slice, mesh env fingerprint, 24621 canonicalization) unchanged.
- agents.spec's "Custom agent" is catalog detail-pane label text, not a
  menu click — it survives verbatim.

Behavioral delta, disclosed: the legacy dialog allowed saving with an empty
model under anthropic (nudge-after-spawn); the unified dialog's pre-existing
gate blocks submit until an explicit model is set. Readiness shot 02 now
asserts the stricter gate.

Also fixes the edit-mode full-clear resurrect found in review: clearing
every quad field on an edit now submits an explicit empty behavior group
(replace-with-empty) instead of silently submitting nothing, while a no-op
edit of an already-quad-less definition still submits nothing so unrelated
renames stay hash-quiet.

Co-authored-by: Brain <21994759fc7a6fa6b965551d35cfd7897d262f2495467f2d78694ddcfa6a5c7e@sprout-oss.stage.blox.sqprod.co>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
…spec (B5)

Two follow-ups to the CreateAgentDialog deletion (6972fbcd8), both found
in review:

Dead-code sweep: AgentProviderField and AgentModelField in
personaProviderModelFields.tsx had zero importers once the legacy dialog
died — AgentInstanceEditDialog uses PersonaDropdownField, not these — and
the file header (rewritten in the deletion commit) wrongly claimed
otherwise. The file shrinks to its sole live export, RequiredFieldLabel,
with an honest header; two readiness-spec comments naming the deleted
components are updated.

mesh-compute "saved relay-mesh agents restart" fix: the migrated create
flow is definition_start, which mints the instance WITH a personaId — so
buildUnifiedGroups files it under the persona group card
(persona-agent-row-<personaId>), and StandaloneAgentCard's
managed-agent-<pubkey> testid never renders. The spec's row lookup was
legacy-shaped for the persona-less instances the old dialog created. The
card assertion now locates the persona card via the runtime control keyed
on the instance pubkey and verifies the agent's NAME reaches that same
card; triggerManagedAgentPrimaryAction opens the profile via the card's
aria-label. The reveal-dialog dismissal moves BEFORE the card assertions
because the modal holds the rest of the page aria-hidden. END assertions
(stop/start command order, preflight rejection toast, backend error
string) are untouched.

The deletion commit's scoped e2e run was vacuous for this spec:
mesh-compute and agents live in the integration project, and
`--project=smoke <file>` silently matches zero tests. Verified here with
per-test ✓ output: integration mesh-compute 7/7 + agents 10/10, smoke
readiness 7/7.

Co-authored-by: Brain <21994759fc7a6fa6b965551d35cfd7897d262f2495467f2d78694ddcfa6a5c7e@sprout-oss.stage.blox.sqprod.co>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
@wesbillman wesbillman enabled auto-merge (squash) July 9, 2026 14:14

@wpfleger96 wpfleger96 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Consolidated team review — Paul (orchestrator), Duncan (implementer-lens), Thufir (independent analyst), Alia (copy + mechanical verification).

All four reviewers independently examined the PR at head 18961f24d in detached worktrees. Duncan and Alia: Approve. Thufir: Request changes (one production correctness gap — provider payload drops mcp_toolsets). Paul's initial assessment missed the provider payload issue; Thufir caught it, verified at source.

Finding not attached inline (file not in diff):

IMPORTANT — Edit-path empty-allowlist guard untested (convergent: Paul, Duncan, Thufir) — personaRuntimeModel.ts:258: computeEditAgentFormValidity rejects respondTo === "allowlist" with an empty allowlist, but the edit-path tests (agentInstanceEditPinning.test.mjs:165) use respondTo: "mentions", so the guard is never exercised. The create-path guard (personaBehaviorDraftValid) has a direct test. Fix: add one false case with respondTo: "allowlist" + respondToAllowlistLength: 0 → false, and one true case with length > 0 → true.

See inline comments for all other findings, each tagged with provenance and severity.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 IMPORTANT — Provider deploy payload drops mcp_toolsets (Thufir-sourced, Paul source-verified)

build_deploy_payload (lines 367–399) serializes parallelism, respond_to, and respond_to_allowlist into the provider agent_json, but omits record.mcp_toolsets. The local spawn path correctly sets BUZZ_TOOLSETS from the record at runtime.rs:1768–1772. All three provider paths (create at line 961, start at line 1070, redeploy) funnel through build_deploy_payload, so provider-backed agents silently lose inherited or explicit toolset settings.

Fix: Add "mcp_toolsets": &record.mcp_toolsets to the serde_json::json! block in build_deploy_payload, and add a regression test for provider-create payload completeness.


should-fix — Parallelism input validation at wrong layer + misleading doc (Paul-raised; Duncan and Thufir independently corrected the framing)

resolve_mint_behavioral_defaults at types.rs:932 passes explicit input parallelism through without range validation, while the definition branch validates loudly. The doc comment on MintBehavioralDefaults.parallelism ("Validated (1..=32) when present") is false for the input branch.

However, call-site validation at agents.rs:525–529 guards the full create path before this function is reached — so the behavior is not broken end-to-end. This is a wrong-layer defense + inaccurate doc comment, not a live behavior defect.

Fix: Either add range validation inside the function's input branch, or update the doc comment to state the caller is responsible for validating input parallelism.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 IMPORTANT — Mock-bridge mint-parity missing mcp_toolsets (convergent: Paul, Duncan, Thufir)

handleCreateManagedAgent (~line 6587) resolves respondTo, respondToAllowlist, and parallelism from the linked definition, but has no mcp_toolsets resolution at all. An e2e test asserting toolset inheritance would pass regardless of correctness.

Fix: Add mcpToolsets to the mock input, add mcp_toolsets to RawManagedAgent/clone output (~lines 440–475, 1027–1063), and resolve as input ?? linkedPersona?.mcp_toolsets ?? null.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 MINOR — User-facing copy uses internal jargon (Alia)

Line 72–73: "Create an agent profile and start its managed agent instance" → "Create an agent and start it immediately."
Line 50: "without starting an instance" → "without starting it."

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 MINOR — Grammatically incomplete toggle label (Alia)

Line 135: "Start agent after create" → "Start agent after creating" or "Start agent after creation."

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 MINOR — Inconsistent branding + missing article + unused prop (Alia + Paul)

  1. Inconsistent branding: "Run on relay mesh" (line 122) vs "Relay compute" (line 67) vs "Relay mesh" (line 170). Pick one canonical term.
  2. Line 90: Missing article — "Selected relay mesh target…" → "The selected relay mesh target…"
  3. Line 45: modelId: string prop declared but never destructured or read in the component body. (Alia CONFIRMED)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 MINOR — Parallelism error string inconsistency (Alia)

Line 940: Error string prefixes with "definition" ("definition parallelism {count} is out of range…"). The corresponding string in types/requests.rs:55 does not. Both can surface to users. Align one way or the other.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 MINOR — Button label is a state, not an action (Alia)

Line 27: deselectAction: "Selected" — the paired aria label (line 52) correctly says "Deselect". The visible button text should match the action: "Deselect" or "Remove".

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 MINOR — Two nits (Paul + Alia)

  1. Line 11: PERSONA_LABEL_OPTIONAL_CLASS redefined byte-identically — already exported from personaDialogPickers.tsx:8, and this file imports two other constants from that module. Import instead of redeclaring. (Alia CONFIRMED)
  2. Line 90: Parallelism hint "Concurrent turns each started instance handles (1-32)" → "How many conversations each running instance handles at once (1–32)." (Alia)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 MINOR — Dead export (Paul, Alia CONFIRMED)

Line 352: openCreate() defined and returned at line 449, but has zero call sites in desktop/src. B3-migration leftover — delete or add TODO(B3).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 MINOR — No partial-input quad test (convergent: Paul, Duncan, Thufir)

Lines 555–635: mint_explicit_input_wins_over_definition passes all four fields at once. No test covers field-by-field independence (e.g., explicit mcp_toolsets + inherited respond_to). Add one mixed-input test to pin the field-by-field contract.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 MINOR — Slug-collision skip is silent and permanent (convergent: all reviewers)

Lines 84–89: An agent whose pubkey collides with an existing definition's slug is skipped with eprintln! and stays persona_id: None forever. Worth a surfaced warning or a comment documenting the recovery path (delete the colliding definition).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 IMPORTANT (pre-existing, elevated by this PR) — Two security boundary gaps (convergent: Paul, Duncan, Thufir)

  1. No sig verification — Line 386: nostr::Event::from_json skips sig checks. This PR routes the behavioral quad through this path, so a forged event could now change who may command an agent.
  2. Tombstone owner unchecked — Line 491: parse_deletion_coordinate discards _owner; a forged kind:5 could delete another owner's record.

Fix (follow-up PR): Add event.verify() after parse + require tombstone coordinate owner to match event.pubkey.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 MINOR (pre-existing) (Paul, Duncan, Thufir, Alia confirmed)

  1. Lines 234–280: Flush loop opens the retention DB ~3× per pending row. B5's republish wave makes this O(N agents) on first post-upgrade boot. Hold one connection across the sweep.
  2. Line 296: persona_content_hash uses unwrap_or_default() on serde_json::to_vec — silently hashes empty bytes on failure. expect() would fail visibly.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 MINOR (pre-existing) (Paul, Alia CONFIRMED)

Lines 1367–1375: spawn_config_hash recomputed per running agent on every list_managed_agents poll (~30s). This PR added quad fields to the hash input. Cache-on-stamp candidate someday.

wesbillman added a commit that referenced this pull request Jul 9, 2026
…ts, copy sweep + restart-after-edit offer

Review findings (Will's consolidated 4-reviewer pass), all in-scope items:

IMPORTANT fixes:
- Provider deploy payload now carries mcp_toolsets. build_deploy_payload
  splits into agents_deploy.rs (file-size guard) with a pure
  deploy_payload_json serialization half and a payload-completeness
  regression test — a behavioral field missing from the payload now fails
  a test instead of silently stripping provider-backed agents.
- Mock-bridge mint-parity extended with mcp_toolsets (input, else linked
  definition, else null) through RawManagedAgent and both clone paths.
- The edit-path empty-allowlist crash-loop guard (respondToValid) gets its
  two-case test: allowlist+empty blocks Save, allowlist+1 allows.

Should-fix:
- resolve_mint_behavioral_defaults now validates INPUT parallelism in-range
  too, making the "validated when present" doc contract unskippable rather
  than delegated to call sites; error strings distinguish input vs
  definition without the bare "definition" prefix.
- Two convergent test asks: per-field quad independence at mint (explicit
  toolsets + inherited respond_to/parallelism) and input-range rejection.

Minors:
- Copy: "Create an agent and start it immediately", "without starting it",
  "Start agent after creation", canonical "relay mesh" casing + articles,
  catalog Deselect action label, parallelism hint in plain words.
- Dead code: unused modelId prop on RelayMeshAgentSection, unused
  openCreate export, duplicated PERSONA_LABEL_OPTIONAL_CLASS now imported.
- Backfill slug-collision skip documents the retry semantics + recovery
  path in both the comment and the printed warning.

Deferred with the reviewers' own framing (logged in
PLANS/UNIFIED_AGENT_RESIDUE_BACKLOG.md): the two pre-existing security
gaps (inbound sig verification, tombstone owner check) and the
pre-existing perf notes, all marked follow-up PR in the review.

Plus the restart-after-edit offer (Wes, in-channel): saving an edit on a
stopped/failing instance now raises an action toast ("Start now") — the
auto-restart policy deliberately never fires for broken agents, so the
edit meant to fix one otherwise waits silently for a manual start. The
Chunk F policy gates are unchanged.

Co-authored-by: Brain <21994759fc7a6fa6b965551d35cfd7897d262f2495467f2d78694ddcfa6a5c7e@sprout-oss.stage.blox.sqprod.co>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
@wesbillman wesbillman requested a review from a team as a code owner July 9, 2026 15:21
@wesbillman

Copy link
Copy Markdown
Collaborator Author

All review findings addressed in acbe4d3a0:

IMPORTANT:

  • Provider payload mcp_toolsets — added; build_deploy_payload split into agents_deploy.rs with a pure deploy_payload_json half and a payload-completeness regression test (deploy_payload_carries_the_full_behavioral_quad), so a missing behavioral field now fails a test instead of silently stripping provider agents.
  • Mock-bridge mint-parity mcp_toolsets — resolved as input ?? linkedPersona?.mcp_toolsets ?? null, threaded through RawManagedAgent + both clone paths.
  • Edit-path allowlist guard test — two-case row added (editValidity_allowlistWithEmptyList_blocksSave): empty list blocks Save, non-empty allows.

should-fix: input-branch parallelism now validated inside resolve_mint_behavioral_defaults (the doc contract is now unskippable, not call-site-delegated), with distinct input-vs-definition error strings + tests (mint_rejects_out_of_range_input_parallelism, mint_resolves_each_quad_field_independently).

Minors: all copy fixes applied (create/start descriptions, "Start agent after creation", canonical "relay mesh" + articles, Deselect action label, plain-words parallelism hint); dead modelId prop, dead openCreate export, and the duplicated class constant removed; backfill slug-collision skip now documents retry semantics + recovery path in comment and warning text.

Deferred as the review itself framed them (tracked in the workstream backlog): the two pre-existing security gaps (inbound sig verification, tombstone owner check — follow-up PR) and the pre-existing perf notes.

Also riding: a small owner-requested UX addition — saving an edit on a stopped/failing instance now offers a "Start now" action toast, since the auto-restart policy deliberately never fires for broken agents. Policy gates unchanged.

Gates on the new head: Rust 1118/1118, frontend 2246/2246, tsc/biome/checks/fmt clean, edit-agent + readiness e2e ✓.

…ts, copy sweep + restart-after-edit offer

Review findings (Will's consolidated 4-reviewer pass), all in-scope items:

IMPORTANT fixes:
- Provider deploy payload now carries mcp_toolsets. build_deploy_payload
  splits into agents_deploy.rs (file-size guard) with a pure
  deploy_payload_json serialization half and a payload-completeness
  regression test — a behavioral field missing from the payload now fails
  a test instead of silently stripping provider-backed agents.
- Mock-bridge mint-parity extended with mcp_toolsets (input, else linked
  definition, else null) through RawManagedAgent and both clone paths.
- The edit-path empty-allowlist crash-loop guard (respondToValid) gets its
  two-case test: allowlist+empty blocks Save, allowlist+1 allows.

Should-fix:
- resolve_mint_behavioral_defaults now validates INPUT parallelism in-range
  too, making the "validated when present" doc contract unskippable rather
  than delegated to call sites; error strings distinguish input vs
  definition without the bare "definition" prefix.
- Two convergent test asks: per-field quad independence at mint (explicit
  toolsets + inherited respond_to/parallelism) and input-range rejection.

Minors:
- Copy: "Create an agent and start it immediately", "without starting it",
  "Start agent after creation", canonical "relay mesh" casing + articles,
  catalog Deselect action label, parallelism hint in plain words.
- Dead code: unused modelId prop on RelayMeshAgentSection, unused
  openCreate export, duplicated PERSONA_LABEL_OPTIONAL_CLASS now imported.
- Backfill slug-collision skip documents the retry semantics + recovery
  path in both the comment and the printed warning.

Deferred with the reviewers' own framing (logged in
PLANS/UNIFIED_AGENT_RESIDUE_BACKLOG.md): the two pre-existing security
gaps (inbound sig verification, tombstone owner check) and the
pre-existing perf notes, all marked follow-up PR in the review.

Plus the restart-after-edit offer (Wes, in-channel): saving an edit on a
stopped/failing instance now raises an action toast ("Start now") — the
auto-restart policy deliberately never fires for broken agents, so the
edit meant to fix one otherwise waits silently for a manual start. The
Chunk F policy gates are unchanged.

Co-authored-by: Brain <21994759fc7a6fa6b965551d35cfd7897d262f2495467f2d78694ddcfa6a5c7e@sprout-oss.stage.blox.sqprod.co>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
@wesbillman wesbillman force-pushed the feat/b5-one-create-path branch from acbe4d3 to 8c1a249 Compare July 9, 2026 15:31
@wesbillman wesbillman disabled auto-merge July 9, 2026 16:00
@wesbillman wesbillman merged commit 3a7e400 into main Jul 9, 2026
46 of 47 checks passed
@wesbillman wesbillman deleted the feat/b5-one-create-path branch July 9, 2026 16:10
wpfleger96 added a commit that referenced this pull request Jul 9, 2026
#1667 (B5) added behavioral quad fields to ManagedAgentRecord and
PersonaRecord (definition_respond_to/allowlist/mcp_toolsets/parallelism)
and updated spawn_config_hash to take a 4th &GlobalAgentConfig arg.

Update exhaustive struct initializers in test files to include the new
fields (all None/vec![] defaults), and add the missing &Default::default()
arg to spawn_config_hash calls in backfill_tests.rs and spawn_hash/tests.rs.

Also:
- Run cargo fmt (two trivial whitespace corrections)
- Fix smoke.spec.ts biome formatting (object literal expansion)
- Bump file-size overrides for readiness.rs (+2), AgentInstanceEditDialog.tsx
  (+23), and add new entry for agent_config.rs (1002 lines)

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
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.

2 participants