feat: multi-agent session orchestration - #80
Merged
Conversation
Adds the generic pi-web vocabulary that multi-agent workflows need, without
teaching core anything about orchestration.
Extension settings platform:
- ctx.ui.web.registerSettings/getSettings, backed by a process-global
active-schema registry so validation and rendering are independent of which
session the browser happens to be viewing.
- Values persist under settings.extensions[id] as
{ schemaVersion, revision, values, backup? } and are carried through
verbatim, so an unloaded extension never loses its configuration. This also
fixes normalizeSettings silently dropping unknown top-level keys.
- Validated writes with an optimistic revision guard, bounded payloads,
schemaVersion migration with a one-slot backup, and per-owner reset.
- A generic settings renderer: toggle/text/textarea/number/select/list, with
accordion rows for repeaters, live model options (optionsSource: "models"),
cross-field references (optionsFromField) rendered as a per-row default star,
stable row ids, and inline accessible validation.
Session lineage and derived state:
- Session origin tracking, so sessions spawned by another session are indented
under their parent in the drawer with a lineage glyph.
- A derived "waiting on spawned sessions" indicator (drawer, tab, status bar)
with a single precedence rule: running > waiting > unread.
- Custom (extension-injected) messages render as notification cards, and cards
link to referenced sessions from structured details. Tool result cards can
surface the same session links.
Tests cover the store (verbatim preservation across unrelated patches, revision
conflicts, bounds, backup placement, reset, on-disk round trip) and the
descriptor validator (canonical schema identity, stable row ids, option and
cross-field reference checks, uniqueness, bounds).
Ships the multi-agent orchestration semantics as a userland pi-web extension rather than core behaviour: sessions_spawn/status/read/prompt/abort plus a zero-token background poller that delivers a wakeup when a worker goes idle, so the parent never polls. Workers are ordinary, fully visible sessions. Worker models come from user-authored categories (name, model, and "when to use" prose) configured through the settings API. The category to model mapping stays private to the config; the spawn tool resolves it fail-closed against the worker's own registry and cleans up if resolution fails. Documents the settings API and the orchestration example, and adds the companion skill that teaches the delegation loop.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5f326a82b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| async function save(schema: WebSettingsSchema) { | ||
| const draft = ensureDraft(schema); | ||
| const expectedRevision = storedFor(schema.id)?.revision; |
There was a problem hiding this comment.
When two browsers have this form open, the first browser's settings_updated event replaces state.settings, but ensureDraft retains the second browser's older draft. This line then submits the new global revision rather than the revision on which that draft was based, so the stale save passes the guard and silently overwrites the first browser's changes. Store the base revision alongside each draft and advance it only after the draft is resynchronized.
Useful? React with 👍 / 👎.
| backup?: { schemaVersion: number; values: JsonObject }; | ||
| }, | ||
| ) { | ||
| return write(applyExtensionValues(await read(), ownerId, nextValues, opts)); |
There was a problem hiding this comment.
When two extension PATCH requests overlap, both can complete read() against the same cached revision before either write() finishes, so both optimistic checks pass and the last write silently discards the other change. The same race exists between extension writes and the general settings PATCH/reset paths; serialize the complete read/apply/write transaction rather than only performing atomic file replacement.
Useful? React with 👍 / 👎.
| error = result.error; | ||
| activeSettingsSchemas.set(id, { schema, canonicalKey: canonical, refCount: 0 }); | ||
| } else { | ||
| existing.schema = schema; // refresh callbacks to the latest registrant |
There was a problem hiding this comment.
When multiple live sessions register the same owner, as the bundled orchestrator normally does, this assignment replaces all earlier sessions' onChange callback with the latest registrant's callback. The PATCH/reset handlers invoke only entry.schema.onChange, so only one extension instance reacts while the other sessions retain stale derived tools or state; track callbacks per registered session and notify each live owner.
Useful? React with 👍 / 👎.
Comment on lines
+127
to
+130
| if (existing && existing.canonicalKey !== canonical) { | ||
| // First-registered wins; a divergent schema for the same id is rejected. | ||
| console.warn(`pi-web: settings id "${id}" already registered with a different schema; rejecting new registration.`); | ||
| return { registered: false, migrated: false, usedBackup: false, error: `settings id "${id}" already registered with a different schema` }; |
There was a problem hiding this comment.
When an extension changes its schema and /api/extensions/reload reloads the same live session, the old registration remains in activeSettingsSchemas because registrations are released only when the entire session is disposed. The new registration therefore hits this canonical-key rejection before its version migration can run, making schema upgrades through the existing Retry extensions flow impossible without a server restart; remove that session's previous registration during reload or allow it to replace its own schema.
Useful? React with 👍 / 👎.
Examples were outside tsconfig's include, so extension sources shipped untypechecked. Add examples/**/*.ts to the typecheck surface and map the self-referencing "@ashwin-pc/pi-web/extensions" specifier to src/extensions.ts so examples resolve in-repo rather than against built output. Enabling that guard surfaced real problems it had been hiding: - session-orchestrator now uses PiWebExtensionAPI/PiWebExtensionContext instead of untyped contexts, which caught a dead getId() fallback, unchecked coercion of JSON-valued settings, and a schema literal that did not satisfy PiWebSettingsSchema. It also makes the async settings API impossible to misuse by accident: reading getSettings() without awaiting is now a type error. - git-footer passed a spawn-only `stdio` option to execFile. - recap called a SessionManager method that is now a standalone buildSessionContext() export, and used a discriminated auth result without narrowing it. Extension coverage (24 tests): first-colon token parsing, exact-match before region substitution and no arbitrary fallback, category menu that exposes names and prose but never the model mapping, default-category selection, unknown category and missing default erroring before anything is created, unavailable model creating then deleting the unprompted session without dispatching work, reported region substitution, virtual default leaving the worker on the session model, recorded origin, worker marker in the dispatched task, depth cap, and the concurrent worker cap. Also fixes a regression: custom messages rendered as cards dropped the custom--<type> class that plain custom messages expose, breaking existing selectors and styling hooks. The e2e suite now also asserts the card renders session-link chips from structured details.
Correctness - Serialize every settings mutation through one promise chain so the optimistic revision guard actually holds; two concurrent writers can no longer both pass CAS, and a generic patch can no longer clobber a concurrent extension write. First writes must present revision 0, reset is revision-guarded, and the HTTP layer rejects a missing/negative/fractional revision. Atomic writes use a collision-proof temp filename. - Rework the schema registry: the owner id is reserved synchronously before any await (so concurrent first registrations migrate exactly once and a divergent schema is rejected rather than replacing the incumbent), registrants are tracked per session instead of by refcount, a session that shuts down during migration is never registered, and a schema stays hidden from validation and transport until migration settles. Descriptors are structurally validated and owner ids must be namespaced before anything is reserved. - Notify every live registrant with its own callback and session id, rather than letting the newest registration overwrite the single stored callback. - Spawning is fail-closed on the remaining paths: a failed task dispatch now deletes the worker, cleanup failures are reported honestly instead of claiming "no orphan", and recording session lineage is best-effort so it cannot fail a request that already created a session. - Keep the worker poller from re-arming or polling after shutdown, and retain a completed worker until its wakeup is actually delivered. Layering - Core no longer knows about the orchestrator: session-link chips are rendered from an explicit reference list in a tool result's `details` for any tool, with no tool-name check and no parsing of English result text. A bare `sessionId` is incidental metadata and renders nothing, so tools that merely echo a session id stay quiet. References are bounded, de-duplicated, length-capped and id-checked because `details` is untrusted persisted input, and both custom-message cards and tool cards now share one helper. - Stop leaking the category-to-model mapping to the model: tool results and details name the category only, and report that a region substitution happened without naming the substituted model. Settings UI - Deleting a referenced row no longer silently clears the reference; the dangling reference surfaces as an inline validation error. Renames follow the reference reliably by resolving the referenced row through its stable id. - Accordion headers are real buttons (keyboard operable), required fields expose required/aria-required, and list-level errors are associated via aria-describedby. - Conflicts and resets re-render from the server response instead of leaving a stale draft behind a "reloaded latest" message. - Retained data for an unloaded extension can be reset (reset needs stored data, not a live schema), and a failed migration is surfaced through the published schema so the UI can show it. Scope - Drop unrelated session time-filter work that rode along in the drawer diff. Tests (257 unit, 28 files) - Store: concurrent same-revision writes, generic patch racing an extension write, reset racing a write, first-write revision rule, and many rapid writes keeping gapless revisions with valid JSON on disk. - Registry: migrate-once under concurrent registration, divergent-schema rejection, last-registrant lifecycle, notify-all-with-session-id, throwing onChange isolation, shutdown during migration, migration failure keeping a backup and surfacing the error, and hidden-until-ready. - References: explicit-list-only rule, dedupe, caps, hostile ids, label sanitizing. - Orchestrator: privacy of the mapping in results and details, dispatch-failure cleanup, honest cleanup reporting, no re-arm after shutdown, and retry when every wakeup delivery path fails.
Two defects sat in exactly the corner cases the previous fix claimed to handle,
and neither had a test. Both were reproduced empirically by the reviewer.
- A failed migration write permanently poisoned the owner id. The migration
promise is shared by every concurrent registration of an id, so a rejecting
store write (ENOSPC, bounds, EACCES) left the reservation in the map with
ready=false forever: every later registration re-awaited the rejected promise
and threw, instead of returning the documented { registered: false, error }.
migrateStoredSettings now resolves with an error instead of rejecting, and
registration additionally releases its reservation if the shared promise ever
rejects, so the id stays usable.
- A disposed reserving session could delete the registry entry from under a
concurrent live registrant. The reserver's post-migration cleanup ran while the
second registration had not yet attached itself, so the entry was removed and
the survivor was told registered: true while its schema stayed invisible to
transport, validation and notification. Registrations now count themselves as
in-flight before awaiting, cleanup only drops an entry with no registrants AND
no pending registrations, and a registrant always attaches to the entry the
registry currently holds.
Regression tests reproduce both sequences and fail against the pre-fix code
(verified by reverting each defence and re-running).
Also from the review:
- Never render an empty custom-message card; upstream drops text-less custom
messages and the card form should too.
- Restore upstream's colour-filter menu sizing; removing the unrelated
time-filter feature had left a widened menu class and rule behind.
- Warn instead of silently discarding a stored extension record that exceeds
bounds on load, so losing user configuration is at least visible.
Settings panel (second review, non-blocking findings): - The default-star button was nested inside the accordion header button, which is invalid HTML and breaks focus and screen-reader behaviour. Star and header are now siblings in a header row; the header remains the only element that toggles the accordion, the star toggles the default independently, and the inline header styles moved into CSS. - The "data retained" card for an unloaded extension now offers Reset, so the server capability the docs advertise is actually reachable. - A failed migration is now shown: WebSettingsSchema carries the published migrationError and the panel renders it as a role="alert" warning explaining that values were reset to defaults and the previous ones kept as a backup. Test gaps the review flagged as unfalsifiable: - tests/settings-endpoints.test.ts exercises the real HTTP server: PATCH and reset reject a missing, negative, fractional, non-numeric or NaN expectedRevision; conflicts report actualRevision; PATCH is refused without a live schema; a malformed percent-encoded owner id is a 400; and stored values for an unloaded extension can still be reset (with conflict detection). - The orchestrator tests now serialize the whole registered tool definition and assert it is byte-identical when only a category's model changes, while changing on a name, prose or default-category edit, and stable across repeated registration. This is the prompt-cache stability acceptance test the spec asked for and previously only checked content, not stability. Verified in a browser against an isolated settings file: no nested buttons, the star does not expand the accordion, the header is a focusable button with aria-expanded, required inputs and list role=group are present, and the retained card's Reset works.
A cold review (no prior findings supplied, to avoid anchoring) found two real defects in the lineage surfaces that two earlier reviews had missed. Sessions disappearing from the drawer. orderItemsWithChildren only re-emitted deferred children one level deep, so with origins C->B->A the grandchild C was dropped from the rendered list entirely, and a 2-cycle (A->B, B->A) left no root at all so BOTH sessions disappeared. Origins are client-asserted — /api/new-chat records an origin without checking the parent exists or that the graph is acyclic, and the session-ui-state PATCH accepts an arbitrary origins array — so this is reachable without any bug in the bundled extension, and the bundled extension's depth cap does not protect core. A session that still exists but cannot be found in the primary navigation surface is close to data loss from the user's point of view. The ordering logic now walks descendants iteratively with a visited set, and falls back to emitting anything the walk did not reach, so imperfect ordering is the worst case and a session can never be dropped. Lineage wiped by legacy-state migration. hasAnySessionUiState deliberately ignored sessionOrigins, so a server holding only lineage looked "empty" to a browser carrying legacy localStorage state; that browser then pushed its own normalized state, which always includes sessionOrigins: [], and the patch path replaces the array whenever the key is present — erasing every recorded origin. Lineage now counts as state. The drawer's derived logic had no tests at all, which is why both defects were invisible. The rules that decide what a user sees are now a pure module (src/sessions/lineage.ts): child ordering, the running > waiting > unread precedence, and which spawned children are still running. 16 tests cover deep chains, cycles, self-parenting, absent parents and de-duplication; reverting to the one-level expansion fails five of them. Also: warn when the stored owner list is truncated at the bounds limit, instead of silently discarding owners beyond it.
From the cold review. Validator - An optional number field could never be cleared: an empty input sends undefined, Number(undefined) is NaN, and the validator errored regardless of whether the field was required — a 422 the user could only escape by typing a value. Meanwhile an empty string coerced to 0, so the same "empty" input was sometimes an error and sometimes zero. Empty now means unset for optional fields; required and genuinely non-numeric values still error. - A list value that was present but not an array was silently coerced to [], destroying stored rows with no error. Present-but-wrong-shape list values and list rows now produce validation errors at their path. Settings panel - Nested lists are rejected at registration, because the client cannot render them; if one still reaches the scalar renderer it now shows a disabled, read-only view instead of a text input that would have flattened the array to a string on the next save. - The panel no longer steals the caret. It re-rendered on every settings_updated and every schema-list broadcast, and with the orchestrator installed the latter fires whenever a worker session spawns, so typing was interrupted routinely. Broadcast-driven renders are now skipped when the structural signature is unchanged and focus is inside the panel; explicit edits and save/reset still render. - The schema-changed broadcast now only fires when the published list actually changes, so an additional session registering the same schema is silent. Registry - A stale in-flight registration could overwrite a different schema registered under the same id after a release, leaving two sessions both told they had registered while one entry became invisible and never received onChange. An entry with pending registrations is no longer deleted, and a late continuation re-checks ownership: it joins a matching replacement and refuses to overwrite a divergent one. - Migration wrote without a revision guard, so a user reset that landed during a registration's migration was silently undone moments later. Migration writes now carry the revision from their own read and treat a conflict as "someone else acted, skip", still without ever rejecting the shared migration promise. Also removed a dead pointer to a local planning artifact that is not in the repo. Reverting either registry fix fails its regression test (verified by mutation).
From the cold review. Wakeup durability. Re-arming watches from the ledger treated ANY error while checking a child as "the child is gone", resolved the ledger entry and ended the watch — permanently, with no wakeup and no message to the parent. But that check runs exactly when the server is restarting or loaded, and the API client has a 20-second timeout, so an ordinary timeout or 5xx silently destroyed the durability the skill advertises. Only a definitive 404 now resolves the entry; timeouts, network failures and 5xx hand the child to the normal paced poll loop, which keeps its existing error threshold and still delivers a "lost track" wakeup rather than going quiet. Region residency. The Bedrock inference-profile fallback stripped an existing region prefix from the configured model id and substituted the parent's, so a category deliberately pinned to eu.* could be silently retargeted to us.* — a data-residency change the user never asked for, reported to the model only as "a region prefix was substituted". A parent prefix is now only ADDED to an unprefixed id; an explicit region is never replaced, and the spawn fails closed instead. Fail-closed categories. An explicitly requested category was silently ignored in two paths — when the settings API was unavailable, and when no categories were configured (where the name was overwritten with "Default") — contradicting the documented "unknown category, nothing created". An explicit category that cannot be resolved now errors and creates nothing; only an omitted category may fall back to the session default. Spawn cap. The worker cap was checked before several awaits, so parallel tool calls could exceed it. Slots are now reserved synchronously and released in a finally. The cap counts spawned workers only, not sessions attached via sessions_prompt; the refusal message and skill state this. Hygiene: the [ext vN] development marker is gone from anything the model sees (it remains in developer logs), and the file header's install paths now match the documented ones. Reverting the durability guard or the region guard fails their tests (verified by mutation).
While a session waits on sessions it spawned, those sessions are now listed as links directly above the composer, so a worker can be opened while it is still working. Previously the waiting indicator named the workers as plain text inside the context-meter button, which could not link anywhere — and putting links there would have nested interactive content inside a button. Every session link in the UI now switches session in place. The chips on wakeup cards and tool cards were plain anchors, so clicking one triggered a full document navigation: a complete app restart that refetched state, models and the whole transcript. Opening a session server-side takes about two milliseconds, so all of that latency was the reload. Chip construction is now one factory shared by the wakeup cards, the tool cards and the new strip: a plain left click switches via the same path the session tabs use, while modified and middle clicks still open a new tab and the real href keeps keyboard access and "copy link" intact. Spawned sessions are identified by name rather than an id fragment. Names arrived for sessions that were not yet in the cached list — a worker is named immediately after it is created, before any list refresh — and updateSessionName discarded them, so links fell back to the last eight characters of the id. Names are now remembered regardless of list membership, and discovering a new session schedules a coalesced list refresh. The strip keeps the existing precedence: it appears only while the session itself is idle, because a running session shows its own progress instead. It occupies no space when there is nothing to wait for, scrolls horizontally rather than wrapping, and each entry carries a pulsing dot to show the worker is still going. waitingInfoFrom is pure and tested (which children count, self-running precedence, and the id fallback when a name is genuinely unknown).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Turns pi-web into a multi-agent workspace where one session can spawn, monitor, steer, and interrupt others — and every worker is a normal, fully visible session in the sidebar, not a hidden subagent.
Two commits, deliberately split:
Why this split
The whole design rule was: only add mechanisms to core that have plausible non-orchestrator writers and consumers. Everything opinionated — when to delegate, how workers are named, how models are chosen — lives in the extension, which can be deleted without touching core. What core gains is generic:
Core: extension settings platform
ctx.ui.web.registerSettings(schema)/getSettings(id), backed by a process-global active-schema registry so validation and the settings UI don't depend on which session the browser is viewing (PATCH /api/settingscarries no session identity).{ schemaVersion, revision, values, backup? }undersettings.extensions[id]and are carried through verbatim. This also fixes a real bug:normalizeSettingsrebuilt known fields only, so any unknown top-level key was silently erased on the next write.revisionguard (concurrent browser edits can't drop fields), bounded payloads,schemaVersionmigration with a one-slotbackup, and per-owner reset.toggle/text/textarea/number/select/list. Repeaters render as accordion rows (collapsed one-liners, expand to edit) to stay usable on mobile.selectsupports static options, the live model registry (optionsSource: "models"), and cross-field references (optionsFromField) — and when a select references a list column, it renders as a per-row default star instead of a second dropdown. Rows carry stable__ids so renames keep references intact. Validation errors render inline withrole="alert"/aria-describedby.Core: lineage and derived state
running > waiting > unread. Rendered as an aurora ribbon on tabs and an hourglass + count in the drawer/status bar.details. Tool-result cards can surface the same links (the spawn card links to its worker).Userland: the orchestration extension
examples/pi-web-extensions/session-orchestrator.tsregisterssessions_spawn,sessions_status,sessions_read,sessions_prompt,sessions_abort, plus a zero-token background poller that delivers a wakeup message when a worker goes idle — so the parent never polls and can end its turn while work continues. Watches are persisted in the session file, so wakeups survive reload/restart and produce catch-up notifications.Worker models are chosen from user-authored categories (name + model + "when to use" prose) configured through the settings API. Design notes:
sessions_spawndescription and rebuilt only on config edit, keeping the prompt prefix immutable in normal operation.The companion skill (
examples/pi-web-skills/session-orchestration/SKILL.md) teaches the loop: what to delegate, how to write self-contained worker tasks, and why ending your turn while workers run is correct.Tests
New unit coverage (19 tests):
maxItemsbounds.npx tsc --noEmitis clean andnpm run buildsucceeds. The settings platform, accordion editor, category persistence, fail-closed resolution, lineage nesting, and waiting indicator were also validated live against a running server on desktop and a mobile viewport.Risk / reversibility
Core additions are additive and inert when no extension registers a schema. The orchestration behaviour lives entirely in the example extension and skill, so it can be removed by deleting two files.