Repository navigation
feat(agents): start agents from relay-synced config copies - #1185
Conversation
485e058 to
fd9bd64
Compare
Step-5 source review — first passReviewed commit IMPORTANT — launch tests do not exercise the resolver/transport seamCorrectness: Consequently the request-count tests do not protect the newly added production call to Required correction: add a launch-boundary test that drives the real runner → Scope judgments and PR C conditions
The stable writer/signer, separate fresh snapshot reader, and profile-cache traces support the implementation choices. The store's Evidence limitsAt inspection, this head reports successful DCO, Semgrep and zizmor checks. The latest broad CI run returned for this branch is on |
Step-5 source review — pass 2 of 3Pinned to The new tests genuinely connect IMPORTANT — Correctness: the request evidence is still narrower than T7
The positive control calls the wrapped mock Correction: preferably compose the real relay session over a counted fake IMPORTANT — Correctness: offline and no-retry assertions lack the boundaries they claimCodex offline behavior is permitted; the current assertion is incomplete. A context-read failure is not a config fallback failure: §4.3 explicitly allows those reads. Starting Correction: leave the failing transport in place until an observable terminal outcome for the second mention; assert the context-only failing requests, the activity error, no second Refusal proves no runtime launch and a local reason, but not no retry. Correction: count calls to the binding/resolver for this trigger, await the refusal/drain outcome, advance a controlled clock through the next service tick, and assert exactly one attempt with the triggering message's structured Source reasoning still finds no retry bug in the runner: the job is shifted before resolution ( IMPORTANT — Correctness: no-merge refusal rejects valid local/migrating settings
Correction: require a merge only when settings are effectively synced: PR description and evidence limitsThe rewritten description now matches the architecture and correctly says production remains local, names the binding input, and lists the NIP-11 and broker-cap omissions. Two precision corrections remain: the positive control is presently a direct mock call, not a real sync read; the 15-minute age/retry block belongs to the native writer, not the outbox itself ( Scores: Minimalism 9/10 — the approved narrow binding and plugin-owned schema avoid saved test-only state; Elegance 9/10 — the shared resolver/run parts preserve ownership; Correctness 8/10 — fix the observation/async gaps and the effective-mode guard above. At this head DCO, Semgrep and zizmor report success. The latest broad CI returned for the branch is still the successful run on |
Final source review — pass 3 of 3Pinned to Disposition of the four requested corrections
IMPORTANT — Correctness: the allowed reads are classified by kind, not conversation filter
For example, an additional Smallest correction: derive the expected requests from the triggering mention and assert their complete filter arrays, including cardinality. For these empty-history, thread-scoped fixtures, the production builder emits the root-ID and Answers to the additional review questions
Evidence and descriptionDuncan supplied terminal excerpts and mutation commands in the thread’s mutation-evidence message ( The description now correctly names the real Hosted evidence inspected: DCO, Semgrep and zizmor succeeded at this head. The newest broad CI run returned for this branch is still the successful run at The shared guard and real-session fixture are structurally sound; the remaining agreed-contract test defect prevents a correctness pass. The three-pass budget is exhausted. |
adc59d6 to
49f95f4
Compare
Scoped follow-up review —
|
…tings split Groundwork for relay-synced agent config (PR A steps 1-4): a local store with a fenced state machine, a NIP-AR artifact client, a bounded sign lane for kind 45010, and per-plugin synced-settings allowlists. Nothing is wired in yet. Signed-off-by: Hayt <211b96e6a2b7f45fd4047988976c7bbbeeda0c15f3ae7b32eec20834b5a55118@buzz.block.builderlab.xyz>
…unds The relay rejects consistency on artifact queries, the HTTP seam dropped typed refusal reasons, decoded NUL and foreign team ids passed validation, live applies lacked a generation fence, the signing ceiling ignored preserved tags, and prototype keys passed the settings allowlist. Signed-off-by: Hayt <211b96e6a2b7f45fd4047988976c7bbbeeda0c15f3ae7b32eec20834b5a55118@buzz.block.builderlab.xyz>
…mmand The size regression called the signer directly, so it could not catch relay_sign choosing the ordinary signer or dropping the full-event check. Signed-off-by: Hayt <211b96e6a2b7f45fd4047988976c7bbbeeda0c15f3ae7b32eec20834b5a55118@buzz.block.builderlab.xyz>
The relay leaves system channels out of any read whose #h does not name them (NIP-CL), members included, so a #d-only head read of a config channel returned nothing. The test fixture now serves nothing to such a filter. Signed-off-by: Hayt <211b96e6a2b7f45fd4047988976c7bbbeeda0c15f3ae7b32eec20834b5a55118@buzz.block.builderlab.xyz>
NIP-AP settings must not carry machine-local paths, so each device keeps its own workspace in the local row. Remote settings naming workspace are now invalid as an unknown key. Signed-off-by: Hayt <211b96e6a2b7f45fd4047988976c7bbbeeda0c15f3ae7b32eec20834b5a55118@buzz.block.builderlab.xyz>
Adds the synchronous start resolver over the synced store (local, migrating and synced modes, team sections, had-version and the offline rule), the relay-only snapshot reader and native artifact writer on the session, run revision tracking for restart-to-apply, and zero-request start tests for Claude Code and Codex. Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
Each run's config and synced revisions now come from resolveStart over this device's config store; with no config channel yet every type is local, so launches are unchanged. Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…d runtimes Agent types now carry their synced-settings schema and merge, and AgentsService takes an optional config binding that PR C fills from the config channel. launch-boundary.test.ts drives Claude Code, Codex and Goose mentions end to end from present heads, counts relay-session requests online and offline, and checks that a deleted head refuses with its reason on the mention. Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
The refusal fired for any settings binding, so a local or migrating agent of a type without syncedSettings could not start. It now uses the resolver's own synced test (explicit synced mode, or a synced revision this device has had), exported as isSynced. Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…d attempt Runs the start boundary on a real relay session over a counting transport, allows only Codex conversation reads, keeps Codex offline through its failed mention, and advances a fake clock past the service tick to show a deleted head is attempted once. Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…l discovery A kind-only check let a wider or wrong-channel read pass as conversation traffic; each mention's read is now compared filter for filter, and counted. Setup waits for the channel list to be ready, whose access change would otherwise cancel later setup reads, instead of sleeping 50 ms. Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
49f95f4 to
a11650c
Compare
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Approved with non-blocking P3 follow-ups at a11650c. No normal-path launch regression or actionable security defect was established. The inline comments cover storage recovery, launch-test evidence, and optional refusal-policy cleanup. Production config bindings remain deferred. Typecheck, changed-file lint, and diff checks passed; full Vitest validation was not green (timeouts/incomplete runs). Chromium and WebKit fixture-backed app journeys passed, but live staging validation was blocked by the no-Keychain constraint; native signing and actual harness launches remain unverified.
| } | ||
| if (value?.version !== 1 || typeof value.entries !== "object") return empty; | ||
| const entries: Record<string, Entry> = {}; | ||
| for (const [k, entry] of Object.entries(value.entries)) { |
There was a problem hiding this comment.
🤖 [P3] Validate persisted entry shapes before iterating. typeof null === "object", so {"version":1,"entries":null} reaches Object.entries(null), while {"version":1,"entries":{"k":null}} throws at entry.head. These cases were reproduced in review. Since this store is constructed in AgentsService, malformed storage can prevent service construction. Please reject null/non-record containers and malformed entries, including unknown states and invalid generation/sequence values. Non-blocking here: production does not yet write this key, so the demonstrated trigger requires corrupted or manually edited storage.
There was a problem hiding this comment.
👍 will address in a downstack PR
| run: (delivery) => { | ||
| deliveries.push(delivery); | ||
| return real.run?.(delivery); | ||
| }, |
There was a problem hiding this comment.
🤖 [P3] Include production runtime synchronization or narrow the evidence claim. This fixture registers run handlers, but the Claude Code and Goose plugins also subscribe runtime.sync(running), which creates entries from local agent.config before delivery. Their entry lookup then reuses those entries, so the fixture overstates adoption of resolved config even for new sessions. Please wire sync into the fixture and mark synced adoption pending #1226, or explicitly narrow the test/documentation claim. This is a statically traced coverage gap, not a reproduced current launch regression; production bindings remain deferred.
There was a problem hiding this comment.
👍 will address in a downstack PR
| const merge = this.types().find( | ||
| (type) => type.key === agent.type, | ||
| )?.syncedSettings; | ||
| if (!merge && isSynced(this.configStore, settings)) |
There was a problem hiding this comment.
🤖 [P3] Optional maintenance follow-up: keep missing-merge refusal in the resolver with the other refusal rules. This callback repeats the runner's type lookup and handles one refusal outside resolveStart, then casts withSynced below. Passing the optional synced-settings capability into the resolver would give this policy one owner. No runtime failure was established; this is non-blocking cleanup.
There was a problem hiding this comment.
👍 will address in a downstack PR
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account.
One P2 finding is detailed inline: a synced Codex effort value that this device cannot support passes validation and is silently replaced by the model default. This affects the new bound/synced launch path; production agents without config bindings continue to use the existing local path. Source assessment: 8/10 pending that explicit-error contract fix.
Reviewed head a11650c against base 85ebd91. Inspected the config store/resolver, relay snapshot and artifact publication boundaries, native signing limits, harness integration, regression-test source, public PR description (no attached images), and commit messages. The documented follow-up requirements for team integration, run lifetimes, new-session adoption, and browser/advertised size limits remain outside this foundation’s completed scope.
Source-only review: no PR code, tests, builds, or app were executed; current CI, native/live behavior, performance, error/retry focus, and human acceptance were not verified. This COMMENT review is non-blocking and is not approval.
|
|
||
| export const syncedSettingsSchema = { | ||
| model: "string", | ||
| effort: "string", |
There was a problem hiding this comment.
🤖 P2 — Reject unsupported synced effort instead of silently defaulting. This schema accepts every string, so a confirmed synced settings head with a nonempty effort unsupported by this device’s selected model passes resolveStart and reaches CodexRuntime.prepare. There, src/bundled/codex/runtime.ts:971–976 discards unsupported settings.effort and substitutes selected.defaultReasoningEffort. For example, a synced xhigh setting with a local model catalog offering only low/high starts at the default while the delivery records that synced revision as applied. NIP-AP’s Settings contract requires an explicit error for an unsupported setting, rather than a silent default. Keep the existing local/per-turn fallback behavior where intended, but validate an explicit synced effort against the effective model’s supported levels before starting the turn (using the model data the runtime already loads), and add a launch-boundary case that asserts refusal/no turn for an unsupported synced effort. This concerns the new synced-binding path; the production local path is currently unbound.
There was a problem hiding this comment.
👍 will address in a downstack PR
…ui-polish * buzz-app/main: (43 commits) Unify file attachment cards and file-type artwork (#1220) feat(agents): start agents from relay-synced config copies (#1185) render shared sessions as attachment cards (#1217) fix(channels): keep other channels' permission reads alive across a delete (#1245) feat(channels): mark temporary channels in the sidebar (#1249) test(browser): fix six intermittent browser test failures and a composer caret bug (#1239) Close channel tabs together and make tab shortcuts predictable (#1154) Make message retry controls compact and accessible (#1221) fix(messages): end deferred composer focus on any focus move; stabilise hover-preview probe (#1244) fix(messages): keep restored-channel focus off the composer (#1243) fix(me): record reads in Solo chats opened from the sidebar (#1183) feat(inbox): archive or restore the open conversation with Control-Command-A (#1181) fix: keep panel divider dragging responsive (#1067) feat(agents): find people for agents through the relay's name-only profile search (#1124) fix(agents): explain why Create agent is disabled (#1232) feat(agents): run up to 8 Interests at once for every agent type (#1100) fix(codex): set up Codex from the agent creation card, as Claude does (#1231) Shrink the thread loading mark by a third (#1214) feat(agents): plugins add tools to every agent session (#1216) fix(sidebar): re-measure every drop target when a section resizes mid-drag (#1230) ... Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Main fails typecheck since #1250 made effort required and #1185 added this fixture without it. Same one-line fix as in #1257 and #1260. Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Signed-off-by: Logan Johnson <loganj@squareup.com>
Relay-synced agent config, up to the point where agents start from it. Every agent run now goes through a start resolver that reads only this device's confirmed copies. In production every agent is still
local(no config channel binding exists until PR C), so there is no visible change yet.src/features/agents/agent-config.ts): keeps each agent's last confirmed instructions and settings artifacts locally, with a state machine (present, invalid, deleted, unavailable) and a generation/sequence fence, so nothing from a retired binding can overwrite a newer answer. Stored heads are re-verified on load. The settings allowlist checks own keys only, soconstructor,toStringand__proto__are unknown settings.src/features/agents/agent-config-artifacts.ts): deterministicd(UUIDv5 of the type in the channel's namespace), current-heads and head-by-dreads that name the config channel in#h(the relay leaves system channels out of reads that don't name them, NIP-CL), per-revision validation, and accepted-vs-current handling for publishes. Two heads for oneddecide nothing. Refusals are classified from the relay's response body; transport errors and 5xx stay uncertain.snapshotReadinsrc/features/relay/session.tsbacks the client's reads. Kind 45010 artifacts publish through a dedicatedconfigArtifactswriter innative.tsthat skips the native writer's 15-minute exact-byte retry block, and sign under a bounded lane insrc-tauri/src/relay.rs(the relay's default 512 KiB frame less the["EVENT",…]wrapper, measured on the complete signed event).src/bundled/{claude-code,codex,goose}/settings.ts): per-plugin allowlists. Claude Code and Codex syncmodel,effort,scope; Goose syncsmodel,provider,scope. Instructions are their own artifact. The agent folder (workspace) is device-local and never synced. Each plugin now exposes its schema and merge onAgentType.syncedSettings.src/features/agents/agent-start.ts):resolveStartbuilds a run's config from the local config plus any synced copies the binding names, and returns the revision IDs it used, or a refusal (invalid,deleted, lost access) with a reason. It makes no relay request. The runner calls it for every delivery, passesconfigandrevisionsto the runtime, records them inRuns, and shows a refusal as the mention's error without launching.AgentsServicetakes an optionalconfigBinding(agent)returning{ settings?: { mode, subject }, instructions?: { mode, subject } }. Production leaves it unset; PR C fills it from the config channel binding.src/features/agents/launch-boundary.test.ts): mentions go through the real service, runner, resolver and Claude Code, Codex and Goose runtimes from signedpresentheads, on a realcreateRelaySessionover a transport that records every request. They check the model, instructions and workspace each runtime actually used and the revision IDs on each delivery. During a start the only request allowed is each Codex mention's own conversation read, compared filter for filter (its channel, the mention byidsand its thread by#e, kinds 9 and 40002,untilthe mention, limits 1 and 50) and counted; any other request, or a wider or different one, fails the test, online or with the relay unreachable. Offline, Claude Code still receives the synced prompt, and Codex's second mention ends with the read error on that mention and no secondturn/start. With the relay back, a real config read throughreadHeadshows up on the same counter. Adeletedsettings head is attempted once: nothing launches, its reason is on the mention, and nothing retries after the service's next 5-second tick.syncedSettingsmerge refuses to start only when its settings are synced (explicitsyncedmode, or this device has had a synced revision), using the resolver's ownisSyncedcheck;localandmigratingagents with no synced revision start from their local config.Not in this PR: reading the relay's NIP-11
max_message_length, and the browser broker's 64 KiB cap on kind 45010.Constraints PR C must meet before using this:
host.team). PR C must format the resolver's admitted teams and stop injecting the legacy list after migration.Runsrecords each agent's latest delivery, not every live session; a runtime can return whileworkingUntilwork continues. Before "Restart to apply", PR C must track actual session and run lifetimes and not clear older staleness just because another conversation starts.entry.config, so passing resolved config on a delivery does not yet change an existing entry. PR C must prove new sessions use the resolved config while existing work keeps its snapshot.Related: #1155