Repository navigation
feat(agents): sync teams through their home community - #1281
Conversation
0f11be2 to
12ecf5e
Compare
ff29773 to
8d2cc59
Compare
0e7de81 to
94c9957
Compare
390fa1e to
2296646
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord's automated source review via Wes's account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) reviews.
Reviewed head 2296646 against base 20d8bb7. Assessment: 8/10; three P2 defects and one smaller P3 error-path defect are detailed inline. The new behavior remains dormant while CONFIG_SYNC_ENABLED is false. These are non-blocking review comments; humans decide merge readiness.
The saved-operation model, exact-byte retries, storage-before-send ordering and owned-member removal checks have clear roles. The remaining problems are transitions between the local row, saved operation and open editor. The overlapping migration reports are consolidated into one comment.
Validation: source only; no PR code, tests, builds or app were executed. The CI snapshot attached to this head included a failed Vitest flake check: iteration 3/10 failed at service.test.ts:6960 because configBase was undefined in the same-id relay/local-team test (391 passed, one failed); later iterations passed. Root cause is not established. Other checks were still running at the snapshot. Live relay/native behavior, cross-device recovery, rendered UI/focus, performance and human acceptance remain unverified. The referenced §6 design plan was not available in the reviewed sources; the PR's stated behavior and repository design rules informed this review. Public description and commit text were inspected; no attachment images or disclosure issue were found there.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord's automated source review via Wes's account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) reviews.
Reviewed head 2f57f72a29407654714099962dd9512f12cac2e3 against base 20d8bb7, focusing on the seven-file follow-up since 2296646 and its surrounding call paths. Assessment: 8/10; one remaining P2 is detailed inline. This is non-blocking feedback, not an approval or merge recommendation. CONFIG_SYNC_ENABLED remains false, so these relay-sync paths remain disabled by default.
The prior revised-Create membership removal and archived-import dismissal findings are addressed in source. The reopened editor now keeps its draft mounted when the operation finishes. The local move guard addresses the connected case, but the offline/reconnect case in the inline comment still needs repair. Prior review: #1281 (review).
Optional product follow-up: a reopened form whose operation finishes before its first Save can still report a conflict even without a remote edit: createTeam consults finishedAt through forms before registering the supplied operation ID. The existing service.test.ts:7285–7340 explicitly permits this stale-form refusal, so I am not treating it as a new correctness requirement. If seamless Save is intended, consult the completed operation's head while retaining genuine conflict protection, and exercise the real service path; the new component test mocks saveTeam. The completed form also retains its “New team” / “Create team” labels; updating those labels while keeping the key and unsaved draft stable would make this transition clearer.
Validation: immutable source and added tests inspected, no PR code, tests, builds, or apps executed. The public description and commit messages were inspected; no attached images or actionable disclosure finding. One hosted-check snapshot had application/browser/flake checks still running and Windows validation skipped. Native/live relay behavior, reconnect timing, rendered keyboard and recovery focus, screen-reader behavior, themes/viewports, performance, and human acceptance remain unverified. This follow-up does not re-audit the unchanged stack.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord's automated source review via Wes's account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/concurrency), and Groot (product/accessibility) review.
Follow-up review of 2f57f72a29407654714099962dd9512f12cac2e3 → 15908f134be280fb1dddc7ac5bca35f01a5a10d4, against base 20d8bb7. The six changed files and their affected callers were reviewed; the unchanged stack was not exhaustively reviewed again.
Two P2 defects introduced by the fixes are detailed inline. The prior unavailable-community move-guard defect is addressed: persisted TeamOps now protect the local row even without a live community config. The first Save after a reopened Create finishes also now edits the existing team. Source assessment: 8/10 pending the two fixes below; this is a non-blocking COMMENT, not approval or merge authorization.
Optional recovery wording: service.ts:2455–2459 directs an offline user to Unfinished changes, but that list is built from live communityConfigs (2406–2447). When the config is unavailable, explain that reconnecting is required. In team-ops.ts:805–806, “already joined” could be “joined or may have joined” for maybe-sent adds. These are optional copy improvements, separate from the correctness findings.
Public PR description and nine commit messages inspected; no attachments or actionable disclosure findings. Source only: no tests, builds, app launches, or PR code execution. One hosted-check snapshot showed application/browser and Vitest flake checks still running, Windows skipped, and the listed completed checks successful. Native/live relay behavior, rendered keyboard/focus recovery, screen readers, themes/viewports, runtime timings and human acceptance remain unverified. No new browser cases in this follow-up; service/runner/component regression coverage was inspected as source, not executed.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord's automated source review via Wes's account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/concurrency), and Groot (product/accessibility) review.
Follow-up at head 2577e37a6aeaf2f81b557c0dcd1281e19a9c10f6 against base 20d8bb7, covering the seven-file change since 15908f134be280fb1dddc7ac5bca35f01a5a10d4 and affected callers. One remaining P2 correctness defect is detailed inline. Assessment: 8/10 pending that fix. This is non-blocking feedback, not approval or merge authorization. CONFIG_SYNC_ENABLED remains false.
The reopened form now uses its latest completed edit's head, and its labels change to Edit team / Save while retaining the draft. Offline move refusals now direct the user to reconnect. The joined-member guard handles the normal successful-discovery retry, but still accepts a membership revision after a failed roster refresh.
Public description and ten commit messages inspected; no attached images or actionable disclosure finding. Source only: no PR code, tests, builds, or apps executed. One hosted-check snapshot showed application/browser/Vitest flake checks still running, with Windows and browser flake validation skipped. Native/live relay behavior, rendered keyboard and recovery focus, screen readers, themes/viewports, timings and human acceptance remain unverified. The unchanged stack was not exhaustively reviewed again.
20d8bb7 to
a774f01
Compare
2577e37 to
a61df2e
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord's automated source review via Wes's account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/concurrency), and Groot (product/accessibility) review.
Reviewed head a61df2e against base a774f01. The rebase preserves all ten feature patches; the affected source integration uses the new Solo owners. Three newly evidenced P2 defects are detailed inline. These are existing feature paths reopened on concrete material evidence, not claims that the rebase introduced them. Assessment: 8/10 pending the fixes; non-blocking comments, not approval or merge authorization. CONFIG_SYNC_ENABLED remains false.
The previously reported stale-roster Create-revision defect is also unchanged: #1281 (comment). That finding is referenced here rather than duplicated. Unknown member/lifecycle command outcomes intentionally remain fenced; a later roster showing absence alone does not establish that an earlier command cannot still land.
Validation: source and relevant test cases inspected; no PR code, tests, builds or apps executed. One hosted-check snapshot showed CI required and the other completed checks successful, with Windows native and browser flake validation skipped. Public description and ten commit messages inspected; no attached images or actionable disclosure finding. Native/live relay and cross-device behavior, rendered keyboard/recovery focus, screen readers, themes/viewports, performance and human acceptance remain unverified. This follow-up does not claim a fresh exhaustive audit of the unchanged stack.
a61df2e to
df10982
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord's automated source review via Wes's account, integrating completed independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) reviews.
Head: df10982331eccd569d3f0735c4f77d5c64e9b9c9
Base: bf7d7fd
No new production defects found in this follow-up. The fresh roster read and per-team serialization address the stale-roster Create revision finding. Save from a card now revises its unfinished Create, and the later run promise is awaited outside the exclusive queue, avoiding self-deadlock.
Overall assessment remains 8/10 because three previously reported P2s remain unchanged; please address them in their existing threads:
- Ownership-read failure can silently skip a requested removal.
- Moving a local team loses saved template references and agent-facing inventory.
- Resubmitting an unestablished Create/Import becomes an Edit that cannot create its first head.
The feature remains disabled by CONFIG_SYNC_ENABLED=false. One optional regression-test ordering improvement is inline.
Scope: endpoint comparison against a61df2e; range-diff confirms its ten feature patches are preserved, followed by two fixes. Reviewed changed implementation, callers, regression source, inherited Claude metadata adjustment, and public description/commit messages (no image attachments). Unchanged stack areas were not exhaustively re-audited.
Source only: no PR code, tests, builds or app execution. One hosted CI snapshot showed 30 successful checks and two skipped (Windows native validation and browser flake check). Live relay/cross-device behavior, native flows, runtime performance, rendered focus/accessibility and human acceptance remain unverified. This is a non-blocking COMMENT review, not approval or merge authorization.
df10982 to
7d2714d
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord's automated source review via Wes's account, integrating completed independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) reviews.
Two new findings are inline: P2, a retained unsent Delete can replay after a successful Recover; P3, a form opened without a head offers a retry that cannot succeed without reopening. Assessment: 8/10 pending the lifecycle-ordering repair. These are non-blocking comments; no approval or merge authorization. CONFIG_SYNC_ENABLED remains false.
The three previously reported P2s have source-level repairs: failed ownership reads keep removals pending (discussion_r4315487857), local-team moves preserve saved template references and agent-facing expansion (discussion_r4315487877), and a refused first Create/Import retains creation semantics on Resubmit (discussion_r4315487886).
Scope: the eight-file endpoint follow-up from df10982331eccd569d3f0735c4f77d5c64e9b9c9, affected callers and regression source, plus the public description and commit messages. No attached images or actionable disclosure finding. Unchanged stack areas were not exhaustively re-audited.
Source only: no PR code, tests, builds or apps executed; CI was not assessed in this cycle. Live relay/cross-device behavior, native flows, rendered focus/accessibility, runtime performance and human acceptance remain unverified. The held-revision absence repair relies on the relay rejecting obsolete prev/create revisions; backend enforcement was not independently audited or exercised.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord's automated source review via Wes's account, integrating completed independent Rocket (systems/lifecycle/storage), Mantis (correctness/security/concurrency), and Groot (product/accessibility) reviews.
No new functional defects found in the four-file follow-up from 7d2714d. Explicit Import now uses the same template-before-local-row retirement as migration; ownership uncertainty retains newly proved members so unreadable-team admission blocks their starts. The added restart and recovery regression source covers the intended contracts. One optional documentation cleanup is inline.
Two previously reported findings remain in unchanged code: P2 Delete replay after Recover and P3 missing-base form retry guidance. This review links those existing discussions without duplicating them. Assessment: the two repairs meet 9/10 at source level; overall the prior 8/10 assessment remains pending the lifecycle-ordering repair. CONFIG_SYNC_ENABLED remains false. These are non-blocking comments, not approval or merge authorization.
Scope: the follow-up, affected callers, prior findings above, public PR description and both follow-up commit messages. No attached images or actionable disclosure finding. Unchanged stack areas were not exhaustively re-audited.
Source only: no PR code, tests, builds or apps executed. One hosted CI snapshot showed 30 successful checks and two skips (Windows native validation and the unselected browser flake job). Live relay/cross-device behavior, native flows, rendered focus/accessibility, runtime performance and human acceptance remain unverified; CI does not close these gaps.
d480618 to
976e825
Compare
666f503 to
79581d7
Compare
aed144b to
e0dad3b
Compare
79581d7 to
f225e1d
Compare
85dd8a5 to
187e580
Compare
0d6cf63 to
e767c12
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account.
Two previously reported P2s remain at this head:
- Partial-Create roster:
team-ops.ts:167–207still reports success when a revised Create omits an already-added member. Refuse that revision and require reopening as an Edit; keep Import additive. - Synced-team storage migration:
state.ts:isAgentKeystill omitsSYNCED_TEAMS_KEY, losing the retained roster on fallback-to-native recovery. Include the key and cover that recovery path.
Sync now follows relay support; the startup/reconnect roster contract is reflected in the description. GitHub reports merge conflicts with the current base. Preserve its newer reconnect/retype/delete and Goose closed-session fixes when resolving them; that integration remains unreviewed.
Head 187e5802c66a5aae0d6896717691884c75cf556f; base e767c12ef85da8b6036e3bc2b30767771de6edfd. Source-only follow-up integrating Rocket, Mantis and Groot; no tests, app execution or CI assessment. Live relay/restart behavior and rendered focus remain unverified.
8d74771 to
2cfc0de
Compare
187e580 to
d789ec2
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account.
Two previously reported P2s remain unchanged:
- Partial-Create roster: retrying after deselecting an already-added member still reports success while retaining that member. Refuse that revised Create and require reopening as Edit; preserve additive Import.
- Synced-team storage migration: include
SYNCED_TEAMS_KEYinisAgentKeyand cover fallback-to-native recovery, which currently drops the retained roster until rediscovery.
No new defects found in the targeted rebase integration. Head d789ec2445014090b101099343d22bb4e95431d3; base 1b55594d6907a33aceecccc14b2ad8af5ea91c3d. Source-only follow-up integrating Rocket, Mantis and Groot; no tests, app execution or CI assessment. Live relay/restart behavior and rendered focus remain unverified.
d789ec2 to
516754a
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account.
No further changes requested: the two prior findings are addressed—the synced-team roster now migrates into agent storage, and a Create retry refuses to silently retain a removed member. This follow-up checks those fixes and their callers; the five unchanged feature patches were not re-reviewed in full.
Rocket, Mantis and Groot completed independent storage/lifecycle, correctness/concurrency and recovery/accessibility lanes. Head 516754a172ff1359a11e1fec3ed169a78ebd146b; base bb895c224f0f21be5288ac6a7c85b1fa1d928037. Source-only: tests, live relay/native restart flows and rendered UI behavior were not exercised; CI was not assessed.
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 The roster migration and partial-Create fixes hold. The existing unknown-support finding remains open; one additional non-blocking recovery-message issue is inline.
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 reviewed at 516754a. both fixes from the last round look right: a retried Create that drops an already-added member is refused now, and SYNCED_TEAMS_KEY moves with agent storage, with a recovery test. reusing #1226's config channel, sync and writer owners instead of a parallel sync path keeps this focused. I haven't run it against a live relay or across two devices; the second-device, pending-support and other-account cases below were reproduced with service-level tests.
three things I'd want fixed before merge (editing from a second device, Create going local while the support check is pending or failed, synced teams showing up for another account on the same machine), plus three small ones inline. also the "Stack: #1226 → this PR" line in the description is stale now that #1226 merged.
…nity A team created, or imported from old Buzz, where config sync is on lives in one home community, like an agent: its NIP-AP config channel holds the name and instructions, and its members are the owner's agents there. Members in any community start from the last confirmed copy. Changes run online, one per team, and report partial results; local teams are not migrated. Behind CONFIG_SYNC_ENABLED, so nothing changes while it is off. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Hayt <211b96e6a2b7f45fd4047988976c7bbbeeda0c15f3ae7b32eec20834b5a55118@buzz.block.builderlab.xyz>
The "add first" refusal, a retried Create keeping its id and home, and Import's unadded members run through the service with the community config mock, so the flag-gated paths are checked while the flag ships off. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Hayt <211b96e6a2b7f45fd4047988976c7bbbeeda0c15f3ae7b32eec20834b5a55118@buzz.block.builderlab.xyz>
Discovery no longer restores a member a readable roster dropped, Save only succeeds when every requested member is present and the channel is active, and Import seeds a new synced team from an edited local copy. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Hayt <211b96e6a2b7f45fd4047988976c7bbbeeda0c15f3ae7b32eec20834b5a55118@buzz.block.builderlab.xyz>
the team editor sends a draft id with a new team so every retry is the same team; the page test now expects it. Co-authored-by: peon <9ac6794b000690b7e814eb1805ad32405d0bec7d52838de3a86cf967565dacc0@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
config sync ships on in the base, so synced teams follow relay support instead of a flag: the service tests no longer mock CONFIG_SYNC_ENABLED or a coordinator check, and the docs name relay support as the condition. Co-authored-by: peon <9ac6794b000690b7e814eb1805ad32405d0bec7d52838de3a86cf967565dacc0@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
a start that fell back to localStorage could save the synced team index there; the next native start moved only agent keys, so the confirmed rosters were lost until rediscovery. the index is now an agent key. Co-authored-by: peon <9ac6794b000690b7e814eb1805ad32405d0bec7d52838de3a86cf967565dacc0@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
a create has no base, so its retry never removed anyone: dropping a member an earlier try added reported success with that member kept. that retry is now refused so the team is reopened as an edit; import stays additive. Co-authored-by: peon <9ac6794b000690b7e814eb1805ad32405d0bec7d52838de3a86cf967565dacc0@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
- check only members being added against this device: existing members are proven by the roster and may run on another device. - show and look up only the signed-in account's synced teams. - tell a failed support check from an unsupported relay, retry it, and refuse new team until support is known instead of saving it local. - name agents not on this device by short npub in team errors. - say reopen after a partial edit, without blaming another device, and say a deleted imported team can't be imported again. Co-authored-by: peon <9ac6794b000690b7e814eb1805ad32405d0bec7d52838de3a86cf967565dacc0@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
516754a to
ff4fafc
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account.
No further changes requested. The cross-device edit, account scoping and support-check retry changes preserve the prior fixes; Rocket, Mantis and Groot completed independent lanes, reconciled with my source review.
Reviewed head ff4fafc against base 90c3e83, focusing on the six-file follow-up and affected integration; seven earlier feature patches are unchanged. Source only: tests/app not run, CI not assessed, and live relay behavior and rendered focus/accessibility remain unverified; the public description and changed publication surfaces were checked.
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Found one creation-retry defect and two regression-test gaps.
| const subject = await teamSubject(id); | ||
| const channel = base | ||
| ? await channels.find("team-config", subject, signal) | ||
| : await channels.ensure("team-config", subject, edit.name, signal); |
There was a problem hiding this comment.
🤖 [P2] Serialize team creation with agent-config creation. After support discovery recovers, retrying New team while background agent-config creation is pending calls the session's shared channelCreation.create. Its single-flight implementation (channel-templates/setup.ts:150) returns the active admission regardless of input, so this team ensure receives the agent-config admission. Only the agent-config channel exists and the team fails with ‘The new config channel isn’t readable yet’; another Save after settlement succeeds. Reproduced in Chromium and WebKit for pending and failed discovery recovery, with different requested names/labels and coalesced:true. Serialize these ensures per session, or distinguish and serialize different creation intents in the shared creator; the per-team busy guard cannot cover this overlap.
| id: draft, | ||
| name: "Renamed", | ||
| instructions: "Ship it", | ||
| members: [alder], |
There was a problem hiding this comment.
🤖 [P3] Preserve the remote member in this regression's saved roster. Saving members: [alder] removes away, so the opened-member exemption at service.ts:3787 is never exercised. Removing that exemption still leaves this test passing. Saving [away, alder] instead passes with the fix and fails without it, protecting the second-device edit scenario while retaining the useful npub-label assertion.
| if (!writer || !read || !relayAuthor) return null; | ||
| const support = configSupport(await input.info(signal).catch(() => null)); | ||
| // A failed fetch rejects: it is checked again, unlike a relay without them. | ||
| const support = configSupport(await input.info(signal)); |
There was a problem hiding this comment.
🤖 [P3] Test the real support-discovery rejection contract. Service regressions replace communityConfig through coordinators.make, so they do not protect this distinction between failed discovery and confirmed unsupported. Restoring the old catch(() => null) here still passes all 909 agents tests, although it would again make a transient fetch failure silently select permanent local-team creation. Add direct communityConfig coverage asserting that info rejection propagates and an info response without nip-cl resolves null.
…el creates - notify subscribers when the derived team list changes, reusing unchanged teams so config churn notifies nobody. - refuse import while the community support check is pending or failed, through the same gate as new team, instead of saving a local team. - queue a different channel create behind an in-flight admission instead of handing it that admission; only identical requests share one. - reject queued work while an unconfirmed different create remains, so its retry receipt and uuid are kept. Co-authored-by: peon <9ac6794b000690b7e814eb1805ad32405d0bec7d52838de3a86cf967565dacc0@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account.
No further changes requested. Team notifications, Import’s support gate, and queued channel creation were reviewed with completed Rocket, Mantis and Groot lanes. Head b2ca4970fec7e311787a7b816a1d2ff9cf743024, base 90c3e83b30f45aa53b47a9dbe5cac9b6599ea422; follow-up to ff4fafcd. Source only: no tests, app/relay execution, performance measurements or rendered focus checks; runtime and human acceptance remain unverified.
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 One new regression-test gap; the prior creation-admission collision is repaired in the exercised paths.
| if ( | ||
| teams.length === prior.length && | ||
| teams.every((team, index) => team === prior[index]) | ||
| ) |
There was a problem hiding this comment.
🤖 [P3] Protect unchanged-team notification suppression with a regression. Replacing the reuse/early return here with unconditional teams = allTeams() and notify() still passes all 911 agents tests (one skipped). AgentsPage.tsx:206–212 reruns readLegacy(), including native portable(), whenever snapshot.teams changes identity, so losing this guard would make unrelated agent-config updates trigger legacy reads and rerenders while Agents is open. Add a test that reconfirms the same team head or updates an unrelated agent-config head and asserts both stable snapshot().teams identity and no subscriber notification. This is a demonstrated coverage gap, not a current runtime failure.
Teams sync through a config channel in their home community, the same model #1226 uses for agents. Each team's name and instructions are a
buzz.teamNIP-AR artifact in a private NIP-CL config channel, and its members are the owner's agents in that channel. There is no flag: like #1226, it is on wherever the community's relay supports config channels.