Repository navigation
feat(agents): sync agent config through config channels - #1226
Conversation
ed64d51 to
7710ccb
Compare
be38e4b to
4649139
Compare
wpfleger96
left a comment
There was a problem hiding this comment.
Independent source review — pass 2 of 3
Verdict: not ready for completed-feature handoff. Reviewed 7710ccbd3..464913923 at 46491392352f1c6546b32762a266bdaba6e62a72, excluding #965's own authorship but tracing its consumed seams. Origin and GitHub matched. Minimalism 8/10, elegance 8/10, correctness 5/10. The concrete causes of the lower scores are below: separate, well-tested helpers have not been connected into one complete application lifecycle. The smallest correction is to connect the existing owners, not introduce another framework.
The default-off guard is real: communityConfig returns before creating a feed, inspecting support or writing anything; no production caller supplies enabled. Delete uses the old hard-delete path, the existing local Teams tab stays local, saves do not publish config, and starts use the existing local binding path. Deleted-agent Settings registration is also gated. This establishes those dormant paths by source inspection, not blanket equivalence for the inherited #965 diff or live acceptance.
The findings below are defects in the implementation promised by PR C when enabled, not demands to turn it on now. Shipping it as explicitly incomplete dormant infrastructure would require an explicit scope decision; a later one-line flag change is not sufficient.
Blocking findings
-
CRITICAL — Correctness: losing the in-memory community object restores destructive/local behavior after migration.
service.ts:996-1001,1889-1905,2453-2458,2696-2702,2725-2745. Bindings/modes and lifecycle authority are consulted only throughcommunityConfigs; replacement removes that object and the asynchronous support check recreates it later, or returns null on failure/unsupported relay. Without it, start receives{}, lifecycle becomesactive, and Delete callsremoveLocal, which ultimately deletes the key.isSyncedcannot protect an omitted side. After a previously migrated device reconnects, or its relay downgrades, it can launch stripped/default/local draft config and hard-delete an identity that should remain recoverable. Fix: retain/load the persisted synced binding and lifecycle independently of live setup; distinguish unsupported-never-migrated from previously-synced unavailable. Block destructive fallback, refuse edits on downgrade, and use only eligible confirmed copies for offline starts. Exercise held setup, restart, disconnect and downgrade through the real service. -
IMPORTANT — Correctness: discovery and config-change results have no production integration.
community-config.ts:134-150callscreateConfigSyncwithout listeners. Its discovery/rediscovery/problem/history callbacks inconfig-sync.ts:100-112,504-523therefore do nothing at the application boundary.attachingalso retains completed attachments (community-config.ts:269-284). A new remote team or an older duplicate selected later is never discovered/reselected by the application; ordinary roster changes do not refresh the cached team members; store changes do not notify the service/UI. Fix: supply callbacks owned by the community module; validate/reselect/rebind on metadata changes, refresh team membership on active roster changes as well as archive, notify consumers, and preserve blocking evidence on denial. Test remote create, member add/remove, reselection and reconnect through production wiring rather than a fake listener. -
IMPORTANT — Correctness: an already-archived channel can still leave an agent active; archive rollback also loses its binding.
community-config.ts:247-255,302-310;config-lifecycle.ts:118-132. When another device archived the channel, a fresh lifecycle journal isactive; callingsettle(..., true)changes only pending intents, so it remains active andrunreturns without a binding. Starts then take the local path, and Deleted agents does not list it. Separately, a definite archive refusal deletes the binding first, rolls lifecycle back to active, and never rebinds. Fix: admit authoritative archive state even without a local intent, block before exposing starts, and rebind behind the barrier after a confirmed rollback. Keep pending intent settlement distinct from external lifecycle discovery. -
IMPORTANT — Correctness: retry/replay stops at helper APIs.
config-writes.ts:386-392has no production import/caller (community-config.tsandteam-config.tsimport migration/save, not save resume). An uncertain save persists forever and all subsequent saves throw atconfig-writes.ts:368-369, including after restart. Lifecycle resume runs only duringstartConfig(service.ts:2753-2754); same-session socket recovery (service.ts:2815-2818) does not resume it. Likewise, completed attachments retain uncertain migration outcomes without retry. Fix: invoke the existing durable replay/migration/lifecycle operations on reconnect/restart with bounded retries and surface remaining uncertainty; verify byte-identical resend at the service boundary. -
IMPORTANT — Correctness: ordinary saves erase uncertainty and report unconfirmed writes as success.
config-writes.ts:395-454;service.ts:1759-1765,2020-2025;team-config.ts:339-351. Held saves contain only the signed event, not whether it was uncertain; after a lost acknowledgement,resumefollowed by conflict or permission refusal callsdoneand deletes that evidence. An accepted, redacted original is then indistinguishable from an unaccepted edit, contrary to §3.5. Agent save silently returns foruncertain/accepted-unavailable; team save also ignores refused/superseded outcomes and treats uncertain membership as success because onlyfailedis collected. Fix: persist acceptance/uncertainty and competing-head outcomes before retiring replay entries; propagate all non-success results to the editor. Do not close/declare saved when the result is unknown. Test lost-OK → redaction → conflicting/refused replay, and uncertain member readback. -
IMPORTANT — Correctness: editors still read retired local fields, and saves do not carry the editor's base revision.
service.ts:2447,community-config.ts:394-418,config-writes.ts:370-377; example consumerclaude-code/tabs.tsx:803-825,920-923. Migration strips instructions/model/etc., but the production editor still receivesrecord.config. Reopening it displays defaults/empty instructions, not the synced head. The forms submit a complete normalized draft, so changing only workspace can publish empty instructions and default settings over the real head. Also,saveobtainsprevfrom the store at save time rather than the head the editor opened: a stale form can overwrite a remote change without producing a conflict. Fix: expose resolved editor snapshots while keeping drafts separate, and carry their per-side base revision through Save. Test migrate → reopen Settings → change only workspace, and open editor → remote update → save stale draft. -
IMPORTANT — Correctness: edits drop unrecognised fields and tags.
community-config.ts:74-90,403-418;team-config.ts:316-337;config-writes.ts:372-378. These paths reconstruct JSON from known fields and pass no prior extra tags (team edit keeps onlyteam). A valid revision with an additional JSON field or client tag loses it on the next normal edit, violating NIP-AP Editing / plan §3.1. Fix: merge recognised changes into the captured head and retain its unknown JSON fields/tags, or refuse an edit that cannot preserve them. Preserveimported-fromon team edits. -
IMPORTANT — Correctness: missing/unreadable team heads disappear instead of blocking, and arbitrary channel occupants become team members.
team-config.ts:282-303;service.ts:2087-2093. Discovery catches failed head reads as undefined and skips channels whose team id cannot be recovered. A known member can then start with that team's section silently omitted. The returned member list excludes only the owner, not humans or agents with another owner; a later edit can also try removing those occupants as if they were team members.showTeamsparses any cached head without checkingstate === "present", so a malformed/deletion head can throw instead of presenting its blocking state. Fix: retain known channel/team/roster evidence across unreadability, block affected starts, verify NIP-OA ownership before classifying team membership, preserve non-team occupants, and render invalid/deleted states safely. -
IMPORTANT — Correctness: the user-facing team import never uses the approved import identity/publisher.
service.ts:1556,1602-1618,2053-2069,3024-3029;community-config.ts:356-361. Import still writes a localbeta-<sha256>row and does not call theold-buzz-teampublication path. A later migration treats that id aslocal-team, producing a different UUID from §6.3, and no immediate publication happens on an already-bound community. Additionally, production passes the relay origin ascommunity(service.ts:2730), not the specified community UUID. Helper golden tests do not prove the production identity bytes. Fix: route supported import through the existing publisher with source kindold-buzz-team, verbatim original source id, owner and approved community identity; keep the legacy path only for local/unsupported behavior. Verify two real service instances and restart convergence, including an archived existing copy. -
IMPORTANT — Correctness: durable pending team members are never resumed after retiring the local row.
community-config.ts:351-365;team-config.ts:55,219-253.retireddeliberately permits a local row to be removed while members remain durably pending, but production iterates only remaining local rows. Nothing iteratesTeamJournal.all(). Once removed, the record is never retried even after the agent channel appears or the app reconnects. Fix: resume journal entries independently of local rows, using persisted channel/identity/intended members, and trigger that work when agent channels appear. Include new-team crash recovery as well, since a fresh Create click otherwise mints another UUID. -
IMPORTANT — Correctness: Delete team is a successful no-op for a synced team, and there is no team recovery path.
service.ts:2073-2074; callerAgentsPage.tsx:1291. It filters onlyteamList; synced teams are not in that list. The editor closes while the channel and team membership stay active. Archived teams are filtered out of display without a Recover action. Fix: route synced deletion/recovery through the selected team channel lifecycle, keep unsupported/local deletion unchanged, and expose archived teams plus recovery/cancel for an archived import collision, as §6 requires. -
IMPORTANT — Correctness / Elegance: Restart to apply remains disconnected and turn-scoped, not session-scoped.
runner.ts:395,1769-1771;agent-start.ts:172-203. Agit grepacrosssrc/findsrestartToApplyconsumers only in tests and no productionstarted.changedcaller. Runs are removed when turn work settles even if their harness sessions remain alive. The two-conversation helper test cannot establish the required visible mark. Fix: connect config changes to actual runtime/session revision lifetimes and expose the result to the existing UI; test two conversations, completion of one turn with its session retained, then a remote change and a second start. -
IMPORTANT — Correctness: PR C does not mount the history view or provide a restore path. Across production
src/,AgentConfigHistoryhas no caller; only its own module and tests reference it. Flipping the constant leaves History unreachable while blocked instructions say “restore from History” (agent-start.ts:62-64). The component deliberately rejects a deleted head rather than implementing restore. Fix: add the gated editor/history integration with the selected reader/writer/schema and a realop=restoreaction for deletion, or explicitly rescope that incomplete requirement before handoff. History redaction invalidation and the mounted visual/live checks remain separate flag-on gates. -
IMPORTANT — Correctness: the agent is never added to its own private config channel.
community-config.ts:239-258;config-channels.ts:167-183. Ensure creates a channel with the owner, family and subject labels, but no member setup. The agent attachment path never callschannels.addMember; that call exists only for team members. The owner-side live probe succeeds without asserting the agent can read its own config, hiding this NIP-AP §2 membership gap. Fix: confirm owner-attested agent membership through the existing narrow administration capability before completing attachment, retaining uncertainty/failure and testing a read authenticated as that agent.
Earlier six findings
The narrow system-member administration extension (not its missing agent-channel caller), exact relay-signed strong archive read, retired binding read/event fences, successor-checked removal prev, real native/browser config generation/EOSE seam, and denial-vs-transport read classification are materially improved and the original helper defects are closed by source inspection. The production lifecycle/discovery failures above mean those helper fixes do not establish end-to-end closure. Current relay handle_put_user preserves the explicitly requested member role; I am not re-reporting bot-role conversion based on an older assumption.
Every installation path I traced uses artifactProblem/checkedHead against an Expected (sync's live path passes the same validator directly). That does not validate the surrounding authority, draft, retry or UI integration.
Evidence and remaining gates
- Existing helper tests cover many meaningful invalid-head, lifetime, reconnect and uncertainty scenarios. Runtime delivery changes correctly assign delivered config for Claude/Goose; Codex consumes it per call. These are solid building blocks.
- The author probe in
~/.buzz/.scratch/pr1226-live/exercisescommunityConfigdirectly with a custom artifact signer, in-memory storage, and two config objects sharing one session. It does not exerciseAgentsService, native signing/credential persistence, mounted editors, socket-loss replay, or concurrent independent app instances. Mutation claims remain author-reported. The missing real service-enabled test is material, given the disconnected paths above. Hosted exclusion is visible in the local-running inventory path but still needs the T16 regression. - Native-signing/real credential-store acceptance, system-channel deletion-head/history/revert checks, history redaction reload and mounted UI acceptance remain unfulfilled flag-on gates. No local suites, builds, live traffic, edits or pushes were performed for this review.
- CI inspection: the newer PR C run
38835815774passed, includingCI required; the cancelled duplicate run left an older failed aggregate. This is not a source approval. Keep the PR draft; do not request human review or flip the flag on this verdict.
7710ccb to
056a15a
Compare
056a15a to
6cd54b9
Compare
4649139 to
03a8183
Compare
wpfleger96
left a comment
There was a problem hiding this comment.
Independent review — third and final pass
Verdict: not ready for completed-feature handoff. Four IMPORTANT blockers remain. Escalate to Will: the agreed three-pass review budget is exhausted. This is a COMMENTED review, not an approval or a request-changes action.
Reviewed 03a818346466dfff585e2dfb285448a3619146d7, verified against both GitHub and the origin branch. Scope: f2d4bfdb6..03a818346, with particular attention to fixes since 464913923; excluded the five inherited system-channel commits as requested. The PR remains draft, stacked on the history PR #1208. This verdict does not cover a future restack.
Minimalism 9/10: the repairs reuse the existing store, journals, resolver and runtime/session owners rather than introducing another framework. Elegance 8/10: agent and team save paths still interpret the same outcomes differently; the team path lacks the editor-base contract already implemented for agents. Reuse those existing contracts to reach the bar. Correctness 6/10: the concrete failures below remain, despite substantial genuine repairs.
Blocking findings
1. IMPORTANT — Correctness: persisted authority still omits teams when the live community disappears
storedBinding, community-config.ts:150–166 reconstructs only settings/instructions from the migration journal. syncedBinding, service.ts:2663–2675 falls back to it; the runner then resolves teams only if that binding contains them (service.ts:1096–1111).
Reproduced: enable the real service/config wiring, create a synced team with TEAM RULE, deliver a mention and verify that section is present. Reconnect with no live config community, then deliver another mention. The second run occurs with team === undefined. Agent settings/instructions retain authority, but confirmed team instructions silently disappear. The same omission bypasses resolution of any team blocking state; that latter consequence is source-traced, not a separate live reproduction.
Fix: retain/load team identity, membership and lifecycle evidence independently of the temporary community object, and include it in start resolution. Use eligible confirmed copies only where the offline rule permits; otherwise block rather than omit the team. Cover reconnect/setup loss, downgrade and restart with a previously synced team, not just agent sides. This is a residual of prior findings 1/8, not a demand to enable the feature.
Witness: service.test.ts — review witness: keeps team sections while a synced community is unavailable.
2. IMPORTANT — Correctness / Elegance: team editing still reports unconfirmed work as successful
service.ts:2166–2173 rejects membership failures and artifact conflict only. Artifact outcomes uncertain, accepted-unavailable, refused and superseded still resolve as a saved team. TeamEditor, AgentsPage.tsx:1273–1292 closes on that resolution. Separately, team-config.ts:428–439 collects only membership status failed, treating uncertain as success.
Reproduced at both boundaries: the actual service resolves a team when its config API returns saved.status = "uncertain"; the actual team editor helper returns failed: [] when removal returns status: "uncertain". These are controlled dependency outcomes, not fabricated success assertions. A user can close the editor believing instructions or member removal completed when the result is unknown. An uncertain removal can leave an agent receiving team instructions.
Fix: reuse the agent path's honest outcome handling: only a confirmed matching artifact save (or a genuine no-op) and confirmed membership changes complete the edit. Surface partial/unknown/refused/superseded outcomes without closing the editor; retain the intended edit and unresolved membership work for recovery. Add service/editor tests for these outcomes. Prior finding 5 is only partially closed.
Witnesses: review witness: rejects an uncertain team artifact save in service.test.ts; review witness: does not confirm uncertain membership changes in team-config.test.ts.
3. IMPORTANT — Correctness: the team editor still overwrites concurrent edits without carrying its opened base
TeamEditor, AgentsPage.tsx:1266–1292 captures editable values but no artifact revision. editTeam, team-config.ts:399–426 builds from the store's head at submission and calls save without its base argument. The shared writer therefore selects the current head as prev (config-writes.ts:477–481), legitimizing a stale full form over a newer revision.
Reproduced: capture a team rename draft with the old instructions; save REMOTE_RULE as another edit; submit the captured rename. The second edit returns saved, not conflict, and replaces the remote instructions with the stale text. The agent editor now has the correct base contract; the team editor still does not.
Fix: carry the team's opened head through the service/community/helper calls, use it for content composition and prev, and report a conflict rather than silently rebasing a stale form. Keep the draft available. This is the remaining team portion of prior finding 6.
Witness: team-config.test.ts — review witness: refuses a stale team editor after a remote edit.
4. IMPORTANT — Correctness: Claude's temporary metadata-discovery cache survives remote config changes
sessions.ts:149–160 keys discovery only by the restart fingerprint. Remote config edits deliberately change the snapshot without changing that fingerprint (runtime.ts:701–708). Selecting live processes on the current snapshot is a real improvement, but the old temporary discovery result remains a fallback and can be returned directly. refusedEffort, runtime.ts:266–279 uses that reported model when the delivered config has no explicit model.
Reproduced through ClaudeRuntime: sync a local haiku row; open model choices before a conversation, caching metadata; remotely clear the explicit model and set synced effort xhigh; deliver a new conversation. The fake Claude's default is opus and offers xhigh, but the runtime refuses it with The synced effort "xhigh" isn't available for Haiku 5.5. A separate session-level witness returns the old haiku reported model after the snapshot changes. This is a stale-cache defect in the requested unsupported-setting/reporting chain, not a claim about a live Claude binary.
Fix: key discovery and its fallback reuse by the current config snapshot as well as the restart fingerprint; invalidate or fence stale in-flight discovery without restarting pinned conversations. Verify new-conversation and one-off admission after clearing/changing remote model settings. Wes's #1185 comment is not fully closed.
Witnesses: review witness: refreshes metadata discovery after remote config changes in sessions.test.ts; review witness: checks default-model effort against fresh discovery after a remote edit in runtime.test.ts.
Closure of the second review
| Prior finding | Assessment at this head |
|---|---|
| 1 — persisted authority | Agent-side binding/lifecycle and destructive fallback protections are materially fixed; team authority remains incomplete (blocker 1). |
| 2 — discovery/change callbacks | Production callbacks now connect discovery/reselection, roster refresh, config and history changes. Closed by source assessment, not native live acceptance. |
| 3 — remotely archived discovery / rollback binding | External archive observation and rebind after refused archive are connected. Closed by source assessment. |
| 4 — replay/reconnect | Agent held-save replay, reconnect rebind and lifecycle resume are connected; the existing reconnect receipt is meaningful. This does not establish every team-edit recovery outcome. |
| 5 — honest saves | Agent outcomes/uncertainty retention improved; team outcome handling still blocks (finding 2 above). |
| 6 — editor snapshot/base | Agent resolved snapshot and base flow are repaired; team opened-base is still absent (finding 3 above). |
| 7 — unknown fields/tags | edited() retains JSON fields; the shared save() calls carried(head, extra), including for team edits. The explicit team tag does not discard other tags, including imported-from. Closed by source assessment. |
| 8 — unreadable teams/ownership/rendering | Known/journal identity retention, owner-attested members, explicit unreadable lifecycle and safe rendering are implemented. The cached-present-head transition lacked a regression test; independently demonstrated below. Loss of the whole live community remains blocker 1. |
| 9 — Import | Synced import now reaches the publisher with the approved source kind/id and community UUID; independent production-path mutation is detected. |
| 10 — pending team members / stopped Create | Journal enumeration, agent-attach/reconnect resume and unfinished-Create identity reuse are connected, independent of retired local rows. |
| 11 — team deletion/recovery | Synced archive/recover APIs and visible recovery paths are connected. |
| 12 — Restart to apply | Host/UI mark and runtime retained-session behavior are connected; independent Goose mutations are detected. |
| 13 — History | Gated production History mount, restore action and redaction invalidation exist. Mounted/native/live acceptance is still outstanding. |
| 14 — private agent membership | Attachment now requires confirmed membership before binding. Actual agent-authenticated access remains a live acceptance check. |
Both required enabled-service tests exist and passed in my baseline: with config sync on, keeps a synced agent's authority across reconnect, downgrade and restart and with config sync on, leaves hosted agents out of config sync. The former does not exercise team authority, which is why its passing result does not close blocker 1.
The four #1185 comments
- Bad saved data: record/entry shape, state and counter validation plus the requested malformed-data cases are present; closed by source assessment.
- Launch-test setup: Claude/Goose launch tests now seed the runtime with
syncand inspect processes that actually consume prompts; closed. The tests no longer accidentally prove only the warm spare. - Refusal ownership: the unsupported-type rule lives in
resolveStart, with shared service input construction. My mutation is detected by the resolver test and three service tests; closed. - Unsupported synced setting: delivered-model checks, an honest empty-model-list refusal, skipping checks for pinned older conversations, and current-snapshot live-process selection are implemented. Not fully closed because temporary discovery is stale across remote changes (blocker 4).
Independent mutation evidence and the unreadable-team test decision
The unchanged-head baseline passed 6 complete files / 357 tests: agent-start, team-config, community-config, launch-boundary, agents service, and Codex runtime. These are focused independent review checks, not the author's full 673-file / 10,239-test run and not an independent full-package run.
I reran these production mutations individually in a disposable worktree at the verified head, restoring each afterward:
| Mutation | Independent result |
|---|---|
| Remove the team journal's identity fallback on unreadable discovery | Killed by keeps a known team whose head can't be read as unreadable, with its members (full team-config.test.ts). |
| Disable the service's synced Import branch | Killed by with config sync on, imports an old Buzz team onto the one team every device and restart reaches (full service.test.ts). |
| Make Goose fingerprint follow delivered config | Killed by the running-Goose-until-Restart test (full launch-boundary.test.ts). |
Remove Goose Restart's increment of restarts |
Killed by the same test (full launch-boundary.test.ts). |
Skip resolveStart's unsupported-settings refusal |
Killed by its resolver test plus all three unsupported-type service tests (both complete files). |
Map unreadable teams to active in binding() |
Survives existing tests: both full service.test.ts and community-config.test.ts pass, 229 tests. |
Yes, the last mutation needs a test. I added a temporary service witness that first confirms a team/head and a successful delivery, then makes the authoritative head lookup return no result and refreshes the roster while the last present head remains cached. The unchanged implementation blocks the next start. With only the unreadable-to-active mutation, that same witness admits a second run and fails. This establishes both the correctness of the current guard and the missing regression's practical value; it is not evidence that the unchanged guard is broken. Add this case to the branch's permanent coverage.
Verification limits and CI
- The final independent witness run against unchanged production code exercised 4 complete files / 290 tests: 284 passed, 6 deliberate witness failures corresponding to the four blockers. The cached-unreadable positive control passed. No timeouts explain those six failures. The portable test-only patch is below; no author-branch edits, commits or pushes were made.
- I did not duplicate CI's broad suites or run an independent full package suite. Mutation runs are scoped to the complete affected test files, not full-package mutation proof.
- CI run 38942070471 is red. JavaScript, Rust/tool integration, measurements and Vitest flake checks pass. Chromium and WebKit shard 6/9 both fail
agent-library.spec.mjs'sagent cards expose selection and creation preserves focus and selected borders(missing Ada gridcell at line 14) andowner profile keeps its pane and caret stable during inline name editing(missing profile region at line 129). Cause and attribution are not established; these cannot be waved away as the local overloaded-machine hook failures. Windows native validation is skipped. CONFIG_SYNC_ENABLEDremains false before setup, with no production enabled override found in the reviewed source. This protects the dormant paths; it does not make the enabled feature complete. No independent live relay/process probe, native signing/credential-store acceptance, agent-authenticated private-channel read, or mounted visual/interaction acceptance was performed in this pass. Those remain separate activation gates.
Decision required — no routine fourth pass
Keep the PR draft and the feature off. Recommended: Will authorizes a bounded repair scope for the four blockers, permanent cached-unreadable coverage, CI diagnosis and the required independent source/live verification after repair and restack. Alternative: explicitly reduce the deliverable to incomplete dormant infrastructure, recording the defects and activation gates rather than declaring the feature complete. A restack alone does not resolve these findings, and the exhausted budget does not authorize another automatic iteration.
Portable independent test-only witnesses (apply to the reviewed SHA in a disposable worktree)
The patch adds six failing witnesses and one passing cached-unreadable regression. Run the four complete files with bin/pnpm exec vitest run src/features/agents/service.test.ts src/features/agents/team-config.test.ts src/bundled/claude-code/sessions.test.ts src/bundled/claude-code/runtime.test.ts. Production mutations are not included.
diff --git a/src/bundled/claude-code/runtime.test.ts b/src/bundled/claude-code/runtime.test.ts
index fd5e2a1c0..ea957229c 100644
--- a/src/bundled/claude-code/runtime.test.ts
+++ b/src/bundled/claude-code/runtime.test.ts
@@ -1763,3 +1763,22 @@ it("checks a synced effort only for a conversation that will run on it", async (
f.deliver(mention(event("3")), haiku.config, haiku.synced),
).rejects.toThrow(/isn't available for/);
});
+
+// Temporary independent review witness; not part of the proposed change.
+it("review witness: checks default-model effort against fresh discovery after a remote edit", async () => {
+ const f = setup();
+ try {
+ f.runtime.sync([{ pubkey: self, name: "Claude", config: { ...DEFAULT_CONFIG, model: "haiku" } }] as unknown as Agent[]);
+ // Opening the model chooser before any conversation caches haiku discovery.
+ await f.runtime.models(self);
+ const next = { ...DEFAULT_CONFIG, model: "", effort: "xhigh" };
+ const trigger = { type: "mention", event: message("a", "@Claude fresh") } as Delivery["trigger"];
+ // Remote config clears the explicit model. The fake's default is opus,
+ // which supports xhigh, unlike the previously discovered haiku.
+ await expect(f.deliver(trigger, next, { model: "", effort: "xhigh" })).resolves.toBeUndefined();
+ await flush(10);
+ expect(f.claudes().some((c) => c.prompts.length > 0)).toBe(true);
+ } finally {
+ f.runtime.dispose();
+ }
+});
diff --git a/src/bundled/claude-code/sessions.test.ts b/src/bundled/claude-code/sessions.test.ts
index 3f7d562d0..3c8e7d2de 100644
--- a/src/bundled/claude-code/sessions.test.ts
+++ b/src/bundled/claude-code/sessions.test.ts
@@ -573,3 +573,17 @@ it("reads the reported model only from a process on current config", async () =>
expect((await pool.metadata("c/new")).model).toBe("claude-opus-5-5");
pool.dispose();
});
+
+// Temporary independent review witness; not part of the proposed change.
+it("review witness: refreshes metadata discovery after remote config changes", async () => {
+ let launched = "haiku";
+ const { pool, setConfig } = sessions({ launchModel: () => launched });
+ expect((await pool.metadata("c/first")).model).toBe("claude-haiku-5-5");
+ launched = ""; // A remote edit clears the explicit model; Claude now reports its default, opus.
+ setConfig("remote");
+ try {
+ expect((await pool.metadata("c/new")).model).toBe("claude-opus-5-5");
+ } finally {
+ pool.dispose();
+ }
+});
diff --git a/src/features/agents/service.test.ts b/src/features/agents/service.test.ts
index 242b3ccde..76fe391e5 100644
--- a/src/features/agents/service.test.ts
+++ b/src/features/agents/service.test.ts
@@ -6318,6 +6318,69 @@ describe("Delete as a recoverable archive", () => {
await f.ctx.fiber.dispose();
});
+ it("review witness: keeps team sections while a synced community is unavailable", async () => {
+ const relay = configRelay();
+ communityFake.value = relay.opens;
+ const f = await setup({ identities: [here(alder)] });
+ try {
+ await vi.waitFor(() => expect(f.service.find(alder)?.configBase).toBeDefined());
+ await f.service.saveTeam({ name: "Core", instructions: "TEAM RULE", members: [alder] });
+ mentionAlder(f, "before");
+ await vi.waitFor(() => expect(f.run).toHaveBeenCalledTimes(1));
+ expect(f.run.mock.calls[0]?.[0].team).toContain("TEAM RULE");
+ communityFake.value = null;
+ f.reconnecting();
+ f.connect(true);
+ await settle();
+ mentionAlder(f, "after");
+ await vi.waitFor(() => expect(f.run).toHaveBeenCalledTimes(2));
+ expect(f.run.mock.calls[1]?.[0].team).toContain("TEAM RULE");
+ } finally {
+ await f.ctx.fiber.dispose();
+ }
+ });
+
+ it("review witness: blocks an unreadable team even with a cached present head", async () => {
+ const relay = configRelay();
+ communityFake.value = relay.opens;
+ const f = await setup({ identities: [here(alder)] });
+ try {
+ await vi.waitFor(() => expect(f.service.find(alder)?.configBase).toBeDefined());
+ const team = await f.service.saveTeam({ name: "Core", instructions: "TEAM RULE", members: [alder] });
+ mentionAlder(f, "before");
+ await vi.waitFor(() => expect(f.run).toHaveBeenCalledTimes(1));
+ expect(f.run.mock.calls[0]?.[0].team).toContain("TEAM RULE");
+ const config = relay.opens.mock.results[0]?.value;
+ const synced = config?.teams()?.find((t) => t.id === team.id);
+ if (!config || !synced) throw new Error("no synced team");
+ for (const [d, head] of relay.heads)
+ if (head.tags.some(([name, value]) => name === "h" && value === synced.channel)) relay.heads.delete(d);
+ config.rosterChanged(synced.channel);
+ await vi.waitFor(() => expect(f.service.snapshot().teams.find((t) => t.id === team.id)?.note).toBe(UNREADABLE_TEAM));
+ mentionAlder(f, "after");
+ await settle();
+ expect(f.run).toHaveBeenCalledTimes(1);
+ } finally {
+ await f.ctx.fiber.dispose();
+ }
+ });
+
+ it("review witness: rejects an uncertain team artifact save", async () => {
+ const relay = configRelay();
+ communityFake.value = relay.opens;
+ const f = await setup({ identities: [here(alder)] });
+ try {
+ await vi.waitFor(() => expect(f.service.find(alder)?.configBase).toBeDefined());
+ const team = await f.service.saveTeam({ name: "Core", instructions: "", members: [alder] });
+ const config = relay.opens.mock.results[0]?.value;
+ if (!config) throw new Error("no config");
+ vi.spyOn(config, "editTeam").mockResolvedValue({ saved: { status: "uncertain", message: "lost OK", retired: false }, failed: [] });
+ await expect(f.service.saveTeam({ ...team, instructions: "edit" })).rejects.toThrow();
+ } finally {
+ await f.ctx.fiber.dispose();
+ }
+ });
+
it("with config sync on, deletes a synced team by archiving its channel, and recovers it", async () => {
const relay = configRelay();
communityFake.value = relay.opens;
diff --git a/src/features/agents/team-config.test.ts b/src/features/agents/team-config.test.ts
index 60e00d2a0..e08de426f 100644
--- a/src/features/agents/team-config.test.ts
+++ b/src/features/agents/team-config.test.ts
@@ -297,6 +297,25 @@ describe("synced teams", () => {
expect(f.channels.removeMember).toHaveBeenCalledWith(teamChannel, b);
});
+ it("review witness: refuses a stale team editor after a remote edit", async () => {
+ const f = await published();
+ const [found] = await discoverTeams(f.deps);
+ if (!found) throw new Error("no team");
+ const opened = { ...team, name: "My rename" };
+ expect((await editTeam(f.deps, found, { ...team, instructions: "REMOTE RULE" })).saved?.status).toBe("saved");
+ const result = await editTeam(f.deps, found, opened);
+ expect(result.saved?.status).toBe("conflict");
+ });
+
+ it("review witness: does not confirm uncertain membership changes", async () => {
+ const f = await published();
+ const [found] = await discoverTeams(f.deps);
+ if (!found) throw new Error("no team");
+ f.channels.removeMember.mockResolvedValue({ status: "uncertain" } as never);
+ const result = await editTeam(f.deps, found, { ...team, members: [a] });
+ expect(result.failed).toEqual([b]);
+ });
+
it("saves an edit as a team revision and changes membership by diff", async () => {
const f = await published();
const [found] = await discoverTeams(f.deps);6cd54b9 to
b108aa7
Compare
03a8183 to
e666328
Compare
wpfleger96
left a comment
There was a problem hiding this comment.
Fourth review: scoped repairs and restack at e666328127117c1adc55431bbb8bc834b6589ff8
Verdict: does not pass. Two IMPORTANT blocking findings remain within the requested scope. The existing-team repairs work in the scenarios from my third review; the wider team-write invariant is still incomplete, and the Goose conflict resolution introduced a separate launch race. This is the extra review Will authorized after the original three-pass budget. Further repair/review needs Will's decision, not another automatic pass.
Quality: minimalism 9, elegance 9, correctness 7. The persisted team projection and editor-base propagation are necessary and follow the existing journal/save owners; Claude's snapshot predicate is a small, clear correction. Correctness remains below the shipping bar for the concrete failures below.
IMPORTANT 1 — Team writes still report unconfirmed work as success
Axis: correctness.
The changed existing-team path now rejects every non-saved artifact result and every non-confirmed member result, but the same contract does not hold across the team's other UI actions:
- Create:
community-config.ts:622–647checks onlyentry.status === "confirmed". That status means the artifact is confirmed, not that its members joined.team-config.ts:309–320,327–352returns it unchanged when additions are uncertain/failed, recording those members as pending.service.ts:2197–2202then returns the requested member list as though it were confirmed, andAgentsPage.tsx:1403–1412closes the editor. I independently injecteduncertain, thenfailed, for the team-channel addition (not the agent's own config-channel membership): both Create calls resolved successfully. The retry journal is valuable, but silently queued work is not a confirmed save; callers cannot tell that team rules do not yet apply to the requested members. - Delete while the config community is unavailable:
service.ts:2243–2246looks only in live community configs viasyncedTeam()(2306–2317). After losing that object, a known synced team falls into local deletion, removes no synced row, and resolvesundefinedwithout archiving its channel. The new durable team journal knows the team, but this write path does not consult it. My enabled-service witness creates the synced team, drops the live community through reconnect, then proves Delete resolves rather than refusing. This is a service contract defect even though a reconnect may also remove the currently displayed team/editor.
Risk: the UI claims completion while requested members lack team instructions, or while a supposedly deleted team's instructions remain active. The latter can continue to launch from the newly repaired offline authority.
Required fix: distinguish artifact publication from full/partial team-save completion. Create must return an explicit pending/partial result or throw while preserving the same durable team id and pending membership intent; retry must not create a duplicate even if the artifact was already confirmed. Every write to a journal-known synced team must refuse while its community is unavailable rather than taking the local path. Retain draft/member intent and expose the unconfirmed state. Ordinary migration/import may retain their documented pending-member behavior; do not break that to repair UI success reporting.
IMPORTANT 2 — Goose's restacked launch mixes two config snapshots
Axis: correctness.
In src/bundled/goose/runtime.ts:563–578, launch captures workspace, instructions and scope before awaiting core memory, but now reads choiceOf(entry.config) after that await. Before the restack, provider/model were captured before the await too. Meanwhile sessions.ts:375–379,425–430 records the pre-await snapshot as the session's immutable started config.
Reproduction: hold the core-memory promise; deliver a first conversation on instructions BEFORE / provider openai / model before-model / effort high; deliver a second conversation with synced instructions AFTER / provider anthropic / model after-model / effort low; release memory. The first session has BEFORE instructions, but starts on anthropic / after-model / low. All three choice assertions fail against unchanged production. It is labeled with the old snapshot, so it is not the configuration either delivery intended.
Risk: a remote edit changes the provider/model/thinking level of a conversation already opening, defeating the promised config pin and potentially routing its work to the wrong provider.
Required fix: capture the complete launch config (or provider/model/effort alongside the other fields) before the first await, and build the launch's choice from that capture. Keep a deterministic deferred-memory regression. A temporary diagnostic that captured entry.config before the await made this witness pass; it is reverted, not an author-branch contribution.
Closure of the four prior blockers
| Prior blocker | This head |
|---|---|
| 1. Team authority without a live community | Closed for the reviewed start path. Discovery persists identity/members/lifecycle in the owner/community-keyed team journal; storedBinding projects it to starts. Original offline-team witness passes, and removing that projection fails it. Persisted unreadable/archive state is retained. |
| 2. Every team save reports non-success | Partial, still blocking. Existing-team artifact/member editing is repaired; Create and unavailable-community Delete still claim success as detailed above. |
| 3. Team form carries opened revision | Closed for the reviewed stale-artifact scenario. Shown head → form state → TeamChange.base → community → editTeam → save's conflict check. Original stale-editor witness passes and current-head mutation fails it. |
| 4. Claude discovery across synced edits | Closed at source/test level. Discovery fingerprint AND config snapshot now gate reuse; every live-process candidate is current-config-only, including the chooser. Both original witnesses pass and fail when the snapshot condition is removed. The chooser test fails if its pinned-process bypass is restored. |
Claude call-path audit agrees with Duncan: the runtime model chooser calls metadata() without a key; mention refusal calls metadata(key) after the pinned-conversation early return; standalone refusal calls it without a key. readModel() is consumed through metadata's process reader. Current-config candidate filtering plus snapshot-keyed discovery also protects the reported-model fallback. The separate setup probe is not this runtime cache. Existing pinned-conversation, current-config fallback, unknown-model refusal and unsupported-effort checks pass. This does not substitute for real Claude/native acceptance.
Restack and browser fixture
- Source-verified PR head remains
e66632812; base PR #1208 isb108aa717. The sync range has 53 commits. Range-diff confirms exactly the seven changed old commits requested: 18, 23, 25, 31, 34, 40, 45. Six changes are context from main; the material Goose launch change in 40 is finding 2. - #1208's two patches remain range-diff
=.src/features/relay/store.tsandstore-system-discovery.test.tshave no diff from fetchedorigin/main; the config routes use main's owner. I did not independently rerun tsc/build; hosted results were pending when inspected. - The Claude cherry-pick has the same stable patch-id as
ec5e50a5d:0a2d4dedaeeffe24a5bc87b2d3858e33ed4ec7a7. - Goose's steady-state thinking semantics are sound: local choice clearing reopens; remote choice clearing leaves an existing session on high until Restart to apply. I added a separate regression for the latter and it passes. Removing the local reset guard fails the existing chosen-level test; a deliberately over-eager reset fails the new remote-pinning witness. This does not repair the opening-session race.
- The browser correction is at the right owner: the cast fake
AgentslackedconfigHistory, and selecting Ada mounts the real editor's History access. Returningundefinedcorrectly models a local/non-synced agent. The expectations/components need no relaxation. Hayt reported 16 passing Chromium/WebKit cases; I inspected the fixture and source-serving entry point, not an independent browser rerun. Browser cases/assertions were not removed. - The unreadable-team cached-present-head coverage gap is closed: its permanent service test passes and fails my unreadable-to-active mutation.
Independent execution and limits
Command for every baseline/mutation/witness execution:
bin/pnpm exec vitest run src/features/agents src/bundled/claude-code src/bundled/goose src/bundled/agentsHEAD was checked in the shell for each execution, including programmatically before mutation runs. These are the complete owning feature/plugin suites, not filtered individual test names. I ran them to answer the explicitly requested witness/mutation questions, not as substitutes for hosted broad checks.
- Unchanged production and unchanged tests at this head: 1,020 passed, 6 skipped; 72 files passed, 1 skipped.
- Independently removed offline-team projection, existing-team artifact outcome handling, existing-team membership handling, opened editor base, Claude snapshot cache key, Claude chooser filtering, unreadable-team state and Goose local effort reset: each relevant regression went red. Mutations were restored individually. An initial membership mutation also touched publication's success predicate; I reran an edit-only mutation and it fails only the intended uncertain-membership regression.
- Final test-only witnesses, unchanged production: 1,021 passed, 4 failed, 6 skipped across 73 files. Four failures prove Create uncertain, Create failed, unavailable-community Delete, and mixed-snapshot Goose launch. The added remote-effort-pinning witness passes. Goose's one failed witness contains three failing soft assertions (provider, model, effort); its old-instruction assertion passes.
- Diagnostic positive control: capturing Goose config before awaiting memory makes its witness pass, leaving only the three team-outcome witnesses red. All production changes are reverted.
- CI inspected once during this review: 6 success, 23 in progress, 2 skipped; none failed at inspection. This is not a final CI verdict. No polling and no native/live relay, real-Claude or real-Goose acceptance performed.
Recommendation for Will: keep draft/off. Authorize a bounded repair of the two findings (team outcome/authority handling and the one Goose pre-await capture) followed by independent source and appropriate live verification; alternatively explicitly accept known-incomplete dormant infrastructure with the defects recorded. Do not enable or describe the feature as complete. No automatic fifth review.
Portable test-only witnesses (apply at e66632812)
diff --git a/src/bundled/goose/runtime.test.ts b/src/bundled/goose/runtime.test.ts
index d924f2371..5f700563d 100644
--- a/src/bundled/goose/runtime.test.ts
+++ b/src/bundled/goose/runtime.test.ts
@@ -608,3 +608,25 @@ function slowMemories(viewer: string) {
() => true,
).capability;
}
+
+it("review witness: captures Goose choice with the launch snapshot before memory is awaited", async () => {
+ const f = setup();
+ let release!: (value: string | null) => void;
+ const gate = new Promise<string | null>((done) => { release = done; });
+ f.core.mockImplementation(() => gate as never);
+ const before = { ...DEFAULT_CONFIG, instructions: "BEFORE", provider: "openai", model: "before-model", effort: "high" };
+ const after = { ...DEFAULT_CONFIG, instructions: "AFTER", provider: "anthropic", model: "after-model", effort: "low" };
+ try {
+ await f.deliver(message("1", "first"), { config: before });
+ await vi.waitFor(() => expect(f.core).toHaveBeenCalledTimes(1));
+ await f.deliver(message("2", "second"), { config: after });
+ await vi.waitFor(() => expect(f.core).toHaveBeenCalledTimes(2));
+ release(null);
+ await vi.waitFor(() => expect(f.fake.geese[0]?.turns).toHaveLength(2));
+ const goose = f.fake.geese[0];
+ expect.soft(goose?.prompts.get("session-1")).toContain("BEFORE");
+ expect.soft(goose?.opened[0]?._meta).toMatchObject({ provider: "openai" });
+ expect.soft(goose?.configured).toContain("session-1 model=before-model");
+ expect.soft(goose?.configured).toContain("session-1 thinking_effort=high");
+ } finally { release(null); f.runtime.dispose(); }
+});
diff --git a/src/bundled/goose/sessions.test.ts b/src/bundled/goose/sessions.test.ts
index 8f336efeb..9a95386c9 100644
--- a/src/bundled/goose/sessions.test.ts
+++ b/src/bundled/goose/sessions.test.ts
@@ -701,3 +701,19 @@ describe("AgentSessions", () => {
expect(other.sessions.snapshot().map((live) => live.key)).toEqual(["a"]);
});
});
+
+it("review witness: a remote Goose thinking edit stays pinned until Restart to apply", async () => {
+ const { sessions, change, fake } = setup({ choice: { provider: "", model: "", effort: "high" } });
+ try {
+ await sessions.deliver("a", "before", 1);
+ change({ choice: { provider: "", model: "" } }, false);
+ await sessions.deliver("a", "still before", 2);
+ expect(fake.geese[0]?.closed).toEqual([]);
+ expect(fake.geese[0]?.configured).toEqual(["session-1 thinking_effort=high"]);
+ expect(fake.geese[0]?.turns.map((t) => t.sessionId)).toEqual(["session-1", "session-1"]);
+ change({ systemPrompt: "Restart applied" });
+ await sessions.deliver("a", "after restart", 3);
+ expect(fake.geese[0]?.closed).toEqual(["session-1"]);
+ expect(fake.geese[0]?.turns.at(-1)?.sessionId).toBe("session-2");
+ } finally { sessions.dispose(); }
+});
diff --git a/src/features/agents/service.test.ts b/src/features/agents/service.test.ts
index 35150dca2..897cba439 100644
--- a/src/features/agents/service.test.ts
+++ b/src/features/agents/service.test.ts
@@ -6390,6 +6390,48 @@ describe("Delete as a recoverable archive", () => {
}
});
+ it("review witness: Create does not report unconfirmed team membership as success", async () => {
+ const relay = configRelay();
+ communityFake.value = relay.opens;
+ const f = await setup({ identities: [here(alder)] });
+ try {
+ await vi.waitFor(() => expect(f.service.find(alder)?.configBase).toBeDefined());
+ relay.addMember.mockImplementation(async (id) =>
+ [...relay.channels.values()].some((c) => c.id === id)
+ ? { status: "uncertain" } as never
+ : { status: "confirmed" });
+ await expect(f.service.saveTeam({ name: "Pending", instructions: "RULE", members: [alder] })).rejects.toThrow();
+ } finally { await f.ctx.fiber.dispose(); }
+ });
+
+ it("review witness: Delete does not report an unavailable synced team as success", async () => {
+ const relay = configRelay();
+ communityFake.value = relay.opens;
+ const f = await setup({ identities: [here(alder)] });
+ try {
+ await vi.waitFor(() => expect(f.service.find(alder)?.configBase).toBeDefined());
+ const team = await f.service.saveTeam({ name: "Offline", instructions: "RULE", members: [alder] });
+ communityFake.value = null;
+ f.reconnecting(); f.connect(true); await settle();
+ await expect(f.service.removeTeam(team.id)).rejects.toThrow();
+ expect([...relay.channels.values()][0]?.archived).toBe(false);
+ } finally { await f.ctx.fiber.dispose(); }
+ });
+
+ it("review witness: Create does not report failed team membership as success", async () => {
+ const relay = configRelay();
+ communityFake.value = relay.opens;
+ const f = await setup({ identities: [here(alder)] });
+ try {
+ await vi.waitFor(() => expect(f.service.find(alder)?.configBase).toBeDefined());
+ relay.addMember.mockImplementation(async (id) =>
+ [...relay.channels.values()].some((c) => c.id === id)
+ ? { status: "failed", error: "denied" } as never
+ : { status: "confirmed" });
+ await expect(f.service.saveTeam({ name: "Denied", instructions: "RULE", members: [alder] })).rejects.toThrow();
+ } finally { await f.ctx.fiber.dispose(); }
+ });
+
it("rejects an uncertain team artifact save", async () => {
const relay = configRelay();
communityFake.value = relay.opens;
wpfleger96
left a comment
There was a problem hiding this comment.
Scoped repair check: blockers remain
Reviewed pushed head 280968a566a4bf193ba45530e92f0b82736ed8af, verified from refs/pull/1226/head, against e66632812. Scope: only the two prior findings, the repairs, and their directly affected retry/write/launch paths. Draft/off must remain. This exhausts the human-authorized repair/check; remaining blockers go back to Will, not another automatic repair pass.
Minimalism 9 / elegance 9 / correctness 7. The production repair is small and uses existing ownership; all supplied regression tests pass. Correctness remains below the bar for the concrete failures below. All findings are IMPORTANT, affect correctness, and block handoff.
1. A confirmed artifact still masquerades as confirmed team membership
src/features/agents/community-config.ts:643-656checkspendingMembers.length, whereas retry selection at:631-634checks whether every intended member was added. These conditions are not equivalent. Insrc/features/agents/team-config.ts:330-338, the membership call can throw before any pending member is recorded.publishTeamcatches it at:316-321, retainingstatus: confirmedand an empty pending list. Create consequently returns plain success with the requested member missing. This is reachable in production:config-channels.ts:285-292awaitsowns(), whose relay read at:234-240can reject;administer()also awaitsrefresh()outside its catch. Independent service witness: a thrown member operation resolves Create instead of rejecting. Temporary diagnostic checking all intended members againstaddedMembersmakes that witness pass.- Import is not just background migration.
AgentsService.portTeamis a user-facing Import action (src/features/agents/service.ts:1741-1756): it reports a team whenentry.status === confirmed, ignoring pending membership andentry.error. With an uncertain member addition, my independent Import witness returns a team with no member, noteamError, no failed item and no note.teamPortNote(src/bundled/agents/AgentsPage.tsx:965-979) therefore supplies no warning.
Risk: the user is told their team operation succeeded even though agents are not on the team and do not receive its rules. Fix: apply one complete-membership success condition to user Create and Import; carry durable pending/partial intent and surface every unresolved member operation. Background migration can remain journal-driven without pretending to be a confirmed user save.
2. Create retry identity still depends on mutable form contents and pending status
src/features/agents/community-config.ts:625-640 matches by name/instructions and excludes a fully completed entry. TeamEditor (src/bundled/agents/AgentsPage.tsx:1394-1423) keeps neither a creation id nor a receipt after the partial error; its editable form calls Create again with no id.
Independent witnesses show:
- unchanged form + still-pending members: correctly reuses the original team;
- change name, then Save again: creates another id/channel;
- change instructions, then Save again: creates another id/channel;
- background
resumeTeams()confirms the first attempt, then Save again without editing: creates another id/channel.
The old team and its journal entry survive, so its pending memberships can still be applied after the user believes they corrected the original operation. Fix: allocate/return a stable creation operation/team id and retain it in the open form across errors, edits, and background completion; reconcile later saves against that same team and latest membership intent rather than matching mutable content. Do not simply match all completed teams by name: distinct intentional creates must remain possible.
3. Offline protection omits teams discovered on another device
src/features/agents/service.ts:2312-2316 searches only TeamJournal.all() (migration attempts), not the separate remembered discovered teams. TeamJournal.remember/known (src/features/agents/team-config.ts:98-117) persists teams discovered from the relay without creating a migration entry.
Independent two-device/service witness: device A creates the team; fresh device B discovers it and has a remembered team but zero migration entries. After B loses its live community, removeTeam(id) resolves successfully without archiving it. Edit is refused as No such team, so it does not falsely succeed, but it loses the intended unavailable-community diagnosis.
Risk: the original silent offline Delete defect remains for ordinary synced teams not created/imported on this device. Fix: route writes using saved synced-team authority, including remembered discovered teams, not only migration-attempt existence. Preserve the local/synced distinction. Paul's specific local-migration question checks out in the ordinary path: a local team's original id differs from its journaled UUIDv5 target; my unpublished-local-migration edit/delete witness passes. The new guard does not generally block those local rows merely because a migration attempt exists.
4. Goose team instructions remain a post-await launch input; the equality fence has an ABA hole
The provider/model/effort repair is correct. However src/bundled/goose/runtime.ts:565-578 captures only config; name and team are still read after memory. A single name/team change is caught by AgentSessions.open's fingerprint comparison (src/bundled/goose/sessions.ts:377-379,433-436), so the simple one-change race does not establish a defect. My independent single-change tests pass.
A controlled change-and-revert does: hold first launch in memory with config A/team A; install config B/team B via a second delivery; release first memory so its prompt captures A instructions/B team; hold MCP-server opening; install config A/team A again via a third delivery; finish opening. The fingerprint equals its original A value, so the mixed prompt is admitted and receives a turn. My witness verifies the actual fake-ACP prompt used by that turn, not an intermediate discarded launch. It contains BEFORE INSTRUCTIONS with AFTER TEAM. Capturing { config, team, name } before memory makes the witness pass; that diagnostic is reverted.
Risk: a conversation can execute with another configuration's team rules while labeled as the old snapshot. Fix: capture all system-prompt launch inputs before the first await, not just the settings object, and keep this ordering regression test. Name has the same mutable-read structure; I proved the team variant, not a separate name ABA case. Also, entry.team is assigned by run/runOnce, not sync; sync assigns entry.name.
Complete team-write trace
- Create: unconfirmed artifact and ordinary uncertain/failed/missing-channel member results now reject. Thrown member operations still escape the completeness test as described above.
- Edit: the service rejects every returned artifact outcome other than
savedand every member result other thanconfirmed; thrown calls reject too. Offline migration-known targets refuse; discovered-only targets still miss that guard (edit nevertheless rejects locally). - Archive/Delete and Recover:
teamLifecyclerejects unknown/wrong-state teams and awaits the shared channel lifecycle owner. Traced throughconfig-channels.ts:186-193intorelay/channel-lifecycle.ts:313-459: permissions/busy/unavailable/refused/unconfirmed operations throw; success requires relay-signed archive-state readback. The discovered-only offline Delete routing hole occurs before this owner. - Member add/remove: occur through Create/Import/migration and Edit; handled as above. There is no separate user member-write API in the reviewed agents surface.
- User Import:
portTeam -> importTeam -> publishTeamis user-visible and still returns unqualified partial success; see finding 1. Its retirement of an earlier local copy also checks artifact confirmation alone at the service layer. - Background local migration and reconnect/attach resume: use journaled
publishTeam; local migration removes its row only whenretired()permits it. These are distinct from user Import and do not need a false success claim. - Local save/delete and local import: synchronous local persistence; storage exceptions propagate or become the existing
teamError. No relay confirmation applies.
Evidence and limits
Command: bin/pnpm exec vitest run src/features/agents src/bundled/goose in a bootstrapped disposable worktree whose HEAD was checked in the same shell. This executes the complete owning feature/plugin suites, not selected test names.
- Unchanged production at
280968a56plus test-only witnesses: 784 passed, 7 failed, 6 skipped across 56 files. All shipped tests pass in this run; the seven failures are the new witnesses above (three Create-identity cases, thrown member operation, discovered-only offline Delete, user Import partial membership, Goose team change-and-revert). - Independent mutation: reversed only all three production repair hunks, kept tests, ran the same suites: 779 passed, 11 failed, 6 skipped before the final Goose ABA witness was added. Both shipped Create-result witnesses, shipped offline Delete witness and shipped Goose choice witness fail as expected. Production hunks were restored immediately afterward.
- Diagnostic only: capture Goose name/team before memory and use the complete intended/added-members Create predicate: same full suites give 786 passed, 5 failed, 6 skipped. The two relevant witnesses turn green; remaining failures persist. Both diagnostic changes are reverted. These are causal probes, not an authored repair.
- Verified the three production files' hashes match HEAD after restoration. Only test files are changed. No author branch edits, commits, pushes, native/live relay acceptance, or broad CI duplication. Portable tests below can be applied directly to this head.
CI separately: inspected once near review start: 3 success, 24 in progress, 2 skipped, none failed then. This is not a final-CI claim. Final CI and independent native/live verification remain separate gates.
Recommendation for Will: keep draft/off. Authorize a bounded end-to-end repair of the explicit creation identity/completion and saved-team-routing contracts plus the Goose launch capture, followed by independent checks; or explicitly accept/record these defects only as dormant infrastructure. Do not enable or call the feature complete. No further automatic repair iteration is authorized by this review.
Portable independent test-only witnesses (apply at 280968a56)
diff --git a/src/bundled/goose/runtime.test.ts b/src/bundled/goose/runtime.test.ts
index 7a39bc882..836a55eed 100644
--- a/src/bundled/goose/runtime.test.ts
+++ b/src/bundled/goose/runtime.test.ts
@@ -153,6 +153,7 @@ function setup(
} as unknown as Parameters<GooseRuntime["runOnce"]>[0]);
return {
agent,
+ host,
core,
deliver,
runOnce,
@@ -647,3 +648,120 @@ it("captures Goose choice with the launch snapshot before memory is awaited", as
f.runtime.dispose();
}
});
+
+it.each(["team", "name"] as const)(
+ "review witness: Goose fences a %s change while memory is awaited",
+ async (field) => {
+ const f = setup();
+ let release!: (value: string | null) => void;
+ const gate = new Promise<string | null>((done) => {
+ release = done;
+ });
+ f.core.mockImplementation(() => gate as never);
+ const before = {
+ ...DEFAULT_CONFIG,
+ instructions: "BEFORE INSTRUCTIONS",
+ model: "before-model",
+ };
+ const after = {
+ ...DEFAULT_CONFIG,
+ instructions: "AFTER INSTRUCTIONS",
+ model: "after-model",
+ };
+ const row = { ...f.agent, config: before, saves: 0 } as unknown as Agent;
+ f.runtime.sync([row]);
+ try {
+ await f.deliver(message("1", "first"), {
+ config: before,
+ team: "BEFORE TEAM",
+ });
+ await vi.waitFor(() => expect(f.core).toHaveBeenCalledTimes(1));
+ if (field === "name")
+ f.runtime.sync([{ ...row, name: "NEW NAME", config: after }]);
+ await f.deliver(message("2", "second"), {
+ config: after,
+ team: field === "team" ? "AFTER TEAM" : "BEFORE TEAM",
+ });
+ await vi.waitFor(() => expect(f.core).toHaveBeenCalledTimes(2));
+ release(null);
+ await vi.waitFor(() => expect(f.fake.geese[0]?.turns).toHaveLength(2));
+ const goose = f.fake.geese[0]!;
+ expect(goose.closed).toContain("session-1");
+ for (const turn of goose.turns) {
+ const prompt = goose.prompts.get(turn.sessionId);
+ expect(prompt).toContain("AFTER INSTRUCTIONS");
+ expect(prompt).not.toContain("BEFORE INSTRUCTIONS");
+ expect(prompt).toContain(field === "team" ? "AFTER TEAM" : "NEW NAME");
+ }
+ } finally {
+ release(null);
+ f.runtime.dispose();
+ }
+ },
+);
+
+it("review witness: a Goose team change and revert during launch never admits mixed inputs", async () => {
+ const f = setup();
+ let firstMemory!: (value: string | null) => void;
+ let otherMemory!: (value: string | null) => void;
+ let openServer!: () => void;
+ const firstGate = new Promise<string | null>((done) => {
+ firstMemory = done;
+ });
+ const otherGate = new Promise<string | null>((done) => {
+ otherMemory = done;
+ });
+ const serverGate = new Promise<void>((done) => {
+ openServer = done;
+ });
+ f.core
+ .mockImplementationOnce(() => firstGate as never)
+ .mockImplementation(() => otherGate as never);
+ const serve = f.host.serveMcp!.bind(f.host);
+ const serving = vi.fn(async (...args: Parameters<typeof serve>) => {
+ await serverGate;
+ return serve(...args);
+ });
+ f.host.serveMcp = serving;
+ const before = {
+ ...DEFAULT_CONFIG,
+ instructions: "BEFORE INSTRUCTIONS",
+ model: "before-model",
+ };
+ const after = {
+ ...DEFAULT_CONFIG,
+ instructions: "AFTER INSTRUCTIONS",
+ model: "after-model",
+ };
+ try {
+ await f.deliver(message("1", "first"), {
+ config: before,
+ team: "BEFORE TEAM",
+ });
+ await vi.waitFor(() => expect(f.core).toHaveBeenCalledTimes(1));
+ await f.deliver(message("2", "second"), {
+ config: after,
+ team: "AFTER TEAM",
+ });
+ await vi.waitFor(() => expect(f.core).toHaveBeenCalledTimes(2));
+ firstMemory(null);
+ await vi.waitFor(() => expect(serving).toHaveBeenCalledTimes(1));
+ await f.deliver(message("3", "third"), {
+ config: before,
+ team: "BEFORE TEAM",
+ });
+ await vi.waitFor(() => expect(f.core).toHaveBeenCalledTimes(3));
+ openServer();
+ await vi.waitFor(() => expect(f.fake.geese[0]?.turns).toHaveLength(1));
+ const goose = f.fake.geese[0]!;
+ const prompt = goose.prompts.get(goose.turns[0]!.sessionId);
+ expect.soft(prompt).toContain("BEFORE INSTRUCTIONS");
+ expect.soft(prompt).toContain("BEFORE TEAM");
+ expect.soft(prompt).not.toContain("AFTER TEAM");
+ } finally {
+ firstMemory(null);
+ otherMemory(null);
+ openServer();
+ f.runtime.dispose();
+ }
+});
diff --git a/src/features/agents/service.test.ts b/src/features/agents/service.test.ts
index d0a27ef95..3b12ba91d 100644
--- a/src/features/agents/service.test.ts
+++ b/src/features/agents/service.test.ts
@@ -6390,6 +6390,236 @@ describe("Delete as a recoverable archive", () => {
}
});
+ it("review witness: retry of a partial Create keeps its team id", async () => {
+ const relay = configRelay();
+ communityFake.value = relay.opens;
+ const f = await setup({ identities: [here(alder)] });
+ try {
+ await vi.waitFor(() =>
+ expect(f.service.find(alder)?.configBase).toBeDefined(),
+ );
+ const add = relay.addMember.getMockImplementation()!;
+ relay.addMember.mockImplementation(async (id, pubkey) =>
+ [...relay.channels.values()].some((c) => c.id === id)
+ ? ({ status: "uncertain" } as never)
+ : add(id, pubkey),
+ );
+ const form = { name: "Partial", instructions: "RULE", members: [alder] };
+ await expect(f.service.saveTeam(form)).rejects.toThrow();
+ const [started] = new TeamJournal(f.storage).all();
+ expect(started?.pendingMembers).toEqual([alder]);
+ relay.addMember.mockImplementation(add);
+ const completed = await f.service.saveTeam(form);
+ expect(completed.id).toBe(started?.teamId);
+ expect(relay.channels.size).toBe(1);
+ expect(new TeamJournal(f.storage).all()[0]?.pendingMembers).toEqual([]);
+ } finally {
+ await f.ctx.fiber.dispose();
+ }
+ });
+
+ it.each(["name", "instructions"] as const)(
+ "review witness: a partial Create retry after changing %s does not duplicate",
+ async (field) => {
+ const relay = configRelay();
+ communityFake.value = relay.opens;
+ const f = await setup({ identities: [here(alder)] });
+ try {
+ await vi.waitFor(() =>
+ expect(f.service.find(alder)?.configBase).toBeDefined(),
+ );
+ const add = relay.addMember.getMockImplementation()!;
+ relay.addMember.mockImplementation(async (id, pubkey) =>
+ [...relay.channels.values()].some((c) => c.id === id)
+ ? ({ status: "uncertain" } as never)
+ : add(id, pubkey),
+ );
+ const form = {
+ name: "Partial",
+ instructions: "RULE",
+ members: [alder],
+ };
+ await expect(f.service.saveTeam(form)).rejects.toThrow();
+ const [started] = new TeamJournal(f.storage).all();
+ relay.addMember.mockImplementation(add);
+ const completed = await f.service.saveTeam({
+ ...form,
+ [field]: "Changed",
+ });
+ expect.soft(completed.id).toBe(started?.teamId);
+ expect.soft(relay.channels.size).toBe(1);
+ expect.soft(new TeamJournal(f.storage).all()).toHaveLength(1);
+ } finally {
+ await f.ctx.fiber.dispose();
+ }
+ },
+ );
+
+ it("review witness: Create rejects a member operation that throws before it can be journaled pending", async () => {
+ const relay = configRelay();
+ communityFake.value = relay.opens;
+ const f = await setup({ identities: [here(alder)] });
+ try {
+ await vi.waitFor(() =>
+ expect(f.service.find(alder)?.configBase).toBeDefined(),
+ );
+ const add = relay.addMember.getMockImplementation()!;
+ relay.addMember.mockImplementation(async (id, pubkey) => {
+ if ([...relay.channels.values()].some((c) => c.id === id))
+ throw new Error("Disconnected during ownership read");
+ return add(id, pubkey);
+ });
+ await expect(
+ f.service.saveTeam({
+ name: "Throwing",
+ instructions: "RULE",
+ members: [alder],
+ }),
+ ).rejects.toThrow();
+ } finally {
+ await f.ctx.fiber.dispose();
+ }
+ });
+
+ it("review witness: a team discovered on another device refuses offline Delete and edit without a migration entry", async () => {
+ const relay = configRelay();
+ communityFake.value = relay.opens;
+ const laptop = await setup({ identities: [here(alder)] });
+ let team: Awaited<ReturnType<typeof laptop.service.saveTeam>>;
+ try {
+ await vi.waitFor(() =>
+ expect(laptop.service.find(alder)?.configBase).toBeDefined(),
+ );
+ team = await laptop.service.saveTeam({
+ name: "Remote",
+ instructions: "RULE",
+ members: [alder],
+ });
+ } finally {
+ await laptop.ctx.fiber.dispose();
+ }
+ const other = await setup({
+ identities: [here(alder)],
+ storage: memoryStorage(),
+ });
+ try {
+ await vi.waitFor(() =>
+ expect(other.service.snapshot().teams).toContainEqual(
+ expect.objectContaining({ id: team.id }),
+ ),
+ );
+ expect(new TeamJournal(other.storage).all()).toHaveLength(0);
+ expect(
+ new TeamJournal(other.storage).known(
+ viewer,
+ "https://relay.example.test",
+ ),
+ ).toContainEqual(expect.objectContaining({ id: team.id }));
+ communityFake.value = null;
+ other.reconnecting();
+ other.connect(true);
+ await settle();
+ await expect
+ .soft(other.service.removeTeam(team.id))
+ .rejects.toThrow("isn't available");
+ await expect
+ .soft(other.service.saveTeam({ ...team, name: "Changed" }))
+ .rejects.toThrow();
+ } finally {
+ await other.ctx.fiber.dispose();
+ }
+ });
+
+ it("review witness: an unpublished local migration does not prevent editing or deleting the local team", async () => {
+ const relay = configRelay();
+ relay.offline.value = true;
+ communityFake.value = relay.opens;
+ const storage = memoryStorage();
+ const local = {
+ id: "local-before-sync",
+ name: "Local",
+ instructions: "RULE",
+ members: [alder],
+ };
+ writeTeams(storage, [local]);
+ const f = await setup({ identities: [here(alder)], storage });
+ try {
+ await vi.waitFor(() =>
+ expect(new TeamJournal(storage).all()).toHaveLength(1),
+ );
+ const [migration] = new TeamJournal(storage).all();
+ expect(migration?.teamId).not.toBe(local.id);
+ communityFake.value = null;
+ f.reconnecting();
+ f.connect(true);
+ await settle();
+ await expect(
+ f.service.saveTeam({ ...local, name: "Changed" }),
+ ).resolves.toMatchObject({ id: local.id, name: "Changed" });
+ await expect(f.service.removeTeam(local.id)).resolves.toBeUndefined();
+ expect(readTeams(storage)).toEqual([]);
+ } finally {
+ await f.ctx.fiber.dispose();
+ }
+ });
+
+ it("review witness: Save again after background completion retains the partial Create id", async () => {
+ const relay = configRelay();
+ communityFake.value = relay.opens;
+ const f = await setup({ identities: [here(alder)] });
+ try {
+ await vi.waitFor(() =>
+ expect(f.service.find(alder)?.configBase).toBeDefined(),
+ );
+ const add = relay.addMember.getMockImplementation()!;
+ relay.addMember.mockImplementation(async (id, pubkey) =>
+ [...relay.channels.values()].some((c) => c.id === id)
+ ? ({ status: "uncertain" } as never)
+ : add(id, pubkey),
+ );
+ const form = { name: "Partial", instructions: "RULE", members: [alder] };
+ await expect(f.service.saveTeam(form)).rejects.toThrow();
+ const [started] = new TeamJournal(f.storage).all();
+ relay.addMember.mockImplementation(add);
+ await relay.opens.mock.results[0]!.value.resumeTeams();
+ expect(new TeamJournal(f.storage).all()[0]?.addedMembers).toEqual([
+ alder,
+ ]);
+ const completed = await f.service.saveTeam(form);
+ expect.soft(completed.id).toBe(started?.teamId);
+ expect.soft(relay.channels.size).toBe(1);
+ } finally {
+ await f.ctx.fiber.dispose();
+ }
+ });
+
+ it("review witness: user Import reports unconfirmed team membership", async () => {
+ const relay = configRelay();
+ communityFake.value = relay.opens;
+ const f = await setup({ identities: [here(alder)] });
+ try {
+ await vi.waitFor(() =>
+ expect(f.service.find(alder)?.configBase).toBeDefined(),
+ );
+ const add = relay.addMember.getMockImplementation()!;
+ relay.addMember.mockImplementation(async (id, pubkey) =>
+ [...relay.channels.values()].some((c) => c.id === id)
+ ? ({ status: "uncertain" } as never)
+ : add(id, pubkey),
+ );
+ listsTeam(f.native, nightShift([alder]));
+ const imported = await importTeam(f.service);
+ expect.soft(imported.team).toBeUndefined();
+ expect
+ .soft(
+ imported.teamError || imported.notes.length || imported.failed.length,
+ )
+ .toBeTruthy();
+ } finally {
+ await f.ctx.fiber.dispose();
+ }
+ });
+
it("Create does not report unconfirmed team membership as success", async () => {
const relay = configRelay();
communityFake.value = relay.opens;
wpfleger96
left a comment
There was a problem hiding this comment.
Limited independent repair check: two IMPORTANT retry defects remain
Verdict: keep draft and config sync disabled. Reviewed the two-commit repair diff 280968a56..4f9fd8e909329f5a7153c686c29883455513d030 against the four previously reported gaps. Origin's PR head was verified as 4f9fd8e909329f5a7153c686c29883455513d030 before review and again after validation. This is the last authorized check, not permission for another automatic repair round. Return the decision to Will; my recommendation is to split team writes into a separately planned PR while retaining the agent sync and Goose work, subject to rechecking the resulting boundaries.
Quality: minimalism 9, elegance 9, correctness 7. The repairs are small, reuse the existing journal/write owners, and have a clear form-lifetime identity. Correctness remains below the bar because current-form reconciliation can both become impossible and falsely report success. The finding set below is scoped to these repairs, not a new full-feature review.
IMPORTANT — Correctness: removing a refused member cannot finish the same Create
Source: src/features/agents/team-config.ts:215-243; src/features/agents/community-config.ts:630-655.
publishTeam unions the earlier intended members with every new form submission. createTeam then requires that entire historical union to be confirmed before it reaches the reconcile edit that could remove a member. The reported “extra add then remove” is therefore not generally harmless.
Independent reproduced case: Create with Alder and Birch; Birch's add returns a definite ownership refusal while Alder is confirmed. In the still-open form, remove Birch and press Save again. Expected: finish the existing team with Alder. Actual: Save continues throwing “some members weren't confirmed” because Birch remains intended forever, even though Birch is no longer requested. Exactly one channel exists, but the current form cannot complete. The same ordering also unnecessarily admits a removed member before removing them when their add does succeed; that intermediate membership is real, not an atomic form change.
Corrective action: retain the stable creation id but reconcile the new team's pending intent against the current form before demanding completeness. A removed, never-confirmed member must not remain a prerequisite; a member actually present must be removed through the confirmed membership path. Keep migration/import union semantics separate if those need to preserve accumulated membership.
Failing test: independent witness: retry can remove the member whose add remains refused.
IMPORTANT — Correctness: reverting the form after an uncertain reconcile returns false success
Source: src/features/agents/community-config.ts:648-662; the old journal body is retained at src/features/agents/team-config.ts:215-241.
changed compares the current form with the migration's original entry.team, not the current confirmed team artifact. That original journal body is not advanced by reconcile edits. Once an attempted reconcile changes the relay team, a later form matching the original journal contents bypasses editTeam completely.
Independent reproduced case: an initial partial Create has name Original. Retry with Intervening name; the relay accepts that revision, but its publish reply is lost. The save correctly rejects as unconfirmed and keeps the form open. Restore the form name to Original and press Save again. Actual: Save resolves successfully with Original in the returned object, while the relay artifact still says Intervening name. Discovery has already read that intervening head, so this is not a cache-delay assertion. Instructions have the same faulty comparison.
Corrective action: decide reconciliation against the current authoritative team body and membership, not the historical migration snapshot; unresolved discovery must not skip confirmation. The existing editTeam owner already avoids publishing identical content, so delegating a resumed Create to that owner is simpler than maintaining this second content comparison. Preserve conflict/unconfirmed outcomes and the same creation id.
Failing test: independent witness: reverting the form after an uncertain reconcile applies the current name (expected relay head Original, received Intervening name).
Answers to the requested checks
1. Intended-versus-confirmed completeness and the thrown-add catch: keep both
They overlap for the single thrown-add Create witness, but are not redundant:
- The local
.catchinaddMemberspreserves a durable pending member, continues later members, and permits background local migration to retire its row only after each member is added or durably pending. My two-member witness throws for the first add and confirms the second: pending[first], added[second], andretired()true. Removing only that catch fails this witness even withteamCompleteintact. - The completeness predicate at Create rejects an incomplete confirmed journal whose pending list is empty, including entries written by the previous thrown-add path. My service witness starts with such an entry, makes the first team bind fail before the add loop, lets the following discovery bind succeed, and asserts Create rejects. Replacing only Create's
teamCompletecheck withpendingMembers.lengthfalsely resolves that save even with the add catch intact. This is persisted-state compatibility, not another thrown-add double mutation.
Coverage request for the next authorized change: add both single-guard witnesses as ordinary regressions. The current thrown-add test checks only rejection; it does not prove pending persistence, later-member progress or legacy-entry completeness.
Import as a user action: passes my stronger service witness. With an earlier local imported copy and one unconfirmed membership, Import returns no team, returns teamError, includes the exact missing member name in failed, and keeps the local storage row. Repeating Import after confirmation returns the team and removes that row. The existing teamPortNote renders these failed-member names and keeps Import open (src/features/agents/service.ts:1752-1781; src/bundled/agents/AgentsPage.tsx:964-1000). Background migration remains deliberately different: retired() permits a durably pending member. I did not treat it as a user save.
2. Stable Create identity, reconciliation and discovery
- Independently mounted the actual TeamEditor through AgentsPage: a failed Create followed by name/instructions edits sends the same
creationvalue on both saves. ReactuseStateandkey={editing}preserve it for that open form, without changing normal existing-team ids. - Independent service witnesses verify the actual relay artifact's changed name and the same team/channel, both with and without background completion first. The unchanged-form/background-completion witness also passes. These check persisted head content, not just the returned form object and channel count.
- Independent accepted-write/lost-reply witness confirms reconcile reports uncertainty instead of success. The subsequent form-reversion boundary is the second IMPORTANT finding above.
- Lagging discovery re-adding an already-present member: duplicate addition itself is harmless in the production channel adapter, not just in the fake.
addMemberfirst verifies ownership andadministerrefreshes the signed roster; an existing role returnsconfirmedwithout sending another mutation (src/features/agents/config-channels.ts:199-231; src/features/agents/config-channels.ts:282-294). An unavailable ownership/roster read can still reject the save, honestly. I did not exercise a real relay here. - Removing a member before retry: not harmless, as reproduced above. Do not accept the listed limitation as merely redundant traffic.
3. Offline refusal and owner/community scope
The two-device discovery-only offline Delete/edit witness passes, as does the ordinary unpublished-local migration edit/delete witness. My added test seeds remembered teams for another owner and another community and proves unrelated local edits/deletes still work.
The broad scan does not authorize a write or choose another owner's config; it only refuses local fallback when the exact id is known as synced. New local teams use random UUIDs; imported synced ids derive from owner + community + source + source id (src/features/agents/config-channels.ts:71-85). No normal generated-id collision was found in these paths. An explicitly reused identical id or corrupt persisted record can conservatively refuse a local row; the predicate does not distinguish that artificial collision. I found no realistic wrong-owner write introduced by this repair. This assessment is scoped to the reviewed constructors and routing, not a claim that arbitrary persisted ids can never collide.
4. Goose startup snapshot
Passes the scoped source check and existing controlled change/revert tests. src/bundled/goose/runtime.ts:563-579 captures config, team, name together before core memory. Launch inputs are:
- captured config:
workspace,instructions,scope,provider,model,effort; - captured
team, capturedname, fixed closurepubkey; - the result of
this.core(pubkey); prompt module constants are immutable imports.
systemPrompt consumes exactly scope, cwd, optional agent name/pubkey, instructions, team and memory (src/bundled/claude-code/prompt.ts:18-49). choiceOf(config) consumes provider/model/effort (src/bundled/goose/runtime.ts:622-630). No entry read occurs after the await. Precision correction to the report: choiceOf(config) does read those fields after the await, but from the captured config object, not from entry; the reviewed runtime replaces config objects rather than mutating them. The comment should mean “all mutable entry inputs captured before await,” not literally “no field is read later.” No additional Goose blocker found.
5. MINOR — Elegance: misplaced known() documentation
Confirmed: src/features/agents/team-config.ts:98-111 places the known(owner, community) comment immediately above the new synced(id) documentation. Move that existing comment back to known. Optional; does not block.
Evidence and limits
- Exact production SHA:
4f9fd8e909329f5a7153c686c29883455513d030, verified in the same shell as each run. Only independent tests/fake controls were retained in the disposable review worktree; every diagnostic production mutation was restored and compared with that SHA. No edits to the author's branch, commits or pushes. - Final bounded full affected-suite command:
bin/pnpm exec vitest run src/features/agents src/bundled/agents src/bundled/goose --maxWorkers=2: 890 passed, 2 failed, 6 skipped, across 65 files. Both failures are the new retry witnesses above; all shipped tests passed. No scoped test-name run, lint, typecheck or build was used to duplicate CI. This local run answers the specifically requested independent witness questions. - Earlier full run before the final UI test: 889 passed, 2 failed, 6 skipped. Separate one-at-a-time guard mutations each introduced exactly its expected extra failure: 888 passed, 3 failed, 6 skipped. Both mutations were restored. These mutation runs preceded the extra owner/community/UI coverage where applicable; do not conflate their test counts with the final run.
- An intervening default-concurrency run also timed out in existing
settles already enqueued channel removals when a later enqueue throwsandcatches up no faster than its Interest's lane takes turns, so none is dropped(5s each). Load immediately afterward: 46.39 / 33.93 / 23.81. These two passed in the bounded full rerun with no timeout/assertion changes; I am not attributing those timeouts to this diff. - CI inspected once near review start, separately: 3 success, 24 in progress, 2 skipped, none failed then at this head. Final hosted CI remains for the loop owner; no polling/waiting.
- No independent native app, real relay or external Goose acceptance was performed. This is a source/service/runtime-fake repair check, not live acceptance or permission to enable the feature. PR remained draft and
CONFIG_SYNC_ENABLED = falseat the inspected ref.
Decision for Will
- Recommended: split team Create/Import/lifecycle work into a separately planned PR with a clear current-intent reconciliation rule; retain the passing agent/Goose work, then review the resulting scope.
- Explicitly authorize another bounded repair only for these two shared retry causes, returning their tests to independent review and appropriate live verification. This would be new authorization, not continuation of the exhausted budget.
- Accept the defects only as documented dormant infrastructure while the feature remains off; this does not meet the correctness bar or authorize activation.
Portable independent test-only patch
Apply to 4f9fd8e909329f5a7153c686c29883455513d030 with git apply in a disposable worktree, then use the full affected-suite command above. No production fix is included. Fixture additions model an accepted team revision with a lost publish reply and a one-shot team bind failure.
diff --git a/src/bundled/agents/AgentsPage.test.tsx b/src/bundled/agents/AgentsPage.test.tsx
index 0e41d4857..fcc5e18cc 100644
--- a/src/bundled/agents/AgentsPage.test.tsx
+++ b/src/bundled/agents/AgentsPage.test.tsx
@@ -1359,6 +1359,29 @@ it("makes a team of chosen agents and lists it with its members", async () => {
expect(screen.queryByRole("list", { name: "Teams" })).not.toBeInTheDocument();
});
+
+it("independent witness: a failed Create form retains its creation id across edits", async () => {
+ const user = userEvent.setup();
+ const ada = agent("Ada", 1);
+ const { agents } = setup([ada]);
+ agents.saveTeam.mockRejectedValueOnce(new Error("Member not confirmed"));
+ await user.click(screen.getByRole("tab", { name: "Your teams" }));
+ await user.click(screen.getByRole("button", { name: "New team" }));
+ await user.click(await screen.findByRole("menuitem", { name: "Create a new team" }));
+ await user.type(screen.getByRole("textbox", { name: "Name" }), "Before");
+ await user.click(screen.getByRole("checkbox", { name: "Ada" }));
+ await user.click(screen.getByRole("button", { name: "Create team" }));
+ await screen.findByText("Member not confirmed");
+ const first = agents.saveTeam.mock.calls[0]?.[0];
+ await user.clear(screen.getByRole("textbox", { name: "Name" }));
+ await user.type(screen.getByRole("textbox", { name: "Name" }), "After");
+ await user.type(screen.getByRole("textbox", { name: "Instructions" }), "Current instructions");
+ await user.click(screen.getByRole("button", { name: "Create team" }));
+ const second = agents.saveTeam.mock.calls[1]?.[0];
+ expect(first?.creation).toEqual(expect.any(String));
+ expect(second).toMatchObject({ creation: first?.creation, name: "After", instructions: "Current instructions", members: [ada.pubkey] });
+});
+
it("closes the team editor when leaving Your teams", async () => {
const user = userEvent.setup();
const { agents } = setup([agent("Ada", 1)]);
diff --git a/src/features/agents/service.test.ts b/src/features/agents/service.test.ts
index 5c25d506c..969ff1bfb 100644
--- a/src/features/agents/service.test.ts
+++ b/src/features/agents/service.test.ts
@@ -6000,6 +6000,13 @@ describe("Delete as a recoverable archive", () => {
};
/** While set, no config channel can be made. */
const offline = { value: false };
+ const witnessFaults = { loseTeamUpdateReply: false, bindTeam: false };
+ const bind = vi.fn(async (bindings: { family: string }[]) => {
+ if (witnessFaults.bindTeam && bindings.some((b) => b.family === "team-config")) {
+ witnessFaults.bindTeam = false;
+ throw new Error("Disconnected while binding the team");
+ }
+ });
const addMember = vi.fn(async (id: string, pubkey: string) => {
const found = [...channels.values()].find((c) => c.id === id);
if (found && !found.members.includes(pubkey)) found.members.push(pubkey);
@@ -6044,7 +6051,7 @@ describe("Delete as a recoverable archive", () => {
},
teamJournal: input.journals.teams,
lifecycle: input.journals.lifecycle,
- sync: { bind: vi.fn(async () => {}), unbind: vi.fn(), dispose() {} },
+ sync: { bind, unbind: vi.fn(), dispose() {} },
deps: {
read: async (filters) =>
[...heads.values()].filter((event) =>
@@ -6058,6 +6065,8 @@ describe("Delete as a recoverable archive", () => {
const head = heads.get(key(event));
const prev = event.tags.find(([name]) => name === "prev")?.[1];
if (!head || prev === head.id) heads.set(key(event), event);
+ if (witnessFaults.loseTeamUpdateReply && event.tags.some(([k, v]) => k === "type" && v === "buzz.team") && prev)
+ throw new Error("Lost reply after accepting the team update");
},
},
store: input.store,
@@ -6072,7 +6081,7 @@ describe("Delete as a recoverable archive", () => {
changed: input.changed,
}),
);
- return { opens, heads, addMember, channels, offline };
+ return { opens, heads, addMember, channels, offline, witnessFaults, bind };
}
const mentionAlder = (f: Awaited<ReturnType<typeof setup>>, id: string) =>
f.emit({
@@ -6395,6 +6404,156 @@ describe("Delete as a recoverable archive", () => {
}
});
+
+ async function independentPartial() {
+ const relay = configRelay();
+ communityFake.value = relay.opens;
+ const f = await setup({ identities: [here(alder), here(birch)] });
+ await vi.waitFor(() => expect(f.service.find(alder)?.configBase).toBeDefined());
+ await vi.waitFor(() => expect(f.service.find(birch)?.configBase).toBeDefined());
+ await relay.opens.mock.results[0]?.value.resumeTeams();
+ const add = relay.addMember.getMockImplementation();
+ if (!add) throw new Error("No add implementation");
+ relay.addMember.mockImplementation(async (id, pubkey) =>
+ [...relay.channels.values()].some((c) => c.id === id) && pubkey === birch
+ ? ({ status: "failed", error: "Not an agent you own" } as never)
+ : add(id, pubkey),
+ );
+ const form = { creation: crypto.randomUUID(), name: "Original", instructions: "ORIGINAL RULE", members: [alder, birch] };
+ await expect(f.service.saveTeam(form)).rejects.toThrow("members weren't confirmed");
+ return { relay, f, add, form };
+ }
+
+ it.each([false, true])("independent witness: changed name reaches the same relay team after background completion %s", async (background) => {
+ const { relay, f, add, form } = await independentPartial();
+ try {
+ relay.addMember.mockImplementation(add);
+ if (background) await relay.opens.mock.results[0]?.value.resumeTeams();
+ const saved = await f.service.saveTeam({ ...form, name: "Current name" });
+ expect(saved.id).toBe(form.creation);
+ expect(relay.channels.size).toBe(1);
+ const head = [...relay.heads.values()].find((h) => h.tags.some(([k, v]) => k === "type" && v === "buzz.team"));
+ expect(JSON.parse(head?.content ?? "{}")).toMatchObject({ name: "Current name", instructions: form.instructions });
+ expect([...relay.channels.values()][0]?.members).toEqual(expect.arrayContaining([alder, birch]));
+ expect(f.service.snapshot().teams).toContainEqual(expect.objectContaining({ id: form.creation, name: "Current name" }));
+ } finally { await f.ctx.fiber.dispose(); }
+ });
+
+ it("independent witness: unchanged retry after background completion keeps the relay team", async () => {
+ const { relay, f, add, form } = await independentPartial();
+ try {
+ relay.addMember.mockImplementation(add);
+ await relay.opens.mock.results[0]?.value.resumeTeams();
+ const saved = await f.service.saveTeam(form);
+ expect(saved.id).toBe(form.creation);
+ expect(relay.channels.size).toBe(1);
+ expect(new TeamJournal(f.storage).all()).toHaveLength(1);
+ } finally { await f.ctx.fiber.dispose(); }
+ });
+
+ it("independent witness: reconcile refuses a lost reply even when the relay accepted the edit", async () => {
+ const { relay, f, add, form } = await independentPartial();
+ try {
+ relay.addMember.mockImplementation(add);
+ relay.witnessFaults.loseTeamUpdateReply = true;
+ await expect(f.service.saveTeam({ ...form, name: "Changed" })).rejects.toThrow("latest changes weren't confirmed");
+ expect(relay.channels.size).toBe(1);
+ const head = [...relay.heads.values()].find((h) => h.tags.some(([k, v]) => k === "type" && v === "buzz.team"));
+ expect(JSON.parse(head?.content ?? "{}").name).toBe("Changed");
+ } finally { await f.ctx.fiber.dispose(); }
+ });
+
+ it("independent witness: retry can remove the member whose add remains refused", async () => {
+ const { relay, f, form } = await independentPartial();
+ try {
+ const result = await f.service.saveTeam({ ...form, members: [alder] });
+ expect(result.id).toBe(form.creation);
+ expect(relay.channels.size).toBe(1);
+ expect([...relay.channels.values()][0]?.members).not.toContain(birch);
+ expect(f.service.snapshot().teams).toContainEqual(expect.objectContaining({ id: form.creation, members: [alder] }));
+ } finally { await f.ctx.fiber.dispose(); }
+ });
+
+ it("independent witness: reverting the form after an uncertain reconcile applies the current name", async () => {
+ const { relay, f, add, form } = await independentPartial();
+ try {
+ relay.addMember.mockImplementation(add);
+ relay.witnessFaults.loseTeamUpdateReply = true;
+ await expect(f.service.saveTeam({ ...form, name: "Intervening name" })).rejects.toThrow("latest changes weren't confirmed");
+ relay.witnessFaults.loseTeamUpdateReply = false;
+ const completed = await f.service.saveTeam(form);
+ expect(completed.id).toBe(form.creation);
+ expect(relay.channels.size).toBe(1);
+ const head = [...relay.heads.values()].find((h) => h.tags.some(([k, v]) => k === "type" && v === "buzz.team"));
+ expect(JSON.parse(head?.content ?? "{}").name).toBe(form.name);
+ } finally { await f.ctx.fiber.dispose(); }
+ });
+
+ it("independent witness: a legacy confirmed entry without pending members still cannot claim success after a bind throw", async () => {
+ const { relay, f, form } = await independentPartial();
+ try {
+ const input = relay.opens.mock.calls[0]?.[0];
+ const prior = input?.journals.teams.all()[0];
+ if (!prior || !input) throw new Error("No journal entry");
+ input.journals.teams.put({ ...prior, pendingMembers: [] });
+ relay.witnessFaults.bindTeam = true;
+ await expect(f.service.saveTeam(form)).rejects.toThrow("members weren't confirmed");
+ expect(new TeamJournal(f.storage).all()[0]?.pendingMembers).toEqual([]);
+ } finally { await f.ctx.fiber.dispose(); }
+ });
+
+ it("independent witness: Import names missing members and retains an earlier local copy", async () => {
+ const relay = configRelay();
+ communityFake.value = null;
+ const storage = memoryStorage();
+ const local = savedShift([alder, birch]);
+ writeTeams(storage, [local]);
+ const f = await setup({ storage, identities: [here(alder), here(birch)] });
+ try {
+ listsTeam(f.native, nightShift([alder, birch]));
+ relay.offline.value = true;
+ communityFake.value = relay.opens;
+ f.reconnecting(); f.connect(true);
+ await vi.waitFor(() => expect(new TeamJournal(storage).all()).toHaveLength(1));
+ await relay.opens.mock.results[0]?.value.migrateTeams([], () => undefined, () => {});
+ expect(readTeams(storage)).toEqual([local]);
+ const add = relay.addMember.getMockImplementation();
+ if (!add) throw new Error("No add implementation");
+ relay.addMember.mockImplementation(async (id, pubkey) =>
+ [...relay.channels.values()].some((c) => c.id === id) && pubkey === birch
+ ? ({ status: "uncertain" } as never) : add(id, pubkey));
+ relay.offline.value = false;
+ const result = await importTeam(f.service);
+ expect(result.team).toBeUndefined();
+ expect(result.teamError).toEqual({ name: "Night Shift" });
+ expect(result.failed).toContainEqual({ name: "Birch", error: "Not confirmed on the team; import it again" });
+ expect(readTeams(storage)).toEqual([local]);
+ relay.addMember.mockImplementation(add);
+ const done = await importTeam(f.service);
+ expect(done.team?.members).toEqual(expect.arrayContaining([alder, birch]));
+ expect(readTeams(storage)).toEqual([]);
+ } finally { await f.ctx.fiber.dispose(); }
+ });
+
+
+ it("independent witness: remembered other-owner and other-community teams do not refuse unrelated local writes", async () => {
+ const storage = memoryStorage();
+ const local = { id: crypto.randomUUID(), name: "Local", instructions: "LOCAL", members: [alder] };
+ writeTeams(storage, [local]);
+ const journal = new TeamJournal(storage);
+ journal.remember(other, "https://different.example.test", [{
+ id: crypto.randomUUID(), channel: crypto.randomUUID(), archived: false, unreadable: false, members: [birch],
+ subject: { owner: other, community: "https://different.example.test", subject: "unused", channel: "unused", type: "buzz.team" },
+ }]);
+ communityFake.value = null;
+ const f = await setup({ storage, identities: [here(alder)] });
+ try {
+ await expect(f.service.saveTeam({ ...local, name: "Edited" })).resolves.toMatchObject({ id: local.id, name: "Edited" });
+ await expect(f.service.removeTeam(local.id)).resolves.toBeUndefined();
+ expect(readTeams(storage)).toEqual([]);
+ } finally { await f.ctx.fiber.dispose(); }
+ });
+
it("retry of a partial Create keeps its team id", async () => {
const relay = configRelay();
communityFake.value = relay.opens;
diff --git a/src/features/agents/team-config.test.ts b/src/features/agents/team-config.test.ts
index 20d2225fd..c009c87b9 100644
--- a/src/features/agents/team-config.test.ts
+++ b/src/features/agents/team-config.test.ts
@@ -101,6 +101,18 @@ function setup(
const source = { kind: "local-team", id: "beta-abc" } as const;
describe("team migration", () => {
+ it("independent witness: a thrown add stays pending while later members continue and the local move is safely retired", async () => {
+ const f = setup();
+ f.channels.addMember.mockImplementation((async (_channel: string, pubkey: string) => {
+ if (pubkey === a) throw new Error("Ownership read disconnected");
+ return { status: "confirmed" };
+ }) as never);
+ const result = await publishTeam(f.deps, source, team);
+ expect(result).toMatchObject({ status: "confirmed", pendingMembers: [a], addedMembers: [b] });
+ expect(retired(result as never)).toBe(true);
+ expect(new TeamJournal(f.storage).all()[0]).toMatchObject({ pendingMembers: [a], addedMembers: [b] });
+ });
+
it("publishes a local team under its UUIDv5 import id, journaled before any write", async () => {
const f = setup();
const entry = await publishTeam(f.deps, source, team);
0f11be2 to
12ecf5e
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) lanes.
Reviewed head 0e7de8113fee008b45ff5981f6476e68a463424d against base f91f77c. Overall source assessment: 8/10. Five P2 findings are inline: two local-save runtime races apply even with config sync OFF; the other three require enabling sync. Two optional P3 UI improvements are also inline. The concrete repairs are local-save generation fencing, checking the effective pinned effort, a fresh retry barrier generation, and per-subject save/replay serialization.
The deliberate OFF flag and read-only relay-team scope are preserved; this review does not ask to enable sync or restore team writes here. Active sessions need not be interrupted by archive under NIP-AP. Public description (no attachments), changed source/tests and commit messages were inspected; no new disclosure finding was identified.
Source-only review: no PR code, tests, builds or app were executed. One current-head check snapshot showed 30 successful checks and 2 skipped checks (Windows native validation and browser flake lane). Native/cross-device behavior, keyboard/screen-reader behavior, runtime timing/test cost and human acceptance remain unverified. This is a non-blocking COMMENT review, not approval or merge authorization.
0e7de81 to
94c9957
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source follow-up via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) reviews.
Reviewed head 94c9957 against base 6166034, comparing the repairs and rebase integration with previously reviewed 0e7de8113fee008b45ff5981f6476e68a463424d. Assessment: 8/10, with two P2 findings and one optional P3 inline.
The relay retry-generation and save/resend serialization repairs look correct, as do the History labels, Recover focus, ordinary Goose opening-choice propagation and Codex pinned-effort selection. The Codex restart fix still has an earlier capture window; the service-to-Goose trace also establishes a material local-save integration defect when sync is enabled. Preserve CONFIG_SYNC_ENABLED=false and the deliberate read-only relay-team scope.
Public description (no attached images), repair source and commit messages inspected; no new disclosure finding identified. Source-only: no PR code, tests, builds or app executed. One current-head CI snapshot showed 30 successful and 2 skipped checks (Windows native validation and browser flake lane). Native/cross-device behavior, live keyboard/focus behavior, runtime performance, comparative test cost and human acceptance remain unverified. This COMMENT review is non-blocking and does not approve merging.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source follow-up via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) reviews.
Reviewed head 8a8c407 against base 6166034, with follow-up scope from 94c9957. Assessment: 8/10. Two residual P2s and one optional P3 are inline. The earlier blockers are narrowed but not fully resolved: Codex still has a pre-capture authorization window, and accepted-save notification does not cover reconnect replay.
The direct accepted-Save path, Goose’s multiple saves during opening, persisted Codex synced-effort validation, and deferred Recover focus look repaired in source. Preserve CONFIG_SYNC_ENABLED=false and the deliberate read-only relay-team scope.
Public description (no attachments), the follow-up source/tests and all five follow-up commit messages were inspected; no new disclosure finding identified. Source-only: no PR code, tests, builds or app executed. The single current-head CI snapshot was incomplete (16 successful, 13 in progress, 2 skipped). Native/cross-device behavior, live keyboard/screen-reader behavior, rendered themes/layouts, runtime performance, comparative test cost and human acceptance remain unverified. This non-blocking COMMENT review is not approval or merge authorization.
There was a problem hiding this comment.
🤖 Findings are now posted inline in review #1226 (review). Approval remains withheld.
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Two residual findings are recorded inline. Approval is withheld pending their resolution.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source follow-up via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) coverage.
Reviewed head 3ed5f7f20d88af3eb8e81ecd36c6b3a79f3c15ad against base 6166034b30a7d3561cc9e8982fcb4d4fb8aab381; follow-up comparison from 8a8c4070753a670ebca521677cf13c5a94cb605c covers nine changed files and supported callers.
8/10: two remaining P2s and one optional P3, detailed inline. The await inside Codex runtime.run is fenced, but the production codexRun readiness await still precedes capture. Ordinary initial and reconnect acceptance now notify runtimes, but the newer-draft branch still misses that notification. Recovery focus ownership and delayed completion announcements are repaired in source; the toast note is optional. Config sync remains disabled by default, so the accepted-save P2 concerns the enabled-sync contract; the Codex readiness issue also affects ordinary local config saves.
Public PR description (no attached images) and the four follow-up commit messages were inspected; no disclosure finding in that material. The nine-file follow-up retains existing owners; new regression cases use existing unit/integration fixtures, with no browser journey or CI configuration changes.
Validation: source inspection and clean full/follow-up diff checks only; no PR code, tests, builds or app execution. One hosted-check snapshot had 6 successful, 23 in progress and 2 skipped checks; this is not final CI evidence. Native/cross-device runtime behavior, real browser focus, screen-reader announcements, themes/narrow layouts, performance, comparative test cost and human acceptance remain unverified. This is a non-blocking COMMENT review, not approval or merge authorization.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source follow-up via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) reviews.
Reviewed head 75ef7e75fb5e9cbc2d432a11df7dbe04517aa133 against base 6166034b30a7d3561cc9e8982fcb4d4fb8aab381; this follow-up covers the seven-file repair since 3ed5f7f20d88af3eb8e81ecd36c6b3a79f3c15ad and its supported callers.
9/10 for the reviewed repair: no actionable findings. Both P2s from the previous review are repaired in source:
- Codex captures the restart generation in the registered
codexRunwrapper before installation/provider readiness waits, then carries it through the owner check to the pin-write comparison. The regression case exercises that wrapper. - Accepted saves advance the runtime save token and notify subscribers even when a newer local draft is retained. Direct acceptance, reconnect replay, both config sides, and History revision have source-level regression coverage.
The optional recovery feedback note is also addressed: the shared success toast fires once when the requested agent is active again, including the last row, without treating community disconnection as recovery. Existing error and focus ownership paths are preserved. Config sync remains disabled by default.
The public PR description has no image attachments; it and all three follow-up commit messages were inspected with no disclosure finding. The repair reuses existing runtime, service, Button and Toast owners. New cases stay in existing service/runtime and jsdom component tests; no browser journeys or CI configuration were added. Unchanged feature areas were not re-audited.
Validation: immutable source inspection and a clean follow-up diff check only. No PR code, tests, builds or apps were executed. One hosted-check snapshot showed 6 successful, 23 running and 2 skipped checks; final CI is unresolved. Native/cross-device runtime behavior, rendered keyboard focus, screen-reader announcements, both themes, narrow/enlarged layouts, performance, comparative test cost and human acceptance remain unverified. This COMMENT is not approval or merge authorization.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source follow-up via Wes’s account, integrating Rocket, Mantis and Groot. Two new P2s are inline: fallback can roll back a newer revision or miss updates during subscription startup. The prior P2 retype-coordinator race remains unresolved. Optional: update the description’s claim that synced agents cannot change type.
Head 60c923f6cf3dcf1ca787d6e554df3db6216c4ec3, base d2f80496a01ad057baeb5147b19d98db35a74769; follow-up from d8ec071b. Source-only: no tests, app/relay execution, performance measurements or CI assessment; rendered focus remains unverified. Non-blocking COMMENT; no approval.
## Summary
`buzz.agent.settings` content already carries `agentType` in practice
(block/buzz-app writes it and refuses to launch with another type's
settings), but NIP-AP defines the content as `{"version": 1, "settings":
{...}}` and never mentions it. block/buzz-app#1226 makes `agentType`
drive cross-device behaviour (following a type change, and starting
settings fresh on a retype), so the spec should define it.
## Changes
- Config artifacts table: settings content is `{"version": 1,
"agentType": "<string>", "settings": {...}}`.
- New **Agent type** rule:
- `agentType` is a client-defined string, compared exactly.
- Writers MUST include it; a record without it counts as the agent's
current type (keeps older records working).
- A client MUST NOT launch an agent with settings for a different type.
- A type change is an edit that writes the new `agentType`; a client
reading a current revision for another type MAY switch the agent to that
type when it supports it.
- **Editing**: one explicit exception to keeping unrecognised fields: an
edit that changes `agentType` MAY drop the fields inside `settings`,
since they belong to the old type. Top-level fields and tags are still
kept.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Signed-off-by: Kalvin Chau <kalvin@block.xyz>
Co-authored-by: peon <9ac6794b000690b7e814eb1805ad32405d0bec7d52838de3a86cf967565dacc0@buzz.block.builderlab.xyz>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Two P2s remain: cached config survives confirmed authority loss, and History revert bypasses the cross-type settings guard. The four previously reported exclusion/ordering findings are addressed.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 One P2 remains: I independently confirm the fallback authority-loss finding; retire cached config on confirmed archive/membership loss while preserving transport-only offline starts. The three earlier race findings are repaired in source.
Optional: distinguish other-harness revisions from invalid data in History and disclose the harness restored by Revert. NIP-AP permits restoring an older complete snapshot, so a blanket cross-type revert ban is not required.
Star Lord’s automated source follow-up via Wes’s account, integrating Rocket, Mantis and Groot. Head 4ec765a92fb6cfd968e0367d4a278c68fba3397d, base d2f80496a01ad057baeb5147b19d98db35a74769. No tests, app/relay execution, CI assessment or runtime/focus validation. Non-blocking COMMENT; no approval.
4ec765a to
c393e7e
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 One P2 remains in the fallback membership-loss repair (inline). Archive forwarding and the History type guard are repaired in source.
Star Lord’s automated source follow-up via Wes’s account, integrating Rocket, Mantis and Groot. Head c393e7eb11dd1190b114c6f061686a39aecaea6d, base bfaf0cbf508eaa6eab31c5b254aa696d5492eb7a. No tests, app/relay execution, CI assessment or runtime/focus validation. Non-blocking COMMENT; no approval.
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 One additional P2 in fallback authority ordering.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account. Two P2s remain in fallback confirmation: failed reads retain revoked config, and older responses can undo a newer regrant (inline). The earlier removal-delivery issue is repaired. Reviewed follow-up c393e7e → 963bc85 against base bfaf0cb, with relevant callers and independent Guardians coverage. Source-only: tests, app behavior and CI were not executed or verified.
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 One additional test reliability finding. The failed-confirmation revocation defect is already recorded in discussion_r4394114679; it remains unresolved at this commit.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source follow-up via Wes’s account. Three P2 fixes are needed in the new confirmation/recovery paths; details are inline. The prior immediate revocation issue is repaired, and the confirmation-ordering repair still loses failed outcomes.
Rocket, Mantis and Groot completed independent lanes, reconciled against source. Reviewed head 8d74771207691edfedd4af9d5c0e23fdfdc9cfe3, base bfaf0cbf508eaa6eab31c5b254aa696d5492eb7a; follow-up from 963bc85d5b38ad5919a84cdb2a69cbd2c4cd654b. Source only: tests, CI and live/native recovery were not exercised.
- store each agent's settings and instructions as nip-ar artifacts in a private nip-cl config channel in its home community (nip-ap "agent and team config"); sync is on where the relay advertises nip-cl and nip-ar - keep every bound channel current through one nip-ar sync; without artifacts.sync, read each channel once per connection and follow its revisions, metadata and roster live through session.artifacts.follow - save online only, on the head the editor opened, keeping unknown fields; migrate local config into an empty channel without overwriting a copy made elsewhere - sync the agent type with its settings revision; other devices follow a type change, and no attach or revert writes the old type over the new - starts use only confirmed synced copies; unsupported synced values, archived, redacted or access-lost config refuse the start - add config history and revert, and archive the config channel on delete Co-authored-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz> Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Co-authored-by: Hayt <211b96e6a2b7f45fd4047988976c7bbbeeda0c15f3ae7b32eec20834b5a55118@buzz.block.builderlab.xyz> Co-authored-by: peon <9ac6794b000690b7e814eb1805ad32405d0bec7d52838de3a86cf967565dacc0@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz>
… raw rosters - session follow reports followed channels an access revocation took away, which the access-epoch fence keeps from the follow's own events - config sync confirms the channel's current metadata and roster on such a revocation, or on any metadata or roster heard live, before retiring it, so a superseded removal never unbinds; a revoked channel the relay no longer shows is denied - real-session regressions for owner removal and a delayed removal, live and replay 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>
…nfirms - an access revocation gives the binding a new lifetime, drops its heads and holds live revisions, so nothing starts from it until a confirm and a fresh read give it back; any failure, network included, is a denial - a confirm that finishes after a newer one started takes the newer answer, so a delayed removal never undoes a regrant 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>
8d74771 to
2cfc0de
Compare
… recovery by epoch - a confirm keeps its settled promise, so an older one inherits the newest outcome, failure included, instead of proceeding as confirmed - a live revision held during a recovery or fallback read is read by d once the binding is synced, so recovery converges on it - a revoked binding's recovery is fenced by the connection epoch: after a reconnect it installs, marks and denies nothing 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>
…g config survives 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 follow-up via Wes’s account: one P2 remains in the catch-up repair, detailed inline. The confirm-outcome and reconnect-fencing fixes address their prior findings; catch-up can still lose a revision when an older binding has a read waiting to retry.
Reviewed head c5ea186 against base b68700f, integrating Rocket, Mantis and Groot. Scope: the three-file follow-up and rebase adaptations; source-only, with no tests/apps run and no CI assessment. Live recovery and rendered accessibility remain unverified.
a revoked binding gets fresh artifact wrappers, so a reread still out from the old lifetime cannot absorb or delete a catch-up request from the new one. adds a regression with an old reread pending through recovery and a newer revision heard before recovery settles. 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 follow-up via Wes’s account, integrating Rocket, Mantis and Groot. The two-file repair resolves the prior reread ownership collision; no new defect found in that repair. The separately reported P2 obsolete-denial finding remains actionable: reconnect retains the Bound, so fallback/reread error handlers can retire recovered config unless fenced by the operation’s epoch; this references the existing finding without duplicating it. Reviewed head 31736e6 against base b68700f; source-only, no tests/apps executed or reported runtime probes independently reproduced, and CI remains unverified.
a fallback read or reread still out across a reconnect no longer retires the binding when it fails: the reconnect reads again, so only a failure from the current epoch calls failure(). adds deferred-denial regressions for both after the reconnect has installed a newer head. 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>
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Two remaining recovery-ordering defects.
…cope-gates Preserve avatar initial focus, known community presentation, and Escape layering from #1765 together with the live membership gates and hosted-agent fallback. Retain the config history fixture contracts and tests introduced by #1226. Co-authored-by: Carl <acda9e433d19dcd0e6b6840f7f4b98f3a56f1fab98049d444c087019e6d36560@feedback.communities.buzz.xyz> Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
…ted-setup * origin/main: (24 commits) feat(agents): share agents to the community catalog and export snapshots (#1649) feat(navigation): dialogs never become Back/Forward destinations (#1696) fix(builderlab): sign in when finishing a hosted agent's setup (#1768) fix(messages): link #name prose to open channels the directory has named (#1706) feat(agents): keep the Mac awake while agents work (#1718) perf(panels): skip layout for covered workspace tabs (#1542) feat(composer): offer model and effort in a DM with an agent without a mention (#1621) test(workflows): wait for dialog initial focus before pasting YAML in WorkflowEditor test (#1705) fix(builderlab): show all captured activity without a conversation toggle (#1764) feat(agents): add a UI for Attention watches (#1663) Keep thread reply branches one level deep (#1751) fix(messages): keep table scrollbars inside webkit message rows (#1677) feat(agents): sync agent config through config channels (#1226) fix(agents): offer a new hosted agent in Solo chats right away (#1759) test(usage): make UsagePage read-window test DST-safe (#1668) fix(panels): scope split motion selectors to layout owners (#1681) feat(agents): simplify imports and clarify community scope (#1660) fix(pi): stop posting replies the agent did not send (#1758) feat(identity): support a local debug restore helper (#1517) test(perf): cover rich thread open close and reopen (#1683) ... Signed-off-by: Riley Crane <rileycrane@squareup.com>
…into rileycrane/bestie-hourly-wake * origin/rileycrane/bestie-hosted-setup: (25 commits) Give Bestie's test fixture the native keepAwake that main now requires feat(agents): share agents to the community catalog and export snapshots (#1649) feat(navigation): dialogs never become Back/Forward destinations (#1696) fix(builderlab): sign in when finishing a hosted agent's setup (#1768) fix(messages): link #name prose to open channels the directory has named (#1706) feat(agents): keep the Mac awake while agents work (#1718) perf(panels): skip layout for covered workspace tabs (#1542) feat(composer): offer model and effort in a DM with an agent without a mention (#1621) test(workflows): wait for dialog initial focus before pasting YAML in WorkflowEditor test (#1705) fix(builderlab): show all captured activity without a conversation toggle (#1764) feat(agents): add a UI for Attention watches (#1663) Keep thread reply branches one level deep (#1751) fix(messages): keep table scrollbars inside webkit message rows (#1677) feat(agents): sync agent config through config channels (#1226) fix(agents): offer a new hosted agent in Solo chats right away (#1759) test(usage): make UsagePage read-window test DST-safe (#1668) fix(panels): scope split motion selectors to layout owners (#1681) feat(agents): simplify imports and clarify community scope (#1660) fix(pi): stop posting replies the agent did not send (#1758) feat(identity): support a local debug restore helper (#1517) ... Signed-off-by: Riley Crane <rileycrane@squareup.com>
Syncs each agent's settings and instructions through a NIP-CL config channel in the agent's home community (NIP-AP "Agent and team config"). Sync is on at merge, with no flag: a community syncs when its relay advertises
nip-cl,nip-arandartifacts {version: 1, revision_kind: 45010}. Users on a relay that advertises all three get sync when this merges. Team writes are in #1281.Config channels and sync
config-channels.tsholds the support check (NIP-CL plus NIP-AR) and the only calls into the system-channel API. Lookup selects the oldest verified channel per subject, archived ones included. Ensure creates a private system channel only after a strong lookup finds none.config-sync.tskeeps every bound channel current through one NIP-AR sync over all of them, with one apply path for reads and live events. It confirms each channel (active, owner a member) in the first complete set after the channel binds or the connection or sync restarts; a set reopened for a new binding confirms and looks up only that binding. Agents attached together bind in one batch, so the sync reopens once. Every revision is checked against the selected channel,dand type before it is installed (checkedHead). Redaction, removal and relay-signed archive metadata retire the head or binding.artifacts.sync), or the session has no sync capacity, it reads each bound channel once per connection and follows the channels' live 45010 revisions through the newsession.artifacts.follow. System channels never list, sofollowis what keeps them in the session's live routes. A live revision applies over the head it names; otherwise the channel is read again. A read from before a reconnect installs nothing. Edits from another device show without a reconnect.community-config.tswires the agent's home community: support check, ensure, bind and migrate. A selected binding, including an archived one, is saved as a fact so a restart can't fall back to an earlier channel. A type that gains its settings schema after the agent attached starts syncing its settings then.Saves and migration
config-writes.tssaves online only: one serialized attempt per agent and side, on the head the editor opened, keeping unknown JSON fields and tags. A stale editor conflicts and keeps its draft. Results are reported as accepted, conflict, refused or unknown; there is no offline queue or automatic resend.Type
Starts
workspacestay local.History and Delete
Validation
tscand biome are clean.